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

Health Check - Exclude entries from check #4168

Closed
OLLI-S opened this issue Jan 12, 2020 · 31 comments · Fixed by #4542
Closed

Health Check - Exclude entries from check #4168

OLLI-S opened this issue Jan 12, 2020 · 31 comments · Fixed by #4542

Comments

@OLLI-S
Copy link

OLLI-S commented Jan 12, 2020

Summary

I have some entries, where the password is a PIN (only 4 digits) but here only 4 digits are allowed.
I also have entries where I have unsafe passwords that I can not change (password of the website of my karate teacher, password of the emails of the karate dojo, where I manage the emails, email account of my brother, where I backup his password in KeePassXC).
So a checkbox "Exclude from health check" when editing an entry would really be helpful!
But maybe I should create a new issue here for this feature?

@droidmonkey droidmonkey changed the title Health Report - Exclude and Entry from Health check Health Report - Exclude entries from check Jan 13, 2020
@OLLI-S OLLI-S changed the title Health Report - Exclude entries from check Health Check - Exclude entries from check Mar 11, 2020
@wolframroesler
Copy link
Contributor

The new flag should exclude the entry not only from Health Check but also from the HIBP check (#1083).

@wolframroesler
Copy link
Contributor

But once again the question is, how do we store the new flag in the kdbx file so that other KeePass clients stay compatible? Cf. #4293 for discussion.

@droidmonkey
Copy link
Member

droidmonkey commented Mar 21, 2020

A simple flag like that should be stored in the custom data of the entry. Custom data is meant to be non-user editable attributes for plugins to use. Core features like "password modified time" can also be stored there, but it is not appropriate (per the conversation had in that thread). The benefit to custom data is that all KeePass clients read it in and write it out regardless on whether they use it or not.

@OLLI-S
Copy link
Author

OLLI-S commented Mar 22, 2020

The new flag should exclude the entry not only from Health Check but also from the HIBP check (#1083).

@wolframroesler Excellent idea, it makes no sense to check these passwords also against HIBP.

A simple flag like that should be stored in the custom data of the entry.

@droidmonkey I like the idea to store this flag in the custom data, also because there will be no problems with other KeePass clients.
And I would also store the Password Modification Date in the custom data, but this discussion is made in an other topic...

@rugk
Copy link

rugk commented Mar 22, 2020

Also for usability: In the health check windows, one should be able to just do a right-click -> "exclude from this check" (probably split it to "this check"/"from all checks" etc. depending on what is useful).

@OLLI-S
Copy link
Author

OLLI-S commented Mar 22, 2020

I rally love this idea @rugk has made.
Excluding entries is just one mouse click instead of many!

@wolframroesler
Copy link
Contributor

While @rugk's suggestion gives the user finely-grained control over the checking process, I'd rather keep it simple and have something like a single "this is a known bad password" flag for every entry. If the flag is set, the entry is excluded from Health Check and HIBP, and a new row "Number of known bad passwords" is added to the Statistics report. Setting and clearing the flag is possible from the entry editor dialog, and maybe it should also be possible to set (but, of course, not clear) it from the context menu of the Health Check and HIBP tables.

The flag is stored in custom data; hopefully the authors of other clients will pick it up instead of re-inventing it when they have a similar requirement. Still thinking about ways to document custom fields like this one in a way that other client authors can benefit from (some Wiki perhaps).

What do you think about that?

@droidmonkey
Copy link
Member

I like it, the flag should reset if the password is changed though

@wolframroesler
Copy link
Contributor

Not sure about that. The known bad flag usually means that the user can do nothing to improve the password (for example, my ATM PIN will always be four digits), so if the password changes (=I get a new banking card with a new PIN) it's probably still as bad as before. Do we really want to force the user to set the flag again? Wouldn't silently resetting the flag be surprising, like "why does this come up in Health Check again, I thought I set that flag"?

Which brings us to the question where in the edit dialog to put the checkbox for the known bad flag. On the "Properties" pane? Sounds logical (it has all the custom fields already) but people rarely go there, nobody will guess that this is where you set a crucial security flag (if you consider Health Check and HIBP crucial for your security workflow, inadvertently setting the flag for a password would reduce security). "Advanced" maybe? It's crowded already. "Entry" then, so it's in plain sight all the time? My idea is to put it prominently next to the password itself, so we don't need more vertical space. In this case, we don't have to clear the flag when the user changes the password because the user sees the checkbox all the time anyway.

@wolframroesler
Copy link
Contributor

How about this:

Screenshot from 2020-03-28 23 19 10

@wolframroesler
Copy link
Contributor

By the way, how long until the feature freeze for 2.6.0? Would be great if we could get this in along with Health Check and HIBP.

wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Mar 28, 2020
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Mar 28, 2020
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Mar 28, 2020
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Mar 28, 2020
Just a POC/design suggestion. Checkbox behavior still TBD.

Fixes keepassxreboot#4168
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Mar 28, 2020
Just a POC/design suggestion. Checkbox behavior still TBD.

Fixes keepassxreboot#4168
@droidmonkey
Copy link
Member

droidmonkey commented Mar 28, 2020

I don't want it on the main page, if anything this is an advanced feature. We can direct the user to the advanced tab to set it to ignore. This is an (rare) exception, not the norm.

@wolframroesler
Copy link
Contributor

How about this, then?

Screenshot from 2020-03-29 16 37 46

The "Advanced" page is quite full already, the attachment buttons even have some truncation on my system (the Additional Attributes buttons don't, for some reason). On my laptop's built-in screen I can't even make the window large enough to show the "known bad" checkbox without scrolling.

Wondering if there really should be a context menu in Health Check and HIBP to set the flag. It would be super easy to set the flag but difficult to clear it, and I'm afraid this asymmetry is going to cause problems when people set the flag by accident. Like, a user right-clicks Health Check and slips the mouse, and suddenly an entry (the one that happened be just under the one the user wanted) is gone from the report. How is the user supposed to find out which entry he clicked inadvertently? We could show a "do you really want to" message box but that would remove the convenience of having a context menu in the first place.

@OLLI-S
Copy link
Author

OLLI-S commented Mar 29, 2020

I really like the idea of @wolframroesler to show the checkbox directly behind the password field, because the checkbox belongs to the password, so these two fields should stand together.
It would not make any sense to "hide" this field in a separate tab and force the user so search for it.
There are some guidelines of visual design (Gestalt psychology) where this is written in.

Edit
So when I get some results from the password check telling me that there are some very weak passwords, I normally edit them and when the checkbox is behind the password field, then I just need less mouse clicks than having the field in an other tab.

Edit 2
I have 1010 entries in KeePassXC and I suppose that at least 80 - 100 entries are entries where I need the checkbox, because either these passwords are PIN numbers or these passwords are from family members or my karate teacher (where I can not change the passwords).

@OLLI-S
Copy link
Author

OLLI-S commented Mar 29, 2020

@wolframroesler By the way, I would name the checkbox Exclude from Security Checks.
A shorter name would be Exclude from Reports but the first one is more understandable.

wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Mar 29, 2020
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Mar 29, 2020
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Mar 29, 2020
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Apr 9, 2020
From the context menu we can edit entries and (which is the actual
motivation to add a context menu) toggle the "known bad" (aka
"exclude from reports") flag.

Fixes keepassxreboot#4168
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Apr 9, 2020
to edit entries and toggle the "known bad" flag.

Fixes keepassxreboot#4168
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Apr 10, 2020
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Apr 10, 2020
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Apr 10, 2020
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Apr 10, 2020
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Apr 10, 2020
Just a POC/design suggestion. Checkbox behavior still TBD.

Fixes keepassxreboot#4168
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Apr 10, 2020
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Apr 10, 2020
Previously was: Password is known to be bad, exclude from reports
Now is: Exclude from security checks/reports (e. g. known bad password)
because this is an entry-level setting and it's not only about the
password. For eaxmple, the password might be good but it may be a
duplicate of another entry so we want to exclude this entry from
Health Check; in this case, "password is known to be bad" would be
misleading.

Changed internal names accordingly (e. g. OPTION_KNOWN_PAD_PASSWD
is now OPTION_KNOWN_BAD).

Fixes keepassxreboot#4168
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Apr 10, 2020
Checkbox now sets/clears the KnownBad custom data field.

Fixes keepassxreboot#4168
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Apr 10, 2020
This realistically excludes ATM and SIM PIN from Health Check
and HIBP reports, as requested in keepassxreboot#4168.
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Apr 10, 2020
Previously, after editing an entry from out of the HIBP report, the
entry would only be re-evaluated against HIBP if the password was changed.
Now, we also re-evaluate if the "known bad" flag was changed.

Fixes keepassxreboot#4168
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Apr 10, 2020
These are usually excluded from the report. If the checkbox is checked
they will be shown anyway, in a lighter color. This is mainly to allow
the user to remove the known bad flag via context menu.

Fixes keepassxreboot#4168
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Apr 10, 2020
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Apr 10, 2020
As Health Check before, HIBP now also has an option to show
those items that are usually not shown.

Fixes keepassxreboot#4168
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Apr 10, 2020
Use a shining lightbulb instead of the generic "i in a circle" for
"Database -> Reports". The main reason for that is that we need
something that indicates "no report" for the "exclude entry from
reports" context menu item, and we've got a crossed-out lightbulb.

Fixes keepassxreboot#4168
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Apr 10, 2020
From the context menu we can edit entries and (which is the actual
motivation to add a context menu) toggle the "known bad" (aka
"exclude from reports") flag.

Fixes keepassxreboot#4168
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue Apr 10, 2020
to edit entries and toggle the "known bad" flag.

Fixes keepassxreboot#4168
droidmonkey pushed a commit to wolframroesler/keepassxc that referenced this issue Apr 22, 2020
* Fixes keepassxreboot#4168

* Introduce a custom data element stored with an entry to indicate that it is a "Known Bad" entry. This flag causes database reports to skip these entries.
* The current number of known bad entries is displayed in the statistics report.
* Add context menu to reports to easily exclude entries.
wolframroesler added a commit to wolframroesler/keepassxc that referenced this issue May 6, 2020
* Fixes keepassxreboot#4168

* Introduce a custom data element stored with an entry to indicate that it is a "Known Bad" entry. This flag causes database reports to skip these entries.
* The current number of known bad entries is displayed in the statistics report.
* Add context menu to reports to easily exclude entries.
droidmonkey pushed a commit that referenced this issue May 9, 2020
* Fixes #4168

* Introduce a custom data element stored with an entry to indicate that it is a "Known Bad" entry. This flag causes database reports to skip these entries.
* The current number of known bad entries is displayed in the statistics report.
* Add context menu to reports to easily exclude entries.
@stefan123t
Copy link

stefan123t commented May 18, 2021

Dear @droidmonkey,
I just tried the Health Check Report and it shows a large number of known bad passwords which have been saved over the years under Root/Backup folder by either Keepass or XKeepass.
Is there a way to apply the "Known Bad" flag to a complete folder ?
Wouldn't this be a sensible default to configure for all/most users as Root/Backup is afair a default Folder same as Root/Recycle Bin ?

@stefan123t
Copy link

I just tried to figure out where the folders came from, they are from Keepass 1.x (Backup) and Keepass 2.x (Recycle Bin) respecitively.

@droidmonkey
Copy link
Member

Recycle Bin is already excluded as long as it is the official one for your database. We won't exclude folders by name. There is a feature request to apply the exclusion to multiple entries at once, that has not been implemented yet.

@yutamago
Copy link

yutamago commented Jun 4, 2021

@droidmonkey can you link the feature request? I can't find it.
I was just looking for this feature because i'm keeping a list of about a hundred TAN numbers in KeepassXC and I'm aware those aren't safe passwords, but I don't want to flag every single entry by hand.

@droidmonkey
Copy link
Member

The feature has been merged to develop branch. Complete!

@yutamago
Copy link

yutamago commented Jun 4, 2021

Cool! Thanks! Looking forward to the next update! :)

@tunbridgep
Copy link

If this has been merged for 2.7.0, and the KDBX 4.1 standard has been implemented, don't they both do the same thing?

@droidmonkey
Copy link
Member

No they are the same thing. We pushed to make this a standard and keepass added it to kdbx 4.1.

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

Successfully merging a pull request may close this issue.

7 participants