From c6d26c0c5c7c9b3530b4e9a56534e67aa00f07a2 Mon Sep 17 00:00:00 2001 From: Marick van Tuil Date: Sun, 6 Dec 2020 16:03:54 +0100 Subject: [PATCH 01/11] Add retry if signature verification fails --- src/OpenIdVerificator.php | 5 +++++ src/TaskHandler.php | 24 ++++++++++++++++++++++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/OpenIdVerificator.php b/src/OpenIdVerificator.php index 0a818bb..062d0d1 100644 --- a/src/OpenIdVerificator.php +++ b/src/OpenIdVerificator.php @@ -70,4 +70,9 @@ public function isCached() { return Cache::has(self::V3_CERTS); } + + public function forgetFromCache() + { + Cache::forget(self::V3_CERTS); + } } diff --git a/src/TaskHandler.php b/src/TaskHandler.php index 09341df..71d7903 100644 --- a/src/TaskHandler.php +++ b/src/TaskHandler.php @@ -2,11 +2,13 @@ namespace Stackkit\LaravelGoogleCloudTasksQueue; +use Firebase\JWT\SignatureInvalidException; use Google\Cloud\Tasks\V2\CloudTasksClient; use Illuminate\Http\Request; use Illuminate\Queue\Worker; use Illuminate\Queue\WorkerOptions; use Firebase\JWT\JWT; +use Illuminate\Support\Facades\Cache; class TaskHandler { @@ -47,13 +49,31 @@ public function authorizeRequest() $openIdToken = $this->request->bearerToken(); $kid = $this->publicKey->getKidFromOpenIdToken($openIdToken); - $publicKey = $this->publicKey->getPublicKey($kid); - $decodedToken = $this->jwt->decode($openIdToken, $publicKey, ['RS256']); + $decodedToken = $this->decodeOpenIdToken($openIdToken, $kid); $this->validateToken($decodedToken); } + private function decodeOpenIdToken($openIdToken, $kid, $cache = true) + { + if (!$cache) { + $this->publicKey->forgetFromCache(); + } + + $publicKey = $this->publicKey->getPublicKey($kid); + + try { + return $this->jwt->decode($openIdToken, $publicKey, ['RS256']); + } catch (SignatureInvalidException $e) { + if (!$cache) { + throw $e; + } + + return $this->decodeOpenIdToken($openIdToken, $kid, false); + } + } + /** * https://developers.google.com/identity/protocols/oauth2/openid-connect#validatinganidtoken * From 44b636ca76e9e5a5574a6bb52f597e6b16902820 Mon Sep 17 00:00:00 2001 From: Marick van Tuil Date: Sun, 6 Dec 2020 16:22:13 +0100 Subject: [PATCH 02/11] Cache certificate as per cache control headers --- src/OpenIdVerificator.php | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/OpenIdVerificator.php b/src/OpenIdVerificator.php index 062d0d1..3aa0463 100644 --- a/src/OpenIdVerificator.php +++ b/src/OpenIdVerificator.php @@ -17,6 +17,7 @@ class OpenIdVerificator private $guzzle; private $rsa; + private $maxAge = []; public function __construct(Client $guzzle, RSA $rsa) { @@ -26,9 +27,12 @@ public function __construct(Client $guzzle, RSA $rsa) public function getPublicKey($kid = null) { - $v3Certs = Cache::rememberForever(self::V3_CERTS, function () { - return $this->getv3Certs(); - }); + if (Cache::has(self::V3_CERTS)) { + $v3Certs = Cache::get(self::V3_CERTS); + } else { + $v3Certs = $this->getv3Certs(); + Cache::put(self::V3_CERTS, $v3Certs, $this->maxAge[self::URL_OPENID_CONFIG]); + } $cert = $kid ? collect($v3Certs)->firstWhere('kid', '=', $kid) : $v3Certs[0]; @@ -63,6 +67,14 @@ private function callApiAndReturnValue($url, $value) $data = json_decode($response->getBody(), true); + $maxAge = 0; + foreach ($response->getHeader('Cache-Control') as $line) { + preg_match('/max-age=(\d+)/', $line, $matches); + $maxAge = isset($matches[1]) ? (int) $matches[1] : 0; + } + + $this->maxAge[$url] = $maxAge; + return Arr::get($data, $value); } From 2ad69341722a21de9572c5f01e9bb7423351ee94 Mon Sep 17 00:00:00 2001 From: Marick van Tuil Date: Sun, 6 Dec 2020 17:40:38 +0100 Subject: [PATCH 03/11] Add test to assert key is cached --- .gitignore | 1 + src/OpenIdVerificator.php | 24 ++++++++++++++++++++- src/TaskHandler.php | 23 +------------------- tests/GooglePublicKeyTest.php | 40 ++++++++++++++++++++++++++++++++++- 4 files changed, 64 insertions(+), 24 deletions(-) diff --git a/.gitignore b/.gitignore index 2234772..087ee96 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ /vendor/ composer.lock .idea/ +.phpunit.result.cache \ No newline at end of file diff --git a/src/OpenIdVerificator.php b/src/OpenIdVerificator.php index 3aa0463..3d228fa 100644 --- a/src/OpenIdVerificator.php +++ b/src/OpenIdVerificator.php @@ -3,6 +3,7 @@ namespace Stackkit\LaravelGoogleCloudTasksQueue; use Firebase\JWT\JWT; +use Firebase\JWT\SignatureInvalidException; use GuzzleHttp\Client; use Illuminate\Support\Arr; use Illuminate\Support\Facades\Cache; @@ -17,12 +18,33 @@ class OpenIdVerificator private $guzzle; private $rsa; + private $jwt; private $maxAge = []; - public function __construct(Client $guzzle, RSA $rsa) + public function __construct(Client $guzzle, RSA $rsa, JWT $jwt) { $this->guzzle = $guzzle; $this->rsa = $rsa; + $this->jwt = $jwt; + } + + public function decodeOpenIdToken($openIdToken, $kid, $cache = true) + { + if (!$cache) { + $this->forgetFromCache(); + } + + $publicKey = $this->getPublicKey($kid); + + try { + return $this->jwt->decode($openIdToken, $publicKey, ['RS256']); + } catch (SignatureInvalidException $e) { + if (!$cache) { + throw $e; + } + + return $this->decodeOpenIdToken($openIdToken, $kid, false); + } } public function getPublicKey($kid = null) diff --git a/src/TaskHandler.php b/src/TaskHandler.php index 71d7903..a1eaa85 100644 --- a/src/TaskHandler.php +++ b/src/TaskHandler.php @@ -2,13 +2,11 @@ namespace Stackkit\LaravelGoogleCloudTasksQueue; -use Firebase\JWT\SignatureInvalidException; use Google\Cloud\Tasks\V2\CloudTasksClient; use Illuminate\Http\Request; use Illuminate\Queue\Worker; use Illuminate\Queue\WorkerOptions; use Firebase\JWT\JWT; -use Illuminate\Support\Facades\Cache; class TaskHandler { @@ -50,30 +48,11 @@ public function authorizeRequest() $openIdToken = $this->request->bearerToken(); $kid = $this->publicKey->getKidFromOpenIdToken($openIdToken); - $decodedToken = $this->decodeOpenIdToken($openIdToken, $kid); + $decodedToken = $this->publicKey->decodeOpenIdToken($openIdToken, $kid); $this->validateToken($decodedToken); } - private function decodeOpenIdToken($openIdToken, $kid, $cache = true) - { - if (!$cache) { - $this->publicKey->forgetFromCache(); - } - - $publicKey = $this->publicKey->getPublicKey($kid); - - try { - return $this->jwt->decode($openIdToken, $publicKey, ['RS256']); - } catch (SignatureInvalidException $e) { - if (!$cache) { - throw $e; - } - - return $this->decodeOpenIdToken($openIdToken, $kid, false); - } - } - /** * https://developers.google.com/identity/protocols/oauth2/openid-connect#validatinganidtoken * diff --git a/tests/GooglePublicKeyTest.php b/tests/GooglePublicKeyTest.php index f219375..a4cf01c 100644 --- a/tests/GooglePublicKeyTest.php +++ b/tests/GooglePublicKeyTest.php @@ -2,8 +2,14 @@ namespace Tests; +use Carbon\Carbon; +use Firebase\JWT\JWT; use GuzzleHttp\Client; +use Illuminate\Cache\Events\CacheHit; +use Illuminate\Cache\Events\CacheMissed; +use Illuminate\Cache\Events\KeyWritten; use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\Event; use Mockery; use phpseclib\Crypt\RSA; use Stackkit\LaravelGoogleCloudTasksQueue\OpenIdVerificator; @@ -26,7 +32,7 @@ protected function setUp(): void $this->guzzle = Mockery::mock(new Client()); - $this->publicKey = new OpenIdVerificator($this->guzzle, new RSA()); + $this->publicKey = new OpenIdVerificator($this->guzzle, new RSA(), new JWT()); } /** @test */ @@ -48,10 +54,42 @@ public function it_caches_the_gcloud_public_key() /** @test */ public function it_will_return_the_cached_gcloud_public_key() { + Event::fake(); + $this->publicKey->getPublicKey(); + Event::assertDispatched(CacheMissed::class); + Event::assertDispatched(KeyWritten::class); + $this->publicKey->getPublicKey(); + Event::assertDispatched(CacheHit::class); + $this->guzzle->shouldHaveReceived('get')->twice(); } + + /** @test */ + public function public_key_is_cached_according_to_cache_control_headers() + { + Event::fake(); + + $this->publicKey->getPublicKey(); + Event::assertDispatched(CacheMissed::class); + Event::assertDispatched(KeyWritten::class); + Event::fake(); // reset + + $this->publicKey->getPublicKey(); + Event::assertDispatched(CacheHit::class); + Event::fake(); // reset + + Carbon::setTestNow(Carbon::now()->addSeconds(3600)); + $this->publicKey->getPublicKey(); + Event::assertDispatched(CacheHit::class); + Event::fake(); // reset + + Carbon::setTestNow(Carbon::now()->addSeconds(1)); + $this->publicKey->getPublicKey(); + Event::assertDispatched(CacheMissed::class); + Event::assertDispatched(KeyWritten::class); + } } From 801f044d3a23e137fa1b659739d1437df114054c Mon Sep 17 00:00:00 2001 From: Marick van Tuil Date: Sun, 6 Dec 2020 18:15:10 +0100 Subject: [PATCH 04/11] Fix assertion to support Laravel 5 --- tests/GooglePublicKeyTest.php | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/tests/GooglePublicKeyTest.php b/tests/GooglePublicKeyTest.php index a4cf01c..cc30c84 100644 --- a/tests/GooglePublicKeyTest.php +++ b/tests/GooglePublicKeyTest.php @@ -74,22 +74,17 @@ public function public_key_is_cached_according_to_cache_control_headers() Event::fake(); $this->publicKey->getPublicKey(); - Event::assertDispatched(CacheMissed::class); - Event::assertDispatched(KeyWritten::class); - Event::fake(); // reset $this->publicKey->getPublicKey(); - Event::assertDispatched(CacheHit::class); - Event::fake(); // reset Carbon::setTestNow(Carbon::now()->addSeconds(3600)); $this->publicKey->getPublicKey(); - Event::assertDispatched(CacheHit::class); - Event::fake(); // reset Carbon::setTestNow(Carbon::now()->addSeconds(1)); $this->publicKey->getPublicKey(); - Event::assertDispatched(CacheMissed::class); - Event::assertDispatched(KeyWritten::class); + + Event::assertDispatched(CacheMissed::class, 2); + Event::assertDispatched(KeyWritten::class, 2); + } } From 31a824f4fe46dc1300d9981650c614ee6652dd4a Mon Sep 17 00:00:00 2001 From: Marick van Tuil Date: Sun, 6 Dec 2020 18:17:57 +0100 Subject: [PATCH 05/11] Increase wait time --- tests/GooglePublicKeyTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/GooglePublicKeyTest.php b/tests/GooglePublicKeyTest.php index cc30c84..07a8da6 100644 --- a/tests/GooglePublicKeyTest.php +++ b/tests/GooglePublicKeyTest.php @@ -80,7 +80,7 @@ public function public_key_is_cached_according_to_cache_control_headers() Carbon::setTestNow(Carbon::now()->addSeconds(3600)); $this->publicKey->getPublicKey(); - Carbon::setTestNow(Carbon::now()->addSeconds(1)); + Carbon::setTestNow(Carbon::now()->addSeconds(5)); $this->publicKey->getPublicKey(); Event::assertDispatched(CacheMissed::class, 2); From 42bb67857e20eb1e225d61e5f9c223a5d6e71036 Mon Sep 17 00:00:00 2001 From: Marick van Tuil Date: Sun, 6 Dec 2020 19:10:16 +0100 Subject: [PATCH 06/11] Fix for Laravel 5.6 --- src/OpenIdVerificator.php | 3 ++- tests/TestCase.php | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/OpenIdVerificator.php b/src/OpenIdVerificator.php index 3d228fa..f0ca933 100644 --- a/src/OpenIdVerificator.php +++ b/src/OpenIdVerificator.php @@ -2,6 +2,7 @@ namespace Stackkit\LaravelGoogleCloudTasksQueue; +use Carbon\Carbon; use Firebase\JWT\JWT; use Firebase\JWT\SignatureInvalidException; use GuzzleHttp\Client; @@ -53,7 +54,7 @@ public function getPublicKey($kid = null) $v3Certs = Cache::get(self::V3_CERTS); } else { $v3Certs = $this->getv3Certs(); - Cache::put(self::V3_CERTS, $v3Certs, $this->maxAge[self::URL_OPENID_CONFIG]); + Cache::put(self::V3_CERTS, $v3Certs, Carbon::now()->addSeconds($this->maxAge[self::URL_OPENID_CONFIG])); } $cert = $kid ? collect($v3Certs)->firstWhere('kid', '=', $kid) : $v3Certs[0]; diff --git a/tests/TestCase.php b/tests/TestCase.php index 8556126..5cc8041 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -2,6 +2,8 @@ namespace Tests; +use Illuminate\Support\Facades\Artisan; + class TestCase extends \Orchestra\Testbench\TestCase { /** @@ -29,6 +31,11 @@ protected function getPackageProviders($app) */ protected function getEnvironmentSetUp($app) { + foreach (glob(storage_path('framework/cache/data/*/*/*')) as $file) { + unlink($file); + } + + $app['config']->set('cache.default', 'file'); $app['config']->set('queue.default', 'cloudtasks'); $app['config']->set('queue.connections.cloudtasks', [ 'driver' => 'cloudtasks', From 830e0c3a05cffecdba8a6dfe426d84076193ac8e Mon Sep 17 00:00:00 2001 From: Marick van Tuil Date: Sun, 6 Dec 2020 19:12:10 +0100 Subject: [PATCH 07/11] Add test to assert fresh cert is fetched after signature failure --- tests/TaskHandlerTest.php | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/tests/TaskHandlerTest.php b/tests/TaskHandlerTest.php index edfae4d..2b46931 100644 --- a/tests/TaskHandlerTest.php +++ b/tests/TaskHandlerTest.php @@ -3,12 +3,13 @@ namespace Tests; use Firebase\JWT\JWT; +use Firebase\JWT\SignatureInvalidException; use Google\Cloud\Tasks\V2\CloudTasksClient; -use GuzzleHttp\Client; -use Illuminate\Support\Facades\Cache; +use Illuminate\Cache\Events\CacheHit; +use Illuminate\Cache\Events\KeyWritten; +use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Mail; use Mockery; -use phpseclib\Crypt\RSA; use Stackkit\LaravelGoogleCloudTasksQueue\CloudTasksException; use Stackkit\LaravelGoogleCloudTasksQueue\OpenIdVerificator; use Stackkit\LaravelGoogleCloudTasksQueue\TaskHandler; @@ -116,6 +117,21 @@ public function it_will_validate_the_token_expiration() $this->handler->handle($this->simpleJob()); } + /** @test */ + public function in_case_of_signature_verification_failure_it_will_retry() + { + Event::fake(); + + $this->jwt->shouldReceive('decode')->andThrow(SignatureInvalidException::class); + + $this->expectException(SignatureInvalidException::class); + + $this->handler->handle($this->simpleJob()); + + Event::assertDispatched(CacheHit::class); + Event::assertDispatched(KeyWritten::class); + } + /** @test */ public function it_runs_the_incoming_job() { From f4421b5a3cb33b08ecb063cee0ad3ef8560ce013 Mon Sep 17 00:00:00 2001 From: Marick van Tuil Date: Sun, 6 Dec 2020 19:14:29 +0100 Subject: [PATCH 08/11] Rename method to be more clear --- src/OpenIdVerificator.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/OpenIdVerificator.php b/src/OpenIdVerificator.php index f0ca933..23e0f16 100644 --- a/src/OpenIdVerificator.php +++ b/src/OpenIdVerificator.php @@ -53,7 +53,7 @@ public function getPublicKey($kid = null) if (Cache::has(self::V3_CERTS)) { $v3Certs = Cache::get(self::V3_CERTS); } else { - $v3Certs = $this->getv3Certs(); + $v3Certs = $this->getFreshCertificates(); Cache::put(self::V3_CERTS, $v3Certs, Carbon::now()->addSeconds($this->maxAge[self::URL_OPENID_CONFIG])); } @@ -62,7 +62,7 @@ public function getPublicKey($kid = null) return $this->extractPublicKeyFromCertificate($cert); } - private function getv3Certs() + private function getFreshCertificates() { $jwksUri = $this->callApiAndReturnValue(self::URL_OPENID_CONFIG, 'jwks_uri'); From 28de875e3ec860ca66d45fa9ed76376e0de25a40 Mon Sep 17 00:00:00 2001 From: Marick van Tuil Date: Sun, 6 Dec 2020 19:18:36 +0100 Subject: [PATCH 09/11] Add newline to gitignore --- .gitignore | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 087ee96..beb6383 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,4 @@ /vendor/ composer.lock .idea/ -.phpunit.result.cache \ No newline at end of file +.phpunit.result.cache From c4185f39b1c5de15fbdc05c825046dcee419a354 Mon Sep 17 00:00:00 2001 From: Marick van Tuil Date: Sun, 6 Dec 2020 19:21:24 +0100 Subject: [PATCH 10/11] Cleanup unused code --- src/CloudTasksQueue.php | 1 - src/Errors.php | 10 ---------- 2 files changed, 11 deletions(-) diff --git a/src/CloudTasksQueue.php b/src/CloudTasksQueue.php index 13901f3..7c892a4 100644 --- a/src/CloudTasksQueue.php +++ b/src/CloudTasksQueue.php @@ -11,7 +11,6 @@ use Illuminate\Contracts\Queue\Queue as QueueContract; use Illuminate\Queue\Queue as LaravelQueue; use Illuminate\Support\InteractsWithTime; -use Illuminate\Support\Str; class CloudTasksQueue extends LaravelQueue implements QueueContract { diff --git a/src/Errors.php b/src/Errors.php index f6554d6..c34a345 100644 --- a/src/Errors.php +++ b/src/Errors.php @@ -4,16 +4,6 @@ class Errors { - public static function invalidCredentials() - { - return 'Google Cloud credentials not provided. To fix this, in config/queue.php, connections.cloudtasks.credentials, provide the path to your credentials JSON file'; - } - - public static function credentialsFileDoesNotExist() - { - return 'Google Cloud credentials JSON file does not exist'; - } - public static function invalidProject() { return 'Google Cloud project not provided. To fix this, set the STACKKIT_CLOUD_TASKS_PROJECT environment variable'; From 21d0db2d216ab37049684ec2dcc3b61f4322a559 Mon Sep 17 00:00:00 2001 From: Marick van Tuil Date: Sun, 6 Dec 2020 21:52:45 +0100 Subject: [PATCH 11/11] Update CHANGELOG.md --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 88b2546..32ee90f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). +## 2.0.1 - 2020-12-06 + +**Fixed** + +- Fixed certificates cached too long ([#3](https://github.com/stackkit/laravel-google-cloud-tasks-queue/issues/3)) + ## 2.0.0 - 2020-10-11 **Added**