feat(server): default ConnectionPolicy to Preempt under Hybrid

[Anton Mostovoy (antonmos)]

Follow-up to the review discussion on #1476 (discussion_r3962619360), where we agreed takeover should ultimately be the default because it is the least surprising for the single-session servers ironrdp-server typically backs.

Rebuilt on top of #1913 (which replaced the preempt_existing_session bool with ConnectionPolicy { Queue, Reject, Preempt }), and narrowed per the review: the default is mode-aware, not a blanket Preempt.

What changes

• ConnectionPolicy::default_for(&RdpServerSecurity) — Preempt under Hybrid, Queue under Tls and None. Both RdpServerBuilder initializers call it with the security already chosen on the builder. The derived Default (a mode-independent Queue) is removed so there is one source of truth; docs on the enum, the field and with_connection_policy point at default_for, which carries the per-mode table and the rationale.
• Set Error Info PDU is now gated on the client's SUPPORT_ERR_INFO_PDU opt-in (MS-RDPBCGR 3.3.5.7.1). The eviction arm's KNOWN-GAP comment claimed AcceptorResult exposes no early-capability field; it does (client_early_capability_flags, already used in the same file to gate heartbeats), so the check needs no API change. The flag is threaded through client_loop into dispatch_server_events exactly 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. Docs on both note the exception.
• Tests in ironrdp-testsuite-core/tests/server/connection_policy.rs, where 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). Tls/Hybrid values come from a certificate-less TlsAcceptor (stub ResolvesServerCert), hence the tokio-rustls dev-dependency.

Why mode-aware

Under Tls/None the per-mode table says any peer that can complete the handshake clears the bar, and the anti-storm cooldown bars the victim rather than the attacker — so a Preempt default there would make an authenticated session remotely evictable out of the box. Only Hybrid gates a takeover on client authentication. Keeping Queue there preserves the pre-existing behaviour and makes an unauthenticated takeover an explicit choice (the startup warning for that choice is unchanged).

Not a released-behaviour break

