Consider validating key length

[DeanPDX]

The Problem

Right now the following code executes without error:

func main() {
	bogusKey := []byte("")

	token := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{
		"foo": "bar",
		"nbf": time.Date(2015, 10, 10, 12, 0, 0, 0, time.UTC).Unix(),
	})

	// Using bogus key, doesn't complain
	tokenString, err := token.SignedString(bogusKey)

	fmt.Println(tokenString, err)
}

Output:

eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJmb28iOiJiYXIiLCJuYmYiOjE0NDQ0Nzg0MDB9.aGTWgif4pwMnjF8My859yqoueBN9ueg95F58WNFt1ps <nil>

Program exited.

Per RFC 7518 I believe the key should be 256 bits or longer:

Hash-based Message Authentication Codes (HMACs) enable one to use a
secret plus a cryptographic hash function to generate a MAC. This
can be used to demonstrate that whoever generated the MAC was in
possession of the MAC key. The algorithm for implementing and
validating HMACs is provided in RFC 2104 [RFC2104].

A key of the same size as the hash output (for instance, 256 bits for
"HS256") or larger MUST be used with this algorithm. (This
requirement is based on Section 5.3.4 (Security Effect of the HMAC
Key) of NIST SP 800-117 [NIST.800-107], which states that the
effective security strength is the minimum of the security strength
of the key and two times the size of the internal hash value.)

Possible Solutions

1. Documentation/Examples

This might be a documentation issue. I believe some of the onus is on the developers consuming this package to understand how JWTs/HMAC works. But - it would be nice if secret key size was mentioned somewhere. For example here:

// Ideal key size will be determined by your signing method. For example, if you 
// are using SigningMethodHS256 you will want at least 256 randomly-generate
// bits here. Further reading:
// https://www.rfc-editor.org/rfc/rfc7518#section-3.2
mySigningKey := []byte("AllYourBase")

We could also include an example of how to generate a key using crypto/rand.

2. Return an error

Since the RFC says in all caps "MUST be used", this might be considered an error state. In that case, token.SignedString could return an error in the event that len(key) < appropriate based on the signing method. I think this would be preferable to people thinking their JWT implementation is secure when it's not. To maintain backwards compatibility we could make a new token.SignedStringStrict that enforces this, leaving SignedString alone.

Consider the following scenario: you deploy an API using golang-jwt to a cloud platform. You expose your signing secret via a secret manager as an environment variable. If something went wrong and the signing secret was actually an empty byte array, I would want that to error out. Thus I would default to the new SignedStringStrict for projects going forward.

[mfridman]

Thanks for opening a clear and concise issue with possible solutions.

Option 1 - Seems like we should do it regardless.

Option 2 - I'll dig up the issue(s), but I think @oxisto was fairly against doing key validation as this opens up questions like "what is a reasonable key size", however, for certain signing methods maybe there's an argument. I like the idea of a "strict" function to avoid breaking existing users, although one could also argue that there should be exactly one way to do things and the function should do the right thing.

I need to refresh my memory on a few bits, but will come back to this and also wait on @oxisto to chime in.

[oxisto]

I am thinking whether this would be the first use-case for the TokenOption type that we introduce to provide forward compatibility in case we ever had to introduce some options into New and NewWithClaims. So we could think about a "strict" mode using a token option and then make this the default at some future point in time - which might make sense after re-reading the JWA spec.

[mfridman]

Yes! I think this is an excellent use case for that option. Although it's too bad this is an opt-in, instead of the default behavior.

• WithKeyValidation
• WithStrictSigning

Not sure what the right name is, but I think this is probably the correct path forward.

[DeanPDX]

Thanks for the comments. I hadn't even noticed that TokenOption type yet. I agree that is a better option than branching into multiple functions.

• WithKeyValidation

I like this but my only comment is: I think "Validation" might be too broad a term. Because somebody could still create a totally bogus signing secret (all 0s for example) that is the correct length but not random. Maybe WithKeyLengthValidation or something? Also if there is a future where this becomes the default I wonder if SkipKeyLengthValidation might make sense to make it known that this is not necessarily the desired behavior.

So we could think about a "strict" mode using a token option and then make this the default at some future point in time - which might make sense after re-reading the JWA spec.

As long as it's in a major version I think this is a good path forward. I just don't want to brick any APIs that are currently functioning (though you might argue that this is better than people discovering their JWTs are less securely signed than they think they are via other means).

I use this library on multiple projects and I'd be willing to help out on this if you want a pull request. I think the documentation/examples approach is for sure the best first step. I think key length validation might get a bit more complicated for signing methods other than HMAC (where the RFC has easy-to-follow guidelines) but I'd be willing to take a stab at it.

[mfridman]

We're always open to contributions. While some issues might become stale and responses may be delayed, we remain quite active overall.

For me, one of the key aspects is ensuring stability and backward compatibility across minor versions. We're treading a delicate balance where we might argue a security fix warrants a breaking change, but these instances should be extremely rare.

Feel free to update the documentation or take a stab at an implementation, even if it serves only as a reference for future major version considerations.