From b2915e58af30ec70bea47f8d8077abc1c3d27304 Mon Sep 17 00:00:00 2001 From: naosense Date: Wed, 17 Apr 2024 14:12:24 +0800 Subject: [PATCH 1/7] Make KeyringProvider::fetch_* async --- crates/uv-auth/Cargo.toml | 1 + crates/uv-auth/src/keyring.rs | 64 ++++++++++++++++---------------- crates/uv-auth/src/middleware.rs | 1 + 3 files changed, 35 insertions(+), 31 deletions(-) diff --git a/crates/uv-auth/Cargo.toml b/crates/uv-auth/Cargo.toml index d6ec437016ab..6325389ad35c 100644 --- a/crates/uv-auth/Cargo.toml +++ b/crates/uv-auth/Cargo.toml @@ -14,6 +14,7 @@ rust-netrc = { workspace = true } serde = { workspace = true, optional = true } thiserror = { workspace = true } tracing = { workspace = true } +tokio = { workspace = true } url = { workspace = true } urlencoding = { workspace = true } diff --git a/crates/uv-auth/src/keyring.rs b/crates/uv-auth/src/keyring.rs index 7f3f0ad2e145..28470ab102cf 100644 --- a/crates/uv-auth/src/keyring.rs +++ b/crates/uv-auth/src/keyring.rs @@ -1,5 +1,6 @@ -use std::{collections::HashSet, process::Command, sync::Mutex}; +use std::{collections::HashSet, sync::Mutex}; +use tokio::process::Command; use tracing::{debug, instrument, warn}; use url::Url; @@ -37,7 +38,7 @@ impl KeyringProvider { /// /// Returns [`None`] if no password was found for the username or if any errors /// are encountered in the keyring backend. - pub(crate) fn fetch(&self, url: &Url, username: &str) -> Option { + pub(crate) async fn fetch(&self, url: &Url, username: &str) -> Option { // Validate the request debug_assert!( url.host_str().is_some(), @@ -74,7 +75,7 @@ impl KeyringProvider { // Check the full URL first // let mut password = match self.backend { - KeyringProviderBackend::Subprocess => self.fetch_subprocess(url.as_str(), username), + KeyringProviderBackend::Subprocess => self.fetch_subprocess(url.as_str(), username).await, #[cfg(test)] KeyringProviderBackend::Dummy(ref store) => { self.fetch_dummy(store, url.as_str(), username) @@ -83,7 +84,7 @@ impl KeyringProvider { // And fallback to a check for the host if password.is_none() { password = match self.backend { - KeyringProviderBackend::Subprocess => self.fetch_subprocess(host, username), + KeyringProviderBackend::Subprocess => self.fetch_subprocess(host, username).await, #[cfg(test)] KeyringProviderBackend::Dummy(ref store) => self.fetch_dummy(store, host, username), }; @@ -97,12 +98,13 @@ impl KeyringProvider { } #[instrument] - fn fetch_subprocess(&self, service_name: &str, username: &str) -> Option { + async fn fetch_subprocess(&self, service_name: &str, username: &str) -> Option { let output = Command::new("keyring") .arg("get") .arg(service_name) .arg(username) .output() + .await .inspect_err(|err| warn!("Failure running `keyring` command: {err}")) .ok()?; @@ -189,27 +191,27 @@ mod test { assert!(result.is_err()); } - #[test] - fn fetch_url_no_auth() { + #[tokio::test] + async fn fetch_url_no_auth() { let url = Url::parse("https://example.com").unwrap(); let keyring = KeyringProvider::empty(); let credentials = keyring.fetch(&url, "user"); - assert!(credentials.is_none()); + assert!(credentials.await.is_none()); } - #[test] - fn fetch_url() { + #[tokio::test] + async fn fetch_url() { let url = Url::parse("https://example.com").unwrap(); let keyring = KeyringProvider::dummy([((url.host_str().unwrap(), "user"), "password")]); assert_eq!( - keyring.fetch(&url, "user"), + keyring.fetch(&url, "user").await, Some(Credentials::new( Some("user".to_string()), Some("password".to_string()) )) ); assert_eq!( - keyring.fetch(&url.join("test").unwrap(), "user"), + keyring.fetch(&url.join("test").unwrap(), "user").await, Some(Credentials::new( Some("user".to_string()), Some("password".to_string()) @@ -217,37 +219,37 @@ mod test { ); } - #[test] - fn fetch_url_no_match() { + #[tokio::test] + async fn fetch_url_no_match() { let url = Url::parse("https://example.com").unwrap(); let keyring = KeyringProvider::dummy([(("other.com", "user"), "password")]); - let credentials = keyring.fetch(&url, "user"); + let credentials = keyring.fetch(&url, "user").await; assert_eq!(credentials, None); } - #[test] - fn fetch_url_prefers_url_to_host() { + #[tokio::test] + async fn fetch_url_prefers_url_to_host() { let url = Url::parse("https://example.com/").unwrap(); let keyring = KeyringProvider::dummy([ ((url.join("foo").unwrap().as_str(), "user"), "password"), ((url.host_str().unwrap(), "user"), "other-password"), ]); assert_eq!( - keyring.fetch(&url.join("foo").unwrap(), "user"), + keyring.fetch(&url.join("foo").unwrap(), "user").await, Some(Credentials::new( Some("user".to_string()), Some("password".to_string()) )) ); assert_eq!( - keyring.fetch(&url, "user"), + keyring.fetch(&url, "user").await, Some(Credentials::new( Some("user".to_string()), Some("other-password".to_string()) )) ); assert_eq!( - keyring.fetch(&url.join("bar").unwrap(), "user"), + keyring.fetch(&url.join("bar").unwrap(), "user").await, Some(Credentials::new( Some("user".to_string()), Some("other-password".to_string()) @@ -258,24 +260,24 @@ mod test { /// Demonstrates "incorrect" behavior in our cache which avoids an expensive fetch of /// credentials for _every_ request URL at the cost of inconsistent behavior when /// credentials are not scoped to a realm. - #[test] - fn fetch_url_caches_based_on_host() { + #[tokio::test] + async fn fetch_url_caches_based_on_host() { let url = Url::parse("https://example.com/").unwrap(); let keyring = KeyringProvider::dummy([((url.join("foo").unwrap().as_str(), "user"), "password")]); // If we attempt an unmatching URL first... - assert_eq!(keyring.fetch(&url.join("bar").unwrap(), "user"), None); + assert_eq!(keyring.fetch(&url.join("bar").unwrap(), "user").await, None); // ... we will cache the missing credentials on subsequent attempts - assert_eq!(keyring.fetch(&url.join("foo").unwrap(), "user"), None); + assert_eq!(keyring.fetch(&url.join("foo").unwrap(), "user").await, None); } - #[test] - fn fetch_url_username() { + #[tokio::test] + async fn fetch_url_username() { let url = Url::parse("https://example.com").unwrap(); let keyring = KeyringProvider::dummy([((url.host_str().unwrap(), "user"), "password")]); - let credentials = keyring.fetch(&url, "user"); + let credentials = keyring.fetch(&url, "user").await; assert_eq!( credentials, Some(Credentials::new( @@ -285,16 +287,16 @@ mod test { ); } - #[test] - fn fetch_url_username_no_match() { + #[tokio::test] + async fn fetch_url_username_no_match() { let url = Url::parse("https://example.com").unwrap(); let keyring = KeyringProvider::dummy([((url.host_str().unwrap(), "foo"), "password")]); - let credentials = keyring.fetch(&url, "bar"); + let credentials = keyring.fetch(&url, "bar").await; assert_eq!(credentials, None); // Still fails if we have `foo` in the URL itself let url = Url::parse("https://foo@example.com").unwrap(); - let credentials = keyring.fetch(&url, "bar"); + let credentials = keyring.fetch(&url, "bar").await; assert_eq!(credentials, None); } } diff --git a/crates/uv-auth/src/middleware.rs b/crates/uv-auth/src/middleware.rs index f028d7dc9f78..191ca92e08a6 100644 --- a/crates/uv-auth/src/middleware.rs +++ b/crates/uv-auth/src/middleware.rs @@ -142,6 +142,7 @@ impl Middleware for AuthMiddleware { .and_then(|credentials| credentials.username()) { debug!("Checking keyring for credentials for {url}"); + // how to fix this keyring.fetch(request.url(), username) } else { trace!("Skipping keyring lookup for {url} with no username"); From 97b6b2ed6dc0a2943d5526795508df1af573cd22 Mon Sep 17 00:00:00 2001 From: naosense Date: Wed, 17 Apr 2024 14:38:01 +0800 Subject: [PATCH 2/7] format code --- crates/uv-auth/src/keyring.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/uv-auth/src/keyring.rs b/crates/uv-auth/src/keyring.rs index 28470ab102cf..3c66d89f7371 100644 --- a/crates/uv-auth/src/keyring.rs +++ b/crates/uv-auth/src/keyring.rs @@ -75,7 +75,9 @@ impl KeyringProvider { // Check the full URL first // let mut password = match self.backend { - KeyringProviderBackend::Subprocess => self.fetch_subprocess(url.as_str(), username).await, + KeyringProviderBackend::Subprocess => { + self.fetch_subprocess(url.as_str(), username).await + } #[cfg(test)] KeyringProviderBackend::Dummy(ref store) => { self.fetch_dummy(store, url.as_str(), username) From cf02b3e8d3a6f69bc31d00f53e899751b131e062 Mon Sep 17 00:00:00 2001 From: naosense Date: Thu, 18 Apr 2024 15:13:10 +0800 Subject: [PATCH 3/7] split cache lock to avoid across await --- crates/uv-auth/src/keyring.rs | 19 ++++++++++++------- crates/uv-auth/src/middleware.rs | 30 ++++++++++++++++++------------ 2 files changed, 30 insertions(+), 19 deletions(-) diff --git a/crates/uv-auth/src/keyring.rs b/crates/uv-auth/src/keyring.rs index 3c66d89f7371..0bd471dae3d3 100644 --- a/crates/uv-auth/src/keyring.rs +++ b/crates/uv-auth/src/keyring.rs @@ -62,14 +62,16 @@ impl KeyringProvider { // This behavior avoids adding ~80ms to every request when the subprocess keyring // provider is being used, but makes assumptions about the typical keyring // use-cases. - let mut cache = self.cache.lock().unwrap(); - let key = (host.to_string(), username.to_string()); - if cache.contains(&key) { - debug!( + { + let cache = self.cache.lock().unwrap(); + + if cache.contains(&key) { + debug!( "Skipping keyring lookup for {username} at {host}, already attempted and found no credentials." ); - return None; + return None; + } } // Check the full URL first @@ -92,8 +94,11 @@ impl KeyringProvider { }; } - if password.is_none() { - cache.insert(key); + { + let mut cache = self.cache.lock().unwrap(); + if password.is_none() { + cache.insert(key); + } } password.map(|password| Credentials::new(Some(username.to_string()), Some(password))) diff --git a/crates/uv-auth/src/middleware.rs b/crates/uv-auth/src/middleware.rs index 191ca92e08a6..451f690a704f 100644 --- a/crates/uv-auth/src/middleware.rs +++ b/crates/uv-auth/src/middleware.rs @@ -136,19 +136,25 @@ impl Middleware for AuthMiddleware { // falls back to the host, but we cache the result per host so if a keyring // implementation returns different credentials for different URLs in the // same realm we will use the wrong credentials. - } else if let Some(credentials) = self.keyring.as_ref().and_then(|keyring| { - if let Some(username) = credentials - .get() - .and_then(|credentials| credentials.username()) - { - debug!("Checking keyring for credentials for {url}"); - // how to fix this - keyring.fetch(request.url(), username) - } else { - trace!("Skipping keyring lookup for {url} with no username"); - None + } else if let Some(credentials) = match self.keyring { + Some(ref keyring) => { + match credentials + .get() + .and_then(|credentials| credentials.username()) + { + Some(username) => { + debug!("Checking keyring for credentials for {url}"); + // how to fix this + keyring.fetch(request.url(), username).await + } + None => { + trace!("Skipping keyring lookup for {url} with no username"); + None + } + } } - }) { + None => None, + } { debug!("Found credentials in keyring for {url}"); request = credentials.authenticate(request); new_credentials = Some(Arc::new(credentials)); From a7d745c2b7cb12a70199eb0fff259c4d5271caf4 Mon Sep 17 00:00:00 2001 From: naosense Date: Thu, 18 Apr 2024 16:05:03 +0800 Subject: [PATCH 4/7] fix test error --- Cargo.lock | 1 + crates/uv-auth/Cargo.toml | 1 + crates/uv-auth/src/keyring.rs | 25 ++++++++++++++++--------- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ddb5478f46cc..599bbf745f02 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4449,6 +4449,7 @@ version = "0.0.1" dependencies = [ "async-trait", "base64 0.22.0", + "futures", "http", "insta", "once_cell", diff --git a/crates/uv-auth/Cargo.toml b/crates/uv-auth/Cargo.toml index 6325389ad35c..031670d429bd 100644 --- a/crates/uv-auth/Cargo.toml +++ b/crates/uv-auth/Cargo.toml @@ -6,6 +6,7 @@ edition = "2021" [dependencies] async-trait = { workspace = true } base64 = { workspace = true } +futures = { workspace = true } http = { workspace = true } once_cell = { workspace = true } reqwest = { workspace = true } diff --git a/crates/uv-auth/src/keyring.rs b/crates/uv-auth/src/keyring.rs index 0bd471dae3d3..095abec98166 100644 --- a/crates/uv-auth/src/keyring.rs +++ b/crates/uv-auth/src/keyring.rs @@ -170,31 +170,38 @@ impl KeyringProvider { #[cfg(test)] mod test { use super::*; + use futures::FutureExt; - #[test] - fn fetch_url_no_host() { + #[tokio::test] + async fn fetch_url_no_host() { let url = Url::parse("file:/etc/bin/").unwrap(); let keyring = KeyringProvider::empty(); // Panics due to debug assertion; returns `None` in production - let result = std::panic::catch_unwind(|| keyring.fetch(&url, "user")); + let result = std::panic::AssertUnwindSafe(keyring.fetch(&url, "user")) + .catch_unwind() + .await; assert!(result.is_err()); } - #[test] - fn fetch_url_with_password() { + #[tokio::test] + async fn fetch_url_with_password() { let url = Url::parse("https://user:password@example.com").unwrap(); let keyring = KeyringProvider::empty(); // Panics due to debug assertion; returns `None` in production - let result = std::panic::catch_unwind(|| keyring.fetch(&url, url.username())); + let result = std::panic::AssertUnwindSafe(keyring.fetch(&url, url.username())) + .catch_unwind() + .await; assert!(result.is_err()); } - #[test] - fn fetch_url_with_no_username() { + #[tokio::test] + async fn fetch_url_with_no_username() { let url = Url::parse("https://example.com").unwrap(); let keyring = KeyringProvider::empty(); // Panics due to debug assertion; returns `None` in production - let result = std::panic::catch_unwind(|| keyring.fetch(&url, url.username())); + let result = std::panic::AssertUnwindSafe(keyring.fetch(&url, url.username())) + .catch_unwind() + .await; assert!(result.is_err()); } From 75f9a8e81f1ab2bfd8d6284716d548521f3cdec7 Mon Sep 17 00:00:00 2001 From: naosense Date: Tue, 23 Apr 2024 10:45:09 +0800 Subject: [PATCH 5/7] remove comment --- crates/uv-auth/src/middleware.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/uv-auth/src/middleware.rs b/crates/uv-auth/src/middleware.rs index 451f690a704f..6fc0406493ec 100644 --- a/crates/uv-auth/src/middleware.rs +++ b/crates/uv-auth/src/middleware.rs @@ -144,7 +144,6 @@ impl Middleware for AuthMiddleware { { Some(username) => { debug!("Checking keyring for credentials for {url}"); - // how to fix this keyring.fetch(request.url(), username).await } None => { From 06bec6547e0405044a54a7277709448af895bda9 Mon Sep 17 00:00:00 2001 From: naosense Date: Tue, 23 Apr 2024 10:54:39 +0800 Subject: [PATCH 6/7] sync remote code --- crates/uv-auth/src/middleware.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/uv-auth/src/middleware.rs b/crates/uv-auth/src/middleware.rs index 09fb4f317b91..311dc46042fb 100644 --- a/crates/uv-auth/src/middleware.rs +++ b/crates/uv-auth/src/middleware.rs @@ -321,7 +321,6 @@ impl AuthMiddleware { } else if let Some(credentials) = match self.keyring { Some(ref keyring) => { match credentials - .get() .and_then(|credentials| credentials.username()) { Some(username) => { From 533d7b673ea8d46942167cc0c9aa008cbdc18e82 Mon Sep 17 00:00:00 2001 From: naosense Date: Tue, 23 Apr 2024 14:02:32 +0800 Subject: [PATCH 7/7] fmt --- crates/uv-auth/src/middleware.rs | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/crates/uv-auth/src/middleware.rs b/crates/uv-auth/src/middleware.rs index 311dc46042fb..08f47eff27d9 100644 --- a/crates/uv-auth/src/middleware.rs +++ b/crates/uv-auth/src/middleware.rs @@ -319,20 +319,16 @@ impl AuthMiddleware { // implementation returns different credentials for different URLs in the // same realm we will use the wrong credentials. } else if let Some(credentials) = match self.keyring { - Some(ref keyring) => { - match credentials - .and_then(|credentials| credentials.username()) - { - Some(username) => { - debug!("Checking keyring for credentials for {username}@{url}"); - keyring.fetch(url, username).await - } - None => { - trace!("Skipping keyring lookup for {url} with no username"); - None - } + Some(ref keyring) => match credentials.and_then(|credentials| credentials.username()) { + Some(username) => { + debug!("Checking keyring for credentials for {username}@{url}"); + keyring.fetch(url, username).await } - } + None => { + trace!("Skipping keyring lookup for {url} with no username"); + None + } + }, None => None, } { debug!("Found credentials in keyring for {url}");