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
|
Warning Review limit reached
Next review available in: 2 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds typed security policies, provider-backed cryptography, bounded XMLDSig and XMLEnc processing, legacy DSA-SHA1 and HMAC-SHA1 verification, X.509 path and CRL validation, Merlin interoperability coverage, XMLSec tooling, and fuzz coverage. ChangesXMLDSig and XMLEnc security 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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
- Validate EncryptedData IDs with the shared XML NCName grammar - Reject malformed custom-provider AES framing before serialization - Document provider-controlled content-key randomness
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18eb17c8a1
ℹ️ 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".
- Reject empty NameConstraints DER structures before path matching - Bound complete standalone EncryptedData fragments after serialization - Document both validation contracts and their regression coverage
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15e0a81937
ℹ️ 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 3394 output framing at the XMLEnc facade boundary so malformed custom-provider output cannot be serialized as an interoperable EncryptedKey.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4d67126d7
ℹ️ 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 provider-independent KEK and metadata bounds - add typed leaf EKU policy with end-to-end coverage - clarify resolved retrieval identities and CA criticality semantics
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e42d41800a
ℹ️ 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".
- compose resolver and operation EKU policy across complete X.509 paths - reject invalid digest, CRL reason, and RSA transport provider output - add regression coverage and synchronize XMLDSig documentation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fab10a481
ℹ️ 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: 0448fb03a2
ℹ️ 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: 6f8ee3e987
ℹ️ 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/encrypt.rs`:
- Around line 572-574: Update the encryption flow around provider.encrypt_data
and validate_ciphertext_framing to require the exact ciphertext length derived
from algorithm and plaintext.len(), rejecting overlong AES-GCM output and
block-aligned overlong AES-CBC output before serialization. Add provider tests
covering both overlong GCM and CBC results.
🪄 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: 8115be1a-83c8-4b1f-af38-e16d28d7b486
📒 Files selected for processing (24)
README.mddocs/xmldsig.mddocs/xmlenc.mdsrc/hard_limits.rssrc/policy.rssrc/provider.rssrc/xml.rssrc/xmldsig/builder.rssrc/xmldsig/digest.rssrc/xmldsig/keys.rssrc/xmldsig/parse.rssrc/xmldsig/sign.rssrc/xmldsig/signature.rssrc/xmldsig/transforms.rssrc/xmldsig/verify.rssrc/xmldsig/x509.rssrc/xmlenc/decrypt.rssrc/xmlenc/encrypt.rssrc/xmlenc/parse.rssrc/xmlenc/types.rstests/donor_negative_vectors.rstests/signing_digest.rstests/x509_chain_integration.rstests/xmlenc_encrypt_integration.rs
- Measure RSA modulus strength by mathematical bit length - Bound generated EncryptedData nodes before returning output - Preserve projected-document and provider-framing coverage
- Derive the exact CBC and GCM wire length from plaintext - Reject overlong custom-provider output before serialization - Cover block-aligned CBC and one-byte GCM excess
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3822b334db
ℹ️ 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".
- validate custom decryption output framing - accept unsigned RSA exponents with a high bit - require bounded CRL validity windows - reject conflicting verification clocks
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 765c6fa1d1
ℹ️ 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".
- bypass inherited xml:base for absolute resource identities - share XML byte ceilings across signing, verification, and encryption - exclude internal fragment wrappers from caller node accounting
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34971e11a4
ℹ️ 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".
- reject malformed provider digests before ECDSA prehash signing - align X.509 decimal selectors with sign-padded serials - validate every revoked-certificate serial in CRLs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/xmlenc.md (1)
37-48: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winScope the explicit MGF claim to the modern RSA-OAEP URI.
The text says
xml-secalways emits an explicitxenc11:MGF. It also says that the legacyrsa-oaep-mgf1pURI has no MGF field. These statements conflict.State that the explicit
DigestMethodandMGFclaim applies to the XML Encryption 1.1 RSA-OAEP URI. Document the legacy URI exception.Proposed documentation fix
-`xml-sec` always emits explicit `ds:DigestMethod` and `xenc11:MGF` values rather than relying on -those implicit legacy defaults. SHA-1 OAEP remains available only through explicit parameters. +For the XML Encryption 1.1 RSA-OAEP URI, `xml-sec` emits explicit `ds:DigestMethod` and +`xenc11:MGF` values rather than relying on implicit legacy defaults. The legacy +`rsa-oaep-mgf1p` URI fixes MGF1 to SHA-1 and has no `xenc11:MGF` field.🤖 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/xmlenc.md` around lines 37 - 48, Update the RSA-OAEP documentation around the explicit DigestMethod and MGF statement to scope it to the XML Encryption 1.1 RSA-OAEP URI, and explicitly state that the legacy rsa-oaep-mgf1p URI is an exception because it has no wire-level MGF field. Keep the existing SHA-1 availability and validation details consistent with this distinction.
🤖 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/xmlenc.md`:
- Around line 116-117: Update the decryption section in docs/xmlenc.md to
document the two-gate DTD requirement: internal DTD parsing is enabled only when
both EncryptionPolicy::xml.allow_internal_dtd and
DocumentDecryptionOptions::allow_dtd are true. State that the per-call option
cannot weaken the operation policy.
---
Outside diff comments:
In `@docs/xmlenc.md`:
- Around line 37-48: Update the RSA-OAEP documentation around the explicit
DigestMethod and MGF statement to scope it to the XML Encryption 1.1 RSA-OAEP
URI, and explicitly state that the legacy rsa-oaep-mgf1p URI is an exception
because it has no wire-level MGF field. Keep the existing SHA-1 availability and
validation details consistent with this distinction.
🪄 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: 11883530-e1e2-40e2-89f1-60102bbd64ab
📒 Files selected for processing (15)
docs/xmldsig.mddocs/xmlenc.mdsrc/c14n/xml_base.rssrc/hard_limits.rssrc/policy.rssrc/provider.rssrc/xmldsig/parse.rssrc/xmldsig/sign.rssrc/xmldsig/uri.rssrc/xmldsig/verify.rssrc/xmldsig/x509.rssrc/xmlenc/decrypt.rssrc/xmlenc/encrypt.rssrc/xmlenc/parse.rstests/signing_digest.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f696d6669b
ℹ️ 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".
- validate XMLDSig signature framing before provider dispatch - exclude temporary fragment wrappers from caller node limits - clarify OAEP and DTD policy documentation
1 participant
Summary
xml:base, including RFC 3986 normalization, inherited-base bypass for scheme-bearing identities, network-path URIs, and a consistent internal-DTD policy for root and detached XMLRetrievalMethodresolution, authenticated CRL handling, and bounded manifest referencesanyExtendedKeyUsagesemantics and fail-closed resolver/operation policy intersectioncAandkeyCertSignKeySizemetadata before integer parsingxml:baseplus dot-segment normalizationKeyValue, or X.509 key source while preserving backend interoperability capabilityKeySizevalues and non-SHA1 MGF configuration for legacyrsa-oaep-mgf1pbefore resolver or custom-provider dispatchX509Dataretrieval 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