Skip to content

feat(xmldsig): resolve RSAKeyValue keys#68

Merged
polaz merged 3 commits into
mainfrom
feat/#67-rsa-key-value
Jun 29, 2026
Merged

feat(xmldsig): resolve RSAKeyValue keys#68
polaz merged 3 commits into
mainfrom
feat/#67-rsa-key-value

Conversation

@polaz

@polaz polaz commented Jun 29, 2026

Copy link
Copy Markdown
Member

Summary

  • Parse bounded XMLDSig RSAKeyValue modulus and exponent CryptoBinary parameters.
  • Convert embedded RSA parameters to validated SubjectPublicKeyInfo DER and resolve them through DefaultKeyResolver.
  • Verify a real RSA-2048 donor signature end-to-end without a preset key.

Security behavior

  • Require ordered Modulus and Exponent children in the XMLDSig namespace.
  • Reject missing, duplicated, nested, malformed, empty, and oversized parameters.
  • Apply the existing RSA policy to embedded keys, including the 2048-bit minimum and exponent validation.
  • Reject RSA key material for ECDSA signature methods with a typed algorithm mismatch.

Testing

  • cargo check --workspace
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo nextest run --workspace (473/473)
  • cargo test --doc --workspace (3/3)
  • cargo build --workspace --all-features

Closes #67

polaz added 2 commits June 28, 2026 23:26
- parse bounded Modulus and Exponent CryptoBinary values
- construct validated RSA SPKI keys for verification
- cover malformed, weak, and incompatible key parameters

Closes #67
@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown

Review Change Stack

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: 4efe72de-c857-41e9-90d8-de9eb699a8c9

📥 Commits

Reviewing files that changed from the base of the PR and between a2d958a and 2f891bc.

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

📝 Walkthrough

Summary by CodeRabbit

  • New Features
    • Added built-in resolution of XML signature verification keys from RSA KeyValue sources (in addition to existing key sources).
  • Bug Fixes
    • Strengthened RSA KeyValue parsing and validation (required modulus/exponent structure and correct element ordering).
    • Improved base64 handling with whitespace normalization and length limits.
    • Rejects weak RSA keys and enforces RSA-compatible signature algorithms for verification.
  • Tests
    • Expanded end-to-end and negative coverage for valid/legacy RSA KeyValue scenarios and failure modes.

Walkthrough

Adds structured RSAKeyValue parsing with bounded XML base64 decoding, resolves decoded RSA key material into verification keys, and updates verification tests, donor-suite handling, and README status text.

Changes

RSAKeyValue Parsing and Key Resolution

Layer / File(s) Summary
Bounded XML base64 normalization
src/xmldsig/whitespace.rs
Adds a bounded XML base64 normalizer, length-limited error types, and tests for capped and uncapped whitespace handling.
RSAKeyValue parser and key material shape
src/xmldsig/parse.rs
Changes KeyValueInfo::Rsa to carry decoded modulus and exponent bytes, routes RSAKeyValue parsing to a strict parser, and adds tests for accepted and rejected RSAKeyValue structures and payloads.
RSA key resolution
src/xmldsig/keys.rs
Adds RSA DER/SPKI imports, resolves KeyValueInfo::Rsa into a VerificationKey, and routes KeyInfoSource::KeyValue through the new resolver path.
Verification tests and donor suite
src/xmldsig/keys.rs, tests/donor_full_verification_suite.rs, README.md
Adds resolver tests for valid, weak, and algorithm-mismatched RSAKeyValue inputs, updates donor-suite skip handling, and adds the README status entry.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 62.50% 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: resolving RSAKeyValue keys in xmldsig.
Description check ✅ Passed The description matches the implemented RSAKeyValue parsing, resolution, validation, and testing changes.
Linked Issues check ✅ Passed The PR covers bounded RSAKeyValue parsing, RSA-only resolution, weak/incompatible key rejection, and donor-vector verification.
Out of Scope Changes check ✅ Passed The changes stay focused on RSAKeyValue parsing, resolution, validation, tests, and related README 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/#67-rsa-key-value

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

@greptile-apps

greptile-apps Bot commented Jun 29, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds built-in XMLDSig RSA KeyValue parsing and verification-key resolution. The main changes are:

  • Parses bounded RSAKeyValue Modulus and Exponent CryptoBinary values.
  • Converts embedded RSA parameters into SubjectPublicKeyInfo DER for the default resolver.
  • Applies existing RSA algorithm, modulus-size, and exponent checks to embedded keys.
  • Adds donor-suite coverage for successful RSA KeyValue verification and weak-key rejection.

Confidence Score: 5/5

The RSA KeyValue parsing and resolver changes appear merge-safe based on the bounded parsing, algorithm-policy enforcement, and donor-suite coverage described.

No correctness or security issues were identified in the reviewed changes, and the implementation is covered by workspace checks, linting, end-to-end donor verification, and negative tests for malformed or weak embedded RSA parameters.

T-Rex T-Rex Logs

What T-Rex did

  • Ran the RSA keyvalue end-to-end test on the base commit and observed a failure with VerifyResult status Invalid(KeyNotFound) and exit code 101.
  • Ran the RSA keyvalue end-to-end test on the head commit and observed a passing verification with status Valid and exit code 0.
  • Inspected the before artifact for RSA keyvalue security to verify baseline Rust test source and environment showing base lacks KeyValueInfo::Rsa.
  • Inspected the after artifact for RSA keyvalue security to verify the test source and verbose cargo output show 11 RSAKeyValue tests passed, including parser rejection cases and key resolver policy/mismatch tests, with test result: ok and exit code 0.

View all artifacts

T-Rex Ran code and verified through T-Rex

Reviews (2): Last reviewed commit: "fix(xmldsig): bound crypto binary normal..." | Re-trigger Greptile

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

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 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 592-606: The length check in the XML base64 parsing path is
happening after `normalize_xml_base64_text` has already appended the full child
text into `cleaned`, so oversized `<Modulus>`/`<Exponent>` nodes can still drive
large allocations. Update the `parse.rs` base64 normalization flow around
`normalize_xml_base64_text` and `cleaned` so the maximum decoded length is
enforced during accumulation, rejecting input before `cleaned` grows past
`max_base64_len` while preserving the existing XML-whitespace normalization
behavior.
🪄 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: 4faf5dcd-520d-4314-b3d6-e30f2bd15185

📥 Commits

Reviewing files that changed from the base of the PR and between 400410b and a2d958a.

📒 Files selected for processing (4)
  • README.md
  • src/xmldsig/keys.rs
  • src/xmldsig/parse.rs
  • tests/donor_full_verification_suite.rs

Comment thread src/xmldsig/parse.rs
@polaz polaz merged commit 0f296c3 into main Jun 29, 2026
10 checks passed
@polaz polaz deleted the feat/#67-rsa-key-value branch June 29, 2026 20:46
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.

feat(xmldsig): parse RSAKeyValue parameters

1 participant