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.

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

Add custom JWKS path and kid check to JWKFetcher + tests #287

Merged
merged 2 commits into from
Sep 26, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .phpcs.xml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@
<rule ref="Squiz.PHP">
<exclude name="Squiz.PHP.DisallowComparisonAssignment.AssignedComparison"/>
<exclude name="Squiz.PHP.DisallowInlineIf.Found"/>
<exclude name="Squiz.PHP.DisallowBooleanStatement.Found"/>
</rule>
<rule ref="Squiz.Scope"/>
<rule ref="Squiz.Strings"/>
Expand Down
4 changes: 2 additions & 2 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@
}
},
"scripts": {
"test": "SHELL_INTERACTIVE=1 vendor/bin/phpunit --colors=always --verbose ",
"test-ci": "vendor/bin/phpunit --coverage-text --coverage-clover=build/coverage.xml",
"test": "SHELL_INTERACTIVE=1 vendor/bin/phpunit --colors=always --coverage-text",
"test-ci": "vendor/bin/phpunit --colors=always --coverage-clover=build/coverage.xml",
"phpcs": "\"vendor/bin/phpcs\"",
"phpcs-path": "SHELL_INTERACTIVE=1 ./vendor/bin/phpcs",
"phpcbf": "\"vendor/bin/phpcbf\"",
Expand Down
7 changes: 3 additions & 4 deletions phpunit.xml.dist
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
<phpunit
colors="true"
bootstrap="./vendor/autoload.php"
>
coverage-text="true"
bootstrap="./tests/bootstrap.php">
<testsuites>
<testsuite name="Auth0 Test Suite">
<testsuite name="Auth0 PHP SDK Test Suite">
<directory>./tests</directory>
</testsuite>
</testsuites>
Expand Down
128 changes: 122 additions & 6 deletions src/Helpers/JWKFetcher.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,26 @@
use Auth0\SDK\Helpers\Cache\CacheHandler;
use Auth0\SDK\Helpers\Cache\NoCacheHandler;

use GuzzleHttp\Exception\RequestException;
use GuzzleHttp\Exception\ClientException;

/**
* Class JWKFetcher.
*
* @package Auth0\SDK\Helpers
*/
class JWKFetcher
{

/**
* Cache handler or null for no caching.
*
* @var CacheHandler|NoCacheHandler
* @var CacheHandler|null
*/
private $cache = null;

/**
* Options for the Guzzle HTTP client.
*
* @var array
*/
Expand All @@ -24,10 +34,10 @@ class JWKFetcher
/**
* JWKFetcher constructor.
*
* @param CacheHandler|null $cache
* @param array $guzzleOptions
* @param CacheHandler|null $cache Cache handler or null for no caching.
Copy link
Contributor

Choose a reason for hiding this comment

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

But above you say "NoCacheHandler" as well. Should people pass NoCacheHandler or null for this to have no cache?

* @param array $guzzleOptions Options for the Guzzle HTTP client.
*/
public function __construct(CacheHandler $cache = null, $guzzleOptions = [])
public function __construct(CacheHandler $cache = null, array $guzzleOptions = [])
{
if ($cache === null) {
$cache = new NoCacheHandler();
Expand All @@ -38,22 +48,32 @@ public function __construct(CacheHandler $cache = null, $guzzleOptions = [])
}

/**
* Convert a certificate to PEM format.
*
* @param string $cert X509 certificate to convert to PEM format.
*
* @param string $cert
* @return string
*/
protected function convertCertToPem($cert)
lbalmaceda marked this conversation as resolved.
Show resolved Hide resolved
{
return '-----BEGIN CERTIFICATE-----'.PHP_EOL.chunk_split($cert, 64, PHP_EOL).'-----END CERTIFICATE-----'.PHP_EOL;
$output = '-----BEGIN CERTIFICATE-----'.PHP_EOL;
$output .= chunk_split($cert, 64, PHP_EOL);
$output .= '-----END CERTIFICATE-----'.PHP_EOL;
return $output;
}

// phpcs:disable
/**
* Appends the default JWKS path to a token issuer to return all keys from a JWKS.
* TODO: Deprecate, use $this->getJwksX5c() instead and explain why/how.
*
* @param string $iss
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is going to get deprecated. Anyway, I'd document this parameter like: "The auth0 issuer which will get appended the /.well-known/jwks.json path to fetch the jwks."

*
* @return array|mixed|null
*
* @throws \Exception
*
* @codeCoverageIgnore
Copy link
Contributor

Choose a reason for hiding this comment

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

👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/**
  * @luchoIngore
  */

*/
public function fetchKeys($iss)
lbalmaceda marked this conversation as resolved.
Show resolved Hide resolved
{
Expand All @@ -79,4 +99,100 @@ public function fetchKeys($iss)

return $secret;
}
// phpcs:enable

/**
* Fetch x509 cert for RS256 token decoding.
*
* @param string $jwks_url URL to the JWKS.
* @param string|null $kid Key ID to use; returns first JWK if $kid is null or empty.
*
* @return string|null - Null if an x5c key could not be found for a key ID or if the JWKS is empty/invalid.
*/
public function requestJwkX5c($jwks_url, $kid = null)
{
$cache_key = $jwks_url.'|'.(string) $kid;
Copy link
Contributor

Choose a reason for hiding this comment

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

think of the case that kid is null (not passed as arg). This will also break the cache impl below ;) I personally would skip cache in this particular case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

null is technically a value we can handle (first key) so why not cache for that value?

Copy link
Contributor

Choose a reason for hiding this comment

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

if null get's converted to "null" (note the quotes) then I guess it's a valid string and you will end up having a valid key, so yes, you will be able to handle just fine null kids (pun intended)


$x5c = $this->cache->get($cache_key);
if (! is_null($x5c)) {
return $x5c;
}

$jwks = $this->requestJwks($jwks_url);
$jwk = $this->findJwk($jwks, $kid);

if ($this->subArrayHasEmptyFirstItem($jwk, 'x5c')) {
return null;
}

$x5c = $this->convertCertToPem($jwk['x5c'][0]);
$this->cache->set($cache_key, $x5c);
return $x5c;
}

/**
* Get a JWKS from a specific URL.
*
* @param string $jwks_url URL to the JWKS.
*
* @return mixed|string
*
* @throws RequestException If $jwks_url is empty or malformed.
* @throws ClientException If the JWKS cannot be retrieved.
*
* @codeCoverageIgnore
*/
protected function requestJwks($jwks_url)
{
$request = new RequestBuilder([
'domain' => $jwks_url,
'method' => 'GET',
'guzzleOptions' => $this->guzzleOptions
]);
return $request->call();
}

/**
* Get a JWK from a JWKS using a key ID, if provided.
*
* @param array $jwks JWKS to parse.
* @param null|string $kid Key ID to return; returns first JWK if $kid is null or empty.
*
* @return array|null Null if the keys array is empty or if the key ID is not found.
*
* @codeCoverageIgnore
*/
private function findJwk(array $jwks, $kid = null)
{
if ($this->subArrayHasEmptyFirstItem($jwks, 'keys')) {
return null;
}

if (! $kid) {
return $jwks['keys'][0];
}

foreach ($jwks['keys'] as $key) {
if (isset($key['kid']) && $key['kid'] === $kid) {
return $key;
}
}

return null;
}

/**
* Check if an array within an array has a non-empty first item.
*
* @param array|null $array Main array to check.
* @param string $key Key pointing to a sub-array.
*
* @return boolean
*
* @codeCoverageIgnore
Copy link
Contributor

Choose a reason for hiding this comment

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

why? you could easily unit-test this :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's private, why would I do that? Also:

Please note that final, private, protected, and static methods cannot be stubbed or mocked.

*/
private function subArrayHasEmptyFirstItem($array, $key)
{
return empty($array) || ! is_array($array[$key]) || empty($array[$key][0]);
}
}
9 changes: 8 additions & 1 deletion src/JWTVerifier.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ class JWTVerifier

protected $secret_base64_encoded = null;

protected $jwks_path = '.well-known/jwks.json';

/**
* JWTVerifier Constructor.
*
Expand Down Expand Up @@ -87,6 +89,10 @@ public function __construct($config)
$this->client_secret = $config['client_secret'];
}

if (isset($config['jwks_path'])) {
$this->jwks_path = (string) $config['jwks_path'];
}

$this->JWKFetcher = new JWKFetcher($cache, $guzzleOptions);
}

Expand Down Expand Up @@ -125,7 +131,8 @@ public function verifyAndDecode($jwt)
throw new CoreException("We can't trust on a token issued by: `{$body->iss}`.");
}

$secret = $this->JWKFetcher->fetchKeys($body->iss);
$jwks_url = $body->iss.$this->jwks_path;
$secret = $this->JWKFetcher->fetchKeys($jwks_url);
} else if ($head->alg === 'HS256') {
if ($this->secret_base64_encoded) {
$secret = JWT::urlsafeB64Decode($this->client_secret);
Expand Down
3 changes: 2 additions & 1 deletion src/Store/SessionStore.php
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,10 @@ public function delete($key)
public function getSessionKeyName($key)
{
$key_name = $key;
if ( ! empty( $this->session_base_name ) ) {
if (! empty( $this->session_base_name )) {
$key_name = $this->session_base_name.'_'.$key_name;
}

return $key_name;
}
}
Loading