jaraco / keyring

MIT License
1.24k stars 152 forks source link

Support for empty string usernames is inconsistent #668

Open djkawa opened 5 months ago

djkawa commented 5 months ago

Describe the bug:

Storing a password with an empty string "" username in windows 64, returns None with python 3.12.

To Reproduce:

All on windows 64, set up an environment with python 3.12 and keyring 24.3.1. Set a password with an empty string username "". Retrieve the password -> None.

Expected behavior Doing the same thing with python 3.11, the password is retrieved.

Environment

Windows 64
Python 3.12.2
Keyring 24.3.1
jaraco commented 5 months ago

On my Windows system, I see consistent behavior across Python versions:

 ~ # py -3.11 -m pip-run keyring -- -m keyring set system ''
Password for '' in 'system': 
 ~ # py -3.11 -m pip-run keyring -- -m keyring get system ''
 ~ # py -3.12 -m pip-run keyring -- -m keyring get system ''

I can confirm in credential manager that the password is being stored with an empty username. I'm unsure why it's not able to be retrieved.

jaraco commented 5 months ago

I'm not confident that empty usernames was ever supported. I searched the code and there are no tests guaranteeing that empty usernames work, so if they happened to work, it was a lucky accident. In bc34083, I added such a test, and it's failing right away (on Windows only), seemingly due to a failure to delete the password on cleanup.

More troubleshooting is called for. Can you tell me more about your use-case? How long have you been using empty usernames? Do you know why I've been unable to replicate your findings? What version of Python 3.11 do you have? Can you confirm that you have the latest keyring on both Python 3.11 and 3.12?

djkawa commented 5 months ago

Hi Jason,Thanks for your answers.

I use conda forge to create my environments under windows 10 64 bits.

Python 3.11 is 3.11.8 Python 3.12 is 3.12.2

Using Keyring 25.0.0, I now observe the same behavior as you do which is that the password is saved but cannot be retrieved when the username is an empty string. Weirdly, I downgraded keyring to 24.3.1, and I cannot retrieve the password under python 3.11 as well. Which I swear I could when I wrote that ticket. So I am quite puzzled. I am perfectly happy to go with the lucky accident use case. When I started using the keyring library a while ago, I wrote a little pytest file on its behavior to see what was working and what was not and familiarize myself with the library. And among those tests, I parametrized the user to be an empty string username.I wanted to see if that was working as this could be useful if I wanted to save a password for something that did not have a notion of username (maybe a global password for a software). Furthermore, on windows, as you probably know, the credential manager is already user specific. It turns out that I never used an empty username string in my keyring usage, but that little test has now started to fail. It’s fairly easy to circumvent so I completely understand if you just want to call this “unsupported”. But since my little test had started to fail, I thought I would report it in case that was a supported feature. I hope this answers all your questions and I’ll let you decide what to do with this. Many thanks.

jaraco commented 3 weeks ago

It's been a crazy year for me, and I'm just getting around to responding. Sorry for the delay.

on windows, as you probably know, the credential manager is already user specific

That's pretty much the case on every platform; each user gets their own credential store (protected by the user's login password). The notion of the username, however, is not for the user saving the password, it's for the username in the system for which the password is being saved. So my local username might be WORKGROUP\JARACO, the username in the credential store might be jaraco or jaraco@contoso.com or sharedaccount.

In most cases, in my experience, there's some name or identifier for the account for which this credential is relevant. Even for something like PyPI, I set the username to __token__ and the password to the API token because that's what twine expects when uploading artifacts.

I could when I wrote that ticket

It's conceivable that a Windows update broke the behavior, or maybe a bugfix Python version altered the way ctypes worked with empty strings.

I believe you when you say it was working at one point, but it seems it's no longer the case.


I do think we want to say that the empty username is unsupported, since it seems that not all upstreams support it. Moreover, I think we should make that behavior explicitly disallowed, so someone doesn't use blank usernames on one system only to find that it breaks on a different one. We'll want a deprecation period, in case there are non-Windows users reliant on the lucky accident.

jaraco commented 3 weeks ago

In 7ae5dd0, I've started exploring validating the username to deprecate the acceptance of an empty username. Unfortunately, that approach is messy, requiring each backend to call the function explicitly (and fail to enforce otherwise). Due to the base class design, there's no good hook for applying this behavior across all backends.

I can conceive of an approach that could apply the behavior universally without requiring the backend to implement anything, but it would involve creating a metaclass that wraps cls.set_password when the subclass is created.

Another option would be to change the interface for backends so they have to implement a different method for setting a password (e.g. ._set_password_impl).

The first approach seems less intrusive and might be effective, if a little involved, so I'm going to explore that.

jaraco commented 3 weeks ago

I plan to do a gradual rollout of the transition:

dimbleby commented 3 weeks ago

Is the API consistent after disallowing empty usernames? at get_credential() we have

        The *username* argument is optional and may be omitted by
        the caller or ignored by the backend. Callers must use the
        returned username.

but what is the meaning of omitting the username here? What value can it return, if it was not possible to set a credential with no username?

jaraco commented 3 weeks ago

Since it'll no longer be possible to set a credential without a username, some username will need to be provided, like "username" or "token":

>>> keyring.set_password('myservice', 'token', 'secret123')

Then, retrieving the credential will include that name:

>>> cred = keyring.get_credential('myservice')
>>> cred.username == 'token'  # or just ignore the username if it's irrelevant to the application
True

I do see a problem with the Windows.WinVaultKeyring.get_credential; it doesn't allow the username to be omitted (default of None as indicated in the base class), so for now you need to pass cred = keyring.get_credential('myservice', username=None).

Maybe we want to allow backends to accept username=None for set_password, but that also is also currently unsupported and untested.

Apparently, the current configuration supports username=None on Windows:

>>> import keyring
>>> keyring.set_password('foo', None, 'secret123')
>>> keyring.get_password('foo', None)
'secret123'
>>> keyring.get_credential('foo', username=None).username is None
True

I would not recommend using that, though, as a None username is also deprecated (at least as currently written). If we want to support null usernames, we'll want to build that in as an explicit feature rather than a lucky accident.