Implement reading db password from a file

This is useful for systems where configuration files might be(come) public or
world readable and thus secrets must be kept separately in files. Examples are
NixOS or Docker (secrets).

Author: Alexey Abel

README.md

@@ -49,16 +49,32 @@ This app has no user interface. All configuration is done via Nextcloud's system
 
 
 There are three types of configuration parameters:
+
 ### 1. Database
-that *User Backend SQL Raw* will connect to
-- *db_type* is optional and defaults to `postgresql`. The only other valid non-empty value is `mariadb`, which can be used for MySQL, too.
-- *db_host* is optional and defaults to `localhost`
-- *db_port* is optional and defaults to `5432`
-- *db_name*, *db_user* and *db_password* are mandatory
-- *mariadb_charset* sets the charset for mariadb connections, is optional and defaults to `utf8mb4`
+
+that *User Backend SQL Raw* will connect to.
+
+| key                | value                                   | default value |
+| ------------------ | --------------------------------------- | ------------- |
+| `db_type`          | `postgresql` or `mariadb`               | `postgresql`  |
+| `db_host`          | your db host                            | `localhost`   |
+| `db_port`          | your db port                            | `5432`        |
+| `db_name`          | your db name                            |               |
+| `db_user`          | your db user                            |               |
+| `db_password`      | your db password                        |               |
+| `db_password_file` | path to file containing the db password |               |
+| `mariadb_charset`  | the charset for mariadb connections     | `utf8mb4`     |
+
+* Values without a default value are mandatory, except that
+	* only one of `db_password` or `db_passowrd_file` must be set.
+* Only the first line of the file specified by `db_passowrd_file` is read.
+	* Not more than 100 characters of the first line are read.
+  * Whitespace-like characters are [stripped](https://www.php.net/manual/en/function.trim.php) from the beginning and end of the read password.
 
 ### 2. SQL Queries
-that will be used to read/write data
+
+that will be used to read/write data.
+
 - queries use named parameters. You have to use the exact names as shown in the examples. For
  example, to retrieve the hash for a user, the query named `get_password_hash_for_user` will be 
  used. Write your custom SQL query and simply put `:username` where you are referring to 
@@ -67,8 +83,7 @@ that will be used to read/write data
  leave the query `get_home` commented. This app will recognize 
  this and [communicate](https://docs.nextcloud.com/server/13/developer_manual/api/OCP/UserInterface.html#OCP\UserInterface::implementsActions) to Nextcloud that this feature is not available.
     - `user_exists` and `get_users` are required, the rest is optional.
-    -  For user authentication (i.e. login) you need at least `get_password_hash_for_user`, 
-	`user_exists` and `get_users`.
+    -  For user authentication (i.e. login) you need at least `get_password_hash_for_user`, `user_exists` and `get_users`.
 
  - For all queries that read data, only the first column is interpreted.
  - Two queries require a little bit of attention:
@@ -84,7 +99,9 @@ that will be used to read/write data
     [prepare()](http://php.net/manual/en/pdo.prepare.php) method of a PDO object.
 
 ### 3. Hash Algorithm For New Passwords
-used for the creation of new passwords
+
+used for the creation of new passwords.
+
 - is optional and, if you leave it empty, defaults to `bcrypt` ($2y$).
 - Other supported hash algorithms are MD5-CRYPT, SHA-256-CRYPT, SHA-512-CRYPT, Argon2i and Argon2id. 
 The config values are `md5`, `sha256`, `sha512`, `argon2i`, `argon2id` respectively, e.g. 
@@ -100,13 +117,15 @@ The config values are `md5`, `sha256`, `sha512`, `argon2i`, `argon2id` respectiv
 
 
 ## Security
+
 - Password length is limited to 100 characters to prevent denial of service attacks against the 
 web server. Without a limit, malicious users could feed your Nextcloud instance with passwords that have a length of tens of thousands of characters, which could cause a very
  high load due to expensive password hashing operations.
 - The username during user creation (`create_user`) and the display name (`set_display_name`) are
  not limited in length. You should limit this on the db layer.
 
 ## Troubleshooting
+
 - **TL;DR**: check the log file
 - This app has no UI, therefore all error output (exceptions and explicit logs) is written to [Nextcloud's log](https://docs.nextcloud.com/server/20/admin_manual/configuration_server/logging_configuration.html), 
 by default  */var/www/nextcloud/data/nextcloud.log* or */var/log/syslog*. Log level 3 is sufficient for all non-debug output.

lib/Config.php

@@ -24,6 +24,7 @@
 use Psr\Log\LoggerInterface;
 use \OCP\IConfig;
 
+
 class Config {
 
 	const DEFAULT_DB_TYPE = 'postgresql';
@@ -42,6 +43,7 @@ class Config {
 	const CONFIG_KEY_DB_NAME = 'db_name';
 	const CONFIG_KEY_DB_USER = 'db_user';
 	const CONFIG_KEY_DB_PASSWORD = 'db_password';
+	const CONFIG_KEY_DB_PASSWORD_FILE = 'db_password_file';
 	const CONFIG_KEY_MARIADB_CHARSET = 'mariadb_charset';
 	const CONFIG_KEY_HASH_ALGORITHM_FOR_NEW_PASSWORDS = 'hash_algorithm_for_new_passwords';
 
@@ -61,9 +63,16 @@ class Config {
 	private $logger;
 	private $appConfiguration;
 
+	/*
+ 	* Design decision: Judging from the Nextcloud debug logs the Config class is
+ 	* constructed at least as often as queries to the DB are made. Therefore,
+ 	* reading all config options once and then returning them from the runtime
+ 	* object would not yield any performance advantage. On the contrary, most
+ 	* options would be read but never used. So, lazy loading all options seems
+ 	* to be the better way.
+	*/
 	public function __construct(LoggerInterface $logger, IConfig $nextCloudConfiguration) {
 		$this->logger = $logger;
-
 		$this->appConfiguration = $nextCloudConfiguration->getSystemValue(self::CONFIG_KEY);
 		if (empty($this->appConfiguration)) {
 			throw new \UnexpectedValueException('The Nextcloud '
@@ -128,9 +137,53 @@ public function getDbUser() {
 
 	/**
 	 * @return string password of db user
+	 * @throws \UnexpectedValueException
 	 */
 	public function getDbPassword() {
-		return $this->getConfigValueOrThrowException(self::CONFIG_KEY_DB_PASSWORD);
+
+		$password = $this->getConfigValueOrFalse(self::CONFIG_KEY_DB_PASSWORD);
+		$passwordFilePath = $this->getConfigValueOrFalse(self::CONFIG_KEY_DB_PASSWORD_FILE);
+
+		$passwordIsSet = $password !== FALSE;
+		$passwordFileIsSet = $passwordFilePath !== FALSE;
+
+		if ($passwordIsSet === $passwordFileIsSet) { // expression is a "not XOR"
+			throw new \UnexpectedValueException('Exactly one of ' . self::CONFIG_KEY_DB_PASSWORD . ' or ' . self::CONFIG_KEY_DB_PASSWORD_FILE . ' must be set (not be empty) in the config.');
+		}
+
+		if ($passwordIsSet) {
+			$this->logger->debug("Will use db password specified directly in config.php.");
+			return $password;
+		}
+
+		if ($passwordFileIsSet) {
+			$this->logger->debug("Will use db password stored in file " . $passwordFilePath). ".";
+			$error_message_prefix = "Specified db password file with path {$passwordFilePath}";
+
+			if (!file_exists($passwordFilePath)) {
+				throw new \UnexpectedValueException("{$error_message_prefix} does not exist or is not accessible.");
+			}
+			if (is_link($passwordFilePath)) {
+				throw new \UnexpectedValueException("{$error_message_prefix} is a symbolic link, which might be a security problem and is therefore not allowed.");
+			}
+			if (is_dir($passwordFilePath)) {
+				throw new \UnexpectedValueException("{$error_message_prefix} is a directory but I need a file to read the password.");
+			}
+			$file = fopen($passwordFilePath, "r");
+			if ($file === FALSE) {
+				throw new \UnexpectedValueException("{$error_message_prefix} can not be opened. Maybe insufficient permissions?");
+			}
+			// + 1 because fgets() reads one less byte than specified and we want to keep the promise of reading 100 bytes
+			$first_line = fgets($file, self::MAXIMUM_ALLOWED_PASSWORD_LENGTH + 1);
+			if ($first_line === FALSE) {
+				fclose($file);
+				throw new \UnexpectedValueException("{$error_message_prefix} was opened but the first line could not be read.");
+			}
+			fclose($file);
+			$this->logger->debug("Successfully read db password from file " . $passwordFilePath). ".";
+			return trim($first_line);
+		}
+
 	}
 
 	/**
@@ -257,24 +310,39 @@ private function getConfigValueOrDefaultValue($configKey, $defaultValue) {
 	}
 
 	/**
-	 * Tries to read a config value (query) and if it is set returns its value,
+	 * Tries to read a config value and if it is set returns its value,
 	 * otherwise returns FALSE. This is used for optional configuration keys
 	 * where default values are not known, i.e. SQL queries.
 	 * @param $configKey string key name of configuration parameter
 	 * @return string|bool value of configuration parameter or false if it is
 	 * not set
 	 */
-	private function getQueryStringOrFalse ($configKey) {
-		$queryArray = $this->getConfigValueOrThrowException(self::CONFIG_KEY_QUERIES);
-
-		if (empty($queryArray[$configKey])) {
+	private function getConfigValueOrFalse ($configKey) {
+		if (empty($this->appConfiguration[$configKey])) {
 			return FALSE;
 		}
 		else {
-			return $queryArray[$configKey];
+			return $this->appConfiguration[$configKey];
 		}
 	}
 
+	private function getValueOrFalse ($value) {
+		return empty($value) ? FALSE : $value;
+	}
+
+
+	/**
+	 * Tries to read a query value and if it is set returns its value,
+	 * otherwise returns FALSE.
+	 * @param $configKey string key name of configuration parameter
+	 * @return string|bool value of configuration parameter or false if it is
+	 * not set
+	 */
+	private function getQueryStringOrFalse ($configKey) {
+		$queryArray = $this->getConfigValueOrThrowException(self::CONFIG_KEY_QUERIES);
+		return $this->getValueOrFalse($queryArray[$configKey] ?? FALSE);
+	}
+
 	/**
 	 * @param $dbType string db descriptor to check
 	 * @return bool whether the db is supported

tests/Unit/ConfigTest.php

@@ -72,6 +72,21 @@ public function testThrowsExceptionIfMandatorySettingIsEmpty() {
 		$config->getDbName();
 	}
 
+	public function testThrowsExceptionIfDbPasswordAndDbPasswordFileAreBothSet() {
+		$this->nextcloudConfigStub->method('getSystemValue')
+			->willReturn(array(
+				'db_password' => 'such_secret',
+				// Specify a file that is always there, so that test does not fail due to missing db password file.
+				'db_password_file' => '/dev/zero'
+			));
+
+		$this->expectException(\UnexpectedValueException::class);
+		$config = new Config($this->logStub, $this->nextcloudConfigStub);
+		$config->getDbPassword();
+	}
+
+
+
 	// Tests that check if default values are uses correctly
 
 	public function testDefaultDbTypeIsUsedWhenThatParameterIsNotSet() {
@@ -162,7 +177,7 @@ public function testEmptyQueryParameterReturnsFalse() {
 
 		$config = new Config($this->logStub, $this->nextcloudConfigStub);
 
-		$actualReturnValue = $config->getQueryUserExists();
+		$actualReturnValue = $config->getQueryGetUsers();
 		self::assertSame(FALSE, $actualReturnValue);
 	}
 
@@ -220,6 +235,36 @@ public function testDbPortIsReturnedWhenThisParameterIsSet() {
 		self::assertEquals($expectedPort, $actualPort);
 	}
 
+	public function testDBPasswordFromPasswordFileIsTrimmedAndReturned() {
+
+		$expectedPassword = 'very-secret 909!&äßZ';
+
+		$db_password_file = tempnam("/tmp", "user_backend_sql_raw-db_password_file");
+		if($db_password_file === FALSE) {
+			self::fail("Temporary db password file could not be created.");
+		}
+
+		$file = fopen($db_password_file, "w");
+		if($file === FALSE) {
+		self::fail("Temporary db password file could not be opened for writing.");
+		}
+
+		// add whitespace at the end which will be trimmed
+		fwrite($file, "{$expectedPassword} ");
+		fclose($file);
+
+		$this->nextcloudConfigStub->method('getSystemValue')
+			->willReturn(array(
+				'db_password_file' => $db_password_file
+			));
+
+		$config = new Config($this->logStub, $this->nextcloudConfigStub);
+
+		$actualPassword = $config->getDbPassword();
+		unlink($db_password_file);
+		self::assertEquals($expectedPassword, $actualPassword);
+	}
+
 	public function testMariaDbCharsetIsReturnedWhenThisParameterIsSet() {
 		$this->nextcloudConfigStub->method('getSystemValue')
 			->willReturn(array(