Authentication system updates #278
Comments
|
the vast majority of plugins for QGIS do not require a security framework to operate.. to say the obvious. is it me, or is this a case of a subset of the community demanding that every other person must also be part of that community via security .. ? To add injury, it appears that the discussion must be resolved on github ? I have no credentials in the QGIS core, but I want to add this comment right away.. security demanding more security is exactly the direction I want to stay away from.. |
|
@darkblue-b I honestly have no idea what you're talking about |
Force synchronisation of password to keychain, remove manually triggered action On systems with keychain support, automatically just create a random master authentication password and store in keychain use a key which is unique per QGIS user profile -- |
|
With all due respect, you're talking from a place of ignorance about existing QGIS internals. This support is ALREADY there. What I propose here is just changes to make it more user friendly -- see the details listed above under "Current Situation".
I'd also ask you to review the people submitting this proposal and those commenting on qgis/QGIS#55144 and compare against https://github.com/qgis/QGIS/graphs/contributors . This is not a random subset of the QGIS community pushing for these changes, it is a large number of the most active QGIS developers who see a need for improving this. |
I'm not a security expert so it's probably a stupid question but when we are on a system with keychain support, why do we have an intermediary encrypted password database instead of storing directly all passwords in the keychain? That would avoid the master password generation, and QGIS would use this encrypted database only as a fallback for systems which don't have keychain support. |
@darkblue-b a) this discussion/qep affects also core functionalities, as storing credentials for postgis/services connections b) any proposed change is always discussed in the clear and c) you don't need to received and read any discussion, just unsubscribe the notifications from the qep repo, or just ignore the discussions do not interest you. |
@troopa81 I believe the bulk (if not all) of the actual implementation was done by @elpaso and @dakcarto back during the Boundless time. |
That's a good point, to put this into perspective, the keychain (wallet) storage was added later on top of the existing auth framework based on a local sqlite encrypted DB to automatically retrieve the master password from the wallet without user interaction. I agree that we could abstract the credentials storage to use different storages:
|
|
here are a few considerations/comments:
We have to be very careful about this: we cannot provide to QGIS plugins unrestricted access to a wallet. One of the most important points that @dakcarto (as he told me) had in mind when he designed the auth system was to make sure that the most critical parts of the system were NOT accessible/exposed to Python: a plugin should NOT be able to interact with the auth system except for a few limited and carefully selected operations, for example it is possible to create a new authentication configuration or to access the existing authentication configurations. As for the general UX, there is of course room for improvements: the process of creating the master password can certainly be explained better with a few GUI improvements, note that it is a one-time setup process: as a regular user I have set up my master password once many years ago and never messed up with it, I have never seen a password prompt again since I have it set in my wallet. Regarding you points:
This can be fixed with a better wording in the master password setup dialog by adding more explanation about what a master password is. If it's not already there we could also provide a method for plugins to trigger the master password input if it wasn't set before. A pre-generated default random password in the dialog could also be set and shown to the user (<- this is important IMHO).
This could be solved as suggested for the previous point.
This can be fixed by offering the user the option to sync the password in the wallet when she changes the password in from the QGIS menu, I think it is probably a very uncommon operation anyway.
This can be fixed easily by prefixing the key in the storage with the profile name. |
Here's the thing though -- right now plugins CAN do this. And they do. Eg the koordinates plugin at https://github.com/koordinates/koordinates-qgis-plugin/blob/a12bdba2292a31f9589a58450329a6b96b62ecb2/koordinates/gui/login_widget.py#L315 There is NOTHING in the auth manager API which prevents any other plugin the user has installed from reading that some secure key. This is why I think we need to draw a firm line in the sand, and say "the auth framework prevents access to secrets from outside of the users operating system login. It DOES NOT prevent other plugins accessing secrets from the same user profile. Users must take care to only install plugins they trust in their qgis profile." That's a perfectly reasonable line to draw. I just can't see any other way around this -- either we completely block secure storage from plugins entirely, or they have unfettered access to any securely stored value from that same user profile.
Except... This was never done. It was discussed, but in the end all the methods are exposed. A plugin can even set a master password for a profile if it doesn't have an existing one. (and I've relied on this in enterprise deployments in the past, just so I can ensure that oauth authentication works out of the box). Here's the really random thing - check out https://docs.qgis.org/3.28/en/docs/user_manual/auth_system/auth_overview.html#python-bindings , and follow the link to security considerations. It leads to a strange discussion about potentially locking down the system which might happen in future. And this is in our user documentation?!?! I'd honestly go as far as to say that right now, the auth system is broken. It doesn't offer either a completely secure store,yet at the same time it does it meet the needs of plugin developers or enterprise rollouts. It's stuck in a weird middle place where it partially tries to satisfy both requirements, but doesn't satisfy either. |
|
Thanks for opening this, really appretiated. Do you consider usage of qtkeychain in the future implementation? There is not a massive development in the repository recently, but maybe it is stable/working enough (?). |
That's exactly what we are already using: https://github.com/qgis/QGIS/blob/master/CMakeLists.txt#L545 |
That's not access to the OS wallet but to the auth DB.
Yeah, that makes sense IMHO.
Yeah, I see that, it looks dangerous to me.
Ok, you are right, we can certainly do better. Here's what I would suggest if you want to address the PyQgis auth security issues:
|
Yes, I know, i wanted to ask if we plan to continue using it? |
I personally don't see a pressing need to change, and would defer that discussion to a later stage. |
I agree in theory, but this is a large project. Do you think there's any smaller patch-level changes we can make immediately to improve things? Otherwise all this will need to get deferred till a future grant, as it's not something I'd want to spend volunteer time on. For now I've worked around my immediate need by just hacking around this and abusing the exposed api in the plugin 🤷 |
Yes, sure, as I suggested you could:
I understand, but the auth system is so critical and delicate that patching it without a thorough analysis makes me nervous. |
I don't know anything about the auth system, but efforts to reliably restrict some access to part of it from Python seem vain to me. Sure you will stop 99% of plugin authors by not making stuff available through SIP, but sufficiently willing devs (and if the threat model includes hostile plugins, then you need to take into account those ones) could abuse ctypes or other low level mechanisms to still call C++ exported symbols (would be painful due to different mangling depending on platforms/compilers, but pretty sure that's doable). |
I 100% agree -there are myriad ways a malicious user could retrieve these secure secrets from both within and outside of qgis if they have physical access to a user's operating system profile. Attempting to advertise the system as "secure" without making these limitations known is deceptive. We should just rewrite all the documentation for the framework to firmly state "this provides a way to store secrets privately within a user's profile. It does not prevent other applications or malicious scripts run from that user's profile from accessing these passwords or secrets. Users storing secrets through qgis have the responsibility to be diligent and exercise all best practice security considerations (such as automatic screen locking) to prevent unwanted physical access to their operating system login sessions." And then we just make the auth API work well under these considerations. We'll never get better than that. 🤷 |
|
I apologise if I'm sounding frustrated here 😂 -- that's not intended to be directed at you. It's frustration with what I see as a fundamentally broken part of QGIS, and it's causing a lot of horrible time consuming hacks and user/client frustration. |
|
I don't think that plugins or scripts are the only risk to the current credential store design. Consider an organisation with an authenticated data store where all projects use the authentication config ID An attacker with knowledge of that authentication config ID1 could create QGIS project file with a layer that fetches from a URL/host under their control, but referencing the organisation's authentication config ID ( Then, if an authorised user opens the attacker's QGIS project file, even if the user disables scripts, the attacker can still collect those credentials. I suspect that some OAuth2 configurations (which fetch a JWT) are impacted by the same issue to a limited degree:
All authentication configurations have a For Basic authentication, the protocol doesn't even need to match. An attacker could use a layer referencing a WMS server under their control to collect credentials originally intended for PGSQL. These errors could even happen inadvertently, such as using HTTP instead of HTTPS to access a WMS server. This somewhat relates to the idea of trusted projects (#296) – but once an attacker gets inside of that trusted space, it's easy for them to move laterally within an organisation (ie: compromise user A to get credentials for user B). Footnotes
|
QGIS Enhancement: Authentication system updates
Date 2023/11/16
Author Nyall Dawson (@nyalldawson)
Version QGIS 3.36
Current situation
QGIS has an inbuilt secure store for user secrets, known as the "Authentication" system. The authentication framework is designed where secrets are stored in an encrypted sqlite database within the user's QGIS profile.
This database must be decrypted via a "master password" before secrets can either be read or stored in it by QGIS core or plugins. On some systems (linux, Windows) there is support for storing this master password within the user's operating system keychain/wallet, so that the user does not need to re-enter it when starting a new QGIS session. (Wallet storage is NOT currently available on Macos without user configuration tweaks, due to lack of notarization on the QGIS install. See qgis/QGIS#46175).
By default, there is NO password the authentication system and secure store are disabled for out-of-the-box for QGIS installs.
By its nature, the authentication framework needs to be secure and any changes to the authentication logic need careful consideration.
Issues and limitations
The current setup has several issues which make it unusable in many situations:
)
Because there's no out-of-the-box secure store available, plugins resort to all kinds of hacks to work around this. Eg just throwing secrets in plain text in the user profile (isogeo/isogeo-plugin-qgis@631b08f), or trying to talk users through this complex setup (https://github.com/north-road/cesium-ion-plugin/blob/main/cesium_ion/plugin.py#L115), or even just generating a random password and setting it for the user within the plugin (seen in a private plugin).
There are UX issues with the workflows for creating and storing passwords. Eg even when the system keychain support is enabled, the user must trigger a manual operation to update/store the new master password when they change it from within QGIS. This means that it's very easy to have an outdated password in the wallet, out of sync with that used to encrypt the database. (Leading to loss of encrypted data).
If a user uses multiple QGIS profiles, the same key is used for wallet storage of the password. This means that a secondary profile will overwrite the main profile stored password in the wallet, leading to an unreadable auth database.
Motivation
These issues MUST be fixed. The current situation is basically impossible to manage as a plugin developer, and leads to insecure storage and multiple fragile downstream hacks.
Proposed solutions
Force synchronisation of password to keychain, remove manually triggered action
Let's err on the side of being helpful and assume that if the user is storing the password in their keychain, then they want that stored password always to be the correct one. This will prevent the keychain password from being out-of-sync with the encryption password, avoiding data loss.
PR at qgis/QGIS#55227
On systems with keychain support, automatically just create a random master authentication password and store in keychain
This avoids the need for users to manually set a master password, and makes the authentication system immediately available out of the box for these users. Plugins (and core functionality) which relies on the auth framework will "just work", without any hacks or user confusion.
In this approach, an average user will never see or need to know what the (randomly generated) master password is. It becomes an internal hidden detail used for secure storage of secrets only.
This logic will live at app level only, and not apply to server installations, or downstream clients like QField/Mergin.
The auto generated password logic can be disabled via a settings ini key, for enterprise deployments which have alternative logic for authentication password handling.
If a user wants to set a specific master password (eg to allow their encrypted auth db to be shared with a QGIS server installation or another user), they still have the option to change the master password via the Settings - Options - Authentication tab. Changing the password this way automatically decrypts/re-encrypts the authentication database. Accordingly these knowledgeable users will still have a way to easily set a fixed password, addressing the server configuration use case.
PR at qgis/QGIS#55144
Use a unique keychain key per user profile for master password
Instead of a hardcoded keychain ID key, use a key which is unique per QGIS user profile so that we don't overwrite other profile's master passwords when using secondary profiles, avoiding an un-encryptable auth database in other profiles.
PR at qgis/QGIS#55141
When changing master password, don't prompt for the existing password if it is stored in the system password helper and is valid
If the user already has a master password stored in their system keychain (eg if we move to a default, random generated one), then the user shouldn't be prompted to enter this password when they opt to change the master password in the QGIS authentication settings tab.
This is especially important if the default password becomes randomly autogenerated, as the user will NOT know what this existing random password is. If they are forced to enter it in order to set a specific fixed password, they will be unable to proceed!
PR at qgis/QGIS#55228
Potential security risk
The issue was raised that if the user is NOT prompted to re-enter their existing password in order to change to a new password, then there exists a security risk if someone else gains access to an unlocked computer and changes that user's password to something new.
My belief is that this situation is firmly out-of-scope. If a malicious user has access to a logged in user profile (regardless of any security in QGIS), then all assumptions for that logged in user's security are completely VOID and there is nothing we can do in QGIS to securely handle this situation.
Alternative ideas
Some alternative ideas have been raised about re-working the authentication framework
The text was updated successfully, but these errors were encountered: