Require minimum key size for HMAC algorithm

[Slamdunk]

Hi, as reported to us by @Spomky this library currently lacks a key security requirement of JWT standard reported here: https://www.rfc-editor.org/rfc/rfc7518.html#section-3.2

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

Current version of lcobucci/jwt allows indeed any weak key like 1234 to be used for HMAC, for example.

While this issue will hopefully be resolved for future algorithms by low level APIs (see sodium_crypto_sign_detached and sodium_crypto_generichash implementations) I think we should design this library to require any current and future Signer to provide and apply explicit Key validation.

I'm throwing here a PR because, you know, it may be easier to fix something rather than creating it from scratch.
Comments on the code will follow to explain it.

BC-Break is intentional even though I know a MAJOR bump is a blocker: I'd like to shape it the correct way, and only after that adapt it to the current v4 branch.

[Slamdunk]

The diff below represents a way to be sure that:

1. Signers provide key validation
2. Key are actually validated against it

Everything here can easily be backported in v4 implementations, waiting v5 to updated Signer interface

[lcobucci]

I need to think more about this but I think this is not the way to go. I'll review it on the laptop later today

[lcobucci]

I believe this PR is going way beyond to solve the issue and by doing so creates an API that doesn't make sense to me (e.g. Key#sign()).

We need to rethink this.

[Slamdunk]

@lcobucci new day, new RFC

[Slamdunk]

Ping 😳

[Spomky]

Hi,

I see some possibilities on Signer side that could solve this:

1. Each HMAC overrides the sign method and add an assertion to check the key length before calling the parent function.
2. The creation of an interface (let say HasKeyConstraintInterface) with a method that will check the key (protected function checkKey(Key $key): void). In signer classes, if $this implements that interface, the method is called before computation.

Note that I have not tested these solutions. 1. will require relaxing on the final statement. Solution 2. seems more convenient and can be applied to the HMAC or any other Signer.

WDYT?

[lcobucci]

I believe that introducing an interface to handle the key validation generically might create more confusion on the usage.
I honestly see that process tightly coupled to the algorithm performing the signing.

Each HMAC overrides the sign method and add an assertion to check the key length before calling the parent function.

I think that's the way I'd go, but I'd leverage the capabilities of abstract classes instead of overriding Hmac#sign():

abstract class Hmac implements Signer
{
    final public function sign(string $payload, Key $key): string
    {
        $this->guardAgainstWeakKeys($key);

        return hash_hmac($this->algorithm(), $payload, $key->contents(), true);
    }

    private function guardAgainstWeakKeys(Key $key): void
    {
        if (mb_strlen($key->contents(), '8bit') >= $this->minimumLengthForKeys()) {
            return;
        }

        throw new InvalidKeyProvided();
    }

    abstract public function minimumLengthForKeys(): int;

    final public function verify(string $expected, string $payload, Key $key): bool
    {
        return hash_equals($expected, $this->sign($payload, $key));
    }

    abstract public function algorithm(): string;
}

Then, we need to check the trade-offs on the migration path (do we want to introduce classes on the side and mark the existing ones as deprecated, do we want to trigger deprecation errors instead of exceptions on v4.x, how do we avoid breaking BC on the abstract class, ...).

[Slamdunk]

Then, we need to check the trade-offs on the migration path (do we want to introduce classes on the side and mark the existing ones as deprecated, do we want to trigger deprecation errors instead of exceptions on v4.x, how do we avoid breaking BC on the abstract class, ...).

Considering that the current state of art is both out of standard and unsafe, my last push proposes the following:

1. Sha256, Sha384 and Sha512 now raise an InvalidKeyProvided exception when key doesn't fulfill requirements
2. new classes UnsafeSha256, UnsafeSha384 and UnsafeSha512 are introduced with minimum key length of 1, and are already marked as @deprecated

After upgrading to the new release:

1. Users with already valid keys notice nothing
2. Users with invalid keys must choose to either update their keys or swap to UnsafeShaXXX alternative

As far as I can tell, this approach is both backward compatible and caring for security of end users.

[lcobucci]

This is behavioural BC-break, introducing the old behaviour on the side might be alright - especially because people using decent keys should not be affected - but we must document this.

@Slamdunk can you modify the docs to mention HMAC key length requirements?

[Slamdunk]

@lcobucci I'm sorry I lost where we were here