feat(dgw): provision the target-side KDC for credential injection by irvingoujAtDevolution · Pull Request #1865 · Devolutions/devolutions-gateway

irvingouj@Devolutions (irvingoujAtDevolution) · 2026-07-24T17:54:55Z

WIP NOT READY

Copilot

Pull request overview

Refactors credential provisioning into a generic token-keyed store and adds target KDC connection options.

Changes:

Introduces generic provisioning storage with TTL cleanup.
Separates credential storage from KDC session management.
Exposes and consumes krb_kdc through preflight APIs and generated clients.

Reviewed changes

Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`devolutions-gateway/src/token_keyed_store.rs`	Adds generic JTI-keyed storage.
`devolutions-gateway/src/target_connection_options.rs`	Defines target KDC options.
`devolutions-gateway/src/target_addr.rs`	Adds URL conversion.
`devolutions-gateway/src/service.rs`	Registers provisioning services.
`devolutions-gateway/src/rdp_proxy.rs`	Applies provisioned KDC configuration.
`devolutions-gateway/src/rd_clean_path.rs`	Resolves provisioned credentials.
`devolutions-gateway/src/provisioning.rs`	Adds encrypted provisioning storage.
`devolutions-gateway/src/openapi.rs`	Adds connection-options schema.
`devolutions-gateway/src/ngrok.rs`	Passes new service handles.
`devolutions-gateway/src/listener.rs`	Passes new service handles.
`devolutions-gateway/src/lib.rs`	Exposes modules and state fields.
`devolutions-gateway/src/kdc_connector.rs`	Shares KDC scheme validation.
`devolutions-gateway/src/generic_client.rs`	Resolves credential injection through provisioning.
`devolutions-gateway/src/credential/mod.rs`	Removes credential-store responsibilities.
`devolutions-gateway/src/credential/crypto.rs`	Updates encryption documentation.
`devolutions-gateway/src/credential_injection_kdc.rs`	Separates KDC session caching from provisioning.
`devolutions-gateway/src/api/rdp.rs`	Wires provisioning into RDP handling.
`devolutions-gateway/src/api/preflight.rs`	Provisions connection options.
`devolutions-gateway/src/api/kdc_proxy.rs`	Resolves KDC state from provisioning.
`openapi/ts-angular-client/model/targetConnectionOptions.ts`	Adds generated TypeScript model.
`openapi/ts-angular-client/model/preflightOperation.ts`	Exposes connection options.
`openapi/ts-angular-client/model/models.ts`	Exports the new model.
`openapi/ts-angular-client/.openapi-generator/FILES`	Tracks generated TypeScript output.
`openapi/gateway-api.yaml`	Defines the public schema.
`openapi/dotnet-client/src/Devolutions.Gateway.Client/Model/TargetConnectionOptions.cs`	Adds generated .NET model.
`openapi/dotnet-client/src/Devolutions.Gateway.Client/Model/PreflightOperation.cs`	Adds connection options to preflight operations.
`openapi/dotnet-client/README.md`	Lists the new model.
`openapi/dotnet-client/docs/TargetConnectionOptions.md`	Documents the new model.
`openapi/dotnet-client/docs/PreflightOperation.md`	Documents the new property.
`openapi/dotnet-client/.openapi-generator/FILES`	Tracks generated .NET output.
`openapi/doc/index.adoc`	Updates generated API documentation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Benoît Cortier (CBenoit) · 2026-07-27T12:58:55Z

+#[derive(Debug)]
+struct CachedSession {
+    entry: WeakProvisioningEntry,
+    session: Arc<CredentialInjectionKdcSession>,
 }


question: This logic sounds a bit weak to me. If some other part of the code ends up holding a "strong" Arc on the same provisioning entry, then this logic will fail at detecting the re-provisioning happened. Also, this raises another question: do we need to cache the provisioning entry, at all, if we can just lookup again? And also, what are we trying to achieve exactly? What is the use case for re-provisioning? What happens when this happens in the middle of credential injection? Is it really something we want to deal with? I appreciate hardened code, but I’m not really sure this one makes sense

