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

Support for empty string usernames is inconsistent #668

Open
djkawa opened this issue Mar 7, 2024 · 8 comments
Open

Support for empty string usernames is inconsistent #668

djkawa opened this issue Mar 7, 2024 · 8 comments
Labels

Comments

@djkawa
Copy link

djkawa commented Mar 7, 2024

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
@djkawa djkawa changed the title (Windows) Empty string username works in python 3.11 but returns None in python 3.12 (Windows) Empty string username works in python 3.11 but returns None in python 3.12 (keyring 24.3.1) Mar 7, 2024
@jaraco jaraco added the Windows label Mar 23, 2024
Repository owner deleted a comment from djkawa Mar 23, 2024
@jaraco jaraco changed the title (Windows) Empty string username works in python 3.11 but returns None in python 3.12 (keyring 24.3.1) Empty string username works in python 3.11 but returns None in python 3.12 (keyring 24.3.1) Mar 23, 2024
@jaraco
Copy link
Owner

jaraco commented Mar 23, 2024

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
Copy link
Owner

jaraco commented Mar 23, 2024

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
Copy link
Author

djkawa commented Mar 26, 2024 via email

@jaraco
Copy link
Owner

jaraco commented Aug 1, 2024

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 [email protected] 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
Copy link
Owner

jaraco commented Aug 1, 2024

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
Copy link
Owner

jaraco commented Aug 1, 2024

I plan to do a gradual rollout of the transition:

  • First, raise a DeprecationWarning, which will show up in tests and API usage, but not for users (Deprecate empty usernames #687).
  • Next, change the DeprecationWarning to a UserWarning so that it's visible to users.
  • Finally, if there are no reported and unmitigated concerns, replace the warning with an error.

@jaraco jaraco changed the title Empty string username works in python 3.11 but returns None in python 3.12 (keyring 24.3.1) Support for empty string usernames is inconsistent Aug 1, 2024
@dimbleby
Copy link
Contributor

dimbleby commented Aug 5, 2024

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
Copy link
Owner

jaraco commented Aug 7, 2024

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.

abn added a commit to abn/poetry that referenced this issue Nov 16, 2024
abn added a commit to abn/poetry that referenced this issue Nov 16, 2024
abn added a commit to abn/poetry that referenced this issue Nov 17, 2024
abn added a commit to abn/poetry that referenced this issue Nov 17, 2024
abn added a commit to abn/poetry that referenced this issue Nov 17, 2024
abn added a commit to abn/poetry that referenced this issue Nov 17, 2024
abn added a commit to abn/poetry that referenced this issue Nov 17, 2024
abn added a commit to python-poetry/poetry that referenced this issue Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants