From fff6aa812252e6ac091c65568eefa315b88ec9f0 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sat, 15 Jun 2024 15:42:09 +1000 Subject: [PATCH] Improve use of github token (#1769) * Add new dep zeroize * Use Zeroizing to avoid leaking the token * Optimize gh-auth-token Spawn it as a task, and only await it when using GhApiClient * Fix binstalk-git-repo-api unit tests --- Cargo.lock | 3 + crates/bin/Cargo.toml | 1 + crates/bin/src/args.rs | 15 ++++- crates/bin/src/entry.rs | 61 +++++++++---------- crates/bin/src/gh_token.rs | 13 ++-- crates/bin/src/git_credentials.rs | 12 ++-- crates/binstalk-git-repo-api/Cargo.toml | 1 + .../src/gh_api_client.rs | 10 +-- crates/binstalk/Cargo.toml | 1 + crates/binstalk/src/helpers.rs | 1 + .../src/helpers/lazy_gh_api_client.rs | 53 ++++++++++++++++ crates/binstalk/src/ops.rs | 7 ++- crates/binstalk/src/ops/resolve.rs | 4 +- 13 files changed, 128 insertions(+), 54 deletions(-) create mode 100644 crates/binstalk/src/helpers/lazy_gh_api_client.rs diff --git a/Cargo.lock b/Cargo.lock index 4188da21..2b1d3b50 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -274,6 +274,7 @@ dependencies = [ "tokio", "tracing", "url", + "zeroize", ] [[package]] @@ -364,6 +365,7 @@ dependencies = [ "tracing", "tracing-subscriber", "url", + "zeroize", ] [[package]] @@ -573,6 +575,7 @@ dependencies = [ "tracing-log", "tracing-subscriber", "vergen", + "zeroize", ] [[package]] diff --git a/crates/bin/Cargo.toml b/crates/bin/Cargo.toml index 605f225a..592f36c6 100644 --- a/crates/bin/Cargo.toml +++ b/crates/bin/Cargo.toml @@ -43,6 +43,7 @@ tracing-core = "0.1.32" tracing = { version = "0.1.39", default-features = false } tracing-log = { version = "0.2.0", default-features = false } tracing-subscriber = { version = "0.3.17", features = ["fmt", "json", "ansi"], default-features = false } +zeroize = "1.8.1" [build-dependencies] embed-resource = "2.4.1" diff --git a/crates/bin/src/args.rs b/crates/bin/src/args.rs index d32ea65a..e81f1970 100644 --- a/crates/bin/src/args.rs +++ b/crates/bin/src/args.rs @@ -15,11 +15,11 @@ use binstalk::{ }; use clap::{error::ErrorKind, CommandFactory, Parser, ValueEnum}; use compact_str::CompactString; - use log::LevelFilter; use semver::VersionReq; use strum::EnumCount; use strum_macros::EnumCount; +use zeroize::Zeroizing; #[derive(Debug, Parser)] #[clap( @@ -308,7 +308,7 @@ pub struct Args { /// token from `$HOME/.git-credentials` or `$HOME/.config/gh/hosts.yml` /// unless `--no-discover-github-token` is specified. #[clap(help_heading = "Options", long, env = "GITHUB_TOKEN")] - pub(crate) github_token: Option, + pub(crate) github_token: Option, /// Only install packages that are signed /// @@ -365,6 +365,15 @@ pub struct Args { pub(crate) quiet: bool, } +#[derive(Debug, Clone)] +pub(crate) struct GithubToken(pub(crate) Zeroizing>); + +impl From<&str> for GithubToken { + fn from(s: &str) -> Self { + Self(Zeroizing::new(s.into())) + } +} + #[derive(Debug, Copy, Clone, ValueEnum)] pub(crate) enum TLSVersion { #[clap(name = "1.2")] @@ -575,7 +584,7 @@ You cannot use --{option} and specify multiple packages at the same time. Do one if opts.github_token.is_none() { if let Ok(github_token) = env::var("GH_TOKEN") { - opts.github_token = Some(github_token.into()); + opts.github_token = Some(GithubToken(Zeroizing::new(github_token.into()))); } } diff --git a/crates/bin/src/entry.rs b/crates/bin/src/entry.rs index e8e30079..48288aa2 100644 --- a/crates/bin/src/entry.rs +++ b/crates/bin/src/entry.rs @@ -9,8 +9,8 @@ use binstalk::{ fetchers::{Fetcher, GhCrateMeta, QuickInstall, SignaturePolicy}, get_desired_targets, helpers::{ - gh_api_client::GhApiClient, jobserver_client::LazyJobserverClient, + lazy_gh_api_client::LazyGhApiClient, remote::{Certificate, Client}, tasks::AutoAbortJoinHandle, }, @@ -27,7 +27,7 @@ use file_format::FileFormat; use home::cargo_home; use log::LevelFilter; use miette::{miette, Report, Result, WrapErr}; -use tokio::{runtime::Handle, task::block_in_place}; +use tokio::task::block_in_place; use tracing::{debug, error, info, warn}; use crate::{ @@ -82,28 +82,6 @@ pub fn install_crates( // Launch target detection let desired_targets = get_desired_targets(args.targets); - // Launch scraping of gh token - let no_discover_github_token = args.no_discover_github_token; - let github_token = args.github_token.or_else(|| { - if args.no_discover_github_token { - None - } else { - git_credentials::try_from_home() - } - }); - let get_gh_token_task = (github_token.is_none() && !no_discover_github_token).then(|| { - AutoAbortJoinHandle::spawn(async move { - match gh_token::get().await { - Ok(token) => Some(token), - Err(err) => { - debug!(?err, "Failed to retrieve token from `gh auth token`"); - debug!("Failed to read git credential file"); - None - } - } - }) - }); - // Computer cli_overrides let cli_overrides = PkgOverride { pkg_url: args.pkg_url, @@ -129,14 +107,33 @@ pub fn install_crates( ) .map_err(BinstallError::from)?; - let gh_api_client = GhApiClient::new( - client.clone(), - if let Some(task) = get_gh_token_task { - Handle::current().block_on(task)? - } else { - github_token - }, - ); + let gh_api_client = args + .github_token + .map(|token| token.0) + .or_else(|| { + if args.no_discover_github_token { + None + } else { + git_credentials::try_from_home() + } + }) + .map(|token| LazyGhApiClient::new(client.clone(), Some(token))) + .unwrap_or_else(|| { + if args.no_discover_github_token { + LazyGhApiClient::new(client.clone(), None) + } else { + LazyGhApiClient::with_get_gh_token_future(client.clone(), async { + match gh_token::get().await { + Ok(token) => Some(token), + Err(err) => { + debug!(?err, "Failed to retrieve token from `gh auth token`"); + debug!("Failed to read git credential file"); + None + } + } + }) + } + }); // Create binstall_opts let binstall_opts = Arc::new(Options { diff --git a/crates/bin/src/gh_token.rs b/crates/bin/src/gh_token.rs index b7a46655..837f9f95 100644 --- a/crates/bin/src/gh_token.rs +++ b/crates/bin/src/gh_token.rs @@ -1,12 +1,13 @@ use std::{ io, process::{Output, Stdio}, + str, }; -use compact_str::CompactString; use tokio::process::Command; +use zeroize::Zeroizing; -pub(super) async fn get() -> io::Result { +pub(super) async fn get() -> io::Result>> { let Output { status, stdout, .. } = Command::new("gh") .args(["auth", "token"]) .stdin(Stdio::null()) @@ -15,6 +16,8 @@ pub(super) async fn get() -> io::Result { .output() .await?; + let stdout = Zeroizing::new(stdout); + if !status.success() { return Err(io::Error::new( io::ErrorKind::Other, @@ -22,14 +25,12 @@ pub(super) async fn get() -> io::Result { )); } - // Use String here instead of CompactString here since - // `CompactString::from_utf8` allocates if it's longer than 24B. - let s = String::from_utf8(stdout).map_err(|err| { + let s = str::from_utf8(&stdout).map_err(|err| { io::Error::new( io::ErrorKind::InvalidData, format!("Invalid output, expected utf8: {err}"), ) })?; - Ok(s.trim().into()) + Ok(Zeroizing::new(s.trim().into())) } diff --git a/crates/bin/src/git_credentials.rs b/crates/bin/src/git_credentials.rs index d2e420f7..9c9a35ba 100644 --- a/crates/bin/src/git_credentials.rs +++ b/crates/bin/src/git_credentials.rs @@ -1,9 +1,9 @@ use std::{env, fs, path::PathBuf}; -use compact_str::CompactString; use dirs::home_dir; +use zeroize::Zeroizing; -pub fn try_from_home() -> Option { +pub fn try_from_home() -> Option>> { if let Some(mut home) = home_dir() { home.push(".git-credentials"); if let Some(cred) = from_file(home) { @@ -23,12 +23,12 @@ pub fn try_from_home() -> Option { None } -fn from_file(path: PathBuf) -> Option { - fs::read_to_string(path) - .ok()? +fn from_file(path: PathBuf) -> Option>> { + Zeroizing::new(fs::read_to_string(path).ok()?) .lines() .find_map(from_line) - .map(CompactString::from) + .map(Box::::from) + .map(Zeroizing::new) } fn from_line(line: &str) -> Option<&str> { diff --git a/crates/binstalk-git-repo-api/Cargo.toml b/crates/binstalk-git-repo-api/Cargo.toml index f2f0361f..0a807f61 100644 --- a/crates/binstalk-git-repo-api/Cargo.toml +++ b/crates/binstalk-git-repo-api/Cargo.toml @@ -22,6 +22,7 @@ thiserror = "1.0.52" tokio = { version = "1.35.0", features = ["sync"], default-features = false } tracing = "0.1.39" url = "2.3.1" +zeroize = "1.8.1" [dev-dependencies] binstalk-downloader = { version = "0.11.3", path = "../binstalk-downloader" } diff --git a/crates/binstalk-git-repo-api/src/gh_api_client.rs b/crates/binstalk-git-repo-api/src/gh_api_client.rs index 5823b53e..b2e3d6a3 100644 --- a/crates/binstalk-git-repo-api/src/gh_api_client.rs +++ b/crates/binstalk-git-repo-api/src/gh_api_client.rs @@ -14,6 +14,7 @@ use compact_str::{format_compact, CompactString, ToCompactString}; use tokio::sync::OnceCell; use tracing::{instrument, Level}; use url::Url; +use zeroize::Zeroizing; mod common; mod error; @@ -129,7 +130,7 @@ struct Inner { release_artifacts: Map>>, retry_after: Mutex>, - auth_token: Option, + auth_token: Option>>, is_auth_token_valid: AtomicBool, only_use_restful_api: AtomicBool, @@ -141,7 +142,7 @@ struct Inner { pub struct GhApiClient(Arc); impl GhApiClient { - pub fn new(client: remote::Client, auth_token: Option) -> Self { + pub fn new(client: remote::Client, auth_token: Option>>) -> Self { Self(Arc::new(Inner { client, release_artifacts: Default::default(), @@ -184,7 +185,7 @@ impl GhApiClient { fn get_auth_token(&self) -> Option<&str> { if self.0.is_auth_token_valid.load(Relaxed) { - self.0.auth_token.as_deref() + self.0.auth_token.as_deref().map(|s| &**s) } else { None } @@ -526,7 +527,8 @@ mod test { let auth_token = env::var("CI_UNIT_TEST_GITHUB_TOKEN") .ok() - .map(CompactString::from); + .map(Box::::from) + .map(zeroize::Zeroizing::new); let gh_client = GhApiClient::new(client.clone(), auth_token.clone()); gh_client.set_only_use_restful_api(); diff --git a/crates/binstalk/Cargo.toml b/crates/binstalk/Cargo.toml index fc39d74a..7ad056e8 100644 --- a/crates/binstalk/Cargo.toml +++ b/crates/binstalk/Cargo.toml @@ -43,6 +43,7 @@ tokio = { version = "1.35.0", features = [ ], default-features = false } tracing = "0.1.39" url = { version = "2.3.1", features = ["serde"] } +zeroize = "1.8.1" [features] default = ["static", "rustls", "git"] diff --git a/crates/binstalk/src/helpers.rs b/crates/binstalk/src/helpers.rs index 4e5792e6..5222f440 100644 --- a/crates/binstalk/src/helpers.rs +++ b/crates/binstalk/src/helpers.rs @@ -3,6 +3,7 @@ pub mod remote { pub use binstalk_downloader::remote::*; pub use url::ParseError as UrlParseError; } +pub mod lazy_gh_api_client; pub(crate) mod target_triple; pub mod tasks; diff --git a/crates/binstalk/src/helpers/lazy_gh_api_client.rs b/crates/binstalk/src/helpers/lazy_gh_api_client.rs new file mode 100644 index 00000000..26869e91 --- /dev/null +++ b/crates/binstalk/src/helpers/lazy_gh_api_client.rs @@ -0,0 +1,53 @@ +use std::{future::Future, sync::Mutex}; + +use binstalk_git_repo_api::gh_api_client::GhApiClient; +use tokio::sync::OnceCell; +use zeroize::Zeroizing; + +use crate::{ + errors::BinstallError, + helpers::{remote, tasks::AutoAbortJoinHandle}, +}; + +pub type GitHubToken = Option>>; + +#[derive(Debug)] +pub struct LazyGhApiClient { + client: remote::Client, + inner: OnceCell, + task: Mutex>>, +} + +impl LazyGhApiClient { + pub fn new(client: remote::Client, auth_token: GitHubToken) -> Self { + Self { + inner: OnceCell::new_with(Some(GhApiClient::new(client.clone(), auth_token))), + client, + task: Mutex::new(None), + } + } + + pub fn with_get_gh_token_future(client: remote::Client, get_auth_token_future: Fut) -> Self + where + Fut: Future + Send + Sync + 'static, + { + Self { + inner: OnceCell::new(), + task: Mutex::new(Some(AutoAbortJoinHandle::spawn(get_auth_token_future))), + client, + } + } + + pub async fn get(&self) -> Result<&GhApiClient, BinstallError> { + self.inner + .get_or_try_init(|| async { + let task = self.task.lock().unwrap().take(); + Ok(if let Some(task) = task { + GhApiClient::new(self.client.clone(), task.await?) + } else { + GhApiClient::new(self.client.clone(), None) + }) + }) + .await + } +} diff --git a/crates/binstalk/src/ops.rs b/crates/binstalk/src/ops.rs index 66366be6..ae810a3d 100644 --- a/crates/binstalk/src/ops.rs +++ b/crates/binstalk/src/ops.rs @@ -6,7 +6,10 @@ use semver::VersionReq; use crate::{ fetchers::{Data, Fetcher, SignaturePolicy, TargetDataErased}, - helpers::{gh_api_client::GhApiClient, jobserver_client::LazyJobserverClient, remote::Client}, + helpers::{ + gh_api_client::GhApiClient, jobserver_client::LazyJobserverClient, + lazy_gh_api_client::LazyGhApiClient, remote::Client, + }, manifests::cargo_toml_binstall::PkgOverride, registry::Registry, DesiredTargets, @@ -47,7 +50,7 @@ pub struct Options { pub cargo_root: Option, pub client: Client, - pub gh_api_client: GhApiClient, + pub gh_api_client: LazyGhApiClient, pub jobserver_client: LazyJobserverClient, pub registry: Registry, diff --git a/crates/binstalk/src/ops/resolve.rs b/crates/binstalk/src/ops/resolve.rs index 0ab49faf..805bfb4a 100644 --- a/crates/binstalk/src/ops/resolve.rs +++ b/crates/binstalk/src/ops/resolve.rs @@ -106,6 +106,8 @@ async fn resolve_inner( }, ); + let gh_api_client = opts.gh_api_client.get().await?; + let mut handles_fn = |data: Arc, filter_fetcher_by_name_predicate: fn(&'static str) -> bool| { handles.extend( @@ -132,7 +134,7 @@ async fn resolve_inner( .filter_map(|(f, target_data)| { let fetcher = f( opts.client.clone(), - opts.gh_api_client.clone(), + gh_api_client.clone(), data.clone(), target_data, opts.signature_policy,