Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions crates/ironrdp-acceptor/src/connection.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use core::mem;

use ironrdp_connector::sspi::AuthIdentity;
use ironrdp_connector::{
ConnectorError, ConnectorErrorExt as _, ConnectorResult, DesktopSize, Sequence, State, Written, encode_x224_packet,
general_err, reason_err,
Expand Down Expand Up @@ -96,11 +97,11 @@ pub struct AcceptorResult {
/// announce one. Servers can use it to pick a server-side keyboard layout
/// matching the client without changing any local input state.
pub keyboard_layout: u32,
/// Credentials received from the client during SecureSettingsExchange.
/// Credentials received from the client.
///
/// Present for TLS-mode connections where the client sends credentials
/// in the ClientInfoPdu. `None` for CredSSP/Hybrid connections (where
/// authentication happens during the CredSSP exchange instead).
/// in the ClientInfoPdu, and for CredSSP/Hybrid connections once the
/// delegated TSPasswordCreds have been decrypted by CredSSP.
///
/// Servers that need to validate credentials (e.g., via PAM or LDAP)
/// can use this field for post-handshake validation.
Expand Down Expand Up @@ -231,6 +232,17 @@ impl Acceptor {
matches!(self.state, AcceptorState::Credssp { .. })
}

/// Store credentials delegated by CredSSP/NLA so server code can use the
/// same post-handshake validation and binding path as TLS ClientInfo
/// credentials.
pub(crate) fn set_received_credssp_credentials(&mut self, identity: AuthIdentity) {
self.received_credentials = Some(Credentials {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: This copies the delegated secret into Credentials.password: String, which has no zeroize on drop semantic. The server-side cache then holds a clone for the whole TCP-connection lifetime. This is consistent with how TLS ClientInfo credentials are already handled, so it's not a regression, but this PR meaningfully increases how many plaintext passwords sit in Strings and how long they live. If secret hygiene is a goal for the sesman path, this is the moment to consider a zeroizing wrapper. Out of scope for this PR, but it would be a good follow up.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, and I would keep that as a follow-up rather than broadening this PR: this does not introduce a new representation for ClientInfo passwords, but the delegated-credential cache makes the existing secret-hygiene tradeoff more visible.

@piclaw-bot Piclaw (piclaw-bot) Jul 6, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rui pointed out that I had treated this too much as a generic future zeroizing-wrapper topic, and he was right. I made the focused PR fix in rcarmo/IronRDP@a87ea4f1ae66: the server no longer caches/clones authenticated credentials for the TCP connection lifetime. Validation and binding now use the credentials borrowed from the current AcceptorResult, and reactivation without resent credentials does not retain the prior password.

username: identity.username.account_name().to_owned(),
password: identity.password.as_ref().clone(),
domain: identity.username.domain_name().map(str::to_owned),
});
}

/// # Panics
///
/// Panics if state is not [AcceptorState::Credssp].
Expand Down
19 changes: 11 additions & 8 deletions crates/ironrdp-acceptor/src/credssp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,18 +153,19 @@ impl<'a> CredsspSequence<'a> {
&mut self,
result: Result<ServerState, ServerError>,
output: &mut WriteBuf,
) -> ConnectorResult<Written> {
let (ts_request, next_state) = match result {
Ok(ServerState::ReplyNeeded(ts_request)) => (Some(ts_request), CredsspState::Ongoing),
Ok(ServerState::Finished(_id)) => (None, CredsspState::Finished),
) -> ConnectorResult<(Written, Option<AuthIdentity>)> {
let (ts_request, next_state, credentials) = match result {
Ok(ServerState::ReplyNeeded(ts_request)) => (Some(ts_request), CredsspState::Ongoing, None),
Ok(ServerState::Finished(id)) => (None, CredsspState::Finished, Some(id)),
Err(err) => (
err.ts_request.map(|ts_request| *ts_request),
CredsspState::ServerError(err.error),
None,
),
};

self.state = next_state;
if let Some(ts_request) = ts_request {
let written = if let Some(ts_request) = ts_request {
debug!(?ts_request, "Send");
let length = usize::from(ts_request.buffer_len());
let unfilled_buffer = output.unfilled_to(length);
Expand All @@ -175,9 +176,11 @@ impl<'a> CredsspSequence<'a> {

output.advance(length);

Ok(Written::from_size(length)?)
Written::from_size(length)?
} else {
Ok(Written::Nothing)
}
Written::Nothing
};

Ok((written, credentials))
}
}
5 changes: 4 additions & 1 deletion crates/ironrdp-acceptor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,10 @@ where
}; // drop generator

buf.clear();
let written = sequence.handle_process_result(result, buf)?;
let (written, delegated_credentials) = sequence.handle_process_result(result, buf)?;
if let Some(credentials) = delegated_credentials {
acceptor.set_received_credssp_credentials(credentials);
}

if let Some(response_len) = written.size() {
let response = &buf[..response_len];
Expand Down
26 changes: 18 additions & 8 deletions crates/ironrdp-server/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ use super::display::{DesktopSize, RdpServerDisplay};
#[cfg(feature = "egfx")]
use super::gfx::GfxServerFactory;
use super::handler::{KeyboardEvent, MouseEvent, RdpServerInputHandler};
use super::server::{ConnectionHandler, CredentialValidator, RdpServer, RdpServerOptions, RdpServerSecurity};
use super::server::{
ConnectionBinder, ConnectionHandler, CredentialValidator, RdpServer, RdpServerOptions, RdpServerSecurity,
};
use crate::{DisplayUpdate, RdpServerDisplayUpdates, SoundServerFactory};

pub struct WantsAddr {}
Expand All @@ -38,6 +40,7 @@ pub struct BuilderDone {
sound_factory: Option<Box<dyn SoundServerFactory>>,
connection_handler: Option<Box<dyn ConnectionHandler>>,
credential_validator: Option<Arc<dyn CredentialValidator>>,
connection_binder: Option<Arc<dyn ConnectionBinder>>,
#[cfg(feature = "egfx")]
gfx_factory: Option<Box<dyn GfxServerFactory>>,
display_suppressed: Option<Arc<AtomicBool>>,
Expand Down Expand Up @@ -137,6 +140,7 @@ impl RdpServerBuilder<WantsDisplay> {
cliprdr_factory: None,
connection_handler: None,
credential_validator: None,
connection_binder: None,
codecs: server_codecs_capabilities(&[]).expect("can't panic for &[]"),
max_request_size: RdpServerOptions::DEFAULT_MAX_REQUEST_SIZE,
#[cfg(feature = "egfx")]
Expand All @@ -159,6 +163,7 @@ impl RdpServerBuilder<WantsDisplay> {
cliprdr_factory: None,
connection_handler: None,
credential_validator: None,
connection_binder: None,
codecs: server_codecs_capabilities(&[]).expect("can't panic for &[]"),
max_request_size: RdpServerOptions::DEFAULT_MAX_REQUEST_SIZE,
#[cfg(feature = "egfx")]
Expand Down Expand Up @@ -263,21 +268,25 @@ impl RdpServerBuilder<BuilderDone> {
self
}

/// Set a credential validator for TLS-mode connections.
/// Set a credential validator for accepted client credentials.
///
/// When set, credentials received from the client during
/// `SecureSettingsExchange` (`ClientInfoPdu`) are passed to this
/// validator before the session is established. Rejection or a backend
/// When set, credentials surfaced by the acceptor are passed to this
/// validator before the session is established. This includes
/// `SecureSettingsExchange` (`ClientInfoPdu`) credentials and, when
/// available, CredSSP/Hybrid delegated credentials. Rejection or a backend
/// error closes the connection. Pass `None` (the default) to skip
/// validation entirely.
///
/// Not used for CredSSP/Hybrid connections (those use pre-loaded
/// credentials for NTLM challenge-response).
pub fn with_credential_validator(mut self, validator: Option<Arc<dyn CredentialValidator>>) -> Self {
self.state.credential_validator = validator;
self
}

/// Set a binder that replaces display/input handlers after credentials are accepted.
pub fn with_connection_binder(mut self, binder: Option<Arc<dyn ConnectionBinder>>) -> Self {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: The Option param reads oddly at the call site (.with_connection_binder(Some(Arc::new(x)))), but it's consistent with with_credential_validator and with_connection_handler. This is consistent, but I’m wondering if we should change that. Out of scope for this PR.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed; I left this unchanged for this PR to stay consistent with the adjacent optional hooks and avoid mixing API ergonomics into the correctness fixes.

self.state.connection_binder = binder;
Comment on lines +284 to +286

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in rcarmo/IronRDP@4fc52f5bb565: the credential-validator docs now state that accepted credentials surfaced by the acceptor include ClientInfo credentials and, when available, CredSSP/Hybrid delegated credentials.

self
}

/// Inject a shared NetworkAutoDetect RTT handle (milliseconds, `u32::MAX`
/// until the first measurement). The server writes the latest measured RTT
/// to the same instance the backend reads. When not called, the server
Expand Down Expand Up @@ -309,6 +318,7 @@ impl RdpServerBuilder<BuilderDone> {
self.state.autodetect_rtt,
);
server.set_credential_validator(self.state.credential_validator);
server.set_connection_binder(self.state.connection_binder);
server
}
}
Expand Down
6 changes: 3 additions & 3 deletions crates/ironrdp-server/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ pub use handler::{KeyboardEvent, MouseEvent, RdpServerInputHandler};
#[cfg(feature = "helper")]
pub use helper::TlsIdentityCtx;
pub use server::{
ConnectionHandler, CredentialDecision, CredentialValidationError, CredentialValidator, Credentials,
ExactMatchCredentialValidator, PostConnectionAction, RdpServer, RdpServerOptions, RdpServerSecurity, ServerEvent,
ServerEventSender, TransportTls,
BoundConnection, ConnectionBinder, ConnectionHandler, CredentialDecision, CredentialValidationError,
CredentialValidator, Credentials, ExactMatchCredentialValidator, PostConnectionAction, RdpServer, RdpServerOptions,
RdpServerSecurity, ServerEvent, ServerEventSender, TransportTls,
};
pub use sound::{RdpsndServerHandler, RdpsndServerMessage, SoundServerFactory};

Expand Down
Loading