feat(xmldsig): build x509 certificate chains#61
Conversation
- Order embedded X509Data certificates from signing cert to issuer chain - Select signing cert by SubjectName, IssuerSerial, or SKI hints when present - Preserve serial metadata for later chain verification - Add regression coverage for unordered chains and ambiguity Dependency update check: cargo update produced no tracked diff because Cargo.lock is untracked; workspace verified against the resolved updated graph. Closes #60
|
Warning Review limit reached
More reviews will be available in 38 minutes and 26 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds ordered X.509 certificate chain construction to ChangesX.509 Certificate Chain Building
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/xmldsig/parse.rs`:
- Around line 681-684: The issuer-serial matching in the certificate scan is
repeatedly calling x509_serial_decimal_to_hex inside the any() predicate, which
is unnecessary work. Refactor the logic around the issuer_serials check in
parse.rs so the decimal-to-hex normalization is computed once per issuer-serial
entry before comparing against cert.serial_number_hex, then reuse that
precomputed value in the closure. Keep the existing matching behavior in the
certificate × issuer-serial scan, but move the conversion out of the inner
comparison to avoid repeated conversions.
- Around line 696-705: The X509Data candidate selection in parse_certificates
should not fall through when lookup hints are present but produce no matches; in
the match on candidates.as_slice(), treat the empty case as an invalid lookup
instead of continuing to the leaf heuristic or default index. Update the logic
around the candidate resolution in parse.rs so subject/issuer-serial/SKI hints
either resolve to a single certificate or return a ParseError::InvalidStructure
when they match nothing or multiple certificates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ad198800-cb79-4b82-9c5c-867943115e59
📒 Files selected for processing (1)
src/xmldsig/parse.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/xmldsig/parse.rs (1)
675-710: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not let one matching X.509 hint mask another unmatched hint.
filter_mapdrops issuer-serials that fail decimal normalization, and the candidate scan accepts the cert if any subject/issuer-serial/SKI hint matches. That means a matchingX509SubjectNamecan still allow a malformed or non-matchingX509IssuerSerial/X509SKIto be ignored, despite the PR objective to reject unmatched lookup hints deterministically. Track whether each provided lookup hint matched an embedded certificate, and returnInvalidStructureif any provided hint is malformed or unmatched.Also applies to: 1757-1774
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/xmldsig/parse.rs` around lines 675 - 710, The X509Data lookup logic is currently treating subject names, issuer serials, and SKIs as one combined OR match, so a single successful hint can hide a malformed or unmatched one. Update the lookup flow around the candidate scan in parse.rs to track match status per hint type in the relevant X509Data parsing function(s), including the issuer_serial_hints normalization path, and require every provided lookup hint to either match an embedded certificate or be rejected. If any issuer-serial fails decimal normalization or any provided SubjectName, IssuerSerial, or SKI does not match, return InvalidStructure instead of allowing another hint to satisfy the lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/xmldsig/parse.rs`:
- Around line 675-710: The X509Data lookup logic is currently treating subject
names, issuer serials, and SKIs as one combined OR match, so a single successful
hint can hide a malformed or unmatched one. Update the lookup flow around the
candidate scan in parse.rs to track match status per hint type in the relevant
X509Data parsing function(s), including the issuer_serial_hints normalization
path, and require every provided lookup hint to either match an embedded
certificate or be rejected. If any issuer-serial fails decimal normalization or
any provided SubjectName, IssuerSerial, or SKI does not match, return
InvalidStructure instead of allowing another hint to satisfy the lookup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4a113b83-cdc0-4e98-b837-0d4187bba171
📒 Files selected for processing (1)
src/xmldsig/parse.rs
|
@coderabbitai Re: outside-diff X509 lookup hints - fixed in 60abb57. Each provided SubjectName, IssuerSerial, and SKI hint is now validated independently; malformed/unmatched hints return InvalidStructure, and hints that resolve to different certs remain ambiguous. Added regression coverage for malformed issuer serial and unmatched SKI masked by a matching subject. |
|
Tip For best results, initiate chat on the files or code changes.
|
1 participant
Summary
Testing
Closes #60