[RFC] Implement Freedesktop.org Secret Storage spec server side API

[Aetf]

This PR adds the feature requested in #1403, adding a new plugin implementing the Freedesktop.org Secret Storage spec version 0.2. Basically this exposes a new DBus service that is described in the spec which can be used by libsecret-based applications.

Type of change

• ✅ New feature (non-breaking change which adds functionality)

Description and Context

For motivation, please see #1403 for discussion.

For a description of the plugin, please see the README file.

There are also some modifications to existing code:

• Add a helper class to override the message box's parent window
• EntrySearcher can now search attribute values including custom ones
• Extract applyGroupIconTo to be a member function of Group
• DatabaseTabWidget::newDatabase returns the created DatabaseWidget
• DatabaseTabWidget emits databaseOpened signal when a new tab is added
• DatabaseWidget can delete a list of entries now
• DatabaseWidget emits databaseReplaced signal when replacing the database
• Add Aes128 in SymmetricCipher::algorithmIvSize
• Add necessary slots in MainWindow to show desktop notifications

Screenshots

Testing strategy

• There are unit tests for session negotiation, but have no idea how to test other parts, as they are directly exposed on DBus.
• In develop, I've been successfully using the python-secretstorage as a DBus client and I find its own test cases cover most of the common usage patterns and can exercise the server quite well.
• In addition, I tried to directly use seahorse and connecting to KeePassXC. It can successfully add/change/remove entries, as well as lock/unlock databases.
• I've been using this as the password backend for Vivaldi for a few weeks.

Checklist:

• ✅ I have read the CONTRIBUTING document. [REQUIRED]
• ✅ My code follows the code style of this project. [REQUIRED]
• ✅ All new and existing tests passed. [REQUIRED]
• ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
• ✅ My change requires a change to the documentation, and I have updated it accordingly.
• ✅ I have added tests to cover my changes.

[Aetf]

Rebased on the latest develop branch and fixed EntrySearcher unit test.

[droidmonkey]

Can you please squash a majority of your interstitial commits? It is hard to see the forest from the trees at the current state.

[Aetf]

Sure, will do once I get to my computer.

[Aetf]

Re-ordered and squashed commits. Changes to other parts of KeepassXC are isolated into individual commits before the one last big commit adding the plugin. It should be a lot easier to read now.

[droidmonkey]

Oh very nice, thank you. I will review after 2.4.1 is settled.

[Aetf]

Force-pushed to fix some bugs when the database was reloaded due to external file changes. A new signal databaseReplaced is added to DatabaseWidget.

[Aetf]

Rebased on develop

[droidmonkey]

You have an issue in our CI, are we missing a dependency?

