Skip to content
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

Add support for X25519 algorithm alias to XDH #1156

Merged
merged 2 commits into from
Aug 15, 2023

Conversation

exceptionfactory
Copy link
Contributor

This pull request adds X25519 as a supported algorithm name alias for KeyPairGenerator, KeyFactory, and KeyAgreement implementations in the OpenSSLProvider.

Based on the work completed in PR #950 providing the X25519 implementation, this pull request allows integrating libraries to use the X25519 alias instead of XDH.

The alias references align with the JDK 11 implementation of Curve25519 Key Agreement described in JEP 324, which notes that referencing the specific algorithm name provides convenient shorthand for KeyAgreement and related components. It is also worth noting that the JDK 11 implementation returns XDH for calls to getAlgorithm() on generated keys, and this alias strategy follows the same convention.

@prbprbprb
Copy link
Collaborator

Thanks very much for this! I noticed it was missing but hadn't got around to it.

However, it should be done as an alias rather than a top-level entry, e.g. instead of

        put("KeyPairGenerator.X25519", PREFIX + "OpenSSLXDHKeyPairGenerator");

you need

        put("Alg.Alias.KeyPairGenerator.X25519", "XDH");

and so on. That way you should need fewer test changes too.

Are you OK with making this change?

@prbprbprb prbprbprb self-requested a review August 15, 2023 15:19
* Added X25519 aliases for KeyPairGenerator, KeyFactory, and KeyAgreement in OpenSSLProvider
@exceptionfactory
Copy link
Contributor Author

Thanks for the review and feedback @prbprbprb!

I adjusted the alias settings to use the appropriate Alg.Alias names. I also removed most of the unit test changes and rebased from the current master branch.

I retained the X25519 Key Agreement test and related test changes, but if you don't think that is necessary, I can also remove it as well.

Copy link
Collaborator

@prbprbprb prbprbprb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for the quick turnaround!

@prbprbprb prbprbprb merged commit e65479f into google:master Aug 15, 2023
4 of 5 checks passed
@prbprbprb
Copy link
Collaborator

Good news, you can use that in source builds right away. Bad news, the earliest it will be available in the Android platform is November.

Plus we plan a maven release "soon" once we finish figuring out Gradle issues.

@exceptionfactory
Copy link
Contributor Author

Good news, you can use that in source builds right away. Bad news, the earliest it will be available in the Android platform is November.

Plus we plan a maven release "soon" once we finish figuring out Gradle issues.

Thanks again @prbprbprb!

prbprbprb added a commit to prbprbprb/conscrypt that referenced this pull request Sep 5, 2023
And add missing tests to the OpenJDK suite which
is how this got missed in google#1156.
prbprbprb added a commit that referenced this pull request Sep 5, 2023
And add missing tests to the OpenJDK suite which
is how this got missed in #1156.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants