feat(xmldsig): add key resolver configuration#66
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds XMLDSig key-resolution types and resolver wiring that consume parsed ChangesXMLDSig Key Resolver
Changelog Configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/ci.yml:
- Around line 13-23: The pr-title job currently checks out untrusted PR code and
then runs .github/scripts/test-validate-pr-title.sh and
.github/scripts/validate-pr-title.sh, so harden the job before those steps.
Update the workflow to grant only read-only contents permissions for this job
and configure actions/checkout to avoid persisting credentials in git config.
Keep the fix scoped to the pr-title job and the checkout step so the PR-branch
scripts cannot access an unnecessarily exposed GITHUB_TOKEN.
- Line 17: The workflow currently uses the mutable actions/checkout@v7
reference, which should be pinned to an immutable commit SHA. Update the
checkout step in the CI workflow to use the full SHA for actions/checkout so the
action version cannot drift. Keep the fix localized to the checkout entry in the
workflow file.
🪄 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: 471768ac-6c12-41e8-942c-92dfa80ee984
📒 Files selected for processing (6)
.github/scripts/test-validate-pr-title.sh.github/scripts/validate-pr-title.sh.github/workflows/ci.yml.release-plz.tomlsrc/xmldsig/keys.rssrc/xmldsig/mod.rs
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 250-251: The EcdsaP384Sha384 branch in keys.rs is incorrectly
allowing a P-521 curve OID alongside P-384. Tighten the match in the
signature/public-key compatibility check so SignatureAlgorithm::EcdsaP384Sha384
only accepts the P-384 curve OID and no longer includes 1.3.132.0.35; leave the
other algorithm-to-curve branches unchanged and keep the logic centered in the
existing PublicKey::EC matching arm.
- Around line 198-202: The KeyName lookup path currently only checks
VerificationKey.algorithm before returning a cloned key, so invalid SPKI bytes
can slip through and fail later in crypto. Update the KeyName resolution flow
around the map/lookup in keys.rs to validate the named key’s SPKI bytes against
the requested algorithm before returning it, and surface any mismatch as a typed
key-resolution failure (for example by extending the existing KeyResolutionError
path) rather than handing back the misconfigured key.
🪄 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: f98e6bae-9631-4487-9416-146987947210
📒 Files selected for processing (3)
src/xmldsig/keys.rssrc/xmldsig/mod.rssrc/xmldsig/verify.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Line 202: The named-key path in keys::validate should not surface malformed
SPKI as InvalidCertificate; instead, update the validation around
validate_spki_algorithm for configured public_key_bytes so malformed <KeyName>
and DEREncodedKeyValue inputs map to a public-key parse error. Adjust the typed
resolver API’s error handling in this flow so certificate-specific failures are
only used for real certificates, and keep the named-key parsing path distinct
via the relevant resolver/validation helpers in src/xmldsig/keys.rs.
🪄 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: 2510f89d-ccb4-4ea4-a525-4368ef3789a3
📒 Files selected for processing (1)
src/xmldsig/keys.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 480-483: The test currently asserts the Display text for the key
resolution failure, but it should verify the typed error contract instead.
Update the assertion in the test around KeyResolutionError handling to match the
KeyResolutionError::InvalidPublicKey variant rather than comparing
error.to_string(), using the same error-producing path in keys.rs so the test
stays stable against wrapper ტექxt changes.
🪄 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: 6ac27ea9-6370-42bb-b552-f703913dc59e
📒 Files selected for processing (1)
src/xmldsig/keys.rs
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Jun 28, 2026
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
VerifyContext.testcommits from release-plz changelogs.Resolver behavior
DefaultKeyResolverconsumes the already-parsed optionalKeyInfoand declaredSignatureAlgorithm, avoiding a second XML parse. Sources are tried in document order. Embedded X.509 certificates can be accepted directly or validated against configured trust anchors, verification time, and maximum path depth. Raw RSA/ECKeyValuemarkers remain unresolved because the current parser model does not retain their key parameters.Release automation finding
Release-plz worked as configured for #64. PR #63 used merge-commit strategy, so every conventional commit in that branch became a separate changelog entry. One entry per PR requires squash-only repository merges. This repository setting is intentionally not changed by this PR; this PR does not add PR-title validation to CI.
Testing
cargo fmt --all -- --checkcargo check --workspace --all-featurescargo clippy --workspace --all-targets --all-features -- -D warningscargo nextest run --workspace --all-features(462/462)cargo test --doc --workspace --all-features(3/3)cargo build --workspace --all-featuresCloses #65