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 jwks support, enable use of jwks rotation feature and upgrade underlying JWT library #71

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tarcisioN
Copy link

As #40 and #27, we need to support multiple keys (jwks) and support rotation.

This PR implements new methods to handle with that.
Also we update github.com/lestrrat-go/jwx to v2 to support those features.

@tanmaij
Copy link

tanmaij commented Jun 4, 2022

jwtauth. go file has some errors

jwtauth.go:39:40: too many arguments in call to jwt.WithVerify
have (jwa.SignatureAlgorithm, interface{})
want (bool)
C:\Users\HP\go\src\github.com\go-chi\jwtauth\jwtauth.go:41:40: too many arguments in call to jwt.WithVerify
have (jwa.SignatureAlgorithm, interface{})
want (bool)
C:\Users\HP\go\src\github.com\go-chi\jwtauth\jwtauth.go:135:25: cannot use ja.alg (variable of type jwa.SignatureAlgorithm) as type jwt.SignOption in argument to jwt.Sign:
jwa.SignatureAlgorithm does not implement jwt.SignOption (missing Ident method)
C:\Users\HP\go\src\github.com\go-chi\jwtauth\jwtauth.go:135:33: cannot use ja.signKey (variable of type interface{}) as type jwt.SignOption in argument to jwt.Sign:
interface{} does not implement jwt.SignOption (missing Ident method)

@tarcisioN
Copy link
Author

tarcisioN commented Jun 4, 2022

@tanmaij Building fine and all tests passing here. It seems to be something wrong on your environment. Have you updated (go mod tidy) lestrrat-go/jwx/v2?

Actually, i think you are running master code:

that is jwtauth.go:39:
func New(alg string, signKey interface{}, verifyKey interface{}) *JWTAuth {

and that is jwtauth.go:135:
func VerifyRequest(ja *JWTAuth, r *http.Request, findTokenFns ...func(r *http.Request) string)

@Shaun1
Copy link

Shaun1 commented Sep 7, 2022

Any plans on merging this feature in? Would be useful if so.

@pkieltyka
Copy link
Member

hi @tanmaij thanks for your work on this PR -- I've merged another PR which upgrades us to the latest version of jwx/v2, and tagged https://github.com/go-chi/jwtauth/releases/tag/v5.1.0

can you rebase your branch? I will provide a review here too, and then we can get this one merged too

jwtauth.go Outdated Show resolved Hide resolved
// be the generic `jwtauth.Authenticator` middleware or your own custom handler
// which checks the request context jwt token and error to prepare a custom
// http response.
func VerifierDynamic(jaf func() (*JWTAuth, error)) func(http.Handler) http.Handler {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this method. Instead just update the Verifier method to check ja.keySet, if its not nil, then verify against the keySet instead of verifyKey, etc..

Copy link
Author

@tarcisioN tarcisioN Aug 11, 2023

Choose a reason for hiding this comment

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

I apologize, I left the project where I was working on this feature.

Now I'm a bit outdated, from what I remember this function is necessary for the possibility of modifying the keys available at runtime. As can be seen in the test: jwtauth_test.go:393.

@@ -76,6 +114,23 @@ func Verify(ja *JWTAuth, findTokenFns ...func(r *http.Request) string) func(http
}
}

func VerifyDynamic(jaf func() (*JWTAuth, error), findTokenFns ...func(r *http.Request) string) func(http.Handler) http.Handler {
Copy link
Member

Choose a reason for hiding this comment

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

we don't need this method, as we can check which "mode" the JWTAuth object is in by checking verifyKey or keySet

Copy link
Author

Choose a reason for hiding this comment

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

The idea behind this functionality is to allow keys to be obtained dynamically, such as in integration with Vault.

@adam2k
Copy link

adam2k commented Apr 13, 2023

@tarcisioN this PR is super helpful! Do you want help rebasing and getting this merged? I'm running into this exact problem right now and this PR would solve it for me.

@jhutsonCTL
Copy link

My team needs this key set support for upcoming work. What do we need to do to get this PR merged in? Looks like the author hasn't responded to this PR in over a year. Would it make sense to create a new branch and PR for this change?

I'd be happy to help move this forward.

@pkieltyka
Copy link
Member

I propose, fork it, finish it, use it in prod and then submit PR with update. Or link to fork and we can consume your changes once they’re refined. See my comments above.

@jhutsonCTL
Copy link

jhutsonCTL commented Aug 21, 2023

I propose, fork it, finish it, use it in prod and then submit PR with update. Or link to fork and we can consume your changes once they’re refined. See my comments above.

Given it doesn't look like this PR will be completed enough to be merged, this strategy looks like the right path forward.

@davidallendj
Copy link

For anyone interested, I tried forking, made the requested changes, and rebased. Test seems to pass and things seem to work as expected. This is what I'm using in my own code below.

ctx, cancel := context.WithCancel(context.Background())
defer cancel()
set, err := jwk.Fetch(ctx, url)
if err != nil {
	return fmt.Errorf("%v", err)
}
jwks, err := json.Marshal(set)
if err != nil {
	return fmt.Errorf("failed to marshal key set: %v", err)
}
tokenAuth, err = jwtauth.NewKeySet(jwks)
if err != nil {
	return fmt.Errorf("failed to initialize key set: %v", err)
}

Fork with changes: https://github.com/davidallendj/jwtauth

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.

7 participants