Closed andkopp closed 4 years ago
Oh no please do not define class functions in a totally unrelated cpp file. I think it is more than appropriate to just make the health function a static function that takes a db shared pointer.
@droidmonkey you're right and it doesn't get us anywhere. I reworked it to use a class (PasswordHealth), which fits much better into the existing applications. I also moved all entropy assessment ("weak if < 65" etc.) into the PasswordHealth.cpp file. Please have another look (same link as above).
About integrating HIBP, it seems pretty easy to integrate, may take some ideas from this: https://github.com/fopina/kdbxpasswordpwned
Added new menu item "Reporting" between "Change master key" and "Database settings" in the "Database" menu. Moved the statistics panel from "Database settings" to "Reporting", so that "Database settings" only contains things that can be changed, and "Reporting" contains information only (I'm sure you'll find it more intuitive that way, @andkopp; I do anyway). "Password Health" is going to be a new page in the "Reporting" dialog.
Database Reports? Reporting is a verb, you want a noun there
Non-native speaker here :) I remember having heard "reporting" in noun context, but that may have been something in the pseudo-English that people speak in this part of the world. Will change to "Reports" ("Database reports" would be too much "Database" IMHO, it's in the menu title and also in the following "Database settings", simple "Reports" will make it stand out better.)
First look at the new "Database -> Reports" dialog. "Statistics" is as before (just moved here from Database Settings). In Health Check, nothing is there yet except the panel itself. Content to be added as I find the time.
I'm not sure yet if a table (as in Statistics) is the best way to display the health check information. Let's be agile and find out later.
Damn I love it! Great icon, haha
It's taking shape :)
This is the new "Database -> Reports" dialog in action. As I said before, "Statistics" is the same as before, just moved here from Database Settings; Health Check is new.
For Health Check, Password quality assessment is based on a score which is computed like this:
Passwords with a score of 0 or less are marked red ("bad passwords"). Score < 40 is orange ("poor"), score < 65 is yellow ("weak"), score < 100 is "good", score >= 100 is "excellent"; the latter two are not shown in Health Check. The limits (40, 65, 100) are identical to those used for entropy-based assessment e. g. in the password generator dialog.
So far, this is almost ready for a merge request, however I'd like to add one more feature: Double-click a row in the list to jump right into the entry editor.
How do you like it?
Looks great! Could we also show the color indicator in the entry list? That would make it easy to notice weak entries.
Alos just a random comment and I have no idea how that works/is handled in Qt, but did you consider the accessibility of these colors, i.e. is there an "alt" text or so, so the entry colors can be noticed by screen readers?
Putting the alt text straight into the table cell and setting identical foreground and background colors should do the trick.
@ba32107 That would indeed be possible. I've added a function to the password assessment class that returns the color, that function could be invoked in the entry list. It would also be possible to configure the color column away, in case someone doesn't like it.
Will save this for later, however. I'd like to get the actual health check report finished first.
First version finished, PR submitted. Looking forward to your feedback.
Hello all, do you consider the requirements covered, or is there anything open?
@wolframroesler Hi. Is there a compiled dmg file to try on MacOS?
@andkopp I'm afraid I don't have one because I'm on Linux, and the automatic build doesn't seem to have created one (https://ci.keepassxc.org/viewLog.html?buildId=42860&buildTypeId=KeepassXC_MacOS&tab=artifacts).
Any Mac users around who can create a dmg from the branch in question (https://github.com/wolframroesler/keepassxc/tree/feature/healthcheck)?
I need to review your PR, was going to wait until we release 2.5.2
Would love to see this feature too
Please add also the following warning message:
I would love to see in the tree view a new main knot that is called "security report" with the child elements
The password manager Enpass has this, this is called "Password Audit":
@droidmonkey Should I make a separate topic for my last sentence (make the password audit quickly accessible)?
Just to have it noted here, no 2FA secret in KeePassXC does not mean missing 2FA. Many people don't store their 2FA secret in their password manager to keep the goal of 2FA - actually having a second factor.
@Nothing4You Yes, you are right. So when users store their 2FA keys in a separate app, then they should just ignore the list shown at Missing 2FA.
But even for these people the list can be interesting: They see what websites they have in KeePassXC support 2FA and they can check in their Atthenticator app if they have set up 2FA for all these websites.
In https://github.com/keepassxreboot/keepassxc/issues/1083#issuecomment-571751841 I suggested:
Next: you report "Expired Passwords". I have many expired passwords and use them for accounts that I no longer use (where I want to keep the entry in KeePassXC and not delete it or move it to an "Archive" kdbx).
Here @wolframroesler replied:
About your expired passwords, you could simply delete them and retrieve them from the history when needed.
In KeePassXC I only know the history that is available for each entry. So when I change the password, then I see the old one in the history.
Did you suggest that I delete the passwords of expired entries (so they are not used in any password check)? Or did you suggest to delete the whole password entry (but where is here a global history?) ?
@droidmonkey can you change the title of this issue to include the term "health check"? That'll make it easier to find it in the future.
@OLLI-S
I would love to see in the tree view a new main knot that is called "security report" with the child elements [...]
The idea is that KeePassXC has only one function (called Health Check) that combines all of these. So far we have "duplicate passwords" and "weak passwords", it seems like we can't do "old passwords" because we don't know how old a password is (as discussed above; the suggested workaround is to use the expiration date), "pwned passwords" are covered in #1083, and "missing 2FA" in #4096. So, I suggest you donate your $20 as bounty on one of these issues.
The HaveIBeenPwnd plugin for KeePass gets the password-modification-date from the history. It looks when the entry was created in KeePass and checks via the history when the password was changed (when the password is different). This way the password-modification-date can be used to check if the password was changed after the breach date.
If you get the password-modification-date then this can be used:
But I read at c't magazine that security experts say that old passwords are OK as long as they fulfill the rules of safe passwords. Changing passwords can also be dangerous...
I already donated $30 for Suggest TOTP #4096 When I suggested to have all these functions in the tree view, then my intention was to have them all accessible with one click (in KeePass I have multiple clicks). So if you have one health report than these functions are all accessible with one click (I hope you add a button for this) or with two clicks. So I increased - as I promised above - the bounty for this issue by $20 (my wife will kill me :-P )
I still think, that an option to exclude expired passwords is a good idea (or to exclude a folder from the health check).
Please let me export the health report (CSV and TXT). This way I open the exported file and go though it line by line and can change multiple passwords before I have to go back to the health report. Without the export I have to write down 2-3 entries, change them, run the report again, write down, change and so on. But maybe I should create a new issue here for this feature?
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 KeePass). 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?
So a checkbox "Exclude from health check" when editing an entry would really be helpful!
Generally good idea, but not a checkbox, but rather a context menu item when you select an entry in the health check -> "Exclude from health check".
@OLLI-S If the question is "shall I create a new issue for that" then the answer is usually "yes". :) It prevents dependencies and avoids "never-ending stories" which are never finished because more and more features are requested.
Your donation is very much appreciated :) and give my regards to your wife. I'm sure she'll understand the importance of what we are doing here.
I like the idea of having a toolbar button for Health Check, however that should be yet another issue of its own.
@rugk Exclusion by context menu is great but we'll also need the checkbox, in order to undo the setting in case someone excluded an entry from Health Check inadvertently. Also, I like the idea of being able to exclude an entry from Health Check right when I create it. But let's discuss this in the new issue @OLLI-S is going to create.
@wolframroesler I created the following issues
I am really keen about this feature!
Let's summarize where we are in the Health Check project.
Current status: Originally requested features are implemented (AFAICT), merge request submitted (#3993), awaiting approval.
Additional features, or new features building on those implemented already, have been requested:
(EDIT: Updated list according to @OLLI-S's comment below)
In order to keep things nicely separated, I suggest we finish and merge the current issue before beginning work on the others. I've got some ideas about the HIBP thing, for example, and other developers may wish to join in.
So, regarding the current issue, is there anything left I need to add to the implementation before it can be merged?
@wolframroesler in the list of additional features you may add these two features too:
I totally agree that implementing all these features separate (in separate merges) is the the best way. I am so happy that all these features will be implemented (whenever you plan to implement them).
Hi @wolframroesler, thanks for your work, this feature looks very promising. One question, does your PR include the password expiry warning? If not, which one of the above issues would incorporate this? None of them mention expired passwords explicitly.
Thanks for the thumbs-up @ba32107 :) the expiry warning is already included in my PR.
Maybe you could introduce a new tag for all these features related to that whole thing? (Or a GitHub projects page??)
So, regarding the current issue, is there anything left I need to add to the implementation before it can be merged?
Yep, it's a finding and merging of duplicate entries. Issue #750, which was included here.
@LightTemplar Health Check already reports duplicated passwords, but I think #750 is about entries that are duplicated as a whole, or very similar to other entries (say, same URL and user name but different password, because an entry was cloned, the clone was modified, and the original was forgotten). While I consider that an interesting feature, I don't think it belongs into Health Check because it doesn't affect password quality or security. Maybe #750 should be re-opened, and a definition of what it means that two entries are "similar" should be supplied.
I don't think such a feature should exist. It is near impossible to tell if an entry is a duplicate or intentionally the same. You might have only 1 field different, is that a duplicate entry? It doesn't make sense to automate that process.
@wolframroesler, I don't mind, if #750 will be reopened.
@droidmonkey, I think, the app should let user decide, what values should be same, to count entries as duplicate. I personally need username and second level domain name, sometimes even without first level, e.g. aliexpress.de and aliexpress.com - they should be in one entry, but now they are in different.
Letting the user decide exponentially complicates the feature and implementation. From a cost benefit viewpoint it's not worth implementing. You can easily search for *
and then sort the results by the various columns to find, and fix, duplicates in your database.
@droidmonkey, I have 824 entries in the database. I don't think, that the option to sort them and go through all of them is the best solution.
KeePass offers a feature to "Delete Duplicate Entries":
I have 1002 entries in KeePasXC and if I search for duplicate entries in KeePass, the 2 entries are found/deleted (I did not save the database in KeePass). Finding these 2 duplicate entries out of 1002 entries manually is much work, so I think #750 should be re-opened. Defining what duplicate means is a total different discussion (that should be made on #750) but I would re-open it.
My plan - besides world domination - is to have some more database-maintenance tools (I will create separate issues for my suggestions). So maybe you rename the title of #750 to "Database-Maintenance - Find/merge duplicated password entries"
I don't think such a feature should exist. It is near impossible to tell if an entry is a duplicate or intentionally the same.
Well… it is? Could not you then just offer a list of duplicates and the user can choose:
This way, they are either marked as unintentional (duplicate deleted) or as intentional (then properly link them).
Use case E.g. I have the problem with Stackexchange. Their 1000 domains are driving me crazy, as I of course want to have password-autofill on each page, but with same username+password.
--> I anyway wanted to have a solution for that problem too, because the current UX is not that good when you need to clone each entry again just to link them. (Because I still have old entries in there I need to delete.)
(maybe better to put that into a new issue, is not it? Although "duplicate finding" is exactly the problem/process I have here.)
I personally think that this feature should be implemented. Is there a special string that I can copy-paste into the search box to find duplicate entries?
If KeePassXC can report non-unique passwords and maximum password reuse in the statistics area of database settings, there should be a way to deal with that.
What is duplicate to you is not duplicate to the next user. That means YOU need to decide what to search for to find duplicates. If you want to consolidate stackexchange entries, search for stackexchange. If you want to find entries with a similar field (Title, URL, etc) then search for *
which shows ALL entries in your database. Then click the header you want to sort by and look for entries that are grouped together.
Aside from finding EXACT duplicates, automating this process is fruitless since it depends on how you define a duplicate between entries you are concerned with.
You should add a feature called "Find Duplicates" and show a list with possible duplicate entries. In the first column you show the title of the entry, in the second column you show a percentage of equality (100% means all fields are equal, 90% means that all fields except one field are equal). You also should show the columns Username, Password (masked) and URL and above the table a checkbox to show the password in clear text.
In this form (below the table) you should also show a link to a documentation that explains how to avoid duplicate entries and how linking entries works (and what the benefit of linking is).
@droidmonkey I suggest that we continue the discussion in #750 and that you reopen it. Maybe you also change the title of #750 to "Database-Maintenance - Find/merge duplicated password entries"
Fair enough, I reopened #750
Another point for the wish list: I am looking for an integrated password analysis tool that evaluates the passwords in my database both for their equality or similarity among themselves, their password strength as well as their age.
Regarding the implementation, I can imagine several variants. It could be a separate menu point, over which a test procedure can be started. At the end of the check, the result per database entry is displayed in a list. The list should be filterable according to problem type and criticality. Another possibility would be to always check the passwords in the background. For each entry, a kind of traffic light (eg green, yellow, red) and, if necessary, a warning message is displayed which indicates how secure the password of an entry is.
Examples of warning messages:
Edit: The following plugins for the original Keepass try to achieve the same or similiar goals.