/opt/buildagent/work/c401303cba1b4098/src/fdosecrets/FdoSecretsPlugin.cpp:58:37: error: no member named 'get' in 'QScopedPointer<FdoSecrets::Service, QScopedPointerDeleter<FdoSecrets::Service> >'
[18:54:29]	            connect(m_secretService.get(), &Service::error, this, [this](const QString& msg) {
[18:54:29]	                    ~~~~~~~~~~~~~~~ ^
[18:54:29]	/opt/buildagent/work/c401303cba1b4098/src/fdosecrets/FdoSecretsPlugin.cpp:74:28: error: no member named 'get' in 'QScopedPointer<FdoSecrets::Service, QScopedPointerDeleter<FdoSecrets::Service> >'
[18:54:29]	    return m_secretService.get();
[18:54:29]	           ~~~~~~~~~~~~~~~ ^

[Aetf]

Ah, I see. QScopedPointer::get was introduced in Qt 5.11. I'm on Qt 5.12 so it's working locally... What is our lowest Qt version requirement? I need to find a way to check I'm not using any newer APIs.

[Aetf]

Remove any usages of APIs later than Qt 5.3. (I used the docker image at ci/trusty, which installs Qt 5.3.2, but isn't the requirement >= 5.2?)

[Aetf]

Just figured out that I can view the build log by logging in as a guest on TeamCity... I'll fix other CI test failures in a few days.

[droidmonkey]

@Aetf I haven't forgot about this. I will review this week and merge if acceptable. I'll also approve your bounty win.

[Aetf]

No problem 😄

[droidmonkey]

Honestly this is the most beautiful code I have reviewed. I squashed all the small commits on existing code, will merge once the CI finishes.

[droidmonkey]

@hifi We can use the desktop notification that was added here to notify when SSH credentials are being accessed (as an option).

This notification path would also be nice for KeeShare.

[Aetf]

@phddoom, unfortunately that behavior is intended. secret-tool search --unlock works like the following:

• Call SearchItems to get a list of locked items, and a list of unlocked items.
• Call Unlock on the list of locked items

However, in KeepassXC, when a database is locked, it's "gone" and there is no information left for the program to be able to search inside it. So the list of locked items is empty. So nothing will happen and the database remains locked.

@elvirolo The "exposed database groups" list enumerates exposed database groups for currently opened databases (either locked or not). You can then go from there to the per-database setting page to configure the exposed group. See my previous comment.

If you have databases open but that list is empty, it's a bug and please create a new issue for that.

@droidmonkey What is the canonical place for this kind of plugin manual or "get started" like documentation? There are indeed some cases that are not so intuitive and I'd like to help document them.

Thanks

[ghost]

Thank you for your reply. My database is indeed open and unlocked, but the list is empty.

[Aetf]

@elvirolo Okay. So there's indeed something went wrong. A few things to try:

• Close the app settings page and reopen it, see if the list is populated with your database.
• Outside of the app settings page, you can go directly to the database settings page using menu: Database -> Database settings.... Then you can use the Secret Service Integration section to adjust the exposed group for the current database.

[droidmonkey]

@Aetf I am going to flesh out our documentation section of the website once I get the final products from our tech writer. The best place for documentation will be (in the future) https://github.com/keepassxreboot/keepassxreboot.github.io

As for the locked database issue, you could issue a call to bring the KeePassXC window to the front (mainWindow()->raiseWindow()) when you receive an unlock request.

[Aetf]

@droidmonkey Cool! Let me know if anything is needed from my part documenting this plugin.

[ghost]

@Aetf Thank you for your help. Exposing a group from the database settings page did make it work for the Nextcloud client (which uses qtkeychain). However, NetworkManager doesn't seem to be able to store Wifi secret in Keepassxc. Here are the errors which are displayed in the journal when I try to configure a connection for a specific user (which is supposed to store the secret in the user's keyring):

no secrets: No agents were available for this request.
state change: need-auth -> failed (reason 'no-secrets', sys-iface-state: 'managed')

[Aetf]

@elvirolo Could you please post a record of dbus messages while reproducing the error using the following command?

dbus-monitor "interface='org.freedesktop.Secret.Service'" "interface='org.freedesktop.Secret.Collection'" "interface='org.freedesktop.Secret.Item'" "interface='org.freedesktop.Secret.Prompt'" > dbus.log

Please run the command right after starting KeepassXC, and make sure it's running when you use NetworkManager. Then, stop monitoring by hit ctrl-c. The log will be written in file dbus.log.

Note that the recording contains secret values passing through the interface, so be sure to NOT use your actual database file.

[ghost]

@Aetf Thank you for your reply. Here is the content of the logfile:

signal time=1558741284.373695 sender=org.freedesktop.DBus -> destination=:1.76 serial=2 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameAcquired
   string ":1.76"
signal time=1558741284.373797 sender=org.freedesktop.DBus -> destination=:1.76 serial=4 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameLost
   string ":1.76"

[Aetf]

@elvirolo Sorry for the late reply, I was busy last week...

It that all the content of the log file? It means no one ever contacted KeepassXC through the API. Could you double check if the dbus service is running when KeepassXC is open and while using NetworkManager? The command is

dbus-send --session --dest=org.freedesktop.DBus --type=method_call --print-reply  /org/freedesktop/DBus org.freedesktop.DBus.ListNames | grep 'org.freedesktop.secrets'

If it prints nothing, then the service is not running.

If the dbus service is running, then the issue is with NetworkManager, which doesn't correct use the API.

[ghost]

It is indeed running (the command outputs "string "org.freedesktop.secrets""). I'll contact the NetworkManager team to try and find out what's wrong. Thanks for your help.

[kocjs]

What is the right way to disable gnome-keyring-daemon?
On my Debian Buster system with GNOME this was the only way that worked so far.
chmod 000 /usr/bin/gnome-keyring-daemon

[michaelk83]

Could you please post a record of dbus messages while reproducing the error using the following command?

dbus-monitor "interface='org.freedesktop.Secret.Service'" "interface='org.freedesktop.Secret.Collection'" "interface='org.freedesktop.Secret.Item'" "interface='org.freedesktop.Secret.Prompt'" > dbus.log

...
Note that the recording contains secret values passing through the interface, so be sure to NOT use your actual database file.

If this is still the current behavior, isn't that a security issue? A malicious script could launch dbus-monitor and then access those secret values via the log with a real database (or would that require root access?). Ideally, all communications should be encrypted, so that no secrets would be exposed. If that wasn't included in the original implementation, then that should be fixed separately.

This comment by @ntninja seems relevant (last two paragraphs):

The browser protocol is better insofar as that it forces clients to provision their own HOTP key for encryption and authentication rather than regressing to a basic “if it can connect, it's trusted for everything”. See point 4 of my proposal/notes on my ideas on how this can be implemented (within certain limits) based on the sender's identity however.

I will admit however that provisioning those keys inside the bridge however is not ideal, but it's not like we can upgrade the existing SecretStorage protocol with this property.

If this needs changes to libsecret API to implement better, then we should reach out to Stef Walter, who is the maintainer of libsecret and co-author of the Secret Service API. But this should be fixed as possible with the current API.

This libsecret issue also may be relevant.

[droidmonkey]

This feature has come quite a long way since this PR. Dbus traffic is encrypted using asymmetric cryptography and we have implemented a prompt window when credentials are requested.

[michaelk83]

@droidmonkey , while we're at it, what's the situation on client isolation (point 4 of @ntninja 's plan)? One of the things that was bugging me with KWallet was the lack of such isolation: any app could get the password of any other app. I understand this is a general problem with such password stores, and is difficult to solve. But anything we can do to start moving the needle towards a solution, would be welcome IMO. It may be worthwhile to involve Stef Walter on that, at least.

[droidmonkey]

There is nothing stopping an app from requesting arbitrary data from the collection. This is part of the spec, and without strict AAA guarantees at the process level I'm not sure you'll ever get there. (e.g. Process A can cryptographically prove they are really Process A and has access to this specific credential set)

We did implement the prompt to allow access which tells you which process is requesting which entry. We also create a new encryption pipe for each client.

[michaelk83]

We did implement the prompt to allow access which tells you which process is requesting which entry. We also create a new encryption pipe for each client.

That's already a start, which is good. But how is the process identity determined here? If it's just a string passed from the client, it's very easy to impersonate. Or more commonly, a client may neglect setting a sensible string - a known issue with KWallet.

If we can determine the process ID as has been suggested, that can be converted to the binary path. Maybe that could be tapmered-with too, if there's no tamper-proof system call for that, but that would already be more reliable than a client-specified string.

Once we have the app identity, KPXC can create a separate collection for each app, and let the user control which apps can access which collections (by default, only the one created for that app). Maybe that's already done?

Using collections for this lets binaries store more than secret. That's especially useful for interpreted scripts. This way, each script can save its own secrets in the interpreter's collection. The scripts for a particular interpreter still won't be isolated from each other, but at least they'll be isolated from other interpreters and binaries.

For further security, KPXC could fingerprint the binary on first access, and use that to control subsequent access. At its simplest, that could just be a SHA hash of the binary. That would mean that new binary versions will need to re-authenticate, but as far as I'm concerned, that's a feature. As long as the user has access control, he could tell KPXC to "let this newer version use that older version's collection". And the whole fingerprinting feature can be optional.

(In the future, there might be an online database of trusted binary fingerprints for each package, maintained by the package publishers. But that's a long way off.)

This isn't a perfect solution, but I think it's better than no isolation at all, a la KWallet.
edit: Should I post a separate feature request to discuss this further?

[droidmonkey]

You should read up on all the subsequent FDO Secrets PRs from this one first. A lot of work has been done and almost everything you mention above is done already. Exception being the hashing of the binary.

[michaelk83]

A lot of work has been done and almost everything you mention above is done already. Exception being the hashing of the binary.

Awesome, thanks! I have a bunch of PRs on my reading list already. Going over them.