Repository navigation
Conversation
A document whose SignatureMethod is rsa-sha384, or whose DigestMethod is sha384, could be neither signed nor verified: both names were missing from the algorithm maps. XMLDSig 1.1 lists them as optional (6.2.4, 6.4.2) and RFC 9231 defines the identifiers (2.1.3, 2.3.3). .NET, the JDK, goxmldsig, xmlsec and ruby-saml all have both. Adds `Sha384` and `RsaSha384`, registers them by default, and names the two identifiers in the algorithm type unions and the README. The fixture is signed by JDK 21. One test verifies it, and another checks that signing the same document here writes the same DigestValue and SignatureValue. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (9)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe change adds SHA-384 digest and RSA-SHA384 signature support. It updates the algorithm types and registries, documents the supported algorithms, and adds a signed XML fixture with integration and unit test coverage. ChangesSHA-384 support
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~12 minutes Change: Feature Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue remains from this review; the change is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 6 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
…next major rejects `validateSignatureForRedirect` still falls back to a lookup among OpenSSL digest names for a `SigAlg` xml-crypto does not implement. It accepts rsa-sha224, rsa-md5 and names that are no identifier at all, such as a bare `RSA-SHA256`. The next major version removes it (node-saml#489). A signature that verifies through the lookup now logs a warning under `NODE_DEBUG=node-saml`, and the README says which identifiers remain. rsa-sha384 is left out of the warning: the next major keeps it through xml-crypto (node-saml/xml-crypto#654). Closes node-saml#490. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
xml-crypto has SHA-1, SHA-256 and SHA-512, and no SHA-384. A document whose
SignatureMethodisrsa-sha384fails withsignature algorithm '…#rsa-sha384' is not supported, one whoseDigestMethodissha384fails withhash algorithm '…#sha384' is not supported, and neither name can be used to sign.XMLDSig 1.1 lists both as optional (§6.2.4, §6.4.2), and RFC 9231 defines the identifiers (§2.1.3, §2.3.3). The implementations a signed document is likely to come from or go to have both:
rsa-sha384sha384digestmasterSystem.Security.Cryptography.Xmljavax.xml.crypto.dsignode-saml is the caller that needs it now. Since node-saml/node-saml#479 it takes the algorithm for a Redirect-binding signature from
SignedXml'sSignatureAlgorithms, and it keeps a second lookup among OpenSSL digest names only for the identifiers that map lacks.rsa-sha384is the one of those worth keeping, so that lookup can go once this is released.This PR adds:
Sha384, the digesthttp://www.w3.org/2001/04/xmldsig-more#sha384.RsaSha384, the signaturehttp://www.w3.org/2001/04/xmldsig-more#rsa-sha384: PKCS Initial support for enveloped signatures #1 v1.5, asrsa-sha256andrsa-sha512are.HashAlgorithmsandSignatureAlgorithms, so a document that names them verifies with no configuration, like the other SHA-2 sizes.HashAlgorithmTypeandSignatureAlgorithmType, and in the README's lists. Both unions already end instring, so this changes editor completion and nothing else.Left out on purpose:
sha384-rsa-MGF1,sha512-rsa-MGF1). Nothing here needs them.Tests
test/static/rsa_sha384_signature.xmlis signed by JDK 21 withclient.pem, usingrsa-sha384over asha384digest. It verifies, and stops verifying when the signed content changes.DigestValueandSignatureValuethe JDK wrote. PKCS Initial support for enveloped signatures #1 v1.5 is deterministic, so they are equal byte for byte.rsa-sha384joins the sign-and-verify list intest/signature-unit-tests.spec.ts.All five tests failed before the change with the two
is not supportederrors above. Each also fails when SHA-256 is put in place of SHA-384 inRsaSha384's signing, in its verifying, or inSha384.Checks
npm run build && npm test && npm run lintpass (555 tests).rsa-sha384and asha384digest over anIdreference, and rejects it after an edit.🤖 Generated with Claude Code
Summary by CodeRabbit