Skip to content

fix(dgw): target Gateway hostname when using the credential-injection fake KDC proxy #1856

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Pavlo Myroniuk (TheBestTvarynka) wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(dgw): target Gateway hostname when using the credential-injection fake KDC proxy #1856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 12 additions & 49 deletions devolutions-gateway/src/credential_injection_kdc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,6 @@ pub(crate) enum CredentialInjectionKdcResolveError {
ExpiredCredential { jti: Uuid },
#[error("credential-injection state is not available for {jti}")]
NonInjectionCredential { jti: Uuid },
#[error("association token for {jti} is not valid for credential injection")]
InvalidAssociationToken {
jti: Uuid,
#[source]
source: anyhow::Error,
},
#[error("credential-injection KDC config could not be initialized for {jti}")]
BuildKdcConfig {
jti: Uuid,
Expand Down Expand Up @@ -444,19 +438,15 @@ fn random_32_bytes() -> Vec<u8> {
/// through this service, so callers see one handle instead of coordinating a store and a registry.
#[derive(Debug, Clone)]
pub struct CredentialService {
hostname: String,

@CBenoit Benoît Cortier (CBenoit) Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is something I would prefer we address before merging

credentials: CredentialStoreHandle,
sessions: Arc<Mutex<HashMap<Uuid, Arc<CredentialInjectionKdcSession>>>>,
}

impl Default for CredentialService {
fn default() -> Self {
Self::new()
}
}

impl CredentialService {
pub fn new() -> Self {
pub fn new(hostname: String) -> Self {

@CBenoit Benoît Cortier (CBenoit) Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

issue: I know it’s probably not a real issue today because we don’t change the hostname dynamically typically; that being said ConfHandle supports hot-reloading, and here we retrieve a snapshot of the hostname based on what was configured at startup time. The better style would be to take the ConfHandle and to retrieve the hostname each time it is necessary, so we have an up to date value (get_conf() for each request). This is relevant because CredentialService is not short-lived.

@CBenoit Benoît Cortier (CBenoit) Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

question: Also a question I got while reviewing. This is a fine, minimal fix, but it’s worth questioning if it’s not better to use the hostname of the external URL of the listener being used for this request. This makes the fix a bit more involved, and may be a follow up, but it would be "more correct". If a client reaches the gateway through a listener whose external hostname differs from conf.hostname, the SPN mismatch fixed here could resurface.

Self {
hostname,
credentials: CredentialStoreHandle::new(),
sessions: Arc::new(Mutex::new(HashMap::new())),
}
Expand Down Expand Up @@ -524,16 +514,6 @@ impl CredentialService {
CredentialInjectionKdcResolveError::NonInjectionCredential { jti }
})?;

let target_hostname = crate::token::extract_credential_injection_target_hostname(&credential_entry.token)
.map_err(|source| {
warn!(
%jti,
error = format!("{source:#}"),
"KDC token references invalid credential-injection association token"
);
CredentialInjectionKdcResolveError::InvalidAssociationToken { jti, source }
})?;

let proxy_username = app_credential_username(&mapping.proxy).to_owned();
// Atomic get-or-insert: holds the lock long enough to guarantee a single Arc<Session>
// wins for this JTI even under concurrent `kdc_for` calls. The derivation is fast (a few
Expand All @@ -546,7 +526,7 @@ impl CredentialService {
Arc::clone(session)
};

CredentialInjectionKdc::from_parts(jti, credential_entry, target_hostname, session)
CredentialInjectionKdc::from_parts(jti, credential_entry, self.hostname.clone(), session)
.map_err(|source| CredentialInjectionKdcResolveError::BuildKdcConfig { jti, source })
}

Expand Down Expand Up @@ -701,7 +681,7 @@ mod tests {

#[test]
fn service_kdc_for_rejects_expired_credential_entry() {
let service = CredentialService::new();
let service = CredentialService::new("dgateway.localhost.com".to_owned());
let jti = Uuid::new_v4();

// Negative TTL: entry is born already expired. `CredentialStoreHandle::get` does not
Expand All @@ -726,7 +706,7 @@ mod tests {

#[test]
fn service_kdc_for_returns_same_session_under_concurrent_calls() {
let service = CredentialService::new();
let service = CredentialService::new("dgateway.localhost.com".to_owned());
let jti = Uuid::new_v4();

service
Expand All @@ -753,7 +733,7 @@ mod tests {

#[test]
fn service_insert_drops_stale_session_even_without_credential_replacement() {
let service = CredentialService::new();
let service = CredentialService::new("dgateway.localhost.com".to_owned());
let jti = Uuid::new_v4();

// Simulate the race called out by Codex: a previous provisioning's session is still
Expand Down Expand Up @@ -782,7 +762,7 @@ mod tests {

#[test]
fn service_insert_replacement_drops_cached_kerberos_material() {
let service = CredentialService::new();
let service = CredentialService::new("dgateway.localhost.com".to_owned());
let jti = Uuid::new_v4();

service
Expand Down Expand Up @@ -818,7 +798,7 @@ mod tests {

#[test]
fn service_sweep_orphans_drops_sessions_with_no_credential_entry() {
let service = CredentialService::new();
let service = CredentialService::new("dgateway.localhost.com".to_owned());
let jti = Uuid::new_v4();

service
Expand All @@ -837,6 +817,7 @@ mod tests {
// drive `credential::cleanup_task` to expire the entry, but it sleeps for 15 minutes
// between ticks. Swapping the inner store is the deterministic equivalent.
let orphaned_service = CredentialService {
hostname: "dgateway.localhost.com".to_owned(),
credentials: CredentialStoreHandle::new(),
sessions: Arc::clone(&service.sessions),
};
Expand Down Expand Up @@ -901,7 +882,7 @@ mod tests {

#[test]
fn service_kdc_for_rejects_unknown_jti() {
let service = CredentialService::new();
let service = CredentialService::new("dgateway.localhost.com".to_owned());

assert!(
matches!(
Expand All @@ -914,7 +895,7 @@ mod tests {

#[test]
fn service_kdc_for_rejects_non_injection_entry() {
let service = CredentialService::new();
let service = CredentialService::new("dgateway.localhost.com".to_owned());
let jti = Uuid::new_v4();

service
Expand All @@ -930,24 +911,6 @@ mod tests {
);
}

#[test]
fn service_kdc_for_lazily_extracts_target_hostname_from_entry_token() {
let service = CredentialService::new();
let jti = Uuid::new_v4();

service
.insert(
association_token(jti),
Some(cleartext_mapping_with_target_username("target")),
time::Duration::minutes(5),
)
.expect("credential entry inserts");

let kdc = service.kdc_for(jti).expect("credential-injection KDC resolves");

assert_eq!(kdc.target_hostname, "target.example");
}

#[test]
fn intercept_ignores_non_loopback_host() {
let jti = Uuid::new_v4();
Expand Down
2 changes: 1 addition & 1 deletion devolutions-gateway/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,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 credentials = credential_injection_kdc::CredentialService::new();
let credentials = credential_injection_kdc::CredentialService::new(conf_handle.get_conf().hostname.clone());
let monitoring_state = Arc::new(network_monitor::State::new(Arc::new(MockMonitorsCache))?);

let state = Self {
Expand Down
2 changes: 1 addition & 1 deletion devolutions-gateway/src/service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result<Tasks> {
.await
.context("failed to initialize traffic audit manager")?;

let credentials = devolutions_gateway::credential_injection_kdc::CredentialService::new();
let credentials = devolutions_gateway::credential_injection_kdc::CredentialService::new(conf.hostname.clone());

let filesystem_monitor_config_cache = devolutions_gateway::api::monitoring::FilesystemConfigCache::new(
config::get_data_dir().join("monitors_cache.json"),
Expand Down
Loading