feat(acceptor): expose received client credentials in AcceptorResult by glamberson · Pull Request #1155 · Devolutions/IronRDP

Greg Lamberson (glamberson) · 2026-03-07T05:02:54Z

Summary

Adds credentials: Option<Credentials> to AcceptorResult so servers can access the credentials received from the client during SecureSettingsExchange.

This is a companion to #1150 and an interim solution for #1154. Together, they enable servers to implement post-handshake credential validation (PAM, LDAP, etc.) for TLS-mode connections.

Changes

Add received_credentials: Option<Credentials> field to Acceptor (private)
Store client credentials during SecureSettingsExchange (connection.rs, TLS-mode only)
Add credentials: Option<Credentials> field to AcceptorResult (public)
Pass stored credentials through get_result() into AcceptorResult
Propagate field through new_deactivation_reactivation

Behavior

TLS-mode connections: credentials is Some(...) containing the credentials from ClientInfoPdu
CredSSP/Hybrid connections: credentials is None (authentication happens via CredSSP, not ClientInfoPdu)
BREAKING CHANGE: AcceptorResult is not #[non_exhaustive], so adding a new public field is semver-breaking. Existing code that destructures AcceptorResult will need to add the field.

Context

This is explicitly a stopgap for #1154, which proposes a CredentialValidator trait as the proper long-term solution. This PR enables the use case immediately with minimal changes while the trait design is discussed.

Use case

I'm building a standalone Linux RDP server using IronRDP. For Linux authentication, I need to validate client-supplied credentials against PAM (pam_authenticate). The current acceptor compares credentials and drops them; after the handshake there's no way to retrieve what the client sent. This PR retains them in AcceptorResult so the server can extract and validate them post-handshake.

Checks

cargo xtask check fmt -v: pass
cargo xtask check lints -v: pass
cargo xtask check tests -v: pass

Ref: #1154, #1149, #1150

Benoît Cortier (CBenoit) · 2026-03-09T13:47:21Z

Thanks!

No API breakage: New field is Option, defaults to None. Existing code that destructures AcceptorResult will need to add the field, but the AcceptorResult struct was not #[non_exhaustive] before either.

This sounds like an API breakage because AcceptorResult struct is not #[non_exhaustive].

Copilot

Pull request overview

This PR adds an Option<Credentials> field to AcceptorResult in the ironrdp-acceptor crate, allowing server implementations to access client-supplied credentials after the RDP handshake completes. This is an interim solution enabling post-handshake credential validation (e.g., PAM, LDAP) for TLS-mode connections, as described in issue #1154.

Changes:

Stores received client credentials during SecureSettingsExchange (TLS-mode only) in a new private received_credentials field on Acceptor, and exposes them via a new public credentials: Option<Credentials> field on AcceptorResult.
Propagates stored credentials through new_deactivation_reactivation for server reactivation flows.

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

You can also share your feedback on Copilot code review. Take the survey.

Benoît Cortier (CBenoit) · 2026-03-09T13:55:26Z

I’m not against the change, but the commit and PR should be updated to reflect that the change is breaking. Also you can fix the place spotted by Copilot

Benoît Cortier (CBenoit) · 2026-03-09T13:58:07Z

Also note that we already merged work to address #1154 in the proper way in sspi-rs, and we could expect a proper solution in the near future as Guillaume Gelin (@ramnes) is already working on it. Do you still want to merge the stopgap? Might be unnecessary as I would wait for the Guillaume Gelin (@ramnes) changes before releasing the new IronRDP crates.

Add an optional `credentials` field to `AcceptorResult` that contains the client credentials received during SecureSettingsExchange. For TLS-mode connections, the client sends credentials in the ClientInfoPdu. This field exposes them so servers can validate against external systems (PAM, LDAP, etc.) after the handshake. None for CredSSP/Hybrid connections where authentication happens during the CredSSP exchange instead. BREAKING CHANGE: AcceptorResult has a new public field. Code that destructures AcceptorResult directly will need to be updated.

Greg Lamberson (glamberson) · 2026-03-09T16:48:20Z

