Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat!: v6.0 #376

Merged
merged 6 commits into from Dec 30, 2021
Merged

feat!: v6.0 #376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7480196
fix: require key object (#373)
bshaffer Nov 9, 2021
86d8310
chore: cleanup tests (#372)
bshaffer Nov 9, 2021
073a98a
chore: make internal constants private (#374)
bshaffer Nov 10, 2021
070b74d
chore: use JSON_UNESCAPED_SLASHES (#375)
bshaffer Nov 10, 2021
d23c5f4
chore!: make the alg argument required (#377)
bshaffer Nov 11, 2021
2a83728
Merge branch 'main' into v6.0
bshaffer Dec 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Expand Up @@ -3,3 +3,4 @@ phpunit.phar
phpunit.phar.asc
composer.phar
composer.lock
.phpunit.result.cache
3 changes: 1 addition & 2 deletions README.md
View file
Open in desktop
Expand Up @@ -200,8 +200,7 @@ $jwks = ['keys' => []];

// JWK::parseKeySet($jwks) returns an associative array of **kid** to private
// key. Pass this as the second parameter to JWT::decode.
// NOTE: The deprecated $supportedAlgorithm must be supplied when parsing from JWK.
JWT::decode($payload, JWK::parseKeySet($jwks), $supportedAlgorithm);
JWT::decode($payload, JWK::parseKeySet($jwks));
```

Changelog
Expand Down
10 changes: 9 additions & 1 deletion src/JWK.php
View file
Open in desktop
Expand Up @@ -47,7 +47,15 @@ public static function parseKeySet(array $jwks)
foreach ($jwks['keys'] as $k => $v) {
$kid = isset($v['kid']) ? $v['kid'] : $k;
if ($key = self::parseKey($v)) {
$keys[$kid] = $key;
if (isset($v['alg'])) {
$keys[$kid] = new Key($key, $v['alg']);
} else {
// 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"');
}
Comment on lines +50 to +58
Copy link

@Nextra Nextra Apr 29, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bshaffer

This is a breaking change that is currently not obvious from the 6.0 release notes.

As a notable example, Microsoft do not output JWK with the alg key populated:
https://login.microsoftonline.com/common/discovery/keys

I think the release notes should encourage developers to inspect JWK::parseKeySet beyond just its return type.

Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just updated the release notes, @Nextra, thanks for the feedback.

I think it would be a better user-experience to have a second optional argument $algorithm to parseKeySet, to help with this edgecase.

$algorithm = 'RS256'; // default algorithm used when "alg" isn't set
$jwks = JWK::parseKeySet($jwks, $algorithm);

I didn't initially add it because I was hoping in practice that most JWKS would be populating the alg parameter. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #426

}
}

Expand Down
94 changes: 41 additions & 53 deletions src/JWT.php
View file
Open in desktop
Expand Up @@ -25,9 +25,12 @@
*/
class JWT
{
const ASN1_INTEGER = 0x02;
const ASN1_SEQUENCE = 0x10;
const ASN1_BIT_STRING = 0x03;
// const ASN1_INTEGER = 0x02;
// const ASN1_SEQUENCE = 0x10;
// const ASN1_BIT_STRING = 0x03;
private static $asn1Integer = 0x02;
private static $asn1Sequence = 0x10;
private static $asn1BitString = 0x03;

/**
* When checking nbf, iat or expiration times,
Expand Down Expand Up @@ -60,13 +63,11 @@ class JWT
* Decodes a JWT string into a PHP object.
*
* @param string $jwt The JWT
* @param Key|array<Key>|mixed $keyOrKeyArray The Key or array of Key objects.
* @param Key|array<Key> $keyOrKeyArray The Key or array of Key objects.
* If the algorithm used is asymmetric, this is the public key
* Each Key object contains an algorithm and matching key.
* Supported algorithms are 'ES384','ES256', 'HS256', 'HS384',
* 'HS512', 'RS256', 'RS384', and 'RS512'
* @param array $allowed_algs [DEPRECATED] List of supported verification algorithms. Only
* should be used for backwards compatibility.
*
* @return object The JWT's payload as a PHP object
*
Expand All @@ -81,8 +82,9 @@ class JWT
* @uses jsonDecode
* @uses urlsafeB64Decode
*/
public static function decode($jwt, $keyOrKeyArray, array $allowed_algs = array())
public static function decode($jwt, $keyOrKeyArray)
{
// Validate JWT
$timestamp = \is_null(static::$timestamp) ? \time() : static::$timestamp;

if (empty($keyOrKeyArray)) {
Expand All @@ -109,31 +111,18 @@ public static function decode($jwt, $keyOrKeyArray, array $allowed_algs = array(
throw new UnexpectedValueException('Algorithm not supported');
}

list($keyMaterial, $algorithm) = self::getKeyMaterialAndAlgorithm(
$keyOrKeyArray,
empty($header->kid) ? null : $header->kid
);
$key = self::getKey($keyOrKeyArray, empty($header->kid) ? null : $header->kid);

if (empty($algorithm)) {
// Use deprecated "allowed_algs" to determine if the algorithm is supported.
// This opens up the possibility of an attack in some implementations.
// @see https://github.com/firebase/php-jwt/issues/351
if (!\in_array($header->alg, $allowed_algs)) {
throw new UnexpectedValueException('Algorithm not allowed');
}
} else {
// Check the algorithm
if (!self::constantTimeEquals($algorithm, $header->alg)) {
// See issue #351
throw new UnexpectedValueException('Incorrect key for this algorithm');
}
// Check the algorithm
if (!self::constantTimeEquals($key->getAlgorithm(), $header->alg)) {
// See issue #351
throw new UnexpectedValueException('Incorrect key for this algorithm');
}
if ($header->alg === 'ES256' || $header->alg === 'ES384') {
// OpenSSL expects an ASN.1 DER sequence for ES256/ES384 signatures
$sig = self::signatureToDER($sig);
}

if (!static::verify("$headb64.$bodyb64", $sig, $keyMaterial, $header->alg)) {
if (!static::verify("$headb64.$bodyb64", $sig, $key->getKeyMaterial(), $header->alg)) {
throw new SignatureInvalidException('Signature verification failed');
}

Expand Down Expand Up @@ -179,7 +168,7 @@ public static function decode($jwt, $keyOrKeyArray, array $allowed_algs = array(
* @uses jsonEncode
* @uses urlsafeB64Encode
*/
public static function encode($payload, $key, $alg = 'HS256', $keyId = null, $head = null)
public static function encode($payload, $key, $alg, $keyId = null, $head = null)
{
$header = array('typ' => 'JWT', 'alg' => $alg);
if ($keyId !== null) {
Expand Down Expand Up @@ -212,7 +201,7 @@ public static function encode($payload, $key, $alg = 'HS256', $keyId = null, $he
*
* @throws DomainException Unsupported algorithm or bad key was specified
*/
public static function sign($msg, $key, $alg = 'HS256')
public static function sign($msg, $key, $alg)
{
if (empty(static::$supported_algs[$alg])) {
throw new DomainException('Algorithm not supported');
Expand Down Expand Up @@ -345,7 +334,12 @@ public static function jsonDecode($input)
*/
public static function jsonEncode($input)
{
$json = \json_encode($input);
if (PHP_VERSION_ID >= 50400) {
$json = \json_encode($input, \JSON_UNESCAPED_SLASHES);
} else {
// PHP 5.3 only
$json = \json_encode($input);
}
if ($errno = \json_last_error()) {
static::handleJsonError($errno);
} elseif ($json === 'null' && $input !== null) {
Expand Down Expand Up @@ -394,40 +388,34 @@ public static function urlsafeB64Encode($input)
*
* @return array containing the keyMaterial and algorithm
*/
private static function getKeyMaterialAndAlgorithm($keyOrKeyArray, $kid = null)
private static function getKey($keyOrKeyArray, $kid = null)
{
if (
is_string($keyOrKeyArray)
|| is_resource($keyOrKeyArray)
|| $keyOrKeyArray instanceof OpenSSLAsymmetricKey
) {
return array($keyOrKeyArray, null);
}

if ($keyOrKeyArray instanceof Key) {
return array($keyOrKeyArray->getKeyMaterial(), $keyOrKeyArray->getAlgorithm());
return $keyOrKeyArray;
}

if (is_array($keyOrKeyArray) || $keyOrKeyArray instanceof ArrayAccess) {
foreach ($keyOrKeyArray as $keyId => $key) {
if (!$key instanceof Key) {
throw new UnexpectedValueException(
'$keyOrKeyArray must be an instance of Firebase\JWT\Key key or an '
. 'array of Firebase\JWT\Key keys'
);
}
}
if (!isset($kid)) {
throw new UnexpectedValueException('"kid" empty, unable to lookup correct key');
}
if (!isset($keyOrKeyArray[$kid])) {
throw new UnexpectedValueException('"kid" invalid, unable to lookup correct key');
}

$key = $keyOrKeyArray[$kid];

if ($key instanceof Key) {
return array($key->getKeyMaterial(), $key->getAlgorithm());
}

return array($key, null);
return $keyOrKeyArray[$kid];
}

throw new UnexpectedValueException(
'$keyOrKeyArray must be a string|resource key, an array of string|resource keys, '
. 'an instance of Firebase\JWT\Key key or an array of Firebase\JWT\Key keys'
'$keyOrKeyArray must be an instance of Firebase\JWT\Key key or an '
. 'array of Firebase\JWT\Key keys'
);
}

Expand Down Expand Up @@ -515,9 +503,9 @@ private static function signatureToDER($sig)
}

return self::encodeDER(
self::ASN1_SEQUENCE,
self::encodeDER(self::ASN1_INTEGER, $r) .
self::encodeDER(self::ASN1_INTEGER, $s)
self::$asn1Sequence,
self::encodeDER(self::$asn1Integer, $r) .
self::encodeDER(self::$asn1Integer, $s)
);
}

Expand All @@ -531,7 +519,7 @@ private static function signatureToDER($sig)
private static function encodeDER($type, $value)
{
$tag_header = 0;
if ($type === self::ASN1_SEQUENCE) {
if ($type === self::$asn1Sequence) {
$tag_header |= 0x20;
}

Expand Down Expand Up @@ -596,7 +584,7 @@ private static function readDER($der, $offset = 0)
}

// Value
if ($type == self::ASN1_BIT_STRING) {
if ($type == self::$asn1BitString) {
$pos++; // Skip the first contents octet (padding indicator)
$data = \substr($der, $pos, $len - 1);
$pos += $len - 1;
Expand Down
38 changes: 27 additions & 11 deletions tests/JWKTest.php
View file
Open in desktop
Expand Up @@ -38,34 +38,50 @@ public function testParsePrivateKey()
'UnexpectedValueException',
'RSA private keys are not supported'
);

$jwkSet = json_decode(
file_get_contents(__DIR__ . '/rsa-jwkset.json'),
file_get_contents(__DIR__ . '/data/rsa-jwkset.json'),
true
);
$jwkSet['keys'][0]['d'] = 'privatekeyvalue';


JWK::parseKeySet($jwkSet);
}

public function testParsePrivateKeyWithoutAlg()
{
$this->setExpectedException(
'InvalidArgumentException',
'JWK key is missing "alg"'
);

$jwkSet = json_decode(
file_get_contents(__DIR__ . '/data/rsa-jwkset.json'),
true
);
unset($jwkSet['keys'][0]['alg']);

JWK::parseKeySet($jwkSet);
}

public function testParseKeyWithEmptyDValue()
{
$jwkSet = json_decode(
file_get_contents(__DIR__ . '/rsa-jwkset.json'),
file_get_contents(__DIR__ . '/data/rsa-jwkset.json'),
true
);

// empty or null values are ok
$jwkSet['keys'][0]['d'] = null;

$keys = JWK::parseKeySet($jwkSet);
$this->assertTrue(is_array($keys));
}

public function testParseJwkKeySet()
{
$jwkSet = json_decode(
file_get_contents(__DIR__ . '/rsa-jwkset.json'),
file_get_contents(__DIR__ . '/data/rsa-jwkset.json'),
true
);
$keys = JWK::parseKeySet($jwkSet);
Expand Down Expand Up @@ -93,7 +109,7 @@ public function testParseJwkKeySet_empty()
*/
public function testDecodeByJwkKeySetTokenExpired()
{
$privKey1 = file_get_contents(__DIR__ . '/rsa1-private.pem');
$privKey1 = file_get_contents(__DIR__ . '/data/rsa1-private.pem');
$payload = array('exp' => strtotime('-1 hour'));
$msg = JWT::encode($payload, $privKey1, 'RS256', 'jwk1');

Expand All @@ -107,7 +123,7 @@ public function testDecodeByJwkKeySetTokenExpired()
*/
public function testDecodeByJwkKeySet()
{
$privKey1 = file_get_contents(__DIR__ . '/rsa1-private.pem');
$privKey1 = file_get_contents(__DIR__ . '/data/rsa1-private.pem');
$payload = array('sub' => 'foo', 'exp' => strtotime('+10 seconds'));
$msg = JWT::encode($payload, $privKey1, 'RS256', 'jwk1');

Expand All @@ -121,7 +137,7 @@ public function testDecodeByJwkKeySet()
*/
public function testDecodeByMultiJwkKeySet()
{
$privKey2 = file_get_contents(__DIR__ . '/rsa2-private.pem');
$privKey2 = file_get_contents(__DIR__ . '/data/rsa2-private.pem');
$payload = array('sub' => 'bar', 'exp' => strtotime('+10 seconds'));
$msg = JWT::encode($payload, $privKey2, 'RS256', 'jwk2');

Expand Down