-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SANTUARIO-511: Implementation of the Diffie-Hellman-ES key exchange for EC and XEC keys #234
SANTUARIO-511: Implementation of the Diffie-Hellman-ES key exchange for EC and XEC keys #234
Conversation
…r main branch (4.0.x)
src/main/java/org/apache/xml/security/keys/derivedKey/KeyDerivationMethodImpl.java
Fixed
Show fixed
Hide fixed
@jrihtarsic Please see the codeql comments |
src/main/java/org/apache/xml/security/encryption/params/ConcatKeyDerivationParameter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/params/KeyDerivationParameter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/keys/content/AgreementMethodImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/keys/content/DEREncodedKeyValue.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/keys/derivedKey/ConcatKDF.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/keys/derivedKey/ConcatKDF.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/keys/derivedKey/ConcatKDF.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/keys/derivedKey/DerivationAlgorithm.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/xml/security/testutils/KeyTestUtils.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/xml/security/testutils/KeyTestUtils.java
Outdated
Show resolved
Hide resolved
@phax many thanks for all of the the comments. Let me know if you spot anything else. |
src/test/java/org/apache/xml/security/testutils/KeyTestUtils.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot comment on the crypto stuff itself, so please don't bother too much with my notes :)
src/main/java/org/apache/xml/security/encryption/params/ConcatKeyDerivationParameter.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/xml/security/testutils/JDKTestUtils.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/xml/security/testutils/JDKTestUtils.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/xml/security/testutils/JDKTestUtils.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/xml/security/testutils/JDKTestUtils.java
Outdated
Show resolved
Hide resolved
Let me know please when this is ready for review |
I like it ;-) Thanks @jrihtarsic for all the changes 😍 |
src/main/java/org/apache/xml/security/keys/OriginatorKeyInfo.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/keys/content/AgreementMethodImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments. I still need to do another couple of review passes thru the code though.
src/main/java/org/apache/xml/security/encryption/keys/RecipientKeyInfo.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/params/ConcatKeyDerivationParameter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/keys/KeyInfoEncExtension.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more comments. Probably will need one more round of review.
src/main/java/org/apache/xml/security/utils/Encryption11ElementProxy.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/utils/EncryptionElementProxy.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/utils/EncryptionElementProxy.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/resource/xmlsecurity_en.properties
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/resource/xmlsecurity_en.properties
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/resource/xmlsecurity_en.properties
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/keys/content/KeyValue.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/keys/content/DEREncodedKeyValue.java
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/params/KeyDerivationParameter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/params/ConcatKeyDerivationParameter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/exceptions/DERDecodingException.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/params/KeyAgreementParameterSpec.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/params/KeyDerivationParameter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/params/KeyAgreementParameterSpec.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/XMLCipher.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/XMLCipher.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/XMLCipher.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/XMLCipher.java
Outdated
Show resolved
Hide resolved
As a follow-on enhancement, it would probably be useful to implement HKDF (see https://www.rfc-editor.org/rfc/rfc9231#name-hkdf-key-derivation) which seems to be more relevant than ConcatKDF. |
@seanjmullan I have already prepared an implementation for HKDF a few weeks ago. I did not add it to this one since the change is already big and it is easier to follow changes with smaller and feature-oriented PRs ) Here is the branch I will prepare a PR for that as soon as this one is merged. |
src/main/java/org/apache/xml/security/encryption/params/KeyDerivationParameter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/params/ConcatKeyDerivationParameter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/params/KeyAgreementParameterSpec.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/params/KeyAgreementParameterSpec.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/params/KeyAgreementParameterSpec.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/keys/content/KeyValue.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/keys/content/KeyValue.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/utils/DERDecoderUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/utils/DERDecoderUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/keys/OriginatorKeyInfo.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/keys/RecipientKeyInfo.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/keys/KeyInfoEnc.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/keys/KeyInfoEnc.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/keys/content/AgreementMethodImpl.java
Outdated
Show resolved
Hide resolved
...ain/java/org/apache/xml/security/encryption/keys/content/derivedKey/DerivationAlgorithm.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/xml/security/encryption/keys/content/derivedKey/ConcatKDF.java
Outdated
Show resolved
Hide resolved
I am pretty much done with my review. I think the javadoc could be improved in various methods/classes but I don't want to hold this up anymore and this can probably be improved in the next revision or over time. I did not review the tests, so hopefully those are ok or someone else has reviewed them. Sorry it took so long. I'll do a final sanity check tomorrow on your latest commits and then if all looks good, add my approval. |
@seanjmullan Thank you very much for your valuable comments and suggestions. I will prepare the next PR for HKDF in the next week, where I will also focus more on code documentation. |
…or EC and XEC keys (#234) * Implementation of the Diffie-Hellman agreement for EC and XEC keys for main branch (4.0.x) * Fix CodeQL warnings * Update for the PR comments * Update for the PR comments part 2 * Update for the PR comments part 3 * Update for the PR comments part 4 * Update for the PR comments part 4 * Update for the PR comments part 5 * Improve code quality * Update for the PR comments part 6 * Move encryption specific classes to org.apache.xml.security.encryption package. * Fix the PR comments * Added DEREncodedKeyValue for DH and RSASSA-PSS keys, other PR fixes * PR updates on usage of the AlgorithmParameterSpec interface * PR updates for method XMLCipher.encryptKey * PR update descriptions and class names * PR update descriptions and class names * PR - fix typos and javadoc * PR updates --------- Co-authored-by: RIHTARSIC Joze <[email protected]>
Details of the task are here: https://issues.apache.org/jira/browse/SANTUARIO-511