Backend inference between KWallet and Gnome leads to unexpected results

[jaraco]

The SecretService keyring will be queried first, but if it's a miss (no password defined), the kwallet backend(s) will be queried, triggering the wallet creation UI. I believe the SecretService store is being queried first, but just not satisfying the operation. [...]
In some ways, the behavior you describe is by design. But you've stumbled on a flaw in the design. [...]
[...] Where a backend triggers blocking UI elements and raises KeyringLocked exceptions, however, that's not desirable.

My scenario is the same described by the OP, including exactly the same set of backends and priorities: Gnome user, kwallet installed but never used (possibly pulled as a dependency of some other software), on first run (i.e, when trying to read data that was not yet set) it triggers a KDE Wallet Creating UI.

I realize that after some data is set (possibly to Gnome Keyring, given my backend priorities) the KDE UI will not show anymore. But still, this first-time UI is very annoying and surprising.

What's the current status on this? I mean, what should I do, as a third-party developer, to prevent this from happening with my end-users? Is there any "official" (or "blessed") workaround?

A possible approach I'm thinking for my specific software is to try to detect a first-time usage by other means (a missing config file, for example) and the set the data to a dummy/blank value before attempting to read it. That, I think, would avoid triggering the "backend found but no data set, let's try another backend" behavior.

Is this a good approach? Any recommendations?

Originally posted by @MestreLion in #391 (comment)

[jaraco]

See #458 where I explored one mitigation to make explicit the opt-in for KWallet, but that seems like a non-starter.

A possible approach I'm thinking for my specific software is to try to detect a first-time usage by other means (a missing config file, for example) and the set the data to a dummy/blank value before attempting to read it.

It shouldn't be necessary to configure some proxy state on the system to cause the desired behavior.

Can you report the output of python -m keyring --list-backends?

[MestreLion]

$ python3 -m keyring --list-backends
keyrings.alt.file.PlaintextKeyring (priority: 0.5)
keyring.backends.fail.Keyring (priority: 0)
keyring.backends.kwallet.DBusKeyringKWallet4 (priority: 3.9)
keyring.backends.kwallet.DBusKeyring (priority: 4.9)
keyring.backends.SecretService.Keyring (priority: 5)
keyring.backends.chainer.ChainerBackend (priority: 10)
keyrings.alt.file.EncryptedKeyring (priority: 0.6)

$ pip3 list | grep keyring
keyring                       21.2.1
keyrings.alt                  3.4.0

Which, IIRC, is the same backends list and priorities as the other issue's OP.

I believe the behavior on first run goes like this:
Chainer (10) goes first, and it selects SecretService (5) (that's Gnome's keyring, right?). But since that password has not been set yet, Chainer considers SecretService a failure, and tries the next on the list, kwallet (4.9), and since kwallet itself has never been initialized in my environment, it triggers the Wallet creation UI.

[MestreLion]

Maybe the solution is for backends not to create their infrastructure when being queried about a password. KWallet initialization UI, Encrypted File creation, etc, should only be triggered when setting a password. KWallet should be able to say "no" to Chainer when asked for a password before (and instead of) its initialization.

[jaraco]

Maybe the solution is for backends not to create their infrastructure when being queried about a password. KWallet initialization UI, Encrypted File creation, etc, should only be triggered when setting a password. KWallet should be able to say "no" to Chainer when asked for a password before (and instead of) its initialization.

I like where you're going with this. I'd think about it slightly differently, and phrase it like this: KWallet should present an interface that allows querying for the state of the wallet to determine if the wallet is initialized. If the wallet is not initialized, queries to (get_password, get_credentials) should return without initializing a wallet.

When I look at the code, everything goes through the connected method, which boils down to these steps:

        bus = dbus.SessionBus(mainloop=DBusGMainLoop())
        wId = 0
            remote_obj = bus.get_object(self.bus_name, self.object_path)
            self.iface = dbus.Interface(remote_obj, 'org.kde.KWallet')
            self.handle = self.iface.open(self.iface.networkWallet(), wId, self.appid)

There's nothing here about initialization.

There is a check in get_password for hasEntry, but I suspect that call is happening after the initialization.

So the question is, does the KWallet API allow to probe the wallet for its initialization state?

[jaraco]

KWallet seems to be extremely sparsely documented, and the unit tests for this backend are skipped, even in CI, so I have very little confidence in the backend.

I've found kde-in-docker, so I may be able to get spun up with a KDE environment, but I'm not sure that will help with the missing API.

Would someone volunteer to engage with the KDE/KWallet maintainers and explain the situation and request a feature to support the use-case above (or point to documentation that describes existing functionality for the same)?

[MestreLion]

Why should keyring bother with a new API method to determine if a backend is initialized? Chainer (or keyring) should not have to deal with backend's implementation details.

IMHO the API description for the existing get_password, get_credentials, etc could be changed to explicitly request backends to return non-success if not initialized, without performing any initialization for queries. Then each backend could adjust themselves for this new requirement. No new API methods required.

This might also be good change for other backends, so for example keyrings.alt.file.EncryptedKeyring does not create the its (blank) file upon a simple query.

[MestreLion]

everything goes through the connected method, which boils down to these steps:

        bus = dbus.SessionBus(mainloop=DBusGMainLoop())
        wId = 0
            remote_obj = bus.get_object(self.bus_name, self.object_path)
            self.iface = dbus.Interface(remote_obj, 'org.kde.KWallet')
            self.handle = self.iface.open(self.iface.networkWallet(), wId, self.appid)

There's nothing here about initialization.

