Skip to content
This repository has been archived by the owner on Jul 27, 2022. It is now read-only.

Problem: (CRO-577) Client allows weak passphrases #705

Merged
merged 1 commit into from
Dec 20, 2019
Merged

Problem: (CRO-577) Client allows weak passphrases #705

merged 1 commit into from
Dec 20, 2019

Conversation

devashishdxt
Copy link
Collaborator

Solution: Estimate password strength using zxcvbn crate. Return Error when password score is less than 3.

@devashishdxt
Copy link
Collaborator Author

bors try

bors bot added a commit that referenced this pull request Dec 19, 2019
Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks ok, sans small dependency bloat and mild testing/development inconvenience

Comment on lines +195 to +199
[[package]]
name = "bit-vec"
version = "0.5.1"
source = "registry+https://github.com/rust-lang/crates.io-index"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is a bit annoying / bloaty, as there's already bit-vec 0.6.1 (and bitvec).

probably open an issue for another dependency review / cleanup

Comment on lines 112 to 117
return Err(Error::new(
ErrorKind::IllegalInput,
format!(
"Weak passphrase: {}",
parse_feedback(password_entropy.feedback().as_ref())
),
Copy link
Contributor

@tomtau tomtau Dec 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is fine, but not sure how annoying it'd be in quick testing -- we'll see, perhaps later there could be a "unsafe_development_only" flag to bypass that

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add following code in future to only build this in release builds:

if !cfg!(debug_assertions) {
    // Check password score only in non-debug builds
}

@bors
Copy link
Contributor

bors bot commented Dec 19, 2019

try

Build failed

@devashishdxt
Copy link
Collaborator Author

bors try

bors bot added a commit that referenced this pull request Dec 19, 2019
@bors
Copy link
Contributor

bors bot commented Dec 19, 2019

try

Build failed

@codecov
Copy link

codecov bot commented Dec 19, 2019

Codecov Report

Merging #705 into master will decrease coverage by 0.05%.
The diff coverage is 53.84%.

@@            Coverage Diff             @@
##           master     #705      +/-   ##
==========================================
- Coverage   69.69%   69.64%   -0.06%     
==========================================
  Files         131      131              
  Lines       16742    16767      +25     
==========================================
+ Hits        11669    11678       +9     
- Misses       5073     5089      +16
Impacted Files Coverage Δ
...work/src/network_ops/default_network_ops_client.rs 79.2% <100%> (ø) ⬆️
client-core/src/wallet/syncer.rs 60.21% <100%> (ø) ⬆️
client-core/src/signer/wallet_signer.rs 85.82% <100%> (ø) ⬆️
client-core/src/service/hd_key_service.rs 83.63% <100%> (ø) ⬆️
...tion_builder/default_wallet_transaction_builder.rs 91.66% <100%> (ø) ⬆️
client-rpc/src/rpc/multisig_rpc.rs 33.65% <100%> (ø) ⬆️
client-core/src/wallet/syncer_logic.rs 93.01% <100%> (ø) ⬆️
client-common/src/error.rs 67.12% <100%> (+0.45%) ⬆️
client-rpc/src/rpc/wallet_rpc.rs 64.8% <77.77%> (+0.08%) ⬆️
client-core/src/wallet/default_wallet_client.rs 52.39% <8.33%> (-1.96%) ⬇️
... and 3 more

@devashishdxt
Copy link
Collaborator Author

bors try

bors bot added a commit that referenced this pull request Dec 19, 2019
@bors
Copy link
Contributor

bors bot commented Dec 19, 2019

try

Build failed

Solution: Estimate password strength using zxcvbn crate. Return Error when password score is less than 3.
@devashishdxt
Copy link
Collaborator Author

bors try

bors bot added a commit that referenced this pull request Dec 20, 2019
Copy link
Contributor

@leejw51 leejw51 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@tomtau
Copy link
Contributor

tomtau commented Dec 20, 2019

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 20, 2019

try

Build failed

@bors
Copy link
Contributor

bors bot commented Dec 20, 2019

This PR was included in a batch with a merge conflict, it will be automatically retried

bors bot added a commit that referenced this pull request Dec 20, 2019
705: Problem: (CRO-577) Client allows weak passphrases r=tomtau a=devashishdxt

Solution: Estimate password strength using `zxcvbn` crate. Return `Error` when password score is less than 3.

Co-authored-by: Devashish Dixit <devashish@crypto.com>
@bors
Copy link
Contributor

bors bot commented Dec 20, 2019

@bors bors bot merged commit 9884d2a into crypto-com:master Dec 20, 2019
@devashishdxt devashishdxt deleted the password-score branch December 20, 2019 05:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants