jwtauth icon indicating copy to clipboard operation
jwtauth copied to clipboard

add jwks support, enable use of jwks rotation feature and upgrade underlying JWT library

Open tarcisioN opened this issue 2 years ago • 9 comments

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.

tarcisioN avatar May 13 '22 19:05 tarcisioN

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)

tanmaij avatar Jun 04 '22 01:06 tanmaij

@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)

tarcisioN avatar Jun 04 '22 04:06 tarcisioN

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

Shaun1 avatar Sep 07 '22 17:09 Shaun1

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

pkieltyka avatar Nov 13 '22 16:11 pkieltyka

@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.

adam2k avatar Apr 13 '23 13:04 adam2k

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.

jhutsonCTL avatar Aug 11 '23 21:08 jhutsonCTL

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.

pkieltyka avatar Aug 11 '23 22:08 pkieltyka

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.

jhutsonCTL avatar Aug 21 '23 17:08 jhutsonCTL

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

davidallendj avatar Mar 06 '24 21:03 davidallendj