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

Jump to bottom

[DRAFT / Poof of Concept] keyring with --get-mode creds #3081

Closed
wants to merge 1 commit into from
Closed

[DRAFT / Poof of Concept] keyring with --get-mode creds #3081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
161 changes: 137 additions & 24 deletions crates/uv-auth/src/keyring.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,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<Credentials> {
pub(crate) fn fetch(&self, url: &Url, username: Option<&str>) -> Option<Credentials> {
// Validate the request
debug_assert!(
url.host_str().is_some(),
Expand All @@ -48,8 +48,8 @@ impl KeyringProvider {
"Should only use keyring for urls without a password"
);
debug_assert!(
!username.is_empty(),
"Should only use keyring with a username"
!username.is_some_and(|username| username.is_empty()),
"Should only use keyring for urls with a non-empty username or no username at all"
);

let host = url.host_str()?;
Expand All @@ -63,14 +63,30 @@ impl KeyringProvider {
// use-cases.
let mut cache = self.cache.lock().unwrap();

let key = (host.to_string(), username.to_string());
let key = (
host.to_string(),
username.map(|u| u.to_string()).unwrap_or_default(),
);
if cache.contains(&key) {
debug!(
"Skipping keyring lookup for {username} at {host}, already attempted and found no credentials."
"Skipping keyring lookup for {username:?} at {host}, already attempted and found no credentials."
);
return None;
}

let creds = match username {
Some(username) => self.fetch_with_username(url, host, username),
None => self.fetch_without_username(url, host),
};

if creds.is_none() {
cache.insert(key);
}

creds
}

fn fetch_with_username(&self, url: &Url, host: &str, username: &str) -> Option<Credentials> {
// Check the full URL first
// <https://github.com/pypa/pip/blob/ae5fff36b0aad6e5e0037884927eaa29163c0611/src/pip/_internal/network/auth.py#L376C1-L379C14>
let mut password = match self.backend {
Expand All @@ -89,11 +105,27 @@ impl KeyringProvider {
};
}

if password.is_none() {
cache.insert(key);
password.map(|password| Credentials::new(Some(username.to_string()), Some(password)))
}

fn fetch_without_username(&self, url: &Url, host: &str) -> Option<Credentials> {
// Check the full URL first
// <https://github.com/pypa/pip/blob/ae5fff36b0aad6e5e0037884927eaa29163c0611/src/pip/_internal/network/auth.py#L376C1-L379C14>
let mut creds = match self.backend {
KeyringProviderBackend::Subprocess => self.fetch_subprocess_creds(url.as_str()),
#[cfg(test)]
KeyringProviderBackend::Dummy(ref store) => self.fetch_dummy_creds(store, url.as_str()),
};
// And fallback to a check for the host
if creds.is_none() {
creds = match self.backend {
KeyringProviderBackend::Subprocess => self.fetch_subprocess_creds(host),
#[cfg(test)]
KeyringProviderBackend::Dummy(ref store) => self.fetch_dummy_creds(store, host),
};
}

password.map(|password| Credentials::new(Some(username.to_string()), Some(password)))
creds.map(|creds| Credentials::new(Some(creds.0), Some(creds.1)))
}

#[instrument]
Expand All @@ -118,6 +150,44 @@ impl KeyringProvider {
}
}

#[instrument]
fn fetch_subprocess_creds(&self, service_name: &str) -> Option<(String, String)> {
let output = Command::new("keyring")
.arg("--get-mode")
.arg("creds")
.arg("get")
.arg(service_name)
.output()
.inspect_err(|err| warn!("Failure running `keyring` command: {err}"))
.ok()?;

if output.status.success() {
// On success, parse the newline deliniated and newline terminated user/password
String::from_utf8(output.stdout)
.inspect_err(|err| warn!("Failed to parse response from `keyring` command: {err}"))
.ok()
.map(|creds| {
let creds: Vec<_> = creds.trim_end().lines().collect();
match creds.len() {
2 => Some((creds[0].to_string(), creds[1].to_string())),
_ => {
warn!("Unexpected response from `keyring` command: {creds:?}");
None
}
}
})
.flatten()
} else {
if output.status.code() == Some(2) {
warn!("Failed to fetch credentials from keyring; consider upgrading keyring");
// Potetially memoize the failure to avoid repeated attempts
}

// On failure, no password was available
None
}
}

#[cfg(test)]
fn fetch_dummy(
&self,
Expand All @@ -130,6 +200,17 @@ impl KeyringProvider {
.map(|password| password.to_string())
}

#[cfg(test)]
fn fetch_dummy_creds(
&self,
store: &std::collections::HashMap<(String, &'static str), &'static str>,
service_name: &str,
) -> Option<(String, String)> {
store
.get(&(service_name.to_string(), "NO_USERNAME"))
.map(|password| ("NO_USERNAME".to_string(), password.to_string()))
}

/// Create a new provider with [`KeyringProviderBackend::Dummy`].
#[cfg(test)]
pub fn dummy<S: Into<String>, T: IntoIterator<Item = ((S, &'static str), &'static str)>>(
Expand Down Expand Up @@ -167,7 +248,7 @@ mod test {
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::catch_unwind(|| keyring.fetch(&url, Some("user")));
assert!(result.is_err());
}

Expand All @@ -176,24 +257,35 @@ mod test {
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::catch_unwind(|| keyring.fetch(&url, url.username()));
let result = std::panic::catch_unwind(|| keyring.fetch(&url, Some(url.username())));
assert!(result.is_err());
}

#[test]
fn fetch_url_with_no_username() {
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::catch_unwind(|| keyring.fetch(&url, url.username()));
let result = std::panic::catch_unwind(|| keyring.fetch(&url, Some(url.username())));
assert!(result.is_err());
}

#[test]
fn fetch_url_with_no_username() {
let url = Url::parse("https://example.com").unwrap();
let keyring = KeyringProvider::empty();
// Doesn't panic
let result = std::panic::catch_unwind(|| keyring.fetch(&url, None));
assert!(result.is_ok());
}

#[test]
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.is_none());
let credentials = keyring.fetch(&url, None);
assert!(credentials.is_none());
}

Expand All @@ -202,26 +294,47 @@ mod test {
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, Some("user")),
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(), Some("user")),
Some(Credentials::new(
Some("user".to_string()),
Some("password".to_string())
))
);
}

#[test]
fn fetch_url_no_username() {
let url = Url::parse("https://example.com").unwrap();
let keyring =
KeyringProvider::dummy([((url.host_str().unwrap(), "NO_USERNAME"), "password")]);
assert_eq!(
keyring.fetch(&url, None),
Some(Credentials::new(
Some("NO_USERNAME".to_string()),
Some("password".to_string())
))
);
assert_eq!(
keyring.fetch(&url.join("test").unwrap(), None),
Some(Credentials::new(
Some("NO_USERNAME".to_string()),
Some("password".to_string())
))
);
}

#[test]
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, Some("user"));
assert_eq!(credentials, None);
}

