Repository navigation
refactor: change nonce parameter type from byte slice to u32 - #762
Conversation
Keep the nonce as a u32 from generation to the encoding boundary instead of round-tripping it through big-endian bytes at every call site. nonce_to_asn1 now takes the u32 directly, and the PKINIT authenticator_nonce is a u32 as well. BREAKING CHANGE: GenerateAsReqOptions::nonce is now a u32 instead of &[u8]. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A55uC5v7JmckNf5r2X4t4g
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The type migration is consistently applied and existing DER edge-case coverage confirms preserved encoding behavior.
Review effort: Balanced
Findings: None
What changed in this PR
Refactors Kerberos and PKU2U nonce handling to use u32 while preserving DER encoding behavior.
Changes:
- Centralizes big-endian conversion in
nonce_to_asn1. - Updates nonce option fields and call sites to use
u32. - Migrates nonce-related tests to integer literals.
| File | Description |
|---|---|
src/pku2u/mod.rs |
Passes generated nonces directly. |
src/pku2u/extractors.rs |
Updates test nonce inputs. |
src/pk_init.rs |
Changes authenticator nonce to u32. |
src/kerberos/server/as_exchange.rs |
Removes call-site byte conversion. |
src/kerberos/client/mod.rs |
Passes generated nonces directly. |
src/kerberos/client/generators.rs |
Refactors nonce API, encoding, and tests. |
src/kerberos/client/change_password.rs |
Uses the generated u32 nonce directly. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
With the nonce carried as a u32, the DER conversion now happens inline where each nonce is encoded, and the encoding rationale lives on the GenerateAsReqOptions::nonce field. The helper's test cases move to the AS-REQ body test so they exercise the real encoding path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A55uC5v7JmckNf5r2X4t4g
Benoît Cortier (CBenoit)
changed the title
Jordan Borean (jborean93)
left a comment
Jordan Borean (jborean93)
left a comment
There was a problem hiding this comment.
Just a comment, feel free to ignore it but thought I would just ask.
Do you also need to bump the major version now that there is a breaking change or is the versioning done separately?
I can also verify that this does't regress my original problem so the changes are good logic wise there.
Keep the nonce DER encoding and its rationale in one helper so every encoding site (AS-REQ, TGS-REQ, PKINIT) uses the same signed minimal encoding instead of each repeating the conversion. The nonce stays a u32 up to that boundary. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A55uC5v7JmckNf5r2X4t4g
|
Jordan Borean (@jborean93) Thank you for the review!
Rust crate is indeed done via a separate PR through release-plz, with breaking change detection, and the NuGet package is automatically published with today’s date upon running the workflow 🙂
I would appreciate that! If you’re okay with that, I’ll publish the next version once you confirm everything is good for you? |
Would be great to get a new release but if it was me I would probably wait for #758 to be in before the next release. This has some unexpected consequences where credentials set by other threads could be shared unexpectedly. Entirely up to you though, this is your project and I've worked around the problem #758 fixes by just using unique usernames in my test. |
Agreed; in fact I already merged that other PR, so it will be part of the next release. I’ll just wait for an approval on this one too, and then I think we’re good to go. |
Pavlo Myroniuk (TheBestTvarynka)
left a comment
Pavlo Myroniuk (TheBestTvarynka)
left a comment
There was a problem hiding this comment.
LGTM
Summary
Follow-up to #759, applying this review suggestion: keep the KDC-REQ nonce as a
u32all the way to the encoding boundary instead of round-tripping it through big-endian bytes at every call site.Key Changes
GenerateAsReqOptions::nonceis nowu32instead of&'a [u8](breaking change for external users of this public struct).GenerateAsPaDataOptions::authenticator_nonce(PKINIT) is nowu32instead of[u8; 4].nonce_to_asn1now takes au32and remains the single place that encodes a nonce (signed minimal DERInt32), with the rationale documented there. It is used bygenerate_as_req_kdc_body,generate_tgs_reqand the PKINITPkAuthenticator..to_be_bytes()conversions at the call sites in the Kerberos client, change-password, server AS exchange and PKU2U.u32literals (e.g.0x0009_a792) instead of byte arrays.The encoded bytes are identical to before; this is a type/API refactor only.
Testing
cargo test --features network_client,dns_resolver,scard,__test-datapasses.cargo fmtand clippy (lib/bins/examples,-D warnings) are clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01A55uC5v7JmckNf5r2X4t4g