Tests for sha256-rsa-MGF1 #515
Conversation
valid_saml_sha256_rsa_mgf1.xml has been signed with:
xmlsectool --sign \
--inFile unsigned_saml_response.xml \
--outFile valid_saml_sha256_rsa_mgf1.xml \
--keyFile idp_private_key.pem \
--certificate idp_certificate.pem \
--signatureAlgorithm http://www.w3.org/2007/05/xmldsig-more#sha256-rsa-MGF1
WalkthroughAdds new unit tests for SAML XML signature verification (including SHA-256 with RSA-MGF1), plus three static SAML XML fixtures (valid signed, invalid/tampered signed, and unsigned). Tests exercise positive validation, tamper detection, and parameterized sign-and-verify checks across multiple XMLDSig RSA algorithms. No production code or public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test Runner
participant F as Fixture (XML)
participant P as xmldom Parser
participant S as SignedXml Validator
T->>F: Load SAML response XML
T->>P: Parse XML into DOM
P-->>T: DOM with ds:Signature node
T->>S: Initialize SignedXml with Signature node & cert
S->>S: Compute digest(s) and verify signature (RSA variants)
alt signature valid
S-->>T: true (verification passes)
else signature invalid / tampered
S-->>T: false (verification fails)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
test/static/idp_certificate.pemis excluded by!**/*.pemtest/static/idp_private_key.pemis excluded by!**/*.pem
📒 Files selected for processing (5)
test/saml-response-tests.spec.ts(1 hunks)test/signature-unit-tests.spec.ts(1 hunks)test/static/invalid_saml_sha256_rsa_mgf1.xml(1 hunks)test/static/unsigned_saml_response.xml(1 hunks)test/static/valid_saml_sha256_rsa_mgf1.xml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/signature-unit-tests.spec.ts (1)
src/signed-xml.ts (2)
SignedXml(29-1291)loadSignature(604-693)
🔇 Additional comments (8)
test/static/valid_saml_sha256_rsa_mgf1.xml (1)
1-94: LGTM! Well-formed test fixture.This static test fixture properly represents a SAML 2.0 Response signed with the sha256-rsa-MGF1 algorithm. The XML structure is valid, the signature block follows XMLDSig standards, and all required elements are present.
test/static/invalid_saml_sha256_rsa_mgf1.xml (1)
1-94: LGTM! Appropriate negative test fixture.This fixture correctly simulates a tampered SAML response by modifying the NameID value (line 78) while keeping the original signature intact. This is an effective approach for testing signature verification failure scenarios.
test/saml-response-tests.spec.ts (3)
26-40: LGTM! Well-structured positive test case.This test correctly validates a SAML response signed with sha256-rsa-MGF1. It follows the established testing pattern and includes appropriate assertions.
42-56: LGTM! Appropriate negative test case.This test correctly verifies that signature verification fails for a tampered SAML response. The test structure and assertions are appropriate for detecting tampering.
35-35: Certificate file verified.
Verified thattest/static/idp_certificate.pemexists; no further action required.test/signature-unit-tests.spec.ts (2)
9-14: LGTM! Well-defined algorithm list for parameterized testing.The constant provides a clear list of RSA signature algorithms to test, including the newly supported sha256-rsa-MGF1. This enables comprehensive parameterized testing across all supported algorithms.
18-74: LGTM! Excellent parameterized test suite.This test suite provides comprehensive coverage for all RSA signature algorithms through parameterized testing:
- Helper functions:
signWithandloadSignaturepromote code reuse and maintainability- Positive testing: Verifies that signatures created with each algorithm validate successfully
- Negative testing: Ensures tampered content is detected for all algorithms
- Consistent approach: All algorithms are tested using the same logic
This design makes it easy to add support for additional algorithms in the future.
test/static/unsigned_saml_response.xml (1)
1-46: LGTM! Valid unsigned SAML fixture.This is a well-formed unsigned SAML 2.0 Response fixture with a complete assertion structure. However, it doesn't appear to be used by any tests in this PR. If it's intended for future use, consider adding a comment in the code or commit message explaining its purpose. If it's not needed, consider removing it to keep the test fixtures minimal.
|
@kaibernhard , I was all set to merge this, but it needs to have |
|
@cjbarth thanks a lot, I merged master and hope we're good to go now. I would be happy to help on the related PR node-saml/node-saml#387 as well. |
|
@kaibernhard , it appears that some linting needs to be done too. |
|
@cjbarth oops, missed that, it's late here 😄 |
2 participants
This PR adds tests for the signature algorithm sha256-rsa-MGF1 as implemented in
#488
Summary by CodeRabbit