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

Setting a blank password causes subsequent 'set' calls to fail #573

Closed
AlexanderVatov opened this issue May 10, 2022 · 2 comments
Closed
Labels

Comments

@AlexanderVatov
Copy link

AlexanderVatov commented May 10, 2022

Describe the bug
If a password is set to '', any subsequent attempts to set it to another value result in the following error:

Traceback (most recent call last):
  File "/Library/Python/3.8/site-packages/keyring/backends/macOS/__init__.py", line 40, in set_password
    api.set_generic_password(self.keychain, service, username, password)
  File "/Library/Python/3.8/site-packages/keyring/backends/macOS/api.py", line 161, in set_generic_password
    Error.raise_for_status(status)
  File "/Library/Python/3.8/site-packages/keyring/backends/macOS/api.py", line 114, in raise_for_status
    raise cls(status, "Unknown Error")
keyring.backends.macOS.api.Error: (-25299, 'Unknown Error')
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "/Library/Python/3.8/site-packages/keyring/core.py", line 60, in set_password
    get_keyring().set_password(service_name, username, password)
  File "/Library/Python/3.8/site-packages/keyring/backends/macOS/__init__.py", line 44, in set_password
    raise PasswordSetError("Can't store password on keychain: " "{}".format(e))
keyring.errors.PasswordSetError: Can't store password on keychain: (-25299, 'Unknown Error')

To Reproduce

import keyring
keyring.set_password('test-keyring-123', 'test', 'a') # Succeeds
keyring.set_password('test-keyring-123', 'test', 'b') # Succeeds
keyring.set_password('test-keyring-123', 'test', '')  # Succeeds
keyring.set_password('test-keyring-123', 'test', 'a') # Fails

Expected behavior
One would expect set_password not to be affected by previous calls of set_password.

Environment

Python 3.8.9 on macOS

$ pip list | grep keyring
keyring                 23.5.0
$ keyring --list-backends
keyring.backends.chainer.ChainerBackend (priority: -1)
keyring.backends.macOS.Keyring (priority: 5)
keyring.backends.fail.Keyring (priority: 0)

Additional context
Getting the blank password works fine. Deleting it and then setting it to any non-blank password also works as a workaround.

@jaraco jaraco added the macOS label May 23, 2022
@IbrahimAH
Copy link

IbrahimAH commented Dec 15, 2022

@AlexanderVatov this occurs in

if find_generic_password(name, service, username, not_found_ok=True):

as seen below
image
as you can see, the function find generic password fetches the password as ''. however, in python, empty strings resolve as False. The if statement therefore resolves as false and never deletes the password.
error -25299 means that an item with this identifier already exists. Of course it throws this error, as the item was never deleted!

in your code it can be protected by ensuring set passwords are len() > 1, but looking into this issue now


looking into this further. keyring.get_password has strange behaviour for getting empty passwords too. i think by luck it ends up being '' (empty string). when it tries to call cfstr_to_str, the data it passes in is a None pointer of size 0. this resolves to b'' which is then decoded to '' at https://github.com/jaraco/keyring/blob/main/keyring/backends/macOS/api.py#L95

@IbrahimAH IbrahimAH mentioned this issue Dec 15, 2022
@jaraco jaraco closed this as completed in aa2a9bd Dec 18, 2022
jaraco added a commit that referenced this issue Dec 18, 2022
@IbrahimAH
Copy link

IbrahimAH commented Dec 19, 2022

thanks for the super quick review and fix @jaraco, awesome stuff!

just one question, would it be worth removing the not_found_ok flag from find_generic_password now that nothing else uses it?
https://github.com/jaraco/keyring/blob/main/keyring/backends/macOS/api.py#L130
and then youd also what to remove the 2 lines of code using that flag in the same function (142-143)

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