fixture: add x.509 trust/time/path negatives#314
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (16)
📒 Files selected for processing (27)
Summary by CodeRabbitRelease Notes
WalkthroughThis pull request expands X.509 chain negative fixtures by adding four new test scenarios (not-yet-valid leaf/intermediate, intermediate lacking CA status, intermediate with incorrect key usage) and refactors timing configuration from numeric day-offsets to an enum-based Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ChainNegative
participant ChainSpec
participant X509Chain
participant Certificate
Caller->>X509Chain: not_yet_valid_leaf()
X509Chain->>ChainNegative: apply(spec)
ChainNegative->>ChainSpec: set leaf_not_before = DaysFromNow(730)
ChainSpec-->>ChainNegative: modified spec
X509Chain->>Certificate: generate with future not_before
Certificate-->>X509Chain: certificate (not yet valid)
X509Chain-->>Caller: X509Chain with not-yet-valid leaf
Caller->>X509Chain: intermediate_wrong_key_usage()
X509Chain->>ChainNegative: apply(spec)
ChainNegative->>ChainSpec: set intermediate_is_ca = Some(true)
ChainNegative->>ChainSpec: set intermediate_key_usage = KeyUsage{digital_signature: true, key_cert_sign: false, ...}
ChainSpec-->>ChainNegative: modified spec
X509Chain->>Certificate: generate intermediate with restricted usages
Certificate-->>X509Chain: intermediate (wrong key usage)
X509Chain-->>Caller: X509Chain with mismatched CA/usage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request expands the X.509 negative fixture capabilities by adding support for not-yet-valid certificates and intermediate CA/key-usage violations. It introduces the NotBeforeOffset enum to distinguish between past and future offsets and updates the ChainSpec to include intermediate certificate overrides. Consequently, the deterministic stable_bytes representation has been bumped to version 3 to incorporate these rich offsets and new fields. Extensive test coverage and documentation have been updated to reflect these changes. I have no feedback to provide.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42fcc4da5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| spec.leaf_not_before = Some(NotBeforeOffset::DaysAgo(730)); | ||
| } | ||
| ChainNegative::NotYetValidLeaf => { | ||
| spec.leaf_not_before = Some(NotBeforeOffset::DaysFromNow(730)); |
There was a problem hiding this comment.
Derive not-yet-valid offsets from current date
ChainNegative::NotYetValidLeaf sets DaysFromNow(730) relative to the deterministic base time, but deterministic_base_time_from_parts is anchored to a fixed 2025 window (BASE_TIME_EPOCH_UNIX + BASE_TIME_WINDOW_DAYS), so these certs land around 2027 and stop being future-dated as wall-clock time advances (partially during 2027, fully by 2028-01-01); at that point the advertised “not yet valid” chain negative can become valid in production tests. The same time-bomb applies to NotYetValidIntermediate a few lines below.
Useful? React with 👍 / 👎.
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
1 participant
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.
Summary
ChainSpecto useNotBeforeOffsetfor leaf/intermediatenot_beforeand add minimal intermediate CA/key-usage overridesIntermediateNotCa,IntermediateWrongKeyUsage)X509Chainhelpers and extend tests, snapshots, README/docs, and the X.509 example to cover the expanded chain-negative surfaceCloses #279.
Verification
cargo test -p uselesskey-core-x509-speccargo test -p uselesskey-core-x509-chain-negativecargo test -p uselesskey-x509 --test x509_unitcargo test -p uselesskey-x509 --test x509_comprehensivecargo check -p uselesskey --example x509_certificates --features x509cargo xtask gate --check