With the two-store split and replace semantics below, I think we can drop entry-identity tracking entirely, see https://github.com/Devolutions/devolutions-gateway/pull/1865/changes#r3657581558

Benoît Cortier (CBenoit) · 2026-07-27T13:29:28Z

+    pub(crate) mapping: Option<AppCredentialMapping>,
+    pub(crate) connection_options: Option<TargetConnectionOptions>,


suggestion: split this into two token-keyed stores, one per provisioned payload.

Two independent Options under one key is really two separate provisioning events sharing a JTI. Rather than encoding "the other one wasn't provided" as None, let's split the preflight operation into provision-credentials and provision-connection-options, backed by two TokenKeyedStores keyed by the same JTI:

TokenKeyedStore<AppCredentialMapping>: stays behind ProvisioningStore, which remains the encryption boundary.

TokenKeyedStore<TargetConnectionOptions>: no crypto dependency at all, which actually strengthens the "master key never leaves this module" property claimed in the doc comment above.

Rationale:

Distinct trust semantics. One half is master-key-encrypted secret material, the other is plaintext routing metadata. Separate operations let them be authorized, logged and audited differently.

Protocol variance. TargetConnectionOptions will grow per-protocol shapes (SSH, VNC, …). A dedicated operation gives it its own schema without dragging credential fields along.

Independent provisioning. DVLS may know one before the other; today the second call has to either re-send the first payload or clobber it with None.

Keep replace semantics: no merge API. Merging in place would require Arc<Mutex<TokenKeyedEntry<V>>> so entries could be mutated, on top of the store-level mutex. That turns every read into a lock+clone (or a guard whose lifetime has to be threaded through the CredSSP paths) and forces a merge contract onto V, which would undo the opacity the generic store just bought us. Immutable snapshots per entry are much friendlier to the async consumers.

Suggested surface: one insert entry point dispatching on operation kind, plus get_credentials(jti) / get_connection_options(jti) and a combined getter returning a unifying view. The RDP path wants both; the SSH/VNC paths will likely want only the options half. Two cleanup_task()s (or one sweeping both) is a trivial cost.

TTLs may diverge: that's fine. I initially thought we should keep the longer of the two, but a missing half is already a normal, handled outcome: credential_injection_kerberos_configs errors out (and per my other review comment, should arguably fall back to NTLM) when krb_kdc is absent. So an early-expired half degrades into a path that has to exist anyway. Silently extending one store's TTL to match the other would also mean retaining provisioned data longer than the API user asked for. We should document on the preflight API that both operations should carry the same timeToLive, and warn! with the JTI when a lookup finds one half but not the other, so a support case reads "connection options expired before credentials" instead of the generic krb_kdc context error.

It does feel like a bad design, let me update accordingly

Benoît Cortier (CBenoit) · 2026-07-27T15:54:24Z

+            // 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()))?;


