Auto merge of #11545 - kylematsuda:secret-type, r=Eh2406

Wrapper type for data that should never be logged

Fixes #11519.

So far this is just creating the new wrapper type. If this looks okay, I'll start adding this wrapper in places where tokens and secret keys are held or passed.

src/bin/cargo/commands/login.rs

@@ -36,7 +36,7 @@ pub fn cli() -> Command {
 pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
     ops::registry_login(
         config,
-        args.get_one("token").map(String::as_str),
+        args.get_one::<String>("token").map(|s| s.as_str().into()),
         args.get_one("registry").map(String::as_str),
         args.flag("generate-keypair"),
         args.flag("secret-key"),

src/bin/cargo/commands/owner.rs

@@ -1,6 +1,7 @@
 use crate::command_prelude::*;
 
 use cargo::ops::{self, OwnersOptions};
+use cargo::util::auth::Secret;
 
 pub fn cli() -> Command {
     subcommand("owner")
@@ -34,7 +35,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
     let registry = args.registry(config)?;
     let opts = OwnersOptions {
         krate: args.get_one::<String>("crate").cloned(),
-        token: args.get_one::<String>("token").cloned(),
+        token: args.get_one::<String>("token").cloned().map(Secret::from),
         index: args.get_one::<String>("index").cloned(),
         to_add: args
             .get_many::<String>("add")

src/bin/cargo/commands/publish.rs

@@ -36,7 +36,9 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
         &ws,
         &PublishOpts {
             config,
-            token: args.get_one::<String>("token").map(|s| s.to_string()),
+            token: args
+                .get_one::<String>("token")
+                .map(|s| s.to_string().into()),
             index,
             verify: !args.flag("no-verify"),
             allow_dirty: args.flag("allow-dirty"),

src/bin/cargo/commands/yank.rs

@@ -1,6 +1,7 @@
 use crate::command_prelude::*;
 
 use cargo::ops;
+use cargo::util::auth::Secret;
 
 pub fn cli() -> Command {
     subcommand("yank")
@@ -37,7 +38,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
         config,
         krate.map(|s| s.to_string()),
         version.map(|s| s.to_string()),
-        args.get_one::<String>("token").cloned(),
+        args.get_one::<String>("token").cloned().map(Secret::from),
         args.get_one::<String>("index").cloned(),
         args.flag("undo"),
         registry,

src/cargo/ops/registry.rs

@@ -30,7 +30,7 @@ use crate::ops;
 use crate::ops::Packages;
 use crate::sources::{RegistrySource, SourceConfigMap, CRATES_IO_DOMAIN, CRATES_IO_REGISTRY};
 use crate::util::auth::{
-    paserk_public_from_paserk_secret, {self, AuthorizationError},
+    paserk_public_from_paserk_secret, Secret, {self, AuthorizationError},
 };
 use crate::util::config::{Config, SslVersionConfig, SslVersionConfigRange};
 use crate::util::errors::CargoResult;
@@ -45,11 +45,11 @@ use crate::{drop_print, drop_println, version};
 pub enum RegistryCredentialConfig {
     None,
     /// The authentication token.
-    Token(String),
+    Token(Secret<String>),
     /// Process used for fetching a token.
     Process((PathBuf, Vec<String>)),
     /// Secret Key and subject for Asymmetric tokens.
-    AsymmetricKey((String, Option<String>)),
+    AsymmetricKey((Secret<String>, Option<String>)),
 }
 
 impl RegistryCredentialConfig {
@@ -71,9 +71,9 @@ impl RegistryCredentialConfig {
     pub fn is_asymmetric_key(&self) -> bool {
         matches!(self, Self::AsymmetricKey(..))
     }
-    pub fn as_token(&self) -> Option<&str> {
+    pub fn as_token(&self) -> Option<Secret<&str>> {
         if let Self::Token(v) = self {
-            Some(&*v)
+            Some(v.as_deref())
         } else {
             None
         }
@@ -85,7 +85,7 @@ impl RegistryCredentialConfig {
             None
         }
     }
-    pub fn as_asymmetric_key(&self) -> Option<&(String, Option<String>)> {
+    pub fn as_asymmetric_key(&self) -> Option<&(Secret<String>, Option<String>)> {
         if let Self::AsymmetricKey(v) = self {
             Some(v)
         } else {
@@ -96,7 +96,7 @@ impl RegistryCredentialConfig {
 
 pub struct PublishOpts<'cfg> {
     pub config: &'cfg Config,
-    pub token: Option<String>,
+    pub token: Option<Secret<String>>,
     pub index: Option<String>,
     pub verify: bool,
     pub allow_dirty: bool,
@@ -174,7 +174,7 @@ pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> {
 
     let (mut registry, reg_ids) = registry(
         opts.config,
-        opts.token.as_deref(),
+        opts.token.as_ref().map(Secret::as_deref),
         opts.index.as_deref(),
         publish_registry.as_deref(),
         true,
@@ -512,7 +512,7 @@ fn wait_for_publish(
 /// * `token_required`: If `true`, the token will be set.
 fn registry(
     config: &Config,
-    token_from_cmdline: Option<&str>,
+    token_from_cmdline: Option<Secret<&str>>,
     index: Option<&str>,
     registry: Option<&str>,
     force_update: bool,
@@ -786,7 +786,7 @@ fn http_proxy_exists(config: &Config) -> CargoResult<bool> {
 
 pub fn registry_login(
     config: &Config,
-    token: Option<&str>,
+    token: Option<Secret<&str>>,
     reg: Option<&str>,
     generate_keypair: bool,
     secret_key_required: bool,
@@ -795,7 +795,7 @@ pub fn registry_login(
     let source_ids = get_source_id(config, None, reg)?;
     let reg_cfg = auth::registry_credential_config(config, &source_ids.original)?;
 
-    let login_url = match registry(config, token, None, reg, false, None) {
+    let login_url = match registry(config, token.clone(), None, reg, false, None) {
         Ok((registry, _)) => Some(format!("{}/me", registry.host())),
         Err(e) if e.is::<AuthorizationError>() => e
             .downcast::<AuthorizationError>()
@@ -830,29 +830,33 @@ pub fn registry_login(
             }
             _ => (None, None),
         };
-        let secret_key: String;
+        let secret_key: Secret<String>;
         if generate_keypair {
             assert!(!secret_key_required);
             let kp = AsymmetricKeyPair::<pasetors::version3::V3>::generate().unwrap();
-            let mut key = String::new();
+            secret_key = Secret::default().map(|mut key| {
-            FormatAsPaserk::fmt(&kp.secret, &mut key).unwrap();
+                FormatAsPaserk::fmt(&kp.secret, &mut key).unwrap();
-            secret_key = key;
+                key
+            });
         } else if secret_key_required {
             assert!(!generate_keypair);
             drop_println!(config, "please paste the API secret key below");
-            let mut line = String::new();
+            secret_key = Secret::default()
-            let input = io::stdin();
+                .map(|mut line| {
-            input
+                    let input = io::stdin();
-                .lock()
+                    input
-                .read_line(&mut line)
+                        .lock()
-                .with_context(|| "failed to read stdin")?;
+                        .read_line(&mut line)
-            secret_key = line.trim().to_string();
+                        .with_context(|| "failed to read stdin")
+                        .map(|_| line.trim().to_string())
+                })
+                .transpose()?;
         } else {
             secret_key = old_secret_key
                 .cloned()
                 .ok_or_else(|| anyhow!("need a secret_key to set a key_subject"))?;
         }
-        if let Some(p) = paserk_public_from_paserk_secret(&secret_key) {
+        if let Some(p) = paserk_public_from_paserk_secret(secret_key.as_deref()) {
             drop_println!(config, "{}", &p);
         } else {
             bail!("not a validly formatted PASERK secret key");
@@ -866,7 +870,7 @@ pub fn registry_login(
         ));
     } else {
         new_token = RegistryCredentialConfig::Token(match token {
-            Some(token) => token.to_string(),
+            Some(token) => token.owned(),
             None => {
                 if let Some(login_url) = login_url {
                     drop_println!(
@@ -890,7 +894,7 @@ pub fn registry_login(
                     .with_context(|| "failed to read stdin")?;
                 // Automatically remove `cargo login` from an inputted token to
                 // allow direct pastes from `registry.host()`/me.
-                line.replace("cargo login", "").trim().to_string()
+                Secret::from(line.replace("cargo login", "").trim().to_string())
             }
         });
 
@@ -938,7 +942,7 @@ pub fn registry_logout(config: &Config, reg: Option<&str>) -> CargoResult<()> {
 
 pub struct OwnersOptions {
     pub krate: Option<String>,
-    pub token: Option<String>,
+    pub token: Option<Secret<String>>,
     pub index: Option<String>,
     pub to_add: Option<Vec<String>>,
     pub to_remove: Option<Vec<String>>,
@@ -960,7 +964,7 @@ pub fn modify_owners(config: &Config, opts: &OwnersOptions) -> CargoResult<()> {
 
     let (mut registry, _) = registry(
         config,
-        opts.token.as_deref(),
+        opts.token.as_ref().map(Secret::as_deref),
         opts.index.as_deref(),
         opts.registry.as_deref(),
         true,
@@ -1019,7 +1023,7 @@ pub fn yank(
     config: &Config,
     krate: Option<String>,
     version: Option<String>,
-    token: Option<String>,
+    token: Option<Secret<String>>,
     index: Option<String>,
     undo: bool,
     reg: Option<String>,
@@ -1051,7 +1055,7 @@ pub fn yank(
 
     let (mut registry, _) = registry(
         config,
-        token.as_deref(),
+        token.as_ref().map(Secret::as_deref),
         index.as_deref(),
         reg.as_deref(),
         true,

src/cargo/util/auth.rs

@@ -10,6 +10,7 @@ use serde::Deserialize;
 use std::collections::HashMap;
 use std::error::Error;
 use std::io::{Read, Write};
+use std::ops::Deref;
 use std::path::PathBuf;
 use std::process::{Command, Stdio};
 use time::format_description::well_known::Rfc3339;
@@ -21,6 +22,102 @@ use crate::ops::RegistryCredentialConfig;
 
 use super::config::CredentialCacheValue;
 
+/// A wrapper for values that should not be printed.
+///
+/// This type does not implement `Display`, and has a `Debug` impl that hides
+/// the contained value.
+///
+/// ```
+/// # use cargo::util::auth::Secret;
+/// let token = Secret::from("super secret string");
+/// assert_eq!(format!("{:?}", token), "Secret { inner: \"REDACTED\" }");
+/// ```
+///
+/// Currently, we write a borrowed `Secret<T>` as `Secret<&T>`.
+/// The [`as_deref`](Secret::as_deref) and [`owned`](Secret::owned) methods can
+/// be used to convert back and forth between `Secret<String>` and `Secret<&str>`.
+#[derive(Default, Clone, PartialEq, Eq)]
+pub struct Secret<T> {
+    inner: T,
+}
+
+impl<T> Secret<T> {
+    /// Unwraps the contained value.
+    ///
+    /// Use of this method marks the boundary of where the contained value is
+    /// hidden.
+    pub fn expose(self) -> T {
+        self.inner
+    }
+
+    /// Converts a `Secret<T>` to a `Secret<&T::Target>`.
+    /// ```
+    /// # use cargo::util::auth::Secret;
+    /// let owned: Secret<String> = Secret::from(String::from("token"));
+    /// let borrowed: Secret<&str> = owned.as_deref();
+    /// ```
+    pub fn as_deref(&self) -> Secret<&<T as Deref>::Target>
+    where
+        T: Deref,
+    {
+        Secret::from(self.inner.deref())
+    }
+
+    /// Converts a `Secret<T>` to a `Secret<&T>`.
+    pub fn as_ref(&self) -> Secret<&T> {
+        Secret::from(&self.inner)
+    }
+
+    /// Converts a `Secret<T>` to a `Secret<U>` by applying `f` to the contained value.
+    pub fn map<U, F>(self, f: F) -> Secret<U>
+    where
+        F: FnOnce(T) -> U,
+    {
+        Secret::from(f(self.inner))
+    }
+}
+
+impl<T: ToOwned + ?Sized> Secret<&T> {
+    /// Converts a `Secret` containing a borrowed type to a `Secret` containing the
+    /// corresponding owned type.
+    /// ```
+    /// # use cargo::util::auth::Secret;
+    /// let borrowed: Secret<&str> = Secret::from("token");
+    /// let owned: Secret<String> = borrowed.owned();
+    /// ```
+    pub fn owned(&self) -> Secret<<T as ToOwned>::Owned> {
+        Secret::from(self.inner.to_owned())
+    }
+}
+
+impl<T, E> Secret<Result<T, E>> {
+    /// Converts a `Secret<Result<T, E>>` to a `Result<Secret<T>, E>`.
+    pub fn transpose(self) -> Result<Secret<T>, E> {
+        self.inner.map(|v| Secret::from(v))
+    }
+}
+
+impl<T: AsRef<str>> Secret<T> {
+    /// Checks if the contained value is empty.
+    pub fn is_empty(&self) -> bool {
+        self.inner.as_ref().is_empty()
+    }
+}
+
+impl<T> From<T> for Secret<T> {
+    fn from(inner: T) -> Self {
+        Self { inner }
+    }
+}
+
+impl<T> fmt::Debug for Secret<T> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("Secret")
+            .field("inner", &"REDACTED")
+            .finish()
+    }
+}
+
 /// Get the credential configuration for a `SourceId`.
 pub fn registry_credential_config(
     config: &Config,
@@ -61,9 +158,9 @@ pub fn registry_credential_config(
         return registry_credential_config_inner(
             true,
             None,
-            token,
+            token.map(Secret::from),
             credential_process,
-            secret_key,
+            secret_key.map(Secret::from),
             secret_key_subject,
             config,
         );
@@ -152,9 +249,9 @@ pub fn registry_credential_config(
     registry_credential_config_inner(
         false,
         name.as_deref(),
-        token,
+        token.map(Secret::from),
         credential_process,
-        secret_key,
+        secret_key.map(Secret::from),
         secret_key_subject,
         config,
     )
@@ -163,9 +260,9 @@ pub fn registry_credential_config(
 fn registry_credential_config_inner(
     is_crates_io: bool,
     name: Option<&str>,
-    token: Option<String>,
+    token: Option<Secret<String>>,
     credential_process: Option<config::PathAndArgs>,
-    secret_key: Option<String>,
+    secret_key: Option<Secret<String>>,
     secret_key_subject: Option<String>,
     config: &Config,
 ) -> CargoResult<RegistryCredentialConfig> {
@@ -298,14 +395,14 @@ my-registry = {{ index = "{}" }}
 }
 
 // Store a token in the cache for future calls.
-pub fn cache_token(config: &Config, sid: &SourceId, token: &str) {
+pub fn cache_token(config: &Config, sid: &SourceId, token: Secret<&str>) {
     let url = sid.canonical_url();
     config.credential_cache().insert(
         url.clone(),
         CredentialCacheValue {
             from_commandline: true,
             independent_of_endpoint: true,
-            token_value: token.to_string(),
+            token_value: token.owned(),
         },
     );
 }
@@ -320,7 +417,7 @@ pub fn auth_token(
     mutation: Option<Mutation<'_>>,
 ) -> CargoResult<String> {
     match auth_token_optional(config, sid, mutation.as_ref())? {
-        Some(token) => Ok(token),
+        Some(token) => Ok(token.expose()),
         None => Err(AuthorizationError {
             sid: sid.clone(),
             login_url: login_url.cloned(),
@@ -335,7 +432,7 @@ fn auth_token_optional(
     config: &Config,
     sid: &SourceId,
     mutation: Option<&'_ Mutation<'_>>,
-) -> CargoResult<Option<String>> {
+) -> CargoResult<Option<Secret<String>>> {
     let mut cache = config.credential_cache();
     let url = sid.canonical_url();
 
@@ -353,15 +450,21 @@ fn auth_token_optional(
     let credential = registry_credential_config(config, sid)?;
     let (independent_of_endpoint, token) = match credential {
         RegistryCredentialConfig::None => return Ok(None),
-        RegistryCredentialConfig::Token(config_token) => (true, config_token.to_string()),
+        RegistryCredentialConfig::Token(config_token) => (true, config_token),
         RegistryCredentialConfig::Process(process) => {
             // todo: PASETO with process
-            run_command(config, &process, sid, Action::Get)?.unwrap()
+            let (independent_of_endpoint, token) =
+                run_command(config, &process, sid, Action::Get)?.unwrap();
+            (independent_of_endpoint, Secret::from(token))
         }
         RegistryCredentialConfig::AsymmetricKey((secret_key, secret_key_subject)) => {
-            let secret: AsymmetricSecretKey<pasetors::version3::V3> =
+            let secret: Secret<AsymmetricSecretKey<pasetors::version3::V3>> =
-                secret_key.as_str().try_into()?;
+                secret_key.map(|key| key.as_str().try_into()).transpose()?;
-            let public: AsymmetricPublicKey<pasetors::version3::V3> = (&secret).try_into()?;
+            let public: AsymmetricPublicKey<pasetors::version3::V3> = secret
+                .as_ref()
+                .map(|key| key.try_into())
+                .transpose()?
+                .expose();
             let kip: pasetors::paserk::Id = (&public).try_into()?;
             let iat = OffsetDateTime::now_utc();
 
@@ -413,18 +516,22 @@ fn auth_token_optional(
 
             (
                 false,
-                pasetors::version3::PublicToken::sign(
+                secret
-                    &secret,
+                    .map(|secret| {
-                    serde_json::to_string(&message)
+                        pasetors::version3::PublicToken::sign(
-                        .expect("cannot serialize")
+                            &secret,
-                        .as_bytes(),
+                            serde_json::to_string(&message)
-                    Some(
+                                .expect("cannot serialize")
-                        serde_json::to_string(&footer)
+                                .as_bytes(),
-                            .expect("cannot serialize")
+                            Some(
-                            .as_bytes(),
+                                serde_json::to_string(&footer)
-                    ),
+                                    .expect("cannot serialize")
-                    None,
+                                    .as_bytes(),
-                )?,
+                            ),
+                            None,
+                        )
+                    })
+                    .transpose()?,
             )
         }
     };
@@ -435,7 +542,7 @@ fn auth_token_optional(
             CredentialCacheValue {
                 from_commandline: false,
                 independent_of_endpoint,
-                token_value: token.to_string(),
+                token_value: token.clone(),
             },
         );
     }
@@ -519,6 +626,7 @@ pub fn login(config: &Config, sid: &SourceId, token: RegistryCredentialConfig) -
             let token = token
                 .as_token()
                 .expect("credential_process cannot use login with a secret_key")
+                .expose()
                 .to_owned();
             run_command(config, &process, sid, Action::Store(token))?;
         }
@@ -530,9 +638,15 @@ pub fn login(config: &Config, sid: &SourceId, token: RegistryCredentialConfig) -
 }
 
 /// Checks that a secret key is valid, and returns the associated public key in Paserk format.
-pub(crate) fn paserk_public_from_paserk_secret(secret_key: &str) -> Option<String> {
+pub(crate) fn paserk_public_from_paserk_secret(secret_key: Secret<&str>) -> Option<String> {
-    let secret: AsymmetricSecretKey<pasetors::version3::V3> = secret_key.try_into().ok()?;
+    let secret: Secret<AsymmetricSecretKey<pasetors::version3::V3>> =
-    let public: AsymmetricPublicKey<pasetors::version3::V3> = (&secret).try_into().ok()?;
+        secret_key.map(|key| key.try_into()).transpose().ok()?;
+    let public: AsymmetricPublicKey<pasetors::version3::V3> = secret
+        .as_ref()
+        .map(|key| key.try_into())
+        .transpose()
+        .ok()?
+        .expose();
     let mut paserk_pub_key = String::new();
     FormatAsPaserk::fmt(&public, &mut paserk_pub_key).unwrap();
     Some(paserk_pub_key)

src/cargo/util/config/mod.rs

@@ -70,6 +70,7 @@ use crate::core::compiler::rustdoc::RustdocExternMap;
 use crate::core::shell::Verbosity;
 use crate::core::{features, CliUnstable, Shell, SourceId, Workspace, WorkspaceRootConfig};
 use crate::ops::{self, RegistryCredentialConfig};
+use crate::util::auth::Secret;
 use crate::util::errors::CargoResult;
 use crate::util::validate_package_name;
 use crate::util::CanonicalUrl;
@@ -137,6 +138,7 @@ enum WhyLoad {
 }
 
 /// A previously generated authentication token and the data needed to determine if it can be reused.
+#[derive(Debug)]
 pub struct CredentialCacheValue {
     /// If the command line was used to override the token then it must always be reused,
     /// even if reading the configuration files would lead to a different value.
@@ -144,17 +146,7 @@ pub struct CredentialCacheValue {
     /// If nothing depends on which endpoint is being hit, then we can reuse the token
     /// for any future request even if some of the requests involve mutations.
     pub independent_of_endpoint: bool,
-    pub token_value: String,
+    pub token_value: Secret<String>,
-}
-
-impl fmt::Debug for CredentialCacheValue {
-    /// This manual implementation helps ensure that the token value is redacted from all logs.
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        f.debug_struct("CredentialCacheValue")
-            .field("from_commandline", &self.from_commandline)
-            .field("token_value", &"REDACTED")
-            .finish()
-    }
 }
 
 /// Configuration information for cargo. This is not specific to a build, it is information
@@ -2191,7 +2183,7 @@ pub fn save_credentials(
                 // login with token
 
                 let key = "token".to_string();
-                let value = ConfigValue::String(token, path_def.clone());
+                let value = ConfigValue::String(token.expose(), path_def.clone());
                 let map = HashMap::from([(key, value)]);
                 let table = CV::Table(map, path_def.clone());
 
@@ -2206,7 +2198,7 @@ pub fn save_credentials(
                 // login with key
 
                 let key = "secret-key".to_string();
-                let value = ConfigValue::String(secret_key, path_def.clone());
+                let value = ConfigValue::String(secret_key.expose(), path_def.clone());
                 let mut map = HashMap::from([(key, value)]);
                 if let Some(key_subject) = key_subject {
                     let key = "secret-key-subject".to_string();