Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 1 addition & 16 deletions crates/uv-auth/src/credentials.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use reqsign::aws::DefaultSigner as AwsDefaultSigner;
use reqsign::azure::DefaultSigner as AzureDefaultSigner;
use reqsign::google::DefaultSigner as GcsDefaultSigner;
use reqwest::Request;
use reqwest::header::{HeaderName, HeaderValue, InvalidHeaderValue};
use reqwest::header::{HeaderValue, InvalidHeaderValue};
use serde::{Deserialize, Serialize};
use thiserror::Error;
use url::Url;
Expand All @@ -21,8 +21,6 @@ use uv_netrc::Netrc;
use uv_redacted::DisplaySafeUrl;
use uv_static::EnvVars;

const AZURE_STORAGE_VERSION: &str = "2023-11-03";

#[derive(Clone, Debug, PartialEq, Eq)]
pub enum Credentials {
/// RFC 7617 HTTP Basic Authentication
Expand Down Expand Up @@ -602,10 +600,6 @@ impl Authentication {
source,
})?;
*http_req.headers_mut() = request.headers().clone();
http_req
.headers_mut()
.entry(HeaderName::from_static("x-ms-version"))
.or_insert(HeaderValue::from_static(AZURE_STORAGE_VERSION));

// Sign the parts.
let (mut parts, ()) = http_req.into_parts();
Expand Down Expand Up @@ -823,15 +817,6 @@ mod tests {
.expect("Authorization header should be set");
assert_eq!(authorization.to_str().unwrap(), "Bearer token");
assert!(request.headers().contains_key("x-ms-date"));
assert_eq!(
request
.headers()
.get("x-ms-version")
.expect("x-ms-version header should be set")
.to_str()
.unwrap(),
AZURE_STORAGE_VERSION
);
}

