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

@@ -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]]

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"

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<CompactString>,
+    pub(crate) github_token: Option<GithubToken>,
 
     /// 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<Box<str>>);
+
+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())));
         }
     }
 

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)?
+    let gh_api_client = args
+        .github_token
+        .map(|token| token.0)
+        .or_else(|| {
+            if args.no_discover_github_token {
+                None
             } else {
-            github_token
-        },
-    );
+                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 {

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<CompactString> {
+pub(super) async fn get() -> io::Result<Zeroizing<Box<str>>> {
     let Output { status, stdout, .. } = Command::new("gh")
         .args(["auth", "token"])
         .stdin(Stdio::null())
@@ -15,6 +16,8 @@ pub(super) async fn get() -> io::Result<CompactString> {
         .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<CompactString> {
         ));
     }
 
-    // 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()))
 }

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<CompactString> {
+pub fn try_from_home() -> Option<Zeroizing<Box<str>>> {
     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<CompactString> {
     None
 }
 
-fn from_file(path: PathBuf) -> Option<CompactString> {
-    fs::read_to_string(path)
-        .ok()?
+fn from_file(path: PathBuf) -> Option<Zeroizing<Box<str>>> {
+    Zeroizing::new(fs::read_to_string(path).ok()?)
         .lines()
         .find_map(from_line)
-        .map(CompactString::from)
+        .map(Box::<str>::from)
+        .map(Zeroizing::new)
 }
 
 fn from_line(line: &str) -> Option<&str> {

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" }

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<GhRelease, OnceCell<Option<release_artifacts::Artifacts>>>,
     retry_after: Mutex<Option<Instant>>,
 
-    auth_token: Option<CompactString>,
+    auth_token: Option<Zeroizing<Box<str>>>,
     is_auth_token_valid: AtomicBool,
 
     only_use_restful_api: AtomicBool,
@@ -141,7 +142,7 @@ struct Inner {
 pub struct GhApiClient(Arc<Inner>);
 
 impl GhApiClient {
-    pub fn new(client: remote::Client, auth_token: Option<CompactString>) -> Self {
+    pub fn new(client: remote::Client, auth_token: Option<Zeroizing<Box<str>>>) -> 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::<str>::from)
+            .map(zeroize::Zeroizing::new);
 
         let gh_client = GhApiClient::new(client.clone(), auth_token.clone());
         gh_client.set_only_use_restful_api();

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"]

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;
 

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<Zeroizing<Box<str>>>;
+
+#[derive(Debug)]
+pub struct LazyGhApiClient {
+    client: remote::Client,
+    inner: OnceCell<GhApiClient>,
+    task: Mutex<Option<AutoAbortJoinHandle<GitHubToken>>>,
+}
+
+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<Fut>(client: remote::Client, get_auth_token_future: Fut) -> Self
+    where
+        Fut: Future<Output = GitHubToken> + 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
+    }
+}

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<PathBuf>,
 
     pub client: Client,
-    pub gh_api_client: GhApiClient,
+    pub gh_api_client: LazyGhApiClient,
     pub jobserver_client: LazyJobserverClient,
     pub registry: Registry,
 

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<Data>, 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,