Repository navigation
Wrdp/reactivation credential cache - #1412
Closed
Rui Carmo (rcarmo) wants to merge 2 commits into
Closed
Rui Carmo (rcarmo) wants to merge 2 commits into
Rui Carmo (rcarmo) wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is the final piece of the #1409/#1410/#1411 "wrdp" credential-handover series. It adds a per-connection credential cache so that deactivation-reactivation flows (where some clients omit a second credentials PDU) can reuse the identity validated earlier on the same TCP connection, and it introduces a post-auth ConnectionBinder hook that lets multi-user servers swap in per-session display/input handlers after credentials are accepted.
Changes:
- Add
resolve_authenticated_credentials, backed by a cache scoped toaccept_finalize()(one TCP connection), to reuse validated credentials across same-connection reactivations while preventing cross-connection inheritance. - Add a
ConnectionBindertrait /BoundConnectiontype plusRdpServer::set_connection_binderand builderwith_connection_binder, running the binder inclient_acceptedafter credential resolution. - Add two docs describing cache scope and post-auth binding, and unit tests for the cache behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ironrdp-server/src/server.rs | Adds ConnectionBinder/BoundConnection, the resolve_authenticated_credentials cache helper, binder execution in client_accepted, and unit tests. |
| crates/ironrdp-server/src/builder.rs | Threads connection_binder through the builder state and applies it in build(). |
| crates/ironrdp-server/src/lib.rs | Exports BoundConnection and ConnectionBinder. |
| docs/wrdp/reactivation-credential-cache.md | Documents the per-connection cache scope rationale. |
| docs/wrdp/auth-delegation.md | Documents the post-auth connection binding model. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1376
to
+1382
| let authenticated_credentials = resolve_authenticated_credentials( | ||
| self.credential_validator.clone(), | ||
| result.credentials.as_ref(), | ||
| result.reactivation, | ||
| authenticated_credentials_cache, | ||
| ) | ||
| .await?; |
Contributor
There was a problem hiding this comment.
Fixed on the canonical rcarmo/IronRDP:wrdp branch in b51bd0a68fb26bfc179571f59ea137eccc850fce and retained at 3b05f6fc70d91c2529df8454ce73a292c05195e6: credential rejection/backend-error paths now send the denial PDU before returning the error.
Comment on lines
+1384
to
1398
| if let Some(binder) = self.connection_binder.clone() { | ||
| let Some(credentials) = authenticated_credentials.as_ref() else { | ||
| warn!("Connection binder configured but no authenticated credentials are available"); | ||
| send_access_denied(result.io_channel_id, result.user_channel_id, writer).await?; | ||
| bail!("no authenticated credentials for connection binding"); | ||
| }; | ||
|
|
||
| let bound = binder | ||
| .bind_connection(credentials) | ||
| .await | ||
| .context("connection binder failed")?; | ||
| *self.display.lock().await = bound.display; | ||
| *self.handler.lock().await = bound.input; | ||
| debug!("Connection binder installed display/input handlers"); | ||
| } |
Contributor
There was a problem hiding this comment.
Fixed on the canonical rcarmo/IronRDP:wrdp branch in 3b05f6fc70d91c2529df8454ce73a292c05195e6: the binder block is gated on !result.reactivation, avoiding repeated per-user session lookup/setup and handler replacement during resize/reactivation.
Contributor
Contributor
Author
|
Closing in favor of #1413, which my agent put together based on Copilot feedback |
Marc-André Moreau (mamoreau-devolutions)
added a commit
that referenced
this pull request
Oct 8, 2026
…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 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.
This is the final piece in the #1409 #1410 #1411 series - it is effectively an in-session cache to cope with Windows App’s foibles when negotiating credentials, and only holds on to the credentials during session setup.