feat(xmldsig): add full signature verify pipeline#30
Merged
Conversation
- Add end-to-end XMLDSig verification API over SignedInfo references and SignatureValue - Add RSA/ECDSA integration tests for valid and tampered donor vectors - Update README status to reflect full verification pipeline support Closes #29
|
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 end-to-end XMLDSig verification pipeline: new public entrypoint Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant Pipeline as "verify_signature_with_pem_key"
participant Parser as "XML Parser"
participant RefProcessor as "process_all_references"
participant Canonicalizer as "c14n::canonicalize"
participant CryptoVerifier as "RSA/ECDSA verifier"
Caller->>Pipeline: call(xml, public_key_pem, store_pre_digest)
Pipeline->>Parser: parse XML
Parser-->>Pipeline: Document
Pipeline->>Pipeline: locate Signature & SignedInfo (ensure single Signature)
Pipeline->>RefProcessor: process_all_references(SignedInfo)
RefProcessor-->>Pipeline: ReferencesResult
alt references all valid
Pipeline->>Canonicalizer: canonicalize(SignedInfo)
Canonicalizer-->>Pipeline: canonical bytes
Pipeline->>Pipeline: normalize & base64-decode SignatureValue
Pipeline->>CryptoVerifier: verify(signature_bytes, alg, public_key_pem)
CryptoVerifier-->>Pipeline: signature_valid / crypto error
Pipeline-->>Caller: SignatureVerificationResult { references, signature_checked=true, signature_valid }
else references invalid
Pipeline-->>Caller: SignatureVerificationResult { references, signature_checked=false, signature_valid=false }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 the current code and only fix it if needed.
Inline comments:
In `@src/xmldsig/verify.rs`:
- Around line 166-179: The SignatureVerificationResult struct can be dropped
without inspection, so add the #[must_use] attribute to the
SignatureVerificationResult declaration to force callers to handle verification
outcomes; update the struct definition (the one starting with "pub struct
SignatureVerificationResult") to include #[must_use] above it and ensure callers
of verify_signature_with_pem_key(...) respond to the returned value accordingly.
In `@tests/signature_pipeline_integration.rs`:
- Around line 87-126: Add a new unit test (e.g.,
tampered_signature_value_malformed_base64_fails) similar to
tampered_signature_value_fails_after_references_pass that reads the same
fixture, mutates the <SignatureValue> to inject an invalid base64 character (use
mutate_ds_tag_content(&xml, "SignatureValue") but introduce a non-base64 char),
calls verify_signature_with_pem_key(&tampered_xml, &public_key_pem, false) and
assert that it returns
Err(SignatureVerificationPipelineError::SignatureValueBase64(_)); reference
verify_signature_with_pem_key, mutate_ds_tag_content, and the
SignatureVerificationPipelineError::SignatureValueBase64 variant to locate the
relevant code paths.
🪄 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: dbcfb7b3-a2f1-4b05-8217-703294f58cd3
📒 Files selected for processing (4)
README.mdsrc/xmldsig/mod.rssrc/xmldsig/verify.rstests/signature_pipeline_integration.rs
- Mark SignatureVerificationResult as #[must_use] to avoid ignored verification outcomes - Add malformed SignatureValue base64 regression test for decode-stage errors
polaz
commented
Mar 27, 2026
polaz
commented
Mar 27, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an end-to-end XMLDSig verification pipeline to the xmldsig module, allowing callers to verify <SignedInfo> reference digests first and then verify <SignatureValue> over canonicalized <SignedInfo> using an RSA/ECDSA public key.
Changes:
- Introduces
verify_signature_with_pem_keyplus structuredSignatureVerificationResult/SignatureVerificationPipelineError. - Adds integration tests covering RSA/ECDSA donor vectors and negative cases (tampered digest/signature, malformed base64).
- Updates README to reflect full signature verification pipeline support.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/xmldsig/verify.rs |
Implements the end-to-end signature verification pipeline and new public result/error types. |
src/xmldsig/mod.rs |
Re-exports the new pipeline API and associated types. |
tests/signature_pipeline_integration.rs |
Adds integration coverage for full pipeline success/failure scenarios. |
README.md |
Updates project status to reflect full pipeline support and ECDSA helpers. |
- Parse SignatureValue node first, then read text with unwrap_or("")
- Add regression test to ensure empty SignatureValue is not treated as MissingElement
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 `@tests/signature_pipeline_integration.rs`:
- Around line 67-92: The helper inject_invalid_base64_in_signature_value only
searches for an unprefixed "<SignatureValue>" and will panic if the fixture uses
a prefixed "<ds:SignatureValue>"; to fix this, replace the custom logic in
inject_invalid_base64_in_signature_value with a call to the existing
replace_ds_tag_content_with helper (which handles both prefixed and unprefixed
ds tags) or implement the same prefix-aware search/replace using
replace_ds_tag_content_with's approach; update
inject_invalid_base64_in_signature_value to locate the SignatureValue content
via replace_ds_tag_content_with, mutate one base64 char to '!' in the returned
content, and return the reconstructed XML so the behavior and fixture coverage
match other mutation helpers.
🪄 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: e57a35d2-aaa2-4f5a-840b-77ab4ef1b92e
📒 Files selected for processing (2)
src/xmldsig/verify.rstests/signature_pipeline_integration.rs
- Reuse replace_ds_tag_content_with in inject_invalid_base64_in_signature_value - Avoid unprefixed-only tag handling in malformed base64 test helper
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 `@tests/signature_pipeline_integration.rs`:
- Around line 117-156: Add an ECDSA variant of the two tampering tests to
exercise the ECDSA code path: duplicate
tampered_signature_value_fails_after_references_pass and
tampered_digest_value_fails_before_signature_stage but load the ECDSA fixtures
(use the ECDSA XML fixture and the corresponding ECDSA public key PEM), keep
calls to mutate_ds_tag_content and verify_signature_with_pem_key, and assert the
same expectations (references.all_valid / !references.all_valid,
signature_checked true/false, signature_valid false) to ensure
algorithm-specific dispatch handles negative cases.
🪄 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: d6848fce-0e07-4ee2-becb-ac8c74b6f785
📒 Files selected for processing (1)
tests/signature_pipeline_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 `@tests/signature_pipeline_integration.rs`:
- Around line 41-58: The helper replace_ds_tag_content_with is too strict when
building exact opening strings (prefixed_open/plain_open) and panics if there is
harmless whitespace or attributes before the closing '>' — change the logic to
locate the opening element by scanning for the start marker "<ds:{tag}" or
"<{tag}" (using contains or find for that prefix), then advance from that index
to the next '>' to compute start = index_of('>') + 1 so the mutation targets
element content only; keep the existing closing-tag detection (</ds:{tag}> or
</{tag}>) but ensure you search for the closing sequence after start to avoid
mismatches.
🪄 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: 834314c2-f18f-4012-b45e-0930da373cd5
📒 Files selected for processing (1)
tests/signature_pipeline_integration.rs
Member
Author
|
@coderabbitai re-review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/signature_pipeline_integration.rs (1)
41-55:⚠️ Potential issue | 🟡 MinorMake the mutation helper tolerant of start-tag trivia.
This still requires an exact
<ds:{tag}>/<{tag}>opening tag. A benign attribute or whitespace before>makes the test panic before it reachesverify_signature_with_pem_key, so the corruption case no longer isolates verifier behavior.♻️ Proposed fix
- let prefixed_open = format!("<ds:{tag}>"); + let prefixed_open = format!("<ds:{tag}"); let prefixed_close = format!("</ds:{tag}>"); - let plain_open = format!("<{tag}>"); + let plain_open = format!("<{tag}"); let plain_close = format!("</{tag}>"); @@ - let start = xml - .find(&open) - .unwrap_or_else(|| panic!("missing opening tag {open}")) - + open.len(); + let open_start = xml + .find(&open) + .unwrap_or_else(|| panic!("missing opening tag prefix {open}")); + let start = xml[open_start..] + .find('>') + .unwrap_or_else(|| panic!("missing closing `>` for opening tag {tag}")) + + open_start + + 1;As per coding guidelines, corruption/validation tests: tamper the relevant field (e.g., digest value, signature bytes, base64 encoding) and assert the error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/signature_pipeline_integration.rs` around lines 41 - 55, The helper currently requires an exact opening tag string (prefixed_open/plain_open) and fails if there are attributes or whitespace before the closing '>'; change the discovery logic to search for the start of the tag prefix (e.g., look for "<ds:{tag}" and "<{tag}" using the existing prefixed_open/plain_open variables or xml.contains checks), then locate the next '>' after that match and set start to the index after that '>' (instead of matching a full "<...>"); keep the existing close/tag selection but ensure you still find the matching closing tag string (prefixed_close/plain_close) for later use.
🤖 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/verify.rs`:
- Around line 308-315: The code currently uses signature_value_node.text() which
only returns the first text child and then removes characters with
char::is_whitespace(), causing truncated signature data and over-removal of
Unicode whitespace; update the logic that builds the base64 string from
SignatureValue by iterating all child nodes of signature_value_node, filtering
for text nodes (e.g., children().filter(|n| n.is_text())), concatenating each
node.text() result to form the full text content, then strip only the four XML
whitespace characters (' ', '\t', '\r', '\n') (not char::is_whitespace()) before
calling base64::engine::general_purpose::STANDARD.decode; adjust the code around
signature_value_node and the existing normalized variable to perform these steps
so decoding receives the correct concatenated/normalized string.
---
Duplicate comments:
In `@tests/signature_pipeline_integration.rs`:
- Around line 41-55: The helper currently requires an exact opening tag string
(prefixed_open/plain_open) and fails if there are attributes or whitespace
before the closing '>'; change the discovery logic to search for the start of
the tag prefix (e.g., look for "<ds:{tag}" and "<{tag}" using the existing
prefixed_open/plain_open variables or xml.contains checks), then locate the next
'>' after that match and set start to the index after that '>' (instead of
matching a full "<...>"); keep the existing close/tag selection but ensure you
still find the matching closing tag string (prefixed_close/plain_close) for
later use.
🪄 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: 398ad8ee-c2d3-47db-8515-f5ec699cb821
📒 Files selected for processing (4)
README.mdsrc/xmldsig/mod.rssrc/xmldsig/verify.rstests/signature_pipeline_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/verify.rs`:
- Around line 431-440: The conversion u8::try_from(u32::from(ch)) in the
ch.is_ascii_whitespace() branch is infallible and the map_err arm is dead code;
replace that try_from(...).map_err(...) with a direct cast or an expect (e.g.,
let invalid_byte = ch as u8 or use u8::try_from(...).expect("ch is ASCII
whitespace") ) and then return
SignatureVerificationPipelineError::SignatureValueBase64(base64::DecodeError::InvalidByte(normalized.len(),
invalid_byte)); keep references to ch.is_ascii_whitespace(),
SignatureVerificationPipelineError::SignatureValueBase64,
base64::DecodeError::InvalidByte and normalized.len() so the intent and error
payload remain identical.
🪄 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: 2e487e9e-7be3-4c5f-b6b2-e5a044899b47
📒 Files selected for processing (3)
src/xmldsig/parse.rssrc/xmldsig/verify.rstests/signature_pipeline_integration.rs
- replace infallible try_from+map_err with direct u8 cast in SignatureValue normalization - keep InvalidByte payload identical while removing unreachable error mapping
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/xmldsig/verify.rs (1)
431-435: 🛠️ Refactor suggestion | 🟠 MajorMake the narrowing on Line 432 explicit.
The ASCII guard makes this safe, but a raw
as u8still violates the repo conversion rule. Please either switch this branch back to an explicitu8::try_from(u32::from(ch))conversion or annotate the cast with the required lint justification.The first command should still show
let invalid_byte = ch as u8;without a nearbycast_possible_truncationannotation.#!/bin/bash sed -n '428,436p' src/xmldsig/verify.rs rg -n 'cast_possible_truncation|is_ascii_whitespace|as u8' src/xmldsig/verify.rs -C 2As per coding guidelines "Use
TryFrom/TryIntofor fallible conversions;ascasts need#[expect(clippy::cast_possible_truncation)]with reason".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/verify.rs` around lines 431 - 435, In the SignatureVerificationPipelineError::SignatureValueBase64 branch inside the verify flow, replace the raw cast `let invalid_byte = ch as u8;` with an explicit, fallible conversion using `u8::try_from(u32::from(ch))` (propagate/map the error as needed) so the narrowing is explicit and complies with the conversion guideline; update the code that constructs the base64::DecodeError to use the resulting u8 value. This targets the block that checks `if ch.is_ascii_whitespace()` in src/xmldsig/verify.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/xmldsig/verify.rs`:
- Around line 431-435: In the
SignatureVerificationPipelineError::SignatureValueBase64 branch inside the
verify flow, replace the raw cast `let invalid_byte = ch as u8;` with an
explicit, fallible conversion using `u8::try_from(u32::from(ch))` (propagate/map
the error as needed) so the narrowing is explicit and complies with the
conversion guideline; update the code that constructs the base64::DecodeError to
use the resulting u8 value. This targets the block that checks `if
ch.is_ascii_whitespace()` in src/xmldsig/verify.rs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7f7461a2-4b22-431d-b4a4-83b23c9e53f5
📒 Files selected for processing (1)
src/xmldsig/verify.rs
- use explicit u8::try_from(u32::from(ch)) in SignatureValue whitespace rejection - keep InvalidByte payload unchanged while satisfying conversion rule
Member
Author
|
@coderabbitai Fixed the duplicate finding in |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 62 🏁 Script executed: Length of output: 35031
if ch.is_ascii_whitespace() {
let invalid_byte =
u8::try_from(u32::from(ch)).expect("ASCII whitespace always fits into u8");
return Err(SignatureVerificationPipelineError::SignatureValueBase64(
base64::DecodeError::InvalidByte(normalized.len(), invalid_byte),
));
}A few confirmations:
✏️ Learnings added
🧠 Learnings used |
This was referenced Mar 28, 2026
This was referenced Apr 9, 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
Issue
Closes #29
Validation