question: Is it possible for a TargetAddr to "not be a URL"? (I think it already guarantees a shape like: <scheme>://<host>:<port>

Benoît Cortier (CBenoit) · 2026-07-27T15:55:11Z

+#[derive(Debug, Clone, Deserialize)]
+#[serde(try_from = "RawTargetConnectionOptions")]
+pub(crate) struct TargetConnectionOptions {
+    krb_kdc: Option<TargetAddr>,
+}


praise: It’s robust, fail-fast design. The type has good invariants.

Benoît Cortier (CBenoit) · 2026-07-27T16:03:22Z


-    let credentials = devolutions_gateway::credential_injection_kdc::CredentialService::new();
+    let provisioning = devolutions_gateway::provisioning::ProvisioningStore::new();
+    let credential_injection = devolutions_gateway::credential_injection_kdc::CredentialInjectionKdcService::new();


suggestion: This connects to what I suggested above about having two TokenKeyedStore, but I think the provisioning facade could assemble everything together:

#[derive(Debug, Clone)] pub struct Provisioning { credentials: TokenKeyedStore<AppCredentialMapping>, connection_options: TokenKeyedStore<TargetConnectionOptions>, kdc_sessions: CredentialInjectionKdcService, }

With a few helper functions to access things in a consistent manner; we are still keeping concerns separated without needing extra threading.

Copilot

Pull request overview

Copilot reviewed 34 out of 34 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

devolutions-gateway/src/target_connection_options.rs:25

Parseability does not fully validate the routing host. For example, tcp://user@dc.example.com:88 passes these checks, but TargetAddr::host() remains user@dc.example.com; KdcConnector::send_network_request reparses that value and attempts to route/DNS-resolve user@dc.example.com, causing the late session failure this validation is intended to prevent. Reject URL userinfo and other cases where the parsed URL host differs from the TargetAddr host, or canonicalize from the parsed URL.

+        // Replace rather than reject: a fast RDP reconnect can register before the previous
+        // connection's registration has dropped. The newest connection owns the JTI.
+        self.sessions.lock().insert(jti, Arc::clone(&session));


+            // provision-token stores nothing: the current model has no credential-less entry, and a
+            // token-only entry never affected connection handling. It is kept only so older callers
+            // that still send it get a well-formed token validated and acknowledged.


irvingouj@Devolutions (irvingoujAtDevolution) · 2026-07-27T21:17:57Z

Benoît Cortier (@CBenoit) You were very right. I revisited the design — I couldn't stand the complexity the previous one introduced, so I did a big refactor to get it under control.

Now it's three layers:

Provisioning (ProvisioningStore) — holds the parsed provisioning data (ProvisionedConnection), protocol-agnostic, so it's good for future extension.
Credential injection (CredentialInjection) — built from the provisioning data.
Synthetic KDC registry (SyntheticKdcRegistry) — keyed by JTI.

We don't ad-hoc retrieve the KDC and credential info to build a Kerberos injection session anymore. We construct it once from the provisioning store and register it across handlers, so both the RDP path and the KDC-proxy path use the same session instead of each rebuilding its own.

And we don't mix credential injection and Kerberos the same ad-hoc way — Kerberos is officially an enum variant of CredentialInjection now (Kerberos vs NTLM), handled through the enum for better semantics.

Tested end to end and all working. I think this is a much cleaner design.

irvingouj@Devolutions (irvingoujAtDevolution) · 2026-07-28T17:57:27Z

Did the split — provision-credentials and provision-connection-options are separate preflight ops now, each backed by its own JTI-keyed store (f804807a). Credentials keep the encryption boundary; connection options are plaintext routing metadata with no crypto dependency. Each half is provisioned, expires and is replaced on its own, and since preflight is batched the caller just sends both in one request. get() folds them back together for the RDP path — credentials required, connection options optional.

Synthetic KDC registry stays its own handle, like we agreed.

Tested end to end: updated the DVLS side to send the two ops, and RDP Kerberos credential injection works through the gateway against a lab DC.

…nAPI spec (#1883) ## What `UpdateProductInfo.version` is typed `VersionSpecification`, which lives in `devolutions-agent-shared` — a crate that does not depend on utoipa. The derived OpenAPI schema therefore emitted a `$ref` to a `VersionSpecification` component that is never generated, leaving a dangling reference in `gateway-api.yaml`. ## Why it matters The dangling `$ref` makes the spec fail strict OpenAPI validation. The client generator (`openapi-generator` 7.9.0, validation on, as `generate_clients.ps1` runs it) refuses **every** gateway target: ``` attribute components.schemas.UpdateProductInfo.VersionSpecification is not of type `schema` [error] Spec has 1 error. ``` So the C#, TypeScript and doc clients cannot be regenerated at all until this is fixed. The bug is pre-existing on `master` (introduced with the Update API in #1726) and stayed latent because nobody had regenerated the clients since. ## Fix Mark the field `#[schema(value_type = String)]`. `VersionSpecification` serializes as `"latest"` or a `YYYY.M.D[.R]` string, so a plain `string` schema matches the wire format exactly — the same treatment as the neighbouring `UpdateProduct`. Regenerated `gateway-api.yaml`; validation now passes with 0 errors. ## Stack Base of the credential-injection stack (bottom to top): 1. **this PR** — make the OpenAPI spec valid again 2. #1862 — decide credential-injection CredSSP protocol once for both legs 3. #1865 — provision the target-side KDC for credential injection

…legs (#1862) ## 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: - Removes the old static `debug.kerberos` configuration block (fake-KDC users and keys) — it was never a real deployment path. - Adds an explicit unstable opt-in, `debug.kerberos_credential_injection`, gating the in-process Kerberos acceptor the Gateway presents to the client. Off by default; still requires `enable_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

The previous run used openapi-generator 7.24.0 instead of the 7.9.0 pinned in devolutions-gateway/openapi/openapitools.json. That churned every generated file and bumped the Angular peer dependency from 18 to 21. Also drops the stray openapitools.json that the generator wrapper left at the repository root, and restores the UTF-8 punctuation in the generated gateway document.

Provisioned session data lived in `credential`, mixed in with the credential types themselves, and its consumer sat inside the credential-injection KDC module. Move the store into a `provisioning` module of its own so `credential` is left holding just credentials, and let the KDC module borrow the store instead of owning it. Callers follow the rename. `CredentialInjectionKdc` also drops its `raw_token` copy, which the stored record already carries. No other behavior change.

Credential injection needed the real KDC of the target's domain and had no way to learn it — the Kerberos leg carried a TODO and passed no KDC proxy URL at all, so only NTLM worked end to end. Add `TargetConnectionOptions`, a second thing a session can have provisioned alongside its credentials, validated at construction so the rest of the code can assume a usable address. The CredSSP client leg now takes its KDC from there. The Kerberos-or-NTLM decision moves to the caller as an `Option`, which is what the absence of connection options means: no KDC, no Kerberos.

…arget options Mechanical output of `tools/generate-openapi/generate.ps1` then `devolutions-gateway/openapi/generate_clients.ps1`, with the pinned 7.9.0 generator. Publishes the `TargetConnectionOptions` schema and its client models, which the previous commit made storable but never described.

…ugh them The KDC session a connection derives is needed in two places at once: the in-process CredSSP intercept and the KKDCP HTTP endpoint, which are different requests. Publish it in a registry keyed by the session JTI, and hand the connection an RAII registration so the entry goes away when the session ends. A reconnect that races the old connection's teardown supersedes it, and the late drop retracts only its own publication. With sessions living in the registry, provisioning no longer has to hand out shared entries: it splits into two independent JTI-keyed stores, credentials and connection options, read back as one snapshot. That removes the weak-reference bookkeeping the old cache needed to notice re-provisioning. Also splits `rdp_proxy` into the proxy and its CredSSP half, and renames `credential_injection_kdc` to `credential_injection` now that it covers both the Kerberos and NTLM paths. The spec and clients are regenerated for the new `provision-connection-options` preflight operation.

irvingouj@Devolutions (irvingoujAtDevolution) marked this pull request as ready for review July 24, 2026 20:40

irvingouj@Devolutions (irvingoujAtDevolution) requested review from Benoît Cortier (CBenoit) and Copilot July 24, 2026 20:42

Copilot started reviewing on behalf of irvingouj@Devolutions (irvingoujAtDevolution) July 24, 2026 20:43 View session

irvingouj@Devolutions (irvingoujAtDevolution) changed the title ~~refactor(dgw): move credential-injection KDC into a generic provisioning store~~ feat(dgw): provision the target-side KDC for credential injection Jul 24, 2026

irvingouj@Devolutions (irvingoujAtDevolution) mentioned this pull request Jul 24, 2026

fix(dgw): decide credential-injection CredSSP protocol once for both legs #1862

Merged

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Comment thread devolutions-gateway/src/rdp_proxy.rs Outdated

Comment thread devolutions-gateway/src/target_connection_options.rs Outdated

Comment thread ...ons-gateway/openapi/dotnet-client/src/Devolutions.Gateway.Client/Model/PreflightOperation.cs

irvingouj@Devolutions (irvingoujAtDevolution) force-pushed the DVLS-14697-kerberos-kdc-injection branch from 552a4d3 to f655d68 Compare July 24, 2026 20:49

irvingouj@Devolutions (irvingoujAtDevolution) force-pushed the DVLS-14697-kdc-provisioning-store branch from e30602e to bc71a43 Compare July 24, 2026 20:51