From f97331a494af176b2d889358b17df937f44b31b3 Mon Sep 17 00:00:00 2001 From: Ben Peachey Date: Fri, 30 Sep 2022 13:19:32 +0200 Subject: [PATCH 1/2] Change DPop::validateJwtDpop() to make ATH claim optional. --- src/Utils/DPop.php | 22 ++++++++++++---------- tests/unit/Utils/DPOPTest.php | 35 ++++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 11 deletions(-) diff --git a/src/Utils/DPop.php b/src/Utils/DPop.php index ffcc95e..51d6d76 100644 --- a/src/Utils/DPop.php +++ b/src/Utils/DPop.php @@ -157,6 +157,7 @@ public function makeJwkThumbprint($jwk) { * See also: https://datatracker.ietf.org/doc/html/draft-ietf-oauth-dpop#section-7 * * Validates the above part of the oauth dpop specification + * * @param string $jwt JWT access token, raw * @param string $dpop DPoP token, raw * @param ServerRequestInterface $request Server Request @@ -165,16 +166,17 @@ public function makeJwkThumbprint($jwk) { public function validateJwtDpop($jwt, $dpop, $request) { $this->validateDpop($dpop, $request); $jwtConfig = Configuration::forUnsecuredSigner(); - $dpopJWT = $jwtConfig->parser()->parse($dpop); - - $ath = $dpopJWT->claims()->get('ath'); - if ($ath === null) { - throw new InvalidTokenException('DPoP "ath" claim is missing'); - } - - $hash = hash('sha256', $jwt); - $encoded = Base64Url::encode($hash); - return ($ath === $encoded); + $jwtConfig->parser()->parse($dpop); + + /** + * @FIXME: ATH claim is not yet supported/required by the Solid OIDC specification. + * Once the Solid spec catches up to the DPOP spec, not having an ATH is incorrect. + * At that point, instead of returning "true", throw an exception: + * + * @see https://github.com/pdsinterop/php-solid-auth/issues/34 + */ + // throw new InvalidTokenException('DPoP "ath" claim is missing'); + return true; } /** diff --git a/tests/unit/Utils/DPOPTest.php b/tests/unit/Utils/DPOPTest.php index 132a7c0..b86515b 100644 --- a/tests/unit/Utils/DPOPTest.php +++ b/tests/unit/Utils/DPOPTest.php @@ -517,6 +517,36 @@ final public function testGetWebIdWithDpopWithoutSub(): void $dpop->getWebId($request); } + /** + * @testdox Dpop SHOULD not complain WHEN asked to get WebId from Request with valid DPOP without "ath" + * + * @covers ::getWebId + * + * @uses \Pdsinterop\Solid\Auth\Utils\DPop::getDpopKey + * @uses \Pdsinterop\Solid\Auth\Utils\DPop::validateDpop + * @uses \Pdsinterop\Solid\Auth\Utils\DPop::validateJwtDpop + */ + final public function testGetWebIdWithDpopWithoutOptionalAth(): void + { + unset($this->dpop['payload']['ath']); + $token = $this->sign($this->dpop); + + $mockJtiValidator = $this->createMockJtiValidator(); + $mockJtiValidator->expects($this->once()) + ->method('validate') + ->willReturn(true) + ; + $dpop = new DPop($mockJtiValidator); + + $request = new ServerRequest(array( + 'HTTP_AUTHORIZATION' => "dpop {$this->accessToken['token']}", + 'HTTP_DPOP' => $token['token'], + ),array(), $this->url); + + $webId = $dpop->getWebId($request); + + $this->assertEquals(self::MOCK_SUBJECT, $webId); + } /** * @testdox Dpop SHOULD complain WHEN asked to get WebId from Request with valid DPOP without "ath" * @@ -526,8 +556,11 @@ final public function testGetWebIdWithDpopWithoutSub(): void * @uses \Pdsinterop\Solid\Auth\Utils\DPop::validateDpop * @uses \Pdsinterop\Solid\Auth\Utils\DPop::validateJwtDpop */ - final public function testGetWebIdWithDpopWithoutAth(): void + final public function testGetWebIdWithDpopWithoutRequiredAth(): void { + /*/ @see https://github.com/pdsinterop/php-solid-auth/issues/34 /*/ + $this->markTestSkipped('ATH claim is not yet supported/required by the Solid OIDC specification.'); + unset($this->dpop['payload']['ath']); $token = $this->sign($this->dpop); From 7787c5ee5e6c2420b339b4df9cdcb17be12fad7d Mon Sep 17 00:00:00 2001 From: Ben Peachey Date: Fri, 30 Sep 2022 13:21:30 +0200 Subject: [PATCH 2/2] Fix issue in TokenGenerator caused by incorrect JWK thumbnail (JKT) being returned. --- src/TokenGenerator.php | 42 +++++++++++++-------- tests/unit/TokenGeneratorTest.php | 63 ++++++++++++++++++++++++------- 2 files changed, 75 insertions(+), 30 deletions(-) diff --git a/src/TokenGenerator.php b/src/TokenGenerator.php index 5d805ae..e11570a 100644 --- a/src/TokenGenerator.php +++ b/src/TokenGenerator.php @@ -2,9 +2,10 @@ namespace Pdsinterop\Solid\Auth; -use Pdsinterop\Solid\Auth\Utils\Jwks; +use Pdsinterop\Solid\Auth\Exception\InvalidTokenException; +use Pdsinterop\Solid\Auth\Utils\DPop; use Pdsinterop\Solid\Auth\Enum\OpenId\OpenIdConnectMetadata as OidcMeta; -use Laminas\Diactoros\Response\JsonResponse as JsonResponse; +use Laminas\Diactoros\Response\JsonResponse; use League\OAuth2\Server\CryptTrait; use DateTimeImmutable; @@ -21,14 +22,17 @@ class TokenGenerator public Config $config; private \DateInterval $validFor; + private DPop $dpopUtil; //////////////////////////////// PUBLIC API \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ final public function __construct( Config $config, - \DateInterval $validFor + \DateInterval $validFor, + DPop $dpopUtil, ) { $this->config = $config; + $this->dpopUtil = $dpopUtil; $this->validFor = $validFor; $this->setEncryptionKey($this->config->getKeys()->getEncryptionKey()); @@ -48,10 +52,9 @@ public function generateRegistrationAccessToken($clientId, $privateKey) { return $token->toString(); } - public function generateIdToken($accessToken, $clientId, $subject, $nonce, $privateKey, $dpopKey, $now=null) { + public function generateIdToken($accessToken, $clientId, $subject, $nonce, $privateKey, $dpop, $now=null) { $issuer = $this->config->getServer()->get(OidcMeta::ISSUER); - $jwks = $this->getJwks(); $tokenHash = $this->generateTokenHash($accessToken); // Create JWT @@ -61,6 +64,8 @@ public function generateIdToken($accessToken, $clientId, $subject, $nonce, $priv $expire = $now->add($this->validFor); + $jkt = $this->makeJwkThumbprint($dpop); + $token = $jwtConfig->builder() ->issuedBy($issuer) ->permittedFor($clientId) @@ -74,10 +79,8 @@ public function generateIdToken($accessToken, $clientId, $subject, $nonce, $priv ->withClaim("at_hash", $tokenHash) //FIXME: at_hash should only be added if the response_type is a token ->withClaim("c_hash", $tokenHash) // FIXME: c_hash should only be added if the response_type is a code ->withClaim("cnf", array( - "jkt" => $dpopKey, - // "jwk" => $jwks['keys'][0] + "jkt" => $jkt, )) - ->withHeader('kid', $jwks['keys'][0]['kid']) ->getToken($jwtConfig->signer(), $jwtConfig->signingKey()); return $token->toString(); } @@ -104,7 +107,7 @@ public function respondToRegistration($registration, $privateKey) { return array_merge($registrationBase, $registration); } - public function addIdTokenToResponse($response, $clientId, $subject, $nonce, $privateKey, $dpopKey=null) { + public function addIdTokenToResponse($response, $clientId, $subject, $nonce, $privateKey, $dpop=null) { if ($response->hasHeader("Location")) { $value = $response->getHeaderLine("Location"); @@ -115,7 +118,7 @@ public function addIdTokenToResponse($response, $clientId, $subject, $nonce, $pr $subject, $nonce, $privateKey, - $dpopKey + $dpop ); $value = preg_replace("/#access_token=(.*?)&/", "#access_token=\$1&id_token=$idToken&", $value); $response = $response->withHeader("Location", $value); @@ -126,7 +129,7 @@ public function addIdTokenToResponse($response, $clientId, $subject, $nonce, $pr $subject, $nonce, $privateKey, - $dpopKey + $dpop ); $value = preg_replace("/code=(.*?)&/", "code=\$1&id_token=$idToken&", $value); $response = $response->withHeader("Location", $value); @@ -143,7 +146,7 @@ public function addIdTokenToResponse($response, $clientId, $subject, $nonce, $pr $subject, $nonce, $privateKey, - $dpopKey + $dpop ); $body['access_token'] = $body['id_token']; @@ -178,9 +181,16 @@ private function generateTokenHash($accessToken) { return $atHash; } - private function getJwks() { - $key = $this->config->getKeys()->getPublicKey(); - $jwks = new Jwks($key); - return json_decode($jwks->__toString(), true); + private function makeJwkThumbprint($dpop): string + { + $dpopConfig = Configuration::forUnsecuredSigner(); + $parsedDpop = $dpopConfig->parser()->parse($dpop); + $jwk = $parsedDpop->headers()->get("jwk"); + + if (empty($jwk)) { + throw new InvalidTokenException('Required JWK header missing in DPOP'); + } + + return $this->dpopUtil->makeJwkThumbprint($jwk); } } diff --git a/tests/unit/TokenGeneratorTest.php b/tests/unit/TokenGeneratorTest.php index 0d5a7cc..3afaa26 100644 --- a/tests/unit/TokenGeneratorTest.php +++ b/tests/unit/TokenGeneratorTest.php @@ -6,6 +6,7 @@ use Pdsinterop\Solid\Auth\Config\ServerInterface; use Pdsinterop\Solid\Auth\Enum\OpenId\OpenIdConnectMetadata as OidcMeta; use Pdsinterop\Solid\Auth\Utils\Base64Url; +use Pdsinterop\Solid\Auth\Utils\DPop; use PHPUnit\Framework\MockObject\MockObject; function time() { return 1234;} @@ -21,10 +22,13 @@ class TokenGeneratorTest extends AbstractTestCase { ////////////////////////////////// FIXTURES \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ + const MOCK_DPOP = "mock.dpop.value"; + const MOCK_JKT = 'mock jkt'; + private MockObject|Config $mockConfig; private MockObject|KeysInterface $mockKeys; - private function createTokenGenerator($interval = null): TokenGenerator + private function createTokenGenerator($interval = null, $jkt = null): TokenGenerator { $this->mockConfig = $this->getMockBuilder(Config::class) ->disableOriginalConstructor() @@ -51,7 +55,19 @@ private function createTokenGenerator($interval = null): TokenGenerator ->willReturn('mock encryption key') ; - return new TokenGenerator($this->mockConfig, $interval??$mockInterval); + $mockDpopUtil = $this->getMockBuilder(DPop::class) + ->disableOriginalConstructor() + ->getMock() + ; + + if ($jkt) { + $mockDpopUtil->expects($this->once()) + ->method('makeJwkThumbprint') + ->willReturn($jkt) + ; + } + + return new TokenGenerator($this->mockConfig, $interval??$mockInterval, $mockDpopUtil); } /////////////////////////////////// TESTS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ @@ -84,6 +100,27 @@ final public function testInstantiateWithoutValidFor(): void new TokenGenerator($mockConfig); } + /** + * @testdox Token Generator SHOULD complain WHEN instantiated without Dpop Utility + * + * @coversNothing + */ + final public function testInstantiateWithoutDpopUtility(): void + { + $this->expectArgumentCountError(3); + + $mockConfig = $this->getMockBuilder(Config::class) + ->disableOriginalConstructor() + ->getMock(); + + $mockInterval = $this->getMockBuilder(\DateInterval::class) + ->disableOriginalConstructor() + ->getMock() + ; + + new TokenGenerator($mockConfig, $mockInterval); + } + /** * @testdox Token Generator SHOULD be created WHEN instantiated with Config and validity period * @@ -271,14 +308,7 @@ final public function testIdTokenGeneration(): void { $validFor = new \DateInterval('PT1S'); - $tokenGenerator = $this->createTokenGenerator($validFor); - - $mockKey = \Lcobucci\JWT\Signer\Key\InMemory::file(__DIR__.'/../fixtures/keys/public.key'); - - $this->mockKeys->expects($this->once()) - ->method('getPublicKey') - ->willReturn($mockKey) - ; + $tokenGenerator = $this->createTokenGenerator($validFor, self::MOCK_JKT); $mockServer = $this->getMockBuilder(ServerInterface::class) ->disableOriginalConstructor() @@ -300,26 +330,31 @@ final public function testIdTokenGeneration(): void $now = new \DateTimeImmutable('1234-01-01 12:34:56.789'); + $encodedDpop = vsprintf("%s.%s.%s", [ + 'header' => Base64Url::encode('{"jwk":"mock jwk"}'), + 'body' => Base64Url::encode('{}'), + 'signature' => Base64Url::encode('mock signature') + ]); + $actual = $tokenGenerator->generateIdToken( 'mock access token', 'mock clientId', 'mock subject', 'mock nonce', $privateKey, - 'mock dpop', + $encodedDpop, $now ); $this->assertJwtEquals([[ "alg"=>"RS256", - "kid"=>"0c3932ca20f3a00ad2eb72035f6cc9cb", "typ"=>"JWT", ],[ + 'at_hash' => '1EZBnvsFWlK8ESkgHQsrIQ', 'aud' => 'mock clientId', 'azp' => 'mock clientId', 'c_hash' => '1EZBnvsFWlK8ESkgHQsrIQ', - 'at_hash' => '1EZBnvsFWlK8ESkgHQsrIQ', - 'cnf' => ["jkt" => "mock dpop"], + 'cnf' => ["jkt" => self::MOCK_JKT], 'exp' => -23225829903.789, 'iat' => -23225829904.789, 'iss' => 'mock issuer',