-
Notifications
You must be signed in to change notification settings - Fork 648
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
Rewrite AuthMiddleware
and credential caching
#2984
Conversation
350bd0b
to
b4fdde9
Compare
AuthMiddleware
and improve authentication coverageAuthMiddleware
and credential caching
AuthMiddleware
and credential cachingAuthMiddleware
and credential caching
5ec0d0a
to
f7087ee
Compare
I think I should bench this for performance regressions in request-heavy workloads. |
f7087ee
to
585bb8d
Compare
585bb8d
to
2d1c3cf
Compare
072426e
to
5b75701
Compare
Benchmarks not showing change when there's not authentication involved
I want to craft a bench that uses the keyring so we can ensure we're good there. |
// Check for credentials attached to (1) the request itself | ||
let credentials = Credentials::from_request(&request); | ||
// In the middleware, existing credentials are already moved from the URL | ||
// to the headers so for display purposes we restore some information |
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.
Not a huge fan of code that only runs in debug. Maybe we just do this every time (or never)?
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.
It's just for display, I wanted to avoid the extra clone on every request. It's really helpful for seeing what username we're processing and if there's a password. I think it's worth having for debugging and I'd want it on-always rather than removed.
debug!("Found credentials in netrc file for {url}"); | ||
request = credentials.authenticate(request); | ||
new_credentials = Some(Arc::new(credentials)); | ||
// (4) The keyring |
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 confirming: if we have both netrc and keyring enabled, and the URL isn't in the netrc, we subsequently check the keyring, right? (That's my read of the code. Confirming my understanding + that it's intended.)
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.
Yep! and that's the same as the behavior on main
.
request.url(), | ||
credentials | ||
.get() | ||
.and_then(|credentials| credentials.username()), |
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 this case exactly -- you provided a username but no password, and we want to lookup in the netrc based on the username?
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 provide a username, it needs to match the one in the netrc. If you don't, the full credential can be retrieve from the netrc (username and password).
uv/crates/uv-auth/src/credentials.rs
Lines 44 to 47 in 65ac260
// Ensure the username matches if provided | |
if username.is_some_and(|username| username != entry.login) { | |
return None; | |
}; |
# Conflicts: # Cargo.lock # crates/uv-configuration/Cargo.toml # crates/uv/src/commands/pip_compile.rs # crates/uv/src/commands/venv.rs # crates/uv/src/main.rs # Conflicts: # crates/uv-auth/src/keyring.rs
# Conflicts: # crates/uv-auth/src/cache.rs
de57cef
to
c909cbe
Compare
Closes - #2822 - #2563 (via #2984) Partially address: - #2465 - #2464 Supersedes: - #2947 - #2570 (via #2984) Some significant refactors to the whole `uv-auth` crate: - Improving the API - Adding test coverage - Fixing handling of URL-encoded passwords - Fixing keyring authentication - Updated middleware (see #2984 for more)
Further work based on #2976 with a focus on improving the correctness of our
AuthMiddleware
.I ended up needing to make significant changes to ensure we properly populate passwords on requests that already have a username attached.
The behavior can be summarized as follows:
trace
levelsWe also add some changes to keyring authentication:
Closes #2570
Closes #2563
Partially address: