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

Make Ed25519 KeyFactory registration Opt-in #410

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

amirhosv
Copy link
Contributor

@amirhosv amirhosv commented Nov 8, 2024

Description of changes:

  • The keys generated by KeyFactory of ACCP for Ed25519 do not implement EdEcKey interface from JCA since this interface is not present in JDKs prior to 15.
  • Once ACCP supports multi-jar release, then this option can be removed and the KeyFactory for Ed25519 can always be registered.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@amirhosv amirhosv marked this pull request as ready for review November 8, 2024 15:09
@amirhosv amirhosv requested a review from a team as a code owner November 8, 2024 15:09

// In case the user does not register Ed25519 KeyFactories by ACCP, we still need one to be used
// internally.
private static class EdKeyFactory extends KeyFactory {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this belongs inside the AmazonCorrettoCryptoProvider class, way too specific to a specific algorithm to be here. Maybe make it a standalone class or move it to the EvpKeyFactory class instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

KeyFactory is not intended to be instantiated explicitly: one should get an instance by calling the static factory methods KeyFactory::getInstance.
This usage is not common and EdKeyFactory is only used by translateKey. I think it's better to be kept in this class.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we could do it all over again, I'd avoid having translateKey in this class as well. Way too much stuff inside here right now and it'd all benefit from a good refactor. Won't block the PR over it though.

README.md Show resolved Hide resolved
+ The keys generated by KeyFactory of ACCP for Ed25519 do not implemet
  EdEcKey interface from JCA since this interface is not present to JDKs
  prior to 15.
@amirhosv amirhosv merged commit c1709e7 into corretto:main Nov 13, 2024
11 checks passed
@amirhosv amirhosv deleted the EdKeyFactory branch November 13, 2024 15:36
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.

4 participants