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

get_credential(service, None) does not find anything after deleting one of two usernames #664

Open
mkaut opened this issue Dec 12, 2023 · 3 comments

Comments

@mkaut
Copy link

mkaut commented Dec 12, 2023

on Windows 10, using the default backend:

keyring.set_password('test', 'user-1', 'pwd-1')
keyring.set_password('test', 'user-2', 'pwd-2')
cred = keyring.get_credential('test', None)
print(cred.username, cred.password)

This returns user-2 pwd-2. So far, so good.

keyring.delete_password('test', 'user-2')
cred = keyring.get_credential('test', 'user-1')
print(cred.username, cred.password)
cred = keyring.get_credential('test', None)
print(cred)

Here, the first output is user-1 pwd-1, showing that the first credential is still there.
However, the second output None, i.e. the credential is not found without a username.

Is this a bug, or an expected behaviour?

@jaraco jaraco added the Windows label Mar 5, 2024
@jaraco
Copy link
Owner

jaraco commented Mar 5, 2024

At first blush, it looks like it's a bug... and probably has to do with the way that Windows doesn't support per-user credentials so keyring hacks it by creating "compound names" when the user names conflict for a given service.

After the first set_password, the credential store looks like:

system user
test user-1

Then adding the second credential causes the first entry to get the username prepended:

system user
user-1@test user-1
test user-2

At this stage, it's possible to get the most recent credential using just the system name, but without the username, it's not possible to get user-1. Even after you delete user-2, the state is:

system user
user-1@test user-1

It's no longer possible to query simply for system='test'.

When retrieving a credential, the following logic is used:

if username:
res = self._get_password(self._compound_name(username, service))
# get any first password under the service name
if not res:
res = self._get_password(service)
if not res:
return None

See #47 for the motivation for this approach.

I can't imagine a way for keyring to locate the compound name without being given the original username unless there were some way to enumerate all credentials. In #555, there was an attempt to implement some changes that would expose enumerating all credentials. If we had that capability, we could potentially scan all credentials for those where the username matches the {user}@{system} format. I'm not confident that could be done in a performant way.

I think we may be stuck with assuming that if the user is storing multiple passwords for the same system that they must always supply the username to retrieve them (and maybe document that expectation).

I'm open to other ideas.

@rtr1
Copy link

rtr1 commented Mar 14, 2024

To me the most straightforward path is to create an escape hatch by enabling an option like use_compound_name=False.

In my use cases, I much prefer the consistency that if I call keyring.set_password(a,b,c) twice, it’s updating the same entry in Windows Credential Manager. That’s usually what I want, e.g. when I’m updating a connection string, I just want to overwrite what’s already there. And I prefer that if I give it a compound name like keyring.set_password(‘user@service’, …) I get a single user@service entry, whereas the current approach will create a new entry named user@user@service, which doesn’t smell like the right path to me. At that point, we might as well store JSON strings as the password value to facilitate multiple usernames for a single service.

We’d need to keep use_compound_name=True as the default for backward compatibility of course.

Thoughts?

@mkaut
Copy link
Author

mkaut commented Mar 16, 2024

I can see a couple of different solutions, though I should stress that I don't know much about either this app or Windows credential manager - so feel free to ignore anything I write.

  • If I get what @rtr1 is suggesting, it is storing the data in @jaraco's example as

    system user password
    test {"user-1": "pwd-1", "user-2": "pwd-2"} ?
  • The first user can be stored as today. When a second user is added, move both to a compound name and use the base name to store the list of users:

    system user password
    user-1@test user-1 pwd-1
    user-2@test user-2 pwd-2
    test user-1,user-2 --

    One could also use JSON or similar to store the user names.
    One could also add a specified prefix or postfix to the system name, to avoid conflicts.

  • Like above, but store all the user names under one common system name (like "keyring_user_list"), either as a list or a dictionary with systems as keys and user lists as values. This would not work if the usernames in Windows credential manager have a limit of string length.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants