fix(kerberos): remove unnecessary sequence number incrementation - #717
Merged
Benoît Cortier (CBenoit) merged 2 commits intoJul 31, 2026
Merged
Conversation
Pavlo Myroniuk (TheBestTvarynka)
requested a review
from Benoît Cortier (CBenoit)
July 29, 2026 15:19
Benoît Cortier (CBenoit)
approved these changes
Jul 31, 2026
Benoît Cortier (CBenoit)
left a comment
Benoît Cortier (CBenoit)
left a comment
Member
There was a problem hiding this comment.
LGTM! Thank you!
Benoît Cortier (CBenoit)
deleted the
fix/kerberos-sequence-number-incrementation
branch
July 31, 2026 09:39
There was a problem hiding this comment.
Pull request overview
Fixes Kerberos authentication failures during SPNEGO MIC exchange by correctly tracking peer sequence numbers.
Changes:
- Removes the unnecessary client sequence increment.
- Validates MIC sequence numbers against peer state.
- Refactors AP-REP decryption and field extraction.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/kerberos/client/extractors.rs |
Refactors AP-REP extraction. |
src/kerberos/client/mod.rs |
Tracks the server sequence number. |
src/kerberos/mod.rs |
Stores and validates remote sequence numbers. |
src/kerberos/server/mod.rs |
Tracks the client sequence number. |
src/kerberos/tests.rs |
Updates Kerberos test fixtures. |
src/kerberos/utils.rs |
Adds MIC sequence validation. |
src/pku2u/mod.rs |
Adopts shared AP-REP decryption. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+252
to
+259
| let seq_number = u32::from_be_bytes(seq_number_bytes.try_into().map_err(|err| { | ||
| Error::new( | ||
| ErrorKind::InvalidToken, | ||
| format!("invalid ApRep sequence number: {:?}", err), | ||
| ) | ||
| })?); | ||
|
|
||
| Ok(seq_number) |
|
|
||
| let ap_rep_enc_part: EncApRepPart = picky_asn1_der::from_bytes(&res)?; | ||
|
|
||
| pub fn extract_sub_session_key_from_ap_rep(ap_rep_enc_part: &EncApRepPart) -> Result<Secret<Vec<u8>>> { |
|
|
||
| /// Extracts a sequence number from the [EncApRepPart]. | ||
| #[instrument(level = "trace", ret)] | ||
| pub fn extract_seq_number_from_ap_rep(ap_rep_enc_part: &EncApRepPart) -> Result<u32> { |
| } | ||
|
|
||
| /// Extracts the sequence number from the [ApRep]. | ||
| /// Decrypt and decodes the encrypted part of the encoded [ApRep] message. |
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.
Hi,
This PR fixes the authentication issue inside the Kerberos implementation.
The problem was in an unnecessary sequence number incrementation:
sspi-rs/src/kerberos/client/mod.rs
Line 515 in 847304f
Why did we add it?
It was necessary back then. But after the big Negotiate refactoring and NTLM fallback improvement, this sequence number incrementation became unneeded.
Why did the tests not catch it?
Because our Kerberos implementation did not check the MIC token sequence number properly. I improved the implementation, and now it will fail if we mess it up again.
Why did we not notice it during dev testing?
Because it happens only when the MIC token exchange is present. After the Negotiate module refactoring and NTLM fallback improvements, we skip MIC token exchange in many cases. For example, regular password-based Kerberos auth using FreeRDP always worked.
Related issues
Devolutions/IronRDP#1469
closes #668