[Greg Lamberson (glamberson)]

Summary

• Adds RdpServer::run_connection_with(stream, tls: TransportTls), one per-connection entry point that selects who performs the TLS handshake.
• New #[non_exhaustive] enum TransportTls: Managed (IronRDP performs the TLS accept, standard TCP+TLS) and AlreadyDone (stream already past TLS at a lower layer such as a WSS terminator, so IronRDP skips the handshake).
• run_connection is now a thin delegate that calls it with TransportTls::Managed; behavior unchanged. Additive and non-breaking.
• Both modes share one body: the only branch is the TLS accept, and the post-handshake tail (mark upgrade done, optional Hybrid CredSSP, finalize, shutdown) is single-sourced, so per-connection state cannot drift between them.
• Motivation: RDCleanPath (Gateway, Cloudflare IronRDP-WASM) and embedded WSS-terminator servers, where the client terminates TLS below RDP. X.224 negotiation is untouched.

Validation

• cargo xtask check fmt/lints/tests/typos/locks all pass.

Notes

• Reshaped during review: this began as a separate sibling method and was collapsed into the single run_connection_with + TransportTls shape so the two paths cannot rot out of sync.
• Hybrid over AlreadyDone: CredSSP pubKeyAuth binds to the certificate the client validated at the lower TLS layer, so the RdpServerSecurity::Hybrid key must match that certificate (holds by construction under an RDCleanPath proxy). Documented as a precondition.
• Wire-level PreSecured was tried and closed (feat(server): add PreSecured transport mode for pre-encrypted connections #1210); RDCleanPath solves this above the wire. Builds on feat(server): make run_connection generic over stream type #1181.

[Greg Lamberson (glamberson)]

Benoît Cortier (@CBenoit) This is probably my highest priority right now. Thanks for your attention to it.

[Copilot]

Pull request overview

This PR extends ironrdp-server’s per-connection entry points by adding a new RdpServer method intended for deployments where the underlying byte stream has already been transport-encrypted by a lower layer (e.g., a WSS terminator), so the server must skip the IronRDP-managed TLS handshake while still running the same accept/state-machine flow.

Changes:

• Add RdpServer::run_connection_pre_authenticated, a sibling to run_connection that, on BeginResult::ShouldUpgrade, calls Acceptor::mark_security_upgrade_as_done() instead of performing tls_acceptor.accept(...).
• Add extensive API documentation describing the intended proxy/WSS/RDCleanPath use case, preconditions, and wire-level invariants.
• Reuse the existing finalize + optional Hybrid/CredSSP path and perform a post-connection shutdown of the underlying stream.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Benoît Cortier (CBenoit)]

Hi! I'm sorry for the wait.

I'm greatly in favor of adding an API for the use case. Very useful for IronRDP <-> IronRDP worlds where the transport can be as custom as it comes.

However, the addition is seemingly simple, but since RdpServer is stateful, I'm worried it will break things in subtle ways.

For instance, here I see that the display_suppressed state is not reset. It does not seem intended to me, unless I'm missing something. display_suppressed is an Arc<AtomicBool> on RdpServer, so it lives across connections (and can be shared externally via display_suppressed_handle()). Without the reset, a client that disconnected while suppressed leaves the flag true, and the next session silently drops frames until a RefreshRectangle/SuppressOutput{Some} arrives.

This echoes with the // NOTE: keep in sync with the ShouldUpgrade arm of run_connection above that has already failed, and note it failed outside the arm the NOTE points at. The drift is in the function prologue, which the comment doesn't cover, so the safeguard couldn't have caught it. That's my cue that a comment is the wrong tool for this job.

My next point is on affordability / cleanliness. It's cheap to write but not cheap to own. The cost isn't the line count, but that we now have two public per-connection entry points whose bodies are ~95% identical and have to evolve together, with the only thing holding them in sync being reviewer vigilance. The display_suppressed case is the concrete demonstration that this doesn't hold: the copy rotted on the first commit. So it reads clean in isolation, but as a fork of run_connection it's structurally the opposite, the invariant "these two stay identical" is unenforceable as written.

Then, the next one is not a blocker per se, but I think we could do better on misuse resistance. The type signature makes the easy thing the unsafe thing. S: AsyncRead + AsyncWrite + Send + Sync + Unpin is satisfied by a bare TcpStream, so calling this on an unencrypted socket compiles and runs and silently puts plaintext RDP on the wire. Precondition
1 is the load-bearing safety contract of the whole method, and it's only enforced by a doc comment, and there is no signal at the call site.

I think the variant-on-RdpServerSecurity you considered and rejected was rightly rejected. I don't want to reopen that, your reasoning on the single-server/two-listener deployment and the exhaustive-match break is sound. But the sibling method isn't the only additive, non-breaking shape. The axis that actually varies here is who performs the TLS handshake, so I would rather name that axis and keep a single state machine:

#[derive(Debug, Clone, Copy)]
#[non_exhaustive]
pub enum TransportTls {
    /// IronRDP performs the TLS accept on the stream (standard TCP+TLS).
    Managed,
    /// The stream is already past TLS, terminated by a lower layer (e.g. WSS).
    /// The caller guarantees the transport is already encrypted.
    AlreadyDone,
}

pub async fn run_connection_with<S>(&mut self, stream: S, tls: TransportTls) -> Result<()>
where S: AsyncRead + AsyncWrite + Send + Sync + Unpin
{ /* single body; the only branch is inside the ShouldUpgrade arm */ }

