refact(password): Store permanent password as hashed verifier#507
refact(password): Store permanent password as hashed verifier#507fufesou wants to merge 4 commits intorustdesk:mainfrom
Conversation
Signed-off-by: fufesou <linlong1266@gmail.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPermanent password storage is migrated from plaintext to a salted SHA256 verifier with on-load/on-store migration. Config gains hashing/decoding/verification APIs, stricter salt/update normalization, and refuses unsafe sync/overwrite actions. Tests added for migration and normalization rules. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/config.rs (2)
1560-1568:⚠️ Potential issue | 🟠 MajorHash plaintext
Config::set()updates before storing them in memory.When the current password is empty or legacy,
normalize_incoming_permanent_password_update()returns without canonicalizingincoming.password.Config::set()then keeps plaintext inCONFIGand only hashes a clone insidestore(). Until restart, password checks fall back to plaintext comparison andset_salt()still allows salt changes.Suggested fix
fn normalize_incoming_permanent_password_update(current: &Config, incoming: &mut Config) { if !incoming.password.is_empty() && is_permanent_password_hashed_storage(&incoming.password) && incoming.salt.is_empty() { @@ let current_is_hashed = !current.password.is_empty() && is_permanent_password_hashed_storage(¤t.password); if !current_is_hashed { + let _ = Self::migrate_permanent_password_to_hashed_storage(incoming); return; }Also applies to: 1578-1593
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 1560 - 1568, Config::set currently can place plaintext or legacy passwords into CONFIG because normalize_incoming_permanent_password_update may return early; move or duplicate the hashing step so the in-memory update never stores plaintext: after calling normalize_incoming_permanent_password_update(&lock, &mut cfg) but before assigning *lock = cfg, ensure the incoming cfg.password is canonicalized/hashed (the same hash logic used by store()) so the value written into CONFIG is the hashed password; also ensure set_salt() checks/operates on the hashed password in CONFIG (not a plaintext field). Update Config::set to perform the hashing of cfg (or call the existing helper used by store()) prior to *lock = cfg so temporary plaintext is never kept in memory.
1560-1575:⚠️ Potential issue | 🟠 MajorClear trusted devices when
Config::set()changes the permanent password.This path can now accept hashed→hashed password replacements, but unlike
set_permanent_password()andset_permanent_password_storage_for_sync()it never invalidates trusted devices. A sync-driven password change will therefore keep remembered devices trusted.Suggested fix
pub fn set(cfg: Config) -> bool { let mut cfg = cfg; let mut lock = CONFIG.write().unwrap(); Self::normalize_incoming_permanent_password_update(&lock, &mut cfg); + let password_changed = lock.password != cfg.password || lock.salt != cfg.salt; if *lock == cfg { return false; } *lock = cfg; lock.store(); @@ #[cfg(target_os = "macos")] let new_key_pair = lock.key_pair.clone(); drop(lock); + if password_changed { + Self::clear_trusted_devices(); + } #[cfg(target_os = "macos")] Self::invalidate_key_pair_cache_if_changed(&new_key_pair); true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 1560 - 1575, Config::set currently misses invalidating remembered devices when a permanent password changes; detect whether the normalized permanent password actually changed by comparing lock.permanent_password with cfg.permanent_password after calling Self::normalize_incoming_permanent_password_update, and if different call the same trusted-device clearing logic used by set_permanent_password / set_permanent_password_storage_for_sync (e.g. invoke the helper that clears/invalidate trusted devices) before storing the new config and before dropping the CONFIG lock (i.e., do this while you still hold lock, then proceed to *lock = cfg; lock.store(); ... drop(lock); and keep the existing key‑pair cache invalidation flow using new_key_pair/invalidate_key_pair_cache_if_changed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config.rs`:
- Around line 1288-1303: The sync setter
set_permanent_password_storage_for_sync() currently overwrites config.salt which
bypasses the immutability enforced by set_salt() and Config::set(); change it to
refuse to change an already-set salt: if config.salt is non-empty (or otherwise
marked immutable) and differs from the incoming salt, return an Err (or
propagate the same "Refusing to persist..." style error); only accept and assign
the incoming salt when config.salt is empty (or unset), and otherwise require
equality with the incoming salt before updating config.password or returning Ok.
Update the implementation in set_permanent_password_storage_for_sync() to
perform that check and avoid config.salt = salt.to_owned() when a differing salt
is already present.
---
Outside diff comments:
In `@src/config.rs`:
- Around line 1560-1568: Config::set currently can place plaintext or legacy
passwords into CONFIG because normalize_incoming_permanent_password_update may
return early; move or duplicate the hashing step so the in-memory update never
stores plaintext: after calling
normalize_incoming_permanent_password_update(&lock, &mut cfg) but before
assigning *lock = cfg, ensure the incoming cfg.password is canonicalized/hashed
(the same hash logic used by store()) so the value written into CONFIG is the
hashed password; also ensure set_salt() checks/operates on the hashed password
in CONFIG (not a plaintext field). Update Config::set to perform the hashing of
cfg (or call the existing helper used by store()) prior to *lock = cfg so
temporary plaintext is never kept in memory.
- Around line 1560-1575: Config::set currently misses invalidating remembered
devices when a permanent password changes; detect whether the normalized
permanent password actually changed by comparing lock.permanent_password with
cfg.permanent_password after calling
Self::normalize_incoming_permanent_password_update, and if different call the
same trusted-device clearing logic used by set_permanent_password /
set_permanent_password_storage_for_sync (e.g. invoke the helper that
clears/invalidate trusted devices) before storing the new config and before
dropping the CONFIG lock (i.e., do this while you still hold lock, then proceed
to *lock = cfg; lock.store(); ... drop(lock); and keep the existing key‑pair
cache invalidation flow using
new_key_pair/invalidate_key_pair_cache_if_changed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c240f6ad-667f-413e-8c4c-2b73e6df8569
📒 Files selected for processing (2)
src/config.rssrc/password_security.rs
Signed-off-by: fufesou <linlong1266@gmail.com>
Signed-off-by: fufesou <linlong1266@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/config.rs (3)
53-64: Consider using a key derivation function for stronger offline attack resistance.The current design uses a single SHA256 iteration:
H1 = SHA256(password || salt). While this prevents plaintext storage, it's vulnerable to offline brute-force attacks if the config file is compromised. Password hashing best practices recommend key derivation functions like Argon2, bcrypt, or PBKDF2 with high iteration counts.If backward compatibility or performance constraints require SHA256, document this trade-off. The PR description mentions this is for verifier storage, so the threat model may be acceptable—but it's worth explicit consideration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 53 - 64, The function compute_permanent_password_h1 currently computes H1 with a single SHA256(password || salt), which is weak against offline brute force; replace its implementation to use a proper KDF (e.g., Argon2id, bcrypt, or PBKDF2 with a high iteration count) inside compute_permanent_password_h1 so the derived output matches PERMANENT_PASSWORD_H1_LEN (truncate or encode as needed), or if you must keep SHA256 for compatibility, add explicit documentation and a config flag noting the trade-off and threat model; update any callers that assume SHA256 output format and ensure salt handling and parameter storage (e.g., Argon2 params) are persisted for verification.
3250-3356: Good test coverage; consider adding tests for sync and verification APIs.The tests thoroughly cover:
- Hash encoding/decoding roundtrip
- Migration from plaintext (including "00"-prefixed edge case)
Config::set()normalization rulesConsider adding tests for:
set_permanent_password_storage_for_sync()behavior (valid sync, rejection cases)matches_permanent_password_plain()verification (correct password, wrong password, empty input)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 3250 - 3356, Add unit tests to cover the sync and verification APIs: write tests for set_permanent_password_storage_for_sync() that assert successful acceptance of a correctly-formed hashed storage value, and rejection/unchanged state for invalid inputs (e.g., plaintext or wrong prefix) and when salt mismatches; also add tests for matches_permanent_password_plain() verifying it returns true for the correct plaintext with the configured salt, false for wrong plaintext and empty input, and that it behaves correctly when the stored password is hashed vs plaintext. Locate these behaviors via the functions set_permanent_password_storage_for_sync and matches_permanent_password_plain and mirror the style of existing tests (construct Config instances, mutate password/salt, call the function, then assert expected password/salt and boolean outcomes).
45-48: Consider increasing salt length for stronger security.
DEFAULT_SALT_LEN = 6produces a salt from 32 characters (CHARS), yielding ~31 bits of entropy. While this provides basic uniqueness, OWASP recommends at least 128 bits of entropy for cryptographic salts. Consider increasing to 22+ characters or usingrand::thread_rng().gen::<[u8; 16]>()for direct byte generation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 45 - 48, DEFAULT_SALT_LEN is too small (6) for strong salts; increase it to provide ~128 bits of entropy (e.g., set DEFAULT_SALT_LEN to 22+ for char-based salts) or switch to byte-based salts (generate 16 random bytes via rand::thread_rng().gen::<[u8; 16]>()). Update the constant DEFAULT_SALT_LEN and, if the code that creates salts (where DEFAULT_SALT_LEN is used) expects character salts, adjust that logic to either use the longer length or replace it with secure byte generation and store/encode the bytes appropriately; ensure PERMANENT_PASSWORD_H1_LEN and any code consuming the salt are kept consistent with the new salt representation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config.rs`:
- Around line 1578-1580: Config::set currently replaces the config without
invalidating trusted devices when the permanent password or salt changes; update
Config::set to detect changes to permanent_password and/or salt by comparing the
incoming cfg.permanent_password and cfg.salt against the existing stored values,
and if either changed invoke the same trusted-device-clearing logic used by
set_permanent_password() / set_permanent_password_storage_for_sync() (or call
the shared helper those methods use) so synced password updates invalidate
trusted devices consistently; preserve existing behavior for non-password
changes.
---
Nitpick comments:
In `@src/config.rs`:
- Around line 53-64: The function compute_permanent_password_h1 currently
computes H1 with a single SHA256(password || salt), which is weak against
offline brute force; replace its implementation to use a proper KDF (e.g.,
Argon2id, bcrypt, or PBKDF2 with a high iteration count) inside
compute_permanent_password_h1 so the derived output matches
PERMANENT_PASSWORD_H1_LEN (truncate or encode as needed), or if you must keep
SHA256 for compatibility, add explicit documentation and a config flag noting
the trade-off and threat model; update any callers that assume SHA256 output
format and ensure salt handling and parameter storage (e.g., Argon2 params) are
persisted for verification.
- Around line 3250-3356: Add unit tests to cover the sync and verification APIs:
write tests for set_permanent_password_storage_for_sync() that assert successful
acceptance of a correctly-formed hashed storage value, and rejection/unchanged
state for invalid inputs (e.g., plaintext or wrong prefix) and when salt
mismatches; also add tests for matches_permanent_password_plain() verifying it
returns true for the correct plaintext with the configured salt, false for wrong
plaintext and empty input, and that it behaves correctly when the stored
password is hashed vs plaintext. Locate these behaviors via the functions
set_permanent_password_storage_for_sync and matches_permanent_password_plain and
mirror the style of existing tests (construct Config instances, mutate
password/salt, call the function, then assert expected password/salt and boolean
outcomes).
- Around line 45-48: DEFAULT_SALT_LEN is too small (6) for strong salts;
increase it to provide ~128 bits of entropy (e.g., set DEFAULT_SALT_LEN to 22+
for char-based salts) or switch to byte-based salts (generate 16 random bytes
via rand::thread_rng().gen::<[u8; 16]>()). Update the constant DEFAULT_SALT_LEN
and, if the code that creates salts (where DEFAULT_SALT_LEN is used) expects
character salts, adjust that logic to either use the longer length or replace it
with secure byte generation and store/encode the bytes appropriately; ensure
PERMANENT_PASSWORD_H1_LEN and any code consuming the salt are kept consistent
with the new salt representation.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37802208aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: fufesou <linlong1266@gmail.com>
2 participants
This PR changes
hbb_commonto store the permanent password as a non-reversible verifier rather than a reversible value on disk. It also preventssaltfrom being changed once the hashed storage format is active, so password verification remains stable across config sync paths.Issue
saltis part of the verifier derivation. Ifsaltcan change after switching to hashed storage, the stored verifier becomes unverifiable and effectively breaks the permanent password.Solution
01<base64(H1)>H1 = SHA256(password || salt)(32 bytes)H1and rewrite storage as01....Config::store()only encrypts legacy formats;01...is stored as-is.saltonce hashed storage is active:Config::set_salt()refuses updates whenpasswordis stored as01....Config::set(cfg)(used by config sync) is guarded sosalt(and the associated hashedpassword) cannot be silently overwritten.Notes / Risk
saltis intentionally long-lived to maintain verifier consistency.Summary by CodeRabbit