fix(dgw): decide credential-injection CredSSP protocol once for both legs#1862
fix(dgw): decide credential-injection CredSSP protocol once for both legs#1862irvingouj@Devolutions (irvingoujAtDevolution) wants to merge 4 commits into
Conversation
Let maintainers know that an action is required on their side
|
There was a problem hiding this comment.
Pull request overview
Uses provisioned KDC addresses for target-side Kerberos credential injection while retaining NTLM fallback.
Changes:
- Adds
krb_kdccredential provisioning and client API support. - Replaces legacy Kerberos debug configuration with an unstable opt-in.
- Routes provisioned KDC settings through both RDP paths.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
devolutions-gateway/src/rdp_proxy.rs |
Builds target-side Kerberos configuration. |
devolutions-gateway/src/rd_clean_path.rs |
Uses provisioned KDC in clean-path sessions. |
devolutions-gateway/src/openapi.rs |
Documents the preflight KDC field. |
devolutions-gateway/src/credential/mod.rs |
Validates and stores provisioned KDC addresses. |
devolutions-gateway/src/credential_injection_kdc.rs |
Exposes the stored KDC address. |
devolutions-gateway/src/config.rs |
Adds the unstable credential-injection opt-in. |
devolutions-gateway/openapi/ts-angular-client/model/preflightOperation.ts |
Exposes krb_kdc to TypeScript clients. |
devolutions-gateway/openapi/gateway-api.yaml |
Updates the API schema and version. |
devolutions-gateway/openapi/dotnet-client/src/Devolutions.Gateway.Client/Model/PreflightOperation.cs |
Exposes KrbKdc to .NET clients. |
devolutions-gateway/openapi/dotnet-client/docs/PreflightOperation.md |
Documents the .NET property. |
config_schema.json |
Documents the new debug setting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
devolutions-gateway/src/rdp_proxy.rs:139
- This enables target-side Kerberos whenever
krb_kdcis provisioned, even whenenable_unstableorkerberos_credential_injectionis false, whilecredential_injection_kerberos_server_configreturns no Kerberos acceptor in that case. The acceptor is documented to require the same auth package as the target leg, so this configuration cannot complete and the new opt-in does not actually gate the target-side behavior. Gate both legs with the same condition (or reject a Kerberos-provisioned session explicitly when the feature is disabled).
let krb_client_config =
credential_injection_kerberos_client_config(credential_injection_kdc.krb_kdc(), &gateway_hostname)?;
devolutions-gateway/src/rdp_proxy.rs:381
- When the opt-in is enabled but
krb_kdcis omitted (the documented NTLM case), this still callsclient_acceptor_protocol, which selects Kerberos for any domain-qualified target username. Meanwhile the target-side helper returnsNoneand uses NTLM, violating the required mirrored auth package and breaking existingDOMAIN\\user/UPN NTLM injections. Skip the Kerberos acceptor whenever no KDC was provisioned.
if !conf.debug.enable_unstable || !conf.debug.kerberos_credential_injection {
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
Thank you!
I have one blocking architectural concern: where krb_kdc lives. Everything else is non-blocking issues reported by Claude that you may want to address too.
issue (blocking): krb_kdc should not live on AppCredentialMapping. I also considered placing it on the association token, but I think that’s also not ideal.
krb_kdc was added to AppCredentialMapping, and it is exposed on the preflight DTO. Two problems, and they rule out the two obvious alternatives, so I want to lay out the whole path to the recommendation:
-
It's not a credential.
AppCredentialis deliberately protocol-agnostic (UsernamePasswordonly). A KDC address is routing/topology metadata, not a secret; nothing else in the credential module reasons about it, and it's the first protocol-specific leak into a generic type. It's conceptually similar tocert_thumb256: config for how the Gateway's internal client connects to the target. -
But
cert_thumb256's home, the association token, is also wrong for this value. The token is read from the preconnection blob inread_pcbbefore the TLS upgrade (X.224 layer), so in the native RDP path it's cleartext on the wire and sniffable. The token is a signed JWT, so this is a confidentiality exposure, not integrity: an on-path observer can't redirect the KDC, only read it. But a KDC address is internal AD topology, and we shouldn't hand that to a passive observer.cert_thumb256on the other hand, is safe there because it's a hash of a public cert, it is typically served through HTTPS (for the jet/fwd/tls route), and it’s also useful to lock that down on the provisioner side (e.g.: DVLS). Adding the proxy client/target options to the preflight operations still allows the provisioner to lock the extra config, because the provisioner is the one calling that endpoint. It’s also fully encrypted via HTTPS. -
As such, the preflight channel was already right, and the problem is the storage location. Provisioning over authenticated HTTPS (as this PR does) correctly keeps
krb_kdcoff the cleartext wire. So the fix is not to move the channel, but to move the home out of the credential abstraction. -
Don't move it into the credential store as a sibling either. That store is our sensitive one: master key in
credential::crypto, field-levelEncryptedPassword. Config needs none of that, and putting it there blurs "this module is where the protected material lives." (Note the encryption is already field-level, not entry-level:token/username/expires_atare plaintext-at-rest today, so there's no whole-entry invariant to protect, the concern is legibility of the boundary, not crypto.)
suggestion: The store is really a token-JTI-keyed, TTL'd provisioning cache that happens to hold credentials first: it's documented as "the protocol-neutral CredentialStoreHandle." Keep it that way and generalize it:
token_keyed_store.rs: a genericTokenKeyedStore<V>owning JTI keying, TTL, and cleanup, knowing nothing aboutV. It stays protocol-neutral; no dependency oncredentials.credentials/crypto.rs+credentials/mod.rs: the credential mapping type and its crypto, self-contained.- Composition at
CredentialService: which already exists to wrap the store and coordinate. It defines one composite entry per JTI holdingmapping: Option<CredentialMapping>(encrypted) alongsideoptions: Option<TargetConnectionOptions>(plaintext,krb_kdclives here), and instantiatesTokenKeyedStore<ProvisioningEntry>(withProvisioningEntrybeingCredentialEntryrenamed and extended).
The invariant, independent of file layout: the store never encrypts or decrypts. Today CredentialStoreHandle::insert takes a CleartextAppCredentialMapping and calls .encrypt() itself. Move that step out to the credentials boundary so the service hands the store an already-encrypted CredentialMapping it holds opaquely. Then MASTER_KEY/EncryptedPassword appear only in credentials/, and the type system carries the boundary: the options field can't hold an EncryptedPassword. Extend provision-credentials with an optional connection_options block, that will start by holding kdc_krb only for now.
In terms of scope: this is its own change, not something to smuggle into a bug fix. Since krb_kdc is behind enable_unstable with no stable consumer, landing it in the right home beats placing it in a provisioning DTO + credential type and migrating later. I suggest splitting: the decide-once fix and the dead-config removal can land now, but the KDC-provisioning storage returns as a focused follow-up on the factored store.
keep: Removing the static debug.kerberos block is the right call.
keep: The decide-once fix in rdp_proxy is structurally sound: both legs derive from one injection_uses_kerberos result, so they agree by construction.
Claude review:
question (non-blocking): enabling the feature makes
krb_kdcmandatory for every domain-qualified target, not just Protected Users.client_acceptor_protocol()(credential_injection_kdc.rs:178) returns Kerberos for any domain part — the tests confirm bothuser@realmandDOMAIN\user(:870,:880). Combined with the flag,rdp_proxy.rs:407hard-errors any session lacking a provisioned KDC, and there's no NTLM fallback for a domain account that isn't Kerberos-enforced. This also drops the oldkdc_url: None→ sspi DNS-SRV path. Is that the intended contract with DVLS provisioning? Worth confirming the blast radius is deliberate.thought (non-blocking): target username is now parsed unconditionally, ahead of the feature gate.
rdp_proxy.rs:385callsclient_acceptor_protocol()?before theinjection_uses_kerberoscheck, so a malformed target username aborts even a feature-off session. Low impact (the credssp path parses it downstream anyway), but it's a genuine ordering change — a short-circuit on the flag first would avoid it.suggestion (non-blocking): advertise the scheme constraint in the API.
deserialize_optional_kdc_addr(credential/mod.rs:120) rejects non-tcp/udp cleanly, butopenapi.rsdocumentskrb_kdconly asOption<String>. A client sendinghttps://…(a valid KKDCP URL sspi itself supports) gets a deserialize error with no hint from the schema. Note the tcp/udp-only constraint in the field doc. (Moot if the rework moves this off the preflight DTO.)issue (non-blocking): two scheme allowlists must stay in sync.
credential/mod.rs:127(tcp/udp) andkdc_connector.rs:184(tcp/udp) are independent. Addhttpsto one without the other and provisioning accepts a URL the dispatcher rejects at runtime. A shared constant, or a cross-reference comment, de-risks the drift — and theTODO(sspi-rs#664)trampoline is the eventual escape from both.nitpick (non-blocking):
TargetAddr→String→url::Urlreparse atrdp_proxy.rs:410.TryFrom<&url::Url> for TargetAddralready exists; the reverse could live next to it instead of round-tripping through a string. Cosmetic — the round-trip is provably lossless for tcp/udp.
|
I will create a stacked PR for refactoring, thanks |
|
Benoît Cortier (@CBenoit) updated, the refactor will come in separate PR |
Route target-side Kerberos authentication through the KDC provisioned with the credential mapping, and add an explicit unstable opt-in for the client-facing Kerberos acceptor. Issue: DVLS-14697
Add the optional krb_kdc field to the provision-credentials OpenAPI contract and synchronize the generated .NET and TypeScript client models. Issue: DVLS-14697
…legs Review follow-up (Copilot + end-to-end review). The two CredSSP legs previously chose Kerberos vs NTLM from independent inputs and could disagree (e.g. a provisioned krb_kdc with the opt-in off gave a Kerberos target leg but an NTLM acceptor), which fails the handshake reading one package as the other. - Collapse the two per-leg helpers into a single credential_injection_kerberos_configs that makes one decision (enable_unstable && kerberos_credential_injection && domain-qualified target) and builds both legs from it; require the provisioned KDC when Kerberos is chosen, else NTLM on both legs. - A stray krb_kdc (feature off or domainless target) is now ignored with a warning instead of silently diverging or hard-failing an NTLM-capable session. - Keep the generated PreflightOperation ctor source-compatible by appending the new optional krbKdc parameter last. - Add unit coverage for the protocol decision and the krb_kdc round-trip.
552a4d3 to
f655d68
Compare
What
Proxy-based RDP credential injection runs two CredSSP legs — the client-facing acceptor and the target-facing initiator. Each leg decided independently whether to speak Kerberos or NTLM, and when they disagreed the handshake failed reading one package as the other. Both legs now derive from a single decision, so they always agree by construction.
Alongside that fix:
debug.kerberosconfiguration block (fake-KDC users and keys) — it was never a real deployment path.debug.kerberos_credential_injection, gating the in-process Kerberos acceptor the Gateway presents to the client. Off by default; still requiresenable_unstable.Provisioning a real target-side KDC is a separate, focused change and lands in the follow-up #1865.
Tests
cargo +nightly fmt --all -- --check;cargo clippy -p devolutions-gateway --all-targets -- -D warnings;cargo test -p devolutions-gateway.Issue: DVLS-14697