// behaviour-preserving, no API break:
pub async fn run_connection<S>(&mut self, stream: S) -> Result<()>
where S: AsyncRead + AsyncWrite + Send + Sync + Unpin {
    self.run_connection_with(stream, TransportTls::Managed).await
}

What this buys us over the sibling method: one state machine, so the keep-in-sync problem and the whole display_suppressed class of bug disappear structurally. The cost is one small #[non_exhaustive] enum, which I think is a smaller conceptual surface than a second near-duplicate method.

One naming point regardless of which shape we land on: run_connection_pre_authenticated worries me, because nothing about authentication is pre-done, in Hybrid we still run the full CredSSP exchange, and all three preconditions are about transport encryption, not auth. A caller who reads "pre_authenticated" might skip their own credential validation on the assumption that auth already happened. I prefer if the name track the thing that actually varies (the TLS handshake), which TransportTls::AlreadyDone seems to be doing well.

If we want to eventually close the plaintext-TcpStream gap too, the follow-up increment would be a thin AlreadySecured<S>(S) newtype whose only constructor forces the caller to acknowledge the contract; moving precondition 1 from a silent doc note to an explicit opt-in in the types.

I also have a question on the "CredSSP works the same" claim. Precondition 3 (and the commit message) lean on "CredSSP carries its own crypto via TSRequest, so it works the same." That's right for the SPNEGO/NTLM auth exchange, but I want to make sure we're not glossing the server-public-key-confirmation step. accept_credssp is passed pub_key, which per MS-CSSP ends up in pubKeyAuth and the client checks it against the public key of the certificate it saw during the TLS handshake. On the standard path that's our own TLS cert, so the binding is meaningful. On this path the TLS the client saw was terminated at the WSS layer, so doesn't this only work if the WSS terminator presents the same cert/key as the pub_key we hand to accept_credssp? If they differ, I would expect Hybrid clients to fail the pubKeyAuth check. Is that guaranteed by construction in the RDCleanPath deployment (i.e. RDCleanPath surfaces the same backend cert the WSS layer terminates with), or is it an additional precondition we should spell out for Hybrid? Genuinely asking, you've run this on the fork, so you may have already confirmed the WASM client is happy here.

[Greg Lamberson (glamberson)]

Hi, I was in the middle of responding to the Copilot review when you posted the more substantial review above. Since checks now take a fair amount of time I determined to just finish what I was already doing rather than addressing your larger review. I'm considering it now and will respond very shortly.

[Greg Lamberson (glamberson)]

Thanks, this is all fair and I have reworked to your shape.

There is now a single run_connection_with(stream, tls: TransportTls); run_connection delegates with TransportTls::Managed, and the only branch is whether IronRDP does the TLS accept or wraps an already-post-TLS stream. The shared tail (mark upgrade done, optional Hybrid CredSSP, finalize, shutdown) is one helper both modes call, and the display_suppressed reset lives once ahead of the branch. The keep-in-sync comment and the Synchronization doc section are gone, since with one body there is nothing to keep in sync. That was your point exactly: The drift was in the prologue, outside the arm the comment guarded, so a comment could never have caught it.

Naming: Folded in. The axis that varies is named directly (TransportTls::Managed vs AlreadyDone), and run_connection_pre_authenticated is gone.

On CredSSP you are right, and it is the sharpest point. pubKeyAuth binds to the certificate the client validated at the TLS layer, so on AlreadyDone it only holds if the lower layer presented the same key we pass to accept_credssp. In a canonical RDCleanPath proxy the backend actually runs the managed path (the proxy does real TLS with it and relays its cert to the client), so the binding holds by construction. AlreadyDone is really the embedded shape, where the server terminates the client's WSS/TLS itself; there the client saw the server's own cert, so it holds as long as RdpServerSecurity::Hybrid is configured with that same certificate. I made that explicit in precondition 3 instead of leaving the "works the same" claim. To be straight about testing: My consumer runs this path with RdpServerSecurity::Tls and authenticates out of band, and the browser/WASM client cannot do CredSSP at all, so I have exercised the TLS path here, not Hybrid.

The AlreadySecured newtype is a good call for moving precondition 1 into the type system; I would rather do it as the follow-up increment you describe than fold it in here.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[Benoît Cortier (CBenoit)]

Thanks, the new shape looks good to me.

I made one edit to the docs: the framing tied this to RDCleanPath and cited Devolutions Gateway / Cloudflare as consumers, but that's not accurate for the Gateway topology. In a classic RDCleanPath deployment the proxy does a real TLS handshake with a separate backend RDP server (that's how it gets server_cert_chain to relay), so the backend owns its own TLS and uses Managed and never hits this path. The real use case for AlreadyDone is an ironrdp-server endpoint that terminates transport encryption itself (embedded WSS, or a pre-accepted TLS stream), i.e. decoupling transport encryption from the RDP security-upgrade step. RDCleanPath is just one client-side way to tell a client not to expect the inner handshake.

Same reason I dropped the "holds by construction in an RDCleanPath proxy deployment" line from precondition 3: that described a topology that doesn't use this code, so the cert-match requirement really is on the embedder. Let me know if the framing doesn't match your actual deployment.

[Greg Lamberson (glamberson)]

Looks good, thank you!