fix: use secret wrapper type for secrets in config

Author: matt-codecov

Cargo.lock

@@ -21,6 +21,7 @@ sentry = { version = "0.41.0", features = [
     "tracing",
     "logs",
 ] }
+secrecy = { version = "0.10.3", features = ["serde"] }
 serde = { workspace = true }
 serde_json = { workspace = true }
 tokio = { workspace = true, features = ["full"] }

objectstore-server/Cargo.toml

@@ -1,14 +1,49 @@
 use std::collections::BTreeMap;
+use std::fmt;
 use std::net::SocketAddr;
 use std::path::PathBuf;
 
 use anyhow::Result;
 use argh::FromArgs;
 use figment::providers::{Env, Format, Serialized, Yaml};
+use secrecy::{CloneableSecret, SecretBox, SerializableSecret, zeroize::Zeroize};
 use serde::{Deserialize, Serialize};
 
 const ENV_PREFIX: &str = "FSS_";
 
+/// Newtype around `String` that may protect against accidental
+/// logging of secrets in our configuration struct. Use with
+/// [`secrecy::SecretBox`].
+#[derive(Clone, Default, Serialize, Deserialize, PartialEq)]
+pub(crate) struct ConfigSecret(String);
+
+impl ConfigSecret {
+    pub fn as_str(&self) -> &str {
+        self.0.as_str()
+    }
+}
+
+impl std::ops::Deref for ConfigSecret {
+    type Target = String;
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl fmt::Debug for ConfigSecret {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
+        write!(f, "[redacted]")
+    }
+}
+
+impl CloneableSecret for ConfigSecret {}
+impl SerializableSecret for ConfigSecret {}
+impl Zeroize for ConfigSecret {
+    fn zeroize(&mut self) {
+        self.0.zeroize();
+    }
+}
+
 #[derive(Debug, Clone, Deserialize, Serialize)]
 #[serde(tag = "type", rename_all = "lowercase")]
 pub enum Storage {
@@ -33,7 +68,7 @@ pub enum Storage {
 
 #[derive(Debug, Clone, Deserialize, Serialize)]
 pub struct Sentry {
-    pub dsn: String,
+    pub dsn: SecretBox<ConfigSecret>,
     pub sample_rate: Option<f32>,
     pub traces_sample_rate: Option<f32>,
 }
@@ -49,7 +84,7 @@ pub struct Config {
 
     // others
     pub sentry: Option<Sentry>,
-    pub datadog_key: Option<String>,
+    pub datadog_key: Option<SecretBox<ConfigSecret>>,
     pub metric_tags: BTreeMap<String, String>,
 }
 
@@ -103,8 +138,16 @@ pub struct Args {
 mod tests {
     use std::io::Write;
 
+    use secrecy::ExposeSecret;
+
     use super::*;
 
+    impl PartialEq<str> for ConfigSecret {
+        fn eq(&self, other: &str) -> bool {
+            self.0 == other
+        }
+    }
+
     #[test]
     fn configurable_via_env() {
         figment::Jail::expect_with(|jail| {
@@ -135,7 +178,7 @@ mod tests {
                 sample_rate,
                 traces_sample_rate,
             } = &dbg!(&config).sentry.as_ref().unwrap();
-            assert_eq!(dsn, "abcde");
+            assert_eq!(dsn.expose_secret(), "abcde");
             assert_eq!(sample_rate, &Some(0.5));
             assert_eq!(traces_sample_rate, &Some(0.5));
 
@@ -177,8 +220,43 @@ mod tests {
             sample_rate,
             traces_sample_rate,
         } = &dbg!(&config).sentry.as_ref().unwrap();
-        assert_eq!(dsn, "abcde");
+        assert_eq!(dsn.expose_secret(), "abcde");
         assert_eq!(sample_rate, &Some(0.5));
         assert_eq!(traces_sample_rate, &Some(0.5));
     }
+
+    #[test]
+    fn configured_with_env_and_yaml() {
+        let mut tempfile = tempfile::NamedTempFile::new().unwrap();
+        tempfile
+            .write_all(
+                br#"
+            long_term_storage:
+                type: s3compatible
+                endpoint: http://localhost:8888
+                bucket: whatever
+            "#,
+            )
+            .unwrap();
+        figment::Jail::expect_with(|jail| {
+            jail.set_env("fss_long_term_storage__endpoint", "http://localhost:9001");
+
+            let args = Args {
+                config: Some(tempfile.path().into()),
+            };
+            let config = Config::from_args(args).unwrap();
+
+            let Storage::S3Compatible {
+                endpoint,
+                bucket: _bucket,
+            } = &dbg!(&config).long_term_storage
+            else {
+                panic!("expected s3 storage");
+            };
+            // Env should overwrite the yaml config
+            assert_eq!(endpoint, "http://localhost:9001");
+
+            Ok(())
+        });
+    }
 }

objectstore-server/src/config.rs

@@ -1,5 +1,6 @@
 use std::env;
 
+use secrecy::ExposeSecret;
 use sentry::integrations::tracing as sentry_tracing;
 use tracing::Level;
 use tracing::level_filters::LevelFilter;
@@ -12,7 +13,8 @@ pub fn maybe_initialize_metrics(config: &Config) -> std::io::Result<Option<merni
         .datadog_key
         .as_ref()
         .map(|api_key| {
-            let mut builder = merni::datadog(api_key.as_str()).prefix("objectstore.");
+            let mut builder =
+                merni::datadog(api_key.expose_secret().as_str()).prefix("objectstore.");
             for (k, v) in &config.metric_tags {
                 builder = builder.global_tag(k, v);
             }
@@ -24,7 +26,7 @@ pub fn maybe_initialize_metrics(config: &Config) -> std::io::Result<Option<merni
 pub fn maybe_initialize_sentry(config: &Config) -> Option<sentry::ClientInitGuard> {
     config.sentry.as_ref().map(|sentry_config| {
         sentry::init(sentry::ClientOptions {
-            dsn: sentry_config.dsn.parse().ok(),
+            dsn: sentry_config.dsn.expose_secret().parse().ok(),
             enable_logs: true,
             sample_rate: sentry_config.sample_rate.unwrap_or(1.0),
             traces_sample_rate: sentry_config.traces_sample_rate.unwrap_or(0.01),