feat: Ed25519 signatures, XML Encryption 1.1, and crypto.Signer support#101
feat: Ed25519 signatures, XML Encryption 1.1, and crypto.Signer support#101leifj wants to merge 6 commits intomoov-io:masterfrom
Conversation
This adds comprehensive XML Encryption 1.1 support including: - AES Key Wrap (RFC 3394) - AES-128/192/256-KW - X25519 Key Agreement with HKDF key derivation (RFC 5869) - AES-GCM and AES-CBC content encryption - Complete XML Encryption types: EncryptedData, EncryptedKey, AgreementMethod, KeyDerivationMethod - XML serialization/parsing compatible with W3C XML Encryption 1.1 EU eDelivery AS4 2.0 Compatibility: - X25519 key agreement - HKDF key derivation with HMAC-SHA256 - AES-128-KW key wrapping - AES-128-GCM content encryption Also adds Ed25519 signature support for XML Digital Signatures. Test coverage includes: - RFC 3394 official NIST test vectors - W3C XML Encryption interop test cases - Full encryption/decryption round-trip tests
The parseAgreementMethod function was missing support for parsing OriginatorKeyInfo and RecipientKeyInfo elements, which contain the ephemeral public key needed for X25519 key agreement during decryption. This fix enables full round-trip XML serialization/deserialization of EncryptedData elements with X25519 key agreement.
Remove the hard dependency on crypto11/PKCS#11 by using the standard crypto.Signer interface. This allows signedxml to work with: - Standard Go private keys (*rsa.PrivateKey, ed25519.PrivateKey) - PKCS#11/HSM signers (crypto11 implements crypto.Signer) - Any other crypto.Signer implementation This removes the CGO dependency from signedxml itself, making it easier to build static binaries. PKCS#11 support is still available when the caller provides a crypto11.Signer or any other crypto.Signer. BREAKING CHANGE: None - crypto11.Signer embeds crypto.Signer, so all existing code continues to work.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the cryptographic capabilities of the library by integrating modern signature and encryption standards. The addition of Ed25519 signatures enhances security and interoperability, particularly for emerging protocols. Furthermore, the introduction of a dedicated XML Encryption 1.1 package provides robust data confidentiality features, supporting key agreement, key wrapping, and content encryption. A crucial architectural improvement is the adoption of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant new features: Ed25519 signature support, a comprehensive XML Encryption 1.1 implementation in a new xmlenc package, and a refactoring to use the crypto.Signer interface, which improves support for HSMs. The changes are extensive and well-tested, particularly the new xmlenc package which includes W3C interop and NIST test vectors.
My review has identified a few areas for improvement. The most important is updating the very old golang.org/x/crypto dependency to a recent version to mitigate potential security risks. Additionally, there are several places in the new XML parsing logic where errors from base64 decoding are ignored, which could lead to panics or incorrect behavior with malformed input. I've also noted a minor issue in a test file where an error is ignored.
Overall, this is a great contribution that adds valuable functionality. Addressing the identified issues will improve the robustness and security of the new code.
Incorporates all review feedback from upstream PRs: - PR moov-io#98 (c14n-fixes): WriteToString error handling, redundant assignment removal - PR moov-io#99 (rsa-pss): getDigestAlgorithm helper, rsaPSSHashAlgorithms shared map - PR moov-io#100 (ecdsa-fix): hash messages before ECDSA sign/verify in tests, nolint:exhaustive - PR moov-io#101 (ed25519-xmlenc-cryptosigner): error handling in xmlenc parse functions, gosec nolint for safe integer conversions, NewSigner error check in tests, .gitleaks.toml for W3C test vectors Also updates golang.org/x/crypto to v0.49.0.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for XML Encryption 1.1, including AES-GCM, AES-CBC, AES Key Wrap, X25519 Key Agreement with HKDF key derivation, and Ed25519 XML signatures. It adds a new xmlenc package with encryption/decryption functionalities, key agreement mechanisms, and extensive W3C interoperability tests. Additionally, it updates the Go version, adds golang.org/x/crypto dependency, and improves XML canonicalization by handling inclusive namespaces and skipping external references. Feedback includes addressing a hardcoded SHA256 hash function in the HKDF implementation, defining URIs for HMAC-SHA384 and HMAC-SHA512, and clarifying a misleading comment about uint64 overflow in the key wrap logic.
| hashFunc := sha256.New | ||
|
|
||
| var salt []byte | ||
| var info []byte | ||
|
|
||
| if params != nil { | ||
| salt = params.Salt | ||
| info = params.Info | ||
| if params.KeyLength > 0 { | ||
| keyLength = params.KeyLength / 8 // Convert bits to bytes | ||
| } | ||
| } |
There was a problem hiding this comment.
The hashFunc for HKDF is hardcoded to sha256.New, which ignores the PRF field in HKDFParams. This prevents the use of other hash functions like HMAC-SHA384 or HMAC-SHA512. The implementation should select the hash function based on params.PRF.
You will also need to add "crypto/sha512" to the imports.
hashFunc := sha256.New
var salt []byte
var info []byte
if params != nil {
if params.PRF != "" {
switch params.PRF {
case AlgorithmHMACSHA256:
hashFunc = sha256.New
case AlgorithmHMACSHA384:
hashFunc = sha512.New384
case AlgorithmHMACSHA512:
hashFunc = sha512.New
default:
return nil, fmt.Errorf("unsupported PRF algorithm: %s", params.PRF)
}
}
salt = params.Salt
info = params.Info
if params.KeyLength > 0 {
keyLength = params.KeyLength / 8 // Convert bits to bytes
}
}| AlgorithmSHA256 = "http://www.w3.org/2001/04/xmlenc#sha256" | ||
| AlgorithmSHA384 = "http://www.w3.org/2001/04/xmlenc#sha384" | ||
| AlgorithmSHA512 = "http://www.w3.org/2001/04/xmlenc#sha512" | ||
| AlgorithmHMACSHA256 = "http://www.w3.org/2001/04/xmldsig-more#hmac-sha256" |
There was a problem hiding this comment.
To fully support HMAC-SHA384 and HMAC-SHA512 for HKDF as mentioned in the PR description, you should also define their algorithm URIs here. This will allow the key derivation logic to correctly select the hash function.
| AlgorithmHMACSHA256 = "http://www.w3.org/2001/04/xmldsig-more#hmac-sha256" | |
| AlgorithmHMACSHA256 = "http://www.w3.org/2001/04/xmldsig-more#hmac-sha256" | |
| AlgorithmHMACSHA384 = "http://www.w3.org/2001/04/xmldsig-more#hmac-sha384" | |
| AlgorithmHMACSHA512 = "http://www.w3.org/2001/04/xmldsig-more#hmac-sha512" |
| for j := 5; j >= 0; j-- { | ||
| for i := n; i >= 1; i-- { | ||
| // t = n*j+i | ||
| t := uint64(n*j + i) //nolint:gosec // n<=6, j<=5, i<=n; overflow impossible |
There was a problem hiding this comment.
The comment // n<=6, j<=5, i<=n; overflow impossible is misleading. The value of n is derived from the ciphertext length and is not limited to 6. For example, wrapping a 512-bit key would result in n=8. While an overflow of t is practically unlikely for typical key sizes, the comment should be corrected to avoid confusion. A better explanation would be that uint64 is large enough for practical values of n.
|
Do you want to fixup this PR's merge conflicts and address the hardcoded HKDF? |
|
This PR brings a refactor that I’ve been thinking about proposing for a long time. Thank you for making this :) I would suggest just one thing: the type of the Also, if I am not mistaken, the majority of the logic in |
|
Thanks @leifj - I've included these commits on master - couldn't push to this PR, but the tests pass. |
3 participants
Summary
Add Ed25519 (EdDSA) signature support, a comprehensive XML Encryption 1.1 package, and refactor signing to use the standard
crypto.Signerinterface for HSM/PKCS#11 compatibility.Ed25519 Signature Support
http://www.w3.org/2021/04/xmldsig-more#eddsa-ed25519algorithm (RFC 9231 Section 2.3.1)ed25519_test.goXML Encryption 1.1 Package (
xmlenc/)New
xmlencsub-package implementing W3C XML Encryption 1.1:EU eDelivery AS4 2.0 compatible: X25519 + HKDF-SHA256 + AES-128-KW + AES-128-GCM
Test coverage includes RFC 3394 NIST vectors, W3C interop test cases, and full round-trip tests.
crypto.SignerRefactorReplace concrete key type assertions with the standard
crypto.Signerinterface:crypto.Signerwhen the key is not*rsa.PrivateKeycrypto.SignerwithHash(0)for no-pre-hashP11SignerOptsimplementingcrypto.SignerOptsProcessElementhelper for canonicalizing elements with namespace contextThis enables HSM/PKCS#11 signers (which implement
crypto.Signer) without requiring a direct dependency on any PKCS#11 library. No CGO dependency is introduced.Other Enhancements
cid:URI references in digest calculation (MIME attachments in WS-Security)Testing
All existing tests pass. New test files:
ed25519_test.go— Ed25519 sign/verify round-tripxmlenc/*_test.go— Key wrap, key agreement, encryption, W3C interop