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

On systems with keychain support, automatically just create a random master authentication password and store in keychain #55144

Closed
wants to merge 2 commits into from

Conversation

nyalldawson
Copy link
Collaborator

@nyalldawson nyalldawson commented Nov 3, 2023

This avoids the need for users to manually create a pw, and makes things MUCH nicer for plugins which want to utilise the secure authentication framework.

Right now the options for plugins are:

  1. Create a auto generated password themselves and force it on the user (basically what we are doing here automatically now)
  2. Show a confusing/scary message to users asking them to set a master password. From my experience users have NO idea what this means and consider it a QGIS bug.

Assuming this is acceptable, some follow up is needed:

  • If the user goes to change the master password, they'll be prompted to enter the current (random) password which they've never seen. We need some way to handle this more gracefully and skip the existing password re-entry if the password is stored in the keychain
  • When the user does change their master password manually, we don't automatically store the new password in the keychain and overwrite the old one. Instead the user must manually select "store / update the master password in your wallet/keychain" from the utilities menu, which is horrible UX and should just happen automatically.

@nyalldawson nyalldawson added the TRICKY!! For controversial or tricky changes, which need VERY careful consideration and review before action label Nov 3, 2023
@github-actions github-actions bot added this to the 3.36.0 milestone Nov 3, 2023
@nyalldawson
Copy link
Collaborator Author

@nyalldawson nyalldawson added the Authentication Related to the QGIS Authentication subsystem or user/password handling label Nov 3, 2023
@m-kuhn
Copy link
Member

m-kuhn commented Nov 3, 2023

Much welcome. 👍
The current behavior is scary and tedious. The UX will be better and I cannot think of any downsides.

@elpaso
Copy link
Contributor

elpaso commented Nov 3, 2023

Great UX improvement, I give you that.

But I am a little worried about the user not knowing the automatically created master password.

How would this work with use case (quite common AFAIK) of creating an auth DB on the client to be used and transferred to the server?

Also, it would be make it difficult to transfer the auth DB to another machine.

@m-kuhn
Copy link
Member

m-kuhn commented Nov 3, 2023

Great UX improvement, I give you that.

But I am a little worried about the user not knowing the automatically created master password.

How would this work with use case (quite common AFAIK) of creating an auth DB on the client to be used and transferred to the server?

Also, it would be make it difficult to transfer the auth DB to another machine.

Ideally we would improve the export/import workflow for this, either by making it possible to import the current transfer xml on server or by allowing to export a complete auth db. This will allow the user to distinguish from their local password (which they may or may not know) and a transfer / shared password per shared config.

For any other case the "follow ups needed" mentioned by @nyalldawson sound reasonable.

@nirvn
Copy link
Contributor

nirvn commented Nov 3, 2023

Big +1, I can't count the number of times friends and users I've initiated to QGIS come to me about this scary dialog.

On the transferability, we have import/export to/from XML which we can rely on.

@benoitdm-oslandia
Copy link
Contributor

Testimony

I am not a daily user of the QGIS password manager but we have done a couple of QGIS server installations but one issue we stumble on is the data (project and password) synchronisation between the desktop and the server. For what we have seen, end users do not know the *.db files and how to use them (they mainly save the passwords in the provider UI).

So we explain to the desktop end users the usage of pg_service.conf or the usage of password configuration (via the definition of a master password). It works fine: desktop end users understand the why of the pop pup asking for the master password, QGIS Server is happy with the QGIS_AUTH_PASSWORD_FILE and sysadmin are happy too because they can update (on regular basis) the password "easily".

