feat(strata-cli)!: add hash version and optional password #392
Conversation
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #392 +/- ##
==========================================
- Coverage 57.35% 57.17% -0.19%
==========================================
Files 255 255
Lines 26953 27008 +55
==========================================
- Hits 15459 15441 -18
- Misses 11494 11567 +73
|
There was a problem hiding this comment.
@Zk2u can you please check the impl?
@AaronFeickert can you check if the messages are proper?
storopoli
force-pushed
the
STR-493-allow-user-to-enter-empty-password
branch
from
41239e4
to
af124fa
Compare
bin/strata-cli/src/cmd/change_pwd.rs
Outdated
| let entropy = new_pw.entropy(); | ||
| let _ = term.write_line( | ||
| format!( | ||
| "Password strength (estimated crack time): {}", |
There was a problem hiding this comment.
What about?
| "Password strength (estimated crack time): {}", | |
| "Password strength (estimated time needed by an attacker): {}", |
There was a problem hiding this comment.
I think it's neat that the library can do time estimates, but I'm not sure it's particularly useful for the end user. Is a decade sufficiently strong? A thousand years? It's not really actionable.
My recommendation is instead to use the score provided by the library (but without showing it to the user directly). If the score is at least 3 (sufficiently strong), don't provide any feedback. If the score is below that threshold, warn the user and consider providing feedback.
There was a problem hiding this comment.
Thanks for the suggestions, totally agreed.
I've implemented in b3709c1
There was a problem hiding this comment.
PS: Screenshots updated
bin/strata-cli/src/seed/password.rs
Outdated
| HashVersion::V0 => ( | ||
| Algorithm::Argon2id, | ||
| Version::V0x13, | ||
| Params::new(19_456, 2, 1, None), |
There was a problem hiding this comment.
Recommend including a citation comment to the relevant OWASP recommendation to justify these magic numbers.
There was a problem hiding this comment.
Also recommend explicitly specifying the output length instead of relying on the None default behavior for the parameters.
There was a problem hiding this comment.
Good catch, done in 953d9f4
storopoli
force-pushed
the
STR-493-allow-user-to-enter-empty-password
branch
from
af124fa
to
b3709c1
Compare
| @@ -12,6 +13,17 @@ pub struct ChangePwdArgs {} | |||
| pub async fn change_pwd(_args: ChangePwdArgs, seed: Seed, persister: impl EncryptedSeedPersister) { | |||
| let term = Term::stdout(); | |||
| let mut new_pw = Password::read(true).unwrap(); | |||
| let entropy = new_pw.entropy(); | |||
| let _ = term.write_line(format!("Password strength (Overall strength score from 0-4, where anything below 3 is too weak): {}", entropy.score()).as_str()); | |||
There was a problem hiding this comment.
It's a nit, but I wouldn't even bother showing the user the score, since it's not really actionable on its own. If the passphrase is weak and there's feedback, let the user know. If it's strong, just set the passphrase.
| @@ -188,6 +195,17 @@ pub fn load_or_create( | |||
| }; | |||
|
|
|||
| let mut password = Password::read(true).map_err(OneOf::new)?; | |||
| let entropy = password.entropy(); | |||
| let _ = term.write_line(format!("Password strength (Overall strength score from 0-4, where anything below 3 is too weak): {}", entropy.score()).as_str()); | |||
|
Fntests failing, it's probably the flaky test. |
* feat(strata-cli)!: add hash version and optional password * feat(strata-cli): warn user about password strength
* feat(strata-cli)!: add hash version and optional password * feat(strata-cli): warn user about password strength
* feat(strata-cli)!: add hash version and optional password * feat(strata-cli): warn user about password strength
* feat(strata-cli)!: add hash version and optional password * feat(strata-cli): warn user about password strength
Description
HashVersionenum to the wallet encryption.zxcvbn.Type of Change
Checklist
Related Issues
STR-493 and STR-500.
Screenshot
Empty or very low entropy password:
