Credential hand-over to per-user server sessions by piclaw-bot · Pull Request #1413 · Devolutions/IronRDP

Piclaw (piclaw-bot) · 2026-07-05T09:35:39Z

Summary

This replaces and consolidates #1409, #1410, #1411, and #1412 into a single branch from rcarmo:wrdp.

The split PR heads use wrdp/* branch names, but the fork now uses the canonical wrdp branch. Because Git cannot keep both refs/heads/wrdp and refs/heads/wrdp/* at the same time, this PR carries the updated commits and review fixes together.

Why

wrdp follows the xrdp-sesman multi-user architecture model, so it needs a way to hand authenticated credentials from the protocol handshake into the per-user session binder.

Changes

Adds an async post-auth connection binder hook in ironrdp-server.
Scopes reactivation credential reuse to the current TCP connection.
Exposes CredSSP delegated credentials to server consumers via the same Credentials shape used by ClientInfo.
Restores the client-facing denial PDU on credential reject/backend-error paths before returning the error.
Clarifies binder docs so they refer to authenticated credentials being available, not only to configured validator acceptance.
Skips binder re-execution during deactivation/reactivation so resize/reactivation does not restart per-user binding or replace handlers mid-session.

Copilot review follow-up

This includes fixes for the Copilot comments from the superseded PRs:

Credential hand-over to sub-servers (Wrdp/canonical stack) #1409: denial PDU regression and binder doc wording.
Add post-auth connection binder hook #1411: binder should not run again during reactivation.
Wrdp/reactivation credential cache #1412: denial PDU regression and reactivation rebinding.
Expose CredSSP delegated credentials to servers #1410 had no inline Copilot review comments.

Validation

cargo check -p ironrdp-server -p ironrdp-acceptor
git diff --check

Copilot

Pull request overview

This PR extends the IronRDP server/acceptor stack to support multi-user “sesman”-style architectures by handing authenticated credentials from the protocol handshake into a post-auth, per-user connection binding phase, while also handling deactivation/reactivation credential reuse safely on a per-TCP-connection basis.

Changes:

Add a post-auth async ConnectionBinder hook to ironrdp-server to swap in per-user display/input handlers after credentials are available.
Propagate CredSSP (NLA/Hybrid) delegated credentials into AcceptorResult::credentials so servers can validate/bind using the same Credentials shape as TLS ClientInfo.
Introduce a per-connection authenticated-credentials cache to support deactivation/reactivation flows where clients may omit a second credentials PDU.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
docs/wrdp/reactivation-credential-cache.md	Documents the per-TCP-connection scope of the reactivation credential cache.
docs/wrdp/credssp-delegated-credentials.md	Documents CredSSP delegated credential handoff into `AcceptorResult::credentials`.
docs/wrdp/auth-delegation.md	Documents the post-auth connection binding model for multi-user servers.
crates/ironrdp-server/src/server.rs	Adds `BoundConnection`/`ConnectionBinder`, integrates binder + reactivation credential caching, and adds targeted tests.
crates/ironrdp-server/src/lib.rs	Re-exports the new binder/bound-connection API.
crates/ironrdp-server/src/builder.rs	Adds builder plumbing to configure the optional connection binder.
crates/ironrdp-acceptor/src/lib.rs	Captures CredSSP delegated credentials from the CredSSP sequence and stores them in the acceptor.
crates/ironrdp-acceptor/src/credssp.rs	Extends CredSSP result handling to return delegated credentials when the exchange finishes.
crates/ironrdp-acceptor/src/connection.rs	Updates `AcceptorResult` credential semantics and adds internal storage for CredSSP delegated credentials.

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

Piclaw (piclaw-bot) · 2026-07-06T17:48:32Z

+                let bound = binder
+                    .bind_connection(credentials)
+                    .await
+                    .context("connection binder failed")?;
+                *self.display.lock().await = bound.display;
+                *self.handler.lock().await = bound.input;


Fixed in rcarmo/IronRDP@4d216af99bba: binder failures now send the access-denied PDU before returning the error, matching the validator rejection/backend-error paths.

Piclaw (piclaw-bot) · 2026-07-06T17:48:33Z

+/// Display/input objects bound after a credential validator accepts a client.
+///
+/// Servers with per-user desktop/session isolation can start with placeholder
+/// display and input handlers, validate the client's credentials, and then
+/// replace those placeholders with handlers attached to the authenticated
+/// user's session before the RDP client loop starts.


Updated in rcarmo/IronRDP@4fc52f5bb565: the binder docs now describe binding after the server authenticates the client, without overstating the validator as the only authentication source.

Piclaw (piclaw-bot) · 2026-07-06T17:48:35Z

+    /// Set a binder that replaces display/input handlers after credentials are accepted.
+    pub fn with_connection_binder(mut self, binder: Option<Arc<dyn ConnectionBinder>>) -> Self {
+        self.state.connection_binder = binder;


Updated in rcarmo/IronRDP@4fc52f5bb565: the credential-validator docs now state that accepted credentials surfaced by the acceptor include ClientInfo credentials and, when available, CredSSP/Hybrid delegated credentials.

Benoît Cortier (CBenoit)

Thank you!

This is covering a real dead angle in our API.

We generally prefer smaller, scoped PRs for each change, but since everything is intertwined I see where you come from.

The two added tests exercise resolve_authenticated_credentials in isolation with same-connection reuse and cross-connection isolation. Those are good and directly pin the #1412 scoping fix. However, the actual new surface is untested: the binder handler-replacement, the !reactivation skip (the #1411 fix), and the "binder set + no creds -> deny" branch. The reactivation-skip in particular is the behavior most likely to regress under future refactor, and it has zero coverage. A test with a fake ConnectionBinder asserting it fires once on initial accept and not on a synthesized reactivation AcceptorResult would lock the contract. This can be a follow up PR if you prefer.

Benoît Cortier (CBenoit) · 2026-07-06T13:20:59Z

+    } else if let Some(creds) = result_credentials {
+        *authenticated_credentials_cache = Some(creds.clone());
+        Ok(Some(creds.clone()))


issue: The no-validator path returns client-supplied credentials as "authenticated".
Those flow straight into the binder at server.rs:1400 (bind_connection(credentials)), which then attaches per-user display/input to that identity. For CredSSP/Hybrid that's fine because the handshake authenticated them. But for TLS ClientInfo mode (and with_no_security) with no validator configured, result.credentials is whatever the client asserted in the ClientInfoPdu, i.e., never verified. Yet the trait doc at server.rs:193-198 says these are bound "after a credential validator accepts a client" and the local is named authenticated_credentials. The name and docs promise validation the code doesn't enforce, so a consumer who wires a ConnectionBinder but forgets a CredentialValidator silently binds a per-user session to an unauthenticated, client-chosen username. Given the sesman/PAM use case this API exists for, that is an auth-bypass footgun in a public API.

suggestion: Make the wrong thing hard: either require a validator to be present when a binder is set (fail fast at build/first-accept), or rename to received_credentials and document loudly that CredSSP is the only mode where the binder input is authenticated absent a validator.

Fixed in rcarmo/IronRDP@4d216af99bba: the binder now refuses client-supplied ClientInfo/no-security credentials unless a validator authenticated them. The only no-validator path still allowed for binding is CredSSP/Hybrid, where the handshake authenticated the credentials.

Benoît Cortier (CBenoit) · 2026-07-06T13:24:06Z

+                let bound = binder
+                    .bind_connection(credentials)
+                    .await
+                    .context("connection binder failed")?;


issue: Every sibling failure path in this function sends send_access_denied(...) before bailing. The PR description explicitly says "Restores the client-facing denial PDU on credential reject/backend-error paths before returning the error". A binder failure (PAM session open fails, user session unreachable) is exactly a backend error in the same class, but here the client just gets a dropped connection with no ServerSetErrorInfoPdu. Either this is an inconsistency with the stated goal, or an intentional exception that needs a comment justifying why binder failure is treated differently from validator-backend failure.

suggestion: Wrap with send_access_denied on the error, matching the validator path.

Fixed in rcarmo/IronRDP@4d216af99bba: binder failures now send the same access-denied PDU before returning the error, matching the validator rejection/backend-error paths.

Benoît Cortier (CBenoit) · 2026-07-06T13:33:13Z

+                    .context("connection binder failed")?;
+                *self.display.lock().await = bound.display;
+                *self.handler.lock().await = bound.input;
+                debug!("Connection binder installed display/input handlers");


issue: self.display/self.handler are RdpServer-scoped, and run() processes connections serially and reuses the same RdpServer across them. So after connection A binds user A's handlers, they persist into self.display/self.handler until connection B's binder overwrites them. Correctness here rests entirely on the invariant "the binder replaces both handlers on every non-reactivation connection." Today that holds (binder requires creds, else bails). But it's an undocumented invariant: any future path that reaches client_loop without rebinding (a new security mode, an early-return refactor, a second binder-optional entry point via the public run_connection) would serve the previous authenticated user's display/input to a new client. That's a cross-user session leak waiting to happen, and it's the kind of state-scoping the design should not leave implicit.

suggestion: Per-connection identity-bound resources want per-connection ownership (bind into a value threaded through client_accepted/client_loop), not mutation of shared server slots. At minimum, document the invariant at the binder call site and at the self.display/self.handler fields.

Fixed in rcarmo/IronRDP@4d216af99bba: bound display/input handlers are now connection-local slots, cleared after each connection, with the original server handlers retained as defaults. A later connection can no longer inherit a previous user's bound handlers.

Benoît Cortier (CBenoit) · 2026-07-06T13:41:05Z

+    /// same post-handshake validation and binding path as TLS ClientInfo
+    /// credentials.
+    pub(crate) fn set_received_credssp_credentials(&mut self, identity: AuthIdentity) {
+        self.received_credentials = Some(Credentials {


thought: This copies the delegated secret into Credentials.password: String, which has no zeroize on drop semantic. The server-side cache then holds a clone for the whole TCP-connection lifetime. This is consistent with how TLS ClientInfo credentials are already handled, so it's not a regression, but this PR meaningfully increases how many plaintext passwords sit in Strings and how long they live. If secret hygiene is a goal for the sesman path, this is the moment to consider a zeroizing wrapper. Out of scope for this PR, but it would be a good follow up.

Agreed, and I would keep that as a follow-up rather than broadening this PR: this does not introduce a new representation for ClientInfo passwords, but the delegated-credential cache makes the existing secret-hygiene tradeoff more visible.

Rui pointed out that I had treated this too much as a generic future zeroizing-wrapper topic, and he was right. I made the focused PR fix in rcarmo/IronRDP@a87ea4f1ae66: the server no longer caches/clones authenticated credentials for the TCP connection lifetime. Validation and binding now use the credentials borrowed from the current AcceptorResult, and reactivation without resent credentials does not retain the prior password.

Benoît Cortier (CBenoit) · 2026-07-06T13:42:39Z

+        }
+    } else if let Some(creds) = result_credentials {
+        *authenticated_credentials_cache = Some(creds.clone());
+        Ok(Some(creds.clone()))


question: On reactivation, resolve_authenticated_credentials returns the cached creds, but the binder block is skipped and nothing else consumes authenticated_credentials in that branch, so the only observable effect on reactivation is the debug log. Is re-validation-on-reactivation (when result_credentials is present, since that branch is checked before reactivation) the intended reason to keep calling it, or is the reactivation lookup effectively dead? A one-line comment would settle it.

Clarified in rcarmo/IronRDP@4c20d9ff98e8: reactivation still resolves credentials so clients that resend them are re-validated before the existing channel state is reused; the binder remains skipped during reactivation.

Benoît Cortier (CBenoit) · 2026-07-06T13:44:02Z

    }

+    /// Set a binder that replaces display/input handlers after credentials are accepted.
+    pub fn with_connection_binder(mut self, binder: Option<Arc<dyn ConnectionBinder>>) -> Self {


thought: The Option param reads oddly at the call site (.with_connection_binder(Some(Arc::new(x)))), but it's consistent with with_credential_validator and with_connection_handler. This is consistent, but I’m wondering if we should change that. Out of scope for this PR.

Agreed; I left this unchanged for this PR to stay consistent with the adjacent optional hooks and avoid mixing API ergonomics into the correctness fixes.

Rui Carmo (rcarmo) · 2026-07-06T17:24:12Z

Hey there Benoît Cortier (@CBenoit)! Apologies for the longish PR, yes--I did try four smaller ones earlier, but then it was just simpler to get it all out in one chunk. I made sure my agent explained that when it got them together because, well, Copilot actually pointed out part of the inter-dependencies :)

We'll add the follow-ups and tests for these in small, focused commits (sorry, had written PRs) - right now this is the minimal surface I need to get the "sesman" thing to work, and I think it really belongs here.

I revised the above - I think it would be better to have a complete single PR and get this tidied up, since the tests will be very few lines and easy to track.

And regarding zeroing the credentials... Yeah. I know. I hate cacheing them in the first place, but I need to get them over to PAM in the actual RDP server for the user (which is in another context), and Windows App on the Mac tends to reconnect quickly for some reason, and... well, you know how this sort of thing goes.

Rui Carmo (rcarmo) · 2026-07-06T17:52:04Z

OK, after a bit of to and fro, and while Piclaw (@piclaw-bot) tidies up the mess I made, I would like to know if you're interested in us looking at other ways of handling credentials - I am only looking at passwords for Linux, but I can do Yubikeys as well - later, though.

Rui Carmo (rcarmo) added 5 commits July 5, 2026 07:37

Add post-auth connection binder hook

53c7353

Scope reactivation credential cache to connection

92f28ea

Expose CredSSP delegated credentials to servers

368d3e1

Address post-auth binder review feedback

b51bd0a

Avoid rebinding during RDP reactivation

3b05f6f

This was referenced Jul 5, 2026

Credential hand-over to sub-servers (Wrdp/canonical stack) #1409

Closed

Expose CredSSP delegated credentials to servers #1410

Closed

Add post-auth connection binder hook #1411

Closed

Wrdp/reactivation credential cache #1412

Closed

Benoît Cortier (CBenoit) requested a review from Copilot July 6, 2026 11:11

Copilot started reviewing on behalf of Benoît Cortier (CBenoit) July 6, 2026 11:11 View session

Copilot AI reviewed Jul 6, 2026

View reviewed changes

Benoît Cortier (CBenoit) reviewed Jul 6, 2026

View reviewed changes

Rui Carmo added 3 commits July 6, 2026 17:40

Keep bound server handlers connection-local

4d216af

Clarify credential checks during reactivation

4c20d9f

Update server credential hook docs

4fc52f5

Avoid caching authenticated credentials

a87ea4f

Uh oh!

Conversation

Piclaw (piclaw-bot) commented Jul 5, 2026

Summary

Why

Changes

Copilot review follow-up

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piclaw (piclaw-bot) Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rui Carmo (rcarmo) commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rui Carmo (rcarmo) commented Jul 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Piclaw (piclaw-bot) Jul 6, 2026 •

edited

Loading

Rui Carmo (rcarmo) commented Jul 6, 2026 •

edited

Loading