Add ID token validation #285

joshcanhelp · 2018-09-05T23:52:25Z

This SDK already had a JWTVerifier class to verify ID tokens but that class was not being used in the default login route. This PR adds this validation to Auth0\SDK\Auth0::setIdToken() before the incoming ID token is stored. Underlying validation library is Firebase JWT.

What is validated

Issuer
Audience
Signature
NBF
IAT
Expiration

Potential issues

NBF and IAT validation

Both of these values check a timecode in the token payload against the time set on the server doing the validation. If your server time is different from the issuing server, this could cause the validation to fail. If you're getting an error message stating Cannot handle token prior to ..., then you can try setting a leeway in the Firebase JWT library, like this:

// Set this before the Auth0 class is initialized.
// Value is in seconds and should be no greater than a few minutes.

\Firebase\JWT\JWT::$leeway = 30;

// ... 
$auth0 = new Auth0( [ 
    // ...
] );

Client Secret encoding

Most tenants will not have a base64-encoded Client Secret but older tenants might. It will display on your Application settings page whether this is the case or not:

By default, the JWT verifier uses the Client Secret as-is with no decoding. If your Client Secret is encoded, add a new value to your Auth0 initialization array like this:

$auth0 = new Auth0( [ 
    // ...
    'secret_base64_encoded' => true,
    // ...
] );

lbalmaceda

please add the missing tests

lbalmaceda · 2018-09-06T18:19:43Z

src/Auth0.php

+                'client_secret' => $this->clientSecret,
+                'secret_base64_encoded' => $this->clientSecretEncoded,
+                'authorized_iss' => [ $issuer ],
+                'supported_algs' => [ 'HS256', 'RS256' ],


Can this be a setting / parameter?

lbalmaceda · 2018-09-06T18:20:24Z

src/Auth0.php

