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

Adds a detailed discussion of the Go/Rust Interface to the README #29762

Merged
merged 4 commits into from
Jul 31, 2023

Conversation

ibeckermayer
Copy link
Contributor

No description provided.

lib/srv/desktop/rdp/rdpclient/README.md Outdated Show resolved Hide resolved
lib/srv/desktop/rdp/rdpclient/README.md Outdated Show resolved Hide resolved
lib/srv/desktop/rdp/rdpclient/README.md Show resolved Hide resolved
lib/srv/desktop/rdp/rdpclient/README.md Outdated Show resolved Hide resolved
lib/srv/desktop/rdp/rdpclient/README.md Show resolved Hide resolved
Copy link
Contributor

@espadolini espadolini left a comment

Choose a reason for hiding this comment

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

This ends up being normative for the later PRs so we should absolutely clarify that Client must be Send + Sync, otherwise it's unsound to drop.

Isaiah Becker-Mayer and others added 3 commits July 31, 2023 08:51
Co-authored-by: Forrest <30576607+fspmarshall@users.noreply.github.com>
Co-authored-by: Edoardo Spadolini <edoardo.spadolini@goteleport.com>
@ibeckermayer ibeckermayer merged commit c2edddf into ironrdp Jul 31, 2023
@ibeckermayer ibeckermayer deleted the explain-client branch July 31, 2023 16:19
github-merge-queue bot pushed a commit that referenced this pull request Dec 20, 2023
* adds ironrdp client and has it connect, alongside rdp-rs

* Integrates IronRDP client into the TDP png-response system. Instead of 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

* Changes arrayBufferToBase64 to use reduce, which fixes the "Maximum call stack size exceeded" bug

* removes convert_bgra_to_rgba which isn't needed with the bitmaps being produced by IronRDP

* Adds support for updating only a specific image region

* removes mistaken read_rdp_output_inner call from connect_rdp_inner

* Adds support for mouse input. I believe this code has race conditions for client.write_frame though this requires more research.

* Patches things up with the new IronRDP structure

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.

* Adds RemoteFX Frame to the backend

* Proof of concept for integrating a wasm module into the frontend

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.

* Swaps out vite-plugin-wasm-pack for vite-plugin-wasm and adds build-wasm 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]

* Adds backend structure for RPC PoC

Untested as of yet and it isn't pretty, but this more or less imitates the
RPC approach we have for directory sharing.

* hack solution which calls everything 'static. THIS IS VERY UNSAFE, WE SHOULD DO SOMETHING ELSE

* Gets the full display working end to end
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.

* Full display working end to end with the updated IronRDP

* updates init_wasm_log to enable logging in the console

* updates hardcoded io_channel_id and user_channel_id

* Uses specific IronRDP sub-crates (i.e. ironrdp-session, ironrdp-tokio, etc.) rather than relying on whatever is re-exported from the ironrdp sub-crate itself

* Gets FastPathFrameResponse up to speed and the PoC is now working

* deletes all the rpc stuff which it turns out we don't need

* more cleanup including consolidating the FastPathFrame message between wasm and js, adding some comments

* Updates the documentation string for top level Client in lib.rs

* Removes hardcoded rdp server address, screen size, and username

* Swaps to online IronRDP repo

* Updates the TDP RFD with new messages
renaming them to be precise.

* remove hardcoded ip

* creates asRustBackedSlice

* removing some old todos

* Add cleanup for wasm-pack build artifacts to make clean

