Remove the native system store from the keyring providers#15612
Merged
zanieb merged 1 commit intofeature/authfrom Sep 2, 2025
Merged
Remove the native system store from the keyring providers#15612zanieb merged 1 commit intofeature/authfrom
zanieb merged 1 commit intofeature/authfrom
Conversation
6ac2385 to
9a93ce8
Compare
1a62120 to
feacd5d
Compare
feacd5d to
3d3f798
Compare
3d3f798 to
fe28362
Compare
charliermarsh
approved these changes
Sep 1, 2025
| // If preview is enabled, we'll use the system-native store | ||
| if preview.is_enabled(PreviewFeatures::NATIVE_AUTH) { | ||
| return Ok(Self::System(KeyringProvider::native())); | ||
| } |
Member
There was a problem hiding this comment.
I think this could have some surprising effects... For example, if you run with --preview, suddenly all the creds you stored with the default backend are now gone, and auth will fail. How can we avoid that? Make this a dedicated setting? Make it tiered?
Member
Author
There was a problem hiding this comment.
I think it'd need a dedicated setting, which I was wary to add here but we should definitely figure out. I think we'll actually still always read from the plaintext store in the middleware? but, e.g., logout wouldn't work.
Member
There was a problem hiding this comment.
(Wary because it expands the scope of the PR? Or for another reason?)
Member
Author
There was a problem hiding this comment.
Yeah not necessarily because of the scope of the pull request, but wary because we then need to choose a name for a flag and be fairly certain that's what we want. Part of the intent of this pull request was to defer that decision.
fe28362 to
db5cd8b
Compare
db5cd8b to
cf140b2
Compare
cf140b2 to
8ed40ee
Compare
zanieb
added a commit
that referenced
this pull request
Sep 2, 2025
We're not sure what the best way to expose the native store to users is yet and it's a bit weird that you can use this in the `uv auth` commands but can't use any of the other keyring provider options. The simplest path forward is to just not expose it to users as a keyring provider, and instead frame it as a preview alternative to the plaintext uv credentials store. We can revisit the best way to expose configuration before stabilization. Note this pull request retains the _internal_ keyring provider implementation — we can refactor it out later but I wanted to avoid a bunch of churn here.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We're not sure what the best way to expose the native store to users is yet and it's a bit weird that you can use this in the
uv authcommands but can't use any of the other keyring provider options. The simplest path forward is to just not expose it to users as a keyring provider, and instead frame it as a preview alternative to the plaintext uv credentials store. We can revisit the best way to expose configuration before stabilization.Note this pull request retains the internal keyring provider implementation — we can refactor it out later but I wanted to avoid a bunch of churn here.