feat(test): add donor test fixtures and C14N golden file tests#16
Merged
feat(test): add donor test fixtures and C14N golden file tests#16
Conversation
- Copy xmlsec1 test keys (RSA 2048/4096, EC P-256/P-384, CA chain, HMAC) into tests/fixtures/keys/ - Copy Merlin C14N test vectors (c14n-three, exc-c14n-one, c14n11) into tests/fixtures/c14n/ - Generate xmllint reference outputs for full-document C14N comparison - Add 13 fixture smoke tests validating PEM structure and completeness - Add 11 C14N golden file tests: 7 full-document byte-compare against xmllint, 4 subtree SHA-1 digest tests against Merlin DigestValues - Subtree digest tests prove exclusive C14N correctness where our implementation correctly diverges from xmllint on namespace rendering Closes #15
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Rust C14N golden-file test module and fixture smoke tests plus ~60 XML/text fixture assets (Merlin vectors, exclusive C14N one, C14N 1.1 xml:base inputs). Tests perform byte-for-byte canonicalization comparisons and subtree SHA‑1 digest checks against xmllint-generated golden outputs. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds donor test fixtures from the xmlsec1 test suite (cryptographic keys and C14N reference vectors) and implements golden file tests to validate the C14N implementation against reference outputs.
Changes:
- Adds 62 test fixture files: RSA 2048/4096/expired keys, EC P-256/P-384 keys, CA chain certificates, HMAC key, Merlin C14N-three vectors (28 golden outputs + signature.xml), Merlin exclusive C14N-one vectors, and C14N 1.1 xml:base test vectors
- Adds
tests/fixtures_smoke.rswith 13 smoke tests validating PEM structure, file existence, and fixture completeness - Adds
tests/c14n_golden.rswith 11 C14N golden file tests: 7 full-document byte-compare tests and 4 subtree SHA-1 digest tests against Merlin DigestValues
Reviewed changes
Copilot reviewed 61 out of 64 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/fixtures_smoke.rs | 13 smoke tests verifying fixture files are present, readable, and structurally valid |
| tests/c14n_golden.rs | 11 C14N golden file tests (7 full-doc byte-compare + 4 subtree digest) |
| tests/fixtures/keys/** | RSA, EC, CA chain, and HMAC key fixtures (21 files) |
| tests/fixtures/c14n/** | C14N reference vectors and golden outputs (41 files across 3 directories) |
You can also share your feedback on Copilot code review. Take the survey.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/fixtures_smoke.rs (1)
172-182: Consider: File count assertion requires maintenance.The hardcoded count of 62 files will fail when fixtures are added or removed, requiring manual updates. This is a reasonable integrity check to guard against accidental deletions, but consider:
- Adding a comment near fixture additions reminding to update this count
- Or using a more lenient lower-bound check:
assert!(count >= 62, ...)The current approach is acceptable if the maintenance burden is understood.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/fixtures_smoke.rs` around lines 172 - 182, The test fixture_file_count_matches_expected currently asserts exact equality of count returned by count_files_recursive(fixtures_dir(), &mut count) to 62 which is brittle; change the assertion to a lenient lower-bound check (e.g., assert!(count >= 62, "expected at least 62 fixture files ...")) or add a clear comment next to the test and any places that add fixtures reminding developers to update the expected count; update the assertion call in fixture_file_count_matches_expected (or add the comment above it) and keep the existing explanatory message.tests/c14n_golden.rs (1)
44-46: Clarify:assert_c14n_matches_goldendefaults towith_comments=true.The helper defaults to
with_comments: true, but C14N without comments is the more common default. This works correctly for the current tests since the goldens were generated accordingly, but consider renaming toassert_c14n_matches_golden_with_commentsor passingfalseby default for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/c14n_golden.rs` around lines 44 - 46, The helper assert_c14n_matches_golden currently calls assert_c14n_matches_golden_comments(xml, mode, true, golden, label) so it defaults to with_comments=true; change this to the clearer behavior by either renaming the wrapper to assert_c14n_matches_golden_with_comments or (preferred) flip the default to false by calling assert_c14n_matches_golden_comments(xml, mode, false, golden, label) and update any tests/golden expectations that rely on the previous default; ensure you update all call sites of assert_c14n_matches_golden (or rename usages if you choose the rename) and adjust goldens if necessary so the tests remain consistent with the new default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/c14n_golden.rs`:
- Around line 44-46: The helper assert_c14n_matches_golden currently calls
assert_c14n_matches_golden_comments(xml, mode, true, golden, label) so it
defaults to with_comments=true; change this to the clearer behavior by either
renaming the wrapper to assert_c14n_matches_golden_with_comments or (preferred)
flip the default to false by calling assert_c14n_matches_golden_comments(xml,
mode, false, golden, label) and update any tests/golden expectations that rely
on the previous default; ensure you update all call sites of
assert_c14n_matches_golden (or rename usages if you choose the rename) and
adjust goldens if necessary so the tests remain consistent with the new default.
In `@tests/fixtures_smoke.rs`:
- Around line 172-182: The test fixture_file_count_matches_expected currently
asserts exact equality of count returned by
count_files_recursive(fixtures_dir(), &mut count) to 62 which is brittle; change
the assertion to a lenient lower-bound check (e.g., assert!(count >= 62,
"expected at least 62 fixture files ...")) or add a clear comment next to the
test and any places that add fixtures reminding developers to update the
expected count; update the assertion call in fixture_file_count_matches_expected
(or add the comment above it) and keep the existing explanatory message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 784957a4-8eb0-46fa-b157-649b20f10d4f
⛔ Files ignored due to path filters (21)
tests/fixtures/keys/ca2cert.pemis excluded by!**/*.pemtests/fixtures/keys/ca2key.pemis excluded by!**/*.pemtests/fixtures/keys/cacert.pemis excluded by!**/*.pemtests/fixtures/keys/cakey.pemis excluded by!**/*.pemtests/fixtures/keys/ec/ec-prime256v1-cert.pemis excluded by!**/*.pemtests/fixtures/keys/ec/ec-prime256v1-key.pemis excluded by!**/*.pemtests/fixtures/keys/ec/ec-prime256v1-pubkey.pemis excluded by!**/*.pemtests/fixtures/keys/ec/ec-prime384v1-cert.pemis excluded by!**/*.pemtests/fixtures/keys/ec/ec-prime384v1-key.pemis excluded by!**/*.pemtests/fixtures/keys/ec/ec-prime384v1-pubkey.pemis excluded by!**/*.pemtests/fixtures/keys/hmackey.binis excluded by!**/*.bintests/fixtures/keys/rsa/rsa-2048-cert-revoked-crl.pemis excluded by!**/*.pemtests/fixtures/keys/rsa/rsa-2048-cert.pemis excluded by!**/*.pemtests/fixtures/keys/rsa/rsa-2048-key.pemis excluded by!**/*.pemtests/fixtures/keys/rsa/rsa-2048-pubkey.pemis excluded by!**/*.pemtests/fixtures/keys/rsa/rsa-4096-cert.pemis excluded by!**/*.pemtests/fixtures/keys/rsa/rsa-4096-key.pemis excluded by!**/*.pemtests/fixtures/keys/rsa/rsa-4096-pubkey.pemis excluded by!**/*.pemtests/fixtures/keys/rsa/rsa-expired-cert.pemis excluded by!**/*.pemtests/fixtures/keys/rsa/rsa-expired-key.pemis excluded by!**/*.pemtests/fixtures/keys/rsa/rsa-expired-pubkey.pemis excluded by!**/*.pem
📒 Files selected for processing (43)
tests/c14n_golden.rstests/fixtures/c14n/c14n11/full-doc-c14n10.xmltests/fixtures/c14n/c14n11/full-doc-c14n11.xmltests/fixtures/c14n/c14n11/xml-base-input.xmltests/fixtures/c14n/merlin-c14n-three/Readme.txttests/fixtures/c14n/merlin-c14n-three/c14n-0.txttests/fixtures/c14n/merlin-c14n-three/c14n-1.txttests/fixtures/c14n/merlin-c14n-three/c14n-10.txttests/fixtures/c14n/merlin-c14n-three/c14n-11.txttests/fixtures/c14n/merlin-c14n-three/c14n-12.txttests/fixtures/c14n/merlin-c14n-three/c14n-13.txttests/fixtures/c14n/merlin-c14n-three/c14n-14.txttests/fixtures/c14n/merlin-c14n-three/c14n-15.txttests/fixtures/c14n/merlin-c14n-three/c14n-16.txttests/fixtures/c14n/merlin-c14n-three/c14n-17.txttests/fixtures/c14n/merlin-c14n-three/c14n-18.txttests/fixtures/c14n/merlin-c14n-three/c14n-19.txttests/fixtures/c14n/merlin-c14n-three/c14n-2.txttests/fixtures/c14n/merlin-c14n-three/c14n-20.txttests/fixtures/c14n/merlin-c14n-three/c14n-21.txttests/fixtures/c14n/merlin-c14n-three/c14n-22.txttests/fixtures/c14n/merlin-c14n-three/c14n-23.txttests/fixtures/c14n/merlin-c14n-three/c14n-24.txttests/fixtures/c14n/merlin-c14n-three/c14n-25.txttests/fixtures/c14n/merlin-c14n-three/c14n-26.txttests/fixtures/c14n/merlin-c14n-three/c14n-27.txttests/fixtures/c14n/merlin-c14n-three/c14n-3.txttests/fixtures/c14n/merlin-c14n-three/c14n-4.txttests/fixtures/c14n/merlin-c14n-three/c14n-5.txttests/fixtures/c14n/merlin-c14n-three/c14n-6.txttests/fixtures/c14n/merlin-c14n-three/c14n-7.txttests/fixtures/c14n/merlin-c14n-three/c14n-8.txttests/fixtures/c14n/merlin-c14n-three/c14n-9.txttests/fixtures/c14n/merlin-c14n-three/full-doc-c14n10.xmltests/fixtures/c14n/merlin-c14n-three/full-doc-c14n11.xmltests/fixtures/c14n/merlin-c14n-three/full-doc-exc-c14n.xmltests/fixtures/c14n/merlin-c14n-three/signature.xmltests/fixtures/c14n/merlin-exc-c14n-one/Readme.txttests/fixtures/c14n/merlin-exc-c14n-one/exc-signature.tmpltests/fixtures/c14n/merlin-exc-c14n-one/exc-signature.xmltests/fixtures/c14n/merlin-exc-c14n-one/full-doc-c14n10.xmltests/fixtures/c14n/merlin-exc-c14n-one/full-doc-exc-c14n.xmltests/fixtures_smoke.rs
xmllint --c14n/--c14n11/--exc-c14n always include XML nodes of all types in output. Document this in the module docstring to explain the with_comments:true default in assert_c14n_matches_golden.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/c14n_golden.rs`:
- Around line 66-69: The test slices result_str and golden_content by byte index
which can panic on multi-byte UTF-8 boundaries; replace the byte-slicing used in
the format call with char-safe previews by building short strings like let
preview_result = result_str.chars().take(500).collect::<String>(); and let
preview_golden = golden_content.chars().take(500).collect::<String>(); then use
preview_result and preview_golden in place of
&result_str[..result_str.len().min(500)] and
&golden_content[..golden_content.len().min(500)] in the failing message so
truncation never cuts a UTF-8 character (referencing result_str and
golden_content in the test).
Replace byte-level string slicing with chars().take(500) to avoid panic on multi-byte UTF-8 content in C14N output.
Update all GitHub Actions workflows to use actions/checkout@v6. Supersedes #1.
There was a problem hiding this comment.
Pull request overview
This PR adds donor test fixtures (keys, C14N reference vectors) and golden file tests for the C14N implementation. It provides a comprehensive test foundation for validating the canonicalization implementation against reference outputs from xmllint and Merlin's XML Security test suite.
Changes:
- Add test key fixtures (RSA 2048/4096, EC P-256/P-384, CA chain, HMAC) and C14N reference vectors (Merlin C14N three, exclusive C14N one, C14N 1.1) under
tests/fixtures/ - Add 13 fixture smoke tests in
tests/fixtures_smoke.rsvalidating PEM structure, file completeness, and structural integrity - Add 11 C14N golden file tests in
tests/c14n_golden.rs: 7 full-document byte-compare tests and 4 subtree SHA-1 digest tests against Merlin DigestValues
Reviewed changes
Copilot reviewed 63 out of 66 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/fixtures_smoke.rs | Smoke tests verifying fixture files are present, readable, and structurally valid |
| tests/c14n_golden.rs | Golden file tests comparing C14N output against xmllint references and subtree SHA-1 digests |
| tests/fixtures/keys/* | RSA, EC, CA chain, HMAC key fixtures from xmlsec1 test suite |
| tests/fixtures/c14n/merlin-c14n-three/* | Merlin C14N three test vectors (signature.xml, 28 golden outputs, 3 full-doc golden files) |
| tests/fixtures/c14n/merlin-exc-c14n-one/* | Merlin exclusive C14N one test vectors (exc-signature.xml/tmpl, 2 full-doc golden files) |
| tests/fixtures/c14n/c14n11/* | C14N 1.1 xml:base test vector (input XML, 2 full-doc golden files) |
You can also share your feedback on Copilot code review. Take the survey.
Replace silent if-let/flatten with unwrap_or_else so missing or unreadable fixture directories produce a clear panic message instead of a confusing "expected 62 vs 0" assertion.
There was a problem hiding this comment.
Pull request overview
This PR adds xmlsec1 donor test fixtures (RSA 2048/4096, EC P-256/P-384, CA chain, HMAC keys) and C14N reference vectors, along with comprehensive smoke tests and golden file tests validating the C14N implementation against xmllint reference outputs and Merlin DigestValues.
Changes:
- Added 62 test fixture files (21 key files + 41 C14N files) under
tests/fixtures/ - Added 13 fixture smoke tests (
tests/fixtures_smoke.rs) and 11 C14N golden file tests (tests/c14n_golden.rs) - Bumped
actions/checkoutfrom v4 to v6 in CI and release workflows
Reviewed changes
Copilot reviewed 63 out of 66 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/fixtures_smoke.rs | Smoke tests verifying fixture files exist and have valid PEM structure |
| tests/c14n_golden.rs | Golden file tests: 7 full-document byte-compare + 4 subtree SHA-1 digest tests |
| tests/fixtures/keys/* | RSA, EC, CA, HMAC test key files |
| tests/fixtures/c14n/* | C14N golden output files and source XML test vectors |
| .github/workflows/ci.yml | Bumps actions/checkout from v4 to v6 |
| .github/workflows/release.yml | Bumps actions/checkout from v4 to v6 |
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/fixtures_smoke.rs`:
- Around line 111-117: The test loop in tests/fixtures_smoke.rs currently only
checks path.exists() for each golden file (variables name and path in the for i
in 0..28 loop), which won't fail on unreadable files; change the assertion to
actually open or read the file (e.g., call std::fs::File::open(&path) or
std::fs::read_to_string(&path)) and assert that the open/read succeeds,
including the error in the assertion message so permission/corruption issues
surface during the test run.
- Line 104: Change the five test functions to return Result<(), Box<dyn
std::error::Error>> and replace each fs::read_to_string(...).expect("...") with
the ? operator to propagate I/O errors; specifically update
merlin_c14n_three_non_empty_goldens_are_xml,
merlin_exc_c14n_one_input_files_present, c14n11_xml_base_input_present and the
test defining sig_xml so that all read_to_string calls use ? and each test ends
with Ok(()). Ensure signatures and returns are updated accordingly and no other
expect() calls remain for these fixture reads.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tests/fixtures/Technical Details
xmllint --c14n/--c14n11/--exc-c14nexc-signature.xmlgenerated by our implementation (not xmllint) because xmllint incorrectly rendersxmlns:baron<Foo>wherebaris not visibly utilized — our implementation follows Exclusive C14N spec §3<dsig:Object Id="to-be-signed">from Merlin exc-c14n-one, canonicalize with 4 exclusive C14N configurations, and compare SHA-1 digests against the original DigestValues embedded in the signed XMLKnown Limitations
exc-signature.tmplincluded for future signing testsTest Plan
cargo test --workspace— 206 tests passcargo clippy --all-targets --all-features -- -D warnings— cleancargo build— cleanCloses #15
Summary by CodeRabbit