Worries

  • With this automatic password generation, I do not see how to deploy a QGIS server with the *.db files because I need to retrieve the generated password to save it to the file targeted by QGIS_AUTH_PASSWORD_FILE.

    One solution could be to display the generated password once, at the generation time in a more frightening pop up: "Never forget this password or die..." for the sysadmin to retrieve it.

  • In the case we need to change it (sysadmin needs), the only existing use case (if we do not know the master password) is to reset the auth configuration database and to loose all the saved data. May be we should think to an other process? (I have no idea for now)

  • With this automatic password generation and without the master password, any one who has the *.db files can easily access the protected resources (targeted by the stored passwords) without constraints. IMHO it introduces a new security flaw.

    Delegating to the OS authentication or keychain mechanisms may be a good solution but is far more difficult to achieve.

  • As you said about the master password: users have NO idea what this means and consider it a QGIS bug.. I am agree but (for now) I have no better solution than improve the UI to explain more clearly what is the purpose of this password. The documentation page goes too deep in details and is not designed (IMHO) for end users.

@nyalldawson
Copy link
Collaborator Author

Thanks for all the discussion!

Here's a couple of answers to questions raised:

@elpaso

But I am a little worried about the user not knowing the automatically created master password.

The user should (theoretically?) still be able to retrieve the auto generated password by checking their system keychain, but I'm not 100% sure if that's possible on all platforms. At least on KDE/GNOME I know it is, not sure about Windows/Mac.

How would this work with use case (quite common AFAIK) of creating an auth DB on the client to be used and transferred to the server?

I'd say we address this by improving the ability to change the default random password to a user set one, by:

  1. Making the UI work nicely in the case of changing auto generated password -> user set password (ie not prompting the user for the existing password which they've never seen)
  2. Ensuring that the auth db is automatically reencrypted with the new password
  3. Ensuring that new password is ALWAYS saved to the keychain, instead of requiring the user to do this manually

Also, it would be make it difficult to transfer the auth DB to another machine.

I think the above workflow solves this use case -- a user who needs a specific password to transfer/reuse databases would just change the auto generated pw to their desired pw.

@benoitdm-oslandia

I am not a daily user of the QGIS password manager but we have done a couple of QGIS server installations but one issue we stumble on is the data (project and password) synchronisation between the desktop and the server. For what we have seen, end users do not know the *.db files and how to use them (they mainly save the passwords in the provider UI).

Agreed. I think the auth system is a good design, but the current UI for it is too complex for mere-mortal users to understand 🤣 . And this ultimately results in users making less secure choices...!

With this automatic password generation, I do not see how to deploy a QGIS server with the *.db files because I need to retrieve the generated password to save it to the file targeted by QGIS_AUTH_PASSWORD_FILE.

See above answer to @elpaso -- I think for this use case you would instruct the user to change the default auto password to a specific manually set one, which they can reuse on the server.

One solution could be to display the generated password once, at the generation time in a more frightening pop up: "Never forget this password or die..." for the sysadmin to retrieve it.

😱 I'd rather not! 🤣 Again, theoretically the user can always retrieve the password at any stage from their keychain/wallet software itself. So it's not actually the "set once and it's lost" scenario which we'd normally get with private keys.

In the case we need to change it (sysadmin needs), the only existing use case (if we do not know the master password) is to reset the auth configuration database and to loose all the saved data. May be we should think to an other process? (I have no idea for now)

There's existing code in the auth framework which allows users to change their password, and this already has the logic for decrypt using old -> reencrypt using new. So it should be possible already to change a password in place without resetting the database, assuming that the user has the existing password. (Or are there bugs in this process which block this from working? I did a quick scan earlier to look for open tickets relating to master passwords and couldn't find much.)

With this automatic password generation and without the master password, any one who has the *.db files can easily access the protected resources (targeted by the stored passwords) without constraints. IMHO it introduces a new security flaw.

Can you expand on this? In my understanding it won't be any different to the situation where a user manually creates the master password and it's stored in the keychain. It's just that instead of forcing a user to set that password, we're doing it automatically for them. But there's a very good chance I'm missing something here 😆

