diff --git a/devolutions-gateway/src/api/preflight.rs b/devolutions-gateway/src/api/preflight.rs index d5183c081..5598d44ec 100644 --- a/devolutions-gateway/src/api/preflight.rs +++ b/devolutions-gateway/src/api/preflight.rs @@ -23,6 +23,7 @@ const OP_GET_RUNNING_SESSION_COUNT: &str = "get-running-session-count"; const OP_GET_RECORDING_STORAGE_HEALTH: &str = "get-recording-storage-health"; const OP_PROVISION_TOKEN: &str = "provision-token"; const OP_PROVISION_CREDENTIALS: &str = "provision-credentials"; +const OP_PROVISION_CONNECTION_OPTIONS: &str = "provision-connection-options"; const OP_RESOLVE_HOST: &str = "resolve-host"; const DEFAULT_TTL: Duration = Duration::minutes(15); @@ -50,6 +51,13 @@ struct ProvisionCredentialsParams { time_to_live: Option, } +#[derive(Debug, Deserialize)] +struct ProvisionConnectionOptionsParams { + token: String, + connection_options: crate::target_connection_options::TargetConnectionOptions, + time_to_live: Option, +} + #[derive(Debug, Deserialize)] struct ResolveHostParams { #[serde(rename = "host_to_resolve")] @@ -310,6 +318,8 @@ async fn handle_operation( }); } OP_PROVISION_TOKEN | OP_PROVISION_CREDENTIALS => { + // Same store path as master: provision-token inserts a token-only row (mapping=None); + // provision-credentials inserts with a mapping. Connection options are a separate op. 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 } = @@ -323,20 +333,7 @@ async fn handle_operation( } = from_params(operation.params).map_err(PreflightError::invalid_params)?; (token, time_to_live, Some(mapping)) }; - - let time_to_live = time_to_live - .map(i64::from) - .map(Duration::seconds) - .unwrap_or(DEFAULT_TTL); - - if time_to_live > MAX_TTL { - return Err(PreflightError { - status: PreflightAlertStatus::InvalidParams, - message: format!( - "provided time_to_live ({time_to_live}) is exceeding the maximum TTL duration ({MAX_TTL})" - ), - }); - } + let time_to_live = validate_time_to_live(time_to_live)?; // 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 @@ -358,22 +355,20 @@ async fn handle_operation( })?; } - let previous_entry = credentials - .insert(token, mapping, time_to_live) + let replaced = credentials + .insert_credentials(token, mapping, time_to_live) .inspect_err(|error| warn!(%operation.id, error = format!("{error:#}"), "Failed to insert credentials")) .map_err(|error| match error { InsertError::InvalidToken(error) => { PreflightError::new(PreflightAlertStatus::InvalidParams, format!("invalid token: {error:#}")) } - InsertError::Internal(_) => PreflightError::new( + InsertError::CredentialEncryption(_) => PreflightError::new( PreflightAlertStatus::InternalServerError, "an internal error occurred".to_owned(), ), })?; - // `CredentialService::insert` already drops the cached Kerberos session for a - // replaced entry, so no explicit invalidation is needed here. - if previous_entry.is_some() { + if replaced { outputs.push(PreflightOutput { operation_id: operation.id, kind: PreflightOutputKind::Alert { @@ -388,6 +383,37 @@ async fn handle_operation( kind: PreflightOutputKind::Ack, }); } + OP_PROVISION_CONNECTION_OPTIONS => { + let ProvisionConnectionOptionsParams { + token, + connection_options, + time_to_live, + } = from_params(operation.params).map_err(PreflightError::invalid_params)?; + let time_to_live = validate_time_to_live(time_to_live)?; + + // Connection options are generic routing metadata, not credential-injection state, so + // they only need a JTI to key by — not the full credential-injection token shape. + let jti = crate::token::extract_jti(&token).map_err(|error| { + PreflightError::new(PreflightAlertStatus::InvalidParams, format!("invalid token: {error:#}")) + })?; + + let replaced = credentials.insert_connection_options(jti, connection_options, time_to_live); + + if replaced { + outputs.push(PreflightOutput { + operation_id: operation.id, + kind: PreflightOutputKind::Alert { + status: PreflightAlertStatus::Info, + message: "existing provisioned connection options were replaced".to_owned(), + }, + }); + } + + outputs.push(PreflightOutput { + operation_id: operation.id, + kind: PreflightOutputKind::Ack, + }); + } OP_RESOLVE_HOST => { let ResolveHostParams { host } = from_params(operation.params).map_err(PreflightError::invalid_params)?; @@ -422,6 +448,21 @@ async fn handle_operation( Ok(()) } +fn validate_time_to_live(time_to_live: Option) -> Result { + let time_to_live = time_to_live + .map(i64::from) + .map(Duration::seconds) + .unwrap_or(DEFAULT_TTL); + + if time_to_live > MAX_TTL { + return Err(PreflightError::new( + PreflightAlertStatus::InvalidParams, + format!("provided time_to_live ({time_to_live}) is exceeding the maximum TTL duration ({MAX_TTL})"), + )); + } + + Ok(time_to_live) +} fn from_params(params: serde_json::Map) -> serde_json::Result { serde_json::from_value(serde_json::Value::Object(params)) } diff --git a/devolutions-gateway/src/credential/mod.rs b/devolutions-gateway/src/credential/mod.rs index 0ab260e51..8c8673089 100644 --- a/devolutions-gateway/src/credential/mod.rs +++ b/devolutions-gateway/src/credential/mod.rs @@ -40,7 +40,7 @@ pub struct AppCredentialMapping { /// Cleartext credential received from the API, used for deserialization only. /// /// Passwords are encrypted and stored as [`AppCredential`] inside the provisioning store. -/// This type is never stored directly — hand it to [`crate::provisioning::ProvisioningStore::insert`]. +/// This type is never stored directly — hand it to [`crate::provisioning::ProvisioningStore::insert_credentials`]. #[derive(Debug, Deserialize)] #[serde(tag = "kind")] pub enum CleartextAppCredential { @@ -67,7 +67,7 @@ impl CleartextAppCredential { /// Cleartext credential mapping received from the API, used for deserialization only. /// -/// Passwords are encrypted on write. Hand this directly to [`crate::provisioning::ProvisioningStore::insert`]. +/// Passwords are encrypted on write. Hand this directly to [`crate::provisioning::ProvisioningStore::insert_credentials`]. #[derive(Debug, Deserialize)] pub struct CleartextAppCredentialMapping { #[serde(rename = "proxy_credential")] diff --git a/devolutions-gateway/src/credential_injection_kdc.rs b/devolutions-gateway/src/credential_injection_kdc.rs index 8596b026c..bfd28b42d 100644 --- a/devolutions-gateway/src/credential_injection_kdc.rs +++ b/devolutions-gateway/src/credential_injection_kdc.rs @@ -29,6 +29,7 @@ use uuid::Uuid; use crate::config::ConfHandle; use crate::credential::{AppCredential, AppCredentialMapping}; use crate::provisioning::{ArcProvisioningEntry, ProvisioningStore}; +use crate::target_connection_options::TargetConnectionOptions; // 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 @@ -42,6 +43,7 @@ pub(crate) struct CredentialInjectionKdc { jti: Uuid, raw_token: String, credential_mapping: AppCredentialMapping, + connection_options: Option, // Client target hostname. It is not a hostname of the end machine, but a DGW hostname the client // uses when connecting. target_hostname: String, @@ -55,10 +57,14 @@ pub(crate) struct CredentialInjectionKdc { 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("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, @@ -141,12 +147,17 @@ impl CredentialInjectionKdc { jti, raw_token: credential_entry.token.clone(), credential_mapping: mapping.clone(), + connection_options: credential_entry.connection_options.clone(), target_hostname, session, kdc_config, }) } + pub(crate) fn krb_kdc(&self) -> Option<&crate::target_addr::TargetAddr> { + self.connection_options.as_ref()?.krb_kdc() + } + pub(crate) fn jti(&self) -> Uuid { self.jti } @@ -472,34 +483,48 @@ impl CredentialService { } } - /// Insert (or replace) a credential entry keyed by the token's JTI. + /// Insert (or replace) the credentials half 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 - /// `ProvisioningStore::insert` reports no replacement, because the prior entry may have - /// already been evicted by `provisioning::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( + /// the store reports no replacement, because the prior entry may have already been evicted by + /// `provisioning::CleanupTask` while its session cache entry was still awaiting the next + /// `sweep_orphans` tick. + pub(crate) fn insert_credentials( &self, token: String, mapping: Option, time_to_live: time::Duration, - ) -> Result, crate::provisioning::InsertError> { + ) -> Result { // Snapshot the JTI from the new token so we can invalidate the matching session entry - // regardless of whether the credential store reports a replacement. `ProvisioningStore::insert` + // regardless of whether the credential store reports a replacement. `ProvisioningStore::insert_credentials` // 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::provisioning::InsertError::InvalidToken)?; - let previous = self.credentials.insert(token, mapping, time_to_live)?; + let replaced = self.credentials.insert_credentials(token, mapping, time_to_live)?; + self.sessions.lock().remove(&jti); + Ok(replaced) + } + + /// Insert (or replace) the connection-options half. Drops any cached Kerberos session for the + /// JTI because `krb_kdc` is part of the session's routing inputs. + pub(crate) fn insert_connection_options( + &self, + jti: Uuid, + connection_options: TargetConnectionOptions, + time_to_live: time::Duration, + ) -> bool { + let replaced = self + .credentials + .insert_connection_options(jti, connection_options, time_to_live); self.sessions.lock().remove(&jti); - Ok(previous) + replaced } /// Look up a credential entry by its association-token JTI. - pub fn get(&self, jti: Uuid) -> Option { + pub(crate) fn get(&self, jti: Uuid) -> Option { self.credentials.get(jti) } @@ -520,20 +545,22 @@ impl CredentialService { CredentialInjectionKdcResolveError::MissingCredential { jti } })?; - // `ProvisioningStore::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 } })?; + // Validate association-token shape for credential injection (dst_hst present, etc.). + // SPN / acceptor hostname comes from gateway config below (#1856), not dst_hst. + 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 @@ -667,7 +694,7 @@ mod tests { fn dummy_entry_with_target_username(jti: Uuid, target_username: &str) -> ArcProvisioningEntry { let store = ProvisioningStore::new(); store - .insert( + .insert_credentials( association_token(jti), Some(cleartext_mapping_with_target_username(target_username)), time::Duration::minutes(5), @@ -726,7 +753,7 @@ mod tests { // filter on expiry, so the service's own check is what guarantees we never build a KDC // over stale credentials. service - .insert( + .insert_credentials( association_token(jti), Some(cleartext_mapping_with_target_username("target")), time::Duration::seconds(-1), @@ -736,7 +763,7 @@ mod tests { assert!( matches!( service.kdc_for(jti), - Err(CredentialInjectionKdcResolveError::ExpiredCredential { .. }) + Err(CredentialInjectionKdcResolveError::MissingCredential { .. }) ), "expired credentials must not yield a KDC" ); @@ -748,7 +775,7 @@ mod tests { let jti = Uuid::new_v4(); service - .insert( + .insert_credentials( association_token(jti), Some(cleartext_mapping_with_target_username("target")), time::Duration::minutes(5), @@ -778,19 +805,19 @@ mod tests { // cached, but the credential entry has already been evicted (e.g. by // `provisioning::cleanup_task`) and `sweep_orphans` has not run yet. A fresh provisioning // under the same JTI must drop the stale session regardless of whether - // `ProvisioningStore::insert` reports a replacement, otherwise the next `kdc_for` + // `ProvisioningStore::insert_credentials` 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( + let replaced = service + .insert_credentials( 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!(!replaced, "test precondition: no credential replacement"); assert!( !service.sessions.lock().contains_key(&jti), @@ -804,7 +831,7 @@ mod tests { let jti = Uuid::new_v4(); service - .insert( + .insert_credentials( association_token(jti), Some(cleartext_mapping_with_target_username("target")), time::Duration::minutes(5), @@ -818,7 +845,7 @@ mod tests { // automatically, otherwise the new KDC would carry stale key material that the freshly // provisioned credentials no longer match. service - .insert( + .insert_credentials( association_token(jti), Some(cleartext_mapping_with_target_username("target")), time::Duration::minutes(5), @@ -840,7 +867,7 @@ mod tests { let jti = Uuid::new_v4(); service - .insert( + .insert_credentials( association_token(jti), Some(cleartext_mapping_with_target_username("target")), time::Duration::minutes(5), @@ -937,7 +964,7 @@ mod tests { let jti = Uuid::new_v4(); service - .insert(association_token(jti), None, time::Duration::minutes(5)) + .insert_credentials(association_token(jti), None, time::Duration::minutes(5)) .expect("provision-token entry inserts"); assert!( @@ -949,6 +976,26 @@ mod tests { ); } + #[test] + fn service_kdc_for_uses_gateway_hostname_for_spn() { + // #1856: SPN / acceptor hostname is the Gateway hostname from config, not dst_hst. + // Token dst_hst is still validated (missing/invalid shape fails kdc_for). + let service = CredentialService::new(mock_conf_handle()); + let jti = Uuid::new_v4(); + + service + .insert_credentials( + association_token(jti), + Some(cleartext_mapping_with_target_username("target")), + time::Duration::minutes(5), + ) + .expect("credential entry inserts"); + + let kdc = service.kdc_for(jti).expect("credential-injection KDC resolves"); + + assert_eq!(kdc.target_hostname, "dgateway.localhost.com"); + } + #[test] fn intercept_ignores_non_loopback_host() { let jti = Uuid::new_v4(); diff --git a/devolutions-gateway/src/kdc_connector.rs b/devolutions-gateway/src/kdc_connector.rs index 7b5cd1eca..f1840b643 100644 --- a/devolutions-gateway/src/kdc_connector.rs +++ b/devolutions-gateway/src/kdc_connector.rs @@ -261,7 +261,7 @@ impl KdcConnector { /// goes away entirely. pub async fn send_network_request(&self, request: &NetworkRequest) -> anyhow::Result> { match request.url.scheme() { - "tcp" | "udp" => { + scheme if crate::target_connection_options::is_supported_krb_kdc_scheme(scheme) => { let target_addr = TargetAddr::parse(request.url.as_str(), Some(88))?; self.send(&target_addr, &request.data) diff --git a/devolutions-gateway/src/lib.rs b/devolutions-gateway/src/lib.rs index f40dc1a4d..ae5ef4190 100644 --- a/devolutions-gateway/src/lib.rs +++ b/devolutions-gateway/src/lib.rs @@ -39,6 +39,7 @@ pub mod session; pub mod streaming; pub mod subscriber; pub mod target_addr; +pub(crate) mod target_connection_options; pub mod tls; pub mod token; pub mod traffic_audit; diff --git a/devolutions-gateway/src/provisioning.rs b/devolutions-gateway/src/provisioning.rs index 8047d9e8c..06b21c40b 100644 --- a/devolutions-gateway/src/provisioning.rs +++ b/devolutions-gateway/src/provisioning.rs @@ -2,42 +2,77 @@ use std::collections::HashMap; use std::fmt; use std::sync::Arc; -use anyhow::Context; +use anyhow::Context as _; use async_trait::async_trait; use devolutions_gateway_task::{ShutdownSignal, Task}; use parking_lot::Mutex; +use tracing::{debug, instrument, warn}; use uuid::Uuid; use crate::credential::{AppCredentialMapping, CleartextAppCredentialMapping}; +use crate::target_connection_options::TargetConnectionOptions; -/// Error returned by [`ProvisioningStore::insert`]. +/// Error returned when inserting into the credentials half of the provisioning store. #[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), + /// Credential encryption failed. + CredentialEncryption(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), + Self::CredentialEncryption(e) => e.fmt(f), } } } impl std::error::Error for InsertError {} -/// Data provisioned ahead of a connection, keyed by association-token JTI. +/// Combined, point-in-time view of everything provisioned for a session. +/// +/// Assembled on read from the two independent stores. The credentials half may be token-only +/// (`mapping` is `None`, as with `provision-token`) or carry a credential mapping +/// (`provision-credentials`). Connection options are optional and may be absent. +#[derive(Debug)] +pub struct ProvisioningEntry { + pub(crate) token: String, + pub(crate) mapping: Option, + pub(crate) connection_options: Option, +} + +pub type ArcProvisioningEntry = Arc; + +#[derive(Debug, Clone)] +struct CredentialsEntry { + token: String, + mapping: Option, + expires_at: time::OffsetDateTime, +} + +#[derive(Debug, Clone)] +struct ConnectionOptionsEntry { + connection_options: TargetConnectionOptions, + expires_at: time::OffsetDateTime, +} + +/// Two independent token-keyed stores that together provision a session. +/// +/// The credentials store is the encryption boundary: cleartext mappings are encrypted on the way +/// in, so entries only ever hold encrypted material. Token-only rows (`mapping = None`) match the +/// existing `provision-token` behavior on master. The connection-options store holds plaintext +/// routing metadata only and has no crypto dependency. /// -/// Credentials are the encryption boundary: cleartext material is encrypted on the way in, so -/// entries only ever hold encrypted passwords. +/// Both are keyed by the association-token JTI. The halves are provisioned by separate preflight +/// operations and may arrive, expire, or be replaced independently. #[derive(Debug, Clone)] -pub struct ProvisioningStore(Arc>); +pub struct ProvisioningStore { + credentials: Arc>>, + connection_options: Arc>>, +} impl Default for ProvisioningStore { fn default() -> Self { @@ -47,71 +82,89 @@ impl Default for ProvisioningStore { impl ProvisioningStore { pub fn new() -> Self { - Self(Arc::new(Mutex::new(ProvisioningEntries::new()))) + Self { + credentials: Arc::new(Mutex::new(HashMap::new())), + connection_options: Arc::new(Mutex::new(HashMap::new())), + } } - pub fn insert( + /// Insert or replace the credentials half (token-only or with a mapping). + /// + /// Same contract as master: `provision-token` passes `mapping = None`; + /// `provision-credentials` passes `Some(mapping)`. + pub(crate) fn insert_credentials( &self, token: String, mapping: Option, time_to_live: time::Duration, - ) -> Result, InsertError> { + ) -> Result { + let jti = crate::token::extract_jti(&token) + .context("failed to extract token ID") + .map_err(InsertError::InvalidToken)?; let mapping = mapping .map(CleartextAppCredentialMapping::encrypt) .transpose() - .map_err(InsertError::Internal)?; - self.0.lock().insert(token, mapping, time_to_live) - } - - pub fn get(&self, token_id: Uuid) -> Option { - self.0.lock().get(token_id) - } -} + .context("encrypt provisioned credentials") + .map_err(InsertError::CredentialEncryption)?; -#[derive(Debug)] -struct ProvisioningEntries { - entries: HashMap, -} + let entry = CredentialsEntry { + token, + mapping, + expires_at: time::OffsetDateTime::now_utc() + time_to_live, + }; -#[derive(Debug)] -pub struct ProvisioningEntry { - pub token: String, - pub mapping: Option, - pub expires_at: time::OffsetDateTime, -} + Ok(self.credentials.lock().insert(jti, entry).is_some()) + } -pub type ArcProvisioningEntry = Arc; + /// Insert or replace the connection-options half. Returns whether a prior entry was replaced. + pub(crate) fn insert_connection_options( + &self, + jti: Uuid, + connection_options: TargetConnectionOptions, + time_to_live: time::Duration, + ) -> bool { + let entry = ConnectionOptionsEntry { + connection_options, + expires_at: time::OffsetDateTime::now_utc() + time_to_live, + }; -impl ProvisioningEntries { - fn new() -> Self { - Self { - entries: HashMap::new(), - } + self.connection_options.lock().insert(jti, entry).is_some() } - fn insert( - &mut self, - token: String, - mapping: 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)?; + /// Assemble the provisioned view for a session. + /// + /// Returns `None` unless the credentials half (token and/or mapping) is present and live. + /// Folds in connection options when that half is also present and live. + pub(crate) fn get(&self, jti: Uuid) -> Option { + let now = time::OffsetDateTime::now_utc(); - let entry = ProvisioningEntry { - token, - mapping, - expires_at: time::OffsetDateTime::now_utc() + time_to_live, + let (token, mapping) = { + let entries = self.credentials.lock(); + let entry = entries.get(&jti)?; + if now >= entry.expires_at { + warn!(%jti, "Provisioned credentials expired before the connection arrived"); + return None; + } + (entry.token.clone(), entry.mapping.clone()) }; - let previous_entry = self.entries.insert(jti, Arc::new(entry)); + let connection_options = self.get_live_connection_options(jti, now); - Ok(previous_entry) + Some(Arc::new(ProvisioningEntry { + token, + mapping, + connection_options, + })) } - fn get(&self, token_id: Uuid) -> Option { - self.entries.get(&token_id).map(Arc::clone) + fn get_live_connection_options(&self, jti: Uuid, now: time::OffsetDateTime) -> Option { + let entries = self.connection_options.lock(); + let entry = entries.get(&jti)?; + if now >= entry.expires_at { + warn!(%jti, "Provisioned connection options expired before the connection arrived"); + return None; + } + Some(entry.connection_options.clone()) } } @@ -135,7 +188,7 @@ impl Task for CleanupTask { async fn cleanup_task(handle: ProvisioningStore, mut shutdown_signal: ShutdownSignal) { use tokio::time::{Duration, sleep}; - const TASK_INTERVAL: Duration = Duration::from_secs(60 * 15); // 15 minutes + const TASK_INTERVAL: Duration = Duration::from_secs(60 * 15); debug!("Task started"); @@ -148,9 +201,118 @@ async fn cleanup_task(handle: ProvisioningStore, mut shutdown_signal: ShutdownSi } let now = time::OffsetDateTime::now_utc(); - - handle.0.lock().entries.retain(|_, src| now < src.expires_at); + handle.credentials.lock().retain(|_, entry| now < entry.expires_at); + handle + .connection_options + .lock() + .retain(|_, entry| now < entry.expires_at); } debug!("Task terminated"); } + +#[cfg(test)] +mod tests { + use secrecy::SecretString; + use uuid::Uuid; + + use super::*; + use crate::credential::CleartextAppCredential; + + fn mapping() -> CleartextAppCredentialMapping { + CleartextAppCredentialMapping { + proxy: CleartextAppCredential::UsernamePassword { + username: "proxy".to_owned(), + password: SecretString::from("pwd"), + }, + target: CleartextAppCredential::UsernamePassword { + username: "target".to_owned(), + password: SecretString::from("pwd"), + }, + } + } + + fn association_token(jti: Uuid) -> String { + use base64::Engine as _; + 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(&serde_json::json!({ + "jti": jti, + "dst_hst": "target.example:3389" + })) + .expect("payload serializes"), + ); + let signature = engine.encode(b"signature"); + format!("{header}.{payload}.{signature}") + } + + fn options() -> TargetConnectionOptions { + serde_json::from_value(serde_json::json!({ "krb_kdc": "tcp://dc.example:88" })).expect("options") + } + + #[test] + fn get_returns_token_only_entry() { + let store = ProvisioningStore::new(); + let jti = Uuid::new_v4(); + store + .insert_credentials(association_token(jti), None, time::Duration::minutes(5)) + .expect("insert"); + let entry = store.get(jti).expect("live entry"); + assert!(entry.mapping.is_none()); + assert!(entry.connection_options.is_none()); + } + + #[test] + fn get_returns_live_credentials_without_options() { + let store = ProvisioningStore::new(); + let jti = Uuid::new_v4(); + store + .insert_credentials(association_token(jti), Some(mapping()), time::Duration::minutes(5)) + .expect("insert"); + let entry = store.get(jti).expect("live entry"); + assert!(entry.mapping.is_some()); + assert!(entry.connection_options.is_none()); + } + + #[test] + fn get_folds_in_live_connection_options() { + let store = ProvisioningStore::new(); + let jti = Uuid::new_v4(); + store + .insert_credentials(association_token(jti), Some(mapping()), time::Duration::minutes(5)) + .expect("insert credentials"); + assert!(!store.insert_connection_options(jti, options(), time::Duration::minutes(5))); + let entry = store.get(jti).expect("live entry"); + assert!(entry.connection_options.is_some()); + } + + #[test] + fn get_treats_expired_credentials_as_absent() { + let store = ProvisioningStore::new(); + let jti = Uuid::new_v4(); + store + .insert_credentials(association_token(jti), Some(mapping()), time::Duration::seconds(-1)) + .expect("insert"); + assert!(store.get(jti).is_none()); + } + + #[test] + fn credentials_and_options_replace_independently() { + let store = ProvisioningStore::new(); + let jti = Uuid::new_v4(); + assert!( + !store + .insert_credentials(association_token(jti), Some(mapping()), time::Duration::minutes(5)) + .expect("insert") + ); + assert!( + store + .insert_credentials(association_token(jti), Some(mapping()), time::Duration::minutes(5)) + .expect("replace") + ); + + assert!(!store.insert_connection_options(jti, options(), time::Duration::minutes(5))); + assert!(store.insert_connection_options(jti, options(), time::Duration::minutes(5))); + } +} diff --git a/devolutions-gateway/src/rdp_proxy.rs b/devolutions-gateway/src/rdp_proxy.rs index 856210fa1..9625b312d 100644 --- a/devolutions-gateway/src/rdp_proxy.rs +++ b/devolutions-gateway/src/rdp_proxy.rs @@ -394,12 +394,14 @@ pub(crate) fn credential_injection_kerberos_configs( }); } + let krb_kdc = credential_injection_kdc + .krb_kdc() + .context("kerberos credential injection requires the krb_kdc target connection option")?; + Ok(CredentialInjectionKerberosConfigs { server: Some(credential_injection_kdc.server_kerberos_config(client_addr)?), client: Some(ironrdp_connector::credssp::KerberosConfig { - // TODO: Provision the target KDC through connection options after the store is generalized. - // See https://github.com/Devolutions/devolutions-gateway/pull/1862#pullrequestreview-4774565673. - kdc_proxy_url: None, + kdc_proxy_url: Some(url::Url::try_from(krb_kdc).context("convert target kdc address to url")?), hostname: gateway_hostname.to_owned(), }), }) diff --git a/devolutions-gateway/src/target_addr.rs b/devolutions-gateway/src/target_addr.rs index 7e533a4fa..14b6ca064 100644 --- a/devolutions-gateway/src/target_addr.rs +++ b/devolutions-gateway/src/target_addr.rs @@ -216,6 +216,14 @@ impl TryFrom for TargetAddr { } } +impl TryFrom<&TargetAddr> for url::Url { + type Error = url::ParseError; + + fn try_from(target: &TargetAddr) -> Result { + url::Url::parse(target.as_str()) + } +} + impl fmt::Display for TargetAddr { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{}", self.serialization) diff --git a/devolutions-gateway/src/target_connection_options.rs b/devolutions-gateway/src/target_connection_options.rs new file mode 100644 index 000000000..d391b29b7 --- /dev/null +++ b/devolutions-gateway/src/target_connection_options.rs @@ -0,0 +1,108 @@ +use crate::target_addr::TargetAddr; + +/// How the Gateway's internal client should reach the target, provisioned alongside the credentials. +/// +/// The KDC address is fully validated at construction — supported scheme, a host, and a parseable +/// URL — so the rest of the code can trust `krb_kdc` without re-checking it or failing late when a +/// session starts. +#[derive(Debug, Clone, Deserialize)] +#[serde(try_from = "RawTargetConnectionOptions")] +pub(crate) struct TargetConnectionOptions { + krb_kdc: Option, +} + +impl TargetConnectionOptions { + pub(crate) fn new(krb_kdc: Option) -> Result { + if let Some(krb_kdc) = &krb_kdc { + if !is_supported_krb_kdc_scheme(krb_kdc.scheme()) { + return Err(InvalidKdcAddr::UnsupportedScheme(krb_kdc.scheme().to_owned())); + } + if krb_kdc.host().is_empty() { + return Err(InvalidKdcAddr::MissingHost(krb_kdc.as_str().to_owned())); + } + // The target-side CredSSP leg turns this into a URL. Reject a value that won't parse here, + // so provisioning fails fast instead of at session start. + url::Url::try_from(krb_kdc).map_err(|_| InvalidKdcAddr::NotAUrl(krb_kdc.as_str().to_owned()))?; + } + Ok(Self { krb_kdc }) + } + + pub(crate) fn krb_kdc(&self) -> Option<&TargetAddr> { + self.krb_kdc.as_ref() + } +} + +pub(crate) fn is_supported_krb_kdc_scheme(scheme: &str) -> bool { + matches!(scheme, "tcp" | "udp") +} + +#[derive(Debug, thiserror::Error)] +pub(crate) enum InvalidKdcAddr { + #[error("unsupported kdc protocol: {0}")] + UnsupportedScheme(String), + #[error("kdc address is missing a host: {0}")] + MissingHost(String), + #[error("kdc address is not a valid url: {0}")] + NotAUrl(String), +} + +#[derive(Deserialize)] +struct RawTargetConnectionOptions { + #[serde(default)] + krb_kdc: Option, +} + +impl TryFrom for TargetConnectionOptions { + type Error = InvalidKdcAddr; + + fn try_from(raw: RawTargetConnectionOptions) -> Result { + Self::new(raw.krb_kdc) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn accepts_supported_kdc_protocols() { + for krb_kdc in ["tcp://dc.example.com:88", "udp://dc.example.com:88"] { + let options: TargetConnectionOptions = serde_json::from_value(serde_json::json!({ + "krb_kdc": krb_kdc, + })) + .expect("supported KDC protocol should deserialize"); + + assert_eq!( + options.krb_kdc().expect("KDC address should be present").as_str(), + krb_kdc + ); + } + } + + #[test] + fn rejects_unsupported_kdc_protocol() { + let error = serde_json::from_value::(serde_json::json!({ + "krb_kdc": "https://dc.example.com:443", + })) + .expect_err("unsupported KDC protocol should be rejected"); + + assert!(error.to_string().contains("unsupported kdc protocol: https")); + } + + #[test] + fn rejects_kdc_without_a_host() { + assert!( + serde_json::from_value::(serde_json::json!({ + "krb_kdc": "tcp://:88", + })) + .is_err(), + "a host-less KDC address must be rejected at provisioning time" + ); + } + + #[test] + fn new_rejects_unsupported_scheme_for_in_crate_callers() { + let krb_kdc = TargetAddr::parse("https://dc.example.com:443", Some(443)).expect("addr parses"); + assert!(TargetConnectionOptions::new(Some(krb_kdc)).is_err()); + } +} diff --git a/devolutions-gateway/tests/preflight.rs b/devolutions-gateway/tests/preflight.rs index 8246696ac..1d0de8866 100644 --- a/devolutions-gateway/tests/preflight.rs +++ b/devolutions-gateway/tests/preflight.rs @@ -81,9 +81,11 @@ 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/`); preflight rejects tokens without it. - let token = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiI1ZTNlODMzZi04NGM3LTQ1NDEtYjY3Ni1hY2MzMjk5ZTM5YjgiLCJkc3RfaHN0IjoidGFyZ2V0LmV4YW1wbGU6MzM4OSJ9.1qECGlrW7y9HWFArc6GPHLGTOY7PhAvzKJ5XMRBg4k4"; + let jti = Uuid::new_v4(); + let token = unsigned_jws(json!({ + "jti": jti, + "dst_hst": "target.example:3389" + }))?; let op_id = Uuid::new_v4(); @@ -97,12 +99,12 @@ async fn test_provision_credentials_success() -> anyhow::Result<()> { }]); let request = preflight_request(op)?; - - let response = app.oneshot(request).await.unwrap(); + let response = app.oneshot(request).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]["operation_id"], op_id.to_string()); assert_eq!(body[0]["kind"], "ack", "{:?}", body[0]); @@ -114,12 +116,16 @@ async fn test_provision_credentials_success() -> anyhow::Result<()> { async fn test_provision_credentials_success_when_unstable_disabled() -> anyhow::Result<()> { let _guard = init_logger(); + // `provision-credentials` is protocol-neutral: NTLM credential injection relies on this + // path even when the unstable feature flag is off. let config = CONFIG.replace("\"enable_unstable\": true", "\"enable_unstable\": false"); let (app, _state, _handles) = make_router_with_config(&config)?; - // `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 jti = Uuid::new_v4(); + let token = unsigned_jws(json!({ + "jti": jti, + "dst_hst": "target.example:3389" + }))?; let op_id = Uuid::new_v4(); @@ -132,11 +138,12 @@ async fn test_provision_credentials_success_when_unstable_disabled() -> anyhow:: "time_to_live": 15 }]); - let response = app.oneshot(preflight_request(op)?).await.unwrap(); + 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]["operation_id"], op_id.to_string()); assert_eq!(body[0]["kind"], "ack", "{:?}", body[0]); @@ -236,7 +243,11 @@ async fn test_provision_token_overwrite_alert() -> anyhow::Result<()> { let (app, _state, _handles) = make_router()?; - let token = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiI1ZTNlODMzZi04NGM3LTQ1NDEtYjY3Ni1hY2MzMjk5ZTM5YjgifQ.1qECGlrW7y9HWFArc6GPHLGTOY7PhAvzKJ5XMRBg4k4"; + // Same JTI twice: second provision-token replaces the stored token-only entry (master behavior). + let token = unsigned_jws(json!({ + "jti": "5e3e833f-84c7-4541-b676-acc3299e39b8", + "dst_hst": "target.example:3389" + }))?; let op_id1 = Uuid::new_v4(); let op_id2 = Uuid::new_v4(); @@ -267,6 +278,78 @@ async fn test_provision_token_overwrite_alert() -> anyhow::Result<()> { Ok(()) } +#[tokio::test] +async fn test_provision_connection_options_success() -> anyhow::Result<()> { + let _guard = init_logger(); + + let (app, _state, _handles) = make_router()?; + + let jti = Uuid::new_v4(); + let token = unsigned_jws(json!({ + "jti": jti, + "dst_hst": "target.example:3389" + }))?; + + let op_id = Uuid::new_v4(); + let op = json!([{ + "id": op_id, + "kind": "provision-connection-options", + "token": token, + "connection_options": { "krb_kdc": "tcp://dc.example:88" }, + "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"], "ack", "{:?}", body[0]); + + Ok(()) +} + +#[tokio::test] +async fn test_provision_credentials_and_connection_options_fold() -> anyhow::Result<()> { + let _guard = init_logger(); + + let (app, _state, _handles) = make_router()?; + + let jti = Uuid::new_v4(); + let token = unsigned_jws(json!({ + "jti": jti, + "dst_hst": "target.example:3389" + }))?; + + let ops = json!([ + { + "id": Uuid::new_v4(), + "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 + }, + { + "id": Uuid::new_v4(), + "kind": "provision-connection-options", + "token": token, + "connection_options": { "krb_kdc": "tcp://dc.example:88" }, + "time_to_live": 15 + } + ]); + + let response = app.oneshot(preflight_request(ops)?).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(), 2); + assert_eq!(body[0]["kind"], "ack", "{:?}", body[0]); + assert_eq!(body[1]["kind"], "ack", "{:?}", body[1]); + + Ok(()) +} + #[tokio::test] async fn test_provision_invalid_params() -> anyhow::Result<()> { let _guard = init_logger();