feat(xmldsig): complete Merlin interop - #106
Conversation
- add DSA-SHA1 and HMAC-SHA1 verification paths - resolve bounded external references and X.509 key retrieval - cover all Merlin documents, references, and failure policies - update dependency requirements and public support documentation Closes #105
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds provider-based cryptography, typed security policies, bounded XMLDSig and XMLEnc processing, DSA-SHA1 and HMAC-SHA1 verification, X.509 resolution, Merlin interoperability coverage, XMLSec tooling, and fuzz coverage. ChangesXMLDSig, XMLEnc, and provider foundations
XMLDSig verification and certificate processing
Interoperability and validation tooling
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant VerifyContext
participant UriReferenceResolver
participant KeyResolver
participant X509Chain
participant CryptoProvider
VerifyContext->>UriReferenceResolver: Resolve bounded same-document or caller-supplied external data
VerifyContext->>KeyResolver: Resolve KeyInfo and RetrievalMethod sources
KeyResolver->>X509Chain: Build and validate certificate path
VerifyContext->>CryptoProvider: Digest and verify the signature
CryptoProvider-->>VerifyContext: Return verification result
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 164a9bb4e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/xmldsig/verify.rs (1)
1168-1172: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThe per-signature Reference cap no longer counts unsupported-transform Manifest references.
Line 1168 compares
references.len()againstMAX_REFERENCES_PER_SIGNATURE. After this change, a reference whose transform chain is unsupported is pushed toinvalidat lines 1179-1190 and never toreferences. A Manifest that contains only such references therefore leavesreferencesempty whileinvalidgrows for every entry, and each entry allocates aReferenceResultwith an owned URIString.The cap intends to bound the total references one signature may process, as its own message states. Count both collections.
Reachability is limited: Manifest parsing runs only after every SignedInfo reference digest and the SignatureValue validate, so the attacker must already hold a valid signature over the enclosing
ObjectorManifest.nodes_limit: 100_000also caps total growth. The check is still wrong relative to its stated intent.🐛 Proposed fix: apply the cap to parsed and invalid references together
- if references.len() == MAX_REFERENCES_PER_SIGNATURE { + if references.len() + invalid.len() == MAX_REFERENCES_PER_SIGNATURE { return Err(SignatureVerificationPipelineError::InvalidStructure { reason: "signed Manifests exceed the per-signature Reference limit", }); }🤖 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/verify.rs` around lines 1168 - 1172, Update the per-signature limit check in the Manifest reference-processing logic to count both supported references in references and unsupported-transform entries in invalid. Enforce MAX_REFERENCES_PER_SIGNATURE against their combined count before accepting another entry, while preserving the existing InvalidStructure error and collection behavior.
🤖 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 `@docs/xmldsig.md`:
- Around line 6-7: Update the lead sentence in the XMLDSIG documentation to
remove the outdated “same-document” limitation, aligning its stated scope with
the caller-supplied external references documented later. Preserve the existing
feature list and wording otherwise.
In `@src/xmldsig/keys.rs`:
- Around line 44-66: Update HmacSha1VerificationKey and its VerifyingKey::verify
implementation to bind and enforce a configured expected HMAC-SHA1 output
length, rejecting signature_value lengths that differ before comparison. Remove
the caller-controlled prefix-length behavior while preserving the algorithm
mismatch and invalid-length failure paths.
In `@src/xmldsig/parse.rs`:
- Around line 495-504: Extract the shared “first element child after optional
XMLDSIG Transforms” traversal into a helper near the parsing logic, preserving
the existing missing-element and namespace checks. Update both
parse_reference_with_xpath_budget and reference_digest_method to call this
helper so their Transforms-then-DigestMethod walks remain identical, while
keeping each function’s subsequent parsing and error handling unchanged.
- Around line 796-819: Update parse_dsa_key_value to accept and ignore the
schema-defined optional children J, Seed, and PgenCounter after Y, while
retaining the required P, Q, G, and Y validation and ordering. Consume only
valid trailing elements, including the required Seed/PgenCounter pairing, and
return KeyValueInfo::Dsa for supported inputs instead of rejecting them as extra
children; preserve ParseError handling for malformed required structure.
- Around line 632-679: Update parse_retrieval_method_transforms to validate the
XPath expression by its namespace-resolved QName rather than requiring the
literal dsig prefix. Accept any prefix bound to XMLDSIG_NS while preserving the
ancestor-or-self::X509Data selection requirement, and retain the existing
namespace binding validation behavior.
In `@src/xmldsig/signature.rs`:
- Around line 298-303: Rename minimum_rsa_modulus_bits to reflect that it only
validates or enforces the algorithm in validate_rsa_public_key, and discard its
return value explicitly since minimum_modulus_bits remains the caller-provided
policy. Update the direct call in
ecdsa_algorithms_are_rejected_for_rsa_verification to use the renamed helper.
- Around line 219-228: Update DSA signature handling in VerificationKey::verify
and the DSA arm of verify_with_algorithm so Signature::from_components failures
are treated as a verification miss, returning Ok(false) and ultimately
DsigStatus::Invalid(SignatureMismatch) rather than propagating
InvalidSignatureFormat as DsigError::Crypto. Preserve the existing wrong-length
behavior and ensure malformed r or s components follow the same path.
In `@tests/merlin_interop.rs`:
- Around line 403-414: Replace the bare negative assertions with exact
error-variant matches and remove earlier competing failures: in
tests/merlin_interop.rs lines 403-414, configure UriTypeSet::ALL, provide
external_resources(&resources), and match the ambiguous-ID error; at lines
332-342, build the aggregate map from external_resources() and match the
total-size bound error; at lines 428-446, match the internal-DTD error for the
first assertion, then allow the URI class and provide resources for the
unsupported RetrievalMethod case so it matches the transform-compatibility
error.
---
Outside diff comments:
In `@src/xmldsig/verify.rs`:
- Around line 1168-1172: Update the per-signature limit check in the Manifest
reference-processing logic to count both supported references in references and
unsupported-transform entries in invalid. Enforce MAX_REFERENCES_PER_SIGNATURE
against their combined count before accepting another entry, while preserving
the existing InvalidStructure error and collection behavior.
🪄 Autofix
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: 4f956903-ad41-42c5-bd61-327533d316e1
📒 Files selected for processing (15)
Cargo.tomlREADME.mddocs/xmldsig.mdsrc/xmldsig/keys.rssrc/xmldsig/mod.rssrc/xmldsig/parse.rssrc/xmldsig/signature.rssrc/xmldsig/types.rssrc/xmldsig/uri.rssrc/xmldsig/verify.rssrc/xmldsig/x509.rssrc/xmldsig/xpath.rstests/donor_full_verification_suite.rstests/merlin_interop.rstests/uri_integration.rs
- track the complete Merlin fixture snapshot for hermetic CI\n- harden HMAC, legacy RSA, X509, RetrievalMethod, and Manifest paths\n- add regression coverage for every reviewed failure mode
|
@coderabbitai Re: outside-diff Manifest reference cap finding — fixed in |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline --decorate --no-renames d9a013c
git show --format=fuller --find-renames --find-copies -- src/xmldsig/verify.rs tests/merlin_interop.rs d9a013c
printf '\n--- Manifest limit guard contexts on the PR branch ---\n'
rg -n -C 8 'manifest_reference_limit_counts_unsupported_entries|unsupported.*transform|reference.*limit|Manifest' \
src/xmldsig/verify.rs tests/merlin_interop.rsLength of output: 83253
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/merlin_interop.rs (1)
480-486: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNegative assertions in
rejects_dtd_and_unsupported_retrieval_defaultsdo not pin the intended failure. Both assertions match an outer error variant only. In each case a different code path can satisfy the match, so the rule under test is not proven. Pin the exact failure at each site.
tests/merlin_interop.rs#L480-L486: bind the error and assertDsigError::DisallowedUri { uri }whereuri == "http://www.w3.org/TR/xml-stylesheet", becauseenforce_reference_policiesrejects the SignedInfo reference beforematerialize_retrieval_methodsevaluates theRetrievalMethodURI. Add a second case that allows the reference URI class but not theRetrievalMethodURI class to prove that policy.tests/merlin_interop.rs#L458-L472: match the innerParseKeyInfoerror for the unsupportedRetrievalMethodtransform shape instead ofParseKeyInfo(_).🤖 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 `@tests/merlin_interop.rs` around lines 480 - 486, The negative assertions in rejects_dtd_and_unsupported_retrieval_defaults must pin the intended failures: at tests/merlin_interop.rs:480-486, bind DsigError::DisallowedUri and assert uri equals "http://www.w3.org/TR/xml-stylesheet", then add a case permitting the reference URI class while rejecting the RetrievalMethod URI class; at tests/merlin_interop.rs:458-472, match the specific inner ParseKeyInfo error for the unsupported RetrievalMethod transform rather than accepting any ParseKeyInfo variant.
🤖 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/verify.rs`:
- Around line 2429-2459: Extend retrieval-method test coverage for the ambiguous
`(Some, Some)` relation by adding a fixture with an `X509Data` target containing
a descendant `X509Data`, then add a separate test named
`retrieval_method_rejects_ambiguous_x509_data_relation` that expects
`materialize_retrieval_methods` to return `InvalidStructure` with reason
`"X509Data RetrievalMethod selected multiple X509Data elements"`.
In `@tests/fixtures/xmldsig/merlin-xmldsig-twenty-three/Readme.txt`:
- Around line 36-40: Update the key-resolution instructions in the README to
replace the placeholder common name “Xxx” with “Lugh” and replace
“certs/xxx.crt” with the actual certificate filename under certs/ that contains
Lugh’s subject common name.
In
`@tests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloping-hmac-sha1-40.tmpl`:
- Line 6: Restore the 40-bit negative HMAC test vector by changing
HMACOutputLength to 40 in both
tests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloping-hmac-sha1-40.tmpl:6-6
and
tests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloping-hmac-sha1-40.xml:6-14,
then regenerate the XML fixture’s matching SignatureValue to reflect the updated
SignedInfo.
In `@tests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature.xml`:
- Around line 143-152: Add a concise code comment at the descendant-selection
branch in materialize_retrieval_methods documenting that this fixture declares
ancestor-or-self::dsig:X509Data while `#object-4` contains X509Data as a
descendant, so the deliberate relaxation must be preserved. Do not alter the
selection behavior.
---
Outside diff comments:
In `@tests/merlin_interop.rs`:
- Around line 480-486: The negative assertions in
rejects_dtd_and_unsupported_retrieval_defaults must pin the intended failures:
at tests/merlin_interop.rs:480-486, bind DsigError::DisallowedUri and assert uri
equals "http://www.w3.org/TR/xml-stylesheet", then add a case permitting the
reference URI class while rejecting the RetrievalMethod URI class; at
tests/merlin_interop.rs:458-472, match the specific inner ParseKeyInfo error for
the unsupported RetrievalMethod transform rather than accepting any ParseKeyInfo
variant.
🪄 Autofix
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: e72f5e14-8e41-4653-aa57-4fb1e380a5d8
⛔ Files ignored due to path filters (10)
tests/fixtures/xmldsig/merlin-xmldsig-twenty-three/certs/badb.pemis excluded by!**/*.pemtests/fixtures/xmldsig/merlin-xmldsig-twenty-three/certs/balor.pemis excluded by!**/*.pemtests/fixtures/xmldsig/merlin-xmldsig-twenty-three/certs/bres.pemis excluded by!**/*.pemtests/fixtures/xmldsig/merlin-xmldsig-twenty-three/certs/ca.pemis excluded by!**/*.pemtests/fixtures/xmldsig/merlin-xmldsig-twenty-three/certs/lugh-cert.pemis excluded by!**/*.pemtests/fixtures/xmldsig/merlin-xmldsig-twenty-three/certs/lugh.pemis excluded by!**/*.pemtests/fixtures/xmldsig/merlin-xmldsig-twenty-three/certs/macha.pemis excluded by!**/*.pemtests/fixtures/xmldsig/merlin-xmldsig-twenty-three/certs/merlin.pemis excluded by!**/*.pemtests/fixtures/xmldsig/merlin-xmldsig-twenty-three/certs/morigu.pemis excluded by!**/*.pemtests/fixtures/xmldsig/merlin-xmldsig-twenty-three/certs/nemain.pemis excluded by!**/*.pem
📒 Files selected for processing (46)
.gitattributesdocs/xmldsig.mdscripts/import-donor-fixtures.shsrc/xmldsig/keys.rssrc/xmldsig/parse.rssrc/xmldsig/signature.rssrc/xmldsig/verify.rssrc/xmldsig/x509.rstests/fixtures/xmldsig/external-data/xml-stylesheet-2005tests/fixtures/xmldsig/external-data/xml-stylesheet-2005.b64tests/fixtures/xmldsig/merlin-xmldsig-twenty-three/Readme.txttests/fixtures/xmldsig/merlin-xmldsig-twenty-three/certs/badb.dertests/fixtures/xmldsig/merlin-xmldsig-twenty-three/certs/balor.dertests/fixtures/xmldsig/merlin-xmldsig-twenty-three/certs/ca.dertests/fixtures/xmldsig/merlin-xmldsig-twenty-three/certs/lugh-cert.dertests/fixtures/xmldsig/merlin-xmldsig-twenty-three/certs/lugh.dertests/fixtures/xmldsig/merlin-xmldsig-twenty-three/certs/macha.dertests/fixtures/xmldsig/merlin-xmldsig-twenty-three/certs/merlin.dertests/fixtures/xmldsig/merlin-xmldsig-twenty-three/certs/nemain.dertests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloped-dsa.tmpltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloping-b64-dsa.tmpltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloping-b64-dsa.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloping-dsa.tmpltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloping-dsa.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloping-hmac-sha1-40.tmpltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloping-hmac-sha1-40.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloping-hmac-sha1.tmpltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloping-hmac-sha1.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloping-rsa.tmpltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-external-b64-dsa.tmpltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-external-b64-dsa.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-external-dsa.tmpltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-external-dsa.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-keyname.tmpltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-keyname.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-retrievalmethod-rawx509crt.tmpltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-retrievalmethod-rawx509crt.xmltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-x509-crt-crl.tmpltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-x509-crt.tmpltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-x509-is.tmpltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-x509-ski.tmpltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-x509-sn.tmpltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature.tmpltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature.xmltests/fixtures_smoke.rstests/merlin_interop.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9a013c335
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Preserve KeyInfo source order and bound X.509 materialization - Parse complete simple-content text across XML node splits - Separate reference and key-retrieval URI policies - Normalize misleading Merlin donor artifacts reproducibly
|
@coderabbitai Re: the outside-diff negative-assertion finding — fixed. The tests now match the exact unsupported-XPath ParseError and exact disallowed URI, and VerifyContext now separates signed-payload URI policy from RetrievalMethod URI policy so the second test isolates external key retrieval. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Recent commits ---'
git log -8 --oneline --decorate
printf '%s\n' '--- Candidate files ---'
fd -t f '^(verify|merlin_interop)\.rs$' src tests
printf '%s\n' '--- Module outline ---'
ast-grep outline src/xmldsig/verify.rs --items all --type function,struct,impl --match 'VerifyContext|RetrievalMethod|Uri|URI|verify'
printf '%s\n' '--- Exact assertion and URI policy contexts ---'
rg -n -C 10 -i \
'unsupported.*xpath|xpath.*unsupported|disallowed.*uri|uri.*disallowed|retrieval.*uri|external.*uri|VerifyContext|RetrievalMethod' \
src/xmldsig/verify.rs tests/merlin_interop.rsLength of output: 1584 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- e0ac4b2 patch for relevant files ---'
git show --format=fuller --find-renames --find-copies e0ac4b2 -- \
src/xmldsig/verify.rs tests/merlin_interop.rs
printf '%s\n' '--- VerifyContext and verification function map ---'
ast-grep outline src/xmldsig/verify.rs --items all --type struct,function --match 'VerifyContext|verify|retrieval|uri|Uri'
printf '%s\n' '--- Relevant implementation and test contexts ---'
rg -n -i -C 12 \
'unsupported.*xpath|xpath.*unsupported|disallowed.*uri|uri.*disallowed|retrieval.*uri|external.*uri|VerifyContext|RetrievalMethod' \
src/xmldsig/verify.rs tests/merlin_interop.rsLength of output: 163989
The test also separates the URI cases. It first asserts rejection of the signed-payload URI. It then allows signed-payload URIs and asserts rejection of the external
✏️ Learnings added
You are interacting with an AI system. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0ac4b2ccf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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 `@scripts/import-donor-fixtures.sh`:
- Around line 51-55: Update the HMAC rename loop in normalize_imported_snapshot
to verify each signature-enveloping-hmac-sha1-40 source file exists before
calling mv, and fail with an actionable message identifying the changed or
missing donor fixture. Because this function may now return non-zero, update its
caller to propagate the status so cleanup occurs and replace_target is not run
on an incomplete snapshot.
🪄 Autofix
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: 3397fad8-dd2c-426c-a530-5be96c22bd3d
📒 Files selected for processing (8)
docs/xmldsig.mdscripts/import-donor-fixtures.shsrc/xmldsig/parse.rssrc/xmldsig/verify.rstests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloping-hmac-sha1-80.tmpltests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-enveloping-hmac-sha1-80.xmltests/fixtures_smoke.rstests/merlin_interop.rs
- Require external URIs for raw X509 retrieval - Preserve DSA fallback during rollover validation - Fail donor fixture normalization without partial installs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fd4a488d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- accept schema-valid partial DSAKeyValue sources without aborting ordered fallback - share same-document ID parsing across retrieval and manifest paths - redact HMAC secret material from Debug output
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 155520bf3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Pin the unreleased upstream snapshot by commit and checksum - Enforce RFC 5280 X.509 serial bounds and XML whitespace rules - Add SHA-256 X509Digest coverage and a verification fuzz target
- Decode only XML text nodes in CryptoBinary simple content - Cover comment-split DSA and RSA key parameters
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbfb5c76af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- enforce transform policy from the terminal data type - preserve unsupported advisory retrieval methods - bound external XML parsing and retained diagnostics - run fuzz smoke explicitly on nightly
Keep cargo-fuzz 0.13.1 pinned while allowing compatible transitive patch releases on current nightly.
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@scripts/install-xmlsec1.sh`:
- Around line 63-66: Update the installation replacement flow around the
staged-prefix mv to restore work_dir/previous-install to prefix if that move
fails, before the EXIT trap removes the working installation. Preserve the
existing backup move and successful staged installation behavior.
In `@src/xmldsig/parse.rs`:
- Around line 1531-1557: Update the serial conversion logic before
format_x509_serial_value_hex to reject a bytes buffer containing only zeroes,
while preserving existing validation and overflow checks. Extend the relevant
rejection tests to cover zero-valued inputs such as "0" and "000".
In `@tests/common/xmlsec1.rs`:
- Around line 11-22: Update version_supports_interop to locate the xmlsec1
prefix, parse only the immediately following token as the version, and reject
inputs without that prefix or with non-numeric, missing, or extra version
components. Preserve the REQUIRED_VERSION comparison using exactly three numeric
components.
In `@tests/fixtures/xmldsig/README.md`:
- Around line 29-30: Update the README’s algorithm support statements to reflect
that DSA-SHA1 and HMAC-SHA1 verification are supported, while documenting only
the remaining unsupported DSA and HMAC variants as fail-closed. Keep the
surrounding X.509 and signing support descriptions unchanged.
🪄 Autofix
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: 6cc7e228-8650-48c2-83a5-bac724cb8751
📒 Files selected for processing (21)
.github/workflows/ci.yml.gitignoreREADME.mdfuzz/Cargo.tomlfuzz/corpus/xmldsig_verify/signature.xmlfuzz/fuzz_targets/xmldsig_verify.rsscripts/import-donor-fixtures.shscripts/install-xmlsec1.shsrc/hard_limits.rssrc/lib.rssrc/xmldsig/keys.rssrc/xmldsig/parse.rssrc/xmldsig/transforms.rssrc/xmldsig/verify.rstests/common/xmlsec1.rstests/fixtures/xmldsig/README.mdtests/fixtures/xmldsig/aleksey-xmldsig-01/enveloped-x509-digest-sha256.xmltests/fixtures/xmlenc/README.mdtests/fixtures_smoke.rstests/xmlenc_encrypt_xmlsec1.rstests/xmlsec1_interop.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa4a088110
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- make xmlsec1 replacement transactional through validation - enforce ordered X.509 names and positive serials - support direct typed X509Data retrieval safely - tighten interop version parsing and fixture documentation
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 173ccb388b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Allow zero-valued deny-all resource ceilings while retaining nonzero X.509 path bounds - Reject CRL checking without certificate-chain validation - Share bounded XML Base resolution across references, key retrieval, and C14N
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 (3)
src/c14n/xml_base.rs (2)
98-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelete the test-only duplicate of the XML Base walk.
compute_effective_xml_baseis now#[cfg(test)]. Every production caller usescompute_effective_xml_base_with_budget. Two implementations of the same ancestor walk now exist, and only the budgeted one is exercised outside tests.The two functions must stay in agreement, but nothing enforces that. A change to the boundary rule or the resolution order in one function would not fail the other. Tests that call the unbudgeted function would then assert behavior that production does not have.
Remove
compute_effective_xml_baseand callcompute_effective_xml_base_with_budgetwithXmlBaseResolutionBudget::default()from the tests, or keep a thin#[cfg(test)]wrapper that delegates to the budgeted function.♻️ Proposed refactor
#[cfg(test)] pub(crate) fn compute_effective_xml_base( start: Node<'_, '_>, visibility: Option<&dyn NodeVisibility>, ) -> Option<String> { - let mut bases: Vec<&str> = Vec::new(); - let mut current = Some(start); - while let Some(n) = current { - // ... duplicated walk ... - } - Some(effective) + // Tests exercise the same algorithm production uses; the default budget + // never triggers for the small fixtures in this module. + compute_effective_xml_base_with_budget(start, visibility, &XmlBaseResolutionBudget::default()) + .expect("test fixtures stay within the default XML Base budget") }🤖 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/c14n/xml_base.rs` around lines 98 - 134, Remove the duplicated test-only ancestor walk in compute_effective_xml_base and update its test callers to use compute_effective_xml_base_with_budget with XmlBaseResolutionBudget::default(). Alternatively, retain only a thin test wrapper that delegates to the budgeted implementation, ensuring tests exercise the same production logic.
239-295: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the external resource key contract.
external_resourcesand the resolver API only say callers provide external URI payloads, but external references resolvexml:baseand lookup payload withresources.get(&resolved). Add one sentence stating that these keys should match the RFC 3986 resolved identity, including dot-segment removal and retained query/fragment suffixes, so callers do not use pre-normalization keys and miss payloads.🤖 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/c14n/xml_base.rs` around lines 239 - 295, Document the external resource key contract near the external_resources/resolver API: state that keys must match the RFC 3986 resolved URI identity, including dot-segment removal and retained query or fragment suffixes. Do not alter the resolution logic; clarify that callers must use the normalized resolved value rather than a pre-normalization reference.src/c14n/mod.rs (1)
425-456: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe unbounded dispatch arm discards the supplied XML Base budget.
serialize_canonical_visible_with_position_dispatchacceptsxml_base_resolutionand forwards it only in theSome(max_output_bytes)arm. TheNonearm callsserialize_canonical_visible_with_position, which allocates a freshXmlBaseResolutionBudget::default()insrc/c14n/serialize.rs(line 201) and ignores the argument.No caller reaches this today: the only caller that supplies a budget is
canonicalize_with_visibility_and_position_bounded_with_xml_base_budget, and it always suppliesSome(max_output_bytes). The signature nonetheless promises to honor the budget on both arms. A later unbounded caller would silently fall back to the hard-limit default instead of the operation policy, with no compile error.Forward the budget through the unbounded arm.
♻️ Proposed refactor
Add a budget-aware unbounded entry point in
src/c14n/serialize.rs:pub(crate) fn serialize_canonical_visible_with_position_with_xml_base_budget( doc: &Document, visibility: Option<&dyn NodeVisibility>, with_comments: bool, ns_renderer: &dyn NsRenderer, config: C14nConfig, tracked_element: Option<NodeId>, xml_base_resolution: &XmlBaseResolutionBudget, output: &mut Vec<u8>, ) -> Result<Option<usize>, C14nError> { serialize_canonical_visible_with_position_bounded( doc, visibility, with_comments, ns_renderer, config, CanonicalOutputOptions::unbounded(tracked_element, xml_base_resolution), output, ) }Then call it from the
Nonearm:- None => serialize_canonical_visible_with_position( + None => serialize_canonical_visible_with_position_with_xml_base_budget( doc, visibility, with_comments, renderer, config, tracked_element, + xml_base_resolution, output, ),🤖 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/c14n/mod.rs` around lines 425 - 456, Update serialize_canonical_visible_with_position_dispatch so the None(max_output_bytes) arm preserves the supplied xml_base_resolution budget. Add a budget-aware unbounded entry point alongside serialize_canonical_visible_with_position that constructs CanonicalOutputOptions::unbounded with the provided budget, then call it from the dispatch function while leaving the bounded arm unchanged.
🤖 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/policy.rs`:
- Around line 180-193: Update the zero-value branch in nonzero_within so it
reports an accurate nonzero-violation reason instead of claiming the value
exceeds the configured ceiling; use ResourceLimit with maximum set to selected,
or the existing PolicyViolation::KeyTrust variant with wording that requires a
nonzero value. Preserve Self::within for positive values and keep actual set to
selected.
In `@src/xmldsig/uri.rs`:
- Around line 178-184: Update the None branch of the resolved URI match in the
URI resolution flow to normalize non-fragment URIs before returning them,
matching the behavior used when an xml:base exists while preserving fragment
handling. Add a regression test covering an external URI containing path
segments such as “..” with no xml:base, and verify it matches the normalized
caller-map key.
In `@src/xmlenc/encrypt.rs`:
- Around line 1139-1157: Extend
zero_resource_ceilings_allow_operations_that_consume_none with a second
assertion using a non-empty binary payload, and verify encryption fails under
the zero plaintext ceiling. Keep the existing empty-payload success assertion
unchanged so the test covers both zero consumption being allowed and any
positive consumption being denied.
---
Outside diff comments:
In `@src/c14n/mod.rs`:
- Around line 425-456: Update serialize_canonical_visible_with_position_dispatch
so the None(max_output_bytes) arm preserves the supplied xml_base_resolution
budget. Add a budget-aware unbounded entry point alongside
serialize_canonical_visible_with_position that constructs
CanonicalOutputOptions::unbounded with the provided budget, then call it from
the dispatch function while leaving the bounded arm unchanged.
In `@src/c14n/xml_base.rs`:
- Around line 98-134: Remove the duplicated test-only ancestor walk in
compute_effective_xml_base and update its test callers to use
compute_effective_xml_base_with_budget with XmlBaseResolutionBudget::default().
Alternatively, retain only a thin test wrapper that delegates to the budgeted
implementation, ensuring tests exercise the same production logic.
- Around line 239-295: Document the external resource key contract near the
external_resources/resolver API: state that keys must match the RFC 3986
resolved URI identity, including dot-segment removal and retained query or
fragment suffixes. Do not alter the resolution logic; clarify that callers must
use the normalized resolved value rather than a pre-normalization reference.
🪄 Autofix
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: 7bc16b7f-9341-4b84-b332-6604b056fee3
📒 Files selected for processing (15)
README.mddocs/xmldsig.mdsrc/c14n/mod.rssrc/c14n/serialize.rssrc/c14n/xml_base.rssrc/hard_limits.rssrc/lib.rssrc/policy.rssrc/xmldsig/keys.rssrc/xmldsig/parse.rssrc/xmldsig/transforms.rssrc/xmldsig/types.rssrc/xmldsig/uri.rssrc/xmldsig/verify.rssrc/xmlenc/encrypt.rs
💤 Files with no reviewable changes (1)
- src/lib.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88b185dd3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Meter repeated external dereferences through one resolver budget - Preserve URI normalization and XML Base limits across all paths - Report invalid lower-bound policy limits accurately
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92bf93e66f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- share the operation XML Base budget with SignedInfo C14N - route X.509 path and CRL authentication through the selected provider - cover verification, signing, path-building, and CRL regressions
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a470602823
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- select ECDSA digest from SignatureMethod or X.509 OID - select P-256, P-384, or P-521 exclusively from SPKI - cover cross-pair signing, verification, and donor vectors
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/sign.rs`:
- Around line 16-19: Override sign_with_provider on EcdsaP256SigningKey and
EcdsaP384SigningKey to obtain the digest through provider.digest for the
requested DigestAlgorithm, then pass the result to sign_prehash; retain sign as
the default-provider wrapper. Add regression coverage showing signing fails when
a custom CryptoProvider rejects each ECDSA digest.
In `@src/xmldsig/signature.rs`:
- Around line 374-381: Remove the unreachable 521 alternative from the P-521 arm
in the curve_oid/point_len match, keeping only point_len 528 mapped to
EcCurve::P521 with a 66-byte coordinate size.
In `@src/xmldsig/x509.rs`:
- Around line 357-368: Update x509_signature_algorithm to explicitly handle the
omitted ECDSA-SHA512, RSASSA-PSS, and SHA-2 DSA OIDs, either by mapping
supported algorithms or returning a distinct unsupported-algorithm result that
callers preserve as a diagnosable X509ChainError. Ensure unknown and
intentionally unsupported algorithms are not silently converted into an ordinary
InvalidSignature outcome.
🪄 Autofix
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: d2ef2442-3a8b-4718-a3bc-ef5f9782f453
📒 Files selected for processing (15)
README.mddocs/xmldsig.mdsrc/c14n/mod.rssrc/xmldsig/keys.rssrc/xmldsig/parse.rssrc/xmldsig/sign.rssrc/xmldsig/signature.rssrc/xmldsig/verify.rssrc/xmldsig/x509.rstests/donor_full_verification_suite.rstests/donor_interop_suite.rstests/ecdsa_signature_integration.rstests/signature_builder.rstests/signing_digest.rstests/xmlsec1_interop.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42a25e8b5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- route ECDSA prehashing through the selected provider\n- preserve typed X.509 RSA-PSS and Ed25519 verification\n- share canonicalization work across the verification pipeline
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/parse.rs`:
- Around line 1288-1290: Defer unsupported-signature failures in both builders:
in src/xmldsig/parse.rs#L1288-L1290 within build_x509_certificate_chain_from,
record the OID and continue candidate evaluation, returning the error only when
verified is empty and an OID was recorded; apply the same change at
src/xmldsig/parse.rs#L1369-L1371 within
build_x509_certificate_paths_to_trusted_prefix, reporting the recorded OID only
when completed is empty.
🪄 Autofix
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: 7bd7ce92-3b27-4241-9120-5d78ea04a71f
📒 Files selected for processing (10)
Cargo.tomlREADME.mddocs/xmldsig.mdsrc/provider.rssrc/xmldsig/keys.rssrc/xmldsig/parse.rssrc/xmldsig/sign.rssrc/xmldsig/signature.rssrc/xmldsig/verify.rssrc/xmldsig/x509.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37ad953fbb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- defer unsupported certificate branches during bounded path search\n- route every modeled certificate OID to the selected provider\n- retain typed diagnostics when no supported path completes
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 (2)
docs/xmldsig.md (2)
85-88: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winScope the processing-error rule to core references.
Manifest processing records disallowed URIs and unsupported transforms as independent per-reference statuses. Lines 85-88 currently classify all such cases as processing errors. Limit this sentence to core
<SignedInfo>processing and state the Manifest exception.Proposed documentation fix
-Malformed XMLDSig structure, unsupported algorithms, disallowed reference URIs, and -inconsistent `KeyInfo` metadata are processing errors rather than validity statuses. +Malformed XMLDSig structure, unsupported algorithms in core signature processing, +disallowed URIs in `<SignedInfo>` references, and inconsistent `KeyInfo` metadata are +processing errors rather than validity statuses. Manifest policy violations and unsupported +transforms remain independent per-reference statuses as described above.🤖 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 `@docs/xmldsig.md` around lines 85 - 88, Update the processing-error statement in the XMLDSig documentation to apply only to core `<SignedInfo>` processing, and explicitly exclude Manifest per-reference statuses, including disallowed URIs and unsupported transforms, from that classification.
115-119: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDocument both internal-DTD gates.
VerifyContext::allow_internal_dtd(true)is only the per-call opt-in. Verification and detached XML parsing also require the applicable policy permission. State both requirements so readers do not assume that the per-call flag overrides policy.Proposed documentation fix
-Internal DTD declarations are disabled by default and require -`VerifyContext::allow_internal_dtd(true)`. The policy applies consistently to the signed document +Internal DTD declarations are disabled by default. Verification requires both the applicable +policy permission and `VerifyContext::allow_internal_dtd(true)`. The policy applies consistently +to the signed document🤖 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 `@docs/xmldsig.md` around lines 115 - 119, Update the internal DTD documentation around VerifyContext::allow_internal_dtd and detached XML parsing to state that both the per-call opt-in and the applicable verification policy permission are required; clarify that the per-call flag does not override policy, while preserving the existing TransformOptions and SigningPolicy descriptions.
♻️ Duplicate comments (1)
src/xmldsig/parse.rs (1)
1381-1392: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winContinue evaluating same-DN issuers after a provider capability failure.
Line 1391 stops at the first unsupported issuer. Provider support can depend on the issuer SPKI, including the ECDSA curve. A later same-DN issuer can still verify the child certificate.
Record the OID, then continue the issuer loop. Return the unsupported-algorithm diagnostic only after all issuer candidates and DFS paths fail. Add a regression with an unsupported issuer candidate before a usable same-DN issuer.
Proposed fix
// Unsupported is an algorithm capability, so changing - // the issuer key cannot make this child verifiable. - // Prune this DFS branch but retain sibling paths. + // the current candidate cannot verify this child. + // Continue with other same-DN issuer candidates. unsupported_oid.get_or_insert(oid); - break; + continue;🤖 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 1381 - 1392, In the issuer-evaluation loop of the certificate-chain DFS, update the ProviderError::Unsupported handling to record the OID in unsupported_oid but continue checking remaining same-DN issuers instead of breaking. Preserve the unsupported-algorithm diagnostic only after all issuer candidates and DFS paths fail, and add a regression covering an unsupported issuer preceding a usable same-DN issuer.
🤖 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 `@docs/xmldsig.md`:
- Around line 85-88: Update the processing-error statement in the XMLDSig
documentation to apply only to core `<SignedInfo>` processing, and explicitly
exclude Manifest per-reference statuses, including disallowed URIs and
unsupported transforms, from that classification.
- Around line 115-119: Update the internal DTD documentation around
VerifyContext::allow_internal_dtd and detached XML parsing to state that both
the per-call opt-in and the applicable verification policy permission are
required; clarify that the per-call flag does not override policy, while
preserving the existing TransformOptions and SigningPolicy descriptions.
---
Duplicate comments:
In `@src/xmldsig/parse.rs`:
- Around line 1381-1392: In the issuer-evaluation loop of the certificate-chain
DFS, update the ProviderError::Unsupported handling to record the OID in
unsupported_oid but continue checking remaining same-DN issuers instead of
breaking. Preserve the unsupported-algorithm diagnostic only after all issuer
candidates and DFS paths fail, and add a regression covering an unsupported
issuer preceding a usable same-DN issuer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 92c7f864-365a-4f9e-9441-4221fa4a91bb
📒 Files selected for processing (4)
docs/xmldsig.mdsrc/xmldsig/keys.rssrc/xmldsig/parse.rssrc/xmldsig/x509.rs
There was a problem hiding this comment.
💡 Codex Review
Lines 498 to 500 in 168b648
Exclude self-issued CAs from path-length counting
When a valid path contains a self-issued rollover CA below an anchor with pathLenConstraint = 0, this positional calculation counts the rollover certificate as a subordinate CA and rejects the path, although self-issued intermediates are excluded from the path-length count. Fresh evidence beyond the earlier path-construction issue is that construction now preserves these rollover paths, but this later constraint check still rejects them; compute the count from the actual lower path certificates while skipping self-issued CAs.
Lines 202 to 205 in 168b648
Enforce CA name constraints during path validation
When a validated intermediate contains a critical NameConstraints extension, this loop checks only validity, leaf key usage, and CA constraints, so a cryptographically valid leaf outside the CA's permitted namespace is still accepted. This can bypass restrictions imposed by a configured trust path; process NameConstraints against every subordinate certificate, and reject any other critical extension the validator does not implement.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Enforce RFC 4055 and RFC 5280 certificate restrictions - Bind selector categories to one signature-valid path - Reject unrepresentable legacy OAEP MGF parameters - Clarify public validation and Manifest semantics
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/xmldsig.md (1)
136-144: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDocument the legacy-algorithm policy gate.
This section lists RSA-SHA1, DSA-SHA1, and HMAC-SHA1 as implemented but does not state the policy required to use them. Document the default and explicit opt-in behavior, including that RSA-SHA1 enforcement occurs before key resolution.
🤖 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 `@docs/xmldsig.md` around lines 136 - 144, Update the implemented-algorithms section to document the legacy-algorithm policy gate: state that RSA-SHA1, DSA-SHA1, and HMAC-SHA1 are disabled by default and require explicit opt-in, and note that RSA-SHA1 policy enforcement occurs before key resolution.src/xmlenc/encrypt.rs (2)
1116-1141: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAdd the policy-denied DTD assertion.
The test covers policy permission with per-call opt-in and per-call denial when policy permission is enabled. It does not cover per-call opt-in when policy permission is disabled. A regression that checks only
DocumentEncryptionOptions::allow_dtdwould pass the current assertions.Add an assertion that uses the default policy, sets
allow_dtd: true, and expectsErr(XmlEncError::XmlParse(_)).Proposed test addition
assert!(matches!( EncryptedDataBuilder::new(DataEncryptionAlgorithm::Aes128Gcm) .direct_key([0_u8; 16]) .policy(policy) .encrypt_document(document, DocumentEncryptionOptions::default()), Err(XmlEncError::XmlParse(_)) )); + + assert!(matches!( + EncryptedDataBuilder::new(DataEncryptionAlgorithm::Aes128Gcm) + .direct_key([0_u8; 16]) + .encrypt_document( + document, + DocumentEncryptionOptions { + element_id: None, + allow_dtd: true, + }, + ), + Err(XmlEncError::XmlParse(_)) + ));🤖 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/xmlenc/encrypt.rs` around lines 1116 - 1141, Add a third assertion to document_dtd_requires_policy_and_per_call_opt_in using the default EncryptionPolicy with DocumentEncryptionOptions.allow_dtd set to true, and assert encrypt_document returns Err(XmlEncError::XmlParse(_)).
1170-1200: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTest rejection at the zero-recipient ceiling.
The test sets
max_encryption_recipientsto zero but only runs an operation with zero recipients. It does not prove that one configured recipient is rejected. Add an empty-payload AES Key Wrap case and assertTooManyRecipients { maximum: 0, actual: 1 }.Proposed test addition
assert!(matches!( EncryptedDataBuilder::new(DataEncryptionAlgorithm::Aes128Gcm) .direct_key([0_u8; 16]) - .policy(policy) + .policy(policy.clone()) .encrypt_binary(b"x"), Err(XmlEncError::PlaintextTooLarge { maximum: 0, actual: 1 }) )); + + assert!(matches!( + EncryptedDataBuilder::new(DataEncryptionAlgorithm::Aes128Gcm) + .add_recipient(EncryptionRecipient::aes_key_wrap( + [0_u8; 16], + KeyWrapAlgorithm::AesKw128, + )) + .policy(policy) + .encrypt_binary(&[]), + Err(XmlEncError::TooManyRecipients { + maximum: 0, + actual: 1 + }) + ));🤖 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/xmlenc/encrypt.rs` around lines 1170 - 1200, Extend zero_resource_ceilings_allow_operations_that_consume_none with an empty-payload AES Key Wrap encryption using one configured recipient, and assert it returns XmlEncError::TooManyRecipients { maximum: 0, actual: 1 }. Keep the existing direct-key empty and non-empty plaintext assertions unchanged.
🤖 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 `@docs/xmldsig.md`:
- Around line 89-90: Update the Manifest reference policy documentation near the
existing transform-limit statement to describe the aggregate
ResourcePolicy::max_references budget: parsed Manifest references and
unsupported-transform results must consume the shared limit, including when
every reference is unsupported.
In `@src/provider.rs`:
- Around line 1569-1592: Add a short comment immediately before the
incompatible-case assertions explaining that each case must return Ok(false)
because rsa_pss_key_parameters_allow rejects the SPKI before verify_signature
can report unsupported, particularly for the Sha256/Sha384 digest mismatch;
record that this behavior depends on the current validation order and must be
preserved.
In `@src/xmldsig/parse.rs`:
- Around line 1659-1687: Document the intentional strict fallback in the `_ =>
left.value == right.value` arm of `x509_attribute_values_equal`: note that raw
`Any` equality requires identical ASN.1 tags and bytes, including for
`emailAddress`, `domainComponent`, and values that are not `DirectoryString`. Do
not alter the comparison behavior unless adding explicit IA5String handling.
In `@src/xmldsig/x509.rs`:
- Around line 562-588: Document the intentional rejection of critical
extendedKeyUsage (2.5.29.37) and certificatePolicies (2.5.29.32) alongside the
X.509 validation policy and support matrix. Explain that only the extensions
allowlisted by validate_critical_extensions are processed, so certificates
marking these extensions critical may fail with UnsupportedCriticalExtension,
while preserving the existing validation behavior.
- Around line 760-762: Update the URI handling in general_name_within_subtree
and its callers in validate_general_name to distinguish a non-matching URI from
one whose host cannot be parsed by uri_host. For excluded_subtrees, treat an
unevaluable URI constraint as excluded rather than allowing the certificate;
preserve fail-closed rejection for permitted_subtrees and normal matching
behavior for parseable hosts. Apply the same behavior to the related handling
around the additional URI cases.
- Around line 654-661: Update validate_certificate_names to call
validate_general_name for the subject DirectoryName only when
certificate.subject() contains at least one RDN; skip this check for an empty
subject DN while preserving the existing validation flow for non-empty subjects.
- Around line 1363-1370: Update the assertion in the test around
verify_generated_path to match the exact
X509ChainError::UnsupportedCriticalExtension variant and verify it contains OID
1.2.3.4, rather than accepting any error via is_err(). Preserve the test’s
rejection expectation while ensuring the failure is specifically caused by the
unprocessed critical extension allowlist.
---
Outside diff comments:
In `@docs/xmldsig.md`:
- Around line 136-144: Update the implemented-algorithms section to document the
legacy-algorithm policy gate: state that RSA-SHA1, DSA-SHA1, and HMAC-SHA1 are
disabled by default and require explicit opt-in, and note that RSA-SHA1 policy
enforcement occurs before key resolution.
In `@src/xmlenc/encrypt.rs`:
- Around line 1116-1141: Add a third assertion to
document_dtd_requires_policy_and_per_call_opt_in using the default
EncryptionPolicy with DocumentEncryptionOptions.allow_dtd set to true, and
assert encrypt_document returns Err(XmlEncError::XmlParse(_)).
- Around line 1170-1200: Extend
zero_resource_ceilings_allow_operations_that_consume_none with an empty-payload
AES Key Wrap encryption using one configured recipient, and assert it returns
XmlEncError::TooManyRecipients { maximum: 0, actual: 1 }. Keep the existing
direct-key empty and non-empty plaintext assertions unchanged.
🪄 Autofix
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: f1a59e62-6595-48b0-a560-0a3a62fa8d3b
📒 Files selected for processing (8)
README.mddocs/xmldsig.mddocs/xmlenc.mdsrc/provider.rssrc/xmldsig/keys.rssrc/xmldsig/parse.rssrc/xmldsig/x509.rssrc/xmlenc/encrypt.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2213a8d51f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- distinguish unevaluable URI constraints from ordinary non-matches - match IA5 distinguished-name values using RFC 5280 rules - select rollover CRLs by key identity and authenticated applicability - document policy budgets and critical-extension support
|
Too many files changed for review (101 files, 100 file limit). Bypass the limit by tagging |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f46334b41
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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/xmlenc/types.rs`:
- Around line 415-425: Update fixed_aes_key_size to derive sizes through the
existing DataEncryptionAlgorithm and KeyWrapAlgorithm typed parsers and their
key_len methods, rather than matching URI literals directly. Preserve None for
unsupported algorithms and ensure all supported AES variants continue returning
the parser-defined key length.
🪄 Autofix
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: 1d3d5a8f-800f-4102-80d4-4420abeeb635
📒 Files selected for processing (11)
README.mddocs/xmldsig.mddocs/xmlenc.mdsrc/provider.rssrc/xmldsig/parse.rssrc/xmldsig/x509.rssrc/xmlenc/decrypt.rssrc/xmlenc/encrypt.rssrc/xmlenc/parse.rssrc/xmlenc/types.rstests/x509_chain_integration.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ff58453ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 082534bf14
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .extensions() | ||
| .iter() | ||
| .find_map(|extension| match extension.parsed_extension() { | ||
| ParsedExtension::NameConstraints(value) => Some(value), | ||
| _ => None, |
There was a problem hiding this comment.
When chain validation receives a CA certificate containing two critical NameConstraints extensions, this first-match lookup applies only the first one, while validate_critical_extensions accepts both as recognized extensions. A permissive first constraint followed by a restrictive second constraint can therefore allow an out-of-scope leaf to validate even though RFC 5280 forbids duplicate extension OIDs; reject duplicates before interpreting any extension.
Useful? React with 👍 / 👎.
| if let (Some(actual), Some(expected)) = | ||
| (self.key_size_bits, fixed_aes_key_size(&self.algorithm)) | ||
| && actual != expected |
There was a problem hiding this comment.
When callers construct an RSA-OAEP EncryptionMethod with key_size_bits: Some(0), fixed_aes_key_size returns None, so this condition skips validation and decrypt_data can proceed through key resolution successfully. The XML parser rejects the identical zero KeySize, so typed input still bypasses the structural invariant this method is intended to share with parsed input; validate that every supplied KeySize is positive before applying algorithm-specific equality checks.
Useful? React with 👍 / 👎.
1 participant
Summary
xml:base, including RFC 3986 normalization for scheme-bearing and network-path URIs and a consistent internal-DTD policy for root and detached XMLRetrievalMethodresolution, authenticated CRL handling, and bounded manifest referencesKeyValue, or X.509 key source while preserving backend interoperability capabilityrsa-oaep-mgf1pbefore custom-provider dispatch because the wire algorithm cannot represent another MGFX509Dataretrieval without a transform while requiring explicit XPath selection when the dereferenced root is a wrapperSignedInfoand authenticated Manifests, bound external XML reparsing, and cap canonicalizedSignedInfoplus retained pre-digest diagnostics across a signatureX509Digestcoverage and a bounded XMLDSig verification fuzz targetTesting
cargo buildcargo build --no-default-featurescargo build --all-featurescargo clippy --all-targets --all-features -- -D warningscargo nextest run --all-featurescargo test --doc --all-featurescargo +nightly fuzz run xmldsig_verify -- -runs=256 -max_len=65536Closes #105