feat(credssp): use the `KdcResolution` enum instead of KDC url by Rostyslav-Romanets · Pull Request #1987 · Devolutions/IronRDP

Rostyslav-Romanets · 2026-09-24T15:43:10Z

This PR adds the KdcResolution enum, which specifies how Kerberos should resolve the KDC, either by using IAKERB proxy or by connecting directly to an external KDC specified by URL.

THis PR is part of integration of the IAKERB extension in sspi-rs and updates the codebase to use the updated public API of sspi-rs.

Related PRs

picky-rs: feat(picky-krb): add IAKerb proxy message encoding/decoding picky-rs#531
sspi-rs: feat(kerberos): add support for IAKerb extension for Kerberos client sspi-rs#751

Copilot

Copilot review overview

🟡 Changes recommended

Local path overrides break clean checkouts, and the testsuite imports an incompatible KdcResolution type.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 1 · 1

Open (3)

Remove invalid local Cargo overrides and regenerate the lockfile · New
Mark the publicly re-exported sspi dependency as public · New
Import the correct KdcResolution enum for the connector field · New

What changed in this PR

Updates CredSSP Kerberos configuration for sspi 0.22 and IAKERB-based KDC resolution.

Changes:

Adds KdcResolution and Kerberos configuration constructors.
Migrates client, web, and test code to the new API.
Updates sspi, picky, and related dependencies.

File	Description
`Cargo.toml`	Adds local dependency overrides.
`Cargo.lock`	Updates the resolved dependency graph.
`ffi/Cargo.toml`	Upgrades `sspi`.
`crates/ironrdp/Cargo.toml`	Upgrades example dependency on `sspi`.
`crates/ironrdp-web/src/session.rs`	Uses URL-based KDC resolution.
`crates/ironrdp-testsuite-extra/tests/client/config.rs`	Updates the KDC URL assertion.
`crates/ironrdp-mstsgu/Cargo.toml`	Upgrades `sspi` and `picky`.
`crates/ironrdp-connector/src/credssp.rs`	Adds KDC strategies and conversion to `sspi`.
`crates/ironrdp-connector/Cargo.toml`	Upgrades authentication dependencies.
`crates/ironrdp-client/src/config.rs`	Maps configuration properties to KDC resolution.
`crates/ironrdp-acceptor/src/credssp.rs`	Propagates request length errors.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Benoît Cortier (CBenoit) · 2026-09-29T08:51:52Z

Thank you for adding this. I’ll review the picky-rs/sspi-rs PRs first.

Copilot

Copilot review overview

🔵 Needs a closer look

IAKERB configuration can retain a conflicting legacy KDC proxy property, and the local dependency patches remain nonportable.

Review effort: Balanced
Findings: None

Resolved since last review (3)

Remove invalid local Cargo overrides and regenerate the lockfile
Mark the publicly re-exported sspi dependency as public
Import the correct KdcResolution enum for the connector field

Previously missed (1)

In code that hasn't changed since last review

Clear stale kdcproxyname when mirroring explicit Kerberos config

crates/ironrdp-client/src/config.rs:1347

When cfg selects IAKerb, an existing kdcproxyname loaded from the property set survives because only kdcproxyurl is cleared. The built config uses IAKERB, but its public properties() still advertises the old proxy; feeding those properties into another builder then selects KdcUrl instead. Clear kdcproxyname whenever an explicit Kerberos config is mirrored so the two representations cannot conflict.