+                'secret_base64_encoded' => $this->clientSecretEncoded,
+                'authorized_iss' => [ $issuer ],
+                'supported_algs' => [ 'HS256', 'RS256' ],
+                'valid_audiences' => [


IMO this should be a param too. What if the user requests a token for a custom API ? audience will appear here

lbalmaceda · 2018-09-06T18:20:57Z

src/Auth0.php

+            $issuer      = 'https://'.$this->domain.'/';
+            $jwtVerifier = new JWTVerifier([
+                'guzzle_options' => $this->guzzleOptions,
+                'client_secret' => $this->clientSecret,


what's this and the param below user for if the alg happens to be RS?

lbalmaceda · 2018-09-06T18:21:46Z

src/Auth0.php

+        } catch (\Exception $e) {
+            $message = 'ID token could not be validated.';
+            if ($e->getMessage()) {
+                $message .= ' '.$e->getMessage();


why do you need to prepend ' '? Can't you do that on the "if" with something like isEmpty($e->getMessage())?

lbalmaceda · 2018-09-06T18:22:52Z

src/Auth0.php

@@ -556,9 +565,35 @@ public function setAccessToken($accessToken)
     * @param string $idToken - ID token returned from the code exchange.
     *
     * @return \Auth0\SDK\Auth0
+     *
+     * @throws CoreException If ID token did not validate.


I'd add here or in the readme (docs, wherever) all the special cases you mentioned on the PR's description. e.g. the leeway when time verification fails

lbalmaceda

💣 💥 🙉

lbalmaceda · 2018-10-01T20:45:08Z

src/API/Helpers/TokenGenerator.php

-
-    /**
+     * @param array   $scopes         Array of scopes to include.
+     * @param integer $lifetime       Lifetime of the token.


minor but "in seconds"

lbalmaceda · 2018-10-01T20:47:17Z

src/API/Helpers/TokenGenerator.php

     */
-    protected function bstr2bin($input)
+    public function __construct($client_id, $client_secret)


since this class is called "TokenGenerator" what about renaming this props to audience and secret, so they refer more to a generic token rather than auth0

lbalmaceda · 2018-10-01T20:47:47Z

src/API/Helpers/TokenGenerator.php

+ * Class TokenGenerator.
+ * Generates HS256 ID tokens.
+ *
+ * @package Auth0\SDK\API\Helpers


I imagine this class is only used on tests, right? If so, wouldn't the correct namespace be under a test folder?

That's how it's used in the SDK, yes, but it's already in a public namespace so removing would be breaking. Making a note of this for the major.

lbalmaceda · 2018-10-01T20:49:32Z

src/Auth0.php

@@ -71,6 +72,13 @@ class Auth0
     */
    protected $clientSecret;

+    /**
+     * Is the Auth0 Client Secret encoded?


I'd not make this a question and add where I can know this information. e.g. "Check in your Application's settings under the Client Secret field."

lbalmaceda · 2018-10-01T20:52:09Z

src/Auth0.php

+    /**
+     * Algorithm to use for ID token validation.
+     *
+     * @var string


I guess there are a set of allowed values?

lbalmaceda · 2018-10-01T22:05:31Z

src/JWTVerifier.php

+     *
+     * @codeCoverageIgnore
+     */
+    private function supportsAlg($alg)


I know you're using this in 2 or 3 places so I get why you moved this to a method. But this sounds more like a "array contains" method. (which you're using in many other places such as when comparing audiences). Consider refactoring this 🆗

lbalmaceda · 2018-10-01T22:07:33Z

src/JWTVerifier.php

+     */
+    private function decodeTokenSegment($segment)
+    {
+        return JWT::jsonDecode(JWT::urlsafeB64Decode($segment));


why not make the "base64 decode part" call the method you defined above --> decodeB64 ?

lbalmaceda · 2018-10-01T22:08:38Z

tests/API/ApiTests.php

@@ -1,12 +1,14 @@
 <?php
 namespace Auth0\Tests\API;

+use Auth0\Tests\Traits\ErrorHelpers;
 use Auth0\SDK\API\Helpers\TokenGenerator;


I think I mentioned this in a comment above, but this token generator seems to be only used on this test. Please move it to the /Test/ folder so users don't use it

lbalmaceda · 2018-10-01T22:12:23Z

tests/API/Helpers/TokenTest.php

+     *
+     * @return void
+     */
+    public function testThatMissingSecretThrowsException()


in general, use the method name to describe the test as most as possible. e.g. testThatMissingSecretWhenUsingHS256ThrowsException

lbalmaceda · 2018-10-01T22:14:16Z

tests/API/Helpers/TokenTest.php

-                'client_secret' => $client_secret,
-                'secret_base64_encoded' => false
-            ]
+        // 2. Test that a non-encoded client secret can be used.


You might want to split this test into 2 separate ones. When a test fails is easier to read that by reading the test/method name rather than having to look at the line of code that caused this to fail.

lbalmaceda · 2018-10-08T20:27:21Z

src/JWTVerifier.php

+     *
+     * @var JWKFetcher|null
+     */
+    protected $jwks_fetcher = null;


this is protected. Changing this name will make subclasses break AFAIK.

github-actions · 2022-08-24T00:19:56Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

joshcanhelp added the CH: Security label Sep 5, 2018

joshcanhelp added this to the v5-Next milestone Sep 5, 2018

joshcanhelp force-pushed the add-id-token-validation branch from 9b4558b to ffa8e49 Compare September 5, 2018 23:54

lbalmaceda suggested changes Sep 6, 2018

View reviewed changes

joshcanhelp force-pushed the add-id-token-validation branch 2 times, most recently from aec4347 to 281bd0b Compare September 28, 2018 17:28

joshcanhelp added the Ready to Review label Sep 28, 2018

lbalmaceda reviewed Oct 1, 2018

View reviewed changes

joshcanhelp added 4 commits October 8, 2018 10:02

Add ID token validation

a9c487b

Refactor ID token verification and add tests

d00be63

PR feedback

e5e5163

Explicitly require ErrorHelper trait

e155c56

joshcanhelp force-pushed the add-id-token-validation branch from 23d010c to e155c56 Compare October 8, 2018 17:02

lbalmaceda reviewed Oct 8, 2018

View reviewed changes

PR feedback

6070dda

lbalmaceda approved these changes Oct 8, 2018

View reviewed changes

Fix error message expected text

3eb1762

joshcanhelp removed the Ready to Review label Oct 8, 2018

joshcanhelp merged commit bdbe01e into master Oct 8, 2018

joshcanhelp deleted the add-id-token-validation branch October 8, 2018 20:57

github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ID token validation #285

Add ID token validation #285

joshcanhelp commented Sep 5, 2018

lbalmaceda left a comment

lbalmaceda Sep 6, 2018

lbalmaceda Sep 6, 2018

lbalmaceda Sep 6, 2018

lbalmaceda Sep 6, 2018

lbalmaceda Sep 6, 2018

lbalmaceda left a comment

lbalmaceda Oct 1, 2018

lbalmaceda Oct 1, 2018

lbalmaceda Oct 1, 2018

joshcanhelp Oct 2, 2018

lbalmaceda Oct 1, 2018

lbalmaceda Oct 1, 2018

lbalmaceda Oct 1, 2018

lbalmaceda Oct 1, 2018

lbalmaceda Oct 1, 2018

joshcanhelp Oct 2, 2018

lbalmaceda Oct 1, 2018

lbalmaceda Oct 1, 2018

lbalmaceda Oct 8, 2018

github-actions bot commented Aug 24, 2022

Add ID token validation #285

Add ID token validation #285

Conversation

joshcanhelp commented Sep 5, 2018

What is validated

Potential issues

NBF and IAT validation

Client Secret encoding

lbalmaceda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 24, 2022