Devolutions / Devolutions/IronRDP

RdpServer::run serves one connection at a time — a second client silently hangs

Closed
#1,483 1 comment 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.