Apply from-URL credentials in authentication middleware (#2449)

## Summary

Right now, the middleware doesn't apply credentials that were
_originally_ sourced from a URL. This requires that we call
`with_url_encoded_auth` whenever we create a request to ensure that any
credentials that were passed in as part of an index URL (for example)
are respected.

This PR modifies `uv-auth` to instead apply those credentials in the
middleware itself. This seems preferable to me. As far as I can tell, we
can _only_ add in-URL credentials to the store ourselves (since in-URL
credentials are converted to headers by the time they reach the
middleware). And if we ever _didn't_ apply those credentials to new
URLs, it'd be a bug in the logic that precedes the middleware (i.e., us
forgetting to call `with_url_encoded_auth`).

## Test Plan

`cargo run pip install` with an authenticated index.
This commit is contained in:
Charlie Marsh 2024-03-15 09:21:37 -07:00 committed by GitHub
parent 10abeae3c6
commit 8463d6d672
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 17 additions and 62 deletions

View file

@ -3,11 +3,10 @@ use std::path::PathBuf;
use serde::{Deserialize, Serialize};
use thiserror::Error;
use url::Url;
use pep440_rs::{VersionSpecifiers, VersionSpecifiersParseError};
use pypi_types::{DistInfoMetadata, Hashes, Yanked};
use url::Url;
use uv_auth::GLOBAL_AUTH_STORE;
/// Error converting [`pypi_types::File`] to [`distribution_type::File`].
#[derive(Debug, Error)]
@ -53,13 +52,8 @@ impl File {
size: file.size,
upload_time_utc_ms: file.upload_time.map(|dt| dt.timestamp_millis()),
url: if file.url.contains("://") {
// Copy over any credentials from the global store.
let url = Url::parse(&file.url)
.map_err(|err| FileConversionError::Url(file.url.clone(), err))?;
let url = GLOBAL_AUTH_STORE.with_url_encoded_auth(url);
FileLocation::AbsoluteUrl(url.to_string())
FileLocation::AbsoluteUrl(file.url)
} else {
// It's assumed that the base URL already contains any necessary credentials.
FileLocation::RelativeUrl(base.to_string(), file.url)
},
yanked: file.yanked,

View file

@ -1,7 +1,8 @@
use std::path::Path;
use netrc::Netrc;
use reqwest::{header::HeaderValue, Request, Response};
use reqwest_middleware::{Middleware, Next};
use std::path::Path;
use task_local_extensions::Extensions;
use tracing::{debug, warn};
@ -57,16 +58,10 @@ impl Middleware for AuthMiddleware {
// If we've already seen this URL, we can use the stored credentials
if let Some(auth) = stored_auth {
debug!("Adding authentication to already-seen URL: {url}");
match auth {
Credential::Basic(_) => {
req.headers_mut().insert(
reqwest::header::AUTHORIZATION,
basic_auth(auth.username(), auth.password()),
);
}
// Url must already have auth if before middleware runs - see `AuthenticationStore::with_url_encoded_auth`
Credential::UrlEncoded(_) => (),
}
req.headers_mut().insert(
reqwest::header::AUTHORIZATION,
basic_auth(auth.username(), auth.password()),
);
} else {
debug!("No credentials found for already-seen URL: {url}");
}
@ -137,14 +132,16 @@ where
#[cfg(test)]
mod tests {
use super::*;
use std::io::Write;
use reqwest::Client;
use reqwest_middleware::ClientBuilder;
use std::io::Write;
use tempfile::NamedTempFile;
use wiremock::matchers::{basic_auth, method, path};
use wiremock::{Mock, MockServer, ResponseTemplate};
use super::*;
const NETRC: &str = r#"default login myuser password mypassword"#;
#[tokio::test]

View file

@ -91,17 +91,6 @@ impl AuthenticationStore {
credentials.insert(netloc, auth);
}
/// Copy authentication from one URL to another URL if applicable.
pub fn with_url_encoded_auth(&self, url: Url) -> Url {
let netloc = NetLoc::from(&url);
let credentials = self.credentials.lock().unwrap();
if let Some(Some(Credential::UrlEncoded(url_auth))) = credentials.get(&netloc) {
url_auth.apply_to_url(url)
} else {
url
}
}
/// Store in-URL credentials for future use.
pub fn save_from_url(&self, url: &Url) {
let netloc = NetLoc::from(url);
@ -155,28 +144,6 @@ mod test {
assert!(not_found_res.is_none());
}
#[test]
fn store_with_url_encoded_auth() {
let store = AuthenticationStore::new();
let url = Url::parse("https://example.com/simple/").unwrap();
let auth = Credential::UrlEncoded(UrlAuthData {
username: "u".to_string(),
password: Some("p".to_string()),
});
// Before adding to the store there's no change
let url = store.with_url_encoded_auth(url);
assert_eq!(url.username(), "");
assert_eq!(url.password(), None);
store.set(&url, Some(auth.clone()));
// After adding to the store, the url is updated
let url = store.with_url_encoded_auth(url);
assert_eq!(url.username(), "u");
assert_eq!(url.password(), Some("p"));
}
#[test]
fn store_save_from_url() {
let store = AuthenticationStore::new();

View file

@ -17,7 +17,6 @@ use pep440_rs::Version;
use pep508_rs::VerbatimUrl;
use platform_tags::Tags;
use pypi_types::Hashes;
use uv_auth::GLOBAL_AUTH_STORE;
use uv_cache::{Cache, CacheBucket};
use uv_normalize::PackageName;
@ -157,17 +156,16 @@ impl<'a> FlatIndexClient<'a> {
async {
// Use the response URL, rather than the request URL, as the base for relative URLs.
// This ensures that we handle redirects and other URL transformations correctly.
let url = GLOBAL_AUTH_STORE.with_url_encoded_auth(response.url().clone());
let url = response.url().clone();
let text = response.text().await.map_err(ErrorKind::from)?;
let SimpleHtml { base, files } = SimpleHtml::parse(&text, &url)
.map_err(|err| Error::from_html_err(err, url.clone()))?;
let base = GLOBAL_AUTH_STORE.with_url_encoded_auth(base.into_url());
let files: Vec<File> = files
.into_iter()
.filter_map(|file| {
match File::try_from(file, &base) {
match File::try_from(file, base.as_url()) {
Ok(file) => Some(file),
Err(err) => {
// Ignore files with unparsable version specifiers.

View file

@ -21,7 +21,7 @@ use distribution_types::{BuiltDist, File, FileLocation, IndexUrl, IndexUrls, Nam
use install_wheel_rs::metadata::{find_archive_dist_info, is_metadata_entry};
use pep440_rs::Version;
use pypi_types::{Metadata23, SimpleJson};
use uv_auth::{AuthMiddleware, KeyringProvider, GLOBAL_AUTH_STORE};
use uv_auth::{AuthMiddleware, KeyringProvider};
use uv_cache::{Cache, CacheBucket, WheelCache};
use uv_fs::Simplified;
use uv_normalize::PackageName;
@ -317,7 +317,7 @@ impl RegistryClient {
async {
// Use the response URL, rather than the request URL, as the base for relative URLs.
// This ensures that we handle redirects and other URL transformations correctly.
let url = GLOBAL_AUTH_STORE.with_url_encoded_auth(response.url().clone());
let url = response.url().clone();
let content_type = response
.headers()
@ -346,9 +346,8 @@ impl RegistryClient {
let text = response.text().await.map_err(ErrorKind::from)?;
let SimpleHtml { base, files } = SimpleHtml::parse(&text, &url)
.map_err(|err| Error::from_html_err(err, url.clone()))?;
let base = GLOBAL_AUTH_STORE.with_url_encoded_auth(base.into_url());
SimpleMetadata::from_files(files, package_name, &base)
SimpleMetadata::from_files(files, package_name, base.as_url())
}
};
OwnedArchive::from_unarchived(&unarchived)