Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show password entropy when setting database password #124

Closed
droidmonkey opened this issue Dec 3, 2016 · 12 comments · Fixed by #1952
Closed

Show password entropy when setting database password #124

droidmonkey opened this issue Dec 3, 2016 · 12 comments · Fixed by #1952
Assignees
Milestone

Comments

@droidmonkey
Copy link
Member

When you create a new DB or change an existing one the password entry doesn't indicate the strength of the entered password.

Expected Behavior

The password entry would behave and look the same as for database entries.

Current Behavior

Only the password entries are shown.

Possible Solution

Add the password generator widget to the database password widget.

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Dec 3, 2016

All the password fields are PasswordEdit widget (the parent and the repeat).

In the password generator widget there is only one QLineEdit instead of two PasswordEdit. The entropy meter is added to the QLineEdit.

The issue can be resolved by adding the entropy meter to the PasswordEdit parent and disable it in the password generator or instead deleting the QLineEdit in the password generator and emit the password directly in the PasswordEdit.

@phoerious
Copy link
Member

phoerious commented Dec 7, 2016

Before we implement that, I would suggest, we add a few things to the PasswordEdit widget. Right now, the password visibility toggle is not part of the widget and re-implemented about three or four times in total throughout the KeePassXC source code. That's unnecessary code duplication and a possible source of inconsistency. Also the interaction between primary and secondary (confirmation) password input fields is not quite as encapsulated as it should be. Once that is done, the widget should also be used when creating a new database.

@phoerious phoerious self-assigned this Dec 7, 2016
@droidmonkey
Copy link
Member Author

Concur, i think this is turning into a redesign effort and should be pushed to v2.2.0

@phoerious
Copy link
Member

phoerious commented Dec 7, 2016 via email

@phoerious phoerious modified the milestones: v2.2.0, v2.1.0 Dec 7, 2016
@dannysu
Copy link
Contributor

dannysu commented Jan 2, 2017

Update: Ah nevermind, zxcvbn is simply an estimation and not the actual entropy.

Please correct me if I'm wrong, but isn't the password entropy a factor of how a password was generated?
If a user simply types in whatever master password to be used, then isn't it impossible to show the entropy?

@phoerious phoerious removed their assignment Jan 14, 2017
@phoerious
Copy link
Member

phoerious commented Jan 17, 2017

@louib To answer your question from IRC: yes, the entropy meter should be part of the PasswordEdit widget. We basically want to have it everywhere where passwords are entered or generated. However, making its display optional by a settings property would still be a good idea.

@phoerious phoerious self-assigned this Feb 26, 2017
@droidmonkey droidmonkey modified the milestones: v2.2.0, v2.2.1 May 30, 2017
@droidmonkey droidmonkey modified the milestones: v2.2.1, v2.3.0 Sep 23, 2017
@frostasm
Copy link
Contributor

guys, has anyone started working on this task?

@frostasm
Copy link
Contributor

There is a wonderful set of widgets wwWidgets, which contains a QLineEdit widget with built-in button. We can use the code as a base to build our own AdvancedLineEdit widget.

@phoerious
Copy link
Member

I don't think we need to copy code from third-party libraries. Creating a self-contained PasswordEdit widget is rather trivial, but requires some work. Somebody simply needs to do it.

@gwillen
Copy link

gwillen commented Jun 20, 2018

I strongly suggest that the master password dialog gain a copy of (or be replaced with) the generate-passphrase widget, per the original "possible solution" to this issue. (Versus just adding an entropy estimator.) Inviting the user to hand-create a password is not a best practice and will lead to the use of master passwords with inadequate entropy. They should be strongly encouraged to generated a passphrase instead.

(While I'm here, I notice that there isn't an option to select or calibrate the difficulty of the key derivation function in this dialog -- i.e. to make opening the database slower so it's harder to break. It doesn't seem like KeePassX had this either, or I just can't find it, but KeePass does.) #

@MohamadNajem
Copy link

Hey,

I am in a training session with several digital security practitioners and this issue has come up while evaluating KeePassXC to include in the toolset we provide to our beneficiaries. We were all surprised that there is no password strength indicator when creating the master password for the DB.

Glad to see this issue has already been submitted. Hope it is resolved soon.

Many thanks and great work.

@droidmonkey
Copy link
Member Author

This is being addressed in 2.4.0 under PR #1952

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants