From 49035073c2ccaa0c66e79365458ade0849c48aab Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Fri, 24 Jul 2026 13:54:09 -0400 Subject: [PATCH 01/11] refactor(dgw): move credential-injection KDC into a generic provisioning store Extract a protocol-neutral `TokenKeyedStore` that owns JTI keying, TTL, and cleanup and knows nothing about its value type. The target KDC address moves off the credential type into a plaintext `TargetConnectionOptions`, provisioned via a new `connection_options` block on `provision-credentials`. Encryption stays at the credential boundary, so the store never touches the master key. --- devolutions-gateway/openapi/doc/index.adoc | 29 +++ .../dotnet-client/.openapi-generator/FILES | 2 + .../openapi/dotnet-client/README.md | 1 + .../dotnet-client/docs/PreflightOperation.md | 1 + .../docs/TargetConnectionOptions.md | 9 + .../Model/PreflightOperation.cs | 12 +- .../Model/TargetConnectionOptions.cs | 90 +++++++++ devolutions-gateway/openapi/gateway-api.yaml | 14 ++ .../.openapi-generator/FILES | 1 + .../openapi/ts-angular-client/model/models.ts | 1 + .../model/preflightOperation.ts | 5 + .../model/targetConnectionOptions.ts | 20 ++ devolutions-gateway/src/api/preflight.rs | 17 +- devolutions-gateway/src/credential/crypto.rs | 2 +- devolutions-gateway/src/credential/mod.rs | 157 +-------------- .../src/credential_injection_kdc.rs | 180 +++++++++++------- devolutions-gateway/src/generic_client.rs | 4 +- devolutions-gateway/src/kdc_connector.rs | 2 +- devolutions-gateway/src/lib.rs | 2 + devolutions-gateway/src/openapi.rs | 13 ++ devolutions-gateway/src/rd_clean_path.rs | 4 +- devolutions-gateway/src/rdp_proxy.rs | 46 ++--- devolutions-gateway/src/service.rs | 4 +- devolutions-gateway/src/target_addr.rs | 8 + .../src/target_connection_options.rs | 61 ++++++ devolutions-gateway/src/token_keyed_store.rs | 167 ++++++++++++++++ 26 files changed, 584 insertions(+), 268 deletions(-) create mode 100644 devolutions-gateway/openapi/dotnet-client/docs/TargetConnectionOptions.md create mode 100644 devolutions-gateway/openapi/dotnet-client/src/Devolutions.Gateway.Client/Model/TargetConnectionOptions.cs create mode 100644 devolutions-gateway/openapi/ts-angular-client/model/targetConnectionOptions.ts create mode 100644 devolutions-gateway/src/target_connection_options.rs create mode 100644 devolutions-gateway/src/token_keyed_store.rs diff --git a/devolutions-gateway/openapi/doc/index.adoc b/devolutions-gateway/openapi/doc/index.adoc index 405c6a6bf..7dd3f60bb 100644 --- a/devolutions-gateway/openapi/doc/index.adoc +++ b/devolutions-gateway/openapi/doc/index.adoc @@ -3179,6 +3179,13 @@ Service configuration diagnostic |=== | Field Name| Required| Nullable | Type| Description | Format +| connection_options +| +| X +| <> +| Options used by the Gateway when connecting to the target. Optional for "provision-credentials" kind. +| + | host_to_resolve | | X @@ -3646,6 +3653,28 @@ Subscriber configuration +[#TargetConnectionOptions] +=== _TargetConnectionOptions_ + + + + +[.fields-TargetConnectionOptions] +[cols="2,1,1,2,4,1"] +|=== +| Field Name| Required| Nullable | Type| Description | Format + +| krb_kdc +| +| X +| String +| Kerberos KDC address for the target-side CredSSP connection. Supported schemes are `tcp` and `udp`. +| + +|=== + + + [#TrafficEventResponse] === _TrafficEventResponse_ diff --git a/devolutions-gateway/openapi/dotnet-client/.openapi-generator/FILES b/devolutions-gateway/openapi/dotnet-client/.openapi-generator/FILES index 16fd9a133..edaa83c0d 100644 --- a/devolutions-gateway/openapi/dotnet-client/.openapi-generator/FILES +++ b/devolutions-gateway/openapi/dotnet-client/.openapi-generator/FILES @@ -51,6 +51,7 @@ docs/SessionsApi.md docs/SetConfigResponse.md docs/SubProvisionerKey.md docs/Subscriber.md +docs/TargetConnectionOptions.md docs/TrafficApi.md docs/TrafficEventResponse.md docs/TransportProtocolResponse.md @@ -128,5 +129,6 @@ src/Devolutions.Gateway.Client/Model/SessionTokenSignRequest.cs src/Devolutions.Gateway.Client/Model/SetConfigResponse.cs src/Devolutions.Gateway.Client/Model/SubProvisionerKey.cs src/Devolutions.Gateway.Client/Model/Subscriber.cs +src/Devolutions.Gateway.Client/Model/TargetConnectionOptions.cs src/Devolutions.Gateway.Client/Model/TrafficEventResponse.cs src/Devolutions.Gateway.Client/Model/TransportProtocolResponse.cs diff --git a/devolutions-gateway/openapi/dotnet-client/README.md b/devolutions-gateway/openapi/dotnet-client/README.md index 8f17bd5a4..f2988f1db 100644 --- a/devolutions-gateway/openapi/dotnet-client/README.md +++ b/devolutions-gateway/openapi/dotnet-client/README.md @@ -209,6 +209,7 @@ Class | Method | HTTP request | Description - [Model.SetConfigResponse](docs/SetConfigResponse.md) - [Model.SubProvisionerKey](docs/SubProvisionerKey.md) - [Model.Subscriber](docs/Subscriber.md) + - [Model.TargetConnectionOptions](docs/TargetConnectionOptions.md) - [Model.TrafficEventResponse](docs/TrafficEventResponse.md) - [Model.TransportProtocolResponse](docs/TransportProtocolResponse.md) diff --git a/devolutions-gateway/openapi/dotnet-client/docs/PreflightOperation.md b/devolutions-gateway/openapi/dotnet-client/docs/PreflightOperation.md index 926754235..36eba4b4f 100644 --- a/devolutions-gateway/openapi/dotnet-client/docs/PreflightOperation.md +++ b/devolutions-gateway/openapi/dotnet-client/docs/PreflightOperation.md @@ -4,6 +4,7 @@ Name | Type | Description | Notes ------------ | ------------- | ------------- | ------------- +**ConnectionOptions** | [**TargetConnectionOptions**](TargetConnectionOptions.md) | Options used by the Gateway when connecting to the target. Optional for \"provision-credentials\" kind. | [optional] **HostToResolve** | **string** | The hostname to perform DNS resolution on. Required for \"resolve-host\" kind. | [optional] **Id** | **Guid** | Unique ID identifying the preflight operation. | **Kind** | **PreflightOperationKind** | | diff --git a/devolutions-gateway/openapi/dotnet-client/docs/TargetConnectionOptions.md b/devolutions-gateway/openapi/dotnet-client/docs/TargetConnectionOptions.md new file mode 100644 index 000000000..c4efc0917 --- /dev/null +++ b/devolutions-gateway/openapi/dotnet-client/docs/TargetConnectionOptions.md @@ -0,0 +1,9 @@ +# Devolutions.Gateway.Client.Model.TargetConnectionOptions + +## Properties + +Name | Type | Description | Notes +------------ | ------------- | ------------- | ------------- +**KrbKdc** | **string** | Kerberos KDC address for the target-side CredSSP connection. Supported schemes are `tcp` and `udp`. | [optional] + +[[Back to Model list]](../README.md#documentation-for-models) [[Back to API list]](../README.md#documentation-for-api-endpoints) [[Back to README]](../README.md) diff --git a/devolutions-gateway/openapi/dotnet-client/src/Devolutions.Gateway.Client/Model/PreflightOperation.cs b/devolutions-gateway/openapi/dotnet-client/src/Devolutions.Gateway.Client/Model/PreflightOperation.cs index f2d1e9b56..dca0ba001 100644 --- a/devolutions-gateway/openapi/dotnet-client/src/Devolutions.Gateway.Client/Model/PreflightOperation.cs +++ b/devolutions-gateway/openapi/dotnet-client/src/Devolutions.Gateway.Client/Model/PreflightOperation.cs @@ -47,6 +47,7 @@ protected PreflightOperation() { } /// /// Initializes a new instance of the class. /// + /// Options used by the Gateway when connecting to the target. Optional for \"provision-credentials\" kind.. /// The hostname to perform DNS resolution on. Required for \"resolve-host\" kind.. /// Unique ID identifying the preflight operation. (required). /// kind (required). @@ -54,10 +55,11 @@ protected PreflightOperation() { } /// targetCredential. /// Minimum persistance duration in seconds for the data provisioned via this operation. Optional parameter for \"provision-token\" and \"provision-credentials\" kinds.. /// The token to be stored on the proxy-side. Required for \"provision-token\" and \"provision-credentials\" kinds.. - public PreflightOperation(string hostToResolve = default(string), Guid id = default(Guid), PreflightOperationKind kind = default(PreflightOperationKind), AppCredential proxyCredential = default(AppCredential), AppCredential targetCredential = default(AppCredential), int? timeToLive = default(int?), string token = default(string)) + public PreflightOperation(TargetConnectionOptions connectionOptions = default(TargetConnectionOptions), string hostToResolve = default(string), Guid id = default(Guid), PreflightOperationKind kind = default(PreflightOperationKind), AppCredential proxyCredential = default(AppCredential), AppCredential targetCredential = default(AppCredential), int? timeToLive = default(int?), string token = default(string)) { this.Id = id; this.Kind = kind; + this.ConnectionOptions = connectionOptions; this.HostToResolve = hostToResolve; this.ProxyCredential = proxyCredential; this.TargetCredential = targetCredential; @@ -65,6 +67,13 @@ protected PreflightOperation() { } this.Token = token; } + /// + /// Options used by the Gateway when connecting to the target. Optional for \"provision-credentials\" kind. + /// + /// Options used by the Gateway when connecting to the target. Optional for \"provision-credentials\" kind. + [DataMember(Name = "connection_options", EmitDefaultValue = true)] + public TargetConnectionOptions ConnectionOptions { get; set; } + /// /// The hostname to perform DNS resolution on. Required for \"resolve-host\" kind. /// @@ -113,6 +122,7 @@ public override string ToString() { StringBuilder sb = new StringBuilder(); sb.Append("class PreflightOperation {\n"); + sb.Append(" ConnectionOptions: ").Append(ConnectionOptions).Append("\n"); sb.Append(" HostToResolve: ").Append(HostToResolve).Append("\n"); sb.Append(" Id: ").Append(Id).Append("\n"); sb.Append(" Kind: ").Append(Kind).Append("\n"); diff --git a/devolutions-gateway/openapi/dotnet-client/src/Devolutions.Gateway.Client/Model/TargetConnectionOptions.cs b/devolutions-gateway/openapi/dotnet-client/src/Devolutions.Gateway.Client/Model/TargetConnectionOptions.cs new file mode 100644 index 000000000..efc1389f6 --- /dev/null +++ b/devolutions-gateway/openapi/dotnet-client/src/Devolutions.Gateway.Client/Model/TargetConnectionOptions.cs @@ -0,0 +1,90 @@ +/* + * devolutions-gateway + * + * Protocol-aware fine-grained relay server + * + * The version of the OpenAPI document: 2025.3.2 + * Contact: infos@devolutions.net + * Generated by: https://github.com/openapitools/openapi-generator.git + */ + + +using System; +using System.Collections; +using System.Collections.Generic; +using System.Collections.ObjectModel; +using System.Linq; +using System.IO; +using System.Runtime.Serialization; +using System.Text; +using System.Text.RegularExpressions; +using Newtonsoft.Json; +using Newtonsoft.Json.Converters; +using Newtonsoft.Json.Linq; +using System.ComponentModel.DataAnnotations; +using FileParameter = Devolutions.Gateway.Client.Client.FileParameter; +using OpenAPIDateConverter = Devolutions.Gateway.Client.Client.OpenAPIDateConverter; + +namespace Devolutions.Gateway.Client.Model +{ + /// + /// TargetConnectionOptions + /// + [DataContract(Name = "TargetConnectionOptions")] + public partial class TargetConnectionOptions : IValidatableObject + { + /// + /// Initializes a new instance of the class. + /// + [JsonConstructorAttribute] + protected TargetConnectionOptions() { } + /// + /// Initializes a new instance of the class. + /// + /// Kerberos KDC address for the target-side CredSSP connection. Supported schemes are `tcp` and `udp`.. + public TargetConnectionOptions(string krbKdc = default(string)) + { + this.KrbKdc = krbKdc; + } + + /// + /// Kerberos KDC address for the target-side CredSSP connection. Supported schemes are `tcp` and `udp`. + /// + /// Kerberos KDC address for the target-side CredSSP connection. Supported schemes are `tcp` and `udp`. + [DataMember(Name = "krb_kdc", EmitDefaultValue = true)] + public string KrbKdc { get; set; } + + /// + /// Returns the string presentation of the object + /// + /// String presentation of the object + public override string ToString() + { + StringBuilder sb = new StringBuilder(); + sb.Append("class TargetConnectionOptions {\n"); + sb.Append(" KrbKdc: ").Append(KrbKdc).Append("\n"); + sb.Append("}\n"); + return sb.ToString(); + } + + /// + /// Returns the JSON string presentation of the object + /// + /// JSON string presentation of the object + public virtual string ToJson() + { + return Newtonsoft.Json.JsonConvert.SerializeObject(this, Newtonsoft.Json.Formatting.Indented); + } + + /// + /// To validate all properties of the instance + /// + /// Validation context + /// Validation Result + IEnumerable IValidatableObject.Validate(ValidationContext validationContext) + { + yield break; + } + } + +} \ No newline at end of file diff --git a/devolutions-gateway/openapi/gateway-api.yaml b/devolutions-gateway/openapi/gateway-api.yaml index 8fabd66fe..bdb34991f 100644 --- a/devolutions-gateway/openapi/gateway-api.yaml +++ b/devolutions-gateway/openapi/gateway-api.yaml @@ -1772,6 +1772,10 @@ components: - id - kind properties: + connection_options: + allOf: + - $ref: '#/components/schemas/TargetConnectionOptions' + nullable: true host_to_resolve: type: string description: |- @@ -2105,6 +2109,16 @@ components: Url: type: string description: HTTP URL where notification messages are to be sent + TargetConnectionOptions: + type: object + properties: + krb_kdc: + type: string + description: |- + Kerberos KDC address for the target-side CredSSP connection. + + Supported schemes are `tcp` and `udp`. + nullable: true TrafficEventResponse: type: object required: diff --git a/devolutions-gateway/openapi/ts-angular-client/.openapi-generator/FILES b/devolutions-gateway/openapi/ts-angular-client/.openapi-generator/FILES index 2f0c2d88c..d522859fc 100644 --- a/devolutions-gateway/openapi/ts-angular-client/.openapi-generator/FILES +++ b/devolutions-gateway/openapi/ts-angular-client/.openapi-generator/FILES @@ -59,6 +59,7 @@ model/sessionTokenSignRequest.ts model/setConfigResponse.ts model/subProvisionerKey.ts model/subscriber.ts +model/targetConnectionOptions.ts model/trafficEventResponse.ts model/transportProtocolResponse.ts ng-package.json diff --git a/devolutions-gateway/openapi/ts-angular-client/model/models.ts b/devolutions-gateway/openapi/ts-angular-client/model/models.ts index 652b6b61f..186064f4e 100644 --- a/devolutions-gateway/openapi/ts-angular-client/model/models.ts +++ b/devolutions-gateway/openapi/ts-angular-client/model/models.ts @@ -38,5 +38,6 @@ export * from './sessionTokenSignRequest'; export * from './setConfigResponse'; export * from './subProvisionerKey'; export * from './subscriber'; +export * from './targetConnectionOptions'; export * from './trafficEventResponse'; export * from './transportProtocolResponse'; diff --git a/devolutions-gateway/openapi/ts-angular-client/model/preflightOperation.ts b/devolutions-gateway/openapi/ts-angular-client/model/preflightOperation.ts index 440e61c4a..8f387af8c 100644 --- a/devolutions-gateway/openapi/ts-angular-client/model/preflightOperation.ts +++ b/devolutions-gateway/openapi/ts-angular-client/model/preflightOperation.ts @@ -9,9 +9,14 @@ */ import { AppCredential } from './appCredential'; import { PreflightOperationKind } from './preflightOperationKind'; +import { TargetConnectionOptions } from './targetConnectionOptions'; export interface PreflightOperation { + /** + * Options used by the Gateway when connecting to the target. Optional for "provision-credentials" kind. + */ + connection_options?: TargetConnectionOptions | null; /** * The hostname to perform DNS resolution on. Required for \"resolve-host\" kind. */ diff --git a/devolutions-gateway/openapi/ts-angular-client/model/targetConnectionOptions.ts b/devolutions-gateway/openapi/ts-angular-client/model/targetConnectionOptions.ts new file mode 100644 index 000000000..1dee1b7eb --- /dev/null +++ b/devolutions-gateway/openapi/ts-angular-client/model/targetConnectionOptions.ts @@ -0,0 +1,20 @@ +/** + * devolutions-gateway + * + * Contact: infos@devolutions.net + * + * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). + * https://openapi-generator.tech + * Do not edit the class manually. + */ + + +export interface TargetConnectionOptions { + /** + * Kerberos KDC address for the target-side CredSSP connection. Supported schemes are `tcp` and `udp`. + */ + krb_kdc?: string | null; +} +export namespace TargetConnectionOptions { +} + diff --git a/devolutions-gateway/src/api/preflight.rs b/devolutions-gateway/src/api/preflight.rs index 95216cc25..1c56e650c 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::InsertError; -use crate::credential_injection_kdc::CredentialService; +use crate::credential_injection_kdc::{CredentialService, InsertError}; use crate::extract::PreflightScope; use crate::http::HttpError; use crate::session::SessionMessageSender; @@ -47,6 +46,7 @@ struct ProvisionCredentialsParams { token: String, #[serde(flatten)] mapping: crate::credential::CleartextAppCredentialMapping, + connection_options: Option, time_to_live: Option, } @@ -311,17 +311,18 @@ 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 (token, time_to_live, mapping, connection_options) = if operation.kind.as_str() == OP_PROVISION_TOKEN { let ProvisionTokenParams { token, time_to_live } = from_params(operation.params).map_err(PreflightError::invalid_params)?; - (token, time_to_live, None) + (token, time_to_live, None, None) } else { let ProvisionCredentialsParams { token, mapping, + connection_options, time_to_live, } = from_params(operation.params).map_err(PreflightError::invalid_params)?; - (token, time_to_live, Some(mapping)) + (token, time_to_live, Some(mapping), connection_options) }; let time_to_live = time_to_live @@ -340,7 +341,7 @@ async fn handle_operation( // 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. + // never reaches the provisioning store with malformed input. if is_provision_credentials { crate::token::validate_credential_injection_association_token(&token) .inspect_err(|error| { @@ -359,7 +360,7 @@ async fn handle_operation( } let previous_entry = credentials - .insert(token, mapping, time_to_live) + .insert(token, mapping, connection_options, time_to_live) .inspect_err(|error| warn!(%operation.id, error = format!("{error:#}"), "Failed to insert credentials")) .map_err(|error| match error { InsertError::InvalidToken(error) => { @@ -378,7 +379,7 @@ async fn handle_operation( operation_id: operation.id, kind: PreflightOutputKind::Alert { status: PreflightAlertStatus::Info, - message: "an existing credential entry was replaced".to_owned(), + message: "an existing provisioning entry was replaced".to_owned(), }, }); } diff --git a/devolutions-gateway/src/credential/crypto.rs b/devolutions-gateway/src/credential/crypto.rs index 2f1a84d70..380d13ff6 100644 --- a/devolutions-gateway/src/credential/crypto.rs +++ b/devolutions-gateway/src/credential/crypto.rs @@ -1,6 +1,6 @@ //! In-memory credential encryption using ChaCha20-Poly1305. //! -//! This module provides encryption-at-rest for passwords stored in the credential store. +//! This module provides encryption-at-rest for passwords managed by the credential service. //! A randomly generated 256-bit master key is held in a [`ProtectedBytes<32>`] allocation backed by `secure-memory`, which applies //! the best available OS hardening (mlock, guard pages, core-dump exclusion) and always zeroizes on drop. //! diff --git a/devolutions-gateway/src/credential/mod.rs b/devolutions-gateway/src/credential/mod.rs index 166be9412..d83cb297e 100644 --- a/devolutions-gateway/src/credential/mod.rs +++ b/devolutions-gateway/src/credential/mod.rs @@ -3,41 +3,10 @@ mod crypto; #[rustfmt::skip] 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 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 { @@ -70,8 +39,7 @@ pub struct AppCredentialMapping { /// Cleartext credential received from the API, used for deserialization only. /// -/// Passwords are encrypted and stored as [`AppCredential`] inside the credential store. -/// This type is never stored directly — hand it to [`CredentialStoreHandle::insert`]. +/// Passwords are encrypted and stored as [`AppCredential`] by the credential service. #[derive(Debug, Deserialize)] #[serde(tag = "kind")] pub enum CleartextAppCredential { @@ -97,8 +65,6 @@ impl CleartextAppCredential { } /// Cleartext credential mapping received from the API, used for deserialization only. -/// -/// Passwords are encrypted on write. Hand this directly to [`CredentialStoreHandle::insert`]. #[derive(Debug, Deserialize)] pub struct CleartextAppCredentialMapping { #[serde(rename = "proxy_credential")] @@ -108,129 +74,10 @@ pub struct CleartextAppCredentialMapping { } impl CleartextAppCredentialMapping { - fn encrypt(self) -> anyhow::Result { + pub(crate) fn encrypt(self) -> anyhow::Result { Ok(AppCredentialMapping { proxy: self.proxy.encrypt()?, target: self.target.encrypt()?, }) } } - -#[derive(Debug, Clone)] -pub struct CredentialStoreHandle(Arc>); - -impl Default for CredentialStoreHandle { - fn default() -> Self { - Self::new() - } -} - -impl CredentialStoreHandle { - pub fn new() -> Self { - Self(Arc::new(Mutex::new(CredentialStore::new()))) - } - - pub fn insert( - &self, - token: String, - mapping: Option, - 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) - } - - pub fn get(&self, token_id: Uuid) -> Option { - self.0.lock().get(token_id) - } -} - -#[derive(Debug)] -struct CredentialStore { - entries: HashMap, -} - -#[derive(Debug)] -pub struct CredentialEntry { - pub token: String, - pub mapping: Option, - pub expires_at: time::OffsetDateTime, -} - -pub type ArcCredentialEntry = Arc; - -impl CredentialStore { - fn new() -> Self { - Self { - entries: HashMap::new(), - } - } - - 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)?; - - let entry = CredentialEntry { - token, - mapping, - expires_at: time::OffsetDateTime::now_utc() + time_to_live, - }; - - let previous_entry = self.entries.insert(jti, Arc::new(entry)); - - Ok(previous_entry) - } - - fn get(&self, token_id: Uuid) -> Option { - self.entries.get(&token_id).map(Arc::clone) - } -} - -pub struct CleanupTask { - pub handle: CredentialStoreHandle, -} - -#[async_trait] -impl Task for CleanupTask { - type Output = anyhow::Result<()>; - - const NAME: &'static str = "credential store 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: CredentialStoreHandle, 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().entries.retain(|_, src| now < src.expires_at); - } - - debug!("Task terminated"); -} diff --git a/devolutions-gateway/src/credential_injection_kdc.rs b/devolutions-gateway/src/credential_injection_kdc.rs index b789c2b74..714296039 100644 --- a/devolutions-gateway/src/credential_injection_kdc.rs +++ b/devolutions-gateway/src/credential_injection_kdc.rs @@ -26,7 +26,9 @@ use thiserror::Error; use url::Url; use uuid::Uuid; -use crate::credential::{AppCredential, AppCredentialMapping, ArcCredentialEntry, CredentialStoreHandle}; +use crate::credential::{AppCredential, AppCredentialMapping, CleartextAppCredentialMapping}; +use crate::target_connection_options::TargetConnectionOptions; +use crate::token_keyed_store::{ArcTokenKeyedEntry, TokenKeyedStore}; // 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 @@ -40,6 +42,7 @@ pub(crate) struct CredentialInjectionKdc { jti: Uuid, raw_token: String, credential_mapping: AppCredentialMapping, + connection_options: Option, target_hostname: String, session: Arc, // The KDC crate models users with plaintext passwords, so this object owns those secrets @@ -124,25 +127,27 @@ impl fmt::Debug for CredentialInjectionKdc { impl CredentialInjectionKdc { fn from_parts( jti: Uuid, - credential_entry: ArcCredentialEntry, + provisioning_entry: ArcProvisioningEntry, target_hostname: String, session: Arc, ) -> anyhow::Result { - let mapping = credential_entry + let mapping = provisioning_entry + .value .mapping .as_ref() - .context("credential entry has no credential-injection mapping")?; + .context("provisioning entry has no credential-injection mapping")?; anyhow::ensure!( jti == session.jti, - "credential entry JTI does not match credential-injection KDC session JTI", + "provisioning entry JTI does not match credential-injection KDC session JTI", ); let kdc_config = build_kdc_config(&session, &mapping.proxy)?; Ok(Self { jti, - raw_token: credential_entry.token.clone(), + raw_token: provisioning_entry.token.clone(), credential_mapping: mapping.clone(), + connection_options: provisioning_entry.value.connection_options.clone(), target_hostname, session, kdc_config, @@ -165,6 +170,10 @@ impl CredentialInjectionKdc { &self.credential_mapping.target } + pub(crate) fn krb_kdc(&self) -> Option<&crate::target_addr::TargetAddr> { + self.connection_options.as_ref()?.krb_kdc.as_ref() + } + /// Selects the CredSSP acceptor backend Gateway should present to the RDP client. /// /// The acceptor side must mirror the target-side auth package. @@ -435,18 +444,26 @@ fn random_32_bytes() -> Vec { bytes } -/// One-stop service for credential storage and credential-injection KDC state. -/// -/// 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)] +pub(crate) struct ProvisioningEntry { + pub mapping: Option, + pub connection_options: Option, +} + +pub(crate) type ArcProvisioningEntry = ArcTokenKeyedEntry; + +#[derive(Debug, thiserror::Error)] +pub enum InsertError { + #[error(transparent)] + InvalidToken(#[from] crate::token_keyed_store::TokenKeyError), + #[error("credential encryption failed")] + Internal(#[source] anyhow::Error), +} + +/// Coordinates provisioned credentials and credential-injection KDC state. #[derive(Debug, Clone)] pub struct CredentialService { - credentials: CredentialStoreHandle, + provisioning: TokenKeyedStore, sessions: Arc>>>, } @@ -459,46 +476,51 @@ impl Default for CredentialService { impl CredentialService { pub fn new() -> Self { Self { - credentials: CredentialStoreHandle::new(), + provisioning: TokenKeyedStore::new(), sessions: Arc::new(Mutex::new(HashMap::new())), } } - /// Insert (or replace) a credential entry keyed by the token's JTI. + /// Insert (or replace) a provisioning 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 + /// [`TokenKeyedStore::insert`] reports no replacement, because the prior entry may have + /// already been evicted by its cleanup task while the 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( + pub(crate) fn insert( &self, token: String, - mapping: Option, + mapping: Option, + connection_options: 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) + ) -> Result, InsertError> { + let mapping = mapping + .map(CleartextAppCredentialMapping::encrypt) + .transpose() + .map_err(InsertError::Internal)?; + let result = self.provisioning.insert( + token, + ProvisioningEntry { + mapping, + connection_options, + }, + time_to_live, + )?; + self.sessions.lock().remove(&result.jti); + Ok(result.previous) } - /// Look up a credential entry by its association-token JTI. - pub fn get(&self, jti: Uuid) -> Option { - self.credentials.get(jti) + /// Look up a provisioning entry by its association-token JTI. + pub(crate) fn get(&self, jti: Uuid) -> Option { + self.provisioning.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 + pub fn provisioning_cleanup_task(&self) -> impl Task> + 'static + use<> { + crate::token_keyed_store::CleanupTask { + store: self.provisioning.clone(), + } } /// Resolve the credential-injection KDC bound to the given association-token JTI. @@ -507,26 +529,26 @@ impl CredentialService { /// 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(|| { + let provisioning_entry = self.provisioning.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 + // `TokenKeyedStore::get` does not enforce expiry — entries are evicted asynchronously + // by the provisioning 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 { + if time::OffsetDateTime::now_utc() >= provisioning_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(|| { + let mapping = provisioning_entry.value.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) + let target_hostname = crate::token::extract_credential_injection_target_hostname(&provisioning_entry.token) .map_err(|source| { warn!( %jti, @@ -548,7 +570,7 @@ impl CredentialService { Arc::clone(session) }; - CredentialInjectionKdc::from_parts(jti, credential_entry, target_hostname, session) + CredentialInjectionKdc::from_parts(jti, provisioning_entry, target_hostname, session) .map_err(|source| CredentialInjectionKdcResolveError::BuildKdcConfig { jti, source }) } @@ -558,7 +580,7 @@ impl CredentialService { sessions .keys() .copied() - .filter(|jti| self.credentials.get(*jti).is_none()) + .filter(|jti| self.provisioning.get(*jti).is_none()) .collect() }; @@ -648,20 +670,21 @@ mod tests { })) } - fn dummy_entry_with_target_username(jti: Uuid, target_username: &str) -> ArcCredentialEntry { - let store = CredentialStoreHandle::new(); - store + fn dummy_entry_with_target_username(jti: Uuid, target_username: &str) -> ArcProvisioningEntry { + let service = CredentialService::new(); + service .insert( association_token(jti), Some(cleartext_mapping_with_target_username(target_username)), + None, time::Duration::minutes(5), ) .expect("credential entry inserts"); - store.get(jti).expect("credential entry is indexed by JTI") + service.get(jti).expect("provisioning entry is indexed by JTI") } - fn dummy_entry(jti: Uuid) -> ArcCredentialEntry { + fn dummy_entry(jti: Uuid) -> ArcProvisioningEntry { dummy_entry_with_target_username(jti, "target") } @@ -706,13 +729,14 @@ mod tests { let service = CredentialService::new(); let jti = Uuid::new_v4(); - // Negative TTL: entry is born already expired. `CredentialStoreHandle::get` does not + // Negative TTL: entry is born already expired. `TokenKeyedStore::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")), + None, time::Duration::seconds(-1), ) .expect("credential entry inserts"); @@ -735,6 +759,7 @@ mod tests { .insert( association_token(jti), Some(cleartext_mapping_with_target_username("target")), + None, time::Duration::minutes(5), ) .expect("credential entry inserts"); @@ -759,10 +784,10 @@ mod tests { 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 + // cached, but the provisioning entry has already been evicted 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` + // `TokenKeyedStore::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)); @@ -771,6 +796,7 @@ mod tests { .insert( association_token(jti), Some(cleartext_mapping_with_target_username("target")), + None, time::Duration::minutes(5), ) .expect("credential entry inserts"); @@ -791,6 +817,7 @@ mod tests { .insert( association_token(jti), Some(cleartext_mapping_with_target_username("target")), + None, time::Duration::minutes(5), ) .expect("credential entry inserts"); @@ -805,6 +832,7 @@ mod tests { .insert( association_token(jti), Some(cleartext_mapping_with_target_username("target")), + None, time::Duration::minutes(5), ) .expect("credential entry re-inserts"); @@ -827,6 +855,7 @@ mod tests { .insert( association_token(jti), Some(cleartext_mapping_with_target_username("target")), + None, time::Duration::minutes(5), ) .expect("credential entry inserts"); @@ -834,19 +863,16 @@ mod tests { 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. + // Simulate provisioning eviction with an empty store that shares the original session cache. let orphaned_service = CredentialService { - credentials: CredentialStoreHandle::new(), + provisioning: TokenKeyedStore::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" + "sweep must drop sessions whose JTI is no longer in the provisioning store" ); } @@ -896,7 +922,7 @@ mod tests { .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"), + msg.contains("provisioning entry JTI does not match credential-injection KDC session JTI"), "actual: {msg}" ); } @@ -920,7 +946,7 @@ mod tests { let jti = Uuid::new_v4(); service - .insert(association_token(jti), None, time::Duration::minutes(5)) + .insert(association_token(jti), None, None, time::Duration::minutes(5)) .expect("provision-token entry inserts"); assert!( @@ -941,6 +967,7 @@ mod tests { .insert( association_token(jti), Some(cleartext_mapping_with_target_username("target")), + None, time::Duration::minutes(5), ) .expect("credential entry inserts"); @@ -950,6 +977,29 @@ mod tests { assert_eq!(kdc.target_hostname, "target.example"); } + #[test] + fn service_exposes_provisioned_target_kdc() { + let service = CredentialService::new(); + let jti = Uuid::new_v4(); + let krb_kdc = crate::target_addr::TargetAddr::parse("tcp://kdc.example.invalid:88", Some(88)) + .expect("KDC address parses"); + + service + .insert( + association_token(jti), + Some(cleartext_mapping_with_target_username("target@example.invalid")), + Some(TargetConnectionOptions { + krb_kdc: Some(krb_kdc.clone()), + }), + time::Duration::minutes(5), + ) + .expect("provisioning entry inserts"); + + let kdc = service.kdc_for(jti).expect("credential-injection KDC resolves"); + + assert_eq!(kdc.krb_kdc(), Some(&krb_kdc)); + } + #[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 dfc31df7f..d6d54f35f 100644 --- a/devolutions-gateway/src/generic_client.rs +++ b/devolutions-gateway/src/generic_client.rs @@ -149,11 +149,11 @@ where // // RdpProxy is generic over the server stream, so credential injection works // regardless of whether the upstream is direct TCP or tunnelled via an agent. - // The credential store is keyed on the association token's JTI, so a direct + // The provisioning 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) = credentials.get(claims.jti) - && entry.mapping.is_some() + && entry.value.mapping.is_some() { anyhow::ensure!(token == entry.token, "token mismatch"); let credential_injection_kdc = credentials.kdc_for(claims.jti)?; 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 a55556b59..84d656552 100644 --- a/devolutions-gateway/src/lib.rs +++ b/devolutions-gateway/src/lib.rs @@ -39,8 +39,10 @@ pub mod session; pub mod streaming; pub mod subscriber; pub mod target_addr; +pub mod target_connection_options; pub mod tls; pub mod token; +pub mod token_keyed_store; pub mod traffic_audit; pub mod upstream; pub mod utils; diff --git a/devolutions-gateway/src/openapi.rs b/devolutions-gateway/src/openapi.rs index 1e8b3e212..47a290c2f 100644 --- a/devolutions-gateway/src/openapi.rs +++ b/devolutions-gateway/src/openapi.rs @@ -382,6 +382,10 @@ struct PreflightOperation { /// /// Required for "provision-credentials" kind. target_credential: Option, + /// Options used by the Gateway when connecting to the target. + /// + /// Optional for "provision-credentials" kind. + connection_options: Option, /// The hostname to perform DNS resolution on. /// /// Required for "resolve-host" kind. @@ -392,6 +396,15 @@ struct PreflightOperation { time_to_live: Option, } +#[allow(unused)] +#[derive(Deserialize, utoipa::ToSchema)] +struct TargetConnectionOptions { + /// Kerberos KDC address for the target-side CredSSP connection. + /// + /// Supported schemes are `tcp` and `udp`. + krb_kdc: Option, +} + #[derive(Deserialize, utoipa::ToSchema)] enum PreflightOperationKind { #[serde(rename = "get-version")] diff --git a/devolutions-gateway/src/rd_clean_path.rs b/devolutions-gateway/src/rd_clean_path.rs index d36ebabe4..aa3f02471 100644 --- a/devolutions-gateway/src/rd_clean_path.rs +++ b/devolutions-gateway/src/rd_clean_path.rs @@ -539,10 +539,10 @@ 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. + // clean path procedure. The provisioning store is keyed on the association token's JTI. if let Some(jti) = crate::token::extract_jti(token).ok() && let Some(entry) = credentials.get(jti) - && entry.mapping.is_some() + && entry.value.mapping.is_some() { let credential_injection_kdc = credentials.kdc_for(jti)?; anyhow::ensure!(token == credential_injection_kdc.raw_token(), "token mismatch"); diff --git a/devolutions-gateway/src/rdp_proxy.rs b/devolutions-gateway/src/rdp_proxy.rs index 856210fa1..94a3cecfe 100644 --- a/devolutions-gateway/src/rdp_proxy.rs +++ b/devolutions-gateway/src/rdp_proxy.rs @@ -359,18 +359,10 @@ pub(crate) struct CredentialInjectionKerberosConfigs { pub client: Option, } -/// Whether a credential-injection session speaks Kerberos (vs NTLM). Decided once so both CredSSP -/// legs agree — sspi's acceptor and initiator must speak the same package or the handshake fails -/// reading one as the other. Kerberos needs the experimental opt-in AND a domain-qualified target -/// (a domainless account can't get a ticket). -fn injection_uses_kerberos( - enable_unstable: bool, - kerberos_credential_injection: bool, - protocol: CredentialInjectionClientAcceptorProtocol, -) -> bool { - enable_unstable - && kerberos_credential_injection - && matches!(protocol, CredentialInjectionClientAcceptorProtocol::Kerberos) +/// Whether a credential-injection session speaks Kerberos (vs NTLM). `None` means the feature is +/// disabled; otherwise the target credential determines the package for both CredSSP legs. +fn injection_uses_kerberos(protocol: Option) -> bool { + matches!(protocol, Some(CredentialInjectionClientAcceptorProtocol::Kerberos)) } /// Build the Kerberos config for both CredSSP legs from the single [`injection_uses_kerberos`] @@ -381,25 +373,25 @@ pub(crate) fn credential_injection_kerberos_configs( gateway_hostname: &str, credential_injection_kdc: &CredentialInjectionKdc, ) -> anyhow::Result { - let protocol = credential_injection_kdc.client_acceptor_protocol()?; + let protocol = (conf.debug.enable_unstable && conf.debug.kerberos_credential_injection) + .then(|| credential_injection_kdc.client_acceptor_protocol()) + .transpose()?; - if !injection_uses_kerberos( - conf.debug.enable_unstable, - conf.debug.kerberos_credential_injection, - protocol, - ) { + if !injection_uses_kerberos(protocol) { return Ok(CredentialInjectionKerberosConfigs { server: None, client: None, }); } + let krb_kdc = credential_injection_kdc + .krb_kdc() + .context("Kerberos credential injection requires target connection option krb_kdc")?; + 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(), }), }) @@ -676,14 +668,8 @@ mod tests { fn injection_uses_kerberos_requires_optin_and_domain_qualified_target() { use CredentialInjectionClientAcceptorProtocol::{Kerberos, Ntlm}; - assert!(injection_uses_kerberos(true, true, Kerberos)); - - // Either opt-in off => NTLM, even for a Kerberos-capable target. - assert!(!injection_uses_kerberos(false, true, Kerberos)); - assert!(!injection_uses_kerberos(true, false, Kerberos)); - - // Domainless target can't get a ticket => NTLM regardless of the flags. - assert!(!injection_uses_kerberos(true, true, Ntlm)); - assert!(!injection_uses_kerberos(false, false, Ntlm)); + assert!(injection_uses_kerberos(Some(Kerberos))); + assert!(!injection_uses_kerberos(Some(Ntlm))); + assert!(!injection_uses_kerberos(None)); } } diff --git a/devolutions-gateway/src/service.rs b/devolutions-gateway/src/service.rs index e847e1d94..ffb5356d5 100644 --- a/devolutions-gateway/src/service.rs +++ b/devolutions-gateway/src/service.rs @@ -350,9 +350,7 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { tasks.register(devolutions_gateway::token::CleanupTask { token_cache }); - tasks.register(devolutions_gateway::credential::CleanupTask { - handle: credentials.credential_store().clone(), - }); + tasks.register(credentials.provisioning_cleanup_task()); tasks.register(devolutions_gateway::credential_injection_kdc::CleanupTask { service: credentials }); 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..9b2054b16 --- /dev/null +++ b/devolutions-gateway/src/target_connection_options.rs @@ -0,0 +1,61 @@ +use serde::{Deserialize as _, de}; + +use crate::target_addr::TargetAddr; + +#[derive(Debug, Clone, Default, Deserialize)] +pub struct TargetConnectionOptions { + #[serde(default, deserialize_with = "deserialize_optional_krb_kdc")] + pub krb_kdc: Option, +} + +pub(crate) fn is_supported_krb_kdc_scheme(scheme: &str) -> bool { + matches!(scheme, "tcp" | "udp") +} + +fn deserialize_optional_krb_kdc<'de, D>(deserializer: D) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + let krb_kdc = Option::::deserialize(deserializer)?; + + if let Some(krb_kdc) = &krb_kdc + && !is_supported_krb_kdc_scheme(krb_kdc.scheme()) + { + return Err(de::Error::custom(format!( + "unsupported KDC protocol: {}", + krb_kdc.scheme() + ))); + } + + Ok(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")); + } +} diff --git a/devolutions-gateway/src/token_keyed_store.rs b/devolutions-gateway/src/token_keyed_store.rs new file mode 100644 index 000000000..218dca531 --- /dev/null +++ b/devolutions-gateway/src/token_keyed_store.rs @@ -0,0 +1,167 @@ +use std::collections::HashMap; +use std::sync::Arc; + +use anyhow::Context as _; +use async_trait::async_trait; +use devolutions_gateway_task::{ShutdownSignal, Task}; +use parking_lot::Mutex; +use uuid::Uuid; + +#[derive(Debug, thiserror::Error)] +#[error("invalid token")] +pub struct TokenKeyError(#[source] anyhow::Error); + +#[derive(Debug)] +pub struct TokenKeyedEntry { + pub token: String, + pub value: V, + pub expires_at: time::OffsetDateTime, +} + +pub type ArcTokenKeyedEntry = Arc>; + +#[derive(Debug)] +pub struct InsertResult { + pub jti: Uuid, + pub previous: Option>, +} + +#[derive(Debug)] +struct Store { + entries: HashMap>, +} + +#[derive(Debug)] +pub struct TokenKeyedStore(Arc>>); + +impl Clone for TokenKeyedStore { + fn clone(&self) -> Self { + Self(Arc::clone(&self.0)) + } +} + +impl Default for TokenKeyedStore { + fn default() -> Self { + Self::new() + } +} + +impl TokenKeyedStore { + pub fn new() -> Self { + Self(Arc::new(Mutex::new(Store { + entries: HashMap::new(), + }))) + } + + pub fn insert( + &self, + token: String, + value: V, + time_to_live: time::Duration, + ) -> Result, TokenKeyError> { + let jti = crate::token::extract_jti(&token) + .context("failed to extract token ID") + .map_err(TokenKeyError)?; + let entry = TokenKeyedEntry { + token, + value, + expires_at: time::OffsetDateTime::now_utc() + time_to_live, + }; + + let previous = self.0.lock().entries.insert(jti, Arc::new(entry)); + + Ok(InsertResult { jti, previous }) + } + + pub fn get(&self, jti: Uuid) -> Option> { + self.0.lock().entries.get(&jti).map(Arc::clone) + } + + fn remove_expired(&self, now: time::OffsetDateTime) { + self.0.lock().entries.retain(|_, entry| now < entry.expires_at); + } +} + +pub struct CleanupTask { + pub store: TokenKeyedStore, +} + +#[async_trait] +impl Task for CleanupTask +where + V: Send + Sync + 'static, +{ + type Output = anyhow::Result<()>; + + const NAME: &'static str = "token-keyed store cleanup"; + + async fn run(self, shutdown_signal: ShutdownSignal) -> Self::Output { + cleanup_task(self.store, shutdown_signal).await; + Ok(()) + } +} + +#[instrument(skip_all)] +async fn cleanup_task(store: TokenKeyedStore, mut shutdown_signal: ShutdownSignal) { + use tokio::time::{Duration, sleep}; + + const TASK_INTERVAL: Duration = Duration::from_secs(60 * 15); + + debug!("Task started"); + + loop { + tokio::select! { + _ = sleep(TASK_INTERVAL) => {} + _ = shutdown_signal.wait() => { + break; + } + } + + store.remove_expired(time::OffsetDateTime::now_utc()); + } + + debug!("Task terminated"); +} + +#[cfg(test)] +mod tests { + use base64::Engine as _; + + use super::*; + + fn token(jti: Uuid) -> 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(&serde_json::json!({ "jti": jti })).expect("token payload should serialize")); + let signature = engine.encode(b"signature"); + format!("{header}.{payload}.{signature}") + } + + #[test] + fn stores_opaque_values_by_token_jti() { + let store = TokenKeyedStore::new(); + let jti = Uuid::new_v4(); + + let result = store + .insert(token(jti), 42u32, time::Duration::minutes(5)) + .expect("entry inserts"); + + assert_eq!(result.jti, jti); + assert!(result.previous.is_none()); + assert_eq!(store.get(jti).expect("entry is indexed").value, 42); + } + + #[test] + fn removes_expired_values() { + let store = TokenKeyedStore::new(); + let jti = Uuid::new_v4(); + store + .insert(token(jti), (), time::Duration::seconds(-1)) + .expect("entry inserts"); + + store.remove_expired(time::OffsetDateTime::now_utc()); + + assert!(store.get(jti).is_none()); + } +} From 155f0a377c842d07186075be518eb323a32dbcbd Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Fri, 24 Jul 2026 14:28:45 -0400 Subject: [PATCH 02/11] refactor(dgw): scope the provisioning store to the crate Narrow the new token-keyed store, target connection options, and the service-level InsertError to `pub(crate)`. Only the binary-facing CredentialService surface stays `pub`. --- .../src/credential_injection_kdc.rs | 2 +- devolutions-gateway/src/lib.rs | 4 +-- .../src/target_connection_options.rs | 4 +-- devolutions-gateway/src/token_keyed_store.rs | 30 +++++++++---------- 4 files changed, 20 insertions(+), 20 deletions(-) diff --git a/devolutions-gateway/src/credential_injection_kdc.rs b/devolutions-gateway/src/credential_injection_kdc.rs index 714296039..ffbf8b55c 100644 --- a/devolutions-gateway/src/credential_injection_kdc.rs +++ b/devolutions-gateway/src/credential_injection_kdc.rs @@ -453,7 +453,7 @@ pub(crate) struct ProvisioningEntry { pub(crate) type ArcProvisioningEntry = ArcTokenKeyedEntry; #[derive(Debug, thiserror::Error)] -pub enum InsertError { +pub(crate) enum InsertError { #[error(transparent)] InvalidToken(#[from] crate::token_keyed_store::TokenKeyError), #[error("credential encryption failed")] diff --git a/devolutions-gateway/src/lib.rs b/devolutions-gateway/src/lib.rs index 84d656552..bab850cda 100644 --- a/devolutions-gateway/src/lib.rs +++ b/devolutions-gateway/src/lib.rs @@ -39,10 +39,10 @@ pub mod session; pub mod streaming; pub mod subscriber; pub mod target_addr; -pub mod target_connection_options; +pub(crate) mod target_connection_options; pub mod tls; pub mod token; -pub mod token_keyed_store; +pub(crate) mod token_keyed_store; pub mod traffic_audit; pub mod upstream; pub mod utils; diff --git a/devolutions-gateway/src/target_connection_options.rs b/devolutions-gateway/src/target_connection_options.rs index 9b2054b16..c03a27df2 100644 --- a/devolutions-gateway/src/target_connection_options.rs +++ b/devolutions-gateway/src/target_connection_options.rs @@ -3,9 +3,9 @@ use serde::{Deserialize as _, de}; use crate::target_addr::TargetAddr; #[derive(Debug, Clone, Default, Deserialize)] -pub struct TargetConnectionOptions { +pub(crate) struct TargetConnectionOptions { #[serde(default, deserialize_with = "deserialize_optional_krb_kdc")] - pub krb_kdc: Option, + pub(crate) krb_kdc: Option, } pub(crate) fn is_supported_krb_kdc_scheme(scheme: &str) -> bool { diff --git a/devolutions-gateway/src/token_keyed_store.rs b/devolutions-gateway/src/token_keyed_store.rs index 218dca531..2bd030748 100644 --- a/devolutions-gateway/src/token_keyed_store.rs +++ b/devolutions-gateway/src/token_keyed_store.rs @@ -9,21 +9,21 @@ use uuid::Uuid; #[derive(Debug, thiserror::Error)] #[error("invalid token")] -pub struct TokenKeyError(#[source] anyhow::Error); +pub(crate) struct TokenKeyError(#[source] anyhow::Error); #[derive(Debug)] -pub struct TokenKeyedEntry { - pub token: String, - pub value: V, - pub expires_at: time::OffsetDateTime, +pub(crate) struct TokenKeyedEntry { + pub(crate) token: String, + pub(crate) value: V, + pub(crate) expires_at: time::OffsetDateTime, } -pub type ArcTokenKeyedEntry = Arc>; +pub(crate) type ArcTokenKeyedEntry = Arc>; #[derive(Debug)] -pub struct InsertResult { - pub jti: Uuid, - pub previous: Option>, +pub(crate) struct InsertResult { + pub(crate) jti: Uuid, + pub(crate) previous: Option>, } #[derive(Debug)] @@ -32,7 +32,7 @@ struct Store { } #[derive(Debug)] -pub struct TokenKeyedStore(Arc>>); +pub(crate) struct TokenKeyedStore(Arc>>); impl Clone for TokenKeyedStore { fn clone(&self) -> Self { @@ -47,13 +47,13 @@ impl Default for TokenKeyedStore { } impl TokenKeyedStore { - pub fn new() -> Self { + pub(crate) fn new() -> Self { Self(Arc::new(Mutex::new(Store { entries: HashMap::new(), }))) } - pub fn insert( + pub(crate) fn insert( &self, token: String, value: V, @@ -73,7 +73,7 @@ impl TokenKeyedStore { Ok(InsertResult { jti, previous }) } - pub fn get(&self, jti: Uuid) -> Option> { + pub(crate) fn get(&self, jti: Uuid) -> Option> { self.0.lock().entries.get(&jti).map(Arc::clone) } @@ -82,8 +82,8 @@ impl TokenKeyedStore { } } -pub struct CleanupTask { - pub store: TokenKeyedStore, +pub(crate) struct CleanupTask { + pub(crate) store: TokenKeyedStore, } #[async_trait] From 3517aff0321a634f6aecb62a2a1ebf28ad3afdb2 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Fri, 24 Jul 2026 16:01:56 -0400 Subject: [PATCH 03/11] refactor(dgw): split credential injection into provisioning and KDC services Separate the provisioning store (credentials + target options, keyed by JTI) from the credential-injection KDC service (Kerberos session cache). Neither owns the other: the KDC service reads the store passed in at resolution time, and notices re-provisioning through a weak reference to the entry rather than an insert-time callback. Encryption stays in the provisioning store, at the credential boundary. --- devolutions-gateway/src/api/kdc_proxy.rs | 5 +- devolutions-gateway/src/api/preflight.rs | 16 +- devolutions-gateway/src/api/rdp.rs | 4 + .../src/credential_injection_kdc.rs | 375 ++++++++---------- devolutions-gateway/src/generic_client.rs | 10 +- devolutions-gateway/src/lib.rs | 4 + devolutions-gateway/src/listener.rs | 1 + devolutions-gateway/src/ngrok.rs | 1 + devolutions-gateway/src/provisioning.rs | 81 ++++ devolutions-gateway/src/rd_clean_path.rs | 7 +- devolutions-gateway/src/service.rs | 4 +- devolutions-gateway/src/token_keyed_store.rs | 15 +- 12 files changed, 291 insertions(+), 232 deletions(-) create mode 100644 devolutions-gateway/src/provisioning.rs diff --git a/devolutions-gateway/src/api/kdc_proxy.rs b/devolutions-gateway/src/api/kdc_proxy.rs index 865fc369f..1cf6f6a7d 100644 --- a/devolutions-gateway/src/api/kdc_proxy.rs +++ b/devolutions-gateway/src/api/kdc_proxy.rs @@ -23,6 +23,7 @@ async fn kdc_proxy( State(DgwState { conf_handle, credentials, + provisioning, agent_tunnel_handle, .. }): State, @@ -47,7 +48,9 @@ async fn kdc_proxy( KdcDestination::Inject { jti } => { enforce_credential_injection_enabled(jti, conf.debug.enable_unstable)?; - let kdc = credentials.kdc_for(jti).map_err(credential_injection_resolve_error)?; + let kdc = credentials + .kdc_for(&provisioning, 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 1c56e650c..8cc920d5f 100644 --- a/devolutions-gateway/src/api/preflight.rs +++ b/devolutions-gateway/src/api/preflight.rs @@ -11,9 +11,9 @@ use uuid::Uuid; use crate::DgwState; use crate::config::Conf; -use crate::credential_injection_kdc::{CredentialService, InsertError}; use crate::extract::PreflightScope; use crate::http::HttpError; +use crate::provisioning::{InsertError, ProvisioningStore}; use crate::session::SessionMessageSender; const OP_GET_VERSION: &str = "get-version"; @@ -196,7 +196,7 @@ pub(super) async fn post_preflight( State(DgwState { conf_handle, sessions, - credentials, + provisioning, .. }): State, _scope: PreflightScope, @@ -223,13 +223,13 @@ pub(super) async fn post_preflight( let outputs = outputs.clone(); let conf = conf_handle.get_conf(); let sessions = sessions.clone(); - let credentials = credentials.clone(); + let provisioning = provisioning.clone(); async move { let operation_id = operation.id; trace!(%operation.id, "Process preflight operation"); - if let Err(error) = handle_operation(operation, &outputs, &conf, &sessions, &credentials).await { + if let Err(error) = handle_operation(operation, &outputs, &conf, &sessions, &provisioning).await { outputs.push(PreflightOutput { operation_id, kind: PreflightOutputKind::Alert { @@ -256,7 +256,7 @@ async fn handle_operation( outputs: &Outputs, conf: &Conf, sessions: &SessionMessageSender, - credentials: &CredentialService, + provisioning: &ProvisioningStore, ) -> Result<(), PreflightError> { match operation.kind.as_str() { OP_GET_VERSION => outputs.push(PreflightOutput { @@ -359,7 +359,7 @@ async fn handle_operation( })?; } - let previous_entry = credentials + let previous_entry = provisioning .insert(token, mapping, connection_options, time_to_live) .inspect_err(|error| warn!(%operation.id, error = format!("{error:#}"), "Failed to insert credentials")) .map_err(|error| match error { @@ -372,8 +372,8 @@ async fn handle_operation( ), })?; - // `CredentialService::insert` already drops the cached Kerberos session for a - // replaced entry, so no explicit invalidation is needed here. + // A replaced entry produces a new provisioning entry, so the credential-injection KDC + // service re-derives its session on the next lookup — no explicit invalidation 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 b3d45dbcb..85db29da3 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, credentials, + provisioning, agent_tunnel_handle, .. }): State, @@ -47,6 +48,7 @@ pub async fn handler( recordings.active_recordings, source_addr, credentials, + provisioning, agent_tunnel_handle, ) .instrument(span) @@ -67,6 +69,7 @@ async fn handle_socket( active_recordings: Arc, source_addr: SocketAddr, credentials: crate::credential_injection_kdc::CredentialService, + provisioning: crate::provisioning::ProvisioningStore, agent_tunnel_handle: Option>, ) { let (stream, close_handle) = crate::ws::handle( @@ -85,6 +88,7 @@ async fn handle_socket( subscriber_tx, &active_recordings, &credentials, + &provisioning, agent_tunnel_handle, ) .await; diff --git a/devolutions-gateway/src/credential_injection_kdc.rs b/devolutions-gateway/src/credential_injection_kdc.rs index ffbf8b55c..34e2c5bf8 100644 --- a/devolutions-gateway/src/credential_injection_kdc.rs +++ b/devolutions-gateway/src/credential_injection_kdc.rs @@ -26,9 +26,9 @@ use thiserror::Error; use url::Url; use uuid::Uuid; -use crate::credential::{AppCredential, AppCredentialMapping, CleartextAppCredentialMapping}; +use crate::credential::{AppCredential, AppCredentialMapping}; +use crate::provisioning::{ArcProvisioningEntry, ProvisioningStore, WeakProvisioningEntry}; use crate::target_connection_options::TargetConnectionOptions; -use crate::token_keyed_store::{ArcTokenKeyedEntry, TokenKeyedStore}; // 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 @@ -40,7 +40,6 @@ const IN_PROCESS_KDC_HOST: &str = "cred.invalid"; pub(crate) struct CredentialInjectionKdc { jti: Uuid, - raw_token: String, credential_mapping: AppCredentialMapping, connection_options: Option, target_hostname: String, @@ -145,7 +144,6 @@ impl CredentialInjectionKdc { Ok(Self { jti, - raw_token: provisioning_entry.token.clone(), credential_mapping: mapping.clone(), connection_options: provisioning_entry.value.connection_options.clone(), target_hostname, @@ -158,10 +156,6 @@ impl CredentialInjectionKdc { self.jti } - pub(crate) fn raw_token(&self) -> &str { - &self.raw_token - } - pub(crate) fn proxy_credential(&self) -> &AppCredential { &self.credential_mapping.proxy } @@ -444,27 +438,26 @@ fn random_32_bytes() -> Vec { bytes } -#[derive(Debug)] -pub(crate) struct ProvisioningEntry { - pub mapping: Option, - pub connection_options: Option, -} - -pub(crate) type ArcProvisioningEntry = ArcTokenKeyedEntry; - -#[derive(Debug, thiserror::Error)] -pub(crate) enum InsertError { - #[error(transparent)] - InvalidToken(#[from] crate::token_keyed_store::TokenKeyError), - #[error("credential encryption failed")] - Internal(#[source] anyhow::Error), -} - -/// Coordinates provisioned credentials and credential-injection KDC state. +/// Owns the Kerberos sessions used by proxy-based credential injection, keyed by association-token +/// JTI. +/// +/// This is deliberately separate from the [`ProvisioningStore`]: credentials live there, sessions +/// live here, and the two are keyed by the same JTI but never reach into each other. Resolution +/// reads the provisioning store passed in by the caller. #[derive(Debug, Clone)] pub struct CredentialService { - provisioning: TokenKeyedStore, - sessions: Arc>>>, + sessions: Arc>>, +} + +/// A cached Kerberos session tagged with the provisioning entry it was derived for. +/// +/// The weak reference is how we notice re-provisioning without the two stores talking to each other: +/// a fresh provisioning under the same JTI produces a new entry, so the cached session stops matching +/// and gets re-derived on the next lookup. +#[derive(Debug)] +struct CachedSession { + entry: WeakProvisioningEntry, + session: Arc, } impl Default for CredentialService { @@ -476,80 +469,69 @@ impl Default for CredentialService { impl CredentialService { pub fn new() -> Self { Self { - provisioning: TokenKeyedStore::new(), sessions: Arc::new(Mutex::new(HashMap::new())), } } - /// Insert (or replace) a provisioning 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 - /// [`TokenKeyedStore::insert`] reports no replacement, because the prior entry may have - /// already been evicted by its cleanup task while the 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(crate) fn insert( - &self, - token: String, - mapping: Option, - connection_options: Option, - time_to_live: time::Duration, - ) -> Result, InsertError> { - let mapping = mapping - .map(CleartextAppCredentialMapping::encrypt) - .transpose() - .map_err(InsertError::Internal)?; - let result = self.provisioning.insert( - token, - ProvisioningEntry { - mapping, - connection_options, - }, - time_to_live, - )?; - self.sessions.lock().remove(&result.jti); - Ok(result.previous) - } - - /// Look up a provisioning entry by its association-token JTI. - pub(crate) fn get(&self, jti: Uuid) -> Option { - self.provisioning.get(jti) - } - - pub fn provisioning_cleanup_task(&self) -> impl Task> + 'static + use<> { - crate::token_keyed_store::CleanupTask { - store: self.provisioning.clone(), - } - } - /// 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 provisioning_entry = self.provisioning.get(jti).ok_or_else(|| { + pub(crate) fn kdc_for( + &self, + provisioning: &ProvisioningStore, + jti: Uuid, + ) -> Result { + let entry = provisioning.get(jti).ok_or_else(|| { warn!(%jti, "KDC token references missing credential-injection state"); CredentialInjectionKdcResolveError::MissingCredential { jti } })?; + self.kdc_from_entry(entry, jti) + } - // `TokenKeyedStore::get` does not enforce expiry — entries are evicted asynchronously - // by the provisioning cleanup task. Treat a stale entry as already gone so we never build a - // KDC against expired credentials. - if time::OffsetDateTime::now_utc() >= provisioning_entry.expires_at { + /// Look up the credential-injection KDC for an association token, if one was provisioned. + /// + /// Returns `Ok(None)` when the token was not provisioned for credential injection: either no + /// entry exists, or it is a provision-token entry carrying no credential mapping. + pub(crate) fn resolve_injection_kdc( + &self, + provisioning: &ProvisioningStore, + jti: Uuid, + token: &str, + ) -> anyhow::Result> { + let Some(entry) = provisioning.get(jti) else { + return Ok(None); + }; + if entry.value.mapping.is_none() { + return Ok(None); + } + + anyhow::ensure!(token == entry.token, "token mismatch"); + self.kdc_from_entry(entry, jti).map(Some).map_err(Into::into) + } + + fn kdc_from_entry( + &self, + entry: ArcProvisioningEntry, + jti: Uuid, + ) -> Result { + // `TokenKeyedStore::get` does not enforce expiry — entries are evicted asynchronously by the + // provisioning cleanup task. Treat a stale entry as already gone so we never build a KDC + // against expired credentials. + if time::OffsetDateTime::now_utc() >= entry.expires_at { warn!(%jti, "KDC token references expired credential-injection state"); self.sessions.lock().remove(&jti); return Err(CredentialInjectionKdcResolveError::ExpiredCredential { jti }); } - let mapping = provisioning_entry.value.mapping.as_ref().ok_or_else(|| { + let mapping = entry.value.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(&provisioning_entry.token) - .map_err(|source| { + let target_hostname = + crate::token::extract_credential_injection_target_hostname(&entry.token).map_err(|source| { warn!( %jti, error = format!("{source:#}"), @@ -559,39 +541,45 @@ impl CredentialService { })?; 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. + // Atomic get-or-derive under the lock: guarantees a single session wins per JTI even under + // concurrent calls. A cached session is reused only when it was derived for this exact + // provisioning entry — a re-provisioning produces a new entry, so its stale session is + // dropped and re-derived here. Derivation is fast (a few hundred bytes of OsRng), so holding + // the lock across it 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) + let reuse = sessions.get(&jti).and_then(|cached| { + cached + .entry + .upgrade() + .filter(|cached_entry| Arc::ptr_eq(cached_entry, &entry)) + .map(|_| Arc::clone(&cached.session)) + }); + match reuse { + Some(session) => session, + None => { + let session = Arc::new(derive_credential_injection_kdc_session(&proxy_username, jti)); + sessions.insert( + jti, + CachedSession { + entry: Arc::downgrade(&entry), + session: Arc::clone(&session), + }, + ); + session + } + } }; - CredentialInjectionKdc::from_parts(jti, provisioning_entry, target_hostname, session) + CredentialInjectionKdc::from_parts(jti, entry, target_hostname, session) .map_err(|source| CredentialInjectionKdcResolveError::BuildKdcConfig { jti, source }) } + /// Drop cached sessions whose provisioning entry is gone (evicted or expired). fn sweep_orphans(&self) { - let stale_jtis: Vec = { - let sessions = self.sessions.lock(); - sessions - .keys() - .copied() - .filter(|jti| self.provisioning.get(*jti).is_none()) - .collect() - }; - - if stale_jtis.is_empty() { - return; - } - - let mut sessions = self.sessions.lock(); - for jti in stale_jtis { - sessions.remove(&jti); - } + self.sessions + .lock() + .retain(|_, cached| cached.entry.upgrade().is_some()); } } @@ -670,18 +658,23 @@ mod tests { })) } - fn dummy_entry_with_target_username(jti: Uuid, target_username: &str) -> ArcProvisioningEntry { - let service = CredentialService::new(); - service + fn provisioned_store(jti: Uuid, target_username: &str, time_to_live: time::Duration) -> ProvisioningStore { + let provisioning = ProvisioningStore::new(); + provisioning .insert( association_token(jti), Some(cleartext_mapping_with_target_username(target_username)), None, - time::Duration::minutes(5), + time_to_live, ) .expect("credential entry inserts"); + provisioning + } - service.get(jti).expect("provisioning entry is indexed by JTI") + fn dummy_entry_with_target_username(jti: Uuid, target_username: &str) -> ArcProvisioningEntry { + provisioned_store(jti, target_username, time::Duration::minutes(5)) + .get(jti) + .expect("provisioning entry is indexed by JTI") } fn dummy_entry(jti: Uuid) -> ArcProvisioningEntry { @@ -732,18 +725,11 @@ mod tests { // Negative TTL: entry is born already expired. `TokenKeyedStore::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")), - None, - time::Duration::seconds(-1), - ) - .expect("credential entry inserts"); + let provisioning = provisioned_store(jti, "target", time::Duration::seconds(-1)); assert!( matches!( - service.kdc_for(jti), + service.kdc_for(&provisioning, jti), Err(CredentialInjectionKdcResolveError::ExpiredCredential { .. }) ), "expired credentials must not yield a KDC" @@ -754,18 +740,10 @@ mod tests { fn service_kdc_for_returns_same_session_under_concurrent_calls() { let service = CredentialService::new(); let jti = Uuid::new_v4(); + let provisioning = provisioned_store(jti, "target", time::Duration::minutes(5)); - service - .insert( - association_token(jti), - Some(cleartext_mapping_with_target_username("target")), - None, - 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"); + let first = service.kdc_for(&provisioning, jti).expect("first call resolves"); + let second = service.kdc_for(&provisioning, 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 @@ -779,56 +757,44 @@ mod tests { } #[test] - fn service_insert_drops_stale_session_even_without_credential_replacement() { + fn service_kdc_for_ignores_cached_session_from_a_gone_entry() { 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 provisioning entry has already been evicted and `sweep_orphans` has not - // run yet. A fresh provisioning - // under the same JTI must drop the stale session regardless of whether - // `TokenKeyedStore::insert` reports a replacement, otherwise the next `kdc_for` - // would reuse the old key material. + // Simulate the race called out by Codex: a previous provisioning's session is still cached, + // but its provisioning entry is already gone (dead weak reference). A lookup against a fresh + // provisioning entry must not reuse that stale session; it must re-derive. let stale_session = Arc::new(derive_credential_injection_kdc_session("proxy@example.invalid", jti)); - service.sessions.lock().insert(jti, Arc::clone(&stale_session)); + service.sessions.lock().insert( + jti, + CachedSession { + entry: WeakProvisioningEntry::new(), + session: Arc::clone(&stale_session), + }, + ); - let previous = service - .insert( - association_token(jti), - Some(cleartext_mapping_with_target_username("target")), - None, - time::Duration::minutes(5), - ) - .expect("credential entry inserts"); - assert!(previous.is_none(), "test precondition: no credential replacement"); + let provisioning = provisioned_store(jti, "target", time::Duration::minutes(5)); + let kdc = service.kdc_for(&provisioning, jti).expect("fresh entry resolves"); assert!( - !service.sessions.lock().contains_key(&jti), - "insert must drop stale session even when no credential replacement occurred" + !Arc::ptr_eq(&kdc.session, &stale_session), + "a session cached against a gone entry must not be reused" ); } #[test] - fn service_insert_replacement_drops_cached_kerberos_material() { + fn service_kdc_for_rederives_session_after_reprovisioning() { let service = CredentialService::new(); let jti = Uuid::new_v4(); + let provisioning = provisioned_store(jti, "target", time::Duration::minutes(5)); - service - .insert( - association_token(jti), - Some(cleartext_mapping_with_target_username("target")), - None, - time::Duration::minutes(5), - ) - .expect("credential entry inserts"); - - let first = service.kdc_for(jti).expect("first call resolves"); + let first = service.kdc_for(&provisioning, jti).expect("first call resolves"); let first_key = first.session.acceptor.long_term_key.expose_secret().clone(); - // 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 + // Re-provision under the same JTI: this produces a new provisioning entry, so the cached + // session no longer matches and must be re-derived, otherwise the new KDC would carry stale + // key material that the freshly provisioned credentials no longer match. + provisioning .insert( association_token(jti), Some(cleartext_mapping_with_target_username("target")), @@ -837,12 +803,14 @@ mod tests { ) .expect("credential entry re-inserts"); - let second = service.kdc_for(jti).expect("second call resolves with fresh session"); + let second = service + .kdc_for(&provisioning, 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" + "re-provisioning must force a fresh session derivation" ); } @@ -851,28 +819,20 @@ mod tests { let service = CredentialService::new(); let jti = Uuid::new_v4(); - service - .insert( - association_token(jti), - Some(cleartext_mapping_with_target_username("target")), - None, - 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 provisioning eviction with an empty store that shares the original session cache. - let orphaned_service = CredentialService { - provisioning: TokenKeyedStore::new(), - sessions: Arc::clone(&service.sessions), - }; + { + let provisioning = provisioned_store(jti, "target", time::Duration::minutes(5)); + service + .kdc_for(&provisioning, jti) + .expect("kdc_for populates session cache"); + assert!(service.sessions.lock().contains_key(&jti), "session cached"); + } + // The provisioning store is dropped here, so its entry is gone and the cached session's weak + // reference is now dead. - orphaned_service.sweep_orphans(); + service.sweep_orphans(); assert!( - !orphaned_service.sessions.lock().contains_key(&jti), - "sweep must drop sessions whose JTI is no longer in the provisioning store" + !service.sessions.lock().contains_key(&jti), + "sweep must drop sessions whose provisioning entry is gone" ); } @@ -930,10 +890,11 @@ mod tests { #[test] fn service_kdc_for_rejects_unknown_jti() { let service = CredentialService::new(); + let provisioning = ProvisioningStore::new(); assert!( matches!( - service.kdc_for(Uuid::new_v4()), + service.kdc_for(&provisioning, Uuid::new_v4()), Err(CredentialInjectionKdcResolveError::MissingCredential { .. }) ), "KDC tokens with jet_cred_id must not fall back to real-KDC forwarding" @@ -943,15 +904,16 @@ mod tests { #[test] fn service_kdc_for_rejects_non_injection_entry() { let service = CredentialService::new(); + let provisioning = ProvisioningStore::new(); let jti = Uuid::new_v4(); - service + provisioning .insert(association_token(jti), None, None, time::Duration::minutes(5)) .expect("provision-token entry inserts"); assert!( matches!( - service.kdc_for(jti), + service.kdc_for(&provisioning, jti), Err(CredentialInjectionKdcResolveError::NonInjectionCredential { .. }) ), "KDC tokens with jet_cred_id must require provision-credentials state" @@ -962,31 +924,27 @@ mod tests { fn service_kdc_for_lazily_extracts_target_hostname_from_entry_token() { let service = CredentialService::new(); let jti = Uuid::new_v4(); + let provisioning = provisioned_store(jti, "target", time::Duration::minutes(5)); - service - .insert( - association_token(jti), - Some(cleartext_mapping_with_target_username("target")), - None, - time::Duration::minutes(5), - ) - .expect("credential entry inserts"); - - let kdc = service.kdc_for(jti).expect("credential-injection KDC resolves"); + let kdc = service + .kdc_for(&provisioning, jti) + .expect("credential-injection KDC resolves"); assert_eq!(kdc.target_hostname, "target.example"); } #[test] - fn service_exposes_provisioned_target_kdc() { + fn provisioning_and_credential_service_share_target_options() { + let provisioning = ProvisioningStore::new(); let service = CredentialService::new(); let jti = Uuid::new_v4(); + let token = association_token(jti); let krb_kdc = crate::target_addr::TargetAddr::parse("tcp://kdc.example.invalid:88", Some(88)) .expect("KDC address parses"); - service + provisioning .insert( - association_token(jti), + token.clone(), Some(cleartext_mapping_with_target_username("target@example.invalid")), Some(TargetConnectionOptions { krb_kdc: Some(krb_kdc.clone()), @@ -995,7 +953,20 @@ mod tests { ) .expect("provisioning entry inserts"); - let kdc = service.kdc_for(jti).expect("credential-injection KDC resolves"); + let entry = provisioning.get(jti).expect("provisioning entry is available directly"); + assert_eq!( + entry + .value + .connection_options + .as_ref() + .and_then(|options| options.krb_kdc.as_ref()), + Some(&krb_kdc) + ); + + let kdc = service + .resolve_injection_kdc(&provisioning, jti, &token) + .expect("credential-injection lookup succeeds") + .expect("credential-injection KDC resolves"); assert_eq!(kdc.krb_kdc(), Some(&krb_kdc)); } diff --git a/devolutions-gateway/src/generic_client.rs b/devolutions-gateway/src/generic_client.rs index d6d54f35f..c57bbcf76 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_injection_kdc::CredentialService; +use crate::provisioning::ProvisioningStore; 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, credentials: CredentialService, + provisioning: ProvisioningStore, #[builder(default)] agent_tunnel_handle: Option>, } @@ -52,6 +54,7 @@ where subscriber_tx, active_recordings, credentials, + provisioning, agent_tunnel_handle, } = self; @@ -152,12 +155,9 @@ where // The provisioning 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) = credentials.get(claims.jti) - && entry.value.mapping.is_some() + && let Some(credential_injection_kdc) = + credentials.resolve_injection_kdc(&provisioning, claims.jti, token)? { - anyhow::ensure!(token == entry.token, "token mismatch"); - let credential_injection_kdc = credentials.kdc_for(claims.jti)?; - info!( jti = %credential_injection_kdc.jti(), "RDP-TLS forwarding with credential injection" diff --git a/devolutions-gateway/src/lib.rs b/devolutions-gateway/src/lib.rs index bab850cda..329a98002 100644 --- a/devolutions-gateway/src/lib.rs +++ b/devolutions-gateway/src/lib.rs @@ -30,6 +30,7 @@ pub mod log; pub mod middleware; pub mod ngrok; pub mod plugin_manager; +pub mod provisioning; pub mod proxy; pub mod rd_clean_path; pub mod rdp_pcb; @@ -63,6 +64,7 @@ pub struct DgwState { pub shutdown_signal: devolutions_gateway_task::ShutdownSignal, pub recordings: recording::RecordingMessageSender, pub job_queue_handle: job_queue::JobQueueHandle, + pub provisioning: provisioning::ProvisioningStore, pub credentials: credential_injection_kdc::CredentialService, pub monitoring_state: Arc, pub traffic_audit_handle: traffic_audit::TrafficAuditHandle, @@ -91,6 +93,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 provisioning = provisioning::ProvisioningStore::new(); let credentials = credential_injection_kdc::CredentialService::new(); let monitoring_state = Arc::new(network_monitor::State::new(Arc::new(MockMonitorsCache))?); @@ -104,6 +107,7 @@ impl DgwState { recordings: recording_manager_handle, job_queue_handle, traffic_audit_handle, + provisioning, credentials, monitoring_state, agent_tunnel_handle: None, diff --git a/devolutions-gateway/src/listener.rs b/devolutions-gateway/src/listener.rs index 5e23f5f8a..58515e6a1 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) .credentials(state.credentials) + .provisioning(state.provisioning) .agent_tunnel_handle(state.agent_tunnel_handle) .build() .serve() diff --git a/devolutions-gateway/src/ngrok.rs b/devolutions-gateway/src/ngrok.rs index 9e2e846bd..e064becc1 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) .credentials(state.credentials) + .provisioning(state.provisioning) .agent_tunnel_handle(state.agent_tunnel_handle) .build() .serve() diff --git a/devolutions-gateway/src/provisioning.rs b/devolutions-gateway/src/provisioning.rs new file mode 100644 index 000000000..b1e77e109 --- /dev/null +++ b/devolutions-gateway/src/provisioning.rs @@ -0,0 +1,81 @@ +use std::sync::Weak; + +use devolutions_gateway_task::Task; +use uuid::Uuid; + +use crate::credential::{AppCredentialMapping, CleartextAppCredentialMapping}; +use crate::target_connection_options::TargetConnectionOptions; +use crate::token_keyed_store::{ArcTokenKeyedEntry, TokenKeyError, TokenKeyedEntry, TokenKeyedStore}; + +#[derive(Debug)] +pub(crate) struct ProvisioningEntry { + pub(crate) mapping: Option, + pub(crate) connection_options: Option, +} + +pub(crate) type ArcProvisioningEntry = ArcTokenKeyedEntry; +pub(crate) type WeakProvisioningEntry = Weak>; + +#[derive(Debug, thiserror::Error)] +pub(crate) enum InsertError { + #[error(transparent)] + InvalidToken(#[from] TokenKeyError), + #[error("credential encryption failed")] + Internal(#[source] anyhow::Error), +} + +/// Stores credentials and target options provisioned for a session, keyed by association-token JTI. +/// +/// This is the credential boundary: cleartext mappings are encrypted on the way in, so the +/// underlying token-keyed store only ever holds encrypted material and the master key never leaves +/// this module's dependency graph. +#[derive(Debug, Clone)] +pub struct ProvisioningStore { + entries: TokenKeyedStore, +} + +impl Default for ProvisioningStore { + fn default() -> Self { + Self::new() + } +} + +impl ProvisioningStore { + pub fn new() -> Self { + Self { + entries: TokenKeyedStore::new(), + } + } + + pub(crate) fn insert( + &self, + token: String, + mapping: Option, + connection_options: Option, + time_to_live: time::Duration, + ) -> Result, InsertError> { + let mapping = mapping + .map(CleartextAppCredentialMapping::encrypt) + .transpose() + .map_err(InsertError::Internal)?; + let previous = self.entries.insert( + token, + ProvisioningEntry { + mapping, + connection_options, + }, + time_to_live, + )?; + Ok(previous) + } + + pub(crate) fn get(&self, jti: Uuid) -> Option { + self.entries.get(jti) + } + + pub fn cleanup_task(&self) -> impl Task> + 'static + use<> { + crate::token_keyed_store::CleanupTask { + store: self.entries.clone(), + } + } +} diff --git a/devolutions-gateway/src/rd_clean_path.rs b/devolutions-gateway/src/rd_clean_path.rs index aa3f02471..25fa7e47c 100644 --- a/devolutions-gateway/src/rd_clean_path.rs +++ b/devolutions-gateway/src/rd_clean_path.rs @@ -12,6 +12,7 @@ use tracing::field; use crate::config::Conf; use crate::credential_injection_kdc::{CredentialInjectionKdc, CredentialService}; +use crate::provisioning::ProvisioningStore; use crate::proxy::Proxy; use crate::recording::ActiveRecordings; use crate::session::{ConnectionModeDetails, DisconnectInterest, DisconnectedInfo, SessionInfo, SessionMessageSender}; @@ -521,6 +522,7 @@ pub async fn handle( subscriber_tx: SubscriberSender, active_recordings: &ActiveRecordings, credentials: &CredentialService, + provisioning: &ProvisioningStore, agent_tunnel_handle: Option>, ) -> anyhow::Result<()> { // Special handshake of our RDP extension @@ -541,11 +543,8 @@ pub async fn handle( // proxy-based credential injection mode. Otherwise, we continue the usual // clean path procedure. The provisioning store is keyed on the association token's JTI. if let Some(jti) = crate::token::extract_jti(token).ok() - && let Some(entry) = credentials.get(jti) - && entry.value.mapping.is_some() + && let Some(credential_injection_kdc) = credentials.resolve_injection_kdc(provisioning, jti, token)? { - let credential_injection_kdc = credentials.kdc_for(jti)?; - anyhow::ensure!(token == credential_injection_kdc.raw_token(), "token mismatch"); debug!( jti = %credential_injection_kdc.jti(), "Switching to RdpProxy for credential injection (WebSocket)" diff --git a/devolutions-gateway/src/service.rs b/devolutions-gateway/src/service.rs index ffb5356d5..6b6b2e12f 100644 --- a/devolutions-gateway/src/service.rs +++ b/devolutions-gateway/src/service.rs @@ -267,6 +267,7 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { .await .context("failed to initialize traffic audit manager")?; + let provisioning = devolutions_gateway::provisioning::ProvisioningStore::new(); let credentials = devolutions_gateway::credential_injection_kdc::CredentialService::new(); let filesystem_monitor_config_cache = devolutions_gateway::api::monitoring::FilesystemConfigCache::new( @@ -315,6 +316,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(), + provisioning: provisioning.clone(), credentials: credentials.clone(), monitoring_state, traffic_audit_handle: traffic_audit_task.handle(), @@ -350,7 +352,7 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { tasks.register(devolutions_gateway::token::CleanupTask { token_cache }); - tasks.register(credentials.provisioning_cleanup_task()); + tasks.register(provisioning.cleanup_task()); tasks.register(devolutions_gateway::credential_injection_kdc::CleanupTask { service: credentials }); diff --git a/devolutions-gateway/src/token_keyed_store.rs b/devolutions-gateway/src/token_keyed_store.rs index 2bd030748..5568d50d4 100644 --- a/devolutions-gateway/src/token_keyed_store.rs +++ b/devolutions-gateway/src/token_keyed_store.rs @@ -20,12 +20,6 @@ pub(crate) struct TokenKeyedEntry { pub(crate) type ArcTokenKeyedEntry = Arc>; -#[derive(Debug)] -pub(crate) struct InsertResult { - pub(crate) jti: Uuid, - pub(crate) previous: Option>, -} - #[derive(Debug)] struct Store { entries: HashMap>, @@ -58,7 +52,7 @@ impl TokenKeyedStore { token: String, value: V, time_to_live: time::Duration, - ) -> Result, TokenKeyError> { + ) -> Result>, TokenKeyError> { let jti = crate::token::extract_jti(&token) .context("failed to extract token ID") .map_err(TokenKeyError)?; @@ -70,7 +64,7 @@ impl TokenKeyedStore { let previous = self.0.lock().entries.insert(jti, Arc::new(entry)); - Ok(InsertResult { jti, previous }) + Ok(previous) } pub(crate) fn get(&self, jti: Uuid) -> Option> { @@ -143,12 +137,11 @@ mod tests { let store = TokenKeyedStore::new(); let jti = Uuid::new_v4(); - let result = store + let previous = store .insert(token(jti), 42u32, time::Duration::minutes(5)) .expect("entry inserts"); - assert_eq!(result.jti, jti); - assert!(result.previous.is_none()); + assert!(previous.is_none()); assert_eq!(store.get(jti).expect("entry is indexed").value, 42); } From 79a4484f857ff458af8eebb2ee2cac5c3a618775 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Fri, 24 Jul 2026 16:39:58 -0400 Subject: [PATCH 04/11] refactor(dgw): rename KDC service and validate target options at construction Rename `CredentialService` to `CredentialInjectionKdcService` (it now holds only the Kerberos session cache, no credentials) and the `DgwState` field likewise. Validate the KDC scheme when a `TargetConnectionOptions` is built, not only when deserialized, so in-crate callers can't construct an unsupported address. Document that `TokenKeyedStore::get` may return an expired-but-not-yet-swept entry. --- devolutions-gateway/src/api/kdc_proxy.rs | 4 +- devolutions-gateway/src/api/rdp.rs | 8 +-- .../src/credential_injection_kdc.rs | 43 ++++++------ devolutions-gateway/src/generic_client.rs | 8 +-- devolutions-gateway/src/lib.rs | 6 +- devolutions-gateway/src/listener.rs | 2 +- devolutions-gateway/src/ngrok.rs | 2 +- devolutions-gateway/src/rd_clean_path.rs | 6 +- devolutions-gateway/src/service.rs | 8 ++- .../src/target_connection_options.rs | 65 +++++++++++++------ devolutions-gateway/src/token_keyed_store.rs | 4 ++ 11 files changed, 90 insertions(+), 66 deletions(-) diff --git a/devolutions-gateway/src/api/kdc_proxy.rs b/devolutions-gateway/src/api/kdc_proxy.rs index 1cf6f6a7d..e111b1626 100644 --- a/devolutions-gateway/src/api/kdc_proxy.rs +++ b/devolutions-gateway/src/api/kdc_proxy.rs @@ -22,7 +22,7 @@ pub fn make_router(state: DgwState) -> Router { async fn kdc_proxy( State(DgwState { conf_handle, - credentials, + credential_injection, provisioning, agent_tunnel_handle, .. @@ -48,7 +48,7 @@ async fn kdc_proxy( KdcDestination::Inject { jti } => { enforce_credential_injection_enabled(jti, conf.debug.enable_unstable)?; - let kdc = credentials + let kdc = credential_injection .kdc_for(&provisioning, jti) .map_err(credential_injection_resolve_error)?; diff --git a/devolutions-gateway/src/api/rdp.rs b/devolutions-gateway/src/api/rdp.rs index 85db29da3..31d304b57 100644 --- a/devolutions-gateway/src/api/rdp.rs +++ b/devolutions-gateway/src/api/rdp.rs @@ -25,7 +25,7 @@ pub async fn handler( subscriber_tx, recordings, shutdown_signal, - credentials, + credential_injection, provisioning, agent_tunnel_handle, .. @@ -47,7 +47,7 @@ pub async fn handler( subscriber_tx, recordings.active_recordings, source_addr, - credentials, + credential_injection, provisioning, agent_tunnel_handle, ) @@ -68,7 +68,7 @@ async fn handle_socket( subscriber_tx: SubscriberSender, active_recordings: Arc, source_addr: SocketAddr, - credentials: crate::credential_injection_kdc::CredentialService, + credential_injection: crate::credential_injection_kdc::CredentialInjectionKdcService, provisioning: crate::provisioning::ProvisioningStore, agent_tunnel_handle: Option>, ) { @@ -87,7 +87,7 @@ async fn handle_socket( sessions, subscriber_tx, &active_recordings, - &credentials, + &credential_injection, &provisioning, agent_tunnel_handle, ) diff --git a/devolutions-gateway/src/credential_injection_kdc.rs b/devolutions-gateway/src/credential_injection_kdc.rs index 34e2c5bf8..9b25fa23f 100644 --- a/devolutions-gateway/src/credential_injection_kdc.rs +++ b/devolutions-gateway/src/credential_injection_kdc.rs @@ -165,7 +165,7 @@ impl CredentialInjectionKdc { } pub(crate) fn krb_kdc(&self) -> Option<&crate::target_addr::TargetAddr> { - self.connection_options.as_ref()?.krb_kdc.as_ref() + self.connection_options.as_ref()?.krb_kdc() } /// Selects the CredSSP acceptor backend Gateway should present to the RDP client. @@ -445,7 +445,7 @@ fn random_32_bytes() -> Vec { /// live here, and the two are keyed by the same JTI but never reach into each other. Resolution /// reads the provisioning store passed in by the caller. #[derive(Debug, Clone)] -pub struct CredentialService { +pub struct CredentialInjectionKdcService { sessions: Arc>>, } @@ -460,13 +460,13 @@ struct CachedSession { session: Arc, } -impl Default for CredentialService { +impl Default for CredentialInjectionKdcService { fn default() -> Self { Self::new() } } -impl CredentialService { +impl CredentialInjectionKdcService { pub fn new() -> Self { Self { sessions: Arc::new(Mutex::new(HashMap::new())), @@ -541,11 +541,8 @@ impl CredentialService { })?; let proxy_username = app_credential_username(&mapping.proxy).to_owned(); - // Atomic get-or-derive under the lock: guarantees a single session wins per JTI even under - // concurrent calls. A cached session is reused only when it was derived for this exact - // provisioning entry — a re-provisioning produces a new entry, so its stale session is - // dropped and re-derived here. Derivation is fast (a few hundred bytes of OsRng), so holding - // the lock across it is acceptable. + // Hold the lock across derive so one session wins per JTI; reuse only if the cached session + // was derived for this exact entry (re-provisioning yields a new entry, forcing a re-derive). let session = { let mut sessions = self.sessions.lock(); let reuse = sessions.get(&jti).and_then(|cached| { @@ -584,7 +581,7 @@ impl CredentialService { } pub struct CleanupTask { - pub service: CredentialService, + pub service: CredentialInjectionKdcService, } #[async_trait] @@ -600,7 +597,7 @@ impl Task for CleanupTask { } #[instrument(skip_all)] -async fn cleanup_task(service: CredentialService, mut shutdown_signal: ShutdownSignal) { +async fn cleanup_task(service: CredentialInjectionKdcService, mut shutdown_signal: ShutdownSignal) { use tokio::time::{Duration, sleep}; const TASK_INTERVAL: Duration = Duration::from_secs(60 * 15); // 15 minutes @@ -719,7 +716,7 @@ mod tests { #[test] fn service_kdc_for_rejects_expired_credential_entry() { - let service = CredentialService::new(); + let service = CredentialInjectionKdcService::new(); let jti = Uuid::new_v4(); // Negative TTL: entry is born already expired. `TokenKeyedStore::get` does not @@ -738,7 +735,7 @@ mod tests { #[test] fn service_kdc_for_returns_same_session_under_concurrent_calls() { - let service = CredentialService::new(); + let service = CredentialInjectionKdcService::new(); let jti = Uuid::new_v4(); let provisioning = provisioned_store(jti, "target", time::Duration::minutes(5)); @@ -758,7 +755,7 @@ mod tests { #[test] fn service_kdc_for_ignores_cached_session_from_a_gone_entry() { - let service = CredentialService::new(); + let service = CredentialInjectionKdcService::new(); let jti = Uuid::new_v4(); // Simulate the race called out by Codex: a previous provisioning's session is still cached, @@ -784,7 +781,7 @@ mod tests { #[test] fn service_kdc_for_rederives_session_after_reprovisioning() { - let service = CredentialService::new(); + let service = CredentialInjectionKdcService::new(); let jti = Uuid::new_v4(); let provisioning = provisioned_store(jti, "target", time::Duration::minutes(5)); @@ -816,7 +813,7 @@ mod tests { #[test] fn service_sweep_orphans_drops_sessions_with_no_credential_entry() { - let service = CredentialService::new(); + let service = CredentialInjectionKdcService::new(); let jti = Uuid::new_v4(); { @@ -889,7 +886,7 @@ mod tests { #[test] fn service_kdc_for_rejects_unknown_jti() { - let service = CredentialService::new(); + let service = CredentialInjectionKdcService::new(); let provisioning = ProvisioningStore::new(); assert!( @@ -903,7 +900,7 @@ mod tests { #[test] fn service_kdc_for_rejects_non_injection_entry() { - let service = CredentialService::new(); + let service = CredentialInjectionKdcService::new(); let provisioning = ProvisioningStore::new(); let jti = Uuid::new_v4(); @@ -922,7 +919,7 @@ mod tests { #[test] fn service_kdc_for_lazily_extracts_target_hostname_from_entry_token() { - let service = CredentialService::new(); + let service = CredentialInjectionKdcService::new(); let jti = Uuid::new_v4(); let provisioning = provisioned_store(jti, "target", time::Duration::minutes(5)); @@ -936,7 +933,7 @@ mod tests { #[test] fn provisioning_and_credential_service_share_target_options() { let provisioning = ProvisioningStore::new(); - let service = CredentialService::new(); + let service = CredentialInjectionKdcService::new(); let jti = Uuid::new_v4(); let token = association_token(jti); let krb_kdc = crate::target_addr::TargetAddr::parse("tcp://kdc.example.invalid:88", Some(88)) @@ -946,9 +943,7 @@ mod tests { .insert( token.clone(), Some(cleartext_mapping_with_target_username("target@example.invalid")), - Some(TargetConnectionOptions { - krb_kdc: Some(krb_kdc.clone()), - }), + Some(TargetConnectionOptions::new(Some(krb_kdc.clone())).expect("supported KDC scheme")), time::Duration::minutes(5), ) .expect("provisioning entry inserts"); @@ -959,7 +954,7 @@ mod tests { .value .connection_options .as_ref() - .and_then(|options| options.krb_kdc.as_ref()), + .and_then(|options| options.krb_kdc()), Some(&krb_kdc) ); diff --git a/devolutions-gateway/src/generic_client.rs b/devolutions-gateway/src/generic_client.rs index c57bbcf76..54f614154 100644 --- a/devolutions-gateway/src/generic_client.rs +++ b/devolutions-gateway/src/generic_client.rs @@ -8,7 +8,7 @@ use tracing::field; use typed_builder::TypedBuilder; use crate::config::Conf; -use crate::credential_injection_kdc::CredentialService; +use crate::credential_injection_kdc::CredentialInjectionKdcService; use crate::provisioning::ProvisioningStore; use crate::proxy::Proxy; use crate::rdp_pcb::{extract_association_claims, read_pcb}; @@ -28,7 +28,7 @@ pub struct GenericClient { sessions: SessionMessageSender, subscriber_tx: SubscriberSender, active_recordings: Arc, - credentials: CredentialService, + credential_injection: CredentialInjectionKdcService, provisioning: ProvisioningStore, #[builder(default)] agent_tunnel_handle: Option>, @@ -53,7 +53,7 @@ where sessions, subscriber_tx, active_recordings, - credentials, + credential_injection, provisioning, agent_tunnel_handle, } = self; @@ -156,7 +156,7 @@ where // lookup by `claims.jti` is the primary path. if is_rdp && let Some(credential_injection_kdc) = - credentials.resolve_injection_kdc(&provisioning, claims.jti, token)? + credential_injection.resolve_injection_kdc(&provisioning, claims.jti, token)? { info!( jti = %credential_injection_kdc.jti(), diff --git a/devolutions-gateway/src/lib.rs b/devolutions-gateway/src/lib.rs index 329a98002..9ef441f70 100644 --- a/devolutions-gateway/src/lib.rs +++ b/devolutions-gateway/src/lib.rs @@ -65,7 +65,7 @@ pub struct DgwState { pub recordings: recording::RecordingMessageSender, pub job_queue_handle: job_queue::JobQueueHandle, pub provisioning: provisioning::ProvisioningStore, - pub credentials: credential_injection_kdc::CredentialService, + pub credential_injection: credential_injection_kdc::CredentialInjectionKdcService, pub monitoring_state: Arc, pub traffic_audit_handle: traffic_audit::TrafficAuditHandle, pub agent_tunnel_handle: Option>, @@ -94,7 +94,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 provisioning = provisioning::ProvisioningStore::new(); - let credentials = credential_injection_kdc::CredentialService::new(); + let credential_injection = credential_injection_kdc::CredentialInjectionKdcService::new(); let monitoring_state = Arc::new(network_monitor::State::new(Arc::new(MockMonitorsCache))?); let state = Self { @@ -108,7 +108,7 @@ impl DgwState { job_queue_handle, traffic_audit_handle, provisioning, - credentials, + credential_injection, monitoring_state, agent_tunnel_handle: None, }; diff --git a/devolutions-gateway/src/listener.rs b/devolutions-gateway/src/listener.rs index 58515e6a1..c1691d580 100644 --- a/devolutions-gateway/src/listener.rs +++ b/devolutions-gateway/src/listener.rs @@ -158,7 +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) - .credentials(state.credentials) + .credential_injection(state.credential_injection) .provisioning(state.provisioning) .agent_tunnel_handle(state.agent_tunnel_handle) .build() diff --git a/devolutions-gateway/src/ngrok.rs b/devolutions-gateway/src/ngrok.rs index e064becc1..e75546c8a 100644 --- a/devolutions-gateway/src/ngrok.rs +++ b/devolutions-gateway/src/ngrok.rs @@ -237,7 +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) - .credentials(state.credentials) + .credential_injection(state.credential_injection) .provisioning(state.provisioning) .agent_tunnel_handle(state.agent_tunnel_handle) .build() diff --git a/devolutions-gateway/src/rd_clean_path.rs b/devolutions-gateway/src/rd_clean_path.rs index 25fa7e47c..0c790a91a 100644 --- a/devolutions-gateway/src/rd_clean_path.rs +++ b/devolutions-gateway/src/rd_clean_path.rs @@ -11,7 +11,7 @@ use tokio::io::{AsyncRead, AsyncReadExt as _, AsyncWrite, AsyncWriteExt as _}; use tracing::field; use crate::config::Conf; -use crate::credential_injection_kdc::{CredentialInjectionKdc, CredentialService}; +use crate::credential_injection_kdc::{CredentialInjectionKdc, CredentialInjectionKdcService}; use crate::provisioning::ProvisioningStore; use crate::proxy::Proxy; use crate::recording::ActiveRecordings; @@ -521,7 +521,7 @@ pub async fn handle( sessions: SessionMessageSender, subscriber_tx: SubscriberSender, active_recordings: &ActiveRecordings, - credentials: &CredentialService, + credential_injection: &CredentialInjectionKdcService, provisioning: &ProvisioningStore, agent_tunnel_handle: Option>, ) -> anyhow::Result<()> { @@ -543,7 +543,7 @@ pub async fn handle( // proxy-based credential injection mode. Otherwise, we continue the usual // clean path procedure. The provisioning store is keyed on the association token's JTI. if let Some(jti) = crate::token::extract_jti(token).ok() - && let Some(credential_injection_kdc) = credentials.resolve_injection_kdc(provisioning, jti, token)? + && let Some(credential_injection_kdc) = credential_injection.resolve_injection_kdc(provisioning, jti, token)? { debug!( jti = %credential_injection_kdc.jti(), diff --git a/devolutions-gateway/src/service.rs b/devolutions-gateway/src/service.rs index 6b6b2e12f..7288564b9 100644 --- a/devolutions-gateway/src/service.rs +++ b/devolutions-gateway/src/service.rs @@ -268,7 +268,7 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { .context("failed to initialize traffic audit manager")?; let provisioning = devolutions_gateway::provisioning::ProvisioningStore::new(); - let credentials = devolutions_gateway::credential_injection_kdc::CredentialService::new(); + let credential_injection = devolutions_gateway::credential_injection_kdc::CredentialInjectionKdcService::new(); let filesystem_monitor_config_cache = devolutions_gateway::api::monitoring::FilesystemConfigCache::new( config::get_data_dir().join("monitors_cache.json"), @@ -317,7 +317,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(), provisioning: provisioning.clone(), - credentials: credentials.clone(), + credential_injection: credential_injection.clone(), monitoring_state, traffic_audit_handle: traffic_audit_task.handle(), agent_tunnel_handle, @@ -354,7 +354,9 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { tasks.register(provisioning.cleanup_task()); - tasks.register(devolutions_gateway::credential_injection_kdc::CleanupTask { service: credentials }); + tasks.register(devolutions_gateway::credential_injection_kdc::CleanupTask { + service: credential_injection, + }); tasks.register(devolutions_log::LogDeleterTask::::new( conf.log_file.clone(), diff --git a/devolutions-gateway/src/target_connection_options.rs b/devolutions-gateway/src/target_connection_options.rs index c03a27df2..be1498594 100644 --- a/devolutions-gateway/src/target_connection_options.rs +++ b/devolutions-gateway/src/target_connection_options.rs @@ -1,33 +1,50 @@ -use serde::{Deserialize as _, de}; - use crate::target_addr::TargetAddr; -#[derive(Debug, Clone, Default, Deserialize)] +/// How the Gateway's internal client should reach the target, provisioned alongside the credentials. +/// +/// The KDC scheme is validated at construction, so the rest of the code can trust `krb_kdc` without +/// re-checking it. +#[derive(Debug, Clone, Deserialize)] +#[serde(try_from = "RawTargetConnectionOptions")] pub(crate) struct TargetConnectionOptions { - #[serde(default, deserialize_with = "deserialize_optional_krb_kdc")] - pub(crate) krb_kdc: Option, + krb_kdc: Option, +} + +impl TargetConnectionOptions { + pub(crate) fn new(krb_kdc: Option) -> Result { + if let Some(krb_kdc) = &krb_kdc + && !is_supported_krb_kdc_scheme(krb_kdc.scheme()) + { + return Err(UnsupportedKdcScheme(krb_kdc.scheme().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") } -fn deserialize_optional_krb_kdc<'de, D>(deserializer: D) -> Result, D::Error> -where - D: serde::Deserializer<'de>, -{ - let krb_kdc = Option::::deserialize(deserializer)?; - - if let Some(krb_kdc) = &krb_kdc - && !is_supported_krb_kdc_scheme(krb_kdc.scheme()) - { - return Err(de::Error::custom(format!( - "unsupported KDC protocol: {}", - krb_kdc.scheme() - ))); - } +#[derive(Debug, thiserror::Error)] +#[error("unsupported KDC protocol: {0}")] +pub(crate) struct UnsupportedKdcScheme(String); - Ok(krb_kdc) +#[derive(Deserialize)] +struct RawTargetConnectionOptions { + #[serde(default)] + krb_kdc: Option, +} + +impl TryFrom for TargetConnectionOptions { + type Error = UnsupportedKdcScheme; + + fn try_from(raw: RawTargetConnectionOptions) -> Result { + Self::new(raw.krb_kdc) + } } #[cfg(test)] @@ -43,7 +60,7 @@ mod tests { .expect("supported KDC protocol should deserialize"); assert_eq!( - options.krb_kdc.expect("KDC address should be present").as_str(), + options.krb_kdc().expect("KDC address should be present").as_str(), krb_kdc ); } @@ -58,4 +75,10 @@ mod tests { assert!(error.to_string().contains("unsupported KDC protocol: https")); } + + #[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/src/token_keyed_store.rs b/devolutions-gateway/src/token_keyed_store.rs index 5568d50d4..d010c52b4 100644 --- a/devolutions-gateway/src/token_keyed_store.rs +++ b/devolutions-gateway/src/token_keyed_store.rs @@ -67,6 +67,10 @@ impl TokenKeyedStore { Ok(previous) } + /// Look up an entry by its token JTI. + /// + /// This may return an entry that is already past its `expires_at`: eviction is asynchronous + /// (see [`CleanupTask`]), so a caller that cares about freshness must check `expires_at` itself. pub(crate) fn get(&self, jti: Uuid) -> Option> { self.0.lock().entries.get(&jti).map(Arc::clone) } From f7c96adfa276cefbffa16b6e68fc6668d1f7a7b4 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Fri, 24 Jul 2026 17:10:32 -0400 Subject: [PATCH 05/11] fix(dgw): validate the provisioned KDC address at construction Reject an unsupported scheme, a missing host (e.g. `tcp://:88`), or an unparseable URL when a `TargetConnectionOptions` is built, so provisioning fails fast at preflight instead of at RDP session start. --- .../src/target_connection_options.rs | 44 ++++++++++++++----- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/devolutions-gateway/src/target_connection_options.rs b/devolutions-gateway/src/target_connection_options.rs index be1498594..7f5fea145 100644 --- a/devolutions-gateway/src/target_connection_options.rs +++ b/devolutions-gateway/src/target_connection_options.rs @@ -2,8 +2,9 @@ use crate::target_addr::TargetAddr; /// How the Gateway's internal client should reach the target, provisioned alongside the credentials. /// -/// The KDC scheme is validated at construction, so the rest of the code can trust `krb_kdc` without -/// re-checking it. +/// 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 { @@ -11,11 +12,17 @@ pub(crate) struct TargetConnectionOptions { } impl TargetConnectionOptions { - pub(crate) fn new(krb_kdc: Option) -> Result { - if let Some(krb_kdc) = &krb_kdc - && !is_supported_krb_kdc_scheme(krb_kdc.scheme()) - { - return Err(UnsupportedKdcScheme(krb_kdc.scheme().to_owned())); + 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 }) } @@ -30,8 +37,14 @@ pub(crate) fn is_supported_krb_kdc_scheme(scheme: &str) -> bool { } #[derive(Debug, thiserror::Error)] -#[error("unsupported KDC protocol: {0}")] -pub(crate) struct UnsupportedKdcScheme(String); +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 { @@ -40,7 +53,7 @@ struct RawTargetConnectionOptions { } impl TryFrom for TargetConnectionOptions { - type Error = UnsupportedKdcScheme; + type Error = InvalidKdcAddr; fn try_from(raw: RawTargetConnectionOptions) -> Result { Self::new(raw.krb_kdc) @@ -76,6 +89,17 @@ mod tests { 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"); From a3acc9244f8646c7dc022aee9a0134f45a4a9697 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Mon, 27 Jul 2026 15:32:26 -0400 Subject: [PATCH 06/11] refactor(dgw): store provisioning until TTL and index synthetic KDCs in a registry Rework proxy-based RDP credential injection: the provisioning store becomes a plain TTL-scoped lookup and per-session Kerberos material lives in a global registry, replacing one-shot consumption and the weak-ref session cache. - ProvisioningStore::get is a read-only clone-out; entries live until their TTL. A JTI names a session and the token layer allows several connections per session (reconnects), so consuming on first use dropped legitimate reconnects to plain RDP. Expired entries read as absent. - Store a typed ProvisionedConnection instead of the raw association token. - Model the CredSSP package as a CredentialInjection::{Kerberos,Ntlm} sum type decided at construction. A domain-qualified Kerberos target with no provisioned krb_kdc is a hard error, never an NTLM fallback. - SyntheticKdcRegistry indexes live sessions by JTI. Registration is RAII and replaces on a same-JTI reconnect, retracting only its own entry on drop. - Remove the single-use generic TokenKeyedStore wrapper. - provision-token stays for backwards compatibility: validate the token and acknowledge it, storing nothing. --- devolutions-gateway/src/api/kdc_proxy.rs | 39 +- devolutions-gateway/src/api/preflight.rs | 124 +- devolutions-gateway/src/api/rdp.rs | 8 +- devolutions-gateway/src/credential/mod.rs | 14 +- .../src/credential_injection_kdc.rs | 1055 ++++++----------- devolutions-gateway/src/generic_client.rs | 35 +- devolutions-gateway/src/lib.rs | 7 +- devolutions-gateway/src/listener.rs | 2 +- devolutions-gateway/src/ngrok.rs | 2 +- devolutions-gateway/src/provisioning.rs | 178 ++- devolutions-gateway/src/rd_clean_path.rs | 180 +-- devolutions-gateway/src/rdp_proxy.rs | 292 +++-- devolutions-gateway/src/service.rs | 8 +- devolutions-gateway/src/token.rs | 20 +- devolutions-gateway/src/token_keyed_store.rs | 164 --- devolutions-gateway/tests/preflight.rs | 46 +- 16 files changed, 832 insertions(+), 1342 deletions(-) delete mode 100644 devolutions-gateway/src/token_keyed_store.rs diff --git a/devolutions-gateway/src/api/kdc_proxy.rs b/devolutions-gateway/src/api/kdc_proxy.rs index e111b1626..1b3d474e9 100644 --- a/devolutions-gateway/src/api/kdc_proxy.rs +++ b/devolutions-gateway/src/api/kdc_proxy.rs @@ -5,10 +5,7 @@ use picky_krb::messages::KdcProxyMessage; use uuid::Uuid; use crate::DgwState; -use crate::credential_injection_kdc::{ - CredentialInjectionKdcInterception, CredentialInjectionKdcRequest, CredentialInjectionKdcResolveError, - kdc_proxy_message_realm, -}; +use crate::credential_injection_kdc::{SyntheticKdcInterception, kdc_proxy_message_realm}; use crate::extract::KdcToken; use crate::http::HttpError; use crate::kdc_connector::KdcConnector; @@ -22,8 +19,7 @@ pub fn make_router(state: DgwState) -> Router { async fn kdc_proxy( State(DgwState { conf_handle, - credential_injection, - provisioning, + synthetic_kdc_registry, agent_tunnel_handle, .. }): State, @@ -48,9 +44,9 @@ async fn kdc_proxy( KdcDestination::Inject { jti } => { enforce_credential_injection_enabled(jti, conf.debug.enable_unstable)?; - let kdc = credential_injection - .kdc_for(&provisioning, jti) - .map_err(credential_injection_resolve_error)?; + let kdc = synthetic_kdc_registry + .get(jti) + .ok_or_else(|| HttpError::bad_request().msg("credential-injection state is not available"))?; debug!( jti = %kdc.jti(), @@ -58,16 +54,14 @@ async fn kdc_proxy( ); match kdc - .handle_kdc_proxy_request(CredentialInjectionKdcRequest::from_token(kdc_proxy_message)) + .handle_kdc_proxy_message(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 => { + SyntheticKdcInterception::Intercepted(reply) => Ok(reply), + SyntheticKdcInterception::NotInjectionRealm(mismatch) => Err(HttpError::bad_request() + .with_msg("requested domain is not allowed") + .err()(mismatch)), + SyntheticKdcInterception::NotInjectionRequest => { Err(HttpError::internal().msg("credential-injection KDC did not handle the KDC proxy request")) } } @@ -95,17 +89,6 @@ async fn kdc_proxy( } } -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), - } -} - // Forwards the request to the real KDC indicated by the token (or by the debug override) and // returns the response wrapped as a `KdcProxyMessage`. // diff --git a/devolutions-gateway/src/api/preflight.rs b/devolutions-gateway/src/api/preflight.rs index 8cc920d5f..8de0f62e8 100644 --- a/devolutions-gateway/src/api/preflight.rs +++ b/devolutions-gateway/src/api/preflight.rs @@ -13,7 +13,7 @@ use crate::DgwState; use crate::config::Conf; use crate::extract::PreflightScope; use crate::http::HttpError; -use crate::provisioning::{InsertError, ProvisioningStore}; +use crate::provisioning::ProvisioningStore; use crate::session::SessionMessageSender; const OP_GET_VERSION: &str = "get-version"; @@ -45,7 +45,7 @@ struct ProvisionTokenParams { struct ProvisionCredentialsParams { token: String, #[serde(flatten)] - mapping: crate::credential::CleartextAppCredentialMapping, + credentials: crate::credential::CleartextAppCredentials, connection_options: Option, time_to_live: Option, } @@ -309,72 +309,58 @@ 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, connection_options) = if operation.kind.as_str() == OP_PROVISION_TOKEN { - let ProvisionTokenParams { token, time_to_live } = - from_params(operation.params).map_err(PreflightError::invalid_params)?; - (token, time_to_live, None, None) - } else { - let ProvisionCredentialsParams { - token, - mapping, - connection_options, - time_to_live, - } = from_params(operation.params).map_err(PreflightError::invalid_params)?; - (token, time_to_live, Some(mapping), connection_options) - }; - - 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})" - ), - }); - } + OP_PROVISION_TOKEN => { + let ProvisionTokenParams { token, time_to_live } = + from_params(operation.params).map_err(PreflightError::invalid_params)?; + validate_time_to_live(time_to_live)?; + + // provision-token stores nothing: the current model has no credential-less entry, and a + // token-only entry never affected connection handling. It is kept only so older callers + // that still send it get a well-formed token validated and acknowledged. + crate::token::extract_jti(&token).map_err(|error| { + PreflightError::new(PreflightAlertStatus::InvalidParams, format!("invalid token: {error:#}")) + })?; - // 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 provisioning 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:#}"), - ) - })?; - } + outputs.push(PreflightOutput { + operation_id: operation.id, + kind: PreflightOutputKind::Ack, + }); + } + OP_PROVISION_CREDENTIALS => { + let ProvisionCredentialsParams { + token, + credentials, + 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)?; + + let token_data = 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 = provisioning - .insert(token, mapping, connection_options, time_to_live) + let replaced = provisioning + .insert(token_data, credentials, connection_options, 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( + .map_err(|_| { + PreflightError::new( PreflightAlertStatus::InternalServerError, "an internal error occurred".to_owned(), - ), + ) })?; - // A replaced entry produces a new provisioning entry, so the credential-injection KDC - // service re-derives its session on the next lookup — no explicit invalidation here. - if previous_entry.is_some() { + if replaced { outputs.push(PreflightOutput { operation_id: operation.id, kind: PreflightOutputKind::Alert { @@ -427,6 +413,22 @@ fn from_params(params: serde_json::Map) -> 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) +} + /// Redacts sensitive fields in JSON values. /// /// This function recursively traverses a JSON value and replaces any field diff --git a/devolutions-gateway/src/api/rdp.rs b/devolutions-gateway/src/api/rdp.rs index 31d304b57..7dfdbde2f 100644 --- a/devolutions-gateway/src/api/rdp.rs +++ b/devolutions-gateway/src/api/rdp.rs @@ -25,7 +25,7 @@ pub async fn handler( subscriber_tx, recordings, shutdown_signal, - credential_injection, + synthetic_kdc_registry, provisioning, agent_tunnel_handle, .. @@ -47,7 +47,7 @@ pub async fn handler( subscriber_tx, recordings.active_recordings, source_addr, - credential_injection, + synthetic_kdc_registry, provisioning, agent_tunnel_handle, ) @@ -68,7 +68,7 @@ async fn handle_socket( subscriber_tx: SubscriberSender, active_recordings: Arc, source_addr: SocketAddr, - credential_injection: crate::credential_injection_kdc::CredentialInjectionKdcService, + synthetic_kdc_registry: crate::credential_injection_kdc::SyntheticKdcRegistry, provisioning: crate::provisioning::ProvisioningStore, agent_tunnel_handle: Option>, ) { @@ -87,7 +87,7 @@ async fn handle_socket( sessions, subscriber_tx, &active_recordings, - &credential_injection, + &synthetic_kdc_registry, &provisioning, agent_tunnel_handle, ) diff --git a/devolutions-gateway/src/credential/mod.rs b/devolutions-gateway/src/credential/mod.rs index d83cb297e..598be6b45 100644 --- a/devolutions-gateway/src/credential/mod.rs +++ b/devolutions-gateway/src/credential/mod.rs @@ -30,9 +30,9 @@ impl AppCredential { } } -/// Application protocol level credential mapping +/// Application protocol level credentials. #[derive(Debug, Clone)] -pub struct AppCredentialMapping { +pub struct AppCredentials { pub proxy: AppCredential, pub target: AppCredential, } @@ -64,18 +64,18 @@ impl CleartextAppCredential { } } -/// Cleartext credential mapping received from the API, used for deserialization only. +/// Cleartext credentials received from the API, used for deserialization only. #[derive(Debug, Deserialize)] -pub struct CleartextAppCredentialMapping { +pub struct CleartextAppCredentials { #[serde(rename = "proxy_credential")] pub proxy: CleartextAppCredential, #[serde(rename = "target_credential")] pub target: CleartextAppCredential, } -impl CleartextAppCredentialMapping { - pub(crate) fn encrypt(self) -> anyhow::Result { - Ok(AppCredentialMapping { +impl CleartextAppCredentials { + pub(crate) fn encrypt(self) -> anyhow::Result { + Ok(AppCredentials { proxy: self.proxy.encrypt()?, target: self.target.encrypt()?, }) diff --git a/devolutions-gateway/src/credential_injection_kdc.rs b/devolutions-gateway/src/credential_injection_kdc.rs index 9b25fa23f..a09a917a3 100644 --- a/devolutions-gateway/src/credential_injection_kdc.rs +++ b/devolutions-gateway/src/credential_injection_kdc.rs @@ -1,7 +1,7 @@ //! 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 +//! per-session fake-KDC material, the registry of live sessions, 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. @@ -13,22 +13,19 @@ 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}; -use crate::provisioning::{ArcProvisioningEntry, ProvisioningStore, WeakProvisioningEntry}; -use crate::target_connection_options::TargetConnectionOptions; +use crate::credential::{AppCredential, AppCredentials}; +use crate::provisioning::ProvisionedConnection; +use crate::target_addr::TargetAddr; // 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 @@ -38,37 +35,44 @@ use crate::target_connection_options::TargetConnectionOptions; // 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 { +pub(crate) enum CredentialInjection { + Kerberos(KerberosCredentialInjection), + Ntlm(NtlmCredentialInjection), +} + +pub(crate) struct KerberosCredentialInjection { + jti: Uuid, + credentials: AppCredentials, + target_kdc: TargetAddr, + synthetic_kdc: Arc, +} + +pub(crate) struct NtlmCredentialInjection { + jti: Uuid, + credentials: AppCredentials, +} + +pub(crate) struct SyntheticKdcSession { jti: Uuid, - credential_mapping: AppCredentialMapping, - connection_options: Option, target_hostname: String, - session: Arc, + realm: String, + acceptor_principal_name: String, + acceptor_password: SecretString, + acceptor_long_term_key: SecretBox>, // 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, } -#[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("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, - #[source] - source: anyhow::Error, - }, +impl fmt::Debug for SyntheticKdcSession { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("SyntheticKdcSession") + .field("jti", &self.jti) + .field("target_hostname", &self.target_hostname) + .field("realm", &self.realm) + .field("kdc_config", &"") + .finish() + } } #[derive(Debug, Clone, PartialEq, Eq)] @@ -86,109 +90,176 @@ impl fmt::Display for RealmMismatch { impl std::error::Error for RealmMismatch {} #[derive(Debug)] -pub(crate) enum CredentialInjectionKdcInterception { +pub(crate) enum SyntheticKdcInterception { 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 { +impl fmt::Debug for CredentialInjection { 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() + match self { + Self::Kerberos(injection) => f + .debug_struct("CredentialInjection::Kerberos") + .field("jti", &injection.jti) + .field("target_hostname", &injection.synthetic_kdc.target_hostname) + .field("realm", &injection.synthetic_kdc.realm) + .field("kdc_config", &"") + .finish(), + Self::Ntlm(injection) => f + .debug_struct("CredentialInjection::Ntlm") + .field("jti", &injection.jti) + .finish(), + } } } -impl CredentialInjectionKdc { - fn from_parts( +impl CredentialInjection { + pub(crate) fn from_provisioned( jti: Uuid, - provisioning_entry: ArcProvisioningEntry, - target_hostname: String, - session: Arc, + provisioned_connection: ProvisionedConnection, + target_hostname: &str, + kerberos_enabled: bool, ) -> anyhow::Result { - let mapping = provisioning_entry - .value - .mapping - .as_ref() - .context("provisioning entry has no credential-injection mapping")?; anyhow::ensure!( - jti == session.jti, - "provisioning entry JTI does not match credential-injection KDC session JTI", + target_hostname.eq_ignore_ascii_case(&provisioned_connection.target_hostname), + "credential-injection target mismatch" ); - let kdc_config = build_kdc_config(&session, &mapping.proxy)?; - - Ok(Self { - jti, - credential_mapping: mapping.clone(), - connection_options: provisioning_entry.value.connection_options.clone(), + let ProvisionedConnection { + credentials, + connection_options, target_hostname, - session, - kdc_config, - }) + expires_at: _, + } = provisioned_connection; + + let uses_kerberos = if kerberos_enabled { + let target_username = sspi::Username::parse(app_credential_username(&credentials.target)) + .context("invalid target credential username")?; + target_username.domain_name().is_some() + } else { + false + }; + + if uses_kerberos { + let target_kdc = connection_options + .as_ref() + .and_then(crate::target_connection_options::TargetConnectionOptions::krb_kdc) + .cloned() + .context("Kerberos credential injection requires target connection option krb_kdc")?; + let synthetic_kdc = SyntheticKdcSession::new(jti, target_hostname, &credentials.proxy)?; + + Ok(Self::Kerberos(KerberosCredentialInjection { + jti, + credentials, + target_kdc, + synthetic_kdc: Arc::new(synthetic_kdc), + })) + } else { + Ok(Self::Ntlm(NtlmCredentialInjection { jti, credentials })) + } } pub(crate) fn jti(&self) -> Uuid { - self.jti + match self { + Self::Kerberos(injection) => injection.jti, + Self::Ntlm(injection) => injection.jti, + } } pub(crate) fn proxy_credential(&self) -> &AppCredential { - &self.credential_mapping.proxy + match self { + Self::Kerberos(injection) => &injection.credentials.proxy, + Self::Ntlm(injection) => &injection.credentials.proxy, + } } pub(crate) fn target_credential(&self) -> &AppCredential { - &self.credential_mapping.target + match self { + Self::Kerberos(injection) => &injection.credentials.target, + Self::Ntlm(injection) => &injection.credentials.target, + } + } + + pub(crate) fn kerberos_configs( + &self, + client_addr: SocketAddr, + gateway_hostname: &str, + ) -> anyhow::Result> { + match self { + Self::Kerberos(injection) => Ok(Some(( + injection.synthetic_kdc.server_kerberos_config(client_addr)?, + ironrdp_connector::credssp::KerberosConfig { + kdc_proxy_url: Some( + Url::try_from(&injection.target_kdc).context("convert target KDC address to URL")?, + ), + hostname: gateway_hostname.to_owned(), + }, + ))), + Self::Ntlm(_) => Ok(None), + } } - pub(crate) fn krb_kdc(&self) -> Option<&crate::target_addr::TargetAddr> { - self.connection_options.as_ref()?.krb_kdc() + pub(crate) fn register_synthetic_kdc(&self, registry: &SyntheticKdcRegistry) -> Option { + match self { + Self::Kerberos(injection) => Some(registry.register(Arc::clone(&injection.synthetic_kdc))), + Self::Ntlm(_) => None, + } } - /// 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(app_credential_username(self.target_credential())) - .context("invalid target credential username")?; - - if target_username.domain_name().is_some() { - Ok(CredentialInjectionClientAcceptorProtocol::Kerberos) - } else { - Ok(CredentialInjectionClientAcceptorProtocol::Ntlm) + pub(crate) fn intercept_network_request( + &self, + request: &NetworkRequest, + ) -> anyhow::Result { + match self { + Self::Kerberos(injection) => injection.synthetic_kdc.intercept_network_request(request), + Self::Ntlm(_) => Ok(SyntheticKdcInterception::NotInjectionRequest), } } +} + +impl SyntheticKdcSession { + fn new(jti: Uuid, target_hostname: String, proxy_credential: &AppCredential) -> anyhow::Result { + let proxy_username = app_credential_username(proxy_credential); + let realm = proxy_username + .split_once('@') + .map(|(_, realm)| realm) + .filter(|realm| !realm.is_empty()) + .map(str::to_owned) + .unwrap_or_else(|| synthetic_realm(jti)); + let krbtgt_key = SecretBox::new(Box::new(random_32_bytes())); + let acceptor_principal_name = "jet".to_owned(); + let acceptor_password = SecretString::from(hex::encode(random_32_bytes())); + let acceptor_long_term_key = SecretBox::new(Box::new(random_32_bytes())); + let kdc_config = build_kdc_config( + &realm, + &krbtgt_key, + &acceptor_principal_name, + &acceptor_password, + &acceptor_long_term_key, + proxy_credential, + )?; - pub(crate) fn server_kerberos_config(&self, client_addr: SocketAddr) -> anyhow::Result { + Ok(Self { + jti, + target_hostname, + realm, + acceptor_principal_name, + acceptor_password, + acceptor_long_term_key, + kdc_config, + }) + } + + pub(crate) fn jti(&self) -> Uuid { + self.jti + } + + 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(), + &self.acceptor_principal_name, + &self.realm, + self.acceptor_password.expose_secret(), )); let kdc_url = self.in_process_kdc_url()?; @@ -205,9 +276,7 @@ impl CredentialInjectionKdc { &["TERMSRV", &self.target_hostname], Some(user), Duration::from_secs(300), - Some(sspi::Secret::new( - self.session.acceptor.long_term_key.expose_secret().clone(), - )), + Some(sspi::Secret::new(self.acceptor_long_term_key.expose_secret().clone())), )?, }) } @@ -215,9 +284,9 @@ impl CredentialInjectionKdc { pub(crate) fn intercept_network_request( &self, request: &NetworkRequest, - ) -> anyhow::Result { + ) -> anyhow::Result { if request.url.host_str() != Some(IN_PROCESS_KDC_HOST) { - return Ok(CredentialInjectionKdcInterception::NotInjectionRequest); + return Ok(SyntheticKdcInterception::NotInjectionRequest); } let url_jti = request @@ -227,45 +296,45 @@ impl CredentialInjectionKdc { .parse::() .context("malformed in-process KDC URL")?; anyhow::ensure!( - url_jti == self.jti, + url_jti == self.jti(), "in-process KDC URL JTI does not match current CredSSP session", ); debug!( - jti = %self.jti, + 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)) + self.handle_kdc_proxy_message(kdc_message) } - pub(crate) fn handle_kdc_proxy_request( + pub(crate) fn handle_kdc_proxy_message( &self, - request: CredentialInjectionKdcRequest, - ) -> anyhow::Result { - let request_realm = self.resolve_message_realm(&request.message); + message: KdcProxyMessage, + ) -> anyhow::Result { + let request_realm = self.resolve_message_realm(&message); debug!( - jti = %self.jti, + 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)); + if let Some(mismatch) = realm_mismatch(&self.realm, &request_realm) { + return Ok(SyntheticKdcInterception::NotInjectionRealm(mismatch)); } - let reply = self.handle_message(request.message)?; - Ok(CredentialInjectionKdcInterception::Intercepted(reply)) + let reply = self.handle_message(message)?; + Ok(SyntheticKdcInterception::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") + 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()) + kdc_proxy_message_realm(kdc_proxy_message).unwrap_or_else(|| self.realm.clone()) } fn handle_message(&self, kdc_proxy_message: KdcProxyMessage) -> anyhow::Result> { @@ -302,94 +371,19 @@ fn realm_mismatch(expected: &str, actual: &str) -> Option { }) } -/// 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. -struct CredentialInjectionKdcSession { - jti: Uuid, - 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. -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, + realm: &str, + krbtgt_key: &SecretBox>, + acceptor_principal_name: &str, + acceptor_password: &SecretString, + acceptor_long_term_key: &SecretBox>, 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_principal_name = principal_for_realm(acceptor_principal_name, realm); - let acceptor_password = session.acceptor.password.expose_secret().to_owned(); + let acceptor_password = acceptor_password.expose_secret().to_owned(); Ok(kdc::config::KerberosServer { realm: realm.to_owned(), users: vec![ @@ -405,8 +399,8 @@ fn build_kdc_config( }, ], 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()), + krbtgt_key: krbtgt_key.expose_secret().clone(), + ticket_decryption_key: Some(acceptor_long_term_key.expose_secret().clone()), service_user: Some(kdc::config::DomainUser { username: acceptor_principal_name.clone(), password: acceptor_password, @@ -438,199 +432,75 @@ fn random_32_bytes() -> Vec { bytes } -/// Owns the Kerberos sessions used by proxy-based credential injection, keyed by association-token -/// JTI. -/// -/// This is deliberately separate from the [`ProvisioningStore`]: credentials live there, sessions -/// live here, and the two are keyed by the same JTI but never reach into each other. Resolution -/// reads the provisioning store passed in by the caller. +/// Indexes Kerberos sessions created by active RDP credential-injection connections. #[derive(Debug, Clone)] -pub struct CredentialInjectionKdcService { - sessions: Arc>>, +pub struct SyntheticKdcRegistry { + sessions: Arc>>>, } -/// A cached Kerberos session tagged with the provisioning entry it was derived for. -/// -/// The weak reference is how we notice re-provisioning without the two stores talking to each other: -/// a fresh provisioning under the same JTI produces a new entry, so the cached session stops matching -/// and gets re-derived on the next lookup. -#[derive(Debug)] -struct CachedSession { - entry: WeakProvisioningEntry, - session: Arc, +#[must_use = "dropping the registration unpublishes the synthetic KDC"] +pub(crate) struct SyntheticKdcRegistration { + jti: Uuid, + session: Arc, + registry: SyntheticKdcRegistry, } -impl Default for CredentialInjectionKdcService { +impl Drop for SyntheticKdcRegistration { + fn drop(&mut self) { + // Only retract our own publication: a superseding reconnect may already own this JTI. + let mut sessions = self.registry.sessions.lock(); + if sessions + .get(&self.jti) + .is_some_and(|current| Arc::ptr_eq(current, &self.session)) + { + sessions.remove(&self.jti); + } + } +} + +impl Default for SyntheticKdcRegistry { fn default() -> Self { Self::new() } } -impl CredentialInjectionKdcService { +impl SyntheticKdcRegistry { pub fn new() -> Self { Self { sessions: Arc::new(Mutex::new(HashMap::new())), } } - /// 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, - provisioning: &ProvisioningStore, - jti: Uuid, - ) -> Result { - let entry = provisioning.get(jti).ok_or_else(|| { - warn!(%jti, "KDC token references missing credential-injection state"); - CredentialInjectionKdcResolveError::MissingCredential { jti } - })?; - self.kdc_from_entry(entry, jti) - } - - /// Look up the credential-injection KDC for an association token, if one was provisioned. - /// - /// Returns `Ok(None)` when the token was not provisioned for credential injection: either no - /// entry exists, or it is a provision-token entry carrying no credential mapping. - pub(crate) fn resolve_injection_kdc( - &self, - provisioning: &ProvisioningStore, - jti: Uuid, - token: &str, - ) -> anyhow::Result> { - let Some(entry) = provisioning.get(jti) else { - return Ok(None); - }; - if entry.value.mapping.is_none() { - return Ok(None); - } - - anyhow::ensure!(token == entry.token, "token mismatch"); - self.kdc_from_entry(entry, jti).map(Some).map_err(Into::into) - } - - fn kdc_from_entry( - &self, - entry: ArcProvisioningEntry, - jti: Uuid, - ) -> Result { - // `TokenKeyedStore::get` does not enforce expiry — entries are evicted asynchronously by the - // provisioning cleanup task. Treat a stale entry as already gone so we never build a KDC - // against expired credentials. - if time::OffsetDateTime::now_utc() >= entry.expires_at { - warn!(%jti, "KDC token references expired credential-injection state"); - self.sessions.lock().remove(&jti); - return Err(CredentialInjectionKdcResolveError::ExpiredCredential { jti }); + pub(crate) fn register(&self, session: Arc) -> SyntheticKdcRegistration { + let jti = session.jti(); + // Replace rather than reject: a fast RDP reconnect can register before the previous + // connection's registration has dropped. The newest connection owns the JTI. + self.sessions.lock().insert(jti, Arc::clone(&session)); + SyntheticKdcRegistration { + jti, + session, + registry: self.clone(), } - - let mapping = entry.value.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(&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(); - // Hold the lock across derive so one session wins per JTI; reuse only if the cached session - // was derived for this exact entry (re-provisioning yields a new entry, forcing a re-derive). - let session = { - let mut sessions = self.sessions.lock(); - let reuse = sessions.get(&jti).and_then(|cached| { - cached - .entry - .upgrade() - .filter(|cached_entry| Arc::ptr_eq(cached_entry, &entry)) - .map(|_| Arc::clone(&cached.session)) - }); - match reuse { - Some(session) => session, - None => { - let session = Arc::new(derive_credential_injection_kdc_session(&proxy_username, jti)); - sessions.insert( - jti, - CachedSession { - entry: Arc::downgrade(&entry), - session: Arc::clone(&session), - }, - ); - session - } - } - }; - - CredentialInjectionKdc::from_parts(jti, entry, target_hostname, session) - .map_err(|source| CredentialInjectionKdcResolveError::BuildKdcConfig { jti, source }) } - /// Drop cached sessions whose provisioning entry is gone (evicted or expired). - fn sweep_orphans(&self) { - self.sessions - .lock() - .retain(|_, cached| cached.entry.upgrade().is_some()); + pub(crate) fn get(&self, jti: Uuid) -> Option> { + self.sessions.lock().get(&jti).cloned() } } -pub struct CleanupTask { - pub service: CredentialInjectionKdcService, -} - -#[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.service, shutdown_signal).await; - Ok(()) - } -} - -#[instrument(skip_all)] -async fn cleanup_task(service: CredentialInjectionKdcService, 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; - } - } - - service.sweep_orphans(); - } - - debug!("Task terminated"); -} - #[cfg(test)] mod tests { - use base64::Engine as _; use ironrdp_connector::sspi::network_client::NetworkProtocol; use secrecy::SecretString; use super::*; - use crate::credential::{CleartextAppCredential, CleartextAppCredentialMapping}; + use crate::credential::{CleartextAppCredential, CleartextAppCredentials}; + use crate::target_connection_options::TargetConnectionOptions; - fn cleartext_mapping_with_target_username(target_username: &str) -> CleartextAppCredentialMapping { - CleartextAppCredentialMapping { + fn app_credentials(proxy_username: &str, target_username: &str) -> AppCredentials { + CleartextAppCredentials { proxy: CleartextAppCredential::UsernamePassword { - username: "proxy@example.invalid".to_owned(), + username: proxy_username.to_owned(), password: SecretString::from("pwd"), }, target: CleartextAppCredential::UsernamePassword { @@ -638,58 +508,34 @@ mod tests { password: SecretString::from("pwd"), }, } + .encrypt() + .expect("credentials encrypt") + } + + fn provisioned( + target_username: &str, + krb_kdc: Option, + time_to_live: time::Duration, + ) -> ProvisionedConnection { + ProvisionedConnection { + credentials: app_credentials("proxy@example.invalid", target_username), + connection_options: krb_kdc.map(|kdc| TargetConnectionOptions::new(Some(kdc)).expect("connection options")), + target_hostname: "target.example".to_owned(), + expires_at: time::OffsetDateTime::now_utc() + time_to_live, + } } - 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 provisioned_store(jti: Uuid, target_username: &str, time_to_live: time::Duration) -> ProvisioningStore { - let provisioning = ProvisioningStore::new(); - provisioning - .insert( - association_token(jti), - Some(cleartext_mapping_with_target_username(target_username)), - None, - time_to_live, - ) - .expect("credential entry inserts"); - provisioning - } - - fn dummy_entry_with_target_username(jti: Uuid, target_username: &str) -> ArcProvisioningEntry { - provisioned_store(jti, target_username, time::Duration::minutes(5)) - .get(jti) - .expect("provisioning entry is indexed by JTI") - } - - fn dummy_entry(jti: Uuid) -> ArcProvisioningEntry { - 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(jti, entry, "target.example".to_owned(), session) - .expect("valid credential-injection KDC") + fn target_kdc() -> TargetAddr { + TargetAddr::parse("tcp://kdc.example.invalid:88", Some(88)).expect("KDC address parses") } - 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(jti, entry, "target.example".to_owned(), session) - .expect("valid credential-injection KDC") + fn session(jti: Uuid) -> SyntheticKdcSession { + SyntheticKdcSession::new( + jti, + "target.example".to_owned(), + &app_credentials("proxy@example.invalid", "target").proxy, + ) + .expect("synthetic KDC session") } fn network_request(url: &str) -> NetworkRequest { @@ -702,342 +548,165 @@ mod tests { #[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"); + assert_eq!(session(Uuid::new_v4()).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 service_kdc_for_rejects_expired_credential_entry() { - let service = CredentialInjectionKdcService::new(); - let jti = Uuid::new_v4(); - - // Negative TTL: entry is born already expired. `TokenKeyedStore::get` does not - // filter on expiry, so the service's own check is what guarantees we never build a KDC - // over stale credentials. - let provisioning = provisioned_store(jti, "target", time::Duration::seconds(-1)); - - assert!( - matches!( - service.kdc_for(&provisioning, jti), - Err(CredentialInjectionKdcResolveError::ExpiredCredential { .. }) - ), - "expired credentials must not yield a KDC" - ); - } - - #[test] - fn service_kdc_for_returns_same_session_under_concurrent_calls() { - let service = CredentialInjectionKdcService::new(); - let jti = Uuid::new_v4(); - let provisioning = provisioned_store(jti, "target", time::Duration::minutes(5)); - - let first = service.kdc_for(&provisioning, jti).expect("first call resolves"); - let second = service.kdc_for(&provisioning, 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_kdc_for_ignores_cached_session_from_a_gone_entry() { - let service = CredentialInjectionKdcService::new(); - let jti = Uuid::new_v4(); - - // Simulate the race called out by Codex: a previous provisioning's session is still cached, - // but its provisioning entry is already gone (dead weak reference). A lookup against a fresh - // provisioning entry must not reuse that stale session; it must re-derive. - let stale_session = Arc::new(derive_credential_injection_kdc_session("proxy@example.invalid", jti)); - service.sessions.lock().insert( + let session = SyntheticKdcSession::new( jti, - CachedSession { - entry: WeakProvisioningEntry::new(), - session: Arc::clone(&stale_session), - }, - ); - - let provisioning = provisioned_store(jti, "target", time::Duration::minutes(5)); - let kdc = service.kdc_for(&provisioning, jti).expect("fresh entry resolves"); - - assert!( - !Arc::ptr_eq(&kdc.session, &stale_session), - "a session cached against a gone entry must not be reused" - ); + "target.example".to_owned(), + &app_credentials("just-a-uuid", "target").proxy, + ) + .expect("synthetic KDC session"); + assert_eq!(session.realm, synthetic_realm(jti)); } #[test] - fn service_kdc_for_rederives_session_after_reprovisioning() { - let service = CredentialInjectionKdcService::new(); - let jti = Uuid::new_v4(); - let provisioning = provisioned_store(jti, "target", time::Duration::minutes(5)); - - let first = service.kdc_for(&provisioning, jti).expect("first call resolves"); - let first_key = first.session.acceptor.long_term_key.expose_secret().clone(); - - // Re-provision under the same JTI: this produces a new provisioning entry, so the cached - // session no longer matches and must be re-derived, otherwise the new KDC would carry stale - // key material that the freshly provisioned credentials no longer match. - provisioning - .insert( - association_token(jti), - Some(cleartext_mapping_with_target_username("target")), - None, + fn from_provisioned_selects_kerberos_for_domain_target_with_kdc() { + let kdc = target_kdc(); + let injection = CredentialInjection::from_provisioned( + Uuid::new_v4(), + provisioned( + "administrator@example.invalid", + Some(kdc.clone()), time::Duration::minutes(5), - ) - .expect("credential entry re-inserts"); - - let second = service - .kdc_for(&provisioning, 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, - "re-provisioning must force a fresh session derivation" - ); - } - - #[test] - fn service_sweep_orphans_drops_sessions_with_no_credential_entry() { - let service = CredentialInjectionKdcService::new(); - let jti = Uuid::new_v4(); - - { - let provisioning = provisioned_store(jti, "target", time::Duration::minutes(5)); - service - .kdc_for(&provisioning, jti) - .expect("kdc_for populates session cache"); - assert!(service.sessions.lock().contains_key(&jti), "session cached"); + ), + "target.example", + true, + ) + .expect("Kerberos injection"); + + match injection { + CredentialInjection::Kerberos(injection) => assert_eq!(injection.target_kdc, kdc), + CredentialInjection::Ntlm(_) => panic!("expected Kerberos injection"), } - // The provisioning store is dropped here, so its entry is gone and the cached session's weak - // reference is now dead. - - service.sweep_orphans(); - assert!( - !service.sessions.lock().contains_key(&jti), - "sweep must drop sessions whose provisioning entry is gone" - ); - } - - #[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 - ); + fn from_provisioned_hard_errors_when_kerberos_target_has_no_kdc() { + let error = CredentialInjection::from_provisioned( + Uuid::new_v4(), + provisioned("administrator@example.invalid", None, time::Duration::minutes(5)), + "target.example", + true, + ) + .expect_err("missing KDC must abort, never fall back to NTLM"); + assert!(format!("{error:#}").contains("requires target connection option krb_kdc")); } #[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 - ); + fn from_provisioned_selects_ntlm_for_domainless_target() { + let injection = CredentialInjection::from_provisioned( + Uuid::new_v4(), + provisioned("Administrator", None, time::Duration::minutes(5)), + "target.example", + true, + ) + .expect("NTLM injection"); + assert!(matches!(injection, CredentialInjection::Ntlm(_))); } #[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_jti, entry, "target.example".to_owned(), session) - .expect_err("mismatched entry/session JTI must fail closed"); - let msg = format!("{err:#}"); - assert!( - msg.contains("provisioning entry JTI does not match credential-injection KDC session JTI"), - "actual: {msg}" - ); + fn from_provisioned_selects_ntlm_when_kerberos_disabled() { + let injection = CredentialInjection::from_provisioned( + Uuid::new_v4(), + provisioned( + "administrator@example.invalid", + Some(target_kdc()), + time::Duration::minutes(5), + ), + "target.example", + false, + ) + .expect("NTLM injection"); + assert!(matches!(injection, CredentialInjection::Ntlm(_))); } #[test] - fn service_kdc_for_rejects_unknown_jti() { - let service = CredentialInjectionKdcService::new(); - let provisioning = ProvisioningStore::new(); - - assert!( - matches!( - service.kdc_for(&provisioning, Uuid::new_v4()), - Err(CredentialInjectionKdcResolveError::MissingCredential { .. }) - ), - "KDC tokens with jet_cred_id must not fall back to real-KDC forwarding" - ); + fn from_provisioned_rejects_target_hostname_mismatch() { + let error = CredentialInjection::from_provisioned( + Uuid::new_v4(), + provisioned("Administrator", None, time::Duration::minutes(5)), + "other.example", + true, + ) + .expect_err("target hostname mismatch must fail closed"); + assert!(format!("{error:#}").contains("target mismatch")); } #[test] - fn service_kdc_for_rejects_non_injection_entry() { - let service = CredentialInjectionKdcService::new(); - let provisioning = ProvisioningStore::new(); + fn registered_session_is_retrievable() { + let registry = SyntheticKdcRegistry::new(); let jti = Uuid::new_v4(); - - provisioning - .insert(association_token(jti), None, None, time::Duration::minutes(5)) - .expect("provision-token entry inserts"); - - assert!( - matches!( - service.kdc_for(&provisioning, jti), - Err(CredentialInjectionKdcResolveError::NonInjectionCredential { .. }) - ), - "KDC tokens with jet_cred_id must require provision-credentials state" - ); + let session = Arc::new(session(jti)); + let _registration = registry.register(Arc::clone(&session)); + assert!(Arc::ptr_eq(®istry.get(jti).expect("registered"), &session)); } #[test] - fn service_kdc_for_lazily_extracts_target_hostname_from_entry_token() { - let service = CredentialInjectionKdcService::new(); + fn register_replaces_and_guarded_drop_keeps_successor() { + let registry = SyntheticKdcRegistry::new(); let jti = Uuid::new_v4(); - let provisioning = provisioned_store(jti, "target", time::Duration::minutes(5)); + // Two fresh sessions under one JTI model a reconnect that re-derives its own material. + let first = Arc::new(session(jti)); + let second = Arc::new(session(jti)); - let kdc = service - .kdc_for(&provisioning, jti) - .expect("credential-injection KDC resolves"); + let first_registration = registry.register(Arc::clone(&first)); + let second_registration = registry.register(Arc::clone(&second)); - assert_eq!(kdc.target_hostname, "target.example"); - } - - #[test] - fn provisioning_and_credential_service_share_target_options() { - let provisioning = ProvisioningStore::new(); - let service = CredentialInjectionKdcService::new(); - let jti = Uuid::new_v4(); - let token = association_token(jti); - let krb_kdc = crate::target_addr::TargetAddr::parse("tcp://kdc.example.invalid:88", Some(88)) - .expect("KDC address parses"); - - provisioning - .insert( - token.clone(), - Some(cleartext_mapping_with_target_username("target@example.invalid")), - Some(TargetConnectionOptions::new(Some(krb_kdc.clone())).expect("supported KDC scheme")), - time::Duration::minutes(5), - ) - .expect("provisioning entry inserts"); - - let entry = provisioning.get(jti).expect("provisioning entry is available directly"); - assert_eq!( - entry - .value - .connection_options - .as_ref() - .and_then(|options| options.krb_kdc()), - Some(&krb_kdc) - ); + // The reconnect supersedes the previous publication. + assert!(Arc::ptr_eq(®istry.get(jti).expect("registered"), &second)); - let kdc = service - .resolve_injection_kdc(&provisioning, jti, &token) - .expect("credential-injection lookup succeeds") - .expect("credential-injection KDC resolves"); + // The superseded connection's late teardown must not evict the successor. + drop(first_registration); + assert!(Arc::ptr_eq(®istry.get(jti).expect("still registered"), &second)); - assert_eq!(kdc.krb_kdc(), Some(&krb_kdc)); + // Dropping the current registration retracts it. + drop(second_registration); + assert!(registry.get(jti).is_none()); } #[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) + let result = session(Uuid::new_v4()) + .intercept_network_request(&network_request("http://kdc.real.example/path")) .expect("non-loopback request dispatches"); - - assert!(matches!( - result, - CredentialInjectionKdcInterception::NotInjectionRequest - )); + assert!(matches!(result, SyntheticKdcInterception::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) + let error = session(Uuid::new_v4()) + .intercept_network_request(&network_request("http://cred.invalid/not-a-uuid")) .expect_err("non-UUID path must fail"); - let msg = format!("{err:#}"); - assert!(msg.contains("malformed in-process KDC URL"), "actual: {msg}"); + assert!(format!("{error:#}").contains("malformed in-process KDC URL")); } #[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) + let session = session(Uuid::new_v4()); + let error = session + .intercept_network_request(&network_request(&format!("http://cred.invalid/{}", Uuid::new_v4()))) .expect_err("JTI mismatch must fail"); - let msg = format!("{err:#}"); - assert!(msg.contains("does not match current CredSSP session"), "actual: {msg}"); + assert!(format!("{error:#}").contains("does not match current CredSSP session")); } #[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) + let error = session(jti) + .intercept_network_request(&network_request(&format!("http://cred.invalid/{jti}"))) .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}"); + assert!(format!("{error:#}").contains("malformed in-process KDC proxy payload")); } #[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"); + fn realm_mismatch_reports_case_insensitively() { + assert!(realm_mismatch("AD.EXAMPLE", "ad.example").is_none()); + let mismatch = realm_mismatch("cred.invalid", "evil.example").expect("different realms mismatch"); + assert_eq!(mismatch.expected, "cred.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 54f614154..591295333 100644 --- a/devolutions-gateway/src/generic_client.rs +++ b/devolutions-gateway/src/generic_client.rs @@ -8,14 +8,14 @@ use tracing::field; use typed_builder::TypedBuilder; use crate::config::Conf; -use crate::credential_injection_kdc::CredentialInjectionKdcService; +use crate::credential_injection_kdc::{CredentialInjection, SyntheticKdcRegistry}; use crate::provisioning::ProvisioningStore; use crate::proxy::Proxy; use crate::rdp_pcb::{extract_association_claims, read_pcb}; use crate::recording::ActiveRecordings; use crate::session::{ConnectionModeDetails, DisconnectInterest, SessionInfo, SessionMessageSender}; use crate::subscriber::SubscriberSender; -use crate::token::{self, ConnectionMode, CurrentJrl, RecordingPolicy, TokenCache}; +use crate::token::{self, ConnectionMode, CurrentJrl, Protocol, RecordingPolicy, TokenCache}; use crate::upstream::{self, ConnectedUpstream}; #[derive(TypedBuilder)] @@ -28,7 +28,7 @@ pub struct GenericClient { sessions: SessionMessageSender, subscriber_tx: SubscriberSender, active_recordings: Arc, - credential_injection: CredentialInjectionKdcService, + synthetic_kdc_registry: SyntheticKdcRegistry, provisioning: ProvisioningStore, #[builder(default)] agent_tunnel_handle: Option>, @@ -53,7 +53,7 @@ where sessions, subscriber_tx, active_recordings, - credential_injection, + synthetic_kdc_registry, provisioning, agent_tunnel_handle, } = self; @@ -131,7 +131,7 @@ where span.record("target", selected_target.to_string()); - let is_rdp = claims.jet_ap == token::ApplicationProtocol::Known(token::Protocol::Rdp); + let is_rdp = matches!(claims.jet_ap, token::ApplicationProtocol::Known(Protocol::Rdp)); let info = SessionInfo::builder() .id(claims.jet_aid) @@ -146,20 +146,17 @@ where let disconnect_interest = DisconnectInterest::from_reconnection_policy(claims.jet_reuse); - // We support proxy-based credential injection for RDP. - // 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 works - // regardless of whether the upstream is direct TCP or tunnelled via an agent. - // The provisioning 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(credential_injection_kdc) = - credential_injection.resolve_injection_kdc(&provisioning, claims.jti, token)? - { + if is_rdp && let Some(provisioned_connection) = provisioning.get(claims.jti) { + let credential_injection = CredentialInjection::from_provisioned( + claims.jti, + provisioned_connection, + selected_target.host(), + conf.debug.enable_unstable && conf.debug.kerberos_credential_injection, + )?; + let _synthetic_kdc_registration = + credential_injection.register_synthetic_kdc(&synthetic_kdc_registry); info!( - jti = %credential_injection_kdc.jti(), + jti = %credential_injection.jti(), "RDP-TLS forwarding with credential injection" ); @@ -179,7 +176,7 @@ where .server_stream(server_stream) .sessions(sessions) .subscriber_tx(subscriber_tx) - .credential_injection_kdc(credential_injection_kdc) + .credential_injection(credential_injection) .client_stream_leftover_bytes(leftover_bytes) .server_dns_name(selected_target.host().to_owned()) .disconnect_interest(disconnect_interest) diff --git a/devolutions-gateway/src/lib.rs b/devolutions-gateway/src/lib.rs index 9ef441f70..c7ff98583 100644 --- a/devolutions-gateway/src/lib.rs +++ b/devolutions-gateway/src/lib.rs @@ -43,7 +43,6 @@ pub mod target_addr; pub(crate) mod target_connection_options; pub mod tls; pub mod token; -pub(crate) mod token_keyed_store; pub mod traffic_audit; pub mod upstream; pub mod utils; @@ -65,7 +64,7 @@ pub struct DgwState { pub recordings: recording::RecordingMessageSender, pub job_queue_handle: job_queue::JobQueueHandle, pub provisioning: provisioning::ProvisioningStore, - pub credential_injection: credential_injection_kdc::CredentialInjectionKdcService, + pub synthetic_kdc_registry: credential_injection_kdc::SyntheticKdcRegistry, pub monitoring_state: Arc, pub traffic_audit_handle: traffic_audit::TrafficAuditHandle, pub agent_tunnel_handle: Option>, @@ -94,7 +93,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 provisioning = provisioning::ProvisioningStore::new(); - let credential_injection = credential_injection_kdc::CredentialInjectionKdcService::new(); + let synthetic_kdc_registry = credential_injection_kdc::SyntheticKdcRegistry::new(); let monitoring_state = Arc::new(network_monitor::State::new(Arc::new(MockMonitorsCache))?); let state = Self { @@ -108,7 +107,7 @@ impl DgwState { job_queue_handle, traffic_audit_handle, provisioning, - credential_injection, + synthetic_kdc_registry, monitoring_state, agent_tunnel_handle: None, }; diff --git a/devolutions-gateway/src/listener.rs b/devolutions-gateway/src/listener.rs index c1691d580..5e972019a 100644 --- a/devolutions-gateway/src/listener.rs +++ b/devolutions-gateway/src/listener.rs @@ -158,7 +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_injection(state.credential_injection) + .synthetic_kdc_registry(state.synthetic_kdc_registry) .provisioning(state.provisioning) .agent_tunnel_handle(state.agent_tunnel_handle) .build() diff --git a/devolutions-gateway/src/ngrok.rs b/devolutions-gateway/src/ngrok.rs index e75546c8a..644f6630c 100644 --- a/devolutions-gateway/src/ngrok.rs +++ b/devolutions-gateway/src/ngrok.rs @@ -237,7 +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_injection(state.credential_injection) + .synthetic_kdc_registry(state.synthetic_kdc_registry) .provisioning(state.provisioning) .agent_tunnel_handle(state.agent_tunnel_handle) .build() diff --git a/devolutions-gateway/src/provisioning.rs b/devolutions-gateway/src/provisioning.rs index b1e77e109..ecc0ab3fb 100644 --- a/devolutions-gateway/src/provisioning.rs +++ b/devolutions-gateway/src/provisioning.rs @@ -1,37 +1,31 @@ -use std::sync::Weak; +use std::collections::HashMap; +use std::sync::Arc; -use devolutions_gateway_task::Task; +use anyhow::Context as _; +use async_trait::async_trait; +use devolutions_gateway_task::{ShutdownSignal, Task}; +use parking_lot::Mutex; use uuid::Uuid; -use crate::credential::{AppCredentialMapping, CleartextAppCredentialMapping}; +use crate::credential::{AppCredentials, CleartextAppCredentials}; use crate::target_connection_options::TargetConnectionOptions; -use crate::token_keyed_store::{ArcTokenKeyedEntry, TokenKeyError, TokenKeyedEntry, TokenKeyedStore}; -#[derive(Debug)] -pub(crate) struct ProvisioningEntry { - pub(crate) mapping: Option, +#[derive(Debug, Clone)] +pub(crate) struct ProvisionedConnection { + pub(crate) credentials: AppCredentials, pub(crate) connection_options: Option, -} - -pub(crate) type ArcProvisioningEntry = ArcTokenKeyedEntry; -pub(crate) type WeakProvisioningEntry = Weak>; - -#[derive(Debug, thiserror::Error)] -pub(crate) enum InsertError { - #[error(transparent)] - InvalidToken(#[from] TokenKeyError), - #[error("credential encryption failed")] - Internal(#[source] anyhow::Error), + pub(crate) target_hostname: String, + pub(crate) expires_at: time::OffsetDateTime, } /// Stores credentials and target options provisioned for a session, keyed by association-token JTI. /// -/// This is the credential boundary: cleartext mappings are encrypted on the way in, so the -/// underlying token-keyed store only ever holds encrypted material and the master key never leaves -/// this module's dependency graph. +/// This is the credential boundary: cleartext mappings are encrypted on the way in, so entries +/// only ever hold encrypted material and the master key never leaves this module's dependency +/// graph. #[derive(Debug, Clone)] pub struct ProvisioningStore { - entries: TokenKeyedStore, + entries: Arc>>, } impl Default for ProvisioningStore { @@ -43,39 +37,137 @@ impl Default for ProvisioningStore { impl ProvisioningStore { pub fn new() -> Self { Self { - entries: TokenKeyedStore::new(), + entries: Arc::new(Mutex::new(HashMap::new())), } } pub(crate) fn insert( &self, - token: String, - mapping: Option, + token_data: crate::token::CredentialInjectionTokenData, + credentials: CleartextAppCredentials, connection_options: Option, time_to_live: time::Duration, - ) -> Result, InsertError> { - let mapping = mapping - .map(CleartextAppCredentialMapping::encrypt) - .transpose() - .map_err(InsertError::Internal)?; - let previous = self.entries.insert( - token, - ProvisioningEntry { - mapping, - connection_options, - }, - time_to_live, - )?; - Ok(previous) + ) -> anyhow::Result { + let credentials = credentials.encrypt().context("encrypt provisioned credentials")?; + let entry = ProvisionedConnection { + credentials, + connection_options, + target_hostname: token_data.target_hostname, + expires_at: time::OffsetDateTime::now_utc() + time_to_live, + }; + + Ok(self.entries.lock().insert(token_data.jti, entry).is_some()) } - pub(crate) fn get(&self, jti: Uuid) -> Option { - self.entries.get(jti) + /// Look up the provisioning for a session. + /// + /// Entries live until their TTL, not until first use: a jti names a session, and the token + /// layer allows an RDP session to open several connections (reconnects), each of which needs + /// the same materials. Expired entries are treated as absent; the cleanup task reclaims them. + pub(crate) fn get(&self, jti: Uuid) -> Option { + let entries = self.entries.lock(); + let entry = entries.get(&jti)?; + + if time::OffsetDateTime::now_utc() >= entry.expires_at { + warn!(%jti, "Provisioning expired before the connection arrived"); + return None; + } + + Some(entry.clone()) } pub fn cleanup_task(&self) -> impl Task> + 'static + use<> { - crate::token_keyed_store::CleanupTask { - store: self.entries.clone(), + CleanupTask { + entries: Arc::clone(&self.entries), + } + } +} + +struct CleanupTask { + entries: Arc>>, +} + +#[async_trait] +impl Task for CleanupTask { + type Output = anyhow::Result<()>; + + const NAME: &'static str = "provisioning cleanup"; + + async fn run(self, shutdown_signal: ShutdownSignal) -> Self::Output { + cleanup_task(self.entries, shutdown_signal).await; + Ok(()) + } +} + +#[instrument(skip_all)] +async fn cleanup_task(entries: Arc>>, mut shutdown_signal: ShutdownSignal) { + use tokio::time::{Duration, sleep}; + + const TASK_INTERVAL: Duration = Duration::from_secs(60 * 15); + + debug!("Task started"); + + loop { + tokio::select! { + _ = sleep(TASK_INTERVAL) => {} + _ = shutdown_signal.wait() => { + break; + } + } + + let now = time::OffsetDateTime::now_utc(); + entries.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; + use crate::token::CredentialInjectionTokenData; + + fn credentials() -> CleartextAppCredentials { + CleartextAppCredentials { + proxy: CleartextAppCredential::UsernamePassword { + username: "proxy".to_owned(), + password: SecretString::from("pwd"), + }, + target: CleartextAppCredential::UsernamePassword { + username: "target".to_owned(), + password: SecretString::from("pwd"), + }, + } + } + + fn token_data(jti: Uuid) -> CredentialInjectionTokenData { + CredentialInjectionTokenData { + jti, + target_hostname: "target.example".to_owned(), } } + + #[test] + fn get_returns_a_live_entry() { + let store = ProvisioningStore::new(); + let jti = Uuid::new_v4(); + store + .insert(token_data(jti), credentials(), None, time::Duration::minutes(5)) + .expect("entry inserts"); + assert_eq!(store.get(jti).expect("live entry").target_hostname, "target.example"); + } + + #[test] + fn get_treats_an_expired_entry_as_absent() { + let store = ProvisioningStore::new(); + let jti = Uuid::new_v4(); + store + .insert(token_data(jti), credentials(), None, time::Duration::seconds(-1)) + .expect("entry inserts"); + assert!(store.get(jti).is_none(), "an expired entry must read as absent"); + } } diff --git a/devolutions-gateway/src/rd_clean_path.rs b/devolutions-gateway/src/rd_clean_path.rs index 0c790a91a..7105842b6 100644 --- a/devolutions-gateway/src/rd_clean_path.rs +++ b/devolutions-gateway/src/rd_clean_path.rs @@ -11,7 +11,7 @@ use tokio::io::{AsyncRead, AsyncReadExt as _, AsyncWrite, AsyncWriteExt as _}; use tracing::field; use crate::config::Conf; -use crate::credential_injection_kdc::{CredentialInjectionKdc, CredentialInjectionKdcService}; +use crate::credential_injection_kdc::{CredentialInjection, SyntheticKdcRegistry}; use crate::provisioning::ProvisioningStore; use crate::proxy::Proxy; use crate::recording::ActiveRecordings; @@ -314,13 +314,11 @@ async fn handle_with_credential_injection( mut client_stream: impl AsyncRead + AsyncWrite + Unpin + Send, client_addr: SocketAddr, conf: Arc, - token_cache: &TokenCache, - jrl: &CurrentJrl, sessions: SessionMessageSender, subscriber_tx: SubscriberSender, - active_recordings: &ActiveRecordings, + auth: CleanPathAuth, cleanpath_pdu: RDCleanPathPdu, - credential_injection_kdc: CredentialInjectionKdc, + credential_injection: CredentialInjection, agent_tunnel_handle: Option>, ) -> anyhow::Result<()> { let tls_conf = conf.credssp_tls.get().context("CredSSP TLS configuration")?; @@ -351,18 +349,7 @@ async fn handle_with_credential_injection( ) }; - // Authorize and connect to the RDP server. - let CleanPathAuth { claims } = authorize_cleanpath( - &cleanpath_pdu, - client_addr, - &conf, - token_cache, - jrl, - active_recordings, - &sessions, - ) - .await - .context("RDCleanPath authorization failed")?; + let CleanPathAuth { claims } = auth; let ConnectedRdpServer { tls_stream: server_stream, @@ -415,71 +402,9 @@ async fn handle_with_credential_injection( send_clean_path_response(&mut client_stream, &rd_clean_path_rsp).await?; debug!("RDCleanPath response sent, now performing CredSSP MITM"); - // -- Perform the CredSSP authentication with the client (acting as a server) and the server (acting as a client) -- // - - let mut client_framed = ironrdp_tokio::MovableTokioFramed::new(client_stream); - let mut server_framed = ironrdp_tokio::MovableTokioFramed::new(server_stream); - - let krb_configs = crate::rdp_proxy::credential_injection_kerberos_configs( - &conf, - client_addr, - &gateway_hostname, - &credential_injection_kdc, - )?; - let kdc_connector = crate::kdc_connector::KdcConnector::new(claims.jet_aid, claims.jet_agent_id, agent_tunnel_handle.clone()); - let client_credssp_fut = crate::rdp_proxy::perform_credssp_as_server( - &mut client_framed, - client_addr.ip(), - gateway_public_key, - client_security_protocol, - credential_injection_kdc.proxy_credential(), - krb_configs.server, - &credential_injection_kdc, - &kdc_connector, - ); - - 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_injection_kdc.target_credential(), - krb_configs.client, - &kdc_connector, - ); - - let (client_credssp_res, server_credssp_res) = tokio::join!(client_credssp_fut, server_credssp_fut); - client_credssp_res.context("CredSSP with client")?; - server_credssp_res.context("CredSSP with server")?; - - debug!("CredSSP MITM completed successfully"); - - // -- Intercept the Connect Confirm PDU, to override the server_security_protocol field -- // - - crate::rdp_proxy::intercept_connect_confirm(&mut client_framed, &mut server_framed, server_security_protocol) - .await?; - - let (mut client_stream, client_leftover) = client_framed.into_inner(); - let (mut server_stream, server_leftover) = server_framed.into_inner(); - - // -- At this point, proceed to the usual two-way forwarding -- // - - info!("RDP-TLS forwarding (credential injection)"); - - client_stream - .write_all(&server_leftover) - .await - .context("write server leftover to client")?; - - server_stream - .write_all(&client_leftover) - .await - .context("write client leftover to server")?; - - // Build SessionInfo for forwarding let info = SessionInfo::builder() .id(claims.jet_aid) .application_protocol(claims.jet_ap) @@ -493,21 +418,26 @@ async fn handle_with_credential_injection( let disconnect_interest = DisconnectInterest::from_reconnection_policy(claims.jet_reuse); - // Plain forwarding for now - Proxy::builder() + crate::rdp_proxy::RdpCredsspProxy::builder() .conf(conf) .session_info(info) - .address_a(client_addr) - .transport_a(client_stream) - .address_b(server_addr) - .transport_b(server_stream) + .client_stream(client_stream) + .client_addr(client_addr) + .server_stream(server_stream) + .server_addr(server_addr) + .credential_injection(credential_injection) .sessions(sessions) .subscriber_tx(subscriber_tx) + .server_dns_name(destination.host().to_owned()) .disconnect_interest(disconnect_interest) + .kdc_connector(kdc_connector) + .gateway_public_key(gateway_public_key) + .server_public_key(server_public_key) + .client_security_protocol(client_security_protocol) + .server_security_protocol(server_security_protocol) .build() - .select_dissector_and_forward() + .run() .await - .context("proxy failed") } #[expect(clippy::too_many_arguments)] @@ -521,7 +451,7 @@ pub async fn handle( sessions: SessionMessageSender, subscriber_tx: SubscriberSender, active_recordings: &ActiveRecordings, - credential_injection: &CredentialInjectionKdcService, + synthetic_kdc_registry: &SyntheticKdcRegistry, provisioning: &ProvisioningStore, agent_tunnel_handle: Option>, ) -> anyhow::Result<()> { @@ -533,20 +463,43 @@ pub async fn handle( .await .context("couldn't read cleanpath PDU")?; - // Early credential detection: check if we should use RdpProxy instead. - let token = cleanpath_pdu - .proxy_auth - .as_deref() - .context("missing token in RDCleanPath PDU")?; - - // 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 provisioning store is keyed on the association token's JTI. - if let Some(jti) = crate::token::extract_jti(token).ok() - && let Some(credential_injection_kdc) = credential_injection.resolve_injection_kdc(provisioning, jti, token)? + let auth = match authorize_cleanpath( + &cleanpath_pdu, + client_addr, + &conf, + token_cache, + jrl, + active_recordings, + &sessions, + ) + .await { + Ok(auth) => auth, + Err(error) => { + let response = RDCleanPathPdu::from(&error); + send_clean_path_response(&mut client_stream, &response).await?; + return anyhow::Error::new(error) + .context("RDCleanPath authorization failed") + .pipe(Err); + } + }; + + let target_hostname = match &auth.claims.jet_cm { + crate::token::ConnectionMode::Fwd { targets } => targets.first().host().to_owned(), + crate::token::ConnectionMode::Rdv => anyhow::bail!("authorize_cleanpath rejects rendezvous mode"), + }; + + if let Some(provisioned_connection) = provisioning.get(auth.claims.jti) { + let credential_injection = CredentialInjection::from_provisioned( + auth.claims.jti, + provisioned_connection, + &target_hostname, + conf.debug.enable_unstable && conf.debug.kerberos_credential_injection, + )?; + let _synthetic_kdc_registration = credential_injection.register_synthetic_kdc(synthetic_kdc_registry); + debug!( - jti = %credential_injection_kdc.jti(), + jti = %credential_injection.jti(), "Switching to RdpProxy for credential injection (WebSocket)" ); @@ -554,13 +507,11 @@ pub async fn handle( client_stream, client_addr, conf, - token_cache, - jrl, sessions, subscriber_tx, - active_recordings, + auth, cleanpath_pdu, - credential_injection_kdc, + credential_injection, agent_tunnel_handle.clone(), ) .await; @@ -568,25 +519,8 @@ pub async fn handle( trace!("Processing RDCleanPath"); - let (auth, connected) = match async { - let auth = authorize_cleanpath( - &cleanpath_pdu, - client_addr, - &conf, - token_cache, - jrl, - active_recordings, - &sessions, - ) - .await?; - - let connected = connect_rdp_server(&auth.claims, cleanpath_pdu, agent_tunnel_handle.as_ref()).await?; - - Ok::<_, CleanPathError>((auth, connected)) - } - .await - { - Ok(result) => result, + let connected = match connect_rdp_server(&auth.claims, cleanpath_pdu, agent_tunnel_handle.as_ref()).await { + Ok(connected) => connected, Err(error) => { let response = RDCleanPathPdu::from(&error); send_clean_path_response(&mut client_stream, &response).await?; diff --git a/devolutions-gateway/src/rdp_proxy.rs b/devolutions-gateway/src/rdp_proxy.rs index 94a3cecfe..4d64d1d2b 100644 --- a/devolutions-gateway/src/rdp_proxy.rs +++ b/devolutions-gateway/src/rdp_proxy.rs @@ -13,9 +13,7 @@ use typed_builder::TypedBuilder; use crate::config::Conf; use crate::credential::AppCredential; -use crate::credential_injection_kdc::{ - CredentialInjectionClientAcceptorProtocol, CredentialInjectionKdc, CredentialInjectionKdcInterception, -}; +use crate::credential_injection_kdc::{CredentialInjection, SyntheticKdcInterception}; use crate::kdc_connector::KdcConnector; use crate::proxy::Proxy; use crate::session::{DisconnectInterest, SessionInfo, SessionMessageSender}; @@ -29,7 +27,7 @@ pub struct RdpProxy { client_addr: SocketAddr, server_stream: S, server_addr: SocketAddr, - credential_injection_kdc: CredentialInjectionKdc, + credential_injection: CredentialInjection, client_stream_leftover_bytes: bytes::BytesMut, sessions: SessionMessageSender, subscriber_tx: SubscriberSender, @@ -41,6 +39,26 @@ pub struct RdpProxy { kdc_connector: KdcConnector, } +#[derive(TypedBuilder)] +pub(crate) struct RdpCredsspProxy { + conf: Arc, + session_info: SessionInfo, + client_stream: C, + client_addr: SocketAddr, + server_stream: S, + server_addr: SocketAddr, + credential_injection: CredentialInjection, + sessions: SessionMessageSender, + subscriber_tx: SubscriberSender, + server_dns_name: String, + disconnect_interest: Option, + kdc_connector: KdcConnector, + gateway_public_key: Vec, + server_public_key: Vec, + client_security_protocol: nego::SecurityProtocol, + server_security_protocol: nego::SecurityProtocol, +} + impl RdpProxy where A: AsyncWrite + AsyncRead + Unpin + Send, @@ -64,7 +82,7 @@ where client_addr, server_stream, server_addr, - credential_injection_kdc, + credential_injection, client_stream_leftover_bytes, sessions, subscriber_tx, @@ -92,7 +110,7 @@ where let handshake_result = dual_handshake_until_tls_upgrade( &mut client_framed, &mut server_framed, - credential_injection_kdc.target_credential(), + credential_injection.target_credential(), ) .await?; @@ -116,81 +134,117 @@ where let gateway_public_key = crate::tls::extract_public_key(gateway_cert_chain.first().context("no leaf")?) .context("extract Gateway public key")?; - // -- Perform the CredSSP authentication with the client (acting as a server) and the server (acting as a client) -- // - - let mut client_framed = ironrdp_tokio::MovableTokioFramed::new(client_stream); - let mut server_framed = ironrdp_tokio::MovableTokioFramed::new(server_stream); - - let krb_configs = - credential_injection_kerberos_configs(&conf, client_addr, &gateway_hostname, &credential_injection_kdc)?; - - let client_credssp_fut = perform_credssp_as_server( - &mut client_framed, - client_addr.ip(), - gateway_public_key, - handshake_result.client_security_protocol, - credential_injection_kdc.proxy_credential(), - krb_configs.server, - &credential_injection_kdc, - &kdc_connector, - ); - - let server_credssp_fut = perform_credssp_as_client( - &mut server_framed, - server_dns_name, - server_public_key, - handshake_result.server_security_protocol, - credential_injection_kdc.target_credential(), - krb_configs.client, - &kdc_connector, - ); - - let (client_credssp_res, server_credssp_res) = tokio::join!(client_credssp_fut, server_credssp_fut); - client_credssp_res.context("CredSSP with client")?; - server_credssp_res.context("CredSSP with server")?; - - // -- Intercept the Connect Confirm PDU, to override the server_security_protocol field -- // - - intercept_connect_confirm( - &mut client_framed, - &mut server_framed, - handshake_result.server_security_protocol, - ) - .await?; - - let (mut client_stream, client_leftover) = client_framed.into_inner(); - let (mut server_stream, server_leftover) = server_framed.into_inner(); - - // -- At this point, proceed to the usual two-way forwarding -- // - - info!("RDP-TLS forwarding (credential injection)"); - - client_stream - .write_all(&server_leftover) - .await - .context("write server leftover to client")?; - - server_stream - .write_all(&client_leftover) - .await - .context("write client leftover to server")?; - - Proxy::builder() + RdpCredsspProxy::builder() .conf(conf) .session_info(session_info) - .address_a(client_addr) - .transport_a(client_stream) - .address_b(server_addr) - .transport_b(server_stream) + .client_stream(client_stream) + .client_addr(client_addr) + .server_stream(server_stream) + .server_addr(server_addr) + .credential_injection(credential_injection) .sessions(sessions) .subscriber_tx(subscriber_tx) + .server_dns_name(server_dns_name) .disconnect_interest(disconnect_interest) + .kdc_connector(kdc_connector) + .gateway_public_key(gateway_public_key) + .server_public_key(server_public_key) + .client_security_protocol(handshake_result.client_security_protocol) + .server_security_protocol(handshake_result.server_security_protocol) .build() - .select_dissector_and_forward() + .run() .await - .context("RDP-TLS traffic proxying failed")?; +} - Ok(()) +impl RdpCredsspProxy +where + C: AsyncRead + AsyncWrite + Unpin + Send, + S: AsyncRead + AsyncWrite + Unpin + Send, +{ + pub(crate) async fn run(self) -> anyhow::Result<()> { + let Self { + conf, + session_info, + client_stream, + client_addr, + server_stream, + server_addr, + credential_injection, + sessions, + subscriber_tx, + server_dns_name, + disconnect_interest, + kdc_connector, + gateway_public_key, + server_public_key, + client_security_protocol, + server_security_protocol, + } = self; + + let mut client_framed = ironrdp_tokio::MovableTokioFramed::new(client_stream); + let mut server_framed = ironrdp_tokio::MovableTokioFramed::new(server_stream); + + let (server_kerberos_config, client_kerberos_config) = credential_injection + .kerberos_configs(client_addr, &conf.hostname)? + .unzip(); + + let client_credssp_fut = perform_credssp_as_server( + &mut client_framed, + client_addr.ip(), + gateway_public_key, + client_security_protocol, + credential_injection.proxy_credential(), + server_kerberos_config, + &credential_injection, + &kdc_connector, + ); + + let server_credssp_fut = perform_credssp_as_client( + &mut server_framed, + server_dns_name, + server_public_key, + server_security_protocol, + credential_injection.target_credential(), + client_kerberos_config, + &kdc_connector, + ); + + let (client_credssp_res, server_credssp_res) = tokio::join!(client_credssp_fut, server_credssp_fut); + client_credssp_res.context("CredSSP with client")?; + server_credssp_res.context("CredSSP with server")?; + + intercept_connect_confirm(&mut client_framed, &mut server_framed, server_security_protocol).await?; + + let (mut client_stream, client_leftover) = client_framed.into_inner(); + let (mut server_stream, server_leftover) = server_framed.into_inner(); + + info!("RDP-TLS forwarding (credential injection)"); + + client_stream + .write_all(&server_leftover) + .await + .context("write server leftover to client")?; + + server_stream + .write_all(&client_leftover) + .await + .context("write client leftover to server")?; + + Proxy::builder() + .conf(conf) + .session_info(session_info) + .address_a(client_addr) + .transport_a(client_stream) + .address_b(server_addr) + .transport_b(server_stream) + .sessions(sessions) + .subscriber_tx(subscriber_tx) + .disconnect_interest(disconnect_interest) + .build() + .select_dissector_and_forward() + .await + .context("RDP-TLS traffic proxying failed") + } } #[derive(Debug)] @@ -200,7 +254,7 @@ struct HandshakeResult { } #[instrument(level = "debug", ret, skip_all)] -pub(crate) async fn intercept_connect_confirm( +async fn intercept_connect_confirm( client_framed: &mut ironrdp_tokio::MovableTokioFramed, server_framed: &mut ironrdp_tokio::MovableTokioFramed, server_security_protocol: nego::SecurityProtocol, @@ -350,55 +404,8 @@ where handshake_result } -/// Kerberos configs for the two CredSSP legs of a credential-injection session. -/// -/// `server` drives the client-facing acceptor (Gateway-as-server); `client` drives the -/// target-facing leg (Gateway-as-client). `None` on a leg means that leg authenticates over NTLM. -pub(crate) struct CredentialInjectionKerberosConfigs { - pub server: Option, - pub client: Option, -} - -/// Whether a credential-injection session speaks Kerberos (vs NTLM). `None` means the feature is -/// disabled; otherwise the target credential determines the package for both CredSSP legs. -fn injection_uses_kerberos(protocol: Option) -> bool { - matches!(protocol, Some(CredentialInjectionClientAcceptorProtocol::Kerberos)) -} - -/// Build the Kerberos config for both CredSSP legs from the single [`injection_uses_kerberos`] -/// decision. Everything else is NTLM on both legs. -pub(crate) fn credential_injection_kerberos_configs( - conf: &Conf, - client_addr: SocketAddr, - gateway_hostname: &str, - credential_injection_kdc: &CredentialInjectionKdc, -) -> anyhow::Result { - let protocol = (conf.debug.enable_unstable && conf.debug.kerberos_credential_injection) - .then(|| credential_injection_kdc.client_acceptor_protocol()) - .transpose()?; - - if !injection_uses_kerberos(protocol) { - return Ok(CredentialInjectionKerberosConfigs { - server: None, - client: None, - }); - } - - let krb_kdc = credential_injection_kdc - .krb_kdc() - .context("Kerberos credential injection requires target connection option krb_kdc")?; - - Ok(CredentialInjectionKerberosConfigs { - server: Some(credential_injection_kdc.server_kerberos_config(client_addr)?), - client: Some(ironrdp_connector::credssp::KerberosConfig { - kdc_proxy_url: Some(url::Url::try_from(krb_kdc).context("convert target KDC address to URL")?), - hostname: gateway_hostname.to_owned(), - }), - }) -} - #[instrument(name = "server_credssp", level = "debug", ret, skip_all)] -pub(crate) async fn perform_credssp_as_client( +async fn perform_credssp_as_client( framed: &mut ironrdp_tokio::Framed, server_name: String, server_public_key: Vec, @@ -470,7 +477,7 @@ where async fn resolve_server_generator( generator: &mut CredsspServerProcessGenerator<'_>, - credential_injection_kdc: &CredentialInjectionKdc, + credential_injection: &CredentialInjection, kdc_connector: &KdcConnector, ) -> Result { let mut state = generator.start(); @@ -478,14 +485,14 @@ async fn resolve_server_generator( loop { match state { GeneratorState::Suspended(request) => { - let response = match credential_injection_kdc.intercept_network_request(&request) { - Ok(CredentialInjectionKdcInterception::Intercepted(response)) => Ok(response), - Ok(CredentialInjectionKdcInterception::NotInjectionRequest) => { - kdc_connector.send_network_request(&request).await - } - Ok(CredentialInjectionKdcInterception::NotInjectionRealm(mismatch)) => Err(anyhow::anyhow!( + let response = match credential_injection.intercept_network_request(&request) { + Ok(SyntheticKdcInterception::Intercepted(response)) => Ok(response), + Ok(SyntheticKdcInterception::NotInjectionRealm(mismatch)) => Err(anyhow::anyhow!( "kdc request realm does not match credential-injection session realm: {mismatch}" )), + Ok(SyntheticKdcInterception::NotInjectionRequest) => { + kdc_connector.send_network_request(&request).await + } Err(error) => Err(error), } .map_err(|err| sspi::credssp::ServerError { @@ -525,14 +532,14 @@ async fn resolve_client_generator( #[expect(clippy::too_many_arguments)] #[instrument(name = "client_credssp", level = "debug", ret, skip_all)] -pub(crate) async fn perform_credssp_as_server( +async fn perform_credssp_as_server( framed: &mut ironrdp_tokio::Framed, client_addr: IpAddr, gateway_public_key: Vec, security_protocol: nego::SecurityProtocol, credentials: &AppCredential, kerberos_server_config: Option, - credential_injection_kdc: &CredentialInjectionKdc, + credential_injection: &CredentialInjection, kdc_connector: &KdcConnector, ) -> anyhow::Result<()> where @@ -554,7 +561,7 @@ where gateway_public_key, credentials, kerberos_server_config, - credential_injection_kdc, + credential_injection, kdc_connector, ) .await; @@ -583,7 +590,7 @@ where public_key: Vec, credentials: &AppCredential, kerberos_server_config: Option, - credential_injection_kdc: &CredentialInjectionKdc, + credential_injection: &CredentialInjection, kdc_connector: &KdcConnector, ) -> anyhow::Result<()> where @@ -626,7 +633,7 @@ where let result = { let mut generator = sequence.process_ts_request(ts_request); - resolve_server_generator(&mut generator, credential_injection_kdc, kdc_connector).await + resolve_server_generator(&mut generator, credential_injection, kdc_connector).await }; // drop generator buf.clear(); @@ -656,20 +663,3 @@ where framed.write_all(&payload).await.context("failed to write PDU")?; Ok(()) } - -#[cfg(test)] -mod tests { - use super::*; - - // The two CredSSP legs are built from this single decision, so agreement is guaranteed by - // construction. These cases pin the decision itself (the bug was the two legs deciding - // independently): Kerberos requires BOTH opt-in flags AND a domain-qualified target. - #[test] - fn injection_uses_kerberos_requires_optin_and_domain_qualified_target() { - use CredentialInjectionClientAcceptorProtocol::{Kerberos, Ntlm}; - - assert!(injection_uses_kerberos(Some(Kerberos))); - assert!(!injection_uses_kerberos(Some(Ntlm))); - assert!(!injection_uses_kerberos(None)); - } -} diff --git a/devolutions-gateway/src/service.rs b/devolutions-gateway/src/service.rs index 7288564b9..bb2ad4562 100644 --- a/devolutions-gateway/src/service.rs +++ b/devolutions-gateway/src/service.rs @@ -268,7 +268,7 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { .context("failed to initialize traffic audit manager")?; let provisioning = devolutions_gateway::provisioning::ProvisioningStore::new(); - let credential_injection = devolutions_gateway::credential_injection_kdc::CredentialInjectionKdcService::new(); + let synthetic_kdc_registry = devolutions_gateway::credential_injection_kdc::SyntheticKdcRegistry::new(); let filesystem_monitor_config_cache = devolutions_gateway::api::monitoring::FilesystemConfigCache::new( config::get_data_dir().join("monitors_cache.json"), @@ -317,7 +317,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(), provisioning: provisioning.clone(), - credential_injection: credential_injection.clone(), + synthetic_kdc_registry: synthetic_kdc_registry.clone(), monitoring_state, traffic_audit_handle: traffic_audit_task.handle(), agent_tunnel_handle, @@ -354,10 +354,6 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { tasks.register(provisioning.cleanup_task()); - tasks.register(devolutions_gateway::credential_injection_kdc::CleanupTask { - service: credential_injection, - }); - 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 a3f6e0e7f..7a95c45d4 100644 --- a/devolutions-gateway/src/token.rs +++ b/devolutions-gateway/src/token.rs @@ -1313,15 +1313,21 @@ pub fn extract_dst_alt(token: &str) -> anyhow::Result> { .collect() } -/// Validate the association-token claims required by credential injection. +pub(crate) struct CredentialInjectionTokenData { + pub(crate) jti: Uuid, + pub(crate) target_hostname: String, +} + +/// Parse the association-token data 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 { +/// This is intentionally a token-layer shape check only; normal connection authorization still +/// validates the token before consuming the provisioned data. +pub(crate) 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) + let target_hostname = extract_credential_injection_target_hostname(token)?; + Ok(CredentialInjectionTokenData { jti, target_hostname }) } /// Extract the target hostname used by credential injection from an association token. diff --git a/devolutions-gateway/src/token_keyed_store.rs b/devolutions-gateway/src/token_keyed_store.rs deleted file mode 100644 index d010c52b4..000000000 --- a/devolutions-gateway/src/token_keyed_store.rs +++ /dev/null @@ -1,164 +0,0 @@ -use std::collections::HashMap; -use std::sync::Arc; - -use anyhow::Context as _; -use async_trait::async_trait; -use devolutions_gateway_task::{ShutdownSignal, Task}; -use parking_lot::Mutex; -use uuid::Uuid; - -#[derive(Debug, thiserror::Error)] -#[error("invalid token")] -pub(crate) struct TokenKeyError(#[source] anyhow::Error); - -#[derive(Debug)] -pub(crate) struct TokenKeyedEntry { - pub(crate) token: String, - pub(crate) value: V, - pub(crate) expires_at: time::OffsetDateTime, -} - -pub(crate) type ArcTokenKeyedEntry = Arc>; - -#[derive(Debug)] -struct Store { - entries: HashMap>, -} - -#[derive(Debug)] -pub(crate) struct TokenKeyedStore(Arc>>); - -impl Clone for TokenKeyedStore { - fn clone(&self) -> Self { - Self(Arc::clone(&self.0)) - } -} - -impl Default for TokenKeyedStore { - fn default() -> Self { - Self::new() - } -} - -impl TokenKeyedStore { - pub(crate) fn new() -> Self { - Self(Arc::new(Mutex::new(Store { - entries: HashMap::new(), - }))) - } - - pub(crate) fn insert( - &self, - token: String, - value: V, - time_to_live: time::Duration, - ) -> Result>, TokenKeyError> { - let jti = crate::token::extract_jti(&token) - .context("failed to extract token ID") - .map_err(TokenKeyError)?; - let entry = TokenKeyedEntry { - token, - value, - expires_at: time::OffsetDateTime::now_utc() + time_to_live, - }; - - let previous = self.0.lock().entries.insert(jti, Arc::new(entry)); - - Ok(previous) - } - - /// Look up an entry by its token JTI. - /// - /// This may return an entry that is already past its `expires_at`: eviction is asynchronous - /// (see [`CleanupTask`]), so a caller that cares about freshness must check `expires_at` itself. - pub(crate) fn get(&self, jti: Uuid) -> Option> { - self.0.lock().entries.get(&jti).map(Arc::clone) - } - - fn remove_expired(&self, now: time::OffsetDateTime) { - self.0.lock().entries.retain(|_, entry| now < entry.expires_at); - } -} - -pub(crate) struct CleanupTask { - pub(crate) store: TokenKeyedStore, -} - -#[async_trait] -impl Task for CleanupTask -where - V: Send + Sync + 'static, -{ - type Output = anyhow::Result<()>; - - const NAME: &'static str = "token-keyed store cleanup"; - - async fn run(self, shutdown_signal: ShutdownSignal) -> Self::Output { - cleanup_task(self.store, shutdown_signal).await; - Ok(()) - } -} - -#[instrument(skip_all)] -async fn cleanup_task(store: TokenKeyedStore, mut shutdown_signal: ShutdownSignal) { - use tokio::time::{Duration, sleep}; - - const TASK_INTERVAL: Duration = Duration::from_secs(60 * 15); - - debug!("Task started"); - - loop { - tokio::select! { - _ = sleep(TASK_INTERVAL) => {} - _ = shutdown_signal.wait() => { - break; - } - } - - store.remove_expired(time::OffsetDateTime::now_utc()); - } - - debug!("Task terminated"); -} - -#[cfg(test)] -mod tests { - use base64::Engine as _; - - use super::*; - - fn token(jti: Uuid) -> 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(&serde_json::json!({ "jti": jti })).expect("token payload should serialize")); - let signature = engine.encode(b"signature"); - format!("{header}.{payload}.{signature}") - } - - #[test] - fn stores_opaque_values_by_token_jti() { - let store = TokenKeyedStore::new(); - let jti = Uuid::new_v4(); - - let previous = store - .insert(token(jti), 42u32, time::Duration::minutes(5)) - .expect("entry inserts"); - - assert!(previous.is_none()); - assert_eq!(store.get(jti).expect("entry is indexed").value, 42); - } - - #[test] - fn removes_expired_values() { - let store = TokenKeyedStore::new(); - let jti = Uuid::new_v4(); - store - .insert(token(jti), (), time::Duration::seconds(-1)) - .expect("entry inserts"); - - store.remove_expired(time::OffsetDateTime::now_utc()); - - assert!(store.get(jti).is_none()); - } -} diff --git a/devolutions-gateway/tests/preflight.rs b/devolutions-gateway/tests/preflight.rs index 8246696ac..522cb32eb 100644 --- a/devolutions-gateway/tests/preflight.rs +++ b/devolutions-gateway/tests/preflight.rs @@ -231,62 +231,48 @@ async fn test_provision_credentials_rejects_missing_target_hostname() -> anyhow: } #[tokio::test] -async fn test_provision_token_overwrite_alert() -> anyhow::Result<()> { +async fn test_provision_invalid_params() -> anyhow::Result<()> { let _guard = init_logger(); let (app, _state, _handles) = make_router()?; let token = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiI1ZTNlODMzZi04NGM3LTQ1NDEtYjY3Ni1hY2MzMjk5ZTM5YjgifQ.1qECGlrW7y9HWFArc6GPHLGTOY7PhAvzKJ5XMRBg4k4"; - let op_id1 = Uuid::new_v4(); - let op_id2 = Uuid::new_v4(); - - let op1 = json!([{ - "id": op_id1, - "kind": "provision-token", - "token": token, - }]); - - app.clone().oneshot(preflight_request(op1)?).await?; + let op_id = Uuid::new_v4(); - let op2 = json!([{ - "id": op_id2, - "kind": "provision-token", + let op = json!([{ + "id": op_id, + "kind": "provision-credentials", "token": token, + "proxy_credendial": { "kind": "unknown" }, + "target_credential": { "kind": "username-password", "username": "u", "password": "p" }, }]); - let response = app.oneshot(preflight_request(op2)?).await?; + let request = preflight_request(op)?; + let response = app.oneshot(request).await?; 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.as_array().expect("an array").len(), 1); assert_eq!(body[0]["kind"], "alert"); - assert!(body[0]["alert_message"].as_str().unwrap().contains("replaced")); - assert_eq!(body[1]["kind"], "ack"); + assert_eq!(body[0]["alert_status"], "invalid-parameters"); Ok(()) } #[tokio::test] -async fn test_provision_invalid_params() -> anyhow::Result<()> { +async fn test_provision_token_rejects_garbage_token() -> anyhow::Result<()> { let _guard = init_logger(); let (app, _state, _handles) = make_router()?; - let token = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiI1ZTNlODMzZi04NGM3LTQ1NDEtYjY3Ni1hY2MzMjk5ZTM5YjgifQ.1qECGlrW7y9HWFArc6GPHLGTOY7PhAvzKJ5XMRBg4k4"; - - let op_id = Uuid::new_v4(); - let op = json!([{ - "id": op_id, - "kind": "provision-credentials", - "token": token, - "proxy_credendial": { "kind": "unknown" }, - "target_credential": { "kind": "username-password", "username": "u", "password": "p" }, + "id": Uuid::new_v4(), + "kind": "provision-token", + "token": "not-a-jwt", }]); - let request = preflight_request(op)?; - let response = app.oneshot(request).await?; + let response = app.oneshot(preflight_request(op)?).await?; let body = response.into_body().collect().await?.to_bytes(); let body: serde_json::Value = serde_json::from_slice(&body)?; From 819dfbc01bb09278e6c96f26d530f4ba48082162 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Mon, 27 Jul 2026 17:01:39 -0400 Subject: [PATCH 07/11] fix(dgw): keep the synthetic KDC registered for the whole injection session During CredSSP the client reaches the Gateway KDC proxy (/jet/KdcProxy) to get Kerberos tickets from the per-session synthetic KDC, resolved through the global registry. The registration guard was dropped the instant it was created (`let _ = registry.register(...)`), so the entry was removed immediately and the lookup returned "credential-injection state is not available" (400), killing the session. Store the registration inside KerberosCredentialInjection so it lives as long as the injection (moved into the RDP proxy) and is retracted only when the session ends. Restructure the injection CredSSP stage alongside the fix: model construction-then-registration as CredentialInjectionInitializationResult::maybe_register_synthetic_kdc, split the RDP proxy into CredsspSession + PreparedCredssp (new rdp_proxy/credssp.rs), and replace the rendezvous unreachable!() with a bail!. Realign the unit tests. --- .../src/credential_injection_kdc.rs | 74 +- devolutions-gateway/src/generic_client.rs | 21 +- devolutions-gateway/src/rd_clean_path.rs | 18 +- devolutions-gateway/src/rdp_proxy.rs | 665 ------------------ devolutions-gateway/src/rdp_proxy/credssp.rs | 430 +++++++++++ devolutions-gateway/src/rdp_proxy/mod.rs | 224 ++++++ 6 files changed, 733 insertions(+), 699 deletions(-) delete mode 100644 devolutions-gateway/src/rdp_proxy.rs create mode 100644 devolutions-gateway/src/rdp_proxy/credssp.rs create mode 100644 devolutions-gateway/src/rdp_proxy/mod.rs diff --git a/devolutions-gateway/src/credential_injection_kdc.rs b/devolutions-gateway/src/credential_injection_kdc.rs index a09a917a3..9c1deff07 100644 --- a/devolutions-gateway/src/credential_injection_kdc.rs +++ b/devolutions-gateway/src/credential_injection_kdc.rs @@ -45,6 +45,16 @@ pub(crate) struct KerberosCredentialInjection { credentials: AppCredentials, target_kdc: TargetAddr, synthetic_kdc: Arc, + // Holds the synthetic KDC published in the registry for this session. It lives as long as this + // injection (moved into the RDP proxy), so the client's KKDCP lookups resolve for the whole + // session; dropping it unpublishes the KDC. `None` until registered. + registration: Option, +} + +impl KerberosCredentialInjection { + pub(crate) fn synthetic_kdc(&self) -> Arc { + Arc::clone(&self.synthetic_kdc) + } } pub(crate) struct NtlmCredentialInjection { @@ -114,13 +124,38 @@ impl fmt::Debug for CredentialInjection { } } +pub(crate) enum CredentialInjectionInitializationResult { + Kerberos(KerberosCredentialInjection), + Ntlm(NtlmCredentialInjection), +} + +impl CredentialInjectionInitializationResult { + pub(crate) fn maybe_register_synthetic_kdc(self, registry: &SyntheticKdcRegistry) -> CredentialInjection { + match self { + Self::Kerberos(mut injection) => { + // Keep the registration alive for the session: it rides inside the returned + // injection (moved into the RDP proxy). Dropping it here would immediately + // unpublish the synthetic KDC and the client's KKDCP lookups would fail. + let session = injection.synthetic_kdc(); + injection.registration = Some(registry.register(session)); + debug!( + jti = %injection.jti, + "registered synthetic KDC for credential-injection session" + ); + CredentialInjection::Kerberos(injection) + } + Self::Ntlm(injection) => CredentialInjection::Ntlm(injection), + } + } +} + impl CredentialInjection { pub(crate) fn from_provisioned( jti: Uuid, provisioned_connection: ProvisionedConnection, target_hostname: &str, kerberos_enabled: bool, - ) -> anyhow::Result { + ) -> anyhow::Result { anyhow::ensure!( target_hostname.eq_ignore_ascii_case(&provisioned_connection.target_hostname), "credential-injection target mismatch" @@ -149,14 +184,20 @@ impl CredentialInjection { .context("Kerberos credential injection requires target connection option krb_kdc")?; let synthetic_kdc = SyntheticKdcSession::new(jti, target_hostname, &credentials.proxy)?; - Ok(Self::Kerberos(KerberosCredentialInjection { + Ok(CredentialInjectionInitializationResult::Kerberos( + KerberosCredentialInjection { + jti, + credentials, + target_kdc, + synthetic_kdc: Arc::new(synthetic_kdc), + registration: None, + }, + )) + } else { + Ok(CredentialInjectionInitializationResult::Ntlm(NtlmCredentialInjection { jti, credentials, - target_kdc, - synthetic_kdc: Arc::new(synthetic_kdc), })) - } else { - Ok(Self::Ntlm(NtlmCredentialInjection { jti, credentials })) } } @@ -200,13 +241,6 @@ impl CredentialInjection { } } - pub(crate) fn register_synthetic_kdc(&self, registry: &SyntheticKdcRegistry) -> Option { - match self { - Self::Kerberos(injection) => Some(registry.register(Arc::clone(&injection.synthetic_kdc))), - Self::Ntlm(_) => None, - } - } - pub(crate) fn intercept_network_request( &self, request: &NetworkRequest, @@ -579,8 +613,8 @@ mod tests { .expect("Kerberos injection"); match injection { - CredentialInjection::Kerberos(injection) => assert_eq!(injection.target_kdc, kdc), - CredentialInjection::Ntlm(_) => panic!("expected Kerberos injection"), + CredentialInjectionInitializationResult::Kerberos(injection) => assert_eq!(injection.target_kdc, kdc), + CredentialInjectionInitializationResult::Ntlm(_) => panic!("expected Kerberos injection"), } } @@ -592,7 +626,8 @@ mod tests { "target.example", true, ) - .expect_err("missing KDC must abort, never fall back to NTLM"); + .err() + .expect("missing KDC must abort, never fall back to NTLM"); assert!(format!("{error:#}").contains("requires target connection option krb_kdc")); } @@ -605,7 +640,7 @@ mod tests { true, ) .expect("NTLM injection"); - assert!(matches!(injection, CredentialInjection::Ntlm(_))); + assert!(matches!(injection, CredentialInjectionInitializationResult::Ntlm(_))); } #[test] @@ -621,7 +656,7 @@ mod tests { false, ) .expect("NTLM injection"); - assert!(matches!(injection, CredentialInjection::Ntlm(_))); + assert!(matches!(injection, CredentialInjectionInitializationResult::Ntlm(_))); } #[test] @@ -632,7 +667,8 @@ mod tests { "other.example", true, ) - .expect_err("target hostname mismatch must fail closed"); + .err() + .expect("target hostname mismatch must fail closed"); assert!(format!("{error:#}").contains("target mismatch")); } diff --git a/devolutions-gateway/src/generic_client.rs b/devolutions-gateway/src/generic_client.rs index 591295333..4ab41a48d 100644 --- a/devolutions-gateway/src/generic_client.rs +++ b/devolutions-gateway/src/generic_client.rs @@ -153,8 +153,10 @@ where selected_target.host(), conf.debug.enable_unstable && conf.debug.kerberos_credential_injection, )?; - let _synthetic_kdc_registration = - credential_injection.register_synthetic_kdc(&synthetic_kdc_registry); + + let credential_injection = + credential_injection.maybe_register_synthetic_kdc(&synthetic_kdc_registry); + info!( jti = %credential_injection.jti(), "RDP-TLS forwarding with credential injection" @@ -166,21 +168,24 @@ where agent_tunnel_handle.clone(), ); - // NOTE: In the future, we could imagine performing proxy-based recording as well using RdpProxy. - return crate::rdp_proxy::RdpProxy::builder() + let credssp_session = crate::rdp_proxy::CredsspSession::builder() .conf(conf) .session_info(info) .client_addr(client_addr) - .client_stream(client_stream) .server_addr(server_addr) - .server_stream(server_stream) + .credential_injection(credential_injection) .sessions(sessions) .subscriber_tx(subscriber_tx) - .credential_injection(credential_injection) - .client_stream_leftover_bytes(leftover_bytes) .server_dns_name(selected_target.host().to_owned()) .disconnect_interest(disconnect_interest) .kdc_connector(kdc_connector) + .build(); + + return crate::rdp_proxy::RdpProxy::builder() + .session(credssp_session) + .client_stream(client_stream) + .server_stream(server_stream) + .client_stream_leftover_bytes(leftover_bytes) .build() .run() .await diff --git a/devolutions-gateway/src/rd_clean_path.rs b/devolutions-gateway/src/rd_clean_path.rs index 7105842b6..cee4c9be5 100644 --- a/devolutions-gateway/src/rd_clean_path.rs +++ b/devolutions-gateway/src/rd_clean_path.rs @@ -418,12 +418,10 @@ async fn handle_with_credential_injection( let disconnect_interest = DisconnectInterest::from_reconnection_policy(claims.jet_reuse); - crate::rdp_proxy::RdpCredsspProxy::builder() + let credssp_session = crate::rdp_proxy::CredsspSession::builder() .conf(conf) .session_info(info) - .client_stream(client_stream) .client_addr(client_addr) - .server_stream(server_stream) .server_addr(server_addr) .credential_injection(credential_injection) .sessions(sessions) @@ -431,13 +429,18 @@ async fn handle_with_credential_injection( .server_dns_name(destination.host().to_owned()) .disconnect_interest(disconnect_interest) .kdc_connector(kdc_connector) + .build(); + + let prepared = crate::rdp_proxy::PreparedCredssp::builder() + .client_stream(client_stream) + .server_stream(server_stream) .gateway_public_key(gateway_public_key) .server_public_key(server_public_key) .client_security_protocol(client_security_protocol) .server_security_protocol(server_security_protocol) - .build() - .run() - .await + .build(); + + credssp_session.run(prepared).await } #[expect(clippy::too_many_arguments)] @@ -496,7 +499,8 @@ pub async fn handle( &target_hostname, conf.debug.enable_unstable && conf.debug.kerberos_credential_injection, )?; - let _synthetic_kdc_registration = credential_injection.register_synthetic_kdc(synthetic_kdc_registry); + + let credential_injection = credential_injection.maybe_register_synthetic_kdc(synthetic_kdc_registry); debug!( jti = %credential_injection.jti(), diff --git a/devolutions-gateway/src/rdp_proxy.rs b/devolutions-gateway/src/rdp_proxy.rs deleted file mode 100644 index 4d64d1d2b..000000000 --- a/devolutions-gateway/src/rdp_proxy.rs +++ /dev/null @@ -1,665 +0,0 @@ -use std::net::{IpAddr, SocketAddr}; -use std::sync::Arc; - -use anyhow::Context as _; -use ironrdp_acceptor::credssp::CredsspProcessGenerator as CredsspServerProcessGenerator; -use ironrdp_connector::credssp::CredsspProcessGenerator as CredsspClientProcessGenerator; -use ironrdp_connector::sspi; -use ironrdp_connector::sspi::generator::GeneratorState; -use ironrdp_pdu::{mcs, nego, x224}; -use secrecy::ExposeSecret as _; -use tokio::io::{AsyncRead, AsyncWrite, AsyncWriteExt}; -use typed_builder::TypedBuilder; - -use crate::config::Conf; -use crate::credential::AppCredential; -use crate::credential_injection_kdc::{CredentialInjection, SyntheticKdcInterception}; -use crate::kdc_connector::KdcConnector; -use crate::proxy::Proxy; -use crate::session::{DisconnectInterest, SessionInfo, SessionMessageSender}; -use crate::subscriber::SubscriberSender; - -#[derive(TypedBuilder)] -pub struct RdpProxy { - conf: Arc, - session_info: SessionInfo, - client_stream: C, - client_addr: SocketAddr, - server_stream: S, - server_addr: SocketAddr, - credential_injection: CredentialInjection, - client_stream_leftover_bytes: bytes::BytesMut, - sessions: SessionMessageSender, - subscriber_tx: SubscriberSender, - server_dns_name: String, - disconnect_interest: Option, - /// Outbound dispatcher for CredSSP-originated KDC traffic. Encapsulates whether KDC - /// requests should attempt agent-tunnel routing (and any `jet_agent_id` pin from the - /// parent association token) or always go direct. - kdc_connector: KdcConnector, -} - -#[derive(TypedBuilder)] -pub(crate) struct RdpCredsspProxy { - conf: Arc, - session_info: SessionInfo, - client_stream: C, - client_addr: SocketAddr, - server_stream: S, - server_addr: SocketAddr, - credential_injection: CredentialInjection, - sessions: SessionMessageSender, - subscriber_tx: SubscriberSender, - server_dns_name: String, - disconnect_interest: Option, - kdc_connector: KdcConnector, - gateway_public_key: Vec, - server_public_key: Vec, - client_security_protocol: nego::SecurityProtocol, - server_security_protocol: nego::SecurityProtocol, -} - -impl RdpProxy -where - A: AsyncWrite + AsyncRead + Unpin + Send, - B: AsyncWrite + AsyncRead + Unpin + Send, -{ - pub async fn run(self) -> anyhow::Result<()> { - handle(self).await - } -} - -#[instrument("rdp_proxy", skip_all, fields(session_id = proxy.session_info.id.to_string(), target = proxy.server_addr.to_string()))] -async fn handle(proxy: RdpProxy) -> anyhow::Result<()> -where - C: AsyncRead + AsyncWrite + Unpin + Send, - S: AsyncRead + AsyncWrite + Unpin + Send, -{ - let RdpProxy { - conf, - session_info, - client_stream, - client_addr, - server_stream, - server_addr, - credential_injection, - client_stream_leftover_bytes, - sessions, - subscriber_tx, - server_dns_name, - disconnect_interest, - kdc_connector, - } = proxy; - - let tls_conf = conf.credssp_tls.get().context("CredSSP TLS configuration")?; - let gateway_hostname = conf.hostname.clone(); - - // -- 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( - gateway_hostname.clone(), - tls_conf.acceptor.clone(), - )); - - // -- Dual handshake with the client and the server until the TLS security upgrade -- // - - let mut client_framed = - 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_injection.target_credential(), - ) - .await?; - - let client_stream = client_framed.into_inner_no_leftover(); - let server_stream = server_framed.into_inner_no_leftover(); - - // -- Perform the TLS upgrading for both the client and the server, effectively acting as a man-in-the-middle -- // - - let client_tls_upgrade_fut = tls_conf.acceptor.accept(client_stream); - let server_tls_upgrade_fut = crate::tls::dangerous_connect(server_dns_name.clone(), server_stream); - - let (client_stream, server_stream) = tokio::join!(client_tls_upgrade_fut, server_tls_upgrade_fut); - - let client_stream = client_stream.context("TLS upgrade with client failed")?; - let server_stream = server_stream.context("TLS upgrade with server failed")?; - - let server_public_key = - crate::tls::extract_stream_peer_public_key(&server_stream).context("extract target server TLS public key")?; - - let gateway_cert_chain = gateway_cert_chain_handle.await??; - let gateway_public_key = crate::tls::extract_public_key(gateway_cert_chain.first().context("no leaf")?) - .context("extract Gateway public key")?; - - RdpCredsspProxy::builder() - .conf(conf) - .session_info(session_info) - .client_stream(client_stream) - .client_addr(client_addr) - .server_stream(server_stream) - .server_addr(server_addr) - .credential_injection(credential_injection) - .sessions(sessions) - .subscriber_tx(subscriber_tx) - .server_dns_name(server_dns_name) - .disconnect_interest(disconnect_interest) - .kdc_connector(kdc_connector) - .gateway_public_key(gateway_public_key) - .server_public_key(server_public_key) - .client_security_protocol(handshake_result.client_security_protocol) - .server_security_protocol(handshake_result.server_security_protocol) - .build() - .run() - .await -} - -impl RdpCredsspProxy -where - C: AsyncRead + AsyncWrite + Unpin + Send, - S: AsyncRead + AsyncWrite + Unpin + Send, -{ - pub(crate) async fn run(self) -> anyhow::Result<()> { - let Self { - conf, - session_info, - client_stream, - client_addr, - server_stream, - server_addr, - credential_injection, - sessions, - subscriber_tx, - server_dns_name, - disconnect_interest, - kdc_connector, - gateway_public_key, - server_public_key, - client_security_protocol, - server_security_protocol, - } = self; - - let mut client_framed = ironrdp_tokio::MovableTokioFramed::new(client_stream); - let mut server_framed = ironrdp_tokio::MovableTokioFramed::new(server_stream); - - let (server_kerberos_config, client_kerberos_config) = credential_injection - .kerberos_configs(client_addr, &conf.hostname)? - .unzip(); - - let client_credssp_fut = perform_credssp_as_server( - &mut client_framed, - client_addr.ip(), - gateway_public_key, - client_security_protocol, - credential_injection.proxy_credential(), - server_kerberos_config, - &credential_injection, - &kdc_connector, - ); - - let server_credssp_fut = perform_credssp_as_client( - &mut server_framed, - server_dns_name, - server_public_key, - server_security_protocol, - credential_injection.target_credential(), - client_kerberos_config, - &kdc_connector, - ); - - let (client_credssp_res, server_credssp_res) = tokio::join!(client_credssp_fut, server_credssp_fut); - client_credssp_res.context("CredSSP with client")?; - server_credssp_res.context("CredSSP with server")?; - - intercept_connect_confirm(&mut client_framed, &mut server_framed, server_security_protocol).await?; - - let (mut client_stream, client_leftover) = client_framed.into_inner(); - let (mut server_stream, server_leftover) = server_framed.into_inner(); - - info!("RDP-TLS forwarding (credential injection)"); - - client_stream - .write_all(&server_leftover) - .await - .context("write server leftover to client")?; - - server_stream - .write_all(&client_leftover) - .await - .context("write client leftover to server")?; - - Proxy::builder() - .conf(conf) - .session_info(session_info) - .address_a(client_addr) - .transport_a(client_stream) - .address_b(server_addr) - .transport_b(server_stream) - .sessions(sessions) - .subscriber_tx(subscriber_tx) - .disconnect_interest(disconnect_interest) - .build() - .select_dissector_and_forward() - .await - .context("RDP-TLS traffic proxying failed") - } -} - -#[derive(Debug)] -struct HandshakeResult { - client_security_protocol: nego::SecurityProtocol, - server_security_protocol: nego::SecurityProtocol, -} - -#[instrument(level = "debug", ret, skip_all)] -async fn intercept_connect_confirm( - client_framed: &mut ironrdp_tokio::MovableTokioFramed, - server_framed: &mut ironrdp_tokio::MovableTokioFramed, - server_security_protocol: nego::SecurityProtocol, -) -> anyhow::Result<()> -where - C: AsyncWrite + AsyncRead + Unpin + Send, - S: AsyncWrite + AsyncRead + Unpin + Send, -{ - let (_, received_frame) = client_framed - .read_pdu() - .await - .context("read MCS Connect Initial from client")?; - let received_connect_initial: x224::X224> = - ironrdp_core::decode(&received_frame).context("decode PDU from client")?; - let mut received_connect_initial: mcs::ConnectInitial = - ironrdp_core::decode(&received_connect_initial.0.data).context("decode Connect Initial PDU")?; - trace!(message = ?received_connect_initial, "Received Connect Initial PDU from client"); - - let mut gcc_blocks = received_connect_initial.conference_create_request.into_gcc_blocks(); - gcc_blocks.core.optional_data.server_selected_protocol = Some(server_security_protocol); - // Update the conference request with modified gcc_blocks. - received_connect_initial.conference_create_request = ironrdp_pdu::gcc::ConferenceCreateRequest::new(gcc_blocks)?; - trace!(message = ?received_connect_initial, "Send Connection Request PDU to server"); - let x224_msg_buf = ironrdp_core::encode_vec(&received_connect_initial)?; - let pdu = x224::X224Data { - data: std::borrow::Cow::Owned(x224_msg_buf), - }; - send_pdu(server_framed, &x224::X224(pdu)) - .await - .context("send connection request to server")?; - - Ok(()) -} - -#[instrument(name = "dual_handshake", level = "debug", ret, skip_all)] -async fn dual_handshake_until_tls_upgrade( - client_framed: &mut ironrdp_tokio::MovableTokioFramed, - server_framed: &mut ironrdp_tokio::MovableTokioFramed, - target_credential: &AppCredential, -) -> anyhow::Result -where - C: AsyncWrite + AsyncRead + Unpin + Send, - S: AsyncWrite + AsyncRead + Unpin + Send, -{ - let (_, received_frame) = client_framed.read_pdu().await.context("read PDU from client")?; - let received_connection_request: x224::X224 = - ironrdp_core::decode(&received_frame).context("decode PDU from client")?; - trace!(message = ?received_connection_request, "Received Connection Request PDU from client"); - - // Choose the security protocol to use with the client. - let received_connection_request_protocol = received_connection_request.0.protocol; - let client_security_protocol = if received_connection_request_protocol.contains(nego::SecurityProtocol::HYBRID_EX) { - nego::SecurityProtocol::HYBRID_EX - } else if received_connection_request - .0 - .protocol - .contains(nego::SecurityProtocol::HYBRID) - { - nego::SecurityProtocol::HYBRID - } else { - anyhow::bail!( - "client does not support CredSSP (received {})", - received_connection_request.0.protocol - ) - }; - - let connection_request_to_send = nego::ConnectionRequest { - nego_data: match target_credential { - AppCredential::UsernamePassword { username, .. } => { - Some(nego::NegoRequestData::cookie(username.to_owned())) - } - }, - flags: received_connection_request.0.flags, - // https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpbcgr/902b090b-9cb3-4efc-92bf-ee13373371e3 - // - // The spec states that `PROTOCOL_SSL` "SHOULD" also be set when using `PROTOCOL_HYBRID`: - // - // > PROTOCOL_HYBRID (0x00000002) - // > Credential Security Support Provider protocol (CredSSP) (section 5.4.5.2). - // > If this flag is set, then the PROTOCOL_SSL (0x00000001) flag SHOULD also be set - // > because Transport Layer Security (TLS) is a subset of CredSSP. - // - // However, in practice `mstsc` is picky about these flags: it expects the - // SupportedProtocol bits in the ConnectionRequestPDU that reach the target - // server to match what the client originally sent. If the proxy modifies - // them (for example, forcing HYBRID | HYBRID_EX and/or clearing SSL), - // the connection can fail with an authentication error (Code: 0x609). - // - // We therefore *do not* synthesize a new protocol bitmask here anymore. - // Instead, we forward the client's SupportedProtocol flags as-is and - // enforce our policy by validating them: if HYBRID / HYBRID_EX are not - // present (i.e. NLA is not negotiated), we fail the connection rather - // than trying to "fix" the flags ourselves. - // - // See also: https://serverfault.com/a/720161 - protocol: received_connection_request_protocol, - }; - trace!(?connection_request_to_send, "Send Connection Request PDU to server"); - send_pdu(server_framed, &x224::X224(connection_request_to_send)) - .await - .context("send connection request to server")?; - - let (_, received_frame) = server_framed.read_pdu().await.context("read PDU from server")?; - let received_connection_confirm: x224::X224 = - ironrdp_core::decode(&received_frame).context("decode PDU from server")?; - trace!(message = ?received_connection_confirm, "Received Connection Confirm PDU from server"); - - let (connection_confirm_to_send, handshake_result) = match &received_connection_confirm.0 { - nego::ConnectionConfirm::Response { - flags, - protocol: server_security_protocol, - } => { - debug!(?server_security_protocol, ?flags, "Server confirmed connection"); - - let result = if !server_security_protocol - .intersects(nego::SecurityProtocol::HYBRID | nego::SecurityProtocol::HYBRID_EX) - { - Err(anyhow::anyhow!( - "server selected security protocol {server_security_protocol}, which is not supported for credential injection" - )) - } else { - Ok(HandshakeResult { - client_security_protocol, - server_security_protocol: *server_security_protocol, - }) - }; - - ( - x224::X224(nego::ConnectionConfirm::Response { - flags: *flags, - protocol: client_security_protocol, - }), - result, - ) - } - nego::ConnectionConfirm::Failure { code } => ( - x224::X224(received_connection_confirm.0.clone()), - Err(anyhow::anyhow!("RDP session initiation failed with code {code}")), - ), - }; - - trace!(?connection_confirm_to_send, "Send Connection Request PDU to client"); - send_pdu(client_framed, &connection_confirm_to_send) - .await - .context("send connection confirm to client")?; - - handshake_result -} - -#[instrument(name = "server_credssp", level = "debug", ret, skip_all)] -async fn perform_credssp_as_client( - framed: &mut ironrdp_tokio::Framed, - server_name: String, - server_public_key: Vec, - security_protocol: nego::SecurityProtocol, - credentials: &AppCredential, - kerberos_config: Option, - kdc_connector: &KdcConnector, -) -> anyhow::Result<()> -where - S: ironrdp_tokio::FramedRead + ironrdp_tokio::FramedWrite, -{ - use ironrdp_tokio::FramedWrite as _; - - // Decrypt password into short-lived buffer. - let (username, decrypted_password) = credentials - .decrypt_password() - .context("failed to decrypt credentials")?; - - let credentials = ironrdp_connector::Credentials::UsernamePassword { - username, - password: decrypted_password.expose_secret().to_owned(), - }; - // decrypted_password drops here, zeroizing its buffer; note: a copy of the plaintext - // remains in `credentials` above, which is a regular String (downstream API limitation). - - let (mut sequence, mut ts_request) = ironrdp_connector::credssp::CredsspSequence::init( - credentials, - None, - security_protocol, - ironrdp_connector::ServerName::new(server_name.clone()), - server_public_key, - kerberos_config, - )?; - - let mut buf = ironrdp_pdu::WriteBuf::new(); - - loop { - let client_state = { - let mut generator = sequence.process_ts_request(ts_request); - resolve_client_generator(&mut generator, kdc_connector).await? - }; // drop generator - - buf.clear(); - let written = sequence.handle_process_result(client_state, &mut buf)?; - - if let Some(response_len) = written.size() { - let response = &buf[..response_len]; - framed - .write_all(response) - .await - .map_err(|e| ironrdp_connector::custom_err!("write all", e))?; - } - - let Some(next_pdu_hint) = sequence.next_pdu_hint() else { - break; - }; - - let pdu = framed.read_by_hint(next_pdu_hint).await.context("read frame by hint")?; - - if let Some(next_request) = sequence.decode_server_message(&pdu)? { - ts_request = next_request; - } else { - break; - } - } - - Ok(()) -} - -async fn resolve_server_generator( - generator: &mut CredsspServerProcessGenerator<'_>, - credential_injection: &CredentialInjection, - kdc_connector: &KdcConnector, -) -> Result { - let mut state = generator.start(); - - loop { - match state { - GeneratorState::Suspended(request) => { - let response = match credential_injection.intercept_network_request(&request) { - Ok(SyntheticKdcInterception::Intercepted(response)) => Ok(response), - Ok(SyntheticKdcInterception::NotInjectionRealm(mismatch)) => Err(anyhow::anyhow!( - "kdc request realm does not match credential-injection session realm: {mismatch}" - )), - Ok(SyntheticKdcInterception::NotInjectionRequest) => { - kdc_connector.send_network_request(&request).await - } - 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)); - } - GeneratorState::Completed(client_state) => { - break client_state; - } - } - } -} - -async fn resolve_client_generator( - generator: &mut CredsspClientProcessGenerator<'_>, - kdc_connector: &KdcConnector, -) -> anyhow::Result { - let mut state = generator.start(); - - loop { - match state { - GeneratorState::Suspended(request) => { - let response = kdc_connector.send_network_request(&request).await?; - state = generator.resume(Ok(response)); - } - GeneratorState::Completed(client_state) => { - break Ok(client_state.map_err(|e| { - ironrdp_connector::ConnectorError::new("CredSSP", ironrdp_connector::ConnectorErrorKind::Credssp(e)) - })?); - } - }; - } -} - -#[expect(clippy::too_many_arguments)] -#[instrument(name = "client_credssp", level = "debug", ret, skip_all)] -async fn perform_credssp_as_server( - framed: &mut ironrdp_tokio::Framed, - client_addr: IpAddr, - gateway_public_key: Vec, - security_protocol: nego::SecurityProtocol, - credentials: &AppCredential, - kerberos_server_config: Option, - credential_injection: &CredentialInjection, - kdc_connector: &KdcConnector, -) -> anyhow::Result<()> -where - S: ironrdp_tokio::FramedRead + ironrdp_tokio::FramedWrite, -{ - use ironrdp_connector::sspi::credssp::EarlyUserAuthResult; - use ironrdp_tokio::FramedWrite as _; - - let mut buf = ironrdp_pdu::WriteBuf::new(); - - // Are we supposed to use the actual computer name of the client? - // But this does not seem to matter so far, so we stringify the IP address of the client instead. - let client_computer_name = ironrdp_connector::ServerName::new(client_addr.to_string()); - - let result = credssp_loop( - framed, - &mut buf, - client_computer_name, - gateway_public_key, - credentials, - kerberos_server_config, - credential_injection, - kdc_connector, - ) - .await; - - if security_protocol.intersects(nego::SecurityProtocol::HYBRID_EX) { - trace!(?result, "HYBRID_EX"); - - let result = if result.is_ok() { - EarlyUserAuthResult::Success - } else { - EarlyUserAuthResult::AccessDenied - }; - - buf.clear(); - result.to_buffer(&mut buf).context("write early user auth result")?; - let response = &buf[..result.buffer_len()]; - framed.write_all(response).await.context("write_all")?; - } - - return result; - - async fn credssp_loop( - framed: &mut ironrdp_tokio::Framed, - buf: &mut ironrdp_pdu::WriteBuf, - client_computer_name: ironrdp_connector::ServerName, - public_key: Vec, - credentials: &AppCredential, - kerberos_server_config: Option, - credential_injection: &CredentialInjection, - kdc_connector: &KdcConnector, - ) -> anyhow::Result<()> - where - S: ironrdp_tokio::FramedRead + ironrdp_tokio::FramedWrite, - { - // Decrypt password into short-lived buffer. - let (username, decrypted_password) = credentials - .decrypt_password() - .context("failed to decrypt credentials")?; - - let username = sspi::Username::parse(&username).context("invalid username")?; - - let identity = sspi::AuthIdentity { - username, - password: decrypted_password.expose_secret().to_owned().into(), - }; - // decrypted_password drops here, zeroizing its buffer; note: a copy of the plaintext - // remains in `identity` above (downstream API limitation). - - let mut sequence = ironrdp_acceptor::credssp::CredsspSequence::init( - &identity, - client_computer_name, - public_key, - kerberos_server_config, - )?; - - loop { - let Some(next_pdu_hint) = sequence.next_pdu_hint()? else { - break; - }; - - let pdu = framed - .read_by_hint(next_pdu_hint) - .await - .map_err(|e| ironrdp_connector::custom_err!("read frame by hint", e))?; - - let Some(ts_request) = sequence.decode_client_message(&pdu)? else { - break; - }; - - let result = { - let mut generator = sequence.process_ts_request(ts_request); - resolve_server_generator(&mut generator, credential_injection, kdc_connector).await - }; // drop generator - - buf.clear(); - let written = sequence.handle_process_result(result, buf)?; - - if let Some(response_len) = written.size() { - let response = &buf[..response_len]; - framed - .write_all(response) - .await - .map_err(|e| ironrdp_connector::custom_err!("write all", e))?; - } - } - - Ok(()) - } -} - -async fn send_pdu(framed: &mut ironrdp_tokio::MovableTokioFramed, pdu: &P) -> anyhow::Result<()> -where - S: AsyncWrite + Unpin + Send, - P: ironrdp_core::Encode, -{ - use ironrdp_tokio::FramedWrite as _; - - let payload = ironrdp_core::encode_vec(pdu).context("failed to encode PDU")?; - framed.write_all(&payload).await.context("failed to write PDU")?; - Ok(()) -} diff --git a/devolutions-gateway/src/rdp_proxy/credssp.rs b/devolutions-gateway/src/rdp_proxy/credssp.rs new file mode 100644 index 000000000..f8bc293fc --- /dev/null +++ b/devolutions-gateway/src/rdp_proxy/credssp.rs @@ -0,0 +1,430 @@ +use std::net::SocketAddr; +use std::sync::Arc; + +use anyhow::Context as _; +use ironrdp_acceptor::credssp::CredsspProcessGenerator as CredsspServerProcessGenerator; +use ironrdp_connector::credssp::CredsspProcessGenerator as CredsspClientProcessGenerator; +use ironrdp_connector::sspi; +use ironrdp_connector::sspi::generator::GeneratorState; +use ironrdp_pdu::{mcs, nego, x224}; +use secrecy::ExposeSecret as _; +use tokio::io::{AsyncRead, AsyncWrite, AsyncWriteExt as _}; +use typed_builder::TypedBuilder; + +use super::send_pdu; +use crate::config::Conf; +use crate::credential::AppCredential; +use crate::credential_injection_kdc::{CredentialInjection, SyntheticKdcInterception}; +use crate::kdc_connector::KdcConnector; +use crate::proxy::Proxy; +use crate::session::{DisconnectInterest, SessionInfo, SessionMessageSender}; +use crate::subscriber::SubscriberSender; + +#[derive(TypedBuilder)] +pub(crate) struct CredsspSession { + conf: Arc, + session_info: SessionInfo, + client_addr: SocketAddr, + server_addr: SocketAddr, + credential_injection: CredentialInjection, + sessions: SessionMessageSender, + subscriber_tx: SubscriberSender, + server_dns_name: String, + disconnect_interest: Option, + kdc_connector: KdcConnector, +} + +#[derive(TypedBuilder)] +pub(crate) struct PreparedCredssp { + client_stream: C, + server_stream: S, + gateway_public_key: Vec, + server_public_key: Vec, + client_security_protocol: nego::SecurityProtocol, + server_security_protocol: nego::SecurityProtocol, +} + +impl CredsspSession { + pub(super) fn conf(&self) -> &Conf { + &self.conf + } + + pub(super) fn server_dns_name(&self) -> &str { + &self.server_dns_name + } + + pub(super) fn target_credential(&self) -> &AppCredential { + self.credential_injection.target_credential() + } + + pub(crate) async fn run(self, prepared: PreparedCredssp) -> anyhow::Result<()> + where + C: AsyncRead + AsyncWrite + Unpin + Send, + S: AsyncRead + AsyncWrite + Unpin + Send, + { + let Self { + conf, + session_info, + client_addr, + server_addr, + credential_injection, + sessions, + subscriber_tx, + server_dns_name, + disconnect_interest, + kdc_connector, + } = self; + let PreparedCredssp { + client_stream, + server_stream, + gateway_public_key, + server_public_key, + client_security_protocol, + server_security_protocol, + } = prepared; + + let mut client_framed = ironrdp_tokio::MovableTokioFramed::new(client_stream); + let mut server_framed = ironrdp_tokio::MovableTokioFramed::new(server_stream); + + let (server_kerberos_config, client_kerberos_config) = credential_injection + .kerberos_configs(client_addr, &conf.hostname)? + .unzip(); + + let client_credssp_fut = perform_credssp_as_server( + &mut client_framed, + client_addr.ip(), + gateway_public_key, + client_security_protocol, + credential_injection.proxy_credential(), + server_kerberos_config, + &credential_injection, + &kdc_connector, + ); + + let server_credssp_fut = perform_credssp_as_client( + &mut server_framed, + server_dns_name, + server_public_key, + server_security_protocol, + credential_injection.target_credential(), + client_kerberos_config, + &kdc_connector, + ); + + let (client_credssp_res, server_credssp_res) = tokio::join!(client_credssp_fut, server_credssp_fut); + client_credssp_res.context("CredSSP with client")?; + server_credssp_res.context("CredSSP with server")?; + + intercept_connect_confirm(&mut client_framed, &mut server_framed, server_security_protocol).await?; + + let (mut client_stream, client_leftover) = client_framed.into_inner(); + let (mut server_stream, server_leftover) = server_framed.into_inner(); + + info!("RDP-TLS forwarding (credential injection)"); + + client_stream + .write_all(&server_leftover) + .await + .context("write server leftover to client")?; + + server_stream + .write_all(&client_leftover) + .await + .context("write client leftover to server")?; + + Proxy::builder() + .conf(conf) + .session_info(session_info) + .address_a(client_addr) + .transport_a(client_stream) + .address_b(server_addr) + .transport_b(server_stream) + .sessions(sessions) + .subscriber_tx(subscriber_tx) + .disconnect_interest(disconnect_interest) + .build() + .select_dissector_and_forward() + .await + .context("RDP-TLS traffic proxying failed") + } +} + +#[instrument(level = "debug", ret, skip_all)] +async fn intercept_connect_confirm( + client_framed: &mut ironrdp_tokio::MovableTokioFramed, + server_framed: &mut ironrdp_tokio::MovableTokioFramed, + server_security_protocol: nego::SecurityProtocol, +) -> anyhow::Result<()> +where + C: AsyncWrite + AsyncRead + Unpin + Send, + S: AsyncWrite + AsyncRead + Unpin + Send, +{ + let (_, received_frame) = client_framed + .read_pdu() + .await + .context("read MCS Connect Initial from client")?; + let received_connect_initial: x224::X224> = + ironrdp_core::decode(&received_frame).context("decode PDU from client")?; + let mut received_connect_initial: mcs::ConnectInitial = + ironrdp_core::decode(&received_connect_initial.0.data).context("decode Connect Initial PDU")?; + trace!(message = ?received_connect_initial, "Received Connect Initial PDU from client"); + + let mut gcc_blocks = received_connect_initial.conference_create_request.into_gcc_blocks(); + gcc_blocks.core.optional_data.server_selected_protocol = Some(server_security_protocol); + received_connect_initial.conference_create_request = ironrdp_pdu::gcc::ConferenceCreateRequest::new(gcc_blocks)?; + trace!(message = ?received_connect_initial, "Send Connection Request PDU to server"); + let x224_msg_buf = ironrdp_core::encode_vec(&received_connect_initial)?; + let pdu = x224::X224Data { + data: std::borrow::Cow::Owned(x224_msg_buf), + }; + send_pdu(server_framed, &x224::X224(pdu)) + .await + .context("send connection request to server")?; + + Ok(()) +} + +#[instrument(name = "server_credssp", level = "debug", ret, skip_all)] +async fn perform_credssp_as_client( + framed: &mut ironrdp_tokio::Framed, + server_name: String, + server_public_key: Vec, + security_protocol: nego::SecurityProtocol, + credentials: &AppCredential, + kerberos_config: Option, + kdc_connector: &KdcConnector, +) -> anyhow::Result<()> +where + S: ironrdp_tokio::FramedRead + ironrdp_tokio::FramedWrite, +{ + use ironrdp_tokio::FramedWrite as _; + + let (username, decrypted_password) = credentials + .decrypt_password() + .context("failed to decrypt credentials")?; + + let credentials = ironrdp_connector::Credentials::UsernamePassword { + username, + password: decrypted_password.expose_secret().to_owned(), + }; + + let (mut sequence, mut ts_request) = ironrdp_connector::credssp::CredsspSequence::init( + credentials, + None, + security_protocol, + ironrdp_connector::ServerName::new(server_name.clone()), + server_public_key, + kerberos_config, + )?; + + let mut buf = ironrdp_pdu::WriteBuf::new(); + + loop { + let client_state = { + let mut generator = sequence.process_ts_request(ts_request); + resolve_client_generator(&mut generator, kdc_connector).await? + }; + + buf.clear(); + let written = sequence.handle_process_result(client_state, &mut buf)?; + + if let Some(response_len) = written.size() { + let response = &buf[..response_len]; + framed + .write_all(response) + .await + .map_err(|error| ironrdp_connector::custom_err!("write all", error))?; + } + + let Some(next_pdu_hint) = sequence.next_pdu_hint() else { + break; + }; + + let pdu = framed.read_by_hint(next_pdu_hint).await.context("read frame by hint")?; + + if let Some(next_request) = sequence.decode_server_message(&pdu)? { + ts_request = next_request; + } else { + break; + } + } + + Ok(()) +} + +async fn resolve_server_generator( + generator: &mut CredsspServerProcessGenerator<'_>, + credential_injection: &CredentialInjection, + kdc_connector: &KdcConnector, +) -> Result { + let mut state = generator.start(); + + loop { + match state { + GeneratorState::Suspended(request) => { + let response = match credential_injection.intercept_network_request(&request) { + Ok(SyntheticKdcInterception::Intercepted(response)) => Ok(response), + Ok(SyntheticKdcInterception::NotInjectionRealm(mismatch)) => Err(anyhow::anyhow!( + "kdc request realm does not match credential-injection session realm: {mismatch}" + )), + Ok(SyntheticKdcInterception::NotInjectionRequest) => { + kdc_connector.send_network_request(&request).await + } + Err(error) => Err(error), + } + .map_err(|error| sspi::credssp::ServerError { + ts_request: None, + error: sspi::Error::new(sspi::ErrorKind::InternalError, error), + })?; + + state = generator.resume(Ok(response)); + } + GeneratorState::Completed(client_state) => { + break client_state; + } + } + } +} + +async fn resolve_client_generator( + generator: &mut CredsspClientProcessGenerator<'_>, + kdc_connector: &KdcConnector, +) -> anyhow::Result { + let mut state = generator.start(); + + loop { + match state { + GeneratorState::Suspended(request) => { + let response = kdc_connector.send_network_request(&request).await?; + state = generator.resume(Ok(response)); + } + GeneratorState::Completed(client_state) => { + break Ok(client_state.map_err(|error| { + ironrdp_connector::ConnectorError::new( + "CredSSP", + ironrdp_connector::ConnectorErrorKind::Credssp(error), + ) + })?); + } + }; + } +} + +#[expect(clippy::too_many_arguments)] +#[instrument(name = "client_credssp", level = "debug", ret, skip_all)] +async fn perform_credssp_as_server( + framed: &mut ironrdp_tokio::Framed, + client_addr: std::net::IpAddr, + gateway_public_key: Vec, + security_protocol: nego::SecurityProtocol, + credentials: &AppCredential, + kerberos_server_config: Option, + credential_injection: &CredentialInjection, + kdc_connector: &KdcConnector, +) -> anyhow::Result<()> +where + S: ironrdp_tokio::FramedRead + ironrdp_tokio::FramedWrite, +{ + use ironrdp_connector::sspi::credssp::EarlyUserAuthResult; + use ironrdp_tokio::FramedWrite as _; + + let mut buf = ironrdp_pdu::WriteBuf::new(); + let client_computer_name = ironrdp_connector::ServerName::new(client_addr.to_string()); + + let result = credssp_loop( + framed, + &mut buf, + client_computer_name, + gateway_public_key, + credentials, + kerberos_server_config, + credential_injection, + kdc_connector, + ) + .await; + + if security_protocol.intersects(nego::SecurityProtocol::HYBRID_EX) { + trace!(?result, "HYBRID_EX"); + + let result = if result.is_ok() { + EarlyUserAuthResult::Success + } else { + EarlyUserAuthResult::AccessDenied + }; + + buf.clear(); + result.to_buffer(&mut buf).context("write early user auth result")?; + let response = &buf[..result.buffer_len()]; + framed.write_all(response).await.context("write_all")?; + } + + result +} + +#[expect(clippy::too_many_arguments)] +async fn credssp_loop( + framed: &mut ironrdp_tokio::Framed, + buf: &mut ironrdp_pdu::WriteBuf, + client_computer_name: ironrdp_connector::ServerName, + public_key: Vec, + credentials: &AppCredential, + kerberos_server_config: Option, + credential_injection: &CredentialInjection, + kdc_connector: &KdcConnector, +) -> anyhow::Result<()> +where + S: ironrdp_tokio::FramedRead + ironrdp_tokio::FramedWrite, +{ + use ironrdp_tokio::FramedWrite as _; + + let (username, decrypted_password) = credentials + .decrypt_password() + .context("failed to decrypt credentials")?; + + let username = sspi::Username::parse(&username).context("invalid username")?; + + let identity = sspi::AuthIdentity { + username, + password: decrypted_password.expose_secret().to_owned().into(), + }; + + let mut sequence = ironrdp_acceptor::credssp::CredsspSequence::init( + &identity, + client_computer_name, + public_key, + kerberos_server_config, + )?; + + loop { + let Some(next_pdu_hint) = sequence.next_pdu_hint()? else { + break; + }; + + let pdu = framed + .read_by_hint(next_pdu_hint) + .await + .map_err(|error| ironrdp_connector::custom_err!("read frame by hint", error))?; + + let Some(ts_request) = sequence.decode_client_message(&pdu)? else { + break; + }; + + let result = { + let mut generator = sequence.process_ts_request(ts_request); + resolve_server_generator(&mut generator, credential_injection, kdc_connector).await + }; + + buf.clear(); + let written = sequence.handle_process_result(result, buf)?; + + if let Some(response_len) = written.size() { + let response = &buf[..response_len]; + framed + .write_all(response) + .await + .map_err(|error| ironrdp_connector::custom_err!("write all", error))?; + } + } + + Ok(()) +} diff --git a/devolutions-gateway/src/rdp_proxy/mod.rs b/devolutions-gateway/src/rdp_proxy/mod.rs new file mode 100644 index 000000000..73395efce --- /dev/null +++ b/devolutions-gateway/src/rdp_proxy/mod.rs @@ -0,0 +1,224 @@ +mod credssp; + +use anyhow::Context as _; +pub(crate) use credssp::{CredsspSession, PreparedCredssp}; +use ironrdp_pdu::{nego, x224}; +use tokio::io::{AsyncRead, AsyncWrite}; +use typed_builder::TypedBuilder; + +use crate::credential::AppCredential; + +#[derive(TypedBuilder)] +pub(crate) struct RdpProxy { + session: CredsspSession, + client_stream: C, + server_stream: S, + client_stream_leftover_bytes: bytes::BytesMut, +} + +impl RdpProxy +where + A: AsyncWrite + AsyncRead + Unpin + Send, + B: AsyncWrite + AsyncRead + Unpin + Send, +{ + pub(crate) async fn run(self) -> anyhow::Result<()> { + handle(self).await + } +} + +#[instrument("rdp_proxy", skip_all)] +async fn handle(proxy: RdpProxy) -> anyhow::Result<()> +where + C: AsyncRead + AsyncWrite + Unpin + Send, + S: AsyncRead + AsyncWrite + Unpin + Send, +{ + let RdpProxy { + session, + client_stream, + server_stream, + client_stream_leftover_bytes, + } = proxy; + + let tls_conf = session.conf().credssp_tls.get().context("CredSSP TLS configuration")?; + let gateway_hostname = session.conf().hostname.clone(); + + // -- 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( + gateway_hostname.clone(), + tls_conf.acceptor.clone(), + )); + + // -- Dual handshake with the client and the server until the TLS security upgrade -- // + + let mut client_framed = + 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, session.target_credential()).await?; + + let client_stream = client_framed.into_inner_no_leftover(); + let server_stream = server_framed.into_inner_no_leftover(); + + // -- Perform the TLS upgrading for both the client and the server, effectively acting as a man-in-the-middle -- // + + let client_tls_upgrade_fut = tls_conf.acceptor.accept(client_stream); + let server_tls_upgrade_fut = crate::tls::dangerous_connect(session.server_dns_name().to_owned(), server_stream); + + let (client_stream, server_stream) = tokio::join!(client_tls_upgrade_fut, server_tls_upgrade_fut); + + let client_stream = client_stream.context("TLS upgrade with client failed")?; + let server_stream = server_stream.context("TLS upgrade with server failed")?; + + let server_public_key = + crate::tls::extract_stream_peer_public_key(&server_stream).context("extract target server TLS public key")?; + + let gateway_cert_chain = gateway_cert_chain_handle.await??; + let gateway_public_key = crate::tls::extract_public_key(gateway_cert_chain.first().context("no leaf")?) + .context("extract Gateway public key")?; + + let prepared = PreparedCredssp::builder() + .client_stream(client_stream) + .server_stream(server_stream) + .gateway_public_key(gateway_public_key) + .server_public_key(server_public_key) + .client_security_protocol(handshake_result.client_security_protocol) + .server_security_protocol(handshake_result.server_security_protocol) + .build(); + + session.run(prepared).await +} + +#[derive(Debug)] +struct HandshakeResult { + client_security_protocol: nego::SecurityProtocol, + server_security_protocol: nego::SecurityProtocol, +} + +#[instrument(name = "dual_handshake", level = "debug", ret, skip_all)] +async fn dual_handshake_until_tls_upgrade( + client_framed: &mut ironrdp_tokio::MovableTokioFramed, + server_framed: &mut ironrdp_tokio::MovableTokioFramed, + target_credential: &AppCredential, +) -> anyhow::Result +where + C: AsyncWrite + AsyncRead + Unpin + Send, + S: AsyncWrite + AsyncRead + Unpin + Send, +{ + let (_, received_frame) = client_framed.read_pdu().await.context("read PDU from client")?; + let received_connection_request: x224::X224 = + ironrdp_core::decode(&received_frame).context("decode PDU from client")?; + trace!(message = ?received_connection_request, "Received Connection Request PDU from client"); + + // Choose the security protocol to use with the client. + let received_connection_request_protocol = received_connection_request.0.protocol; + let client_security_protocol = if received_connection_request_protocol.contains(nego::SecurityProtocol::HYBRID_EX) { + nego::SecurityProtocol::HYBRID_EX + } else if received_connection_request + .0 + .protocol + .contains(nego::SecurityProtocol::HYBRID) + { + nego::SecurityProtocol::HYBRID + } else { + anyhow::bail!( + "client does not support CredSSP (received {})", + received_connection_request.0.protocol + ) + }; + + let connection_request_to_send = nego::ConnectionRequest { + nego_data: match target_credential { + AppCredential::UsernamePassword { username, .. } => { + Some(nego::NegoRequestData::cookie(username.to_owned())) + } + }, + flags: received_connection_request.0.flags, + // https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpbcgr/902b090b-9cb3-4efc-92bf-ee13373371e3 + // + // The spec states that `PROTOCOL_SSL` "SHOULD" also be set when using `PROTOCOL_HYBRID`: + // + // > PROTOCOL_HYBRID (0x00000002) + // > Credential Security Support Provider protocol (CredSSP) (section 5.4.5.2). + // > If this flag is set, then the PROTOCOL_SSL (0x00000001) flag SHOULD also be set + // > because Transport Layer Security (TLS) is a subset of CredSSP. + // + // However, in practice `mstsc` is picky about these flags: it expects the + // SupportedProtocol bits in the ConnectionRequestPDU that reach the target + // server to match what the client originally sent. If the proxy modifies + // them (for example, forcing HYBRID | HYBRID_EX and/or clearing SSL), + // the connection can fail with an authentication error (Code: 0x609). + // + // We therefore *do not* synthesize a new protocol bitmask here anymore. + // Instead, we forward the client's SupportedProtocol flags as-is and + // enforce our policy by validating them: if HYBRID / HYBRID_EX are not + // present (i.e. NLA is not negotiated), we fail the connection rather + // than trying to "fix" the flags ourselves. + // + // See also: https://serverfault.com/a/720161 + protocol: received_connection_request_protocol, + }; + trace!(?connection_request_to_send, "Send Connection Request PDU to server"); + send_pdu(server_framed, &x224::X224(connection_request_to_send)) + .await + .context("send connection request to server")?; + + let (_, received_frame) = server_framed.read_pdu().await.context("read PDU from server")?; + let received_connection_confirm: x224::X224 = + ironrdp_core::decode(&received_frame).context("decode PDU from server")?; + trace!(message = ?received_connection_confirm, "Received Connection Confirm PDU from server"); + + let (connection_confirm_to_send, handshake_result) = match &received_connection_confirm.0 { + nego::ConnectionConfirm::Response { + flags, + protocol: server_security_protocol, + } => { + debug!(?server_security_protocol, ?flags, "Server confirmed connection"); + + let result = if !server_security_protocol + .intersects(nego::SecurityProtocol::HYBRID | nego::SecurityProtocol::HYBRID_EX) + { + Err(anyhow::anyhow!( + "server selected security protocol {server_security_protocol}, which is not supported for credential injection" + )) + } else { + Ok(HandshakeResult { + client_security_protocol, + server_security_protocol: *server_security_protocol, + }) + }; + + ( + x224::X224(nego::ConnectionConfirm::Response { + flags: *flags, + protocol: client_security_protocol, + }), + result, + ) + } + nego::ConnectionConfirm::Failure { code } => ( + x224::X224(received_connection_confirm.0.clone()), + Err(anyhow::anyhow!("RDP session initiation failed with code {code}")), + ), + }; + + trace!(?connection_confirm_to_send, "Send Connection Request PDU to client"); + send_pdu(client_framed, &connection_confirm_to_send) + .await + .context("send connection confirm to client")?; + + handshake_result +} + +async fn send_pdu(framed: &mut ironrdp_tokio::MovableTokioFramed, pdu: &P) -> anyhow::Result<()> +where + S: AsyncWrite + Unpin + Send, + P: ironrdp_core::Encode, +{ + use ironrdp_tokio::FramedWrite as _; + + let payload = ironrdp_core::encode_vec(pdu).context("failed to encode PDU")?; + framed.write_all(&payload).await.context("failed to write PDU")?; + Ok(()) +} From 4408b88b81db64c15983593277b19a4675017e51 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Mon, 27 Jul 2026 17:14:43 -0400 Subject: [PATCH 08/11] refactor(dgw): rename credential injection module --- devolutions-gateway/src/api/kdc_proxy.rs | 2 +- devolutions-gateway/src/api/rdp.rs | 2 +- ...ential_injection_kdc.rs => credential_injection.rs} | 10 ++++------ devolutions-gateway/src/generic_client.rs | 2 +- devolutions-gateway/src/lib.rs | 6 +++--- devolutions-gateway/src/rd_clean_path.rs | 2 +- devolutions-gateway/src/rdp_proxy/credssp.rs | 2 +- devolutions-gateway/src/service.rs | 2 +- 8 files changed, 13 insertions(+), 15 deletions(-) rename devolutions-gateway/src/{credential_injection_kdc.rs => credential_injection.rs} (98%) diff --git a/devolutions-gateway/src/api/kdc_proxy.rs b/devolutions-gateway/src/api/kdc_proxy.rs index 1b3d474e9..988cb83b4 100644 --- a/devolutions-gateway/src/api/kdc_proxy.rs +++ b/devolutions-gateway/src/api/kdc_proxy.rs @@ -5,7 +5,7 @@ use picky_krb::messages::KdcProxyMessage; use uuid::Uuid; use crate::DgwState; -use crate::credential_injection_kdc::{SyntheticKdcInterception, kdc_proxy_message_realm}; +use crate::credential_injection::{SyntheticKdcInterception, kdc_proxy_message_realm}; use crate::extract::KdcToken; use crate::http::HttpError; use crate::kdc_connector::KdcConnector; diff --git a/devolutions-gateway/src/api/rdp.rs b/devolutions-gateway/src/api/rdp.rs index 7dfdbde2f..5a93dcead 100644 --- a/devolutions-gateway/src/api/rdp.rs +++ b/devolutions-gateway/src/api/rdp.rs @@ -68,7 +68,7 @@ async fn handle_socket( subscriber_tx: SubscriberSender, active_recordings: Arc, source_addr: SocketAddr, - synthetic_kdc_registry: crate::credential_injection_kdc::SyntheticKdcRegistry, + synthetic_kdc_registry: crate::credential_injection::SyntheticKdcRegistry, provisioning: crate::provisioning::ProvisioningStore, agent_tunnel_handle: Option>, ) { diff --git a/devolutions-gateway/src/credential_injection_kdc.rs b/devolutions-gateway/src/credential_injection.rs similarity index 98% rename from devolutions-gateway/src/credential_injection_kdc.rs rename to devolutions-gateway/src/credential_injection.rs index 9c1deff07..4f5f15bf1 100644 --- a/devolutions-gateway/src/credential_injection_kdc.rs +++ b/devolutions-gateway/src/credential_injection.rs @@ -1,10 +1,8 @@ -//! In-memory Kerberos KDC used by proxy-based credential injection. +//! Proxy-based credential injection using Kerberos or NTLM. //! -//! This module owns the Kerberos side of credential injection end-to-end: -//! per-session fake-KDC material, the registry of live sessions, 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. +//! This module selects the authentication protocol and owns the credentials used by the proxy. +//! For Kerberos, it also owns per-session fake-KDC material, the registry of live sessions, +//! KDC proxy handling, and in-process KDC requests from the server-side CredSSP acceptor. use std::collections::HashMap; use std::fmt; diff --git a/devolutions-gateway/src/generic_client.rs b/devolutions-gateway/src/generic_client.rs index 4ab41a48d..17d552da1 100644 --- a/devolutions-gateway/src/generic_client.rs +++ b/devolutions-gateway/src/generic_client.rs @@ -8,7 +8,7 @@ use tracing::field; use typed_builder::TypedBuilder; use crate::config::Conf; -use crate::credential_injection_kdc::{CredentialInjection, SyntheticKdcRegistry}; +use crate::credential_injection::{CredentialInjection, SyntheticKdcRegistry}; use crate::provisioning::ProvisioningStore; use crate::proxy::Proxy; use crate::rdp_pcb::{extract_association_claims, read_pcb}; diff --git a/devolutions-gateway/src/lib.rs b/devolutions-gateway/src/lib.rs index c7ff98583..6752ddab0 100644 --- a/devolutions-gateway/src/lib.rs +++ b/devolutions-gateway/src/lib.rs @@ -17,7 +17,7 @@ pub mod api; pub mod cli; pub mod config; pub mod credential; -pub mod credential_injection_kdc; +pub mod credential_injection; pub mod extract; pub mod generic_client; pub mod http; @@ -64,7 +64,7 @@ pub struct DgwState { pub recordings: recording::RecordingMessageSender, pub job_queue_handle: job_queue::JobQueueHandle, pub provisioning: provisioning::ProvisioningStore, - pub synthetic_kdc_registry: credential_injection_kdc::SyntheticKdcRegistry, + pub synthetic_kdc_registry: credential_injection::SyntheticKdcRegistry, pub monitoring_state: Arc, pub traffic_audit_handle: traffic_audit::TrafficAuditHandle, pub agent_tunnel_handle: Option>, @@ -93,7 +93,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 provisioning = provisioning::ProvisioningStore::new(); - let synthetic_kdc_registry = credential_injection_kdc::SyntheticKdcRegistry::new(); + let synthetic_kdc_registry = credential_injection::SyntheticKdcRegistry::new(); let monitoring_state = Arc::new(network_monitor::State::new(Arc::new(MockMonitorsCache))?); let state = Self { diff --git a/devolutions-gateway/src/rd_clean_path.rs b/devolutions-gateway/src/rd_clean_path.rs index cee4c9be5..589be13fb 100644 --- a/devolutions-gateway/src/rd_clean_path.rs +++ b/devolutions-gateway/src/rd_clean_path.rs @@ -11,7 +11,7 @@ use tokio::io::{AsyncRead, AsyncReadExt as _, AsyncWrite, AsyncWriteExt as _}; use tracing::field; use crate::config::Conf; -use crate::credential_injection_kdc::{CredentialInjection, SyntheticKdcRegistry}; +use crate::credential_injection::{CredentialInjection, SyntheticKdcRegistry}; use crate::provisioning::ProvisioningStore; use crate::proxy::Proxy; use crate::recording::ActiveRecordings; diff --git a/devolutions-gateway/src/rdp_proxy/credssp.rs b/devolutions-gateway/src/rdp_proxy/credssp.rs index f8bc293fc..8909f187d 100644 --- a/devolutions-gateway/src/rdp_proxy/credssp.rs +++ b/devolutions-gateway/src/rdp_proxy/credssp.rs @@ -14,7 +14,7 @@ use typed_builder::TypedBuilder; use super::send_pdu; use crate::config::Conf; use crate::credential::AppCredential; -use crate::credential_injection_kdc::{CredentialInjection, SyntheticKdcInterception}; +use crate::credential_injection::{CredentialInjection, SyntheticKdcInterception}; use crate::kdc_connector::KdcConnector; use crate::proxy::Proxy; use crate::session::{DisconnectInterest, SessionInfo, SessionMessageSender}; diff --git a/devolutions-gateway/src/service.rs b/devolutions-gateway/src/service.rs index bb2ad4562..1f0175260 100644 --- a/devolutions-gateway/src/service.rs +++ b/devolutions-gateway/src/service.rs @@ -268,7 +268,7 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { .context("failed to initialize traffic audit manager")?; let provisioning = devolutions_gateway::provisioning::ProvisioningStore::new(); - let synthetic_kdc_registry = devolutions_gateway::credential_injection_kdc::SyntheticKdcRegistry::new(); + let synthetic_kdc_registry = devolutions_gateway::credential_injection::SyntheticKdcRegistry::new(); let filesystem_monitor_config_cache = devolutions_gateway::api::monitoring::FilesystemConfigCache::new( config::get_data_dir().join("monitors_cache.json"), From 426ece87f81380ef3cc55a8d5608a510ced72834 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Mon, 27 Jul 2026 17:34:16 -0400 Subject: [PATCH 09/11] refactor(dgw): make the synthetic-KDC registration a typestate Hold the registration in the `CredentialInjection::Kerberos` variant as a non-optional field instead of an always-`Some` `Option` on `KerberosCredentialInjection`. Holding a `CredentialInjection::Kerberos` now proves the synthetic KDC is registered and live for the session; it unpublishes on drop when the session ends. --- .../src/credential_injection.rs | 39 ++++++++++--------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/devolutions-gateway/src/credential_injection.rs b/devolutions-gateway/src/credential_injection.rs index 4f5f15bf1..3569df80d 100644 --- a/devolutions-gateway/src/credential_injection.rs +++ b/devolutions-gateway/src/credential_injection.rs @@ -34,7 +34,14 @@ use crate::target_addr::TargetAddr; const IN_PROCESS_KDC_HOST: &str = "cred.invalid"; pub(crate) enum CredentialInjection { - Kerberos(KerberosCredentialInjection), + // The registration proves the synthetic KDC is published in the registry: holding a + // `Kerberos` value means the KDC is live for the client's KKDCP lookups. It unpublishes on + // drop, so it rides inside the value that the RDP proxy owns for the whole session. + Kerberos( + KerberosCredentialInjection, + #[expect(dead_code, reason = "held only for its RAII Drop, which unpublishes the KDC at session end")] + SyntheticKdcRegistration, + ), Ntlm(NtlmCredentialInjection), } @@ -43,10 +50,6 @@ pub(crate) struct KerberosCredentialInjection { credentials: AppCredentials, target_kdc: TargetAddr, synthetic_kdc: Arc, - // Holds the synthetic KDC published in the registry for this session. It lives as long as this - // injection (moved into the RDP proxy), so the client's KKDCP lookups resolve for the whole - // session; dropping it unpublishes the KDC. `None` until registered. - registration: Option, } impl KerberosCredentialInjection { @@ -107,7 +110,7 @@ pub(crate) enum SyntheticKdcInterception { impl fmt::Debug for CredentialInjection { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - Self::Kerberos(injection) => f + Self::Kerberos(injection, _) => f .debug_struct("CredentialInjection::Kerberos") .field("jti", &injection.jti) .field("target_hostname", &injection.synthetic_kdc.target_hostname) @@ -130,17 +133,16 @@ pub(crate) enum CredentialInjectionInitializationResult { impl CredentialInjectionInitializationResult { pub(crate) fn maybe_register_synthetic_kdc(self, registry: &SyntheticKdcRegistry) -> CredentialInjection { match self { - Self::Kerberos(mut injection) => { - // Keep the registration alive for the session: it rides inside the returned - // injection (moved into the RDP proxy). Dropping it here would immediately - // unpublish the synthetic KDC and the client's KKDCP lookups would fail. - let session = injection.synthetic_kdc(); - injection.registration = Some(registry.register(session)); + Self::Kerberos(injection) => { + // The registration rides inside the returned CredentialInjection (owned by the RDP + // proxy), so the synthetic KDC stays published for the whole session and the + // client's KKDCP lookups resolve; it unpublishes on drop when the session ends. + let registration = registry.register(injection.synthetic_kdc()); debug!( jti = %injection.jti, "registered synthetic KDC for credential-injection session" ); - CredentialInjection::Kerberos(injection) + CredentialInjection::Kerberos(injection, registration) } Self::Ntlm(injection) => CredentialInjection::Ntlm(injection), } @@ -188,7 +190,6 @@ impl CredentialInjection { credentials, target_kdc, synthetic_kdc: Arc::new(synthetic_kdc), - registration: None, }, )) } else { @@ -201,21 +202,21 @@ impl CredentialInjection { pub(crate) fn jti(&self) -> Uuid { match self { - Self::Kerberos(injection) => injection.jti, + Self::Kerberos(injection, _) => injection.jti, Self::Ntlm(injection) => injection.jti, } } pub(crate) fn proxy_credential(&self) -> &AppCredential { match self { - Self::Kerberos(injection) => &injection.credentials.proxy, + Self::Kerberos(injection, _) => &injection.credentials.proxy, Self::Ntlm(injection) => &injection.credentials.proxy, } } pub(crate) fn target_credential(&self) -> &AppCredential { match self { - Self::Kerberos(injection) => &injection.credentials.target, + Self::Kerberos(injection, _) => &injection.credentials.target, Self::Ntlm(injection) => &injection.credentials.target, } } @@ -226,7 +227,7 @@ impl CredentialInjection { gateway_hostname: &str, ) -> anyhow::Result> { match self { - Self::Kerberos(injection) => Ok(Some(( + Self::Kerberos(injection, _) => Ok(Some(( injection.synthetic_kdc.server_kerberos_config(client_addr)?, ironrdp_connector::credssp::KerberosConfig { kdc_proxy_url: Some( @@ -244,7 +245,7 @@ impl CredentialInjection { request: &NetworkRequest, ) -> anyhow::Result { match self { - Self::Kerberos(injection) => injection.synthetic_kdc.intercept_network_request(request), + Self::Kerberos(injection, _) => injection.synthetic_kdc.intercept_network_request(request), Self::Ntlm(_) => Ok(SyntheticKdcInterception::NotInjectionRequest), } } From 4302ef4805d95f78dc132417641fa745099a811f Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Tue, 28 Jul 2026 09:55:41 -0400 Subject: [PATCH 10/11] style(dgw): satisfy rustfmt on the KDC registration attribute --- devolutions-gateway/src/credential_injection.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/devolutions-gateway/src/credential_injection.rs b/devolutions-gateway/src/credential_injection.rs index 3569df80d..35ab3e21a 100644 --- a/devolutions-gateway/src/credential_injection.rs +++ b/devolutions-gateway/src/credential_injection.rs @@ -39,7 +39,10 @@ pub(crate) enum CredentialInjection { // drop, so it rides inside the value that the RDP proxy owns for the whole session. Kerberos( KerberosCredentialInjection, - #[expect(dead_code, reason = "held only for its RAII Drop, which unpublishes the KDC at session end")] + #[expect( + dead_code, + reason = "held only for its RAII Drop, which unpublishes the KDC at session end" + )] SyntheticKdcRegistration, ), Ntlm(NtlmCredentialInjection), From f804807af1652fa51997ef03b70c7c0cc653dfd5 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Tue, 28 Jul 2026 11:17:23 -0400 Subject: [PATCH 11/11] refactor(dgw): split provisioning into credentials and connection options MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Separate `provision-credentials` and `provision-connection-options` preflight operations, backed by two independent JTI-keyed stores. The credentials store keeps the encryption boundary; the connection-options store holds plaintext routing metadata with no crypto dependency. The two halves are provisioned, expire and are replaced independently — a caller that wants both sends two operations in one batched preflight request. `ProvisioningStore::get` assembles a combined view for RDP credential injection: credentials required, connection options optional. This lets connection options serve future protocols that do not use credential injection, instead of being welded to a mandatory-credentials operation. Register TargetConnectionOptions in the OpenAPI schema list and regenerate gateway-api.yaml. --- devolutions-gateway/openapi/gateway-api.yaml | 7 +- devolutions-gateway/src/api/preflight.rs | 46 ++++++- .../src/credential_injection.rs | 26 +--- devolutions-gateway/src/openapi.rs | 9 +- devolutions-gateway/src/provisioning.rs | 121 +++++++++++++----- 5 files changed, 150 insertions(+), 59 deletions(-) diff --git a/devolutions-gateway/openapi/gateway-api.yaml b/devolutions-gateway/openapi/gateway-api.yaml index bdb34991f..e85706e23 100644 --- a/devolutions-gateway/openapi/gateway-api.yaml +++ b/devolutions-gateway/openapi/gateway-api.yaml @@ -7,7 +7,7 @@ info: email: infos@devolutions.net license: name: MIT/Apache-2.0 - version: 2026.1.2 + version: 2026.2.4 paths: /jet/config: patch: @@ -1803,7 +1803,7 @@ components: description: |- Minimum persistence duration in seconds for the data provisioned via this operation. - Optional parameter for "provision-token" and "provision-credentials" kinds. + Optional parameter for "provision-token", "provision-credentials" and "provision-connection-options" kinds. nullable: true minimum: 0 token: @@ -1811,7 +1811,7 @@ components: description: |- The token to be stored on the proxy-side. - Required for "provision-token" and "provision-credentials" kinds. + Required for "provision-token", "provision-credentials" and "provision-connection-options" kinds. nullable: true PreflightOperationKind: type: string @@ -1822,6 +1822,7 @@ components: - get-recording-storage-health - provision-token - provision-credentials + - provision-connection-options - resolve-host PreflightOutput: type: object diff --git a/devolutions-gateway/src/api/preflight.rs b/devolutions-gateway/src/api/preflight.rs index 8de0f62e8..0fb83e546 100644 --- a/devolutions-gateway/src/api/preflight.rs +++ b/devolutions-gateway/src/api/preflight.rs @@ -22,6 +22,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); @@ -46,7 +47,13 @@ struct ProvisionCredentialsParams { token: String, #[serde(flatten)] credentials: crate::credential::CleartextAppCredentials, - connection_options: Option, + time_to_live: Option, +} + +#[derive(Debug, Deserialize)] +struct ProvisionConnectionOptionsParams { + token: String, + connection_options: crate::target_connection_options::TargetConnectionOptions, time_to_live: Option, } @@ -330,7 +337,6 @@ async fn handle_operation( let ProvisionCredentialsParams { token, credentials, - 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)?; @@ -351,7 +357,7 @@ async fn handle_operation( })?; let replaced = provisioning - .insert(token_data, credentials, connection_options, time_to_live) + .insert_credentials(token_data, credentials, time_to_live) .inspect_err(|error| warn!(%operation.id, error = format!("{error:#}"), "Failed to insert credentials")) .map_err(|_| { PreflightError::new( @@ -365,7 +371,39 @@ async fn handle_operation( operation_id: operation.id, kind: PreflightOutputKind::Alert { status: PreflightAlertStatus::Info, - message: "an existing provisioning entry was replaced".to_owned(), + message: "existing provisioned credentials were replaced".to_owned(), + }, + }); + } + + outputs.push(PreflightOutput { + operation_id: operation.id, + 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 that + // provision-credentials requires. + let jti = crate::token::extract_jti(&token).map_err(|error| { + PreflightError::new(PreflightAlertStatus::InvalidParams, format!("invalid token: {error:#}")) + })?; + + let replaced = provisioning.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(), }, }); } diff --git a/devolutions-gateway/src/credential_injection.rs b/devolutions-gateway/src/credential_injection.rs index 35ab3e21a..0f66aa73d 100644 --- a/devolutions-gateway/src/credential_injection.rs +++ b/devolutions-gateway/src/credential_injection.rs @@ -168,7 +168,6 @@ impl CredentialInjection { credentials, connection_options, target_hostname, - expires_at: _, } = provisioned_connection; let uses_kerberos = if kerberos_enabled { @@ -548,16 +547,11 @@ mod tests { .expect("credentials encrypt") } - fn provisioned( - target_username: &str, - krb_kdc: Option, - time_to_live: time::Duration, - ) -> ProvisionedConnection { + fn provisioned(target_username: &str, krb_kdc: Option) -> ProvisionedConnection { ProvisionedConnection { credentials: app_credentials("proxy@example.invalid", target_username), connection_options: krb_kdc.map(|kdc| TargetConnectionOptions::new(Some(kdc)).expect("connection options")), target_hostname: "target.example".to_owned(), - expires_at: time::OffsetDateTime::now_utc() + time_to_live, } } @@ -604,11 +598,7 @@ mod tests { let kdc = target_kdc(); let injection = CredentialInjection::from_provisioned( Uuid::new_v4(), - provisioned( - "administrator@example.invalid", - Some(kdc.clone()), - time::Duration::minutes(5), - ), + provisioned("administrator@example.invalid", Some(kdc.clone())), "target.example", true, ) @@ -624,7 +614,7 @@ mod tests { fn from_provisioned_hard_errors_when_kerberos_target_has_no_kdc() { let error = CredentialInjection::from_provisioned( Uuid::new_v4(), - provisioned("administrator@example.invalid", None, time::Duration::minutes(5)), + provisioned("administrator@example.invalid", None), "target.example", true, ) @@ -637,7 +627,7 @@ mod tests { fn from_provisioned_selects_ntlm_for_domainless_target() { let injection = CredentialInjection::from_provisioned( Uuid::new_v4(), - provisioned("Administrator", None, time::Duration::minutes(5)), + provisioned("Administrator", None), "target.example", true, ) @@ -649,11 +639,7 @@ mod tests { fn from_provisioned_selects_ntlm_when_kerberos_disabled() { let injection = CredentialInjection::from_provisioned( Uuid::new_v4(), - provisioned( - "administrator@example.invalid", - Some(target_kdc()), - time::Duration::minutes(5), - ), + provisioned("administrator@example.invalid", Some(target_kdc())), "target.example", false, ) @@ -665,7 +651,7 @@ mod tests { fn from_provisioned_rejects_target_hostname_mismatch() { let error = CredentialInjection::from_provisioned( Uuid::new_v4(), - provisioned("Administrator", None, time::Duration::minutes(5)), + provisioned("Administrator", None), "other.example", true, ) diff --git a/devolutions-gateway/src/openapi.rs b/devolutions-gateway/src/openapi.rs index 47a290c2f..8b0b72d8d 100644 --- a/devolutions-gateway/src/openapi.rs +++ b/devolutions-gateway/src/openapi.rs @@ -68,6 +68,7 @@ use crate::config::dto::{DataEncoding, PubKeyFormat, Subscriber}; PreflightOperationKind, AppCredential, AppCredentialKind, + TargetConnectionOptions, PreflightOutput, PreflightOutputKind, PreflightAlertStatus, @@ -372,7 +373,7 @@ struct PreflightOperation { kind: PreflightOperationKind, /// The token to be stored on the proxy-side. /// - /// Required for "provision-token" and "provision-credentials" kinds. + /// Required for "provision-token", "provision-credentials" and "provision-connection-options" kinds. token: Option, /// The credential to use to authorize the client at the proxy-level. /// @@ -384,7 +385,7 @@ struct PreflightOperation { target_credential: Option, /// Options used by the Gateway when connecting to the target. /// - /// Optional for "provision-credentials" kind. + /// Required for "provision-connection-options" kind. connection_options: Option, /// The hostname to perform DNS resolution on. /// @@ -392,7 +393,7 @@ struct PreflightOperation { host_to_resolve: Option, /// Minimum persistence duration in seconds for the data provisioned via this operation. /// - /// Optional parameter for "provision-token" and "provision-credentials" kinds. + /// Optional parameter for "provision-token", "provision-credentials" and "provision-connection-options" kinds. time_to_live: Option, } @@ -419,6 +420,8 @@ enum PreflightOperationKind { ProvisionToken, #[serde(rename = "provision-credentials")] ProvisionCredentials, + #[serde(rename = "provision-connection-options")] + ProvisionConnectionOptions, #[serde(rename = "resolve-host")] ResolveHost, } diff --git a/devolutions-gateway/src/provisioning.rs b/devolutions-gateway/src/provisioning.rs index ecc0ab3fb..fa6df48be 100644 --- a/devolutions-gateway/src/provisioning.rs +++ b/devolutions-gateway/src/provisioning.rs @@ -10,22 +10,44 @@ use uuid::Uuid; use crate::credential::{AppCredentials, CleartextAppCredentials}; use crate::target_connection_options::TargetConnectionOptions; +/// A combined, point-in-time view of everything provisioned for a session. +/// +/// Assembled on read from the two independent stores. Credentials are required by the only consumer +/// today (RDP credential injection); connection options are optional and may be absent — never +/// provisioned, or expired before the credentials half. #[derive(Debug, Clone)] pub(crate) struct ProvisionedConnection { pub(crate) credentials: AppCredentials, pub(crate) connection_options: Option, pub(crate) target_hostname: String, - pub(crate) expires_at: time::OffsetDateTime, } -/// Stores credentials and target options provisioned for a session, keyed by association-token JTI. +#[derive(Debug, Clone)] +struct CredentialsEntry { + credentials: AppCredentials, + target_hostname: String, + 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 and the master key never leaves the credential +/// module's dependency graph. The connection-options store holds plaintext routing metadata only +/// and has no crypto dependency at all — keeping the two apart is what preserves that property. /// -/// This is the credential boundary: cleartext mappings are encrypted on the way in, so entries -/// only ever hold encrypted material and the master key never leaves this module's dependency -/// graph. +/// Both are keyed by the association-token JTI, but the two halves are provisioned by separate +/// preflight operations and may arrive, expire, or be replaced independently. #[derive(Debug, Clone)] pub struct ProvisioningStore { - entries: Arc>>, + credentials: Arc>>, + connection_options: Arc>>, } impl Default for ProvisioningStore { @@ -37,54 +59,90 @@ impl Default for ProvisioningStore { impl ProvisioningStore { pub fn new() -> Self { Self { - entries: Arc::new(Mutex::new(HashMap::new())), + credentials: Arc::new(Mutex::new(HashMap::new())), + connection_options: Arc::new(Mutex::new(HashMap::new())), } } - pub(crate) fn insert( + pub(crate) fn insert_credentials( &self, token_data: crate::token::CredentialInjectionTokenData, credentials: CleartextAppCredentials, - connection_options: Option, time_to_live: time::Duration, ) -> anyhow::Result { let credentials = credentials.encrypt().context("encrypt provisioned credentials")?; - let entry = ProvisionedConnection { + let entry = CredentialsEntry { credentials, - connection_options, target_hostname: token_data.target_hostname, expires_at: time::OffsetDateTime::now_utc() + time_to_live, }; - Ok(self.entries.lock().insert(token_data.jti, entry).is_some()) + Ok(self.credentials.lock().insert(token_data.jti, entry).is_some()) } - /// Look up the provisioning for a session. + 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, + }; + + self.connection_options.lock().insert(jti, entry).is_some() + } + + /// Combined view for the RDP credential-injection path. /// - /// Entries live until their TTL, not until first use: a jti names a session, and the token - /// layer allows an RDP session to open several connections (reconnects), each of which needs - /// the same materials. Expired entries are treated as absent; the cleanup task reclaims them. + /// Requires the credentials half and folds in the connection-options half when it is present. + /// Entries live until their TTL, not until first use: a jti names a session, and the token layer + /// allows an RDP session to open several connections (reconnects), each of which needs the same + /// materials. Either half is treated as absent once expired; the cleanup task reclaims them. pub(crate) fn get(&self, jti: Uuid) -> Option { - let entries = self.entries.lock(); - let entry = entries.get(&jti)?; + let now = time::OffsetDateTime::now_utc(); + + let (credentials, target_hostname) = { + 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.credentials.clone(), entry.target_hostname.clone()) + }; - if time::OffsetDateTime::now_utc() >= entry.expires_at { - warn!(%jti, "Provisioning expired before the connection arrived"); + let connection_options = self.get_live_connection_options(jti, now); + + Some(ProvisionedConnection { + credentials, + connection_options, + target_hostname, + }) + } + + 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.clone()) + Some(entry.connection_options.clone()) } pub fn cleanup_task(&self) -> impl Task> + 'static + use<> { CleanupTask { - entries: Arc::clone(&self.entries), + credentials: Arc::clone(&self.credentials), + connection_options: Arc::clone(&self.connection_options), } } } struct CleanupTask { - entries: Arc>>, + credentials: Arc>>, + connection_options: Arc>>, } #[async_trait] @@ -94,13 +152,17 @@ impl Task for CleanupTask { const NAME: &'static str = "provisioning cleanup"; async fn run(self, shutdown_signal: ShutdownSignal) -> Self::Output { - cleanup_task(self.entries, shutdown_signal).await; + cleanup_task(self.credentials, self.connection_options, shutdown_signal).await; Ok(()) } } #[instrument(skip_all)] -async fn cleanup_task(entries: Arc>>, mut shutdown_signal: ShutdownSignal) { +async fn cleanup_task( + credentials: Arc>>, + connection_options: Arc>>, + mut shutdown_signal: ShutdownSignal, +) { use tokio::time::{Duration, sleep}; const TASK_INTERVAL: Duration = Duration::from_secs(60 * 15); @@ -116,7 +178,8 @@ async fn cleanup_task(entries: Arc>>, } let now = time::OffsetDateTime::now_utc(); - entries.lock().retain(|_, entry| now < entry.expires_at); + credentials.lock().retain(|_, entry| now < entry.expires_at); + connection_options.lock().retain(|_, entry| now < entry.expires_at); } debug!("Task terminated"); @@ -156,7 +219,7 @@ mod tests { let store = ProvisioningStore::new(); let jti = Uuid::new_v4(); store - .insert(token_data(jti), credentials(), None, time::Duration::minutes(5)) + .insert_credentials(token_data(jti), credentials(), time::Duration::minutes(5)) .expect("entry inserts"); assert_eq!(store.get(jti).expect("live entry").target_hostname, "target.example"); } @@ -166,7 +229,7 @@ mod tests { let store = ProvisioningStore::new(); let jti = Uuid::new_v4(); store - .insert(token_data(jti), credentials(), None, time::Duration::seconds(-1)) + .insert_credentials(token_data(jti), credentials(), time::Duration::seconds(-1)) .expect("entry inserts"); assert!(store.get(jti).is_none(), "an expired entry must read as absent"); }