Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show clear error when session can't be exported #35880

Merged
merged 110 commits into from
Dec 21, 2023

Conversation

probakowski
Copy link
Contributor

As tsh doesn't understand IronRDP frames yet it outputs completely black video when exporting new session recordings.
This changes returns error message in that case pointing user to web UI.

Isaiah Becker-Mayer added 30 commits April 12, 2023 15:07
…f sending back chunks of the screen, its sends the full screen every time, and this is my running hypothesis for the current sticking point: a "Maximum call stack size exceeded" from the tdp client in the react app
… for client.write_frame though this requires more research.
Requires some modifications to IronRDP, see the corresponding commit hash
for IronRDP in lib/srv/desktop/rdp/rdpclient/Cargo.toml.

Started getting call stack overflows when manual testing again, but I'm
going to ignore those for now and focus on the version of this architecture
that passes the remoteFX encoded messages over TDP.
Building the wasm module still needs to be done manually, this was
just to get to a point where tools are configured and it can be successfully
called in both dev and prod modes.

Required that I comment out "default-src 'self'" in the CSP, this will
need to be figured out later.

We're also using a package called vite-plugin-wasm-pack whereas ironrdp
uses vite-plugin-wasm. I'm not sure why, I'm going to look into this and
swap out to vite-plugin-wasm if possible.
…asm script to the teleport package.json.

vite-plugin-wasm-pack looks relatively unmaintained in comparison
to vite-plugin-wasm-pack.

build-wasm script is needed to build the wasm binary before calling vite [build]
Untested as of yet and it isn't pretty, but this more or less imitates the
RPC approach we have for directory sharing.
with a particular commit of IronRDP. Unfortunately this commit is now
well out of date, and IronRDP had some bugs that were preventing RemoteFX
frames from being sent back.
…, etc.) rather than relying on whatever is re-exported from the ironrdp sub-crate itself
renaming them to be precise.
Isaiah Becker-Mayer and others added 3 commits December 15, 2023 14:38
Updates IronRDP to latest with the changes necessary to keep up with the changes in Devolutions/IronRDP#328. We select the `"rustls"` feature explicitly now, this is in line with our implicit choice of it as [the default](https://github.com/Devolutions/IronRDP/pull/325/files#diff-1f64036f3e460819b2b1e954803b7f266feca0e2787ca0614ae1253323944f59L19) before these latest changes.

Also creates a custom debug for `ScardBackend` so as to not leak `key_der` in the logs.
@github-actions github-actions bot added size/sm tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Dec 19, 2023
Copy link

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@zmb3
Copy link
Collaborator

zmb3 commented Dec 19, 2023

This works. We could make it even better by marking the session as errored when we see the RemoteFX frame, but continuing to stream through until we hit the session end event. The session end event has enough information to construct the URL to playback the session, so we could link the user directly to it..

@probakowski probakowski added the no-changelog Indicates that a PR does not require a changelog entry label Dec 19, 2023
@probakowski
Copy link
Contributor Author

@zmb3 I've added url to the error message (I don't know if it's quite correct, can we always assume https?)

Base automatically changed from ironrdp to master December 20, 2023 23:40
webProxyAddr,
evt.ClusterName,
evt.SessionID,
lastEmitted)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using lastEmitted, you could compute the duration by looking at the StartTime and EndTime fields of the WindowsDesktopSessionEnd event.

This would be more correct in cases where there was idle time prior to the session end event.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's better, I've changed it, thanks!

@ibeckermayer
Copy link
Contributor

ibeckermayer commented Dec 21, 2023

can we always assume https?

IIRC the proxy always redirects http connection attempts to https. Could be wrong, but either way seems safe to assume the vast majority of users will want https.

@probakowski probakowski added this pull request to the merge queue Dec 21, 2023
Merged via the queue into master with commit a27a558 Dec 21, 2023
32 checks passed
@probakowski probakowski deleted the probakowski/tsh-ironrdp-error branch December 21, 2023 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry size/sm tsh tsh - Teleport's command line tool for logging into nodes running Teleport.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants