From 17109f8eb5827066d4153572353c4ceee39ff7ec Mon Sep 17 00:00:00 2001 From: ErickSkrauch Date: Fri, 14 Jun 2024 03:21:00 +0200 Subject: [PATCH] Remove unused HS256 signing algorithm --- .env.dist | 1 - api/components/Tokens/AlgorithmsManager.php | 5 +++++ api/components/Tokens/Component.php | 11 ---------- api/config/config-test.php | 1 - api/config/config.php | 1 - api/tests/functional/accounts/GetCest.php | 21 ++++++++++++++++++- .../unit/components/Tokens/ComponentTest.php | 8 ------- 7 files changed, 25 insertions(+), 23 deletions(-) diff --git a/.env.dist b/.env.dist index 6ec156a..86da162 100644 --- a/.env.dist +++ b/.env.dist @@ -7,7 +7,6 @@ AUTHSERVER_HOST=authserver.ely.by EMAILS_RENDERER_HOST=http://emails-renderer:3000 ## Security params -JWT_USER_SECRET=replace_me_for_production JWT_ENCRYPTION_KEY=thisisadummyvalue32latterslength JWT_PRIVATE_KEY_PATH= JWT_PRIVATE_KEY_PASS= diff --git a/api/components/Tokens/AlgorithmsManager.php b/api/components/Tokens/AlgorithmsManager.php index 7d1bb3c..9746484 100644 --- a/api/components/Tokens/AlgorithmsManager.php +++ b/api/components/Tokens/AlgorithmsManager.php @@ -6,6 +6,11 @@ namespace api\components\Tokens; use api\components\Tokens\Algorithms\AlgorithmInterface; use Webmozart\Assert\Assert; +/** + * This class is used to hold multiple keys signing mechanisms. + * This may be useful when we change the key signing algorithm to allow during the transition period + * the keys with both algorithms to work simultaneously. + */ final class AlgorithmsManager { /** diff --git a/api/components/Tokens/Component.php b/api/components/Tokens/Component.php index 56795c5..c617eaa 100644 --- a/api/components/Tokens/Component.php +++ b/api/components/Tokens/Component.php @@ -16,15 +16,6 @@ class Component extends BaseComponent { private const PREFERRED_ALGORITHM = 'ES256'; - /** - * @var string - * @deprecated In earlier versions of the application, JWT were signed by a synchronous encryption algorithm. - * Now asynchronous encryption is used instead, and this logic is saved for a transitional period. - * I think it can be safely removed, but I'll not do it yet, because at the time of writing the comment - * there were enough changes in the code already. - */ - public $hmacKey; - /** * @var string */ @@ -44,7 +35,6 @@ class Component extends BaseComponent { public function init(): void { parent::init(); - Assert::notEmpty($this->hmacKey, 'hmacKey must be set'); Assert::notEmpty($this->privateKeyPath, 'privateKeyPath must be set'); Assert::notEmpty($this->encryptionKey, 'encryptionKey must be set'); } @@ -121,7 +111,6 @@ class Component extends BaseComponent { private function getAlgorithmManager(): AlgorithmsManager { if ($this->algorithmManager === null) { $this->algorithmManager = new AlgorithmsManager([ - new Algorithms\HS256($this->hmacKey), new Algorithms\ES256("file://{$this->privateKeyPath}", $this->privateKeyPass), ]); } diff --git a/api/config/config-test.php b/api/config/config-test.php index 119d03b..b3ea410 100644 --- a/api/config/config-test.php +++ b/api/config/config-test.php @@ -2,7 +2,6 @@ return [ 'components' => [ 'tokens' => [ - 'hmacKey' => 'tests-secret-key', 'privateKeyPath' => codecept_data_dir('certs/private.pem'), 'privateKeyPass' => null, 'encryptionKey' => 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', diff --git a/api/config/config.php b/api/config/config.php index d9c239a..77b138d 100644 --- a/api/config/config.php +++ b/api/config/config.php @@ -31,7 +31,6 @@ return [ ], 'tokens' => [ 'class' => api\components\Tokens\Component::class, - 'hmacKey' => getenv('JWT_USER_SECRET'), 'privateKeyPath' => getenv('JWT_PRIVATE_KEY_PATH') ?: __DIR__ . '/../../data/certs/private.pem', 'privateKeyPass' => getenv('JWT_PRIVATE_KEY_PASS') ?: null, 'encryptionKey' => getenv('JWT_ENCRYPTION_KEY'), diff --git a/api/tests/functional/accounts/GetCest.php b/api/tests/functional/accounts/GetCest.php index 31d9ad6..2dc3cbb 100644 --- a/api/tests/functional/accounts/GetCest.php +++ b/api/tests/functional/accounts/GetCest.php @@ -95,6 +95,25 @@ class GetCest { } public function testGetInfoWithExpiredToken(FunctionalTester $I) { + // We're setting up a known expired token + $I->amBearerAuthenticated( + 'eyJhbGciOiJFUzI1NiJ9.eyJpYXQiOjE0NjQ2Mjc1NDUsImV4cCI6MTQ2NDYzMTE0NSwic3ViIjoiZWx5fDEiLCJlbHktc2NvcGVzIjoi' . + 'YWNjb3VudHNfd2ViX3VzZXIifQ.m9Di3MC1SkF0dwKP0zIw1Hl0H2mB3PqwoRCXfoF0VuIQnnMurkmJoxa3A02B1zolmCPy3Wd1wKvJz3' . + 'TMpKJY2g', + ); + + $this->route->get(1); + $I->canSeeResponseCodeIs(401); + $I->canSeeResponseIsJson(); + $I->canSeeResponseContainsJson([ + 'name' => 'Unauthorized', + 'message' => 'Token expired', + 'code' => 0, + 'status' => 401, + ]); + } + + public function testGetInfoWithTokenWithOutdatedAlg(FunctionalTester $I) { // We're setting up a known expired token $I->amBearerAuthenticated( 'eyJhbGciOiJIUzI1NiJ9.eyJpYXQiOjE0NjQ2Mjc1NDUsImV4cCI6MTQ2NDYzMTE0NSwic3ViIjoiZWx5fDEiLCJlbHktc' . @@ -106,7 +125,7 @@ class GetCest { $I->canSeeResponseIsJson(); $I->canSeeResponseContainsJson([ 'name' => 'Unauthorized', - 'message' => 'Token expired', + 'message' => 'Incorrect token', 'code' => 0, 'status' => 401, ]); diff --git a/api/tests/unit/components/Tokens/ComponentTest.php b/api/tests/unit/components/Tokens/ComponentTest.php index e2d9e87..6bafdd8 100644 --- a/api/tests/unit/components/Tokens/ComponentTest.php +++ b/api/tests/unit/components/Tokens/ComponentTest.php @@ -40,10 +40,6 @@ class ComponentTest extends TestCase { } public function testParse() { - // Valid token signed with HS256 - $token = $this->component->parse('eyJhbGciOiJIUzI1NiJ9.eyJlbHktc2NvcGVzIjoiYWNjb3VudHNfd2ViX3VzZXIiLCJpYXQiOjE1NjQ1Mjc0NzYsImV4cCI6MTU2NDUzMTA3Niwic3ViIjoiZWx5fDEiLCJqdGkiOjMwNjk1OTJ9.ixapBbhaUCejbcPTnFi5nqk75XKd1_lQJd1ZPgGTLEc'); - $this->assertValidParsedToken($token, 'HS256'); - // Valid token signed with ES256 $token = $this->component->parse('eyJhbGciOiJFUzI1NiJ9.eyJlbHktc2NvcGVzIjoiYWNjb3VudHNfd2ViX3VzZXIiLCJpYXQiOjE1NjQ1Mjc0NzYsImV4cCI6MTU2NDUzMTA3Niwic3ViIjoiZWx5fDEiLCJqdGkiOjMwNjk1OTJ9.M8Kam9bv0BXui3k7Posq_vc0I95Kb_Tw7L2vPdEPlwsHqh1VJHoWtlQc32_SlsotttL7j6RYbffBkRFX2wDGFQ'); $this->assertValidParsedToken($token, 'ES256'); @@ -65,10 +61,6 @@ class ComponentTest extends TestCase { } public function getVerifyCases() { - yield 'HS256' => [ - (new Parser())->parse('eyJhbGciOiJIUzI1NiJ9.eyJlbHktc2NvcGVzIjoiYWNjb3VudHNfd2ViX3VzZXIiLCJpYXQiOjE1NjQ1Mjc0NzYsImV4cCI6MTU2NDUzMTA3Niwic3ViIjoiZWx5fDEiLCJqdGkiOjMwNjk1OTJ9.ixapBbhaUCejbcPTnFi5nqk75XKd1_lQJd1ZPgGTLEc'), - true, - ]; yield 'ES256' => [ (new Parser())->parse('eyJhbGciOiJFUzI1NiJ9.eyJlbHktc2NvcGVzIjoiYWNjb3VudHNfd2ViX3VzZXIiLCJpYXQiOjE1NjQ1Mjc0NzYsImV4cCI6MTU2NDUzMTA3Niwic3ViIjoiZWx5fDEiLCJqdGkiOjMwNjk1OTJ9.M8Kam9bv0BXui3k7Posq_vc0I95Kb_Tw7L2vPdEPlwsHqh1VJHoWtlQc32_SlsotttL7j6RYbffBkRFX2wDGFQ'), true,