-
Notifications
You must be signed in to change notification settings - Fork 877
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
Add support for retrieving credentials from keyring
#2254
Conversation
21d4571
to
4933953
Compare
|
|
b79188e
to
d785d55
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thank you so much for contributing and welcome to the project :)
This change overlaps with some work I've been considering, so I've left some notes about future plans — let me know how interested you are in pursuing some of the broader strokes I've outlined.
crates/uv/src/main.rs
Outdated
/// Function's similar to `pip`'s `--keyring-provider subprocess` argument, | ||
/// `uv` will try to use `keyring` via CLI when this flag is used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not match pip
's interface exactly here? This interface seems more limited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented it more limited on purpose - because we only have access to the CLI version of keyring, the other option (python import) isn't available.
Also, I can't say I'm a fan of pip default ("auto"). The way "auto" works in pip
:
Try endpoint. If 401
on first request, try python import keyring.get_credential
method. If python import method fails, try CLI.
You could implement the keyring retry on 401
via middleware as default I guess, but... do you want to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see they support --keyring-provider subprocess
or --keyring-provider auto
— I read this as --keyring-provider <name>
where you could specify what command to spawn for keyring information. I'm more okay with this then.
Does pip
use the --keyring-provider auto
by default? i.e. does it always do keyring lookups?
Do you have more context on how the keyring
Python library differs from the keyring
command line tool? Happy to look into this myself some too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does pip use the --keyring-provider auto by default? i.e. does it always do keyring lookups?
It does use auto
by default - but it will only do keyring lookups if the first request fails with a 401
status code when in auto
mode.
As for how CLI vs package differ:
The CLI tool uses the keyring python package under the hood. The only difference is that the python package doesn't require you to supply a username to keyring.get_credentials
, but the CLI tool does for keyring get
. The username passed to the CLI tool can be anything for the GCP plugin (because it's not actually checked). I don't know for sure about other cloud providers' plugins because I have not used them, but I would guess it's the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering, why don't you like the auto
behavior? It seems like it could be sensible to avoid sending credentials to URLs that do not require it. I don't have a preference at this point but every difference from pip
can cause confusion so I want to make an informed decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's just a personal distaste - but totally understand that differences from pip
are problematic and can understand the point about sending credentials unnecessarily.
Implementing the retry on 401
seems non-trivial, but I'm sure it's doable.
Steps:
Create policy to retry on first 401
status (using reqwest-retry
probably)
Before retry, delete stored auth state for URL so it will not short-circuit auth middleware
Somehow inform auth middleware to use keyring for future requests to this URL
To test:
Make sure auth middleware still runs on retry (I haven't tested this with reqwest-retry
so don't know)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that feels convoluted. I'll see if there are other opinions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to use same interface as pip
- and I have a good idea of how to implement the retry, but think it would be better to do in a follow-up PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be curious to hear @charliermarsh's opinion regarding only searching for authentication on 401s.
It does seem brittle to me, we've seen all sorts of quirky behavior from package indexes when unauthorized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't thought about it deeply but I have a negative reaction to only sending credentials after seeing a 401.
While changes are WIP, do you guys like the PR converted to Draft, or kept Open? |
@BakerNet if you don't want my feedback for a period of time you could mark it as a draft, otherwise I'll read through it periodically. |
Heads up that we had to change the netrc implementation to use middleware in #2325 because it caused a regression. Let me know if you have any problems resolving the conflicts. |
hey folks, happy to see this being implemented -- I'm using uv with artifact registry. Couple of questions:
|
I could probably easily add something like |
That would be really good! I'm even less involved than you -- just a user -- so let's hear from astral team perhaps -- @zanieb what do you think? |
@vlad-ivanov-name it sounds nice to bypass |
Just wanted to say (as only a user) thank you for the contribution and to mention that I have test-driven 177bdee and
works perfectly with The only (extremely minor) quirk I encountered is that if I include the username in the index URL
then the command above errors with
But when using non-uv
But to be clear this is not a problem for me at all, just something that could be a useful piece of info for users trying to convert from pip to uv who stumble upon this PR. (If anything it was inconvenient to me that pip required the username - maybe it is actually a bug specific to And possibly totally unrelated to this PR, if there might be a |
support UV_KEYRING_SUBPROCESS env var
Seems like a totally uncontroversial QoL improvement to me, so I merged it in. Thanks for testing this out, @thomasgilgenast btw! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Thanks you!
I have some specific questions as well as a more general one regarding keyring lookups in Rust.
lazy_static! { | ||
// Store credentials for NetLoc | ||
static ref PASSWORDS: Mutex<HashMap<NetLoc, Option<Credential>>> = Mutex::new(HashMap::new()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like @charliermarsh or @BurntSushi to review this part
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we store None
credentials in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we store
None
credentials in here?
Just so we don't re-check netrc or keyring when they already failed to find credentials for the url's net location
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, thanks!
crates/uv-auth/src/keyring.rs
Outdated
/// Keyring provider to use for authentication | ||
/// | ||
/// See <https://pip.pypa.io/en/stable/topics/authentication/#keyring-support> | ||
#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] | ||
#[cfg_attr(feature = "clap", derive(clap::ValueEnum))] | ||
pub enum KeyringProvider { | ||
// Will not use keyring for authentication | ||
#[default] | ||
Disabled, | ||
// Will use keyring CLI command for authentication | ||
Subprocess, | ||
// Auto, - not yet implemented | ||
// Import, - will probably never be implemented | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think that it's worth using something like https://docs.rs/keyring/latest/keyring/ instead of a subprocess? It looks like it overlaps a bit with the abstractions you introduce and it might make more sense if it can provide the same functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think that it's worth using something like docs.rs/keyring/latest/keyring instead of a subprocess?
Instead? No. In addition? Yes.
As mentioned in issue #1520 - the cloud providers provide backend plugins for the python keyring tool.
Using something like https://docs.rs/keyring/latest/keyring/ would be fine for the Import
case, but supporting a command-line keyring
should still exist regardless (especially if you're trying to keep parity with pip) and if someone makes a CLI keyring with cloud provider support based on keyring-rs
in the future, cool. If it has the same CLI interface, it should just work. If it has a different name, add to the KeyringProvider
s or implement the custom version like discussed with @vlad-ivanov-name above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am only commenting as an observer, but commenters on #1520 appeared to be in favor of at least reproducing pip's subprocess behavior first as the simplest way to automatically ensure compatibility with existing python-based keyring implementations for authenticating with python package indices (which may not have rust keyring counterparts at the current time).
But maybe I am misunderstanding how plugging in the rust keyring here would work, for example if uv would call out via a subprocess to python-based keyring implementations, then stash the result in rust keyring? Or if rust keyring has already implemented a standard way to call out to other keyrings via a subprocess and uv can somehow reuse that instead of re-implementing it? From a naive point of view, I don't see a simple/obvious way to avoid a subprocess entirely.
All that being said, I think that users who don't need any specific/"customized" keyring implementation and instead simply need a basic mechanism to store their password to avoid typing it out every time could be served with rust keyring (without a subprocess). But I think that the "customized" python keyring implementations like keyrings.google_artifactregistry_auth
are "common enough" (subjective) that it could be worth considering how those could also be supported. (The subprocess-based path taken by the current state of this PR does support both "simple" and "customized" keyring use cases.)
Edit: sorry, totally redundant with the discussion above.
/// Attempt to use `keyring` for authentication for index urls | ||
/// | ||
/// Due to not having Python imports, only `--keyring-provider subprocess` argument is currently | ||
/// implemented `uv` will try to use `keyring` via CLI when this flag is used. | ||
#[clap(long, default_value_t, value_enum, env = "UV_KEYRING_PROVIDER")] | ||
keyring_provider: KeyringProvider, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little hesitant on this interface. We might want to consider what our ideal interface for this then just include a dedicated interoperability flag for pip's interface.
I think in part this depends on our future plans around keyring support (which I know there was some discussion about already). In particular, if we use something like the Rust keyring
crate do we need to provide any options other than a boolean toggle? Perhaps we do still want to allow the user to select a specific provider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like a value enum is a pretty flexible interface - you can just add additional options to the enum and change logic based on the variant. If there are additional items needed for an option, you can just add another argument that relies on a specific KeyringProvider
value like --keyring-provider custom --custom-keyring-command keyring-rs
as a possible example (I'm certain clap
supports this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, sold. Thanks.
crates/uv/src/main.rs
Outdated
/// Function's similar to `pip`'s `--keyring-provider subprocess` argument, | ||
/// `uv` will try to use `keyring` via CLI when this flag is used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be curious to hear @charliermarsh's opinion regarding only searching for authentication on 401s.
It does seem brittle to me, we've seen all sorts of quirky behavior from package indexes when unauthorized.
Co-authored-by: Zanie Blue <[email protected]>
crates/uv-auth/src/middleware.rs
Outdated
if let Ok(auth) = get_keyring_auth(&url) { | ||
// Keyring auth found - save it and return the request | ||
req.headers_mut().insert( | ||
reqwest::header::AUTHORIZATION, | ||
basic_auth(auth.username(), auth.password()), | ||
); | ||
AuthenticationStore::set(&url, Some(auth)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do the refactor to separate None
from the Error
case we should be able to log failures here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the correct way to log errors in uv
codebase, btw? warn!
or warn_user!
maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm that's tough.
Ideally we'd attach this as a hint to request failures so when we error we'd be able to suggest why. In the meantime, I think a tracing message is the way to go unless it's very actionable for the user to fix the problem. I believe the difference is a tracing message will only show up with -v
but warn_user!
will always display.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Thanks for addressing all the feedback so promptly. I've added @charliermarsh as a reviewer — once he's happy we'll be good to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! I think I need to better understand some of the credential flows before merging.
/// | ||
/// Due to not having Python imports, only `--keyring-provider subprocess` argument is currently | ||
/// implemented `uv` will try to use `keyring` via CLI when this flag is used. | ||
#[clap(long, default_value_t, value_enum, env = "UV_KEYRING_PROVIDER")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add this to the list of supported environment variables in the README.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll also want to document support and precedence in a registry authentication section following https://github.com/astral-sh/uv?tab=readme-ov-file#git-authentication but I can do that in a follow-up (just posting so I don't forget).
|
||
// Try auth strategies in order of precedence: | ||
if let Some(stored_auth) = AuthenticationStore::get(&url) { | ||
// If we've already seen this URL, we can use the stored credentials |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my own clarification... this will propagate the credentials based purely on host
etc., right? In other words, the URLs do not need to be identical, is that correct? Just the same "realm" and so on?
Is this generally a safe and accepted thing to do? (That is: propagate the credentials here.) It's a little different than what we do on main
which is more limited: when we make a request, we propagate credentials to the URLs that are found on the page returned by the request (like with_url_encoded_auth
, I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we're using the "net loc" which is the same "realm". This is compliant with the relevant web standard RFC and AFAIK exactly what pip does. It seems totally fine to me but I appreciate the extra scrutiny.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why we also need to call with_url_encoded_auth
before the middleware then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't follow the full flow after the url
is copied - I saw it was being used in File
structs and didn't want to potentially introduce regression by assuming theses urls
having the auth copied to were going to be used in the same run of uv
My other comment on the URLEncodedAuth being insecure, but relying on this copying from trusted urls instead was also a factor in my keeping the copy functionality.
); | ||
} | ||
// Url must already have auth if before middleware runs - see `AuthenticationStore::with_url_encoded_auth` | ||
Credential::UrlEncoded(_) => (), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you expand on the comment here? Should this be unreachable!
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea here is, if the url does not come to the middleware with URL-encoded auth, we shouldn't introduce URL-encoded auth because it's an inherently unsafe way to send auth data. On main, there was already code copying the auth from trusted urls onto response/body urls so this line shouldn't be hit under normal circumstances anyways.
pub password: Option<String>, | ||
} | ||
|
||
pub struct AuthenticationStore; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a huge fan of the singleton struct here but I don't need to block the PR on it. It's just not something we've really done anywhere else that I can remember. Maybe @BurntSushi would have a better opinion on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagined the passwords would live on it but I think that's infeasible for some reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial plan was for a store instance to be a field of the AuthMiddleware
struct and the store to hold an instance of the password table - but you do not have access to mutable self in the Middleware
trait, so that was the limitation I ran into. Combine that with keeping the copying from trusted URL that wouldn't have access to the same instance - I went with this approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you just use interior mutability to get around this? e.g. RefCell<T>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you could just do it with the Mutex within the struct. But we can change this later if desired.
crates/uv/src/main.rs
Outdated
/// Function's similar to `pip`'s `--keyring-provider subprocess` argument, | ||
/// `uv` will try to use `keyring` via CLI when this flag is used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't thought about it deeply but I have a negative reaction to only sending credentials after seeing a 401.
FileLocation::AbsoluteUrl(url) | ||
let url = Url::parse(&file.url) | ||
.map_err(|err| FileConversionError::Url(file.url.clone(), err))?; | ||
let url = AuthenticationStore::with_url_encoded_auth(url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the assumption here is that we've already hit the base URL on a previous invocation of the middleware, and so its credentials are already stored in the AuthenticationStore
, and will be propagated here if needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was my assumption. That if base
has auth, it received it from a copy from a previous request (in FlatIndexClient.read_from_url
or RegistryClient.simple_single_index
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm cool with this, thanks for the nice PR! I defer to @zanieb to merge.
keyring
Summary
Adds basic keyring auth support for
uv
commands. Adds clone ofpip
's--keyring-provider subprocess
argument (using CLIkeyring
tool).See issue: #1520
Test Plan
Hard to write full-suite unit tests due to reliance on
process::Command
forkeyring
cliManually tested end-to-end in a project with GCP artifact registry using keyring password:
requirements.txt