From cb53e28339c21a78bc73921c8e3628728265963b Mon Sep 17 00:00:00 2001 From: irving ou Date: Fri, 15 May 2026 15:51:48 -0400 Subject: [PATCH 1/5] fix(DGW-378): support Kerberos credential injection --- Cargo.lock | 1 + devolutions-gateway/Cargo.toml | 1 + devolutions-gateway/src/api/kdc_proxy.rs | 227 +++-- devolutions-gateway/src/api/preflight.rs | 90 +- devolutions-gateway/src/api/rdp.rs | 4 + devolutions-gateway/src/credential/mod.rs | 147 ++-- devolutions-gateway/src/credential/tests.rs | 131 +++ .../src/credential_injection_kdc.rs | 788 ++++++++++++++++++ devolutions-gateway/src/extract.rs | 47 +- devolutions-gateway/src/generic_client.rs | 65 +- devolutions-gateway/src/lib.rs | 4 + devolutions-gateway/src/listener.rs | 1 + devolutions-gateway/src/ngrok.rs | 1 + devolutions-gateway/src/rd_clean_path.rs | 87 +- devolutions-gateway/src/rdp_proxy.rs | 168 ++-- devolutions-gateway/src/service.rs | 7 + devolutions-gateway/src/token.rs | 67 +- devolutions-gateway/tests/preflight.rs | 113 ++- .../src/AssociationClaims.cs | 2 +- .../src/KdcClaims.cs | 2 +- .../src/ProvisionCredentialsRequest.cs | 57 ++ 21 files changed, 1675 insertions(+), 335 deletions(-) create mode 100644 devolutions-gateway/src/credential/tests.rs create mode 100644 devolutions-gateway/src/credential_injection_kdc.rs create mode 100644 utils/dotnet/Devolutions.Gateway.Utils/src/ProvisionCredentialsRequest.cs diff --git a/Cargo.lock b/Cargo.lock index d8a02aa66..7efc390e9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1618,6 +1618,7 @@ dependencies = [ "axum 0.8.9", "axum-extra", "backoff", + "base64 0.22.1", "bitflags 2.11.1", "bytes 1.11.1", "cadeau", diff --git a/devolutions-gateway/Cargo.toml b/devolutions-gateway/Cargo.toml index a4465f93d..0149d19a6 100644 --- a/devolutions-gateway/Cargo.toml +++ b/devolutions-gateway/Cargo.toml @@ -140,6 +140,7 @@ windows-sys = { version = "0.61", features = ["Win32_Storage_FileSystem", "Win32 embed-resource = "3.0" [dev-dependencies] +base64 = "0.22" tokio-test = "0.4" proptest = "1.7" tempfile = "3" diff --git a/devolutions-gateway/src/api/kdc_proxy.rs b/devolutions-gateway/src/api/kdc_proxy.rs index 664ebc7d4..4c1a5e981 100644 --- a/devolutions-gateway/src/api/kdc_proxy.rs +++ b/devolutions-gateway/src/api/kdc_proxy.rs @@ -1,19 +1,22 @@ use std::io; -use std::net::SocketAddr; use axum::Router; -use axum::extract::{self, ConnectInfo, State}; +use axum::extract::State; use axum::http::StatusCode; use axum::routing::post; -use kdc::handle_kdc_proxy_message; use picky_krb::messages::KdcProxyMessage; use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::net::{TcpStream, UdpSocket}; use crate::DgwState; +use crate::credential_injection_kdc::{ + CredentialInjectionKdc, CredentialInjectionKdcInterception, CredentialInjectionKdcRequest, + CredentialInjectionKdcResolveError, kdc_proxy_message_realm, +}; +use crate::extract::KdcToken; use crate::http::{HttpError, HttpErrorBuilder}; use crate::target_addr::TargetAddr; -use crate::token::{AccessTokenClaims, KdcDestination}; +use crate::token::{KdcDestination, KdcTokenClaims}; pub fn make_router(state: DgwState) -> Router { Router::new().route("/{token}", post(kdc_proxy)).with_state(state) @@ -22,106 +25,148 @@ pub fn make_router(state: DgwState) -> Router { async fn kdc_proxy( State(DgwState { conf_handle, - token_cache, - jrl, - recordings, + credential_store, + kerberos_session_store, .. }): State, - extract::Path(token): extract::Path, - ConnectInfo(source_addr): ConnectInfo, + KdcToken(KdcTokenClaims { destination }): KdcToken, body: axum::body::Bytes, ) -> Result, HttpError> { let conf = conf_handle.get_conf(); - let claims = crate::middleware::auth::authenticate( - source_addr, - &token, - &conf, - &token_cache, - &jrl, - &recordings.active_recordings, - None, - ) - .map_err(HttpError::unauthorized().err())?; - - let AccessTokenClaims::Kdc(claims) = claims else { - return Err(HttpError::forbidden().msg("token not allowed (expected KDC token)")); - }; - let kdc_proxy_message = KdcProxyMessage::from_raw(&body).map_err(HttpError::bad_request().err())?; trace!(?kdc_proxy_message, "Received KDC message"); - debug!( ?kdc_proxy_message.target_domain, ?kdc_proxy_message.dclocator_hint, "KDC message", ); - let realm = if let Some(realm) = &kdc_proxy_message.target_domain.0 { - realm.0.to_string() - } else { - return Err(HttpError::bad_request().msg("realm is missing from KDC request")); - }; - - debug!("Request is for realm (target_domain): {realm}"); + match destination { + KdcDestination::Inject { jti } => { + enforce_credential_injection_enabled(jti, conf.debug.enable_unstable)?; - let (claims_realm, claims_kdc) = match &claims.destination { - KdcDestination::Real { krb_realm, krb_kdc } => (krb_realm, krb_kdc), - KdcDestination::Inject { .. } => { - // TODO(DGW-378): dispatch credential-injection KDC requests to the in-process - // sspi-rs server backed by the credentials provisioned at session establishment. - return Err(HttpError::internal().msg("credential injection KDC dispatch is not implemented yet")); - } - }; + let resolution = CredentialInjectionKdc::resolve(Some(jti), &credential_store, &kerberos_session_store) + .map_err(credential_injection_resolve_error)?; + let kdc = resolution + .ok_or_else(|| HttpError::internal().msg("credential-injection KDC resolution returned no state"))?; - if !claims_realm.eq_ignore_ascii_case(&realm) { - if conf.debug.disable_token_validation { - warn!( - token_realm = %claims_realm, - request_realm = %realm, - "**DEBUG OPTION** Allowed a KDC request towards a KDC whose Kerberos realm differs from what's inside the KDC token" + debug!( + jti = %kdc.jti(), + "Proxy-based credential injection with Kerberos. Processing KdcProxy message internally" ); - } else { - let error_message = format!("expected: {}, got: {}", claims_realm, realm); - return Err(HttpError::bad_request() - .with_msg("requested domain is not allowed") - .err()(error_message)); + match kdc + .handle_kdc_proxy_request(CredentialInjectionKdcRequest::from_token(kdc_proxy_message)) + .map_err(HttpError::internal().err())? + { + CredentialInjectionKdcInterception::Intercepted(reply) => Ok(reply), + CredentialInjectionKdcInterception::NotInjectionRealm(mismatch) => { + Err(HttpError::bad_request() + .with_msg("requested domain is not allowed") + .err()(mismatch)) + } + CredentialInjectionKdcInterception::NotInjectionRequest => { + Err(HttpError::internal().msg("credential-injection KDC did not handle the KDC proxy request")) + } + } + } + KdcDestination::Real { krb_realm, krb_kdc } => { + let envelope_realm = kdc_proxy_message_realm(&kdc_proxy_message); + forward_to_real_kdc( + kdc_proxy_message, + envelope_realm, + &krb_realm, + &krb_kdc, + conf.debug.override_kdc.as_ref(), + conf.debug.disable_token_validation, + ) + .await } } +} - let gateway_id = conf - .id - .ok_or_else(|| HttpError::internal().build("Gateway ID is missing"))?; - if let Some(krb_config) = &conf.debug.kerberos - && realm.eq_ignore_ascii_case(&krb_config.kerberos_server.realm(gateway_id)) - && conf.debug.enable_unstable - { - debug!("Proxy-based credential injection with Kerberos. Processing KdcProxy message internally..."); - - let config = krb_config.kerberos_server.clone().into_kdc_kerberos_config(gateway_id); - let kdc_reply_message = handle_kdc_proxy_message(kdc_proxy_message, &config, &conf.hostname) - .map_err(HttpError::internal().err())?; - - return kdc_reply_message.to_vec().map_err(HttpError::internal().err()); +fn credential_injection_resolve_error(error: CredentialInjectionKdcResolveError) -> HttpError { + match error { + CredentialInjectionKdcResolveError::BuildKdcConfig { .. } => HttpError::internal() + .with_msg("credential-injection KDC could not be initialized") + .build(error), + _ => HttpError::bad_request() + .with_msg("credential-injection state is not available") + .build(error), } +} - let kdc_addr = if let Some(kdc_addr) = &conf.debug.override_kdc { - warn!("**DEBUG OPTION** KDC address has been overridden with {kdc_addr}"); - kdc_addr - } else { - claims_kdc +// Forwards the request to the real KDC indicated by the token (or by the debug override) and +// returns the response wrapped as a `KdcProxyMessage`. +// +// The forward path requires the envelope realm to be set: there is no fallback since this is +// not a credential-injection session. After resolving, validates the realm against the +// token's `krb_realm` claim before forwarding anything. +async fn forward_to_real_kdc( + kdc_proxy_message: KdcProxyMessage, + envelope_realm: Option, + token_realm: &str, + token_kdc_addr: &TargetAddr, + override_kdc: Option<&TargetAddr>, + bypass_realm_check: bool, +) -> Result, HttpError> { + let realm = envelope_realm.ok_or_else(|| HttpError::bad_request().msg("realm is missing from KDC request"))?; + debug!(resolved_realm = %realm, "Forward-to-real-KDC realm resolved"); + enforce_realm_token_match(token_realm, &realm, bypass_realm_check)?; + + let kdc_addr = match override_kdc { + Some(override_addr) => { + warn!(%override_addr, "**DEBUG OPTION** KDC address has been overridden"); + override_addr + } + None => token_kdc_addr, }; - let kdc_reply_message = send_krb_message(kdc_addr, &kdc_proxy_message.kerb_message.0.0).await?; + let kdc_reply_bytes = send_krb_message(kdc_addr, &kdc_proxy_message.kerb_message.0.0).await?; - let kdc_reply_message = KdcProxyMessage::from_raw_kerb_message(&kdc_reply_message) + let reply = KdcProxyMessage::from_raw_kerb_message(&kdc_reply_bytes) .map_err(HttpError::internal().with_msg("couldn't create KDC proxy reply").err())?; - trace!(?kdc_reply_message, "Sending back KDC reply"); + trace!(?reply, "Sending back KDC reply"); + + reply.to_vec().map_err(HttpError::internal().err()) +} + +fn enforce_credential_injection_enabled(jet_cred_id: uuid::Uuid, enable_unstable: bool) -> Result<(), HttpError> { + if enable_unstable { + return Ok(()); + } + + warn!( + %jet_cred_id, + "Credential-injection KDC token rejected because unstable Kerberos injection is disabled" + ); + Err(HttpError::bad_request().msg("credential-injection KDC proxy is not enabled")) +} + +/// Refuses to forward a KDC request whose realm disagrees with the realm the token was issued for. +/// +/// `bypass=true` (only when `__debug__.disable_token_validation` is on) downgrades the mismatch +/// to a warning. Production never opts into this. +fn enforce_realm_token_match(token_realm: &str, request_realm: &str, bypass: bool) -> Result<(), HttpError> { + if token_realm.eq_ignore_ascii_case(request_realm) { + return Ok(()); + } + + if bypass { + warn!( + %token_realm, + %request_realm, + "**DEBUG OPTION** Allowed a KDC request towards a KDC whose Kerberos realm differs from what's inside the KDC token" + ); + return Ok(()); + } - kdc_reply_message.to_vec().map_err(HttpError::internal().err()) + Err(HttpError::bad_request() + .with_msg("requested domain is not allowed") + .err()(format!("expected: {token_realm}, got: {request_realm}"))) } async fn read_kdc_reply_message(connection: &mut TcpStream) -> io::Result> { @@ -221,3 +266,37 @@ pub async fn send_krb_message(kdc_addr: &TargetAddr, message: &[u8]) -> Result, _scope: PreflightScope, @@ -223,12 +225,21 @@ pub(super) async fn post_preflight( let conf = conf_handle.get_conf(); let sessions = sessions.clone(); let credential_store = credential_store.clone(); + let kerberos_session_store = kerberos_session_store.clone(); async move { let operation_id = operation.id; trace!(%operation.id, "Process preflight operation"); - if let Err(error) = handle_operation(operation, &outputs, &conf, &sessions, &credential_store).await + if let Err(error) = handle_operation( + operation, + &outputs, + &conf, + &sessions, + &credential_store, + &kerberos_session_store, + ) + .await { outputs.push(PreflightOutput { operation_id, @@ -257,6 +268,7 @@ async fn handle_operation( conf: &Conf, sessions: &SessionMessageSender, credential_store: &CredentialStoreHandle, + kerberos_session_store: &CredentialInjectionKdcSessionStoreHandle, ) -> Result<(), PreflightError> { match operation.kind.as_str() { OP_GET_VERSION => outputs.push(PreflightOutput { @@ -337,17 +349,77 @@ async fn handle_operation( }); } + // Token signature isn't validated at the preflight scope (DVLS signs and pushes; + // gateway only re-uses what DVLS already verified). We do parse JTI / dst_hst here + // because the credential store no longer touches JWT shape — see the contract on + // `CredentialStoreHandle::insert`. + let jti = crate::token::extract_jti(&token).map_err(|error| { + PreflightError::new(PreflightAlertStatus::InvalidParams, format!("invalid token: {error:#}")) + })?; + + // For `provision-credentials`: resolve `target_hostname` from `dst_hst` *before* + // calling either store, so an invalid target is reported as `invalid-parameters` + // at preflight rather than mid-CredSSP. Also derive per-session Kerberos material + // from the proxy username; both stores receive their entries paired and keyed by + // the same JTI. + let injection_payload = if let Some(mapping) = mapping { + const DEFAULT_DST_PORT: u16 = 3389; + let raw_dst_hst = crate::token::extract_dst_hst(&token) + .map_err(|error| { + PreflightError::new( + PreflightAlertStatus::InvalidParams, + format!("read dst_hst from association token: {error:#}"), + ) + })? + .ok_or_else(|| { + PreflightError::new( + PreflightAlertStatus::InvalidParams, + "association token has no dst_hst, required for credential injection", + ) + })?; + + let dst_alt = crate::token::extract_dst_alt(&token).map_err(|error| { + PreflightError::new( + PreflightAlertStatus::InvalidParams, + format!("read dst_alt from association token: {error:#}"), + ) + })?; + if !dst_alt.is_empty() { + return Err(PreflightError::new( + PreflightAlertStatus::InvalidParams, + "association token dst_alt is not supported for credential injection", + )); + } + + let target_hostname = crate::target_addr::TargetAddr::parse(&raw_dst_hst, DEFAULT_DST_PORT) + .map_err(|error| { + PreflightError::new( + PreflightAlertStatus::InvalidParams, + format!("parse dst_hst as target address: {error:#}"), + ) + })? + .host() + .to_owned(); + + let kdc_session = crate::credential_injection_kdc::derive_credential_injection_kdc_session( + mapping.proxy_username(), + jti, + ); + kerberos_session_store.insert(kdc_session, time_to_live); + + Some((mapping, target_hostname)) + } else { + None + }; + let previous_entry = credential_store - .insert(token, mapping, time_to_live) + .insert(jti, token, injection_payload, time_to_live) .inspect_err(|error| warn!(%operation.id, error = format!("{error:#}"), "Failed to insert credentials")) - .map_err(|e| match e { - InsertError::InvalidToken(_) => { - PreflightError::new(PreflightAlertStatus::InvalidParams, format!("{e:#}")) - } - InsertError::Internal(_) => PreflightError::new( + .map_err(|_| { + PreflightError::new( PreflightAlertStatus::InternalServerError, "an internal error occurred".to_owned(), - ), + ) })?; if previous_entry.is_some() { diff --git a/devolutions-gateway/src/api/rdp.rs b/devolutions-gateway/src/api/rdp.rs index 6129a776c..b684a28d5 100644 --- a/devolutions-gateway/src/api/rdp.rs +++ b/devolutions-gateway/src/api/rdp.rs @@ -26,6 +26,7 @@ pub async fn handler( recordings, shutdown_signal, credential_store, + kerberos_session_store, agent_tunnel_handle, .. }): State, @@ -47,6 +48,7 @@ pub async fn handler( recordings.active_recordings, source_addr, credential_store, + kerberos_session_store, agent_tunnel_handle, ) .instrument(span) @@ -67,6 +69,7 @@ async fn handle_socket( active_recordings: Arc, source_addr: SocketAddr, credential_store: crate::credential::CredentialStoreHandle, + kerberos_session_store: crate::credential_injection_kdc::CredentialInjectionKdcSessionStoreHandle, agent_tunnel_handle: Option>, ) { let (stream, close_handle) = crate::ws::handle( @@ -85,6 +88,7 @@ async fn handle_socket( subscriber_tx, &active_recordings, &credential_store, + &kerberos_session_store, agent_tunnel_handle, ) .await; diff --git a/devolutions-gateway/src/credential/mod.rs b/devolutions-gateway/src/credential/mod.rs index 166be9412..fea6829d6 100644 --- a/devolutions-gateway/src/credential/mod.rs +++ b/devolutions-gateway/src/credential/mod.rs @@ -4,40 +4,16 @@ mod crypto; pub use crypto::EncryptedPassword; use std::collections::HashMap; -use std::fmt; use std::sync::Arc; -use anyhow::Context; use async_trait::async_trait; use devolutions_gateway_task::{ShutdownSignal, Task}; use parking_lot::Mutex; -use secrecy::ExposeSecret as _; +use secrecy::{ExposeSecret as _, SecretString}; use uuid::Uuid; use self::crypto::MASTER_KEY; -/// Error returned by [`CredentialStoreHandle::insert`]. -#[derive(Debug)] -pub enum InsertError { - /// The provided token is invalid (e.g., missing or malformed JTI). - /// - /// This is a client-side error: the caller supplied bad input. - InvalidToken(anyhow::Error), - /// An internal error occurred (e.g., encryption failure). - Internal(anyhow::Error), -} - -impl fmt::Display for InsertError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::InvalidToken(e) => e.fmt(f), - Self::Internal(e) => e.fmt(f), - } - } -} - -impl std::error::Error for InsertError {} - /// Credential at the application protocol level #[derive(Debug, Clone)] pub enum AppCredential { @@ -48,10 +24,16 @@ pub enum AppCredential { } impl AppCredential { + pub(crate) fn username(&self) -> &str { + match self { + AppCredential::UsernamePassword { username, password: _ } => username, + } + } + /// Decrypt the password using the global master key. /// /// Returns the username and a short-lived decrypted password that zeroizes on drop. - pub fn decrypt_password(&self) -> anyhow::Result<(String, secrecy::SecretString)> { + pub fn decrypt_password(&self) -> anyhow::Result<(String, SecretString)> { match self { AppCredential::UsernamePassword { username, password } => { let decrypted = MASTER_KEY.lock().decrypt(password)?; @@ -74,12 +56,9 @@ pub struct AppCredentialMapping { /// This type is never stored directly — hand it to [`CredentialStoreHandle::insert`]. #[derive(Debug, Deserialize)] #[serde(tag = "kind")] -pub enum CleartextAppCredential { +pub(crate) enum CleartextAppCredential { #[serde(rename = "username-password")] - UsernamePassword { - username: String, - password: secrecy::SecretString, - }, + UsernamePassword { username: String, password: SecretString }, } impl CleartextAppCredential { @@ -100,20 +79,30 @@ impl CleartextAppCredential { /// /// Passwords are encrypted on write. Hand this directly to [`CredentialStoreHandle::insert`]. #[derive(Debug, Deserialize)] -pub struct CleartextAppCredentialMapping { +pub(crate) struct CleartextAppCredentialMapping { #[serde(rename = "proxy_credential")] - pub proxy: CleartextAppCredential, + pub(crate) proxy: CleartextAppCredential, #[serde(rename = "target_credential")] - pub target: CleartextAppCredential, + pub(crate) target: CleartextAppCredential, } impl CleartextAppCredentialMapping { - fn encrypt(self) -> anyhow::Result { + pub(crate) fn encrypt(self) -> anyhow::Result { Ok(AppCredentialMapping { proxy: self.proxy.encrypt()?, target: self.target.encrypt()?, }) } + + /// Expose the proxy-side username without dropping ownership of the rest of the mapping. + /// + /// Used by the preflight layer to derive per-session Kerberos material before the mapping + /// is moved into the credential store. The password is not exposed. + pub(crate) fn proxy_username(&self) -> &str { + match &self.proxy { + CleartextAppCredential::UsernamePassword { username, password: _ } => username, + } + } } #[derive(Debug, Clone)] @@ -130,21 +119,32 @@ impl CredentialStoreHandle { Self(Arc::new(Mutex::new(CredentialStore::new()))) } - pub fn insert( + /// Insert a credential entry for the association token whose JTI is `jti`. + /// + /// The caller is responsible for extracting `jti` from a token whose signature has already + /// been validated upstream. `injection` carries the optional `provision-credentials` payload: + /// the cleartext mapping and the parsed RDP target hostname. The store has no other + /// dependency on the token's JWT shape. + pub(crate) fn insert( &self, + jti: Uuid, token: String, - mapping: Option, + injection: Option<(CleartextAppCredentialMapping, String)>, time_to_live: time::Duration, - ) -> Result, InsertError> { - let mapping = mapping - .map(CleartextAppCredentialMapping::encrypt) - .transpose() - .map_err(InsertError::Internal)?; - self.0.lock().insert(token, mapping, time_to_live) + ) -> anyhow::Result> { + let injection = injection + .map(|(mapping, target_hostname)| -> anyhow::Result { + Ok(InjectionState { + mapping: mapping.encrypt()?, + target_hostname, + }) + }) + .transpose()?; + Ok(self.0.lock().insert(jti, token, injection, time_to_live)) } - pub fn get(&self, token_id: Uuid) -> Option { - self.0.lock().get(token_id) + pub(crate) fn get(&self, jti: Uuid) -> Option { + self.0.lock().get(jti) } } @@ -154,13 +154,31 @@ struct CredentialStore { } #[derive(Debug)] -pub struct CredentialEntry { - pub token: String, - pub mapping: Option, - pub expires_at: time::OffsetDateTime, +pub(crate) struct CredentialEntry { + /// The association token's JTI. Doubles as the credential-store key and as the value the + /// matching KDC token's `jet_cred_id` claim points back to. + pub(crate) jti: Uuid, + pub(crate) token: String, + pub(crate) expires_at: time::OffsetDateTime, + /// Credential-injection state for this entry, set when (and only when) the entry was + /// provisioned via `provision-credentials`. Plain `provision-token` entries leave this `None` + /// and are inert from the routing layer's perspective. Grouping the three correlated fields + /// into one option captures the "all set or all unset" invariant in the type system. + pub(crate) injection: Option, +} + +#[derive(Debug)] +pub(crate) struct InjectionState { + pub(crate) mapping: AppCredentialMapping, + /// Hostname of the target RDP server, parsed from the association token's `dst_hst` claim + /// at preflight. Fake-KDC validates client TGS-REQ sname against `TERMSRV/`; + /// without it the SPN check would silently fall back to Gateway's own hostname, so the + /// preflight handler rejects `provision-credentials` requests with missing/malformed `dst_hst` + /// or with alternate targets (`dst_alt`) until credential injection supports target failover. + pub(crate) target_hostname: String, } -pub type ArcCredentialEntry = Arc; +pub(crate) type ArcCredentialEntry = Arc; impl CredentialStore { fn new() -> Self { @@ -171,27 +189,29 @@ impl CredentialStore { fn insert( &mut self, + jti: Uuid, token: String, - mapping: Option, + injection: Option, time_to_live: time::Duration, - ) -> Result, InsertError> { - let jti = crate::token::extract_jti(&token) - .context("failed to extract token ID") - .map_err(InsertError::InvalidToken)?; - + ) -> Option { let entry = CredentialEntry { + jti, token, - mapping, expires_at: time::OffsetDateTime::now_utc() + time_to_live, + injection, }; - let previous_entry = self.entries.insert(jti, Arc::new(entry)); - - Ok(previous_entry) + self.entries.insert(jti, Arc::new(entry)) } - fn get(&self, token_id: Uuid) -> Option { - self.entries.get(&token_id).map(Arc::clone) + fn get(&self, jti: Uuid) -> Option { + // Filter expired entries at lookup. The 15-minute cleanup task is best-effort; + // without this filter an expired entry remains usable until the next sweep, which + // makes `time_to_live` a soft hint rather than a hard limit. + self.entries + .get(&jti) + .filter(|entry| time::OffsetDateTime::now_utc() < entry.expires_at) + .map(Arc::clone) } } @@ -234,3 +254,6 @@ async fn cleanup_task(handle: CredentialStoreHandle, mut shutdown_signal: Shutdo debug!("Task terminated"); } + +#[cfg(test)] +mod tests; diff --git a/devolutions-gateway/src/credential/tests.rs b/devolutions-gateway/src/credential/tests.rs new file mode 100644 index 000000000..f970db617 --- /dev/null +++ b/devolutions-gateway/src/credential/tests.rs @@ -0,0 +1,131 @@ +use super::*; + +fn cleartext_mapping(proxy_username: &str) -> CleartextAppCredentialMapping { + CleartextAppCredentialMapping { + proxy: CleartextAppCredential::UsernamePassword { + username: proxy_username.to_owned(), + password: SecretString::from("proxy-password"), + }, + target: CleartextAppCredential::UsernamePassword { + username: "target@example.invalid".to_owned(), + password: SecretString::from("target-password"), + }, + } +} + +#[test] +fn insert_indexes_by_jti() { + let store = CredentialStoreHandle::new(); + let jti = Uuid::new_v4(); + + let previous = store + .insert( + jti, + "raw-token".to_owned(), + Some((cleartext_mapping("proxy@example.invalid"), "target.example".to_owned())), + time::Duration::minutes(5), + ) + .expect("insert succeeds"); + + assert!(previous.is_none()); + let entry = store.get(jti).expect("entry is indexed by JTI"); + assert_eq!(entry.jti, jti); + assert_eq!(entry.token, "raw-token"); + assert_eq!( + entry + .injection + .as_ref() + .expect("injection state present") + .target_hostname, + "target.example" + ); +} + +#[test] +fn re_insert_under_same_jti_replaces_entry() { + let store = CredentialStoreHandle::new(); + let jti = Uuid::new_v4(); + + store + .insert( + jti, + "token-a".to_owned(), + Some((cleartext_mapping("proxy@example.invalid"), "host-a".to_owned())), + time::Duration::minutes(5), + ) + .expect("first insert succeeds"); + let previous = store + .insert( + jti, + "token-b".to_owned(), + Some((cleartext_mapping("proxy@example.invalid"), "host-b".to_owned())), + time::Duration::minutes(5), + ) + .expect("second insert succeeds"); + + let previous = previous.expect("replacement must report previous entry"); + assert_eq!(previous.jti, jti); + assert_eq!(previous.token, "token-a"); + + let current = store.get(jti).expect("replacement entry present"); + assert_eq!(current.token, "token-b"); +} + +#[test] +fn distinct_jtis_coexist() { + let store = CredentialStoreHandle::new(); + let first_jti = Uuid::new_v4(); + let second_jti = Uuid::new_v4(); + + store + .insert( + first_jti, + "token-1".to_owned(), + Some((cleartext_mapping("proxy@example.invalid"), "h1".to_owned())), + time::Duration::minutes(5), + ) + .expect("first insert succeeds"); + store + .insert( + second_jti, + "token-2".to_owned(), + Some((cleartext_mapping("proxy@example.invalid"), "h2".to_owned())), + time::Duration::minutes(5), + ) + .expect("second insert succeeds"); + + assert_eq!(store.get(first_jti).expect("first entry").jti, first_jti); + assert_eq!(store.get(second_jti).expect("second entry").jti, second_jti); +} + +#[test] +fn lookup_filters_expired_entries() { + let store = CredentialStoreHandle::new(); + let jti = Uuid::new_v4(); + + store + .insert( + jti, + "raw-token".to_owned(), + Some((cleartext_mapping("proxy@example.invalid"), "host".to_owned())), + // Negative TTL: entry is born already expired. Validates the lookup-time filter + // without depending on real elapsed time in tests. + time::Duration::seconds(-1), + ) + .expect("insert succeeds"); + + assert!(store.get(jti).is_none(), "expired entry must not be returned"); +} + +#[test] +fn provision_token_entry_has_no_injection_state() { + let store = CredentialStoreHandle::new(); + let jti = Uuid::new_v4(); + + store + .insert(jti, "raw-token".to_owned(), None, time::Duration::minutes(5)) + .expect("insert succeeds"); + + let entry = store.get(jti).expect("entry exists"); + assert!(entry.injection.is_none()); +} diff --git a/devolutions-gateway/src/credential_injection_kdc.rs b/devolutions-gateway/src/credential_injection_kdc.rs new file mode 100644 index 000000000..d762a24a6 --- /dev/null +++ b/devolutions-gateway/src/credential_injection_kdc.rs @@ -0,0 +1,788 @@ +//! In-memory Kerberos KDC used by proxy-based credential injection. +//! +//! This module owns the Kerberos side of credential injection end-to-end: +//! per-session fake-KDC material, the session store, KDC proxy handling, and the +//! in-process KDC requests emitted by the server-side CredSSP acceptor. +//! Callers should only decide whether credential injection applies; once it does, this +//! component owns the Kerberos-specific behavior. + +use std::collections::HashMap; +use std::fmt; +use std::net::SocketAddr; +use std::sync::Arc; +use std::time::Duration; + +use anyhow::Context as _; +use async_trait::async_trait; +use chacha20poly1305::aead::OsRng; +use chacha20poly1305::aead::rand_core::RngCore as _; +use devolutions_gateway_task::{ShutdownSignal, Task}; +use ironrdp_connector::sspi; +use ironrdp_connector::sspi::generator::NetworkRequest; +use parking_lot::Mutex; +use picky_krb::messages::KdcProxyMessage; +use secrecy::{ExposeSecret as _, SecretBox, SecretString}; +use thiserror::Error; +use url::Url; +use uuid::Uuid; + +use crate::credential::{ + AppCredential, AppCredentialMapping, ArcCredentialEntry, CredentialStoreHandle, InjectionState, +}; + +const IN_PROCESS_KDC_HOST: &str = "cred.invalid"; + +pub(crate) struct CredentialInjectionKdc { + jti: Uuid, + raw_token: String, + credential_mapping: AppCredentialMapping, + target_hostname: String, + session: Arc, + // The KDC crate models users with plaintext passwords, so this object owns those secrets + // for the lifetime of the credential-injection KDC. Keep Debug redacted. + kdc_config: kdc::config::KerberosServer, +} + +pub(crate) type CredentialInjectionKdcResolution = Option>; + +#[derive(Debug, Error)] +pub(crate) enum CredentialInjectionKdcResolveError { + #[error("credential-injection state is not available for {jti}")] + MissingCredential { jti: Uuid }, + #[error("credential-injection state is not available for {jti}")] + NonInjectionCredential { jti: Uuid }, + #[error("credential-injection Kerberos state is not available for {jti}")] + MissingKerberosSession { jti: Uuid }, + #[error("credential-injection KDC config could not be initialized for {jti}")] + BuildKdcConfig { + jti: Uuid, + #[source] + source: anyhow::Error, + }, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct RealmMismatch { + pub(crate) expected: String, + pub(crate) actual: String, +} + +impl fmt::Display for RealmMismatch { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "expected: {}, got: {}", self.expected, self.actual) + } +} + +impl std::error::Error for RealmMismatch {} + +#[derive(Debug)] +pub(crate) enum CredentialInjectionKdcInterception { + Intercepted(Vec), + NotInjectionRequest, + NotInjectionRealm(RealmMismatch), +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum CredentialInjectionClientAcceptorProtocol { + Kerberos, + Ntlm, +} + +pub(crate) struct CredentialInjectionKdcRequest { + message: KdcProxyMessage, +} + +impl CredentialInjectionKdcRequest { + pub(crate) fn from_token(message: KdcProxyMessage) -> Self { + Self { message } + } + + fn in_process(message: KdcProxyMessage) -> Self { + Self { message } + } +} + +impl fmt::Debug for CredentialInjectionKdc { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("CredentialInjectionKdc") + .field("jti", &self.jti) + .field("target_hostname", &self.target_hostname) + .field("realm", &self.session.realm) + .field("kdc_config", &"") + .finish() + } +} + +impl CredentialInjectionKdc { + pub(crate) fn from_parts( + credential_entry: ArcCredentialEntry, + session: Arc, + ) -> anyhow::Result { + let InjectionState { + mapping, + target_hostname, + } = credential_entry + .injection + .as_ref() + .context("credential entry has no credential-injection state")?; + anyhow::ensure!( + credential_entry.jti == session.jti, + "credential entry JTI does not match credential-injection KDC session JTI", + ); + + let kdc_config = build_kdc_config(&session, &mapping.proxy)?; + + Ok(Self { + jti: credential_entry.jti, + raw_token: credential_entry.token.clone(), + credential_mapping: mapping.clone(), + target_hostname: target_hostname.clone(), + session, + kdc_config, + }) + } + + pub(crate) fn resolve( + jet_cred_id: Option, + credential_store: &CredentialStoreHandle, + session_store: &CredentialInjectionKdcSessionStoreHandle, + ) -> Result { + let Some(jti) = jet_cred_id else { + return Ok(None); + }; + + let credential_entry = credential_store.get(jti).ok_or_else(|| { + warn!(%jti, "KDC token references missing credential-injection state"); + CredentialInjectionKdcResolveError::MissingCredential { jti } + })?; + + if credential_entry.injection.is_none() { + warn!(%jti, "KDC token references non-injection credential state"); + return Err(CredentialInjectionKdcResolveError::NonInjectionCredential { jti }); + } + + let session = session_store.get(jti).ok_or_else(|| { + warn!(%jti, "KDC token references missing credential-injection Kerberos state"); + CredentialInjectionKdcResolveError::MissingKerberosSession { jti } + })?; + + let kdc = Self::from_parts(credential_entry, session) + .map_err(|source| CredentialInjectionKdcResolveError::BuildKdcConfig { jti, source })?; + + Ok(Some(Box::new(kdc))) + } + + pub(crate) fn jti(&self) -> Uuid { + self.jti + } + + pub(crate) fn raw_token(&self) -> &str { + &self.raw_token + } + + pub(crate) fn proxy_credential(&self) -> &AppCredential { + &self.credential_mapping.proxy + } + + pub(crate) fn target_credential(&self) -> &AppCredential { + &self.credential_mapping.target + } + + /// Selects the CredSSP acceptor backend Gateway should present to the RDP client. + /// + /// The acceptor side must mirror the target-side auth package. + /// Domainless target credentials cannot acquire Kerberos tickets. + /// Enabling the Kerberos acceptor for those sessions would make incoming NTLMSSP tokens fail in Kerberos parsing. + pub(crate) fn client_acceptor_protocol(&self) -> anyhow::Result { + let target_username = + sspi::Username::parse(self.target_credential().username()).context("invalid target credential username")?; + + if target_username.domain_name().is_some() { + Ok(CredentialInjectionClientAcceptorProtocol::Kerberos) + } else { + Ok(CredentialInjectionClientAcceptorProtocol::Ntlm) + } + } + + pub(crate) fn server_kerberos_config(&self, client_addr: SocketAddr) -> anyhow::Result { + let user = sspi::CredentialsBuffers::AuthIdentity(sspi::AuthIdentityBuffers::from_utf8( + &self.session.acceptor.principal_name, + &self.session.realm, + self.session.acceptor.password.expose_secret(), + )); + + let kdc_url = self.in_process_kdc_url()?; + + // The SPN that the client puts on its AP-REQ ticket is the one for the target RDP + // server (`TERMSRV/`). Gateway-as-CredSSP-server is impersonating that target, + // so ServerProperties must claim the same SPN or sspi-rs rejects the ticket. + Ok(sspi::KerberosServerConfig { + kerberos_config: sspi::KerberosConfig { + kdc_url: Some(kdc_url), + client_computer_name: Some(client_addr.to_string()), + }, + server_properties: sspi::kerberos::ServerProperties::new( + &["TERMSRV", &self.target_hostname], + Some(user), + Duration::from_secs(300), + Some(self.session.acceptor.long_term_key.expose_secret().clone()), + )?, + }) + } + + pub(crate) fn intercept_network_request( + &self, + request: &NetworkRequest, + ) -> anyhow::Result { + if request.url.host_str() != Some(IN_PROCESS_KDC_HOST) { + return Ok(CredentialInjectionKdcInterception::NotInjectionRequest); + } + + let url_jti = request + .url + .path() + .trim_start_matches('/') + .parse::() + .context("malformed in-process KDC URL")?; + anyhow::ensure!( + url_jti == self.jti, + "in-process KDC URL JTI does not match current CredSSP session", + ); + + debug!( + jti = %self.jti, + scheme = %request.url.scheme(), + "Credential-injection KDC intercepted in-process request" + ); + + let kdc_message = KdcProxyMessage::from_raw(&request.data).context("malformed in-process KDC proxy payload")?; + self.handle_kdc_proxy_request(CredentialInjectionKdcRequest::in_process(kdc_message)) + } + + pub(crate) fn handle_kdc_proxy_request( + &self, + request: CredentialInjectionKdcRequest, + ) -> anyhow::Result { + let request_realm = self.resolve_message_realm(&request.message); + debug!( + jti = %self.jti, + resolved_realm = %request_realm, + "Credential-injection KDC realm resolved" + ); + + if let Some(mismatch) = realm_mismatch(&self.session.realm, &request_realm) { + return Ok(CredentialInjectionKdcInterception::NotInjectionRealm(mismatch)); + } + + let reply = self.handle_message(request.message)?; + Ok(CredentialInjectionKdcInterception::Intercepted(reply)) + } + + fn in_process_kdc_url(&self) -> anyhow::Result { + Url::parse(&format!("http://{}/{}", IN_PROCESS_KDC_HOST, self.jti)).context("build in-process KDC URL") + } + + fn resolve_message_realm(&self, kdc_proxy_message: &KdcProxyMessage) -> String { + kdc_proxy_message_realm(kdc_proxy_message).unwrap_or_else(|| self.session.realm.clone()) + } + + fn handle_message(&self, kdc_proxy_message: KdcProxyMessage) -> anyhow::Result> { + let reply = kdc::handle_kdc_proxy_message(kdc_proxy_message, &self.kdc_config, &self.target_hostname) + .context("handle credential-injection KDC message")?; + + reply.to_vec().context("encode credential-injection KDC reply") + } +} + +pub(crate) fn kdc_proxy_message_realm(kdc_proxy_message: &KdcProxyMessage) -> Option { + kdc_proxy_message + .target_domain + .0 + .as_ref() + .map(|realm| realm.0.to_string()) + .filter(|realm| !realm.is_empty()) +} + +fn realm_mismatch(expected: &str, actual: &str) -> Option { + if expected.eq_ignore_ascii_case(actual) { + return None; + } + + Some(RealmMismatch { + expected: expected.to_owned(), + actual: actual.to_owned(), + }) +} + +/// Per-session Kerberos material for proxy-based credential injection. +/// +/// The key material and the acceptor PA-ENC-TIMESTAMP password are wrapped in [`SecretBox`] / +/// [`SecretString`] so they cannot be accidentally written to logs through structured tracing. +/// Access requires an explicit `expose_secret()` call, which is greppable and reviewable. +pub(crate) struct CredentialInjectionKdcSession { + jti: Uuid, + pub(crate) realm: String, + kdc: CredentialInjectionKdcState, + acceptor: CredentialInjectionAcceptorState, +} + +struct CredentialInjectionKdcState { + krbtgt_key: SecretBox>, +} + +struct CredentialInjectionAcceptorState { + principal_name: String, + password: SecretString, + long_term_key: SecretBox>, +} + +impl fmt::Debug for CredentialInjectionKdcSession { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("CredentialInjectionKdcSession") + .field("jti", &self.jti) + .field("realm", &self.realm) + .field("kdc", &self.kdc) + .field("acceptor", &self.acceptor) + .finish() + } +} + +impl fmt::Debug for CredentialInjectionKdcState { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("CredentialInjectionKdcState") + .field("krbtgt_key", &"<32 bytes redacted>") + .finish() + } +} + +impl fmt::Debug for CredentialInjectionAcceptorState { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("CredentialInjectionAcceptorState") + .field("principal_name", &self.principal_name) + .field("password", &"") + .field("long_term_key", &"<32 bytes redacted>") + .finish() + } +} + +/// Derive per-session Kerberos material from the proxy username and the association token's JTI. +/// +/// The proxy username's optional `@realm` suffix selects the realm DVLS supplied; otherwise +/// fall back to a per-session synthetic realm derived from the JTI. The two sides agree +/// because DVLS derives the synthetic value the same way. +pub(crate) fn derive_credential_injection_kdc_session( + proxy_username: &str, + jti: Uuid, +) -> CredentialInjectionKdcSession { + let realm = proxy_username + .split_once('@') + .map(|(_, realm)| realm) + .filter(|realm| !realm.is_empty()) + .map(str::to_owned) + .unwrap_or_else(|| synthetic_realm(jti)); + + CredentialInjectionKdcSession { + jti, + realm, + kdc: CredentialInjectionKdcState { + krbtgt_key: SecretBox::new(Box::new(random_32_bytes())), + }, + acceptor: CredentialInjectionAcceptorState { + principal_name: "jet".to_owned(), + password: SecretString::from(hex::encode(random_32_bytes())), + long_term_key: SecretBox::new(Box::new(random_32_bytes())), + }, + } +} + +fn build_kdc_config( + session: &CredentialInjectionKdcSession, + proxy_credential: &AppCredential, +) -> anyhow::Result { + let realm = &session.realm; + let (proxy_user_name, proxy_password) = proxy_credential.decrypt_password()?; + let proxy_user_name = principal_for_realm(&proxy_user_name, realm); + let acceptor_principal_name = principal_for_realm(&session.acceptor.principal_name, realm); + + let acceptor_password = session.acceptor.password.expose_secret().to_owned(); + Ok(kdc::config::KerberosServer { + realm: realm.to_owned(), + users: vec![ + kdc::config::DomainUser { + username: proxy_user_name.clone(), + password: proxy_password.expose_secret().to_owned(), + salt: kerberos_salt(realm, &proxy_user_name), + }, + kdc::config::DomainUser { + username: acceptor_principal_name.clone(), + password: acceptor_password.clone(), + salt: kerberos_salt(realm, &acceptor_principal_name), + }, + ], + max_time_skew: 300, + krbtgt_key: session.kdc.krbtgt_key.expose_secret().clone(), + ticket_decryption_key: Some(session.acceptor.long_term_key.expose_secret().clone()), + service_user: Some(kdc::config::DomainUser { + username: acceptor_principal_name.clone(), + password: acceptor_password, + salt: kerberos_salt(realm, &acceptor_principal_name), + }), + }) +} + +fn principal_for_realm(user_name: &str, realm: &str) -> String { + if user_name.contains('@') { + user_name.to_owned() + } else { + format!("{user_name}@{realm}") + } +} + +fn kerberos_salt(realm: &str, principal: &str) -> String { + let local_name = principal.split('@').next().unwrap_or(principal); + format!("{}{local_name}", realm.to_ascii_uppercase()) +} + +pub(crate) fn synthetic_realm(jti: Uuid) -> String { + format!("CRED-{}.INVALID", jti.simple()).to_ascii_uppercase() +} + +fn random_32_bytes() -> Vec { + let mut bytes = vec![0u8; 32]; + OsRng.fill_bytes(&mut bytes); + bytes +} + +/// Parallel store of [`CredentialInjectionKdcSession`] entries keyed by association-token JTI. +/// +/// Pairs 1:1 with [`crate::credential::CredentialStoreHandle`]: a credential entry provisioned +/// with `provision-credentials` has a matching entry here under the same JTI. The two stores +/// share neither lock nor map so that the credential store stays Kerberos-unaware. +#[derive(Debug, Clone, Default)] +pub struct CredentialInjectionKdcSessionStoreHandle(Arc>>); + +#[derive(Debug)] +struct Entry { + session: Arc, + expires_at: time::OffsetDateTime, +} + +impl CredentialInjectionKdcSessionStoreHandle { + pub fn new() -> Self { + Self(Arc::new(Mutex::new(HashMap::new()))) + } + + pub(crate) fn insert(&self, session: CredentialInjectionKdcSession, time_to_live: time::Duration) { + let jti = session.jti; + let entry = Entry { + session: Arc::new(session), + expires_at: time::OffsetDateTime::now_utc() + time_to_live, + }; + self.0.lock().insert(jti, entry); + } + + pub(crate) fn get(&self, jti: Uuid) -> Option> { + // Lookup-time TTL enforcement mirrors `CredentialStoreHandle::get`: the cleanup task is + // best-effort, and we don't want to hand out Kerberos material whose paired credential + // entry has already expired. + self.0 + .lock() + .get(&jti) + .filter(|entry| time::OffsetDateTime::now_utc() < entry.expires_at) + .map(|entry| Arc::clone(&entry.session)) + } +} + +pub struct CleanupTask { + pub handle: CredentialInjectionKdcSessionStoreHandle, +} + +#[async_trait] +impl Task for CleanupTask { + type Output = anyhow::Result<()>; + + const NAME: &'static str = "credential injection kdc cleanup"; + + async fn run(self, shutdown_signal: ShutdownSignal) -> Self::Output { + cleanup_task(self.handle, shutdown_signal).await; + Ok(()) + } +} + +#[instrument(skip_all)] +async fn cleanup_task(handle: CredentialInjectionKdcSessionStoreHandle, mut shutdown_signal: ShutdownSignal) { + use tokio::time::{Duration, sleep}; + + const TASK_INTERVAL: Duration = Duration::from_secs(60 * 15); // 15 minutes + + debug!("Task started"); + + loop { + tokio::select! { + _ = sleep(TASK_INTERVAL) => {} + _ = shutdown_signal.wait() => { + break; + } + } + + let now = time::OffsetDateTime::now_utc(); + handle.0.lock().retain(|_, entry| now < entry.expires_at); + } + + debug!("Task terminated"); +} + +#[cfg(test)] +mod tests { + use ironrdp_connector::sspi::network_client::NetworkProtocol; + use secrecy::SecretString; + + use super::*; + use crate::credential::{ + AppCredentialMapping, CleartextAppCredential, CleartextAppCredentialMapping, CredentialEntry, + }; + + fn cleartext_mapping_with_target_username(target_username: &str) -> CleartextAppCredentialMapping { + CleartextAppCredentialMapping { + proxy: CleartextAppCredential::UsernamePassword { + username: "proxy@example.invalid".to_owned(), + password: SecretString::from("pwd"), + }, + target: CleartextAppCredential::UsernamePassword { + username: target_username.to_owned(), + password: SecretString::from("pwd"), + }, + } + } + + fn dummy_entry_with_target_username(jti: Uuid, target_username: &str) -> ArcCredentialEntry { + let mapping: AppCredentialMapping = cleartext_mapping_with_target_username(target_username) + .encrypt() + .expect("encrypt mapping"); + Arc::new(CredentialEntry { + jti, + token: "raw-token".to_owned(), + expires_at: time::OffsetDateTime::now_utc() + time::Duration::minutes(5), + injection: Some(InjectionState { + mapping, + target_hostname: "target.example".to_owned(), + }), + }) + } + + fn dummy_entry(jti: Uuid) -> ArcCredentialEntry { + dummy_entry_with_target_username(jti, "target") + } + + fn dummy_kdc(jti: Uuid) -> CredentialInjectionKdc { + let entry = dummy_entry(jti); + let session = Arc::new(derive_credential_injection_kdc_session("proxy@example.invalid", jti)); + CredentialInjectionKdc::from_parts(entry, session).expect("valid credential-injection KDC") + } + + fn dummy_kdc_with_target_username(jti: Uuid, target_username: &str) -> CredentialInjectionKdc { + let entry = dummy_entry_with_target_username(jti, target_username); + let session = Arc::new(derive_credential_injection_kdc_session("proxy@example.invalid", jti)); + CredentialInjectionKdc::from_parts(entry, session).expect("valid credential-injection KDC") + } + + fn network_request(url: &str) -> NetworkRequest { + NetworkRequest { + protocol: NetworkProtocol::Http, + url: Url::parse(url).expect("test URL parses"), + data: Vec::new(), + } + } + + #[test] + fn proxy_user_at_realm_is_used_as_realm() { + let session = derive_credential_injection_kdc_session("proxy@example.invalid", Uuid::new_v4()); + assert_eq!(session.realm, "example.invalid"); + } + + #[test] + fn bare_proxy_username_yields_synthetic_realm() { + let jti = Uuid::new_v4(); + let session = derive_credential_injection_kdc_session("just-a-uuid", jti); + assert_eq!(session.realm, synthetic_realm(jti)); + assert!(!session.realm.is_empty()); + } + + #[test] + fn store_lookup_filters_expired_entries() { + let store = CredentialInjectionKdcSessionStoreHandle::new(); + let jti = Uuid::new_v4(); + let session = derive_credential_injection_kdc_session("proxy@example.invalid", jti); + + // Negative TTL: entry is born already expired. + store.insert(session, time::Duration::seconds(-1)); + + assert!(store.get(jti).is_none(), "expired entry must not be returned"); + } + + #[test] + fn store_returns_fresh_entry() { + let store = CredentialInjectionKdcSessionStoreHandle::new(); + let jti = Uuid::new_v4(); + let session = derive_credential_injection_kdc_session("proxy@example.invalid", jti); + + store.insert(session, time::Duration::minutes(5)); + + assert_eq!(store.get(jti).expect("fresh entry returned").realm, "example.invalid"); + } + + #[test] + fn client_acceptor_protocol_is_ntlm_for_domainless_target_credential() { + let kdc = dummy_kdc_with_target_username(Uuid::new_v4(), "Administrator"); + + assert_eq!( + kdc.client_acceptor_protocol().expect("protocol selected"), + CredentialInjectionClientAcceptorProtocol::Ntlm + ); + } + + #[test] + fn client_acceptor_protocol_is_kerberos_for_upn_target_credential() { + let kdc = dummy_kdc_with_target_username(Uuid::new_v4(), "administrator@example.invalid"); + + assert_eq!( + kdc.client_acceptor_protocol().expect("protocol selected"), + CredentialInjectionClientAcceptorProtocol::Kerberos + ); + } + + #[test] + fn client_acceptor_protocol_is_kerberos_for_downlevel_target_credential() { + let kdc = dummy_kdc_with_target_username(Uuid::new_v4(), "EXAMPLE\\Administrator"); + + assert_eq!( + kdc.client_acceptor_protocol().expect("protocol selected"), + CredentialInjectionClientAcceptorProtocol::Kerberos + ); + } + + #[test] + fn resolve_with_no_jet_cred_id_forwards_to_real_kdc() { + let credential_store = CredentialStoreHandle::new(); + let session_store = CredentialInjectionKdcSessionStoreHandle::new(); + + let dispatch = CredentialInjectionKdc::resolve(None, &credential_store, &session_store) + .expect("plain KDC token should dispatch"); + + assert!(dispatch.is_none()); + } + + #[test] + fn from_parts_rejects_mismatched_entry_and_session_jti() { + let entry_jti = Uuid::new_v4(); + let session_jti = Uuid::new_v4(); + assert_ne!(entry_jti, session_jti); + + let entry = dummy_entry(entry_jti); + let session = Arc::new(derive_credential_injection_kdc_session( + "proxy@example.invalid", + session_jti, + )); + + let err = CredentialInjectionKdc::from_parts(entry, session) + .expect_err("mismatched entry/session JTI must fail closed"); + let msg = format!("{err:#}"); + assert!( + msg.contains("credential entry JTI does not match credential-injection KDC session JTI"), + "actual: {msg}" + ); + } + + #[test] + fn resolve_with_missing_jet_cred_id_fails_closed() { + let credential_store = CredentialStoreHandle::new(); + let session_store = CredentialInjectionKdcSessionStoreHandle::new(); + + assert!( + CredentialInjectionKdc::resolve(Some(Uuid::new_v4()), &credential_store, &session_store).is_err(), + "KDC tokens with jet_cred_id must not fall back to real-KDC forwarding" + ); + } + + #[test] + fn resolve_with_non_injection_entry_fails_closed() { + let credential_store = CredentialStoreHandle::new(); + let session_store = CredentialInjectionKdcSessionStoreHandle::new(); + let jti = Uuid::new_v4(); + + credential_store + .insert(jti, "raw-token".to_owned(), None, time::Duration::minutes(5)) + .expect("provision-token entry inserts"); + + assert!( + CredentialInjectionKdc::resolve(Some(jti), &credential_store, &session_store).is_err(), + "KDC tokens with jet_cred_id must require provision-credentials state" + ); + } + + #[test] + fn intercept_ignores_non_loopback_host() { + let jti = Uuid::new_v4(); + let kdc = dummy_kdc(jti); + + let request = network_request("http://kdc.real.example/path"); + let result = kdc + .intercept_network_request(&request) + .expect("non-loopback request dispatches"); + + assert!(matches!( + result, + CredentialInjectionKdcInterception::NotInjectionRequest + )); + } + + #[test] + fn intercept_rejects_malformed_url_path() { + let jti = Uuid::new_v4(); + let kdc = dummy_kdc(jti); + + let request = network_request("http://cred.invalid/not-a-uuid"); + let err = kdc + .intercept_network_request(&request) + .expect_err("non-UUID path must fail"); + let msg = format!("{err:#}"); + assert!(msg.contains("malformed in-process KDC URL"), "actual: {msg}"); + } + + #[test] + fn intercept_rejects_mismatched_jti() { + let entry_jti = Uuid::new_v4(); + let other_jti = Uuid::new_v4(); + assert_ne!(entry_jti, other_jti); + + let kdc = dummy_kdc(entry_jti); + + let request = network_request(&format!("http://cred.invalid/{}", other_jti)); + let err = kdc + .intercept_network_request(&request) + .expect_err("JTI mismatch must fail"); + let msg = format!("{err:#}"); + assert!(msg.contains("does not match current CredSSP session"), "actual: {msg}"); + } + + #[test] + fn intercept_accepts_matching_url_path_before_payload_decode() { + let jti = Uuid::new_v4(); + let kdc = dummy_kdc(jti); + + let request = network_request(&format!("http://cred.invalid/{jti}")); + let err = kdc + .intercept_network_request(&request) + .expect_err("empty KDC payload must fail after URL/JTI validation"); + let msg = format!("{err:#}"); + assert!(msg.contains("malformed in-process KDC proxy payload"), "actual: {msg}"); + } + + #[test] + fn realm_mismatch_is_reported_as_not_injection_realm() { + let mismatch = + realm_mismatch("cred-session.invalid", "evil.example").expect("different realms produce a mismatch"); + assert_eq!(mismatch.expected, "cred-session.invalid"); + assert_eq!(mismatch.actual, "evil.example"); + } +} diff --git a/devolutions-gateway/src/extract.rs b/devolutions-gateway/src/extract.rs index bbf3be4e4..227029aba 100644 --- a/devolutions-gateway/src/extract.rs +++ b/devolutions-gateway/src/extract.rs @@ -1,11 +1,14 @@ +use std::net::SocketAddr; + use axum::Extension; -use axum::extract::{FromRequest, FromRequestParts, RawQuery, Request}; +use axum::extract::{ConnectInfo, FromRequest, FromRequestParts, Path, RawQuery, Request}; use axum::http::request::Parts; +use crate::DgwState; use crate::http::HttpError; use crate::token::{ AccessScope, AccessTokenClaims, AssociationTokenClaims, BridgeTokenClaims, JmuxTokenClaims, JrecTokenClaims, - JrlTokenClaims, ScopeTokenClaims, WebAppTokenClaims, + JrlTokenClaims, KdcTokenClaims, ScopeTokenClaims, WebAppTokenClaims, }; #[derive(Clone)] @@ -98,6 +101,46 @@ where } } +/// Extractor for the KDC proxy route's path-bound token. +/// +/// `/jet/KdcProxy/{token}` carries the token in the URL path rather than the standard +/// `Authorization: Bearer` header or `?token=` query parameter, so the global auth middleware +/// (`middleware/auth.rs`) skips it (see `AUTH_EXCEPTIONS`). This extractor reads the token from +/// the path, runs it through the same `authenticate()` routine the middleware would, and +/// unwraps the `Kdc` variant so handlers receive `KdcTokenClaims` directly. +#[derive(Clone)] +pub struct KdcToken(pub KdcTokenClaims); + +impl FromRequestParts for KdcToken { + type Rejection = HttpError; + + async fn from_request_parts(parts: &mut Parts, state: &DgwState) -> Result { + let Path(token) = Path::::from_request_parts(parts, state) + .await + .map_err(HttpError::bad_request().with_msg("KDC token missing from path").err())?; + let ConnectInfo(source_addr) = ConnectInfo::::from_request_parts(parts, state) + .await + .map_err(HttpError::internal().with_msg("source address unavailable").err())?; + + let conf = state.conf_handle.get_conf(); + let claims = crate::middleware::auth::authenticate( + source_addr, + &token, + &conf, + &state.token_cache, + &state.jrl, + &state.recordings.active_recordings, + None, + ) + .map_err(HttpError::unauthorized().err())?; + + match claims { + AccessTokenClaims::Kdc(claims) => Ok(Self(claims)), + _ => Err(HttpError::forbidden().msg("token not allowed (expected KDC token)")), + } + } +} + #[derive(Clone)] pub struct ScopeToken(pub ScopeTokenClaims); diff --git a/devolutions-gateway/src/generic_client.rs b/devolutions-gateway/src/generic_client.rs index 7b5e6c47b..7966b946e 100644 --- a/devolutions-gateway/src/generic_client.rs +++ b/devolutions-gateway/src/generic_client.rs @@ -9,6 +9,7 @@ use typed_builder::TypedBuilder; use crate::config::Conf; use crate::credential::CredentialStoreHandle; +use crate::credential_injection_kdc::{CredentialInjectionKdc, CredentialInjectionKdcSessionStoreHandle}; use crate::proxy::Proxy; use crate::rdp_pcb::{extract_association_claims, read_pcb}; use crate::recording::ActiveRecordings; @@ -28,6 +29,7 @@ pub struct GenericClient { subscriber_tx: SubscriberSender, active_recordings: Arc, credential_store: CredentialStoreHandle, + kerberos_session_store: CredentialInjectionKdcSessionStoreHandle, #[builder(default)] agent_tunnel_handle: Option>, } @@ -52,6 +54,7 @@ where subscriber_tx, active_recordings, credential_store, + kerberos_session_store, agent_tunnel_handle, } = self; @@ -147,35 +150,41 @@ where // If a credential mapping has been pushed, we automatically switch to this mode. // Otherwise, we continue the generic procedure. // - // RdpProxy is generic over the server stream, so credential injection now works + // RdpProxy is generic over the server stream, so credential injection works // regardless of whether the upstream is direct TCP or tunnelled via an agent. - if is_rdp { - let token_id = token::extract_jti(token).context("failed to extract jti claim from token")?; - - if let Some(entry) = credential_store.get(token_id) { - anyhow::ensure!(token == entry.token, "token mismatch"); - - // NOTE: In the future, we could imagine performing proxy-based recording as well using RdpProxy. - if entry.mapping.is_some() { - return crate::rdp_proxy::RdpProxy::builder() - .conf(conf) - .session_info(info) - .client_addr(client_addr) - .client_stream(client_stream) - .server_addr(server_addr) - .server_stream(server_stream) - .sessions(sessions) - .subscriber_tx(subscriber_tx) - .credential_entry(entry) - .client_stream_leftover_bytes(leftover_bytes) - .server_dns_name(selected_target.host().to_owned()) - .disconnect_interest(disconnect_interest) - .build() - .run() - .await - .context("encountered a failure during RDP proxying (credential injection)"); - } - } + // The credential store is keyed on the association token's JTI, so a direct + // lookup by `claims.jti` is the primary path. + if is_rdp + && let Some(entry) = credential_store.get(claims.jti) + && entry.injection.is_some() + && let Some(kdc_session) = kerberos_session_store.get(claims.jti) + { + anyhow::ensure!(token == entry.token, "token mismatch"); + let credential_injection_kdc = CredentialInjectionKdc::from_parts(entry, kdc_session)?; + + info!( + jti = %credential_injection_kdc.jti(), + "RDP-TLS forwarding with credential injection" + ); + + // NOTE: In the future, we could imagine performing proxy-based recording as well using RdpProxy. + return crate::rdp_proxy::RdpProxy::builder() + .conf(conf) + .session_info(info) + .client_addr(client_addr) + .client_stream(client_stream) + .server_addr(server_addr) + .server_stream(server_stream) + .sessions(sessions) + .subscriber_tx(subscriber_tx) + .credential_injection_kdc(credential_injection_kdc) + .client_stream_leftover_bytes(leftover_bytes) + .server_dns_name(selected_target.host().to_owned()) + .disconnect_interest(disconnect_interest) + .build() + .run() + .await + .context("encountered a failure during RDP proxying (credential injection)"); } info!("Upstream forwarding"); diff --git a/devolutions-gateway/src/lib.rs b/devolutions-gateway/src/lib.rs index 3af9f7999..019252365 100644 --- a/devolutions-gateway/src/lib.rs +++ b/devolutions-gateway/src/lib.rs @@ -17,6 +17,7 @@ pub mod api; pub mod cli; pub mod config; pub mod credential; +pub mod credential_injection_kdc; pub mod extract; pub mod generic_client; pub mod http; @@ -60,6 +61,7 @@ pub struct DgwState { pub recordings: recording::RecordingMessageSender, pub job_queue_handle: job_queue::JobQueueHandle, pub credential_store: credential::CredentialStoreHandle, + pub kerberos_session_store: credential_injection_kdc::CredentialInjectionKdcSessionStoreHandle, pub monitoring_state: Arc, pub traffic_audit_handle: traffic_audit::TrafficAuditHandle, pub agent_tunnel_handle: Option>, @@ -88,6 +90,7 @@ impl DgwState { let (job_queue_handle, job_queue_rx) = job_queue::JobQueueHandle::new(); let (traffic_audit_handle, traffic_audit_rx) = traffic_audit::TrafficAuditHandle::new(); let credential_store = credential::CredentialStoreHandle::new(); + let kerberos_session_store = credential_injection_kdc::CredentialInjectionKdcSessionStoreHandle::new(); let monitoring_state = Arc::new(network_monitor::State::new(Arc::new(MockMonitorsCache))?); let state = Self { @@ -101,6 +104,7 @@ impl DgwState { job_queue_handle, traffic_audit_handle, credential_store, + kerberos_session_store, monitoring_state, agent_tunnel_handle: None, }; diff --git a/devolutions-gateway/src/listener.rs b/devolutions-gateway/src/listener.rs index 0b7ce2740..5898b738a 100644 --- a/devolutions-gateway/src/listener.rs +++ b/devolutions-gateway/src/listener.rs @@ -159,6 +159,7 @@ async fn handle_tcp_peer(stream: TcpStream, state: DgwState, peer_addr: SocketAd .subscriber_tx(state.subscriber_tx) .active_recordings(state.recordings.active_recordings) .credential_store(state.credential_store) + .kerberos_session_store(state.kerberos_session_store) .agent_tunnel_handle(state.agent_tunnel_handle) .build() .serve() diff --git a/devolutions-gateway/src/ngrok.rs b/devolutions-gateway/src/ngrok.rs index 71c0c005f..58115db89 100644 --- a/devolutions-gateway/src/ngrok.rs +++ b/devolutions-gateway/src/ngrok.rs @@ -238,6 +238,7 @@ async fn run_tcp_tunnel(mut tunnel: ngrok::tunnel::TcpTunnel, state: DgwState) { .subscriber_tx(state.subscriber_tx) .active_recordings(state.recordings.active_recordings) .credential_store(state.credential_store) + .kerberos_session_store(state.kerberos_session_store) .agent_tunnel_handle(state.agent_tunnel_handle) .build() .serve() diff --git a/devolutions-gateway/src/rd_clean_path.rs b/devolutions-gateway/src/rd_clean_path.rs index 41a118a02..3f790974e 100644 --- a/devolutions-gateway/src/rd_clean_path.rs +++ b/devolutions-gateway/src/rd_clean_path.rs @@ -3,17 +3,16 @@ use std::net::SocketAddr; use std::sync::Arc; use anyhow::Context as _; -use ironrdp_connector::sspi; use ironrdp_pdu::nego; use ironrdp_rdcleanpath::RDCleanPathPdu; -use secrecy::ExposeSecret as _; use tap::prelude::*; use thiserror::Error; use tokio::io::{AsyncRead, AsyncReadExt as _, AsyncWrite, AsyncWriteExt as _}; use tracing::field; use crate::config::Conf; -use crate::credential::{CredentialEntry, CredentialStoreHandle}; +use crate::credential::CredentialStoreHandle; +use crate::credential_injection_kdc::{CredentialInjectionKdc, CredentialInjectionKdcSessionStoreHandle}; use crate::proxy::Proxy; use crate::recording::ActiveRecordings; use crate::session::{ConnectionModeDetails, DisconnectInterest, DisconnectedInfo, SessionInfo, SessionMessageSender}; @@ -316,15 +315,13 @@ async fn handle_with_credential_injection( subscriber_tx: SubscriberSender, active_recordings: &ActiveRecordings, cleanpath_pdu: RDCleanPathPdu, - credential_entry: Arc, + credential_injection_kdc: CredentialInjectionKdc, agent_tunnel_handle: Option>, ) -> anyhow::Result<()> { let tls_conf = conf.credssp_tls.get().context("CredSSP TLS configuration")?; let gateway_hostname = conf.hostname.clone(); - let credential_mapping = credential_entry.mapping.as_ref().context("no credential mapping")?; - let x224_req = cleanpath_pdu .x224_connection_pdu .as_ref() @@ -418,57 +415,17 @@ async fn handle_with_credential_injection( let mut client_framed = ironrdp_tokio::MovableTokioFramed::new(client_stream); let mut server_framed = ironrdp_tokio::MovableTokioFramed::new(server_stream); - let krb_server_config = if conf.debug.enable_unstable - && let Some(crate::config::dto::KerberosConfig { - kerberos_server: - crate::config::dto::KerberosServer { - max_time_skew, - ticket_decryption_key, - service_user, - .. - }, - kdc_url: _, - }) = conf.debug.kerberos.as_ref() - { - let user = service_user.as_ref().map(|user| { - let crate::config::dto::DomainUser { - fqdn, - password, - salt: _, - } = user; - - // The username is in the FQDN format. Thus, the domain field can be empty. - sspi::CredentialsBuffers::AuthIdentity(sspi::AuthIdentityBuffers::from_utf8( - fqdn, - "", - password.expose_secret(), - )) - }); - - Some(sspi::KerberosServerConfig { - kerberos_config: sspi::KerberosConfig { - // The sspi-rs can automatically resolve the KDC host via DNS and/or env variable. - kdc_url: None, - client_computer_name: Some(client_addr.to_string()), - }, - server_properties: sspi::kerberos::ServerProperties::new( - &["TERMSRV", &gateway_hostname], - user, - std::time::Duration::from_secs(*max_time_skew), - ticket_decryption_key.clone(), - )?, - }) - } else { - None - }; + let krb_server_config = + crate::rdp_proxy::credential_injection_kerberos_server_config(&conf, client_addr, &credential_injection_kdc)?; - let client_credssp_fut = crate::rdp_proxy::perform_credssp_with_client( + let client_credssp_fut = crate::rdp_proxy::perform_credssp_as_server( &mut client_framed, client_addr.ip(), gateway_public_key, client_security_protocol, - &credential_mapping.proxy, + credential_injection_kdc.proxy_credential(), krb_server_config, + &credential_injection_kdc, ); let krb_client_config = if conf.debug.enable_unstable @@ -485,12 +442,12 @@ async fn handle_with_credential_injection( None }; - let server_credssp_fut = crate::rdp_proxy::perform_credssp_with_server( + let server_credssp_fut = crate::rdp_proxy::perform_credssp_as_client( &mut server_framed, destination.host().to_owned(), server_public_key, server_security_protocol, - &credential_mapping.target, + credential_injection_kdc.target_credential(), krb_client_config, ); @@ -565,6 +522,7 @@ pub async fn handle( subscriber_tx: SubscriberSender, active_recordings: &ActiveRecordings, credential_store: &CredentialStoreHandle, + kerberos_session_store: &CredentialInjectionKdcSessionStoreHandle, agent_tunnel_handle: Option>, ) -> anyhow::Result<()> { // Special handshake of our RDP extension @@ -583,14 +541,19 @@ pub async fn handle( // If a credential mapping has been pushed, we automatically switch to // proxy-based credential injection mode. Otherwise, we continue the usual - // clean path procedure. - if let Some(entry) = crate::token::extract_jti(token) - .ok() - .and_then(|token_id| credential_store.get(token_id)) - .filter(|entry| entry.mapping.is_some()) - { - anyhow::ensure!(token == entry.token, "token mismatch"); - debug!("Switching to RdpProxy for credential injection (WebSocket)"); + // clean path procedure. The credential store is keyed on the association token's JTI, + // and the per-session Kerberos material lives in a parallel store under the same JTI. + if let Some(credential_injection_kdc) = crate::token::extract_jti(token).ok().and_then(|jti| { + let entry = credential_store.get(jti)?; + entry.injection.as_ref()?; + let kdc_session = kerberos_session_store.get(jti)?; + CredentialInjectionKdc::from_parts(entry, kdc_session).ok() + }) { + anyhow::ensure!(token == credential_injection_kdc.raw_token(), "token mismatch"); + debug!( + jti = %credential_injection_kdc.jti(), + "Switching to RdpProxy for credential injection (WebSocket)" + ); return handle_with_credential_injection( client_stream, @@ -602,7 +565,7 @@ pub async fn handle( subscriber_tx, active_recordings, cleanpath_pdu, - entry, + credential_injection_kdc, agent_tunnel_handle.clone(), ) .await; diff --git a/devolutions-gateway/src/rdp_proxy.rs b/devolutions-gateway/src/rdp_proxy.rs index af7d5f090..1ab4382f1 100644 --- a/devolutions-gateway/src/rdp_proxy.rs +++ b/devolutions-gateway/src/rdp_proxy.rs @@ -13,7 +13,10 @@ use typed_builder::TypedBuilder; use crate::api::kdc_proxy::send_krb_message; use crate::config::Conf; -use crate::credential::{AppCredentialMapping, ArcCredentialEntry}; +use crate::credential::AppCredential; +use crate::credential_injection_kdc::{ + CredentialInjectionClientAcceptorProtocol, CredentialInjectionKdc, CredentialInjectionKdcInterception, +}; use crate::proxy::Proxy; use crate::session::{DisconnectInterest, SessionInfo, SessionMessageSender}; use crate::subscriber::SubscriberSender; @@ -27,7 +30,7 @@ pub struct RdpProxy { client_addr: SocketAddr, server_stream: S, server_addr: SocketAddr, - credential_entry: ArcCredentialEntry, + credential_injection_kdc: CredentialInjectionKdc, client_stream_leftover_bytes: bytes::BytesMut, sessions: SessionMessageSender, subscriber_tx: SubscriberSender, @@ -58,7 +61,7 @@ where client_addr, server_stream, server_addr, - credential_entry, + credential_injection_kdc, client_stream_leftover_bytes, sessions, subscriber_tx, @@ -69,8 +72,6 @@ where let tls_conf = conf.credssp_tls.get().context("CredSSP TLS configuration")?; let gateway_hostname = conf.hostname.clone(); - let credential_mapping = credential_entry.mapping.as_ref().context("no credential mapping")?; - // -- Retrieve the Gateway TLS public key that must be used for client-proxy CredSSP later on -- // let gateway_cert_chain_handle = tokio::spawn(crate::tls::get_cert_chain_for_acceptor_cached( @@ -84,8 +85,12 @@ where ironrdp_tokio::MovableTokioFramed::new_with_leftover(client_stream, client_stream_leftover_bytes); let mut server_framed = ironrdp_tokio::MovableTokioFramed::new(server_stream); - let handshake_result = - dual_handshake_until_tls_upgrade(&mut client_framed, &mut server_framed, credential_mapping).await?; + let handshake_result = dual_handshake_until_tls_upgrade( + &mut client_framed, + &mut server_framed, + credential_injection_kdc.target_credential(), + ) + .await?; let client_stream = client_framed.into_inner_no_leftover(); let server_stream = server_framed.into_inner_no_leftover(); @@ -112,57 +117,16 @@ where let mut client_framed = ironrdp_tokio::MovableTokioFramed::new(client_stream); let mut server_framed = ironrdp_tokio::MovableTokioFramed::new(server_stream); - let krb_server_config = if conf.debug.enable_unstable - && let Some(crate::config::dto::KerberosConfig { - kerberos_server: - crate::config::dto::KerberosServer { - max_time_skew, - ticket_decryption_key, - service_user, - .. - }, - kdc_url: _, - }) = conf.debug.kerberos.as_ref() - { - let user = service_user.as_ref().map(|user| { - let crate::config::dto::DomainUser { - fqdn, - password, - salt: _, - } = user; - - // The username is in the FQDN format. Thus, the domain field can be empty. - sspi::CredentialsBuffers::AuthIdentity(sspi::AuthIdentityBuffers::from_utf8( - fqdn, - "", - password.expose_secret(), - )) - }); - - Some(sspi::KerberosServerConfig { - kerberos_config: sspi::KerberosConfig { - // The sspi-rs can automatically resolve the KDC host via DNS and/or env variable. - kdc_url: None, - client_computer_name: Some(client_addr.to_string()), - }, - server_properties: sspi::kerberos::ServerProperties::new( - &["TERMSRV", &gateway_hostname], - user, - std::time::Duration::from_secs(*max_time_skew), - ticket_decryption_key.clone(), - )?, - }) - } else { - None - }; + let krb_server_config = credential_injection_kerberos_server_config(&conf, client_addr, &credential_injection_kdc)?; - let client_credssp_fut = perform_credssp_with_client( + let client_credssp_fut = perform_credssp_as_server( &mut client_framed, client_addr.ip(), gateway_public_key, handshake_result.client_security_protocol, - &credential_mapping.proxy, + credential_injection_kdc.proxy_credential(), krb_server_config, + &credential_injection_kdc, ); let krb_client_config = if conf.debug.enable_unstable @@ -179,12 +143,12 @@ where None }; - let server_credssp_fut = perform_credssp_with_server( + let server_credssp_fut = perform_credssp_as_client( &mut server_framed, server_dns_name, server_public_key, handshake_result.server_security_protocol, - &credential_mapping.target, + credential_injection_kdc.target_credential(), krb_client_config, ); @@ -282,7 +246,7 @@ where async fn dual_handshake_until_tls_upgrade( client_framed: &mut ironrdp_tokio::MovableTokioFramed, server_framed: &mut ironrdp_tokio::MovableTokioFramed, - mapping: &AppCredentialMapping, + target_credential: &AppCredential, ) -> anyhow::Result where C: AsyncWrite + AsyncRead + Unpin + Send, @@ -311,8 +275,8 @@ where }; let connection_request_to_send = nego::ConnectionRequest { - nego_data: match &mapping.target { - crate::credential::AppCredential::UsernamePassword { username, .. } => { + nego_data: match target_credential { + AppCredential::UsernamePassword { username, .. } => { Some(nego::NegoRequestData::cookie(username.to_owned())) } }, @@ -393,13 +357,36 @@ where handshake_result } +pub(crate) fn credential_injection_kerberos_server_config( + conf: &Conf, + client_addr: SocketAddr, + credential_injection_kdc: &CredentialInjectionKdc, +) -> anyhow::Result> { + if !conf.debug.enable_unstable || conf.debug.kerberos.is_none() { + return Ok(None); + } + + match credential_injection_kdc.client_acceptor_protocol()? { + CredentialInjectionClientAcceptorProtocol::Kerberos => { + credential_injection_kdc.server_kerberos_config(client_addr).map(Some) + } + CredentialInjectionClientAcceptorProtocol::Ntlm => { + debug!( + jti = %credential_injection_kdc.jti(), + "Credential-injection Kerberos acceptor disabled for NTLM target credential" + ); + Ok(None) + } + } +} + #[instrument(name = "server_credssp", level = "debug", ret, skip_all)] -pub(crate) async fn perform_credssp_with_server( +pub(crate) async fn perform_credssp_as_client( framed: &mut ironrdp_tokio::Framed, server_name: String, server_public_key: Vec, security_protocol: nego::SecurityProtocol, - credentials: &crate::credential::AppCredential, + credentials: &AppCredential, kerberos_config: Option, ) -> anyhow::Result<()> where @@ -423,7 +410,7 @@ where credentials, None, security_protocol, - ironrdp_connector::ServerName::new(server_name), + ironrdp_connector::ServerName::new(server_name.clone()), server_public_key, kerberos_config, )?; @@ -465,18 +452,25 @@ where async fn resolve_server_generator( generator: &mut CredsspServerProcessGenerator<'_>, + credential_injection_kdc: &CredentialInjectionKdc, ) -> Result { let mut state = generator.start(); loop { match state { GeneratorState::Suspended(request) => { - let response = send_network_request(&request) - .await - .map_err(|err| sspi::credssp::ServerError { - ts_request: None, - error: sspi::Error::new(sspi::ErrorKind::InternalError, err), - })?; + let response = match credential_injection_kdc.intercept_network_request(&request) { + Ok(CredentialInjectionKdcInterception::Intercepted(response)) => Ok(response), + Ok(CredentialInjectionKdcInterception::NotInjectionRequest) => send_network_request(&request).await, + Ok(CredentialInjectionKdcInterception::NotInjectionRealm(mismatch)) => Err(anyhow::anyhow!( + "kdc request realm does not match credential-injection session realm: {mismatch}" + )), + Err(error) => Err(error), + } + .map_err(|err| sspi::credssp::ServerError { + ts_request: None, + error: sspi::Error::new(sspi::ErrorKind::InternalError, err), + })?; state = generator.resume(Ok(response)); } @@ -508,13 +502,14 @@ async fn resolve_client_generator( } #[instrument(name = "client_credssp", level = "debug", ret, skip_all)] -pub(crate) async fn perform_credssp_with_client( +pub(crate) async fn perform_credssp_as_server( framed: &mut ironrdp_tokio::Framed, client_addr: IpAddr, gateway_public_key: Vec, security_protocol: nego::SecurityProtocol, - credentials: &crate::credential::AppCredential, + credentials: &AppCredential, kerberos_server_config: Option, + credential_injection_kdc: &CredentialInjectionKdc, ) -> anyhow::Result<()> where S: ironrdp_tokio::FramedRead + ironrdp_tokio::FramedWrite, @@ -535,6 +530,7 @@ where gateway_public_key, credentials, kerberos_server_config, + credential_injection_kdc, ) .await; @@ -560,8 +556,9 @@ where buf: &mut ironrdp_pdu::WriteBuf, client_computer_name: ironrdp_connector::ServerName, public_key: Vec, - credentials: &crate::credential::AppCredential, + credentials: &AppCredential, kerberos_server_config: Option, + credential_injection_kdc: &CredentialInjectionKdc, ) -> anyhow::Result<()> where S: ironrdp_tokio::FramedRead + ironrdp_tokio::FramedWrite, @@ -603,7 +600,7 @@ where let result = { let mut generator = sequence.process_ts_request(ts_request); - resolve_server_generator(&mut generator).await + resolve_server_generator(&mut generator, credential_injection_kdc).await }; // drop generator buf.clear(); @@ -634,14 +631,27 @@ where Ok(()) } +/// Generic Kerberos network-request dispatcher. +/// +/// Only handles real-network schemes (`tcp` / `udp`); credential-injection loopback requests +/// are intercepted by [`CredentialInjectionKdc`] before reaching this function. +/// +/// TODO(sspi-rs#NNN): when sspi-rs ships a pluggable KDC dispatcher API, the URL trick for +/// credential injection goes away entirely and this helper can be inlined back into the +/// CredSSP loops. async fn send_network_request(request: &NetworkRequest) -> anyhow::Result> { - let target_addr = TargetAddr::parse(request.url.as_str(), Some(88))?; - - // TODO(DGW-384): plumb `agent_tunnel_handle` through `RdpProxy` so - // CredSSP-originated Kerberos requests can traverse the agent tunnel. - // Currently these go direct from the gateway host, bypassing the - // routing pipeline used by every other proxy path. - send_krb_message(&target_addr, &request.data) - .await - .map_err(|err| anyhow::Error::msg("failed to send KDC message").context(err)) + match request.url.scheme() { + "tcp" | "udp" => { + let target_addr = TargetAddr::parse(request.url.as_str(), Some(88))?; + + // TODO(DGW-384): plumb `agent_tunnel_handle` through `RdpProxy` so + // CredSSP-originated Kerberos requests can traverse the agent tunnel. + // Currently these go direct from the gateway host, bypassing the + // routing pipeline used by every other proxy path. + send_krb_message(&target_addr, &request.data) + .await + .map_err(|err| anyhow::Error::msg("failed to send KDC message").context(err)) + } + unsupported => anyhow::bail!("unsupported KDC request scheme: {unsupported}"), + } } diff --git a/devolutions-gateway/src/service.rs b/devolutions-gateway/src/service.rs index f3de73ccf..47823b872 100644 --- a/devolutions-gateway/src/service.rs +++ b/devolutions-gateway/src/service.rs @@ -269,6 +269,8 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { .context("failed to initialize traffic audit manager")?; let credential_store = CredentialStoreHandle::new(); + let kerberos_session_store = + devolutions_gateway::credential_injection_kdc::CredentialInjectionKdcSessionStoreHandle::new(); let filesystem_monitor_config_cache = devolutions_gateway::api::monitoring::FilesystemConfigCache::new( config::get_data_dir().join("monitors_cache.json"), @@ -317,6 +319,7 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { recordings: recording_manager_handle.clone(), job_queue_handle: job_queue_ctx.job_queue_handle.clone(), credential_store: credential_store.clone(), + kerberos_session_store: kerberos_session_store.clone(), monitoring_state, traffic_audit_handle: traffic_audit_task.handle(), agent_tunnel_handle, @@ -355,6 +358,10 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { handle: credential_store, }); + tasks.register(devolutions_gateway::credential_injection_kdc::CleanupTask { + handle: kerberos_session_store, + }); + tasks.register(devolutions_log::LogDeleterTask::::new( conf.log_file.clone(), )); diff --git a/devolutions-gateway/src/token.rs b/devolutions-gateway/src/token.rs index aa8cf7371..d723140fa 100644 --- a/devolutions-gateway/src/token.rs +++ b/devolutions-gateway/src/token.rs @@ -1222,11 +1222,15 @@ fn validate_token_impl( Ok(claims) } -fn extract_uuid(token: &str, field: &str) -> anyhow::Result { +fn extract_payload(token: &str) -> anyhow::Result { let jws = RawJws::decode(token) .context("failed to parse the provided JWS")? .discard_signature(); - let payload = serde_json::from_slice::(&jws.payload).context("parse JWS payload")?; + serde_json::from_slice::(&jws.payload).context("parse JWS payload") +} + +fn extract_uuid(token: &str, field: &str) -> anyhow::Result { + let payload = extract_payload(token)?; let uuid = payload.get(field).context("claim is missing from the token")?; let uuid = uuid.as_str().context("value is malformed")?; let uuid = Uuid::from_str(uuid).context("value is not a valid UUID string")?; @@ -1242,6 +1246,36 @@ pub fn extract_session_id(token: &str) -> anyhow::Result { extract_uuid(token, "jet_aid").context("extract jet_aid") } +/// Extract the destination host claim (`dst_hst`) from an association token without verifying its +/// signature. Returns `None` if the claim is missing. +/// +/// Used by the credential store to remember the target server's hostname for a credential-injection +/// session, so that fake-KDC can validate the client's TGS-REQ sname against it (the SPN the client +/// actually requested is `TERMSRV/`, not Gateway's own hostname). +pub fn extract_dst_hst(token: &str) -> anyhow::Result> { + let payload = extract_payload(token)?; + let Some(value) = payload.get("dst_hst") else { + return Ok(None); + }; + let dst_hst = value.as_str().context("dst_hst is malformed")?; + Ok(Some(dst_hst.to_owned())) +} + +/// Extract alternate destination hosts (`dst_alt`) from an association token without verifying its +/// signature. +pub fn extract_dst_alt(token: &str) -> anyhow::Result> { + let payload = extract_payload(token)?; + let Some(value) = payload.get("dst_alt") else { + return Ok(Vec::new()); + }; + + let dst_alt = value.as_array().context("dst_alt is malformed")?; + dst_alt + .iter() + .map(|value| value.as_str().context("dst_alt entry is malformed").map(str::to_owned)) + .collect() +} + #[deprecated = "make sure this is never used without a deliberate action"] pub mod unsafe_debug { // Any function in this module should only be used at development stage when deliberately @@ -1724,3 +1758,32 @@ mod serde_impl { } } } + +#[cfg(test)] +mod tests { + use base64::Engine as _; + + use super::*; + + fn unsigned_jws(payload: serde_json::Value) -> String { + let engine = base64::engine::general_purpose::URL_SAFE_NO_PAD; + let header = engine.encode(r#"{"alg":"RS256"}"#); + let payload = engine.encode(serde_json::to_vec(&payload).expect("payload serializes")); + let signature = engine.encode(b"signature"); + format!("{header}.{payload}.{signature}") + } + + #[test] + fn extract_dst_alt_returns_alternate_targets() { + let token = unsigned_jws(serde_json::json!({ + "jti": "5e3e833f-84c7-4541-b676-acc3299e39b8", + "dst_hst": "primary.example:3389", + "dst_alt": ["secondary.example:3389"] + })); + + assert_eq!( + extract_dst_alt(&token).expect("dst_alt parses"), + vec!["secondary.example:3389".to_owned()] + ); + } +} diff --git a/devolutions-gateway/tests/preflight.rs b/devolutions-gateway/tests/preflight.rs index 0f3f175ae..1f78d3094 100644 --- a/devolutions-gateway/tests/preflight.rs +++ b/devolutions-gateway/tests/preflight.rs @@ -2,13 +2,12 @@ #![allow(clippy::unwrap_used)] use std::net::SocketAddr; -use std::str::FromStr as _; use axum::Router; use axum::body::Body; use axum::extract::connect_info::MockConnectInfo; use axum::http::{self, Request, StatusCode}; -use devolutions_gateway::credential::AppCredential; +use base64::Engine as _; use devolutions_gateway::{DgwState, MockHandles}; use http_body_util::BodyExt as _; use serde_json::json; @@ -31,7 +30,8 @@ const CONFIG: &str = r#"{ } ], "__debug__": { - "disable_token_validation": true + "disable_token_validation": true, + "enable_unstable": true } }"#; @@ -46,6 +46,14 @@ fn preflight_request(operations: serde_json::Value) -> anyhow::Result anyhow::Result { + let engine = base64::engine::general_purpose::URL_SAFE_NO_PAD; + let header = engine.encode(r#"{"alg":"RS256"}"#); + let payload = engine.encode(serde_json::to_vec(&payload)?); + let signature = engine.encode(b"signature"); + Ok(format!("{header}.{payload}.{signature}")) +} + fn make_router() -> anyhow::Result<(Router, DgwState, MockHandles)> { let (state, handles) = DgwState::mock(CONFIG)?; let app = devolutions_gateway::make_http_service(state.clone()) @@ -53,6 +61,13 @@ fn make_router() -> anyhow::Result<(Router, DgwState, MockHandles)> { Ok((app, state, handles)) } +fn make_router_with_config(config: &str) -> anyhow::Result<(Router, DgwState, MockHandles)> { + let (state, handles) = DgwState::mock(config)?; + let app = devolutions_gateway::make_http_service(state.clone()) + .layer(MockConnectInfo(SocketAddr::from(([0, 0, 0, 0], 3000)))); + Ok((app, state, handles)) +} + fn init_logger() -> tracing::subscriber::DefaultGuard { tracing_subscriber::fmt() .with_test_writer() @@ -64,10 +79,11 @@ fn init_logger() -> tracing::subscriber::DefaultGuard { async fn test_provision_credentials_success() -> anyhow::Result<()> { let _guard = init_logger(); - let (app, state, _handles) = make_router()?; + let (app, _state, _handles) = make_router()?; - let token_id = Uuid::from_str("5e3e833f-84c7-4541-b676-acc3299e39b8").unwrap(); - let token = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiI1ZTNlODMzZi04NGM3LTQ1NDEtYjY3Ni1hY2MzMjk5ZTM5YjgifQ.1qECGlrW7y9HWFArc6GPHLGTOY7PhAvzKJ5XMRBg4k4"; + // JWT payload includes `dst_hst` because credential injection requires a target hostname + // (fake-KDC validates TGS-REQ sname against `TERMSRV/`); insert rejects tokens without it. + let token = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiI1ZTNlODMzZi04NGM3LTQ1NDEtYjY3Ni1hY2MzMjk5ZTM5YjgiLCJkc3RfaHN0IjoidGFyZ2V0LmV4YW1wbGU6MzM4OSJ9.1qECGlrW7y9HWFArc6GPHLGTOY7PhAvzKJ5XMRBg4k4"; let op_id = Uuid::new_v4(); @@ -89,18 +105,85 @@ async fn test_provision_credentials_success() -> anyhow::Result<()> { let body: serde_json::Value = serde_json::from_slice(&body)?; assert_eq!(body.as_array().expect("an array").len(), 1); assert_eq!(body[0]["operation_id"], op_id.to_string()); - assert_eq!(body[0]["kind"], "ack", "{:?}", body[1]); + assert_eq!(body[0]["kind"], "ack", "{:?}", body[0]); + + Ok(()) +} - let entry = state.credential_store.get(token_id).expect("the provisioned entry"); - assert_eq!(entry.token, token); +#[tokio::test] +async fn test_provision_credentials_success_when_unstable_disabled() -> anyhow::Result<()> { + let _guard = init_logger(); - let now = time::OffsetDateTime::now_utc(); - assert!(now + time::Duration::seconds(10) < entry.expires_at); - assert!(entry.expires_at < now + time::Duration::seconds(20)); + let config = CONFIG.replace("\"enable_unstable\": true", "\"enable_unstable\": false"); + let (app, _state, _handles) = make_router_with_config(&config)?; - let mapping = entry.mapping.as_ref().expect("the provisioned mapping"); - assert!(matches!(mapping.proxy, AppCredential::UsernamePassword { .. })); - assert!(matches!(mapping.target, AppCredential::UsernamePassword { .. })); + // `provision-credentials` is protocol-neutral: NTLM credential injection relies on this + // preflight state even when the Kerberos injection path is disabled. + let token = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiI1ZTNlODMzZi04NGM3LTQ1NDEtYjY3Ni1hY2MzMjk5ZTM5YjgiLCJkc3RfaHN0IjoidGFyZ2V0LmV4YW1wbGU6MzM4OSJ9.1qECGlrW7y9HWFArc6GPHLGTOY7PhAvzKJ5XMRBg4k4"; + + let op_id = Uuid::new_v4(); + + let op = json!([{ + "id": op_id, + "kind": "provision-credentials", + "token": token, + "proxy_credential": { "kind": "username-password", "username": "proxy_user", "password": "secret1" }, + "target_credential": { "kind": "username-password", "username": "target_user", "password": "secret2" }, + "time_to_live": 15 + }]); + + let response = app.oneshot(preflight_request(op)?).await.unwrap(); + assert_eq!(response.status(), StatusCode::OK); + + let body = response.into_body().collect().await?.to_bytes(); + let body: serde_json::Value = serde_json::from_slice(&body)?; + assert_eq!(body.as_array().expect("an array").len(), 1); + assert_eq!(body[0]["operation_id"], op_id.to_string()); + assert_eq!(body[0]["kind"], "ack", "{:?}", body[0]); + + Ok(()) +} + +#[tokio::test] +async fn test_provision_credentials_rejects_alternate_targets() -> anyhow::Result<()> { + let _guard = init_logger(); + + let (app, _state, _handles) = make_router()?; + + let token = unsigned_jws(json!({ + "jti": "5e3e833f-84c7-4541-b676-acc3299e39b8", + "dst_hst": "target-primary.example:3389", + "dst_alt": ["target-secondary.example:3389"] + }))?; + + let op_id = Uuid::new_v4(); + + let op = json!([{ + "id": op_id, + "kind": "provision-credentials", + "token": token, + "proxy_credential": { "kind": "username-password", "username": "proxy_user", "password": "secret1" }, + "target_credential": { "kind": "username-password", "username": "target_user", "password": "secret2" }, + "time_to_live": 15 + }]); + + let response = app.oneshot(preflight_request(op)?).await?; + assert_eq!(response.status(), StatusCode::OK); + + let body = response.into_body().collect().await?.to_bytes(); + let body: serde_json::Value = serde_json::from_slice(&body)?; + + assert_eq!(body.as_array().expect("an array").len(), 1); + assert_eq!(body[0]["kind"], "alert"); + assert_eq!(body[0]["alert_status"], "invalid-parameters"); + assert!( + body[0]["alert_message"] + .as_str() + .expect("alert message") + .contains("dst_alt"), + "{:?}", + body[0] + ); Ok(()) } diff --git a/utils/dotnet/Devolutions.Gateway.Utils/src/AssociationClaims.cs b/utils/dotnet/Devolutions.Gateway.Utils/src/AssociationClaims.cs index eca9a1f29..f07c6a4ae 100644 --- a/utils/dotnet/Devolutions.Gateway.Utils/src/AssociationClaims.cs +++ b/utils/dotnet/Devolutions.Gateway.Utils/src/AssociationClaims.cs @@ -50,4 +50,4 @@ public string GetContentType() { return "ASSOCIATION"; } -} \ No newline at end of file +} diff --git a/utils/dotnet/Devolutions.Gateway.Utils/src/KdcClaims.cs b/utils/dotnet/Devolutions.Gateway.Utils/src/KdcClaims.cs index c08f43f4f..668123fa1 100644 --- a/utils/dotnet/Devolutions.Gateway.Utils/src/KdcClaims.cs +++ b/utils/dotnet/Devolutions.Gateway.Utils/src/KdcClaims.cs @@ -47,4 +47,4 @@ public string GetContentType() { return "KDC"; } -} \ No newline at end of file +} diff --git a/utils/dotnet/Devolutions.Gateway.Utils/src/ProvisionCredentialsRequest.cs b/utils/dotnet/Devolutions.Gateway.Utils/src/ProvisionCredentialsRequest.cs new file mode 100644 index 000000000..ccef5a0dd --- /dev/null +++ b/utils/dotnet/Devolutions.Gateway.Utils/src/ProvisionCredentialsRequest.cs @@ -0,0 +1,57 @@ +using System.Text.Json.Serialization; + +namespace Devolutions.Gateway.Utils; + +public class ProvisionCredentialsRequest +{ + [JsonPropertyName("id")] + public Guid Id { get; set; } + + [JsonPropertyName("kind")] + public string Kind => "provision-credentials"; + + [JsonPropertyName("token")] + public string Token { get; set; } + + [JsonPropertyName("proxy_credential")] + public CleartextCredential ProxyCredential { get; set; } + + [JsonPropertyName("target_credential")] + public CleartextCredential TargetCredential { get; set; } + + [JsonPropertyName("time_to_live")] + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public uint? TimeToLive { get; set; } + + public ProvisionCredentialsRequest( + Guid id, + string token, + CleartextCredential proxyCredential, + CleartextCredential targetCredential, + uint? timeToLive = null) + { + this.Id = id; + this.Token = token; + this.ProxyCredential = proxyCredential; + this.TargetCredential = targetCredential; + this.TimeToLive = timeToLive; + } +} + +public class CleartextCredential +{ + [JsonPropertyName("kind")] + public string Kind => "username-password"; + + [JsonPropertyName("username")] + public string Username { get; set; } + + [JsonPropertyName("password")] + public string Password { get; set; } + + public CleartextCredential(string username, string password) + { + this.Username = username; + this.Password = password; + } +} From 023ce5660ed50a9c0a8ef264f45973cd3197eafd Mon Sep 17 00:00:00 2001 From: irving ou Date: Fri, 15 May 2026 19:01:12 -0400 Subject: [PATCH 2/5] refactor(dgw): keep credential store protocol-neutral --- devolutions-gateway/src/api/preflight.rs | 90 +-------- devolutions-gateway/src/api/rdp.rs | 4 - devolutions-gateway/src/credential/mod.rs | 147 ++++++--------- devolutions-gateway/src/credential/tests.rs | 131 ------------- .../src/credential_injection_kdc.rs | 177 +++++++++++++----- devolutions-gateway/src/generic_client.rs | 9 +- devolutions-gateway/src/listener.rs | 1 - devolutions-gateway/src/ngrok.rs | 1 - devolutions-gateway/src/rd_clean_path.rs | 11 +- devolutions-gateway/src/rdp_proxy.rs | 2 +- devolutions-gateway/src/token.rs | 6 +- 11 files changed, 212 insertions(+), 367 deletions(-) delete mode 100644 devolutions-gateway/src/credential/tests.rs diff --git a/devolutions-gateway/src/api/preflight.rs b/devolutions-gateway/src/api/preflight.rs index 924962535..c8309face 100644 --- a/devolutions-gateway/src/api/preflight.rs +++ b/devolutions-gateway/src/api/preflight.rs @@ -11,8 +11,7 @@ use uuid::Uuid; use crate::DgwState; use crate::config::Conf; -use crate::credential::CredentialStoreHandle; -use crate::credential_injection_kdc::CredentialInjectionKdcSessionStoreHandle; +use crate::credential::{CredentialStoreHandle, InsertError}; use crate::extract::PreflightScope; use crate::http::HttpError; use crate::session::SessionMessageSender; @@ -197,7 +196,6 @@ pub(super) async fn post_preflight( conf_handle, sessions, credential_store, - kerberos_session_store, .. }): State, _scope: PreflightScope, @@ -225,21 +223,12 @@ pub(super) async fn post_preflight( let conf = conf_handle.get_conf(); let sessions = sessions.clone(); let credential_store = credential_store.clone(); - let kerberos_session_store = kerberos_session_store.clone(); async move { let operation_id = operation.id; trace!(%operation.id, "Process preflight operation"); - if let Err(error) = handle_operation( - operation, - &outputs, - &conf, - &sessions, - &credential_store, - &kerberos_session_store, - ) - .await + if let Err(error) = handle_operation(operation, &outputs, &conf, &sessions, &credential_store).await { outputs.push(PreflightOutput { operation_id, @@ -268,7 +257,6 @@ async fn handle_operation( conf: &Conf, sessions: &SessionMessageSender, credential_store: &CredentialStoreHandle, - kerberos_session_store: &CredentialInjectionKdcSessionStoreHandle, ) -> Result<(), PreflightError> { match operation.kind.as_str() { OP_GET_VERSION => outputs.push(PreflightOutput { @@ -349,77 +337,17 @@ async fn handle_operation( }); } - // Token signature isn't validated at the preflight scope (DVLS signs and pushes; - // gateway only re-uses what DVLS already verified). We do parse JTI / dst_hst here - // because the credential store no longer touches JWT shape — see the contract on - // `CredentialStoreHandle::insert`. - let jti = crate::token::extract_jti(&token).map_err(|error| { - PreflightError::new(PreflightAlertStatus::InvalidParams, format!("invalid token: {error:#}")) - })?; - - // For `provision-credentials`: resolve `target_hostname` from `dst_hst` *before* - // calling either store, so an invalid target is reported as `invalid-parameters` - // at preflight rather than mid-CredSSP. Also derive per-session Kerberos material - // from the proxy username; both stores receive their entries paired and keyed by - // the same JTI. - let injection_payload = if let Some(mapping) = mapping { - const DEFAULT_DST_PORT: u16 = 3389; - let raw_dst_hst = crate::token::extract_dst_hst(&token) - .map_err(|error| { - PreflightError::new( - PreflightAlertStatus::InvalidParams, - format!("read dst_hst from association token: {error:#}"), - ) - })? - .ok_or_else(|| { - PreflightError::new( - PreflightAlertStatus::InvalidParams, - "association token has no dst_hst, required for credential injection", - ) - })?; - - let dst_alt = crate::token::extract_dst_alt(&token).map_err(|error| { - PreflightError::new( - PreflightAlertStatus::InvalidParams, - format!("read dst_alt from association token: {error:#}"), - ) - })?; - if !dst_alt.is_empty() { - return Err(PreflightError::new( - PreflightAlertStatus::InvalidParams, - "association token dst_alt is not supported for credential injection", - )); - } - - let target_hostname = crate::target_addr::TargetAddr::parse(&raw_dst_hst, DEFAULT_DST_PORT) - .map_err(|error| { - PreflightError::new( - PreflightAlertStatus::InvalidParams, - format!("parse dst_hst as target address: {error:#}"), - ) - })? - .host() - .to_owned(); - - let kdc_session = crate::credential_injection_kdc::derive_credential_injection_kdc_session( - mapping.proxy_username(), - jti, - ); - kerberos_session_store.insert(kdc_session, time_to_live); - - Some((mapping, target_hostname)) - } else { - None - }; - let previous_entry = credential_store - .insert(jti, token, injection_payload, time_to_live) + .insert(token, mapping, time_to_live) .inspect_err(|error| warn!(%operation.id, error = format!("{error:#}"), "Failed to insert credentials")) - .map_err(|_| { - PreflightError::new( + .map_err(|error| match error { + InsertError::InvalidToken(error) => { + PreflightError::new(PreflightAlertStatus::InvalidParams, format!("invalid token: {error:#}")) + } + InsertError::Internal(_) => PreflightError::new( PreflightAlertStatus::InternalServerError, "an internal error occurred".to_owned(), - ) + ), })?; if previous_entry.is_some() { diff --git a/devolutions-gateway/src/api/rdp.rs b/devolutions-gateway/src/api/rdp.rs index b684a28d5..6129a776c 100644 --- a/devolutions-gateway/src/api/rdp.rs +++ b/devolutions-gateway/src/api/rdp.rs @@ -26,7 +26,6 @@ pub async fn handler( recordings, shutdown_signal, credential_store, - kerberos_session_store, agent_tunnel_handle, .. }): State, @@ -48,7 +47,6 @@ pub async fn handler( recordings.active_recordings, source_addr, credential_store, - kerberos_session_store, agent_tunnel_handle, ) .instrument(span) @@ -69,7 +67,6 @@ async fn handle_socket( active_recordings: Arc, source_addr: SocketAddr, credential_store: crate::credential::CredentialStoreHandle, - kerberos_session_store: crate::credential_injection_kdc::CredentialInjectionKdcSessionStoreHandle, agent_tunnel_handle: Option>, ) { let (stream, close_handle) = crate::ws::handle( @@ -88,7 +85,6 @@ async fn handle_socket( subscriber_tx, &active_recordings, &credential_store, - &kerberos_session_store, agent_tunnel_handle, ) .await; diff --git a/devolutions-gateway/src/credential/mod.rs b/devolutions-gateway/src/credential/mod.rs index fea6829d6..166be9412 100644 --- a/devolutions-gateway/src/credential/mod.rs +++ b/devolutions-gateway/src/credential/mod.rs @@ -4,16 +4,40 @@ mod crypto; pub use crypto::EncryptedPassword; use std::collections::HashMap; +use std::fmt; use std::sync::Arc; +use anyhow::Context; use async_trait::async_trait; use devolutions_gateway_task::{ShutdownSignal, Task}; use parking_lot::Mutex; -use secrecy::{ExposeSecret as _, SecretString}; +use secrecy::ExposeSecret as _; use uuid::Uuid; use self::crypto::MASTER_KEY; +/// Error returned by [`CredentialStoreHandle::insert`]. +#[derive(Debug)] +pub enum InsertError { + /// The provided token is invalid (e.g., missing or malformed JTI). + /// + /// This is a client-side error: the caller supplied bad input. + InvalidToken(anyhow::Error), + /// An internal error occurred (e.g., encryption failure). + Internal(anyhow::Error), +} + +impl fmt::Display for InsertError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::InvalidToken(e) => e.fmt(f), + Self::Internal(e) => e.fmt(f), + } + } +} + +impl std::error::Error for InsertError {} + /// Credential at the application protocol level #[derive(Debug, Clone)] pub enum AppCredential { @@ -24,16 +48,10 @@ pub enum AppCredential { } impl AppCredential { - pub(crate) fn username(&self) -> &str { - match self { - AppCredential::UsernamePassword { username, password: _ } => username, - } - } - /// Decrypt the password using the global master key. /// /// Returns the username and a short-lived decrypted password that zeroizes on drop. - pub fn decrypt_password(&self) -> anyhow::Result<(String, SecretString)> { + pub fn decrypt_password(&self) -> anyhow::Result<(String, secrecy::SecretString)> { match self { AppCredential::UsernamePassword { username, password } => { let decrypted = MASTER_KEY.lock().decrypt(password)?; @@ -56,9 +74,12 @@ pub struct AppCredentialMapping { /// This type is never stored directly — hand it to [`CredentialStoreHandle::insert`]. #[derive(Debug, Deserialize)] #[serde(tag = "kind")] -pub(crate) enum CleartextAppCredential { +pub enum CleartextAppCredential { #[serde(rename = "username-password")] - UsernamePassword { username: String, password: SecretString }, + UsernamePassword { + username: String, + password: secrecy::SecretString, + }, } impl CleartextAppCredential { @@ -79,30 +100,20 @@ impl CleartextAppCredential { /// /// Passwords are encrypted on write. Hand this directly to [`CredentialStoreHandle::insert`]. #[derive(Debug, Deserialize)] -pub(crate) struct CleartextAppCredentialMapping { +pub struct CleartextAppCredentialMapping { #[serde(rename = "proxy_credential")] - pub(crate) proxy: CleartextAppCredential, + pub proxy: CleartextAppCredential, #[serde(rename = "target_credential")] - pub(crate) target: CleartextAppCredential, + pub target: CleartextAppCredential, } impl CleartextAppCredentialMapping { - pub(crate) fn encrypt(self) -> anyhow::Result { + fn encrypt(self) -> anyhow::Result { Ok(AppCredentialMapping { proxy: self.proxy.encrypt()?, target: self.target.encrypt()?, }) } - - /// Expose the proxy-side username without dropping ownership of the rest of the mapping. - /// - /// Used by the preflight layer to derive per-session Kerberos material before the mapping - /// is moved into the credential store. The password is not exposed. - pub(crate) fn proxy_username(&self) -> &str { - match &self.proxy { - CleartextAppCredential::UsernamePassword { username, password: _ } => username, - } - } } #[derive(Debug, Clone)] @@ -119,32 +130,21 @@ impl CredentialStoreHandle { Self(Arc::new(Mutex::new(CredentialStore::new()))) } - /// Insert a credential entry for the association token whose JTI is `jti`. - /// - /// The caller is responsible for extracting `jti` from a token whose signature has already - /// been validated upstream. `injection` carries the optional `provision-credentials` payload: - /// the cleartext mapping and the parsed RDP target hostname. The store has no other - /// dependency on the token's JWT shape. - pub(crate) fn insert( + pub fn insert( &self, - jti: Uuid, token: String, - injection: Option<(CleartextAppCredentialMapping, String)>, + mapping: Option, time_to_live: time::Duration, - ) -> anyhow::Result> { - let injection = injection - .map(|(mapping, target_hostname)| -> anyhow::Result { - Ok(InjectionState { - mapping: mapping.encrypt()?, - target_hostname, - }) - }) - .transpose()?; - Ok(self.0.lock().insert(jti, token, injection, time_to_live)) + ) -> Result, InsertError> { + let mapping = mapping + .map(CleartextAppCredentialMapping::encrypt) + .transpose() + .map_err(InsertError::Internal)?; + self.0.lock().insert(token, mapping, time_to_live) } - pub(crate) fn get(&self, jti: Uuid) -> Option { - self.0.lock().get(jti) + pub fn get(&self, token_id: Uuid) -> Option { + self.0.lock().get(token_id) } } @@ -154,31 +154,13 @@ struct CredentialStore { } #[derive(Debug)] -pub(crate) struct CredentialEntry { - /// The association token's JTI. Doubles as the credential-store key and as the value the - /// matching KDC token's `jet_cred_id` claim points back to. - pub(crate) jti: Uuid, - pub(crate) token: String, - pub(crate) expires_at: time::OffsetDateTime, - /// Credential-injection state for this entry, set when (and only when) the entry was - /// provisioned via `provision-credentials`. Plain `provision-token` entries leave this `None` - /// and are inert from the routing layer's perspective. Grouping the three correlated fields - /// into one option captures the "all set or all unset" invariant in the type system. - pub(crate) injection: Option, -} - -#[derive(Debug)] -pub(crate) struct InjectionState { - pub(crate) mapping: AppCredentialMapping, - /// Hostname of the target RDP server, parsed from the association token's `dst_hst` claim - /// at preflight. Fake-KDC validates client TGS-REQ sname against `TERMSRV/`; - /// without it the SPN check would silently fall back to Gateway's own hostname, so the - /// preflight handler rejects `provision-credentials` requests with missing/malformed `dst_hst` - /// or with alternate targets (`dst_alt`) until credential injection supports target failover. - pub(crate) target_hostname: String, +pub struct CredentialEntry { + pub token: String, + pub mapping: Option, + pub expires_at: time::OffsetDateTime, } -pub(crate) type ArcCredentialEntry = Arc; +pub type ArcCredentialEntry = Arc; impl CredentialStore { fn new() -> Self { @@ -189,29 +171,27 @@ impl CredentialStore { fn insert( &mut self, - jti: Uuid, token: String, - injection: Option, + mapping: Option, time_to_live: time::Duration, - ) -> Option { + ) -> Result, InsertError> { + let jti = crate::token::extract_jti(&token) + .context("failed to extract token ID") + .map_err(InsertError::InvalidToken)?; + let entry = CredentialEntry { - jti, token, + mapping, expires_at: time::OffsetDateTime::now_utc() + time_to_live, - injection, }; - self.entries.insert(jti, Arc::new(entry)) + let previous_entry = self.entries.insert(jti, Arc::new(entry)); + + Ok(previous_entry) } - fn get(&self, jti: Uuid) -> Option { - // Filter expired entries at lookup. The 15-minute cleanup task is best-effort; - // without this filter an expired entry remains usable until the next sweep, which - // makes `time_to_live` a soft hint rather than a hard limit. - self.entries - .get(&jti) - .filter(|entry| time::OffsetDateTime::now_utc() < entry.expires_at) - .map(Arc::clone) + fn get(&self, token_id: Uuid) -> Option { + self.entries.get(&token_id).map(Arc::clone) } } @@ -254,6 +234,3 @@ async fn cleanup_task(handle: CredentialStoreHandle, mut shutdown_signal: Shutdo debug!("Task terminated"); } - -#[cfg(test)] -mod tests; diff --git a/devolutions-gateway/src/credential/tests.rs b/devolutions-gateway/src/credential/tests.rs deleted file mode 100644 index f970db617..000000000 --- a/devolutions-gateway/src/credential/tests.rs +++ /dev/null @@ -1,131 +0,0 @@ -use super::*; - -fn cleartext_mapping(proxy_username: &str) -> CleartextAppCredentialMapping { - CleartextAppCredentialMapping { - proxy: CleartextAppCredential::UsernamePassword { - username: proxy_username.to_owned(), - password: SecretString::from("proxy-password"), - }, - target: CleartextAppCredential::UsernamePassword { - username: "target@example.invalid".to_owned(), - password: SecretString::from("target-password"), - }, - } -} - -#[test] -fn insert_indexes_by_jti() { - let store = CredentialStoreHandle::new(); - let jti = Uuid::new_v4(); - - let previous = store - .insert( - jti, - "raw-token".to_owned(), - Some((cleartext_mapping("proxy@example.invalid"), "target.example".to_owned())), - time::Duration::minutes(5), - ) - .expect("insert succeeds"); - - assert!(previous.is_none()); - let entry = store.get(jti).expect("entry is indexed by JTI"); - assert_eq!(entry.jti, jti); - assert_eq!(entry.token, "raw-token"); - assert_eq!( - entry - .injection - .as_ref() - .expect("injection state present") - .target_hostname, - "target.example" - ); -} - -#[test] -fn re_insert_under_same_jti_replaces_entry() { - let store = CredentialStoreHandle::new(); - let jti = Uuid::new_v4(); - - store - .insert( - jti, - "token-a".to_owned(), - Some((cleartext_mapping("proxy@example.invalid"), "host-a".to_owned())), - time::Duration::minutes(5), - ) - .expect("first insert succeeds"); - let previous = store - .insert( - jti, - "token-b".to_owned(), - Some((cleartext_mapping("proxy@example.invalid"), "host-b".to_owned())), - time::Duration::minutes(5), - ) - .expect("second insert succeeds"); - - let previous = previous.expect("replacement must report previous entry"); - assert_eq!(previous.jti, jti); - assert_eq!(previous.token, "token-a"); - - let current = store.get(jti).expect("replacement entry present"); - assert_eq!(current.token, "token-b"); -} - -#[test] -fn distinct_jtis_coexist() { - let store = CredentialStoreHandle::new(); - let first_jti = Uuid::new_v4(); - let second_jti = Uuid::new_v4(); - - store - .insert( - first_jti, - "token-1".to_owned(), - Some((cleartext_mapping("proxy@example.invalid"), "h1".to_owned())), - time::Duration::minutes(5), - ) - .expect("first insert succeeds"); - store - .insert( - second_jti, - "token-2".to_owned(), - Some((cleartext_mapping("proxy@example.invalid"), "h2".to_owned())), - time::Duration::minutes(5), - ) - .expect("second insert succeeds"); - - assert_eq!(store.get(first_jti).expect("first entry").jti, first_jti); - assert_eq!(store.get(second_jti).expect("second entry").jti, second_jti); -} - -#[test] -fn lookup_filters_expired_entries() { - let store = CredentialStoreHandle::new(); - let jti = Uuid::new_v4(); - - store - .insert( - jti, - "raw-token".to_owned(), - Some((cleartext_mapping("proxy@example.invalid"), "host".to_owned())), - // Negative TTL: entry is born already expired. Validates the lookup-time filter - // without depending on real elapsed time in tests. - time::Duration::seconds(-1), - ) - .expect("insert succeeds"); - - assert!(store.get(jti).is_none(), "expired entry must not be returned"); -} - -#[test] -fn provision_token_entry_has_no_injection_state() { - let store = CredentialStoreHandle::new(); - let jti = Uuid::new_v4(); - - store - .insert(jti, "raw-token".to_owned(), None, time::Duration::minutes(5)) - .expect("insert succeeds"); - - let entry = store.get(jti).expect("entry exists"); - assert!(entry.injection.is_none()); -} diff --git a/devolutions-gateway/src/credential_injection_kdc.rs b/devolutions-gateway/src/credential_injection_kdc.rs index d762a24a6..38308f8f7 100644 --- a/devolutions-gateway/src/credential_injection_kdc.rs +++ b/devolutions-gateway/src/credential_injection_kdc.rs @@ -26,9 +26,8 @@ use thiserror::Error; use url::Url; use uuid::Uuid; -use crate::credential::{ - AppCredential, AppCredentialMapping, ArcCredentialEntry, CredentialStoreHandle, InjectionState, -}; +use crate::credential::{AppCredential, AppCredentialMapping, ArcCredentialEntry, CredentialStoreHandle}; +use crate::target_addr::TargetAddr; const IN_PROCESS_KDC_HOST: &str = "cred.invalid"; @@ -51,8 +50,12 @@ pub(crate) enum CredentialInjectionKdcResolveError { MissingCredential { jti: Uuid }, #[error("credential-injection state is not available for {jti}")] NonInjectionCredential { jti: Uuid }, - #[error("credential-injection Kerberos state is not available for {jti}")] - MissingKerberosSession { jti: Uuid }, + #[error("association token for {jti} is not valid for credential injection")] + InvalidAssociationToken { + jti: Uuid, + #[source] + source: anyhow::Error, + }, #[error("credential-injection KDC config could not be initialized for {jti}")] BuildKdcConfig { jti: Uuid, @@ -114,29 +117,42 @@ impl fmt::Debug for CredentialInjectionKdc { } impl CredentialInjectionKdc { - pub(crate) fn from_parts( + pub(crate) fn from_entry(jti: Uuid, credential_entry: ArcCredentialEntry) -> anyhow::Result { + let target_hostname = target_hostname_from_token(&credential_entry.token)?; + let mapping = credential_entry + .mapping + .as_ref() + .context("credential entry has no credential-injection mapping")?; + let session = Arc::new(derive_credential_injection_kdc_session( + app_credential_username(&mapping.proxy), + jti, + )); + + Self::from_parts(jti, credential_entry, target_hostname, session) + } + + fn from_parts( + jti: Uuid, credential_entry: ArcCredentialEntry, + target_hostname: String, session: Arc, ) -> anyhow::Result { - let InjectionState { - mapping, - target_hostname, - } = credential_entry - .injection + let mapping = credential_entry + .mapping .as_ref() - .context("credential entry has no credential-injection state")?; + .context("credential entry has no credential-injection mapping")?; anyhow::ensure!( - credential_entry.jti == session.jti, + jti == session.jti, "credential entry JTI does not match credential-injection KDC session JTI", ); let kdc_config = build_kdc_config(&session, &mapping.proxy)?; Ok(Self { - jti: credential_entry.jti, + jti, raw_token: credential_entry.token.clone(), credential_mapping: mapping.clone(), - target_hostname: target_hostname.clone(), + target_hostname, session, kdc_config, }) @@ -156,17 +172,19 @@ impl CredentialInjectionKdc { CredentialInjectionKdcResolveError::MissingCredential { jti } })?; - if credential_entry.injection.is_none() { + let Some(mapping) = credential_entry.mapping.as_ref() else { warn!(%jti, "KDC token references non-injection credential state"); return Err(CredentialInjectionKdcResolveError::NonInjectionCredential { jti }); - } + }; - let session = session_store.get(jti).ok_or_else(|| { - warn!(%jti, "KDC token references missing credential-injection Kerberos state"); - CredentialInjectionKdcResolveError::MissingKerberosSession { jti } - })?; + let target_hostname = target_hostname_from_token(&credential_entry.token) + .map_err(|source| CredentialInjectionKdcResolveError::InvalidAssociationToken { jti, source })?; + let proxy_username = app_credential_username(&mapping.proxy).to_owned(); + let session = session_store.get_or_insert_with(jti, credential_entry.expires_at, || { + derive_credential_injection_kdc_session(&proxy_username, jti) + }); - let kdc = Self::from_parts(credential_entry, session) + let kdc = Self::from_parts(jti, credential_entry, target_hostname, session) .map_err(|source| CredentialInjectionKdcResolveError::BuildKdcConfig { jti, source })?; Ok(Some(Box::new(kdc))) @@ -194,8 +212,8 @@ impl CredentialInjectionKdc { /// Domainless target credentials cannot acquire Kerberos tickets. /// Enabling the Kerberos acceptor for those sessions would make incoming NTLMSSP tokens fail in Kerberos parsing. pub(crate) fn client_acceptor_protocol(&self) -> anyhow::Result { - let target_username = - sspi::Username::parse(self.target_credential().username()).context("invalid target credential username")?; + let target_username = sspi::Username::parse(app_credential_username(self.target_credential())) + .context("invalid target credential username")?; if target_username.domain_name().is_some() { Ok(CredentialInjectionClientAcceptorProtocol::Kerberos) @@ -294,6 +312,31 @@ impl CredentialInjectionKdc { } } +fn target_hostname_from_token(token: &str) -> anyhow::Result { + const DEFAULT_DST_PORT: u16 = 3389; + + let dst_alt = crate::token::extract_dst_alt(token).context("read dst_alt from association token")?; + anyhow::ensure!( + dst_alt.is_empty(), + "association token dst_alt is not supported for credential injection", + ); + + let raw_dst_hst = crate::token::extract_dst_hst(token) + .context("read dst_hst from association token")? + .context("association token has no dst_hst, required for credential injection")?; + + Ok(TargetAddr::parse(&raw_dst_hst, DEFAULT_DST_PORT) + .context("parse dst_hst as target address")? + .host() + .to_owned()) +} + +fn app_credential_username(credential: &AppCredential) -> &str { + match credential { + AppCredential::UsernamePassword { username, password: _ } => username, + } +} + pub(crate) fn kdc_proxy_message_realm(kdc_proxy_message: &KdcProxyMessage) -> Option { kdc_proxy_message .target_domain @@ -453,11 +496,11 @@ fn random_32_bytes() -> Vec { bytes } -/// Parallel store of [`CredentialInjectionKdcSession`] entries keyed by association-token JTI. +/// Lazy store of [`CredentialInjectionKdcSession`] entries keyed by association-token JTI. /// -/// Pairs 1:1 with [`crate::credential::CredentialStoreHandle`]: a credential entry provisioned -/// with `provision-credentials` has a matching entry here under the same JTI. The two stores -/// share neither lock nor map so that the credential store stays Kerberos-unaware. +/// Sessions are created on first KDC-proxy use from the credential entry and its original +/// association token. The stores share neither lock nor map so that the credential store stays +/// Kerberos-unaware. #[derive(Debug, Clone, Default)] pub struct CredentialInjectionKdcSessionStoreHandle(Arc>>); @@ -472,6 +515,7 @@ impl CredentialInjectionKdcSessionStoreHandle { Self(Arc::new(Mutex::new(HashMap::new()))) } + #[cfg(test)] pub(crate) fn insert(&self, session: CredentialInjectionKdcSession, time_to_live: time::Duration) { let jti = session.jti; let entry = Entry { @@ -481,6 +525,7 @@ impl CredentialInjectionKdcSessionStoreHandle { self.0.lock().insert(jti, entry); } + #[cfg(test)] pub(crate) fn get(&self, jti: Uuid) -> Option> { // Lookup-time TTL enforcement mirrors `CredentialStoreHandle::get`: the cleanup task is // best-effort, and we don't want to hand out Kerberos material whose paired credential @@ -491,6 +536,30 @@ impl CredentialInjectionKdcSessionStoreHandle { .filter(|entry| time::OffsetDateTime::now_utc() < entry.expires_at) .map(|entry| Arc::clone(&entry.session)) } + + pub(crate) fn get_or_insert_with( + &self, + jti: Uuid, + expires_at: time::OffsetDateTime, + make_session: impl FnOnce() -> CredentialInjectionKdcSession, + ) -> Arc { + let now = time::OffsetDateTime::now_utc(); + let mut entries = self.0.lock(); + + if let Some(entry) = entries.get(&jti).filter(|entry| now < entry.expires_at) { + return Arc::clone(&entry.session); + } + + let session = Arc::new(make_session()); + entries.insert( + jti, + Entry { + session: Arc::clone(&session), + expires_at, + }, + ); + session + } } pub struct CleanupTask { @@ -534,13 +603,12 @@ async fn cleanup_task(handle: CredentialInjectionKdcSessionStoreHandle, mut shut #[cfg(test)] mod tests { + use base64::Engine as _; use ironrdp_connector::sspi::network_client::NetworkProtocol; use secrecy::SecretString; use super::*; - use crate::credential::{ - AppCredentialMapping, CleartextAppCredential, CleartextAppCredentialMapping, CredentialEntry, - }; + use crate::credential::{CleartextAppCredential, CleartextAppCredentialMapping}; fn cleartext_mapping_with_target_username(target_username: &str) -> CleartextAppCredentialMapping { CleartextAppCredentialMapping { @@ -555,19 +623,32 @@ mod tests { } } + fn unsigned_jws(payload: serde_json::Value) -> String { + let engine = base64::engine::general_purpose::URL_SAFE_NO_PAD; + let header = engine.encode(r#"{"alg":"RS256"}"#); + let payload = engine.encode(serde_json::to_vec(&payload).expect("payload serializes")); + let signature = engine.encode(b"signature"); + format!("{header}.{payload}.{signature}") + } + + fn association_token(jti: Uuid) -> String { + unsigned_jws(serde_json::json!({ + "jti": jti, + "dst_hst": "target.example:3389" + })) + } + fn dummy_entry_with_target_username(jti: Uuid, target_username: &str) -> ArcCredentialEntry { - let mapping: AppCredentialMapping = cleartext_mapping_with_target_username(target_username) - .encrypt() - .expect("encrypt mapping"); - Arc::new(CredentialEntry { - jti, - token: "raw-token".to_owned(), - expires_at: time::OffsetDateTime::now_utc() + time::Duration::minutes(5), - injection: Some(InjectionState { - mapping, - target_hostname: "target.example".to_owned(), - }), - }) + let store = CredentialStoreHandle::new(); + store + .insert( + association_token(jti), + Some(cleartext_mapping_with_target_username(target_username)), + time::Duration::minutes(5), + ) + .expect("credential entry inserts"); + + store.get(jti).expect("credential entry is indexed by JTI") } fn dummy_entry(jti: Uuid) -> ArcCredentialEntry { @@ -577,13 +658,15 @@ mod tests { fn dummy_kdc(jti: Uuid) -> CredentialInjectionKdc { let entry = dummy_entry(jti); let session = Arc::new(derive_credential_injection_kdc_session("proxy@example.invalid", jti)); - CredentialInjectionKdc::from_parts(entry, session).expect("valid credential-injection KDC") + CredentialInjectionKdc::from_parts(jti, entry, "target.example".to_owned(), session) + .expect("valid credential-injection KDC") } fn dummy_kdc_with_target_username(jti: Uuid, target_username: &str) -> CredentialInjectionKdc { let entry = dummy_entry_with_target_username(jti, target_username); let session = Arc::new(derive_credential_injection_kdc_session("proxy@example.invalid", jti)); - CredentialInjectionKdc::from_parts(entry, session).expect("valid credential-injection KDC") + CredentialInjectionKdc::from_parts(jti, entry, "target.example".to_owned(), session) + .expect("valid credential-injection KDC") } fn network_request(url: &str) -> NetworkRequest { @@ -684,7 +767,7 @@ mod tests { session_jti, )); - let err = CredentialInjectionKdc::from_parts(entry, session) + let err = CredentialInjectionKdc::from_parts(entry_jti, entry, "target.example".to_owned(), session) .expect_err("mismatched entry/session JTI must fail closed"); let msg = format!("{err:#}"); assert!( @@ -711,7 +794,7 @@ mod tests { let jti = Uuid::new_v4(); credential_store - .insert(jti, "raw-token".to_owned(), None, time::Duration::minutes(5)) + .insert(association_token(jti), None, time::Duration::minutes(5)) .expect("provision-token entry inserts"); assert!( diff --git a/devolutions-gateway/src/generic_client.rs b/devolutions-gateway/src/generic_client.rs index 7966b946e..117eb41bf 100644 --- a/devolutions-gateway/src/generic_client.rs +++ b/devolutions-gateway/src/generic_client.rs @@ -9,7 +9,7 @@ use typed_builder::TypedBuilder; use crate::config::Conf; use crate::credential::CredentialStoreHandle; -use crate::credential_injection_kdc::{CredentialInjectionKdc, CredentialInjectionKdcSessionStoreHandle}; +use crate::credential_injection_kdc::CredentialInjectionKdc; use crate::proxy::Proxy; use crate::rdp_pcb::{extract_association_claims, read_pcb}; use crate::recording::ActiveRecordings; @@ -29,7 +29,6 @@ pub struct GenericClient { subscriber_tx: SubscriberSender, active_recordings: Arc, credential_store: CredentialStoreHandle, - kerberos_session_store: CredentialInjectionKdcSessionStoreHandle, #[builder(default)] agent_tunnel_handle: Option>, } @@ -54,7 +53,6 @@ where subscriber_tx, active_recordings, credential_store, - kerberos_session_store, agent_tunnel_handle, } = self; @@ -156,11 +154,10 @@ where // lookup by `claims.jti` is the primary path. if is_rdp && let Some(entry) = credential_store.get(claims.jti) - && entry.injection.is_some() - && let Some(kdc_session) = kerberos_session_store.get(claims.jti) + && entry.mapping.is_some() { anyhow::ensure!(token == entry.token, "token mismatch"); - let credential_injection_kdc = CredentialInjectionKdc::from_parts(entry, kdc_session)?; + let credential_injection_kdc = CredentialInjectionKdc::from_entry(claims.jti, entry)?; info!( jti = %credential_injection_kdc.jti(), diff --git a/devolutions-gateway/src/listener.rs b/devolutions-gateway/src/listener.rs index 5898b738a..0b7ce2740 100644 --- a/devolutions-gateway/src/listener.rs +++ b/devolutions-gateway/src/listener.rs @@ -159,7 +159,6 @@ async fn handle_tcp_peer(stream: TcpStream, state: DgwState, peer_addr: SocketAd .subscriber_tx(state.subscriber_tx) .active_recordings(state.recordings.active_recordings) .credential_store(state.credential_store) - .kerberos_session_store(state.kerberos_session_store) .agent_tunnel_handle(state.agent_tunnel_handle) .build() .serve() diff --git a/devolutions-gateway/src/ngrok.rs b/devolutions-gateway/src/ngrok.rs index 58115db89..71c0c005f 100644 --- a/devolutions-gateway/src/ngrok.rs +++ b/devolutions-gateway/src/ngrok.rs @@ -238,7 +238,6 @@ async fn run_tcp_tunnel(mut tunnel: ngrok::tunnel::TcpTunnel, state: DgwState) { .subscriber_tx(state.subscriber_tx) .active_recordings(state.recordings.active_recordings) .credential_store(state.credential_store) - .kerberos_session_store(state.kerberos_session_store) .agent_tunnel_handle(state.agent_tunnel_handle) .build() .serve() diff --git a/devolutions-gateway/src/rd_clean_path.rs b/devolutions-gateway/src/rd_clean_path.rs index 3f790974e..ce65b86f5 100644 --- a/devolutions-gateway/src/rd_clean_path.rs +++ b/devolutions-gateway/src/rd_clean_path.rs @@ -12,7 +12,7 @@ use tracing::field; use crate::config::Conf; use crate::credential::CredentialStoreHandle; -use crate::credential_injection_kdc::{CredentialInjectionKdc, CredentialInjectionKdcSessionStoreHandle}; +use crate::credential_injection_kdc::CredentialInjectionKdc; use crate::proxy::Proxy; use crate::recording::ActiveRecordings; use crate::session::{ConnectionModeDetails, DisconnectInterest, DisconnectedInfo, SessionInfo, SessionMessageSender}; @@ -522,7 +522,6 @@ pub async fn handle( subscriber_tx: SubscriberSender, active_recordings: &ActiveRecordings, credential_store: &CredentialStoreHandle, - kerberos_session_store: &CredentialInjectionKdcSessionStoreHandle, agent_tunnel_handle: Option>, ) -> anyhow::Result<()> { // Special handshake of our RDP extension @@ -541,13 +540,11 @@ pub async fn handle( // If a credential mapping has been pushed, we automatically switch to // proxy-based credential injection mode. Otherwise, we continue the usual - // clean path procedure. The credential store is keyed on the association token's JTI, - // and the per-session Kerberos material lives in a parallel store under the same JTI. + // clean path procedure. The credential store is keyed on the association token's JTI. if let Some(credential_injection_kdc) = crate::token::extract_jti(token).ok().and_then(|jti| { let entry = credential_store.get(jti)?; - entry.injection.as_ref()?; - let kdc_session = kerberos_session_store.get(jti)?; - CredentialInjectionKdc::from_parts(entry, kdc_session).ok() + entry.mapping.as_ref()?; + CredentialInjectionKdc::from_entry(jti, entry).ok() }) { anyhow::ensure!(token == credential_injection_kdc.raw_token(), "token mismatch"); debug!( diff --git a/devolutions-gateway/src/rdp_proxy.rs b/devolutions-gateway/src/rdp_proxy.rs index 1ab4382f1..17a37e757 100644 --- a/devolutions-gateway/src/rdp_proxy.rs +++ b/devolutions-gateway/src/rdp_proxy.rs @@ -636,7 +636,7 @@ where /// Only handles real-network schemes (`tcp` / `udp`); credential-injection loopback requests /// are intercepted by [`CredentialInjectionKdc`] before reaching this function. /// -/// TODO(sspi-rs#NNN): when sspi-rs ships a pluggable KDC dispatcher API, the URL trick for +/// TODO(sspi-rs#664): when sspi-rs ships a pluggable KDC dispatcher API, the URL trick for /// credential injection goes away entirely and this helper can be inlined back into the /// CredSSP loops. async fn send_network_request(request: &NetworkRequest) -> anyhow::Result> { diff --git a/devolutions-gateway/src/token.rs b/devolutions-gateway/src/token.rs index d723140fa..90923f1f1 100644 --- a/devolutions-gateway/src/token.rs +++ b/devolutions-gateway/src/token.rs @@ -1249,9 +1249,9 @@ pub fn extract_session_id(token: &str) -> anyhow::Result { /// Extract the destination host claim (`dst_hst`) from an association token without verifying its /// signature. Returns `None` if the claim is missing. /// -/// Used by the credential store to remember the target server's hostname for a credential-injection -/// session, so that fake-KDC can validate the client's TGS-REQ sname against it (the SPN the client -/// actually requested is `TERMSRV/`, not Gateway's own hostname). +/// Used by the credential-injection KDC to validate the client's TGS-REQ sname against the target +/// server hostname (the SPN the client actually requested is `TERMSRV/`, not Gateway's own +/// hostname). pub fn extract_dst_hst(token: &str) -> anyhow::Result> { let payload = extract_payload(token)?; let Some(value) = payload.get("dst_hst") else { From 4ce35ffe16b7307e10f2a0dd372f6b04672c38d9 Mon Sep 17 00:00:00 2001 From: irving ou Date: Fri, 15 May 2026 21:19:18 -0400 Subject: [PATCH 3/5] refactor(dgw): validate credential-injection token at preflight `provision-credentials` now validates the association token shape (JTI, dst_hst present, dst_alt empty) at preflight time and reserves a credential-injection context slot keyed on the token JTI. The Kerberos session and target hostname are derived lazily from the stored token on first CredSSP/KDC use, keeping `CredentialStore` strictly protocol-neutral while still failing fast on bad tokens. Renames `kerberos_session_store` to `credential_injection_context_store` to reflect that it now holds a per-session context (slot + lazy Kerberos session), not the session itself. --- devolutions-gateway/src/api/kdc_proxy.rs | 7 +- devolutions-gateway/src/api/preflight.rs | 40 ++- devolutions-gateway/src/api/rdp.rs | 4 + .../src/credential_injection_kdc.rs | 231 +++++++++++------- devolutions-gateway/src/generic_client.rs | 7 +- devolutions-gateway/src/lib.rs | 7 +- devolutions-gateway/src/listener.rs | 1 + devolutions-gateway/src/ngrok.rs | 1 + devolutions-gateway/src/rd_clean_path.rs | 14 +- devolutions-gateway/src/service.rs | 8 +- devolutions-gateway/src/token.rs | 33 +++ devolutions-gateway/tests/preflight.rs | 44 +++- 12 files changed, 284 insertions(+), 113 deletions(-) diff --git a/devolutions-gateway/src/api/kdc_proxy.rs b/devolutions-gateway/src/api/kdc_proxy.rs index 4c1a5e981..3b58fede8 100644 --- a/devolutions-gateway/src/api/kdc_proxy.rs +++ b/devolutions-gateway/src/api/kdc_proxy.rs @@ -26,7 +26,7 @@ async fn kdc_proxy( State(DgwState { conf_handle, credential_store, - kerberos_session_store, + credential_injection_context_store, .. }): State, KdcToken(KdcTokenClaims { destination }): KdcToken, @@ -47,8 +47,9 @@ async fn kdc_proxy( KdcDestination::Inject { jti } => { enforce_credential_injection_enabled(jti, conf.debug.enable_unstable)?; - let resolution = CredentialInjectionKdc::resolve(Some(jti), &credential_store, &kerberos_session_store) - .map_err(credential_injection_resolve_error)?; + let resolution = + CredentialInjectionKdc::resolve(Some(jti), &credential_store, &credential_injection_context_store) + .map_err(credential_injection_resolve_error)?; let kdc = resolution .ok_or_else(|| HttpError::internal().msg("credential-injection KDC resolution returned no state"))?; diff --git a/devolutions-gateway/src/api/preflight.rs b/devolutions-gateway/src/api/preflight.rs index c8309face..bb0de6981 100644 --- a/devolutions-gateway/src/api/preflight.rs +++ b/devolutions-gateway/src/api/preflight.rs @@ -12,6 +12,7 @@ use uuid::Uuid; use crate::DgwState; use crate::config::Conf; use crate::credential::{CredentialStoreHandle, InsertError}; +use crate::credential_injection_kdc::CredentialInjectionKdcContextStoreHandle; use crate::extract::PreflightScope; use crate::http::HttpError; use crate::session::SessionMessageSender; @@ -196,6 +197,7 @@ pub(super) async fn post_preflight( conf_handle, sessions, credential_store, + credential_injection_context_store, .. }): State, _scope: PreflightScope, @@ -223,12 +225,21 @@ pub(super) async fn post_preflight( let conf = conf_handle.get_conf(); let sessions = sessions.clone(); let credential_store = credential_store.clone(); + let credential_injection_context_store = credential_injection_context_store.clone(); async move { let operation_id = operation.id; trace!(%operation.id, "Process preflight operation"); - if let Err(error) = handle_operation(operation, &outputs, &conf, &sessions, &credential_store).await + if let Err(error) = handle_operation( + operation, + &outputs, + &conf, + &sessions, + &credential_store, + &credential_injection_context_store, + ) + .await { outputs.push(PreflightOutput { operation_id, @@ -257,6 +268,7 @@ async fn handle_operation( conf: &Conf, sessions: &SessionMessageSender, credential_store: &CredentialStoreHandle, + credential_injection_context_store: &CredentialInjectionKdcContextStoreHandle, ) -> Result<(), PreflightError> { match operation.kind.as_str() { OP_GET_VERSION => outputs.push(PreflightOutput { @@ -310,6 +322,7 @@ async fn handle_operation( }); } OP_PROVISION_TOKEN | OP_PROVISION_CREDENTIALS => { + let is_provision_credentials = operation.kind.as_str() == OP_PROVISION_CREDENTIALS; let (token, time_to_live, mapping) = if operation.kind.as_str() == OP_PROVISION_TOKEN { let ProvisionTokenParams { token, time_to_live } = from_params(operation.params).map_err(PreflightError::invalid_params)?; @@ -337,6 +350,27 @@ async fn handle_operation( }); } + let credential_injection_jti = if is_provision_credentials { + Some( + crate::token::validate_credential_injection_association_token(&token) + .inspect_err(|error| { + warn!( + %operation.id, + error = format!("{error:#}"), + "Credential-injection token is not valid" + ) + }) + .map_err(|error| { + PreflightError::new( + PreflightAlertStatus::InvalidParams, + format!("invalid credential-injection token: {error:#}"), + ) + })?, + ) + } else { + None + }; + let previous_entry = credential_store .insert(token, mapping, time_to_live) .inspect_err(|error| warn!(%operation.id, error = format!("{error:#}"), "Failed to insert credentials")) @@ -350,6 +384,10 @@ async fn handle_operation( ), })?; + if let Some(jti) = credential_injection_jti { + credential_injection_context_store.register_provisioned_credentials(jti, time_to_live); + } + if previous_entry.is_some() { outputs.push(PreflightOutput { operation_id: operation.id, diff --git a/devolutions-gateway/src/api/rdp.rs b/devolutions-gateway/src/api/rdp.rs index 6129a776c..468d762ea 100644 --- a/devolutions-gateway/src/api/rdp.rs +++ b/devolutions-gateway/src/api/rdp.rs @@ -26,6 +26,7 @@ pub async fn handler( recordings, shutdown_signal, credential_store, + credential_injection_context_store, agent_tunnel_handle, .. }): State, @@ -47,6 +48,7 @@ pub async fn handler( recordings.active_recordings, source_addr, credential_store, + credential_injection_context_store, agent_tunnel_handle, ) .instrument(span) @@ -67,6 +69,7 @@ async fn handle_socket( active_recordings: Arc, source_addr: SocketAddr, credential_store: crate::credential::CredentialStoreHandle, + credential_injection_context_store: crate::credential_injection_kdc::CredentialInjectionKdcContextStoreHandle, agent_tunnel_handle: Option>, ) { let (stream, close_handle) = crate::ws::handle( @@ -85,6 +88,7 @@ async fn handle_socket( subscriber_tx, &active_recordings, &credential_store, + &credential_injection_context_store, agent_tunnel_handle, ) .await; diff --git a/devolutions-gateway/src/credential_injection_kdc.rs b/devolutions-gateway/src/credential_injection_kdc.rs index 38308f8f7..4d6b8a0d4 100644 --- a/devolutions-gateway/src/credential_injection_kdc.rs +++ b/devolutions-gateway/src/credential_injection_kdc.rs @@ -27,7 +27,6 @@ use url::Url; use uuid::Uuid; use crate::credential::{AppCredential, AppCredentialMapping, ArcCredentialEntry, CredentialStoreHandle}; -use crate::target_addr::TargetAddr; const IN_PROCESS_KDC_HOST: &str = "cred.invalid"; @@ -50,6 +49,8 @@ pub(crate) enum CredentialInjectionKdcResolveError { MissingCredential { jti: Uuid }, #[error("credential-injection state is not available for {jti}")] NonInjectionCredential { jti: Uuid }, + #[error("credential-injection context is not available for {jti}")] + MissingContext { jti: Uuid }, #[error("association token for {jti} is not valid for credential injection")] InvalidAssociationToken { jti: Uuid, @@ -117,16 +118,21 @@ impl fmt::Debug for CredentialInjectionKdc { } impl CredentialInjectionKdc { - pub(crate) fn from_entry(jti: Uuid, credential_entry: ArcCredentialEntry) -> anyhow::Result { - let target_hostname = target_hostname_from_token(&credential_entry.token)?; + pub(crate) fn from_entry( + jti: Uuid, + credential_entry: ArcCredentialEntry, + context_store: &CredentialInjectionKdcContextStoreHandle, + ) -> anyhow::Result { let mapping = credential_entry .mapping .as_ref() .context("credential entry has no credential-injection mapping")?; - let session = Arc::new(derive_credential_injection_kdc_session( - app_credential_username(&mapping.proxy), - jti, - )); + let proxy_username = app_credential_username(&mapping.proxy).to_owned(); + let session = context_store + .get_or_insert_session(jti, || derive_credential_injection_kdc_session(&proxy_username, jti)) + .context("credential-injection context is not available")?; + let target_hostname = crate::token::extract_credential_injection_target_hostname(&credential_entry.token) + .context("extract credential-injection target hostname from association token")?; Self::from_parts(jti, credential_entry, target_hostname, session) } @@ -161,7 +167,7 @@ impl CredentialInjectionKdc { pub(crate) fn resolve( jet_cred_id: Option, credential_store: &CredentialStoreHandle, - session_store: &CredentialInjectionKdcSessionStoreHandle, + context_store: &CredentialInjectionKdcContextStoreHandle, ) -> Result { let Some(jti) = jet_cred_id else { return Ok(None); @@ -177,12 +183,22 @@ impl CredentialInjectionKdc { return Err(CredentialInjectionKdcResolveError::NonInjectionCredential { jti }); }; - let target_hostname = target_hostname_from_token(&credential_entry.token) - .map_err(|source| CredentialInjectionKdcResolveError::InvalidAssociationToken { jti, source })?; let proxy_username = app_credential_username(&mapping.proxy).to_owned(); - let session = session_store.get_or_insert_with(jti, credential_entry.expires_at, || { - derive_credential_injection_kdc_session(&proxy_username, jti) - }); + let session = context_store + .get_or_insert_session(jti, || derive_credential_injection_kdc_session(&proxy_username, jti)) + .ok_or_else(|| { + warn!(%jti, "KDC token references missing credential-injection context"); + CredentialInjectionKdcResolveError::MissingContext { jti } + })?; + let target_hostname = crate::token::extract_credential_injection_target_hostname(&credential_entry.token) + .map_err(|source| { + warn!( + %jti, + error = format!("{source:#}"), + "KDC token references invalid credential-injection association token" + ); + CredentialInjectionKdcResolveError::InvalidAssociationToken { jti, source } + })?; let kdc = Self::from_parts(jti, credential_entry, target_hostname, session) .map_err(|source| CredentialInjectionKdcResolveError::BuildKdcConfig { jti, source })?; @@ -312,25 +328,6 @@ impl CredentialInjectionKdc { } } -fn target_hostname_from_token(token: &str) -> anyhow::Result { - const DEFAULT_DST_PORT: u16 = 3389; - - let dst_alt = crate::token::extract_dst_alt(token).context("read dst_alt from association token")?; - anyhow::ensure!( - dst_alt.is_empty(), - "association token dst_alt is not supported for credential injection", - ); - - let raw_dst_hst = crate::token::extract_dst_hst(token) - .context("read dst_hst from association token")? - .context("association token has no dst_hst, required for credential injection")?; - - Ok(TargetAddr::parse(&raw_dst_hst, DEFAULT_DST_PORT) - .context("parse dst_hst as target address")? - .host() - .to_owned()) -} - fn app_credential_username(credential: &AppCredential) -> &str { match credential { AppCredential::UsernamePassword { username, password: _ } => username, @@ -362,9 +359,9 @@ fn realm_mismatch(expected: &str, actual: &str) -> Option { /// The key material and the acceptor PA-ENC-TIMESTAMP password are wrapped in [`SecretBox`] / /// [`SecretString`] so they cannot be accidentally written to logs through structured tracing. /// Access requires an explicit `expose_secret()` call, which is greppable and reviewable. -pub(crate) struct CredentialInjectionKdcSession { +struct CredentialInjectionKdcSession { jti: Uuid, - pub(crate) realm: String, + realm: String, kdc: CredentialInjectionKdcState, acceptor: CredentialInjectionAcceptorState, } @@ -413,10 +410,7 @@ impl fmt::Debug for CredentialInjectionAcceptorState { /// The proxy username's optional `@realm` suffix selects the realm DVLS supplied; otherwise /// fall back to a per-session synthetic realm derived from the JTI. The two sides agree /// because DVLS derives the synthetic value the same way. -pub(crate) fn derive_credential_injection_kdc_session( - proxy_username: &str, - jti: Uuid, -) -> CredentialInjectionKdcSession { +fn derive_credential_injection_kdc_session(proxy_username: &str, jti: Uuid) -> CredentialInjectionKdcSession { let realm = proxy_username .split_once('@') .map(|(_, realm)| realm) @@ -486,7 +480,7 @@ fn kerberos_salt(realm: &str, principal: &str) -> String { format!("{}{local_name}", realm.to_ascii_uppercase()) } -pub(crate) fn synthetic_realm(jti: Uuid) -> String { +fn synthetic_realm(jti: Uuid) -> String { format!("CRED-{}.INVALID", jti.simple()).to_ascii_uppercase() } @@ -496,74 +490,59 @@ fn random_32_bytes() -> Vec { bytes } -/// Lazy store of [`CredentialInjectionKdcSession`] entries keyed by association-token JTI. +/// Store of credential-injection contexts keyed by association-token JTI. /// -/// Sessions are created on first KDC-proxy use from the credential entry and its original -/// association token. The stores share neither lock nor map so that the credential store stays -/// Kerberos-unaware. +/// Association tokens are validated at `provision-credentials` time, but target hostnames are +/// extracted lazily from the original token on first CredSSP/KDC use. +/// Kerberos sessions are also created lazily from the credential entry on first CredSSP/KDC use. +/// The store is separate from the credential store so that the credential store stays Kerberos-unaware. #[derive(Debug, Clone, Default)] -pub struct CredentialInjectionKdcSessionStoreHandle(Arc>>); +pub struct CredentialInjectionKdcContextStoreHandle(Arc>>); #[derive(Debug)] struct Entry { - session: Arc, + session: Option>, expires_at: time::OffsetDateTime, } -impl CredentialInjectionKdcSessionStoreHandle { +impl CredentialInjectionKdcContextStoreHandle { pub fn new() -> Self { Self(Arc::new(Mutex::new(HashMap::new()))) } - #[cfg(test)] - pub(crate) fn insert(&self, session: CredentialInjectionKdcSession, time_to_live: time::Duration) { - let jti = session.jti; + pub(crate) fn register_provisioned_credentials(&self, jti: Uuid, time_to_live: time::Duration) { let entry = Entry { - session: Arc::new(session), + session: None, expires_at: time::OffsetDateTime::now_utc() + time_to_live, }; self.0.lock().insert(jti, entry); } - #[cfg(test)] - pub(crate) fn get(&self, jti: Uuid) -> Option> { - // Lookup-time TTL enforcement mirrors `CredentialStoreHandle::get`: the cleanup task is - // best-effort, and we don't want to hand out Kerberos material whose paired credential - // entry has already expired. - self.0 - .lock() - .get(&jti) - .filter(|entry| time::OffsetDateTime::now_utc() < entry.expires_at) - .map(|entry| Arc::clone(&entry.session)) - } - - pub(crate) fn get_or_insert_with( + fn get_or_insert_session( &self, jti: Uuid, - expires_at: time::OffsetDateTime, make_session: impl FnOnce() -> CredentialInjectionKdcSession, - ) -> Arc { + ) -> Option> { let now = time::OffsetDateTime::now_utc(); let mut entries = self.0.lock(); - if let Some(entry) = entries.get(&jti).filter(|entry| now < entry.expires_at) { - return Arc::clone(&entry.session); - } + let entry = entries.get_mut(&jti).filter(|entry| now < entry.expires_at)?; - let session = Arc::new(make_session()); - entries.insert( - jti, - Entry { - session: Arc::clone(&session), - expires_at, - }, - ); - session + let session = match &entry.session { + Some(session) => Arc::clone(session), + None => { + let session = Arc::new(make_session()); + entry.session = Some(Arc::clone(&session)); + session + } + }; + + Some(session) } } pub struct CleanupTask { - pub handle: CredentialInjectionKdcSessionStoreHandle, + pub handle: CredentialInjectionKdcContextStoreHandle, } #[async_trait] @@ -579,7 +558,7 @@ impl Task for CleanupTask { } #[instrument(skip_all)] -async fn cleanup_task(handle: CredentialInjectionKdcSessionStoreHandle, mut shutdown_signal: ShutdownSignal) { +async fn cleanup_task(handle: CredentialInjectionKdcContextStoreHandle, mut shutdown_signal: ShutdownSignal) { use tokio::time::{Duration, sleep}; const TASK_INTERVAL: Duration = Duration::from_secs(60 * 15); // 15 minutes @@ -693,25 +672,37 @@ mod tests { #[test] fn store_lookup_filters_expired_entries() { - let store = CredentialInjectionKdcSessionStoreHandle::new(); + let store = CredentialInjectionKdcContextStoreHandle::new(); let jti = Uuid::new_v4(); - let session = derive_credential_injection_kdc_session("proxy@example.invalid", jti); // Negative TTL: entry is born already expired. - store.insert(session, time::Duration::seconds(-1)); + store.register_provisioned_credentials(jti, time::Duration::seconds(-1)); - assert!(store.get(jti).is_none(), "expired entry must not be returned"); + assert!( + store + .get_or_insert_session(jti, || derive_credential_injection_kdc_session( + "proxy@example.invalid", + jti + )) + .is_none(), + "expired entry must not be returned" + ); } #[test] fn store_returns_fresh_entry() { - let store = CredentialInjectionKdcSessionStoreHandle::new(); + let store = CredentialInjectionKdcContextStoreHandle::new(); let jti = Uuid::new_v4(); - let session = derive_credential_injection_kdc_session("proxy@example.invalid", jti); - store.insert(session, time::Duration::minutes(5)); + store.register_provisioned_credentials(jti, time::Duration::minutes(5)); + + let session = store + .get_or_insert_session(jti, || { + derive_credential_injection_kdc_session("proxy@example.invalid", jti) + }) + .expect("fresh entry returned"); - assert_eq!(store.get(jti).expect("fresh entry returned").realm, "example.invalid"); + assert_eq!(session.realm, "example.invalid"); } #[test] @@ -747,9 +738,9 @@ mod tests { #[test] fn resolve_with_no_jet_cred_id_forwards_to_real_kdc() { let credential_store = CredentialStoreHandle::new(); - let session_store = CredentialInjectionKdcSessionStoreHandle::new(); + let context_store = CredentialInjectionKdcContextStoreHandle::new(); - let dispatch = CredentialInjectionKdc::resolve(None, &credential_store, &session_store) + let dispatch = CredentialInjectionKdc::resolve(None, &credential_store, &context_store) .expect("plain KDC token should dispatch"); assert!(dispatch.is_none()); @@ -779,10 +770,10 @@ mod tests { #[test] fn resolve_with_missing_jet_cred_id_fails_closed() { let credential_store = CredentialStoreHandle::new(); - let session_store = CredentialInjectionKdcSessionStoreHandle::new(); + let context_store = CredentialInjectionKdcContextStoreHandle::new(); assert!( - CredentialInjectionKdc::resolve(Some(Uuid::new_v4()), &credential_store, &session_store).is_err(), + CredentialInjectionKdc::resolve(Some(Uuid::new_v4()), &credential_store, &context_store).is_err(), "KDC tokens with jet_cred_id must not fall back to real-KDC forwarding" ); } @@ -790,7 +781,7 @@ mod tests { #[test] fn resolve_with_non_injection_entry_fails_closed() { let credential_store = CredentialStoreHandle::new(); - let session_store = CredentialInjectionKdcSessionStoreHandle::new(); + let context_store = CredentialInjectionKdcContextStoreHandle::new(); let jti = Uuid::new_v4(); credential_store @@ -798,11 +789,56 @@ mod tests { .expect("provision-token entry inserts"); assert!( - CredentialInjectionKdc::resolve(Some(jti), &credential_store, &session_store).is_err(), + CredentialInjectionKdc::resolve(Some(jti), &credential_store, &context_store).is_err(), "KDC tokens with jet_cred_id must require provision-credentials state" ); } + #[test] + fn resolve_lazily_extracts_target_hostname_from_entry_token() { + let credential_store = CredentialStoreHandle::new(); + let context_store = CredentialInjectionKdcContextStoreHandle::new(); + let jti = Uuid::new_v4(); + + credential_store + .insert( + association_token(jti), + Some(cleartext_mapping_with_target_username("target")), + time::Duration::minutes(5), + ) + .expect("credential entry inserts"); + context_store.register_provisioned_credentials(jti, time::Duration::minutes(5)); + + let kdc = CredentialInjectionKdc::resolve(Some(jti), &credential_store, &context_store) + .expect("credential-injection KDC resolves") + .expect("credential-injection KDC is selected"); + + assert_eq!(kdc.target_hostname, "target.example"); + } + + #[test] + fn resolve_with_missing_context_fails_closed() { + let credential_store = CredentialStoreHandle::new(); + let context_store = CredentialInjectionKdcContextStoreHandle::new(); + let jti = Uuid::new_v4(); + + credential_store + .insert( + association_token(jti), + Some(cleartext_mapping_with_target_username("target")), + time::Duration::minutes(5), + ) + .expect("credential entry inserts"); + + assert!( + matches!( + CredentialInjectionKdc::resolve(Some(jti), &credential_store, &context_store), + Err(CredentialInjectionKdcResolveError::MissingContext { .. }) + ), + "KDC tokens with jet_cred_id must require provision-credentials context" + ); + } + #[test] fn intercept_ignores_non_loopback_host() { let jti = Uuid::new_v4(); @@ -868,4 +904,13 @@ mod tests { assert_eq!(mismatch.expected, "cred-session.invalid"); assert_eq!(mismatch.actual, "evil.example"); } + + #[test] + fn missing_kdc_proxy_envelope_realm_falls_back_to_session_realm() { + let jti = Uuid::new_v4(); + let kdc = dummy_kdc(jti); + let message = KdcProxyMessage::from_raw_kerb_message(&[]).expect("KDC proxy wrapper builds"); + + assert_eq!(kdc.resolve_message_realm(&message), "example.invalid"); + } } diff --git a/devolutions-gateway/src/generic_client.rs b/devolutions-gateway/src/generic_client.rs index 117eb41bf..5ea8ee41d 100644 --- a/devolutions-gateway/src/generic_client.rs +++ b/devolutions-gateway/src/generic_client.rs @@ -9,7 +9,7 @@ use typed_builder::TypedBuilder; use crate::config::Conf; use crate::credential::CredentialStoreHandle; -use crate::credential_injection_kdc::CredentialInjectionKdc; +use crate::credential_injection_kdc::{CredentialInjectionKdc, CredentialInjectionKdcContextStoreHandle}; use crate::proxy::Proxy; use crate::rdp_pcb::{extract_association_claims, read_pcb}; use crate::recording::ActiveRecordings; @@ -29,6 +29,7 @@ pub struct GenericClient { subscriber_tx: SubscriberSender, active_recordings: Arc, credential_store: CredentialStoreHandle, + credential_injection_context_store: CredentialInjectionKdcContextStoreHandle, #[builder(default)] agent_tunnel_handle: Option>, } @@ -53,6 +54,7 @@ where subscriber_tx, active_recordings, credential_store, + credential_injection_context_store, agent_tunnel_handle, } = self; @@ -157,7 +159,8 @@ where && entry.mapping.is_some() { anyhow::ensure!(token == entry.token, "token mismatch"); - let credential_injection_kdc = CredentialInjectionKdc::from_entry(claims.jti, entry)?; + let credential_injection_kdc = + CredentialInjectionKdc::from_entry(claims.jti, entry, &credential_injection_context_store)?; info!( jti = %credential_injection_kdc.jti(), diff --git a/devolutions-gateway/src/lib.rs b/devolutions-gateway/src/lib.rs index 019252365..bebcd53a8 100644 --- a/devolutions-gateway/src/lib.rs +++ b/devolutions-gateway/src/lib.rs @@ -61,7 +61,7 @@ pub struct DgwState { pub recordings: recording::RecordingMessageSender, pub job_queue_handle: job_queue::JobQueueHandle, pub credential_store: credential::CredentialStoreHandle, - pub kerberos_session_store: credential_injection_kdc::CredentialInjectionKdcSessionStoreHandle, + pub credential_injection_context_store: credential_injection_kdc::CredentialInjectionKdcContextStoreHandle, pub monitoring_state: Arc, pub traffic_audit_handle: traffic_audit::TrafficAuditHandle, pub agent_tunnel_handle: Option>, @@ -90,7 +90,8 @@ impl DgwState { let (job_queue_handle, job_queue_rx) = job_queue::JobQueueHandle::new(); let (traffic_audit_handle, traffic_audit_rx) = traffic_audit::TrafficAuditHandle::new(); let credential_store = credential::CredentialStoreHandle::new(); - let kerberos_session_store = credential_injection_kdc::CredentialInjectionKdcSessionStoreHandle::new(); + let credential_injection_context_store = + credential_injection_kdc::CredentialInjectionKdcContextStoreHandle::new(); let monitoring_state = Arc::new(network_monitor::State::new(Arc::new(MockMonitorsCache))?); let state = Self { @@ -104,7 +105,7 @@ impl DgwState { job_queue_handle, traffic_audit_handle, credential_store, - kerberos_session_store, + credential_injection_context_store, monitoring_state, agent_tunnel_handle: None, }; diff --git a/devolutions-gateway/src/listener.rs b/devolutions-gateway/src/listener.rs index 0b7ce2740..288884ab4 100644 --- a/devolutions-gateway/src/listener.rs +++ b/devolutions-gateway/src/listener.rs @@ -159,6 +159,7 @@ async fn handle_tcp_peer(stream: TcpStream, state: DgwState, peer_addr: SocketAd .subscriber_tx(state.subscriber_tx) .active_recordings(state.recordings.active_recordings) .credential_store(state.credential_store) + .credential_injection_context_store(state.credential_injection_context_store) .agent_tunnel_handle(state.agent_tunnel_handle) .build() .serve() diff --git a/devolutions-gateway/src/ngrok.rs b/devolutions-gateway/src/ngrok.rs index 71c0c005f..2a1fd3167 100644 --- a/devolutions-gateway/src/ngrok.rs +++ b/devolutions-gateway/src/ngrok.rs @@ -238,6 +238,7 @@ async fn run_tcp_tunnel(mut tunnel: ngrok::tunnel::TcpTunnel, state: DgwState) { .subscriber_tx(state.subscriber_tx) .active_recordings(state.recordings.active_recordings) .credential_store(state.credential_store) + .credential_injection_context_store(state.credential_injection_context_store) .agent_tunnel_handle(state.agent_tunnel_handle) .build() .serve() diff --git a/devolutions-gateway/src/rd_clean_path.rs b/devolutions-gateway/src/rd_clean_path.rs index ce65b86f5..0ca951242 100644 --- a/devolutions-gateway/src/rd_clean_path.rs +++ b/devolutions-gateway/src/rd_clean_path.rs @@ -12,7 +12,7 @@ use tracing::field; use crate::config::Conf; use crate::credential::CredentialStoreHandle; -use crate::credential_injection_kdc::CredentialInjectionKdc; +use crate::credential_injection_kdc::{CredentialInjectionKdc, CredentialInjectionKdcContextStoreHandle}; use crate::proxy::Proxy; use crate::recording::ActiveRecordings; use crate::session::{ConnectionModeDetails, DisconnectInterest, DisconnectedInfo, SessionInfo, SessionMessageSender}; @@ -522,6 +522,7 @@ pub async fn handle( subscriber_tx: SubscriberSender, active_recordings: &ActiveRecordings, credential_store: &CredentialStoreHandle, + credential_injection_context_store: &CredentialInjectionKdcContextStoreHandle, agent_tunnel_handle: Option>, ) -> anyhow::Result<()> { // Special handshake of our RDP extension @@ -541,11 +542,12 @@ pub async fn handle( // If a credential mapping has been pushed, we automatically switch to // proxy-based credential injection mode. Otherwise, we continue the usual // clean path procedure. The credential store is keyed on the association token's JTI. - if let Some(credential_injection_kdc) = crate::token::extract_jti(token).ok().and_then(|jti| { - let entry = credential_store.get(jti)?; - entry.mapping.as_ref()?; - CredentialInjectionKdc::from_entry(jti, entry).ok() - }) { + if let Some(jti) = crate::token::extract_jti(token).ok() + && let Some(entry) = credential_store.get(jti) + && entry.mapping.is_some() + { + let credential_injection_kdc = + CredentialInjectionKdc::from_entry(jti, entry, credential_injection_context_store)?; anyhow::ensure!(token == credential_injection_kdc.raw_token(), "token mismatch"); debug!( jti = %credential_injection_kdc.jti(), diff --git a/devolutions-gateway/src/service.rs b/devolutions-gateway/src/service.rs index 47823b872..3cb1ee5c1 100644 --- a/devolutions-gateway/src/service.rs +++ b/devolutions-gateway/src/service.rs @@ -269,8 +269,8 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { .context("failed to initialize traffic audit manager")?; let credential_store = CredentialStoreHandle::new(); - let kerberos_session_store = - devolutions_gateway::credential_injection_kdc::CredentialInjectionKdcSessionStoreHandle::new(); + let credential_injection_context_store = + devolutions_gateway::credential_injection_kdc::CredentialInjectionKdcContextStoreHandle::new(); let filesystem_monitor_config_cache = devolutions_gateway::api::monitoring::FilesystemConfigCache::new( config::get_data_dir().join("monitors_cache.json"), @@ -319,7 +319,7 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { recordings: recording_manager_handle.clone(), job_queue_handle: job_queue_ctx.job_queue_handle.clone(), credential_store: credential_store.clone(), - kerberos_session_store: kerberos_session_store.clone(), + credential_injection_context_store: credential_injection_context_store.clone(), monitoring_state, traffic_audit_handle: traffic_audit_task.handle(), agent_tunnel_handle, @@ -359,7 +359,7 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { }); tasks.register(devolutions_gateway::credential_injection_kdc::CleanupTask { - handle: kerberos_session_store, + handle: credential_injection_context_store, }); tasks.register(devolutions_log::LogDeleterTask::::new( diff --git a/devolutions-gateway/src/token.rs b/devolutions-gateway/src/token.rs index 90923f1f1..72884fcda 100644 --- a/devolutions-gateway/src/token.rs +++ b/devolutions-gateway/src/token.rs @@ -26,6 +26,7 @@ pub const MAX_SUBKEY_TOKEN_VALIDITY_DURATION_SECS: i64 = 60 * 60 * 2; // 2 hours const LEEWAY_SECS: u16 = 60 * 5; // 5 minutes const RDP_MAX_REUSE_INTERVAL_SECS: i64 = 10; // 10 seconds const BRIDGE_TOKEN_MAX_TOKEN_VALIDITY_DURATION_SECS: i64 = 60 * 60 * 12; // 12 hours +const CREDENTIAL_INJECTION_DEFAULT_DST_PORT: u16 = 3389; /// This is the maximum number of reconnections allowed during the reconnection window. If the /// reconnection window (e.g.: 30 seconds) is over while the connection is still alive, the counter @@ -1276,6 +1277,38 @@ pub fn extract_dst_alt(token: &str) -> anyhow::Result> { .collect() } +/// Validate the association-token claims required by credential injection. +/// +/// This is intentionally a token-layer shape check only. +/// The credential-injection KDC still lazily extracts the target hostname from the original token +/// when it builds its per-session state. +pub fn validate_credential_injection_association_token(token: &str) -> anyhow::Result { + let jti = extract_jti(token).context("read jti from association token")?; + extract_credential_injection_target_hostname(token)?; + Ok(jti) +} + +/// Extract the target hostname used by credential injection from an association token. +/// +/// `dst_alt` is rejected for now because the Kerberos fake-KDC can validate only one target SPN for +/// the current credential-injection session. +pub fn extract_credential_injection_target_hostname(token: &str) -> anyhow::Result { + let dst_alt = extract_dst_alt(token).context("read dst_alt from association token")?; + anyhow::ensure!( + dst_alt.is_empty(), + "association token dst_alt is not supported for credential injection", + ); + + let raw_dst_hst = extract_dst_hst(token) + .context("read dst_hst from association token")? + .context("association token has no dst_hst, required for credential injection")?; + + Ok(TargetAddr::parse(&raw_dst_hst, CREDENTIAL_INJECTION_DEFAULT_DST_PORT) + .context("parse dst_hst as target address")? + .host() + .to_owned()) +} + #[deprecated = "make sure this is never used without a deliberate action"] pub mod unsafe_debug { // Any function in this module should only be used at development stage when deliberately diff --git a/devolutions-gateway/tests/preflight.rs b/devolutions-gateway/tests/preflight.rs index 1f78d3094..8246696ac 100644 --- a/devolutions-gateway/tests/preflight.rs +++ b/devolutions-gateway/tests/preflight.rs @@ -82,7 +82,7 @@ async fn test_provision_credentials_success() -> anyhow::Result<()> { let (app, _state, _handles) = make_router()?; // JWT payload includes `dst_hst` because credential injection requires a target hostname - // (fake-KDC validates TGS-REQ sname against `TERMSRV/`); insert rejects tokens without it. + // (fake-KDC validates TGS-REQ sname against `TERMSRV/`); preflight rejects tokens without it. let token = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiI1ZTNlODMzZi04NGM3LTQ1NDEtYjY3Ni1hY2MzMjk5ZTM5YjgiLCJkc3RfaHN0IjoidGFyZ2V0LmV4YW1wbGU6MzM4OSJ9.1qECGlrW7y9HWFArc6GPHLGTOY7PhAvzKJ5XMRBg4k4"; let op_id = Uuid::new_v4(); @@ -188,6 +188,48 @@ async fn test_provision_credentials_rejects_alternate_targets() -> anyhow::Resul Ok(()) } +#[tokio::test] +async fn test_provision_credentials_rejects_missing_target_hostname() -> anyhow::Result<()> { + let _guard = init_logger(); + + let (app, _state, _handles) = make_router()?; + + let token = unsigned_jws(json!({ + "jti": "5e3e833f-84c7-4541-b676-acc3299e39b8" + }))?; + + let op_id = Uuid::new_v4(); + + let op = json!([{ + "id": op_id, + "kind": "provision-credentials", + "token": token, + "proxy_credential": { "kind": "username-password", "username": "proxy_user", "password": "secret1" }, + "target_credential": { "kind": "username-password", "username": "target_user", "password": "secret2" }, + "time_to_live": 15 + }]); + + let response = app.oneshot(preflight_request(op)?).await?; + assert_eq!(response.status(), StatusCode::OK); + + let body = response.into_body().collect().await?.to_bytes(); + let body: serde_json::Value = serde_json::from_slice(&body)?; + + assert_eq!(body.as_array().expect("an array").len(), 1); + assert_eq!(body[0]["kind"], "alert"); + assert_eq!(body[0]["alert_status"], "invalid-parameters"); + assert!( + body[0]["alert_message"] + .as_str() + .expect("alert message") + .contains("dst_hst"), + "{:?}", + body[0] + ); + + Ok(()) +} + #[tokio::test] async fn test_provision_token_overwrite_alert() -> anyhow::Result<()> { let _guard = init_logger(); From e32df90db1aa621748c613c98da2e7758a34471d Mon Sep 17 00:00:00 2001 From: irving ou Date: Wed, 20 May 2026 11:00:37 -0400 Subject: [PATCH 4/5] refactor(dgw): collapse credential stores into CredentialService DgwState exposed both `credential_store` and a separate registry that internally held a clone of it, with parallel HashMaps keyed on the same JTI. Consolidate behind a single `CredentialService` that owns the protocol-neutral credential store and the Kerberos session cache. - Rename `CredentialInjectionKdcRegistry` to `CredentialService`. Add `insert`/`get` pass-throughs alongside `kdc_for`. Insert unconditionally drops the cached session for the target JTI to close the race where `credential::CleanupTask` purged the entry before the registry sweep ran. - Drop `credential_store` from `DgwState`; expose the single `credentials: CredentialService` field. Cleanup task wiring routes through `credentials.credential_store()` accessor. - Preflight no longer registers a context slot or invalidates sessions explicitly; `CredentialService::insert` covers both paths. - `credential/mod.rs` stays Kerberos-free. - Add tests covering insert-replacement, the credential-cleanup race, and orphan-session sweep. - Ignore `*.csproj.lscache` files emitted by the C# language service. --- .gitignore | 1 + devolutions-gateway/src/api/kdc_proxy.rs | 13 +- devolutions-gateway/src/api/preflight.rs | 71 ++- devolutions-gateway/src/api/rdp.rs | 12 +- .../src/credential_injection_kdc.rs | 457 +++++++++++------- devolutions-gateway/src/generic_client.rs | 14 +- devolutions-gateway/src/lib.rs | 10 +- devolutions-gateway/src/listener.rs | 3 +- devolutions-gateway/src/ngrok.rs | 3 +- devolutions-gateway/src/rd_clean_path.rs | 11 +- devolutions-gateway/src/service.rs | 14 +- 11 files changed, 335 insertions(+), 274 deletions(-) diff --git a/.gitignore b/.gitignore index 809ffc7a2..094292691 100644 --- a/.gitignore +++ b/.gitignore @@ -9,6 +9,7 @@ dist/ /output/ /config/ *.DotSettings +*.csproj.lscache # Downloaded build dependencies tun2socks.exe diff --git a/devolutions-gateway/src/api/kdc_proxy.rs b/devolutions-gateway/src/api/kdc_proxy.rs index 3b58fede8..df5a09c5c 100644 --- a/devolutions-gateway/src/api/kdc_proxy.rs +++ b/devolutions-gateway/src/api/kdc_proxy.rs @@ -10,8 +10,8 @@ use tokio::net::{TcpStream, UdpSocket}; use crate::DgwState; use crate::credential_injection_kdc::{ - CredentialInjectionKdc, CredentialInjectionKdcInterception, CredentialInjectionKdcRequest, - CredentialInjectionKdcResolveError, kdc_proxy_message_realm, + CredentialInjectionKdcInterception, CredentialInjectionKdcRequest, CredentialInjectionKdcResolveError, + kdc_proxy_message_realm, }; use crate::extract::KdcToken; use crate::http::{HttpError, HttpErrorBuilder}; @@ -25,8 +25,7 @@ pub fn make_router(state: DgwState) -> Router { async fn kdc_proxy( State(DgwState { conf_handle, - credential_store, - credential_injection_context_store, + credentials, .. }): State, KdcToken(KdcTokenClaims { destination }): KdcToken, @@ -47,11 +46,7 @@ async fn kdc_proxy( KdcDestination::Inject { jti } => { enforce_credential_injection_enabled(jti, conf.debug.enable_unstable)?; - let resolution = - CredentialInjectionKdc::resolve(Some(jti), &credential_store, &credential_injection_context_store) - .map_err(credential_injection_resolve_error)?; - let kdc = resolution - .ok_or_else(|| HttpError::internal().msg("credential-injection KDC resolution returned no state"))?; + let kdc = credentials.kdc_for(jti).map_err(credential_injection_resolve_error)?; debug!( jti = %kdc.jti(), diff --git a/devolutions-gateway/src/api/preflight.rs b/devolutions-gateway/src/api/preflight.rs index bb0de6981..95216cc25 100644 --- a/devolutions-gateway/src/api/preflight.rs +++ b/devolutions-gateway/src/api/preflight.rs @@ -11,8 +11,8 @@ use uuid::Uuid; use crate::DgwState; use crate::config::Conf; -use crate::credential::{CredentialStoreHandle, InsertError}; -use crate::credential_injection_kdc::CredentialInjectionKdcContextStoreHandle; +use crate::credential::InsertError; +use crate::credential_injection_kdc::CredentialService; use crate::extract::PreflightScope; use crate::http::HttpError; use crate::session::SessionMessageSender; @@ -196,8 +196,7 @@ pub(super) async fn post_preflight( State(DgwState { conf_handle, sessions, - credential_store, - credential_injection_context_store, + credentials, .. }): State, _scope: PreflightScope, @@ -224,23 +223,13 @@ pub(super) async fn post_preflight( let outputs = outputs.clone(); let conf = conf_handle.get_conf(); let sessions = sessions.clone(); - let credential_store = credential_store.clone(); - let credential_injection_context_store = credential_injection_context_store.clone(); + let credentials = credentials.clone(); async move { let operation_id = operation.id; trace!(%operation.id, "Process preflight operation"); - if let Err(error) = handle_operation( - operation, - &outputs, - &conf, - &sessions, - &credential_store, - &credential_injection_context_store, - ) - .await - { + if let Err(error) = handle_operation(operation, &outputs, &conf, &sessions, &credentials).await { outputs.push(PreflightOutput { operation_id, kind: PreflightOutputKind::Alert { @@ -267,8 +256,7 @@ async fn handle_operation( outputs: &Outputs, conf: &Conf, sessions: &SessionMessageSender, - credential_store: &CredentialStoreHandle, - credential_injection_context_store: &CredentialInjectionKdcContextStoreHandle, + credentials: &CredentialService, ) -> Result<(), PreflightError> { match operation.kind.as_str() { OP_GET_VERSION => outputs.push(PreflightOutput { @@ -350,28 +338,27 @@ async fn handle_operation( }); } - let credential_injection_jti = if is_provision_credentials { - Some( - crate::token::validate_credential_injection_association_token(&token) - .inspect_err(|error| { - warn!( - %operation.id, - error = format!("{error:#}"), - "Credential-injection token is not valid" - ) - }) - .map_err(|error| { - PreflightError::new( - PreflightAlertStatus::InvalidParams, - format!("invalid credential-injection token: {error:#}"), - ) - })?, - ) - } else { - None - }; + // Provision-credentials tokens must be valid association tokens with the credential + // injection shape (JTI + dst_hst + no dst_alt). Fail-fast at preflight so the request + // never reaches the credential store with malformed input. + if is_provision_credentials { + crate::token::validate_credential_injection_association_token(&token) + .inspect_err(|error| { + warn!( + %operation.id, + error = format!("{error:#}"), + "Credential-injection token is not valid" + ) + }) + .map_err(|error| { + PreflightError::new( + PreflightAlertStatus::InvalidParams, + format!("invalid credential-injection token: {error:#}"), + ) + })?; + } - let previous_entry = credential_store + let previous_entry = credentials .insert(token, mapping, time_to_live) .inspect_err(|error| warn!(%operation.id, error = format!("{error:#}"), "Failed to insert credentials")) .map_err(|error| match error { @@ -384,10 +371,8 @@ async fn handle_operation( ), })?; - if let Some(jti) = credential_injection_jti { - credential_injection_context_store.register_provisioned_credentials(jti, time_to_live); - } - + // `CredentialService::insert` already drops the cached Kerberos session for a + // replaced entry, so no explicit invalidation is needed here. if previous_entry.is_some() { outputs.push(PreflightOutput { operation_id: operation.id, diff --git a/devolutions-gateway/src/api/rdp.rs b/devolutions-gateway/src/api/rdp.rs index 468d762ea..b3d45dbcb 100644 --- a/devolutions-gateway/src/api/rdp.rs +++ b/devolutions-gateway/src/api/rdp.rs @@ -25,8 +25,7 @@ pub async fn handler( subscriber_tx, recordings, shutdown_signal, - credential_store, - credential_injection_context_store, + credentials, agent_tunnel_handle, .. }): State, @@ -47,8 +46,7 @@ pub async fn handler( subscriber_tx, recordings.active_recordings, source_addr, - credential_store, - credential_injection_context_store, + credentials, agent_tunnel_handle, ) .instrument(span) @@ -68,8 +66,7 @@ async fn handle_socket( subscriber_tx: SubscriberSender, active_recordings: Arc, source_addr: SocketAddr, - credential_store: crate::credential::CredentialStoreHandle, - credential_injection_context_store: crate::credential_injection_kdc::CredentialInjectionKdcContextStoreHandle, + credentials: crate::credential_injection_kdc::CredentialService, agent_tunnel_handle: Option>, ) { let (stream, close_handle) = crate::ws::handle( @@ -87,8 +84,7 @@ async fn handle_socket( sessions, subscriber_tx, &active_recordings, - &credential_store, - &credential_injection_context_store, + &credentials, agent_tunnel_handle, ) .await; diff --git a/devolutions-gateway/src/credential_injection_kdc.rs b/devolutions-gateway/src/credential_injection_kdc.rs index 4d6b8a0d4..daaca852b 100644 --- a/devolutions-gateway/src/credential_injection_kdc.rs +++ b/devolutions-gateway/src/credential_injection_kdc.rs @@ -41,16 +41,14 @@ pub(crate) struct CredentialInjectionKdc { kdc_config: kdc::config::KerberosServer, } -pub(crate) type CredentialInjectionKdcResolution = Option>; - #[derive(Debug, Error)] pub(crate) enum CredentialInjectionKdcResolveError { #[error("credential-injection state is not available for {jti}")] MissingCredential { jti: Uuid }, + #[error("credential-injection state for {jti} has expired")] + ExpiredCredential { jti: Uuid }, #[error("credential-injection state is not available for {jti}")] NonInjectionCredential { jti: Uuid }, - #[error("credential-injection context is not available for {jti}")] - MissingContext { jti: Uuid }, #[error("association token for {jti} is not valid for credential injection")] InvalidAssociationToken { jti: Uuid, @@ -118,25 +116,6 @@ impl fmt::Debug for CredentialInjectionKdc { } impl CredentialInjectionKdc { - pub(crate) fn from_entry( - jti: Uuid, - credential_entry: ArcCredentialEntry, - context_store: &CredentialInjectionKdcContextStoreHandle, - ) -> anyhow::Result { - let mapping = credential_entry - .mapping - .as_ref() - .context("credential entry has no credential-injection mapping")?; - let proxy_username = app_credential_username(&mapping.proxy).to_owned(); - let session = context_store - .get_or_insert_session(jti, || derive_credential_injection_kdc_session(&proxy_username, jti)) - .context("credential-injection context is not available")?; - let target_hostname = crate::token::extract_credential_injection_target_hostname(&credential_entry.token) - .context("extract credential-injection target hostname from association token")?; - - Self::from_parts(jti, credential_entry, target_hostname, session) - } - fn from_parts( jti: Uuid, credential_entry: ArcCredentialEntry, @@ -164,48 +143,6 @@ impl CredentialInjectionKdc { }) } - pub(crate) fn resolve( - jet_cred_id: Option, - credential_store: &CredentialStoreHandle, - context_store: &CredentialInjectionKdcContextStoreHandle, - ) -> Result { - let Some(jti) = jet_cred_id else { - return Ok(None); - }; - - let credential_entry = credential_store.get(jti).ok_or_else(|| { - warn!(%jti, "KDC token references missing credential-injection state"); - CredentialInjectionKdcResolveError::MissingCredential { jti } - })?; - - let Some(mapping) = credential_entry.mapping.as_ref() else { - warn!(%jti, "KDC token references non-injection credential state"); - return Err(CredentialInjectionKdcResolveError::NonInjectionCredential { jti }); - }; - - let proxy_username = app_credential_username(&mapping.proxy).to_owned(); - let session = context_store - .get_or_insert_session(jti, || derive_credential_injection_kdc_session(&proxy_username, jti)) - .ok_or_else(|| { - warn!(%jti, "KDC token references missing credential-injection context"); - CredentialInjectionKdcResolveError::MissingContext { jti } - })?; - let target_hostname = crate::token::extract_credential_injection_target_hostname(&credential_entry.token) - .map_err(|source| { - warn!( - %jti, - error = format!("{source:#}"), - "KDC token references invalid credential-injection association token" - ); - CredentialInjectionKdcResolveError::InvalidAssociationToken { jti, source } - })?; - - let kdc = Self::from_parts(jti, credential_entry, target_hostname, session) - .map_err(|source| CredentialInjectionKdcResolveError::BuildKdcConfig { jti, source })?; - - Ok(Some(Box::new(kdc))) - } - pub(crate) fn jti(&self) -> Uuid { self.jti } @@ -490,59 +427,146 @@ fn random_32_bytes() -> Vec { bytes } -/// Store of credential-injection contexts keyed by association-token JTI. +/// One-stop service for credential storage and credential-injection KDC state. /// -/// Association tokens are validated at `provision-credentials` time, but target hostnames are -/// extracted lazily from the original token on first CredSSP/KDC use. -/// Kerberos sessions are also created lazily from the credential entry on first CredSSP/KDC use. -/// The store is separate from the credential store so that the credential store stays Kerberos-unaware. -#[derive(Debug, Clone, Default)] -pub struct CredentialInjectionKdcContextStoreHandle(Arc>>); +/// Wraps the protocol-neutral [`CredentialStoreHandle`] and adds a Kerberos session cache keyed by +/// association-token JTI. The credential store remains the single source of truth for entry +/// lifetime; the session cache piggybacks on it (Arc-cloned credentials at lookup time, with stale +/// sessions evicted on insert-replacement and by a periodic sweep). +/// +/// All credential reads/writes — provision-credentials, RDP mode detection, KDC dispatch — go +/// through this service, so callers see one handle instead of coordinating a store and a registry. +#[derive(Debug, Clone)] +pub struct CredentialService { + credentials: CredentialStoreHandle, + sessions: Arc>>>, +} -#[derive(Debug)] -struct Entry { - session: Option>, - expires_at: time::OffsetDateTime, +impl Default for CredentialService { + fn default() -> Self { + Self::new() + } } -impl CredentialInjectionKdcContextStoreHandle { +impl CredentialService { pub fn new() -> Self { - Self(Arc::new(Mutex::new(HashMap::new()))) + Self { + credentials: CredentialStoreHandle::new(), + sessions: Arc::new(Mutex::new(HashMap::new())), + } } - pub(crate) fn register_provisioned_credentials(&self, jti: Uuid, time_to_live: time::Duration) { - let entry = Entry { - session: None, - expires_at: time::OffsetDateTime::now_utc() + time_to_live, + /// Insert (or replace) a credential entry keyed by the token's JTI. + /// + /// Any previously-cached Kerberos session for the same JTI is dropped: it was derived from + /// the prior provisioning and is no longer valid for the new entry. We invalidate even when + /// `CredentialStoreHandle::insert` reports no replacement, because the prior entry may have + /// already been evicted by `credential::CleanupTask` while its session cache entry was still + /// awaiting the next `sweep_orphans` tick — without an unconditional drop here, a fresh + /// provisioning under the same JTI would reuse stale key material. + pub fn insert( + &self, + token: String, + mapping: Option, + time_to_live: time::Duration, + ) -> Result, crate::credential::InsertError> { + // Snapshot the JTI from the new token so we can invalidate the matching session entry + // regardless of whether the credential store reports a replacement. `CredentialStore::insert` + // re-extracts internally; both calls go through the same code path, so an invalid token + // here will surface as the same `InvalidToken` error downstream. + let jti = crate::token::extract_jti(&token) + .context("failed to extract token ID") + .map_err(crate::credential::InsertError::InvalidToken)?; + let previous = self.credentials.insert(token, mapping, time_to_live)?; + self.sessions.lock().remove(&jti); + Ok(previous) + } + + /// Look up a credential entry by its association-token JTI. + pub fn get(&self, jti: Uuid) -> Option { + self.credentials.get(jti) + } + + /// Borrow the inner [`CredentialStoreHandle`] for plumbing that genuinely needs the + /// protocol-neutral primitive (e.g. wiring the background expiry task). + pub fn credential_store(&self) -> &CredentialStoreHandle { + &self.credentials + } + + /// Resolve the credential-injection KDC bound to the given association-token JTI. + /// + /// Returns the per-call KDC view; the underlying Kerberos session (krbtgt key, acceptor + /// long-term key, acceptor password) is cached so the in-process KDC and the CredSSP acceptor + /// see identical key material for the lifetime of the provisioned credentials. + pub(crate) fn kdc_for(&self, jti: Uuid) -> Result { + let credential_entry = self.credentials.get(jti).ok_or_else(|| { + warn!(%jti, "KDC token references missing credential-injection state"); + CredentialInjectionKdcResolveError::MissingCredential { jti } + })?; + + // `CredentialStoreHandle::get` does not enforce expiry — entries are evicted asynchronously + // by the credential cleanup task. Treat a stale entry as already gone so we never build a + // KDC against expired credentials. + if time::OffsetDateTime::now_utc() >= credential_entry.expires_at { + warn!(%jti, "KDC token references expired credential-injection state"); + self.sessions.lock().remove(&jti); + return Err(CredentialInjectionKdcResolveError::ExpiredCredential { jti }); + } + + let mapping = credential_entry.mapping.as_ref().ok_or_else(|| { + warn!(%jti, "KDC token references non-injection credential state"); + CredentialInjectionKdcResolveError::NonInjectionCredential { jti } + })?; + + let target_hostname = crate::token::extract_credential_injection_target_hostname(&credential_entry.token) + .map_err(|source| { + warn!( + %jti, + error = format!("{source:#}"), + "KDC token references invalid credential-injection association token" + ); + CredentialInjectionKdcResolveError::InvalidAssociationToken { jti, source } + })?; + + let proxy_username = app_credential_username(&mapping.proxy).to_owned(); + // Atomic get-or-insert: holds the lock long enough to guarantee a single Arc + // wins for this JTI even under concurrent `kdc_for` calls. The derivation is fast (a few + // hundred bytes of OsRng) so doing it under the lock is acceptable. + let session = { + let mut sessions = self.sessions.lock(); + let session = sessions + .entry(jti) + .or_insert_with(|| Arc::new(derive_credential_injection_kdc_session(&proxy_username, jti))); + Arc::clone(session) }; - self.0.lock().insert(jti, entry); + + CredentialInjectionKdc::from_parts(jti, credential_entry, target_hostname, session) + .map_err(|source| CredentialInjectionKdcResolveError::BuildKdcConfig { jti, source }) } - fn get_or_insert_session( - &self, - jti: Uuid, - make_session: impl FnOnce() -> CredentialInjectionKdcSession, - ) -> Option> { - let now = time::OffsetDateTime::now_utc(); - let mut entries = self.0.lock(); - - let entry = entries.get_mut(&jti).filter(|entry| now < entry.expires_at)?; - - let session = match &entry.session { - Some(session) => Arc::clone(session), - None => { - let session = Arc::new(make_session()); - entry.session = Some(Arc::clone(&session)); - session - } + fn sweep_orphans(&self) { + let stale_jtis: Vec = { + let sessions = self.sessions.lock(); + sessions + .keys() + .copied() + .filter(|jti| self.credentials.get(*jti).is_none()) + .collect() }; - Some(session) + if stale_jtis.is_empty() { + return; + } + + let mut sessions = self.sessions.lock(); + for jti in stale_jtis { + sessions.remove(&jti); + } } } pub struct CleanupTask { - pub handle: CredentialInjectionKdcContextStoreHandle, + pub service: CredentialService, } #[async_trait] @@ -552,13 +576,13 @@ impl Task for CleanupTask { const NAME: &'static str = "credential injection kdc cleanup"; async fn run(self, shutdown_signal: ShutdownSignal) -> Self::Output { - cleanup_task(self.handle, shutdown_signal).await; + cleanup_task(self.service, shutdown_signal).await; Ok(()) } } #[instrument(skip_all)] -async fn cleanup_task(handle: CredentialInjectionKdcContextStoreHandle, mut shutdown_signal: ShutdownSignal) { +async fn cleanup_task(service: CredentialService, mut shutdown_signal: ShutdownSignal) { use tokio::time::{Duration, sleep}; const TASK_INTERVAL: Duration = Duration::from_secs(60 * 15); // 15 minutes @@ -573,8 +597,7 @@ async fn cleanup_task(handle: CredentialInjectionKdcContextStoreHandle, mut shut } } - let now = time::OffsetDateTime::now_utc(); - handle.0.lock().retain(|_, entry| now < entry.expires_at); + service.sweep_orphans(); } debug!("Task terminated"); @@ -671,38 +694,152 @@ mod tests { } #[test] - fn store_lookup_filters_expired_entries() { - let store = CredentialInjectionKdcContextStoreHandle::new(); + fn service_kdc_for_rejects_expired_credential_entry() { + let service = CredentialService::new(); + let jti = Uuid::new_v4(); + + // Negative TTL: entry is born already expired. `CredentialStoreHandle::get` does not + // filter on expiry, so the service's own check is what guarantees we never build a KDC + // over stale credentials. + service + .insert( + association_token(jti), + Some(cleartext_mapping_with_target_username("target")), + time::Duration::seconds(-1), + ) + .expect("credential entry inserts"); + + assert!( + matches!( + service.kdc_for(jti), + Err(CredentialInjectionKdcResolveError::ExpiredCredential { .. }) + ), + "expired credentials must not yield a KDC" + ); + } + + #[test] + fn service_kdc_for_returns_same_session_under_concurrent_calls() { + let service = CredentialService::new(); let jti = Uuid::new_v4(); - // Negative TTL: entry is born already expired. - store.register_provisioned_credentials(jti, time::Duration::seconds(-1)); + service + .insert( + association_token(jti), + Some(cleartext_mapping_with_target_username("target")), + time::Duration::minutes(5), + ) + .expect("credential entry inserts"); + + let first = service.kdc_for(jti).expect("first call resolves"); + let second = service.kdc_for(jti).expect("second call resolves"); + + // The Kerberos session is the piece that must be stable across calls; the per-call KDC + // view rebuilds the rest. Compare via the long-term acceptor key as a session-identity + // probe. + let first_key = first.session.acceptor.long_term_key.expose_secret().clone(); + let second_key = second.session.acceptor.long_term_key.expose_secret().clone(); + assert_eq!( + first_key, second_key, + "concurrent kdc_for must share one cached session per JTI" + ); + } + + #[test] + fn service_insert_drops_stale_session_even_without_credential_replacement() { + let service = CredentialService::new(); + let jti = Uuid::new_v4(); + + // Simulate the race called out by Codex: a previous provisioning's session is still + // cached, but the credential entry has already been evicted (e.g. by + // `credential::cleanup_task`) and `sweep_orphans` has not run yet. A fresh provisioning + // under the same JTI must drop the stale session regardless of whether + // `CredentialStoreHandle::insert` reports a replacement, otherwise the next `kdc_for` + // would reuse the old key material. + let stale_session = Arc::new(derive_credential_injection_kdc_session("proxy@example.invalid", jti)); + service.sessions.lock().insert(jti, Arc::clone(&stale_session)); + + let previous = service + .insert( + association_token(jti), + Some(cleartext_mapping_with_target_username("target")), + time::Duration::minutes(5), + ) + .expect("credential entry inserts"); + assert!(previous.is_none(), "test precondition: no credential replacement"); assert!( - store - .get_or_insert_session(jti, || derive_credential_injection_kdc_session( - "proxy@example.invalid", - jti - )) - .is_none(), - "expired entry must not be returned" + !service.sessions.lock().contains_key(&jti), + "insert must drop stale session even when no credential replacement occurred" ); } #[test] - fn store_returns_fresh_entry() { - let store = CredentialInjectionKdcContextStoreHandle::new(); + fn service_insert_replacement_drops_cached_kerberos_material() { + let service = CredentialService::new(); let jti = Uuid::new_v4(); - store.register_provisioned_credentials(jti, time::Duration::minutes(5)); + service + .insert( + association_token(jti), + Some(cleartext_mapping_with_target_username("target")), + time::Duration::minutes(5), + ) + .expect("credential entry inserts"); - let session = store - .get_or_insert_session(jti, || { - derive_credential_injection_kdc_session("proxy@example.invalid", jti) - }) - .expect("fresh entry returned"); + let first = service.kdc_for(jti).expect("first call resolves"); + let first_key = first.session.acceptor.long_term_key.expose_secret().clone(); - assert_eq!(session.realm, "example.invalid"); + // Re-insert under the same JTI: the cached session for the previous entry must be evicted + // automatically, otherwise the new KDC would carry stale key material that the freshly + // provisioned credentials no longer match. + service + .insert( + association_token(jti), + Some(cleartext_mapping_with_target_username("target")), + time::Duration::minutes(5), + ) + .expect("credential entry re-inserts"); + + let second = service.kdc_for(jti).expect("second call resolves with fresh session"); + let second_key = second.session.acceptor.long_term_key.expose_secret().clone(); + + assert_ne!( + first_key, second_key, + "insert-replacement must force a fresh session derivation" + ); + } + + #[test] + fn service_sweep_orphans_drops_sessions_with_no_credential_entry() { + let service = CredentialService::new(); + let jti = Uuid::new_v4(); + + service + .insert( + association_token(jti), + Some(cleartext_mapping_with_target_username("target")), + time::Duration::minutes(5), + ) + .expect("credential entry inserts"); + + service.kdc_for(jti).expect("kdc_for populates session cache"); + assert!(service.sessions.lock().contains_key(&jti), "session cached"); + + // Simulate credential store eviction: build a parallel service whose credential store is + // empty but whose session cache is shared with the original. A more faithful test would + // drive `credential::cleanup_task` to expire the entry, but it sleeps for 15 minutes + // between ticks. Swapping the inner store is the deterministic equivalent. + let orphaned_service = CredentialService { + credentials: CredentialStoreHandle::new(), + sessions: Arc::clone(&service.sessions), + }; + + orphaned_service.sweep_orphans(); + assert!( + !orphaned_service.sessions.lock().contains_key(&jti), + "sweep must drop sessions whose JTI is no longer in credential_store" + ); } #[test] @@ -735,17 +872,6 @@ mod tests { ); } - #[test] - fn resolve_with_no_jet_cred_id_forwards_to_real_kdc() { - let credential_store = CredentialStoreHandle::new(); - let context_store = CredentialInjectionKdcContextStoreHandle::new(); - - let dispatch = CredentialInjectionKdc::resolve(None, &credential_store, &context_store) - .expect("plain KDC token should dispatch"); - - assert!(dispatch.is_none()); - } - #[test] fn from_parts_rejects_mismatched_entry_and_session_jti() { let entry_jti = Uuid::new_v4(); @@ -768,77 +894,54 @@ mod tests { } #[test] - fn resolve_with_missing_jet_cred_id_fails_closed() { - let credential_store = CredentialStoreHandle::new(); - let context_store = CredentialInjectionKdcContextStoreHandle::new(); + fn service_kdc_for_rejects_unknown_jti() { + let service = CredentialService::new(); assert!( - CredentialInjectionKdc::resolve(Some(Uuid::new_v4()), &credential_store, &context_store).is_err(), + matches!( + service.kdc_for(Uuid::new_v4()), + Err(CredentialInjectionKdcResolveError::MissingCredential { .. }) + ), "KDC tokens with jet_cred_id must not fall back to real-KDC forwarding" ); } #[test] - fn resolve_with_non_injection_entry_fails_closed() { - let credential_store = CredentialStoreHandle::new(); - let context_store = CredentialInjectionKdcContextStoreHandle::new(); + fn service_kdc_for_rejects_non_injection_entry() { + let service = CredentialService::new(); let jti = Uuid::new_v4(); - credential_store + service .insert(association_token(jti), None, time::Duration::minutes(5)) .expect("provision-token entry inserts"); assert!( - CredentialInjectionKdc::resolve(Some(jti), &credential_store, &context_store).is_err(), + matches!( + service.kdc_for(jti), + Err(CredentialInjectionKdcResolveError::NonInjectionCredential { .. }) + ), "KDC tokens with jet_cred_id must require provision-credentials state" ); } #[test] - fn resolve_lazily_extracts_target_hostname_from_entry_token() { - let credential_store = CredentialStoreHandle::new(); - let context_store = CredentialInjectionKdcContextStoreHandle::new(); + fn service_kdc_for_lazily_extracts_target_hostname_from_entry_token() { + let service = CredentialService::new(); let jti = Uuid::new_v4(); - credential_store + service .insert( association_token(jti), Some(cleartext_mapping_with_target_username("target")), time::Duration::minutes(5), ) .expect("credential entry inserts"); - context_store.register_provisioned_credentials(jti, time::Duration::minutes(5)); - let kdc = CredentialInjectionKdc::resolve(Some(jti), &credential_store, &context_store) - .expect("credential-injection KDC resolves") - .expect("credential-injection KDC is selected"); + let kdc = service.kdc_for(jti).expect("credential-injection KDC resolves"); assert_eq!(kdc.target_hostname, "target.example"); } - #[test] - fn resolve_with_missing_context_fails_closed() { - let credential_store = CredentialStoreHandle::new(); - let context_store = CredentialInjectionKdcContextStoreHandle::new(); - let jti = Uuid::new_v4(); - - credential_store - .insert( - association_token(jti), - Some(cleartext_mapping_with_target_username("target")), - time::Duration::minutes(5), - ) - .expect("credential entry inserts"); - - assert!( - matches!( - CredentialInjectionKdc::resolve(Some(jti), &credential_store, &context_store), - Err(CredentialInjectionKdcResolveError::MissingContext { .. }) - ), - "KDC tokens with jet_cred_id must require provision-credentials context" - ); - } - #[test] fn intercept_ignores_non_loopback_host() { let jti = Uuid::new_v4(); diff --git a/devolutions-gateway/src/generic_client.rs b/devolutions-gateway/src/generic_client.rs index 5ea8ee41d..d44595864 100644 --- a/devolutions-gateway/src/generic_client.rs +++ b/devolutions-gateway/src/generic_client.rs @@ -8,8 +8,7 @@ use tracing::field; use typed_builder::TypedBuilder; use crate::config::Conf; -use crate::credential::CredentialStoreHandle; -use crate::credential_injection_kdc::{CredentialInjectionKdc, CredentialInjectionKdcContextStoreHandle}; +use crate::credential_injection_kdc::CredentialService; use crate::proxy::Proxy; use crate::rdp_pcb::{extract_association_claims, read_pcb}; use crate::recording::ActiveRecordings; @@ -28,8 +27,7 @@ pub struct GenericClient { sessions: SessionMessageSender, subscriber_tx: SubscriberSender, active_recordings: Arc, - credential_store: CredentialStoreHandle, - credential_injection_context_store: CredentialInjectionKdcContextStoreHandle, + credentials: CredentialService, #[builder(default)] agent_tunnel_handle: Option>, } @@ -53,8 +51,7 @@ where sessions, subscriber_tx, active_recordings, - credential_store, - credential_injection_context_store, + credentials, agent_tunnel_handle, } = self; @@ -155,12 +152,11 @@ where // The credential store is keyed on the association token's JTI, so a direct // lookup by `claims.jti` is the primary path. if is_rdp - && let Some(entry) = credential_store.get(claims.jti) + && let Some(entry) = credentials.get(claims.jti) && entry.mapping.is_some() { anyhow::ensure!(token == entry.token, "token mismatch"); - let credential_injection_kdc = - CredentialInjectionKdc::from_entry(claims.jti, entry, &credential_injection_context_store)?; + let credential_injection_kdc = credentials.kdc_for(claims.jti)?; info!( jti = %credential_injection_kdc.jti(), diff --git a/devolutions-gateway/src/lib.rs b/devolutions-gateway/src/lib.rs index bebcd53a8..5c1777e6e 100644 --- a/devolutions-gateway/src/lib.rs +++ b/devolutions-gateway/src/lib.rs @@ -60,8 +60,7 @@ pub struct DgwState { pub shutdown_signal: devolutions_gateway_task::ShutdownSignal, pub recordings: recording::RecordingMessageSender, pub job_queue_handle: job_queue::JobQueueHandle, - pub credential_store: credential::CredentialStoreHandle, - pub credential_injection_context_store: credential_injection_kdc::CredentialInjectionKdcContextStoreHandle, + pub credentials: credential_injection_kdc::CredentialService, pub monitoring_state: Arc, pub traffic_audit_handle: traffic_audit::TrafficAuditHandle, pub agent_tunnel_handle: Option>, @@ -89,9 +88,7 @@ impl DgwState { let (shutdown_handle, shutdown_signal) = devolutions_gateway_task::ShutdownHandle::new(); let (job_queue_handle, job_queue_rx) = job_queue::JobQueueHandle::new(); let (traffic_audit_handle, traffic_audit_rx) = traffic_audit::TrafficAuditHandle::new(); - let credential_store = credential::CredentialStoreHandle::new(); - let credential_injection_context_store = - credential_injection_kdc::CredentialInjectionKdcContextStoreHandle::new(); + let credentials = credential_injection_kdc::CredentialService::new(); let monitoring_state = Arc::new(network_monitor::State::new(Arc::new(MockMonitorsCache))?); let state = Self { @@ -104,8 +101,7 @@ impl DgwState { recordings: recording_manager_handle, job_queue_handle, traffic_audit_handle, - credential_store, - credential_injection_context_store, + credentials, monitoring_state, agent_tunnel_handle: None, }; diff --git a/devolutions-gateway/src/listener.rs b/devolutions-gateway/src/listener.rs index 288884ab4..5e23f5f8a 100644 --- a/devolutions-gateway/src/listener.rs +++ b/devolutions-gateway/src/listener.rs @@ -158,8 +158,7 @@ async fn handle_tcp_peer(stream: TcpStream, state: DgwState, peer_addr: SocketAd .sessions(state.sessions) .subscriber_tx(state.subscriber_tx) .active_recordings(state.recordings.active_recordings) - .credential_store(state.credential_store) - .credential_injection_context_store(state.credential_injection_context_store) + .credentials(state.credentials) .agent_tunnel_handle(state.agent_tunnel_handle) .build() .serve() diff --git a/devolutions-gateway/src/ngrok.rs b/devolutions-gateway/src/ngrok.rs index 2a1fd3167..9e2e846bd 100644 --- a/devolutions-gateway/src/ngrok.rs +++ b/devolutions-gateway/src/ngrok.rs @@ -237,8 +237,7 @@ async fn run_tcp_tunnel(mut tunnel: ngrok::tunnel::TcpTunnel, state: DgwState) { .sessions(state.sessions) .subscriber_tx(state.subscriber_tx) .active_recordings(state.recordings.active_recordings) - .credential_store(state.credential_store) - .credential_injection_context_store(state.credential_injection_context_store) + .credentials(state.credentials) .agent_tunnel_handle(state.agent_tunnel_handle) .build() .serve() diff --git a/devolutions-gateway/src/rd_clean_path.rs b/devolutions-gateway/src/rd_clean_path.rs index 0ca951242..bbe69290c 100644 --- a/devolutions-gateway/src/rd_clean_path.rs +++ b/devolutions-gateway/src/rd_clean_path.rs @@ -11,8 +11,7 @@ use tokio::io::{AsyncRead, AsyncReadExt as _, AsyncWrite, AsyncWriteExt as _}; use tracing::field; use crate::config::Conf; -use crate::credential::CredentialStoreHandle; -use crate::credential_injection_kdc::{CredentialInjectionKdc, CredentialInjectionKdcContextStoreHandle}; +use crate::credential_injection_kdc::{CredentialInjectionKdc, CredentialService}; use crate::proxy::Proxy; use crate::recording::ActiveRecordings; use crate::session::{ConnectionModeDetails, DisconnectInterest, DisconnectedInfo, SessionInfo, SessionMessageSender}; @@ -521,8 +520,7 @@ pub async fn handle( sessions: SessionMessageSender, subscriber_tx: SubscriberSender, active_recordings: &ActiveRecordings, - credential_store: &CredentialStoreHandle, - credential_injection_context_store: &CredentialInjectionKdcContextStoreHandle, + credentials: &CredentialService, agent_tunnel_handle: Option>, ) -> anyhow::Result<()> { // Special handshake of our RDP extension @@ -543,11 +541,10 @@ pub async fn handle( // proxy-based credential injection mode. Otherwise, we continue the usual // clean path procedure. The credential store is keyed on the association token's JTI. if let Some(jti) = crate::token::extract_jti(token).ok() - && let Some(entry) = credential_store.get(jti) + && let Some(entry) = credentials.get(jti) && entry.mapping.is_some() { - let credential_injection_kdc = - CredentialInjectionKdc::from_entry(jti, entry, credential_injection_context_store)?; + let credential_injection_kdc = credentials.kdc_for(jti)?; anyhow::ensure!(token == credential_injection_kdc.raw_token(), "token mismatch"); debug!( jti = %credential_injection_kdc.jti(), diff --git a/devolutions-gateway/src/service.rs b/devolutions-gateway/src/service.rs index 3cb1ee5c1..e847e1d94 100644 --- a/devolutions-gateway/src/service.rs +++ b/devolutions-gateway/src/service.rs @@ -3,7 +3,6 @@ use std::time::Duration; use anyhow::Context as _; use devolutions_gateway::config::{Conf, ConfHandle}; -use devolutions_gateway::credential::CredentialStoreHandle; use devolutions_gateway::listener::{GatewayListener, ListenerUrls}; use devolutions_gateway::log::GatewayLog; use devolutions_gateway::recording::recording_message_channel; @@ -268,9 +267,7 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { .await .context("failed to initialize traffic audit manager")?; - let credential_store = CredentialStoreHandle::new(); - let credential_injection_context_store = - devolutions_gateway::credential_injection_kdc::CredentialInjectionKdcContextStoreHandle::new(); + let credentials = devolutions_gateway::credential_injection_kdc::CredentialService::new(); let filesystem_monitor_config_cache = devolutions_gateway::api::monitoring::FilesystemConfigCache::new( config::get_data_dir().join("monitors_cache.json"), @@ -318,8 +315,7 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { shutdown_signal: tasks.shutdown_signal.clone(), recordings: recording_manager_handle.clone(), job_queue_handle: job_queue_ctx.job_queue_handle.clone(), - credential_store: credential_store.clone(), - credential_injection_context_store: credential_injection_context_store.clone(), + credentials: credentials.clone(), monitoring_state, traffic_audit_handle: traffic_audit_task.handle(), agent_tunnel_handle, @@ -355,12 +351,10 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { tasks.register(devolutions_gateway::token::CleanupTask { token_cache }); tasks.register(devolutions_gateway::credential::CleanupTask { - handle: credential_store, + handle: credentials.credential_store().clone(), }); - tasks.register(devolutions_gateway::credential_injection_kdc::CleanupTask { - handle: credential_injection_context_store, - }); + tasks.register(devolutions_gateway::credential_injection_kdc::CleanupTask { service: credentials }); tasks.register(devolutions_log::LogDeleterTask::::new( conf.log_file.clone(), From 6839e3b610de7b560d7a7723f505049ee151205f Mon Sep 17 00:00:00 2001 From: irving ou Date: Wed, 20 May 2026 11:53:23 -0400 Subject: [PATCH 5/5] chore(dgw): drop unused ProvisionCredentialsRequest.cs and link sspi-rs#664 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Delete utils/dotnet/Devolutions.Gateway.Utils/src/ProvisionCredentialsRequest.cs. The handwritten C# DTO was added alongside the original DGW-378 work but carries no production callers — DVLS hand-rolls its provisioning payload against the generated Devolutions.Gateway.Client SDK. CBenoit asked for this on PR #1768. - Add a TODO(sspi-rs#664) reference next to IN_PROCESS_KDC_HOST so the loopback trampoline points at the upstream issue Pavlo opened (https://github.com/Devolutions/sspi-rs/issues/664) for a pluggable KDC dispatcher API. Once sspi-rs ships it, this `.invalid`-host intercept_network_request hook collapses into a normal trait impl. --- .../src/credential_injection_kdc.rs | 6 ++ .../src/ProvisionCredentialsRequest.cs | 57 ------------------- 2 files changed, 6 insertions(+), 57 deletions(-) delete mode 100644 utils/dotnet/Devolutions.Gateway.Utils/src/ProvisionCredentialsRequest.cs diff --git a/devolutions-gateway/src/credential_injection_kdc.rs b/devolutions-gateway/src/credential_injection_kdc.rs index daaca852b..b42a83529 100644 --- a/devolutions-gateway/src/credential_injection_kdc.rs +++ b/devolutions-gateway/src/credential_injection_kdc.rs @@ -28,6 +28,12 @@ use uuid::Uuid; use crate::credential::{AppCredential, AppCredentialMapping, ArcCredentialEntry, CredentialStoreHandle}; +// The reserved `.invalid` TLD (RFC 6761) lets sspi-rs CredSSP server emit "KDC requests" that +// never leave the process: `intercept_network_request` recognises this hostname and dispatches +// the message into the in-process `kdc` server below. +// +// TODO(sspi-rs#664): replace this URL-trampoline with a pluggable KDC dispatcher trait once +// sspi-rs ships the API — see https://github.com/Devolutions/sspi-rs/issues/664. const IN_PROCESS_KDC_HOST: &str = "cred.invalid"; pub(crate) struct CredentialInjectionKdc { diff --git a/utils/dotnet/Devolutions.Gateway.Utils/src/ProvisionCredentialsRequest.cs b/utils/dotnet/Devolutions.Gateway.Utils/src/ProvisionCredentialsRequest.cs deleted file mode 100644 index ccef5a0dd..000000000 --- a/utils/dotnet/Devolutions.Gateway.Utils/src/ProvisionCredentialsRequest.cs +++ /dev/null @@ -1,57 +0,0 @@ -using System.Text.Json.Serialization; - -namespace Devolutions.Gateway.Utils; - -public class ProvisionCredentialsRequest -{ - [JsonPropertyName("id")] - public Guid Id { get; set; } - - [JsonPropertyName("kind")] - public string Kind => "provision-credentials"; - - [JsonPropertyName("token")] - public string Token { get; set; } - - [JsonPropertyName("proxy_credential")] - public CleartextCredential ProxyCredential { get; set; } - - [JsonPropertyName("target_credential")] - public CleartextCredential TargetCredential { get; set; } - - [JsonPropertyName("time_to_live")] - [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] - public uint? TimeToLive { get; set; } - - public ProvisionCredentialsRequest( - Guid id, - string token, - CleartextCredential proxyCredential, - CleartextCredential targetCredential, - uint? timeToLive = null) - { - this.Id = id; - this.Token = token; - this.ProxyCredential = proxyCredential; - this.TargetCredential = targetCredential; - this.TimeToLive = timeToLive; - } -} - -public class CleartextCredential -{ - [JsonPropertyName("kind")] - public string Kind => "username-password"; - - [JsonPropertyName("username")] - public string Username { get; set; } - - [JsonPropertyName("password")] - public string Password { get; set; } - - public CleartextCredential(string username, string password) - { - this.Username = username; - this.Password = password; - } -}