From abc5bfcccc6cc41aff9d92503dc486897001ca35 Mon Sep 17 00:00:00 2001 From: Sebastian Krupinski Date: Thu, 30 Jul 2026 22:41:27 -0400 Subject: [PATCH] fix(firewall): enforce system rules independently of tenant context Signed-off-by: Sebastian Krupinski --- core/lib/Service/FirewallRuleCache.php | 18 +++- core/lib/Service/FirewallService.php | 40 +++----- core/lib/Stores/FirewallStore.php | 37 -------- .../Integration/Stores/FirewallStoreTest.php | 7 +- .../Unit/Service/FirewallRuleManagerTest.php | 6 +- .../php/Unit/Service/FirewallServiceTest.php | 92 +++++++++++++++++-- 6 files changed, 119 insertions(+), 81 deletions(-) diff --git a/core/lib/Service/FirewallRuleCache.php b/core/lib/Service/FirewallRuleCache.php index 9948d6d..5ada7ce 100644 --- a/core/lib/Service/FirewallRuleCache.php +++ b/core/lib/Service/FirewallRuleCache.php @@ -10,20 +10,30 @@ use KTXC\Stores\FirewallStore; final class FirewallRuleCache { /** @var array */ - private array $rules = []; + private array $tenantRules = []; + + /** @var FirewallRuleObject[]|null */ + private ?array $systemRules = null; public function __construct(private readonly FirewallStore $store) { } /** @return FirewallRuleObject[] */ - public function applicable(string $tenantId): array + public function tenant(string $tenantId): array { - return $this->rules[$tenantId] ??= $this->store->listApplicableRules($tenantId); + return $this->tenantRules[$tenantId] ??= $this->store->listRules($tenantId); + } + + /** @return FirewallRuleObject[] */ + public function system(): array + { + return $this->systemRules ??= $this->store->listSystemRules(); } public function invalidate(): void { - $this->rules = []; + $this->tenantRules = []; + $this->systemRules = null; } } diff --git a/core/lib/Service/FirewallService.php b/core/lib/Service/FirewallService.php index 9ab8502..7152a3d 100644 --- a/core/lib/Service/FirewallService.php +++ b/core/lib/Service/FirewallService.php @@ -73,26 +73,22 @@ class FirewallService string $ipAddress, ?string $deviceFingerprint = null ): FirewallAnalyzeResult { - // Check if firewall is enabled for this tenant - if (!$this->isEnabled()) { - return new FirewallAnalyzeResult(true); - } - $tenantId = $this->tenantContext->identifier(); - if (!$tenantId) { - return new FirewallAnalyzeResult(true); + $ruleGroups = [ + [$this->ruleCache->system(), FirewallRuleObject::ACTION_BLOCK], + ]; + + if ($tenantId !== null && $this->isEnabled()) { + $tenantRules = $this->ruleCache->tenant($tenantId); + $ruleGroups[] = [$tenantRules, FirewallRuleObject::ACTION_ALLOW]; + $ruleGroups[] = [$tenantRules, FirewallRuleObject::ACTION_BLOCK]; } - $rules = $this->getActiveRules(); + $ruleGroups[] = [$this->ruleCache->system(), FirewallRuleObject::ACTION_ALLOW]; - foreach ([ - [FirewallRuleObject::SCOPE_SYSTEM, FirewallRuleObject::ACTION_BLOCK], - [FirewallRuleObject::SCOPE_TENANT, FirewallRuleObject::ACTION_ALLOW], - [FirewallRuleObject::SCOPE_TENANT, FirewallRuleObject::ACTION_BLOCK], - [FirewallRuleObject::SCOPE_SYSTEM, FirewallRuleObject::ACTION_ALLOW], - ] as [$scope, $action]) { + foreach ($ruleGroups as [$rules, $action]) { foreach ($rules as $rule) { - if ($rule->getScope() !== $scope || $rule->getAction() !== $action) { + if ($rule->getAction() !== $action) { continue; } @@ -310,20 +306,6 @@ class FirewallService return $value; } - /** - * Get active rules (cached) - * @return FirewallRuleObject[] - */ - private function getActiveRules(): array - { - $tenantId = $this->tenantContext->identifier(); - if (!$tenantId) { - return []; - } - - return $this->ruleCache->applicable($tenantId); - } - /** * Cleanup maintenance tasks */ diff --git a/core/lib/Stores/FirewallStore.php b/core/lib/Stores/FirewallStore.php index 2a21de6..a40e238 100644 --- a/core/lib/Stores/FirewallStore.php +++ b/core/lib/Stores/FirewallStore.php @@ -55,43 +55,6 @@ class FirewallStore return $list; } - /** - * List active system-wide rules and active rules owned by a tenant. - */ - public function listApplicableRules(string $tenantId): array - { - $now = (new \DateTimeImmutable())->format(\DateTimeInterface::ATOM); - $filter = [ - 'enabled' => true, - '$and' => [ - [ - '$or' => [ - ['scope' => FirewallRuleObject::SCOPE_SYSTEM], - [ - 'tenantId' => $tenantId, - 'scope' => FirewallRuleObject::SCOPE_TENANT, - ], - ], - ], - [ - '$or' => [ - ['expiresAt' => null], - ['expiresAt' => ['$gt' => $now]], - ], - ], - ], - ]; - - $cursor = $this->dataStore->selectCollection(self::RULES_COLLECTION)->find($filter); - $list = []; - - foreach ($cursor as $entry) { - $list[] = (new FirewallRuleObject())->jsonDeserialize((array)$entry); - } - - return $list; - } - public function listSystemRules(bool $activeOnly = true): array { $filter = ['scope' => FirewallRuleObject::SCOPE_SYSTEM, 'tenantId' => null]; diff --git a/tests/php/Integration/Stores/FirewallStoreTest.php b/tests/php/Integration/Stores/FirewallStoreTest.php index 04010ca..db53971 100644 --- a/tests/php/Integration/Stores/FirewallStoreTest.php +++ b/tests/php/Integration/Stores/FirewallStoreTest.php @@ -45,7 +45,7 @@ class FirewallStoreTest extends TestCase } } - #[TestDox('Applicable rules contain system rules and only the requested tenant rules')] + #[TestDox('System and tenant rule sets remain independently scoped')] public function testApplicableScopes(): void { $tenantA = $this->rule('tenant-a', FirewallRuleObject::SCOPE_TENANT, 'tenant-a'); @@ -60,7 +60,10 @@ class FirewallStoreTest extends TestCase $this->store->depositRule($rule); } - $rules = $this->store->listApplicableRules('tenant-a'); + $rules = array_merge( + $this->store->listSystemRules(), + $this->store->listRules('tenant-a') + ); $reasons = array_map(static fn(FirewallRuleObject $rule): ?string => $rule->getReason(), $rules); sort($reasons); diff --git a/tests/php/Unit/Service/FirewallRuleManagerTest.php b/tests/php/Unit/Service/FirewallRuleManagerTest.php index 870a783..c8f0966 100644 --- a/tests/php/Unit/Service/FirewallRuleManagerTest.php +++ b/tests/php/Unit/Service/FirewallRuleManagerTest.php @@ -94,7 +94,7 @@ class FirewallRuleManagerTest extends TestCase public function testCacheInvalidation(): void { $this->store->expects($this->exactly(2)) - ->method('listApplicableRules') + ->method('listRules') ->with('tenant-a') ->willReturnOnConsecutiveCalls([], []); $cache = new FirewallRuleCache($this->store); @@ -102,8 +102,8 @@ class FirewallRuleManagerTest extends TestCase $this->store->method('findExactIpRule')->willReturn(null); $this->store->method('depositRule')->willReturnArgument(0); - self::assertSame([], $cache->applicable('tenant-a')); + self::assertSame([], $cache->tenant('tenant-a')); $manager->blockIp(FirewallRuleScope::tenant('tenant-a'), '203.0.113.10', null, 'admin'); - self::assertSame([], $cache->applicable('tenant-a')); + self::assertSame([], $cache->tenant('tenant-a')); } } diff --git a/tests/php/Unit/Service/FirewallServiceTest.php b/tests/php/Unit/Service/FirewallServiceTest.php index 1ea1dde..f168312 100644 --- a/tests/php/Unit/Service/FirewallServiceTest.php +++ b/tests/php/Unit/Service/FirewallServiceTest.php @@ -67,10 +67,8 @@ class FirewallServiceTest extends TestCase 'tenant-a' ); - $this->store->expects($this->once()) - ->method('listApplicableRules') - ->with('tenant-a') - ->willReturn([$tenantAllow, $systemBlock]); + $this->store->expects($this->once())->method('listSystemRules')->willReturn([$systemBlock]); + $this->store->expects($this->once())->method('listRules')->with('tenant-a')->willReturn([$tenantAllow]); $this->events->expects($this->once())->method('dispatch'); $result = $this->service->analyze('203.0.113.10'); @@ -95,7 +93,8 @@ class FirewallServiceTest extends TestCase 'tenant-a' ); - $this->store->method('listApplicableRules')->willReturn([$systemAllow, $tenantBlock]); + $this->store->method('listSystemRules')->willReturn([$systemAllow]); + $this->store->method('listRules')->with('tenant-a')->willReturn([$tenantBlock]); $this->events->expects($this->once())->method('dispatch'); $result = $this->service->analyze('203.0.113.10'); @@ -107,8 +106,9 @@ class FirewallServiceTest extends TestCase #[TestDox('Rule caches are isolated by tenant')] public function testTenantCacheIsolation(): void { + $this->store->expects($this->once())->method('listSystemRules')->willReturn([]); $this->store->expects($this->exactly(2)) - ->method('listApplicableRules') + ->method('listRules') ->willReturnCallback(static fn(string $tenantId): array => [ (new FirewallRuleObject()) ->setId($tenantId) @@ -124,6 +124,79 @@ class FirewallServiceTest extends TestCase self::assertSame('tenant-b', $this->service->analyze('203.0.113.10')->ruleId); } + #[TestDox('System blocks apply when no tenant is resolved')] + public function testSystemBlockWithoutTenant(): void + { + $this->currentTenant = null; + $this->store->method('listSystemRules')->willReturn([ + $this->rule( + 'system-block', + FirewallRuleObject::SCOPE_SYSTEM, + FirewallRuleObject::ACTION_BLOCK, + null + ), + ]); + $this->store->expects($this->never())->method('listRules'); + + self::assertSame('system-block', $this->service->analyze('203.0.113.10')->ruleId); + } + + #[TestDox('System allows can match when no tenant is resolved')] + public function testSystemAllowWithoutTenant(): void + { + $this->currentTenant = null; + $this->store->method('listSystemRules')->willReturn([ + $this->rule( + 'system-allow', + FirewallRuleObject::SCOPE_SYSTEM, + FirewallRuleObject::ACTION_ALLOW, + null + ), + ]); + + $result = $this->service->analyze('203.0.113.10'); + + self::assertTrue($result->isAllowed()); + self::assertSame('system-allow', $result->ruleId); + } + + #[TestDox('System blocks remain active when the tenant firewall is disabled')] + public function testSystemBlockWithDisabledTenant(): void + { + $this->disableTenantFirewall(); + $this->store->method('listSystemRules')->willReturn([ + $this->rule( + 'system-block', + FirewallRuleObject::SCOPE_SYSTEM, + FirewallRuleObject::ACTION_BLOCK, + null + ), + ]); + $this->store->expects($this->never())->method('listRules'); + + self::assertSame('system-block', $this->service->analyze('203.0.113.10')->ruleId); + } + + #[TestDox('Tenant rules are ignored when the tenant firewall is disabled')] + public function testTenantRulesDisabled(): void + { + $this->disableTenantFirewall(); + $this->store->method('listSystemRules')->willReturn([]); + $this->store->expects($this->never())->method('listRules'); + + self::assertTrue($this->service->analyze('203.0.113.10')->isAllowed()); + } + + #[TestDox('Requests without a tenant and without a system match are allowed')] + public function testNoTenantDefault(): void + { + $this->currentTenant = null; + $this->store->method('listSystemRules')->willReturn([]); + $this->store->expects($this->never())->method('listRules'); + + self::assertTrue($this->service->analyze('203.0.113.10')->isAllowed()); + } + #[TestDox('Typed tenant firewall settings drive brute-force thresholds')] public function testFirewallConfiguration(): void { @@ -248,4 +321,11 @@ class FirewallServiceTest extends TestCase ->setValue('203.0.113.10') ->setReason($id); } + + private function disableTenantFirewall(): void + { + $this->currentConfiguration = (new TenantConfiguration())->jsonDeserialize([ + 'firewall' => ['enabled' => false], + ]); + } }