Devolutions / Devolutions/IronRDP
RdpServer::run serves one connection at a time — a second client silently hangs
- Dominant language
- Rust
- Stars
- 3.2k
- Forks
- 275
- Avg merge
- 1d 11h
- Merged PRs (30d)
- 189
Description
## Summary
`RdpServer::run` `await`s `run_connection(stream)` inline for each accepted connection, so while one client is connected the accept loop never calls `accept()` again. A second client's TCP connection sits unserved in the listen backlog — it completes the TCP handshake but never gets an RDP negotiation response — until the first session ends. There is currently no way for a server to define what *should* happen when a second client arrives, and the default (a silent hang, then a client-side timeout with no diagnostic) is arguably the worst option.
## Why it matters
Different servers want different things here, and none of them is served today:
- **Single-session servers** (mirror/serve one specific desktop — the xrdp-analogue case) want a new client to **take over**.
- Others may want to **reject** with a clear "session busy" rather than hang.
- Others may want to **queue** and serve sequentially (today's mechanics, minus the silent hang).
## Proposal
Expose the multi-connection behavior as a configurable policy on `RdpServer`, e.g. `Reject` / `Queue` / `Preempt`, defaulting to something non-hanging.
For a **`Preempt`** policy there's one non-negotiable security invariant worth calling out:
> **An unauthenticated connection must never be able to evict a live session.** A candidate must complete real negotiation — TLS, and CredSSP where the security mode is `Hybrid` — *before* it may preempt anything.
A cheaper "peek at the first bytes to see if it looks like RDP" gate is **unsafe**: any socket that sends a plausible handshake prefix can win a race against a live connection's slower real crypto and cancel a session that was about to authenticate — including a failed or malicious attempt. (We learned this the hard way; see below.)
## Reference implementation
macrdp (a downstream `ironrdp-server` consumer, single-console-session by design) has a battle-tested implementation of the `Preempt` policy that could inform the design. The concurrency-safe structure it uses:
- Factor the channel-attach body out of `&mut self` into a free function, so it can run for a candidate without the live connection's `&mut self` borrow.
- A cheap, cloned `NegotiationContext` snapshot (opts / creds / factories / display+input handles) so a candidate negotiates **without** touching `&mut self` — this is what lets it run concurrently with the live `run_connection`.
- Factory storage as `Rc` (public builder API unchanged, still takes `Box`, wrapped internally) so the snapshot holds cheap clones.
- The accept loop races the in-flight connection against `accept()`; a candidate is gated through the connection-accept hook (rate-limit / lockout) **before** it negotiates, and only preempts on full-auth success; a just-evicted peer is briefly barred from immediately bouncing back (anti-reconnect-storm).
The one wrinkle: the candidate's negotiation currently *duplicates* the pre-`accept_finalize` portion of `run_connection` (because `run_connection` is generic over the stream type and needs `&mut self`, while the candidate path needs neither) — which is exactly the sort of thing that would be designed away if this lived upstream.
## Questions for maintainers
1. Is a multi-connection policy in scope for `ironrdp-server`, or do you consider it application policy the consumer should own?
2. If in scope: additive `set_connection_policy(...)` with the default preserving today's behavior (or a non-hanging `Reject`)? What should the default be?
3. For the auth-gated preemption path specifically — would you prefer a full policy enum, or lower-level hooks (e.g. expose the concurrency-safe negotiation split) so consumers assemble their own policy?
Happy to open a PR against whatever shape you prefer.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with RdpServer::run and run_connection to understand how accepted streams are currently handled. Review the proposed NegotiationContext snapshot, connection-accept hook, and macrdp reference implementation, then clarify the policy API and default with maintainers. Done means an agreed multi-connection behavior avoids silent hangs and prevents unauthenticated preemption.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend-api-design, networking
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100