[Greg Lamberson (glamberson)]

Closes #1154

Summary

Add a CredentialValidator trait that lets servers validate client
credentials during connection setup, after the acceptor extracts them
from ClientInfoPdu (enabled by #1155).

This follows up on the API discussion in #1150: Option<Credentials>
is the wrong type to carry security policy. The trait makes auth an
explicit, application-supplied component rather than an implicit
builder default.

What it does

• CredentialValidator trait with validate(&self, &Credentials) -> Result<CredentialDecision, CredentialValidationError>.
• CredentialDecision::{Accept, Reject} enum for the binary outcome (no stringly typed bool).
• CredentialValidationError wrapping any core::error::Error + Send + Sync for backend-failure cases (LDAP/PAM/DB unreachable). Manual Display + core::error::Error impls; the source chain is preserved through .source().
• RdpServerBuilder::with_credential_validator(Option<Arc<dyn CredentialValidator>>) to wire it in at construction time, matching the with_* pattern of every other configurable on BuilderDone.
• RdpServer::set_credential_validator(Option<Arc<dyn CredentialValidator>>) for post-construction reconfiguration.
• Validation runs in client_accepted() before session start.
• No validator configured = no credential check (existing behavior).
• Validator configured + credentials received = validate, reject on Decision::Reject or backend Err(_).
• Validator configured + no credentials received = skip validation (covers CredSSP/Hybrid path and reactivation/resize iterations where AcceptorResult.credentials is None per fix(acceptor): skip credential check when server credentials are None #1150/feat(acceptor): expose received client credentials in AcceptorResult #1155).

Design

For TLS-only connections, Client Info credentials are the only
credentials the application layer receives (no CredSSP). This trait
gives server implementations a clean hook to validate them against
PAM, LDAP, databases, or any external system.

Not used for CredSSP/Hybrid connections, where authentication happens
during the NTLM challenge-response exchange before ClientInfoPdu.

Rejection is modelled as Ok(CredentialDecision::Reject), not as an
Err. A rejection from a working validator is the validator doing
its job, not a failure of the validator itself. Only true backend
infrastructure failures (the LDAP server is down, the PAM transport
broke) return Err(CredentialValidationError::new(_)). This keeps
the three outcomes (accept / reject / infrastructure-error) distinct
at every call site.

Hand-rolled Display + core::error::Error impls follow the
post-#1264 workspace pattern; no thiserror, no anyhow leakage
across the public trait boundary.

Example

use std::sync::Arc;
use ironrdp_server::{
    CredentialDecision, CredentialValidationError, CredentialValidator, Credentials,
    RdpServer,
};

struct PamValidator;

impl CredentialValidator for PamValidator {
    fn validate(
        &self,
        creds: &Credentials,
    ) -> Result<CredentialDecision, CredentialValidationError> {
        match pam_authenticate(&creds.username, &creds.password) {
            Ok(true) => Ok(CredentialDecision::Accept),
            Ok(false) => Ok(CredentialDecision::Reject),
            Err(backend_err) => Err(CredentialValidationError::new(backend_err)),
        }
    }
}

let server = RdpServer::builder()
    .with_addr(addr)
    .with_tls(acceptor)
    .with_input_handler(my_input)
    .with_display_handler(my_display)
    .with_credential_validator(Some(Arc::new(PamValidator)))
    .build();

Tests

Public-API tests for the trait, CredentialDecision, and the error
wrapper's source-chain propagation 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)] for crate-internal
behavior, per the existing autodetect.rs precedent).

Test plan

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

[Copilot]

Pull request overview

Adds a server-side hook to validate client-supplied credentials (from ClientInfoPdu) during connection setup, enabling external authentication backends (PAM/LDAP/DB) in ironrdp-server.

Changes:

• Introduces a new public CredentialValidator trait.
• Adds an optional validator to RdpServer with a setter API.
• Invokes credential validation in client_accepted before entering the session loop.

---

💡 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.

[Greg Lamberson (glamberson)]

Rebased. Thanks.

[Greg Lamberson (glamberson)]

Benoît Cortier (@CBenoit) Hi, I hope you can consider this with some priority as it's a feature I require in both lamco-rdp-server and lamco-qemu-rdp (Proxmox RDP Server component) and currently resides in a fork which I'd like to eliminate.

Thanks!

[Greg Lamberson (glamberson)]

Benoît Cortier (@CBenoit) Hi, could you please review this and merge it ASAP? This one is a real bottleneck for me. Do you have any issues or concerns you'd like me to address or any issues with merging otherwise? Please let me know if so. Thanks.

[Greg Lamberson (glamberson)]

Benoît Cortier (@CBenoit) Today is 2026-05-25. PR #1172 has been open since 2026-03-17, which is 70 days. CI is green across all 20 checks on the current head (1f8d5014, just rebased onto master 879ffed8). All 5 Copilot review threads from the 2026-03-18 pass are resolved. There has never been a maintainer review on this PR; the only review history is Copilot's. I have rebased four times and pushed two substantive reshapes, including the 2026-05-20 typed-error refactor that moved the API from anyhow-leaking Result<bool> to CredentialDecision::{Accept, Reject} plus a CredentialValidationError wrapper with manual Display + core::error::Error impls, and the migration of public-API tests into ironrdp-testsuite-core/tests/server/credential_validator.rs per the autodetect.rs precedent.

I have pinged twice (2026-05-16, 2026-05-20). Both pings explicitly invited "what do you want me to change" feedback. Neither got a response. In the same five-day window since the 2026-05-20 ping, you have merged eight of my other PRs, including a Saturday-filed one (#1303) that landed Monday morning. The signal is that this specific PR is being deferred, not lost in queue.

I respect that Devolutions has priorities I do not see and competing directions I am not party to. Those are legitimate. I have tried to bend over backwards to align my work with what I understand about your priorities: the precheck-driven amend cadence, the testsuite-core test-placement convention, the typed-error-no-anyhow direction, the conventional-commit + BREAKING CHANGE: discipline for release-plz, the with_* builder convention, the spec-MUST-on-emit + liberal-on-receive posture you've prescribed elsewhere. I would appreciate some attention to mine here. lamco-rdp-server and the planned lamco-qemu-rdp (Proxmox RDP server component) both have a hard dependency on a server-side credential-validation hook. I have been carrying that hook on feat/lamco-combined in our fork for 70 days. Every release I cut from that branch is a fork-rebase exercise I would prefer not to do indefinitely.

What I want is engagement, not approval. The options I see, from my side:

1. The trait is fine but you want a different shape. Tell me which shape (different signature, different placement on ConnectionHandler instead of standalone, different name, dropped builder method, different lifecycle hook, anything) and I will rework. I have explicit cycles available this week.
2. You do not want this surface upstream at all (Devolutions has plans for credential validation that conflict, the trait surface is wrong for IronRDP's API direction, or any other reason). Tell me and I will close the PR with a documenting comment and carry the patch downstream permanently. No friction.
3. You want the discussion but the shape needs broader maintainer input first. Tell me and I will wait, with a known target date or a known trigger event.

Competing priorities, conflicting thoughts, structural concerns, scope disagreements: all of these are fine and I welcome the discussion. Silence is what is not working here. What is the path forward?

[Benoît Cortier (CBenoit)]

Greg Lamberson (@glamberson) First, I owe you an apology. The honest answer is that older PRs have ended up buried under notification volume, and this one slipped further than it should have. The pings were warranted.

You probably noticed I've been digging through the backlog over the last few days and merged several of your older PRs; that pass hasn't reached this PR yet, but it wasn't a deliberate deferral. For what it's worth, the volume of your work we've merged should be the signal on our posture toward your contributions, not the silence on this one.

I'll do another read and get you concrete feedback on the trait shape.

Separately: could we move to Matrix for a wider conversation? I'd like to get the big picture from you in case there are other things I've missed that matter for lamco-rdp-server and lamco-qemu-rdp. Ping me in private, I’ll be sure to check your message on Monday.

Thanks for the patience and for the directness.

[Benoît Cortier (CBenoit)]

Back with the feedback I promised. I like the direction on explicit outcome typing (CredentialDecision) and separating backend failure from credential rejection (CredentialValidationError).

I do have a design concern before we lock this API: right now we effectively have two credential-validation paths (set_credentials exact-match in acceptor + CredentialValidator in server). I’d strongly prefer we converge on one straightforward mechanism long-term to avoid confusion and policy drift.

I’m okay with dual support only as a short transition period if that’s needed for velocity, but then we should explicitly define the convergence plan.

Questions/clarifications:

1. Long-term source of truth: which mechanism is intended to survive (acceptor exact-match vs validator), and what is the deprecation plan for the other?
2. Validation stage: why is validator execution in client_accepted() (late) instead of at the acceptor stage where ClientInfoPdu credentials are parsed (early)? Today this yields different rejection behavior depending on path. If we need both, we may consider having two methods (e.g.: early_validate and late_validate) with a blanket implementation, and let the user implement the one he needs.
3. Sync vs async: should CredentialValidator::validate be async? Most real validators (PAM/LDAP/DB) are blocking/IO-heavy; sync here risks runtime stalls and makes spawn_blocking guidance awkward in practice. And block_on on a separate runtime would also requires spawing a dedicated thread. In all cases, we end up blocking the tokio runtime, which is not a good practice.
4. Trait placement: should this remain a standalone trait, or be folded into ConnectionHandler to avoid parallel extension points?
5. Ownership model: what’s the intended rationale for Arc<dyn CredentialValidator> vs Box<dyn ...>? If cross-server sharing is not a goal, I think Box + internal shared state may be simpler and semantically better.

Suggested path forward (follow-up PR is fine):

• commit to a single credential policy path,
• keep temporary dual support only with explicit, documented precedence,
• align rejection semantics across modes/paths (no divergent “early deny PDU” vs “late disconnect” behavior),
• revisit async shape of the validation hook,
• provide a built-in exact-match validator in ironrdp-server so basic users keep a simple default.

Rough built-in validator sketch:

pub struct ExactMatchCredentialValidator {
    expected: Credentials,
}

impl ExactMatchCredentialValidator {
    pub fn new(expected: Credentials) -> Self {
        Self { expected }
    }
}

impl CredentialValidator for ExactMatchCredentialValidator {
    fn validate(
        &self,
        creds: &Credentials,
    ) -> Result<CredentialDecision, CredentialValidationError> {
        if creds == &self.expected {
            Ok(CredentialDecision::Accept)
        } else {
            Ok(CredentialDecision::Reject)
        }
    }
}

If merging now is necessary, I’m okay with that as long as we track this cleanup explicitly and keep the transition window short.

[Greg Lamberson (glamberson)]

First, thank you for the apology. I'll be honest that this one has been an extremely painful bottleneck for me, which is why I pushed as hard as I did, and at the same time I understand the limitations you are working under and that older PRs get buried under the notification volume. I am not looking to relitigate it.

What I think would help beyond this single PR is a broader and more consistent forum for proactive communication on direction. Coming back through the backlog and starting to write the architecture directions down (#1349, #1352) is exactly that kind of step, and I appreciate it. Keeping that habit going would mean individual PRs stop being the only place these things surface.

To that end I have opened #1357 for the broader frame behind this work (the validate-on-receipt server deployment class), so this PR can stay on the concrete API. Answers to your questions below.

"Two paths" (Q1, Q2 source of truth). I don't think these converge to one mechanism, because they serve different security modes rather than being two ways to do the same thing. set_credentials (acceptor exact-match) and the CredSSP CredentialsProxy are the injection / mechanism path: the server holds the secret and either matches it or runs NLA challenge-response with it. CredentialValidator is the validation-policy path for TLS-mode connections, where credentials arrive in ClientInfoPdu after the channel is up and the server validates them against an external system. CredSSP/NLA structurally cannot do this, since it is pre-session challenge-response and needs a password-equivalent secret in hand, so auth_method=validate with security_mode=hybrid is genuinely incompatible, not a config to reconcile. So rather than deprecating either, I'd frame it as one validation-policy interface alongside the existing pre-loaded-credential path, and I'm in favor of shipping the built-in ExactMatchCredentialValidator you sketched as the default so the simple case stays a one-liner.

Validation stage and rejection semantics (Q2). It runs in client_accepted rather than at the acceptor stage because of the sans-I/O boundary: the acceptor is a synchronous state machine, and real validators (PAM/LDAP/DB) are I/O-bound, so the first place an async validator can run is the server's async layer. I agree the rejection behavior must not diverge, so I'll make the reject path emit a ServerSetErrorInfoPdu(ServerDeniedConnection) before closing, matching the acceptor's exact-match denial, instead of bare-dropping. I'd prefer a single validation entry point over early_validate/late_validate, since early-and-async isn't achievable in the sans-I/O acceptor, but I'm happy to revisit if you see a clean way.

Sync vs async (Q3). Agreed, it should be async. I'll make validate async via #[async_trait], matching RdpServerDisplay/RdpServerDisplayUpdates so it adds no new dependency, and document that implementors backed by blocking systems should offload internally (e.g. spawn_blocking) while native-async backends just .await. The trait names no runtime, so it stays agnostic. If you'd rather avoid the macro I'm glad to hand-roll the boxed future (fn validate(&self, ..) -> Pin<Box<dyn Future<..> + Send + '_>>); same semantics, one-line implementor difference.

Trait placement (Q4). I'd keep it standalone rather than folding into ConnectionHandler: different ownership model (Arc + Send + Sync, shareable) and a different lifecycle (fires once when credentials are received, not across the connection lifecycle), so a single-responsibility trait reads cleaner. No strong objection if you prefer consolidation.

Ownership (Q5). Arc<dyn> rather than Box, precisely because once validate is async the validator is shared into spawned tasks (the offload, or concurrent connections), which needs shared ownership. With Box + internal shared state we'd reintroduce an Arc inside anyway.

I'll push the async change, the denial PDU, and the built-in ExactMatchCredentialValidator, then it should be coherent to merge without a tracked follow-up. Thanks again.

[Benoît Cortier (CBenoit)]

Thanks! This addresses the major technical concerns I raised (async validator shape, aligned rejection behavior, and built-in exact-match validator for the simple case). I’m okay to merge on that basis for velocity.

That said, I still want to dig further on the API/design side. I’m not fully convinced yet on the long-term dual-path model from a user-facing clarity standpoint, even if the internal mechanisms differ by security mode. I would like us to continue that discussion in the issue and make sure we end up with one clear, non-confusing credential-policy story for consumers.

So: merge now, and I’ll follow up in the issue for the deeper design pass.

[Greg Lamberson (glamberson)]

Benoît Cortier (@CBenoit) Sounds good. Thank you.