Add enforce_hmac_key_length configuration option

[304]

Description

Add enforce_hmac_key_length configuration option to enforce minimum key lengths for HMAC algorithms. This is a security hardening measure that helps prevent brute-force attacks on weak keys.

Recommended minimum key lengths per RFC 7518 Section 3.2:

• HS256: 256 bits (32 bytes)
• HS384: 384 bits (48 bytes)
• HS512: 512 bits (64 bytes)

See: https://www.rfc-editor.org/rfc/rfc7518.html#section-3.2

Usage:

  JWT.configuration.decode.enforce_hmac_key_length = true

For backward compatibility, it's disabled by default (false). But it would be nice to enforce it in a future major version like it's done for RSA key validation (minimum 2048 bits). For now, users must explicitly opt-in.

Checklist

Before the PR can be merged be sure the following are checked:

• There are tests for the fix or feature added/changed
• A description of the changes and a reference to the PR has been added to CHANGELOG.md. More details in the CONTRIBUTING.md

[anakinj]

Could we instead of letting users set the min length just let the gem validate against the recommended minimums, so just enable or disable the feature. Then in future versions we could enable it by default. Something like enforce_hmac_key_length

I think this would be a bit less complex and easier for users to toggle on.

[anakinj]

Or is there a use-case where you would like to enforce a higher minimum than the default...

[304]

Good point! I initially considered making it configurable, but default minimums should be sufficient.

I’ve pushed new changes to the branch using the enforce_hmac_key_length boolean flag instead.

Thanks for the review, @anakinj! Could you please take another look?

[anakinj]

Future us could wonder where are these values coming from, a comment pointing to the RFC would be useful at this point.

[anakinj]

Great stuff, thanks for the addition. Next major release should swap the default to true

[nevans]

This is great, thanks! I have a few "drive by" comments. If you think they're reasonable, I can convert them into a PR.

[nevans]

This should guard against nil to future proof against unknown algorithms, right?

Should it be permissive for unknown algorithms?

Suggested change

	min_length = MIN_KEY_LENGTHS[alg]
	return if key.bytesize >= min_length
	min_length = MIN_KEY_LENGTHS[alg]
	return if minkey_length &.<= key.bytesize

IMO, that's not appropriate. It just leaves open a vector for potential security vulnerabilities.

Should it enforce that new algorithms are also added to MIN_KEY_LENGTHS?

Suggested change

	min_length = MIN_KEY_LENGTHS[alg]
	return if key.bytesize >= min_length
	min_length = MIN_KEY_LENGTHS.fetch(alg)
	return if key.bytesize >= min_length

Alternatively, you can just ask the digest what its length should be:

Suggested change

	min_length = MIN_KEY_LENGTHS[alg]
	return if key.bytesize >= min_length
	min_length = MIN_KEY_LENGTHS.fetch(alg) { digest.new.digest_length }
	return if key.bytesize >= min_length

You could also avoid the extra allocation by instantiating digest.new prior to calling validate_key_length! and pass in min_length: digest_obj.digest_length as an argument. Then you could also simplify it so the MIN_KEY_LENGTHS hash is completely unnecessary:

Suggested change

	min_length = MIN_KEY_LENGTHS[alg]
	return if key.bytesize >= min_length
	return if key.bytesize >= min_length

I'd personally vote for that last option.

[nevans]

IMO, although it's reasonable to preserve backwards compatibility, this should at least warn by default. This may be insignificant in development or CI, but it could be very significant in production. Also, I always appreciate when breaking changes are preceded by a warning period.

Personally, I'd use enforce_hmac_key_length = :warn (vs other truthy values) for warnings. But you could also use nil to indicate unset/default (to warn instead of raise) vs false for explicitly set (to opt-out of warnings).

Suggested change

	raise_verify_error!("HMAC key must be at least #{min_length} bytes for #{alg} algorithm")
	msg = "HMAC key must be at least #{min_length} bytes for #{alg} algorithm"
	if JWT.configuration.decode.enforce_hmac_key_length == :warn
	warn msg
	else
	raise_verify_error!(msg)
	end

[anakinj]

If you have time to look into these I would suggest we swap the default to warn already in the next release.

[anakinj]

@nevans Your suggestions sounds reasonable. I'd be happy to see a PR with the improvements.