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 NewKeySet method to JWTAuth and demonstrate JWKS rotation #93

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

Conversation

alexlovelltroy
Copy link

This PR supercedes both #85 and #71 and rebases on the most recent jwtauth codebase.

It adds support for KeySets through a new method NewKeySet to the JWTAuth struct.

It includes tests and comments that seek to explain how it works inline.

There's also an example in the _example directory that shows how to use and rotate a KeySet.

	r.Group(func(r chi.Router) {
		dynamicTokenAuth := dynamicTokenAuth{keySet: keySet}
		// Seek, verify and validate JWT tokens based on keys returned by the callback function
		r.Use(jwtauth.VerifierDynamic(dynamicTokenAuth.JWTAuth))

		// Handle valid / invalid tokens. In this example, we use
		// the provided authenticator middleware, but you can write your
		// own very easily, look at the Authenticator method in jwtauth.go
		// and tweak it, its not scary.
		r.Use(jwtauth.Authenticator)

		r.Get("/rotate", func(w http.ResponseWriter, r *http.Request) {
			_, claims, _ := jwtauth.FromContext(r.Context())
			w.Write([]byte(fmt.Sprintf("protected area. hi %v", claims["user_id"])))
		})
	})

This commit adds support for KeySets through a new method `NewKeySet` to the `JWTAuth` struct.

It includes tests and comments that seek to explain how it works inline.

There's also an example in the _example directory that shows how to use and rotate a KeySet.
@alexlovelltroy
Copy link
Author

@pkieltyka As requested, we've rebased the changes from #71 and #85 and added tests and comments. This functionality is in use for us and we'd like to avoid maintaining a fork if possible. What can we do to help move this PR forward?

@Rshep3087
Copy link

➕ we would also benefit from this getting merged in, are there any blockers stopping it from getting merged?

@VojtechVitek
Copy link
Contributor

Hi, I don't think there are any blockers besides the maintainers having to allocate their time & look into this in detail. Please be patient with us :)

For now, it'd be great if you could go get github.com/go-chi/jwtauth@7552877b95a3881fb2ad74381a7d7d5eec16a223, test it in your dev/prod envs and provide us with feedback on this PR :)

@alexlovelltroy Thanks for the contribution. Looks great at first glance. 🎉

@davidallendj
Copy link

davidallendj commented Aug 26, 2024

Hi, I don't think there are any blockers besides the maintainers having to allocate their time & look into this in detail. Please be patient with us :)

For now, it'd be great if you could go get github.com/go-chi/jwtauth@7552877b95a3881fb2ad74381a7d7d5eec16a223, test it in your dev/prod envs and provide us with feedback on this PR :)

@alexlovelltroy Thanks for the contribution. Looks great at first glance. 🎉

FYI I'm getting this error when trying to pull with the command above:

go get github.com/go-chi/jwtauth@7552877b95a3881fb2ad74381a7d7d5eec16a223
go: github.com/go-chi/jwtauth@7552877b95a3881fb2ad74381a7d7d5eec16a223: invalid version: unknown revision 7552877b95a3881fb2ad74381a7d7d5eec16a223

@VojtechVitek Did you mean something like go get github.com/go-chi/jwtauth@alovelltroy/add-jwks-support instead?

Edit: It was pointed out to me that specifying by hash is not supported I suppose: golang/go#31191

@alexlovelltroy
Copy link
Author

In order to make things easier. I created a "release" on my branch that folks can test with.

https://github.com/alexlovelltroy/jwtauth/releases/tag/v5.3.1-20240826

replace github.com/go-chi/jwtauth/v5 => github.com/alexlovelltroy/jwtauth/v5 v5.3.1-20240826 in your go.mod file should allow you to test with it without changing your code.

We've got a handful of services that are using this at the moment. I have done a basic set of tests using the new release. @davidallendj is running things through more exhaustive testing today. No indication of problems yet. @Rshep3087 if you have any tests that you can run and confirm that it behaves the same as the upstream for normal use cases, that would be helpful as well.

@davidallendj
Copy link

davidallendj commented Aug 27, 2024

In order to make things easier. I created a "release" on my branch that folks can test with.

https://github.com/alexlovelltroy/jwtauth/releases/tag/v5.3.1-20240826

replace github.com/go-chi/jwtauth/v5 => github.com/alexlovelltroy/jwtauth/v5 v5.3.1-20240826 in your go.mod file should allow you to test with it without changing your code.

We've got a handful of services that are using this at the moment. I have done a basic set of tests using the new release. @davidallendj is running things through more exhaustive testing today. No indication of problems yet. @Rshep3087 if you have any tests that you can run and confirm that it behaves the same as the upstream for normal use cases, that would be helpful as well.

Just tested and confirmed working as expected after making the changes stated above. Once this get merged, we should be able to update our references without breaking anything.

@alexlovelltroy
Copy link
Author

@VojtechVitek Any feedback? Anything else we can do to help get this merged?

@alexlovelltroy
Copy link
Author

@VojtechVitek and @pkieltyka ?

anything else we can do to push this forward?

@alexlovelltroy
Copy link
Author

@VojtechVitek I hate to be a pest, but the longer this sits out here, the more likely we'll have to rebase again. Anything we should be doing?

@davidallendj
Copy link

davidallendj commented Oct 24, 2024

@VojtechVitek Can we get a review for this? We're dependent on it, and we haven't seen any activity in about a month.

@VojtechVitek
Copy link
Contributor

Sorry guys, we're busy with other product work at the moment.

We will get to review this eventually for sure - but no promises on timing. Sorry :)

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.

4 participants