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

Support ES256 JWK Algo #628

Merged
merged 2 commits into from
Oct 25, 2017
Merged

Support ES256 JWK Algo #628

merged 2 commits into from
Oct 25, 2017

Conversation

joshuarubin
Copy link

@joshuarubin joshuarubin commented Oct 24, 2017

I think this is all that's necessary to support #627

It looks like support for ES256 is available (jwk/generator_ecdsa256.go), but that the JWK handler doesn't support it (https://github.com/ory/hydra/blob/master/jwk/handler.go#L27-L38).

Go's P-256 is implementation is constant-time (which prevents certain types of attacks) while its P-384 and P-521 are not (https://github.com/gtank/cryptopasta).

Signed-off-by: Joshua Rubin <jrubin@zvelo.com>
@joshuarubin joshuarubin changed the title support es256 Support ES256 JWK Algo Oct 24, 2017
Signed-off-by: Joshua Rubin <jrubin@zvelo.com>
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Thank you for your changes, again :)

However, I can't support further divergence of 0.9.x and 0.10.x - 0.9 is advertised as pre-relase and not LTS and I don't want to give the impression that 0.9 is encouraged. If you have issues with upgrading to 0.10 I can help you with that, as you've got so much done here.

@joshuarubin
Copy link
Author

I agree with you, that's why I have updated the master branch of our repo to be in sync with your master and all my recent changes. After you approved this, I planned on making that pull request. You can see it here https://github.com/zvelo/hydra

@aeneasr
Copy link
Member

aeneasr commented Oct 25, 2017

Ok awesome, that works for me - I'll make a code review and merge if everything checks out!

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

The only question that remains is if we want to keep ES521 as a standard generator. Originally, it was included because the JOSE spec says that ES256 might be deprecated some time in the future.

@@ -27,6 +27,6 @@ var keysCreateCmd = &cobra.Command{

func init() {
keysCmd.AddCommand(keysCreateCmd)
keysCreateCmd.Flags().StringP("alg", "a", "", "REQUIRED name that identifies the algorithm intended for use with the key. Supports: RS256, ES521, HS256")
keysCreateCmd.Flags().StringP("alg", "a", "", "REQUIRED name that identifies the algorithm intended for use with the key. Supports: RS256, ES256, ES521, HS256")
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we should remove ES521 completely?

Copy link
Author

Choose a reason for hiding this comment

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

@aeneasr aeneasr merged commit d7b7858 into ory:v0.9.x Oct 25, 2017
@mitar mitar mentioned this pull request Apr 9, 2021
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