Skip to content

Commit 1f8d501

Browse files
committed
refactor(server)!: typed CredentialValidator API + builder method
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.
1 parent a4a81a1 commit 1f8d501

5 files changed

Lines changed: 201 additions & 20 deletions

File tree

crates/ironrdp-server/src/builder.rs

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
use core::net::SocketAddr;
2+
use std::sync::Arc;
23

34
use anyhow::Result;
45
use ironrdp_pdu::rdp::capability_sets::{BitmapCodecs, server_codecs_capabilities};
@@ -9,7 +10,7 @@ use super::display::{DesktopSize, RdpServerDisplay};
910
#[cfg(feature = "egfx")]
1011
use super::gfx::GfxServerFactory;
1112
use super::handler::{KeyboardEvent, MouseEvent, RdpServerInputHandler};
12-
use super::server::{ConnectionHandler, RdpServer, RdpServerOptions, RdpServerSecurity};
13+
use super::server::{ConnectionHandler, CredentialValidator, RdpServer, RdpServerOptions, RdpServerSecurity};
1314
use crate::{DisplayUpdate, RdpServerDisplayUpdates, SoundServerFactory};
1415

1516
pub struct WantsAddr {}
@@ -35,6 +36,7 @@ pub struct BuilderDone {
3536
cliprdr_factory: Option<Box<dyn CliprdrServerFactory>>,
3637
sound_factory: Option<Box<dyn SoundServerFactory>>,
3738
connection_handler: Option<Box<dyn ConnectionHandler>>,
39+
credential_validator: Option<Arc<dyn CredentialValidator>>,
3840
#[cfg(feature = "egfx")]
3941
gfx_factory: Option<Box<dyn GfxServerFactory>>,
4042
}
@@ -130,6 +132,7 @@ impl RdpServerBuilder<WantsDisplay> {
130132
sound_factory: None,
131133
cliprdr_factory: None,
132134
connection_handler: None,
135+
credential_validator: None,
133136
codecs: server_codecs_capabilities(&[]).expect("can't panic for &[]"),
134137
max_request_size: RdpServerOptions::DEFAULT_MAX_REQUEST_SIZE,
135138
#[cfg(feature = "egfx")]
@@ -148,6 +151,7 @@ impl RdpServerBuilder<WantsDisplay> {
148151
sound_factory: None,
149152
cliprdr_factory: None,
150153
connection_handler: None,
154+
credential_validator: None,
151155
codecs: server_codecs_capabilities(&[]).expect("can't panic for &[]"),
152156
max_request_size: RdpServerOptions::DEFAULT_MAX_REQUEST_SIZE,
153157
#[cfg(feature = "egfx")]
@@ -198,8 +202,23 @@ impl RdpServerBuilder<BuilderDone> {
198202
self
199203
}
200204

205+
/// Set a credential validator for TLS-mode connections.
206+
///
207+
/// When set, credentials received from the client during
208+
/// `SecureSettingsExchange` (`ClientInfoPdu`) are passed to this
209+
/// validator before the session is established. Rejection or a backend
210+
/// error closes the connection. Pass `None` (the default) to skip
211+
/// validation entirely.
212+
///
213+
/// Not used for CredSSP/Hybrid connections (those use pre-loaded
214+
/// credentials for NTLM challenge-response).
215+
pub fn with_credential_validator(mut self, validator: Option<Arc<dyn CredentialValidator>>) -> Self {
216+
self.state.credential_validator = validator;
217+
self
218+
}
219+
201220
pub fn build(self) -> RdpServer {
202-
RdpServer::new(
221+
let mut server = RdpServer::new(
203222
RdpServerOptions {
204223
addr: self.state.addr,
205224
security: self.state.security,
@@ -213,7 +232,9 @@ impl RdpServerBuilder<BuilderDone> {
213232
self.state.connection_handler,
214233
#[cfg(feature = "egfx")]
215234
self.state.gfx_factory,
216-
)
235+
);
236+
server.set_credential_validator(self.state.credential_validator);
237+
server
217238
}
218239
}
219240

crates/ironrdp-server/src/lib.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,8 @@ pub use handler::{KeyboardEvent, MouseEvent, RdpServerInputHandler};
3333
#[cfg(feature = "helper")]
3434
pub use helper::TlsIdentityCtx;
3535
pub use server::{
36-
ConnectionHandler, CredentialValidator, Credentials, PostConnectionAction, RdpServer, RdpServerOptions,
37-
RdpServerSecurity, ServerEvent, ServerEventSender,
36+
ConnectionHandler, CredentialDecision, CredentialValidationError, CredentialValidator, Credentials,
37+
PostConnectionAction, RdpServer, RdpServerOptions, RdpServerSecurity, ServerEvent, ServerEventSender,
3838
};
3939
pub use sound::{RdpsndServerHandler, RdpsndServerMessage, SoundServerFactory};
4040

crates/ironrdp-server/src/server.rs

Lines changed: 104 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
use core::fmt;
12
use core::net::SocketAddr;
23
use core::time::Duration;
34
use std::rc::Rc;
@@ -87,6 +88,55 @@ pub trait ConnectionHandler: Send {
8788
}
8889
}
8990

91+
/// Outcome of a successful [`CredentialValidator::validate`] call.
92+
///
93+
/// A rejection from a working validator is not an error: the validator did
94+
/// its job and decided the credentials do not authenticate. Backend failures
95+
/// (LDAP unreachable, PAM transport broken, database connection lost) are
96+
/// reported via [`CredentialValidationError`] instead.
97+
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
98+
pub enum CredentialDecision {
99+
/// Credentials accepted; the connection proceeds.
100+
Accept,
101+
/// Credentials rejected; the connection is closed.
102+
Reject,
103+
}
104+
105+
/// Error returned by a [`CredentialValidator`] when the validator backend
106+
/// itself fails (rather than the credentials being invalid).
107+
///
108+
/// Wraps any [`core::error::Error`] from the backend (LDAP/PAM/DB/etc.) so
109+
/// the trait does not require a particular error library in implementors or
110+
/// consumers.
111+
#[derive(Debug)]
112+
pub struct CredentialValidationError {
113+
source: Box<dyn core::error::Error + Send + Sync>,
114+
}
115+
116+
impl CredentialValidationError {
117+
/// Wrap a backend error as a credential-validation failure.
118+
pub fn new<E>(source: E) -> Self
119+
where
120+
E: core::error::Error + Send + Sync + 'static,
121+
{
122+
Self {
123+
source: Box::new(source),
124+
}
125+
}
126+
}
127+
128+
impl fmt::Display for CredentialValidationError {
129+
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
130+
f.write_str("credential validator backend failure")
131+
}
132+
}
133+
134+
impl core::error::Error for CredentialValidationError {
135+
fn source(&self) -> Option<&(dyn core::error::Error + 'static)> {
136+
Some(&*self.source)
137+
}
138+
}
139+
90140
/// Server-side credential validator for TLS-mode connections.
91141
///
92142
/// Called during connection setup when the server receives client credentials
@@ -96,10 +146,41 @@ pub trait ConnectionHandler: Send {
96146
/// Implement this trait to validate credentials against external systems
97147
/// (PAM, LDAP, database, etc.). For blocking backends, wrap the call in
98148
/// `tokio::task::spawn_blocking` to avoid stalling the async runtime.
149+
///
150+
/// # Example
151+
///
152+
/// ```ignore
153+
/// use std::sync::Arc;
154+
/// use ironrdp_server::{
155+
/// CredentialDecision, CredentialValidationError, CredentialValidator, Credentials,
156+
/// };
157+
///
158+
/// struct StaticValidator {
159+
/// expected_user: String,
160+
/// expected_password: String,
161+
/// }
162+
///
163+
/// impl CredentialValidator for StaticValidator {
164+
/// fn validate(
165+
/// &self,
166+
/// creds: &Credentials,
167+
/// ) -> Result<CredentialDecision, CredentialValidationError> {
168+
/// if creds.username == self.expected_user && creds.password == self.expected_password {
169+
/// Ok(CredentialDecision::Accept)
170+
/// } else {
171+
/// Ok(CredentialDecision::Reject)
172+
/// }
173+
/// }
174+
/// }
175+
/// ```
99176
pub trait CredentialValidator: Send + Sync {
100177
/// Validate credentials received from the client.
101-
/// Return `Ok(true)` to accept, `Ok(false)` to reject.
102-
fn validate(&self, credentials: &Credentials) -> Result<bool>;
178+
///
179+
/// Return `Ok(CredentialDecision::Accept)` to permit the connection,
180+
/// `Ok(CredentialDecision::Reject)` to refuse it. Return
181+
/// `Err(CredentialValidationError::new(_))` only when the validator
182+
/// itself could not produce a decision (backend system error).
183+
fn validate(&self, credentials: &Credentials) -> Result<CredentialDecision, CredentialValidationError>;
103184
}
104185

105186
#[derive(Clone)]
@@ -387,15 +468,23 @@ impl RdpServer {
387468
builder::RdpServerBuilder::new()
388469
}
389470

390-
/// Set a credential validator for TLS-mode connections.
471+
/// Set or clear the credential validator for TLS-mode connections.
472+
///
473+
/// When set, credentials received from the client during
474+
/// `SecureSettingsExchange` are validated through this callback before
475+
/// the session is established. If the validator returns
476+
/// [`CredentialDecision::Reject`] (or a [`CredentialValidationError`]),
477+
/// the connection is rejected. Passing `None` clears any previously
478+
/// configured validator.
391479
///
392-
/// When set, credentials received from the client during `SecureSettingsExchange`
393-
/// are validated through this callback before the session is established.
394-
/// If validation fails, the connection is rejected.
480+
/// Most callers should configure the validator at construction time via
481+
/// the builder's `with_credential_validator` method
482+
/// ([`RdpServer::builder`]); this setter exists for dynamic
483+
/// post-construction reconfiguration.
395484
///
396485
/// Not used for CredSSP/Hybrid connections (those use pre-loaded credentials).
397-
pub fn set_credential_validator(&mut self, validator: Arc<dyn CredentialValidator>) {
398-
self.credential_validator = Some(validator);
486+
pub fn set_credential_validator(&mut self, validator: Option<Arc<dyn CredentialValidator>>) {
487+
self.credential_validator = validator;
399488
}
400489

401490
pub fn event_sender(&self) -> &mpsc::UnboundedSender<ServerEvent> {
@@ -988,16 +1077,16 @@ impl RdpServer {
9881077
if let Some(validator) = &self.credential_validator {
9891078
if let Some(creds) = &result.credentials {
9901079
match validator.validate(creds) {
991-
Ok(true) => {
992-
debug!("Credential validation succeeded");
1080+
Ok(CredentialDecision::Accept) => {
1081+
debug!("Credential validation accepted");
9931082
}
994-
Ok(false) => {
995-
warn!("Credential validation failed");
996-
bail!("credential validation failed");
1083+
Ok(CredentialDecision::Reject) => {
1084+
warn!("Credential validation rejected");
1085+
bail!("credential validation rejected");
9971086
}
9981087
Err(e) => {
999-
error!(error = %e, "Credential validator error");
1000-
bail!("credential validation error");
1088+
error!(error = %e, "Credential validator backend error");
1089+
bail!("credential validation backend error");
10011090
}
10021091
}
10031092
} else {
Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
use core::fmt;
2+
use std::sync::Arc;
3+
4+
use ironrdp_server::{CredentialDecision, CredentialValidationError, CredentialValidator, Credentials};
5+
6+
fn fixed_creds() -> Credentials {
7+
Credentials {
8+
username: "alice".to_owned(),
9+
password: "hunter2".to_owned(),
10+
domain: None,
11+
}
12+
}
13+
14+
struct AlwaysAccept;
15+
impl CredentialValidator for AlwaysAccept {
16+
fn validate(&self, _: &Credentials) -> Result<CredentialDecision, CredentialValidationError> {
17+
Ok(CredentialDecision::Accept)
18+
}
19+
}
20+
21+
struct AlwaysReject;
22+
impl CredentialValidator for AlwaysReject {
23+
fn validate(&self, _: &Credentials) -> Result<CredentialDecision, CredentialValidationError> {
24+
Ok(CredentialDecision::Reject)
25+
}
26+
}
27+
28+
#[derive(Debug)]
29+
struct BackendDown;
30+
impl fmt::Display for BackendDown {
31+
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
32+
f.write_str("ldap server unreachable")
33+
}
34+
}
35+
impl core::error::Error for BackendDown {}
36+
37+
struct AlwaysBackendError;
38+
impl CredentialValidator for AlwaysBackendError {
39+
fn validate(&self, _: &Credentials) -> Result<CredentialDecision, CredentialValidationError> {
40+
Err(CredentialValidationError::new(BackendDown))
41+
}
42+
}
43+
44+
#[test]
45+
fn validator_accept_returns_accept() {
46+
let v = AlwaysAccept;
47+
assert_eq!(v.validate(&fixed_creds()).unwrap(), CredentialDecision::Accept);
48+
}
49+
50+
#[test]
51+
fn validator_reject_returns_reject() {
52+
let v = AlwaysReject;
53+
assert_eq!(v.validate(&fixed_creds()).unwrap(), CredentialDecision::Reject);
54+
}
55+
56+
#[test]
57+
fn validator_backend_error_propagates_source() {
58+
let v = AlwaysBackendError;
59+
let err = v.validate(&fixed_creds()).expect_err("expected backend error");
60+
assert_eq!(err.to_string(), "credential validator backend failure");
61+
let inner = core::error::Error::source(&err).expect("source must be Some");
62+
assert_eq!(inner.to_string(), "ldap server unreachable");
63+
}
64+
65+
#[test]
66+
fn validator_can_be_held_behind_arc_dyn() {
67+
// Exercises the Send + Sync + 'static bounds the trait promises through Arc<dyn _>.
68+
let v: Arc<dyn CredentialValidator> = Arc::new(AlwaysAccept);
69+
assert_eq!(v.validate(&fixed_creds()).unwrap(), CredentialDecision::Accept);
70+
}
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
11
mod acceptor;
22
mod autodetect;
3+
mod credential_validator;
34
mod fast_path;

0 commit comments

Comments
 (0)