From 27e9dba47a36ae9aa6ec2ea32f847758fd6ba3a9 Mon Sep 17 00:00:00 2001 From: Adrien Crivelli Date: Mon, 24 Jul 2023 17:43:12 +0200 Subject: [PATCH] IPs can be configured to bypass signed queries #9696 The configuration `allowedIps` supports a list of IP or subnet in v4 or v6. If the request is coming from one of those ranges, and it is not signed at all, then it will be accepted. However, if the request is signed, the `allowedIps` has no effect at all. This is because we want to be able to test signed queries even from an allowed IP. --- src/ConfigProvider.php | 1 + src/Middleware/SignedQueryMiddleware.php | 29 +++++- .../SignedQueryMiddlewareFactory.php | 2 +- src/Validator/IPRange.php | 98 +++++++++++++++++++ .../Middleware/SignedQueryMiddlewareTest.php | 32 ++++-- tests/Validator/IPRangeTest.php | 55 +++++++++++ 6 files changed, 207 insertions(+), 10 deletions(-) create mode 100644 src/Validator/IPRange.php create mode 100644 tests/Validator/IPRangeTest.php diff --git a/src/ConfigProvider.php b/src/ConfigProvider.php index 38a415d..190f0d7 100644 --- a/src/ConfigProvider.php +++ b/src/ConfigProvider.php @@ -12,6 +12,7 @@ public function __invoke(): array 'signedQueries' => [ 'required' => true, 'keys' => [], + 'allowedIps' => [], ], 'dependencies' => [ 'invokables' => [ diff --git a/src/Middleware/SignedQueryMiddleware.php b/src/Middleware/SignedQueryMiddleware.php index 3cd9574..1acf638 100644 --- a/src/Middleware/SignedQueryMiddleware.php +++ b/src/Middleware/SignedQueryMiddleware.php @@ -5,6 +5,7 @@ namespace Ecodev\Felix\Middleware; use Cake\Chronos\Chronos; +use Ecodev\Felix\Validator\IPRange; use Exception; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; @@ -30,8 +31,11 @@ */ final class SignedQueryMiddleware implements MiddlewareInterface { - public function __construct(private readonly array $keys, private readonly bool $required = true) - { + public function __construct( + private readonly array $keys, + private readonly array $allowedIps, + private readonly bool $required = true + ) { if ($this->required && !$this->keys) { throw new Exception('Signed queries are required, but no keys are configured'); } @@ -50,6 +54,10 @@ private function verify(ServerRequestInterface $request): void { $autorization = $request->getHeader('authorization')[0] ?? ''; if (!$autorization) { + if ($this->isAllowedIp($request)) { + return; + } + throw new Exception('Missing `Authorization` HTTP header in signed query', 403); } @@ -107,4 +115,21 @@ private function getOperations(ServerRequestInterface $request): mixed throw new Exception('Could not find GraphQL operations in request', 403); } + + private function isAllowedIp(ServerRequestInterface $request): bool + { + $remoteAddress = $request->getServerParams()['REMOTE_ADDR'] ?? ''; + + if (!$remoteAddress || !is_string($remoteAddress)) { + return false; + } + + foreach ($this->allowedIps as $allowedIp) { + if (IPRange::matches($remoteAddress, $allowedIp)) { + return true; + } + } + + return false; + } } diff --git a/src/Middleware/SignedQueryMiddlewareFactory.php b/src/Middleware/SignedQueryMiddlewareFactory.php index 2feb7c3..535e000 100644 --- a/src/Middleware/SignedQueryMiddlewareFactory.php +++ b/src/Middleware/SignedQueryMiddlewareFactory.php @@ -18,6 +18,6 @@ public function __invoke(ContainerInterface $container, $requestedName, ?array $ $config = $container->get('config'); $signedQueries = $config['signedQueries']; - return new SignedQueryMiddleware($signedQueries['keys'], $signedQueries['required']); + return new SignedQueryMiddleware($signedQueries['keys'], $signedQueries['allowedIps'], $signedQueries['required']); } } diff --git a/src/Validator/IPRange.php b/src/Validator/IPRange.php new file mode 100644 index 0000000..a424f50 --- /dev/null +++ b/src/Validator/IPRange.php @@ -0,0 +1,98 @@ + 32) { + return false; + } + + $ip = ip2long($ip); + $subnet = ip2long($subnet); + if (false === $ip || false === $subnet) { + // Invalid data + return false; + } + + return 0 === substr_compare( + sprintf('%032b', $ip), + sprintf('%032b', $subnet), + 0, + $mask + ); + } + + private static function matchesIPv6(string $ip, string $cidr): bool + { + $mask = 128; + $subnet = $cidr; + + if (str_contains($cidr, '/')) { + [$subnet, $mask] = explode('/', $cidr, 2); + $mask = (int) $mask; + } + + if ($mask < 0 || $mask > 128) { + return false; + } + + $ip = inet_pton($ip); + $subnet = inet_pton($subnet); + + if (false === $ip || false === $subnet) { + // Invalid data + return false; + } + + // mask 0: if it's a valid IP, it's valid + if ($mask === 0) { + return (bool) unpack('n*', $ip); + } + + /** @see http://stackoverflow.com/questions/7951061/matching-ipv6-address-to-a-cidr-subnet, MW answer */ + $binMask = str_repeat('f', (int) ($mask / 4)); + switch ($mask % 4) { + case 0: + break; + case 1: + $binMask .= '8'; + + break; + case 2: + $binMask .= 'c'; + + break; + case 3: + $binMask .= 'e'; + + break; + } + + $binMask = str_pad($binMask, 32, '0'); + $binMask = pack('H*', $binMask); + + return ($ip & $binMask) === $subnet; + } +} diff --git a/tests/Middleware/SignedQueryMiddlewareTest.php b/tests/Middleware/SignedQueryMiddlewareTest.php index 4fb5764..a742675 100644 --- a/tests/Middleware/SignedQueryMiddlewareTest.php +++ b/tests/Middleware/SignedQueryMiddlewareTest.php @@ -27,9 +27,9 @@ protected function tearDown(): void /** * @dataProvider dataProviderQuery */ - public function testRequiredSignedQuery(array $keys, string $body, null|array $parsedBody, string $signature, string $expectExceptionMessage = ''): void + public function testRequiredSignedQuery(array $keys, string $body, null|array $parsedBody, string $signature, string $expectExceptionMessage = '', string $ip = ''): void { - $this->process($keys, true, $body, $parsedBody, $signature, $expectExceptionMessage); + $this->process($keys, true, $ip, $body, $parsedBody, $signature, $expectExceptionMessage); } /** @@ -37,19 +37,19 @@ public function testRequiredSignedQuery(array $keys, string $body, null|array $p */ public function testNonRequiredSignedQuery(array $keys, string $body, null|array $parsedBody, string $signature): void { - $this->process($keys, false, $body, $parsedBody, $signature, ''); + $this->process($keys, false, '', $body, $parsedBody, $signature, ''); } public function testThrowIfNoKeys(): void { $this->expectExceptionMessage('Signed queries are required, but no keys are configured'); $this->expectExceptionCode(0); - new SignedQueryMiddleware([]); + new SignedQueryMiddleware([], []); } - private function process(array $keys, bool $required, string $body, null|array $parsedBody, string $signature, string $expectExceptionMessage): void + private function process(array $keys, bool $required, string $ip, string $body, null|array $parsedBody, string $signature, string $expectExceptionMessage): void { - $request = new ServerRequest(); + $request = new ServerRequest(['REMOTE_ADDR' => $ip]); $request = $request->withBody(new CallbackStream(fn () => $body))->withParsedBody($parsedBody); if ($signature) { @@ -61,7 +61,7 @@ private function process(array $keys, bool $required, string $body, null|array $ ->method('handle') ->willReturn(new Response()); - $middleware = new SignedQueryMiddleware($keys, $required); + $middleware = new SignedQueryMiddleware($keys, ['1.2.3.4', '2a01:198:603:0::/65'], $required); if ($expectExceptionMessage) { $this->expectExceptionMessage($expectExceptionMessage); @@ -180,5 +180,23 @@ public function dataProviderQuery(): iterable 'v1.1577964600.' . str_repeat('a', 64), 'Could not find GraphQL operations in request', ]; + + yield 'no header, but allowed IPv4' => [ + [$key1], + '{"operationName":"CurrentUser","variables":{},"query":"query CurrentUser { viewer { id }}', + null, + '', + '', + '1.2.3.4', + ]; + + yield 'simple but wrong key will still error even if IP is allowed, because we want to be able to test signature even when allowed' => [ + [$key2], + '{"operationName":"CurrentUser","variables":{},"query":"query CurrentUser { viewer { id }}', + null, + 'v1.1577964600.a4d664cd3d9903e4fecf6f9f671ad953586a7faeb16e67c306fd9f29999dfdd7', + 'Invalid signed query', + '1.2.3.4', + ]; } } diff --git a/tests/Validator/IPRangeTest.php b/tests/Validator/IPRangeTest.php new file mode 100644 index 0000000..5cd90b3 --- /dev/null +++ b/tests/Validator/IPRangeTest.php @@ -0,0 +1,55 @@ + [true, '192.168.1.1', '192.168.1.1'], + 'valid - entirety of class-c (/1)' => [true, '192.168.1.1', '192.168.1.1/1'], + 'valid - class-c private subnet (/24)' => [true, '192.168.1.1', '192.168.1.0/24'], + 'valid - any subnet (/0)' => [true, '1.2.3.4', '0.0.0.0/0'], + 'valid - subnet expands to all' => [true, '1.2.3.4', '192.168.1.0/0'], + 'invalid - class-a invalid subnet' => [false, '192.168.1.1', '1.2.3.4/1'], + 'invalid - CIDR mask out-of-range' => [false, '192.168.1.1', '192.168.1.1/33'], + 'invalid - invalid cidr notation' => [false, '1.2.3.4', '256.256.256/0'], + 'invalid - invalid IP address' => [false, 'an_invalid_ip', '192.168.1.0/24'], + 'invalid - empty IP address' => [false, '', '1.2.3.4/1'], + 'invalid - proxy wildcard' => [false, '192.168.20.13', '*'], + 'invalid - proxy missing netmask' => [false, '192.168.20.13', '0.0.0.0'], + 'invalid - request IP with invalid proxy wildcard' => [false, '0.0.0.0', '*'], + ]; + } + + public function IPv6Data(): iterable + { + return [ + 'valid - ipv4 subnet' => [true, '2a01:198:603:0:396e:4789:8e99:890f', '2a01:198:603:0::/65'], + 'valid - exact' => [true, '0:0:0:0:0:0:0:1', '::1'], + 'valid - all subnets' => [true, '0:0:603:0:396e:4789:8e99:0001', '::/0'], + 'valid - subnet expands to all' => [true, '0:0:603:0:396e:4789:8e99:0001', '2a01:198:603:0::/0'], + 'invalid - not in subnet' => [false, '2a00:198:603:0:396e:4789:8e99:890f', '2a01:198:603:0::/65'], + 'invalid - does not match exact' => [false, '2a01:198:603:0:396e:4789:8e99:890f', '::1'], + 'invalid - compressed notation, does not match exact' => [false, '0:0:603:0:396e:4789:8e99:0001', '::1'], + 'invalid - garbage IP' => [false, '}__test|O:21:"JDatabaseDriverMysqli":3:{s:2', '::1'], + 'invalid - invalid cidr' => [false, '2a01:198:603:0:396e:4789:8e99:890f', 'unknown'], + 'invalid - empty IP address' => [false, '', '::1'], + ]; + } +}