Make uv’s first-index strategy more secure by default by failing early on authentication failure #12805
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,13 +7,13 @@ use std::time::Duration; | |
|
|
||
| use async_http_range_reader::AsyncHttpRangeReader; | ||
| use futures::{FutureExt, StreamExt, TryStreamExt}; | ||
| use http::{HeaderMap, StatusCode}; | ||
| use itertools::Either; | ||
| use reqwest::{Proxy, Response}; | ||
| use reqwest_middleware::ClientWithMiddleware; | ||
| use rustc_hash::FxHashMap; | ||
| use tokio::sync::{Mutex, Semaphore}; | ||
| use tracing::{debug, info_span, instrument, trace, warn, Instrument}; | ||
| use url::Url; | ||
|
|
||
| use uv_auth::Indexes; | ||
|
|
@@ -23,7 +23,7 @@ use uv_configuration::{IndexStrategy, TrustedHost}; | |
| use uv_distribution_filename::{DistFilename, SourceDistFilename, WheelFilename}; | ||
| use uv_distribution_types::{ | ||
| BuiltDist, File, FileLocation, IndexCapabilities, IndexFormat, IndexLocations, | ||
| IndexMetadataRef, IndexStatusCodeDecision, IndexStatusCodeStrategy, IndexUrl, IndexUrls, Name, | ||
| }; | ||
| use uv_metadata::{read_metadata_async_seek, read_metadata_async_stream}; | ||
| use uv_normalize::PackageName; | ||
|
|
@@ -331,12 +331,29 @@ impl RegistryClient { | |
| let _permit = download_concurrency.acquire().await; | ||
| match index.format { | ||
| IndexFormat::Simple => { | ||
| let status_code_strategy = | ||
| self.index_urls.status_code_strategy_for(index.url); | ||
| match self | ||
| .simple_single_index( | ||
| package_name, | ||
| index.url, | ||
| capabilities, | ||
| &status_code_strategy, | ||
| ) | ||
| .await? | ||
| { | ||
| SimpleMetadataSearchOutcome::Found(metadata) => { | ||
| results.push((index.url, MetadataFormat::Simple(metadata))); | ||
| break; | ||
| } | ||
| // Package not found, so we will continue on to the next index (if there is one) | ||
| SimpleMetadataSearchOutcome::NotFound => {} | ||
| // The search failed because of an HTTP status code that we don't ignore for | ||
| // this index. We end our search here. | ||
|
There was a problem hiding this comment. I think this should mention the assumption that the status code failure will be tracked by |
||
| SimpleMetadataSearchOutcome::StatusCodeFailure(status_code) => { | ||
| debug!("Indexes search failed because of status code failure: {status_code}"); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| IndexFormat::Flat => { | ||
|
|
@@ -357,9 +374,21 @@ impl RegistryClient { | |
| let _permit = download_concurrency.acquire().await; | ||
| match index.format { | ||
| IndexFormat::Simple => { | ||
| // For unsafe matches, ignore authentication failures. | ||
| let status_code_strategy = | ||
| IndexStatusCodeStrategy::ignore_authentication_error_codes(); | ||
|
There was a problem hiding this comment. Why is this not There was a problem hiding this comment. I've added a comment. For unsafe index strategies, we ignore authentication failures and just find whatever matches we can across the indexes. |
||
| let metadata = match self | ||
| .simple_single_index( | ||
| package_name, | ||
| index.url, | ||
| capabilities, | ||
| &status_code_strategy, | ||
| ) | ||
| .await? | ||
| { | ||
| SimpleMetadataSearchOutcome::Found(metadata) => Some(metadata), | ||
| _ => None, | ||
| }; | ||
| Ok((index.url, metadata.map(MetadataFormat::Simple))) | ||
| } | ||
| IndexFormat::Flat => { | ||
|
|
@@ -438,14 +467,13 @@ impl RegistryClient { | |
| /// | ||
| /// The index can either be a PEP 503-compatible remote repository, or a local directory laid | ||
| /// out in the same format. | ||
| async fn simple_single_index( | ||
| &self, | ||
| package_name: &PackageName, | ||
| index: &IndexUrl, | ||
| capabilities: &IndexCapabilities, | ||
| status_code_strategy: &IndexStatusCodeStrategy, | ||
| ) -> Result<SimpleMetadataSearchOutcome, Error> { | ||
| // Format the URL for PyPI. | ||
| let mut url = index.url().clone(); | ||
| url.path_segments_mut() | ||
|
|
@@ -487,27 +515,31 @@ impl RegistryClient { | |
| }; | ||
|
|
||
| match result { | ||
| Ok(metadata) => Ok(SimpleMetadataSearchOutcome::Found(metadata)), | ||
| Err(err) => match err.into_kind() { | ||
| // The package could not be found in the remote index. | ||
| ErrorKind::WrappedReqwestError(url, err) => { | ||
|
There was a problem hiding this comment. Do we now lose this error? Does it ever get surfaced to the user? There was a problem hiding this comment. It no longer gets surfaced to the user if it's an error with a response status code. That's because the new interface gives the user the ability to ignore any status code they want. But now I've updated it so that if the strategy check fails on anything other than a 401 or 403, we just return the original error. This still provides the flexibility but preserves the old error information. There was a problem hiding this comment. I've also added a regression test for this case. There was a problem hiding this comment. I'm still struggling a bit. Why do we break when (e.g.) we hit a 401, but 401 isn't included in the list of ignored errors? Why is it not something like this? diff --git a/crates/uv-client/src/registry_client.rs b/crates/uv-client/src/registry_client.rs
index 8bdf2c065..3cb6b4978 100644
--- a/crates/uv-client/src/registry_client.rs
+++ b/crates/uv-client/src/registry_client.rs
@@ -348,12 +348,6 @@ impl RegistryClient {
}
// Package not found, so we will continue on to the next index (if there is one)
SimpleMetadataSearchOutcome::NotFound => {}
- // The search failed because of an HTTP status code that we don't ignore for
- // this index. We end our search here.
- SimpleMetadataSearchOutcome::StatusCodeFailure(status_code) => {
- debug!("Indexes search failed because of status code failure: {status_code}");
- break;
- }
}
}
IndexFormat::Flat => {
@@ -522,17 +516,15 @@ impl RegistryClient {
let Some(status_code) = err.status() else {
return Err(ErrorKind::WrappedReqwestError(url, err).into());
};
- let decision =
- status_code_strategy.handle_status_code(status_code, index, capabilities);
- if let IndexStatusCodeDecision::Fail(status_code) = decision {
- if !matches!(
- status_code,
- StatusCode::UNAUTHORIZED | StatusCode::FORBIDDEN
- ) {
- return Err(ErrorKind::WrappedReqwestError(url, err).into());
+ match status_code_strategy.handle_status_code(status_code, index, capabilities)
+ {
+ IndexStatusCodeDecision::Ignore => {
+ Ok(SimpleMetadataSearchOutcome::NotFound)
+ }
+ IndexStatusCodeDecision::Fail(..) => {
+ Err(ErrorKind::WrappedReqwestError(url, err).into())
}
}
- Ok(SimpleMetadataSearchOutcome::from(decision))
}
// The package is unavailable due to a lack of connectivity.
@@ -998,18 +990,6 @@ pub(crate) enum SimpleMetadataSearchOutcome {
Found(OwnedArchive<SimpleMetadata>),
/// Simple metadata was not found
NotFound,
- /// A status code failure was encountered when searching for
- /// simple metadata and our strategy did not ignore it
- StatusCodeFailure(StatusCode),
-}
-
-impl From<IndexStatusCodeDecision> for SimpleMetadataSearchOutcome {
- fn from(item: IndexStatusCodeDecision) -> Self {
- match item {
- IndexStatusCodeDecision::Ignore => Self::NotFound,
- IndexStatusCodeDecision::Fail(status_code) => Self::StatusCodeFailure(status_code),
- }
- }
}
/// A map from [`IndexUrl`] to [`FlatIndexEntry`] entries found at the given URL, indexed byThere was a problem hiding this comment. Because if we do it that way, we lose the richer error messages from the resolver. I actually originally did it the way you suggest, but @zanieb pointed out that there were regressions in the amount of information the user was getting. There was a problem hiding this comment. This is sort of like the original behavior of returning |
||
| let Some(status_code) = err.status() else { | ||
| return Err(ErrorKind::WrappedReqwestError(url, err).into()); | ||
| }; | ||
| let decision = | ||
| status_code_strategy.handle_status_code(status_code, index, capabilities); | ||
| if let IndexStatusCodeDecision::Fail(status_code) = decision { | ||
| if !matches!( | ||
| status_code, | ||
| StatusCode::UNAUTHORIZED | StatusCode::FORBIDDEN | ||
|
There was a problem hiding this comment. I think this should mention that these specific codes are tracked via |
||
| ) { | ||
| return Err(ErrorKind::WrappedReqwestError(url, err).into()); | ||
| } | ||
| } | ||
| Ok(SimpleMetadataSearchOutcome::from(decision)) | ||
| } | ||
|
|
||
| // The package is unavailable due to a lack of connectivity. | ||
| ErrorKind::Offline(_) => Ok(SimpleMetadataSearchOutcome::NotFound), | ||
|
|
||
| // The package could not be found in the local index. | ||
| ErrorKind::FileNotFound(_) => Ok(SimpleMetadataSearchOutcome::NotFound), | ||
|
|
||
| err => Err(err.into()), | ||
| }, | ||
|
|
@@ -960,6 +992,26 @@ impl RegistryClient { | |
| } | ||
| } | ||
|
|
||
| #[derive(Debug)] | ||
| pub(crate) enum SimpleMetadataSearchOutcome { | ||
| /// Simple metadata was found | ||
| Found(OwnedArchive<SimpleMetadata>), | ||
jtfmumm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| /// Simple metadata was not found | ||
| NotFound, | ||
| /// A status code failure was encountered when searching for | ||
| /// simple metadata and our strategy did not ignore it | ||
| StatusCodeFailure(StatusCode), | ||
|
There was a problem hiding this comment. I think I would find it clearer if we actually just made individual enum variants for Unauthorized and Forbidden, rather than allowing an arbitrary status code here. That way the expectations are encoded in the type system. We should make the invalid state (e.g., |
||
| } | ||
|
|
||
| impl From<IndexStatusCodeDecision> for SimpleMetadataSearchOutcome { | ||
| fn from(item: IndexStatusCodeDecision) -> Self { | ||
| match item { | ||
| IndexStatusCodeDecision::Ignore => Self::NotFound, | ||
| IndexStatusCodeDecision::Fail(status_code) => Self::StatusCodeFailure(status_code), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// A map from [`IndexUrl`] to [`FlatIndexEntry`] entries found at the given URL, indexed by | ||
| /// [`PackageName`]. | ||
| #[derive(Default, Debug, Clone)] | ||
|
|
||
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
Rustdoc please for the variants.
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.
Added