#[tokio::test]
Expand Down
1 change: 1 addition & 0 deletions crates/uv-auth/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ pub use credentials::{Credentials, CredentialsFromUrlError, Username};
pub use index::{AuthPolicy, Index, Indexes};
pub use keyring::KeyringProvider;
pub use middleware::AuthMiddleware;
pub use providers::AzureEndpointProvider;
pub use realm::{Realm, RealmRef};
pub use service::Service;
pub use store::{AuthBackend, TextCredentialStore, TomlCredentialError};
Expand Down
85 changes: 7 additions & 78 deletions crates/uv-auth/src/middleware.rs
Original file line number Diff line number Diff line change
Expand Up @@ -326,8 +326,7 @@ impl Middleware for AuthMiddleware {
///
/// - Check the cache (URL key)
/// - Perform the request
/// - On 401, 403, 404, or an Azure public-access denial, check for authentication if there was
/// a cache miss
/// - On 401, 403, or 404 check for authentication if there was a cache miss
/// - Check the cache (index URL or realm key) for the username and password
/// - Check the netrc for a username and password
/// - Perform the request again if found
Expand Down Expand Up @@ -420,11 +419,12 @@ impl Middleware for AuthMiddleware {

let response = next.clone().run(request, extensions).await?;

// If we don't fail with authorization related codes or authentication policy is
// Never, return the response.
if !response_requires_authentication(&response, retry_request.url(), self.preview)
.map_err(Error::Middleware)?
|| matches!(auth_policy, AuthPolicy::Never)
// If we don't fail with authorization related codes or
// authentication policy is Never, return the response.
if !matches!(
response.status(),
StatusCode::FORBIDDEN | StatusCode::NOT_FOUND | StatusCode::UNAUTHORIZED
) || matches!(auth_policy, AuthPolicy::Never)
Comment on lines +424 to +427

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Retain 409 discovery for explicit older Azure versions

When a caller explicitly supplies an x-ms-version older than 2019-12-12, the new middleware intentionally preserves it via or_insert, so a private Azure blob can still answer the anonymous request with 409 PublicAccessNotPermitted. This status check now returns that response immediately instead of discovering Azure credentials, even though the removed response_requires_authentication path handled precisely this case; retain the targeted 409 fallback for requests whose explicit version is not upgraded.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably don't need to handle this

{
return Ok(response);
}
Expand Down Expand Up @@ -876,32 +876,6 @@ impl AuthMiddleware {
}
}

/// Returns `true` when an unauthenticated response should trigger credential discovery.
fn response_requires_authentication(
response: &Response,
url: &url::Url,
preview: Preview,
) -> anyhow::Result<bool> {
if matches!(
response.status(),
StatusCode::FORBIDDEN | StatusCode::NOT_FOUND | StatusCode::UNAUTHORIZED
) {
return Ok(true);
}

if response.status() != StatusCode::CONFLICT {
return Ok(false);
}

// Azure Storage reports an anonymous request to a private endpoint as a conflict instead of an
// authorization error.
Ok(AzureEndpointProvider::is_azure_endpoint(url, preview)?
&& response
.headers()
.get("x-ms-error-code")
.is_some_and(|value| value == "PublicAccessNotPermitted"))
}

fn tracing_url(request: &Request, credentials: Option<&Authentication>) -> DisplaySafeUrl {
let mut url = DisplaySafeUrl::from_url(request.url().clone());
if let Some(Authentication::Credentials(creds)) = credentials {
Expand Down Expand Up @@ -936,51 +910,6 @@ mod tests {

type Error = Box<dyn std::error::Error>;

#[test]
fn test_response_requires_authentication() -> Result<(), Error> {
let url = Url::parse("https://example.com")?;

let response = Response::from(
http::Response::builder()
.status(401)
.body(Vec::new())
.unwrap(),
);
assert!(response_requires_authentication(
&response,
&url,
Preview::default()
)?);

let response = Response::from(
http::Response::builder()
.status(409)
.header("x-ms-error-code", "PublicAccessNotPermitted")
.body(Vec::new())
.unwrap(),
);
assert!(!response_requires_authentication(
&response,
&url,
Preview::default()
)?);

let response = Response::from(
http::Response::builder()
.status(409)
.header("x-ms-error-code", "ContainerBeingDeleted")
.body(Vec::new())
.unwrap(),
);
assert!(!response_requires_authentication(
&response,
&url,
Preview::default()
)?);

Ok(())
}

async fn start_test_server(username: &'static str, password: &'static str) -> MockServer {
let server = MockServer::start().await;

Expand Down
4 changes: 2 additions & 2 deletions crates/uv-auth/src/providers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,11 +155,11 @@ static AZURE_ENDPOINT_URL: LazyLock<Result<Option<Url>, ParseError>> =

/// A provider for authentication credentials for Azure endpoints.
#[derive(Debug, Clone, PartialEq, Eq)]
pub(crate) struct AzureEndpointProvider;
pub struct AzureEndpointProvider;

impl AzureEndpointProvider {
/// Returns `true` if the URL matches the configured Azure endpoint.
pub(crate) fn is_azure_endpoint(url: &Url, preview: Preview) -> Result<bool> {
pub fn is_azure_endpoint(url: &Url, preview: Preview) -> Result<bool> {
if let Some(azure_endpoint_url) = AZURE_ENDPOINT_URL
.as_ref()
.map_err(|error| *error)
Expand Down
6 changes: 5 additions & 1 deletion crates/uv-client/src/base_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ use uv_version::version;
use uv_warnings::warn_user_once;

use crate::linehaul::LineHaul;
use crate::middleware::OfflineMiddleware;
use crate::middleware::{AzureStorageMiddleware, OfflineMiddleware};
use crate::tls::{Certificates, read_identity};
use crate::{Connectivity, RetriableError, RetryState, UvRetryableStrategy};

Expand Down Expand Up @@ -667,6 +667,10 @@ impl<'a> BaseClientBuilder<'a> {
}
}

client = client.with(AzureStorageMiddleware {
preview: self.preview,
});

// Initialize the authentication middleware to set headers.
match self.auth_integration {
AuthIntegration::Default => {
Expand Down
33 changes: 33 additions & 0 deletions crates/uv-client/src/middleware.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,43 @@
use http::Extensions;
use std::fmt::Debug;
use uv_auth::AzureEndpointProvider;
use uv_preview::Preview;
use uv_redacted::DisplaySafeUrl;

use reqwest::header::HeaderValue;
use reqwest::{Request, Response};
use reqwest_middleware::{Middleware, Next};

pub(crate) struct AzureStorageMiddleware {
pub(crate) preview: Preview,
}

#[async_trait::async_trait]
impl Middleware for AzureStorageMiddleware {
async fn handle(
&self,
mut request: Request,
extensions: &mut Extensions,
next: Next<'_>,
) -> reqwest_middleware::Result<Response> {
if AzureEndpointProvider::is_azure_endpoint(request.url(), self.preview)? {
// Anonymous requests need 2019-12-12 or later to return a 401
// bearer challenge instead of 409 when account-level public access
// is disabled. Use the earliest such version supported by Azure
// Stack Hub (2301 and later).
//
// https://learn.microsoft.com/en-us/rest/api/storageservices/authorize-with-azure-active-directory#bearer-challenge
// https://learn.microsoft.com/en-us/azure-stack/user/azure-stack-acs-differences#api-version
request
.headers_mut()
.entry("x-ms-version")
.or_insert(HeaderValue::from_static("2020-10-02"));
Comment on lines +31 to +34

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Strip the Azure version on built-in redirects

When a BaseClient using the default RedirectPolicy::BypassMiddleware follows a redirect from the configured Azure endpoint to a URL outside that endpoint, reqwest preserves ordinary headers such as x-ms-version while bypassing this middleware, so the destination receives an Azure-specific header even though it does not satisfy is_azure_endpoint. Unlike the previously discussed RetriggerMiddleware case, the fresh evidence here is that BypassMiddleware is the default in base_client.rs and delegates redirects directly to reqwest; remove or recompute this header when a built-in redirect leaves the configured endpoint.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this was previously dismissed?

}

next.run(request, extensions).await
}
}

/// A custom error type for the offline middleware.
#[derive(Debug, Clone, PartialEq, Eq)]
pub(crate) struct OfflineError {
Expand Down
Loading