Repository navigation
fix(kerberos): enforce the sequence number of MIC token only for acce… - #772
Merged
Marc-André Moreau (mamoreau-devolutions) merged 4 commits intoOct 19, 2026
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The shared validator unintentionally disables PKU2U client sequence checks and lacks regression coverage.
3 open findings
What changed in this PR
Restores Windows Kerberos interoperability by skipping initiator-side MIC sequence validation.
Changes:
- Limits sequence-number enforcement to acceptors.
- Documents the interoperability rationale.
| File | Description |
|---|---|
src/kerberos/utils.rs |
Changes MIC sequence validation behavior. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Author
|
The 2 open findings have been resolved, but Copilot hasn't closed them. |
Pavlo Myroniuk (TheBestTvarynka)
self-requested a review
October 13, 2026 19:16
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Marc-André Moreau (mamoreau-devolutions)
enabled auto-merge (squash)
October 19, 2026 18:39
Marc-André Moreau (mamoreau-devolutions)
approved these changes
Oct 19, 2026
Marc-André Moreau (mamoreau-devolutions)
merged commit Oct 19, 2026
08d8622
into
Devolutions:master
68 checks passed
Marc-André Moreau (mamoreau-devolutions)
added a commit
that referenced
this pull request
Oct 23, 2026
## Summary Follow up on [#768](#768), based on freshly fetched `origin/master` at `cf91195338f62a8b01ca5024e06f97c150e03225`, including that merged PR. - Carry the negotiated preauthentication salt out of the AS exchange for password changes and password-backed U2U acceptor TGT acquisition, using the same reply-salt selection helper as the direct/IAKerb client. - Distinguish an absent PA-ETYPE-INFO2 hint, an omitted salt field, and an explicitly empty salt. Retain the existing public extractor signatures. - Share authenticated AS-REP decryption between password/keytab and the actual PKINIT-facing PKU2U extractors, accepting application tags 25 and 26 without weakening decryption or nonce handling. - Add 27 deterministic regressions, reusing the KDC mocks and direct/IAKerb flows. Fix the reported test-target Clippy findings and the RFC section-title capitalization. ## Salt semantics RFC 4120 section 4, “Encryption and Checksum Specifications”, defines the default salt as the realm followed by principal components without separators; section 5.2.7.5, “PA-ETYPE-INFO2”, defines the optional salt field. | Reply information | Salt used | | --- | --- | | PA-ETYPE-INFO2 absent | Salt used for preauthentication | | Entry present, salt field absent | Default from the reply client realm and principal components | | Salt explicitly empty | Empty salt, even when preauthentication used a different salt | | Salt explicitly nonempty | Reply-provided salt, taking precedence over preauthentication | The preauthentication path also distinguishes an absent hint from a present entry requesting the principal's default salt. Existing explicit-salt/tag-25 behavior and the IAKerb state machine are retained. ## Requirement coverage | Requirement | Deterministic evidence | | --- | --- | | Password-change salt fallback | `kerberos_change_password_uses_negotiated_salt_without_as_rep_hint` invokes the actual password-change consumer and decrypts/asserts the outgoing ticket, authenticator, and exact new-password bytes | | Password-backed U2U acceptor TGT acquisition | `kerberos_u2u_server_acquires_tgt_with_password_without_as_rep_hint` starts without a preinstalled ticket-decryption key, acquires the acceptor TGT, and completes authentication plus encrypted application traffic | | Combined missing reply hint and tag 26 | `kerberos_auth_without_as_rep_etype_info2_with_tgs_rep_enc_part_in_as_rep` | | The same combination through IAKerb | `iakerb_auth_without_as_rep_etype_info2_with_tgs_rep_enc_part_in_as_rep`, including cookies/transcript and encrypted application traffic | | Explicit-empty and custom reply salts override a different preauth salt | `kerberos_auth_with_explicit_empty_as_rep_salt`, `kerberos_auth_with_custom_as_rep_salt`, `extract_encryption_params_from_as_rep_with_salt_preserves_explicit_reply_salt` | | Omitted reply salt field uses the reply-principal default | `kerberos_auth_with_missing_as_rep_salt_field`, `extract_encryption_params_from_as_rep_with_salt_defaults_missing_reply_salt_field` assert case-preserving, separator-free realm/multicomponent-principal concatenation | | Missing versus empty preauth salt | `extract_preauth_salt_distinguishes_absent_default_empty_and_custom`, `kerberos_auth_with_missing_preauth_salt_field`, `kerberos_auth_with_explicit_empty_preauth_salt` | | Password/keytab and shared decoder tag compatibility | `kerberos_password_extractor_returns_expected_session_key_for_both_application_tags`, `kerberos_derived_key_extractor_returns_expected_session_key_for_both_application_tags`, `decrypt_as_rep_preserves_fields_for_both_application_tags` | | Actual PKINIT-facing key-only and key+nonce decoding | `pku2u_public_key_extractor_returns_expected_session_key_for_both_application_tags`, `pku2u_key_and_nonce_extractor_preserves_nonce_boundaries` | | Invalid tags, wrong keys, tampering, and nonce overflow still fail | `kerberos_session_key_extractors_reject_unsupported_application_tag`, `pku2u_session_key_extractors_reject_unsupported_application_tag`, `kerberos_session_key_extractors_reject_wrong_key_and_tampered_ciphertext`, `pku2u_session_key_extractors_reject_wrong_key_and_tampered_ciphertext`, `pku2u_nonce_extractor_rejects_nonce_above_u32_max` | | Public compatibility wrappers and malformed metadata | `extract_salt_from_krb_error_preserves_legacy_optional_string_contract`, `extract_encryption_params_from_as_rep_preserves_one_argument_api`, `salt_extractors_reject_invalid_challenges`, `reply_encryption_params_reject_malformed_or_missing_values` | | Cleanup | RFC title is `KRB_KDC_REP definition`; checked IAKerb `usize::try_from` conversion and structured trace logging in the common test helper; strict Clippy passes | ## Validation Local Windows validation with VsDevShell initialized in each build process; `AWS_LC_SYS_NO_ASM=true`, `CMAKE_GENERATOR=Ninja`, and `SSPI_RS_IS_RUNNING_TESTS=true` for tests: - `cargo test --locked -q -p sspi --lib --features network_client,dns_resolver,scard,tsssp,__test-data`: **307 passed, 0 failed**. - `cargo test --locked -q -p sspi --test sspi --features network_client,dns_resolver,scard,tsssp,__test-data client_server::kerberos:: -- --test-threads=1`: **22 passed, 0 failed**. - Scoped extractor runs: **16 Kerberos tests** and **7 PKU2U tests** passed. The affected password-change and U2U tests were also rerun after the final Clippy cleanup. - `cargo clippy --locked -q -p sspi --lib --test sspi --features network_client,dns_resolver,scard,tsssp,__test-data -- -D warnings -D clippy::print_stdout`: passed. - `cargo check --locked -q -p sspi --no-default-features`: passed. - `cargo fmt --all -- --check` and `git diff --check`: passed; style review passed. - Standalone `tools/wasm-testcompile` fresh dependency resolution selected **picky-krb 0.13.0**; both `cargo check` and `cargo build` with `--locked --target wasm32-unknown-unknown` passed. The full `check.ps1` import scan was not run (`wasm2wat` is unavailable). Formatting emits the existing stable-rustfmt warnings for nightly-only options. Cargo emits nonfatal incremental-cache access-denied notes; WASM compilation also emits the existing `encode_digest` dead-code warning. ## Investigated without changes - **WASM dependency failure:** current master already requires `picky-krb = "0.13"` (raised in #772), which excludes the older 0.12.4 release lacking `InvalidMechanismOid`. Fresh resolution and an actual standalone wasm32 build succeeded. No additional pin, lockfile change, or external workspace dependency is warranted. - **Dependency-fetch hang:** the reported historical macOS winscard timeout remains an infrastructure symptom, not a reproduced source failure. The historical failed-run jobs API returned HTTP 502, so its detailed logs could not be revalidated here. The later run [39320794389](https://github.com/Devolutions/sspi-rs/actions/runs/39320794389) has successful WASM and Windows/macOS/Linux winscard jobs; the RustCrypto/KDFs dependency is already revision-locked. No speculative timeout/retry infrastructure or dependency update was added. No live Windows/MIT/Heimdal KDC validation was performed. Explicit-empty reply salt is a theoretical deterministic case, not a claimed real-world reproduction. Password-change coverage verifies the encrypted outgoing request, not a completed password update on a KDC. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Samer Aburabie (SamerX)
pushed a commit
to SamerX/Kubexer-IronRDP
that referenced
this pull request
Oct 27, 2026
Kerberos (CredSSP) sign-in from the web client fails against Windows with: CredSSP: InvalidToken: invalid MIC token sequence number: expected <n> but got 0 sspi 0.23.0 validates the sequence number of the SPNEGO mechListMIC on the initiator side, but the Windows acceptor signs it with sequence number 0 regardless of the AP-REP sequence number. sspi 0.21.0 (used by the upstream npm 0.7.0 package) did not have this check. Upstream issue: Devolutions/sspi-rs#748 Upstream fix: Devolutions/sspi-rs#772 (merge commit 08d86227, 2026-10-19), which enforces the sequence number only on the acceptor side. The fix is not released on crates.io yet (latest is 0.23.0), so patch sspi to that git rev via [patch.crates-io]. The rev is 0.23.0 plus five commits: picky-* dependency bump (picky-krb 0.12 -> 0.13, locked to 0.13.0 as upstream does; 0.13.1 does not build with this sspi), a `.\` local-qualifier username parsing fix (Devolutions#767), FFI tests, agent/style docs, and the MIC fix itself. winscard comes from the same git rev (sspi workspace member, docs-only diff). Drop the patch once an sspi release newer than 0.23.0 is on crates.io. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes #748 by enforcing sequence number verification only on the acceptor side. The initiator verifies the MIC checksum without enforcing the sequence number.