Merge pull request #13 from bedita/fix/jwt-parsing-error

Fix JWT parsing error
bedita · Jun 30, 2023 · 6d00b46 · 6d00b46
2 parents 548a72d + fcabb72
commit 6d00b46
Show file tree

Hide file tree

Showing 3 changed files with 102 additions and 34 deletions.
diff --git a/.github/workflows/php.yml b/.github/workflows/php.yml
@@ -16,12 +16,12 @@ jobs:
   cs:
     uses: bedita/github-workflows/.github/workflows/php-cs.yml@v1
     with:
-      php_versions: '["8.1"]'
+      php_versions: '["8.2"]'
 
   stan:
     uses: bedita/github-workflows/.github/workflows/php-stan.yml@v1
     with:
-      php_versions: '["8.1"]'
+      php_versions: '["8.2"]'
 
   unit:
     name: 'Run unit tests'
@@ -35,6 +35,7 @@ jobs:
           - '7.4'
           - '8.0'
           - '8.1'
+          - '8.2'
 
     env:
       PHP_VERSION: '${{ matrix.php }}'
@@ -49,7 +50,7 @@ jobs:
           php-version: '${{ matrix.php }}'
           tools: 'composer'
           extensions: 'mbstring, intl'
-          coverage: 'none' # Using `phpdbg`
+          coverage: 'xdebug'
 
       - name: 'Discover Composer cache directory'
         id: 'cachedir'
@@ -71,7 +72,7 @@ jobs:
         run: 'composer dump-autoload --classmap-authoritative --no-cache'
 
       - name: 'Run PHPUnit'
-        run: 'phpdbg -qrr vendor/bin/phpunit --coverage-clover=clover.xml'
+        run: 'vendor/bin/phpunit --coverage-clover=clover.xml'
 
       - name: 'Export coverage results'
         uses: 'codecov/codecov-action@v1'

diff --git a/src/Authenticator/AlbAuthenticator.php b/src/Authenticator/AlbAuthenticator.php
@@ -25,10 +25,13 @@
 use Exception;
 use GuzzleHttp\Client;
 use GuzzleHttp\RequestOptions;
+use JsonException;
 use Lcobucci\Clock\FrozenClock;
-use Lcobucci\JWT\Encoding\JoseEncoder;
+use Lcobucci\JWT\Decoder;
+use Lcobucci\JWT\Encoding\CannotDecodeContent;
 use Lcobucci\JWT\Signer\Ecdsa\Sha256;
 use Lcobucci\JWT\Signer\Key;
+use Lcobucci\JWT\SodiumBase64Polyfill;
 use Lcobucci\JWT\Token\Parser as TokenParser;
 use Lcobucci\JWT\UnencryptedToken;
 use Lcobucci\JWT\Validation\Constraint\LooseValidAt;
