feat(server): make run_connection generic over stream type#1181
Merged
Benoît Cortier (CBenoit) merged 3 commits intoMar 23, 2026
Merged
Conversation
Make `run_connection` accept any `AsyncRead + AsyncWrite` stream instead of requiring `TcpStream` concretely. This enables RDP servers to accept connections from Unix domain sockets, VSOCK, in-process test streams, or any other bidirectional byte stream. Everything inside `run_connection` already operated generically: `TokioFramed<S>`, TLS accept, and `accept_finalize` all use trait bounds rather than concrete types. The only `TcpStream`-specific call was `peer_addr()` in the CredSSP/Hybrid path, which the existing comment noted "doesn't seem to matter yet" for NTLM auth. Replace it with `local_addr` as a fallback client name. Backward compatible: callers passing `TcpStream` continue to work unchanged via type inference. `run()` is unaffected.
There was a problem hiding this comment.
Pull request overview
This PR generalizes RdpServer::run_connection to accept arbitrary Tokio AsyncRead + AsyncWrite streams instead of a concrete TcpStream, enabling non-TCP transports (e.g., Unix sockets, VSOCK, in-process streams) to reuse the same server connection logic.
Changes:
- Make
run_connectiongeneric over a stream type implementingAsyncRead + AsyncWrite(and related bounds). - Replace the CredSSP “client name” derivation (previously from
peer_addr()) with alocal_addr-based fallback. - Remove the now-unused
TcpStreamimport.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Drop unnecessary 'static bound (accept_finalize doesn't require it) - Use constant "rdp-client" fallback for CredSSP client name instead of self.local_addr (which is the server address, not the peer) - Add comment explaining the placeholder and its context
Benoît Cortier (CBenoit)
merged commit c30d853
into
Devolutions:master
10 checks passed
Greg Lamberson (glamberson)
added a commit
to lamco-admin/IronRDP
that referenced
this pull request
May 20, 2026
Reshape the public surface introduced earlier in this PR before it lands, to avoid a future breaking touch when ironrdp-server's anyhow-removal pass arrives. Mirrors the post-Devolutions#1264 hand-rolled error pattern (Display + core::error::Error impls, no thiserror). Trait signature was `Result<bool>` (i.e. anyhow::Result through the file-level `use anyhow::Result`). Two issues: - New public surface depending on anyhow at a moment when the workspace is moving the other way (Devolutions#1277 just removed anyhow from rdpsnd-native). - `Ok(true)` vs `Ok(false)` is stringly typed at the call site: a bare bool with no semantic clue. New shape: - `CredentialDecision::{Accept, Reject}` for the binary outcome. - `CredentialValidationError` wrapping any `core::error::Error + Send + Sync` for the case where the validator backend itself failed (LDAP unreachable, PAM transport error, etc.). Manual Display + Error impls; source chains through. - `validate(&self, &Credentials) -> Result<CredentialDecision, CredentialValidationError>`. The accept/reject/backend-error trichotomy is now explicit at every call site. Rejection is no longer an error; backend failure is. Log + bail strings updated to match the trichotomy. Also addresses the API consistency note: every other configurable on `RdpServerBuilder<BuilderDone>` goes through `with_*` on the builder. Added `with_credential_validator(Option<Arc<dyn ...>>)` following the same shape as `with_connection_handler`. The post-construction `set_credential_validator` setter is kept (now takes `Option` to match the field and the builder's setter shape) for dynamic reconfiguration after `build()`. Tests added in `server.rs` cover Accept / Reject / backend-error propagation and `Arc<dyn CredentialValidator>` Send + Sync + 'static bounds. Integration with `client_accepted` is exercised through the existing acceptor-side tests once the validator is wired via the builder; no new integration test in this commit (server-side precedent per Devolutions#1181 / Devolutions#1187 / Devolutions#1281). Verification: `cargo test -p ironrdp-server --lib` (14 passed), `cargo clippy -p ironrdp-server --all-targets -- -D warnings` clean, `cargo check -p ironrdp-server` clean. Refs Devolutions#1154, Devolutions#1150, Devolutions#1155.
Greg Lamberson (glamberson)
added a commit
to lamco-admin/IronRDP
that referenced
this pull request
May 21, 2026
…ypted streams Adds a sibling to RdpServer::run_connection that walks the same per-connection state machine but skips the IronRDP-managed TLS handshake. The caller's stream must already be transport-encrypted at a lower layer (typically a WebSocket Secure terminator in an RDCleanPath-shaped deployment). The implementation mirrors run_connection except for one step: on BeginResult::ShouldUpgrade, instead of calling tls_acceptor.accept(stream), the new method calls Acceptor::mark_security_upgrade_as_done() to advance the state machine and re-wraps the inner stream as already-post-TLS. The Hybrid CredSSP block, accept_finalize, and shutdown sequence are identical to run_connection because CredSSP carries its own crypto via TSRequest and does not require the underlying transport's TLS. Builds on PR Devolutions#1181 which made run_connection generic over any AsyncRead+AsyncWrite stream. This method extends the same design intent to streams that have been TLS-terminated by a lower layer. Wire-level invariant preserved: the X.224 negotiation is untouched. The acceptor still advertises whatever SecurityProtocol it was constructed with; only the TLS-handshake step is skipped. Earlier attempts at a wire-level signal (PR Devolutions#1210, RdpServerSecurity::PreSecured) failed interop with vanilla clients and were closed; this method sidesteps that approach by relying on a higher-layer protocol (RDCleanPath) to inform the client that TLS happened elsewhere. Considered and rejected: a new RdpServerSecurity::PreAuthenticated variant. The canonical deployment serves both vanilla TCP+TLS clients and WSS+RDCleanPath clients from a single server instance on different listeners; per-connection choice fits that use case, while a variant would force splitting into two server instances and break exhaustive matches downstream. Sibling method has zero API breakage. A NOTE comment in the source records the synchronization requirement with run_connection's ShouldUpgrade arm so future rebases catch upstream divergence. The motivating downstream consumer is lamco-rdp-server's WebSocket plus RDCleanPath listener, which retires its external ws-rdp-proxy from the production WASM-client path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Make
run_connectionaccept anyAsyncRead + AsyncWritestream instead of requiringTcpStreamconcretely.run_connection<S>(&mut self, stream: S)whereS: AsyncRead + AsyncWrite + Send + Sync + Unpin + 'staticpeer_addr()call in Hybrid/CredSSP path withlocal_addrfallbackTcpStreamimportMotivation
RDP servers may accept connections from transports other than TCP: Unix domain sockets (for local WebSocket relay), VSOCK (VM-to-host), or in-process streams (integration tests). The current
TcpStreamparameter prevents this, even though everything downstream ofrun_connectionalready operates generically (TokioFramed<S>, TLS accept,accept_finalize<S>,client_loop<R, W>).Concrete use case: a QEMU display server that listens on both TCP (native RDP clients) and a Unix domain socket (browser clients via WebSocket relay) using the same
RdpServerinstance.Changes
1 file, +8/-5 lines:
run_connectionsignature withAsyncRead + AsyncWrite + Send + Sync + Unpin + 'staticbounds (matchingaccept_finalize)peer_addr()in the Hybrid/CredSSP path: the existing comment noted "doesn't seem to matter yet" for NTLM auth. Uselocal_addras fallback since generic streams don't expose peer addressTcpStreamimport (TcpListenerremains forrun())Backward Compatibility
Fully backward compatible. Callers passing
TcpStreamcontinue to work unchanged via type inference.run()is unaffected.Test Plan
cargo xtask check fmt -vpassescargo xtask check lints -vpassescargo xtask check tests -vpassescargo xtask check typos -vpassesrun()compiles with inferredTcpStreamUnixStreamaccepted viarun_connectionin downstream project(Replaces #1180, which was closed due to head branch move.)