Skip to content

Commit 7c1a9ba

Browse files
fix(dgw): address Copilot review on CredSSP/provisioning refactor
Authorize CleanPath tokens before one-shot take, use a registry-wide generation counter, and replace bare clippy allow with expect. SPN remains association-token dst_hst for client-facing CredSSP.
1 parent 810fe16 commit 7c1a9ba

4 files changed

Lines changed: 44 additions & 21 deletions

File tree

‎devolutions-gateway/src/credential_injection.rs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -531,7 +531,8 @@ pub struct SyntheticKdcRegistry {
531531
#[derive(Debug, Default)]
532532
struct RegistryInner {
533533
live: HashMap<Uuid, PublishedSyntheticKdc>,
534-
next_generation: HashMap<Uuid, u64>,
534+
/// Registry-wide monotonic counter (not per-JTI) so generations stay unique without leaking map entries.
535+
next_generation: u64,
535536
}
536537

537538
#[derive(Debug, Clone)]
@@ -573,16 +574,15 @@ impl SyntheticKdcRegistry {
573574
}
574575
}
575576

576-
fn allocate_generation(inner: &mut RegistryInner, jti: Uuid) -> u64 {
577-
let slot = inner.next_generation.entry(jti).or_insert(0);
578-
*slot = slot.wrapping_add(1);
579-
*slot
577+
fn allocate_generation(inner: &mut RegistryInner) -> u64 {
578+
inner.next_generation = inner.next_generation.wrapping_add(1);
579+
inner.next_generation
580580
}
581581

582582
pub(crate) fn register(&self, kdc: Arc<CredentialInjectionKdc>) -> SyntheticKdcRegistration {
583583
let jti = kdc.jti();
584584
let mut inner = self.inner.lock();
585-
let generation = Self::allocate_generation(&mut inner, jti);
585+
let generation = Self::allocate_generation(&mut inner);
586586
inner.live.insert(jti, PublishedSyntheticKdc { generation, kdc });
587587
debug!(%jti, generation, "published synthetic KDC");
588588
SyntheticKdcRegistration {

‎devolutions-gateway/src/provisioning.rs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,19 @@ impl ProvisioningStore {
129129
self.connection_options.lock().insert(jti, entry).is_some()
130130
}
131131

132+
/// True when the credentials half is live and carries an injection mapping.
133+
///
134+
/// Does not consume the entry — use before auth when deciding whether to take the
135+
/// credential-injection path.
136+
pub(crate) fn has_mapping(&self, jti: Uuid) -> bool {
137+
let now = time::OffsetDateTime::now_utc();
138+
let entries = self.credentials.lock();
139+
match entries.get(&jti) {
140+
Some(entry) if now < entry.expires_at => entry.mapping.is_some(),
141+
_ => false,
142+
}
143+
}
144+
132145
/// Take the provisioned view for a session (one-shot).
133146
///
134147
/// Removes the credentials half (required) and any live connection-options half for `jti`.

‎devolutions-gateway/src/rd_clean_path.rs‎

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -320,7 +320,8 @@ async fn handle_with_credential_injection(
320320
subscriber_tx: SubscriberSender,
321321
active_recordings: &ActiveRecordings,
322322
cleanpath_pdu: RDCleanPathPdu,
323-
credential_injection: CredentialInjection,
323+
provisioning: &ProvisioningStore,
324+
synthetic_kdc_registry: &SyntheticKdcRegistry,
324325
agent_tunnel_handle: Option<Arc<agent_tunnel::AgentTunnelHandle>>,
325326
) -> anyhow::Result<()> {
326327
let tls_conf = conf.credssp_tls.get().context("CredSSP TLS configuration")?;
@@ -364,6 +365,19 @@ async fn handle_with_credential_injection(
364365
.await
365366
.context("RDCleanPath authorization failed")?;
366367

368+
let token = cleanpath_pdu
369+
.proxy_auth
370+
.as_deref()
371+
.context("missing token in RDCleanPath PDU")?;
372+
let entry = provisioning
373+
.take(claims.jti)
374+
.context("provisioned credentials missing after authorization")?;
375+
anyhow::ensure!(entry.mapping.is_some(), "provisioned entry has no credential mapping");
376+
anyhow::ensure!(token == entry.token, "token mismatch");
377+
let kerberos_enabled = conf.debug.enable_unstable && conf.debug.kerberos_credential_injection;
378+
let credential_injection = CredentialInjection::from_provisioned(claims.jti, entry, kerberos_enabled)?
379+
.register_if_kerberos(synthetic_kdc_registry);
380+
367381
let ConnectedRdpServer {
368382
tls_stream: server_stream,
369383
server_addr,
@@ -532,20 +546,12 @@ pub async fn handle(
532546

533547
// If a credential mapping has been pushed, we automatically switch to
534548
// proxy-based credential injection mode. Otherwise, we continue the usual
535-
// clean path procedure. The credential store is keyed on the association token's JTI.
549+
// clean path procedure. Peek only here — take after authorize_cleanpath so an
550+
// unverified token cannot burn a victim JTI's one-shot groceries.
536551
if let Some(jti) = crate::token::extract_jti(token).ok()
537-
&& let Some(entry) = provisioning.take(jti)
538-
&& entry.mapping.is_some()
552+
&& provisioning.has_mapping(jti)
539553
{
540-
anyhow::ensure!(token == entry.token, "token mismatch");
541-
let kerberos_enabled = conf.debug.enable_unstable && conf.debug.kerberos_credential_injection;
542-
let credential_injection = CredentialInjection::from_provisioned(jti, entry, kerberos_enabled)?
543-
.register_if_kerberos(synthetic_kdc_registry);
544-
debug!(
545-
jti = %credential_injection.jti(),
546-
kerberos = credential_injection.uses_kerberos(),
547-
"Switching to RdpProxy for credential injection (WebSocket)"
548-
);
554+
debug!(%jti, "Switching to RdpProxy for credential injection (WebSocket)");
549555

550556
return handle_with_credential_injection(
551557
client_stream,
@@ -557,7 +563,8 @@ pub async fn handle(
557563
subscriber_tx,
558564
active_recordings,
559565
cleanpath_pdu,
560-
credential_injection,
566+
provisioning,
567+
synthetic_kdc_registry,
561568
agent_tunnel_handle.clone(),
562569
)
563570
.await;

‎devolutions-gateway/src/rdp_proxy/credssp.rs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -402,7 +402,10 @@ where
402402

403403
return result;
404404

405-
#[allow(clippy::too_many_arguments)]
405+
#[expect(
406+
clippy::too_many_arguments,
407+
reason = "CredSSP loop needs framed IO, identity, optional synthetic KDC, and KdcConnector together",
408+
)]
406409
async fn credssp_loop<S>(
407410
framed: &mut ironrdp_tokio::Framed<S>,
408411
buf: &mut ironrdp_pdu::WriteBuf,

0 commit comments

Comments
 (0)