@@ -185,7 +188,26 @@ function () use ($keyId): string {
      */
     protected function decodeToken(string $token): ?array
     {
-        $jwt = (new TokenParser(new JoseEncoder()))->parse($token);
+        $jwt = (new TokenParser(new class implements Decoder {
+            /** @inheritdoc */
+            public function jsonDecode(string $json)
+            {
+                try {
+                    return json_decode($json, true, 512, JSON_THROW_ON_ERROR);
+                } catch (JsonException $exception) {
+                    throw CannotDecodeContent::jsonIssues($exception);
+                }
+            }
+
+            /** @inheritdoc */
+            public function base64UrlDecode(string $data): string
+            {
+                return SodiumBase64Polyfill::base642bin(
+                    rtrim($data, '='),
+                    SodiumBase64Polyfill::SODIUM_BASE64_VARIANT_URLSAFE_NO_PADDING,
+                );
+            }
+        }))->parse($token);
         $kid = $jwt->headers()->get('kid');
         if (empty($kid) || !is_string($kid) || !$jwt instanceof UnencryptedToken) {
             return null;

diff --git a/tests/TestCase/Authenticator/AlbAuthenticatorTest.php b/tests/TestCase/Authenticator/AlbAuthenticatorTest.php
@@ -81,29 +81,47 @@ protected function setUp(): void
     {
         parent::setUp();
 
-        $curves = openssl_get_curve_names();
-        assert(is_array($curves));
-
-        $key = openssl_pkey_new([
-            'digest_alg' => OPENSSL_ALGO_SHA256,
-            'private_key_type' => OPENSSL_KEYTYPE_EC,
-            'curve_name' => $curves[array_key_last($curves)],
-        ]);
-        assert($key !== false);
-        openssl_pkey_export($key, $privateKey);
-        $keyInfo = openssl_pkey_get_details($key);
-        assert($keyInfo !== false);
-        if (is_resource($key)) {
-            openssl_free_key($key);
-        }
+        $privateKey = static::runCmd(['openssl', 'ecparam', '-name', 'prime256v1', '-genkey', '-noout']);
+        assert($privateKey !== '');
+        $publicKey = static::runCmd(['openssl', 'ec', '-pubout'], $privateKey);
 
         $this->keyId = Text::uuid();
         $this->privateKey = InMemory::plainText($privateKey);
 
-        $this->handler = HandlerStack::create(new MockHandler([new Response(200, [], $keyInfo['key'])]));
+        $this->handler = HandlerStack::create(new MockHandler([new Response(200, [], $publicKey)]));
         $this->handler->push(Middleware::history($this->history));
     }
 
+    /**
+     * Run a command.
+     *
+     * @param string[] $cmd Command to run.
+     * @param string $stdinData Data to provide to command stdin.
+     * @return string Data written to stdout.
+     */
+    protected static function runCmd(array $cmd, string $stdinData = ''): string
+    {
+        $proc = proc_open($cmd, [0 => ['pipe', 'r'], 1 => ['pipe', 'w'], 2 => ['pipe', 'w']], $pipes);
+        assert($proc !== false);
+
+        [$stdin, $stdout, $stderr] = $pipes;
+        fwrite($stdin, $stdinData);
+        fclose($stdin);
+
+        $stdoutData = stream_get_contents($stdout);
+        fclose($stdout);
+
+        $stderrData = stream_get_contents($stderr);
+        fclose($stderr);
+
+        $exitCode = proc_close($proc);
+        if ($exitCode !== 0) {
+            throw new \RuntimeException(sprintf('Process [%s] exited with %d: %s', implode($cmd), $exitCode, $stderrData));
+        }
+
+        return (string)$stdoutData;
+    }
+
     /**
      * @inheritDoc
      */
@@ -218,6 +236,43 @@ public function testAuthenticateMalformedToken(): void
         static::assertNull($authenticator->getPayload());
     }
 
+    /**
+     * Test authentication flow with a token that includes malformed JSON.
+     *
+     * @return void
+     */
+    public function testAuthenticateMalformedJsonInToken(): void
+    {
+        $authenticator = new AlbAuthenticator(
+            new CallbackIdentifier(['callback' => function (): void {
+                static::fail('Unexpected call to identifier');
+            }]),
+            ['region' => 'eu-south-1', 'guzzleClient' => ['handler' => $this->handler]]
+        );
+
+        $encoder = new JoseEncoder();
+        $token = sprintf(
+            '%s.%s',
+            $encoder->base64UrlEncode($encoder->jsonEncode(['typ' => 'JWT', 'alg' => 'ES256', 'kid' => $this->keyId])),
+            $encoder->base64UrlEncode('NOT A JSON'),
+        );
+        $token .= sprintf('.%s', $encoder->base64UrlEncode(Sha256::create()->sign($token, $this->privateKey)));
+
+        $result = $authenticator->authenticate(
+            new ServerRequest(['environment' => ['HTTP_X_AMZN_OIDC_DATA' => $token]])
+        );
+
+        static::assertSame(ResultInterface::FAILURE_CREDENTIALS_INVALID, $result->getStatus());
+        static::assertFalse($result->isValid());
+        $errors = $result->getErrors();
+        static::assertNotEmpty($errors);
+        static::assertSame('Error while decoding from JSON', $errors['message']);
+
+        static::assertCount(0, $this->history);
+
+        static::assertNull($authenticator->getPayload());
+    }
+
     /**
      * Test authentication flow with a token that has no `kid` header.
      *
@@ -358,18 +413,8 @@ public function testAuthenticateInvalidSignature(): void
         );
 
         // Generate another private key, which is different from the first.
-        $curves = openssl_get_curve_names();
-        assert(is_array($curves));
-        $key = openssl_pkey_new([
-            'digest_alg' => OPENSSL_ALGO_SHA256,
-            'private_key_type' => OPENSSL_KEYTYPE_EC,
-            'curve_name' => $curves[array_key_last($curves)],
-        ]);
-        assert($key !== false);
-        openssl_pkey_export($key, $privateKey);
-        if (is_resource($key)) {
-            openssl_free_key($key);
-        }
+        $privateKey = static::runCmd(['openssl', 'ecparam', '-name', 'prime256v1', '-genkey', '-noout']);
+        assert($privateKey !== '');
         $privateKey = InMemory::plainText($privateKey);
 
         $token = (new Builder(new JoseEncoder(), ChainedFormatter::default()))