merge: #3281

3281: chore: prevent credentials & secrets from display in CLI parsing r=fnichol a=fnichol This change is a tweak to #3257 which has the same goal of not accidentally logging sensitive values such as keys, certificates, etc. In this version, we're using our `si_std::SensitiveString` type which has overridden `Display` and `Debug` trait implementations. So when a `debug!()` or `trace!()` call prints the contents of our `Args` CLI type, we won't see the contents of certain "sensitive" fields. <img src="https://media3.giphy.com/media/hj8eOHrXqoLntsCyWz/giphy.gif"/> Co-authored-by: Fletcher Nichol <[email protected]>
systeminit · Feb 7, 2024 · 170af4a · 170af4a
2 parents a69b546 + fcff969
commit 170af4a
Show file tree

Hide file tree

Showing 22 changed files with 103 additions and 64 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/bin/council/BUCK b/bin/council/BUCK
@@ -9,6 +9,7 @@ rust_binary(
     name = "council",
     deps = [
         "//lib/council-server:council-server",
+        "//lib/si-std:si-std",
         "//lib/telemetry-application-rs:telemetry-application",
         "//third-party/rust:clap",
         "//third-party/rust:color-eyre",

diff --git a/bin/council/Cargo.toml b/bin/council/Cargo.toml
@@ -13,6 +13,7 @@ path = "src/main.rs"
 clap = { workspace = true }
 color-eyre = { workspace = true }
 council-server = { path = "../../lib/council-server" }
+si-std = { path = "../../lib/si-std" }
 telemetry-application = { path = "../../lib/telemetry-application-rs" }
 tokio = { workspace = true }
 tokio-util = { workspace = true }
diff --git a/bin/council/src/args.rs b/bin/council/src/args.rs
@@ -1,6 +1,9 @@
+use std::path::PathBuf;
+
 use clap::{ArgAction, Parser};
 
 use council_server::server::config::{Config, ConfigError, ConfigFile, StandardConfigFile};
+use si_std::SensitiveString;
 
 const NAME: &str = "council";
 
@@ -60,11 +63,11 @@ pub(crate) struct Args {
 
     /// NATS credentials string
     #[arg(long, allow_hyphen_values = true)]
-    pub(crate) nats_creds: Option<String>,
+    pub(crate) nats_creds: Option<SensitiveString>,
 
     /// NATS credentials file
     #[arg(long)]
-    pub(crate) nats_creds_path: Option<String>,
+    pub(crate) nats_creds_path: Option<PathBuf>,
 }
 
 impl TryFrom<Args> for Config {
@@ -76,10 +79,10 @@ impl TryFrom<Args> for Config {
                 config_map.set("nats.url", url);
             }
             if let Some(creds) = args.nats_creds {
-                config_map.set("nats.creds", creds);
+                config_map.set("nats.creds", creds.to_string());
             }
             if let Some(creds_file) = args.nats_creds_path {
-                config_map.set("nats.creds_file", creds_file);
+                config_map.set("nats.creds_file", creds_file.display().to_string());
             }
             config_map.set("nats.connection_name", NAME);
         })?

diff --git a/bin/council/src/main.rs b/bin/council/src/main.rs
@@ -50,7 +50,7 @@ async fn async_main() -> Result<()> {
             .set_verbosity_and_wait(args.verbose.into())
             .await?;
     }
-    trace!(arguments =?args, "parsed cli arguments");
+    debug!(arguments =?args, "parsed cli arguments");
 
     let (_shutdown_request_tx, shutdown_request_rx) = watch::channel(());
 

diff --git a/bin/cyclone/src/main.rs b/bin/cyclone/src/main.rs
@@ -45,7 +45,7 @@ async fn main() -> Result<()> {
             .set_verbosity_and_wait(args.verbose.into())
             .await?;
     }
-    trace!(arguments =?args, "parsed cli arguments");
+    debug!(arguments =?args, "parsed cli arguments");
 
     let decryption_key = Server::load_decryption_key(&args.decryption_key).await?;
 

diff --git a/bin/module-index/BUCK b/bin/module-index/BUCK
@@ -9,6 +9,7 @@ rust_binary(
     name = "module-index",
     deps = [
         "//lib/module-index-server:module-index-server",
+        "//lib/si-std:si-std",
         "//lib/telemetry-application-rs:telemetry-application",
         "//third-party/rust:clap",
         "//third-party/rust:color-eyre",

diff --git a/bin/module-index/Cargo.toml b/bin/module-index/Cargo.toml
@@ -13,6 +13,7 @@ path = "src/main.rs"
 clap = { workspace = true }
 color-eyre = { workspace = true }
 module-index-server = { path = "../../lib/module-index-server" }
+si-std = { path = "../../lib/si-std" }
 telemetry-application = { path = "../../lib/telemetry-application-rs" }
 tokio = { workspace = true }
 tokio-util = { workspace = true }
diff --git a/bin/module-index/src/args.rs b/bin/module-index/src/args.rs
@@ -1,5 +1,8 @@
+use std::path::PathBuf;
+
 use clap::{ArgAction, Parser};
 use module_index_server::{Config, ConfigError, ConfigFile, StandardConfigFile};
+use si_std::SensitiveString;
 
 const NAME: &str = "module_index";
 
@@ -78,23 +81,23 @@ pub(crate) struct Args {
 
     /// PostgreSQL connection pool password [example: dbuser]
     #[arg(long, env)]
-    pub(crate) pg_password: Option<String>,
+    pub(crate) pg_password: Option<SensitiveString>,
 
     /// PostgreSQL connection certification path
     #[arg(long)]
-    pub(crate) pg_cert_path: Option<String>,
+    pub(crate) pg_cert_path: Option<PathBuf>,
 
     /// PostgreSQL connection certification base64 string
     #[arg(long)]
-    pub(crate) pg_cert_base64: Option<String>,
+    pub(crate) pg_cert_base64: Option<SensitiveString>,
 
     /// The address and port to bind the HTTP server to [example: 0.0.0.0:80]
     #[arg(long, env)]
     pub(crate) socket_addr: Option<String>,
 
     /// The s3 bucket access key id
     #[arg(long, env)]
-    pub(crate) s3_access_key_id: Option<String>,
+    pub(crate) s3_access_key_id: Option<SensitiveString>,
 
     /// The s3 bucket
     #[arg(long, env)]
@@ -106,15 +109,15 @@ pub(crate) struct Args {
 
     /// The s3 bucket secret access key
     #[arg(long, env)]
-    pub(crate) s3_secret_access_key: Option<String>,
+    pub(crate) s3_secret_access_key: Option<SensitiveString>,
 
     /// The s3 bucket path prefix
     #[arg(long, env)]
     pub(crate) s3_path_prefix: Option<String>,
 
     /// The path to the JWT public signing key
     #[arg(long, env)]
-    pub(crate) jwt_public_key: Option<String>,
+    pub(crate) jwt_public_key: Option<SensitiveString>,
     // /// Database migration mode on startup
     // #[arg(long, value_parser = PossibleValuesParser::new(MigrationMode::variants()))]
 }
@@ -140,23 +143,23 @@ impl TryFrom<Args> for Config {
                 config_map.set("pg.user", user);
             }
             if let Some(password) = args.pg_password {
-                config_map.set("pg.password", password);
+                config_map.set("pg.password", password.to_string());
             }
-            if let Some(cert) = args.pg_cert_path {
-                config_map.set("pg.certificate_path", cert);
+            if let Some(cert_path) = args.pg_cert_path {
+                config_map.set("pg.certificate_path", cert_path.display().to_string());
             }
             if let Some(cert) = args.pg_cert_base64 {
-                config_map.set("pg.certificate_base64", cert);
+                config_map.set("pg.certificate_base64", cert.to_string());
             }
             if let Some(socket_addr) = args.socket_addr {
                 config_map.set("socket_addr", socket_addr);
             }
 
             if let Some(s3_access_key_id) = args.s3_access_key_id {
-                config_map.set("s3.access_key_id", s3_access_key_id);
+                config_map.set("s3.access_key_id", s3_access_key_id.to_string());
             }
             if let Some(s3_secret_access_key) = args.s3_secret_access_key {
-                config_map.set("s3.secret_access_key", s3_secret_access_key);
+                config_map.set("s3.secret_access_key", s3_secret_access_key.to_string());
             }
             if let Some(s3_bucket) = args.s3_bucket {
                 config_map.set("s3.bucket", s3_bucket);
@@ -168,7 +171,7 @@ impl TryFrom<Args> for Config {
                 config_map.set("s3.path_prefix", s3_path_prefix);
             }
             if let Some(jwt_public_key) = args.jwt_public_key {
-                config_map.set("jwt_signing_public_key_path", jwt_public_key);
+                config_map.set("jwt_signing_public_key_path", jwt_public_key.to_string());
             }
 
             // if let Some(migration_mode) = args.migration_mode {

diff --git a/bin/module-index/src/main.rs b/bin/module-index/src/main.rs
@@ -50,7 +50,7 @@ async fn async_main() -> Result<()> {
             .set_verbosity_and_wait(args.verbose.into())
             .await?;
     }
-    trace!(arguments =?args, "parsed cli arguments");
+    debug!(arguments =?args, "parsed cli arguments");
 
     let config = Config::try_from(args)?;
 

diff --git a/bin/pinga/BUCK b/bin/pinga/BUCK
@@ -9,6 +9,7 @@ rust_binary(
     name = "pinga",
     deps = [
         "//lib/pinga-server:pinga-server",
+        "//lib/si-std:si-std",
         "//lib/telemetry-application-rs:telemetry-application",
         "//third-party/rust:clap",
         "//third-party/rust:color-eyre",

diff --git a/bin/pinga/Cargo.toml b/bin/pinga/Cargo.toml
@@ -13,6 +13,7 @@ path = "src/main.rs"
 clap = { workspace = true }
 color-eyre = { workspace = true }
 pinga-server = { path = "../../lib/pinga-server" }
+si-std = { path = "../../lib/si-std" }
 telemetry-application = { path = "../../lib/telemetry-application-rs" }
 tokio = { workspace = true }
 tokio-util = { workspace = true }
diff --git a/bin/pinga/src/args.rs b/bin/pinga/src/args.rs
@@ -1,5 +1,8 @@
+use std::path::PathBuf;
+
 use clap::{ArgAction, Parser};
 use pinga_server::{Config, ConfigError, ConfigFile, StandardConfigFile};
+use si_std::SensitiveString;
 
 const NAME: &str = "pinga";
 
@@ -78,35 +81,35 @@ pub(crate) struct Args {
 
     /// PostgreSQL connection certification path
     #[arg(long)]
-    pub(crate) pg_cert_path: Option<String>,
+    pub(crate) pg_cert_path: Option<PathBuf>,
 
     /// PostgreSQL connection certification base64 string
     #[arg(long)]
-    pub(crate) pg_cert_base64: Option<String>,
+    pub(crate) pg_cert_base64: Option<SensitiveString>,
 
     /// NATS connection URL [example: demo.nats.io]
     #[arg(long)]
     pub(crate) nats_url: Option<String>,
 
     /// NATS credentials string
     #[arg(long, allow_hyphen_values = true)]
-    pub(crate) nats_creds: Option<String>,
+    pub(crate) nats_creds: Option<SensitiveString>,
 
     /// NATS credentials file
     #[arg(long)]
-    pub(crate) nats_creds_path: Option<String>,
+    pub(crate) nats_creds_path: Option<PathBuf>,
 
     /// Cyclone encryption key file location [default: /run/pinga/cyclone_encryption.key]
     #[arg(long)]
-    pub(crate) cyclone_encryption_key_path: Option<String>,
+    pub(crate) cyclone_encryption_key_path: Option<PathBuf>,
 
     /// Cyclone encryption key file contents as a base64 encoded string
     #[arg(long)]
-    pub(crate) cyclone_encryption_key_base64: Option<String>,
+    pub(crate) cyclone_encryption_key_base64: Option<SensitiveString>,
 
     /// Cyclone secret key as base64 string
     #[arg(long)]
-    pub(crate) cyclone_secret_key_base64: Option<String>,
+    pub(crate) cyclone_secret_key_base64: Option<SensitiveString>,
 
     /// The number of concurrent jobs that can be processed [default: 10]
     #[arg(long)]
@@ -140,32 +143,38 @@ impl TryFrom<Args> for Config {
             if let Some(user) = args.pg_user {
                 config_map.set("pg.user", user);
             }
-            if let Some(cert) = args.pg_cert_path {
-                config_map.set("pg.certificate_path", cert);
+            if let Some(cert_path) = args.pg_cert_path {
+                config_map.set("pg.certificate_path", cert_path.display().to_string());
             }
             if let Some(cert) = args.pg_cert_base64 {
-                config_map.set("pg.certificate_base64", cert);
+                config_map.set("pg.certificate_base64", cert.to_string());
             }
             if let Some(url) = args.nats_url {
                 config_map.set("nats.url", url);
             }
             if let Some(creds) = args.nats_creds {
-                config_map.set("nats.creds", creds);
+                config_map.set("nats.creds", creds.to_string());
             }
-            if let Some(creds_file) = args.nats_creds_path {
-                config_map.set("nats.creds_file", creds_file);
+            if let Some(creds_path) = args.nats_creds_path {
+                config_map.set("nats.creds_file", creds_path.display().to_string());
             }
             if let Some(cyclone_encryption_key_file) = args.cyclone_encryption_key_path {
-                config_map.set("crypto.encryption_key_file", cyclone_encryption_key_file);
+                config_map.set(
+                    "crypto.encryption_key_file",
+                    cyclone_encryption_key_file.display().to_string(),
+                );
             }
             if let Some(cyclone_encryption_key_base64) = args.cyclone_encryption_key_base64 {
                 config_map.set(
                     "crypto.encryption_key_base64",
-                    cyclone_encryption_key_base64,
+                    cyclone_encryption_key_base64.to_string(),
                 );
             }
             if let Some(secret_string) = args.cyclone_secret_key_base64 {
-                config_map.set("symmetric_crypto_service.active_key_base64", secret_string);
+                config_map.set(
+                    "symmetric_crypto_service.active_key_base64",
+                    secret_string.to_string(),
+                );
             }
             if let Some(concurrency) = args.concurrency {
                 config_map.set("concurrency_limit", i64::from(concurrency));

diff --git a/bin/pinga/src/main.rs b/bin/pinga/src/main.rs
@@ -50,7 +50,7 @@ async fn async_main() -> Result<()> {
             .set_verbosity_and_wait(args.verbose.into())
             .await?;
     }
-    trace!(arguments =?args, "parsed cli arguments");
+    debug!(arguments =?args, "parsed cli arguments");
 
     let config = Config::try_from(args)?;
 

diff --git a/bin/sdf/BUCK b/bin/sdf/BUCK
@@ -10,6 +10,7 @@ rust_binary(
     deps = [
         "//lib/nats-multiplexer:nats-multiplexer",
         "//lib/sdf-server:sdf-server",
+        "//lib/si-std:si-std",
         "//lib/telemetry-application-rs:telemetry-application",
         "//third-party/rust:clap",
         "//third-party/rust:color-eyre",

diff --git a/bin/sdf/Cargo.toml b/bin/sdf/Cargo.toml
@@ -10,11 +10,11 @@ name = "sdf"
 path = "src/main.rs"
 
 [dependencies]
+clap = { workspace = true }
+color-eyre = { workspace = true }
 nats-multiplexer = { path = "../../lib/nats-multiplexer" }
 sdf-server = { path = "../../lib/sdf-server" }
+si-std = { path = "../../lib/si-std" }
 telemetry-application = { path = "../../lib/telemetry-application-rs" }
-
-clap = { workspace = true }
-color-eyre = { workspace = true }
 tokio = { workspace = true }
 tokio-util = { workspace = true }