diff --git a/lib/private/Setup/AbstractDatabase.php b/lib/private/Setup/AbstractDatabase.php index de09350513898..436c8ee697a1b 100644 --- a/lib/private/Setup/AbstractDatabase.php +++ b/lib/private/Setup/AbstractDatabase.php @@ -19,6 +19,12 @@ use Psr\Log\LoggerInterface; abstract class AbstractDatabase { + /** + * Installer options configuring an encrypted database connection. + * @var string[] + */ + protected const array CONNECTION_ENCRYPTION_OPTIONS = ['dbdriveroptions']; + protected string $dbUser; protected string $dbPassword; protected string $dbName; @@ -47,6 +53,13 @@ public function validate(array $config): array { if (substr_count($config['dbname'], '.') >= 1) { $errors[] = $this->trans->t('You cannot use dots in the database name %s', [$this->dbprettyname]); } + foreach (static::CONNECTION_ENCRYPTION_OPTIONS as $option) { + if (isset($config[$option]) && !is_array($config[$option])) { + // Fail instead of ignoring the option, otherwise the instance would be + // installed with an unencrypted connection without the admin noticing. + $errors[] = $this->trans->t('The database option "%1$s" for %2$s has to be a list of values', [$option, $this->dbprettyname]); + } + } return $errors; } @@ -62,11 +75,27 @@ public function initialize(array $config): void { // accept `false` both as bool and string, since setting config values from env will result in a string $this->tryCreateDbUser = $createUserConfig !== false && $createUserConfig !== 'false'; - $this->config->setValues([ + $configValues = [ 'dbname' => $dbName, 'dbhost' => $dbHost, 'dbtableprefix' => $dbTablePrefix, - ]); + ]; + + // An encrypted connection can only be configured through the system config, so the + // options have to be persisted before the first connection is opened. + foreach (static::CONNECTION_ENCRYPTION_OPTIONS as $option) { + if (empty($config[$option])) { + continue; + } + if (!is_array($config[$option])) { + // Rejected by validate() already, but subclasses may not use that check + $this->logger->error('Ignoring database option "{option}" passed to the installer because it is not a list of values', ['option' => $option]); + continue; + } + $configValues[$option] = $config[$option]; + } + + $this->config->setValues($configValues); $this->dbUser = $dbUser; $this->dbPassword = $dbPass; diff --git a/lib/private/Setup/PostgreSQL.php b/lib/private/Setup/PostgreSQL.php index cdb46010e6847..5b8ceb768f55e 100644 --- a/lib/private/Setup/PostgreSQL.php +++ b/lib/private/Setup/PostgreSQL.php @@ -16,6 +16,9 @@ class PostgreSQL extends AbstractDatabase { public $dbprettyname = 'PostgreSQL'; + // #[\Override] TODO: Uncomment this when we only support PHP 8.5+ support + protected const array CONNECTION_ENCRYPTION_OPTIONS = [...parent::CONNECTION_ENCRYPTION_OPTIONS, 'pgsql_ssl']; + /** * @throws DatabaseSetupException */ diff --git a/tests/lib/Setup/AbstractDatabaseTest.php b/tests/lib/Setup/AbstractDatabaseTest.php index 23560905fecec..70ccfd45f62c6 100644 --- a/tests/lib/Setup/AbstractDatabaseTest.php +++ b/tests/lib/Setup/AbstractDatabaseTest.php @@ -18,9 +18,16 @@ use Test\TestCase; class AbstractDatabaseTest extends TestCase { + /** + * Numeric literal instead of PDO::MYSQL_ATTR_SSL_CA: the constant is deprecated + * since PHP 8.5 and only defined when the MySQL driver is available. + */ + private const MYSQL_ATTR_SSL_CA = 1008; + private SystemConfig&MockObject $config; private ConnectionFactory&MockObject $connectionFactory; private Connection&MockObject $connection; + private LoggerInterface&MockObject $logger; private TestDatabase $database; #[\Override] @@ -30,11 +37,16 @@ protected function setUp(): void { $this->config = $this->createMock(SystemConfig::class); $this->connectionFactory = $this->createMock(ConnectionFactory::class); $this->connection = $this->createMock(Connection::class); + $this->logger = $this->createMock(LoggerInterface::class); + + $l10n = $this->createMock(IL10N::class); + $l10n->method('t') + ->willReturnCallback(fn (string $text, array $parameters = []) => vsprintf($text, $parameters)); $this->database = new TestDatabase( - $this->createMock(IL10N::class), + $l10n, $this->config, - $this->createMock(LoggerInterface::class), + $this->logger, $this->createMock(ISecureRandom::class), ); $this->database->connectionFactory = $this->connectionFactory; @@ -75,6 +87,100 @@ public function testInitializeFallsBackToLocalhost(): void { ]); } + /** + * The connection encryption options are only read from the system config, so they have + * to be persisted by initialize() - before any connection is opened by setupDatabase(). + */ + public function testInitializePersistsDriverOptions(): void { + $this->config->expects($this->once()) + ->method('setValues') + ->with([ + 'dbname' => 'nextcloud', + 'dbhost' => 'db.example.org', + 'dbtableprefix' => 'oc_', + 'dbdriveroptions' => [self::MYSQL_ATTR_SSL_CA => '/ca.pem'], + ]); + + $this->database->initialize($this->options([ + 'dbdriveroptions' => [self::MYSQL_ATTR_SSL_CA => '/ca.pem'], + ])); + } + + /** + * Only the options of the database being set up may be persisted, every database + * configures an encrypted connection differently. + */ + public function testInitializeSkipsOptionsOfOtherDatabases(): void { + $this->config->expects($this->once()) + ->method('setValues') + ->with([ + 'dbname' => 'nextcloud', + 'dbhost' => 'db.example.org', + 'dbtableprefix' => 'oc_', + ]); + + $this->database->initialize($this->options([ + 'pgsql_ssl' => ['mode' => 'verify-full'], + ])); + } + + public static function emptyEncryptionOptions(): array { + return [ + 'not provided' => [[]], + 'empty array' => [['dbdriveroptions' => []]], + 'null' => [['dbdriveroptions' => null]], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('emptyEncryptionOptions')] + public function testInitializeSkipsEmptyEncryptionOptions(array $additional): void { + $this->config->expects($this->once()) + ->method('setValues') + ->with([ + 'dbname' => 'nextcloud', + 'dbhost' => 'db.example.org', + 'dbtableprefix' => 'oc_', + ]); + + $this->database->initialize($this->options($additional)); + } + + /** + * A malformed option must never be persisted, as that would end up configuring an + * unencrypted connection while the admin expects an encrypted one. + */ + public function testInitializeRejectsMalformedEncryptionOptions(): void { + $this->config->expects($this->once()) + ->method('setValues') + ->with([ + 'dbname' => 'nextcloud', + 'dbhost' => 'db.example.org', + 'dbtableprefix' => 'oc_', + ]); + $this->logger->expects($this->once()) + ->method('error'); + + $this->database->initialize($this->options(['dbdriveroptions' => '/ca.pem'])); + } + + public function testValidateRejectsMalformedEncryptionOptions(): void { + $errors = $this->database->validate($this->options(['dbdriveroptions' => '/ca.pem'])); + + $this->assertEquals([ + 'The database option "dbdriveroptions" for Test has to be a list of values', + ], $errors); + } + + public function testValidateAcceptsEncryptionOptions(): void { + $errors = $this->database->validate($this->options([ + 'dbdriveroptions' => [self::MYSQL_ATTR_SSL_CA => '/ca.pem'], + // not an option of this database, so it is not validated either + 'pgsql_ssl' => 'verify-full', + ])); + + $this->assertEquals([], $errors); + } + /** * Host, database name and table prefix must not be passed as additional parameters: * they are resolved from the system config by the connection factory, so that setup diff --git a/tests/lib/Setup/PostgreSQLTest.php b/tests/lib/Setup/PostgreSQLTest.php new file mode 100644 index 0000000000000..335e7014b82e8 --- /dev/null +++ b/tests/lib/Setup/PostgreSQLTest.php @@ -0,0 +1,129 @@ + 'verify-full', + 'rootcert' => '/rootCA.crt', + 'cert' => '/client.crt', + 'key' => '/client.key', + ]; + + private SystemConfig&MockObject $config; + private LoggerInterface&MockObject $logger; + private PostgreSQL $database; + + #[\Override] + protected function setUp(): void { + parent::setUp(); + + $this->config = $this->createMock(SystemConfig::class); + $this->logger = $this->createMock(LoggerInterface::class); + + $l10n = $this->createMock(IL10N::class); + $l10n->method('t') + ->willReturnCallback(fn (string $text, array $parameters = []) => vsprintf($text, $parameters)); + + $this->database = new PostgreSQL( + $l10n, + $this->config, + $this->logger, + $this->createMock(ISecureRandom::class), + ); + } + + /** + * PostgreSQL is configured through its own set of connection parameters instead of PDO + * driver options. They are only read from the system config, so they have to be + * persisted by initialize() - before any connection is opened by setupDatabase(). + */ + public function testInitializePersistsPgsqlSsl(): void { + $this->config->expects($this->once()) + ->method('setValues') + ->with([ + 'dbname' => 'nextcloud', + 'dbhost' => 'db.example.org', + 'dbtableprefix' => 'oc_', + 'pgsql_ssl' => self::PGSQL_SSL, + ]); + + $this->database->initialize($this->options(['pgsql_ssl' => self::PGSQL_SSL])); + } + + public static function emptyPgsqlSsl(): array { + return [ + 'not provided' => [[]], + 'empty array' => [['pgsql_ssl' => []]], + 'null' => [['pgsql_ssl' => null]], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('emptyPgsqlSsl')] + public function testInitializeSkipsEmptyPgsqlSsl(array $additional): void { + $this->config->expects($this->once()) + ->method('setValues') + ->with([ + 'dbname' => 'nextcloud', + 'dbhost' => 'db.example.org', + 'dbtableprefix' => 'oc_', + ]); + + $this->database->initialize($this->options($additional)); + } + + /** + * A malformed option must never be persisted, as that would end up configuring an + * unencrypted connection while the admin expects an encrypted one. + */ + public function testInitializeRejectsMalformedPgsqlSsl(): void { + $this->config->expects($this->once()) + ->method('setValues') + ->with([ + 'dbname' => 'nextcloud', + 'dbhost' => 'db.example.org', + 'dbtableprefix' => 'oc_', + ]); + $this->logger->expects($this->once()) + ->method('error'); + + $this->database->initialize($this->options(['pgsql_ssl' => 'verify-full'])); + } + + public function testValidateRejectsMalformedPgsqlSsl(): void { + $errors = $this->database->validate($this->options(['pgsql_ssl' => 'verify-full'])); + + $this->assertEquals([ + 'The database option "pgsql_ssl" for PostgreSQL has to be a list of values', + ], $errors); + } + + public function testValidateAcceptsPgsqlSsl(): void { + $errors = $this->database->validate($this->options(['pgsql_ssl' => self::PGSQL_SSL])); + + $this->assertEquals([], $errors); + } + + private function options(array $additional = []): array { + return array_merge([ + 'dbuser' => 'admin', + 'dbpass' => 'admin-password', + 'dbname' => 'nextcloud', + 'dbhost' => 'db.example.org', + ], $additional); + } +} diff --git a/tests/lib/Setup/TestDatabase.php b/tests/lib/Setup/TestDatabase.php index 1432ab24eee6a..f984afaa99681 100644 --- a/tests/lib/Setup/TestDatabase.php +++ b/tests/lib/Setup/TestDatabase.php @@ -1,12 +1,11 @@