feat(xmldsig): add reference verification pipeline#20
Merged
Conversation
|
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
WalkthroughImplements XMLDSig Reference verification: adds digest algorithms, strict SignedInfo/Reference parsing, transform execution (including xmlsec-compatible XPath), URI dereferencing, digest computation and constant-time comparison, plus donor-based and integration tests and CI/toolchain updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Parser as Parse
participant Resolver as UriResolver
participant Transform as Transforms
participant Digest as Digest
participant Verify as Verify
Client->>Parser: parse_signed_info(xml)
Parser-->>Client: SignedInfo + References[]
loop for each Reference
Client->>Resolver: dereference(uri)
Resolver-->>Client: bytes
Client->>Transform: execute_transforms(bytes, transforms)
Transform-->>Client: transformed_bytes
Client->>Digest: compute_digest(algorithm, transformed_bytes)
Digest-->>Client: digest_bytes
Client->>Verify: process_reference(ref, resolver)
Verify->>Digest: compute_digest(...)
Verify->>Verify: constant_time_eq(computed, stored)
Verify-->>Client: ReferenceResult { valid, uri, digest_algorithm, pre_digest? }
end
Client->>Verify: process_all_references(all_refs, resolver)
Verify-->>Client: ReferencesResult { all_valid, first_failure }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an XMLDSig <Reference> verification pipeline to the xmldsig module, including strict parsing, transform execution, digest computation, and end-to-end validation against real xmlsec1 donor vectors.
Changes:
- Introduces new
xmldsigmodules for digest computation, SignedInfo/Reference parsing, and reference verification (process_reference/process_all_references). - Extends transform parsing to accept a narrow xmlsec1-compatible XPath transform that is treated as enveloped-signature.
- Adds integration tests plus donor-vector tests covering SHA-1/256/384/512 reference digest verification.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
Cargo.toml |
Moves base64 into regular dependencies for runtime parsing/digest support. |
src/xmldsig/digest.rs |
Adds SHA-family digest computation + constant-time digest comparison helpers. |
src/xmldsig/parse.rs |
Adds strict parsing for <Signature>/<SignedInfo>/<Reference> including base64 digest decode. |
src/xmldsig/transforms.rs |
Adds narrow XPath-compat parsing and maps it to the enveloped transform. |
src/xmldsig/verify.rs |
Implements reference processing pipeline and fail-fast aggregation results. |
src/xmldsig/mod.rs |
Exposes the new XMLDSig modules and re-exports key APIs/types. |
tests/reference_integration.rs |
Adds integration coverage for reference processing behavior (enveloped, subtree URIs, fail-fast, algo variants). |
tests/donor_digest_vectors.rs |
Adds xmlsec1 donor-vector digest verification tests across SHA families. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/digest.rs`:
- Around line 104-106: Replace the #[allow(deprecated)] on the constant_time_eq
function with #[expect(deprecated)] so the deprecation suppression becomes an
expect check; locate the constant_time_eq function that calls
ring::constant_time::verify_slices_are_equal and change the attribute to
#[expect(deprecated)] (preserving any existing behavior and return value).
In `@src/xmldsig/transforms.rs`:
- Around line 175-176: The compat XPath expression that matches
"not(ancestor-or-self::dsig:Signature)" must not be treated as
Transform::Enveloped because it excludes all dsig:Signature subtrees rather than
only the containing Signature; update parse_xpath_compat_transform so it returns
a distinct transform variant (e.g., Transform::XpathCompat or
Transform::XPathExcludeAllSignatures) instead of mapping to
Transform::Enveloped, add that new enum variant to Transform, and update any
match arms that special-case Transform::Enveloped (including wherever XPATH_URI
is parsed and later transform application code) to handle the new variant
correctly (or explicitly reject it where only the narrow donor-vector
enveloped-case is allowed). Ensure code paths that compare or apply transforms
use the new variant rather than collapsing into Enveloped, and include tests to
assert the two behaviors differ.
In `@src/xmldsig/verify.rs`:
- Around line 69-73: The code currently treats a missing Reference.uri as "" and
calls resolver.dereference, which conflates an omitted-URI (meaning the caller
must supply the signed object) with the null same-document URI; change this to
fail closed: detect when reference.uri is None and return an error instead of
using unwrap_or(""), e.g. add or use a ReferenceProcessingError variant like
MissingUri (or a suitably named variant) and return
Err(ReferenceProcessingError::MissingUri) when reference.uri.is_none(); only
call resolver.dereference(uri) when reference.uri is Some and pass the exact
dereferenced string.
In `@tests/donor_digest_vectors.rs`:
- Around line 15-18: The tests currently hard-code the gitignored donor corpus
in donor_xml, which causes panics on clean checkouts; either vendor the four
XMLs under a tracked tests/fixtures/ directory and update donor_xml to read from
that new path (replace Path::new("donors/...") with
Path::new("tests/fixtures/...")), or mark the donor-dependent integration tests
(the ones calling donor_xml around lines 50-67) with #[ignore = "requires donor
corpus"] so they are skipped by default; update the donor_xml function and the
test attributes accordingly to implement one of these two fixes.
In `@tests/reference_integration.rs`:
- Around line 384-385: The test function reference_with_inclusive_c14n should be
renamed to an explicit expected-outcome name (e.g.,
reference_with_inclusive_c14n_valid or inclusive_c14n_reference_verifies) to
follow naming convention; update the function declaration (fn
reference_with_inclusive_c14n -> fn reference_with_inclusive_c14n_valid) and
adjust any references to that symbol (calls, documentation, or CI/test filters)
so the test still runs under the new name.
- Around line 441-442: Rename the test function reference_with_sha384_digest to
a name that encodes the expected outcome (e.g., sha384_digest_reference_verifies
or reference_with_sha384_digest_valid) so the purpose is clear; update the
function declaration fn reference_with_sha384_digest() { to the chosen name and
make sure any references to this test symbol (such as module-level test listings
or documentation comments) are updated accordingly.
- Around line 558-559: The test function name
process_single_reference_with_pre_digest is ambiguous; rename the test to a more
descriptive name (e.g., process_single_reference_with_pre_digest_valid or
process_reference_stores_pre_digest_data) to express the expected outcome;
update the test function identifier process_single_reference_with_pre_digest
accordingly and adjust any references or test annotations so the new name
compiles and runs.
- Around line 142-143: The test function name
saml_assertion_level_enveloped_reference should be renamed to
saml_assertion_level_enveloped_reference_valid to follow the
<what>_<condition>_<expected>() guideline; update the function identifier in the
test declaration and any references (e.g., module tests or test filters) that
refer to saml_assertion_level_enveloped_reference so they use
saml_assertion_level_enveloped_reference_valid instead.
🪄 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: 5a4d4a0f-7216-4261-8df9-2ababe5e6444
📒 Files selected for processing (9)
Cargo.tomlsrc/xmldsig/digest.rssrc/xmldsig/mod.rssrc/xmldsig/parse.rssrc/xmldsig/transforms.rssrc/xmldsig/verify.rstests/c14n_golden.rstests/donor_digest_vectors.rstests/reference_integration.rs
- add digest, SignedInfo parsing, and reference verification modules - cover donor digest vectors and XPath-compatible enveloped parsing - expand XMLDSig integration tests for end-to-end reference processing Closes #19
- switch the crate manifest to Rust 2024 with rust-version 1.85 - apply the edition-driven return-position impl Trait syntax update Refs #19
- pin the default toolchain to Rust 1.94.0 - raise rust-version to 1.92 and add an MSRV CI job - run CI tests with cargo nextest instead of cargo test Refs #19
- require exactly one ds:XPath child in the XMLDSig namespace - reject extra element children and duplicate XPath nodes Refs #19
polaz
force-pushed
the
test/#19-donor-digest-vectors
branch
from
bbfa338 to
ee31e62
Compare
- split compat XPath semantics from enveloped-signature - fail closed on omitted reference URI and vendor donor fixtures - rename integration tests to encode expected outcomes
- remove implicit From mapping from URI dereference errors - reject compat XPath when dsig prefix is bound outside XMLDSig
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/mod.rs`:
- Around line 18-27: The pub use lines in mod.rs are not formatted in rustfmt's
expected ordering; run cargo fmt (or rustfmt) to reorder the imports so they
match project style/lexicographic order—for example adjust the grouped exports
around compute_digest, constant_time_eq, DigestAlgorithm, find_signature_node,
parse_signed_info, ParseError, Reference, SignatureAlgorithm, SignedInfo,
execute_transforms, parse_transforms, Transform, NodeSet, TransformData,
TransformError, process_all_references, process_reference,
ReferenceProcessingError, ReferenceResult, and ReferencesResult so rustfmt
passes CI.
In `@src/xmldsig/parse.rs`:
- Around line 71-74: The public method signing_allowed() should be annotated
with #[must_use] to prevent callers from accidentally ignoring its boolean
result (which enforces the RSA-SHA1 restriction); update the pub fn
signing_allowed(self) -> bool signature by adding the #[must_use] attribute
above the function (leave the implementation using matches!(self, Self::RsaSha1)
unchanged) so any discard of the return value will trigger a compiler warning.
- Around line 136-143: The public function find_signature_node returns an Option
that callers may accidentally ignore; add the #[must_use] attribute to its
declaration to warn users when its return value is discarded. Locate the
find_signature_node function and annotate it with #[must_use] immediately above
pub fn find_signature_node<'a>(doc: &'a Document<'a>) -> Option<Node<'a, 'a>> so
the compiler emits a warning if callers do not use the Option result.
- Around line 293-294: The import ordering violates rustfmt alphabetical rules:
move the `use base64::Engine;` line so it appears before `use
base64::engine::general_purpose::STANDARD;` (i.e., reorder the two `use`
statements referencing `base64` so `Engine` precedes
`engine::general_purpose::STANDARD`).
In `@src/xmldsig/verify.rs`:
- Around line 162-166: The imports inside the tests module are not ordered
according to rustfmt; reorder the use statements (for DigestAlgorithm,
parse_signed_info, Reference, Transform, UriReferenceResolver, and Document)
into the canonical rustfmt order (group extern crates, then crate::... items,
alphabetize within groups) so the tests module passes CI formatting
checks—adjust the use declarations in the tests module of src/xmldsig/verify.rs
accordingly.
- Around line 42-47: The public method ReferencesResult::all_valid() should be
annotated with #[must_use] so callers cannot accidentally ignore its boolean
result; update the impl for ReferencesResult by adding the #[must_use] attribute
directly above the pub fn all_valid(&self) -> bool declaration and keep the
method body unchanged so callers receive a compiler warning if they discard the
returned bool.
- Line 14: The import list in verify.rs is not sorted for rustfmt/CI — reorder
the items in the use super::digest::{...} statement so symbols are alphabetized
(put DigestAlgorithm before compute_digest, and ensure constant_time_eq is
placed in correct alphabetical position) so the line becomes alphabetically
ordered among compute_digest, constant_time_eq, and DigestAlgorithm; update that
single use line accordingly.
In `@tests/c14n_golden.rs`:
- Line 199: Run rustfmt (cargo fmt) to fix formatting and resolve the import
ordering drift; specifically, sort and group the use/import statements around
the top of the file (the block near the subtree_predicate definition) into
canonical alphabetical order and remove the stray "+ use<>" token from the fn
subtree_predicate signature so it reads a valid Rust signature (e.g., fn
subtree_predicate(root: roxmltree::Node) -> impl Fn(roxmltree::Node) -> bool).
After formatting, re-run CI or cargo fmt -- --check to confirm the file is
clean.
In `@tests/reference_integration.rs`:
- Around line 13-18: The imports in tests/reference_integration.rs are
mis-ordered for rustfmt; run cargo fmt or reorder the use statements so they
follow rustfmt's canonical ordering (e.g. group and alphabetize the
base64::Engine import and the xml_sec::xmldsig::* imports such as
digest::{compute_digest, DigestAlgorithm}, parse::{find_signature_node,
parse_signed_info}, transforms::execute_transforms, uri::UriReferenceResolver,
and verify::{process_all_references, process_reference}) to satisfy CI.
🪄 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: 5e61951d-6e97-4884-a20d-d1851a9e780f
📒 Files selected for processing (18)
.github/workflows/ci.yml.github/workflows/release.ymlCargo.tomlREADME.mdrust-toolchain.tomlsrc/xmldsig/digest.rssrc/xmldsig/mod.rssrc/xmldsig/parse.rssrc/xmldsig/transforms.rssrc/xmldsig/verify.rstests/c14n_golden.rstests/donor_digest_vectors.rstests/fixtures/xmldsig/aleksey-xmldsig-01/enveloped-sha1-rsa-sha1.xmltests/fixtures/xmldsig/aleksey-xmldsig-01/enveloped-sha256-ecdsa-sha256.xmltests/fixtures/xmldsig/aleksey-xmldsig-01/enveloped-sha384-ecdsa-sha384.xmltests/fixtures/xmldsig/aleksey-xmldsig-01/enveloped-x509-digest-sha512.xmltests/fixtures_smoke.rstests/reference_integration.rs
- normalize formatter-driven import ordering and layout - collapse clippy-reported nested if chains in c14n and xmldsig - keep workspace build, fmt, clippy, and nextest clean
- annotate SignatureAlgorithm::signing_allowed with #[must_use] - prevent callers from silently discarding the signing policy check
- annotate find_signature_node with #[must_use] - surface discarded signature lookup results to callers
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/transforms.rs (1)
173-212:⚠️ Potential issue | 🟠 MajorReject empty
<ds:Transforms>instead of treating it as “no transforms”.If
<Transforms>is present but contains zero<Transform>children, this returnsOk(vec![]), andexecute_transforms()later falls back to default inclusive C14N. That turns malformed references into no-transform references instead of failing closed.🔧 Suggested fix
for child in transforms_node.children() { if !child.is_element() { continue; } @@ }; chain.push(transform); } + if chain.is_empty() { + return Err(TransformError::UnsupportedTransform( + "<ds:Transforms> must contain at least one <ds:Transform> child".into(), + )); + } + Ok(chain)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/transforms.rs` around lines 173 - 212, The parser currently returns Ok(vec![]) when a <ds:Transforms> element contains no <Transform> children, which causes execute_transforms() to treat it as "no transforms" instead of failing closed; after the loop that builds the local variable chain (the Vec named chain that iterates transforms_node.children()), add a check that if chain.is_empty() you return an Err(TransformError::UnsupportedTransform(...)) indicating an empty <Transforms> element (refer to transforms_node, chain, and the TransformError::UnsupportedTransform variant), otherwise continue to return Ok(chain).
🤖 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 156-165: parse_signed_info currently only reads the
CanonicalizationMethod URI (via required_algorithm_attr) and loses any
ec:InclusiveNamespaces PrefixList child; update the CanonicalizationMethod
handling (around c14n_node / verify_ds_element / required_algorithm_attr) to
also look for a child element named "InclusiveNamespaces" (namespace/ec prefix
as used in verify_ds_element) and extract its PrefixList attribute, then
construct the C14nAlgorithm with those inclusive prefixes (either by calling a
new/overloaded constructor on C14nAlgorithm or passing the prefixes into
C14nAlgorithm::from_uri so the returned variant for Exclusive C14N preserves the
prefixes). If the InclusiveNamespaces child exists but the algorithm URI is not
an Exclusive C14N type, return a ParseError::UnsupportedAlgorithm or a new
ParseError variant. Ensure you reference c14n_node, required_algorithm_attr,
C14nAlgorithm::from_uri (or its new signature) and parse_signed_info in your
changes.
In `@src/xmldsig/transforms.rs`:
- Around line 247-255: The code currently collapses all ASCII whitespace in the
XPath literal by using text.split_whitespace().collect::<String>(), which allows
malformed inputs to normalize to ENVELOPED_SIGNATURE_XPATH_EXPR; change the
normalization to only trim leading/trailing XML whitespace instead (use
text.trim().to_string() from the xpath_node.text() result) so interior
whitespace remains significant, then continue to compare expr ==
ENVELOPED_SIGNATURE_XPATH_EXPR and check the dsig namespace via
xpath_node.lookup_namespace_uri(Some("dsig")) == Some(XMLDSIG_NS_URI) before
returning Transform::XpathExcludeAllSignatures.
---
Outside diff comments:
In `@src/xmldsig/transforms.rs`:
- Around line 173-212: The parser currently returns Ok(vec![]) when a
<ds:Transforms> element contains no <Transform> children, which causes
execute_transforms() to treat it as "no transforms" instead of failing closed;
after the loop that builds the local variable chain (the Vec named chain that
iterates transforms_node.children()), add a check that if chain.is_empty() you
return an Err(TransformError::UnsupportedTransform(...)) indicating an empty
<Transforms> element (refer to transforms_node, chain, and the
TransformError::UnsupportedTransform variant), otherwise continue to return
Ok(chain).
🪄 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: 302a391c-db02-4872-9520-6da89c1719bb
📒 Files selected for processing (14)
src/c14n/mod.rssrc/c14n/ns_exclusive.rssrc/c14n/ns_inclusive.rssrc/c14n/serialize.rssrc/c14n/xml_base.rssrc/xmldsig/mod.rssrc/xmldsig/parse.rssrc/xmldsig/transforms.rssrc/xmldsig/verify.rstests/c14n_golden.rstests/c14n_integration.rstests/reference_integration.rstests/transforms_integration.rstests/uri_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/parse.rs`:
- Around line 46-57: Add the #[must_use] attribute to the public function
from_uri so callers are warned if they discard its Option<Self> result; update
the signature of the impl method from_uri (the XML algorithm URI parser) to be
annotated with #[must_use] to enforce the lint and prevent silent ignores of
unsupported algorithm lookups.
🪄 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: dfec09cd-fbc6-4fc1-9ea1-45a2ef7b0128
📒 Files selected for processing (2)
src/xmldsig/parse.rssrc/xmldsig/transforms.rs
- run doc tests alongside nextest in CI - mark SignatureAlgorithm::from_uri as must_use - remove the empty dev-dependencies section
- keep stable and MSRV validation in matrix jobs - add top-level build and test aggregator jobs for branch protection
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 60-70: The public method uri() should be annotated with
#[must_use] so callers don't accidentally ignore its &'static str return; add
the #[must_use] attribute above the impl method named uri() in the same enum
where from_uri() and signing_allowed() live to match their annotations and
ensure the returned namespace URI isn't discarded by callers.
🪄 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: 2feb73fe-3af5-44b5-8f7c-e8a54dd151c6
📒 Files selected for processing (7)
.github/workflows/ci.ymlCargo.tomlREADME.mdrust-toolchain.tomlsrc/xmldsig/parse.rssrc/xmldsig/transforms.rstests/reference_integration.rs
- add must_use to SignatureAlgorithm::uri for API consistency - keep parse API warnings aligned across xmldsig algorithm helpers
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
buildandteston bothstableand1.92.0Technical Details
base64to regular dependencies for runtime digest and parsing supportInclusiveNamespaces PrefixListfor SignedInfo exclusive C14N, enforce digest-length invariants during parsing, and keep the compat XPath path fail-closedrust-toolchain.tomlfor the default stable developer/compiler targetTest Plan
cargo +stable nextest run --workspacecargo +stable buildcargo +1.92.0 buildCloses #19