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 jwt.Keyfunc utilities and partial JWK support #105

Closed
wants to merge 16 commits into from
Closed

Add jwt.Keyfunc utilities and partial JWK support #105

wants to merge 16 commits into from

Conversation

MicahParks
Copy link
Contributor

@MicahParks MicahParks commented Sep 17, 2021

Purpose

The purpose of this Pull Request is to add jwt.Keyfunc utilities found in github.com/MicahParks/keyfunc.

These utilities include creating a jwt.Keyfunc from a given set of cryptographic keys and or a local/remote JWKs. When using a remote JWKs there are additional features such as a background refresh goroutine. For additional details on the provided features, the README.md in the keyfunc directory is a good place to start.

This would add partial JWK support for this project and help make progress towards this issue: #73.

It would be ideal for those who found github.com/MicahParks/keyfunc useful to only need to import one JWT pacakge. Here is an example of a PR that avoided importing github.com/MicahParks/keyfunc and resorted to copying due to the supported forks being dependencies: gofiber/jwt#57.

I believe I could get away with smaller changes if I would reference https://github.com/MicahParks/keyfunc directly. But that would bring additional unused legacy dependencies to this middleware and to the rest of the projects which will be using it, so I decided to copy files instead.

Additionally, if the github.com/golang-jwt/jwt/v4 does another major semantic version update, the github.com/MicahParks/keyfunc will need to include support for both /v4 and /v5 due to the Go type system. This can be avoided if the github.com/MicahParks/keyfunc is included in its dependent project.

This PR only includes effectual changes in the keyfunc directory and does not modify anything but a comment in the original project.

Terminology

  • "JWKs" is a JSON Web Key Set (JWK Set).
  • "given keys" are cryptographic keys already in the correct Go type provided by the package user before the creation of a keyfunc.JWKs.

Limitations

This package does not yet support EdDSA keys. This means if a JWKs contains a key with an alg of EdDSA will return an error during jwt.Parse if a JWT with a kid associated with the alg of EdDSA. I believe implementing this would be very doable, but haven't come across a JWKs that uses EdDSA keys yet. I may implement this before this PR is unmarked as a draft. The relevant RFC is here: https://datatracker.ietf.org/doc/html/rfc8037#appendix-A.4

This package does not support creating a JWKs. It only consumes a JWKs for the purpose of parsing and validating JWTs.

Tests

Test coverage for this new github.com/golang-jwt/jwt/v4/keyfunc package is >90%. Some tests make keys to sign tokens, then parse and validate them. Other tests use hard coded tokens and JWKs.

Examples

This PR includes an examples directory. Many of the examples reference this service: https://jwks-service.appspot.com/. As it states on the site:

This service is intended to help you as you test basic JWT and JWKS interoperability.

@MicahParks
Copy link
Contributor Author

This would also provide examples that this issue requires: #58

This package can additionally use a given mapping of key IDs, `kid`, to cryptographic keys parse and validate JWTs.

It's common for an identity provider, such as [Keycloak](https://www.keycloak.org/)
or [Amazon Cognito (AWS)](https://aws.amazon.com/cognito/) to expose a JWKs via an HTTPS endpoint. It is important that
Copy link

Choose a reason for hiding this comment

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

I would use the full name before introducing the acronym, ie JSON Web Key Set before JWKS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's present above on line 7

This package can consume a JSON Web Key Set (JWKs) and or a given set of key ID, kid to cryptographic key mappings.

@mfridman
Copy link
Member

Thank you for submitting a PR and writing up a detailed description. Very helpful.

I'm a bit skeptical, however, that supporting JWKs is within the scope of this jwt package. It seems users can already use https://github.com/MicahParks/keyfunc ?

Also, applications may choose to implement their own mechanism for fetching/storing keys which is orthogonal to verifying and parsing JWTs. The code also appears to be more complicated than it needs to be, there is a nested lock and that's a bit of a code smell to me.

But putting aside the implementation, accepting a PR like this would mean having to support this code which is another major consideration.

@MicahParks MicahParks marked this pull request as ready for review September 22, 2021 00:16
@MicahParks
Copy link
Contributor Author

MicahParks commented Sep 22, 2021

I believe having this code in this package would benefit the pacakge users. Parsing and validating JWTs with keys found in a JWK Set is a common use case. Moreover, the documentation around creating a jwt.KeyFunc is sparse and the function signature uses an empty interface, interface{}, which has caused some confusion (GitHub issues, StackOverflow questions). The addition of "given keys" keyfunc.NewGiven, helps by statically typing cryptographic keys used for parsing and validating JWTs (the most common use case I see being HMAC).

I'd be happy to discuss the benefits and drawbacks to any of the specifics of the implementation. I would also like to offer to help maintain the code, if the PR is accepted.

}

// Create a channel that will never send anything unless there is a refresh interval.
refreshInterval := make(<-chan time.Time)
Copy link
Member

Choose a reason for hiding this comment

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

could this work with a time.Ticker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If specifying the RefreshInterval of the keyfunc.Options had a non-zero default, yes.

Since it can be omitted with the expectation of no refresh interval, a time.Ticker cannot be created because it requires a time.Duration greater than zero.

If it were turned into a time.Ticker now, then the below select statement case would need to be guarded by an if refreshInterval != nil statement, which would require two select statements, once for each case.

@lggomez
Copy link
Member

lggomez commented Sep 24, 2021

I kinda agree with @mfridman in that the JWKS concern is in a separate scope of this package, altough related and the implementation can be decoupled in a fashion such that that the consumer decides the http Doer implementation to use (just to name one thing).

I think that, should this go forward, could go into a separate package like golang-jwt/jwks to ease maintenance, versioning and separation of concerns

@MicahParks
Copy link
Contributor Author

Seems this PR has gotten a bit stale. I'm going to close it, but please feel free to reach out if it fits inside the scope of this project and I'd be happy to re-open it.

I plan on making a handful of changes to github.com/MicahParks/keyfunc then releasing v1.0.0 within a week or so.

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

4 participants