feat(connector): add `allow_encryption_level_none` config option by maurerdietmar · Pull Request #1214 · Devolutions/IronRDP

Dietmar Maurer (maurerdietmar) · 2026-04-10T05:57:59Z

Add a new Config flag that permits the connector to accept ENCRYPTION_LEVEL_NONE / ENCRYPTION_METHOD_NONE from the server. This does not imply full Standard RDP Security (RC4) support; it only allows the degenerate case where the server selects PROTOCOL_RDP with no encryption at all. When enabled, the connector skips the TLS/CredSSP upgrade and proceeds directly to basic settings exchange.

This is intended for setups where the transport is already secured by other means (e.g., a TLS WebSocket proxy or SSH tunnel).

Benoît Cortier (CBenoit)

This is looking good to me! Thank you!

Copilot

Pull request overview

Adds an explicit connector configuration flag to permit a no-TLS / no-CredSSP / no RDP-layer encryption connection when the server negotiates PROTOCOL_RDP with ENCRYPTION_LEVEL_NONE / ENCRYPTION_METHOD_NONE, intended for deployments where the transport is secured externally.

Changes:

Introduces allow_encryption_level_none: bool on ironrdp_connector::Config (defaulted to false across call sites).
Updates the connector negotiation state machine to allow Standard RDP Security only when explicitly enabled and to skip the enhanced security upgrade in that case.
Propagates the new field to various config construction sites (FFI builder, CLI config, WASM session config, examples, tests).

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`crates/ironrdp-connector/src/lib.rs`	Adds the new config flag with security-focused documentation.
`crates/ironrdp-connector/src/connection.rs`	Changes negotiation flow to permit/short-circuit Standard RDP Security when allowed.
`ffi/src/connector/config.rs`	Updates FFI config build to populate the new field (currently hardcoded `false`).
`crates/ironrdp-client/src/config.rs`	Updates CLI config construction to include the new field (default `false`).
`crates/ironrdp-web/src/session.rs`	Updates WASM session config construction to include the new field (default `false`).
`crates/ironrdp/examples/screenshot.rs`	Updates example config construction to include the new field (default `false`).
`crates/ironrdp-testsuite-extra/tests/mod.rs`	Updates test default config construction to include the new field (default `false`).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-10T10:26:46Z

