feat(dgw): Kerberos credential injection via explicit jet_cred_id#1768
feat(dgw): Kerberos credential injection via explicit jet_cred_id#1768irvingouj@Devolutions (irvingoujAtDevolution) wants to merge 21 commits into
Conversation
New design that replaces the heuristic AS-REQ session router from the dgw-378-session-redesign experimental branch with an explicit cred_injection_id (JWT claim jet_cred_id) propagated from /jet/preflight provision-credentials through DVLS-issued association and KDC tokens. Covers protocol mechanics (Kerberos AS/TGS/AP, U2U vs standard, two-leg CredSSP MITM), wire contracts (preflight DTO, JWT claims, NuGet builder API), internal structures (CredentialStore reduced to two indices, SessionKerberos simplified), in-process loopback for U2U (inproc:// scheme in send_network_request), backward compatibility matrix, and an implementation outline. This is a design-only commit. The branch dgw-378-explicit-identity forks from master and will receive the implementation in subsequent commits. The dgw-378-session-redesign branch is preserved as experimental record.
…ion_id
Working end-to-end Kerberos credential-injection demo against AD-joined RDP server.
DVLS-side counterpart: dvls-13821-jet-cred-id branch in RDM repo.
Changes (matches docs/plans/2026-04-27-dgw-378-explicit-identity-design.md):
Token + JWT claims (token.rs):
- AssociationTokenClaims.jet_cred_id: Option<Uuid>
- KdcTokenClaims.jet_cred_id: Option<Uuid>
- extract_jet_cred_id, extract_dst_hst helpers
Credential store (credential/mod.rs):
- CredentialEntry primary key renamed session_id -> cred_injection_id
- New SessionKerberos { krbtgt_key, service_long_term_key, service_user_*, realm }
- Synthetic per-session realm CRED-{uuid}.INVALID when proxy username is bare UUID
- target_hostname captured from association token dst_hst at insert time
- 5 unit tests covering insert/lookup/eviction/synthetic-realm
Preflight (api/preflight.rs):
- ProvisionCredentialsParams gains optional cred_injection_id
- New PreflightOutputKind::ProvisionedCredentials returns the final id
KDC proxy (api/kdc_proxy.rs):
- Direct lookup by claims.jet_cred_id, fall through to forward-to-real-KDC
- Empty target_domain envelope falls back to entry.kerberos.realm
- Pass entry.target_hostname to handle_kdc_proxy_message for TGS-REQ sname check
CredSSP MITM (rdp_proxy.rs, rd_clean_path.rs, generic_client.rs):
- /rdp lookup prefers jet_cred_id, falls back to get_by_token (NTLM compat)
- build_credential_injection_server_kerberos_config uses target hostname for SPN
(TERMSRV/<target>, not Gateway hostname) so client AP-REQ tickets validate
- In-process loopback URL http://cred.invalid/{cred_injection_id} dispatches to
fake-KDC without HTTP, no token issuance, no auth middleware
NuGet (utils/dotnet/Devolutions.Gateway.Utils):
- AssociationClaims.JetCredId: Guid? (nullable, JsonIgnore-when-null)
- KdcClaims.JetCredId: Guid? (same)
- New ProvisionCredentialsRequest.cs with non-nullable CredInjectionId
OpenAPI (gateway-api.yaml + dotnet-client SDK):
- cred_injection_id added to PreflightOperation request shape
- cred_injection_id added to PreflightOutput response shape
- ProvisionedCredentials output kind variant
- PreflightOperation.cs SDK model hand-edited to add CredInjectionId property
(matches what openapi-generator would regenerate from the YAML)
tokengen tooling:
- --jet-cred-id flag on RdpTls and Kdc subcommands
Local-dev wiring (intentionally kept; clean up before PR):
- Gateway.Utils Version bumped 2025.10.1.0 -> 2025.10.2 (local pack)
- Gateway.Client Version bumped 2025.12.2 -> 2025.12.3 (local pack)
- review.md captures the round-3 review state
Backward compatibility (Gateway side):
- Old DVLS without jet_cred_id in preflight: Gateway auto-generates UUID, NTLM
injection works through JTI fallback. Kerberos injection requires new DVLS.
- Empty target_domain envelope tolerated when entry exists (defensive against
iron-remote-desktop sspi-rs not populating it).
- Old conf.debug.kerberos / enable_unstable left in for now (review.md §1.5);
production behaviour is correct, dead branch deletion is a follow-up commit.
…y_hostname plumbing Round-4 follow-up review feedback (review-followup.md): F1 — credential/mod.rs: pass DEFAULT_DST_PORT (3389) to TargetAddr::parse so a bare-hostname dst_hst (no scheme/port) doesn't silently degrade target_hostname to None and trigger the SPN-mismatch error this commit was supposed to fix. F2 — kdc_proxy.rs / send_in_process_kdc_request: drop the `unwrap_or(&conf.hostname)` fallback and error out cleanly when entry.target_hostname is missing. The previous fallback would silently revert to Gateway's hostname — exactly the bug the prior commit fixed — making future regressions invisible. Plumbing cleanup: gateway_hostname is no longer threaded through send_network_request → resolve_server_generator / resolve_client_generator / credssp_loop / perform_credssp_with_client. The in-process loopback resolves the Kerberos hostname per session from entry.target_hostname; nothing else on this chain ever needed Gateway's own hostname. End-to-end Kerberos credential injection still works after this cleanup; verified against an AD-joined RDP server via DVLS web client. Tracking: review-followup.md
…st layout)
Round-5 review feedback from codex:
1. cargo +nightly fmt --all --check now passes. Previous commits left three
blocks unformatted (kdc_proxy.rs realm match, kdc_proxy.rs handle_kdc_proxy
call site, rdp_proxy.rs::send_in_process_kdc_request). cargo fmt
auto-applied.
2. cargo clippy -p devolutions-gateway --tests -- -D warnings now passes.
Removed `#[expect(clippy::too_many_arguments)]` on
`perform_credssp_with_client` — the previous commit dropped the
`gateway_hostname` parameter from the chain, so the function is below
clippy's threshold and the lint expectation is now unfulfilled.
3. Generated .NET OpenAPI client (`Devolutions.Gateway.Client`) was missing
the new response shape entirely. Added:
- `PreflightOutput.CredInjectionId: Guid?`
- `PreflightOutputKind.ProvisionedCredentials = 7`
plus the corresponding `ToValue` extension case and constructor parameter.
The OpenAPI YAML already exposes both. Without these the package can't
deserialize the new preflight `provisioned-credentials` response.
4. Moved `credential/mod.rs`'s test module to its own file at
`credential/tests.rs`. Replaced the inline `#[cfg(test)] mod tests { ... }`
block with `#[cfg(test)] mod tests;`. Tests still see private items via
`use super::*;` because `tests` is a child module of `credential`.
Verification:
- `cargo +nightly fmt --all --check` ✓ clean
- End-to-end demo still works: AD-joined RDP server via DVLS web client →
Gateway (proxy MITM with Kerberos credential injection) → target.
irvingouj@Devolutions (irvingoujAtDevolution)
force-pushed
the
dgw-378-explicit-identity
branch
from
89c47b0 to
d6e3e3b
Compare
…esponse `test_provision_credentials_success` expected `ack` for the `provision-credentials` operation. After this branch the operation returns the new `PreflightOutputKind::ProvisionedCredentials` variant carrying the freshly minted `cred_injection_id`. Update the assertion accordingly and switch the credential-store lookup to use the returned id (the previous test used the token JTI as the primary key, which no longer is).
…String
Previous shape on `SessionKerberos`:
#[derive(Debug)]
pub struct SessionKerberos {
pub krbtgt_key: Vec<u8>,
pub service_long_term_key: Vec<u8>,
pub service_user_name: String,
pub service_user_password: String,
pub realm: String,
}
`#[derive(Debug)]` would render the AES-256 key bytes and the
PA_ENC_TIMESTAMP password verbatim into any `tracing::debug!(?kerberos, …)`
call. Nobody currently does that, but it is the kind of leak that is hard
to spot in review and easy to introduce by accident later. The rest of the
codebase already treats secrets through `secrecy::SecretString` / a custom
`Debug`-redacted wrapper (`crypto::EncryptedPassword`), so it is worth
bringing this struct in line with that pattern.
Two layers of protection now apply:
1. Type-level. The two AES-256 keys are wrapped in `SecretBox<Vec<u8>>`
and the service-user password in `SecretString`. Access requires
`expose_secret()`, which is greppable and reviewable. SecretBox /
SecretString implement a `Debug` that prints the redacted form.
2. Struct-level. `SessionKerberos` no longer derives `Debug`; it has a
manual `impl fmt::Debug` that explicitly prints `<redacted>` for the
three secret fields. This keeps the redaction working even if a future
refactor changes the field types back to plain `Vec<u8>` / `String`.
Use sites updated to call `expose_secret()` at the boundary where the
fake-KDC config (`kdc::config::KerberosServer`) and sspi-rs's
`AuthIdentityBuffers` need raw bytes / strings:
- `credential/mod.rs::build_session_kdc_config` (3 fields, plus the
shared service-user password reused twice — extracted into a local).
- `rdp_proxy.rs::build_credential_injection_server_kerberos_config`
(`service_user_password` for sspi-rs `AuthIdentityBuffers`,
`service_long_term_key` for `ServerProperties::new`).
The `realm` and `service_user_name` fields stay as plain `String` —
neither is sensitive: `service_user_name` is a fixed `"jet"`, and
`realm` is either an explicit Kerberos realm parsed from the proxy
username or the synthetic `CRED-{uuid}.INVALID` fallback.
Tests in `credential/tests.rs` only read `kerberos.realm`; no test
changes needed.
The handler had grown to ~115 lines doing six distinct things in one top-to-bottom flow: token auth, message decode, realm resolution (envelope + entry fallback), realm-vs-token check, fake-KDC processing for credential-injection sessions, and forward-to-real-KDC for everything else. The control flow was a mix of `let`-deconstruction, in-place matching, and an early `return` on the injection branch — readable on first pass, hard to skim on the third. Split into the orchestrator + five helpers, each with a one-line doc: - `authenticate_kdc_token` — runs the auth middleware, unwraps `Kdc`. - `lookup_credential_injection` — resolves `claims.jet_cred_id` against the credential store, returning `None` for the forward-to-real-KDC path (legacy DVLS or expired entries). - `resolve_request_realm` — picks the realm from the envelope; falls back to `entry.kerberos.realm` when the envelope is empty and we have an injection entry; errors out otherwise. - `enforce_realm_token_match` — refuses forwarding when realms disagree; the `disable_token_validation` debug option downgrades to a warning. - `process_credential_injection` — builds the per-session fake-KDC config and runs `handle_kdc_proxy_message` synchronously. - `forward_to_real_kdc` — async, sends to the real KDC and wraps the response back as a `KdcProxyMessage`. `kdc_proxy` itself shrinks to ~30 lines that read top-to-bottom as the flow: auth, decode, resolve realm, enforce token/realm match, dispatch. No behavioral change. Same error codes, same trace/debug log fields, same realm-resolution semantics.
…d path `resolve_request_realm` was taking `injection_entry` to compute its fallback. That mixed two concerns in one shared helper: the envelope extraction (common to both paths) and the per-session realm fallback (specific to credential injection). The forward-to-real-KDC path doesn't care about that fallback at all, but it had to know about it because the realm-resolution flow ran in the orchestrator before dispatch. Restructure so injection-specific logic lives entirely inside the injection branch: - `resolve_request_realm` -> `extract_envelope_realm`. Now a pure data extractor: read `target_domain` and treat empty string as absent. No policy. - The orchestrator stops resolving / validating the realm. It just extracts the envelope value and dispatches. - `process_credential_injection` owns the injection fallback: if envelope is empty, fall back to `entry.kerberos.realm` (captured at preflight). Then runs `enforce_realm_token_match` itself before doing any KDC work. - `forward_to_real_kdc` owns its own (simpler) rule: envelope realm is required, no fallback. Then `enforce_realm_token_match` itself. Realm validation (`enforce_realm_token_match`) stays a small shared helper because the rule is identical on both paths. Each dispatch path calls it as part of its own setup. Also destructure `KdcTokenClaims` once at the top of the orchestrator so helpers receive the individual fields they need (`token_realm: &str`, `token_kdc_addr: &TargetAddr`) instead of a borrowed claims struct that they'd have to repeatedly access through `claims.*`. Drops the trivial `lookup_credential_injection` helper — `jet_cred_id.and_then(|id| store.get(id))` is clearer inline now that it's a one-liner. No behavioral change. Same error codes, log fields, and realm semantics.
…ingle-use)
IronRDP's STYLE.md is the closer-fit guide for this codebase. Two rules
were violated by the prior refactor:
- "Avoid creating single-use helper functions" (exception: needs `?` /
`return`). `extract_envelope_realm` did neither, so it should be a
block. Same for `realm_from_username` in `credential/mod.rs`.
- "Local helper functions ... at the end of the enclosing function (this
requires using a return statement)". When a helper does qualify (uses
`?`), the convention is to nest it inside the parent function and
trigger via `return` rather than place it as a sibling at module level.
`api/kdc_proxy.rs`:
- Inlined `extract_envelope_realm` into the orchestrator (5-line method
chain). The naming `envelope_realm` keeps the intent self-documenting.
- Moved `authenticate_kdc_token`, `process_credential_injection`, and
`forward_to_real_kdc` inside `kdc_proxy`. The orchestrator now ends
with `return match injection_entry { … };`, after which the three
nested fns are defined.
- `enforce_realm_token_match` stays at module level — multi-use, so the
rule does not apply.
`credential/mod.rs`:
- Inlined `realm_from_username` into `encrypt_with_kerberos`. The
inline form (split_once + filter + map) is short and the comment
above it explains the why.
- `synthetic_realm` stays at module level — referenced from the test
module via `super::synthetic_realm`, so it has more than one caller.
No behavioral change. `cargo fmt --check` clean.
The KDC proxy route is `/jet/KdcProxy/{token}` — the token sits in the URL
path, not the standard `Authorization: Bearer` header or `?token=` query
parameter the global auth middleware (`middleware/auth.rs`) handles. As a
result the route is listed in `AUTH_EXCEPTIONS`, the middleware skips it,
and the handler had to call `authenticate()` itself.
That works but it makes the handler look different from every other
authenticated endpoint, which use the `FromRequestParts` extractor pattern
(`AssociationToken`, `JmuxToken`, `JrlToken`, `JrecToken`, `ScopeToken`).
Add a dedicated `KdcToken` extractor in `extract.rs` that:
- pulls the token out of the path with `Path::<String>`,
- pulls `ConnectInfo<SocketAddr>` for the source address the auth routine needs,
- runs `crate::middleware::auth::authenticate()` exactly the way the
global middleware would,
- and unwraps the `Kdc` variant before returning.
Unlike the other extractors in the file, this one is concrete on
`DgwState` rather than generic over `<S>` because it actually performs
authentication and needs `state.token_cache`, `state.jrl`, etc. — the
others just pull `AccessTokenClaims` out of request extensions that the
middleware already populated.
`api/kdc_proxy.rs`:
- Handler signature now takes `KdcToken(KdcTokenClaims { krb_realm,
krb_kdc, jet_cred_id }): KdcToken` directly. Path/source-addr
extraction and the `match` on `AccessTokenClaims::Kdc` move into the
extractor.
- The nested `authenticate_kdc_token` helper goes away.
- `AUTH_EXCEPTIONS` configuration in `middleware/auth.rs` is unchanged —
the global middleware still does not know how to find a path-bound
token, so the route still has to be exempted there. The extractor is
the route-local replacement.
No behavioural change. Same error codes (401 from middleware, 403 from
the type mismatch), same auth path, same logged events. The handler
just looks like every other authenticated endpoint now.
…tate> The credential entry's mapping / kerberos / target_hostname triplet was three separate Option fields, which let the type system represent illegal combinations (e.g. mapping set without kerberos). Bundle them into a single InjectionState so the "all set or all unset" invariant lives in the type system rather than in defensive checks scattered across consumers. target_hostname becomes a required String inside InjectionState: credential injection only fires for RDP and the fake-KDC's TGS-REQ sname check needs it, so missing/malformed dst_hst is now rejected at insert time (preflight) instead of surfacing mid-CredSSP.
- token.rs: add `skip_serializing_if = "Option::is_none"` on `jet_cred_id` in both AssociationClaimsHelper and KdcClaimsHelper so absent claims are omitted from the wire instead of serializing as explicit nulls (matches `jet_agent_id` and tokengen). - Revert local-dev `<Version>` bumps on Devolutions.Gateway.Utils.csproj (2025.10.2 → 2025.10.1.0) and Devolutions.Gateway.Client.csproj (2025.12.3 → 2025.12.2); the release pipeline owns publishing.
irvingouj@Devolutions (irvingoujAtDevolution)
changed the title
irvingouj@Devolutions (irvingoujAtDevolution)
requested review from
Benoît Cortier (CBenoit) and
Copilot
There was a problem hiding this comment.
Pull request overview
Implements DGW-378 by introducing an explicit cred_injection_id minted at preflight and propagated as jet_cred_id in JWTs, then used to route Kerberos credential-injection traffic deterministically (not heuristically) for /rdp and /jet/KdcProxy.
Changes:
- Add
cred_injection_idto preflight provision-credentials requests/responses and threadjet_cred_idthrough Association/KDC token claims. - Refactor credential store to key entries by
cred_injection_id, attach per-session Kerberos material, and requiredst_hstto capture the target hostname for SPN validation. - Update Gateway KDC proxy handling and RDP proxy Kerberos loopback dispatch to use the explicit ID, and update tooling/.NET helpers accordingly.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/dotnet/Devolutions.Gateway.Utils/src/ProvisionCredentialsRequest.cs | Adds a typed DTO for provision-credentials including cred_injection_id. |
| utils/dotnet/Devolutions.Gateway.Utils/src/KdcClaims.cs | Adds optional jet_cred_id claim for KDC tokens. |
| utils/dotnet/Devolutions.Gateway.Utils/src/AssociationClaims.cs | Adds optional jet_cred_id claim for association tokens. |
| tools/tokengen/src/server/server_impl.rs | Accepts/forwards jet_cred_id in token generation server endpoints. |
| tools/tokengen/src/main.rs | Adds --jet-cred-id CLI option for relevant subcommands. |
| tools/tokengen/src/lib.rs | Serializes jet_cred_id into Association/KDC token claims. |
| devolutions-gateway/src/token.rs | Adds jet_cred_id to claims + helper extractors (extract_jet_cred_id, extract_dst_hst). |
| devolutions-gateway/src/credential/mod.rs | Refactors credential store indexing to cred_injection_id, adds injection state + per-session Kerberos material, enforces dst_hst presence for injection sessions. |
| devolutions-gateway/src/credential/tests.rs | Adds unit tests for insert/lookup/eviction and synthetic realm behavior. |
| devolutions-gateway/src/api/preflight.rs | Adds optional cred_injection_id input and returns provisioned-credentials output kind with the final id. |
| devolutions-gateway/tests/preflight.rs | Updates preflight test expectations to the new output kind and returned id. |
| devolutions-gateway/src/api/kdc_proxy.rs | Routes injection sessions via jet_cred_id lookup; forwards otherwise; factors realm enforcement. |
| devolutions-gateway/src/extract.rs | Adds KdcToken extractor to authenticate path-bound /jet/KdcProxy/{token} tokens. |
| devolutions-gateway/src/rdp_proxy.rs | Adds in-process KDC dispatch via http://cred.invalid/{id} and threads credential store into CredSSP server generator. |
| devolutions-gateway/src/rd_clean_path.rs | Switches injection lookup to jet_cred_id first, fallback to JTI index; threads credential store into proxy path. |
| devolutions-gateway/src/generic_client.rs | Same injection lookup precedence and threads credential store into RdpProxy. |
| devolutions-gateway/src/openapi.rs | Updates OpenAPI schema representation for cred_injection_id and new output kind. |
| devolutions-gateway/openapi/gateway-api.yaml | Adds cred_injection_id to preflight models and new provisioned-credentials kind. |
| devolutions-gateway/openapi/dotnet-client/.../PreflightOutputKind.cs | Adds provisioned-credentials enum variant. |
| devolutions-gateway/openapi/dotnet-client/.../PreflightOutput.cs | Adds cred_injection_id field to generated model. |
| devolutions-gateway/openapi/dotnet-client/.../PreflightOperation.cs | Adds cred_injection_id to generated model. |
| devolutions-gateway/src/api/webapp.rs | Initializes new claims fields (jet_cred_id: None) when signing tokens. |
| docs/plans/2026-04-27-dgw-378-explicit-identity-design.md | Adds design doc for explicit identity routing. |
| review.md | Adds an internal review write-up (likely not intended for merge). |
| review-followup.md | Adds follow-up internal review notes (likely not intended for merge). |
💡 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 22 out of 22 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- credential store: reject `cred_injection_id` reuse across different association tokens (`InsertError::Conflict`); same-JTI idempotent re-provision still works. - credential store: filter expired entries at lookup so `time_to_live` is a hard limit rather than depending on the 15-minute cleanup sweep. - rdp_proxy: cache the KDC-proxy HTTP client and apply connect/total timeouts so a stalled external KDC proxy cannot hang CredSSP indefinitely.
Mirror the external KDC proxy handler's behavior (`api/kdc_proxy.rs:55`): treat an empty `target_domain` from the wire as missing and fall back to the session's stored realm. Prevents a malformed ASN.1 zero-length OCTET STRING from producing an empty-realm KDC config that would later fail with a cryptic salt/principal mismatch deep in CredSSP.
irvingouj@Devolutions (irvingoujAtDevolution)
changed the title
Benoît Cortier (CBenoit)
requested a review
from Pavlo Myroniuk (TheBestTvarynka)
Pavlo Myroniuk (TheBestTvarynka)
left a comment
Pavlo Myroniuk (TheBestTvarynka)
left a comment
There was a problem hiding this comment.
Wow, amazing feature 🤩. Per-session Kerberos material is a great improvement.
I reviewed the PR. LGTM.
There was a problem hiding this comment.
Thanks for tackling this! This is clearly moving in the right direction, and the test coverage you added on CredentialStore is appreciated. That said, I'd like us to be more conservative on three architectural points before this lands:
-
The
credentialmodule should not own Kerberos.SessionKerberos,build_session_kdc_config,principal_for_realm,kerberos_salt,synthetic_realm,encrypt_with_kerberos, and thedst_hstextraction insideinsertall turn what used to be a protocol-neutral store into a Kerberos-aware one. They belong in a newrdp_proxy::kerberos_session(or similar) submodule, withInjectionStatereduced to{ mapping, target_hostname }and the target hostname passed toinsertas a typed parameter from preflight (which already has the validated token). -
The new
cred_injection_idclaim/field is avoidable. The PR's own backwards-compat fallback (get_by_tokenkeyed by JTI) demonstrates that the association-token JTI is already a sufficient identifier. I'd like us to take the minimal-surface variant: keep ajet_cred_idclaim only onKdcTokenClaims, defined as "the JTI of the associated session token", i.e. it's a reference to another token's ID, not a reuse of one. With that, we drop:jet_cred_idonAssociationTokenClaimscred_injection_idrequest field onProvisionCredentialsParamsPreflightOutputKind::ProvisionedCredentials(revert toAck)InsertError::Conflictand the whole "caller proposes an ID" threat modelassociation_token_index+get_by_token- All the OpenAPI / .NET client additions tied to those
-
The
enable_unstablegate was silently removed. Whether proxy-based Kerberos injection is ready to be promoted out of the unstable gate is a separate decision that deserves its own PR and CHANGELOG entry. Please keep the gate aroundbuild_credential_injection_server_kerberos_confighere. Most likely we’ll simply open a PR once this lands.
A few smaller items in line comments.
| /// `tracing::debug!(?kerberos, ...)`. Access requires an explicit `expose_secret()` call, | ||
| /// which is greppable and reviewable. The custom [`fmt::Debug`] adds a second layer that | ||
| /// stays correct even if a future refactor changes the field types. | ||
| pub struct SessionKerberos { |
There was a problem hiding this comment.
Move all of this out of credential/. SessionKerberos, build_session_kdc_config (line 179), principal_for_realm, kerberos_salt, synthetic_realm, encrypt_with_kerberos, random_32_bytes, plus the hardcoded service_user_name: "jet" and max_time_skew: 300 — all of this is Kerberos/CredSSP session glue that has no business inside what used to be a protocol-neutral credential store.
Suggested layout: a new rdp_proxy::kerberos_session (or credssp_session) submodule owns the SessionKerberos type and the KDC config builder. InjectionState in credential/ becomes just { mapping, target_hostname }. The Arc<SessionKerberos> is either lazily derived on first CredSSP use, or stored in a parallel HashMap<Uuid, Arc<SessionKerberos>> owned by the RDP layer (keyed by the same UUID — see point 2 in the summary, that UUID should be the association-token JTI).
This also lets CleartextAppCredentialMapping::encrypt_with_kerberos go back to a plain encrypt, so non-Kerberos use of the store stops minting AES-256 keys it never uses.
| // (`token.rs::parse_target_address`). Missing/malformed `dst_hst` is fatal | ||
| // here so the failure surfaces at preflight rather than mid-CredSSP. | ||
| const DEFAULT_DST_PORT: u16 = 3389; | ||
| let raw = crate::token::extract_dst_hst(&token) |
There was a problem hiding this comment.
Extracting dst_hst from the JWT inside the credential store is a layering violation. The preflight handler in api/preflight.rs already has the validated token in hand and could resolve target_hostname: String itself, then pass it to insert as a typed parameter. That removes the credential module's dependency on JWT shape beyond jti, and removes the second discard_signature() call on a token whose validation invariant is documented elsewhere.
A short doc comment on CredentialStore::insert stating the upstream-validation invariant for the existing JTI extraction would also be welcome, regardless.
| pub jet_agent_id: Option<Uuid>, | ||
|
|
||
| /// Reference to a credential-injection record provisioned via /jet/preflight. | ||
| pub jet_cred_id: Option<Uuid>, |
There was a problem hiding this comment.
Please drop jet_cred_id from AssociationTokenClaims. The association token already has a jti, and that JTI uniquely identifies the credential-store entry — every existing call site in this PR (generic_client.rs, rd_clean_path.rs) already falls back to credential_store.get_by_token(token) for old DVLS, which proves a JTI-keyed lookup suffices.
Keep jet_cred_id only on KdcTokenClaims (line 624), defined as "the JTI of the associated session token." That preserves your invariant that JTIs are not reused across tokens — the KDC token has its own jti, and jet_cred_id is a reference to a different token's ID. The KDC handler then does credential_store.get(claims.jet_cred_id) against the same JTI-keyed store.
This change cascades into removing extract_jet_cred_id/extract_optional_uuid callers on the association path, the preflight request field, the provisioned-credentials output kind, the Conflict error, the reverse association_token_index, and the get_by_token helper.
| #[serde(flatten)] | ||
| mapping: crate::credential::CleartextAppCredentialMapping, | ||
| #[serde(default)] | ||
| cred_injection_id: Option<Uuid>, |
There was a problem hiding this comment.
Revert this addition (and the PreflightOutputKind::ProvisionedCredentials variant at line 120, plus the provisioned_cred_injection_id plumbing at line 348 and the response shaping at line 372). With the JTI-keyed approach, the preflight contract is unchanged: Ack stays, no new request field, no new output kind. DVLS already knows the association-token JTI it minted; that's the identifier.
| allOf: | ||
| - $ref: '#/components/schemas/AppCredential' | ||
| nullable: true | ||
| cred_injection_id: |
There was a problem hiding this comment.
Revert this cred_injection_id field (and the matching one at line 1361 on PreflightOutput, plus the provisioned-credentials enum value). Falls out of the preflight revert. The .NET client regen will follow automatically — please don't hand-edit it.
| kerberos.service_user_password.expose_secret(), | ||
| )); | ||
|
|
||
| let kdc_url = Url::parse(&format!("http://cred.invalid/{}", credential_entry.cred_injection_id)) |
There was a problem hiding this comment.
About the http://cred.invalid/<id> trampoline: I see why you reached for it (sspi-rs's CredSSP server config exposes kdc_url: Option<Url>, and you needed a way to dispatch a synthetic in-process "KDC"), but the implementation is heavier than the idea. Three concrete asks:
-
Verify whether sspi-rs offers a custom KDC dispatcher API (trait object, async closure,
KdcClient-like abstraction). If yes — use it; the URL trick goes away andCredentialStoreHandleno longer needs to be threaded throughperform_credssp_with_client→process_authentication_with_client→resolve_server_generator→send_network_request. -
If no clean API exists in sspi-rs, that's fine for now (sspi-rs is ours — Devolutions/sspi-rs), but please open a GitHub issue against sspi-rs asking for a pluggable KDC dispatcher and link it from a
// TODO(sspi-rs#NNN)comment here. We can keep a workaround in this PR to unblock; the cleanup lands when sspi-rs ships the API. -
Even with the workaround, please localize the interception. Right now
send_network_requestis a generic Kerberos-network helper that knows about"cred.invalid". Push the loopback dispatch into a non-generic function (closed overArc<SessionKerberos>+mapping+target_hostnamealready in scope atbuild_credential_injection_server_kerberos_config), and keep the generic helper unaware of the magic hostname.
Also related: kdc_proxy_http_client() (line 653) and the "http" | "https" non-loopback branch (line 681) appear to have no real caller in this PR — sspi-rs only emits tcp:///udp:// in the non-injection path and http://cred.invalid/... in the injection path. Either delete this branch or document the external use case it's meant to serve (an undocumented "session token can point at arbitrary HTTP host" capability would be a security concern).
There was a problem hiding this comment.
| None | ||
| }; | ||
| let krb_server_config = | ||
| build_credential_injection_server_kerberos_config(&credential_entry, &server_dns_name, client_addr)?; |
There was a problem hiding this comment.
The previous code in both rdp_proxy.rs and rd_clean_path.rs only built the Kerberos server config when conf.debug.enable_unstable && conf.debug.kerberos.is_some(). The new build_credential_injection_server_kerberos_config is unconditional — every provision-credentials session now goes through the fake-KDC path.
That's a real product-surface change not called out in the PR description. Please keep the enable_unstable. Promotion to stable should be a separate, deliberate PR with its own CHANGELOG entry under feat(dgw).
| } | ||
|
|
||
| #[derive(Debug)] | ||
| pub struct CredentialEntry { |
There was a problem hiding this comment.
Nit: CredentialEntry, InjectionState, SessionKerberos, and build_session_kdc_config are all pub but the only consumer is rdp_proxy in the same crate. Restrict to pub(crate) (or pub(super)) to keep refactor flexibility. Same for the new fields you've added on CredentialEntry.
| store.get_by_token(&token).is_none(), | ||
| "expired entry must not be returned by token" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Test coverage on CredentialStore mechanics is good. Three branches in this PR are still uncovered and I think are reachable from in-process tests (no real RDP client needed):
kdc_proxy::enforce_realm_token_matchbypass underdisable_token_validation.kdc_proxy::process_credential_injectionrealm-fallback when the envelope realm is missing.rdp_proxy::send_in_process_kdc_requestparses the URL path into a UUID, looks up the entry, and dispatches.
Full end-to-end CredSSP testing would need an actual RDP client and isn't the goal here — that's a welcome future improvement. Please add the easy-to-write ones if time permits; otherwise a tracked follow-up PR is fine.
…e on JTI Per review feedback: the association token's JTI already uniquely identifies a credential-injection session, so a separate `cred_injection_id` is redundant. Restructured to that minimal surface: - AssociationTokenClaims no longer carries `jet_cred_id`. The credential store is keyed directly on `extract_jti(token)` / `claims.jti`; the routing layers in `generic_client` and `rd_clean_path` drop the claim-level reference and the `get_by_token` fallback that existed for backwards compatibility. - KdcTokenClaims keeps `jet_cred_id`, redefined as "JTI of the associated session token" — a reference to another token's id, not a shared identity. KDC proxy still resolves it through `CredentialStoreHandle::get`. - CredentialStoreHandle::insert no longer accepts a caller-proposed UUID, no longer returns one, and InsertError::Conflict / association_token_index / get_by_token are gone. CredentialEntry exposes a single `jti` field. - Preflight reverts to `Ack`: ProvisionedCredentials output kind dropped, cred_injection_id request field dropped, OpenAPI schema follows. - tokengen drops --jet-cred-id from the RdpTls subcommand (association). The KDC subcommand keeps it. - .NET utils: AssociationClaims.JetCredId removed; KdcClaims.JetCredId kept; ProvisionCredentialsRequest.CredInjectionId removed. - Hand-edited dotnet-client model classes reverted to master so the release pipeline regenerates them from the updated OpenAPI.
Bundles the remaining structural feedback from the PR review:
- Kerberos out of `credential/`. New `credssp_session` module owns
`SessionKerberos`, `build_session_kdc_config`, principal/salt/synthetic-realm
helpers, and a parallel JTI-keyed `SessionKerberosStoreHandle` with its own
cleanup task. `InjectionState` in the credential store shrinks to
`{mapping, target_hostname}`. `target_hostname` is now resolved at preflight
(where the validated token is) and passed to `insert` as a typed parameter;
the store no longer touches JWT shape. `CredentialStoreHandle::insert`
returns `anyhow::Result` — `InsertError::InvalidToken` no longer applies and
`InsertError::Conflict` is already gone. The two stores are paired at
preflight and threaded through to the RDP layer as direct refs, so no
store lookup happens during CredSSP/KdcProxy.
- `enable_unstable` gate restored. The preflight handler now rejects
`provision-credentials` operations with `unsupported-operation` unless
`__debug__.enable_unstable` is set. Promotion to stable is left to a
follow-up PR with its own CHANGELOG entry.
- `cred.invalid` trampoline localized. `send_network_request` is now a
generic tcp/udp helper that has no knowledge of the credential-injection
loopback; the magic hostname lives entirely inside
`InProcessKdcDispatch::try_handle`, which the server-side CredSSP resolver
consults first. `kdc_proxy_http_client()` and the unused `http`/`https`
non-loopback branch are deleted. The dispatcher also verifies the URL JTI
matches the entry's JTI as defense-in-depth. Tagged with a
`TODO(sspi-rs)` note for the future pluggable-KDC-dispatcher API.
- Visibility tightened. `CleartextAppCredential[Mapping]`, internal helpers,
`CredentialStoreHandle::insert`, and the new `credssp_session` internals are
`pub(crate)`. Types reachable through the public `DgwState` graph stay
`pub` (without breaking the integration test).
- Tests for the three branches CBenoit flagged as reachable without a real
RDP client: `enforce_realm_token_match` (case-insensitive match,
with/without bypass), `resolve_injection_realm` (extracted helper covering
the envelope-realm fallback), and `InProcessKdcDispatch` URL parsing
(non-loopback host, malformed UUID path, JTI mismatch).
Summary
Implements DGW-378: proxy-based Kerberos credential injection for Web Access. A new
cred_injection_id(UUID) is minted at/jet/preflight provision-credentialsand propagated through DVLS-issued association and KDC tokens as the JWT claimjet_cred_id— the gateway uses it as the primary credential-store key for/rdpand/jet/KdcProxy. Replaces the heuristic AS-REQ router from the experimentaldgw-378-session-redesignbranch.Coupled with DVLS-13821; old DVLS still works for NTLM injection via the JTI fallback (no regression).
Highlights
SessionKerberos(krbtgt + service long-term keys,secrecy-wrapped) with aCRED-{uuid}.INVALIDsynthetic realm for bare-UUID proxy usernames.http://cred.invalid/{cred_injection_id}(RFC 6761 reserved TLD) — no HTTP, no token, no middleware.dst_hst(TERMSRV/<target>) so client AP-REQ tickets validate against the impersonated server identity. Required at insert time — missing/malformeddst_hstis rejected at preflight, not mid-CredSSP.Verification
credential/tests.rs(insert / lookup / eviction / synthetic realm).Test plan