-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat!: Require Key object when decoding JWT objects #364
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This aims to provide backwards compatibility by guessing the algorithm for a key based on the key's contents, but it will likely fail in corner cases. If this is merged, users **SHOULD** be explicit about the algorithms they're using. i.e. Instead of $keyAsString, pass in (new JWTKey($keyAsString, 'ES384'))
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
e334e02
to
c6964eb
Compare
// for parsing in this library. Add it manually to your JWK | ||
// array if it doesn't already exist. | ||
// @see https://datatracker.ietf.org/doc/html/rfc7517#section-4.4 | ||
throw new InvalidArgumentException('JWK key is missing "alg"'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paragonie-security if the "alg" parameter is missing from a JWK, is there a reliable way to detect it? Or is it best to just throw the exception and require the users to specify it manually?
We can use the "kty" field to find out of the key is of type RSA or EC. I imagine there is a way to detect more algorithms from there as well, but I don't want to add magic unless I know it's reliable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible, but it's going to require a bit of additional complexity.
First, you have to know if you're dealing with a DER- and PEM-encoded key.
- If it's not base64-encoded, you aren't. (Thus, not PEM.) At this point, you might want to assume symmetric (AES or HMAC).
- Without a breadcrumb to distinguish them, you're safe assuming HMAC. However, in other libraries (i.e. ones with JWE support), our recommendation would be to instead throw an exception. Failure to do so can reintroduce alg confusion.
- If you can base64-decode the string and get a valid byte string, you can then verify that the first few bytes corresponds to a valid ASN.1 object identifier. This will map uniquely to a specific key algorithm.
This table below maps some hex-encoded ASN.1 prefixes to the respective algorithms, based on this encoder and the OIDs specified in IETF RFCs.
ASN.1 Prefix | Key Type |
---|---|
06 09 2A 86 48 86 F7 0D 01 01 01 |
RSA |
06 07 2A 86 48 CE 3D 02 01 |
ECDSA |
30 2E 02 01 00 30 05 06 03 2B 65 70 04 22 04 20 |
EdDSA (Curve25519) |
So that's what you would need to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This is on top of the -----BEGIN PUBLIC KEY-----
and -----END PUBLIC KEY-----
, of course.)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
5 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@googlebot I consent. |
If you need a PHP snippet that will parse a given public key and make a best effort guess at which algorithm is expressed (based on the PEM and DER-encoded bytes of the key), add this method to the JWT class (because private methods): /**
* Attempt to deduce the algorithm for a given key
*
* This should only be used as a last resort.
*
* @param string $key
* @return string|null
* @throws Exception
*/
public static function guessAlgorithmFromKey($key)
{
$length = self::safeStrlen($key);
if ($length <= 64) {
if ($length === 64) return 'HS512';
if ($length === 48) return 'HS384';
// Default to the most common value
return 'HS256';
}
// First, strip off header/footer, to obtain raw base64 value
$expect = '-----BEGIN PUBLIC KEY-----';
$actual = \substr($key, 0, 26);
if (self::constantTimeEquals($expect, $actual)) {
$expect = '-----END PUBLIC KEY-----';
// strlen('-----END PUBLIC KEY-----') === 24
$actual = \substr($key, $length - 24, 24);
if (!self::constantTimeEquals($expect, $actual)) {
// Not a well-formed public key!
return null;
}
// Now we have the base64-encoded portions
$b64 = \trim(\substr($key, 26, $length - 50));
} else {
// Assume base64-encoded
$b64 = $key;
}
// Get raw bytes (provided by sodium_compat)
$raw = sodium_base642bin($b64, SODIUM_BASE64_VARIANT_ORIGINAL, "\r\n");
if (!is_string($raw)) {
// Not a valid PEM-encoded string
return null;
}
$rawLen = self::safeStrlen($raw);
// Is this an EdDSA public key?
$expect = "\x30\x2E\x02\x01\x00\x30\x05\x06\x03\x2B\x65\x70\x04\x22\x04\x20";
$actual = \substr($raw, 0, 16);
if (self::constantTimeEquals($expect, $actual)) {
return 'EdDSA';
}
// Is this an ECDSA public key?
$expect = "\x06\x07\x2A\x86\x48\xCE\x3D\x02\x01";
$actual = \substr($raw, 0, 9);
if (self::constantTimeEquals($expect, $actual)) {
// Okay, which length is it?
if ($rawLen === 91) return 'ES256';
if ($rawLen === 120) return 'ES384';
if ($rawLen === 158) return 'ES512';
}
// Is this an RSA public key?
$expect = "\x06\x09\x2A\x86\x48\x86\xF7\x0D\x01\x01\x01";
$actual = \substr($raw, 0, 11);
if (self::constantTimeEquals($expect, $actual) && $rawLen > 128) {
// An RSA public key can be for multiple algorithms, but we fallback to the most common.
return 'RS256';
}
// Last resort: Return nothing
return null;
} And then your JWK loop can look like this: foreach ($jwks['keys'] as $k => $v) {
$kid = isset($v['kid']) ? $v['kid'] : $k;
if ($key = self::parseKey($v)) {
if (isset($v['alg'])) {
$keys[$kid] = new Key($key, $v['alg']);
} else {
$parsedAlg = JWT::guessAlgorithmFromKey($key);
if (!is_string($parsedAlg)) {
// The "alg" parameter is optional in a KTY, but is required
// for parsing in this library. Add it manually to your JWK
// array if it doesn't already exist.
// @see https://datatracker.ietf.org/doc/html/rfc7517#section-4.4
throw new InvalidArgumentException('JWK key is missing "alg"');
}
$keys[$kid] = new Key($key, $parsedAlg);
}
}
} (Provided as a comment so you can adapt it to your own coding style.) |
Closing in favor of #373 |
see #352 and #351
Builds off #365, but requires the changes (to be tagged in
v6
) for better securityFirebase\JWT\Key
when callingJWT::decode
instead of passing in the$allowed_algs
array.Key
accepts aresource
,string
, or instance ofOpenSSLAsymmetricKey
JWK
now requires the JWK Keyset to havealg
set, which is an optional parameter. If there's a straight foward (and reliable) way to detect the algorithm from the other parameters in JWK, we should add this logic as well.cc @paragonie-security