Yeah, still need this. Right now the acceptor extracts credentials from ClientInfoPdu but doesn't expose them. That blocks my PAM auth flow entirely. I've been doing exhaustive testing across 11 Linux VMs (Fedora, Ubuntu, Arch, RHEL, Tumbleweed, Debian, EndeavourOS, etc.) and this is a P1 blocker for all of them (lamco-admin/lamco-rdp-server#35).

i'm happy to migrate to Guillaume Gelin (@ramnes)'s proper solution when it lands, but this is a one field addition that unblocks me today. The field would still be useful alongside a validator trait anyway since i'd want to inspect credentials for audit logging regardless.

Thanks for the quick review btw, really appreciate it.

Updated the commit to feat! to acknowledge the breaking change, and fixed the unnecessary clone (moved storage after the check so creds is moved, not cloned).

Benoît Cortier (CBenoit)

LGTM!

- Skip validation when credentials are None (reactivation, CredSSP) instead of bailing, consistent with Devolutions#1150/Devolutions#1155 design - Remove username from log messages to avoid leaking sensitive data - Keep validator error details in structured tracing field only - Add spawn_blocking guidance to trait doc for blocking backends

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.

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 for the trait, the `CredentialDecision` enum, the error wrapper's source chaining, and the `Send + Sync + 'static` bounds through `Arc<dyn _>` live in `crates/ironrdp-testsuite-core/tests/server/credential_validator.rs`, matching the canonical IronRDP split: public-API tests in `ironrdp-testsuite-core`, inline `#[cfg(test)]` only for crate-internal behavior (per the `autodetect.rs` precedent which has both: inline tests on internal state machine, public-API tests in `testsuite-core/tests/server/autodetect.rs`). Verification: `cargo xtask check fmt/lints/tests/typos/locks` all pass; new tests pass in the testsuite-core harness (4 added). Refs Devolutions#1154, Devolutions#1150, Devolutions#1155.

- Skip validation when credentials are None (reactivation, CredSSP) instead of bailing, consistent with Devolutions#1150/Devolutions#1155 design - Remove username from log messages to avoid leaking sensitive data - Keep validator error details in structured tracing field only - Add spawn_blocking guidance to trait doc for blocking backends

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 for the trait, the `CredentialDecision` enum, the error wrapper's source chaining, and the `Send + Sync + 'static` bounds through `Arc<dyn _>` live in `crates/ironrdp-testsuite-core/tests/server/credential_validator.rs`, matching the canonical IronRDP split: public-API tests in `ironrdp-testsuite-core`, inline `#[cfg(test)]` only for crate-internal behavior (per the `autodetect.rs` precedent which has both: inline tests on internal state machine, public-API tests in `testsuite-core/tests/server/autodetect.rs`). Verification: `cargo xtask check fmt/lints/tests/typos/locks` all pass; new tests pass in the testsuite-core harness (4 added). Refs Devolutions#1154, Devolutions#1150, Devolutions#1155.

Benoît Cortier (CBenoit) requested a review from Copilot March 9, 2026 13:46

Copilot started reviewing on behalf of Benoît Cortier (CBenoit) March 9, 2026 13:46 View session

Benoît Cortier (CBenoit) reviewed Mar 9, 2026

View reviewed changes

Comment thread crates/ironrdp-acceptor/src/connection.rs Outdated

Copilot AI reviewed Mar 9, 2026

View reviewed changes

Comment thread crates/ironrdp-acceptor/src/connection.rs Outdated

Greg Lamberson (glamberson) force-pushed the feat/acceptor-expose-credentials branch from 854e6a7 to d940b5b Compare March 9, 2026 16:48

Benoît Cortier (CBenoit) approved these changes Mar 9, 2026

View reviewed changes

Benoît Cortier (CBenoit) merged commit eda32d8 into Devolutions:master Mar 9, 2026
10 checks passed

devolutionsbot mentioned this pull request Mar 9, 2026

chore(release): prepare for publishing #1075

Closed

Greg Lamberson (glamberson) mentioned this pull request Mar 17, 2026

Support server-side credential validation (e.g. PAM) via CredentialValidator trait #1154

Open

Greg Lamberson (glamberson) deleted the feat/acceptor-expose-credentials branch March 17, 2026 12:13

This was referenced Mar 17, 2026

fix(acceptor): skip credential check when server credentials are None #1150

Merged

feat(server): add CredentialValidator trait for server-side auth #1172

Open

devolutionsbot mentioned this pull request Apr 9, 2026

chore(release): prepare for publishing #1212

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(acceptor): expose received client credentials in AcceptorResult#1155

feat(acceptor): expose received client credentials in AcceptorResult#1155
Benoît Cortier (CBenoit) merged 1 commit into
Devolutions:masterfrom
glamberson:feat/acceptor-expose-credentials

Greg Lamberson (glamberson) commented Mar 7, 2026 •

edited

Loading

Uh oh!

Benoît Cortier (CBenoit) commented Mar 9, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Benoît Cortier (CBenoit) commented Mar 9, 2026

Uh oh!

Benoît Cortier (CBenoit) commented Mar 9, 2026

Uh oh!

Greg Lamberson (glamberson) commented Mar 9, 2026

Uh oh!

Benoît Cortier (CBenoit) left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Conversation

Greg Lamberson (glamberson) commented Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Behavior

Context

Use case

Checks

Uh oh!

Benoît Cortier (CBenoit) commented Mar 9, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Benoît Cortier (CBenoit) commented Mar 9, 2026

Uh oh!

Benoît Cortier (CBenoit) commented Mar 9, 2026

Uh oh!

Greg Lamberson (glamberson) commented Mar 9, 2026

Uh oh!

Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Greg Lamberson (glamberson) commented Mar 7, 2026 •

edited

Loading