Repository navigation
feat(server): add ConnectionPolicy to reject a second connection - #1913
Benoît Cortier (CBenoit) merged 1 commit into
Conversation
99250f8 to
f0a6387
Compare
|
The overlap with #1476 is real, and I think the two are complementary rather than competing: #1476 is the Preempt case (authenticated takeover), this is the Reject case (fail fast, no takeover) — some deployments want one, some the other. If it's useful, the |
f0a6387 to
cc1dece
Compare
|
I have the same production experience with this that I mentioned on #1483: lamco-rdp-server and lamco-qemu-rdp both sit on On the shape question you raised: A single I will leave which PR carries the merged shape to you and antonmos and whoever reviews it. The architecture question is the one worth settling first. |
|
Anton Mostovoy (@antonmos) — #1476 is clearly the deeper piece here: the auth-gated preemption, the eviction notice, the anti-storm window are all the parts I deliberately left out of this, and it's much closer to landing. I don't want to get in its way. The one thing this PR adds that yours doesn't is the shape — a The cleanest outcome I can see: #1476 keeps all its machinery and swaps |
i am fine with either way. Benoît Cortier (@CBenoit) could you indicate if you would like go with enum approach? |
|
Thank you! I merged #1476, but I would gladly accept the API improvements from this PR. Marynych Oleksandr (@maryny4) could you rebase the PR accordingly? |
cc1dece to
8b270f0
Compare
|
Rebased onto master and converted the API to the enum as suggested: |
8b270f0 to
f1691c1
Compare
f1691c1 to
9c2fe54
Compare
There was a problem hiding this comment.
Replaces `preempt_existing_session: bool` with a `ConnectionPolicy` enum (`Queue`/`Reject`/`Preempt`) in `ironrdp-server`, the shape the maintainer requested in-thread after #1476. The conversion preserves behavior: `Queue` (default) is the pre-existing else-branch verbatim, the `Preempt` arm is #1476's race re-indented with no logic change, and no stale references to the old setter remain; new tests cover both newly visible policies over a real listener. The only new logic is the `Reject` arm, whose main path is sound (biased select, justified unreachable Negotiated entry, events draining via the session's dispatch). Remaining issues are low and concentrated there: accept errors are silently swallowed, the drop path bypasses `ConnectionHandler::on_accept` undocumented, and `Quit` is deferred until the session ends. The single-use `preempt_enabled` binding can be inlined; the extraction and redundant-drop candidates are rejected per repo style.
| accepted = listener.accept() => { | ||
| if let Ok((extra, extra_peer)) = accepted { | ||
| debug!(?extra_peer, "Session active; rejecting connection"); | ||
| drop(extra); | ||
| } | ||
| } |
There was a problem hiding this comment.
[skeptical] Reject arm's accept-error path is silent, unlike every other accept path in run — low 🟡 — The new inner loop's accept arm handles only Ok: on Err it does nothing and the loop re-polls. The Preempt arm logs warn!(?error) for the same 'accept failed while a session was live' condition (line 2442), and the outer accept loop's `else => break` ends run() on accept failure when idle. Under Reject, a persistently failing listener during a live session produces an immediate-Err select arm with no await — a hot spin for the rest of the session with zero diagnostics. If transient errors (e.g. ECONNABORTED) are the reason to swallow, that is undocumented; a warn! matching the Preempt arm closes the asymmetry at one line. Low: needs a persistently broken listener plus an active session.
There was a problem hiding this comment.
Fair — the Reject arm now warns like the Preempt arm does (accept failed while a session was live). The immediate re-poll on a persistently failing listener is the same as Preempt's; I kept the two in step rather than invent a backoff for one of them here.
| ConnectionPolicy::Reject => { | ||
| // Serve while still accepting, but close any newcomer | ||
| // instead of racing it (Preempt) or queuing it (Queue): the | ||
| // extra connection fails fast rather than hanging in the | ||
| // backlog (#8). The session arm is polled first (biased), | ||
| // so a client reconnecting the instant a session ends is | ||
| // taken by the outer loop, not rejected here. | ||
| let result = match entry { | ||
| Entry::Fresh(stream, _) => { | ||
| let mut conn = core::pin::pin!(self.run_connection_inner(stream, TransportTls::Managed)); | ||
| loop { | ||
| tokio::select! { | ||
| biased; | ||
| res = &mut conn => break res, | ||
| accepted = listener.accept() => { | ||
| if let Ok((extra, extra_peer)) = accepted { | ||
| debug!(?extra_peer, "Session active; rejecting connection"); | ||
| drop(extra); | ||
| } |
There was a problem hiding this comment.
[skeptical] ConnectionPolicy::Reject drops extra connections without consulting ConnectionHandler::on_accept, undocumented — low 🟡 — Reject introduces the first accept()-then-drop path in run() that never invokes ConnectionHandler::on_accept, whose contract is 'after accept() returns but before run_connection()' (server.rs:164); the Preempt arm deliberately gates every candidate through it before anything else (line 2421). Concrete operational impact: an embedder's allowlist/rate-limiter/audit hook never sees connections the policy discards — a rate limiter's window is never ticked, an audit log never records the attempt, and a handler holding resources across a Queue->Reject transition is never asked to release them. No regression versus the Queue default (the connection is never accepted there), which keeps this low; but the new docs on ConnectionPolicy::Reject and with_connection_policy are silent on the bypass. Fix: document it and call on_accept on the drop path.
There was a problem hiding this comment.
Deliberately not called, and now documented on ConnectionPolicy::Reject and with_connection_policy. on_accept's contract is a verdict: false drops the stream, true means the peer goes on to be served — which a rejected newcomer never does. Calling it and dropping the stream anyway would show a handler an accepted peer that never disconnects: a rate limiter burns a slot, an audit log records a session that never existed. Under Queue the handler does not see a backlogged connection either; Reject only closes it instead of leaving it waiting. A test now asserts the handler is not consulted for a connection Reject closes.
| let mut pending: Option<(Box<NegotiatedCandidate>, SocketAddr)> = None; | ||
|
|
||
| let preempt_enabled = self.opts.preempt_existing_session; | ||
| let preempt_enabled = self.opts.connection_policy == ConnectionPolicy::Preempt; |
There was a problem hiding this comment.
[code-compressor] Single-use `preempt_enabled` binding can be inlined into the startup warning — low 🟡 — The binding replaced `let preempt_enabled = self.opts.preempt_existing_session;` with a comparison, but it still has exactly one consumer: the warning's `if preempt_enabled && !authenticates_before_eviction(...)` at line 2222. The comparison is self-explanatory at the use site, and the exhaustive policy match at line 2320 already keys on the same value, so the local is needless indirection. Deleting it removes one statement and one name with identical behavior. One-line cleanup.
There was a problem hiding this comment.
Done.
| loop { | ||
| tokio::select! { | ||
| biased; | ||
| res = &mut conn => break res, | ||
| accepted = listener.accept() => { | ||
| if let Ok((extra, extra_peer)) = accepted { | ||
| debug!(?extra_peer, "Session active; rejecting connection"); | ||
| drop(extra); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[general] Reject loop cannot observe ServerEvent::Quit until the live session ends — low 🟡 — Under Reject, the inner select! races only the session future and listener.accept(); unlike the idle accept path (lines 2256-2277), it never polls the server event channel. A `Quit` (or SetCredentials/SetAutoReconnectCookie) arriving mid-session is not observed until the session ends, whereas Queue and Preempt observe Quit promptly at the next loop-top select. This review found the gap independently; the specialists missed it. Severity is low because the window is bounded by the running session's lifetime (and RdpServer serves one connection at a time, so embedders typically drive shutdown through the client), but for a long-lived session a Quit is deferred indefinitely rather than prompt. Worth either a comment stating the deferral or a third select! arm draining control events, mirroring the idle path.
There was a problem hiding this comment.
I don't think there is an asymmetry here. During a live session none of the three policies polls the event channel from the accept loop: Queue awaits run_connection_inner directly, and Preempt's race select! has only the session, accept and the probe arms. Control events are handled by the session's own loop — ServerEvent::Quit returns RunState::Disconnect, SetCredentials / SetAutoReconnectCookie update the server — and Reject polls that same future through &mut conn, exactly like the other two. So a Quit is observed by the session immediately under every policy, and by the accept loop at its next top-of-loop select once the session is gone, the same in all three.
9c2fe54 to
415e80b
Compare
There was a problem hiding this comment.
This PR converts the just-merged preempt bool into a ConnectionPolicy enum (Queue default, Reject new, Preempt carried over, mostly reindentation) per the maintainer-approved thread decision, and adds a Reject accept loop plus three listener-driven tests. Independent verification found the Preempt race logic unchanged, biased session-first polling and the documented on_accept bypass behaving as written, and no protocol/API defect beyond the agreed enum conversion. Three low-severity residues stand: a busy-polling accept-error arm mirroring the existing Preempt shape, a stale downstream '(#8)' citation, and a verbatim duplicated Negotiated entry arm. The test-harness-dedup note was rejected as idiomatic per-test isolation for this suite.
| accepted = listener.accept() => match accepted { | ||
| Ok((extra, extra_peer)) => { | ||
| debug!(?extra_peer, "Session active; rejecting connection"); | ||
| drop(extra); | ||
| } | ||
| Err(error) => warn!(?error, "accept failed while a session was live"), | ||
| }, |
There was a problem hiding this comment.
[skeptical] Reject accept-error arm repolls a persistently failing listener with no backoff — low 🟡 — Inside the Reject `biased` select, a persistently failing `listener.accept()` (e.g. EMFILE under fd exhaustion) resolves immediately every iteration, so the loop spins: sustained CPU burn plus a warn flood for as long as the live session lasts. The trigger is environmental and self-limiting, and the arm deliberately mirrors the pre-existing Preempt arm's identical warn-and-repoll shape, so this is residue of an existing pattern rather than a new design defect; a shared backoff or a yield on the error path would close it for both policies.
There was a problem hiding this comment.
Agreed it is the same shape as the Preempt arm, and that is deliberate for this PR: I'd rather not change the just-merged Preempt loop here. A shared backoff on the accept-error path — one for both policies — is a small follow-up on its own; happy to send it once this lands.
| // Serve while still accepting, but close any newcomer | ||
| // instead of racing it (Preempt) or queuing it (Queue): the | ||
| // extra connection fails fast rather than hanging in the | ||
| // backlog (#8). The session arm is polled first (biased), |
There was a problem hiding this comment.
[skeptical] Reject branch comment cites '(#8)', a downstream-fork issue number — low 🟡 — The comment justifies fail-fast with '(#8)', but this PR's references for the feature are #1483/#1476/#1588 and the body attributes the motivation to hypr-rdp reimplementing this loop downstream. '#8' in a 1900+-issue repository cannot describe this policy, so a reader following the citation lands on an unrelated ancient issue. Cite #1483 or drop the reference.
There was a problem hiding this comment.
Good catch — that was the downstream issue number. It now cites #1483.
| // Unreachable: Reject never produces a Negotiated candidate. | ||
| Entry::Negotiated(candidate, _) => self.serve_negotiated(candidate).await, |
There was a problem hiding this comment.
[code-compressor] Entry::Negotiated arm copied verbatim into Reject and Queue branches — low 🟡 — 'Entry::Negotiated(candidate, _) => self.serve_negotiated(candidate).await' now appears twice with near-identical unreachable-in-practice comments, at 2531-2532 (Reject) and 2541-2543 (Queue). Reject and Queue differ only in the Fresh arm, so they can merge into one 'Queue | Reject' arm whose Fresh handling is guarded on the policy. Tradeoff: the explicit three-arm match is slightly more self-documenting and the saving is only a few lines, so this is an optional compression, not a defect.
There was a problem hiding this comment.
Keeping the explicit arms: the two Negotiated lines are unreachable for different reasons, and the three-arm match reads as the policy table it is. A merged Queue | Reject arm guarded on the policy would save a couple of lines at the cost of that.
Devolutions#1476 added `preempt_existing_session: bool` to answer "what should `run` do when a second connection arrives while a session is live?". A bool answers it with two states, but there are three: leave the newcomer in the backlog, close it immediately, or let it take over. A bool cannot grow the third without changing type at every call site. `ConnectionPolicy { Queue, Reject, Preempt }` carries all three on one knob: - `Queue` (default) is the pre-existing backlog behaviour. - `Preempt` is Devolutions#1476's authenticated takeover, machinery unchanged -- only the option that selects it moves from the bool to this variant. - `Reject` closes the extra connection immediately: `run` polls the session and the listener together and drops any newcomer, so a second client fails fast instead of hanging. The session arm is biased first, so a client reconnecting the instant a session ends is served, not rejected. `with_preempt_existing_session(bool)` becomes `with_connection_policy(ConnectionPolicy)`; `Queue` stays the default, so no existing caller changes behaviour. The preemption security table moves to the `Preempt` variant's docs. Reject is the policy a downstream single-session server (e.g. hypr-rdp, Devolutions#8) otherwise reimplements around `run_connection`; a builder option lets it drop that and adopt this instead.
415e80b to
3d78d93
Compare
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
Thanks, LGTM!
f7732a1
into
Devolutions:master
…ting-session-default * upstream/master: (22 commits) ci(agents): sync shared review skills (Devolutions#1973) [DEVOPS-4678] chore(ci): drop the Artifactory cache refresh step (Devolutions#1972) feat(activex): present EGFX dirty regions (Devolutions#1874) fix(pdu): add missing error info codes (Devolutions#1966) ci(pr-automation): hand off after a clean review (Devolutions#1959) ci(pr-automation): link resolved PR in run summary (Devolutions#1960) ci(pr-automation): show automation mode in the run name (Devolutions#1958) ci(pr-automation): make overlap detection advisory (Devolutions#1957) feat(server): add ConnectionPolicy to reject a second connection (Devolutions#1913) feat(rdpeai): add RdpeaiServer orchestration for AUDIO_INPUT (Devolutions#1945) ci(pr-automation): clarify review repair failures (Devolutions#1956) ci(pr-automation): clarify workflow name and run title (Devolutions#1950) ci(pr-automation): delay maintainer handoff (Devolutions#1937) ci(pr-automation): reserve turns a repair can spend (Devolutions#1939) fix(egfx): do not track frames a suspended client cannot acknowledge (Devolutions#1912) feat(rdpeudp): MS-RDPEUDP version 1/2 reliable data transfer (Devolutions#1919) fix(viewer): map native physical keycodes (Devolutions#1929) ci(pr-automation): classify after final review (Devolutions#1936) fix(egfx): decode AVC420 with full-range BT.709 (Devolutions#1923) test(core): add real-world progressive RemoteFX difference fixtures from Haven (Devolutions#1848) ... # Conflicts: # crates/ironrdp-server/src/builder.rs # crates/ironrdp-server/src/server.rs
Re-expressed on top of Devolutions#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 <noreply@anthropic.com>
Re-expressed on top of Devolutions#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 <noreply@anthropic.com>
…accepts (#2116) This is the follow-up I described on #1934 (comment 5815037145). ConnectionPolicy only reaches connections that RdpServer::run accepts on the TCP socket it binds. An embedder that accepts elsewhere and calls run_connection or run_connection_with gets none of it. lamco-rdp-server accepts on TCP, Unix sockets, AF_VSOCK and a WebSocket listener that terminates TLS itself, so a second client waits unanswered behind a live session, and a takeover can't wait for CredSSP the way run's does, because the negotiation that does the gating is private. What changes: ConnectionSource is a trait with one method, accept, that returns an IncomingConnection. That is the stream, how its TLS is handled (TransportTls), the peer and local addresses when the transport has them, and a tag the embedder can use to find its own record of the connection again. RdpServer::serve is run's accept loop over a ConnectionSource. run now binds the socket, wraps the listener in a private source that sets TCP_NODELAY, and calls serve. The race, the authentication gate, the cooldown for a client that was just evicted and the cookie invalidation are the code run already had, with the stream type and the peer made generic. RdpServer::serve_connection is the part of serve that applies the policy to one session, for an embedder that wants its own loop around it, with its own hooks and its own shutdown. It returns a SessionOutcome. Under Preempt that carries the connection that took the session over, already negotiated and authenticated, which the embedder passes to the next call. A connection with no peer address (a Unix socket, AF_VSOCK) skips on_accept, on_disconnected and the cooldown, since each takes a SocketAddr. An error from a source is logged and accepting goes on. The TCP source waits 100 ms after one so that running out of file descriptors doesn't spin the loop. I considered exposing only the negotiation and leaving the race to each embedder. That would have every embedder re-derive the security rules antonmos and maryny4 settled in #1476 and #1913, so the race stays in one place and the embedder supplies the connections. Four tests in ironrdp-testsuite-extra cover it. In the first, serve takes over a live session for an authenticated newcomer on a connection with no peer address. In the second, serve_connection hands the replacement back to the embedder's own loop with the tag its source gave it. In the third, serve closes a second connection under Reject. In the fourth, on_accept gates a preempting newcomer before it can negotiate. The existing preemption and connection handler tests pass unchanged. This touches ironrdp-testsuite-extra/tests/e2e.rs next to #1985, which adds a field to TestDisplay. Each applies cleanly with the other as text, but whichever lands second needs the offers_display_control field set to true in the TestDisplay initializers of the other's tests (two here). I'll rebase whichever of the two is second. cargo xtask check fmt/lints/tests/typos/locks all pass.
RdpServer::runserves one connection at a time: while a session runs it awaitsrun_connectioninside the acceptselect!, so it does not callaccept()again and a second client sits unanswered in the listen backlog until the first ends. To that client the connection appears to hang.This is the behaviour #1483 raises.
ConnectionPolicylets the caller choose what happens to that second connection:Queue(default) keeps today's behaviour — the extra connection waits in the backlog.Rejectcloses it immediately. While a session runs,runnow polls the session and the listener together and drops any connection that arrives, so the second client fails fast and can retry rather than hanging. The running session is never interrupted: the session arm is polled first (biased), so a client reconnecting the instant a session ends is taken by the outer loop, not rejected.This is the reject half of the policy discussed in #1483. It deliberately leaves authenticated takeover — serving a new client while the incumbent still holds the slot — to a later
Preempt, without foreclosing it: theelsebranch stays inline where #1476/#1588 would add the negotiation race. A downstream consumer (hypr-rdp) currently reimplements exactly this reject loop aroundrun_connectionbecause there was no way to askrunfor it; a policy on the builder lets it drop that and adopt the upstream one with a single line.Set via
RdpServerBuilder::with_connection_policy.Tested
Two tests over a real listener in
tests/server/connection_policy.rs: withRejectthe second connection is closed during a session; withQueueit is left waiting. Both driverun's accept loop end to end.