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

Find password even with no username supplied in OSX backend #655

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

maarten0912
Copy link

@maarten0912 maarten0912 commented Oct 10, 2023

Find a keyring password even if no username is supplied in the OS_X backend

@jaraco
Copy link
Owner

jaraco commented Nov 12, 2023

Can you tell me more about the motivations for making this change? Under what conditions is a null versus empty username relevant for macOS? What compatibility considerations should we make for users relying on the current behavior (None -> "")?

@jaraco
Copy link
Owner

jaraco commented Mar 5, 2024

Any thoughts on my questions?

@maarten0912
Copy link
Author

Any thoughts on my questions?

The motivation for the change is because I wanted to find a keyring based only on its name (kSecAttrService). In my use case, the account of the keyring (kSecAttrAccount, but called username above) was not known and not important. The API will give the first result in case there are multiple keyring with the specified name, which was what I wanted in my case.

I would say that the library should work for querying empty usernames (account name is "") of keyrings and also for querying without a username (account name is None). Unfortunately, I do not see what we can do for users relying on the current behaviour.

@jaraco
Copy link
Owner

jaraco commented Mar 12, 2024

I don't have a good understanding of the differences between the following scenarios:

  • kSetAttrAccount = ''
  • kSetAttrAccount = None
  • no kSetAttrAccount

I'm guessing kSetAttrAccount of None is invalid.

More importantly, we'll probably want to align with other backends to provide an expectation at the keyring level of what should happen for empty or null usernames.

Since there do not appear to be any tests protecting the existing behavior (None → <empty string>), we should at least trace that code and determine when that logic was added to see what the motivation might have been for it.

@jaraco jaraco closed this Mar 12, 2024
@jaraco jaraco reopened this Mar 12, 2024
Copy link
Owner

@jaraco jaraco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting - it appears as if that logic was added when the API was migrated from C to ctypes. The C logic also translated None to the empty string.

This all makes me think the logic is more likely incidental than necessary... and support for None was minimal. Indeed, there are no tests for it and the type spec indicates a string is required.

A couple of requests:

  • Can you add a test or two to cover the new expectation?
  • Can you verify what happens when None is passed explicitly for kSecAttrAccount?
  • Can you unify the logic across all methods (get/set/del/...)?

@jaraco
Copy link
Owner

jaraco commented Aug 1, 2024

I have some news regarding this issue. In #668 and #687, I'm exploring completely deprecating support for empty usernames across all keyrings to provide consistency. The guidance in that change is that all clients of keyring should always pass a non-empty username, even if it's just some static value. Some backends, like Windows, don't behave properly if the username field is empty, and I want to try to avoid inconsistencies (so an application doesn't test on Linux or Mac and then find that users on Windows get a bad experience).

If support for empty usernames is valuable to you, can you comment in #668 as to why? Thanks.

daaniam added a commit to daaniam/keyring that referenced this pull request Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants