Repository navigation
fix(seal): drop the xml-sec fork, pin the signer - #448
LKSNDRTMLKV wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change replaces the workspace ChangesTrusted-list verification
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant TrustListVerification
participant verify_signature_with
participant DefaultKeyResolver
participant XMLDSigVerifier
TrustListVerification->>verify_signature_with: Pass the accepted certificate and signed XML
verify_signature_with->>DefaultKeyResolver: Configure the accepted certificate as the sole trusted certificate
verify_signature_with->>XMLDSigVerifier: Verify the signed XML
XMLDSigVerifier-->>verify_signature_with: Return verification status or error
verify_signature_with-->>TrustListVerification: Return success or rejection
Merge Risk: ⚪ Minimal · up to No actionable issue is established that would block merging. The reported verification results remain subject to normal checks. 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 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 |
Summary
The workspace carried a fork of
xml-secto raise one constant: the 65 536-entry node-set ceiling that the French, Czech, Italian and Spanish trusted lists exceed. Upstream raised it in 0.1.17 (structured-world/xml-sec#158). This drops the fork and requiresxml-sec0.1.20 from crates.io.0.1.18 changed key trust:
xml-secno longer verifies with a key just because the document carries it. So the verifier now hands it the one certificate this node already decided to trust, as an exact pin. For the LOTL, that is the certificate the Official Journal anchor authorises; for a national list, the one its LOTL pointer names. The signature is then checked against that key and no other, byxml-secitself. Before,xml-secpicked its own key from the document, and the two agreed only because ads:KeyInfowith more than one certificate was refused first. That refusal stays, as the earlier and better-named of two locks.Changes
Cargo.toml: the[patch.crates-io]stanza is gone. Its "accepting a pre-release dependency" reasoning moved beside the dependency incrates/dpp-seal/Cargo.tomland was updated (0.1.18 changed trust semantics under a patch version, so a bump is a real change).deny.toml:allow-gitis empty again. The fork was its only entry, and this was the exit condition written there.trustlist/verify.rs:verify_signature_withpins the vetted certificate (KeyResolverConfig::trusted_certs) for both the LOTL and national lists. A national certificate that is not valid base64 is nowMalformedrather than decoded to nothing.the_patched_xml_sec_is_the_one_that_resolved) becomesxml_sec_is_a_release_with_the_raised_node_set_ceiling: it fails on a release before 0.1.17 or a git source. CI runs it; the over-ceiling documents are too large to commit.tests/xml_sec_fork.rsis renamedtests/large_trusted_lists.rsand reworded. The docs that described the fork (trustlist/mod.rs,fetch.rs,tests/fixtures/local/README.md) say what is true now.Dependencies
xml-sec0.1.16 (fork) → 0.1.20 (crates.io). It is on an untrusted-input path: it canonicalises and verifies trusted-list XML fetched from 30 national servers. Failing closed is unchanged. The lock gainsed448,ed448-goldilocks,x25519-dalek,hash2curve,sha3,keccak,shake,sponge-cursor,pkcs12and a secondcms, all pulled in byxml-sec's XML-encryption and key-import features, which this crate does not call. It losessxd-document-no-unsafe,sxd-xpath-no-unsafe,syn0.15,backtraceand their trees. Duplicated crate names go from 72 to 68.cargo deny check bans licenses sourcesandcargo audit --deny yanked --deny unmaintainedare both clean.How this was checked
just checkgreen, 1 488 unit tests.a_list_over_the_old_node_set_ceiling_verifies.verification requires an authorized key. With the pin, all pass, including the tamper tests (a changed service status, a changedSigningTime, a changedSignatureValue).the_signature_is_checked_only_against_the_pinned_certificate. Finland's genuine list verifies with its own certificate pinned and is refused with the LOTL's. It fails if the verifier goes back to trusting the document's key (checked by swapping inCryptographicOnly).Not in this PR
Summary by CodeRabbit