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 EdDSA algorithm support #378

Merged
merged 1 commit into from May 16, 2023
Merged

Conversation

lritter14
Copy link

Allows for providers to use EdDSA algorithm but does not support verifying access tokens.

Copy link
Collaborator

@ericchiang ericchiang left a 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
Copy link
Collaborator

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()?

Copy link
Author

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

Copy link
Collaborator

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 :)

@lritter14
Copy link
Author

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

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.

@ericchiang
Copy link
Collaborator

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.

@lritter14
Copy link
Author

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.
Copy link
Collaborator

@ericchiang ericchiang 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 sending the full change. Lgtm :)

@ericchiang
Copy link
Collaborator

If there are any issues with the at_hash support, we can always fix it up later. I'm not super worried about it.

@ericchiang ericchiang merged commit cd52382 into coreos:v3 May 16, 2023
1 check passed
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.

None yet

3 participants