From a5e0d05957b2d2397b2463253257296245c8d42a Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Tue, 1 Sep 2026 14:55:04 +0200 Subject: [PATCH 1/8] chore(tests): add OC\Core\Command\Base stub The upcoming security self-test command extends OC\Core\Command\Base to get the --output handling and the OUTPUT_FORMAT_* constants. Base is a private server class and is therefore not part of the nextcloud/ocp package this app depends on, and symfony/console is only provided by the server at runtime, not by this app's vendor tree. Psalm runs at errorLevel 1 and would report UndefinedClass for all of them. Add scan-only stubs mirroring the existing tests/stubs layout, declaring just the members this app uses. They are picked up by psalm through the extraFiles entry that already covers tests/stubs. At runtime the real classes always win: the server registers its autoloaders in lib/base.php long before an app's vendor autoloader, which is the same arrangement the existing OCA\Settings\Mailer\NewUserMailHelper stub relies on. Signed-off-by: Misha M.-Kupriyanov --- tests/stubs/OC/Core/Command/Base.php | 85 +++++++++++++++++++ .../Console/Input/InputInterface.php | 26 ++++++ .../Component/Console/Input/InputOption.php | 22 +++++ .../Console/Output/ConsoleOutputInterface.php | 18 ++++ .../Console/Output/OutputInterface.php | 26 ++++++ 5 files changed, 177 insertions(+) create mode 100644 tests/stubs/OC/Core/Command/Base.php create mode 100644 tests/stubs/Symfony/Component/Console/Input/InputInterface.php create mode 100644 tests/stubs/Symfony/Component/Console/Input/InputOption.php create mode 100644 tests/stubs/Symfony/Component/Console/Output/ConsoleOutputInterface.php create mode 100644 tests/stubs/Symfony/Component/Console/Output/OutputInterface.php diff --git a/tests/stubs/OC/Core/Command/Base.php b/tests/stubs/OC/Core/Command/Base.php new file mode 100644 index 0000000..6d60b10 --- /dev/null +++ b/tests/stubs/OC/Core/Command/Base.php @@ -0,0 +1,85 @@ + $messages + */ + public function writeln($messages, int $options = 0): void; + + /** + * @param string|iterable $messages + */ + public function write($messages, bool $newline = false, int $options = 0): void; +} From fb702b26c78f35ac236c92ddb7871f3e2fcf0287 Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Tue, 1 Sep 2026 15:03:56 +0200 Subject: [PATCH 2/8] feat(security): add ncw_tools:security:selftest occ command C5 control PSS-07 requires evidence that this instance hashes passwords with argon2id. Add an occ command that collects that evidence and emits it as a structured artifact, plus the two lib classes behind it. occ ncw_tools:security:selftest [--round-trip] [--sample-size=N] [--output=plain|json|json_pretty] exit 0 = PASS, 1 = FAIL, 2 = usage error HashAlgorithm classifies a stored hash. This is the part an earlier proposal got wrong: Nextcloud does not store a bare password_hash() string. OC\Security\Hasher::hash() prepends a hasher version and a pipe, so a stored value reads 3|$argon2id$v=19$m=65536,t=4,p=1$$ where version 3 is argon2id, 2 is argon2i and 1 is bcrypt. Comparing a stored value against the literal prefix $argon2id$ can therefore never match, no matter how the instance is configured. HashAlgorithm splits the version prefix off first, mirroring the private Hasher::splitHash(), and then asks password_get_info() instead of matching strings by hand. Unprefixed legacy hashes (60 char bcrypt, 40 char sha1 hex) and empty passwords get their own classes, so a dormant account stays distinguishable from a downgraded configuration. SecuritySelfTest is the collector. It returns a plain array and writes no output of its own, so the command and the tests share one code path. It runs three checks plus an optional round trip: - configured_algorithm: hash a random probe through IHasher and classify the result. This is the algorithm every new password gets. - stored_distribution: count the surveyed rows of the users table per algorithm, honouring the sample size (0 = all). Rows with no local password are tolerated (SSO-only accounts); anything that is neither argon2id nor empty fails the survey. - security_config: read the hardening switches through IConfig, so the effective merged configuration is asserted rather than a single file. hashing_default_password is the real downgrade switch -- Hasher::getPrefferedAlgorithm() returns PASSWORD_DEFAULT as soon as it is true. passwordsalt and secret are asserted as presence only. - round_trip (opt-in): create ncw-selftest- with a password covering all four character classes so password_policy accepts it, never set an email address, read the stored hash back through the query builder, and delete the user in a finally. cleaned_up is re-checked by resolving the uid again, and a surviving probe user fails the check. Security invariant, enforced by review: no field of the artifact may carry hash material, a salt or a secret. Only algorithm names, counts, booleans and cost parameters are reported. The cost parameters come from password_get_info() on the probe hash rather than from the hashing* config keys, because the hasher clamps those to the algorithm minimums -- the probe reports the effective values, which is the stronger evidence. The service takes a LoggerInterface alongside the five collaborators because the artifact schema is fixed and has no field for a reason string: a round trip that fails has to explain itself in the log. The command keeps stdout free of everything but the artifact, because the deployment wrapper pipes it straight into jq. Diagnostics go to the console's error output, and the artifact is logged once at info level with itself as structured context, which is what reaches Kibana under log_type=errorlog. A FAIL writes the complete artifact before exiting 1 -- the failure case is exactly what the evidence needs to capture -- while a usage error writes no artifact at all. The classifier, the service and the command land together because psalm runs with findUnusedCode and the only entry point is registered in appinfo/info.xml, which psalm cannot see; splitting them would leave an intermediate commit failing static analysis. Signed-off-by: Misha M.-Kupriyanov --- appinfo/info.xml | 3 + lib/Command/SecuritySelfTest.php | 200 ++++++++++++++++ lib/Security/HashAlgorithm.php | 126 ++++++++++ lib/Security/SecuritySelfTest.php | 368 ++++++++++++++++++++++++++++++ psalm.xml | 4 + 5 files changed, 701 insertions(+) create mode 100644 lib/Command/SecuritySelfTest.php create mode 100644 lib/Security/HashAlgorithm.php create mode 100644 lib/Security/SecuritySelfTest.php diff --git a/appinfo/info.xml b/appinfo/info.xml index eb6bb1e..17e6ec1 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -14,4 +14,7 @@ + + OCA\NcwTools\Command\SecuritySelfTest + diff --git a/lib/Command/SecuritySelfTest.php b/lib/Command/SecuritySelfTest.php new file mode 100644 index 0000000..29ad8d5 --- /dev/null +++ b/lib/Command/SecuritySelfTest.php @@ -0,0 +1,200 @@ +setName(Application::APP_ID . ':security:selftest') + ->setDescription('Verify that password hashing is argon2id and emit an evidence artifact') + ->addOption( + 'round-trip', + null, + InputOption::VALUE_NONE, + 'Create and delete a disposable probe user to observe the algorithm actually written to the database', + ) + ->addOption( + 'sample-size', + null, + InputOption::VALUE_REQUIRED, + 'Number of stored password hashes to survey (0 = all)', + (string)SecuritySelfTestService::DEFAULT_SAMPLE_SIZE, + ); + } + + protected function execute(InputInterface $input, OutputInterface $output): int { + $errors = $this->errorOutput($output); + + $format = $this->outputFormat($input); + if ($format === null) { + $errors->writeln('Invalid --output format, expected one of: ' . implode(', ', self::OUTPUT_FORMATS) . ''); + return 2; + } + + $sampleSize = $this->sampleSize($input); + if ($sampleSize === null) { + $errors->writeln('Invalid --sample-size, expected a non-negative integer (0 = all)'); + return 2; + } + + $report = $this->selfTest->run($input->getOption('round-trip') === true, $sampleSize); + + // The structured context is what reaches Kibana; the deployment sets + // log_type=errorlog, so the context is serialised alongside the message. + $this->logger->info(self::LOG_MESSAGE, $report); + + // A FAIL is the case the evidence exists for, so the complete artifact is + // written before the non-zero exit — never a short-circuited error path. + if ($format === self::OUTPUT_FORMAT_PLAIN) { + $this->writePlain($output, $report); + } else { + $flags = JSON_UNESCAPED_SLASHES | ($format === self::OUTPUT_FORMAT_JSON_PRETTY ? JSON_PRETTY_PRINT : 0); + $json = json_encode($report, $flags); + if ($json === false) { + $errors->writeln('Could not encode the evidence artifact: ' . json_last_error_msg() . ''); + return 1; + } + $output->writeln($json); + } + + return $report['result'] === SecuritySelfTestService::RESULT_PASS ? 0 : 1; + } + + /** + * Diagnostics must not land on stdout: under `--output=json` the deployment + * wrapper pipes stdout straight into `jq`, and a single stray line there + * costs the whole evidence artifact. + */ + private function errorOutput(OutputInterface $output): OutputInterface { + return $output instanceof ConsoleOutputInterface ? $output->getErrorOutput() : $output; + } + + /** + * @param array{ + * schema_version: string, + * timestamp: string, + * result: string, + * instance: array{id: string, url: string, name: string, namespace: string, environment: string}, + * password_hashing: array{ + * result: string, + * configured_algorithm: string, + * round_trip: array{result: string, stored_algorithm: string|null, cleaned_up: bool|null}, + * stored_distribution: array + * }, + * security_config: array{ + * result: string, + * checks: list, + * parameters: array + * } + * } $report + */ + private function writePlain(OutputInterface $output, array $report): void { + $output->writeln('schema_version: ' . $report['schema_version']); + $output->writeln('timestamp: ' . $report['timestamp']); + $output->writeln('result: ' . $report['result']); + $output->writeln('instance:'); + foreach ($report['instance'] as $key => $value) { + $output->writeln(' ' . $key . ': ' . $value); + } + + $hashing = $report['password_hashing']; + $output->writeln('password_hashing: ' . $hashing['result']); + $output->writeln(' configured_algorithm: ' . $hashing['configured_algorithm']); + $output->writeln(' round_trip: ' . $hashing['round_trip']['result']); + $output->writeln(' stored_algorithm: ' . ($hashing['round_trip']['stored_algorithm'] ?? '-')); + $output->writeln(' cleaned_up: ' . $this->formatValue($hashing['round_trip']['cleaned_up'] ?? '-')); + $output->writeln(' stored_distribution:'); + foreach ($hashing['stored_distribution'] as $algorithm => $count) { + $output->writeln(' ' . $algorithm . ': ' . $count); + } + + $security = $report['security_config']; + $output->writeln('security_config: ' . $security['result']); + foreach ($security['checks'] as $check) { + $output->writeln(sprintf( + ' %s: %s (expected %s, actual %s)', + $check['key'], + $check['result'], + $this->formatValue($check['expected']), + $this->formatValue($check['actual']), + )); + } + $output->writeln(' parameters:'); + foreach ($security['parameters'] as $name => $value) { + $output->writeln(' ' . $name . ': ' . $value); + } + } + + private function formatValue(bool|string $value): string { + if (is_bool($value)) { + return $value ? 'true' : 'false'; + } + + return $value; + } + + private function outputFormat(InputInterface $input): ?string { + $format = $input->getOption('output'); + if (!is_string($format) || !in_array($format, self::OUTPUT_FORMATS, true)) { + return null; + } + + return $format; + } + + private function sampleSize(InputInterface $input): ?int { + $sampleSize = $input->getOption('sample-size'); + if (is_int($sampleSize)) { + return $sampleSize >= 0 ? $sampleSize : null; + } + + if (!is_string($sampleSize) || $sampleSize === '' || !ctype_digit($sampleSize)) { + return null; + } + + return (int)$sampleSize; + } +} diff --git a/lib/Security/HashAlgorithm.php b/lib/Security/HashAlgorithm.php new file mode 100644 index 0000000..ce7d4e9 --- /dev/null +++ b/lib/Security/HashAlgorithm.php @@ -0,0 +1,126 @@ +$ + * + * Version 3 is argon2id, version 2 is argon2i, version 1 is bcrypt. Comparing a + * stored value against the literal prefix `$argon2id$` therefore never matches — + * the version prefix has to be split off first, exactly as the private + * `Hasher::splitHash()` does. + * + * Hashes written before the version prefix existed are stored unprefixed: a + * 60 character bcrypt string or a 40 character sha1 hex digest. + * + * No method of this class ever returns hash material, a salt or a secret — + * only algorithm names and cost parameters. + */ +final class HashAlgorithm { + public const ARGON2ID = 'argon2id'; + public const ARGON2I = 'argon2i'; + public const BCRYPT = 'bcrypt'; + public const LEGACY_BCRYPT = 'legacy-bcrypt'; + public const LEGACY_SHA1 = 'legacy-sha1'; + public const EMPTY = 'empty'; + public const UNKNOWN = 'unknown'; + + /** Length of an unprefixed legacy bcrypt/PHPass hash. */ + private const LEGACY_BCRYPT_LENGTH = 60; + + /** Length of an unprefixed legacy sha1 hex digest. */ + private const LEGACY_SHA1_LENGTH = 40; + + /** + * @param string $stored The raw value of the `oc_users.password` column + * @return self::ARGON2ID|self::ARGON2I|self::BCRYPT|self::LEGACY_BCRYPT|self::LEGACY_SHA1|self::EMPTY|self::UNKNOWN + */ + public static function fromStoredHash(string $stored): string { + if ($stored === '') { + return self::EMPTY; + } + + $hash = self::stripVersionPrefix($stored); + if ($hash !== null) { + /** @var string $algoName `password_get_info()` always reports a name, 'unknown' included. */ + $algoName = password_get_info($hash)['algoName']; + + return self::fromAlgoName($algoName); + } + + if (strlen($stored) === self::LEGACY_BCRYPT_LENGTH && str_starts_with($stored, '$2')) { + return self::LEGACY_BCRYPT; + } + + if (strlen($stored) === self::LEGACY_SHA1_LENGTH && ctype_xdigit($stored)) { + return self::LEGACY_SHA1; + } + + return self::UNKNOWN; + } + + /** + * The cost parameters the given hash was produced with, as reported by + * `password_get_info()` — `memory_cost`/`time_cost`/`threads` for argon2, + * `cost` for bcrypt. These are the *effective* values, which is stronger + * evidence than the `hashing*` config keys because the hasher clamps those + * to the algorithm minimums. + * + * Returns an empty array for legacy, empty or unrecognised hashes. + * + * @return array + */ + public static function parametersFromStoredHash(string $stored): array { + $hash = self::stripVersionPrefix($stored) ?? $stored; + + // password_get_info() reports argon2's memory_cost/time_cost/threads and + // bcrypt's cost as integers, and an empty array for anything it does not + // recognise. It never reports hash material. + /** @var array $options */ + $options = password_get_info($hash)['options']; + + return $options; + } + + /** + * Splits `|` and returns the hash part, mirroring the private + * `OC\Security\Hasher::splitHash()`. Returns null when the value carries no + * version prefix, i.e. when it is a legacy hash. + */ + private static function stripVersionPrefix(string $stored): ?string { + $parts = explode('|', $stored, 2); + if (count($parts) !== 2) { + return null; + } + + if ((int)$parts[0] <= 0) { + return null; + } + + return $parts[1]; + } + + /** + * @return self::ARGON2ID|self::ARGON2I|self::BCRYPT|self::UNKNOWN + */ + private static function fromAlgoName(string $algoName): string { + return match ($algoName) { + 'argon2id' => self::ARGON2ID, + 'argon2i' => self::ARGON2I, + 'bcrypt' => self::BCRYPT, + default => self::UNKNOWN, + }; + } +} diff --git a/lib/Security/SecuritySelfTest.php b/lib/Security/SecuritySelfTest.php new file mode 100644 index 0000000..bacfb50 --- /dev/null +++ b/lib/Security/SecuritySelfTest.php @@ -0,0 +1,368 @@ + + * }, + * security_config: array{ + * result: self::RESULT_PASS|self::RESULT_FAIL, + * checks: list, + * parameters: array + * } + * } + */ + public function run(bool $roundTrip = false, int $sampleSize = self::DEFAULT_SAMPLE_SIZE): array { + // One probe hash serves both checks: it reveals the algorithm every new + // password gets, and the cost parameters it was actually produced with. + // The probe hash itself is never reported. + $probeHash = $this->hasher->hash($this->probeMessage()); + + $passwordHashing = $this->checkPasswordHashing($probeHash, $roundTrip, $sampleSize); + $securityConfig = $this->checkSecurityConfig($probeHash); + + return [ + 'schema_version' => self::SCHEMA_VERSION, + 'timestamp' => gmdate('Y-m-d\TH:i:s\Z'), + 'result' => $this->verdict( + $passwordHashing['result'] === self::RESULT_PASS + && $securityConfig['result'] === self::RESULT_PASS, + ), + 'instance' => $this->describeInstance(), + 'password_hashing' => $passwordHashing, + 'security_config' => $securityConfig, + ]; + } + + /** + * @return array{ + * result: self::RESULT_PASS|self::RESULT_FAIL, + * configured_algorithm: string, + * round_trip: array{result: string, stored_algorithm: string|null, cleaned_up: bool|null}, + * stored_distribution: array + * } + */ + private function checkPasswordHashing(string $probeHash, bool $roundTrip, int $sampleSize): array { + $configuredAlgorithm = HashAlgorithm::fromStoredHash($probeHash); + $distribution = $this->surveyStoredHashes($sampleSize); + $roundTripReport = $roundTrip + ? $this->runRoundTrip() + : ['result' => self::RESULT_SKIPPED, 'stored_algorithm' => null, 'cleaned_up' => null]; + + $passed = $configuredAlgorithm === self::EXPECTED_ALGORITHM + && $this->surveyPassed($distribution) + && $roundTripReport['result'] !== self::RESULT_FAIL; + + return [ + 'result' => $this->verdict($passed), + 'configured_algorithm' => $configuredAlgorithm, + 'round_trip' => $roundTripReport, + 'stored_distribution' => $distribution, + ]; + } + + /** + * Counts stored hashes per algorithm. Only counts are returned — never a hash. + * + * @return array + */ + private function surveyStoredHashes(int $sampleSize): array { + $counts = array_fill_keys(self::REPORTED_BUCKETS, 0); + + $query = $this->db->getQueryBuilder(); + // Table name without prefix on purpose: the query builder adds `oc_`. + $query->select('password')->from('users'); + if ($sampleSize > 0) { + $query->setMaxResults($sampleSize); + } + + $result = $query->executeQuery(); + try { + while (($stored = $this->nextStoredHash($result)) !== null) { + $algorithm = HashAlgorithm::fromStoredHash($stored); + $counts[$algorithm] = ($counts[$algorithm] ?? 0) + 1; + } + } finally { + $result->closeCursor(); + } + + return $counts; + } + + /** + * The next `password` value of the result, or null once it is exhausted. + * + * `IResult::fetchAssociative()` would be typed, but it only exists since + * Nextcloud 33 and this app supports 31. + */ + private function nextStoredHash(IResult $result): ?string { + /** @var array|false $row */ + $row = $result->fetch(); + if ($row === false) { + return null; + } + + return $this->asString($row['password'] ?? null); + } + + /** + * @param array $distribution + */ + private function surveyPassed(array $distribution): bool { + foreach ($distribution as $algorithm => $count) { + if ($count > 0 && !in_array($algorithm, self::TOLERATED_BUCKETS, true)) { + return false; + } + } + + return true; + } + + /** + * Hashes a throwaway password through the real user pipeline and reads back + * what landed in the database, then removes the probe user again. + * + * @return array{result: string, stored_algorithm: string|null, cleaned_up: bool|null} + */ + private function runRoundTrip(): array { + $uid = self::ROUND_TRIP_UID_PREFIX . $this->random->generate(12, ISecureRandom::CHAR_ALPHANUMERIC); + $user = null; + $storedAlgorithm = null; + + try { + // Deliberately no email address: a disposable probe account must not + // be able to receive mail or a password reset link. + $user = $this->userManager->createUser($uid, $this->probePassword()); + if ($user === false) { + $this->logger->error('SecuritySelfTest: could not create the round-trip probe user', ['uid' => $uid]); + } else { + $storedAlgorithm = HashAlgorithm::fromStoredHash($this->readStoredHash($uid)); + } + } catch (\Throwable $e) { + // Message only, never the exception: deep frames can carry the + // throwaway password in their stack-trace arguments. + $this->logger->error('SecuritySelfTest: round-trip probe failed', [ + 'uid' => $uid, + 'exceptionClass' => $e::class, + 'message' => $e->getMessage(), + ]); + } finally { + if ($user !== null && $user !== false) { + try { + $user->delete(); + } catch (\Throwable $e) { + $this->logger->error('SecuritySelfTest: could not delete the round-trip probe user', [ + 'uid' => $uid, + 'exceptionClass' => $e::class, + 'message' => $e->getMessage(), + ]); + } + } + } + + $cleanedUp = $this->userManager->get($uid) === null; + if (!$cleanedUp) { + $this->logger->error('SecuritySelfTest: round-trip probe user still exists', ['uid' => $uid]); + } + + return [ + 'result' => $this->verdict($storedAlgorithm === self::EXPECTED_ALGORITHM && $cleanedUp), + 'stored_algorithm' => $storedAlgorithm, + 'cleaned_up' => $cleanedUp, + ]; + } + + private function readStoredHash(string $uid): string { + $query = $this->db->getQueryBuilder(); + $query->select('password') + ->from('users') + ->where($query->expr()->eq('uid', $query->createNamedParameter($uid))); + + $result = $query->executeQuery(); + try { + return $this->asString($result->fetchOne()); + } finally { + $result->closeCursor(); + } + } + + /** + * Reads the hardening switches through IConfig, so the *effective merged* + * configuration is asserted rather than a single config file. + * + * @return array{ + * result: self::RESULT_PASS|self::RESULT_FAIL, + * checks: list, + * parameters: array + * } + */ + private function checkSecurityConfig(string $probeHash): array { + $checks = [ + // The real downgrade switch: Hasher::getPrefferedAlgorithm() falls + // back to PASSWORD_DEFAULT (bcrypt today) when this is true. + $this->check('hashing_default_password', false, $this->config->getSystemValueBool('hashing_default_password', false)), + $this->check('auth.bruteforce.protection.enabled', true, $this->config->getSystemValueBool('auth.bruteforce.protection.enabled', true)), + $this->check('ratelimit.protection.enabled', true, $this->config->getSystemValueBool('ratelimit.protection.enabled', true)), + $this->check('overwriteprotocol', 'https', $this->config->getSystemValueString('overwriteprotocol', '')), + // Presence only — the values are secrets and must never be reported. + $this->check('passwordsalt_present', true, $this->config->getSystemValueString('passwordsalt', '') !== ''), + $this->check('secret_present', true, $this->config->getSystemValueString('secret', '') !== ''), + ]; + + $passed = true; + foreach ($checks as $check) { + if ($check['result'] !== self::RESULT_PASS) { + $passed = false; + } + } + + return [ + 'result' => $this->verdict($passed), + 'checks' => $checks, + // Evidence, not an assertion: the cost parameters new hashes get. + 'parameters' => HashAlgorithm::parametersFromStoredHash($probeHash), + ]; + } + + /** + * @return array{key: string, expected: bool|string, actual: bool|string, result: string} + */ + private function check(string $key, bool|string $expected, bool|string $actual): array { + return [ + 'key' => $key, + 'expected' => $expected, + 'actual' => $actual, + 'result' => $this->verdict($expected === $actual), + ]; + } + + /** + * @return array{id: string, url: string, name: string, namespace: string, environment: string} + */ + private function describeInstance(): array { + return [ + 'id' => $this->config->getSystemValueString('instanceid', ''), + 'url' => $this->config->getSystemValueString('overwrite.cli.url', ''), + 'name' => $this->fromEnvironment('INSTANCE_NAME'), + 'namespace' => $this->fromEnvironment('NAMESPACE'), + 'environment' => $this->fromEnvironment('ENVIRONMENT'), + ]; + } + + private function fromEnvironment(string $name): string { + $value = getenv($name); + + return is_string($value) ? $value : ''; + } + + /** + * @return self::RESULT_PASS|self::RESULT_FAIL + */ + private function verdict(bool $passed): string { + return $passed ? self::RESULT_PASS : self::RESULT_FAIL; + } + + private function asString(mixed $value): string { + return is_string($value) ? $value : ''; + } + + private function probeMessage(): string { + return $this->random->generate(32, ISecureRandom::CHAR_ALPHANUMERIC); + } + + /** + * A throwaway password with all four character classes present, so the + * always-enabled password_policy app accepts it whatever it enforces. + */ + private function probePassword(): string { + return $this->random->generate(8, ISecureRandom::CHAR_LOWER) + . $this->random->generate(8, ISecureRandom::CHAR_UPPER) + . $this->random->generate(8, ISecureRandom::CHAR_DIGITS) + . $this->random->generate(8, ISecureRandom::CHAR_SYMBOLS); + } +} diff --git a/psalm.xml b/psalm.xml index a2f738c..20e5c56 100644 --- a/psalm.xml +++ b/psalm.xml @@ -24,6 +24,8 @@ + + @@ -31,9 +33,11 @@ + + From 13dd2a36840f8cc841e8e69e6b9fe5612d31b055 Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Tue, 1 Sep 2026 15:04:55 +0200 Subject: [PATCH 3/8] test(security): cover argon2id/argon2i/bcrypt/legacy/empty hash fixtures Table-driven coverage for HashAlgorithm over real fixtures, because the version prefix is exactly what an earlier shell-based check missed. Captured fixtures pin the three prefixed forms (3| argon2id, 2| argon2i, 1| bcrypt) as they are actually written to oc_users.password, and the provider additionally generates fresh hashes with password_hash() so the classifier is exercised against what the current PHP build produces rather than only against strings committed a year ago. The rest of the table covers the unprefixed legacy forms ($2y and $2a bcrypt, sha1 in both letter cases), the empty string, and the edge cases that must not be mistaken for a hash: garbage, an empty hash behind a valid prefix, a non-numeric or zero version prefix, an md5 digest, and 40 non-hex or 60 non-bcrypt characters. A future version prefix still classifies by the inner hash, which is the intended behaviour. One dedicated test spells out the defect as a regression guard: a stored argon2id hash does not start with $argon2id$, it starts with 3|$argon2id$. parametersFromStoredHash is covered for argon2id (memory_cost, time_cost, threads), bcrypt (cost) and the forms that carry no parameters, plus an assertion that it only ever yields string keys with integer values -- it must never become a route for hash material to reach the artifact. Signed-off-by: Misha M.-Kupriyanov --- tests/unit/Security/HashAlgorithmTest.php | 147 ++++++++++++++++++++++ 1 file changed, 147 insertions(+) create mode 100644 tests/unit/Security/HashAlgorithmTest.php diff --git a/tests/unit/Security/HashAlgorithmTest.php b/tests/unit/Security/HashAlgorithmTest.php new file mode 100644 index 0000000..dc6dcb1 --- /dev/null +++ b/tests/unit/Security/HashAlgorithmTest.php @@ -0,0 +1,147 @@ + [self::REAL_STORED_ARGON2ID, HashAlgorithm::ARGON2ID], + 'version 2 prefix, real argon2i hash' => [self::REAL_STORED_ARGON2I, HashAlgorithm::ARGON2I], + 'version 1 prefix, real bcrypt hash' => [self::REAL_STORED_BCRYPT, HashAlgorithm::BCRYPT], + + // Legacy, unprefixed hashes. + 'bare bcrypt hash (60 chars, no version prefix)' => [ + '$2y$10$nv5Wx10BmWJnGFj7mhFGPeUaW8wNnFRqP0g09H93X/9yDhxNBjv/m', + HashAlgorithm::LEGACY_BCRYPT, + ], + 'bare bcrypt $2a variant' => [ + '$2a$10$nv5Wx10BmWJnGFj7mhFGPeUaW8wNnFRqP0g09H93X/9yDhxNBjv/m', + HashAlgorithm::LEGACY_BCRYPT, + ], + 'sha1 hex digest' => [sha1('correct horse battery staple'), HashAlgorithm::LEGACY_SHA1], + 'sha1 hex digest, upper case' => [strtoupper(sha1('correct horse battery staple')), HashAlgorithm::LEGACY_SHA1], + + // Nothing stored. + 'empty string' => ['', HashAlgorithm::EMPTY], + + // Garbage and edge cases. + 'garbage' => ['not-a-hash-at-all', HashAlgorithm::UNKNOWN], + 'version prefix with empty hash' => ['3|', HashAlgorithm::UNKNOWN], + 'version prefix with garbage hash' => ['3|nonsense', HashAlgorithm::UNKNOWN], + 'non-numeric prefix is not a version' => ['foo|$argon2id$v=19$m=65536,t=4,p=1$c2FsdA$aGFzaA', HashAlgorithm::UNKNOWN], + 'zero prefix is not a version' => ['0|$argon2id$v=19$m=65536,t=4,p=1$c2FsdA$aGFzaA', HashAlgorithm::UNKNOWN], + '40 non-hex characters' => [str_repeat('z', 40), HashAlgorithm::UNKNOWN], + '60 characters that are not bcrypt' => [str_repeat('z', 60), HashAlgorithm::UNKNOWN], + 'md5 digest' => [md5('correct horse battery staple'), HashAlgorithm::UNKNOWN], + + // An unknown future hasher version still classifies by the inner hash. + 'unknown future version prefix' => ['9|' . self::stripPrefix(self::REAL_STORED_ARGON2ID), HashAlgorithm::ARGON2ID], + ]; + + // Freshly generated hashes, so the classifier is exercised against what + // the current PHP build actually produces rather than only fixtures. + $cases['freshly hashed argon2id with version 3 prefix'] = [ + '3|' . password_hash('probe', PASSWORD_ARGON2ID), + HashAlgorithm::ARGON2ID, + ]; + $cases['freshly hashed argon2i with version 2 prefix'] = [ + '2|' . password_hash('probe', PASSWORD_ARGON2I), + HashAlgorithm::ARGON2I, + ]; + $cases['freshly hashed bcrypt with version 1 prefix'] = [ + '1|' . password_hash('probe', PASSWORD_BCRYPT), + HashAlgorithm::BCRYPT, + ]; + $cases['freshly hashed bcrypt without prefix'] = [ + password_hash('probe', PASSWORD_BCRYPT), + HashAlgorithm::LEGACY_BCRYPT, + ]; + + return $cases; + } + + #[DataProvider('provideStoredHashes')] + public function testFromStoredHash(string $stored, string $expected): void { + $this->assertSame($expected, HashAlgorithm::fromStoredHash($stored)); + } + + public function testBareArgon2idPrefixIsNotHowNextcloudStoresHashes(): void { + // Regression guard for the defect this class replaces: a shell check that + // compared the stored value against the literal '$argon2id$' prefix. + $stored = '3|' . password_hash('probe', PASSWORD_ARGON2ID); + + $this->assertStringStartsNotWith('$argon2id$', $stored); + $this->assertStringStartsWith('3|$argon2id$', $stored); + $this->assertSame(HashAlgorithm::ARGON2ID, HashAlgorithm::fromStoredHash($stored)); + } + + public function testParametersFromArgon2idHash(): void { + $stored = '3|' . password_hash('probe', PASSWORD_ARGON2ID, [ + 'memory_cost' => 65536, + 'time_cost' => 4, + 'threads' => 1, + ]); + + $this->assertSame( + ['memory_cost' => 65536, 'time_cost' => 4, 'threads' => 1], + HashAlgorithm::parametersFromStoredHash($stored), + ); + } + + public function testParametersFromBcryptHash(): void { + $stored = '1|' . password_hash('probe', PASSWORD_BCRYPT, ['cost' => 11]); + + $this->assertSame(['cost' => 11], HashAlgorithm::parametersFromStoredHash($stored)); + } + + public static function provideHashesWithoutParameters(): array { + return [ + 'empty' => [''], + 'garbage' => ['not-a-hash-at-all'], + 'sha1' => [sha1('probe')], + ]; + } + + #[DataProvider('provideHashesWithoutParameters')] + public function testParametersAreEmptyForUnrecognisedHashes(string $stored): void { + $this->assertSame([], HashAlgorithm::parametersFromStoredHash($stored)); + } + + public function testParametersNeverCarryHashMaterial(): void { + $stored = '3|' . password_hash('probe', PASSWORD_ARGON2ID); + + foreach (HashAlgorithm::parametersFromStoredHash($stored) as $name => $value) { + $this->assertIsString($name); + $this->assertIsInt($value); + } + } + + private static function stripPrefix(string $stored): string { + return explode('|', $stored, 2)[1]; + } +} From 87d7c4ee549909b4ed2f299c45a382633961f08e Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Tue, 1 Sep 2026 15:05:37 +0200 Subject: [PATCH 4/8] test(security): cover self-test verdicts and the no-secrets invariant Unit coverage for SecuritySelfTest with every collaborator mocked, so the verdict logic is pinned without touching a database or creating users. The artifact shape itself is asserted key by key, in order, because another stream parses it with jq and the schema is frozen: the six top-level keys, the instance fields, the password_hashing and round_trip fields, the four always-present distribution buckets, and the check entry shape. Verdicts covered: - PASS on argon2id with a hardened configuration. - FAIL when the configured algorithm is bcrypt or argon2i, with the hardening result left untouched, so the two halves of the artifact are shown to be independent. - FAIL for each of the seven security_config assertions in turn -- hashing_default_password enabled, brute-force or rate-limit protection disabled, plain http, an unset protocol, and a missing passwordsalt or secret -- each asserting that exactly the expected key failed. - The distribution counting every algorithm, including the buckets that only appear when observed (argon2i, legacy-bcrypt, legacy-sha1), that a stored bcrypt hash fails the survey, and that rows without a local password do not. - The sample size reaching setMaxResults, and 0 leaving it unset. - The round trip: skipped by default and unable to drag the result down; passing and deleting the probe user; failing when the stored hash is not argon2id, when the probe user survives deletion, when createUser throws, and when it returns false. The failure paths assert the logged message and that the exception object itself is never in the context. - The probe user never getting an email address, and the generated probe password carrying all four character classes. Two tests enforce the security invariant directly: the encoded artifact is searched for the real stored hashes (prefixed and unprefixed) and for the configured passwordsalt and secret, none of which may appear. Signed-off-by: Misha M.-Kupriyanov --- tests/unit/Security/SecuritySelfTestTest.php | 600 +++++++++++++++++++ 1 file changed, 600 insertions(+) create mode 100644 tests/unit/Security/SecuritySelfTestTest.php diff --git a/tests/unit/Security/SecuritySelfTestTest.php b/tests/unit/Security/SecuritySelfTestTest.php new file mode 100644 index 0000000..8368a2b --- /dev/null +++ b/tests/unit/Security/SecuritySelfTestTest.php @@ -0,0 +1,600 @@ + */ + private array $queryBuilders = []; + + protected function setUp(): void { + parent::setUp(); + $this->hasher = $this->createMock(IHasher::class); + $this->db = $this->createMock(IDBConnection::class); + $this->config = $this->createMock(IConfig::class); + $this->userManager = $this->createMock(IUserManager::class); + $this->random = $this->createMock(ISecureRandom::class); + $this->logger = $this->createMock(LoggerInterface::class); + $this->queryBuilders = []; + + // Deterministic randomness: each call returns the head of the requested + // character class, so probe passwords stay assertable. + $this->random->method('generate') + ->willReturnCallback(fn (int $length, string $characters = 'abc'): string => substr(str_repeat($characters, $length), 0, $length)); + } + + public function testPassWhenHashingIsArgon2idAndTheInstanceIsHardened(): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id()]); + + $report = $this->selfTest()->run(); + + $this->assertSame(SecuritySelfTest::RESULT_PASS, $report['result']); + $this->assertSame(SecuritySelfTest::RESULT_PASS, $report['password_hashing']['result']); + $this->assertSame(SecuritySelfTest::RESULT_PASS, $report['security_config']['result']); + $this->assertSame(HashAlgorithm::ARGON2ID, $report['password_hashing']['configured_algorithm']); + } + + public function testArtifactMatchesTheAgreedShape(): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id()]); + + $report = $this->selfTest()->run(); + + $this->assertSame( + ['schema_version', 'timestamp', 'result', 'instance', 'password_hashing', 'security_config'], + array_keys($report), + ); + $this->assertSame('3', $report['schema_version']); + $this->assertSame( + ['id', 'url', 'name', 'namespace', 'environment'], + array_keys($report['instance']), + ); + $this->assertSame('inst-1', $report['instance']['id']); + $this->assertSame('https://cloud.example.com', $report['instance']['url']); + $this->assertSame( + ['result', 'configured_algorithm', 'round_trip', 'stored_distribution'], + array_keys($report['password_hashing']), + ); + $this->assertSame( + ['result', 'stored_algorithm', 'cleaned_up'], + array_keys($report['password_hashing']['round_trip']), + ); + $this->assertSame( + [HashAlgorithm::ARGON2ID, HashAlgorithm::BCRYPT, HashAlgorithm::EMPTY, HashAlgorithm::UNKNOWN], + array_keys($report['password_hashing']['stored_distribution']), + ); + $this->assertSame( + ['result', 'checks', 'parameters'], + array_keys($report['security_config']), + ); + $this->assertSame( + ['key', 'expected', 'actual', 'result'], + array_keys($report['security_config']['checks'][0]), + ); + $this->assertSame( + ['memory_cost' => 65536, 'time_cost' => 4, 'threads' => 1], + $report['security_config']['parameters'], + ); + $this->assertMatchesRegularExpression('/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z$/', $report['timestamp']); + } + + public function testInstanceMetadataComesFromTheEnvironment(): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id()]); + + putenv('INSTANCE_NAME=customer-42'); + putenv('NAMESPACE=ncw-prod'); + putenv('ENVIRONMENT=production'); + try { + $report = $this->selfTest()->run(); + } finally { + putenv('INSTANCE_NAME'); + putenv('NAMESPACE'); + putenv('ENVIRONMENT'); + } + + $this->assertSame('customer-42', $report['instance']['name']); + $this->assertSame('ncw-prod', $report['instance']['namespace']); + $this->assertSame('production', $report['instance']['environment']); + } + + public function testInstanceMetadataIsEmptyWithoutEnvironmentVariables(): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id()]); + + $report = $this->selfTest()->run(); + + $this->assertSame('', $report['instance']['name']); + $this->assertSame('', $report['instance']['namespace']); + $this->assertSame('', $report['instance']['environment']); + } + + public function testFailWhenConfiguredAlgorithmIsBcrypt(): void { + $this->stubHasher(self::bcrypt()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id()]); + + $report = $this->selfTest()->run(); + + $this->assertSame(HashAlgorithm::BCRYPT, $report['password_hashing']['configured_algorithm']); + $this->assertSame(SecuritySelfTest::RESULT_FAIL, $report['password_hashing']['result']); + $this->assertSame(SecuritySelfTest::RESULT_FAIL, $report['result']); + // The hardening checks are independent of the hasher probe. + $this->assertSame(SecuritySelfTest::RESULT_PASS, $report['security_config']['result']); + $this->assertSame(['cost' => 10], $report['security_config']['parameters']); + } + + public function testFailWhenConfiguredAlgorithmIsArgon2i(): void { + $this->stubHasher(self::argon2i()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id()]); + + $report = $this->selfTest()->run(); + + $this->assertSame(HashAlgorithm::ARGON2I, $report['password_hashing']['configured_algorithm']); + $this->assertSame(SecuritySelfTest::RESULT_FAIL, $report['result']); + } + + /** + * @return array}> + */ + public static function provideBrokenSecurityConfig(): array { + return [ + 'hashing_default_password enabled downgrades the hasher' => [ + 'hashing_default_password', + ['hashing_default_password' => true], + ], + 'brute force protection disabled' => [ + 'auth.bruteforce.protection.enabled', + ['auth.bruteforce.protection.enabled' => false], + ], + 'rate limit protection disabled' => [ + 'ratelimit.protection.enabled', + ['ratelimit.protection.enabled' => false], + ], + 'plain http' => [ + 'overwriteprotocol', + ['overwriteprotocol' => 'http'], + ], + 'protocol not configured' => [ + 'overwriteprotocol', + ['overwriteprotocol' => ''], + ], + 'password salt missing' => [ + 'passwordsalt_present', + ['passwordsalt' => ''], + ], + 'secret missing' => [ + 'secret_present', + ['secret' => ''], + ], + ]; + } + + /** + * @param array $overrides + */ + #[DataProvider('provideBrokenSecurityConfig')] + public function testFailForEachSecurityConfigAssertion(string $failingKey, array $overrides): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig($overrides); + $this->stubDatabase([self::argon2id()]); + + $report = $this->selfTest()->run(); + + $this->assertSame(SecuritySelfTest::RESULT_FAIL, $report['security_config']['result']); + $this->assertSame(SecuritySelfTest::RESULT_FAIL, $report['result']); + // Only the broken assertion fails; the password hashing checks are untouched. + $this->assertSame(SecuritySelfTest::RESULT_PASS, $report['password_hashing']['result']); + + $failed = []; + foreach ($report['security_config']['checks'] as $check) { + if ($check['result'] === SecuritySelfTest::RESULT_FAIL) { + $failed[] = $check['key']; + } + } + $this->assertSame([$failingKey], $failed); + } + + public function testSecurityConfigReportsSecretPresenceOnly(): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id()]); + + $report = $this->selfTest()->run(); + + $presence = []; + foreach ($report['security_config']['checks'] as $check) { + $presence[$check['key']] = $check['actual']; + } + $this->assertTrue($presence['passwordsalt_present']); + $this->assertTrue($presence['secret_present']); + + $encoded = json_encode($report); + $this->assertIsString($encoded); + $this->assertStringNotContainsString(self::PASSWORD_SALT, $encoded); + $this->assertStringNotContainsString(self::SECRET, $encoded); + } + + public function testArtifactNeverCarriesHashMaterial(): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id(), self::bcrypt()], self::argon2id()); + + $report = $this->selfTest()->run(true); + + $encoded = json_encode($report); + $this->assertIsString($encoded); + foreach ([self::argon2id(), self::bcrypt(), self::argon2i()] as $hash) { + $this->assertStringNotContainsString($hash, $encoded); + // Not even the bare hash without the version prefix. + $this->assertStringNotContainsString(explode('|', $hash, 2)[1], $encoded); + } + } + + public function testStoredDistributionCountsEveryAlgorithm(): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig(); + $this->stubDatabase([ + self::argon2id(), + self::argon2id(), + self::argon2i(), + self::bcrypt(), + self::legacyBcrypt(), + self::legacySha1(), + '', + 'garbage', + ]); + + $report = $this->selfTest()->run(); + + $this->assertSame([ + HashAlgorithm::ARGON2ID => 2, + HashAlgorithm::BCRYPT => 1, + HashAlgorithm::EMPTY => 1, + HashAlgorithm::UNKNOWN => 1, + HashAlgorithm::ARGON2I => 1, + HashAlgorithm::LEGACY_BCRYPT => 1, + HashAlgorithm::LEGACY_SHA1 => 1, + ], $report['password_hashing']['stored_distribution']); + $this->assertSame(SecuritySelfTest::RESULT_FAIL, $report['password_hashing']['result']); + } + + public function testStoredHashesWithoutAPasswordDoNotFailTheSurvey(): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id(), '', '', '']); + + $report = $this->selfTest()->run(); + + $this->assertSame( + [HashAlgorithm::ARGON2ID => 1, HashAlgorithm::BCRYPT => 0, HashAlgorithm::EMPTY => 3, HashAlgorithm::UNKNOWN => 0], + $report['password_hashing']['stored_distribution'], + ); + $this->assertSame(SecuritySelfTest::RESULT_PASS, $report['result']); + } + + public function testAStoredBcryptHashFailsTheSurvey(): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id(), self::bcrypt()]); + + $report = $this->selfTest()->run(); + + $this->assertSame(1, $report['password_hashing']['stored_distribution'][HashAlgorithm::BCRYPT]); + $this->assertSame(SecuritySelfTest::RESULT_FAIL, $report['password_hashing']['result']); + $this->assertSame(SecuritySelfTest::RESULT_FAIL, $report['result']); + } + + public function testSampleSizeLimitsTheSurvey(): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id()]); + + $this->queryBuilders[0]->expects($this->once())->method('setMaxResults')->with(250); + + $this->selfTest()->run(false, 250); + } + + public function testSampleSizeZeroSurveysEveryRow(): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id()]); + + $this->queryBuilders[0]->expects($this->never())->method('setMaxResults'); + + $this->selfTest()->run(false, 0); + } + + public function testRoundTripIsSkippedUnlessRequested(): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id()]); + + $this->userManager->expects($this->never())->method('createUser'); + + $report = $this->selfTest()->run(); + + $this->assertSame([ + 'result' => SecuritySelfTest::RESULT_SKIPPED, + 'stored_algorithm' => null, + 'cleaned_up' => null, + ], $report['password_hashing']['round_trip']); + // A skipped round trip must not drag the overall result down. + $this->assertSame(SecuritySelfTest::RESULT_PASS, $report['result']); + } + + public function testRoundTripPassesAndRemovesTheProbeUser(): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id()], self::argon2id()); + + $user = $this->createMock(IUser::class); + $user->expects($this->once())->method('delete')->willReturn(true); + // A disposable probe account must never get an email address. + $user->expects($this->never())->method('setEMailAddress'); + + $this->userManager->expects($this->once()) + ->method('createUser') + ->with($this->stringStartsWith('ncw-selftest-'), $this->anything()) + ->willReturn($user); + $this->userManager->method('get')->willReturn(null); + $this->logger->expects($this->never())->method('error'); + + $report = $this->selfTest()->run(true); + + $this->assertSame([ + 'result' => SecuritySelfTest::RESULT_PASS, + 'stored_algorithm' => HashAlgorithm::ARGON2ID, + 'cleaned_up' => true, + ], $report['password_hashing']['round_trip']); + $this->assertSame(SecuritySelfTest::RESULT_PASS, $report['result']); + } + + public function testRoundTripProbePasswordUsesAllCharacterClasses(): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id()], self::argon2id()); + + $user = $this->createMock(IUser::class); + $password = null; + $this->userManager->method('createUser') + ->willReturnCallback(function (string $uid, string $given) use ($user, &$password): IUser { + $password = $given; + return $user; + }); + $this->userManager->method('get')->willReturn(null); + + $this->selfTest()->run(true); + + $this->assertIsString($password); + $this->assertGreaterThanOrEqual(12, strlen($password)); + $this->assertMatchesRegularExpression('/[a-z]/', $password); + $this->assertMatchesRegularExpression('/[A-Z]/', $password); + $this->assertMatchesRegularExpression('/[0-9]/', $password); + $this->assertMatchesRegularExpression('/[^a-zA-Z0-9]/', $password); + } + + public function testRoundTripFailsWhenTheStoredHashIsNotArgon2id(): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id()], self::bcrypt()); + + $user = $this->createMock(IUser::class); + $user->expects($this->once())->method('delete')->willReturn(true); + $this->userManager->method('createUser')->willReturn($user); + $this->userManager->method('get')->willReturn(null); + + $report = $this->selfTest()->run(true); + + $this->assertSame([ + 'result' => SecuritySelfTest::RESULT_FAIL, + 'stored_algorithm' => HashAlgorithm::BCRYPT, + 'cleaned_up' => true, + ], $report['password_hashing']['round_trip']); + $this->assertSame(SecuritySelfTest::RESULT_FAIL, $report['result']); + } + + public function testRoundTripFailsWhenTheProbeUserSurvives(): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id()], self::argon2id()); + + $user = $this->createMock(IUser::class); + $this->userManager->method('createUser')->willReturn($user); + $this->userManager->method('get')->willReturn($user); + + $this->logger->expects($this->once()) + ->method('error') + ->with('SecuritySelfTest: round-trip probe user still exists', $this->anything()); + + $report = $this->selfTest()->run(true); + + $this->assertSame([ + 'result' => SecuritySelfTest::RESULT_FAIL, + 'stored_algorithm' => HashAlgorithm::ARGON2ID, + 'cleaned_up' => false, + ], $report['password_hashing']['round_trip']); + $this->assertSame(SecuritySelfTest::RESULT_FAIL, $report['result']); + } + + public function testRoundTripFailsAndLogsWhenUserCreationThrows(): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id()]); + + $this->userManager->method('createUser') + ->willThrowException(new \InvalidArgumentException('Password is among the most common ones')); + $this->userManager->method('get')->willReturn(null); + + $this->logger->expects($this->once()) + ->method('error') + ->with( + 'SecuritySelfTest: round-trip probe failed', + $this->callback(fn (array $context): bool => $context['exceptionClass'] === \InvalidArgumentException::class + && $context['message'] === 'Password is among the most common ones' + && !array_key_exists('exception', $context)), + ); + + $report = $this->selfTest()->run(true); + + $this->assertSame([ + 'result' => SecuritySelfTest::RESULT_FAIL, + 'stored_algorithm' => null, + 'cleaned_up' => true, + ], $report['password_hashing']['round_trip']); + $this->assertSame(SecuritySelfTest::RESULT_FAIL, $report['result']); + } + + public function testRoundTripFailsAndLogsWhenUserCreationReturnsFalse(): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id()]); + + $this->userManager->method('createUser')->willReturn(false); + $this->userManager->method('get')->willReturn(null); + + $this->logger->expects($this->once()) + ->method('error') + ->with('SecuritySelfTest: could not create the round-trip probe user', $this->anything()); + + $report = $this->selfTest()->run(true); + + $this->assertSame(SecuritySelfTest::RESULT_FAIL, $report['password_hashing']['round_trip']['result']); + $this->assertNull($report['password_hashing']['round_trip']['stored_algorithm']); + } + + private function selfTest(): SecuritySelfTest { + return new SecuritySelfTest( + $this->hasher, + $this->db, + $this->config, + $this->userManager, + $this->random, + $this->logger, + ); + } + + private function stubHasher(string $hash): void { + $this->hasher->method('hash')->willReturn($hash); + } + + /** + * @param array $overrides + */ + private function stubConfig(array $overrides = []): void { + $values = array_merge([ + 'instanceid' => 'inst-1', + 'overwrite.cli.url' => 'https://cloud.example.com', + 'overwriteprotocol' => 'https', + 'passwordsalt' => self::PASSWORD_SALT, + 'secret' => self::SECRET, + 'hashing_default_password' => false, + 'auth.bruteforce.protection.enabled' => true, + 'ratelimit.protection.enabled' => true, + ], $overrides); + + $this->config->method('getSystemValueString') + ->willReturnCallback(function (string $key, string $default = '') use ($values): string { + $value = $values[$key] ?? $default; + return is_string($value) ? $value : $default; + }); + $this->config->method('getSystemValueBool') + ->willReturnCallback(function (string $key, bool $default = false) use ($values): bool { + $value = $values[$key] ?? $default; + return is_bool($value) ? $value : $default; + }); + } + + /** + * The service asks for one query builder to survey the stored hashes and, for + * a round trip, a second one to read the probe user's hash back. + * + * @param list $storedHashes + */ + private function stubDatabase(array $storedHashes, ?string $roundTripHash = null): void { + $surveyResult = $this->createMock(IResult::class); + $rows = array_map(static fn (string $hash): array => ['password' => $hash], $storedHashes); + $rows[] = false; + $surveyResult->method('fetch')->willReturnOnConsecutiveCalls(...$rows); + $this->queryBuilders[] = $this->stubQueryBuilder($surveyResult); + + if ($roundTripHash !== null) { + $roundTripResult = $this->createMock(IResult::class); + $roundTripResult->method('fetchOne')->willReturn($roundTripHash); + $this->queryBuilders[] = $this->stubQueryBuilder($roundTripResult); + } + + $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls(...$this->queryBuilders); + } + + private function stubQueryBuilder(IResult&MockObject $result): IQueryBuilder&MockObject { + $query = $this->createMock(IQueryBuilder::class); + $query->method('select')->willReturnSelf(); + $query->method('from')->willReturnSelf(); + $query->method('where')->willReturnSelf(); + $query->method('setMaxResults')->willReturnSelf(); + $query->method('expr')->willReturn($this->createMock(IExpressionBuilder::class)); + $query->method('createNamedParameter')->willReturn(':uid'); + $query->method('executeQuery')->willReturn($result); + + return $query; + } + + private static function argon2id(): string { + return '3|$argon2id$v=19$m=65536,t=4,p=1$QWg3b3ptdUY2bTMyLm1VSA$shWk3Zo2H45opssZuQLI/XERpr+n4BOC53D4i24CTRE'; + } + + private static function argon2i(): string { + return '2|$argon2i$v=19$m=65536,t=4,p=1$dFcyWjAyNWI3QTJUS2VVWg$neelG9YBu6q+p8TKXAzqAQjQXraqCRisUflPMxjeHLQ'; + } + + private static function bcrypt(): string { + return '1|$2y$10$nv5Wx10BmWJnGFj7mhFGPeUaW8wNnFRqP0g09H93X/9yDhxNBjv/m'; + } + + private static function legacyBcrypt(): string { + return '$2y$10$nv5Wx10BmWJnGFj7mhFGPeUaW8wNnFRqP0g09H93X/9yDhxNBjv/m'; + } + + private static function legacySha1(): string { + return sha1('legacy'); + } +} From 27fc079bd229a8e0318876e213fb4729be350f33 Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Tue, 1 Sep 2026 15:08:07 +0200 Subject: [PATCH 5/8] test(security): add integration coverage for hash survey and round trip The unit suite mocks the query builder, so it proves the counting logic but not that the query works or that the round trip really writes and removes a row. Add integration coverage against the real database. The service is resolved through the app container, so the constructor's autowiring is covered as well. Covered: - The survey sums to the actual row count of the users table, and always reports the four frozen buckets as integers. - A sample size of 1 really limits the query to one row. - The configured algorithm on a real instance is argon2id, and the reported parameters are the argon2 triple rather than bcrypt's cost. - The full round trip: the stored algorithm is argon2id, cleaned_up is true, the probe user is gone from both IUserManager and the users table, and the uid it used starts with ncw-selftest-. - The probe user has no email address. It only exists between createUser() and delete(), so a UserCreatedEvent listener captures the address at the one moment it can be inspected. tearDown deletes any probe account the listener saw, so a failing assertion can never leave one behind. - The security invariant against real data: every stored hash is read straight from the database and, together with the configured passwordsalt and secret, asserted absent from the encoded artifact. Runs on sqlite locally; the phpunit-mysql workflow covers MySQL. Signed-off-by: Misha M.-Kupriyanov --- .../SecuritySelfTestIntegrationTest.php | 190 ++++++++++++++++++ 1 file changed, 190 insertions(+) create mode 100644 tests/integration/SecuritySelfTestIntegrationTest.php diff --git a/tests/integration/SecuritySelfTestIntegrationTest.php b/tests/integration/SecuritySelfTestIntegrationTest.php new file mode 100644 index 0000000..14bf7e1 --- /dev/null +++ b/tests/integration/SecuritySelfTestIntegrationTest.php @@ -0,0 +1,190 @@ + */ + private array $observedProbeUids = []; + + /** @var array */ + private array $observedProbeEmails = []; + + protected function setUp(): void { + parent::setUp(); + + $this->db = Server::get(IDBConnection::class); + $this->userManager = Server::get(IUserManager::class); + // Resolved through the app container, so the DI wiring is covered too. + $this->selfTest = (new Application())->getContainer()->get(SecuritySelfTest::class); + + // The probe user only exists between createUser() and delete(), so the + // creation event is the only place its email address can be inspected. + Server::get(IEventDispatcher::class)->addListener( + UserCreatedEvent::class, + function (UserCreatedEvent $event): void { + $uid = $event->getUser()->getUID(); + if (!str_starts_with($uid, 'ncw-selftest-')) { + return; + } + $this->observedProbeUids[] = $uid; + $this->observedProbeEmails[$uid] = $event->getUser()->getEMailAddress(); + }, + ); + } + + protected function tearDown(): void { + // Safety net: never leave a probe account behind, whatever failed. + foreach ($this->observedProbeUids as $uid) { + $this->userManager->get($uid)?->delete(); + } + $this->observedProbeUids = []; + $this->observedProbeEmails = []; + + parent::tearDown(); + } + + public function testSurveyCountsEveryStoredHashInTheUsersTable(): void { + $report = $this->selfTest->run(); + + $distribution = $report['password_hashing']['stored_distribution']; + foreach ([HashAlgorithm::ARGON2ID, HashAlgorithm::BCRYPT, HashAlgorithm::EMPTY, HashAlgorithm::UNKNOWN] as $bucket) { + $this->assertArrayHasKey($bucket, $distribution); + $this->assertIsInt($distribution[$bucket]); + } + + $this->assertSame($this->countUsers(), array_sum($distribution)); + } + + public function testSurveyRespectsTheSampleSize(): void { + $this->assertGreaterThan(0, $this->countUsers(), 'The test instance needs at least one user'); + + $report = $this->selfTest->run(false, 1); + + $this->assertSame(1, array_sum($report['password_hashing']['stored_distribution'])); + } + + public function testConfiguredAlgorithmIsArgon2idOnThisInstance(): void { + $report = $this->selfTest->run(); + + $this->assertSame(HashAlgorithm::ARGON2ID, $report['password_hashing']['configured_algorithm']); + $this->assertSame( + ['memory_cost', 'time_cost', 'threads'], + array_keys($report['security_config']['parameters']), + ); + } + + public function testRoundTripStoresArgon2idAndRemovesTheProbeUser(): void { + $report = $this->selfTest->run(true); + + $roundTrip = $report['password_hashing']['round_trip']; + $this->assertSame(SecuritySelfTest::RESULT_PASS, $roundTrip['result']); + $this->assertSame(HashAlgorithm::ARGON2ID, $roundTrip['stored_algorithm']); + $this->assertTrue($roundTrip['cleaned_up']); + + $this->assertCount(1, $this->observedProbeUids, 'Exactly one probe user should have been created'); + $uid = $this->observedProbeUids[0]; + + // Never set an email address on a disposable probe account. + $this->assertEmpty($this->observedProbeEmails[$uid], 'The probe user must have no email address'); + + // Gone from the user manager and from the database. + $this->assertNull($this->userManager->get($uid)); + $this->assertNull($this->readStoredHash($uid)); + } + + public function testArtifactCarriesNoHashMaterialFromTheDatabase(): void { + $storedHashes = $this->readAllStoredHashes(); + $this->assertNotEmpty($storedHashes, 'The test instance needs at least one stored hash'); + + $report = $this->selfTest->run(true); + $encoded = json_encode($report); + $this->assertIsString($encoded); + + foreach ($storedHashes as $stored) { + if ($stored === '') { + continue; + } + $this->assertStringNotContainsString($stored, $encoded); + $this->assertStringNotContainsString(explode('|', $stored, 2)[1] ?? $stored, $encoded); + } + + $config = Server::get(IConfig::class); + foreach (['passwordsalt', 'secret'] as $key) { + $value = $config->getSystemValueString($key, ''); + if ($value !== '') { + $this->assertStringNotContainsString($value, $encoded); + } + } + } + + private function countUsers(): int { + $query = $this->db->getQueryBuilder(); + $query->select($query->func()->count('*', 'total'))->from('users'); + $result = $query->executeQuery(); + try { + return (int)$result->fetchOne(); + } finally { + $result->closeCursor(); + } + } + + private function readStoredHash(string $uid): ?string { + $query = $this->db->getQueryBuilder(); + $query->select('password') + ->from('users') + ->where($query->expr()->eq('uid', $query->createNamedParameter($uid))); + $result = $query->executeQuery(); + try { + $stored = $result->fetchOne(); + return $stored === false ? null : (string)$stored; + } finally { + $result->closeCursor(); + } + } + + /** + * @return list + */ + private function readAllStoredHashes(): array { + $query = $this->db->getQueryBuilder(); + $query->select('password')->from('users'); + $result = $query->executeQuery(); + try { + $hashes = []; + while (($row = $result->fetch()) !== false) { + $hashes[] = (string)($row['password'] ?? ''); + } + return $hashes; + } finally { + $result->closeCursor(); + } + } +} From ccb90de9c47f7c038be81614d48a9eb6751615ec Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Tue, 1 Sep 2026 15:08:51 +0200 Subject: [PATCH 6/8] docs(security): document the self-test evidence artifact Whoever reads a PSS-07 artifact in six months will not have this branch in front of them, so write down what each field means, what makes the command pass, and how to find the log line. Contents: - Usage, the three exit codes, and the stdout contract the deployment wrapper depends on: only the artifact on stdout, diagnostics on stderr, and a complete artifact even on FAIL. - Why a stored hash cannot be matched against $argon2id$, with the 3|$argon2id$... shape spelled out, so the defect this replaces cannot be reintroduced from the documentation either. - The security invariant, and the fact that both test suites enforce it. - Field-by-field meaning, including why stored_distribution has four guaranteed buckets and additional ones only when observed, why empty rows are tolerated, and why the cost parameters come from the probe hash rather than from the hashing* config keys. - The Kibana queries, plus a caveat that matters in practice: Nextcloud's log writer serialises nested context arrays into JSON strings, so data.result is directly queryable but data.password_hashing arrives as a string that needs a parse. Consumers that want structured nested fields should use the stdout artifact. - A note that the round trip dispatches user events and therefore causes one extra user-count report on the next cron tick. - A mermaid flow in the style of the existing docs/events pages, and a failure-mode table that maps each symptom to its interpretation -- notably that a bcrypt configured_algorithm with hashing_default_password passing means the PHP build lacks argon2 support, which is an image problem rather than a configuration one. docs/README.md grows a Commands section, since it previously only indexed event flows, and REUSE.toml lists the new page. Signed-off-by: Misha M.-Kupriyanov --- REUSE.toml | 3 +- docs/README.md | 8 +- docs/security-selftest.md | 203 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 212 insertions(+), 2 deletions(-) create mode 100644 docs/security-selftest.md diff --git a/REUSE.toml b/REUSE.toml index 34d2636..7ef0f03 100644 --- a/REUSE.toml +++ b/REUSE.toml @@ -44,7 +44,8 @@ path = [ "CODE_OF_CONDUCT.md", "docs/README.md", "docs/events/post-setup.md", - "docs/events/user-stats.md" + "docs/events/user-stats.md", + "docs/security-selftest.md" ] precedence = "aggregate" SPDX-FileCopyrightText = "2026 STRATO GmbH" diff --git a/docs/README.md b/docs/README.md index 6535d63..99d1614 100644 --- a/docs/README.md +++ b/docs/README.md @@ -1,6 +1,12 @@ -# Event Flows +# NCW Tools documentation + +## Event flows NCW Tools reacts to Nextcloud lifecycle events by registering listeners that enqueue background jobs. Each document below walks through one end-to-end flow — the triggering event, the listener's synchronous work, the job's asynchronous behaviour, and the configuration it relies on. - [Post-setup welcome mail](events/post-setup.md) — sends the initial welcome email to the admin user once the system is reachable after installation. - [User stats reporting](events/user-stats.md) — reports the current total user count to the PSS Stats API after every user create or delete. + +## Commands + +- [Security self-test](security-selftest.md) — `occ ncw_tools:security:selftest` verifies that password hashing is argon2id and emits the C5 PSS-07 evidence artifact. diff --git a/docs/security-selftest.md b/docs/security-selftest.md new file mode 100644 index 0000000..9f2b6a8 --- /dev/null +++ b/docs/security-selftest.md @@ -0,0 +1,203 @@ +# Security self-test + +`occ ncw_tools:security:selftest` verifies that this instance hashes passwords with **argon2id** and that the surrounding hardening switches are in place, then emits a structured evidence artifact for C5 control **PSS-07**. The command is the evidence producer: a deployment wrapper runs it, pipes the artifact into `jq`, and archives the result. The same artifact is written to the log as structured context so it also lands in Kibana. + +## Usage + +``` +occ ncw_tools:security:selftest [--round-trip] [--sample-size=N] [--output=plain|json|json_pretty] +``` + +| Option | Default | Purpose | +| --- | --- | --- | +| `--round-trip` | off | Create a disposable probe user, read back the algorithm that actually landed in `oc_users.password`, then delete it again. This is the only check that observes the *whole* write path rather than a hasher probe. | +| `--sample-size=N` | `1000` | Number of rows to survey in the stored hash distribution. `0` surveys all rows. | +| `--output` | `plain` | `json` and `json_pretty` emit the evidence artifact; `plain` renders it for humans. | + +| Exit code | Meaning | +| --- | --- | +| `0` | PASS — every executed check passed. | +| `1` | FAIL — at least one executed check failed. The complete artifact is still written to stdout. | +| `2` | Usage error (unknown `--output` format, invalid `--sample-size`). No artifact is written. | + +**stdout carries nothing but the artifact.** Diagnostics, warnings and the log line go to stderr, so `occ ncw_tools:security:selftest --output=json | jq` is safe. On a FAIL the artifact is written in full *before* the non-zero exit — the failure case is precisely what the evidence needs to capture. + +## Why a stored hash is not simply matched against `$argon2id$` + +Nextcloud does not store a bare `password_hash()` string. `OC\Security\Hasher::hash()` prepends a hasher version and a pipe: + +``` +3|$argon2id$v=19$m=65536,t=4,p=1$$ +``` + +Version `3` is argon2id, `2` is argon2i, `1` is bcrypt. Hashes written before the version prefix existed are stored unprefixed — a 60 character bcrypt string or a 40 character sha1 hex digest. A check that compares the stored value against the literal prefix `$argon2id$` therefore never matches, no matter how the instance is configured. `OCA\NcwTools\Security\HashAlgorithm` splits the version prefix off first (mirroring the private `Hasher::splitHash()`) and then classifies the remainder with `password_get_info()`. + +## The evidence artifact + +```json +{ + "schema_version": "3", + "timestamp": "2026-09-01T12:00:00Z", + "result": "PASS", + "instance": { "id": "ocb0ycrd5a7h", "url": "https://cloud.example.com", "name": "", "namespace": "", "environment": "" }, + "password_hashing": { + "result": "PASS", + "configured_algorithm": "argon2id", + "round_trip": { "result": "SKIPPED", "stored_algorithm": null, "cleaned_up": null }, + "stored_distribution": { "argon2id": 1, "bcrypt": 0, "empty": 0, "unknown": 0 } + }, + "security_config": { + "result": "PASS", + "checks": [ { "key": "hashing_default_password", "expected": false, "actual": false, "result": "PASS" } ], + "parameters": { "memory_cost": 65536, "time_cost": 4, "threads": 1 } + } +} +``` + +### Security invariant + +**No field ever carries hash material, a salt or a secret value.** Only algorithm names, counts, booleans and cost parameters are reported. `passwordsalt` and `secret` are reported as presence (`true`/`false`) under the keys `passwordsalt_present` and `secret_present` — never as values. Both the unit and the integration suite assert this by encoding the artifact and searching it for the real stored hashes and config secrets. + +### Fields + +| Field | Meaning | +| --- | --- | +| `schema_version` | Artifact schema version. Currently `"3"` (a string). | +| `timestamp` | Collection time, UTC, `YYYY-MM-DDTHH:MM:SSZ`. | +| `result` | `PASS` only when both `password_hashing.result` and `security_config.result` are `PASS`. | +| `instance.id` | `instanceid` from `IConfig`. | +| `instance.url` | `overwrite.cli.url` from `IConfig`. Used downstream as the fallback identifier. | +| `instance.name` / `namespace` / `environment` | From the environment variables `INSTANCE_NAME`, `NAMESPACE`, `ENVIRONMENT`; empty string when unset. | +| `password_hashing.configured_algorithm` | The algorithm a *new* password gets, determined by hashing a random probe through `IHasher` and classifying the result. Must be `argon2id`. | +| `password_hashing.round_trip` | See below. `SKIPPED` unless `--round-trip` is given, and a skipped round trip never affects the result. | +| `password_hashing.stored_distribution` | Row counts per algorithm over the surveyed rows of the `users` table. | +| `security_config.checks` | One entry per asserted config value: `key`, `expected`, `actual`, `result`. | +| `security_config.parameters` | Evidence, not an assertion: the cost parameters the probe hash was actually produced with, as reported by `password_get_info()`. `memory_cost`/`time_cost`/`threads` for argon2, `cost` for bcrypt. These are the *effective* values — stronger evidence than the `hashing*` config keys, which the hasher clamps to the algorithm minimums. | + +### `stored_distribution` + +The four buckets `argon2id`, `bcrypt`, `empty` and `unknown` are always present, in that order. `argon2i`, `legacy-bcrypt` and `legacy-sha1` are added only when rows of that kind are actually observed, so their presence in an artifact is itself the finding. Consumers should read named keys (or sum `to_entries`) rather than assume a fixed key set. + +The survey passes when every counted row is `argon2id` or `empty`. Rows with no local password at all (SSO-only accounts) are not a hashing downgrade and are tolerated; anything else — `bcrypt`, `argon2i`, either legacy form, or `unknown` — fails the survey, because it means either a legacy account that has never re-authenticated or a configuration that has been downgraded. + +### `round_trip` + +| Field | Meaning | +| --- | --- | +| `result` | `SKIPPED` without `--round-trip`. Otherwise `PASS` when `stored_algorithm` is `argon2id` **and** `cleaned_up` is `true`. | +| `stored_algorithm` | The algorithm classified from the row the probe user actually wrote, or `null` when the probe could not be created. | +| `cleaned_up` | Re-checked after deletion by resolving the uid again; `true` means the probe user is gone. | + +The probe user is created as `ncw-selftest-` with a password assembled from all four character classes (so the always-enabled `password_policy` app accepts it) and **never** gets an email address, so it cannot receive mail or a password reset link. Deletion happens in a `finally`, and a probe user that survives is both logged at error level and reported as a failure. The reason a round trip failed is never in the artifact (the schema is fixed) — look for the `SecuritySelfTest: round-trip …` error lines in the log. + +Creating and deleting the probe user dispatches `UserCreatedEvent` and `UserDeletedEvent`, which this app's own `UserEventListener` reacts to by enqueuing a (deduplicated) `UserStatsJob`. Running the round trip therefore causes one extra user-count report on the next cron tick. + +### `security_config.checks` + +| `key` | Expected | Why | +| --- | --- | --- | +| `hashing_default_password` | `false` | The real downgrade switch. `Hasher::getPrefferedAlgorithm()` returns `PASSWORD_DEFAULT` — bcrypt today — as soon as this is `true`, so every new password stops being argon2id. | +| `auth.bruteforce.protection.enabled` | `true` | Brute-force throttling on authentication endpoints. | +| `ratelimit.protection.enabled` | `true` | Rate limiting on annotated controllers. | +| `overwriteprotocol` | `https` | Credentials must never be submitted over plain http. | +| `passwordsalt_present` | `true` | Presence of `passwordsalt`; needed to verify legacy hashes. Value never reported. | +| `secret_present` | `true` | Presence of `secret`. Value never reported. | + +All values are read through `IConfig`, so the **effective merged** configuration is asserted — `config/config.php` plus every `config/*.config.php` overlay — not a single file. + +## Log line and Kibana + +The command logs exactly one line at info level with the artifact as structured context: + +- message: `ncw_tools security selftest` +- app: `ncw_tools` + +The deployment sets `log_type=errorlog`, so the log JSON is written to stderr and picked up by the platform's log shipper. + +``` +app: "ncw_tools" AND message: "ncw_tools security selftest" +``` + +Narrow to failures: + +``` +app: "ncw_tools" AND message: "ncw_tools security selftest" AND data.result: "FAIL" +``` + +The round-trip diagnostics use the same app and are matched with: + +``` +app: "ncw_tools" AND message: "SecuritySelfTest: *" +``` + +**Caveat on nested fields.** Nextcloud's log writer serialises nested context arrays into JSON *strings*. The top-level scalars stay directly queryable — `data.schema_version`, `data.timestamp`, `data.result` — but `data.instance`, `data.password_hashing` and `data.security_config` arrive as strings containing JSON and need a parse (a Logstash/ingest `json` filter, or `| fromjson` at query time) before their inner fields can be filtered on. Use the stdout artifact, not the log line, when a consumer needs the nested fields structured. + +## Flow + +```mermaid +sequenceDiagram + autonumber + + participant OCC as occ ncw_tools:security:selftest + participant CMD as Command\SecuritySelfTest + participant SVC as Security\SecuritySelfTest + participant H as IHasher + participant HA as HashAlgorithm + participant DB as IDBConnection + participant CFG as IConfig + participant UM as IUserManager + participant RND as ISecureRandom + participant LOG as LoggerInterface + + OCC->>CMD: execute(--round-trip?, --sample-size, --output) + + alt invalid --output or --sample-size + CMD-->>OCC: stderr message, exit 2 + else options valid + CMD->>SVC: run(roundTrip, sampleSize) + + SVC->>RND: generate(32, alphanumeric) + SVC->>H: hash(probe) + H-->>SVC: "3|$argon2id$…" + SVC->>HA: fromStoredHash(probe hash) + HA-->>SVC: configured_algorithm + + SVC->>DB: SELECT password FROM users [LIMIT sampleSize] + loop every row + SVC->>HA: fromStoredHash(row) + HA-->>SVC: algorithm + end + + alt --round-trip given + SVC->>RND: generate(mixed character classes) + SVC->>UM: createUser("ncw-selftest-…", probe password) + Note over SVC,UM: no email address is ever set + SVC->>DB: SELECT password FROM users WHERE uid = probe + SVC->>HA: fromStoredHash(stored) + SVC->>UM: delete() in finally + SVC->>UM: get(uid) → cleaned_up? + else round trip skipped + Note over SVC: round_trip.result = SKIPPED + end + + SVC->>CFG: hardening switches + secret presence + SVC->>HA: parametersFromStoredHash(probe hash) + SVC-->>CMD: evidence artifact + + CMD->>LOG: info("ncw_tools security selftest", artifact) + CMD-->>OCC: artifact on stdout + CMD-->>OCC: exit 0 on PASS, 1 on FAIL + end +``` + +## Failure modes + +| Symptom | Interpretation | +| --- | --- | +| `configured_algorithm` is `bcrypt` and `hashing_default_password` fails | The downgrade switch is on. Every password set from now on is bcrypt. | +| `configured_algorithm` is `bcrypt` but `hashing_default_password` passes | The PHP build has no argon2 support (`PASSWORD_ARGON2ID` undefined). An image problem, not a config problem. | +| `configured_algorithm` is `argon2id` but `round_trip.stored_algorithm` is not | Something between `IUserManager` and the database is rewriting the hash — a user backend that hashes on its own, for example. | +| `stored_distribution` shows `bcrypt`, `argon2i` or a `legacy-*` bucket | Accounts that have not authenticated since the algorithm changed. Nextcloud rehashes on the next successful login; a persistent count means those accounts are dormant. | +| `round_trip.cleaned_up` is `false` | A probe account was left behind. Delete `ncw-selftest-*` manually and investigate the logged error. | +| `round_trip.result` is `FAIL` with `stored_algorithm: null` | The probe user could not be created — most likely `password_policy` rejected the generated password. See the logged error. | +| Exit 2 with no artifact | Wrong invocation, not a control failure. | From 8e47b90bb2c3c0c3483b2a12581ed5d3c5eee586 Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Tue, 1 Sep 2026 18:32:37 +0200 Subject: [PATCH 7/8] fix(tests): keep psalm-only stubs out of the runtime autoloader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit composer.json maps tests/stubs/ into the autoload-dev classmap, and lib/AppInfo/Application.php requires vendor/autoload.php at runtime. So a dev-mode `composer install` baked the Symfony Console stubs into the live classmap, where they shadowed the real classes in 3rdparty/ and fataled every occ command on the instance: PHP Fatal error: Declaration of Symfony\...\Output::writeln(...) must be compatible with Symfony\...\OutputInterface::writeln($messages, int $options = 0): void Release builds use `composer install --no-dev -o` (IONOS/Makefile), so production never loaded them — but any developer who ran a plain composer install in this app got a dead occ, for every command, not just ours. Move the five signature-only stubs added for this feature into tests/psalm-stubs/, which nothing autoloads, and point psalm's extraFiles at it. No test references them: at test time the real server classes are available via the bootstrap. The two pre-existing stubs stay put. --- psalm.xml | 12 ++++++++++++ .../{stubs => psalm-stubs}/OC/Core/Command/Base.php | 0 .../Component/Console/Input/InputInterface.php | 0 .../Symfony/Component/Console/Input/InputOption.php | 0 .../Console/Output/ConsoleOutputInterface.php | 0 .../Component/Console/Output/OutputInterface.php | 0 6 files changed, 12 insertions(+) rename tests/{stubs => psalm-stubs}/OC/Core/Command/Base.php (100%) rename tests/{stubs => psalm-stubs}/Symfony/Component/Console/Input/InputInterface.php (100%) rename tests/{stubs => psalm-stubs}/Symfony/Component/Console/Input/InputOption.php (100%) rename tests/{stubs => psalm-stubs}/Symfony/Component/Console/Output/ConsoleOutputInterface.php (100%) rename tests/{stubs => psalm-stubs}/Symfony/Component/Console/Output/OutputInterface.php (100%) diff --git a/psalm.xml b/psalm.xml index 20e5c56..f57efed 100644 --- a/psalm.xml +++ b/psalm.xml @@ -19,6 +19,18 @@ + + diff --git a/tests/stubs/OC/Core/Command/Base.php b/tests/psalm-stubs/OC/Core/Command/Base.php similarity index 100% rename from tests/stubs/OC/Core/Command/Base.php rename to tests/psalm-stubs/OC/Core/Command/Base.php diff --git a/tests/stubs/Symfony/Component/Console/Input/InputInterface.php b/tests/psalm-stubs/Symfony/Component/Console/Input/InputInterface.php similarity index 100% rename from tests/stubs/Symfony/Component/Console/Input/InputInterface.php rename to tests/psalm-stubs/Symfony/Component/Console/Input/InputInterface.php diff --git a/tests/stubs/Symfony/Component/Console/Input/InputOption.php b/tests/psalm-stubs/Symfony/Component/Console/Input/InputOption.php similarity index 100% rename from tests/stubs/Symfony/Component/Console/Input/InputOption.php rename to tests/psalm-stubs/Symfony/Component/Console/Input/InputOption.php diff --git a/tests/stubs/Symfony/Component/Console/Output/ConsoleOutputInterface.php b/tests/psalm-stubs/Symfony/Component/Console/Output/ConsoleOutputInterface.php similarity index 100% rename from tests/stubs/Symfony/Component/Console/Output/ConsoleOutputInterface.php rename to tests/psalm-stubs/Symfony/Component/Console/Output/ConsoleOutputInterface.php diff --git a/tests/stubs/Symfony/Component/Console/Output/OutputInterface.php b/tests/psalm-stubs/Symfony/Component/Console/Output/OutputInterface.php similarity index 100% rename from tests/stubs/Symfony/Component/Console/Output/OutputInterface.php rename to tests/psalm-stubs/Symfony/Component/Console/Output/OutputInterface.php From 563d9518006210a38ac754e08ea4ee3f8e08e71e Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Wed, 2 Sep 2026 10:26:10 +0200 Subject: [PATCH 8/8] fix(security): keep an unencodable env label from costing the artifact MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review on #38: the JSON path could return exit 1 with empty stdout when json_encode() failed, which contradicts the invariant the surrounding comment states and that nc-manager/bin/selftest.sh relies on — an empty stdout there degrades the run to a synthetic ERROR artifact and the real evidence is lost. The realistic trigger is invalid UTF-8 in INSTANCE_NAME, NAMESPACE or ENVIRONMENT, which are arbitrary bytes from the environment. Sanitise them where they are read rather than at the encode boundary, because the Kibana log line encodes the same context and would drop it the same way — fixing only stdout would have left the channel the ticket actually requires exposed. JSON_INVALID_UTF8_SUBSTITUTE stays as a backstop on the command. Also document that the loose (int) cast in stripVersionPrefix() is deliberate, with tests. Upstream Hasher::splitHash() splits on `(int)$parts[0] > 0`, so Nextcloud reads '3foo|' as version 3 and verifies the remainder as argon2id. Tightening to digits-only would report `unknown` for a value the instance verifies happily — inventing an anomaly instead of reporting one. A garbage prefix still does not rescue a garbage hash: the remainder goes through password_get_info(). --- lib/Command/SecuritySelfTest.php | 6 +++- lib/Security/HashAlgorithm.php | 8 +++++ lib/Security/SecuritySelfTest.php | 12 +++++++- tests/unit/Security/HashAlgorithmTest.php | 20 +++++++++++++ tests/unit/Security/SecuritySelfTestTest.php | 31 ++++++++++++++++++++ 5 files changed, 75 insertions(+), 2 deletions(-) diff --git a/lib/Command/SecuritySelfTest.php b/lib/Command/SecuritySelfTest.php index 29ad8d5..870fc16 100644 --- a/lib/Command/SecuritySelfTest.php +++ b/lib/Command/SecuritySelfTest.php @@ -91,7 +91,11 @@ protected function execute(InputInterface $input, OutputInterface $output): int if ($format === self::OUTPUT_FORMAT_PLAIN) { $this->writePlain($output, $report); } else { - $flags = JSON_UNESCAPED_SLASHES | ($format === self::OUTPUT_FORMAT_JSON_PRETTY ? JSON_PRETTY_PRINT : 0); + // JSON_INVALID_UTF8_SUBSTITUTE backs up the sanitising the service + // already does on environment-derived labels: an unencodable byte + // must degrade one field, never cost the entire artifact. + $flags = JSON_UNESCAPED_SLASHES | JSON_INVALID_UTF8_SUBSTITUTE + | ($format === self::OUTPUT_FORMAT_JSON_PRETTY ? JSON_PRETTY_PRINT : 0); $json = json_encode($report, $flags); if ($json === false) { $errors->writeln('Could not encode the evidence artifact: ' . json_last_error_msg() . ''); diff --git a/lib/Security/HashAlgorithm.php b/lib/Security/HashAlgorithm.php index ce7d4e9..8386fd6 100644 --- a/lib/Security/HashAlgorithm.php +++ b/lib/Security/HashAlgorithm.php @@ -98,6 +98,14 @@ public static function parametersFromStoredHash(string $stored): array { * Splits `|` and returns the hash part, mirroring the private * `OC\Security\Hasher::splitHash()`. Returns null when the value carries no * version prefix, i.e. when it is a legacy hash. + * + * The loose `(int)` cast is deliberate, not an oversight: upstream splits on + * `(int)$explodedString[0] > 0`, so Nextcloud itself accepts `3foo|` + * as version 3 and verifies the remainder as argon2id. This is evidence + * about what the instance actually does, so a stricter digits-only rule + * would report `unknown` for a value Nextcloud verifies happily — inventing + * an anomaly rather than reporting one. Malformed *hashes* are still caught: + * the remainder goes through `password_get_info()`, which yields `unknown`. */ private static function stripVersionPrefix(string $stored): ?string { $parts = explode('|', $stored, 2); diff --git a/lib/Security/SecuritySelfTest.php b/lib/Security/SecuritySelfTest.php index bacfb50..e8a2f11 100644 --- a/lib/Security/SecuritySelfTest.php +++ b/lib/Security/SecuritySelfTest.php @@ -334,10 +334,20 @@ private function describeInstance(): array { ]; } + /** + * Environment values are arbitrary bytes, and both evidence channels encode + * the artifact as JSON: `json_encode()` returns false on invalid UTF-8, and + * the log writer would drop the context the same way. Substituting here + * rather than at the encode boundary keeps stdout and the Kibana line + * consistent, and keeps a malformed label from costing the whole artifact. + */ private function fromEnvironment(string $name): string { $value = getenv($name); + if (!is_string($value)) { + return ''; + } - return is_string($value) ? $value : ''; + return mb_convert_encoding($value, 'UTF-8', 'UTF-8'); } /** diff --git a/tests/unit/Security/HashAlgorithmTest.php b/tests/unit/Security/HashAlgorithmTest.php index dc6dcb1..26bd76a 100644 --- a/tests/unit/Security/HashAlgorithmTest.php +++ b/tests/unit/Security/HashAlgorithmTest.php @@ -61,6 +61,26 @@ public static function provideStoredHashes(): array { // An unknown future hasher version still classifies by the inner hash. 'unknown future version prefix' => ['9|' . self::stripPrefix(self::REAL_STORED_ARGON2ID), HashAlgorithm::ARGON2ID], + + // A trailing-garbage version prefix is accepted, deliberately: upstream + // splits on `(int)$parts[0] > 0`, so Nextcloud reads these as version 3 + // and verifies the remainder as argon2id. Reporting `unknown` here would + // invent an anomaly the instance does not actually have. See + // HashAlgorithm::stripVersionPrefix(). + 'version prefix with trailing garbage' => [ + '3foo|' . self::stripPrefix(self::REAL_STORED_ARGON2ID), + HashAlgorithm::ARGON2ID, + ], + 'version prefix with leading whitespace' => [ + ' 3|' . self::stripPrefix(self::REAL_STORED_ARGON2ID), + HashAlgorithm::ARGON2ID, + ], + // ...but a garbage prefix does not rescue a garbage hash. + 'trailing-garbage prefix with garbage hash' => ['3foo|nonsense', HashAlgorithm::UNKNOWN], + 'negative prefix is not a version' => [ + '-1|' . self::stripPrefix(self::REAL_STORED_ARGON2ID), + HashAlgorithm::UNKNOWN, + ], ]; // Freshly generated hashes, so the classifier is exercised against what diff --git a/tests/unit/Security/SecuritySelfTestTest.php b/tests/unit/Security/SecuritySelfTestTest.php index 8368a2b..f690d68 100644 --- a/tests/unit/Security/SecuritySelfTestTest.php +++ b/tests/unit/Security/SecuritySelfTestTest.php @@ -134,6 +134,37 @@ public function testInstanceMetadataComesFromTheEnvironment(): void { $this->assertSame('production', $report['instance']['environment']); } + /** + * Environment values are arbitrary bytes. Both evidence channels encode the + * artifact as JSON, and `json_encode()` returns false on invalid UTF-8 — so + * an unencodable label must degrade its own field rather than cost the whole + * artifact on stdout and the context of the Kibana line. + */ + public function testInvalidUtf8InTheEnvironmentStillYieldsAnEncodableArtifact(): void { + $this->stubHasher(self::argon2id()); + $this->stubConfig(); + $this->stubDatabase([self::argon2id()]); + + // A lone 0x80 continuation byte: valid Latin-1, never valid UTF-8. + putenv("INSTANCE_NAME=ncw-\x80-prod"); + try { + $report = $this->selfTest()->run(); + } finally { + putenv('INSTANCE_NAME'); + } + + $this->assertTrue( + mb_check_encoding($report['instance']['name'], 'UTF-8'), + 'the instance label must be valid UTF-8 once sanitised', + ); + $this->assertNotFalse( + json_encode($report), + 'the artifact must survive json_encode() without the substitute flag', + ); + $this->assertStringContainsString('ncw-', $report['instance']['name']); + $this->assertStringContainsString('-prod', $report['instance']['name']); + } + public function testInstanceMetadataIsEmptyWithoutEnvironmentVariables(): void { $this->stubHasher(self::argon2id()); $this->stubConfig();