ConnectionPolicy (#1913) and the option it replaced (#1476) both landed after the last ironrdp-server release (0.13.0, 2026-07-10), so no released consumer depends on either the Queue default or the derived Default impl.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

The unconditional default permits unauthenticated session eviction under Tls and None.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Changes the server’s session-preemption default from opt-in to enabled.

Changes:

• Enables preemption in both builder paths.
• Updates API documentation for the new default.
• Documents how to restore queue-behind behavior.

File summaries

File	Description
crates/ironrdp-server/src/server.rs	Documents the new default and opt-out behavior.
crates/ironrdp-server/src/builder.rs	Enables preemption by default in both builder paths.

Review details

Suppressed comments (1)

crates/ironrdp-server/src/builder.rs:204

• This second initializer also enables unauthenticated session eviction by default for Tls and None. It needs the same safe default policy as the display-handler path; otherwise choosing with_no_display() silently opts the server into the documented takeover DoS exposure.

                preempt_existing_session: true,

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[github-actions]

PR flips preempt_existing_session's default to true in both RdpServerBuilder constructors and rewrites the paired rustdoc; no runtime logic changes. Verified: docs match the new default and no stale "off by default" text remains. Substantive survivors: (1) the now-default takeover path sends the MS-RDPBCGR Set Error Info PDU without checking the client's SUPPORT_ERRINFO_PDU opt-in, and the in-code "no acceptor field" excuse is false — AcceptorResult.client_early_capability_flags exists and is already used in this file for heartbeat gating, so the check needs no API change; (2) the new default makes unauthenticated eviction of a live session the out-of-the-box posture under Tls/None, though a mode-aware default was feasible. Minor: no test pins the new default, and the opt-out instruction is duplicated within one doc comment. All four specialist candidates verified and accepted.

1. [skeptical] No test pins the new preempt_existing_session default — low 🟡 — crates/ironrdp-server/src/builder.rs
   The entire functional change is the literal true in both BuilderDone constructors (builder.rs:171 with_display_handler, :204 with_no_display). Every existing test hardcodes the value instead of exercising the default — with_preempt_existing_session(true) at server.rs:4375 and :4472, true in a test options struct at server.rs:4166 — so a regression back to false in either constructor passes the suite silently. Tests already access private internals via use super::*, so asserting opts.preempt_existing_session is true on both builder paths is trivial and proportionate for the one line this PR exists to change.

[Greg Lamberson (glamberson)]

Thanks for following through on this, Anton. It's a good change, and it's the right call to ask about the security tradeoff rather than leave it implicit.

One heads-up on the shape: #1913 landed on 2026-09-11 and replaced the preempt_existing_session bool with ConnectionPolicy { Queue, Reject, Preempt }, so that field no longer exists on master and this branch can't be rebased as-is. The equivalent change now is moving #[default] from Queue to Preempt on the enum and updating the docs on the enum and on the connection_policy field. It's a smaller diff than the original.

On the default itself, the part I'd want settled first is the one you flagged. Under Tls and None, the per-mode table says any peer that can reach the port clears the bar, and the anti-storm cooldown bars the victim rather than the attacker, so a Preempt default means an unauthenticated peer can repeatedly evict a live session out of the box. Only Hybrid gates it on authentication. Would it make sense for the default to be Preempt only where the client is authenticated (Hybrid) and stay Queue otherwise? That keeps the least-surprising behavior for the case where it's safe, and makes the unauthenticated case an explicit choice.

This comes from a production angle: Our lamco-rdp-server runs its own accept loop over run_connection, so today a second client hangs behind the live session, and we work around it by draining stale queued connections after each session. We'd like the fail-fast and takeover behavior available to us, and the design here is what makes that possible.

One small related thing I noticed while reading the eviction arm in server.rs: The KNOWN GAP comment says AcceptorResult exposes no early-capability field, but it does. client_early_capability_flags (ironrdp-acceptor/src/connection.rs:123) is already used a bit further down in the same file, so gating the Set Error Info PDU on SUPPORT_ERRINFO_PDU doesn't need an API change. It's independent of the default, but a Preempt default makes that path much more likely to be hit.

[Anton Mostovoy (antonmos)]

Thanks — both points taken, and the branch is now rebuilt on #1913 (merged master in; 8f0d65e has the substance).

Mode-aware default: yes, adopted. ConnectionPolicy::default_for(&RdpServerSecurity) returns Preempt under Hybrid and Queue under Tls/None, and both builder initializers call it with the security already chosen on the builder. I removed the derived Default rather than leaving a mode-independent Queue next to it — with two "defaults" the enum-level one would inevitably drift from what the builder actually does. The per-mode table and rationale live once, on default_for; the enum, the field and with_connection_policy link there. Preempt under Tls/None stays an explicit choice with the same startup warning.

The KNOWN-GAP comment: fixed, and you're right it was wrong. client_early_capability_flags is threaded through client_loop into dispatch_server_events the same way the heartbeat flag already is, and both arms that send a Set Error Info PDU — the eviction arm and the public ErrorInfoDisconnectHandle path — now skip the PDU (and just disconnect) for a client that didn't set SUPPORT_ERR_INFO_PDU, per MS-RDPBCGR 3.3.5.7.1. I bundled it here rather than splitting it out because, as you said, a Preempt default is what makes that path hot.

Tests went into ironrdp-testsuite-core/tests/server/connection_policy.rs next to yours from #1913, since ironrdp-server has [lib] test = false and its in-crate tests don't run under cargo test --workspace: default_for is pinned for all three modes, and the with_no_security default is driven end to end through run (second connection left waiting). The Tls/Hybrid values come from a certificate-less TlsAcceptor with a stub ResolvesServerCert, which is why tokio-rustls shows up as a testsuite dev-dependency.

Re lamco-rdp-server driving its own accept loop over run_connection: with #1913 + this, run now exposes all three behaviours, so hopefully the drain-stale-connections workaround can go.

🤖 Addressed by Claude Code

[Greg Lamberson (glamberson)]

Thanks for the quick turnaround, Anton. Rebuilding on #1913 and making the default mode-aware is a clean result. default_for gives a single source of truth, and the per-mode table on it is the kind of rationale that will still make sense to someone reading the code in a year. Gating the Set Error Info PDU on SUPPORT_ERR_INFO_PDU was a nice extra, and it fits well with this change.

Two small things, neither of them blocking from my side:

• ConnectionPolicy::Preempt drops ConnectionHandler hooks fired during the served connection (on_connection_info never fires) #1969: clintcan's report says on_connection_info never fires under Preempt. With this change, every Hybrid server would get Preempt by default, so that bug would apply out of the box instead of only to embedders who opt in. It might be worth landing the fix for ConnectionPolicy::Preempt drops ConnectionHandler hooks fired during the served connection (on_connection_info never fires) #1969 first, or at least keeping the two in step. I'd defer to the maintainers on the ordering.
• Builder test: the default is pinned through with_no_display, and default_for is unit-tested for all three modes. A small assertion on the with_display_handler path (builder.rs:148) would close the last gap, since the two initializers are separate copies.

Good change overall. It makes the safe case the easy one and the unauthenticated case an explicit choice.

[Greg Lamberson (glamberson)]

Thanks again, Anton, and for the note about lamco-rdp-server. I looked at moving it onto run now that #1913 and this change are in, and it doesn't fit, because the server doesn't accept on a single TCP listener. It runs its own accept loop over TCP, Unix sockets, AF_VSOCK (Hyper-V Enhanced Session Mode) and a WebSocket listener that speaks RDCleanPath and terminates TLS itself, including listeners handed over by systemd socket activation. Each accepted stream goes to run_connection, or for the WebSocket case run_connection_with(stream, TransportTls::AlreadyDone). None of that reaches ConnectionPolicy: The race, the eviction, the re-preempt cooldown and the auto-reconnect cookie invalidation all live inside run's loop over the socket it binds itself.

What would make it usable there is the same race driven by connections the embedder accepts: run's policy logic taking a source of incoming connections, each with its stream, its peer and its TransportTls, with run becoming the TCP case of it. That keeps the security rules you and maryny4 worked out in one place instead of each embedder re-deriving them from private pieces. Two things it would have to settle: The candidate type is fixed to NegotiatedConnection<TcpStream> today, and the peer passed to on_accept and used for the cooldown is a SocketAddr, which a Unix or vsock connection doesn't have. It would also build on the #1969 fix, since a generalized race would take the same path Preempt takes now.

I'd do this as a separate PR after this one lands rather than widen this change. Does that direction match how you see ConnectionPolicy evolving?

[Anton Mostovoy (antonmos)]

Benoît Cortier (@CBenoit) the above makes sense to me. Could you confirm if you agree with this direction ?

[github-actions]

The PR delivers what it claims: a mode-aware ConnectionPolicy::default_for (Preempt under Hybrid, Queue under Tls/None) reused from the pre-existing authenticates_before_eviction predicate, applied in both builder initializers with the mode-independent derived Default removed, plus a correct MS-RDPBCGR 3.3.5.7.1 gate threading client_early_capability_flags through client_loop into dispatch_server_events for the eviction and Disconnect arms, with tests pinned in the CI-visible testsuite. However, the 3.3.5.7.1 fix is incomplete: send_access_denied still sends the Set Error Info PDU unconditionally on the auto-reconnect-cookie and credential-validation rejection paths in the same file, where the flag is already in scope, and the PR deleted the KNOWN-GAP comment that documented exactly this residual violation. Secondary: the default is computed as a copy in both initializers with only one pinned end to end, and the per-mode default mapping is restated in several doc sites despite default…

1. [protocol + skeptical] Set Error Info PDU gate is incomplete: send_access_denied still sends it unconditionally, and the comment recording the gap was deleted — high 🔴 — crates/ironrdp-server/src/server.rs
   MS-RDPBCGR 3.3.5.7.1 forbids sending the Server Set Error Info PDU to a client that did not set RNS_UD_CS_SUPPORT_ERRINFO_PDU (0x0001) in its Client Core Data earlyCapabilityFlags. The PR correctly gates the EvictedByOtherConnection and Disconnect dispatch arms on the flag threaded from AcceptorResult::client_early_capability_flags, but send_access_denied (server.rs:4154) still encodes and writes ServerSetErrorInfoPdu(ServerDeniedConnection) with no check, and it is called on three reachable paths in client_accepted: auto-reconnect cookie rejection (3496) and credential-validation rejection or backend error (3518/3523). A client that did not opt in and fails validation still receives a forbidden PDU, so the conformance defect persists; result.client_early_capability_flags is already in scope there, so the fix needs no new plumbing. Compounding it, the PR deleted the KNOWN-GAP comment that explicitly documented this exact residual gap, removing the in-code record that would otherwise surface it in review. Line fields are null because the defect lives in lines this PR did not add.

[Anton Mostovoy (antonmos)]

Pushed 9eec38b, addressing the latest round:

• send_access_denied gate (automated review, high): right — the rejection paths still sent the Set Error Info PDU unconditionally. send_access_denied now takes the client's SUPPORT_ERR_INFO_PDU opt-in and sends nothing for a client that didn't set it (the caller still closes the connection), covering auto-reconnect cookie rejection and both credential-validation rejection paths. With the eviction arm and ErrorInfoDisconnectHandle already gated, every path that sends the PDU now honours MS-RDPBCGR 3.3.5.7.1.
• Hybrid Preempt test (Benoît Cortier (@CBenoit)): the_default_under_hybrid_lets_an_authenticated_newcomer_take_over in ironrdp-testsuite-extra/tests/e2e.rs builds a with_hybrid server with no with_connection_policy call, drives two real clients through CredSSP, and asserts the second is served while the first's connection is closed. Verified it fails when Queue is forced.
• with_display_handler default (Greg Lamberson (@glamberson)): now pinned end to end as well; see the inline reply.
• ConnectionPolicy::Preempt drops ConnectionHandler hooks fired during the served connection (on_connection_info never fires) #1969: fix is up as fix(server): keep ConnectionHandler reachable during a Preempt race #2065 — the handler is shared as Rc<RefCell<…>> instead of taken for the race, with clintcan's three regression tests from the issue (the Preempt one fails without the fix). Per Benoît Cortier (@CBenoit)'s condition, this PR should land after it.

🤖 Addressed by Claude Code

[Benoît Cortier (CBenoit)]

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.

[Anton Mostovoy (antonmos)]

Benoît Cortier (@CBenoit) rebased onto master (now includes #2087/#2088) — two commits, no merge commits; the Hybrid takeover test from your earlier request is in ironrdp-testsuite-extra/tests/e2e.rs.

🤖 Addressed by Claude Code