Repository navigation
Expose CredSSP delegated credentials to servers - #1410
Rui Carmo (rcarmo) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends ironrdp-acceptor so that credentials delegated by a client during the CredSSP/Hybrid (NLA) handshake are surfaced to server code via AcceptorResult::credentials. Previously that field was populated only for TLS-mode connections (from the ClientInfoPdu), and was documented as None for CredSSP/Hybrid. This is a counterpart to #1409 and targets the "wrdp"/xrdp-sesman style architecture, where IronRDP owns the protocol-level authentication and then hands the decrypted account off to a separate service for authorization and session launch.
The delegated identity is taken from the CredSSP server state machine's ServerState::Finished(AuthIdentity) and converted into the same Credentials shape used by the ClientInfo path, so consumers get a single, uniform post-handshake validation surface.
Changes:
- Changed
CredsspSequence::handle_process_resultto return(Written, Option<AuthIdentity>), extracting the identity from theFinishedstate. - Added
Acceptor::set_received_credssp_credentials, which stores the delegated identity intoreceived_credentials(username/password/domain). - Updated
AcceptorResult::credentialsdocs and addeddocs/wrdp/credssp-delegated-credentials.md.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
crates/ironrdp-acceptor/src/credssp.rs |
Refactors handle_process_result to also return the delegated AuthIdentity from the CredSSP Finished state; control-flow rework preserves prior Written behavior. |
crates/ironrdp-acceptor/src/lib.rs |
Consumes the new tuple return and stores delegated credentials on the acceptor during the CredSSP loop. |
crates/ironrdp-acceptor/src/connection.rs |
Adds set_received_credssp_credentials, imports AuthIdentity, and updates AcceptorResult::credentials documentation. |
docs/wrdp/credssp-delegated-credentials.md |
New documentation explaining the CredSSP credential handoff and its motivation. |
I reviewed the control-flow refactor (behavior for Written is unchanged), confirmed the only caller of the acceptor's handle_process_result (lib.rs:211) is updated, verified the connector/FFI handle_process_result variants take ClientState and are unaffected, and confirmed the CredSSP-captured credentials are not overwritten by the later ClientInfoPdu path (guarded by the HYBRID/HYBRID_EX check at connection.rs:705). The Credentials custom Debug impl continues to hide the password. I did not find concrete defects, but note this touches security-sensitive credential handling and makes a breaking change to the public CredsspSequence::handle_process_result signature (in pub mod credssp), which warrants human sign-off.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Checked against the latest Copilot review state: this PR has no inline Copilot review comments to address. The canonical |
|
Closing in favor of #1413, which my agent put together based on Copilot feedback |
…servers (#1413) ## 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: - #1409: denial PDU regression and binder doc wording. - #1411: binder should not run again during reactivation. - #1412: denial PDU regression and reactivation rebinding. - #1410 had no inline Copilot review comments. ## Validation - `cargo check -p ironrdp-server -p ironrdp-acceptor` - `git diff --check` --------- Co-authored-by: Rui Carmo <rcarmo@users.noreply.github.com> Co-authored-by: Rui Carmo <rcarmo@rcarmo.com> Co-authored-by: Rui Carmo <rui.carmo@gmail.com> Co-authored-by: Marc-André Moreau <mamoreau@devolutions.net> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This is a counterpart to #1409 - I wanted to make it really clear where the credential capture was made.