* Fix wasm in production builds (#27428)

* Converts the default Content-Security-Policy representation to a map
which makes it easier to programmatically add-to/overwrite the defaults
for special cases.

Also adds tests for the various custom CSPs.

* Alphabetize CSP directives for ease of testing/debugging in the future

* makes maps more easily composable

* removes commented code, and topLevelAwait() which was causing production builds to break

* Adds 'wasm-unsafe-eval' to the script-src section of the Content-Security-Policy
when a desktop session's url is detected.

The app already enforces no caching via the Cache-Control, Pragma, and
Expires headers (SetNoCacheHeaders), which means that any time the app
is requested in a new tab, the browser will always request a new index.html
which can be sent back with a new Content-Security-Policy header.

Since the only way to get to a desktop session is to either
click a link in the app which opens a new tab, or to open a new tab and
navigate to the url manually (this differs from most components, which can
be navigated to without a page reload using React Router), we can reliably
check each index.html request for a characteristic desktop session url path,
and only include the 'wasm-unsafe-eval' directive when strictly necessary
(in the spirit of the principle of least privilege).

* Removes vite-plugin-top-level-await from package.json

* moves regex to global so that it only need be compiled once

* CR fixes
updates Cargo.lock
removes commented feature flag
removes -framework SystemConfiguration
fixes makefile comment

* unsafe.SliceData

* fixes phrasing

* changes to docstring

* changes handle_tdp_fast_path_response to handle_tdp_rdp_response_pdu
so that its name is in line with its definition in RFD 0037 and
adds a more descriptive docstring.

* small fixes to comments

* preallocate RDPFastPathPDU Encode buffer

* updating comments

* Adds a detailed discussion of the Go/Rust Interface to the README (#29762)

* Aligns the codebase with the Go/Rust interface principles in the README (#29764)

* Move client functionality into Client (#29765)

* Adds the RDP Channel IDs TDP message (#27514)

* Refactoring `rdpclient` (#29766)

* Ironrdp recording playback (#28228)

* httplib.SetIndexContentSecurityPolicyWithWasm for recordings as well as desktop sessions so that the ironrdp wasm client can be used for recording playback

* Adds desktopplayback path to websocket proxy definitions so that it can be used with the dev server

* adds tdp.TypeRDPFastPathPDU as a tdp message to be recorded

* With this commit, recordings can be played back with the FastPathProcessor

Refactors tdp Client and subclass PlayerClient
such that now the classes are always constructed the same way,
and the internal representation of the screen size (the FastPathProcessor)
is either:

1. Created upon the call to Client.connect(spec) when spec is !undefined.
This is the case when the Client is being used for a desktop session.

2. Created when a client screen spec is received from the server. This is
the case when PlayerClient is being used for playback.

* Switches from sending the width/height via ur parameter string
to sending it via the client screen spec tdp message.

* changes tdpCli* to client* and adds a canvas* to the canvas related functions

* Static tokio (#30066)

* Beginnings of RdpdrBackend integration (#32814)

* Adds handling for the EstablishContextCall (#32971)

* Adds support for `ListReadersCall` and `ListReadersReturn` (#33004)

* Adds support for `GetStatusChangeCall`/`GetStatusChangeReturn` (#33056)

* Adds support for ConnectCall and ConnectReturn (#33136)

* Better error handling (#32925)

This change removes couple of unwrap that could cause panics on the Rust side and prevents panics from invalid cgo handle on the Go side.
It also sends error message from IronRDP to UI and cleans up a little final sequence of closing various goroutines in case of error or sessions end.

* Adds support for `HCardAndDispositionCall` with `ScardIoCtlCode::BeginTransaction` (#33192)

* `TransmitCall` and `TransmitReturn` (#33282)

* `StatusCall` and `StatusReturn` (#33284)

* `ReleaseContext` and refactor `handle_scard_call` (#33434)

* IronRDP clipboard sharing (#33335)

This change adds clipboard sharing to our IronRDP migration using ironrdp_cliprdr crate with out TDP-based backend (reused from rdp-rs times)

* Adds EndTransaction, Disconnect, Cancel, and IsValidContext (#33612)

* Adds support for `GetDeviceTypeId` (#33613)

* Adds support for `ReadCacheW` (#33618)

* `WriteCacheW` (#33620)

* Smartcard Login (#33712)

* `GetReaderIcon` (#33779)

* Update to IronRDP latest commit

* Removes a bunch of old deprecated code (including `rdp-rs`) (#33786)

* Pipes in `SharedDirectoryAnnounce` (#33911)

* Use tokio-boring for TLS upgrade (#33900)

This change switches TLS upgrade in our new RDP approach to tokio-boring in FIPS-compliant builds.
It also adds runtime check for FIPS mode.

Tested locally in custom Docker image.

* Update sspi (#34558)

* Break up rdpdr.rs, add support for the first directory sharing pdus (#34009)

* Adds support for `SharedDirectoryCreateRequest` and `SharedDirectoryCreateResponse`. (#34011)

* `SharedDirectoryDeleteRequest` and `SharedDirectoryDeleteResponse` (#34051)

* remove leftover code

* Server drive query information request (#34082)

* Rdp client refactor (#34122)

* Add handling for `DeviceCloseRequest` (#34139)

* Updates to `IronRDP` latest (#34744)

* Updates to IronRDP latest with a slightly different API

* remove defunct comment

* `ServerDriveQueryDirectoryRequest` and `SharedDirectoryListRequest/Response` (#34746)

* `ServerDriveQueryVolumeInformationRequest` and `ClientDriveQueryVolumeInformationResponse` (#34871)

* `DeviceControlRequest` (#34898)

* `Read` (#34914)

* `Write` (#34919)

* `SetInformation` (#35092)

* `LockControl` (#35146)

* Stack pr feedback and other cleanup (#35306)

* Adds desktop access websocket pattern to vite config

* Updates IronRDP to latest, removes manual context strings in favor of automatically generated (by this latest IronRDP commit) (#35392)

* Small line fixes, comment removal, changing license to AGPL

* Update Go/Rust interface section of README (#35590)

* Ironrdp manual docs (#35455)

* Remove unnecessary whitespace

* Refactor navigation paths in Active Directory documentation for clarity

Replaced textual descriptions with formatted paths using `>` symbols for better readability.

Having been subject to going through many such tutorials myself, I find this to be a more
user-friendly way of presenting the information. It's very common to need to go back and
forth between the documentation and the group policy manager, and having these paths in a
format that can catch the eye quickly is a big help. This also appears to be common practice
e.g.

- https://softwarekeep.com/help-center/how-to-enable-remote-desktop-on-windows
- https://www.helpwire.app/blog/remote-desktop-group-policy/

* Add instructions for enabling RemoteFX

* Add an optional step to the end of each GPO update subsection

This lets us point users to subsections individually without worrying
about reminding them that they need to run a `gpupdate /force` each
time (because its already right there in the subsection).

* lowercase "open"

* Updates configuration script to enable RemoteFX (#35461)

* Restructure Cargo workspace (#35591)

* Update `IronRDP` to latest, fix key log leak (#35707)

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.

* Don't leak pin in the logs

* Updates CI to fix broken checks and builds caused by WASM introduction (#35401)

* removes SystemConfiguration framework (not sure why it was there).

* Updates cspell

* fixes TestSetRedirectPageContentSecurityPolicy

* Updates e ref to a currently-draft-PR ref.

* Fix per session mfa (#35902)

* Updating e-ref to tip of `master`

* dependency-review: Ignore cargo dependencies with invalid license specifications

* Updates OSS build-macos.yaml to support wasm-pack

---------

Co-authored-by: Przemko Robakowski <przemko.robakowski@goteleport.com>
Co-authored-by: Mike Jensen <mike.jensen@goteleport.com>
github-merge-queue bot pushed a commit that referenced this pull request Dec 21, 2023
* adds ironrdp client and has it connect, alongside rdp-rs

* Integrates IronRDP client into the TDP png-response system. Instead of 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

* Changes arrayBufferToBase64 to use reduce, which fixes the "Maximum call stack size exceeded" bug

* removes convert_bgra_to_rgba which isn't needed with the bitmaps being produced by IronRDP

* Adds support for updating only a specific image region

* removes mistaken read_rdp_output_inner call from connect_rdp_inner

* Adds support for mouse input. I believe this code has race conditions for client.write_frame though this requires more research.

* Patches things up with the new IronRDP structure

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.

* Adds RemoteFX Frame to the backend

* Proof of concept for integrating a wasm module into the frontend

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.

* Swaps out vite-plugin-wasm-pack for vite-plugin-wasm and adds build-wasm 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]

* Adds backend structure for RPC PoC

Untested as of yet and it isn't pretty, but this more or less imitates the
RPC approach we have for directory sharing.

* hack solution which calls everything 'static. THIS IS VERY UNSAFE, WE SHOULD DO SOMETHING ELSE

* Gets the full display working end to end
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.

* Full display working end to end with the updated IronRDP

* updates init_wasm_log to enable logging in the console

* updates hardcoded io_channel_id and user_channel_id

* Uses specific IronRDP sub-crates (i.e. ironrdp-session, ironrdp-tokio, etc.) rather than relying on whatever is re-exported from the ironrdp sub-crate itself

* Gets FastPathFrameResponse up to speed and the PoC is now working

* deletes all the rpc stuff which it turns out we don't need

* more cleanup including consolidating the FastPathFrame message between wasm and js, adding some comments

* Updates the documentation string for top level Client in lib.rs

* Removes hardcoded rdp server address, screen size, and username

* Swaps to online IronRDP repo

* Updates the TDP RFD with new messages
renaming them to be precise.

* remove hardcoded ip

* creates asRustBackedSlice

* removing some old todos

* Add cleanup for wasm-pack build artifacts to make clean

* Fix wasm in production builds (#27428)

* Converts the default Content-Security-Policy representation to a map
which makes it easier to programmatically add-to/overwrite the defaults
for special cases.

Also adds tests for the various custom CSPs.

* Alphabetize CSP directives for ease of testing/debugging in the future

* makes maps more easily composable

* removes commented code, and topLevelAwait() which was causing production builds to break

* Adds 'wasm-unsafe-eval' to the script-src section of the Content-Security-Policy
when a desktop session's url is detected.

The app already enforces no caching via the Cache-Control, Pragma, and
Expires headers (SetNoCacheHeaders), which means that any time the app
is requested in a new tab, the browser will always request a new index.html
which can be sent back with a new Content-Security-Policy header.

Since the only way to get to a desktop session is to either
click a link in the app which opens a new tab, or to open a new tab and
navigate to the url manually (this differs from most components, which can
be navigated to without a page reload using React Router), we can reliably
check each index.html request for a characteristic desktop session url path,
and only include the 'wasm-unsafe-eval' directive when strictly necessary
(in the spirit of the principle of least privilege).

* Removes vite-plugin-top-level-await from package.json

* moves regex to global so that it only need be compiled once

* CR fixes
updates Cargo.lock
removes commented feature flag
removes -framework SystemConfiguration
fixes makefile comment

* unsafe.SliceData

* fixes phrasing

* changes to docstring

* changes handle_tdp_fast_path_response to handle_tdp_rdp_response_pdu
so that its name is in line with its definition in RFD 0037 and
adds a more descriptive docstring.

* small fixes to comments

* preallocate RDPFastPathPDU Encode buffer

* updating comments

* Adds a detailed discussion of the Go/Rust Interface to the README (#29762)

* Aligns the codebase with the Go/Rust interface principles in the README (#29764)

* Move client functionality into Client (#29765)

* Adds the RDP Channel IDs TDP message (#27514)

* Refactoring `rdpclient` (#29766)

* Ironrdp recording playback (#28228)

* httplib.SetIndexContentSecurityPolicyWithWasm for recordings as well as desktop sessions so that the ironrdp wasm client can be used for recording playback

* Adds desktopplayback path to websocket proxy definitions so that it can be used with the dev server

* adds tdp.TypeRDPFastPathPDU as a tdp message to be recorded

* With this commit, recordings can be played back with the FastPathProcessor

Refactors tdp Client and subclass PlayerClient
such that now the classes are always constructed the same way,
and the internal representation of the screen size (the FastPathProcessor)
is either:

1. Created upon the call to Client.connect(spec) when spec is !undefined.
This is the case when the Client is being used for a desktop session.

2. Created when a client screen spec is received from the server. This is
the case when PlayerClient is being used for playback.

* Switches from sending the width/height via ur parameter string
to sending it via the client screen spec tdp message.

* changes tdpCli* to client* and adds a canvas* to the canvas related functions

* Static tokio (#30066)

* Beginnings of RdpdrBackend integration (#32814)

* Adds handling for the EstablishContextCall (#32971)

* Adds support for `ListReadersCall` and `ListReadersReturn` (#33004)

* Adds support for `GetStatusChangeCall`/`GetStatusChangeReturn` (#33056)

* Adds support for ConnectCall and ConnectReturn (#33136)

* Better error handling (#32925)

This change removes couple of unwrap that could cause panics on the Rust side and prevents panics from invalid cgo handle on the Go side.
It also sends error message from IronRDP to UI and cleans up a little final sequence of closing various goroutines in case of error or sessions end.

* Adds support for `HCardAndDispositionCall` with `ScardIoCtlCode::BeginTransaction` (#33192)

* `TransmitCall` and `TransmitReturn` (#33282)

* `StatusCall` and `StatusReturn` (#33284)

* `ReleaseContext` and refactor `handle_scard_call` (#33434)

* IronRDP clipboard sharing (#33335)

This change adds clipboard sharing to our IronRDP migration using ironrdp_cliprdr crate with out TDP-based backend (reused from rdp-rs times)

* Adds EndTransaction, Disconnect, Cancel, and IsValidContext (#33612)

* Adds support for `GetDeviceTypeId` (#33613)

* Adds support for `ReadCacheW` (#33618)

* `WriteCacheW` (#33620)

* Smartcard Login (#33712)

* `GetReaderIcon` (#33779)

* Update to IronRDP latest commit

* Removes a bunch of old deprecated code (including `rdp-rs`) (#33786)

* Pipes in `SharedDirectoryAnnounce` (#33911)

* Use tokio-boring for TLS upgrade (#33900)

This change switches TLS upgrade in our new RDP approach to tokio-boring in FIPS-compliant builds.
It also adds runtime check for FIPS mode.

Tested locally in custom Docker image.

* Update sspi (#34558)

* Break up rdpdr.rs, add support for the first directory sharing pdus (#34009)

* Adds support for `SharedDirectoryCreateRequest` and `SharedDirectoryCreateResponse`. (#34011)

* `SharedDirectoryDeleteRequest` and `SharedDirectoryDeleteResponse` (#34051)

* remove leftover code

* Server drive query information request (#34082)

* Rdp client refactor (#34122)

* Add handling for `DeviceCloseRequest` (#34139)

* Updates to `IronRDP` latest (#34744)

* Updates to IronRDP latest with a slightly different API

* remove defunct comment

* `ServerDriveQueryDirectoryRequest` and `SharedDirectoryListRequest/Response` (#34746)

* `ServerDriveQueryVolumeInformationRequest` and `ClientDriveQueryVolumeInformationResponse` (#34871)

* `DeviceControlRequest` (#34898)

* `Read` (#34914)

* `Write` (#34919)

* `SetInformation` (#35092)

* `LockControl` (#35146)

* Stack pr feedback and other cleanup (#35306)

* Adds desktop access websocket pattern to vite config

* Updates IronRDP to latest, removes manual context strings in favor of automatically generated (by this latest IronRDP commit) (#35392)

* Small line fixes, comment removal, changing license to AGPL

* Update Go/Rust interface section of README (#35590)

* Ironrdp manual docs (#35455)

* Remove unnecessary whitespace

* Refactor navigation paths in Active Directory documentation for clarity

Replaced textual descriptions with formatted paths using `>` symbols for better readability.

Having been subject to going through many such tutorials myself, I find this to be a more
user-friendly way of presenting the information. It's very common to need to go back and
forth between the documentation and the group policy manager, and having these paths in a
format that can catch the eye quickly is a big help. This also appears to be common practice
e.g.

- https://softwarekeep.com/help-center/how-to-enable-remote-desktop-on-windows
- https://www.helpwire.app/blog/remote-desktop-group-policy/

* Add instructions for enabling RemoteFX

* Add an optional step to the end of each GPO update subsection

This lets us point users to subsections individually without worrying
about reminding them that they need to run a `gpupdate /force` each
time (because its already right there in the subsection).

* lowercase "open"

* Updates configuration script to enable RemoteFX (#35461)

* Restructure Cargo workspace (#35591)

* Update `IronRDP` to latest, fix key log leak (#35707)

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.

* Don't leak pin in the logs

* Show clear error when export can't be done

* Show url of recording in webui

* patching up merge mistake

* use EndTime and StartTime to calculate duration

---------

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants