test(xmldsig): add donor full verification suite accounting#41
Merged
Conversation
- add P1-025 integration suite with deterministic pass/fail/skip tracking - validate aleksey RSA/ECDSA vectors through verify pipeline - record explicit skip reasons for deferred merlin DSA/KeyInfo/X509 vectors Closes #40
|
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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a donor XMLDSig integration test tracking pass/fail/skip counts, introduces seven Merlin donor XMLDSig fixtures, updates fixture inventory expectations, replaces the Changes
Sequence Diagram(s)sequenceDiagram
participant Test Runner
participant Fixture
participant Verify Pipeline
participant RustCrypto
Test Runner->>Fixture: load XML vector (+ optional PEM)
Test Runner->>Verify Pipeline: verify_signature_with_pem_key(xml, key) / VerifyContext::verify(xml)
Verify Pipeline->>Fixture: parse SignedInfo / References / KeyInfo
Verify Pipeline->>RustCrypto: compute digest (SHA-1/SHA-2)
Verify Pipeline->>RustCrypto: parse SPKI / prepare verifying key
Verify Pipeline->>RustCrypto: verify signature (RSA PKCS#1 v1.5 or ECDSA)
RustCrypto-->>Verify Pipeline: verification result / parse errors
Verify Pipeline-->>Test Runner: Ok(Valid)/Ok(Invalid(reason))/Err(Unsupported)
Test Runner->>Test Runner: increment passed/failed/skipped and record diagnostics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 integration test suite for ROADMAP task P1-025 to run end-to-end XMLDSig verification against donor vectors while tracking deterministic pass/fail/skip accounting.
Changes:
- Introduces a new integration test enumerating donor vector cases with expected outcomes (valid vs explicit skip).
- Runs supported Aleksey RSA/ECDSA vectors through
xmldsig::verify_signature_with_pem_keyand asserts overall validity. - Records skip reasons for deferred Merlin vectors and asserts expected pass/skip counts.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/donor_full_verification_suite.rs`:
- Around line 137-139: The test eagerly calls read_fixture before matching on
case.expectation, causing Skip cases to attempt filesystem reads; move the
read_fixture call inside the match so only the ValidWithKey (or other variants
that actually need the fixture) arm(s) call read_fixture. Specifically, in the
loop over cases() defer calling read_fixture(&root.join(case.xml_path)) until
inside the match for Expectation::ValidWithKey (and any other expectation
variants that require the XML), leaving Skip arms to avoid filesystem access and
simply count as skipped.
🪄 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: a2c9b5fc-5390-4910-af75-81205b2b3a8a
📒 Files selected for processing (1)
tests/donor_full_verification_suite.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/donor_full_verification_suite.rs`:
- Around line 163-167: The test assertion uses assert!(failed.is_empty(), ...)
which should be replaced with assert_eq!(failed.len(), 0, ...) to provide
structured failure output; update the assertion referencing the failed variable
(the existing assert block that prints failed.join("\n")) to use
assert_eq!(failed.len(), 0, "donor full verification suite had failures:\n{}",
failed.join("\n")) so failure counts and messages are clearer.
🪄 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: 275bf27e-5f2c-4941-a1a9-d011af2d416b
📒 Files selected for processing (1)
tests/donor_full_verification_suite.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/donor_full_verification_suite.rs`:
- Around line 16-20: The VectorCase struct's xml_path field should be changed
from Option<&'static str> to &'static str to enforce the invariant at compile
time; update the struct definition (VectorCase { xml_path: &'static str, ... }),
adjust every VectorCase construction to supply the fixture string instead of
Some(...), and remove any runtime .expect(...) / match arms that handle None
(use xml_path directly in the test loop and in helper calls like the XML-loading
logic). Also update other places that referenced xml_path as Option (e.g., where
instances are created around the other noted cases) so they pass plain &'static
str 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: dfd8e466-5690-48a1-9ce6-295af073a94a
📒 Files selected for processing (9)
tests/donor_full_verification_suite.rstests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloped-dsa.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloping-rsa.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-x509-crt-crl.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-x509-crt.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-x509-is.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-x509-ski.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-x509-sn.xmltests/fixtures_smoke.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 `@tests/donor_full_verification_suite.rs`:
- Around line 164-166: Replace the loose length-only check for skipped entries
with an exact comparison: build an expected_skipped Vec (or slice) of the exact
"case.name: reason" strings you expect and assert equality against the collected
skipped Vec (or, if ordering is not stable, sort both before comparing); update
the code that pushes format!("{}: {}", case.name, reason) in the
Expectation::Skip arm (symbols: Expectation::Skip, skipped, case.name, reason,
read_fixture) and replace the later skipped.len() assertion with
assert_eq!(skipped, expected_skipped) (or assert_eq!(sorted_skipped,
sorted_expected_skipped)) so failures show the concrete missing/extra entries
and reasons; apply the same change to the other occurrence referenced around the
later block.
- Around line 70-74: The test entry currently marks the vector
"aleksey-ecdsa-p521-sha384" as Expectation::Skip which incorrectly removes it
from the pass set; change that Expectation to include the vector in the passing
set (e.g., replace Expectation::Skip { ... } with Expectation::ValidWithKey {
... } or the appropriate passing variant used in this suite) so the Aleksey
P-521 ECDSA-SHA384 fixture is exercised and counted as passed, and if you
instead intend to narrow Aleksey coverage, update the suite’s claimed
coverage/summary to exclude P-521 rather than leaving the specific name skipped;
locate the entry by the name "aleksey-ecdsa-p521-sha384" and the
Expectation::Skip construct to apply 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: 81b4231c-235e-4c0f-8e93-15e1bc19c751
📒 Files selected for processing (9)
tests/donor_full_verification_suite.rstests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloped-dsa.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloping-rsa.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-x509-crt-crl.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-x509-crt.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-x509-is.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-x509-ski.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-x509-sn.xmltests/fixtures_smoke.rs
polaz
commented
Apr 3, 2026
- replace ring digests/signature verification with RustCrypto crates - add ECDSA P-521 (ecdsa-sha384) verification path and donor fixture coverage - update integration tests and fixture inventory to match new crypto backend
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
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/donor_full_verification_suite.rs`:
- Around line 165-178: The test's Skip arm calls
verify_signature_with_pem_key(&xml, &skip_probe_key, false), which injects an
explicit PEM key and bypasses the KeyValue/X.509 resolution the skip is supposed
to exercise; instead call the real verification entry that does not supply an
explicit key (the resolver path in src/xmldsig/verify.rs) so the KeyValue/X.509
resolution logic runs (replace the verify_signature_with_pem_key call with the
verification function that performs normal key resolution), then assert the
concrete outcome expected by the skip reason (e.g., specific Err or DsigStatus
result) rather than accepting any Err/Invalid as a successful skip; update the
Expectation::Skip arm in tests/donor_full_verification_suite.rs to use the
non-injecting verifier and concrete assertions.
🪄 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: 950279ea-65a1-4ed0-b618-7e1329c36091
📒 Files selected for processing (2)
src/xmldsig/signature.rstests/donor_full_verification_suite.rs
polaz
added a commit
that referenced
this pull request
Apr 4, 2026
- map manifest reference parse failures to explicit DsigError variant - track reference origin/index for SignedInfo vs Manifest processing - process only direct ds:Manifest children under ds:Object Closes #41
polaz
added a commit
that referenced
this pull request
Apr 4, 2026
- reject non-whitespace mixed text in ds:Manifest - return manifest-specific InvalidStructure for empty manifest children - explicitly handle MissingUri in manifest policy precheck - add regression tests for mixed content and empty manifest Closes #41
polaz
added a commit
that referenced
this pull request
Apr 4, 2026
- clarify process_manifests docs to require ID-based same-document references - document whole-document URI references as non-signing for direct child selection - add regression test for SignedInfo reference to ds:Object with nested Manifest without ID Refs #41
polaz
added a commit
that referenced
this pull request
Apr 4, 2026
…-processing feat(xmldsig): process manifest references
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
Testing
Closes #40