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
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds legacy DSA-SHA1 and HMAC-SHA1 verification, bounded external-resource and ChangesXMLDSig Merlin interoperability
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant VerifyContext
participant UriReferenceResolver
participant KeyResolver
participant X509Chain
participant SignatureVerifier
VerifyContext->>UriReferenceResolver: resolve bounded external or same-document input
VerifyContext->>KeyResolver: resolve KeyInfo and RetrievalMethod sources
KeyResolver->>X509Chain: construct and validate certificate chain
VerifyContext->>SignatureVerifier: verify DSA-SHA1, HMAC-SHA1, or RSA-SHA1 signature
SignatureVerifier-->>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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 92: In the added CI path, update the actions/checkout and
dtolnay/rust-toolchain uses entries at .github/workflows/ci.yml lines 92-93 to
reviewed immutable full commit SHAs instead of tags; also set
persist-credentials to false on checkout if subsequent steps do not require Git
credentials.
- Around line 89-98: Set an explicit, shorter timeout-minutes value on the
fuzz-smoke job so cargo-fuzz cannot run until GitHub Actions’ default limit;
leave the existing checkout, toolchain, installation, and fuzz command steps
unchanged.
In `@scripts/install-xmlsec1.sh`:
- Around line 54-58: Replace the unstable checksum validation of the generated
codeload archive in the install flow with a stable verification strategy: either
download from a maintained mirror with a reproducible pinned artifact, or
extract the archive and verify the resulting source tree against the expected
content. Preserve fail-closed behavior while avoiding dependence on codeload’s
gzip bytes.
In `@src/xmldsig/parse.rs`:
- Around line 1514-1519: Update x509_name_to_rfc4514 to avoid the non-public
X509Name::new constructor; use public x509-parser APIs to normalize the RDNs, or
collect their display() values in RFC 4514 order before returning the
selector-comparison string. Preserve the existing reverse RDN ordering.
In `@tests/install_xmlsec1.rs`:
- Around line 64-67: Update the fake tar setup in the test around root.tool to
derive the created xmlsec source directory from the archive name or
XMLSEC1_COMMIT used by scripts/install-xmlsec1.sh, instead of embedding the
commit in the shell script. Ensure autogen.sh is created under the dynamically
derived directory so the test remains valid when the pinned commit changes.
🪄 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: 60a0becb-6760-4e67-add0-ef2503dfff48
📒 Files selected for processing (22)
.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/install_xmlsec1.rstests/xmlenc_encrypt_xmlsec1.rstests/xmlsec1_interop.rs
- fetch and verify the pinned xmlsec1 Git object - pin workflow actions and bound fuzz runtime - keep X.509 name normalization on public APIs - cover source mismatch and rollback behavior
Keep trusted actions on reviewable version channels while retaining read-only workflow permissions, credential-free checkouts, and the fuzz runtime budget.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/xmldsig/parse.rs (1)
1277-1363: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftSerialize X.509 names with RFC 4514 rules.
x509_name_to_rfc4514reverses RDNs, butX509Name::to_string()emits comma-separated attribute values without RFC 4514 escaping.distinguished_names_equalonly normalizes commas, so selectors containing escaped commas, plus signs, equals signs, backslashes, or boundary spaces can fail to match the certificate name.Compare structured RDNs, or parse and re-serialize names with RFC 4514 escaping. Add regression tests for escaped separators and boundary spaces.
🤖 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 1277 - 1363, Update distinguished_names_equal to compare X.509 names using structured RDNs or canonical RFC 4514 serialization rather than splitting raw strings on commas. Ensure escaped commas, plus signs, equals signs, backslashes, and boundary spaces are handled consistently with x509_name_to_rfc4514. Add regression tests covering escaped separators and boundary spaces.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/xmldsig/parse.rs`:
- Around line 1277-1363: Update distinguished_names_equal to compare X.509 names
using structured RDNs or canonical RFC 4514 serialization rather than splitting
raw strings on commas. Ensure escaped commas, plus signs, equals signs,
backslashes, and boundary spaces are handled consistently with
x509_name_to_rfc4514. Add regression tests covering escaped separators and
boundary spaces.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 90d5807b-36e9-4c84-8420-5f3ad83d319b
📒 Files selected for processing (4)
.github/workflows/ci.ymlscripts/install-xmlsec1.shsrc/xmldsig/parse.rstests/install_xmlsec1.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6905e49c11
ℹ️ 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".
- update direct dependencies to base64 0.23 and x509-cert 0.3 - compare certificate selectors through RFC 4514 structured names - remove failed first-time xmlsec1 installations transactionally
|
@coderabbitai Fixed the outside-diff RFC 4514 finding in eb1840e: certificate Names now decode and serialize through x509-cert structured RFC 4514 handling, selector matching parses structured Names, and regressions cover escaped comma/plus/equals/backslash, boundary spaces, surrounding XML whitespace, and RDN order. The full Merlin corpus passes. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Commit summary ---'
git show --stat --oneline --no-renames eb1840e
printf '%s\n' '--- Changed Rust files ---'
git diff-tree --no-commit-id --name-only -r eb1840e -- '*.rs'
printf '%s\n' '--- Candidate module outlines ---'
for file in src/xmldsig/x509.rs src/xmldsig/parse.rs src/xmldsig/keys.rs tests/merlin_interop.rs; do
if [ -f "$file" ]; then
printf '\n--- %s ---\n' "$file"
ast-grep outline "$file" --items all --type function,impl,struct,enum --match 'Name|name|X509|x509|Selector|selector|rfc'
fi
done
printf '%s\n' '--- Commit patch and regression contexts ---'
git show --format=fuller --find-renames --find-copies eb1840e -- \
src/xmldsig/x509.rs src/xmldsig/parse.rs src/xmldsig/keys.rs tests/merlin_interop.rs
printf '%s\n' '--- RFC 4514 implementation and tests ---'
rg -n -C 10 -i \
'rfc.?4514|escaped.*comma|escaped.*plus|escaped.*equals|escaped.*backslash|boundary.*space|xml.*whitespace|rdn.*order|selector.*name|issuer.*name|subject.*name' \
src testsLength of output: 1562 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- PR commit SHAs matching eb1840e ---'
gh api --paginate 'repos/structured-world/xml-sec/pulls/106/commits?per_page=100' \
--jq '.[] | select(.sha | startswith("eb1840e")) | [.sha, .commit.message] | `@tsv`'
printf '%s\n' '--- PR head SHA ---'
gh api 'repos/structured-world/xml-sec/pulls/106' --jq '.head.sha'
printf '%s\n' '--- Local refs that contain the short SHA ---'
git show-ref | rg 'eb1840e' || trueLength of output: 744 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Current branch and matching files ---'
git rev-parse HEAD
fd -t f -e rs . src tests | xargs -r rg -l -i \
'rfc.?4514|to_rfc4514|from_rfc4514|issuer.*name|subject.*name|x509.*name|x509issuerserial' || true
printf '%s\n' '--- Candidate module outlines ---'
for file in $(fd -t f -e rs . src tests | xargs -r rg -l -i \
'rfc.?4514|to_rfc4514|from_rfc4514|issuer.*name|subject.*name|x509.*name|x509issuerserial' || true); do
printf '\n--- %s ---\n' "$file"
ast-grep outline "$file" --items all --type function,impl,struct,enum --match 'Name|name|X509|x509|Selector|selector|rfc'
done
printf '%s\n' '--- RFC 4514 implementation and regression contexts ---'
fd -t f -e rs . src tests | xargs -r rg -n -C 12 -i \
'rfc.?4514|to_rfc4514|from_rfc4514|escaped.*comma|escaped.*plus|escaped.*equals|escaped.*backslash|boundary.*space|surrounding.*xml.*whitespace|rdn.*order|x509issuerserial'Length of output: 1571 🏁 Script executed: #!/bin/bash
set -euo pipefail
files=(
src/xmldsig/parse.rs
src/xmldsig/keys.rs
src/xmldsig/verify.rs
)
printf '%s\n' '--- Candidate module outlines ---'
for file in "${files[@]}"; do
if [ -f "$file" ]; then
printf '\n--- %s ---\n' "$file"
ast-grep outline "$file" --items all
fi
done
printf '%s\n' '--- RFC 4514 implementation and selector contexts ---'
rg -n -C 16 -i \
'rfc.?4514|to_rfc4514|from_rfc4514|issuer.*name|subject.*name|x509issuerserial|selector.*name|name.*selector' \
src/xmldsig/parse.rs src/xmldsig/keys.rs src/xmldsig/verify.rs
printf '%s\n' '--- Regression assertion contexts ---'
rg -n -C 16 -i \
'escaped.*comma|escaped.*plus|escaped.*equals|escaped.*backslash|boundary.*space|surrounding.*xml.*whitespace|rdn.*order|rfc.?4514' \
tests src/xmldsigLength of output: 126810
Line 1351 still implements The current tests cover RDN order and serial-number whitespace. I did not find regressions for the reported escaped DN characters, DN boundary spaces, or surrounding XML whitespace. GitHub does not show a PR commit that begins with You are interacting with an AI system. |
There was a problem hiding this comment.
💡 Codex Review
Line 287 in eb1840e
Allow a selected certificate to serve as its trust anchor
When selector-only X509Data resolves a self-signed certificate that is explicitly present in trusted_certs, this second policy check removes that certificate from the anchor set and inevitably returns UntrustedRoot if no separate copy/rollover certificate exists. This prevents signatures made directly by a configured root from verifying with verify_chains enabled, despite trusted_certs being documented as accepted trust anchors; only exclude a selector-resolved certificate when it is an untrusted lookup candidate rather than the configured self-signed anchor.
ℹ️ 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".
- Apply X.520 DN matching and XML Base URI resolution - Separate selector lookup certificates from trust anchors
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/install-xmlsec1.sh`:
- Around line 39-45: Update the restoration branch controlled by
had_previous_install so it attempts mv only when previous_install exists.
Preserve the existing restoration error handling when the backup is present, and
avoid reporting a restoration failure or retaining the work directory when no
backup was created.
In `@src/xmldsig/verify.rs`:
- Around line 481-512: Update reference_origin_node for ReferenceSet::Manifest
to traverse only direct-child ds:Object elements of the Signature, then only
their direct-child ds:Manifest elements, collecting each Manifest’s direct-child
Reference elements in that order. Keep the existing SignedInfo behavior
unchanged and align this indexing with parse_manifest_references.
🪄 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: b276e6c6-8a0e-4da7-8352-b795ed107deb
📒 Files selected for processing (13)
.github/workflows/ci.ymlCargo.tomldocs/xmldsig.mdfuzz/fuzz_targets/xmldsig_verify.rsscripts/install-xmlsec1.shsrc/c14n/mod.rssrc/xmldsig/keys.rssrc/xmldsig/parse.rssrc/xmldsig/uri.rssrc/xmldsig/verify.rstests/donor_full_verification_suite.rstests/install_xmlsec1.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: 3bc02d166b
ℹ️ 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".
- Align Manifest and XML Base reference origins - Preserve padded serials and lookup intermediates - Clarify transactional installer rollback state
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/xmldsig/keys.rs`:
- Around line 402-408: Replace the string-based `reason.contains("ambiguous")`
check in the `build_x509_certificate_chain_from` error mapping with a dedicated
typed `ParseError::AmbiguousIssuer` variant. Update the parser and
`build_x509_certificate_chain_from` flow to return that variant for ambiguous
issuer cases, then map it to `KeyResolutionError::AmbiguousCertificate` while
preserving `InvalidCertificate` for other parse errors.
🪄 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: f7feb5df-f040-46a0-b840-af3c31c9cb86
📒 Files selected for processing (6)
docs/xmldsig.mdscripts/install-xmlsec1.shsrc/c14n/xml_base.rssrc/xmldsig/keys.rssrc/xmldsig/parse.rssrc/xmldsig/verify.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcdccc40fc
ℹ️ 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".
- normalize absolute URI paths before external resource lookup - disambiguate same-subject X.509 issuers by certificate signature - replace text-based chain error classification with typed errors - document and test XMLDSig HMAC byte alignment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ec3a96b99
ℹ️ 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 relative URI identity for pathless schemeless bases - split Unicode URI suffixes only at UTF-8 boundaries - terminate paths at explicitly trusted selected certificates
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)
src/xmldsig/parse.rs (1)
1205-1252: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove one of the two depth checks.
Line 1206 tests
chain.len() > MAX_X509_CHAIN_DEPTH. Line 1249 testschain.len() == MAX_X509_CHAIN_DEPTHbefore everypush. The second check makeschain.len()never exceedMAX_X509_CHAIN_DEPTH, so the first check cannot trigger. Keep the pre-push check and drop the loop-head check, or keep the loop-head check and drop the pre-push check. One check is enough and states the bound once.♻️ Proposed simplification
loop { - if chain.len() > MAX_X509_CHAIN_DEPTH { - return Err(X509ChainBuildError::DepthExceeded); - } - let current_idx = *chain🤖 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 1205 - 1252, Remove one redundant depth-bound check in the certificate-chain construction loop: keep either the loop-head `chain.len() > MAX_X509_CHAIN_DEPTH` check or the pre-push `chain.len() == MAX_X509_CHAIN_DEPTH` check, but not both. Preserve the existing `DepthExceeded` behavior and ensure the chain never grows beyond `MAX_X509_CHAIN_DEPTH`.
🤖 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/x509.rs`:
- Around line 222-234: Update certificate_signature_matches to avoid the
structural X509Name equality check between certificate.issuer() and
issuer.subject(), which can reject candidates already matched by the caller’s
normalized issuer-name comparison. After parsing both certificates, rely on
verify_certificate_signature for the signature check while preserving the
existing false result for parse failures.
---
Outside diff comments:
In `@src/xmldsig/parse.rs`:
- Around line 1205-1252: Remove one redundant depth-bound check in the
certificate-chain construction loop: keep either the loop-head `chain.len() >
MAX_X509_CHAIN_DEPTH` check or the pre-push `chain.len() ==
MAX_X509_CHAIN_DEPTH` check, but not both. Preserve the existing `DepthExceeded`
behavior and ensure the chain never grows beyond `MAX_X509_CHAIN_DEPTH`.
🪄 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: de1381c4-5975-4f29-937a-84c19a9a3e5b
📒 Files selected for processing (5)
src/c14n/xml_base.rssrc/xmldsig/keys.rssrc/xmldsig/parse.rssrc/xmldsig/uri.rssrc/xmldsig/x509.rs
There was a problem hiding this comment.
💡 Codex Review
Lines 266 to 267 in 88541da
Add lookup intermediates to embedded certificate paths
When <X509Data> embeds only a leaf certificate—or a raw-X509 RetrievalMethod materializes one—and that leaf chains through lookup_certs to a configured root, this branch validates the unmodified one-certificate path and never adds the lookup intermediate. Although the selector-only branch now builds a path from lookup_certs, the embedded branch still returns UntrustedRoot, contrary to lookup_certs being documented as untrusted path intermediates; augment embedded paths from the configured pool before calling the chain verifier.
ℹ️ 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".
- normalize RFC 3986 absolute references with relative XML bases - defer missing retrieval sources until key alternatives are exhausted - apply RFC 5280 name matching consistently across certificate paths - remove the redundant certificate-chain depth guard
|
@coderabbitai Re: redundant X.509 chain-depth checks in |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Current PR head ---'
git rev-parse HEAD
printf '%s\n' '--- X.509 chain builder outline ---'
ast-grep outline src/xmldsig/parse.rs --items all --type function --match 'chain|Chain|x509|X509'
printf '%s\n' '--- Chain-depth guard and regression contexts ---'
rg -n -C 14 -i \
'maximum.*depth|max.*chain|chain.*depth|depth.*chain|MAX_.*CHAIN|certificate.*chain|build_x509_certificate_chain_from' \
src/xmldsig/parse.rs tests
printf '%s\n' '--- Recent edits affecting the chain builder ---'
git log -12 --oneline -- src/xmldsig/parse.rs
git diff HEAD~1..HEAD -- src/xmldsig/parse.rs || trueLength of output: 70929
The removed loop-head check for
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: c1d0bd7a7c
ℹ️ 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".
- Normalize scheme-bearing rootless URI dot segments per RFC 3986 - Apply internal-DTD policy consistently to root and detached XML - Preserve the external-entity prohibition and add regression coverage
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abd05ed65f
ℹ️ 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 authority when resolving against network-path XML bases - Require matching inner and outer signature algorithms for certificates and CRLs - Cover helper, resolver, certificate, and revocation paths
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)
src/xmldsig/verify.rs (1)
996-1005: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winApply the retention limit to canonicalized
SignedInfounconditionally.
canonical_signed_infois allocated before Line 1003 and remains required for signature verification. Line 1003 charges it only whenstore_pre_digestis enabled. A large canonicalized<SignedInfo>therefore bypasses the 32 MiB ceiling with the default configuration.Charge this buffer unconditionally. Keep reference diagnostic-buffer charging conditional.
Proposed fix
- if ctx.store_pre_digest { - pre_digest_budget.charge(canonical_signed_info.len())?; - } + pre_digest_budget.charge(canonical_signed_info.len())?;🤖 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 996 - 1005, Update the canonicalized SignedInfo flow around canonicalize and canonical_signed_info so pre_digest_budget.charge(canonical_signed_info.len()) runs unconditionally, regardless of ctx.store_pre_digest. Keep only reference diagnostic-buffer charging conditional on ctx.store_pre_digest.docs/xmldsig.md (1)
59-63: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument direct same-document
X509Dataretrieval.The supported-case list omits an untransformed
RetrievalMethodthat directly targets same-documentX509Data.src/xmldsig/verify.rsLines 3011-3036 support this case. Add it so the documented support boundary matches the implementation.🤖 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 59 - 63, Update the supported-case list in the xmldsig documentation to include untransformed RetrievalMethod references that directly target same-document X509Data. Align the documentation with the behavior implemented in verify.rs without changing the existing external rawX509Certificate, URI resolution, or fail-closed transform descriptions.
🤖 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 59-63: Update the supported-case list in the xmldsig documentation
to include untransformed RetrievalMethod references that directly target
same-document X509Data. Align the documentation with the behavior implemented in
verify.rs without changing the existing external rawX509Certificate, URI
resolution, or fail-closed transform descriptions.
In `@src/xmldsig/verify.rs`:
- Around line 996-1005: Update the canonicalized SignedInfo flow around
canonicalize and canonical_signed_info so
pre_digest_budget.charge(canonical_signed_info.len()) runs unconditionally,
regardless of ctx.store_pre_digest. Keep only reference diagnostic-buffer
charging conditional on ctx.store_pre_digest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eb828a35-dd72-4ca8-8c07-f3ec40f74a8e
📒 Files selected for processing (6)
docs/xmldsig.mdsrc/c14n/xml_base.rssrc/xmldsig/transforms.rssrc/xmldsig/uri.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: 1a8d77b780
ℹ️ 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".
| }); | ||
| } | ||
| if references.len() == MAX_REFERENCES_PER_SIGNATURE { | ||
| if references.len() + invalid.len() == MAX_REFERENCES_PER_SIGNATURE { |
There was a problem hiding this comment.
When process_manifests(true) is used, a signature can contain the maximum 64 SignedInfo references and another 64 authenticated Manifest references because this check counts only references + invalid from Manifests. That permits 128 dereference/digest operations despite MAX_REFERENCES_PER_SIGNATURE and the emitted error describing a signature-wide ceiling; with repeated 8 MiB external inputs, this can roughly double the intended CPU and transient-copy workload. Seed the Manifest count with signed_info.references.len() or pass the remaining reference budget into this parser.
Useful? React with 👍 / 👎.
| if [[ "$(uname -s)" == "Darwin" ]]; then | ||
| DYLD_LIBRARY_PATH="$prefix/lib${DYLD_LIBRARY_PATH:+:$DYLD_LIBRARY_PATH}" \ | ||
| "$prefix/bin/xmlsec1" --version | ||
| else | ||
| LD_LIBRARY_PATH="$prefix/lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" \ | ||
| "$prefix/bin/xmlsec1" --version |
There was a problem hiding this comment.
If the newly built executable exits successfully but reports an empty, malformed, or unexpected version, this smoke test still accepts it because stdout is never captured or checked. The source marker has already been written, so subsequent invocations take the fast path and reuse that installation, potentially running interoperability tests against an unverified binary; compare the output with the required 1.3.13 identity before discarding the previous installation.
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 referencesX509Dataretrieval without a transform while requiring explicit XPath selection when the dereferenced root is a wrapperX509Digestcoverage and a bounded XMLDSig verification fuzz targetTesting
cargo check --all-features --all-targetscargo build --all-features --all-targetscargo 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