As you said about the master password: users have NO idea what this means and consider it a QGIS bug.. I am agree but (for now) I have no better solution than improve the UI to explain more clearly what is the purpose of this password. The documentation page goes too deep in details and is not designed (IMHO) for end users.

Agreed. That page really should be split up into two, with the back end bits moved to the developer documentation (ping @DelazJ )

@nyalldawson
Copy link
Collaborator Author

Two more things to add:

  • this logic lives only in app, and only kicks in when qgisapp is opened for the first time. It won't apply on server/headless environments
  • I think there should be a settings key to allow it to be disabled, for enterprise setups where some other logic is used to handle master passwords

@m-kuhn
Copy link
Member

m-kuhn commented Nov 4, 2023

what do you think about relying more on import/export workflows when the configuration needs to be shared with other systems.

  • easier to use throwaway passwords
  • selection of which configurations are shared
  • the user selects a location for the db to send (and does not have to go into qgis' internal folders)

To me it sounds like better UX and possibly more (and certainly not less) security, but I may be missing something?

@nyalldawson
Copy link
Collaborator Author

@m-kuhn I don't disagree, but does it make any actual difference to the functionality we expose? There's already import/export available, but also db sharing with common passwords works too

@m-kuhn
Copy link
Member

m-kuhn commented Nov 4, 2023

Import/export works for the desktop (and mobile) case, but I don't think it works for server.

I think we would benefit from improving this and making this the recommended best practice. This would probably void many (all?) of the questions and doubts raised in here. Or is there a use case where we really need the system/user profile auth db with the system/user password to be shared?

@benoitdm-oslandia
Copy link
Contributor

Can you expand on this? In my understanding it won't be any different to the situation where a user manually creates the master password and it's stored in the keychain. It's just that instead of forcing a user to set that password, we're doing it automatically for them. But there's a very good chance I'm missing something here 😆

My bad! I was misunderstanding the link between the QKeychain lib and the OS keychain manager!

@benoitdm-oslandia
Copy link
Contributor

All these changes are OK if the (advanced) user can easily retrieve the generated password in the keychain.

Running Ubuntu/Gnome, Seahorse was not installed by default and I did not find any solution to show/retrieve registered password without it.

What is the status for Windows or Mac?

@nyalldawson
Copy link
Collaborator Author

When the user does change their master password manually, we don't automatically store the new password in the keychain and overwrite the old one. Instead the user must manually select "store / update the master password in your wallet/keychain" from the utilities menu, which is horrible UX and should just happen automatically.

This is fixed in #55227

@nyalldawson
Copy link
Collaborator Author

If the user goes to change the master password, they'll be prompted to enter the current (random) password which they've never seen. We need some way to handle this more gracefully and skip the existing password re-entry if the password is stored in the keychain

This is fixed in #55228

master authentication password and store in keychain

This avoids the need for users to manually create a pw, and makes
things MUCH nicer for plugins which want to utilise the secure
authentication framework.

Right now the options for plugins are:
1. Create a auto generated password themselves and force it on
the user (basically what we are doing here automatically now)
2. Show a confusing/scary message to users asking them to set
a master password. From my experience users have NO idea what
this means and consider it a QGIS bug.
@nyalldawson
Copy link
Collaborator Author

I think there should be a settings key to allow it to be disabled, for enterprise setups where some other logic is used to handle master passwords

This is done now

Copy link
Contributor

@benoitdm-oslandia benoitdm-oslandia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nyalldawson
Copy link
Collaborator Author

I've created a QEP at qgis/QGIS-Enhancement-Proposals#278 for greater discussion

Copy link

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Nov 30, 2023
Copy link

github-actions bot commented Dec 7, 2023

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

@github-actions github-actions bot closed this Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Authentication Related to the QGIS Authentication subsystem or user/password handling stale Uh oh! Seems this work is abandoned, and the PR is about to close. TRICKY!! For controversial or tricky changes, which need VERY careful consideration and review before action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants