Native support for key rotation in verifications

[fishy]

Add native support for key rotation for ES*, Ed*, HS*, RS*, and PS*
verifications.

In those SigningMethod's Verify implementations, also allow the key to
be the type of the slice of the supported key type, so that the caller
can implement the KeyFunc to return all the accepted keys together to
support key rotation.

While key rotation verification can be done on the callers' side without
this change, this change provides better performance because:

• When trying the next key, the steps before actually using the key do
  not need to be performed again.

• If a verification process failed for non-key reasons (for example,
  because it's already expired), it saves the effort to try the next
  key.

The native key rotation support also helps callers to get more accurate
errors.

[fishy]

note: this is ported from PR from the old repo: dgrijalva/jwt-go#372

[mfridman]

Thanks for submitting a PR, this is actually quite interesting. I've had to do something similar in previous projects I've worked on so can certainly see this being useful.

I'll scope out some time to review this and make sure it covers any corner cases. Would also be interested what others think..

[fishy]

a side note is that we already forked the original project with this change (because there was no one reviewing the original pr), and have been running this version in our production for years now :) see:

• https://github.com/reddit/jwt-go/commits/master
• https://github.com/reddit/edgecontext/blob/42f4ee3d3e330ff70653a99a8ef5e69e88934ed6/go.mod#L9

[oxisto]

Add native support for key rotation for ES*, HS*, RS*, and PS* verifications.

In those SigningMethod's Verify implementations, also allow the key to be the type of the slice of the supported key type, so that the caller can implement the KeyFunc to return all the accepted keys together to support key rotation.

While key rotation verification can be done on the callers' side without this change, this change provides better performance because:

• When trying the next key, the steps before actually using the key do
  not need to be performed again.
• If a verification process failed for non-key reasons (for example,
  because it's already expired), it saves the effort to try the next
  key.

Definitely an interesting idea. I have two main concerns with the approach and we need to discuss if/how we deal with them:

1. While interesting, this is probably only relevant for a very small amount of use cases and I am a little bit worried that this use case now dictates the design of the Verify function by having a loop of all keys in it, etc. Probably, the performance overhead of this loop in a single-key scenario is negligible (although some numbers of it would be nice). However, more importantly, we need to duplicate the logic behind this (loop, error check, etc.) for all signing mechanisms. You thankfully did it already for most of them, but for example: Why is there no EdDSA support? So we are getting some inconsistencies here.

2. The second concern that I have is that we are putting even more things into the interface{} key variable, which at this point could contain anything from a single key, an array of keys to whatever. This gets confusing from a user perspective, and even from the developer side. We already have some inconsistencies between signing methods that some for example take the pointer-form, some take a non-pointer form. Is this now always an array of a pointer struct, etc.

Some random thoughts:

• Can we have something like VerifyKeys in addition to Verify that takes a list of keys and then calls Verify internally However strictly speaking if we extend the interface we would break API compatibility, if someone implemented SigningMethod externally (which is however very very very very very unlikely). This would probably also reduce the performance gain a little bit.

[fishy]

@oxisto Thanks for the feedback!

Why is there no EdDSA support? So we are getting some inconsistencies here.

That's simply because EdDSA support was added after my original PR :) I just added that in a fixup commit.

I also added benchhmark test for RS256 in another fixup commit, with 3 rotating keys (last one being the good one). Here are the numbers:

$ go test -bench BenchmarkRS256VerifyRotation
2022/02/25 11:42:47 Listening...
2022/02/25 11:42:47 Authenticate: user[test] pass[known]
goos: linux
goarch: amd64
pkg: github.com/golang-jwt/jwt/v4
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkRS256VerifyRotation/good/manual-12        49528             24474 ns/op           23737 B/op        143 allocs/op
BenchmarkRS256VerifyRotation/good/native-12        51330             23527 ns/op           18318 B/op         75 allocs/op
BenchmarkRS256VerifyRotation/expired/manual-12             46842             25965 ns/op           24023 B/op        155 allocs/op
BenchmarkRS256VerifyRotation/expired/native-12             49292             24663 ns/op           18426 B/op         79 allocs/op
PASS
ok      github.com/golang-jwt/jwt/v4    5.967s

So although time-wise it's mostly negligible, memory-wise it saves ~1/4 of the memory and ~1/2 of the allocations in both good and claim already expired cases.

Besides performance, another benefit of native key rotation support is error handling. There's potentially a case that the payload has a valid signature, but the claim itself is having issues. With naive, manual key rotation (try each key one by one), the implementation usually needs to do something like this:

var lastErr error
for _, key := range keys {
  token, err := tryValidatePayload(payload, key)
  if err == nil {
    return token, nil
  }
  lastErr = err
}
return nil, lastErr

if the first key was valid (but it got other claim errors), the actual error could be masked by signature errors caused by later keys, so instead of getting the actual error about the claim, they got a key error instead, which could be hard to debug issues.

(if this PR is approved I'll squash the commits, the fixup commits are just for easier reviewing)

[fishy]

btw if I change from parallel benchmark to sequential benchmark, the time saving is slightly more significant:

$ go test -bench BenchmarkRS256VerifyRotation
2022/02/25 11:54:08 Listening...
2022/02/25 11:54:08 Authenticate: user[test] pass[known]
goos: linux
goarch: amd64
pkg: github.com/golang-jwt/jwt/v4
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkRS256VerifyRotation/good/manual-12         8596            140821 ns/op           23702 B/op        143 allocs/op
BenchmarkRS256VerifyRotation/good/native-12         9038            133243 ns/op           18293 B/op         75 allocs/op
BenchmarkRS256VerifyRotation/expired/manual-12              8511            142986 ns/op           23977 B/op        155 allocs/op
BenchmarkRS256VerifyRotation/expired/native-12              9028            134728 ns/op           18388 B/op         79 allocs/op
PASS
ok      github.com/golang-jwt/jwt/v4    5.026s

[oxisto]

@fishy Thanks for providing the benchmarks, I will go over them over on the next couple of days.

Thanks for adding EdDSA!

Don't worry about adding more commits, we are using the squash and merge strategy, so we will squash them on our end eventually.

[fishy]

Don't worry about adding more commits, we are using the squash and merge strategy, so we will squash them on our end eventually.

That doesn't preserve my git commit message, so I always squash myself and keep the git commit message sane/meaningful :)

[fishy]

NOTE: the typecastings here are needed because ParseEdPublicKeyFromPEM returns crypto.PublicKey instead of ed25519.PublicKey, which, in my opinion, was a big mistake (all other similar functions, for example ParseRSAPublicKeyFromPEM, returns the expected type).

I can see this could become a footgun, as people might try to return []crypto.PublicKey in their Keyfunc. If we think that's a problem we want to prevent, there are two ways to do that:

1. Make SigningMethodEd25519.Verify to also accept []crypto.PublicKey, but this will make that one more special than others;
2. Deprecate ParseEdPublicKeyFromPEM and use a different named function to return ed25519.PublicKey. If we go this route we probably would want to do the same with ParseEdPrivateKeyFromPEM.

(We can't just fix ParseEdPublicKeyFromPEM because that's a breaking change, unfortunately)

[mfridman]

Ouch, indeed special casing it by returning an interface was probably not the best approach.

Given these 2 options I think I'd vote for option 1. Although it's my least favorite because it requires doubling-down on making this one even more special. The benefit is we get to keep this an implementation detail of the library instead of punting the problem to the public API.

With option 2 even though we deprecate it, to truly get rid of it would require a major version release. And I think we want to keep the scope of this within the current /v4 release and save breaking changes/re-thinking the package design in /v5.

[fishy]

done in new commit

[mfridman]

Thanks for putting that through, overall this looks good to me given the available options.

[mfridman]

What are your thoughts about? Not advocating for a change in this PR, just curious to tease this out a bit.

Can we have something like VerifyKeys in addition to Verify that takes a list of keys and then calls Verify internally However strictly speaking if we extend the interface we would break API compatibility, if someone implemented SigningMethod externally (which is however very very very very very unlikely). This would probably also reduce the performance gain a little bit.

This isn't a bad idea, I really wish SigningMethod had a private method to avoid external implementations. But that's probably a discussion outside the scope of this PR. The thing we should consider is if we go with VerifyKeys whether we want to make the signature future-proof with a potential /v5.

[fishy]

Can we have something like VerifyKeys in addition to Verify that takes a list of keys and then calls Verify internally However strictly speaking if we extend the interface we would break API compatibility, if someone implemented SigningMethod externally (which is however very very very very very unlikely). This would probably also reduce the performance gain a little bit.

I don't think that would resolve any of the problems. As in most cases people are not using SigningMethod.Verify directly. Instead they use Parse/ParseWithClaims, which then calls SigningMethod.Verify within. If we go that route, that means we need a lot more special handling inside Parse/ParseWithClaims to figure out whether we are getting a single or multiple keys and whether we should call Verify or VerifyKeys.

[mfridman]

This looks good to me, let's see if anyone else has an opinion.

[fishy]

force pushed with a few minor comment updates, here are the diff from the previous state:

$ git diff --cached 
diff --git a/ed25519.go b/ed25519.go
index d62de12..5f0f565 100644
--- a/ed25519.go
+++ b/ed25519.go
@@ -33,7 +33,6 @@ func (m *SigningMethodEd25519) Alg() string {
 }
 
 // Verify implements token verification for the SigningMethod.
-// For this verify method, key must be an ed25519.PublicKey
 // For this verify method, key must be in types of one of ed25519.PublicKey,
 // []ed25519.PublicKey, or []crypto.PublicKey (slice types for rotation keys),
 // and each key must be of the size ed25519.PublicKeySize.
diff --git a/hmac.go b/hmac.go
index 928bbca..c9a2d7e 100644
--- a/hmac.go
+++ b/hmac.go
@@ -46,6 +46,7 @@ func (m *SigningMethodHMAC) Alg() string {
 }
 
 // Verify implements token verification for the SigningMethod. Returns nil if the signature is valid.
+// key must be of type of either []byte (a single key) or [][]byte (rotation keys).
 func (m *SigningMethodHMAC) Verify(signingString, signature string, key interface{}) error {
        // Verify the keys are the right types
        var keys [][]byte
diff --git a/rsa_pss.go b/rsa_pss.go
index 3ad7f28..497f7be 100644
--- a/rsa_pss.go
+++ b/rsa_pss.go
@@ -81,7 +81,6 @@ func init() {
 }
 
 // Verify implements token verification for the SigningMethod.
-// Implements the Verify method from SigningMethod
 // For this verify method, key must be in the types of either *rsa.PublicKey or
 // []*rsa.PublicKey (for rotation keys).
 func (m *SigningMethodRSAPSS) Verify(signingString, signature string, key interface{}) error {

[fishy]

how long do we have to wait? :)

[oxisto]

how long do we have to wait? :)

This is an open source project maintained by a group of people which maintain it in their spare, free time (unpaid). Therefore it might take a while until a verdict is reached.

Personally, I am still struggling a little bit with the overall approach, given that this PR rewrites a lot of the core functionality of this library (although the public API does not change) for a use case that only applies to certain limited scenarios.

The more I think about this, the less am I convinced that this a good approach in general. Implementing key rotation using standards like JWKS (which is supported through the excellent library https://github.com/MicahParks/keyfunc) is probably a more elegant fit. That is of course very opinionated of me and maybe some of the other maintainers can help decide how to move forward with this.

cc @ripienaar @Waterdrips @lggomez

[mfridman]

Ye, I'll echo what @oxisto said .. we are trying our best to maintain this project with the limited bandwidth we have. It's useful to get a few 👀 on a given PR, esp. if it touches the core of the project. A bug would affect a lot of downstream consumers. We have to be extremely careful... so the delay in merging PR's is more the result of being cautious.

We also don't always agree on certain implementations, but that's perfectly normal for an open-source project. Although I'm in favor of this PR, I value that other users and maintainers disagree, which in turn makes this project better overall.

[schmidtw]

It's been a while, have any of the opinions changed?

I need to be able to rotate keys & would rather use this code vs. write my own duplicate if possible. Key rotation seems like a very basic use case and having the library include that would be pretty awesome if possible.

[schmidtw]

@oxisto / @mfridman if you're not happy with this approach for supporting multiple keys, do you have a path that you'd like for this use case? The interface for providing the key to try results in some fairly convoluted code being needed:

1. Duplicate the available keys into a list that can be pruned.
2. Start the validation.
3. Wait for the callback call that indicates which algo used.
4. Look in the list of the specified algo & insert one of the keys. Then remove the key from list held outside the callback function so the next time a new key will be tried.
5. Check to see if the decode failed & see if any valid keys exist for the needed algo & go back to step 2, or fail/succeed.

Alternatively the JWT can be decoded "carefully" to determine the algo, which slightly simplifies things, but there's still a fair number of the same steps above. As @fishy pointed out, the ideal time to try 2 or 3 keys is after everything is setup.

I'm not trying to push the approach in this PR, but I'd like a way that complements this library and it's thoughtful design. I'm happy to help, but I'm not sure how at the present.

[oxisto]

@oxisto / @mfridman if you're not happy with this approach for supporting multiple keys, do you have a path that you'd like for this use case? The interface for providing the key to try results in some fairly convoluted code being needed:

1. Duplicate the available keys into a list that can be pruned.
2. Start the validation.
3. Wait for the callback call that indicates which algo used.
4. Look in the list of the specified algo & insert one of the keys. Then remove the key from list held outside the callback function so the next time a new key will be tried.
5. Check to see if the decode failed & see if any valid keys exist for the needed algo & go back to step 2, or fail/succeed.

Alternatively the JWT can be decoded "carefully" to determine the algo, which slightly simplifies things, but there's still a fair number of the same steps above. As @fishy pointed out, the ideal time to try 2 or 3 keys is after everything is setup.

I'm not trying to push the approach in this PR, but I'd like a way that complements this library and it's thoughtful design. I'm happy to help, but I'm not sure how at the present.

My major concern was (and still is) that the approach presented in this PR touches all existing signing methods and forces them to loop over a key. So we are stuck with a lot of extra code that we (@mfridman and I) need to maintain and is only applicable for a small percentage of our users.

Personally, I still don't really see the use case. What you probably want to have flexibility in terms of exchanging keys is setting up JWKS (see above) and rotate the keys in the set. Although this still leaves you with the issue that some clients might still have old key material, I get that.

Looking forward, what I could possibly get on board with is that we deal with the multiple keys issue directly in ParseWithClaims here:

jwt/parser.go

Lines 83 to 96 in 8b7470d

	if key, err = keyFunc(token); err != nil {
	return token, newError("error while executing keyfunc", ErrTokenUnverifiable, err)
	}
	// Decode signature
	token.Signature, err = p.DecodeSegment(parts[2])
	if err != nil {
	return token, newError("could not base64 decode signature", ErrTokenMalformed, err)
	}
	// Perform signature validation
	if err = token.Method.Verify(strings.Join(parts[0:2], "."), token.Signature, key); err != nil {
	return token, newError("", ErrTokenSignatureInvalid, err)
	}

The "good" thing is that the existing Keyfunc already returns a interface{}, so in theory it is possible to return an array of keys. We could then check if we have an array and loop over the keys instead of using only one and supply each key to token.Method.Verify until one of them is ok.

Hopefully this small "is it an array?" check is negilble in terms of perfomance, but of course it would be good to have some numbers on this after the implementation.

The "bad" thing still remains that we are sort of mis-using interface{} here, which is already confusing from a developer/user perspective and probably something we might want to get rid of with type parameters in the future.

[mfridman]

Closing this in favor of #344

I think we've settled on a descent (and simpler) design while keeping things type-safe.