feat(xmldsig): build x509 certificate chains

[polaz]

Summary

• Implement P2-004 certificate chain building for parsed X509Data certificates.
• Add certificate serial metadata and ordered chain indexes starting at the selected signing certificate.
• Select signing certificate via X509SubjectName, X509IssuerSerial, or X509SKI hints, with deterministic ambiguity errors.
• Checked dependency updates in this PR scope: cargo update produced no tracked diff because Cargo.lock is untracked, but the workspace was verified against the resolved updated graph.

Testing

• cargo fmt -- --check
• cargo check --workspace
• cargo clippy --workspace --all-targets -- -D warnings
• cargo nextest run --workspace
• cargo test --doc --workspace
• cargo build --workspace

Closes #60

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e663f190-ed34-4047-82d3-6b6847314519

📥 Commits

Reviewing files that changed from the base of the PR and between 60abb57 and 0a64137.

📒 Files selected for processing (1)

• src/xmldsig/parse.rs

---

📝 Walkthrough

Summary by CodeRabbit

• New Features

  • XML signature certificate data now includes an ordered certificate chain to better reflect certificate relationships.

• Bug Fixes

  • Improved certificate serial number extraction and canonicalized hex formatting for more reliable matching and display.
  • Enhanced validation for certificate lookups and chain building, including safer handling of ambiguous, missing, or malformed identifiers.

• Tests

  • Expanded test coverage for chain ordering, serial formatting/canonicalization, and edge-case validation behavior.

Walkthrough

The change adds ordered X.509 certificate chain derivation to XMLDSig X509Data parsing. It adds chain and serial fields to parsed certificate data, computes signing-certificate selection from lookup hints, and expands tests and fixtures for ordering and canonicalization behavior.

Changes

X.509 Certificate Chain Building

Layer / File(s)	Summary
Data shape and serial fields src/xmldsig/parse.rs	Adds MAX_X509_CHAIN_DEPTH, certificate_chain: Vec<usize> on X509DataInfo, and serial_number: Vec<u8> plus canonical serial_number_hex on ParsedX509Certificate.
Chain building and signing selection src/xmldsig/parse.rs	Derives certificate_chain after parsing X509Data, selects the signing certificate from subject name, issuer serial, or SKI hints, and orders certificates by issuer/subject links with depth and ambiguity checks.
Tests and fixture updates src/xmldsig/parse.rs	Updates certificate fixture loading and X509Data assertions, and adds coverage for chain ordering, decimal issuer serial conversion, lookup failures, chain-depth rejection, and serial hex canonicalization.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• structured-world/xml-sec#51: Modifies the same src/xmldsig/parse.rs X.509 parsing path and X509DataInfo shape that this PR extends.
• structured-world/xml-sec#53: Expands ParsedX509Certificate metadata in the same certificate parsing flow used by this PR.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 54.17% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: building X.509 certificate chains.
Description check	✅ Passed	The description is directly related to the certificate-chain changes and validation work.
Linked Issues check	✅ Passed	The changes implement ordered X.509 chain building, hint-based signing-certificate selection, and deterministic ambiguity handling for #60.
Out of Scope Changes check	✅ Passed	No unrelated code changes are evident beyond the X.509 chain-building feature and its test updates.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/#60-x509-chain-building

---

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

This PR adds X509 certificate chain construction for XMLDSig parsing. The main changes are:

• Builds ordered certificate chains from embedded X509Data certificates.
• Selects the signing certificate using subject name, issuer serial, or SKI hints.
• Adds certificate serial metadata with canonical hex formatting.
• Adds tests for chain ordering, lookup ambiguity, serial handling, and depth limits.

Confidence Score: 5/5

The certificate chain construction changes appear merge-safe based on the described coverage and absence of identified issues.

The implementation is focused in one parser file and is covered by formatting, workspace checks, linting, unit/doc tests, and build validation.

T-Rex Logs

What T-Rex did

• Compared the x509 chain build workflows across the before and after commits, confirming a transition from a base failure about a missing certificate_chain field to a successful output showing certificate_chain=[2, 1, 0].
• Compared the x509 hint selection runs across the before and after commits, confirming the initial compile failure due to missing certificate_chain and then a passing test with explicit chain selection.
• Captured and archived the related logs as artifacts for both proof sets to support review.

_{Ran code and verified through T-Rex}

_{Reviews (6): Last reviewed commit: "test(xmldsig): cover conflicting x509 hi..." | Re-trigger Greptile}

[polaz]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 4fb8503 and 199a7c1.

📒 Files selected for processing (1)

• src/xmldsig/parse.rs

[polaz]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

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 win

Do not let one matching X.509 hint mask another unmatched hint.

filter_map drops issuer-serials that fail decimal normalization, and the candidate scan accepts the cert if any subject/issuer-serial/SKI hint matches. That means a matching X509SubjectName can still allow a malformed or non-matching X509IssuerSerial/X509SKI to be ignored, despite the PR objective to reject unmatched lookup hints deterministically. Track whether each provided lookup hint matched an embedded certificate, and return InvalidStructure if 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

📥 Commits

Reviewing files that changed from the base of the PR and between 199a7c1 and 5ca0875.

📒 Files selected for processing (1)

• src/xmldsig/parse.rs

[polaz]

@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.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@polaz Thanks for the detailed update! The fix in 60abb57 looks well-reasoned:

• Independent per-hint validation is the right approach — each hint type (SubjectName, IssuerSerial, SKI) now fails independently on mismatch/malformation rather than any one masking another.
• Returning InvalidStructure for malformed or unmatched hints gives callers a clear, deterministic signal rather than silently falling through.
• The ambiguity case (hints resolving to different certs) being preserved as an error is correct — that's a genuinely contradictory input.
• The regression tests for unmatched SKI masked by a matching subject and malformed issuer serial are exactly the edge cases that could otherwise be silently swallowed. Good call on adding those explicitly.

[polaz]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[polaz]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[polaz]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

🤖 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 1825-1866: Add a regression test in parse_key_info around the
existing X509Data/parse_key_info cases to cover ambiguity when valid lookup
hints point to different embedded certificates; specifically, make one case
where X509SubjectName and X509SKI (or X509IssuerSerial) are both valid but
resolve to different certs, and assert parse_key_info returns
ParseError::InvalidStructure with the lookup-identifiers ambiguity message. Use
the existing parse_key_info, fixture_cert_base64, and X509Data test pattern so
the contract around conflicting hints is locked in.

🪄 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: 8d070afc-4c14-4bb0-bf94-8ddc6f575748

📥 Commits

Reviewing files that changed from the base of the PR and between 5ca0875 and 60abb57.

📒 Files selected for processing (1)

• src/xmldsig/parse.rs