feat(security): verify argon2id password hashing via occ selftest (NSW-957) - #38
Open
printminion-co wants to merge 8 commits into
Open
feat(security): verify argon2id password hashing via occ selftest (NSW-957)#38printminion-co wants to merge 8 commits into
printminion-co wants to merge 8 commits into
Conversation
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 <kupriyanov@strato.de>
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$<salt>$<hash>
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-<random> 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 <kupriyanov@strato.de>
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 <kupriyanov@strato.de>
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 <kupriyanov@strato.de>
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 <kupriyanov@strato.de>
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 <kupriyanov@strato.de>
printminion-co
force-pushed
the
mk/dev/NSW-957-password-hashing-selftest
branch
from
September 1, 2026 16:23
50f6471 to
ccb90de
Compare
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.
There was a problem hiding this comment.
🟡 Changes recommended
The hash-version prefix parsing is too permissive (can misclassify invalid prefixed values), and the JSON artifact output path can still short-circuit without emitting an artifact on encoding failure.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds an occ ncw_tools:security:selftest command plus supporting service logic, documentation, and tests to verify that password hashing is argon2id and to emit a structured evidence artifact for C5 control PSS-07 within the real Nextcloud runtime.
Changes:
- Introduces
SecuritySelfTestservice +SecuritySelfTestocc command that surveys stored hashes, verifies hardening config, and optionally performs a round-trip probe user check. - Adds
HashAlgorithmclassifier to correctly interpret Nextcloud’s version-prefixed stored hashes and extract effective cost parameters. - Adds comprehensive unit + integration test coverage, documentation, and psalm stubs/suppressions needed for static analysis.
File summaries
| File | Description |
|---|---|
| lib/Command/SecuritySelfTest.php | New occ command wrapper that validates options, emits artifact to stdout, and logs structured evidence. |
| lib/Security/SecuritySelfTest.php | New service that generates the evidence artifact: hasher probe, stored distribution survey, hardening checks, optional round-trip. |
| lib/Security/HashAlgorithm.php | New classifier for Nextcloud’s stored password hash formats (version-prefixed + legacy) and parameter extraction. |
| tests/unit/Security/SecuritySelfTestTest.php | Unit tests for artifact shape, pass/fail conditions, distribution counting, and round-trip behavior/invariants. |
| tests/unit/Security/HashAlgorithmTest.php | Unit tests for hash classification and parameter extraction, including regression cases for version prefixes. |
| tests/integration/SecuritySelfTestIntegrationTest.php | Integration tests exercising the real DB/user pipeline, including round-trip creation/deletion and “no hash material” invariant. |
| docs/security-selftest.md | User/operator documentation for the command, artifact schema, failure modes, and Kibana caveats. |
| docs/README.md | Documentation index updated to include the new security self-test command docs. |
| appinfo/info.xml | Registers the new occ command with Nextcloud. |
| psalm.xml | Adds psalm stub directory and suppressions for DI-registered classes/constructors. |
| tests/psalm-stubs/OC/Core/Command/Base.php | Psalm stub for Nextcloud private OC\Core\Command\Base used by the command. |
| tests/psalm-stubs/Symfony/Component/Console/Input/InputInterface.php | Psalm stub for Symfony Console input interface used by the command. |
| tests/psalm-stubs/Symfony/Component/Console/Input/InputOption.php | Psalm stub for Symfony Console InputOption constants used by the command. |
| tests/psalm-stubs/Symfony/Component/Console/Output/OutputInterface.php | Psalm stub for Symfony Console output interface used by the command. |
| tests/psalm-stubs/Symfony/Component/Console/Output/ConsoleOutputInterface.php | Psalm stub for ConsoleOutputInterface used to separate stderr from stdout. |
| REUSE.toml | Adds the new documentation file to REUSE coverage. |
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+94
to
+99
| $flags = JSON_UNESCAPED_SLASHES | ($format === self::OUTPUT_FORMAT_JSON_PRETTY ? JSON_PRETTY_PRINT : 0); | ||
| $json = json_encode($report, $flags); | ||
| if ($json === false) { | ||
| $errors->writeln('<error>Could not encode the evidence artifact: ' . json_last_error_msg() . '</error>'); | ||
| return 1; | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Good catch — fixed in 563d951, though one level deeper than suggested.
JSON_INVALID_UTF8_SUBSTITUTE alone would have fixed stdout while leaving the other evidence channel exposed: the same report goes to LoggerInterface, and the log writer encodes that context as JSON too, so invalid UTF-8 would drop the Kibana line — which is the channel NSW-957 actually requires. So the sanitising now happens where the untrusted bytes are read (SecuritySelfTest::fromEnvironment(), the source of INSTANCE_NAME / NAMESPACE / ENVIRONMENT), which protects both channels and keeps them consistent. The flag stays on the command as a backstop.
Regression test: testInvalidUtf8InTheEnvironmentStillYieldsAnEncodableArtifact. Confirmed it fails without the fix (Failed asserting that false is true on mb_check_encoding) and passes with it.
Comment on lines
+102
to
+113
| 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]; | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Deliberate, and I am keeping the behaviour — but you are right that it was undocumented and untested, so 563d951 fixes that.
The loose (int) cast mirrors upstream exactly. OC\Security\Hasher::splitHash():
$explodedString = explode("|", $prefixedHash, 2);
if (\count($explodedString) === 2) {
if ((int)$explodedString[0] > 0) {So Nextcloud itself reads 3foo|<hash> as version 3 and verifies the remainder as argon2id — validate() accepts it too. This command produces audit evidence about what the instance actually does, so classifying that value as argon2id is the truthful answer. Tightening to digits-only would report unknown for a value Nextcloud verifies happily, i.e. invent an anomaly rather than report one.
On "as described in the docs/tests": the doc says the classifier splits the prefix "mirroring the private Hasher::splitHash()", which is what it does — it does not claim a digits-only rule. No contradiction, though the intent was too implicit.
A malformed prefix still does not launder a malformed hash: the remainder goes through password_get_info(), so 3foo|nonsense is unknown.
Added: an explanatory comment on stripVersionPrefix(), and four cases pinning the behaviour — 3foo|<argon2id> and " 3|"<argon2id> classify as argon2id, 3foo|nonsense and -1|<argon2id> as unknown.
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|<hash>' 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().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
occ ncw_tools:security:selftest, which verifies that this instance hashes passwords with argon2id and emits a structured evidence artifact for C5 control PSS-07 (Credential Storage Security).Ticket: NSW-957. Supersedes the shell-based approach in
nextcloud-workspace/images!114.Why it lives here rather than in a shell script
The original proposal verified the algorithm from a standalone PHP script in the
nc-managercontainer, outside the Nextcloud runtime. That is what caused its central defect: it comparedoc_users.passwordagainst the literal prefix$argon2id$, butHasher::hash()returns3 . '|' . password_hash(...)andDatabase::createUser()stores exactly that — so the column holds3|$argon2id$v=19$…and the comparison could never match. The self-test reported a permanent FAIL on a correctly configured instance.Inside the app we get
IHasher(the authority on the algorithm),IConfig(the effective merged config, not one file),IDBConnection(correct driver, table prefix and replica handling instead of a raw PDO DSN hardcoded to MySQL) — and unit tests, which is what actually prevents a recurrence.What it checks
IHasherand classifies the result. This is the algorithm every new password receives.oc_users, so the evidence attests the real user population rather than a fixture. Passes when every row isargon2idor empty (empty = SSO-only accounts, not a downgrade).hashing_default_passwordis false (the one system value that silently downgradesHashertoPASSWORD_DEFAULT), bruteforce and rate-limit protection enabled,overwriteprotocolhttps, andpasswordsalt/secretpresent. Reports the effective argon2 cost parameters as evidence.--round-trip(opt-in) — the ticket's literal scenario: create a disposable user, read its stored hash back from the database, classify it, delete it. Off by default so the command is safe against a production instance; enabled via Helm only on the dedicated self-test instance, which has no live users. The test user never receives an email address.Output
--output=jsonwrites the artifact to stdout; the same artifact is logged throughLoggerInterface, which reaches Kibana vialog_type=errorlog. Exit 0 = PASS, 1 = FAIL, 2 = usage error.stdout is JSON-only — all diagnostics go to stderr — because
nc-manager/bin/selftest.shpipes it intojq. A FAIL still prints the complete artifact before exiting 1, since that is exactly the case the evidence needs to capture.No field ever carries hash material, a salt, or a secret value. Only algorithm names, counts, booleans and cost parameters. The secret checks are keyed
passwordsalt_present/secret_presentsoactual: truecannot be misread as a value. There is a unit test for this invariant.Verified
test:unittest:integrationpsalmcs:checkreuse lintEach commit was verified against every gate individually, not just the tip.
Against a real instance,
configured_algorithmreportsargon2idfrom a3|$argon2id$…stored hash — the case the shell version got wrong — and--round-tripreturnsstored_algorithm: argon2id,cleaned_up: truewith no leftover user. Flippinghashing_default_passwordto true is caught three ways:configured_algorithm: bcrypt,round_trip.stored_algorithm: bcrypt, andparametersswitching from the argon2 triple to{"cost":10}.Notes for review
composer.json/composer.lockare untouched.OC\Core\Command\Baseand four Symfony Console interfaces are stubbed undertests/stubs/for psalm, following the pattern inionos_tools.psalm.xmlsuppressions were added for the command class and constructors, mirroring the existing entries forApplication—findUnusedCodecannot see registrations inappinfo/info.xml.security_config.parametersis read frompassword_get_info()on the probe hash rather than fromIConfig, becauseHasherclamps the configured values to the algorithm minimums. The reported values are therefore the effective ones.data.result,data.schema_versionanddata.timestampare directly queryable;data.instance,data.password_hashinganddata.security_configarrive as strings needing a parse. Consumers wanting structured nested fields should use the stdout artifact. Documented indocs/security-selftest.md.pdo_sqlite/pdo_pgsqlonly); thephpunit-mysqlworkflow covers it. The survey usesIQueryBuilderwith the unprefixed table nameusers, so no dialect-specific SQL is involved.Follow-ups in other repos
nextcloud-workspace/imagesfeature/NSW-957—selftest.shinvokes this command and pipes the artifact intosend-report.sh.nextcloud-workspace/helmfeature/NSW-957—selftest.enabled/roundTrip/failDeployflags.ncw-server— submodule pointer bump, after this merges.