Skip to content

feat(dgw): provision the target-side KDC for credential injection - #1895

Open
irvingouj@Devolutions (irvingoujAtDevolution) wants to merge 11 commits into
masterfrom
feat/target-connection-options
Open

feat(dgw): provision the target-side KDC for credential injection#1895
irvingouj@Devolutions (irvingoujAtDevolution) wants to merge 11 commits into
masterfrom
feat/target-connection-options

Conversation

@irvingoujAtDevolution

@irvingoujAtDevolution irvingouj@Devolutions (irvingoujAtDevolution) commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

This PR add OP_PROVISION_CONNECTION_OPTIONS, that is, for now, just kdc in there.
Then uses it, construct kdc sessions in each handler on demand.

It is already complete in terms of functionality, but I do believe that the bondary is starting to blur.

Most importantly
CredentialService is taking more responsibility than it should.

@irvingoujAtDevolution irvingouj@Devolutions (irvingoujAtDevolution) changed the title feat(dgw): provision the target-side KDC through connection options feat(dgw): provision the target-side KDC for credential injection Jul 29, 2026
@irvingoujAtDevolution
irvingouj@Devolutions (irvingoujAtDevolution) marked this pull request as ready for review July 30, 2026 15:18
Copilot AI review requested due to automatic review settings July 30, 2026 15:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds target-side KDC provisioning for Kerberos credential injection.

Changes:

  • Adds validated connection options and a preflight provisioning operation.
  • Splits credential and connection-option storage with independent expiry.
  • Routes target-side Kerberos requests through the provisioned KDC.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/preflight.rs Tests provisioning operations.
src/target_connection_options.rs Defines validated KDC options.
src/target_addr.rs Adds URL conversion.
src/rdp_proxy.rs Configures target-side Kerberos.
src/provisioning.rs Stores credentials and options independently.
src/lib.rs Registers the new module.
src/kdc_connector.rs Shares supported KDC schemes.
src/credential_injection_kdc.rs Carries options into session state.
src/api/preflight.rs Adds the provisioning operation.
Comments suppressed due to low confidence (1)

devolutions-gateway/src/rdp_proxy.rs:405

  • The core new routing behavior is not covered by the existing tests in this file: they only exercise injection_uses_kerberos, while no test verifies that a provisioned KDC becomes the client kdc_proxy_url (or that Kerberos rejects a missing KDC). Add focused coverage for both cases to prevent the target-side wiring from regressing.
    Ok(CredentialInjectionKerberosConfigs {
        server: Some(credential_injection_kdc.server_kerberos_config(client_addr)?),
        client: Some(ironrdp_connector::credssp::KerberosConfig {
            kdc_proxy_url: Some(url::Url::try_from(krb_kdc).context("convert target KDC address to URL")?),
            hostname: gateway_hostname.to_owned(),

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

Comment thread devolutions-gateway/src/rdp_proxy.rs Outdated
Comment thread devolutions-gateway/src/api/preflight.rs
Comment thread devolutions-gateway/src/provisioning.rs
Comment thread devolutions-gateway/src/target_connection_options.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

The store keyed by association-token JTI is not credential-specific: a session can
need other things provisioned ahead of the connection, and the next commit adds the
first one. Move it to a `provisioning` module and name it for what it holds, leaving
`credential` with just the credential types.

Pure move and rename — same types, same API, same behavior. Callers follow.
Qualify ProvisioningStore doc links and update comments that still named CredentialStore/credential::cleanup_task.
Kerberos credential injection needs the real KDC of the target's domain, and the
Gateway has no way to discover it: the target-side CredSSP leg passed no KDC proxy
URL at all, so a Kerberos session could never complete and only NTLM worked.

Let the caller provision it. `TargetConnectionOptions` is the second thing a session
can carry alongside its credentials, validated at construction — supported scheme, a
host, a parseable URL — so a bad address is rejected at provisioning time instead of
failing when the session starts. The CredSSP client leg now takes its KDC from there.

The spec and clients are regenerated in the next commit.
Credentials and target connection options are independent provisioning
events with different trust boundaries. Back them with two JTI-keyed
stores and expose provision-connection-options alongside
provision-credentials. provision-token stays validate-and-ack only.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
provision-token still inserts a token-only row. It shares the credentials
insert path with provision-credentials again. Only connection-options is
the new separate provision op.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Runtime dual-op does not need hand-edited utoipa schemas. Spec and clients land in the openapi regen layer on top of this branch.
After #1856, kdc_for uses ConfHandle hostname for TERMSRV SPN, not association-token dst_hst. Update the dual-op unit test accordingly.
Lowercase error/context strings and point credential docs at insert_credentials. OpenAPI for the new preflight op stays in the dedicated regen PR above this layer.
Base automatically changed from feat/provisioning-store to master July 31, 2026 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants