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

Secure store passwords #291

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

foldynl
Copy link

@foldynl foldynl commented Jun 17, 2021

Attaching an initial support for #274.

My comments

  • password are saved to a secure storage. The secure storage depends on OS (Linux, Mac or Win).
  • library libqt5keychain1 is used to store the password.
  • I have tested only Linux storage (using Gnome-keyring). Unfortunately, I cannot verify it on Win or Mac. Therefore, it is an EXPERIMENTAL patch that should be properly tested. Maybe there will be some side effects.
  • passwords are removed from the config file and saved to the secure storage automatically when the Klog version is changed or after clicking Setup's OK.
  • unfortunately, I have no knowledge how to correctly configure TRAVIS and Code Analyze system. in general, it is needed to add libqt5keychain1 qt5keychain-dev libraries

please, review the patch. Any your comments are welcomed.

@ea4k
Copy link
Owner

ea4k commented Jun 19, 2021

Before accepting this PR I need to test on mac & Windows and also check the Debian integration of this package.
We also need to evaluate if this packaeg has a "long live" estimation and it is actively maintained as adding a dependency to a third party library brings some risks :-)

@barjac
Copy link

barjac commented Dec 11, 2023

Attaching an initial support for #274.

My comments

* password are saved to a secure storage. The secure storage depends on OS (Linux, Mac or Win).

* library libqt5keychain1 is used to store the password.

* I have tested only Linux storage (using Gnome-keyring). Unfortunately, I cannot verify it on Win or Mac. Therefore, it is an EXPERIMENTAL patch that should be properly tested. Maybe there will be some side effects.

gnome-keyring is a gnome thing.

How would this integrate with KDE Plasma5?

As a packager I would not want to force specifically 'gnome' packages on non-gnome users, so could this be more general, such that any existing secure password storage could be used (if desired)?

* passwords are removed from the config file and saved to the secure storage automatically when the Klog version is changed or after clicking Setup's OK.

Please no! This must be optional and not forced on users.

What threat is this PR intended to mitigate?
Is this really needed?
If not, does it warrant the extra complexity and potential bug reports it will generate?
Just because it can be done, does not make it 'necessary'.

I have a friend who cannot log into LoTW because of their paranoid use of multiple passwords and their confusing re-registration system. He has given up trying to use it.
Food for thought?

@foldynl
Copy link
Author

foldynl commented Dec 12, 2023

Considering that it is a 2-year-old patch, it was probably not accepted by the author. I did it for the project therefore I don't want to delete it. It is up to the author if the PR is accepted or deleted.

Even though the patch is not accepted and it's no longer possible to merge it I am going to answer your questions.

gnome-keyring is a gnome thing.

How would this integrate with KDE Plasma5?

As a packager I would not want to force specifically 'gnome' packages on non-gnome users, so could this be more general, such that any existing secure password storage could be used (if desired)?

The used library (qtkeychain) is multiplatform. Please, see details which platforms and back-ends are supported. It is not just about GNOME.

What threat is this PR intended to mitigate? Is this really needed? If not, does it warrant the extra complexity and potential bug reports it will generate? Just because it can be done, does not make it 'necessary'.

this should solve the problem of how poorly protected passwords are. Passwords should not be stored on the filesystem in an plaintext form. It's about safety. Look at the example of firefox, how users would probably like it if their passwords were stored as plaintext in a file. Such an application would probably not be used for a long time. And that, unfortunately, is an example of most hamradio applications. Authors do not think about safety. I could give you many other such examples of faulty security design.

I have a friend who cannot log into LoTW because of their paranoid use of multiple passwords and their confusing re-registration system. He has given up trying to use it. Food for thought?

This is a slightly different example. If you forget your password or migrate to a new system, it has nothing to do with the fact that your password is stored in the secure store. On the contrary, secure store is useful in situations where you have a lot of passwords. Then these passwords are safely stored in one place under the protection of Master key (password). It's an common situation and I don't know why it should be handled differently in hamradio applications.

Take it as my opinion. I am trying to solve many of these details in my own hamradio Logbook application. But it is difficult, because there are services like DX Cluster or ON4KST Chat, where passwords are sent in plaintext form also via the Internet. And unfortunately, no one cares and no one wants to do anything about it, because it has been done this way for long time and why to change it.

We could probably discuss it for a long time, but since it is not related to this PR, it doesn't belong here.

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 this pull request may close these issues.

3 participants