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

Improved OIDC compliance #386

Merged
merged 11 commits into from
Oct 30, 2019
20 changes: 20 additions & 0 deletions .github/stale.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# Configuration for probot-stale - https://github.com/probot/stale

# Number of days of inactivity before an Issue or Pull Request becomes stale
daysUntilStale: 90

# Number of days of inactivity before an Issue or Pull Request with the stale label is closed.
daysUntilClose: 7

# Issues or Pull Requests with these labels will never be considered stale. Set to `[]` to disable
exemptLabels: []

# Set to true to ignore issues with an assignee (defaults to false)
exemptAssignees: true

# Label to use when marking as stale
staleLabel: closed:stale

# Comment to post when marking as stale. Set to `false` to disable
markComment: >
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you have not received a response for our team (apologies for the delay) and this is still a blocker, please reply with additional information or just a ping. Thank you for your contribution! 🙇‍♂️
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
"php": "^7.1",
"guzzlehttp/guzzle": "~6.0",
"ext-json": "*",
"firebase/php-jwt" : "^5.0"
"firebase/php-jwt" : "^5.0",
"lcobucci/jwt": "^3.3"
},
"require-dev": {
"phpunit/phpunit": "^7.0",
Expand Down
1 change: 0 additions & 1 deletion src/API/Authentication.php
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,6 @@ public function oauth_token(array $options = []) : array
*/
public function code_exchange(string $code, string $redirect_uri) : array
{

if (empty($this->client_secret)) {
throw new ApiException('client_secret is mandatory');
}
Expand Down
87 changes: 38 additions & 49 deletions src/Auth0.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@

use Auth0\SDK\Exception\CoreException;
use Auth0\SDK\Exception\ApiException;
use Auth0\SDK\Helpers\Cache\CacheHandler;
use Auth0\SDK\Helpers\Cache\NoCacheHandler;
use Auth0\SDK\Helpers\JWKFetcher;
use Auth0\SDK\Helpers\Tokens\IdTokenVerifier;
use Auth0\SDK\Helpers\Tokens\AsymmetricVerifier;
use Auth0\SDK\Helpers\Tokens\SymmetricVerifier;
use Auth0\SDK\Store\EmptyStore;
use Auth0\SDK\Store\SessionStore;
use Auth0\SDK\Store\StoreInterface;
Expand Down Expand Up @@ -206,25 +212,18 @@ class Auth0
protected $idTokenAlg;

/**
* Valid audiences for ID tokens.
*
* @var array
*/
protected $idTokenAud = [];

/**
* Valid issuer(s) for ID tokens.
* State Handler.
*
* @var array
* @var StateHandler
*/
protected $idTokenIss = [];
protected $stateHandler;

/**
* State Handler.
* Cache Handler.
*
* @var StateHandler
* @var CacheHandler
*/
protected $stateHandler;
protected $cacheHandler;

/**
* BaseAuth0 Constructor.
Expand Down Expand Up @@ -306,31 +305,9 @@ public function __construct(array $config)
$this->skipUserinfo = $config['skip_userinfo'];
}

// If a token algorithm is passed, make sure it's a specific string.
if (! empty($config['id_token_alg'])) {
if (! in_array( $config['id_token_alg'], ['HS256', 'RS256'] )) {
throw new CoreException('Invalid id_token_alg; must be "HS256" or "RS256"');
}

$this->idTokenAlg = $config['id_token_alg'];
}

// If a token audience is passed, make sure it's an array.
if (! empty($config['id_token_aud'])) {
if (! is_array( $config['id_token_aud'] )) {
throw new CoreException('Invalid id_token_aud; must be an array of string values');
}

$this->idTokenAud = $config['id_token_aud'];
}

// If a token issuer is passed, make sure it's an array.
if (! empty($config['id_token_iss'])) {
if (! is_array( $config['id_token_iss'] )) {
throw new CoreException('Invalid id_token_iss; must be an array of string values');
}

$this->idTokenIss = $config['id_token_iss'];
$this->idTokenAlg = $config['id_token_alg'] ?? 'RS256';
if (! in_array( $this->idTokenAlg, ['HS256', 'RS256'] )) {
throw new CoreException('Invalid id_token_alg; must be "HS256" or "RS256"');
}

$this->debugMode = isset($config['debug']) ? $config['debug'] : false;
Expand Down Expand Up @@ -382,6 +359,12 @@ public function __construct(array $config)
$this->stateHandler = new SessionStateHandler($stateStore);
}

if (isset($config['cache_handler']) && $config['cache_handler'] instanceof CacheHandler ) {
$this->cacheHandler = $config['cache_handler'];
} else {
$this->cacheHandler = new NoCacheHandler();
}

$this->authentication = new Authentication(
$this->domain,
$this->clientId,
Expand Down Expand Up @@ -574,7 +557,7 @@ public function exchange()
$this->setRefreshToken($response['refresh_token']);
}

if (! empty($response['id_token'])) {
if (isset($response['id_token'])) {
$this->setIdToken($response['id_token']);
}

Expand Down Expand Up @@ -616,7 +599,10 @@ public function renewTokens()
}

$this->setAccessToken($response['access_token']);
$this->setIdToken($response['id_token']);

if (isset($response['id_token'])) {
$this->setIdToken($response['id_token']);
}
}

/**
Expand Down Expand Up @@ -665,15 +651,18 @@ public function setAccessToken($accessToken)
*/
public function setIdToken($idToken)
{
$jwtVerifier = new JWTVerifier([
'valid_audiences' => ! empty($this->idTokenAud) ? $this->idTokenAud : [ $this->clientId ],
'supported_algs' => $this->idTokenAlg ? [ $this->idTokenAlg ] : [ 'HS256', 'RS256' ],
'authorized_iss' => $this->idTokenIss ? $this->idTokenIss : [ 'https://'.$this->domain.'/' ],
'client_secret' => $this->clientSecret,
'secret_base64_encoded' => $this->clientSecretEncoded,
'guzzle_options' => $this->guzzleOptions,
]);
$this->idTokenDecoded = (array) $jwtVerifier->verifyAndDecode( $idToken );
$idTokenIss = 'https://'.$this->domain.'/';
$sigVerifier = null;
if ('RS256' === $this->idTokenAlg) {
$jwksFetcher = new JWKFetcher($this->cacheHandler, $this->guzzleOptions);
$jwks = $jwksFetcher->getKeys($idTokenIss.'.well-known/jwks.json');
$sigVerifier = new AsymmetricVerifier($jwks);
} else if ('HS256' === $this->idTokenAlg) {
$sigVerifier = new SymmetricVerifier($this->clientSecret);
}

$idTokenVerifier = new IdTokenVerifier($idTokenIss, $this->clientId, $sigVerifier);
$this->idTokenDecoded = $idTokenVerifier->verify($idToken);

if (in_array('id_token', $this->persistantMap)) {
$this->store->set('id_token', $idToken);
Expand Down
55 changes: 55 additions & 0 deletions src/Helpers/Tokens/AsymmetricVerifier.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
<?php
declare(strict_types=1);

namespace Auth0\SDK\Helpers\Tokens;

use Auth0\SDK\Exception\InvalidTokenException;
use Lcobucci\JWT\Signer\Key;
use Lcobucci\JWT\Signer\Rsa\Sha256 as RsSigner;
use Lcobucci\JWT\Token;

/**
* Class AsymmetricVerifier
*
* @package Auth0\SDK\Helpers
*/
final class AsymmetricVerifier extends SignatureVerifier
{

/**
* JWKS array with kid as keys, PEM cert as values.
*
* @var array
*/
private $jwks;

/**
* JwksVerifier constructor.
*
* @param array $jwks JWKS to use.
*/
public function __construct(array $jwks)
{
$this->jwks = $jwks;
parent::__construct('RS256');
}

/**
* Check the token kid and signature.
*
* @param Token $token Parsed token to check.
*
* @return boolean
*
* @throws InvalidTokenException If ID token kid was not found in the JWKS.
*/
protected function checkSignature(Token $token) : bool
{
$tokenKid = $token->getHeader('kid', false);
if (! array_key_exists($tokenKid, $this->jwks)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would ! isset() or empty() make more sense here? The current code is not type safe, given that the Key constructor requires a string $content:

https://github.com/lcobucci/jwt/blob/bdaf6ae2a78ded9b0a9d2c087f63948bc26d58cd/src/Signer/Key.php#L26

Copy link
Contributor Author

@joshcanhelp joshcanhelp Oct 30, 2019

Choose a reason for hiding this comment

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

Good catch but we're using the stable 3.3 branch and that's not a concern:

https://github.com/lcobucci/jwt/blob/3.3/src/Signer/Key.php#L34

An empty value here will fail validation rather than throw a type error.

throw new InvalidTokenException( 'ID token key ID "'.$tokenKid.'" was not found in the JWKS' );
}

return $token->verify(new RsSigner(), new Key($this->jwks[$tokenKid]));
}
}
Loading