feat(server): default ConnectionPolicy to Preempt under Hybrid

crates/ironrdp-server/src/builder.rs

@@ -151,6 +151,7 @@ impl RdpServerBuilder<WantsDisplay> {
     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<WantsDisplay> {
                 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<WantsDisplay> {
     }
 
     pub fn with_no_display(self) -> RdpServerBuilder<BuilderDone> {
+        let connection_policy = ConnectionPolicy::default_for(&self.state.security);
         RdpServerBuilder {
             state: BuilderDone {
                 addr: self.state.addr,
@@ -213,7 +215,7 @@ impl RdpServerBuilder<WantsDisplay> {
                 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,13 @@ impl RdpServerBuilder<BuilderDone> {
 
     /// 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;
+    /// see [`ConnectionPolicy::default_for`].
+    ///
     /// `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

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,8 @@ pub struct RdpServerOptions {
     /// [`RdpServerBuilder::with_honor_client_desktop_size`](crate::RdpServerBuilder::with_honor_client_desktop_size).
     pub honor_client_desktop_size: Option<DesktopSize>,
     /// 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. Set via
     /// [`RdpServerBuilder::with_connection_policy`](crate::RdpServerBuilder::with_connection_policy).
     pub connection_policy: ConnectionPolicy,
     /// Quantization values the RemoteFX encoder uses once selected. Defaults
@@ -858,6 +894,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<ServerEvent> hands the whole event back on a closed channel; ServerEvent's size is \
@@ -882,6 +923,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
@@ -1164,6 +1212,10 @@ struct ConnectionState {
     /// Whether the client advertised `SUPPORT_HEART_BEAT_PDU` in its GCC
     /// Client Core Data.
     client_supports_heartbeat: bool,
+    /// Whether the client advertised `SUPPORT_ERR_INFO_PDU` in its GCC Client
+    /// Core Data. MS-RDPBCGR 3.3.5.7.1 forbids sending it a Server Set Error
+    /// Info PDU unless it did.
+    client_supports_errinfo: bool,
     /// Auto-detect state, present when the server has auto-detect enabled.
     ///
     /// Probes in flight, RTT samples and the session-lifetime lowest RTT all
@@ -2884,6 +2936,11 @@ impl RdpServer {
         Ok((RunState::Continue, encoder))
     }
 
+    /// `conn.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 dispatch; the parameters are the connection's negotiated identifiers and transports"
@@ -2933,14 +2990,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 !conn.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),
                     ));
@@ -2963,6 +3020,11 @@ impl RdpServer {
                 }
                 ServerEvent::Disconnect(error) => {
                     debug!(?error, "Got disconnect event");
+                    if !conn.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
@@ -4127,10 +4189,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"));
             }
 
@@ -4152,12 +4220,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));
                     }
                 }
@@ -4196,6 +4266,9 @@ impl RdpServer {
         conn.client_supports_heartbeat = result
             .client_early_capability_flags
             .contains(ironrdp_pdu::gcc::ClientEarlyCapabilityFlags::SUPPORT_HEART_BEAT_PDU);
+        conn.client_supports_errinfo = result
+            .client_early_capability_flags
+            .contains(ironrdp_pdu::gcc::ClientEarlyCapabilityFlags::SUPPORT_ERR_INFO_PDU);
         if !result.reactivation {
             for (_channel_key, channel, channel_id) in conn.static_channels.iter_by_key_mut() {
                 debug!(?channel, ?channel_id, "Start");
@@ -4982,11 +5055,18 @@ fn with_connection_handler<R>(
 ///
 /// 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,
     ));
@@ -5857,7 +5937,10 @@ mod cliprdr_error_tests {
         // Left in its initial state, so `require_ready` refuses the request
         // below -- the cheapest reproduction of "the channel said no".
         let cliprdr: CliprdrServer = Cliprdr::new(Box::new(SilentBackend));
-        let mut conn = ConnectionState::default();
+        let mut conn = ConnectionState {
+            client_supports_errinfo: true,
+            ..ConnectionState::default()
+        };
         conn.static_channels.insert(cliprdr);
         conn.static_channels
             .attach_channel_id(TypeId::of::<CliprdrServer>(), 1004);

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