From 5f85e427c3bb47f7afeeaaa5b941bc5616e26ed7 Mon Sep 17 00:00:00 2001 From: Anton Mostovoy Date: Sun, 20 Sep 2026 12:28:32 -0500 Subject: [PATCH 1/2] feat(server): default ConnectionPolicy to Preempt under Hybrid only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-expressed on top of #1913, which replaced the bool this PR originally flipped with `ConnectionPolicy { Queue, Reject, Preempt }`. The default is now mode-aware rather than a blanket `Preempt`, as every reviewer asked: `ConnectionPolicy::default_for(&security)` returns `Preempt` under `Hybrid` — the one mode that authenticates the client before a candidate could evict anything — and `Queue` under `Tls` and `None`, where the per-mode table shows any peer able to complete the handshake clears the bar and the anti-storm cooldown bars the victim, not the attacker. Takeover stays the least-surprising default where it is safe, and an unauthenticated takeover is an explicit opt-in. The derived `Default` (a mode-independent `Queue`) is removed so there is a single source of truth; both builder initializers call `default_for` with the security already chosen on the builder. Also closes the gap glamberson and the reviewer flagged in the eviction arm: the Set Error Info PDU was sent unconditionally behind a KNOWN-GAP comment claiming `AcceptorResult` exposes no early-capability field. It does (`client_early_capability_flags`, already used in this file to gate heartbeats), so MS-RDPBCGR 3.3.5.7.1 is now honoured with no API change — `SUPPORT_ERR_INFO_PDU` is threaded through `client_loop` into `dispatch_server_events` like the heartbeat flag, and both arms that carry a disconnect reason (`EvictedByOtherConnection` and the public `ErrorInfoDisconnectHandle` path) drop the PDU and just disconnect for a client that did not opt in. Tests, in ironrdp-testsuite-core so they actually run in CI (`ironrdp-server` has `[lib] test = false`): `default_for` pinned for all three modes, and the `with_no_security` default driven end to end through `run` (a second connection is left waiting, not served). The `Tls`/`Hybrid` values are built from a certificate-less `TlsAcceptor` (stub `ResolvesServerCert`), hence the `tokio-rustls` dev-dependency. Co-Authored-By: Claude Opus 5 --- Cargo.lock | 1 + crates/ironrdp-server/src/builder.rs | 12 +- crates/ironrdp-server/src/server.rs | 106 +++++++++++++++--- crates/ironrdp-testsuite-core/Cargo.toml | 1 + .../tests/server/connection_policy.rs | 80 +++++++++++-- 5 files changed, 170 insertions(+), 30 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7e3280ae94..f70a30b569 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3435,6 +3435,7 @@ dependencies = [ "proptest", "rstest", "tokio", + "tokio-rustls", "visibility", ] diff --git a/crates/ironrdp-server/src/builder.rs b/crates/ironrdp-server/src/builder.rs index a511adb8dd..3d3ca5d7d9 100644 --- a/crates/ironrdp-server/src/builder.rs +++ b/crates/ironrdp-server/src/builder.rs @@ -151,6 +151,7 @@ impl RdpServerBuilder { where D: RdpServerDisplay + 'static, { + let connection_policy = ConnectionPolicy::default_for(&self.state.security); RdpServerBuilder { state: BuilderDone { addr: self.state.addr, @@ -177,7 +178,7 @@ impl RdpServerBuilder { autodetect_bandwidth: None, autodetect_bandwidth_generation: None, honor_client_desktop_size: None, - connection_policy: ConnectionPolicy::default(), + connection_policy, auto_reconnect_cookie: None, remotefx_quant: Quant::default(), remotefx_entropy_coder: None, @@ -187,6 +188,7 @@ impl RdpServerBuilder { } pub fn with_no_display(self) -> RdpServerBuilder { + let connection_policy = ConnectionPolicy::default_for(&self.state.security); RdpServerBuilder { state: BuilderDone { addr: self.state.addr, @@ -213,7 +215,7 @@ impl RdpServerBuilder { autodetect_bandwidth: None, autodetect_bandwidth_generation: None, honor_client_desktop_size: None, - connection_policy: ConnectionPolicy::default(), + connection_policy, auto_reconnect_cookie: None, remotefx_quant: Quant::default(), remotefx_entropy_coder: None, @@ -354,10 +356,14 @@ impl RdpServerBuilder { /// Choose what [`RdpServer::run`] does with a second connection that /// arrives while a session is already being served: leave it in the backlog - /// ([`ConnectionPolicy::Queue`], the default), close it immediately + /// ([`ConnectionPolicy::Queue`]), close it immediately /// ([`ConnectionPolicy::Reject`]), or let a fully-authenticated newcomer /// take the session over ([`ConnectionPolicy::Preempt`]). /// + /// The default follows the security mode already chosen on this builder -- + /// `Preempt` under [`RdpServerSecurity::Hybrid`], `Queue` otherwise; see + /// [`ConnectionPolicy::default_for`] for why. + /// /// `Preempt`'s takeover is only authentication-gated under /// [`RdpServerSecurity::Hybrid`]; see [`ConnectionPolicy::Preempt`] for the /// per-mode security table. `Reject` closes a newcomer without consulting diff --git a/crates/ironrdp-server/src/server.rs b/crates/ironrdp-server/src/server.rs index 8dc93c4c99..97192fd10a 100644 --- a/crates/ironrdp-server/src/server.rs +++ b/crates/ironrdp-server/src/server.rs @@ -325,18 +325,22 @@ impl CredentialValidator for ExactMatchCredentialValidator { /// What [`RdpServer::run`] does with a second connection that arrives while a /// session is already being served. /// -/// [`RdpServer`] serves one connection at a time. By default a second -/// connection accepted while one is live is left unserved in the OS listen -/// backlog -- from that client's point of view, a silent hang until the first -/// session ends. That is `ironrdp-server`'s pre-existing behaviour, kept as the -/// default ([`Queue`](ConnectionPolicy::Queue)) so an embedder that already -/// relies on it is not surprised by upgrading. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +/// [`RdpServer`] serves one connection at a time, so a second connection +/// accepted while one is live has to go somewhere: left in the OS listen +/// backlog ([`Queue`](ConnectionPolicy::Queue)), closed at once +/// ([`Reject`](ConnectionPolicy::Reject)), or served in place of the running +/// session ([`Preempt`](ConnectionPolicy::Preempt)). +/// +/// The default depends on the security mode -- see +/// [`ConnectionPolicy::default_for`]. There is deliberately no +/// mode-independent [`Default`]: whether takeover is safe out of the box is +/// decided by whether the client is authenticated before it could evict +/// anything, and only the security mode knows that. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum ConnectionPolicy { /// Leave the extra connection in the OS listen backlog until the running /// session ends. The pre-existing behaviour: the second client is not /// answered and appears to hang until the first leaves. - #[default] Queue, /// Close the extra connection immediately. The running session is never /// interrupted; the new client fails fast and can retry rather than @@ -389,6 +393,37 @@ pub enum ConnectionPolicy { Preempt, } +impl ConnectionPolicy { + /// The policy [`RdpServerBuilder`](crate::RdpServerBuilder) starts from + /// for a given security mode. + /// + /// Takeover is the least-surprising behaviour for the single-session + /// servers `ironrdp-server` typically backs (one mirroring one desktop): a + /// newly connecting client should replace a stale or abandoned one, not + /// hang behind it. But takeover is only *safe* out of the box where the + /// newcomer is authenticated before it could evict anything, and per the + /// table on [`Preempt`](ConnectionPolicy::Preempt) that is + /// [`RdpServerSecurity::Hybrid`] alone -- under `Tls` or `None` any peer + /// able to complete the handshake clears the bar, and the anti-storm + /// cooldown bars the victim rather than the attacker. So: + /// + /// | Security mode | Default | + /// |---|---| + /// | [`Hybrid`](RdpServerSecurity::Hybrid) | [`Preempt`](ConnectionPolicy::Preempt) -- CredSSP/NLA gates every takeover | + /// | [`Tls`](RdpServerSecurity::Tls), [`None`](RdpServerSecurity::None) | [`Queue`](ConnectionPolicy::Queue) -- the pre-existing behaviour; an unauthenticated takeover is an explicit opt-in | + /// + /// Either can be overridden with + /// [`RdpServerBuilder::with_connection_policy`](crate::RdpServerBuilder::with_connection_policy). + #[must_use] + pub fn default_for(security: &RdpServerSecurity) -> Self { + if authenticates_before_eviction(security) { + Self::Preempt + } else { + Self::Queue + } + } +} + /// Tunnel payloads held while a Soft-Sync response is pending; beyond this the /// client is sending far more than the handful of messages the race allows. const MAX_EARLY_TUNNEL_PAYLOADS: usize = 64; @@ -410,7 +445,10 @@ pub struct RdpServerOptions { /// [`RdpServerBuilder::with_honor_client_desktop_size`](crate::RdpServerBuilder::with_honor_client_desktop_size). pub honor_client_desktop_size: Option, /// What to do with a second connection while a session is being served. - /// Defaults to [`ConnectionPolicy::Queue`]. Set via + /// Defaults to [`ConnectionPolicy::default_for`] the selected security + /// mode: [`Preempt`](ConnectionPolicy::Preempt) under + /// [`Hybrid`](RdpServerSecurity::Hybrid), [`Queue`](ConnectionPolicy::Queue) + /// otherwise. Set via /// [`RdpServerBuilder::with_connection_policy`](crate::RdpServerBuilder::with_connection_policy). pub connection_policy: ConnectionPolicy, /// Quantization values the RemoteFX encoder uses once selected. Defaults @@ -896,6 +934,11 @@ impl ErrorInfoDisconnectHandle { /// The disconnect takes effect only after the server handles this event. /// Unlike [`ServerEvent::Quit`], the client is told why: it decodes the /// PDU and can surface `error` to the user before the connection drops. + /// + /// The one exception is a client that did not set + /// `RNS_UD_CS_SUPPORT_ERRINFO_PDU` in its Client Core Data: MS-RDPBCGR + /// 3.3.5.7.1 forbids sending it the PDU, so it is disconnected without + /// the reason. #[expect( clippy::result_large_err, reason = "SendError hands the whole event back on a closed channel; ServerEvent's size is \ @@ -920,6 +963,13 @@ pub enum ServerEvent { /// client that replaced it, and the two ping-pong indefinitely. Telling /// the loser WHY it was disconnected is what makes it stay away. /// + /// The PDU is only sent to a client that set + /// `RNS_UD_CS_SUPPORT_ERRINFO_PDU` in its Client Core Data (MS-RDPBCGR + /// 3.3.5.7.1 forbids it otherwise); one that did not is dropped without + /// the reason, and the anti-storm cooldown on [`RdpServer::run`] is then + /// the only thing standing between it and the ping-pong above. (mstsc and + /// FreeRDP both set the flag.) + /// /// A more general version of the same PDU/mechanism exists as /// [`Self::Disconnect`] (upstream, `ErrorInfoDisconnectHandle`) for an /// embedder-chosen [`ErrorInfo`]; this variant stays separate because its @@ -2875,6 +2925,15 @@ impl RdpServer { Ok((RunState::Continue, encoder)) } + /// `client_supports_errinfo` is the client's `RNS_UD_CS_SUPPORT_ERRINFO_PDU` + /// early-capability opt-in: MS-RDPBCGR 3.3.5.7.1 forbids sending a Server + /// Set Error Info PDU to a client that did not set it, so the two arms below + /// that carry a disconnect reason drop the PDU (and just disconnect) when it + /// is `false`. + #[expect( + clippy::too_many_arguments, + reason = "private per-connection dispatcher; the parameters are the connection's negotiated identifiers and capabilities" + )] async fn dispatch_server_events( &mut self, events: &mut Vec, @@ -2882,6 +2941,7 @@ impl RdpServer { io_channel_id: u16, user_channel_id: u16, message_channel_id: Option, + client_supports_errinfo: bool, udp_transport: Option<&multitransport::UdpTransportHandle>, ) -> ServerResult { // Only referenced under `#[cfg(feature = "egfx")]` below (the only @@ -2919,14 +2979,14 @@ impl RdpServer { // against the preempting client — see the variant's docs). ServerEvent::EvictedByOtherConnection => { debug!("evicting this connection -- another client took the session over"); - // KNOWN GAP: MS-RDPBCGR 3.3.5.7.1 says the Set Error Info - // PDU MUST NOT be sent to a client that did not set - // RNS_UD_CS_SUPPORT_ERRINFO_PDU in its Client Core Data - // `earlyCapabilityFlags`, and this sends it unconditionally. - // `AcceptorResult` exposes no early-capability field today, - // so the check is not currently expressible here; the - // pre-existing `send_access_denied` has the identical gap. - // Closing it needs an ironrdp-acceptor API addition. + if !client_supports_errinfo { + // MS-RDPBCGR 3.3.5.7.1: the client did not set + // RNS_UD_CS_SUPPORT_ERRINFO_PDU, so it MUST NOT be + // sent a Set Error Info PDU. Such a client cannot be + // told why it is going away; it just goes. + debug!("client did not opt into Set Error Info PDUs; dropping it without the eviction reason"); + return Ok(RunState::Disconnect); + } let pdu = rdp::headers::ShareDataPdu::ServerSetErrorInfo(ServerSetErrorInfoPdu( ErrorInfo::ProtocolIndependentCode(ProtocolIndependentCode::DisconnectedByOtherconnection), )); @@ -2949,6 +3009,11 @@ impl RdpServer { } ServerEvent::Disconnect(error) => { debug!(?error, "Got disconnect event"); + if !client_supports_errinfo { + // Same MS-RDPBCGR 3.3.5.7.1 rule as the eviction arm. + debug!("client did not opt into Set Error Info PDUs; disconnecting without the reason"); + return Ok(RunState::Disconnect); + } let pdu = rdp::headers::ShareDataPdu::ServerSetErrorInfo(ServerSetErrorInfoPdu(error)); // pduSource=0, not user_channel_id -- same MS-RDPBCGR // 2.2.5.1.1 requirement as the EvictedByOtherConnection @@ -3755,6 +3820,7 @@ impl RdpServer { user_channel_id: u16, message_channel_id: Option, client_supports_heartbeat: bool, + client_supports_errinfo: bool, mut encoder: UpdateEncoder, udp_transport: Rc>>, pending_udp_accept: Option>>, @@ -3909,6 +3975,7 @@ impl RdpServer { io_channel_id, user_channel_id, message_channel_id, + client_supports_errinfo, current_udp_transport.as_ref(), ) .await?; @@ -4322,6 +4389,9 @@ impl RdpServer { result .client_early_capability_flags .contains(ironrdp_pdu::gcc::ClientEarlyCapabilityFlags::SUPPORT_HEART_BEAT_PDU), + result + .client_early_capability_flags + .contains(ironrdp_pdu::gcc::ClientEarlyCapabilityFlags::SUPPORT_ERR_INFO_PDU), encoder, udp_transport, pending_udp_accept, @@ -5887,7 +5957,7 @@ mod cliprdr_error_tests { let mut writer = CapturingWriter::default(); let state = server - .dispatch_server_events(&mut events, &mut writer, 1003, 1002, None, None) + .dispatch_server_events(&mut events, &mut writer, 1003, 1002, None, true, None) .await .expect("a refused clipboard message must not surface as a session error"); diff --git a/crates/ironrdp-testsuite-core/Cargo.toml b/crates/ironrdp-testsuite-core/Cargo.toml index ff7f305975..eb974a35a7 100644 --- a/crates/ironrdp-testsuite-core/Cargo.toml +++ b/crates/ironrdp-testsuite-core/Cargo.toml @@ -76,6 +76,7 @@ pretty_assertions = "1.4" proptest.workspace = true rstest.workspace = true tokio = { version = "1", features = ["macros", "rt", "io-util", "sync", "test-util"] } +tokio-rustls = "0.26" [lints] workspace = true diff --git a/crates/ironrdp-testsuite-core/tests/server/connection_policy.rs b/crates/ironrdp-testsuite-core/tests/server/connection_policy.rs index e21f193ee0..cc453ed12d 100644 --- a/crates/ironrdp-testsuite-core/tests/server/connection_policy.rs +++ b/crates/ironrdp-testsuite-core/tests/server/connection_policy.rs @@ -1,19 +1,22 @@ //! Coverage for [`ConnectionPolicy`] in `RdpServer::run`. //! -//! `run` serves one connection at a time. `Queue` (the default) leaves a -//! second connection unanswered in the listen backlog until the first ends; -//! `Reject` closes it at once so the client fails fast instead of appearing to -//! hang. +//! `run` serves one connection at a time. `Queue` (the default outside +//! `Hybrid` security) leaves a second connection unanswered in the listen +//! backlog until the first ends; `Reject` closes it at once so the client +//! fails fast instead of appearing to hang. use core::net::SocketAddr; use core::sync::atomic::{AtomicUsize, Ordering}; use core::time::Duration; use std::sync::Arc; -use ironrdp_server::{ConnectionHandler, ConnectionPolicy, RdpServer, ServerEvent}; +use ironrdp_server::{ConnectionHandler, ConnectionPolicy, RdpServer, RdpServerSecurity, ServerEvent}; use tokio::io::AsyncReadExt as _; use tokio::net::TcpStream; use tokio::sync::oneshot; +use tokio_rustls::TlsAcceptor; +use tokio_rustls::rustls::server::{ClientHello, ResolvesServerCert}; +use tokio_rustls::rustls::sign::CertifiedKey; async fn bound_addr(sender: &tokio::sync::mpsc::UnboundedSender) -> SocketAddr { // Poll until the accept loop has bound and can answer GetLocalAddr. @@ -71,11 +74,9 @@ async fn reject_closes_a_second_connection_during_a_session() { .await; } -/// With the default `Queue`, a second connection is left unanswered while the +/// Drive `server` and assert a second connection is left unanswered while the /// session runs: the read does not complete within the window. -#[tokio::test] -async fn queue_leaves_a_second_connection_waiting_during_a_session() { - let mut server = build(ConnectionPolicy::Queue); +async fn assert_second_connection_is_left_waiting(mut server: RdpServer) { let sender = server.event_sender().clone(); let local = tokio::task::LocalSet::new(); @@ -102,6 +103,67 @@ async fn queue_leaves_a_second_connection_waiting_during_a_session() { .await; } +/// With `Queue`, a second connection is left unanswered while the session +/// runs. +#[tokio::test] +async fn queue_leaves_a_second_connection_waiting_during_a_session() { + assert_second_connection_is_left_waiting(build(ConnectionPolicy::Queue)).await; +} + +/// Pins the out-of-the-box policy under `with_no_security` end to end: with no +/// `with_connection_policy` call, a second connection is left waiting (`Queue`), +/// NOT served in place of the live one. `None` authenticates nothing, so a +/// `Preempt` default here would let any peer that can reach the port evict the +/// live session. +#[tokio::test] +async fn the_default_under_no_security_leaves_a_second_connection_waiting() { + let server = RdpServer::builder() + .with_addr(([127, 0, 0, 1], 0)) + .with_no_security() + .with_no_input() + .with_no_display() + .build(); + assert_second_connection_is_left_waiting(server).await; +} + +/// A `TlsAcceptor` that can be constructed without any certificate material. +/// It would fail the first handshake, but nothing here handshakes: the tests +/// only need a value of each `RdpServerSecurity` variant. +#[derive(Debug)] +struct NoCert; + +impl ResolvesServerCert for NoCert { + fn resolve(&self, _client_hello: ClientHello<'_>) -> Option> { + None + } +} + +fn tls_acceptor() -> TlsAcceptor { + let config = tokio_rustls::rustls::ServerConfig::builder() + .with_no_client_auth() + .with_cert_resolver(Arc::new(NoCert)); + TlsAcceptor::from(Arc::new(config)) +} + +/// The default policy follows the security mode: takeover out of the box only +/// where the newcomer is authenticated before it could evict anything +/// (`Hybrid`); the pre-existing queue-behind everywhere else. +#[test] +fn the_default_policy_follows_the_security_mode() { + assert_eq!( + ConnectionPolicy::default_for(&RdpServerSecurity::None), + ConnectionPolicy::Queue + ); + assert_eq!( + ConnectionPolicy::default_for(&RdpServerSecurity::Tls(tls_acceptor())), + ConnectionPolicy::Queue + ); + assert_eq!( + ConnectionPolicy::default_for(&RdpServerSecurity::Hybrid((tls_acceptor(), Vec::new()))), + ConnectionPolicy::Preempt + ); +} + struct CountingHandler { accepted: Arc, } From 1050ffd77ad80b5232db446f64d1d166dcfd6481 Mon Sep 17 00:00:00 2001 From: Anton Mostovoy Date: Wed, 30 Sep 2026 21:17:18 -0500 Subject: [PATCH 2/2] fix(server): gate access-denied Set Error Info PDU on SUPPORT_ERR_INFO_PDU; pin Hybrid default - send_access_denied (auto-reconnect cookie and credential-validation rejections) now skips the Set Error Info PDU for a client that did not set SUPPORT_ERR_INFO_PDU, per MS-RDPBCGR 3.3.5.7.1, completing the gate already applied to the eviction and ErrorInfoDisconnectHandle arms. - Pin the default policy on the with_display_handler initializer too, so the two builder copies cannot drift apart. - Add an end-to-end Hybrid test: with no with_connection_policy call, an authenticated newcomer takes the session over and the incumbent is closed (fails under Queue). - Field and builder docs point at ConnectionPolicy::default_for instead of restating the per-mode mapping. Co-Authored-By: Claude Opus 5.5 --- crates/ironrdp-server/src/builder.rs | 5 +- crates/ironrdp-server/src/server.rs | 25 +++++-- .../tests/server/connection_policy.rs | 40 ++++++++++- crates/ironrdp-testsuite-extra/tests/e2e.rs | 71 +++++++++++++++++++ 4 files changed, 131 insertions(+), 10 deletions(-) diff --git a/crates/ironrdp-server/src/builder.rs b/crates/ironrdp-server/src/builder.rs index 3d3ca5d7d9..5152f296d9 100644 --- a/crates/ironrdp-server/src/builder.rs +++ b/crates/ironrdp-server/src/builder.rs @@ -360,9 +360,8 @@ impl RdpServerBuilder { /// ([`ConnectionPolicy::Reject`]), or let a fully-authenticated newcomer /// take the session over ([`ConnectionPolicy::Preempt`]). /// - /// The default follows the security mode already chosen on this builder -- - /// `Preempt` under [`RdpServerSecurity::Hybrid`], `Queue` otherwise; see - /// [`ConnectionPolicy::default_for`] for why. + /// The default follows the security mode already chosen on this builder; + /// see [`ConnectionPolicy::default_for`]. /// /// `Preempt`'s takeover is only authentication-gated under /// [`RdpServerSecurity::Hybrid`]; see [`ConnectionPolicy::Preempt`] for the diff --git a/crates/ironrdp-server/src/server.rs b/crates/ironrdp-server/src/server.rs index 97192fd10a..4066beebcc 100644 --- a/crates/ironrdp-server/src/server.rs +++ b/crates/ironrdp-server/src/server.rs @@ -446,9 +446,7 @@ pub struct RdpServerOptions { pub honor_client_desktop_size: Option, /// What to do with a second connection while a session is being served. /// Defaults to [`ConnectionPolicy::default_for`] the selected security - /// mode: [`Preempt`](ConnectionPolicy::Preempt) under - /// [`Hybrid`](RdpServerSecurity::Hybrid), [`Queue`](ConnectionPolicy::Queue) - /// otherwise. Set via + /// mode. Set via /// [`RdpServerBuilder::with_connection_policy`](crate::RdpServerBuilder::with_connection_policy). pub connection_policy: ConnectionPolicy, /// Quantization values the RemoteFX encoder uses once selected. Defaults @@ -4159,10 +4157,16 @@ impl RdpServer { { debug!("Client accepted"); + // MS-RDPBCGR 3.3.5.7.1: a Set Error Info PDU MUST NOT be sent to a client that did not + // set `SUPPORT_ERR_INFO_PDU`; such a client is just disconnected. + let supports_err_info = result + .client_early_capability_flags + .contains(ironrdp_pdu::gcc::ClientEarlyCapabilityFlags::SUPPORT_ERR_INFO_PDU); + let is_auto_reconnect = if let Some(reconnect) = result.auto_reconnect.as_ref() { if !self.verify_auto_reconnect_cookie(reconnect) { warn!("Auto-reconnect cookie validation rejected"); - send_access_denied(result.io_channel_id, result.user_channel_id, writer).await?; + send_access_denied(result.io_channel_id, result.user_channel_id, supports_err_info, writer).await?; return Err(ServerError::reason("auto-reconnect validation", "cookie rejected")); } @@ -4184,12 +4188,14 @@ impl RdpServer { } Ok(CredentialDecision::Reject) => { warn!("Credential validation rejected"); - send_access_denied(result.io_channel_id, result.user_channel_id, writer).await?; + send_access_denied(result.io_channel_id, result.user_channel_id, supports_err_info, writer) + .await?; return Err(ServerError::reason("credential validation", "rejected by validator")); } Err(e) => { error!(error = %e, "Credential validator backend error"); - send_access_denied(result.io_channel_id, result.user_channel_id, writer).await?; + send_access_denied(result.io_channel_id, result.user_channel_id, supports_err_info, writer) + .await?; return Err(ServerError::custom("credential validation", e)); } } @@ -5008,11 +5014,18 @@ fn with_connection_handler( /// /// Used to deny a connection after credential validation rejects it, mirroring the /// acceptor's exact-match denial so both paths refuse the same spec-defined way. +/// +/// Sends nothing when the client did not opt in via `SUPPORT_ERR_INFO_PDU` +/// (MS-RDPBCGR 3.3.5.7.1); the caller still closes the connection. async fn send_access_denied( io_channel_id: u16, user_channel_id: u16, + supports_err_info: bool, writer: &mut impl FramedWrite, ) -> ServerResult<()> { + if !supports_err_info { + return Ok(()); + } let info = ServerSetErrorInfoPdu(ErrorInfo::ProtocolIndependentCode( ProtocolIndependentCode::ServerDeniedConnection, )); diff --git a/crates/ironrdp-testsuite-core/tests/server/connection_policy.rs b/crates/ironrdp-testsuite-core/tests/server/connection_policy.rs index cc453ed12d..32c1ebbbc6 100644 --- a/crates/ironrdp-testsuite-core/tests/server/connection_policy.rs +++ b/crates/ironrdp-testsuite-core/tests/server/connection_policy.rs @@ -10,7 +10,10 @@ use core::sync::atomic::{AtomicUsize, Ordering}; use core::time::Duration; use std::sync::Arc; -use ironrdp_server::{ConnectionHandler, ConnectionPolicy, RdpServer, RdpServerSecurity, ServerEvent}; +use ironrdp_server::{ + ConnectionHandler, ConnectionPolicy, DesktopSize, DisplayUpdate, RdpServer, RdpServerDisplay, + RdpServerDisplayUpdates, RdpServerSecurity, ServerEvent, ServerResult, +}; use tokio::io::AsyncReadExt as _; use tokio::net::TcpStream; use tokio::sync::oneshot; @@ -126,6 +129,41 @@ async fn the_default_under_no_security_leaves_a_second_connection_waiting() { assert_second_connection_is_left_waiting(server).await; } +struct StubUpdates; + +#[async_trait::async_trait] +impl RdpServerDisplayUpdates for StubUpdates { + async fn next_update(&mut self) -> ServerResult> { + core::future::pending().await + } +} + +struct StubDisplay; + +#[async_trait::async_trait] +impl RdpServerDisplay for StubDisplay { + async fn size(&mut self) -> DesktopSize { + DesktopSize { width: 64, height: 64 } + } + + async fn updates(&mut self) -> ServerResult> { + Ok(Box::new(StubUpdates)) + } +} + +/// The `with_display_handler` initializer resolves the default separately from +/// `with_no_display`; pin it too, so the two copies cannot drift apart. +#[tokio::test] +async fn the_default_under_no_security_with_a_display_handler_leaves_a_second_connection_waiting() { + let server = RdpServer::builder() + .with_addr(([127, 0, 0, 1], 0)) + .with_no_security() + .with_no_input() + .with_display_handler(StubDisplay) + .build(); + assert_second_connection_is_left_waiting(server).await; +} + /// A `TlsAcceptor` that can be constructed without any certificate material. /// It would fail the first handshake, but nothing here handshakes: the tests /// only need a value of each `RdpServerSecurity` variant. diff --git a/crates/ironrdp-testsuite-extra/tests/e2e.rs b/crates/ironrdp-testsuite-extra/tests/e2e.rs index 679b1ba8fb..e9b1c435de 100644 --- a/crates/ironrdp-testsuite-extra/tests/e2e.rs +++ b/crates/ironrdp-testsuite-extra/tests/e2e.rs @@ -1792,3 +1792,74 @@ async fn connection_handler_sees_every_hook_across_a_preemption() { })) .await; } + +/// Under `Hybrid` the out-of-the-box policy is `Preempt`: with no +/// `with_connection_policy` call, a second client that completes CredSSP takes +/// the session over, and the first client's connection is closed. +#[tokio::test] +async fn the_default_under_hybrid_lets_an_authenticated_newcomer_take_over() { + const HYBRID_USER: &str = "user"; + const HYBRID_PASSWORD: &str = "password"; + + let cert_path = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/certs/server-cert.pem"); + let key_path = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/certs/server-key.pem"); + let identity = TlsIdentityCtx::init_from_paths(&cert_path, &key_path).expect("failed to init TLS identity"); + let acceptor = identity.make_acceptor().expect("failed to build TLS acceptor"); + + // Keep the display sender alive: once it drops, the server closes the session. + let (_display_tx, display_rx) = mpsc::unbounded_channel(); + let mut server = RdpServer::builder() + .with_addr(([127, 0, 0, 1], 0)) + .with_hybrid(acceptor, identity.pub_key.clone()) + .with_input_handler(TestInputHandler) + .with_display_handler(TestDisplay { + rx: Arc::new(Mutex::new(display_rx)), + }) + .build(); + server.set_credentials(Some(server::Credentials { + username: HYBRID_USER.into(), + password: HYBRID_PASSWORD.into(), + domain: None, + })); + let ev = server.event_sender().clone(); + + let client_config = || connector::Config { + credentials: connector::Credentials::UsernamePassword { + username: HYBRID_USER.into(), + password: HYBRID_PASSWORD.into(), + }, + ..default_client_config() + }; + + let local = tokio::task::LocalSet::new(); + Box::pin(local.run_until(async move { + let server_task = tokio::task::spawn_local(async move { + let _ = server.run().await; + }); + let server_addr = local_addr_of(&ev).await; + let connect = || { + connect_client(server_addr, |client_addr| { + connector::ClientConnector::new(client_config(), client_addr) + }) + }; + + let (mut first, _) = connect().await; + + // The newcomer can only finish its handshake if the server serves it, + // which under `Queue` it would not do while `first` is live. + let (second, _) = tokio::time::timeout(Duration::from_secs(10), connect()) + .await + .expect("the authenticated newcomer must be served (Preempt), not queued"); + + // The incumbent is evicted: its transport ends rather than staying open. + let evicted = tokio::time::timeout(Duration::from_secs(5), async { + while first.read_pdu().await.is_ok() {} + }) + .await; + assert!(evicted.is_ok(), "the incumbent's connection must be closed"); + + drop(second); + server_task.abort(); + })) + .await; +}