From 2857ba1ccee9f07a2479f5359ec2f8aa1d6a2238 Mon Sep 17 00:00:00 2001 From: Tarvo R Date: Thu, 29 Jun 2023 15:37:26 +0300 Subject: [PATCH 01/12] Make audience signing optional --- README.md | 2 ++ src/CloudTasksQueue.php | 12 +++++++++--- src/Config.php | 12 ++++++++++++ src/OpenIdVerificatorConcrete.php | 2 +- src/OpenIdVerificatorFake.php | 2 +- 5 files changed, 25 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index dfa57b3..662fa1e 100644 --- a/README.md +++ b/README.md @@ -45,6 +45,7 @@ Please check the [Laravel support policy](https://laravel.com/docs/master/releas 'handler' => env('STACKKIT_CLOUD_TASKS_HANDLER', ''), 'queue' => env('STACKKIT_CLOUD_TASKS_QUEUE', 'default'), 'service_account_email' => env('STACKKIT_CLOUD_TASKS_SERVICE_EMAIL', ''), + 'signed_audience' => env('STACKKIT_CLOUD_TASKS_SIGNED_AUDIENCE', false), // Optional: The deadline in seconds for requests sent to the worker. If the worker // does not respond by this deadline then the request is cancelled and the attempt // is marked as a DEADLINE_EXCEEDED failure. @@ -70,6 +71,7 @@ Please check the table below on what the values mean and what their value should | `STACKKIT_CLOUD_TASKS_QUEUE` | The default queue a job will be added to |`emails` | `STACKKIT_CLOUD_TASKS_SERVICE_EMAIL` | The email address of the service account. Important, it should have the correct roles. See the section below which roles. |`my-service-account@appspot.gserviceaccount.com` | `STACKKIT_CLOUD_TASKS_HANDLER` (optional) | The URL that Cloud Tasks will call to process a job. This should be the URL to your Laravel app. By default we will use the URL that dispatched the job. |`https://.com` +| `STACKKIT_CLOUD_TASKS_SIGNED_AUDIENCE` (optional) | True or false depending if you want extra security by signing the audience of your tasks. May misbehave in certain Cloud Run setups. Defaults to false. | `false`
diff --git a/src/CloudTasksQueue.php b/src/CloudTasksQueue.php index dcb9e51..698ce2a 100644 --- a/src/CloudTasksQueue.php +++ b/src/CloudTasksQueue.php @@ -132,12 +132,13 @@ function ($payload, $queue, $delay) { */ protected function pushToCloudTasks($queue, $payload, $delay = 0) { + $handleTargetUrl = $this->getHandler(); $queue = $this->getQueue($queue); $queueName = $this->client->queueName($this->config['project'], $this->config['location'], $queue); $availableAt = $this->availableAt($delay); $httpRequest = $this->createHttpRequest(); - $httpRequest->setUrl($this->getHandler()); + $httpRequest->setUrl($handleTargetUrl); $httpRequest->setHttpMethod(HttpMethod::POST); $payload = json_decode($payload, true); @@ -167,14 +168,14 @@ protected function pushToCloudTasks($queue, $payload, $delay = 0) $token = new OidcToken; $token->setServiceAccountEmail($this->config['service_account_email']); - $token->setAudience(hash_hmac('sha256', $this->getHandler(), config('app.key'))); + $token->setAudience($this->getAudience()); $httpRequest->setOidcToken($token); if ($availableAt > time()) { $task->setScheduleTime(new Timestamp(['seconds' => $availableAt])); } - $createdTask = CloudTasksApi::createTask($queueName, $task); + CloudTasksApi::createTask($queueName, $task); event((new TaskCreated)->queue($queue)->task($task)); @@ -268,4 +269,9 @@ public function getHandler(): string { return Config::getHandler($this->config['handler']); } + + public function getAudience(): string + { + return Config::getAudience($this->config); + } } diff --git a/src/Config.php b/src/Config.php index c1183eb..be9abd2 100644 --- a/src/Config.php +++ b/src/Config.php @@ -82,4 +82,16 @@ public static function getHandler($handler): string ); } } + + /** + * @param string $handler + */ + public static function getAudience(array $config): string + { + $handler = self::getHandler($config['handler']); + + return $config['signed_audience'] ?? false + ? hash_hmac('sha256', $handler, config('app.key')) + : $handler; + } } diff --git a/src/OpenIdVerificatorConcrete.php b/src/OpenIdVerificatorConcrete.php index fa46175..a3c1c4a 100644 --- a/src/OpenIdVerificatorConcrete.php +++ b/src/OpenIdVerificatorConcrete.php @@ -18,7 +18,7 @@ public function verify(?string $token, array $config): void (new AccessToken())->verify( $token, [ - 'audience' => hash_hmac('sha256', app('queue')->getHandler(), config('app.key')), + 'audience' => Config::getAudience($config), 'throwException' => true, ] ); diff --git a/src/OpenIdVerificatorFake.php b/src/OpenIdVerificatorFake.php index 077f917..79cedb6 100644 --- a/src/OpenIdVerificatorFake.php +++ b/src/OpenIdVerificatorFake.php @@ -17,7 +17,7 @@ public function verify(?string $token, array $config): void (new AccessToken())->verify( $token, [ - 'audience' => hash_hmac('sha256', app('queue')->getHandler(), config('app.key')), + 'audience' => Config::getAudience($config), 'throwException' => true, 'certsLocation' => __DIR__ . '/../tests/Support/self-signed-public-key-as-jwk.json', ] From aa2a345560a162438fbc91ef4e896a324cf6f82d Mon Sep 17 00:00:00 2001 From: Tarvo R Date: Thu, 29 Jun 2023 16:04:06 +0300 Subject: [PATCH 02/12] Add additional reporting just in case --- src/TaskHandler.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/TaskHandler.php b/src/TaskHandler.php index 8be84c3..55639f3 100644 --- a/src/TaskHandler.php +++ b/src/TaskHandler.php @@ -86,6 +86,7 @@ private function captureTask($task): array try { $validator->validate(); } catch (ValidationException $e) { + report($e); if (config('app.debug')) { throw $e; } else { @@ -135,6 +136,7 @@ private function handleTask(array $task): void $apiTask = CloudTasksApi::getTask($fullTaskName); } catch (ApiException $e) { if (in_array($e->getStatus(), ['NOT_FOUND', 'PRECONDITION_FAILED'])) { + report($e); abort(404); } From e185f2c7f8f9a5449497c15de264953ca38482d4 Mon Sep 17 00:00:00 2001 From: Tarvo R Date: Thu, 29 Jun 2023 16:08:20 +0300 Subject: [PATCH 03/12] Theoretical fixes --- src/CloudTasksQueue.php | 2 +- src/Config.php | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/CloudTasksQueue.php b/src/CloudTasksQueue.php index 698ce2a..baab7fa 100644 --- a/src/CloudTasksQueue.php +++ b/src/CloudTasksQueue.php @@ -168,7 +168,7 @@ protected function pushToCloudTasks($queue, $payload, $delay = 0) $token = new OidcToken; $token->setServiceAccountEmail($this->config['service_account_email']); - $token->setAudience($this->getAudience()); + if ($audience = $this->getAudience()) $token->setAudience($audience); $httpRequest->setOidcToken($token); if ($availableAt > time()) { diff --git a/src/Config.php b/src/Config.php index be9abd2..5792122 100644 --- a/src/Config.php +++ b/src/Config.php @@ -88,10 +88,8 @@ public static function getHandler($handler): string */ public static function getAudience(array $config): string { - $handler = self::getHandler($config['handler']); - return $config['signed_audience'] ?? false - ? hash_hmac('sha256', $handler, config('app.key')) - : $handler; + ? hash_hmac('sha256', self::getHandler($config['handler']), config('app.key')) + : null; } } From 7db31743c8f0517fd9c0b5f0b5b53f7a4fa5e74a Mon Sep 17 00:00:00 2001 From: Tarvo R Date: Thu, 29 Jun 2023 16:24:20 +0300 Subject: [PATCH 04/12] Fix return value --- src/Config.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Config.php b/src/Config.php index 5792122..362aeec 100644 --- a/src/Config.php +++ b/src/Config.php @@ -86,7 +86,7 @@ public static function getHandler($handler): string /** * @param string $handler */ - public static function getAudience(array $config): string + public static function getAudience(array $config): ?string { return $config['signed_audience'] ?? false ? hash_hmac('sha256', self::getHandler($config['handler']), config('app.key')) From 226f9bddfbac7917c985aabde2fceb7fce1c3b54 Mon Sep 17 00:00:00 2001 From: Tarvo R Date: Thu, 29 Jun 2023 16:25:22 +0300 Subject: [PATCH 05/12] Fix docs --- src/Config.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Config.php b/src/Config.php index 362aeec..94f8515 100644 --- a/src/Config.php +++ b/src/Config.php @@ -84,7 +84,8 @@ public static function getHandler($handler): string } /** - * @param string $handler + * @param array $config + * @return string|null The audience as an hash or null if not needed */ public static function getAudience(array $config): ?string { From 9ab5a817c25932e148178e3e1c7bd89e473602ad Mon Sep 17 00:00:00 2001 From: Tarvo R Date: Thu, 29 Jun 2023 16:40:55 +0300 Subject: [PATCH 06/12] Fix type --- src/CloudTasksQueue.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/CloudTasksQueue.php b/src/CloudTasksQueue.php index baab7fa..9bd36c6 100644 --- a/src/CloudTasksQueue.php +++ b/src/CloudTasksQueue.php @@ -95,7 +95,7 @@ function ($payload, $queue) { */ public function pushRaw($payload, $queue = null, array $options = []) { - $delay = ! empty($options['delay']) ? $options['delay'] : 0; + $delay = !empty($options['delay']) ? $options['delay'] : 0; $this->pushToCloudTasks($queue, $payload, $delay); } @@ -270,7 +270,7 @@ public function getHandler(): string return Config::getHandler($this->config['handler']); } - public function getAudience(): string + public function getAudience(): ?string { return Config::getAudience($this->config); } From 9591b394166fe5c30906276876f96c716010f7d6 Mon Sep 17 00:00:00 2001 From: Tarvo R Date: Thu, 29 Jun 2023 18:29:22 +0300 Subject: [PATCH 07/12] Remove debug reports --- src/TaskHandler.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/TaskHandler.php b/src/TaskHandler.php index 55639f3..8be84c3 100644 --- a/src/TaskHandler.php +++ b/src/TaskHandler.php @@ -86,7 +86,6 @@ private function captureTask($task): array try { $validator->validate(); } catch (ValidationException $e) { - report($e); if (config('app.debug')) { throw $e; } else { @@ -136,7 +135,6 @@ private function handleTask(array $task): void $apiTask = CloudTasksApi::getTask($fullTaskName); } catch (ApiException $e) { if (in_array($e->getStatus(), ['NOT_FOUND', 'PRECONDITION_FAILED'])) { - report($e); abort(404); } From bce2a7c17715e0ef5ad01e412f6423ab413bf425 Mon Sep 17 00:00:00 2001 From: Tarvo R Date: Thu, 29 Jun 2023 18:30:23 +0300 Subject: [PATCH 08/12] Undo style change --- src/CloudTasksQueue.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/CloudTasksQueue.php b/src/CloudTasksQueue.php index 9bd36c6..2c58b8f 100644 --- a/src/CloudTasksQueue.php +++ b/src/CloudTasksQueue.php @@ -95,7 +95,7 @@ function ($payload, $queue) { */ public function pushRaw($payload, $queue = null, array $options = []) { - $delay = !empty($options['delay']) ? $options['delay'] : 0; + $delay = ! empty($options['delay']) ? $options['delay'] : 0; $this->pushToCloudTasks($queue, $payload, $delay); } From f356dc279c38e15af3f73df01d2a9d4b60215f00 Mon Sep 17 00:00:00 2001 From: Tarvo R Date: Thu, 29 Jun 2023 18:31:13 +0300 Subject: [PATCH 09/12] Undo introduced variable --- src/CloudTasksQueue.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/CloudTasksQueue.php b/src/CloudTasksQueue.php index 2c58b8f..51b882d 100644 --- a/src/CloudTasksQueue.php +++ b/src/CloudTasksQueue.php @@ -132,13 +132,12 @@ function ($payload, $queue, $delay) { */ protected function pushToCloudTasks($queue, $payload, $delay = 0) { - $handleTargetUrl = $this->getHandler(); $queue = $this->getQueue($queue); $queueName = $this->client->queueName($this->config['project'], $this->config['location'], $queue); $availableAt = $this->availableAt($delay); $httpRequest = $this->createHttpRequest(); - $httpRequest->setUrl($handleTargetUrl); + $httpRequest->setUrl($this->getHandler()); $httpRequest->setHttpMethod(HttpMethod::POST); $payload = json_decode($payload, true); From 964ddd4ed91bddf37f088739a7e6230502642c2e Mon Sep 17 00:00:00 2001 From: Tarvo R Date: Fri, 30 Jun 2023 16:51:32 +0300 Subject: [PATCH 10/12] Fix task name sanitization --- src/CloudTasksQueue.php | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/CloudTasksQueue.php b/src/CloudTasksQueue.php index 51b882d..2e90c09 100644 --- a/src/CloudTasksQueue.php +++ b/src/CloudTasksQueue.php @@ -192,7 +192,7 @@ private function withUuid(array $payload): array private function taskName(string $queueName, array $payload): string { - $displayName = str_replace("\\", '-', $payload['displayName']); + $displayName = $this->sanitizeTaskName($payload['displayName']); return CloudTasksClient::taskName( $this->config['project'], @@ -202,6 +202,17 @@ private function taskName(string $queueName, array $payload): string ); } + private function sanitizeTaskName(string $taskName) + { + // Remove all characters that are not -, letters, numbers, or whitespace + $sanitizedName = preg_replace('![^-\pL\pN\s]+!u', '-', $taskName); + + // Replace all separator characters and whitespace by a - + $sanitizedName = preg_replace('![-\s]+!u', '-', $sanitizedName); + + return trim($sanitizedName, '-'); + } + private function withAttempts(array $payload): array { if (!isset($payload['internal']['attempts'])) { From e24bbeecba100145bd2eb012690e4c00a7cad49f Mon Sep 17 00:00:00 2001 From: Marick van Tuil Date: Sun, 2 Jul 2023 12:37:16 +0200 Subject: [PATCH 11/12] Enable signed audience by default --- README.md | 2 +- src/Config.php | 2 +- tests/TestCase.php | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 662fa1e..b2a9b24 100644 --- a/README.md +++ b/README.md @@ -71,7 +71,7 @@ Please check the table below on what the values mean and what their value should | `STACKKIT_CLOUD_TASKS_QUEUE` | The default queue a job will be added to |`emails` | `STACKKIT_CLOUD_TASKS_SERVICE_EMAIL` | The email address of the service account. Important, it should have the correct roles. See the section below which roles. |`my-service-account@appspot.gserviceaccount.com` | `STACKKIT_CLOUD_TASKS_HANDLER` (optional) | The URL that Cloud Tasks will call to process a job. This should be the URL to your Laravel app. By default we will use the URL that dispatched the job. |`https://.com` -| `STACKKIT_CLOUD_TASKS_SIGNED_AUDIENCE` (optional) | True or false depending if you want extra security by signing the audience of your tasks. May misbehave in certain Cloud Run setups. Defaults to false. | `false` +| `STACKKIT_CLOUD_TASKS_SIGNED_AUDIENCE` (optional) | True or false depending if you want extra security by signing the audience of your tasks. May misbehave in certain Cloud Run setups. Defaults to true. | `true`
diff --git a/src/Config.php b/src/Config.php index 94f8515..9ffd34b 100644 --- a/src/Config.php +++ b/src/Config.php @@ -89,7 +89,7 @@ public static function getHandler($handler): string */ public static function getAudience(array $config): ?string { - return $config['signed_audience'] ?? false + return $config['signed_audience'] ?? true ? hash_hmac('sha256', self::getHandler($config['handler']), config('app.key')) : null; } diff --git a/tests/TestCase.php b/tests/TestCase.php index 587e802..bbdd1e1 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -103,6 +103,7 @@ protected function getEnvironmentSetUp($app) 'location' => 'europe-west6', 'handler' => env('CLOUD_TASKS_HANDLER', 'https://docker.for.mac.localhost:8080'), 'service_account_email' => 'info@stackkit.io', + 'signed_audience' => true, ]); $app['config']->set('queue.failed.driver', 'database-uuids'); $app['config']->set('queue.failed.database', 'testbench'); From aad2d5ddacef988bb01a439bfb0bd6455a83653e Mon Sep 17 00:00:00 2001 From: Marick van Tuil Date: Sun, 2 Jul 2023 12:44:18 +0200 Subject: [PATCH 12/12] Refactor --- README.md | 2 +- src/CloudTasksQueue.php | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index b2a9b24..23b072e 100644 --- a/README.md +++ b/README.md @@ -45,7 +45,7 @@ Please check the [Laravel support policy](https://laravel.com/docs/master/releas 'handler' => env('STACKKIT_CLOUD_TASKS_HANDLER', ''), 'queue' => env('STACKKIT_CLOUD_TASKS_QUEUE', 'default'), 'service_account_email' => env('STACKKIT_CLOUD_TASKS_SERVICE_EMAIL', ''), - 'signed_audience' => env('STACKKIT_CLOUD_TASKS_SIGNED_AUDIENCE', false), + 'signed_audience' => env('STACKKIT_CLOUD_TASKS_SIGNED_AUDIENCE', true), // Optional: The deadline in seconds for requests sent to the worker. If the worker // does not respond by this deadline then the request is cancelled and the attempt // is marked as a DEADLINE_EXCEEDED failure. diff --git a/src/CloudTasksQueue.php b/src/CloudTasksQueue.php index 2e90c09..d552461 100644 --- a/src/CloudTasksQueue.php +++ b/src/CloudTasksQueue.php @@ -167,7 +167,9 @@ protected function pushToCloudTasks($queue, $payload, $delay = 0) $token = new OidcToken; $token->setServiceAccountEmail($this->config['service_account_email']); - if ($audience = $this->getAudience()) $token->setAudience($audience); + if ($audience = $this->getAudience()) { + $token->setAudience($audience); + } $httpRequest->setOidcToken($token); if ($availableAt > time()) {