feat(xmldsig): add KeyInfo dispatch parsing#47
Conversation
- parse KeyInfo child sources: KeyName, KeyValue, X509Data, DEREncodedKeyValue - wire KeyInfo parsing into signature verification pipeline with explicit ParseKeyInfo error - add parser and pipeline coverage tests for order and malformed input Closes #46
|
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 (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds KeyInfo parsing (KeyName, KeyValue, X509Data, dsig11:DEREncodedKeyValue) and exported KeyInfo models; factors XML whitespace/base64 normalization into a crate-visible helper; wires conditional KeyInfo parsing into verification and tightens child-order and mixed-content validation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Verify as "verify.rs"
participant ParseSigned as "parse.rs (parse_signed_info)"
participant ParseKey as "parse.rs (parse_key_info)"
participant Whitespace as "whitespace.rs"
Caller->>Verify: verify_signature_with_context(signature_node, ctx)
Verify->>ParseSigned: parse_signed_info(signed_info_node)
ParseSigned-->>Verify: SignedInfo
Verify->>Verify: parse_signature_value()
alt KeyInfo present or resolver.consumes_document_key_info() == true
Verify->>ParseKey: parse_key_info(key_info_node)
ParseKey->>Whitespace: normalize_xml_base64_text / is_xml_whitespace_only
ParseKey->>ParseKey: dispatch children (KeyName, KeyValue, X509Data, DEREncodedKeyValue)
ParseKey-->>Verify: KeyInfo or ParseError
else KeyInfo absent / ignored
Note over Verify: continue without KeyInfo
end
Verify-->>Caller: verification result or DsigError::ParseKeyInfo
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 |
There was a problem hiding this comment.
Pull request overview
Adds initial XMLDSig <KeyInfo> child dispatch parsing (ROADMAP P2-001) and wires it into the verification pipeline so malformed/unsupported structures can be surfaced as explicit pipeline parse errors.
Changes:
- Introduces
KeyInfo/KeyInfoSourceparsing with dispatch forKeyName,KeyValue,X509Data, anddsig11:DEREncodedKeyValue. - Integrates
<KeyInfo>parsing into the verify pipeline and enforces<Signature>child ordering with optionalKeyInfoas the 3rd element child. - Re-exports the new parsing API from the
xmldsigmodule and adds targeted unit tests for parser + pipeline behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/xmldsig/parse.rs |
Adds KeyInfo model types and a dispatch parser for supported KeyInfo child elements + unit tests. |
src/xmldsig/verify.rs |
Parses optional KeyInfo in the verify pipeline, adds ParseKeyInfo error mapping, and enforces Signature child ordering + pipeline tests. |
src/xmldsig/mod.rs |
Re-exports new KeyInfo parsing types/functions as part of the public API. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/xmldsig/verify.rs (1)
964-1043:⚠️ Potential issue | 🟠 MajorReject unexpected
<Signature>children after the allowed prefix.This index tracking only proves where
SignedInfo,SignatureValue, andKeyInfoappear; it still accepts"<Signature><SignedInfo/><SignatureValue/><Foo/></Signature>"and"...<KeyInfo/><Foo/>"because any other trailing element is ignored. The content model after the optionalKeyInfois onlyObject*, so this should fail closed and get a regression test.As per coding guidelines,
Tier 1 - Flag logic bugs: missing validation (unvalidated input from network/disk/external source).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/verify.rs` around lines 964 - 1043, The parser currently ignores unexpected XMLDSIG element children after the allowed prefix, so "<Signature><SignedInfo/><SignatureValue/><Foo/></Signature>" wrongly succeeds; fix parse_signature_children to count only XMLDSIG element children (use a xml_element_index counter incremented only when child.tag_name().namespace() == Some(XMLDSIG_NS)) and enforce the content model: require SignedInfo at xml index 1 and SignatureValue at 2, allow optional KeyInfo at 3, and permit only "Object" elements after that; treat any other XMLDSIG element name at any position as SignatureVerificationPipelineError::InvalidStructure (include the element name in the reason) and fail if an unexpected element appears after the allowed prefix. Reference: parse_signature_children, signed_info_index, signature_value_index, key_info_index, and SignatureChildNodes.
🤖 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 357-383: The loop over element_children(key_info_node) accepts
malformed mixed-content like "<KeyValue>junk<RSAKeyValue/></KeyValue>" because
element_children() and collect_text_content() silently drop unexpected node
kinds; update the KeyInfo parsing to explicitly reject non-whitespace mixed
content and unexpected child elements: for KeyName (in the KeyName arm) assert
the node contains no element children and only text (trim() non-empty allowed,
any embedded element nodes or non-whitespace text before/after elements =>
return Err(ParseError::InvalidStructure(...))); for KeyValue and X509Data ensure
the node has no non-whitespace text nodes mixed with element children before
calling parse_key_value_dispatch or parse_x509_data_dispatch; for
DEREncodedKeyValue ensure there are no element children and the text node is the
only content before calling decode_base64_xml_text; return a clear
ParseError::InvalidStructure on violations. Apply the same validation pattern to
the other parsing locations you mentioned (around the
parse_key_value_dispatch/parse_x509_data_dispatch and DER decoding sites).
- Around line 115-161: Add forward-compatibility by marking the public parse
model as non-exhaustive and tighten field visibility: add #[non_exhaustive] to
KeyInfo, KeyInfoSource, KeyValueInfo, and X509DataInfo; change internal fields
that need not be publicly mutable (KeyInfo.sources and the X509DataInfo *_count
fields) from pub to pub(crate) so consumers cannot match exhaustively or rely on
concrete variants/fields; leave enum variants and public types exported but rely
on non_exhaustive to allow adding new variants/fields in future.
---
Outside diff comments:
In `@src/xmldsig/verify.rs`:
- Around line 964-1043: The parser currently ignores unexpected XMLDSIG element
children after the allowed prefix, so
"<Signature><SignedInfo/><SignatureValue/><Foo/></Signature>" wrongly succeeds;
fix parse_signature_children to count only XMLDSIG element children (use a
xml_element_index counter incremented only when child.tag_name().namespace() ==
Some(XMLDSIG_NS)) and enforce the content model: require SignedInfo at xml index
1 and SignatureValue at 2, allow optional KeyInfo at 3, and permit only "Object"
elements after that; treat any other XMLDSIG element name at any position as
SignatureVerificationPipelineError::InvalidStructure (include the element name
in the reason) and fail if an unexpected element appears after the allowed
prefix. Reference: parse_signature_children, signed_info_index,
signature_value_index, key_info_index, and SignatureChildNodes.
🪄 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: f04a3c91-55a7-4840-8c05-39edae4ff650
📒 Files selected for processing (3)
src/xmldsig/mod.rssrc/xmldsig/parse.rssrc/xmldsig/verify.rs
|
Addressed the outside-diff item as well in commit 2004cf8: now fails closed for unexpected XMLDSIG children after the allowed SignedInfo/SignatureValue/(optional KeyInfo) prefix, and the regression suite passes. |
|
Correction: outside-diff item is fixed in commit 2004cf8. Function parse_signature_children now rejects unexpected XMLDSIG children after the allowed SignedInfo/SignatureValue/(optional KeyInfo) prefix, and full regression tests are green. |
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
|
@coderabbitai Outside-diff item (verify.rs 964-1043) is already addressed: parse_signature_children now fails closed on unexpected XMLDSIG children after SignedInfo/SignatureValue/(optional KeyInfo), and workspace tests are green after latest commit 5238c92. |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/xmldsig/verify.rs (1)
976-984:⚠️ Potential issue | 🟠 MajorReject foreign and mixed-content children under
<Signature>.This scan only tracks XMLDSig element children and silently skips everything else, so malformed inputs like
<foo:Bar/>or stray non-whitespace text after the allowed prefix still pass the new suffix checks. If this function is meant to enforce theSignedInfo,SignatureValue, optionalKeyInfo, thenObject*shape, it needs to fail closed on any non-XMLDSig element child and any non-whitespace mixed content under<Signature>.🔒 Suggested hardening
- for (zero_based_index, child) in signature_node - .children() - .filter(|node| node.is_element()) - .enumerate() - { - let element_index = zero_based_index + 1; - if child.tag_name().namespace() != Some(XMLDSIG_NS) { - continue; - } + let mut element_index = 0usize; + for child in signature_node.children() { + if child.is_text() + && child.text().is_some_and(|text| { + text.chars().any(|c| !matches!(c, ' ' | '\t' | '\n' | '\r')) + }) + { + return Err(SignatureVerificationPipelineError::InvalidStructure { + reason: "Signature must not contain non-whitespace mixed content", + }); + } + if !child.is_element() { + continue; + } + element_index += 1; + if child.tag_name().namespace() != Some(XMLDSIG_NS) { + return Err(SignatureVerificationPipelineError::InvalidStructure { + reason: "Signature must contain only ds:SignedInfo, ds:SignatureValue, optional ds:KeyInfo, and ds:Object element children", + }); + }As per coding guidelines,
Tier 1 - Flag logic bugs: missing validation (unchecked index, unvalidated input from network/disk/external source).Also applies to: 1050-1072
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/verify.rs` around lines 976 - 984, The loop over signature_node children in verify.rs currently ignores non-XMLDSIG elements and mixed content; change it to fail-closed: inside the iteration over signature_node.children() (the block around the for ... in signature_node.children().filter(|node| node.is_element()).enumerate()), remove the filter that hides non-element nodes and instead inspect every child node; if a child is an element and child.tag_name().namespace() != Some(XMLDSIG_NS) return an error (reject foreign elements), and if a child is not an element but contains non-whitespace text or non-trivial nodes (comments/PIs as disallowed per spec) return an error (reject mixed content); apply the same stricter checks in the corresponding later block referenced at lines ~1050-1072 so any unexpected non-XMLDSIG element or non-whitespace text under <Signature> causes verification to fail.
🤖 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 381-384: The DEREncodedKeyValue handler currently calls
collect_text_content(child) which clones the entire text payload before
decode_base64_xml_text enforces MAX_DER_ENCODED_KEY_VALUE_TEXT_LEN /
MAX_DER_ENCODED_KEY_VALUE_LEN; change this to stream/validate the text nodes
first by replacing collect_text_content+decode_base64_xml_text with a new
decode_base64_xml_children(child, ...) helper that iterates
child.children().filter(|n| n.is_text()), enforces the existing
length/whitespace caps (MAX_DER_ENCODED_KEY_VALUE_TEXT_LEN and
MAX_DER_ENCODED_KEY_VALUE_LEN) as it appends, and returns the decoded bytes so
you can still push KeyInfoSource::DerEncodedKeyValue(der); keep
ensure_no_element_children(child, "DEREncodedKeyValue") and reuse the same
validation logic elsewhere (e.g., the ranges noted at lines ~550-594).
In `@src/xmldsig/verify.rs`:
- Around line 2510-2529: The test pipeline_reports_keyinfo_parse_error currently
corrupts KeyInfo but leaves <SignedInfo/> invalid, making the failure dependent
on parser order; update the test fixture so <SignedInfo> is a minimally valid
SignedInfo (e.g., include CanonicalizationMethod, SignatureMethod and at least
one Reference with DigestMethod and a DigestValue) and keep only the
dsig11:DEREncodedKeyValue corrupted, then call verify_signature_with_pem_key and
assert the returned error is SignatureVerificationPipelineError::ParseKeyInfo(_)
to make the regression independent of element parse order.
---
Outside diff comments:
In `@src/xmldsig/verify.rs`:
- Around line 976-984: The loop over signature_node children in verify.rs
currently ignores non-XMLDSIG elements and mixed content; change it to
fail-closed: inside the iteration over signature_node.children() (the block
around the for ... in signature_node.children().filter(|node|
node.is_element()).enumerate()), remove the filter that hides non-element nodes
and instead inspect every child node; if a child is an element and
child.tag_name().namespace() != Some(XMLDSIG_NS) return an error (reject foreign
elements), and if a child is not an element but contains non-whitespace text or
non-trivial nodes (comments/PIs as disallowed per spec) return an error (reject
mixed content); apply the same stricter checks in the corresponding later block
referenced at lines ~1050-1072 so any unexpected non-XMLDSIG element or
non-whitespace text under <Signature> causes verification to fail.
🪄 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: 20e18548-9962-42e0-aaa9-80cd3a6c2bc8
📒 Files selected for processing (3)
src/xmldsig/mod.rssrc/xmldsig/parse.rssrc/xmldsig/verify.rs
|
@coderabbitai Outside-diff item (verify.rs 976-984 / 1050-1072) fixed in eec05eb: parse_signature_children now iterates all child nodes and fails closed on foreign element children, non-whitespace text mixed content, and non-element children under . |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
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/parse.rs`:
- Around line 625-628: The helper is_xml_whitespace_only is duplicated; extract
it into a shared internal module (e.g., xmldsig::utils or
xmldsig::internal::whitespace) as a single pub(crate) function, replace the
duplicate definitions in the parse and verify modules to call the new shared
function, and remove the redundant implementation so both parse's and verify's
code reference the same is_xml_whitespace_only symbol.
In `@src/xmldsig/verify.rs`:
- Around line 1123-1126: The function is_xml_whitespace_only is duplicated (one
in verify.rs and one in parse.rs); extract it into a single shared internal
utility module (e.g., xml_utils or utils) as a pub(crate) function named
is_xml_whitespace_only, remove the duplicate definitions, and update both
modules (verify.rs and parse.rs) to import and call the shared function; ensure
module visibility and imports are corrected and run tests to verify nothing
breaks.
🪄 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: bf4a51e6-3bdd-45dc-a152-5b985c143e1a
📒 Files selected for processing (2)
src/xmldsig/parse.rssrc/xmldsig/verify.rs
- gate KeyInfo parsing to resolver/no-explicit-key path and add regression coverage - centralize XML base64 normalization in whitespace helper and reuse in parse/verify - clarify public docs that only dsig11:ECKeyValue is supported
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
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)
539-552:⚠️ Potential issue | 🟠 MajorBound
DigestValuebefore decode.
DigestValuestill reaches this function at arbitrary length and is only rejected afterSTANDARD.decode()plus the final output-length check. A hostile signature can therefore force large transient allocations with an oversized<DigestValue>, even though the decoded size is fixed bydigest_method. Please mirror the bounded streaming approach used forDEREncodedKeyValue: normalize from the text children, cap the normalized base64 length atdigest_method.output_len().div_ceil(3) * 4, and fail before decode.As per coding guidelines,
Tier 1 - Flag logic bugs: missing validation (unchecked index, unvalidated input from network/disk/external source).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/parse.rs` around lines 539 - 552, In base64_decode_digest, prevent oversized allocations by computing the maximum allowed base64 text length from the expected decoded size: let cap = ((digest_method.output_len() + 2) / 3) * 4; allocate the normalized buffer with capacity cap (or min(cap, b64.len())), call normalize_xml_base64_text(b64, &mut cleaned) as before, and immediately fail with a ParseError::Base64 if cleaned.len() > cap (so the function returns before calling STANDARD.decode); update the error message to clearly indicate "DigestValue exceeds maximum allowed base64 length" and reference function names base64_decode_digest, normalize_xml_base64_text, and type DigestAlgorithm to locate the change.
🤖 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 557-564: The rustdoc pipeline comment and function docs
incorrectly state that `<KeyInfo>` parsing/validation occurs for all APIs;
update the documentation to reflect that verify_signature_with_pem_key()
unconditionally installs an explicit key (the code path that sets the key in
verify_signature_with_pem_key) and therefore skips parsing/validating
`<KeyInfo>`—remove or mark step 2 as "skipped for verify_signature_with_pem_key"
and add a short note in the verify_signature_with_pem_key() doc comment
clarifying that this API bypasses `<KeyInfo>` processing and uses the provided
PEM key directly.
- Around line 631-635: The code currently unconditionally calls parse_key_info
when ctx.key is None (using ctx.key, signature_children.key_info_node and
parse_key_info), which hard-fails even for resolver-backed verification that
doesn't consume KeyInfo; change this so parsing is opt-in or the parsed
structure is passed into the resolver: either (A) only call parse_key_info when
the selected KeyResolver indicates it will consume document KeyInfo (modify the
resolver selection flow to query a capability flag on KeyResolver), or (B)
change the KeyResolver API to accept an Option<ParsedKeyInfo> (introduce a
ParsedKeyInfo type produced by parse_key_info and thread it into the resolver
call where KeyResolver is invoked around the code that currently uses
KeyResolver at the 903-913 site), and remove the unconditional parse_key_info
call so failures only occur when the resolver actually consumes the parsed
KeyInfo.
---
Outside diff comments:
In `@src/xmldsig/parse.rs`:
- Around line 539-552: In base64_decode_digest, prevent oversized allocations by
computing the maximum allowed base64 text length from the expected decoded size:
let cap = ((digest_method.output_len() + 2) / 3) * 4; allocate the normalized
buffer with capacity cap (or min(cap, b64.len())), call
normalize_xml_base64_text(b64, &mut cleaned) as before, and immediately fail
with a ParseError::Base64 if cleaned.len() > cap (so the function returns before
calling STANDARD.decode); update the error message to clearly indicate
"DigestValue exceeds maximum allowed base64 length" and reference function names
base64_decode_digest, normalize_xml_base64_text, and type DigestAlgorithm to
locate 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: d0141ed7-7784-4822-8d09-8d17bd2c6dd7
📒 Files selected for processing (4)
src/xmldsig/mod.rssrc/xmldsig/parse.rssrc/xmldsig/verify.rssrc/xmldsig/whitespace.rs
- update PEM verification docs to match explicit-key KeyInfo behavior - add KeyResolver capability flag for document KeyInfo consumption - bound DigestValue base64 length before decode and add regressions
|
@coderabbitai Re: outside-diff finding on |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
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/parse.rs`:
- Around line 543-556: The DigestValue base64 length check must run while
streaming text children instead of after building the full string to avoid
unbounded allocation; update parse_reference (or add a helper like
decode_digest_value_children) to iterate node.children(), reject element
children, call normalize_xml_base64_text on each text chunk into a single String
buffer preallocated with max_base64_len (computed from
digest_method.output_len().div_ceil(3) * 4), and after each append check
cleaned.len() against max_base64_len returning ParseError::Base64(...)
immediately if exceeded; keep the existing base64 decode and decoded-length
checks afterwards and use the same error messages.
🪄 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: 0fcd043c-a4cd-4924-adf7-27936dcb5e18
📒 Files selected for processing (2)
src/xmldsig/parse.rssrc/xmldsig/verify.rs
- decode DigestValue from node children without building an unbounded intermediate string - enforce normalized base64 cap while streaming DigestValue chunks - remove expect-based ASCII conversion in whitespace normalizer
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Summary
Details
src/xmldsig/parse.rssrc/xmldsig/mod.rs<KeyInfo>children are ignored in lax modeds:*/dsig11:*) inside<X509Data>are rejected fail-closed<X509Data>remains accepted (lax), while malformed XMLDSig-owned children are rejectedECKeyValuedispatch is namespace-strict: onlydsig11:ECKeyValueis treated as supported, whileds:ECKeyValueis preserved as unsupportedparse.rsandverify.rsValidation
cargo fmt -- --checkcargo clippy --all-targets --all-features -- -D warningscargo nextest run --workspacecargo test --doc --workspaceCloses #46