Skip to content
This repository has been archived by the owner on May 24, 2023. It is now read-only.

Update keyfunc #73

Closed
MicahParks opened this issue Dec 6, 2021 · 2 comments
Closed

Update keyfunc #73

MicahParks opened this issue Dec 6, 2021 · 2 comments

Comments

@MicahParks
Copy link
Contributor

MicahParks commented Dec 6, 2021

There have been some improvements to github.com/MicahParks/keyfunc since this project first supported JWT verification from JWK Sets.

Two notable ones are:

  1. github.com/MicahParks/keyfunc only imports github.com/golang-jwt/jwt now.
  2. The key type for parsing is now based on the JWK Set's kty header value. This header value is required by the RFC whereas the previous alg value is optional in a JWK Set.

There are a couple others, such as support for given keys (like HMAC bytes) and EdDSA support.

Additionally, some bug fixes have occurred. I see some of the bugs are still present in this project.

  • This project's jwt.KeySet exports a field called Keys. This field is a map that can cause panic: fatal error: concurrent map read and map write. This would happen when a JWK Set is being refreshed and the map is being read at the same time.
  • There could be a data race condition with the rawJWK.precomputed field triggered by two JWTs using the same kid being parsed at the same time, if it's the first time that kid has been seen since the last refresh. See this commit.

This issue could be solved by updating the keyfunc code in this project or switching to github.com/MicahParks/keyfunc. I'd be happy to make a PR to do the latter, if that's a desirable solution 🙂

(github.com/MicahParks/keyfunc has just released v1.0.0 and I intend to avoid /v2 and beyond.)

@MicahParks
Copy link
Contributor Author

There have been multiple bug fixes since I made this issue last year. I imagine most, if not all, of the bug fixes affect this repository. Please note there are some JWT alg and use header parameter changes that help with RFC compliance.

I don't think there are any known vulnerabilities at this point, however RFC compliance is important and it is best to conform to the RFCs to protect against theoretical attacks.

Please see the release notes for every release since this PR to get an idea of the changes.

@MicahParks
Copy link
Contributor Author

Closed with #129

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant