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

Adding canonical Keyfunc functions for RSA, ECDSA, EdDSA and HMAC #275

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

oxisto
Copy link
Collaborator

@oxisto oxisto commented Feb 21, 2023

This PR adds ready-to-use keyfunc functions for the various signing methods. This should simplify a lot of standard use-cases and also includes a proper signing method check.

This allows for a much cleaner experience in probably 90% of all use cases:

token, err := jwt.ParseWithClaims(
  tokenString, &MyCustomClaims{},
  jwt.PresharedKey([]byte("AllYourBase")),
  jwt.WithLeeway(5*time.Second),
)
token, err := jwt.ParseWithClaims(
  tokenString, &MyCustomClaims{},
  jwt.RSAPublicKey(myKey),
  jwt.WithAudience("http://example.com"),
)

@oxisto oxisto requested a review from mfridman February 21, 2023 11:46
@oxisto oxisto deleted the branch golang-jwt:main February 21, 2023 13:32
@oxisto oxisto closed this Feb 21, 2023
@oxisto oxisto reopened this Feb 21, 2023
@oxisto oxisto changed the base branch from v5 to main February 21, 2023 13:38
@oxisto oxisto changed the base branch from main to v5 February 21, 2023 13:44
@oxisto oxisto changed the base branch from v5 to main February 21, 2023 13:46
@oxisto oxisto added this to the v5.0.0 milestone Feb 21, 2023
@oxisto oxisto linked an issue Feb 21, 2023 that may be closed by this pull request
This PR adds ready-to-use keyfunc functions for the various signing methods. This should simplify a lot of standard use-cases and also includes a proper signing method check.
Copy link
Member

@mfridman mfridman left a comment

Choose a reason for hiding this comment

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

I've gone back-forth on this PR. On one hand, it's a nice quality of life improvement for the most common use cases.

On the other hand, the caller needs to decide whether to use this helper function or pass their own function, using the exported structs and calling .Alg().

Also, I've often needed access to the header bits for additional validation, refresh, etc. and so always constructed this myself.

Not against this, just want to make sure we're expanding the API surface area where it makes sense.

@oxisto
Copy link
Collaborator Author

oxisto commented Apr 10, 2023

I've gone back-forth on this PR. On one hand, it's a nice quality of life improvement for the most common use cases.

On the other hand, the caller needs to decide whether to use this helper function or pass their own function, using the exported structs and calling .Alg().

Also, I've often needed access to the header bits for additional validation, refresh, etc. and so always constructed this myself.

Not against this, just want to make sure we're expanding the API surface area where it makes sense.

Yeah same here, the main reason for this was because we had a lot of issues that were not sure how to pass public/private keys properly. This is now somewhat mitigated by the documentation page. I still think it's a nice, clean way of using this library, though.

Maybe we can keep it as "approved", but not merge it yet and aim for a 5.1 release.

@oxisto oxisto mentioned this pull request Aug 14, 2023
10 tasks
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.

Keyfunc usage and token Parsing examples
2 participants