From 8ba1c4e2d497043a828e8558ce02dec176759274 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Fri, 24 Jul 2026 13:54:09 -0400 Subject: [PATCH 1/5] 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 4def9b70d6d1084c3299c36bfb92f0747b463b25 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Fri, 24 Jul 2026 14:28:45 -0400 Subject: [PATCH 2/5] 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 d76a1651d747bb29b688ced6ce1fbbe8bde04178 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Fri, 24 Jul 2026 16:01:56 -0400 Subject: [PATCH 3/5] 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 bc71a4377efeaf050b4df475b65a466b557c545c Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Fri, 24 Jul 2026 16:39:58 -0400 Subject: [PATCH 4/5] 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 5c7ef68d366b11a06c8afcd93a16df14d26bf674 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Fri, 24 Jul 2026 17:10:32 -0400 Subject: [PATCH 5/5] 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");