fix bootstrap credential handling

domenkozar · domenkozar · commit 444d195de8d4 · 2026-07-15T15:37:44.000-04:00
diff --git a/secretspec/src/cli/mod.rs b/secretspec/src/cli/mod.rs
@@ -174,9 +174,9 @@ enum ProviderAction {
         /// Provider URI (e.g., "keyring://", "onepassword://Shared", "dotenv://.env.local")
         uri: String,
         /// Bootstrap credential binding `VAR=PROVIDER` (repeatable): the alias's
-        /// provider reads `VAR` from `PROVIDER` before the environment. Only the
-        /// bare-string source form is expressible here; add a `ref` by editing
-        /// the config.
+        /// provider uses a non-empty `VAR` from the environment first, falling
+        /// back to `PROVIDER`. Only the bare-string source form is expressible
+        /// here; add a `ref` by editing the config.
         #[arg(long = "env", value_name = "VAR=PROVIDER")]
         env: Vec<String>,
     },
@@ -626,7 +626,7 @@ pub fn main() -> Result<()> {
                         for (var, source) in credentials {
                             let entered = inquire::Password::new(&format!(
                                 "Enter {var} for provider '{name}' (source: {}):",
-                                source.provider
+                                source.display_provider()
                             ))
                             .without_confirmation()
                             .prompt()
@@ -1302,6 +1302,18 @@ mod tests {
         }
     }
 
+    #[test]
+    fn provider_add_help_describes_environment_first_precedence() {
+        let help = Cli::try_parse_from(["secretspec", "config", "provider", "add", "--help"])
+            .err()
+            .expect("--help should stop parsing")
+            .to_string();
+
+        assert!(help.contains("uses a non-empty `VAR` from the environment first"));
+        assert!(help.contains("falling back to `PROVIDER`"));
+        assert!(!help.contains("from `PROVIDER` before the environment"));
+    }
+
     #[test]
     fn provider_login_parses() {
         let cli =
diff --git a/secretspec/src/provider/mod.rs b/secretspec/src/provider/mod.rs
@@ -655,7 +655,7 @@ pub trait Provider: Send + Sync {
     fn with_base_dir(&mut self, _base_dir: &std::path::Path) {}
 
     /// Hands the provider its bootstrap-credential overlay (see [`BootstrapEnv`])
-    /// so credential reads can prefer it over the process environment.
+    /// as a fallback when the process environment has no non-empty value.
     ///
     /// Called once inside the registration factory, on the concrete provider
     /// value *before* any `Arc`/`Box` wrapping. This must not be a
diff --git a/secretspec/src/secrets.rs b/secretspec/src/secrets.rs
@@ -19,7 +19,7 @@ use std::env;
 use std::io::{self, IsTerminal, Read};
 use std::path::{Path, PathBuf};
 use std::process::Command;
-use std::sync::Mutex;
+use std::sync::{Arc, Mutex};
 
 /// Emits a warning when a provider in a fallback chain fails so the user
 /// can see why a particular link was skipped, without aborting the chain.
@@ -63,6 +63,11 @@ fn sorted_bootstrap_entries(
 }
 
 impl BootstrapSource {
+    /// Credential-free provider text for prompts and diagnostics.
+    pub(crate) fn display_provider(&self) -> String {
+        crate::audit::redact_uri_strict(&self.provider)
+    }
+
     /// The store location this source reads and writes: the pinned `ref`, or
     /// the convention path for the active project and profile. The single
     /// derivation both [`Secrets::resolve_bootstrap_overlay`] (read) and
@@ -81,14 +86,72 @@ impl BootstrapSource {
     /// (`onepassword+token://tok@Vault`), and this string reaches stderr and
     /// the `config provider login` output.
     fn location(&self, project: &str, profile: &str, var: &str) -> String {
-        let provider = crate::audit::redact_uri_strict(&self.provider);
+        let provider = self.display_provider();
         match &self.reference {
             Some(reference) => format!("{provider} at {}", reference.render()),
             None => format!("{provider} at {project}/{profile}/{var}"),
         }
     }
 }
 
+type BootstrapOverlayKey = (String, String);
+type BootstrapOverlaySlot = Arc<Mutex<Option<BootstrapEnv>>>;
+
+/// Memoized bootstrap overlays with single-flight population per key.
+///
+/// The outer mutex protects only the key-to-slot map. Resolution runs while
+/// holding the selected slot, so callers for the same alias/profile wait for
+/// its first fetch while unrelated keys can populate concurrently.
+#[derive(Default)]
+struct BootstrapOverlayCache {
+    entries: Mutex<HashMap<BootstrapOverlayKey, BootstrapOverlaySlot>>,
+}
+
+impl BootstrapOverlayCache {
+    fn get_or_try_init<F>(&self, key: BootstrapOverlayKey, resolve: F) -> Result<BootstrapEnv>
+    where
+        F: FnOnce() -> Result<BootstrapEnv>,
+    {
+        let slot = {
+            let mut entries = self.entries.lock().unwrap();
+            Arc::clone(
+                entries
+                    .entry(key.clone())
+                    .or_insert_with(|| Arc::new(Mutex::new(None))),
+            )
+        };
+
+        let mut cached = slot.lock().unwrap();
+        if let Some(overlay) = cached.as_ref() {
+            return Ok(overlay.clone());
+        }
+
+        match resolve() {
+            Ok(overlay) => {
+                *cached = Some(overlay.clone());
+                Ok(overlay)
+            }
+            Err(err) => {
+                // Do not memoize failures: a later operation may succeed after
+                // credentials or provider availability change.
+                drop(cached);
+                let mut entries = self.entries.lock().unwrap();
+                if entries
+                    .get(&key)
+                    .is_some_and(|current| Arc::ptr_eq(current, &slot))
+                {
+                    entries.remove(&key);
+                }
+                Err(err)
+            }
+        }
+    }
+
+    fn clear(&self) {
+        self.entries.lock().unwrap().clear();
+    }
+}
+
 /// Emits a warning when the primary provider for a batch fetch fails (either
 /// during construction or during `get_many`); affected secrets will still be
 /// retried via their per-secret fallback chain below.
@@ -194,7 +257,7 @@ pub struct Secrets {
     /// not reuse the other profile's credential. Cleared by
     /// [`Secrets::store_bootstrap_credential`] so a freshly stored credential
     /// is re-read.
-    bootstrap_overlays: Mutex<HashMap<(String, String), BootstrapEnv>>,
+    bootstrap_overlays: BootstrapOverlayCache,
 }
 
 /// secretspec's own opt-in for marking the current process as an agent. Lets any
@@ -341,7 +404,7 @@ impl Secrets {
             reason: None,
             require_reason: RequireReason::Never,
             audit: None,
-            bootstrap_overlays: Mutex::new(HashMap::new()),
+            bootstrap_overlays: BootstrapOverlayCache::default(),
         }
     }
 
@@ -417,7 +480,7 @@ impl Secrets {
             profile: None,
             reason: env_reason(),
             audit,
-            bootstrap_overlays: Mutex::new(HashMap::new()),
+            bootstrap_overlays: BootstrapOverlayCache::default(),
         })
     }
 
@@ -531,25 +594,16 @@ impl Secrets {
     ) -> Result<Box<dyn ProviderTrait>> {
         // When `spec` names an alias with a bootstrap `env` map, resolve those
         // credentials from their source providers into an overlay the built
-        // provider reads before the process environment. Memoized per
-        // (profile, spec) so rebuilding a provider (per-secret chain walks,
+        // provider uses when the process environment has no non-empty value.
+        // Memoized per (profile, spec) so rebuilding a provider (per-secret chain walks,
         // interactive prompting) does not refetch the same credentials from
         // the source store, while a profile switch on this instance does not
         // reuse the other profile's credentials.
         let profile = self.resolve_profile_name(profile);
         let key = (profile.clone(), spec.clone());
-        let cached = self.bootstrap_overlays.lock().unwrap().get(&key).cloned();
-        let overlay = match cached {
-            Some(overlay) => overlay,
-            None => {
-                let overlay = self.resolve_bootstrap_overlay(&spec, &profile)?;
-                self.bootstrap_overlays
-                    .lock()
-                    .unwrap()
-                    .insert(key, overlay.clone());
-                overlay
-            }
-        };
+        let overlay = self
+            .bootstrap_overlays
+            .get_or_try_init(key, || self.resolve_bootstrap_overlay(&spec, &profile))?;
         self.build_provider_with_overlay(&spec, overlay)
     }
 
@@ -583,7 +637,7 @@ impl Secrets {
 
     /// Resolves the bootstrap overlay for a provider spec: for each `(VAR, source)`
     /// its alias declares in `env`, fetch the credential from the source provider
-    /// and collect it, so the built provider reads it before the environment.
+    /// and collect it as a fallback for a missing or empty environment value.
     ///
     /// `profile` scopes the convention path a bare-string source reads from.
     /// Returns an empty overlay for a spec that is not an alias, or an alias with
@@ -702,12 +756,17 @@ impl Secrets {
     }
 
     /// The bootstrap credentials a provider alias declares, sorted by variable
-    /// name, for the `config provider login` flow. Errors if the alias is not
-    /// defined; returns an empty list for an alias with no `env`.
+    /// name, for the `config provider login` flow. Validates every source before
+    /// returning any credentials. Errors if the alias is not defined; returns
+    /// an empty list for an alias with no `env`.
     pub(crate) fn bootstrap_credentials(
         &self,
         alias: &str,
     ) -> Result<Vec<(String, BootstrapSource)>> {
+        // Validate the complete map before returning any entry. The login CLI
+        // prompts and writes only after this method succeeds, so a later-sorted
+        // invalid source cannot leave earlier credentials partially stored.
+        self.validate_bootstrap_sources(alias)?;
         let entry = self
             .lookup_provider_alias_entry(alias)
             .ok_or_else(|| SecretSpecError::ProviderNotFound(alias.to_string()))?;
@@ -752,7 +811,7 @@ impl Secrets {
         result?;
         // The stored credential replaces whatever an earlier resolution
         // memoized; drop the memo so the next build re-reads it.
-        self.bootstrap_overlays.lock().unwrap().clear();
+        self.bootstrap_overlays.clear();
         Ok(source.location(&project, &profile, var))
     }
 
@@ -3207,6 +3266,54 @@ mod policy_tests {
     }
 }
 
+#[cfg(test)]
+mod bootstrap_overlay_cache_tests {
+    use super::*;
+    use std::sync::atomic::{AtomicUsize, Ordering};
+    use std::sync::{Arc, Barrier};
+    use std::thread;
+    use std::time::Duration;
+
+    #[test]
+    fn concurrent_population_for_one_key_is_single_flight() {
+        const CALLERS: usize = 8;
+        let cache = Arc::new(BootstrapOverlayCache::default());
+        let start = Arc::new(Barrier::new(CALLERS));
+        let fetches = Arc::new(AtomicUsize::new(0));
+
+        let threads: Vec<_> = (0..CALLERS)
+            .map(|_| {
+                let cache = Arc::clone(&cache);
+                let start = Arc::clone(&start);
+                let fetches = Arc::clone(&fetches);
+                thread::spawn(move || {
+                    start.wait();
+                    cache
+                        .get_or_try_init(("default".into(), "target".into()), || {
+                            fetches.fetch_add(1, Ordering::SeqCst);
+                            // Keep the first population in flight long enough for
+                            // every caller to contend on the same key.
+                            thread::sleep(Duration::from_millis(50));
+                            let mut overlay = BootstrapEnv::new();
+                            overlay.insert("TOKEN".into(), SecretString::new("value".into()));
+                            Ok(overlay)
+                        })
+                        .unwrap()
+                })
+            })
+            .collect();
+
+        for thread in threads {
+            let overlay = thread.join().unwrap();
+            assert_eq!(
+                overlay.get("TOKEN").map(|value| value.expose_secret()),
+                Some("value")
+            );
+        }
+        assert_eq!(fetches.load(Ordering::SeqCst), 1);
+    }
+}
+
 /// Serializes tests that mutate the process-global current directory. The current
 /// directory is shared across all threads, so two `set_current_dir` tests running
 /// concurrently (the default under `cargo test`) would corrupt each other. Any test
diff --git a/secretspec/src/tests.rs b/secretspec/src/tests.rs
@@ -6123,6 +6123,46 @@ fn bootstrap_overlay_reads_convention_credential_from_source() {
     );
 }
 
+#[test]
+fn bootstrap_source_credential_reaches_target_provider_end_to_end() {
+    let _guard = scrub_resolution_env();
+    let _token = EnvVarGuard::remove("OP_SERVICE_ACCOUNT_TOKEN");
+    let temp = TempDir::new().unwrap();
+
+    let target_scope = |filename: &str, token: &str| {
+        let source = temp.path().join(filename);
+        std::fs::write(&source, format!("OP_SERVICE_ACCOUNT_TOKEN={token}\n")).unwrap();
+        let secrets = secrets_with_bootstrap_alias(
+            "onepassword://Private",
+            HashMap::from([(
+                "OP_SERVICE_ACCOUNT_TOKEN".to_string(),
+                BootstrapSource::from(format!("dotenv://{}", source.display())),
+            )]),
+        );
+
+        secrets
+            .get_provider(Some("target"), Some("default"))
+            .expect("the source credential should build the target provider")
+            .auth_scope_key()
+            .expect("onepassword should identify its effective authentication scope")
+    };
+
+    let first = target_scope("first.env", "source-token-a");
+    let same = target_scope("same.env", "source-token-a");
+    let different = target_scope("different.env", "source-token-b");
+
+    assert_eq!(
+        first, same,
+        "the same fetched credential yields the same scope"
+    );
+    assert_ne!(
+        first, different,
+        "changing the source credential must change the target's effective auth scope"
+    );
+    assert!(!first.contains("source-token-a"));
+    assert!(!different.contains("source-token-b"));
+}
+
 #[test]
 fn bootstrap_overlay_reads_ref_addressed_credential() {
     let _guard = scrub_resolution_env();
@@ -6224,6 +6264,35 @@ fn bootstrap_source_must_name_a_known_provider() {
     );
 }
 
+#[test]
+fn bootstrap_credentials_validates_every_source_before_login() {
+    let secrets = secrets_with_bootstrap_alias(
+        "bws://project",
+        HashMap::from([
+            (
+                "A_VALID_FIRST".to_string(),
+                BootstrapSource::from("dotenv://source.env"),
+            ),
+            (
+                "Z_INVALID_LAST".to_string(),
+                BootstrapSource::from("not_a_real_provider"),
+            ),
+        ]),
+    );
+
+    let error = secrets.bootstrap_credentials("target").unwrap_err();
+    assert!(error.to_string().contains("Z_INVALID_LAST"));
+}
+
+#[test]
+fn bootstrap_source_display_redacts_inline_credentials() {
+    let source = BootstrapSource::from("onepassword+token://ops_secret@Vault");
+    let displayed = source.display_provider();
+
+    assert_eq!(displayed, "onepassword+token://Vault");
+    assert!(!displayed.contains("ops_secret"));
+}
+
 #[test]
 fn bootstrap_chain_is_limited_to_one_hop() {
     let mut config = resolve_test_config(HashMap::new());