Expand All @@ -233,21 +346,21 @@ mod test {
((url.host_str().unwrap(), "user"), "other-password"),
]);
assert_eq!(
keyring.fetch(&url.join("foo").unwrap(), "user"),
keyring.fetch(&url.join("foo").unwrap(), Some("user")),
Some(Credentials::new(
Some("user".to_string()),
Some("password".to_string())
))
);
assert_eq!(
keyring.fetch(&url, "user"),
keyring.fetch(&url, Some("user")),
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(), Some("user")),
Some(Credentials::new(
Some("user".to_string()),
Some("other-password".to_string())
Expand All @@ -265,17 +378,17 @@ mod test {
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(), Some("user")), 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(), Some("user")), None);
}

#[test]
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, Some("user"));
assert_eq!(
credentials,
Some(Credentials::new(
Expand All @@ -289,12 +402,12 @@ mod test {
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, Some("bar"));
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");
let credentials = keyring.fetch(&url, Some("bar"));
assert_eq!(credentials, None);
}
}
13 changes: 4 additions & 9 deletions crates/uv-auth/src/middleware.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,16 +137,11 @@ impl Middleware for AuthMiddleware {
// 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
let username = credentials
.get()
.and_then(|credentials| credentials.username())
{
debug!("Checking keyring for credentials for {url}");
keyring.fetch(request.url(), username)
} else {
trace!("Skipping keyring lookup for {url} with no username");
None
}
.and_then(|credentials| credentials.username());
debug!("Checking keyring for credentials for {url}");
keyring.fetch(request.url(), username)
}) {
debug!("Found credentials in keyring for {url}");
request = credentials.authenticate(request);
Expand Down
Loading