feat(xmldsig): resolve X509 selectors#72
Open
polaz wants to merge 13 commits into
Open
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 |
|
| Filename | Overview |
|---|---|
| src/xmldsig/keys.rs | Adds configured X.509 selector resolution and applies chain policy to selected certificates. |
| src/xmldsig/parse.rs | Updates embedded X.509 selector matching so selectors can identify different certificates in one chain. |
| tests/donor_full_verification_suite.rs | Extends donor verification coverage for embedded certificates, configured selectors, named keys, and validated chains. |
Reviews (4): Last reviewed commit: "test(xmldsig): pin donor chain time" | Re-trigger Greptile
![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
Member
Author
|
@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
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
1 participant
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
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(504 passed)cargo test --doc --workspace(3 passed)git diff --checkCloses #71