feat(xmldsig): resolve X509 selectors#72
Conversation
- Match configured certificates by subject, issuer/serial, SKI, and digest - Verify supported donor vectors through DefaultKeyResolver - Add reproducible donor fixture import and validated chain coverage Closes #71
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds selector-based X.509 certificate resolution in the parser and key resolver, updates donor verification to use embedded, selected, and chain-based certificate paths, and adds fixture tooling, a new chain fixture, count updates, and README wording. ChangesSelector-based X.509 resolution and donor verification
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/import-donor-fixtures.sh (1)
1-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueScript name implies bulk import but only copies one hardcoded file.
import-donor-fixtures.sh(plural) currently hardcodes a single file copy. Fine for now, but if this is meant to be the reusable importer referenced by the PR objective ("adds a reproducible donor fixture importer"), consider parameterizing it (e.g., accept a relative path argument, or loop over a list) so future donor fixtures don't require a near-duplicate script.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/import-donor-fixtures.sh` around lines 1 - 11, The import-donor-fixtures.sh script currently behaves like a one-off copy instead of a reusable importer, since it hardcodes a single fixture path in the install step. Update the script around repo_root, donor_root, fixture_root, and the install logic so it can take a relative path argument or iterate over a list of donor fixture files, allowing additional fixtures to be imported without duplicating the script.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/xmldsig/keys.rs`:
- Around line 231-263: The configured selector-chain handling in the key
resolution logic is too strict because `x509_certificate_matches_selectors` only
accepts one certificate matching all selector categories, so valid leaf/issuer
combinations can be missed. Update the certificate-selection path in the
trusted-certs loop to collect configured certificates matching any selector
category, ensure each selector category is represented across that set, and then
choose the unique leaf certificate from those matches. Keep the ambiguity and
invalid-certificate handling via `KeyResolutionError` consistent with the
existing `parse_x509_certificate` and `matches/leaves` flow.
In `@tests/donor_full_verification_suite.rs`:
- Around line 257-260: The donor full verification test is relying on the
current system time when `DefaultKeyResolver::new` is created with
`verify_chains: true`, which makes fixture validation time-dependent. Update the
`KeyResolverConfig` in this test to set `verification_time: Some(...)` to a
fixed instant that falls within the donor certificate chain validity window so
the verification remains deterministic.
- Around line 115-119: The donor_full_verification_suite coverage is missing the
TOFU/default-resolver verification for the same xml fixture, so add a new
VectorCase alongside the existing ValidChain case that exercises embedded chain
validation for enveloping-rsa-x509chain.xml without a trust anchor. Use the
existing test data and naming patterns in donor_full_verification_suite.rs, and
make sure the new case targets the verification path that uses the default
resolver/TOFU behavior rather than the anchored Expectation::ValidChain branch.
---
Outside diff comments:
In `@scripts/import-donor-fixtures.sh`:
- Around line 1-11: The import-donor-fixtures.sh script currently behaves like a
one-off copy instead of a reusable importer, since it hardcodes a single fixture
path in the install step. Update the script around repo_root, donor_root,
fixture_root, and the install logic so it can take a relative path argument or
iterate over a list of donor fixture files, allowing additional fixtures to be
imported without duplicating the script.
🪄 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 Plus
Run ID: eaad40ac-3249-44a7-91f9-d49609594c28
📒 Files selected for processing (7)
README.mdscripts/import-donor-fixtures.shsrc/xmldsig/keys.rssrc/xmldsig/parse.rstests/donor_full_verification_suite.rstests/fixtures/xmldsig/aleksey-xmldsig-01/enveloping-rsa-x509chain.xmltests/fixtures_smoke.rs
|
@coderabbitai Re: reusable donor importer — fixed. The script now accepts relative fixture paths, defaults to the current chain fixture, creates destination directories, and was validated in both default and explicit-argument modes. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
fd import-donor-fixtures.sh
echo "---"
cat scripts/import-donor-fixtures.shLength of output: 607
#!/usr/bin/env bash
set -euo pipefail
repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
donor_root="$repo_root/donors/xmlsec/tests"
fixture_root="$repo_root/tests/fixtures/xmldsig"
install -m 0644 \
"$donor_root/aleksey-xmldsig-01/enveloping-rsa-x509chain.xml" \
"$fixture_root/aleksey-xmldsig-01/enveloping-rsa-x509chain.xml"There's no argument parsing, default-to-current-fixture logic, or destination directory creation ( (੭ ｀ ᵥ ´ )੭ ⁼³₌₃ puzzled bunny scratches head |
There was a problem hiding this comment.
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)
996-1039: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire every selector value to be represented, not just every category.
Line 999, Line 1006, Line 1018, and Line 1026 currently make each category pass if any selector in that category matches a chain member. That lets mixed data like one matching
X509Digestplus one non-matchingX509Digeststill resolve, which violates the fail-closed selector behavior required here.Proposed direction
- let subject_match = info.subject_names.is_empty() - || info.parsed_certificates.iter().any(|certificate| { - info.subject_names - .iter() - .any(|subject| subject.trim() == certificate.subject_dn) - }); + let subject_match = info.subject_names.iter().all(|subject| { + info.parsed_certificates + .iter() + .any(|certificate| subject.trim() == certificate.subject_dn) + }); - let ski_match = info.skis.is_empty() - || info.parsed_certificates.iter().any(|certificate| { - certificate - .subject_key_identifier - .as_ref() - .is_some_and(|certificate_ski| info.skis.iter().any(|ski| ski == certificate_ski)) - }); + let ski_match = info.skis.iter().all(|ski| { + info.parsed_certificates.iter().any(|certificate| { + certificate + .subject_key_identifier + .as_ref() + .is_some_and(|certificate_ski| ski == certificate_ski) + }) + });Apply the same
all(selector) -> any(certificate)pattern to issuer/serial and digest selectors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/xmldsig/parse.rs` around lines 996 - 1039, The selector matching logic in x509_selector_categories_match_chain is too permissive because each category currently succeeds when any single selector in that category matches; update the subject, issuer/serial, SKI, and digest checks so every provided selector value must be represented by at least one parsed certificate, using an all(selector) over the selector lists rather than category-level any() short-circuiting. Keep the existing ParseError handling and locate the changes within x509_selector_categories_match_chain in parse.rs, especially the subject_names, issuer_serials, skis, and digests branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/xmldsig/parse.rs`:
- Around line 996-1039: The selector matching logic in
x509_selector_categories_match_chain is too permissive because each category
currently succeeds when any single selector in that category matches; update the
subject, issuer/serial, SKI, and digest checks so every provided selector value
must be represented by at least one parsed certificate, using an all(selector)
over the selector lists rather than category-level any() short-circuiting. Keep
the existing ParseError handling and locate the changes within
x509_selector_categories_match_chain in parse.rs, especially the subject_names,
issuer_serials, skis, and digests branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a186001e-957a-4009-adca-3fa3e448a0fb
📒 Files selected for processing (4)
scripts/import-donor-fixtures.shsrc/xmldsig/keys.rssrc/xmldsig/parse.rstests/donor_full_verification_suite.rs
|
@coderabbitai Re: require every selector value to be represented — fixed. Chain-wide matching now applies all(selector) -> any(certificate) independently to SubjectName, IssuerSerial, SKI, and Digest. Added a failing-first regression proving a valid SubjectName cannot mask a bogus SubjectName; donor multi-digest configuration now supplies the complete asserted chain. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
fd parse.rs -p src/xmldsig
echo "---"
rg -n "x509_selector_categories_match_chain|x509_certificate_matches_any_selector|x509_data_has_lookup_identifiers" src/xmldsig/parse.rs
echo "---outline---"
ast-grep run --pattern 'fn x509_selector_categories_match_chain($$$) { $$$ }' --lang rust src/xmldsig/parse.rs
echo "---func body any selector---"
ast-grep run --pattern 'fn x509_certificate_matches_any_selector($$$) { $$$ }' --lang rust src/xmldsig/parse.rs
echo "---test regression---"
rg -n "cannot mask|bogus" src/xmldsig/parse.rs tests -nLength of output: 1500 🏁 Script executed: #!/bin/bash
sed -n '860,1050p' src/xmldsig/parse.rs
echo "---search regression test across repo---"
rg -n "mask|SubjectName" src/xmldsig/keys.rs -i | head -50
echo "---git log---"
git log --oneline -15Length of output: 8623 🏁 Script executed: #!/bin/bash
rg -n "resolve_configured_x509|category|all(" src/xmldsig/keys.rs | head -60
echo "---diff for bf0be3a---"
git show bf0be3a --stat
echo "---"
git show bf0be3a -- src/xmldsig/keys.rs | head -150Length of output: 3417 🏁 Script executed: #!/bin/bash
git show 422ce01 --stat
echo "---"
rg -n "cannot mask|SubjectName.*bogus|bogus.*SubjectName" src/xmldsig/keys.rs
echo "---search test names---"
rg -n "fn .*selector.*chain|fn .*chain.*selector|fn .*mask" src/xmldsig/keys.rsLength of output: 803 🏁 Script executed: #!/bin/bash
sed -n '690,760p' src/xmldsig/keys.rsLength of output: 3365 🏁 Script executed: #!/bin/bash
rg -n "X509SubjectName>.*X509SubjectName>|not-the-signer.*X509SubjectName|SubjectName.*not-" src/xmldsig/keys.rs
echo "---broader search of test fns after 760---"
sed -n '760,900p' src/xmldsig/keys.rsLength of output: 6927 🏁 Script executed: #!/bin/bash
rg -n "SubjectName" src/xmldsig/keys.rs -A2 -B2 | grep -n "X509SubjectName"
echo "---broader---"
rg -n "X509SubjectName>[^<]*</X509SubjectName>.*X509SubjectName" -o src/xmldsig/keys.rs
echo "---full test list keys.rs---"
rg -n "#\[test\]" src/xmldsig/keys.rs | tail -40Length of output: 1553 🏁 Script executed: #!/bin/bash
rg -n "#\[test\]" src/xmldsig/parse.rs | tail -30
echo "---"
rg -n "fn.*selector" src/xmldsig/parse.rsLength of output: 955 🏁 Script executed: #!/bin/bash
sed -n '2130,2230p' src/xmldsig/parse.rsLength of output: 5300 🏁 Script executed: #!/bin/bash
rg -n "not-the-signer" src/xmldsig/parse.rs src/xmldsig/keys.rsLength of output: 357 🏁 Script executed: #!/bin/bash
rg -n "X509SubjectName>.*</X509SubjectName>\s*<X509SubjectName" -U src/xmldsig/parse.rs src/xmldsig/keys.rs
echo "---count subjectname occurrences per test block---"
awk '/#\[test\]/{name="";c=0} /fn /{if(name=="")name=$0} /X509SubjectName/{c++} /^ }/{if(c>1)print name, c; c=0}' src/xmldsig/parse.rs | head -30Length of output: 567 🏁 Script executed: #!/bin/bash
rg -n "digests:|X509Digest" src/xmldsig/keys.rs -A3 | head -80
echo "---search fixtures donor chain config for multi-digest---"
rg -n "digest" tests/donor_full_verification_suite.rs -i | head -40Length of output: 1572 🏁 Script executed: #!/bin/bash
rg -n "ValidChain|x509chain|trust_anchor_path" tests/donor_full_verification_suite.rs -A5 | head -100Length of output: 1737 🏁 Script executed: #!/bin/bash
grep -n "X509Digest" src/xmldsig/keys.rs
sed -n '560,600p' src/xmldsig/keys.rsLength of output: 2410 🏁 Script executed: #!/bin/bash
rg -n "X509_DIGEST_SIGNATURE" src/xmldsig/keys.rs -B2 -A20 | head -60Length of output: 3134
In I also see the corresponding coverage:
One nuance worth noting for future reference: within a single category (e.g., two This looks good to me — thanks for confirming. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/xmldsig/keys.rs (1)
642-649: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPin verification time in chain-policy tests.
Line 642 and Line 666 leave
verification_timeasNone, so these assertions depend onSystemTime::now; once fixture validity windows move, this can surfaceCertificateNotValidbefore the expected trust-anchor behavior or fail the positive chain case.Proposed fix
let resolver = DefaultKeyResolver::new(KeyResolverConfig { trusted_certs: vec![certificate_der], verify_chains: true, + verification_time: Some( + SystemTime::UNIX_EPOCH + std::time::Duration::from_secs(1_800_000_000), + ), ..KeyResolverConfig::default() }); @@ let resolver = DefaultKeyResolver::new(KeyResolverConfig { trusted_certs: vec![leaf, issuer], verify_chains: true, + verification_time: Some( + SystemTime::UNIX_EPOCH + std::time::Duration::from_secs(1_800_000_000), + ), ..KeyResolverConfig::default() });Also applies to: 666-673
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/xmldsig/keys.rs` around lines 642 - 649, The chain-policy tests around VerifyContext::new() are relying on the default verification_time, which makes them depend on SystemTime::now and can cause time-window flakes. Update the test setup in this block and the later positive-chain case to explicitly pin verification_time on the VerifyContext (or shared helper) to a fixed instant that keeps the fixture certificates within their intended validity window, so the assertions exercise trust-anchor behavior instead of time validity failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/xmldsig/keys.rs`:
- Around line 642-649: The chain-policy tests around VerifyContext::new() are
relying on the default verification_time, which makes them depend on
SystemTime::now and can cause time-window flakes. Update the test setup in this
block and the later positive-chain case to explicitly pin verification_time on
the VerifyContext (or shared helper) to a fixed instant that keeps the fixture
certificates within their intended validity window, so the assertions exercise
trust-anchor behavior instead of time validity failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7c96ab18-305c-46fd-9e6a-4f49e6700ae0
📒 Files selected for processing (3)
src/xmldsig/keys.rssrc/xmldsig/parse.rstests/donor_full_verification_suite.rs
1 participant
Summary
X509Dataagainst configured certificates by subject, issuer/serial, SKI, and digestDefaultKeyResolver, including embedded X.509, configured selectors, and an anchored certificate chainTesting
cargo check --workspacecargo clippy --all-targets --all-features -- -D warningscargo build --workspace --all-featurescargo nextest run --workspace(505 passed)cargo test --doc --workspace(3 passed)git diff --checkCloses #71