…751) This PR adds support for the IAKERB extension according to the [IAKERB specification](https://datatracker.ietf.org/doc/html/draft-ietf-kitten-iakerb-03). The main changes are focused on the `Kerberos` state machine. Previously, the `Preauthentication` state handled the entire KDC message exchange withing a single async `intialize_security_context` call. This approach does not work with IAKERB, where KDC messages are exchanged via a server that acts as a proxy. The `Preauthentication` state has therefore been split into multiple states, allowing the state machine to save `Kerberos` state between KDC message exchanges. Additionaly, the `AS` and `TGS` exchanges have been extracted into separate state machines. These new state machines are KDC transport-agnostic and can be used with both external KDC and an IAKERB proxy. ## What is IAKERB IAKERB extends Kerberos to support scenarios where the client cannot directly access the KDC. Instead, KDC messages are encapsulated in GSS-API tokens and exchanged through an IAKERB proxy. The server forwards these messages to the LocalKDC, allowing the client to obtain the required Kerberos tickets without direct network access to the KDC. Microsoft recently introduced IAKERB support in Windows Insider builds as part of its effort to reduce NTLM dependency: https://techcommunity.microsoft.com/blog/windows-itpro-blog/reducing-ntlm-dependency-iakerb-and-localkdc-in-windows-insider-preview/4524615. ## Related PRs * picky-rs: Devolutions/picky-rs#531 * IronRDP: Devolutions/IronRDP#1987

Rostyslav-Romanets · 2026-10-23T10:23:53Z

Benoît Cortier (@CBenoit) Hi! Could you let me know when a new version of sspi-rs containing the IAKERB feature is planned to be released?

Copilot

🟡 Changes recommended

Gateway smart-card authentication still calls the removed SSPI constructor, preventing that feature from compiling.

No open issues 🎉

1 resolved since last review

Move regression test into CI-run testsuite

_{Review effort: Balanced}

…RL method

Copilot

🔵 Needs a closer look

The new IAKERB option and authentication dependency upgrades warrant human confirmation of end-to-end authentication behavior.

No open issues 🎉

_{Review effort: Balanced}

Rostyslav-Romanets · 2026-10-27T15:05:30Z

Benoît Cortier (@CBenoit) Marc-André Moreau (@mamoreau-devolutions) Hi, this PR is ready to be merged now. Could you please take a look?

Benoît Cortier (CBenoit) · 2026-10-27T15:49:16Z

Thank you Rostyslav-Romanets

github-actions

PR #1987 migrates KerberosConfig to a new crate-owned KdcResolution enum (IAKerb vs KdcUrl) mirroring the sspi 0.23.1 API, bumps sspi/picky/picky-krb/winscard in the lockfile and pinned picky floors, always clears kdcproxyname in with_kerberos_config, and adds one regression test. The migration is mechanically correct and protocol-neutral. Five published findings, all low severity: the new enum duplicates an sspi type that is already publicly re-exported; the new_with_iakerb constructor has no callers; both changed Kerberos tests lack independent oracle provenance (the new test's invariant is specified only by the implementation's own rustdoc); the reworked KdcUrl(Some) upsert branch of with_kerberos_config is unpinned by tests; and the sspi manifest floors remain at 0.23 even though the code now requires the 0.23.1 IAKerb API, so minimal-version or 0.23.0-pinned resolutions fail to compile.

[dependency] sspi manifest floors stay at 0.23 while the code now requires the 0.23.1 API — low 🟡 — crates/ironrdp-connector/Cargo.toml
The lockfile widens sspi 0.23.0 -> 0.23.1 via the unchanged "0.23" requirements (connector Cargo.toml:29, also ironrdp and ironrdp-mstsgu), but the migrated code hard-depends on the 0.23.1 IAKerb API: credssp.rs:23 constructs sspi::KdcResolution::IAKerb and mstsgu http_auth.rs:302 calls sspi::KerberosConfig::new_with_kdc_url, which per the PR discussion only exists after the sspi-rs IAKerb release. Consumers resolving without this lockfile, or pinning/minimal-versions on 0.23.0, will fail to compile; raise the sspi floors to at least 0.23.1.

Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.

github-actions · 2026-10-27T16:26:43Z

+/// Strategy for resolving the KDC to use for Kerberos authentication.
+#[derive(Debug, Clone)]
+pub enum KdcResolution {
+    /// Use IAKerb extension to proxy KDC communication through the server to the LocalKDC.
+    IAKerb,
+    /// External KDC URL.
+    KdcUrl(Option<url::Url>),
+}
+
+impl From<KdcResolution> for sspi::KdcResolution {
+    fn from(val: KdcResolution) -> Self {
+        match val {
+            KdcResolution::IAKerb => sspi::KdcResolution::IAKerb,
+            KdcResolution::KdcUrl(url) => sspi::KdcResolution::KdcUrl(url),
+        }
+    }
+}


[skeptical + code-compressor] Local KdcResolution enum and From impl duplicate the already-re-exported sspi::KdcResolution — low 🟡 — The enum, variants, and From impl are a 1:1 mirror of sspi::KdcResolution with no added behavior, and the connector already does `pub use sspi;` (lib.rs:25) with sspi marked `# public`, so sspi types are established public API here. Replacing lines 14-30 with `pub use sspi::KdcResolution;` preserves the public path ironrdp_connector::credssp::KdcResolution (used by ironrdp-client, ironrdp-web, and the testsuite) with no call-site edits and avoids keeping a mirror enum and exhaustive match in sync with upstream sspi.

github-actions · 2026-10-27T16:26:43Z

+    pub fn new_with_iakerb(hostname: String) -> Self {
+        Self {
+            kdc_resolution: KdcResolution::IAKerb,
+            hostname,
+        }
+    }


[skeptical + code-compressor] new_with_iakerb constructor has no callers in the workspace — low 🟡 — A workspace-wide search finds no call of new_with_iakerb; every in-repo producer (client kerberos_config_from_properties, web connect, mstsgu gateway auth) builds KdcResolution::KdcUrl(...), and the only IAKerb construction (the new test) uses a struct literal directly since all fields are public. The constructor is speculative public surface; it can be dropped until a consumer exists, or kept knowingly as ergonomic downstream API for the sspi-rs IAKerb integration announced in the PR body.

github-actions · 2026-10-27T16:26:43Z

+#[test]
+fn explicit_kerberos_config_clears_kdc_proxy_properties() {
+    let mut properties = ironrdp_propertyset::PropertySet::new();
+    properties.set_kdc_proxy_name("kdc.example.com");
+    properties.set_kdc_proxy_url("https://kdc.example.com/KdcProxy");
+
+    let config = complete_builder()
+        .with_property_set(&properties)
+        .expect("valid KDC proxy properties")
+        .with_kerberos_config(KerberosConfig {
+            kdc_resolution: KdcResolution::IAKerb,
+            hostname: "client".to_owned(),
+        })
+        .build()
+        .expect("valid configuration");
+
+    assert!(config.properties().kdc_proxy_name().is_none());
+    assert!(config.properties().kdc_proxy_url().is_none());
+}


[test] Changed Kerberos tests lack independent oracle provenance — low 🟡 — Both changed tests carry no `// oracle:` marker although sibling tests use the convention (line 1697). The new test's requirement — an explicit KerberosConfig clears kdcproxyname/kdcproxyurl so properties cannot select a different KDC — is stated only in with_kerberos_config's own rustdoc (config.rs:1592-1595), i.e. the code under test, and the modified test's https://{name}/KdcProxy normalization value likewise rests only on the implementation. Such self-derived oracles detect change rather than bugs; specify the invariant in an independent intent/contract source and mark both tests with it.

github-actions · 2026-10-27T16:26:43Z

+#[test]
+fn explicit_kerberos_config_clears_kdc_proxy_properties() {
+    let mut properties = ironrdp_propertyset::PropertySet::new();
+    properties.set_kdc_proxy_name("kdc.example.com");
+    properties.set_kdc_proxy_url("https://kdc.example.com/KdcProxy");
+
+    let config = complete_builder()
+        .with_property_set(&properties)
+        .expect("valid KDC proxy properties")
+        .with_kerberos_config(KerberosConfig {
+            kdc_resolution: KdcResolution::IAKerb,
+            hostname: "client".to_owned(),
+        })
+        .build()
+        .expect("valid configuration");
+
+    assert!(config.properties().kdc_proxy_name().is_none());
+    assert!(config.properties().kdc_proxy_url().is_none());
+}


[test] Reworked KdcUrl(Some) upsert branch of with_kerberos_config is unpinned by tests — low 🟡 — with_kerberos_config was reworked to branch on KdcResolution: KdcUrl(Some(url)) upserts kdcproxyurl while everything else clears both properties (config.rs:1598-1603). The new test covers only the IAKerb branch; no existing test pins that an explicit KdcUrl(Some) overrides pre-existing, differing kdcproxyname/kdcproxyurl properties, which is the invariant the updated doc comment introduces. Add a case (e.g. explicit_kerberos_config_kdc_url_overrides_properties) covering that path.

Benoît Cortier (CBenoit) · 2026-10-27T16:35:16Z

Rostyslav-Romanets feel free to open follow up PRs addressing the comments

This was referenced Sep 24, 2026

feat(kerberos): add support for IAKerb extension for Kerberos client Devolutions/sspi-rs#751

Merged

feat(picky-krb): add IAKerb proxy message encoding/decoding Devolutions/picky-rs#531

Merged

Rostyslav-Romanets marked this pull request as ready for review September 24, 2026 15:44

Copilot AI balanced review requested due to automatic review settings September 24, 2026 15:44

Rostyslav-Romanets temporarily deployed to llm-providers September 24, 2026 15:45 — with GitHub Actions Inactive

Copilot started reviewing on behalf of Rostyslav-Romanets September 24, 2026 15:45

View session

Copilot AI reviewed Sep 24, 2026

View reviewed changes

Comment thread Cargo.toml Resolved

Comment thread crates/ironrdp-connector/Cargo.toml Resolved

Comment thread crates/ironrdp-testsuite-extra/tests/client/config.rs Resolved

Rostyslav-Romanets temporarily deployed to llm-providers September 28, 2026 20:02 — with GitHub Actions Inactive

Rostyslav-Romanets requested a balanced review from Copilot September 28, 2026 20:02

Copilot started reviewing on behalf of Rostyslav-Romanets September 28, 2026 20:02

View session

Copilot AI reviewed Sep 28, 2026

Rostyslav-Romanets requested a balanced review from Copilot September 29, 2026 13:58

Copilot started reviewing on behalf of Rostyslav-Romanets September 29, 2026 13:58

View session

Copilot AI reviewed Sep 29, 2026

View reviewed changes

Rostyslav-Romanets and others added 2 commits October 27, 2026 15:10

feat(credssp): use the KdcResolution enum instead of KDC url

3d74d47

refactor: address Copilot's code review suggestions

e359fe7

Rostyslav-Romanets force-pushed the add-iakerb-support branch from cfff55c to e359fe7 Compare October 27, 2026 14:34

test(kerberos): add test to ensure KDC proxy properties are cleared

cb7f7e9

Rostyslav-Romanets requested a balanced review from Copilot October 27, 2026 14:47

Rostyslav-Romanets temporarily deployed to llm-providers October 27, 2026 14:47 — with GitHub Actions Inactive

Copilot started reviewing on behalf of Rostyslav-Romanets October 27, 2026 14:47

View session

Copilot AI reviewed Oct 27, 2026

View reviewed changes

github-actions Bot mentioned this pull request Oct 27, 2026

feat: support Remote Credential Guard over RDPEAR #2256

Merged

fix(http_auth): update KerberosConfig initialization to use new KDC U…

e0b4469

…RL method

Rostyslav-Romanets temporarily deployed to llm-providers October 27, 2026 15:00 — with GitHub Actions Inactive

Rostyslav-Romanets requested a balanced review from Copilot October 27, 2026 15:00

Copilot started reviewing on behalf of Rostyslav-Romanets October 27, 2026 15:01

View session

github-actions Bot added the scope/cross-cutting Spans multiple architectural boundaries label Oct 27, 2026

Copilot AI reviewed Oct 27, 2026

View reviewed changes

github-actions Bot reviewed Oct 27, 2026

View reviewed changes

github-actions Bot added ai-reviewed/1 One automated review completed needs-author-action The pull request author is the current next actor labels Oct 27, 2026

Benoît Cortier (CBenoit) approved these changes Oct 27, 2026

View reviewed changes

Benoît Cortier (CBenoit) merged commit d233486 into Devolutions:master Oct 27, 2026
43 of 45 checks passed

github-actions Bot removed the needs-author-action The pull request author is the current next actor label Oct 27, 2026

devolutionsbot mentioned this pull request Nov 4, 2026

chore(release): prepare for publishing #2369

Open

Conversation

Rostyslav-Romanets commented Sep 24, 2026

Related PRs

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) commented Sep 29, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Rostyslav-Romanets commented Oct 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Rostyslav-Romanets commented Oct 27, 2026

Uh oh!

Benoît Cortier (CBenoit) commented Oct 27, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Benoît Cortier (CBenoit) commented Oct 27, 2026

Uh oh!

This branch was previously deployed

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

4 participants