Skip to content

feat(xmldsig): parse X509Certificate with x509-parser#53

Open
polaz wants to merge 4 commits intomainfrom
feat/p2-003-x509-parser
Open

feat(xmldsig): parse X509Certificate with x509-parser#53
polaz wants to merge 4 commits intomainfrom
feat/p2-003-x509-parser

Conversation

@polaz
Copy link
Copy Markdown
Member

@polaz polaz commented Apr 9, 2026

Summary

  • parse X509Certificate DER bytes with x509-parser during KeyInfo/X509Data parsing
  • extract certificate metadata (subject DN, issuer DN, SKI) and SPKI key info (RSA/EC, plus unsupported algorithm OID marker)
  • keep unsupported key algorithms as parsed metadata instead of failing the whole KeyInfo parse
  • update and extend parse tests to use real fixture certificate DER and add invalid-DER regression

Verification

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

Copilot AI review requested due to automatic review settings April 9, 2026 13:16
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 9, 2026

Warning

Rate limit exceeded

@polaz has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 40 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 40 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4bfd2a51-8ba7-410e-a45d-d6527521f017

📥 Commits

Reviewing files that changed from the base of the PR and between 89b6536 and bcac24f.

📒 Files selected for processing (1)
  • src/xmldsig/parse.rs
📝 Walkthrough

Walkthrough

Adds strict DER parsing of <X509Certificate> elements: extracts subject and issuer DNs, optional SubjectKeyIdentifier, and parses the certificate public key into RSA, EC, or unsupported variants; stores results in a new parsed_certificates field on X509DataInfo.

Changes

Cohort / File(s) Summary
X.509 Certificate Parsing
src/xmldsig/parse.rs
Added parsed_certificates: Vec<ParsedX509Certificate> to X509DataInfo. Introduced ParsedX509Certificate and X509PublicKeyInfo (RSA/EC/Unsupported). Implemented parse_x509_certificate() to DER-parse certificates, extract subject/issuer DNs, optional SubjectKeyIdentifier extension, and parse public keys (RSA modulus/exponent with leading-zero trimming, EC curve OID and point bytes). Updated parse_x509_data_dispatch() to populate parsed certificates and added unit tests replacing dummy DER with a real RSA certificate and asserting invalid DER is rejected.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat(xmldsig): parse X509Certificate with x509-parser' directly and accurately describes the main change: adding X509Certificate parsing functionality using the x509-parser library during KeyInfo/X509Data parsing.
Description check ✅ Passed The description is well-related to the changeset, detailing certificate metadata extraction, key algorithm detection, and test updates that align with the code changes in the raw summary.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/p2-003-x509-parser

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances XMLDSig <KeyInfo>/<X509Data> parsing by decoding <X509Certificate> DER bytes with x509-parser, extracting certificate metadata (DNs, SKI) and public key details while preserving metadata for unsupported key algorithms instead of failing the whole parse.

Changes:

  • Add ParsedX509Certificate and X509PublicKeyInfo types and store parsed certificate metadata in X509DataInfo.
  • Parse <X509Certificate> DER using x509-parser and extract subject/issuer DN, SKI, and SPKI key info (RSA/EC/Unsupported).
  • Update tests to use a real PEM fixture certificate and add an invalid-DER regression test.

polaz added 2 commits April 9, 2026 16:40
- parse <X509Certificate> DER with x509-parser and extract subject/issuer DN, SKI, and SPKI key metadata

- keep unsupported SPKI algorithms as parsed metadata instead of hard parse failure

- extend KeyInfo/X509Data tests to use real fixture cert DER and cover invalid DER rejection
- move add_x509_data_usage call before parse_x509_certificate in X509Certificate branch

- reject oversized X509Data aggregate before expensive DER/X.509 parsing
@polaz polaz force-pushed the feat/p2-003-x509-parser branch from a591d4c to 89b6536 Compare April 9, 2026 13:42
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/xmldsig/parse.rs`:
- Around line 186-188: Add a clear doc comment documenting the parallel-array
invariant between the certificates and parsed_certificates fields: state that
both Vecs have equal length and that entries at the same index correspond to the
same certificate and its parsed representation. Put this comment near the struct
definition (above the struct or above both fields) referencing the field names
certificates and parsed_certificates and the ParsedX509Certificate type so
future maintainers know they must be updated together.
🪄 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

Run ID: a3ab81db-0d88-42cc-bf29-948df54746ae

📥 Commits

Reviewing files that changed from the base of the PR and between a591d4c and 89b6536.

📒 Files selected for processing (1)
  • src/xmldsig/parse.rs

- document 1:1 index correspondence between certificates and parsed_certificates
Copilot AI review requested due to automatic review settings April 9, 2026 14:04
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

- treat EC SPKI params that are present but non-OID as unsupported key algorithm

- keep missing EC params as InvalidStructure

- add regression test for unsupported SPKI algorithm marker using merlin x509 fixture
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants