keepassxreboot / keepassxc

KeePassXC is a cross-platform community-driven port of the Windows application “Keepass Password Safe”.
https://keepassxc.org/
Other
21.29k stars 1.48k forks source link

Show password entropy when setting database password #124

Closed droidmonkey closed 6 years ago

droidmonkey commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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.

droidmonkey commented 7 years ago

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

phoerious commented 7 years ago

I agree. The redesign effort is quite small, but this should still be postponed until the next release. The missing entropy meter is not a critical release-blocking issue.

dannysu commented 7 years ago

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 commented 7 years ago

@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.

frostasm commented 7 years ago

guys, has anyone started working on this task?

frostasm commented 7 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

This is being addressed in 2.4.0 under PR #1952