Skip to content

feat(web): expose enable_standard_rdp_security - #2004

Merged
Benoît Cortier (CBenoit) merged 4 commits into
Devolutions:masterfrom
jeremie-stripe:jeremie-web-security-negotiation
Oct 8, 2026
Merged

Benoît Cortier (CBenoit) merged 4 commits into
Devolutions:masterfrom
jeremie-stripe:jeremie-web-security-negotiation

Conversation

@jeremie-stripe

@jeremie-stripe Jérémie Laval (jeremie-stripe) commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

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:

--external-listener-fd=fd
Specifies a file descriptor inherited from the process that launched weston to be listened on for client connections. Only local (such as AF_VSOCK) sockets should be used, as this will be considered to be a local connection by the RDP backend, and TLS and RDP security will be bypassed.

I manually ran cargo xtask ci which 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

@jeremie-stripe
Jérémie Laval (jeremie-stripe) marked this pull request as ready for review September 25, 2026 20:56
Copilot AI balanced review requested due to automatic review settings September 25, 2026 20:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 Low severity

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.

Comment thread crates/ironrdp-web/src/session.rs
@github-actions github-actions Bot added kind/protocol Affects RDP or related protocol behavior risk/medium Behavioral change that does not substantially alter a core public API scope/web Affects the web/WASM ecosystem size/S Size: up to 199 counted lines and 5 files; exceeds XS in either measure needs-review A human reviewer is the current next actor labels Sep 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

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 master. Maintainer review is required.

@github-actions github-actions Bot added size/M Size: up to 449 counted lines and 10 files; exceeds S in either measure needs-review A human reviewer is the current next actor and removed size/S Size: up to 199 counted lines and 5 files; exceeds XS in either measure needs-review A human reviewer is the current next actor labels Sep 25, 2026

@CBenoit Benoît Cortier (CBenoit) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

I manually ran cargo xtask ci which 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!

@CBenoit Benoît Cortier (CBenoit) changed the title Expose enable_standard_rdp_security in the web-client feat(web): expose enable_standard_rdp_security in the web-client Sep 28, 2026
@CBenoit Benoît Cortier (CBenoit) changed the title feat(web): expose enable_standard_rdp_security in the web-client feat(web): expose enable_standard_rdp_security Sep 28, 2026
auto-merge was automatically disabled September 28, 2026 13:37

Head branch was pushed to by a user without write access

@github-actions github-actions Bot removed the needs-review A human reviewer is the current next actor label Sep 28, 2026
@jeremie-stripe

Copy link
Copy Markdown
Contributor Author

Benoît Cortier (@CBenoit) sorry for the churn, one lint error had gone in the previous commit unnoticed. Fixed it and re-ran cargo xtask check lints -v to make sure there were none left

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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…

Comment thread crates/ironrdp-web/src/session.rs
Comment thread crates/ironrdp-web/src/session.rs
Comment thread crates/ironrdp-web/src/session.rs Outdated
Comment thread crates/ironrdp-web/src/session.rs
@github-actions github-actions Bot added the ai-reviewed/1 One automated review completed label Oct 1, 2026
@github-actions github-actions Bot added the needs-author-action The pull request author is the current next actor label Oct 1, 2026
@jeremie-stripe

Copy link
Copy Markdown
Contributor Author

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?

@github-actions github-actions Bot added automation-failed Exact-head automated classification or review failed or was unavailable risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny and removed needs-author-action The pull request author is the current next actor risk/medium Behavioral change that does not substantially alter a core public API labels Oct 6, 2026
@jeremie-stripe

Jérémie Laval (jeremie-stripe) commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor Author

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

@CBenoit

Benoît Cortier (CBenoit) commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

PR automation is failing because of a picky-krb 0.12.5 incompatibility, fixed on master by #2074. Please rebase on master to fix it.

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.

@github-actions github-actions Bot added risk/medium Behavioral change that does not substantially alter a core public API and removed risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny automation-failed Exact-head automated classification or review failed or was unavailable labels Oct 7, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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…

  1. [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.

Comment on lines +1500 to +1502
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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[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.

Comment on lines +55 to +59
#[derive(Clone, Copy)]
struct SecurityConfig {
enable_credssp: bool,
enable_standard_rdp_security: bool,
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[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.

@github-actions github-actions Bot added ai-reviewed/2 Two automated reviews completed needs-author-action The pull request author is the current next actor and removed ai-reviewed/1 One automated review completed labels Oct 7, 2026
@CBenoit

Copy link
Copy Markdown
Member

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 🙂

@CBenoit
Benoît Cortier (CBenoit) merged commit 27d1571 into Devolutions:master Oct 8, 2026
43 of 44 checks passed
@github-actions github-actions Bot removed the needs-author-action The pull request author is the current next actor label Oct 8, 2026

This branch was successfully deployed

1 active deployment
llm-providers — 4cee32c7 Deployed Oct 7, 2026 by jeremie-stripe via Classify pull request #1702
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed/2 Two automated reviews completed kind/protocol Affects RDP or related protocol behavior risk/medium Behavioral change that does not substantially alter a core public API scope/web Affects the web/WASM ecosystem size/M Size: up to 449 counted lines and 10 files; exceeds S in either measure

Development

Successfully merging this pull request may close these issues.

3 participants