Store character's login passwords securely#3018
Conversation
|
Hey there! Thanks for helping Mudlet improve. 🌟 Test versionsYou can directly test the changes here:
No need to install anything - just unzip and run. |
5b9bdfa to
0cfafeb
Compare
| #include "post_guard.h" | ||
| #include <QSettings> | ||
| #include <sstream> | ||
| #include <../3rdparty/qtkeychain/keychain.h> |
There was a problem hiding this comment.
This looks like an odd include, but that was the only way I could get it to work for both qmake and cmake.
There was a problem hiding this comment.
It has to be a system include and not a local one (using "...") - strange?
|
When you rename the profile, I think the password will be 'forgotten' because it'll still be stored against the old profile - need to fix that. |
This could be problematic for testers - or those who have a lot of profiles and do not remember their passwords themselves - as we do not provide a means to sneak-a-peek at the existing password and destroying the user's existing password storage so other (current/prior) versions have no means of accessing them in a backward compatible manner is not going to be universally welcomed. I would like to see this being phased in over two patch (monthly) versions so that there is ONE version that reads the new encrypted version but also writes any modified ones or new profile ones to the existing storage system as well - then the FOLLOWING release removes the storage by the existing means - and the password storage becomes more secure at THAT release. I admit that this will take longer to introduce but it will prevent short term issues during the change-over. Alternatively, we must add a facility to temporarily reveal the password on demand in the password entry box... |
|
@SlySven I agree with your concern, let's go with the former "slow migration" option. Passwords aren't deleted from the insecure storage right away now. |
|
Ah, I had another thought recently - I share (some) of my profiles with (now) 3 OSes on the same machine (FreeBSD, Linux and Windows) however this secure storage is OS dependent - as I see it the Window's one is not going to be visible to the Linux one and visa-versa. Fair enough this is not a normal setup - but the same principle will apply when we get the portable mode fully operable - if the user plops their profiles onto a USB stick to play on more than one PC/OS how will the secure password storage work in that situation? 😟 |
|
📜 The QtKeyChain is an licensed project (though I am not clear whether it is the 2 or 3 clause BSD license - see frankosterfeld/qtkeychain#147) so we will need a suitable block in the "About Mudlet" - "Third Party" dialogue for it. |
|
@SlySven I tried to do it, but the way the license text is built up is super complicated. Would you mind adding it? |
|
Humm, the library comes with 3 translations for about 33 strings:
We may need to generate (and pass upstream):
I think we can skip, for the moment:
and I will need to revise the translation loading code to also load in these translation files. |
It is in vadi2#17 ... Having tried this out I think having the ability to temporarily show the password in the entry box is not so out of the window after all - I had to scrabble around to find a utility ( |
|
It is on Windows and macOS, and it's on my Ubuntu desktop as well... I think the amount of people you're looking at is very small. But OK, let's see what you've got! |
We don't need those translations as users don't see them. Right now from the users point of view, the interface is unchanged (which is great!). |
I have use the 2-Clause BSD (a.k.a the FreeBSD) licence that is in the upstream's "COPYING" file - this is at varience with the two README files which quote the licence as being the Modified BSD which is the 3-Clause one. I have an open issue on the upstream repository seeking clarification; should things not be as they seem it should be a relatively painless operation to select the other boillerplate for this item. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
See above... 😆 |
|
Cool! Anything left on this specific PR? |
SlySven
left a comment
There was a problem hiding this comment.
I think this can go in now - and then there can be a follow-up in the next release cycle to - under user control (so they can keep the old, insecure method for special cases - sharing profiles across OSes or on USB/dropbox) have the (application-wide) option to disable/delete the old direct to file (or an INI replacement) based storage system which will be enabled (i.e. nuke the insecure means) by default...
* Added QtKeychain * Wire in QtKeychain into qmake build * Install libsecret-dev in Travis * Got read/write working from the secure storage only * Reset password * Change to using a callback instead of a deep chain of nested calls * Wire in CMake build, but doesn't work yet * Finish CMake build, works now * WIP * Add migration to secure storage * Remove left-in debug echo * Don't forget password when profile is renamed * Don't delete passwords upon migration for now * Revise: insert QtKeyChain licence boilerplate (#17) I have use the 2-Clause BSD (a.k.a the FreeBSD) licence that is in the upstream's "COPYING" file - this is at varience with the two README files which quote the licence as being the Modified BSD which is the 3-Clause one. I have an open issue on the upstream repository seeking clarification; should things not be as they seem it should be a relatively painless operation to select the other boillerplate for this item. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
* Added QtKeychain * Wire in QtKeychain into qmake build * Install libsecret-dev in Travis * Got read/write working from the secure storage only * Reset password * Change to using a callback instead of a deep chain of nested calls * Wire in CMake build, but doesn't work yet * Finish CMake build, works now * WIP * Add migration to secure storage * Remove left-in debug echo * Don't forget password when profile is renamed * Don't delete passwords upon migration for now * Revise: insert QtKeyChain licence boilerplate (Mudlet#17) I have use the 2-Clause BSD (a.k.a the FreeBSD) licence that is in the upstream's "COPYING" file - this is at varience with the two README files which quote the licence as being the Modified BSD which is the 3-Clause one. I have an open issue on the upstream repository seeking clarification; should things not be as they seem it should be a relatively painless operation to select the other boillerplate for this item. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Brief overview of PR changes/additions
Store character's login passwords securely (the ones in the connection screen).
Motivation for adding to Mudlet
While it's very unlikely that a hacker who gets access to someone's machine will specifically go after their MUD characters password - there are many more valuable things to go after - it's still a good idea!
How can I tell if it works?
passwordfile in your profile's directory should be goneOther info (issues closed, discussion etc)
Close #1917.
Note that once you use this test version of Mudlet which migrates passwords to secure storage - normal Mudlet which doesn't have support for secured passwords, obviously, won't be able to read them. Keep this in mind: if you notice your password has "disappeared", you probably used this test version which secured your password and now you're back on the normal version which can't access it.