feat(xmldsig): add RSA signature verification#22
Merged
Conversation
- add RSA PKCS#1 v1.5 verification helpers for SHA-1/256/384/512 - cover donor RSA vectors and signature mismatch cases in integration tests - refresh README disclaimer and support footer for the current public status Closes #21
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an xmldsig::signature module implementing RSA PKCS#1 v1.5 XMLDSig signature verification (rsa-sha1/256/384/512) with PEM/SPKI parsing, typed errors, re-exports, integration tests, XML fixtures, README status updates, and FUNDING metadata. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an RSA PKCS#1 v1.5 signature verification layer for XMLDSig, plus tests and README updates, as part of the roadmap work to support XMLDSig verification (#21).
Changes:
- Introduces
xmldsig::signatureRSA verification helpers backed byring, accepting public keys as SPKI (PEM/DER). - Exposes the new verification API via
xmldsigmodule re-exports. - Adds RSA signature integration tests and updates README status/disclaimer/support text.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/rsa_signature_integration.rs |
New integration tests exercising RSA signature verification against fixtures/vectors and negative cases. |
src/xmldsig/signature.rs |
New RSA verification module: maps SignatureAlgorithm → ring algorithms, parses SPKI, verifies signatures. |
src/xmldsig/mod.rs |
Exposes the new signature module and re-exports verification helpers/errors. |
README.md |
Updates project warning/status/spec table and adds a support section. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 58-62: The README's Support section is fine; optionally add a
.github/FUNDING.yml file to surface funding via GitHub's "Sponsor" button —
create .github/FUNDING.yml and include an entry that points to your preferred
funding channel or a short note containing the TRX/TRC-20 address shown in
README.md (TFDsezHa1cBkoeZT5q2T49Wp66K8t2DmdA) or a link to a hosted page with
that info so GitHub can display the Sponsor UI alongside the existing README
donation text.
In `@src/xmldsig/signature.rs`:
- Around line 50-56: The PEM/DER parsers (x509_parser::pem::parse_x509_pem and
SubjectPublicKeyInfo::from_der) currently ignore the parser remainder (`_`),
allowing trailing garbage to be accepted; update the code paths (around the
parse_x509_pem usage and the SubjectPublicKeyInfo::from_der call used by
verify_rsa_signature_spki) to check that the returned remainder is empty and
return the appropriate SignatureVerificationError (e.g., InvalidKeyDer or
InvalidKeyFormat) if any unconsumed bytes remain, so only inputs that are fully
consumed by the parser are treated as valid keys.
In `@tests/rsa_signature_integration.rs`:
- Around line 174-187: The test malformed_pem_returns_typed_error is mislabeled:
the PEM provided is syntactically valid so verify_rsa_signature_pem(...) only
triggers SignatureVerificationError::InvalidKeyFormat; rename this test (e.g.,
non_public_key_pem_returns_invalid_key_format) and keep the CERTIFICATE input to
assert InvalidKeyFormat, and add a separate malformed PEM test that calls
verify_rsa_signature_pem with a truly corrupted PEM (e.g., tamper the base64
body or break the BEGIN/END footer) and assert the appropriate parsing/malformed
PEM error; update assertions to match the specific error variants for each case.
- Around line 123-170: The tests tampered_signed_info_fails_verification and
wrong_spki_key_fails_verification use workstation-specific absolute paths;
update their calls to read_file to build paths relative to the repository (e.g.
using the CARGO_MANIFEST_DIR env var or the same vendored fixtures used by the
positive tests) for both the XML fixtures and the rsa pubkey fixture; locate the
string literals passed to read_file in those functions and replace them with
repo-relative path construction while keeping the rest of the test logic and the
canonicalized_signed_info_and_signature / read_file calls unchanged.
- Around line 96-119: The three integration tests
donor_rsa_sha256_signature_matches, donor_rsa_sha384_signature_matches, and
donor_rsa_sha512_signature_matches call assert_donor_signature_valid with donor
corpus paths that are missing in CI; either vendor those donor XML fixtures into
the repo under tests/fixtures and update the Path::new arguments to point to the
vendored locations, or mark each test with #[test] replaced by #[test] #[ignore
= "donor corpus not available in CI"] (or add a module-level #[ignore = "..."]),
ensuring the test names (donor_rsa_sha256_signature_matches,
donor_rsa_sha384_signature_matches, donor_rsa_sha512_signature_matches) and the
assert_donor_signature_valid calls remain unchanged aside from the path or
attribute update.
🪄 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: c5a29f50-97a0-4c09-8aeb-c7276fe08226
📒 Files selected for processing (4)
README.mdsrc/xmldsig/mod.rssrc/xmldsig/signature.rstests/rsa_signature_integration.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/rsa_signature_integration.rs (1)
167-179:⚠️ Potential issue | 🟡 MinorRename this test and add a truly malformed-PEM case.
Line 167 labels this as malformed PEM, but Line 170 is valid PEM with a wrong label (
CERTIFICATE), so it exercisesInvalidKeyFormat, not malformed parsing.Based on learnings, "Corruption/validation tests: tamper the relevant field (e.g., digest value, signature bytes, base64 encoding) and assert the error".Suggested split for clarity and coverage
#[test] -fn malformed_pem_returns_typed_error() { +fn non_public_key_pem_returns_invalid_key_format() { let err = verify_rsa_signature_pem( SignatureAlgorithm::RsaSha256, "-----BEGIN CERTIFICATE-----\nZm9v\n-----END CERTIFICATE-----\n", b"payload", b"signature", ) .expect_err("non-public-key PEM should be rejected"); assert!(matches!( err, SignatureVerificationError::InvalidKeyFormat { .. } )); } + +#[test] +fn malformed_pem_returns_invalid_key_der() { + let err = verify_rsa_signature_pem( + SignatureAlgorithm::RsaSha256, + "-----BEGIN PUBLIC KEY-----\n!!!not-base64!!!\n-----END PUBLIC KEY-----\n", + b"payload", + b"signature", + ) + .expect_err("malformed PEM should be rejected"); + + assert!(matches!(err, SignatureVerificationError::InvalidKeyDer)); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/rsa_signature_integration.rs` around lines 167 - 179, Rename the existing test function malformed_pem_returns_typed_error to something like non_public_key_pem_returns_invalid_key_format to reflect that the PEM is syntactically valid but has the wrong label, and add a new test (e.g., truly_malformed_pem_returns_parse_error) that calls verify_rsa_signature_pem with an actually malformed PEM (for example a broken header/footer or invalid base64 payload such as "-----BEGIN PUBLIC KEY-----\n!!notbase64!!\n-----END PUBLIC KEY-----") and assert the function returns the appropriate parse/malformed error variant; reference the verify_rsa_signature_pem call site and the existing assertion that expects SignatureVerificationError::InvalidKeyFormat to split coverage between the wrong-label case and the truly malformed PEM case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/rsa_signature_integration.rs`:
- Around line 167-179: Rename the existing test function
malformed_pem_returns_typed_error to something like
non_public_key_pem_returns_invalid_key_format to reflect that the PEM is
syntactically valid but has the wrong label, and add a new test (e.g.,
truly_malformed_pem_returns_parse_error) that calls verify_rsa_signature_pem
with an actually malformed PEM (for example a broken header/footer or invalid
base64 payload such as "-----BEGIN PUBLIC KEY-----\n!!notbase64!!\n-----END
PUBLIC KEY-----") and assert the function returns the appropriate
parse/malformed error variant; reference the verify_rsa_signature_pem call site
and the existing assertion that expects
SignatureVerificationError::InvalidKeyFormat to split coverage between the
wrong-label case and the truly malformed PEM case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ea40b74-800a-4ebe-b5f8-6c17f4fcaeb1
📒 Files selected for processing (5)
tests/fixtures/xmldsig/aleksey-xmldsig-01/enveloping-sha256-rsa-sha256.xmltests/fixtures/xmldsig/aleksey-xmldsig-01/enveloping-sha384-rsa-sha384.xmltests/fixtures/xmldsig/aleksey-xmldsig-01/enveloping-sha512-rsa-sha512.xmltests/fixtures_smoke.rstests/rsa_signature_integration.rs
Member
Author
|
@coderabbitai re-review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/xmldsig/mod.rs (1)
13-25: 🧹 Nitpick | 🔵 TrivialConsider reducing API surface to re-exports only.
If callers are expected to use
xmldsig::{verify_rsa_signature_pem, ...},pub mod signaturecan be made private to avoid committing the whole submodule path as public API.Based on learnings, "Applies to **/*.rs : Tier 3 - Flag API design issues: `pub` visibility where `pub(crate)` suffices".Possible tightening
-pub mod signature; +mod signature;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/mod.rs` around lines 13 - 25, The module currently exposes submodules (pub mod signature, transforms, types, uri, verify) unnecessarily; make these modules private (remove pub) and keep the public API limited to the existing re-exports (e.g. keep pub use digest::..., pub use parse::..., and pub use signature::{SignatureVerificationError, verify_rsa_signature_pem, verify_rsa_signature_spki}); update internal references inside the crate to refer to crate::signature, crate::transforms, etc., so callers only see the intended symbols (SignatureVerificationError, verify_rsa_signature_pem, verify_rsa_signature_spki, DigestAlgorithm, compute_digest, constant_time_eq, ParseError, Reference, SignatureAlgorithm, SignedInfo, find_signature_node, parse_signed_info) while the submodule paths are not part of the public API surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 32-35: Update README wording to avoid ambiguity between
"implemented" and "in progress" by using a consistent phrase such as "Partially
implemented" or "Implemented (partial)" for the features listed; specifically
change the bullet that currently reads "C14N 1.0, C14N 1.1, and Exclusive C14N"
and the lines that mention those specs as "In progress" to a single consistent
status (e.g., "Partially implemented: C14N 1.0, C14N 1.1, Exclusive C14N") and
apply the same consistent phrasing for "XMLDSig" and "RSA PKCS#1 v1.5" entries
so the README no longer mixes "implemented" and "in progress" for the same
features.
In `@src/xmldsig/signature.rs`:
- Around line 119-125: The test
ecdsa_algorithms_are_rejected_for_rsa_verification currently only asserts that
SignatureAlgorithm::EcdsaP256Sha256 is rejected; add an additional check for
SignatureAlgorithm::EcdsaP384Sha384 by calling
verification_algorithm(SignatureAlgorithm::EcdsaP384Sha384).unwrap_err() and
asserting it matches SignatureVerificationError::UnsupportedAlgorithm (same as
the existing assertion) so both ECDSA variants are covered; keep both checks
inside the same test function to lock the RSA-only guard against regressions.
In `@tests/rsa_signature_integration.rs`:
- Around line 144-152: Rename the test function
wrong_spki_key_fails_verification to wrong_public_key_pem_fails_verification to
accurately reflect that it calls verify_rsa_signature_pem (not SPKI); update the
function declaration and any internal references (e.g., test attribute or any
place that calls or documents the test name) to use
wrong_public_key_pem_fails_verification so names align with behavior and the
PEM-focused verification helper verify_rsa_signature_pem is clearly represented.
- Around line 126-127: The test mutates the last byte of canonical_signed_info
without guarding for an empty Vec, which can panic when computing len() - 1;
update the mutation in tests/rsa_signature_integration.rs to check
canonical_signed_info is non-empty (e.g., use an existence check or last_mut()
pattern) before flipping the last byte so the test won't panic if
canonicalization returns an empty buffer; reference the canonical_signed_info
variable and the byte-flip operation where last_index and XOR are used.
---
Outside diff comments:
In `@src/xmldsig/mod.rs`:
- Around line 13-25: The module currently exposes submodules (pub mod signature,
transforms, types, uri, verify) unnecessarily; make these modules private
(remove pub) and keep the public API limited to the existing re-exports (e.g.
keep pub use digest::..., pub use parse::..., and pub use
signature::{SignatureVerificationError, verify_rsa_signature_pem,
verify_rsa_signature_spki}); update internal references inside the crate to
refer to crate::signature, crate::transforms, etc., so callers only see the
intended symbols (SignatureVerificationError, verify_rsa_signature_pem,
verify_rsa_signature_spki, DigestAlgorithm, compute_digest, constant_time_eq,
ParseError, Reference, SignatureAlgorithm, SignedInfo, find_signature_node,
parse_signed_info) while the submodule paths are not part of the public API
surface.
🪄 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: e009f9fa-9453-4306-a829-fc9d4122b300
📒 Files selected for processing (9)
.github/FUNDING.ymlREADME.mdsrc/xmldsig/mod.rssrc/xmldsig/signature.rstests/fixtures/xmldsig/aleksey-xmldsig-01/enveloping-sha256-rsa-sha256.xmltests/fixtures/xmldsig/aleksey-xmldsig-01/enveloping-sha384-rsa-sha384.xmltests/fixtures/xmldsig/aleksey-xmldsig-01/enveloping-sha512-rsa-sha512.xmltests/fixtures_smoke.rstests/rsa_signature_integration.rs
There was a problem hiding this comment.
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/signature.rs`:
- Around line 89-94: The current code collapses all verification failures into
Ok(false) by calling key.verify(...).is_ok(); instead, call
signature::UnparsedPublicKey::verify and propagate its error so callers can
distinguish parsing/constraint errors from signature mismatches — e.g., replace
the is_ok() usage with something that maps success to Ok(true) and maps the
verify error into the function's Err (or use the ? operator) so verification
errors from key.verify(...) are returned rather than converted to false;
reference signature::UnparsedPublicKey::new and key.verify(...) when making the
change.
🪄 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: 66328c44-49e7-4085-8e6b-4df5cc037c5b
📒 Files selected for processing (3)
README.mdsrc/xmldsig/signature.rstests/rsa_signature_integration.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/signature.rs`:
- Around line 132-158: The match arms in minimum_rsa_modulus_bits and
verification_algorithm currently use a wildcard (`_`) which hides new
SignatureAlgorithm variants; replace the wildcard with explicit match arms for
every current non-RSA SignatureAlgorithm variant (returning the same
Err(SignatureVerificationError::UnsupportedAlgorithm { uri:
algorithm.uri().to_string() })) so the compiler will require you to handle any
future additions to the SignatureAlgorithm enum and force deliberate handling in
verification_algorithm and minimum_rsa_modulus_bits.
- Around line 44-48: Add the #[must_use] attribute to the public verification
functions so callers can't ignore the Result; specifically annotate the
functions verify_rsa_signature_pem and verify_rsa_signature_spki with
#[must_use] above their definitions to ensure the Result<bool,
SignatureVerificationError> is not silently discarded and to satisfy the API
design guideline for security-sensitive return values.
🪄 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: b963cd9b-8e9b-41dd-afdf-dac7e3191eee
📒 Files selected for processing (2)
src/xmldsig/signature.rstests/rsa_signature_integration.rs
There was a problem hiding this comment.
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 `@README.md`:
- Around line 32-35: The specifications table is missing a row for Canonical XML
1.1 even though the status bullets list it as implemented; update the README
table to include a "C14N 1.1" entry with the same implementation/status details
as the C14N 1.0 row so the support reporting is consistent (update the table
near the existing C14N/Canonical entries referenced by the bullets that list
"C14N 1.0, C14N 1.1, and Exclusive C14N").
🪄 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: c69db948-4dbe-40b9-8e61-74f6bc302983
📒 Files selected for processing (3)
README.mdsrc/xmldsig/signature.rstests/rsa_signature_integration.rs
This was referenced Mar 26, 2026
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
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
rsa-sha1,rsa-sha256,rsa-sha384, andrsa-sha512Testing
Closes #21