Repository navigation
fix(server): warn on unsupported password hashes and fix hash docs - #355
Merged
Merged
Conversation
The UserDefinition::password_hash rustdoc told operators to run `openssl passwd -6`, which yields a SHA-crypt hash the verifier rejects, so a user created that way could never log in. The rustdoc now recommends Argon2id via `bssh-server hash-password`, notes bcrypt is accepted, and states that SHA-crypt and /etc/shadow values are unsupported. reload_users now warns once per final user whose hash is neither Argon2 (parseable) nor bcrypt, without logging the hash. Loading still succeeds and verify_internal dispatch is unchanged, so auth stays fail-closed. Adds tests for the format helper, verifier construction with unsupported hashes, and a regression matrix for `*`, `!`, empty, and `$6$` hashes across password lengths 0 to 1024. The config test fixture now uses an Argon2id-style placeholder. Refs #354
inureyes
added
status:review
The rewritten rustdoc on UserDefinition::password_hash was a single long line, while every other doc comment in src/server/config/types.rs wraps near 80 columns. Rewrap it without changing the wording. Narrow password and config tests pass. Refs #354
Document in the server configuration guide that SHA-crypt and /etc/shadow hashes are unsupported and now produce a load-time warning per affected user. Add an Unreleased changelog section covering the warning and the corrected password_hash rustdoc. Validation: - cargo fmt --all -- --check: ok - cargo test --lib server::auth::password: 18 passed Refs #354
This was referenced Nov 9, 2026
inureyes
added a commit
that referenced
this pull request
Nov 9, 2026
…ing (#357) Follow-up to #354 / #355. Users with a `$2x$` bcrypt hash can log in but triggered the load-time "cannot log in" warning, and users with an unusable stored hash returned faster than users with a real hash, which the 100 ms floor in `verify` only hides on fast hosts. Login outcomes are unchanged for every input; `verify_internal`'s `$2` dispatch is untouched. - `is_supported_hash_format`, the load-time warning text and the `UserDefinition::password_hash` rustdoc now list `$2x$`. - `verify_dummy_hash` now runs, matching the invalid-username and unknown-user branches, for: unsupported-format hashes (`*`, `!`, empty, `$6$`), malformed Argon2 hashes (the original `Err` is still returned, so `verify_password` still logs it and rejects), and malformed bcrypt hashes (bad version, truncated). - Argon2 PHC strings that parse but lack a salt or hash output (for example `$argon2id` or `$argon2id$v=19$m=19456,t=2,p=1`) are now treated as malformed: `is_supported_hash_format` rejects them, so they get the load-time warning, and `verify_argon2` returns the malformed-hash error, so they take the dummy-hash path. Before, argon2 reported them as a wrong password without hashing anything. - A `#[cfg(test)]` call counter on `PasswordVerifier` lets tests assert exactly one dummy hash per unusable-hash or unknown-user attempt and none for valid users, with no wall-clock assertions. It is absent from non-test builds. ## Changes during review The incomplete-Argon2 case was found by the security review and folded in here since it is the same unusable-hash timing gap #356 targets. Both tests covering it fail with the check removed. ## Verification `cargo fmt --all -- --check`, `cargo clippy --all-targets --all-features -- -D warnings` and `cargo clippy --lib -- -D warnings` are clean. `cargo test --no-fail-fast` passes every target except the four `tests/integration_test.rs` tests, which fail identically on clean `main` locally (localhost SSH sandbox) and are left to CI. Closes #356
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.
Aligns the server password hash docs with the formats the verifier accepts and surfaces unsupported hashes at load time instead of only per login attempt. Auth behavior is unchanged and still fails closed:
verify_internaldispatch is untouched.UserDefinition::password_hashrustdoc recommends Argon2id (bssh-server hash-password), accepts bcrypt ($2a$,$2b$,$2y$), and says SHA-crypt ($5$/$6$,openssl passwd -5/-6) and/etc/shadowvalues are rejected.is_supported_hash_formatinsrc/server/auth/password.rs.reload_userscalls it once per final user (after inline overrides) and warns for each unsupported hash, logging only the username, never the hash. Loading continues andreload_usersstill returnsOk(()).docs/architecture/server-configuration.mdnow states that SHA-crypt and/etc/shadowhashes are unsupported and produce a load-time warning.$6$fixture intest_yaml_parsing_comprehensiveis now an Argon2id-style placeholder.is_supported_hash_formathelper, verifier construction with*and$6$users, and a regression matrix for the Alpine OpenSSH bypass attack shape over*,!, empty and$6$hashes with password lengths 0, 1, 72, 256, 257, 1024, plus Argon2id and bcrypt users still accepting the right password and rejecting a wrong one.Note: a
$2x$hash would get the warning even though bcrypt accepts it at login. The helper follows the prefix set specified in #354; this is a false warning only, not an auth change.Verification:
cargo fmt --all -- --checkandcargo clippy --all-targets --all-features -- -D warningsare clean, andcargo testlib passes (1963 passed). The fourtests/integration_test.rsfailures reproduce on cleanmainlocally (localhost SSH sandbox limitation) and are left to CI.Closes #354