Repository navigation
feat(web): expose enable_standard_rdp_security - #2004
Benoît Cortier (CBenoit) merged 4 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The security-sensitive RDCleanPath connection branch lacks direct test coverage.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Exposes Standard RDP Security to web clients using trusted RDCleanPath transports.
Changes:
- Adds the TypeScript configuration extension and documentation.
- Propagates security settings into the Rust connector.
- Skips TLS certificate handling for Standard RDP Security and adds configuration tests.
| File | Description |
|---|---|
web-client/iron-remote-desktop-rdp/src/main.ts |
Exposes the security extension. |
web-client/iron-remote-desktop-rdp/README.md |
Documents usage and security requirements. |
crates/ironrdp-web/src/session.rs |
Implements negotiation and certificate-handling changes. |
|
Automated review will not run because this contributor is not yet eligible under the automation policy. Contributors become eligible after one qualifying IronRDP pull request is merged into |
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
LGTM!
I manually ran
cargo xtask ciwhich work successfully apart from having to fix wasm2wat installation on linux-arm64 (can do a follow up PR on that, it's currently hardcoded to linux-x64)
I would appreciate that, thank you!
enable_standard_rdp_security in the web-clientenable_standard_rdp_security in the web-client
enable_standard_rdp_security in the web-clientenable_standard_rdp_security
Head branch was pushed to by a user without write access
|
Benoît Cortier (@CBenoit) sorry for the churn, one lint error had gone in the previous commit unnoticed. Fixed it and re-ran |
There was a problem hiding this comment.
The PR exposes an opt-in Standard RDP Security (PROTOCOL_RDP) option to the WASM web client: a SecurityConfig struct plumbs enable_standard_rdp_security into build_config, which disables TLS and CredSSP when set; connect_rdcleanpath derives the negotiated protocol from the X.224 Connection Confirm and skips proxy certificate/public-key extraction on the PROTOCOL_RDP path; a vmconnect mutual-exclusion guard, TS extension, README, and tests are added. The wiring is internally consistent and fail-closed: only PROTOCOL_RDP is advertised when the option is set, out-of-set server selections are rejected, non-empty Server Security Data is rejected, and the Security Exchange PDU is omitted only for the ENCRYPTION_LEVEL_NONE variant. Default-path behavior is unchanged. Four valid issues remain: the newly enabled path advertises zero encryption methods (an MS-RDPBCGR 2.2.1.3.3 conformance gap limiting interop to no-encryption peers, fail-closed rather than a downgrade), a documented but inconsi…
|
Benoît Cortier (@CBenoit) just to check since you previously approved the PR but those new comments from github-actions are new, do you want me to address them? |
15df4f2 to
6391314
Compare
|
Unsure about the "Pull request automation / Check public API compatibility (pull_request_target)" check, it seems to be coming from a dependency that fails to build rather than the changes themselves EDIT: should be fixed by #2071 |
|
Update: no rebase needed anymore. picky-krb 0.12.5 was yanked from crates.io (re-released as 0.13.0), so the API check builds again without changes to this branch. PR automation has been re-run here and passes. |
6391314 to
4cee32c
Compare
There was a problem hiding this comment.
PR exposes the previously merged Standard RDP Security (PROTOCOL_RDP) connector capability to the WASM web client via a new enable_standard_rdp_security builder option, threads a SecurityConfig into build_config, handles the RDCleanPath proxy-certificate-less PROTOCOL_RDP path, and documents/tests the feature. Verified independently: default behavior is unchanged; the connector core rejects a server-selected PROTOCOL_RDP unless requested, so skipping proxy-cert parsing is not downgrade-reachable; forcing enable_tls=false alongside standard RDP security is security-positive; the VMConnect mutual exclusion is justified because the VMConnect flow yields no X.224 response. Three valid specialist findings are published: one medium protocol-conformance limitation (the exposed path sends an empty encryptionMethods set and supports only the ENCRYPTION_LEVEL_NONE variant, so it only interoperates with peers accepting a nonconformant no-encryption configuration, and the README overstates client…
- [code-compressor] Two separate matches over the same x224_connection_response Option can be consolidated — low 🟡 — crates/ironrdp-web/src/session.rs
The added block in connect_rdcleanpath branches on x224_connection_response (if let Some) to check state, step the connector, and derive standard_rdp_security; the pre-existing match below then branches on the same Option again with a Some(_) arm that only calls skip_connect_begin/mark_as_upgraded. A single match returning (standard_rdp_security, upgraded) would remove the duplicated dispatch and the discarded binding. Behavior is preserved, including borrow ordering since is_standard_rdp_security() copies to a bool before the mutable skip_connect_begin call. Optional, low-severity readability/simplification tradeoff rather than a required change.
| enable_tls: !security.enable_standard_rdp_security, | ||
| enable_credssp: security.enable_credssp && !security.enable_standard_rdp_security, | ||
| enable_standard_rdp_security: security.enable_standard_rdp_security, |
There was a problem hiding this comment.
[protocol] Exposed Standard RDP Security path violates Client Security Data and Security Exchange PDU requirements — medium 🟠 — Enabling enable_standard_rdp_security advertises PROTOCOL_RDP, making the Client Security Data relevant, but the connector sends encryptionMethods = 0 (connection.rs:1749) while MS-RDPBCGR requires the client to specify at least one encryption method, and IronRDP only supports the ENCRYPTION_LEVEL_NONE variant with no Security Exchange PDU (connection.rs:466-469). Conformant servers select a nonzero Encryption Level for Standard RDP Security and expect the Security Exchange PDU, so the newly exposed option only interoperates with peers accepting a nonconformant no-encryption configuration (e.g., Weston/FreeRDP local-listener setups). The README's claim of ENCRYPTION_LEVEL_NONE overstates client control, since the server selects the cryptographic configuration per MS-RDPBCGR 5.3.2. Consider documenting this interop limitation more precisely in the README.
| #[derive(Clone, Copy)] | ||
| struct SecurityConfig { | ||
| enable_credssp: bool, | ||
| enable_standard_rdp_security: bool, | ||
| } |
There was a problem hiding this comment.
[skeptical] SecurityConfig struct adds an abstraction with marginal benefit over two bool parameters — low 🟡 — The new private Copy struct bundles exactly two booleans consumed in only two production places (the connect() destructure and build_config). build_config already takes the sibling option as a plain legacy_graphics: bool, so two bool parameters would be the narrower, convention-consistent change. The struct encodes no mutual-invalidation invariant (the standard-RDP/credssp and standard-RDP/vmconnect exclusions live elsewhere) and 'security' hides which flags are passed. Impact is low: the code is correct and the type is crate-private; this is a simplification opportunity, not a defect.
|
Thank you Jérémie Laval (@jeremie-stripe) I’ll merge now, but I would appreciate if you could look into the AI review comments, and see if it’s relevant or not, I would accept that in a follow up PR 🙂 |
27d1571
into
Devolutions:master

Expose the work done in #1580 to the web-client package in addition to the desktop + library version.
In the process, partially work on solving #327 in the sense that at least one security option is now exposed to the client.
The rationale for this change is similar to the original PR. In case where the gateway lives directly on the host serving the RDP server and is able to communicate with it via a OS-level pipe there is no need to have an extraneous TLS connection be forwarded between client and server and the underlying websocket TLS traffic between the client and the gateway is sufficient.
In our situation, we are not dealing with Windows Sandbox but instead are trying to use Weston RDP support (based on FreeRDP). Weston supports a mode where a unix domain socket can be directly passed to the process to listen to incoming connection and, in that mode, it's intentionally wired to not require a TLS connection:
I manually ran
cargo xtask ciwhich work successfully apart from having to fix wasm2wat installation on linux-arm64(can do a follow up PR on that, it's currently hardcoded to linux-x64)which is fixed in #2035