Merge pull request #57 from PierreBeucher/check-tty-stdout

feat: check stdout is a tty to avoid secret leak

Cargo.toml

@@ -43,6 +43,7 @@ sha2 = "0.10.6"
 digest = "0.10.6"
 home = "0.5.5"
 google-secretmanager1 = "5.0.2"
+is-terminal = "0.4.9"
 
 [dependencies.uuid]
 version = "1.1.2"
@@ -53,4 +54,4 @@ features = [
 ]
 
 [dev-dependencies]
-pretty_assertions = "1.3.0"
+pretty_assertions = "1.3.0"

src/lib.rs

@@ -4,7 +4,7 @@ pub mod modules;
 use crate::core::{ResolveTo, NovopsEnvironmentInput, NovopsConfigFile, NovopsContext};
 use crate::modules::files::FileOutput;
 use crate::modules::variables::VariableOutput;
-use log::{info, debug};
+use log::{info, debug, error};
 
 use std::os::linux::fs::MetadataExt;
 use std::os::unix::prelude::OpenOptionsExt;
@@ -14,6 +14,7 @@ use anyhow::{self, Context};
 use std::os::unix;
 use std::path::PathBuf;
 use std::env;
+use is_terminal::IsTerminal; // todo use std::io::IsTerminal; rust 1.71+
 use std::collections::HashMap;
 use schemars::schema_for;
 use std::process::Command;
@@ -45,23 +46,26 @@ pub struct NovopsOutputs {
     pub files: HashMap<String, FileOutput>
 }
 
-pub async fn load_environment_write_vars(args: &NovopsLoadArgs, symlink: &Option<String>, format: &str) -> Result<(), anyhow::Error> {
+/// Load environment and write variables to stdout or file
+/// Checks if stdout is tty for safety to avoid showing secrets on screen
+pub async fn load_environment_write_vars(args: &NovopsLoadArgs, symlink: &Option<String>, format: &str, skip_tty_check: bool) -> Result<(), anyhow::Error> {
+
+    // safety checks
+    check_stdout_tty_and_exit(&skip_tty_check);
 
     let outputs = load_environment_no_write_vars(&args).await?;
 
     let voutputs: Vec<VariableOutput> = outputs.variables.clone().into_iter().map(|(_, v)| v).collect();
     export_variable_outputs(format, symlink, &voutputs, &outputs.context.workdir)?;
 
-    info!("Novops environment loaded ! Export variables with:");
-    info!("  source {:?}", &outputs.context.env_var_filepath);
+    info!("Novops environment loaded !");
 
     Ok(())
 }
 
 pub async fn load_environment_no_write_vars(args: &NovopsLoadArgs) -> Result<NovopsOutputs, anyhow::Error> {
 
     init_logger();
-
     // Read config from args and resolve all inputs to their concrete outputs
     let outputs = load_context_and_resolve(&args).await?;
 
@@ -595,4 +599,15 @@ pub fn get_config_schema() -> Result<String, anyhow::Error>{
     let output = serde_json::to_string_pretty(&schema)
         .with_context(|| "Couldn't convert JSON schema to pretty string.")?;
     return Ok(output)
+}
+
+/// Check if stdout is a tty
+/// Exit with non-0 if true to avoid showing secrets on console
+fn check_stdout_tty_and_exit(skip_tty_check: &bool) {
+    if !skip_tty_check && std::io::stdout().is_terminal() {
+        error!("Stdout is a terminal, secrets won't be loaded to avoid exposing them. \
+            Either use 'novops load' with -s/--symlink to write in a secure directory, 'source<(novops load [OPTIONS]) or 'novops run' to load secrets directly in process memory. \
+            If you really want to show secrets on terminal, use --skip-tty-check to skip this check.");
+        std::process::exit(3)
+    }
 }

src/main.rs

@@ -67,6 +67,13 @@ fn build_cli() -> Command {
                 .value_name("FORMAT")
                 .default_value("dotenv-export")
             )
+            .arg(Arg::new("skip_tty_check")
+                .help("Do not check if stdout is a tty (terminal), risking exposing secrets on screen. This is unsecure.")
+                .long("skip-tty-check")
+                .value_name("DRY_RUN")
+                .action(ArgAction::SetTrue)
+                .required(false)
+            )
         )
         .subcommand(
             Command::new("run")
@@ -129,6 +136,9 @@ async fn main() -> Result<(), anyhow::Error> {
         let env_format = load_subc.get_one::<String>("format")
             .ok_or(anyhow::anyhow!("Format is None. This is probably a bug as CLI defines default value."))?.clone();
 
+        let skip_tty_check = load_subc.get_one::<bool>("skip_tty_check").map(|e| *e)
+            .ok_or(anyhow::anyhow!("skip_tty_check is None. This is probably a bug as CLI defines default value."))?.clone();
+
         let args = novops::NovopsLoadArgs{ 
             config: load_subc.get_one::<String>("config")
                 .ok_or(anyhow::anyhow!("Config is None. This is probably a bug as CLI defines default value."))?.clone(),
@@ -137,7 +147,7 @@ async fn main() -> Result<(), anyhow::Error> {
             dry_run: load_subc.get_one::<bool>("dry_run").map(|e| *e)
         };
 
-        novops::load_environment_write_vars(&args, &symlink, &env_format).await
+        novops::load_environment_write_vars(&args, &symlink, &env_format, skip_tty_check).await
             .with_context(|| "Failed to load environment. Set environment variable RUST_LOG=[trace|debug|info|warn] or RUST_BACKTRACE=1 for more verbosity.")?;
 
         exit(0)

tests/test_core.rs

@@ -87,7 +87,8 @@ mod tests {
                 dry_run: None
             },
             &Some(String::from(".envrc")),
-            &String::from("dotenv-export")
+            &String::from("dotenv-export"),
+            true
         ).await?;   
 
         let expected_var_file = PathBuf::from(&workdir).join("vars");
@@ -138,7 +139,8 @@ mod tests {
                 dry_run: None
             },
             &Some(expect_symlink_at.clone().into_os_string().into_string().unwrap()),
-            &String::from("dotenv-export")
+            &String::from("dotenv-export"),
+            true
         ).await?;
 
         let symlink_metadata = fs::symlink_metadata(&expect_symlink_at)?;
@@ -159,6 +161,7 @@ mod tests {
             },
             &Some(expect_symlink_at.clone().into_os_string().into_string().unwrap()),
             &String::from("dotenv-export"),
+            true
         ).await?;
 
         let overriden_symlink_dest = fs::read_link(&expect_symlink_at).unwrap();
@@ -189,6 +192,7 @@ mod tests {
             }, 
             &Some(symlink_path.clone().into_os_string().into_string().unwrap()),
             &String::from("dotenv-export"),
+            true
         ).await;
 
         result.expect_err("Expected an error when loading with symlink trying to override existing file, got OK.");