+                if selected_protocol.is_standard_rdp_security() && requested_protocol.is_standard_rdp_security() {
+                    // Both sides agreed on standard RDP security — skip the
+                    // enhanced security upgrade and go straight to basic
+                    // settings exchange.
+                    (
+                        Written::Nothing,
+                        ClientConnectorState::BasicSettingsExchangeSendInitial { selected_protocol },
+                    )
+                } else if !selected_protocol.intersects(requested_protocol) {


The new Standard RDP Security path skips EnhancedSecurityUpgrade, but ironrdp_async::connect_begin currently loops until should_perform_security_upgrade() becomes true (see crates/ironrdp-async/src/connector.rs:26-30). With this branch, connect_begin will never return for PROTOCOL_RDP/no-security connections, and connect_finalize can’t be called because it requires an Upgraded token. Consider updating the async/IO wrappers to support a “no-upgrade required” path (e.g., return an enum from connect_begin, and allow connect_finalize to accept either upgraded or plain streams).

Suggested change

if selected_protocol.is_standard_rdp_security() && requested_protocol.is_standard_rdp_security() {

// Both sides agreed on standard RDP security — skip the

// enhanced security upgrade and go straight to basic

// settings exchange.

(

Written::Nothing,

ClientConnectorState::BasicSettingsExchangeSendInitial { selected_protocol },

)

} else if !selected_protocol.intersects(requested_protocol) {

if !selected_protocol.intersects(requested_protocol) {

Copilot · 2026-04-10T10:26:47Z

-                if security_protocol.is_standard_rdp_security() {
-                    return Err(reason_err!("Initiation", "standard RDP security is not supported",));
+                if security_protocol.is_standard_rdp_security() && !self.config.allow_encryption_level_none {
+                    return Err(reason_err!("Initiation", "ENCRYPTION_LEVEL_NONE is not allowed",));


This error is triggered when the client is negotiating Standard RDP Security (protocol field is empty), but at this stage we don’t yet know whether the server will select ENCRYPTION_LEVEL_NONE vs requiring RC4. The message “ENCRYPTION_LEVEL_NONE is not allowed” is misleading for users who simply disabled TLS/NLA. Consider rewording to something like “Standard RDP security without TLS/NLA is disabled by default; set allow_encryption_level_none to enable (plaintext)” and keep the explicit security warning context.

Suggested change

return Err(reason_err!("Initiation", "ENCRYPTION_LEVEL_NONE is not allowed",));

return Err(reason_err!(

"Initiation",

"Standard RDP security without TLS/NLA is disabled by default; set allow_encryption_level_none to enable (plaintext)",

));

Copilot · 2026-04-10T10:26:47Z

+    ///
+    /// Do **not** enable this unless the underlying transport is already
+    /// encrypted. Without TLS, all RDP traffic (including credentials) is
+    /// sent in plaintext.
+    pub allow_encryption_level_none: bool,


Config is a public struct with public fields; adding a new public field is a semver-breaking change for downstream users who construct Config via struct literals. If this crate aims to preserve semver compatibility, consider making Config #[non_exhaustive] and/or providing a builder/constructor so new options can be added without breaking consumers (or ensure the next release version reflects the breaking change).

Copilot · 2026-04-10T10:26:47Z

@@ -176,6 +176,7 @@ pub mod ffi {
                domain: self.domain.clone(),
                enable_tls: self.enable_tls.unwrap_or(false),
                enable_credssp: self.enable_credssp.unwrap_or(true),


allow_encryption_level_none is hardcoded to false in the FFI builder, and there’s no corresponding ConfigBuilder field/setter. If this option is meant to be available to FFI consumers, it should be plumbed through as an Option<bool> with a setter similar to set_enable_tls / set_enable_credssp. If it’s intentionally not exposed for safety, consider documenting that decision in the FFI API (otherwise it looks like an incomplete wiring change).

Suggested change

enable_credssp: self.enable_credssp.unwrap_or(true),

enable_credssp: self.enable_credssp.unwrap_or(true),

// Intentionally not exposed through the FFI builder: allowing

// encryption level "none" weakens transport security, so FFI

// consumers are always pinned to the safe default here.

Copilot · 2026-04-10T10:26:48Z

        desktop_scale_factor: 0, // Default to 0 per FreeRDP
        enable_tls: true,
        enable_credssp: true,
+        allow_encryption_level_none: false,
        credentials: connector::Credentials::UsernamePassword {


This PR introduces a new connector behavior (allowing PROTOCOL_RDP with no security), but the testsuite currently only exercises TLS-secured connections (RdpServer::builder().with_tls(...)). Consider adding coverage here for both cases: (1) default config rejects no-security negotiation, and (2) when allow_encryption_level_none = true + TLS/NLA disabled, the client can connect to a server built with with_no_security() and complete the handshake.

Benoît Cortier (CBenoit) · 2026-04-10T10:30:07Z

There is a compilation error at crates/ironrdp-testsuite-core/tests/session/autodetect.rs:23:5, you need to add the new field to the struct initialization

Add a new `Config` flag that permits the connector to accept `ENCRYPTION_LEVEL_NONE` / `ENCRYPTION_METHOD_NONE` from the server. This does not imply full Standard RDP Security (RC4) support; it only allows the degenerate case where the server selects `PROTOCOL_RDP` with no encryption at all. When enabled, the connector skips the TLS/CredSSP upgrade and proceeds directly to basic settings exchange. This is intended for setups where the transport is already secured by other means (e.g., a TLS WebSocket proxy or SSH tunnel). Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>

Dietmar Maurer (maurerdietmar) · 2026-04-10T10:37:17Z

just fixed the compilation error.

… transport Adds a sibling method to RdpServer::run_connection that walks the same per-connection state machine but skips the IronRDP-managed TLS handshake step. The caller's stream must already be transport-encrypted at a lower layer — typically a WSS terminator in a proxy-shaped deployment. The motivating use case is the RDCleanPath protocol used by Devolutions Gateway and Cloudflare's IronRDP-WASM deployment: an RDCleanPath-aware client connects via WSS; the WSS layer terminates TLS end-to-end with the client; the server side sees a post-TLS plaintext byte stream and must not re-run TLS on it. The implementation mirrors run_connection's body except for the single tls_acceptor.accept(stream).await call. Acceptor::mark_security_upgrade_as_done advances the state machine without performing TLS; the Hybrid CredSSP block runs identically (CredSSP carries its own crypto via TSRequest). A TODO comment marks the in-sync requirement with run_connection's ShouldUpgrade arm so future rebases catch upstream divergence. The doc comment names the two preconditions explicitly: - Caller MUST ensure the stream is transport-encrypted at a lower layer - The connecting client MUST speak a proxy protocol that informs it of the lower-layer TLS (vanilla mstsc/xfreerdp will not work — they expect TLS after seeing PROTOCOL_SSL selected; see PR Devolutions#1210 cautionary tale) Fork-only patch; intended for upstream contribution after lamco-rdp-server v1.5 ships and proves the pattern in production. Mirrors the opt-in-helper shape of PR Devolutions#1214 (allow_encryption_level_none).

Benoît Cortier (CBenoit) · 2026-08-03T08:29:44Z

The public Config field and changed security-negotiation behavior warrant breaking-change and high-risk labels; coverage is missing for the newly reachable standard-RDP no-encryption path, including absent Server Security Data, skipped Security Exchange, and unencrypted Client Info.

Greg Lamberson (glamberson) · 2026-08-26T22:32:08Z

Dietmar Maurer (@maurerdietmar) Following up here. CBenoit's April approval still stands on the design, but his August 3rd comment raised a real gap that hasn't been addressed yet: the newly-reachable no-encryption path needs test coverage for Server Security Data handling, the skipped Security Exchange, and unencrypted Client Info. Given this touches connection security negotiation, that coverage matters.

The branch is also well behind master at this point and will need a rebase. If you can add that coverage and get it rebased, I'll advocate for getting this merged.

Benoît Cortier (CBenoit) · 2026-09-29T09:00:31Z

Copilot I suspect this PR was superseded by another one, already merged. Verify that.

Greg Lamberson (glamberson) · 2026-09-29T21:29:33Z

Dietmar Maurer (@maurerdietmar) I need to correct my comment of August 26, because I lost track of the earlier work on this. On August 8, #1580 merged enable_standard_rdp_security, which already does what this PR sets out to do: When it is on, the connector advertises PROTOCOL_RDP and supports only the ENCRYPTION_LEVEL_NONE variant, with no Security Exchange, and it handles that case inside the existing EnhancedSecurityUpgrade state. So the test coverage and rebase I asked for are no longer needed here, and I should not have said I would advocate for merging it. Thank you for the work on this.

Benoît Cortier (CBenoit) · 2026-10-08T12:50:33Z

Thank you Dietmar Maurer (@maurerdietmar) for this work and your patience. As Greg confirmed above, this is now covered by #1580 (enable_standard_rdp_security), which is also being exposed to the web client in #2004. Closing as superseded; feel free to reopen or open a follow-up if you find a gap in the merged implementation for your use case.

Dietmar Maurer (maurerdietmar) mentioned this pull request Apr 10, 2026

feat(connector): add opt-in standard RDP security support #1201

Closed

Benoît Cortier (CBenoit) requested a review from Copilot April 10, 2026 10:19

Copilot started reviewing on behalf of Benoît Cortier (CBenoit) April 10, 2026 10:19 View session

Benoît Cortier (CBenoit) approved these changes Apr 10, 2026

View reviewed changes

Copilot AI reviewed Apr 10, 2026

View reviewed changes

Dietmar Maurer (maurerdietmar) force-pushed the allow_encryption_level_none branch from 7330926 to f8fb2dd Compare April 10, 2026 10:35

Benoît Cortier (CBenoit) enabled auto-merge (squash) April 10, 2026 10:55

github-actions Bot added scope/web Affects the web/WASM ecosystem scope/core Touches the core architectural tier A-extra size/S Size: up to 199 counted lines and 5 files; exceeds XS in either measure labels Jul 31, 2026

Benoît Cortier (CBenoit) added breaking-change Includes a breaking change, and requires special scrutiny at the boundaries risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny ai-reviewed/1 One automated review completed labels Aug 3, 2026

Benoît Cortier (CBenoit) removed rust labels Aug 4, 2026

Benoît Cortier (CBenoit) closed this Oct 8, 2026

auto-merge was automatically disabled October 8, 2026 12:50
Pull request was closed

github-actions Bot removed the needs-author-action The pull request author is the current next actor label Oct 8, 2026

-                enable_credssp: self.enable_credssp.unwrap_or(true),
+                enable_credssp: self.enable_credssp.unwrap_or(true),
+                // Intentionally not exposed through the FFI builder: allowing
+                // encryption level "none" weakens transport security, so FFI
+                // consumers are always pinned to the safe default here.

Conversation

Dietmar Maurer (maurerdietmar) commented Apr 10, 2026

Uh oh!

Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) commented Apr 10, 2026

Uh oh!

Dietmar Maurer (maurerdietmar) commented Apr 10, 2026

Uh oh!

Benoît Cortier (CBenoit) commented Aug 3, 2026

Uh oh!

Greg Lamberson (glamberson) commented Aug 26, 2026

Uh oh!

Benoît Cortier (CBenoit) commented Sep 29, 2026

Uh oh!

Greg Lamberson (glamberson) commented Sep 29, 2026

Uh oh!

Benoît Cortier (CBenoit) commented Oct 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants