From 06c904af378801fa9caf84b589ae0a82d81227d7 Mon Sep 17 00:00:00 2001 From: Vladyslav Vildanov <117659936+vladvildanov@users.noreply.github.com> Date: Tue, 13 May 2025 19:01:21 +0300 Subject: [PATCH] fix(auth): Adds default user if only password used for authentication (#1535) --- .github/docker-compose.yml | 29 ++++++++++++-- .github/workflows/tests.yml | 17 ++++++++ CHANGELOG.md | 4 ++ phpunit.relay.xml | 8 +++- phpunit.xml.dist | 8 +++- src/Command/Container/ACL.php | 2 + src/Connection/Factory.php | 9 +++-- src/Connection/StreamConnection.php | 8 +++- tests/PHPUnit/PredisConnectionTestCase.php | 28 +++++++++++-- tests/PHPUnit/PredisTestCase.php | 30 +++++++++++++- tests/Predis/ClientTest.php | 40 +++++++++++++++++++ .../CompositeStreamConnectionTest.php | 1 + tests/Predis/Connection/FactoryTest.php | 4 +- .../Connection/StreamConnectionTest.php | 20 +--------- tests/Predis/Monitor/ConsumerTest.php | 1 + .../Response/Iterator/MultiBulkTest.php | 7 ++++ .../Response/Iterator/MultiBulkTupleTest.php | 7 ++++ 17 files changed, 187 insertions(+), 36 deletions(-) diff --git a/.github/docker-compose.yml b/.github/docker-compose.yml index 865557d6..c9984c86 100644 --- a/.github/docker-compose.yml +++ b/.github/docker-compose.yml @@ -12,6 +12,8 @@ services: retries: 3 ports: - "6379:6379" + command: + - --requirepass "foobar" profiles: - all - official @@ -23,9 +25,10 @@ services: environment: - TLS_ENABLED=yes - REDIS_CLUSTER=no + - REDIS_PASSWORD=foobar - PORT=6379 - TLS_PORT=6666 - command: ${REDIS_EXTRA_ARGS:---enable-debug-command yes --enable-module-command yes --tls-auth-clients optional --save ""} + command: ${REDIS_EXTRA_ARGS:---enable-debug-command yes --enable-module-command yes --tls-auth-clients optional --save "" --requirepass "foobar"} ports: - 6379:6379 - 6666:6666 # TLS port @@ -36,17 +39,36 @@ services: - clients - standalone + redis-unprotected: + image: ${REDIS_IMAGE_NAME:-redislabs/client-libs-test:7.4.2} + container_name: redis-unprotected + environment: + - TLS_ENABLED=no + - REDIS_CLUSTER=no + - PORT=6379 + - TLS_PORT=6666 + command: ${REDIS_EXTRA_ARGS:---enable-debug-command yes --enable-module-command yes --tls-auth-clients optional --save ""} + ports: + - 6380:6379 + volumes: + - "./dockers/unprotected:/redis/work" + profiles: + - all + - clients + - unprotected + redis-cluster: image: ${REDIS_IMAGE_NAME:-redislabs/client-libs-test:7.4.2} container_name: redis-cluster environment: - REDIS_CLUSTER=yes + - REDIS_PASSWORD=foobar - NODES=6 - REPLICAS=1 - TLS_ENABLED=yes - PORT=6372 - TLS_PORT=27379 - command: ${REDIS_EXTRA_ARGS:---enable-debug-command yes --enable-module-command yes --tls-auth-clients optional --save ""} + command: ${REDIS_EXTRA_ARGS:---enable-debug-command yes --enable-module-command yes --tls-auth-clients optional --save "" --requirepass "foobar"} ports: - "6372-6377:6372-6377" volumes: @@ -63,8 +85,9 @@ services: - "6479:6379" environment: - REDIS_CLUSTER=no + - REDIS_PASSWORD=foobar - PORT=6379 - command: ${REDIS_EXTRA_ARGS:---enable-debug-command yes --enable-module-command yes --save ""} + command: ${REDIS_EXTRA_ARGS:---enable-debug-command yes --enable-module-command yes --save "" --requirepass "foobar"} volumes: - "./dockers/redis-stack:/redis/work" profiles: diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index bd0cac60..9a06e314 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -78,6 +78,13 @@ jobs: compose-file: .github/docker-compose.yml services: ${{ env.DOCKER_SERVICE }} + - name: Start Redis unprotected image + uses: hoverkraft-tech/compose-action@v2.0.1 + if: ${{ matrix.redis > '4.0' }} + with: + compose-file: .github/docker-compose.yml + services: redis-unprotected + - name: Start Redis stack image id: stack_infra uses: hoverkraft-tech/compose-action@v2.0.1 @@ -123,6 +130,14 @@ jobs: if: ${{ matrix.php == '8.4' && matrix.redis == '7.4' }} run: vendor/bin/phpunit -c phpunit.relay.xml --coverage-php build/cov/coverage-relay.cov --coverage-filter ./src + - name: Run tests against unprotected Redis + if: ${{ (matrix.php != '8.4' || matrix.redis != '7.4') && matrix.redis > '4.0'}} + run: vendor/bin/phpunit --group unprotected + + - name: Run tests against unprotected Redis with coverage + if: ${{ matrix.php == '8.4' && matrix.redis == '7.4' }} + run: vendor/bin/phpunit --group unprotected --coverage-php build/cov/coverage-unprotected.cov --coverage-filter ./src + - name: Run stack tests if: ${{ (matrix.php != '8.4' || matrix.redis != '7.4') && matrix.redis >= '7.2' }} run: vendor/bin/phpunit --group realm-stack @@ -156,11 +171,13 @@ jobs: - name: Run tests against cluster using Relay if: ${{ matrix.php != '8.4' && matrix.redis == '7.4' }} run: | + sleep 5 vendor/bin/phpunit -c phpunit.relay.xml --group cluster - name: Run tests against cluster using Relay with coverage if: ${{ matrix.php == '8.4' && matrix.redis == '7.4' }} run: | + sleep 5 vendor/bin/phpunit -c phpunit.relay.xml --group cluster --coverage-php build/cov/coverage-cluster-relay.cov --coverage-filter ./src - name: Merge coverage reports diff --git a/CHANGELOG.md b/CHANGELOG.md index b3508f95..a89c8d77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,8 @@ ## Changelog +## Unreleased + +### Fixed +- fix(auth): Adds default user if only password used for authentication (#1535) ## Unreleased ### Maintenance diff --git a/phpunit.relay.xml b/phpunit.relay.xml index 1d75c080..e7bb768e 100644 --- a/phpunit.relay.xml +++ b/phpunit.relay.xml @@ -29,6 +29,7 @@ ext-curl cluster gears + unprotected @@ -41,12 +42,17 @@ + + - + diff --git a/phpunit.xml.dist b/phpunit.xml.dist index f2a59fb2..dac30316 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -29,6 +29,7 @@ cluster gears gears-cluster + unprotected @@ -45,12 +46,17 @@ + + - + diff --git a/src/Command/Container/ACL.php b/src/Command/Container/ACL.php index 0b835c1b..70e3ec03 100644 --- a/src/Command/Container/ACL.php +++ b/src/Command/Container/ACL.php @@ -17,8 +17,10 @@ use Predis\Response\Status; /** * @method array cat(string $category = null) * @method Status dryRun(string $username, string $command, ...$arguments) + * @method int delUser(string ...$username) * @method array getUser(string $username) * @method Status setUser(string $username, string ...$rules) + * @method string whoami() */ class ACL extends AbstractContainer { diff --git a/src/Connection/Factory.php b/src/Connection/Factory.php index e003a4fd..719b42df 100644 --- a/src/Connection/Factory.php +++ b/src/Connection/Factory.php @@ -164,13 +164,14 @@ class Factory implements FactoryInterface { $parameters = $connection->getParameters(); - if (isset($parameters->password) && strlen($parameters->password)) { + if (!empty($parameters->password)) { $cmdAuthArgs = [$parameters->protocol, 'AUTH']; - isset($parameters->username) && strlen($parameters->username) - ? array_push($cmdAuthArgs, $parameters->username, $parameters->password) - : $cmdAuthArgs[] = $parameters->password; + if (empty($parameters->username)) { + $parameters->username = 'default'; + } + array_push($cmdAuthArgs, $parameters->username, $parameters->password); array_push($cmdAuthArgs, 'SETNAME', 'predis'); $connection->addConnectCommand( diff --git a/src/Connection/StreamConnection.php b/src/Connection/StreamConnection.php index dde6fa52..bfcdbdc6 100644 --- a/src/Connection/StreamConnection.php +++ b/src/Connection/StreamConnection.php @@ -316,9 +316,13 @@ class StreamConnection extends AbstractConnection if (in_array('AUTH', $failedCommand->getArguments(), true)) { $parameters = $this->getParameters(); - $auth = new RawCommand('AUTH', [$parameters->username, $parameters->password]); + // If Redis <= 6.0 + $auth = new RawCommand('AUTH', [$parameters->password]); $response = $this->executeCommand($auth); - $this->handleOnConnectResponse($response, $auth); + + if ($response instanceof ErrorResponseInterface) { + $this->onConnectionError("Failed: {$response->getMessage()}"); + } } $setName = new RawCommand('CLIENT', ['SETNAME', 'predis']); diff --git a/tests/PHPUnit/PredisConnectionTestCase.php b/tests/PHPUnit/PredisConnectionTestCase.php index bf931bca..fc81535f 100644 --- a/tests/PHPUnit/PredisConnectionTestCase.php +++ b/tests/PHPUnit/PredisConnectionTestCase.php @@ -14,6 +14,7 @@ namespace Predis\Connection; use PHPUnit\Framework\MockObject\MockObject; use Predis\Command\CommandInterface; +use Predis\Command\RawCommand; use PredisTestCase; /** @@ -492,6 +493,7 @@ abstract class PredisConnectionTestCase extends PredisTestCase 'host' => constant('REDIS_SERVER_HOST'), 'port' => constant('REDIS_SERVER_PORT'), 'database' => constant('REDIS_SERVER_DBNUM'), + 'password' => getenv('REDIS_PASSWORD') ?: constant('REDIS_PASSWORD'), 'read_write_timeout' => 2, ]; } @@ -529,9 +531,9 @@ abstract class PredisConnectionTestCase extends PredisTestCase * * @return NodeConnectionInterface */ - protected function createConnection(bool $initialize = false): NodeConnectionInterface + protected function createConnection(bool $initialize = false, bool $noAuth = false): NodeConnectionInterface { - return $this->createConnectionWithParams([], $initialize); + return $this->createConnectionWithParams([], $initialize, $noAuth); } /** @@ -542,8 +544,11 @@ abstract class PredisConnectionTestCase extends PredisTestCase * * @return NodeConnectionInterface */ - protected function createConnectionWithParams($parameters, $initialize = false): NodeConnectionInterface - { + protected function createConnectionWithParams( + $parameters, + $initialize = false, + bool $noAuth = false + ): NodeConnectionInterface { $class = $this->getConnectionClass(); $commands = $this->getCommandFactory(); @@ -553,6 +558,21 @@ abstract class PredisConnectionTestCase extends PredisTestCase $connection = new $class($parameters); + if (!$noAuth) { + if (isset($parameters->password) && strlen($parameters->password)) { + if (!isset($parameters->username) || !strlen($parameters->username)) { + $parameters->username = 'default'; + } + + $connection->addConnectCommand( + new RawCommand( + 'HELLO', + [$parameters->protocol, 'AUTH', $parameters->username, $parameters->password] + ) + ); + } + } + if ($initialize) { $connection->addConnectCommand( $commands->create('select', [$parameters->database]) diff --git a/tests/PHPUnit/PredisTestCase.php b/tests/PHPUnit/PredisTestCase.php index cc4abbef..51a70746 100644 --- a/tests/PHPUnit/PredisTestCase.php +++ b/tests/PHPUnit/PredisTestCase.php @@ -172,11 +172,23 @@ abstract class PredisTestCase extends PHPUnit\Framework\TestCase return $this->prepareClusterEndpoints(); } + $password = getenv('REDIS_PASSWORD') ?: constant('REDIS_PASSWORD'); + + if ($this->isStackTest()) { + $port = getenv('REDIS_STACK_SERVER_PORT'); + } elseif ($this->isUnprotectedTest()) { + $port = constant('REDIS_UNPROTECTED_SERVER_PORT'); + $password = ''; + } else { + $port = constant('REDIS_SERVER_PORT'); + } + return [ 'scheme' => 'tcp', 'host' => constant('REDIS_SERVER_HOST'), - 'port' => ($this->isStackTest()) ? getenv('REDIS_STACK_SERVER_PORT') : constant('REDIS_SERVER_PORT'), + 'port' => $port, 'database' => constant('REDIS_SERVER_DBNUM'), + 'password' => $password, ]; } @@ -604,6 +616,22 @@ abstract class PredisTestCase extends PHPUnit\Framework\TestCase && in_array('realm-stack', $annotations['class']['group'], true); } + /** + * Check annotations if it's matches to unprotected test scenario. + * + * @return bool + */ + protected function isUnprotectedTest(): bool + { + $annotations = TestUtil::parseTestMethodAnnotations( + get_class($this), + $this->getName(false) + ); + + return isset($annotations['method']['group']) + && in_array('unprotected', $annotations['method']['group'], true); + } + /** * Parse comma-separated cluster endpoints and convert them into tcp strings. * diff --git a/tests/Predis/ClientTest.php b/tests/Predis/ClientTest.php index efc36525..592f5f8e 100644 --- a/tests/Predis/ClientTest.php +++ b/tests/Predis/ClientTest.php @@ -1380,6 +1380,46 @@ class ClientTest extends PredisTestCase $this->assertSame('value', $client->get('key')); } + /** + * @group connected + * @group unprotected + * @requiresRedisVersion >= 2.0.0 + * @return void + */ + public function testClientAuthenticationAgainstUnprotectedServer(): void + { + $client = new Client($this->getParameters()); + + $this->assertEquals('OK', $client->set('key', 'value')); + $this->assertSame('value', $client->get('key')); + + // AUTH doesn't throw exception if no authentication requires. + $clientWithPassword = new Client($this->getParameters( + ['password' => getenv('REDIS_PASSWORD') ?: constant('REDIS_PASSWORD')]) + ); + $this->assertEquals('OK', $clientWithPassword->set('key', 'value')); + $this->assertSame('value', $clientWithPassword->get('key')); + + $this->assertEquals( + 'OK', + $client->acl->setUser( + 'test_user', + 'on', + '>foobar', + 'allcommands', + 'allkeys' + ) + ); + + $clientTestUser = new Client($this->getParameters( + ['username' => 'test_user', 'password' => 'foobar']) + ); + $this->assertEquals('test_user', $clientTestUser->acl->whoami()); + $this->assertEquals('OK', $clientTestUser->set('key', 'value')); + $this->assertSame('value', $clientTestUser->get('key')); + $this->assertEquals(1, $clientTestUser->acl->delUser('test_user')); + } + // ******************************************************************** // // ---- HELPER METHODS ------------------------------------------------ // // ******************************************************************** // diff --git a/tests/Predis/Connection/CompositeStreamConnectionTest.php b/tests/Predis/Connection/CompositeStreamConnectionTest.php index e5667412..f5a20842 100644 --- a/tests/Predis/Connection/CompositeStreamConnectionTest.php +++ b/tests/Predis/Connection/CompositeStreamConnectionTest.php @@ -69,6 +69,7 @@ class CompositeStreamConnectionTest extends PredisConnectionTestCase /** @var CompositeConnectionInterface */ $connection = $this->createConnection(true); $commands = $this->getCommandFactory(); + $connection->connect(); $connection->getProtocol()->useIterableMultibulk(true); diff --git a/tests/Predis/Connection/FactoryTest.php b/tests/Predis/Connection/FactoryTest.php index 22f38d10..30493054 100644 --- a/tests/Predis/Connection/FactoryTest.php +++ b/tests/Predis/Connection/FactoryTest.php @@ -307,7 +307,7 @@ class FactoryTest extends PredisTestCase ->expects($this->exactly(4)) ->method('addConnectCommand') ->withConsecutive( - [$this->isRedisCommand('HELLO', [2, 'AUTH', 'foobar', 'SETNAME', 'predis'])], + [$this->isRedisCommand('HELLO', [2, 'AUTH', 'default', 'foobar', 'SETNAME', 'predis'])], [$this->isRedisCommand('CLIENT', ['SETINFO', 'LIB-NAME', 'predis'])], [$this->isRedisCommand('CLIENT', ['SETINFO', 'LIB-VER', Client::VERSION])], [$this->isRedisCommand('SELECT', ['0'])] @@ -338,7 +338,7 @@ class FactoryTest extends PredisTestCase $connection->expects($this->exactly(3)) ->method('addConnectCommand') ->withConsecutive( - [$this->isRedisCommand('HELLO', [2, 'AUTH', 'foobar', 'SETNAME', 'predis'])], + [$this->isRedisCommand('HELLO', [2, 'AUTH', 'default', 'foobar', 'SETNAME', 'predis'])], [$this->isRedisCommand('CLIENT', ['SETINFO', 'LIB-NAME', 'predis'])], [$this->isRedisCommand('CLIENT', ['SETINFO', 'LIB-VER', Client::VERSION])] ); diff --git a/tests/Predis/Connection/StreamConnectionTest.php b/tests/Predis/Connection/StreamConnectionTest.php index e3241a64..71817b4c 100644 --- a/tests/Predis/Connection/StreamConnectionTest.php +++ b/tests/Predis/Connection/StreamConnectionTest.php @@ -25,7 +25,7 @@ use RuntimeException; /** * @method StreamConnection createConnection(bool $initialize = false) - * @method StreamConnection createConnectionWithParams($parameters, $initialize = false) + * @method StreamConnection createConnectionWithParams($parameters, $initialize = false, bool $noAuth = false) */ class StreamConnectionTest extends PredisConnectionTestCase { @@ -844,22 +844,6 @@ class StreamConnectionTest extends PredisConnectionTestCase $this->assertNotNull($connection->getClientId()); } - /** - * @group connected - * @return void - * @requiresRedisVersion >= 6.2.0 - */ - public function testDoNotSetClientIdOnResp2ConnectionIfNotHelloCommand(): void - { - $connection = $this->createConnectionWithParams([]); - $connection->addConnectCommand( - new RawCommand('INFO') - ); - $connection->connect(); - - $this->assertNull($connection->getClientId()); - } - /** * @group connected * @return void @@ -920,7 +904,7 @@ class StreamConnectionTest extends PredisConnectionTestCase { $failedCommand = new RawCommand('HELLO', ['FOOBAR', 'AUTH', 'foobar']); - $connection = $this->createConnection(); + $connection = $this->createConnectionWithParams(['password' => ''], false, true); $connection->addConnectCommand($failedCommand); $this->expectException(ConnectionException::class); diff --git a/tests/Predis/Monitor/ConsumerTest.php b/tests/Predis/Monitor/ConsumerTest.php index fd98a8be..ca8211a3 100644 --- a/tests/Predis/Monitor/ConsumerTest.php +++ b/tests/Predis/Monitor/ConsumerTest.php @@ -211,6 +211,7 @@ class ConsumerTest extends PredisTestCase 'host' => constant('REDIS_SERVER_HOST'), 'port' => constant('REDIS_SERVER_PORT'), 'database' => constant('REDIS_SERVER_DBNUM'), + 'password' => getenv('REDIS_PASSWORD') ?: constant('REDIS_PASSWORD'), // Prevents suite from handing on broken test 'read_write_timeout' => 2, ]; diff --git a/tests/Predis/Response/Iterator/MultiBulkTest.php b/tests/Predis/Response/Iterator/MultiBulkTest.php index 4c8e1b8e..5b1c5404 100644 --- a/tests/Predis/Response/Iterator/MultiBulkTest.php +++ b/tests/Predis/Response/Iterator/MultiBulkTest.php @@ -14,6 +14,7 @@ namespace Predis\Response\Iterator; use Predis\Client; use Predis\ClientInterface; +use Predis\Command\RawCommand; use Predis\Connection\CompositeStreamConnection; use Predis\Protocol\Text\ProtocolProcessor as TextProtocolProcessor; use PredisTestCase; @@ -119,6 +120,12 @@ class MultiBulkTest extends PredisTestCase $protocol->useIterableMultibulk(true); $connection = new CompositeStreamConnection($parameters, $protocol); + $connection->addConnectCommand( + new RawCommand( + 'AUTH', + [getenv('REDIS_PASSWORD') ?: constant('REDIS_PASSWORD')] + ) + ); $client = new Client($connection); $client->connect(); diff --git a/tests/Predis/Response/Iterator/MultiBulkTupleTest.php b/tests/Predis/Response/Iterator/MultiBulkTupleTest.php index 4bf5b78b..9f9ff0c9 100644 --- a/tests/Predis/Response/Iterator/MultiBulkTupleTest.php +++ b/tests/Predis/Response/Iterator/MultiBulkTupleTest.php @@ -14,6 +14,7 @@ namespace Predis\Response\Iterator; use Predis\Client; use Predis\ClientInterface; +use Predis\Command\RawCommand; use Predis\Connection\CompositeStreamConnection; use Predis\Connection\NodeConnectionInterface; use Predis\Protocol\Text\ProtocolProcessor as TextProtocolProcessor; @@ -125,6 +126,12 @@ class MultiBulkTupleTest extends PredisTestCase $protocol->useIterableMultibulk(true); $connection = new CompositeStreamConnection($parameters, $protocol); + $connection->addConnectCommand( + new RawCommand( + 'AUTH', + [getenv('REDIS_PASSWORD') ?: constant('REDIS_PASSWORD')] + ) + ); $client = new Client($connection); $client->connect();