Skip to content
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.

Sign up for GitHub

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

Use keyring --mode creds when authenticate = "always" #12316

Merged
merged 2 commits into from
Mar 19, 2025
Merged

Use keyring --mode creds when authenticate = "always" #12316

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b8b75c4
Use `keyring --mode creds` when `authenticate = "always"`
zanieb Mar 19, 2025
d429f45
Review feedback; improve handling of missing passwords
zanieb Mar 19, 2025
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
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/uv-auth/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ workspace = true
uv-once-map = { workspace = true }
uv-small-str = { workspace = true }
uv-static = { workspace = true }
uv-warnings = { workspace = true }

anyhow = { workspace = true }
async-trait = { workspace = true }
Expand Down
167 changes: 118 additions & 49 deletions crates/uv-auth/src/keyring.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use std::process::Stdio;
use std::{io::Write, process::Stdio};
use tokio::process::Command;
use tracing::{instrument, trace, warn};
use url::Url;
use uv_warnings::warn_user_once;

use crate::credentials::Credentials;

Expand All @@ -19,7 +20,7 @@ pub(crate) enum KeyringProviderBackend {
/// Use the `keyring` command to fetch credentials.
Subprocess,
#[cfg(test)]
Dummy(std::collections::HashMap<(String, &'static str), &'static str>),
Dummy(Vec<(String, &'static str, &'static str)>),
}

impl KeyringProvider {
Expand All @@ -35,7 +36,7 @@ impl KeyringProvider {
/// Returns [`None`] if no password was found for the username or if any errors
/// are encountered in the keyring backend.
#[instrument(skip_all, fields(url = % url.to_string(), username))]
pub async fn fetch(&self, url: &Url, username: &str) -> Option<Credentials> {
pub async fn fetch(&self, url: &Url, username: Option<&str>) -> Option<Credentials> {
// Validate the request
debug_assert!(
url.host_str().is_some(),
Expand All @@ -46,14 +47,14 @@ impl KeyringProvider {
"Should only use keyring for urls without a password"
);
debug_assert!(
!username.is_empty(),
"Should only use keyring with a username"
!username.map(str::is_empty).unwrap_or(false),
"Should only use keyring with a non-empty username"
);

// Check the full URL first
// <https://github.com/pypa/pip/blob/ae5fff36b0aad6e5e0037884927eaa29163c0611/src/pip/_internal/network/auth.py#L376C1-L379C14>
trace!("Checking keyring for URL {url}");
let mut password = match self.backend {
let mut credentials = match self.backend {
KeyringProviderBackend::Subprocess => {
self.fetch_subprocess(url.as_str(), username).await
}
Expand All @@ -63,14 +64,14 @@ impl KeyringProvider {
}
};
// And fallback to a check for the host
if password.is_none() {
if credentials.is_none() {
let host = if let Some(port) = url.port() {
format!("{}:{}", url.host_str()?, port)
} else {
url.host_str()?.to_string()
};
trace!("Checking keyring for host {host}");
password = match self.backend {
credentials = match self.backend {
KeyringProviderBackend::Subprocess => self.fetch_subprocess(&host, username).await,
#[cfg(test)]
KeyringProviderBackend::Dummy(ref store) => {
Expand All @@ -79,19 +80,36 @@ impl KeyringProvider {
};
}

password.map(|password| Credentials::new(Some(username.to_string()), Some(password)))
credentials.map(|(username, password)| Credentials::new(Some(username), Some(password)))
}

#[instrument(skip(self))]
async fn fetch_subprocess(&self, service_name: &str, username: &str) -> Option<String> {
async fn fetch_subprocess(
&self,
service_name: &str,
username: Option<&str>,
) -> Option<(String, String)> {
// https://github.com/pypa/pip/blob/24.0/src/pip/_internal/network/auth.py#L136-L141
let child = Command::new("keyring")
.arg("get")
.arg(service_name)
.arg(username)
let mut command = Command::new("keyring");
command.arg("get").arg(service_name);

if let Some(username) = username {
command.arg(username);
} else {
command.arg("--mode").arg("creds");
}

let child = command
.stdin(Stdio::null())
.stdout(Stdio::piped())
.stderr(Stdio::inherit())
// If we're using `--mode creds`, we need to capture the output in order to avoid
// showing users an "unrecognized arguments: --mode" error; otherwise, we stream stderr
// so the user has visibility into keyring's behavior if it's doing something slow
.stderr(if username.is_some() {
Stdio::inherit()
} else {
Stdio::piped()
})
.spawn()
.inspect_err(|err| warn!("Failure running `keyring` command: {err}"))
.ok()?;
Expand All @@ -103,37 +121,74 @@ impl KeyringProvider {
.ok()?;

if output.status.success() {
// If we captured stderr, display it in case it's helpful to the user
// TODO(zanieb): This was done when we added `--mode creds` support for parity with the
// existing behavior, but it might be a better UX to hide this on success? It also
// might be problematic that we're not streaming it. We could change this given some
// user feedback.
std::io::stderr().write_all(&output.stderr).ok();

// On success, parse the newline terminated password
String::from_utf8(output.stdout)
let output = String::from_utf8(output.stdout)
.inspect_err(|err| warn!("Failed to parse response from `keyring` command: {err}"))
.ok()
.map(|password| password.trim_end().to_string())
.ok();

if let Some(username) = username {
// We're only expecting a password
output.map(|password| (username.to_string(), password.trim_end().to_string()))
} else {
// We're expecting a username and password
output.and_then(|output| {
let mut lines = output.lines();
lines.next().and_then(|username| {
lines
.next()
.map(|password| (username.to_string(), password.to_string()))
})
})
}
} else {
// On failure, no password was available
let stderr = std::str::from_utf8(&output.stderr).ok()?;
if stderr.contains("unrecognized arguments: --mode") {
// N.B. We do not show the `service_name` here because we'll show the warning twice
// otherwise, once for the URL and once for the realm.
warn_user_once!(
"Attempted to fetch credentials using the `keyring` command, but it does not support `--mode creds`; upgrade to `keyring>=v25.2.1` for support or provide a username"
);
} else if username.is_none() {
// If we captured stderr, display it in case it's helpful to the user
std::io::stderr().write_all(&output.stderr).ok();
}
None
}
}

#[cfg(test)]
fn fetch_dummy(
store: &std::collections::HashMap<(String, &'static str), &'static str>,
store: &Vec<(String, &'static str, &'static str)>,
service_name: &str,
username: &str,
) -> Option<String> {
store
.get(&(service_name.to_string(), username))
.map(|password| (*password).to_string())
username: Option<&str>,
) -> Option<(String, String)> {
store.iter().find_map(|(service, user, password)| {
if service == service_name && username.map(|username| username == *user).unwrap_or(true)
{
Some(((*user).to_string(), (*password).to_string()))
} else {
None
}
})
}

/// Create a new provider with [`KeyringProviderBackend::Dummy`].
#[cfg(test)]
pub fn dummy<S: Into<String>, T: IntoIterator<Item = ((S, &'static str), &'static str)>>(
pub fn dummy<S: Into<String>, T: IntoIterator<Item = (S, &'static str, &'static str)>>(
iter: T,
) -> Self {
Self {
backend: KeyringProviderBackend::Dummy(
iter.into_iter()
.map(|((service, username), password)| ((service.into(), username), password))
.map(|(service, username, password)| (service.into(), username, password))
.collect(),
),
}
Expand All @@ -142,10 +197,8 @@ impl KeyringProvider {
/// Create a new provider with no credentials available.
#[cfg(test)]
pub fn empty() -> Self {
use std::collections::HashMap;

Self {
backend: KeyringProviderBackend::Dummy(HashMap::new()),
backend: KeyringProviderBackend::Dummy(Vec::new()),
}
}
}
Expand All @@ -160,7 +213,7 @@ mod tests {
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::AssertUnwindSafe(keyring.fetch(&url, "user"))
let result = std::panic::AssertUnwindSafe(keyring.fetch(&url, Some("user")))
.catch_unwind()
.await;
assert!(result.is_err());
Expand All @@ -171,18 +224,18 @@ mod tests {
let url = Url::parse("https://user:[email protected]").unwrap();
let keyring = KeyringProvider::empty();
// Panics due to debug assertion; returns `None` in production
let result = std::panic::AssertUnwindSafe(keyring.fetch(&url, url.username()))
let result = std::panic::AssertUnwindSafe(keyring.fetch(&url, Some(url.username())))
.catch_unwind()
.await;
assert!(result.is_err());
}

#[tokio::test]
async fn fetch_url_with_no_username() {
async fn fetch_url_with_empty_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::AssertUnwindSafe(keyring.fetch(&url, url.username()))
let result = std::panic::AssertUnwindSafe(keyring.fetch(&url, Some(url.username())))
.catch_unwind()
.await;
assert!(result.is_err());
Expand All @@ -192,23 +245,25 @@ mod tests {
async fn fetch_url_no_auth() {
let url = Url::parse("https://example.com").unwrap();
let keyring = KeyringProvider::empty();
let credentials = keyring.fetch(&url, "user");
let credentials = keyring.fetch(&url, Some("user"));
assert!(credentials.await.is_none());
}

#[tokio::test]
async fn fetch_url() {
let url = Url::parse("https://example.com").unwrap();
let keyring = KeyringProvider::dummy([((url.host_str().unwrap(), "user"), "password")]);
let keyring = KeyringProvider::dummy([(url.host_str().unwrap(), "user", "password")]);
assert_eq!(
keyring.fetch(&url, "user").await,
keyring.fetch(&url, Some("user")).await,
Some(Credentials::new(
Some("user".to_string()),
Some("password".to_string())
))
);
assert_eq!(
keyring.fetch(&url.join("test").unwrap(), "user").await,
keyring
.fetch(&url.join("test").unwrap(), Some("user"))
.await,
Some(Credentials::new(
Some("user".to_string()),
Some("password".to_string())
Expand All @@ -219,34 +274,34 @@ mod tests {
#[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").await;
let keyring = KeyringProvider::dummy([("other.com", "user", "password")]);
let credentials = keyring.fetch(&url, Some("user")).await;
assert_eq!(credentials, None);
}

#[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"),
(url.join("foo").unwrap().as_str(), "user", "password"),
(url.host_str().unwrap(), "user", "other-password"),
]);
assert_eq!(
keyring.fetch(&url.join("foo").unwrap(), "user").await,
keyring.fetch(&url.join("foo").unwrap(), Some("user")).await,
Some(Credentials::new(
Some("user".to_string()),
Some("password".to_string())
))
);
assert_eq!(
keyring.fetch(&url, "user").await,
keyring.fetch(&url, Some("user")).await,
Some(Credentials::new(
Some("user".to_string()),
Some("other-password".to_string())
))
);
assert_eq!(
keyring.fetch(&url.join("bar").unwrap(), "user").await,
keyring.fetch(&url.join("bar").unwrap(), Some("user")).await,
Some(Credentials::new(
Some("user".to_string()),
Some("other-password".to_string())
Expand All @@ -257,8 +312,22 @@ mod tests {
#[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").await;
let keyring = KeyringProvider::dummy([(url.host_str().unwrap(), "user", "password")]);
let credentials = keyring.fetch(&url, Some("user")).await;
assert_eq!(
credentials,
Some(Credentials::new(
Some("user".to_string()),
Some("password".to_string())
))
);
}

#[tokio::test]
async fn fetch_url_no_username() {
let url = Url::parse("https://example.com").unwrap();
let keyring = KeyringProvider::dummy([(url.host_str().unwrap(), "user", "password")]);
let credentials = keyring.fetch(&url, None).await;
assert_eq!(
credentials,
Some(Credentials::new(
Expand All @@ -271,13 +340,13 @@ mod tests {
#[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").await;
let keyring = KeyringProvider::dummy([(url.host_str().unwrap(), "foo", "password")]);
let credentials = keyring.fetch(&url, Some("bar")).await;
assert_eq!(credentials, None);

// Still fails if we have `foo` in the URL itself
let url = Url::parse("https://[email protected]").unwrap();
let credentials = keyring.fetch(&url, "bar").await;
let credentials = keyring.fetch(&url, Some("bar")).await;
assert_eq!(credentials, None);
}
}
Loading
Loading