By reading that snippet, I'd bet this is performed by the networkWallet() method. Its constructor most likely creates the infrastructure and triggers the GUI.

[jaraco]

Its constructor most likely creates the infrastructure and triggers the GUI.

Right. But I can’t imagine a way to determine initialization status prior to invoking that call.

[MestreLion]

I can’t imagine a way to determine initialization status prior to invoking that call.

My point is, why should ChainerBackend (or keyring itself) need to determine initialization status of a backend?

My suggestion is to modify each backend so it does not perform any initialization upon a password query. No need for a new API method. I mean, both approaches require changing current backends, either to add this new is_initialized() method, or to modify the current get_password() behavior, correct? If so, why not go with the simpler approach?

[jaraco]

If so, why not go with the simpler approach?

I'd prefer to go for the simpler approach. Unfortunately, both of these approaches are abstract and I can't conceive of how to make them concrete. Can you provide a demonstration of what you have in mind?

[MestreLion]

For example, take this snippet from keyrings.alt's Keyring class that serve as base for both keyrings.alt.file.PlaintextKeyring and keyrings.alt.file.EncryptedKeyring backends:

    def get_password(self, service, username):
        """
        Read the password from the file.
        """
        assoc = self._generate_assoc(service, username)
        service = escape_for_ini(service)
        username = escape_for_ini(username)

        # load the passwords from the file
        config = configparser.RawConfigParser()
        if os.path.exists(self.file_path):
            config.read(self.file_path)

        # fetch the password
        try:
        ...

With the approach I suggested, the if os.path.exists(self.file_path) test should be performed at the very top of method and, if false, the method should immediately return None. This way it makes sure it does not perform any initialization (such as creating the file) on a password query.

The change to that backend would be:

    def get_password(self, service, username):
        """
        Read the password from the file.
        """
        if not os.path.exists(self.file_path):
            return

        assoc = self._generate_assoc(service, username)
        service = escape_for_ini(service)
        username = escape_for_ini(username)

        # load the passwords from the file
        config = configparser.RawConfigParser()
        try:
            config.read(self.file_path)
            ...

For the case of the file backend this was trivial, although for this particular problem, unnecessary, as it already does not create the file on get_password(). (But it is a welcome change anyway, it optimizes the code and avoid both a race condition and dealing with IOErrors from configparser - Should I PR it?)

In case of KWallet, the concrete change is not so trivial. Judging from a quick look at the code you posted, it seems my approach would require a create flag in connected(), cascaded to iface.networkWallet(), defaulting to True (the current behavior), and set to False by get_password() and get_credential(), something like this:

    def connected(self, service, create=True):
        ...
        try:
            remote_obj = bus.get_object(self.bus_name, self.object_path)
            self.iface = dbus.Interface(remote_obj, 'org.kde.KWallet')

            # Can `create` be passed when open()'ing a DBus interface?
            # If not, it would require a new interface (`networkWalletCheck()` perhaps?)
            self.handle = self.iface.open(self.iface.networkWallet(), wId, self.appid)

        except dbus.DBusException as e:
            raise InitError('Failed to open keyring: %s.' % e)
        ...

    def get_password(self, service, username):
        """Get password of the username for the service"""
        try:
            if not self.connected(service, create=False):
                # the user pressed "cancel" when prompted to unlock their keyring.
                return
        except InitError:
            # keyring does not exist
            return
        ...

In any case, unless KWallet already provides an API to check for keyring existence (without prompting its creation), either networkWallet() needs to be changed, or a new interface must be created, and both changes are, from the looks of it, outside the backend's scope. Maybe open an issue for KWallet?

[jaraco]

In any case, unless KWallet already provides an API to check for keyring existence (without prompting its creation), either networkWallet() needs to be changed, or a new interface must be created, and both changes are, from the looks of it, outside the backend's scope. Maybe open an issue for KWallet?

This issue is exactly the one I was attempting to express above. I'm seeking volunteers to do that work.

[MestreLion]

This issue is exactly the one I was attempting to express above. I'm seeking volunteers to do that work.

Yeah, it took me a quite some time to realize you were talking about KWallet itself, the server/daemon/interface, and not the kwallet keyring backend.

KWallet seems to be extremely sparsely documented

No kidding about that! After dozens of google searches I wasn't able to find any relevant documentation about their DBus interface. Or any documentation whatsoever. And their official bugtracker is very awkward to use...

Would someone volunteer to engage with the KDE/KWallet maintainers and explain the situation and request a feature to support the use-case above (or point to documentation that describes existing functionality for the same)?

It doesn't help that their Github repo does not enable issues. It seems this one is just a mirror, nut...

... I was able to find another repository for KWallet! That one seems to be their "primary" repo, and it has issues enabled! At least now we have a place to ask questions, request features and add PRs!

[MestreLion]

https://invent.kde.org/frameworks/kwallet/-/issues/1

https://bugs.kde.org/show_bug.cgi?id=434480

Hope that helps!

[encukou]

That issue mentions a pull request which has this comment from a year ago:

I'm not sure if we have anyone with knowledge to review something that big for KWallet at this point, you can see that by looking at https://invent.kde.org/frameworks/kwallet/-/commits/master that we've basically been in maintenance mode for a long time.

.. but which has had some activity recently.

[Flimm]

For those looking for the workaround like I was, here it is. This will cause keyring to use GNOME's keyring instead of kwallet on systems similar to mine. Make sure the Python package secretstorage is installed. Then create the configuration file ~/.config/python_keyring/keyringrc.cfg and edit it to look like this:

[backend]
default-keyring=keyring.backends.SecretService.Keyring