-
Notifications
You must be signed in to change notification settings - Fork 398
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 EdDSA algorithm support #378
Conversation
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.
Out of curiosity, what providers support this?
Would it be possible to add some tests that actually exercise the signing and verification? E.g. take a look at jwks_test.go verify_test.go
If not, I can take a crack at it
oidc/oidc.go
Outdated
@@ -450,6 +452,8 @@ func (i *IDToken) VerifyAccessToken(accessToken string) error { | |||
h = sha512.New384() | |||
case RS512, ES512, PS512: | |||
h = sha512.New() | |||
case EdDSA: | |||
return errUnsupportedAlgorithm |
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.
wouldn't this be sha512.New()?
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 believe that verifying EdDSA key with ed25519 protocol is a bit more complicated than the pattern that exists here short explanation
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.
This is only for the at_hash value, it's not doing full verification :)
Here is an example provider that supports EdDSA. I was not originally planning on adding verification functionality as it is more complicated for ed25519 protocol. |
Did you mean to hyperlink something in that comment? I don't think I'd want to merge this if it only implements partial support. I can add verification functionality when I get a sec. |
9f7f5bd
to
46e8a76
Compare
I added some additional tests and took your suggestion for using sha512.New() to verify the hash. I am not super knowledgeable in this space so I am not confident in that change. P.S. closing the PR was a mistake |
Support EdDSA alogrithm for providers.
46e8a76
to
5337bf1
Compare
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.
Awesome, thanks for sending the full change. Lgtm :)
If there are any issues with the at_hash support, we can always fix it up later. I'm not super worried about it. |
Allows for providers to use EdDSA algorithm but does not support verifying access tokens.