From 2635cd701cad5c63f723877df46a1461bd483b58 Mon Sep 17 00:00:00 2001 From: Benjamin Chastanier Date: Wed, 30 Mar 2022 17:07:25 +0200 Subject: [PATCH] fix: avoid leaking details on Dbg print Ticket: ENG-1156 --- Cargo.lock | 12 +++++++++ Cargo.toml | 1 + src/errors/mod.rs | 67 +++++++++++++++++++++++++++++++++++++++++++++- src/events/mod.rs | 24 ++++++++++++++++- src/transaction.rs | 13 ++++++--- 5 files changed, 112 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e5e49ce1..e9711f16 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -493,6 +493,17 @@ version = "2.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3ee2393c4a91429dffb4bedf19f4d6abf27d8a732c8ce4980305d782e5426d57" +[[package]] +name = "derivative" +version = "2.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fcc3dd5e9e9c0b295d6e1e4d811fb6f157d5ffd784b8d202fc62eac8035a770b" +dependencies = [ + "proc-macro2 1.0.28", + "quote 1.0.9", + "syn 1.0.74", +] + [[package]] name = "deunicode" version = "0.4.3" @@ -2076,6 +2087,7 @@ dependencies = [ "base64 0.13.0", "chrono", "cmd_lib", + "derivative", "digitalocean", "dirs", "flate2", diff --git a/Cargo.toml b/Cargo.toml index 7f5b924b..5ae0c0e7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,6 +9,7 @@ edition = "2018" [dependencies] chrono = "0.4.19" cmd_lib = "1.0.13" +derivative = "2.2.0" git2 = "0.14.2" walkdir = "2.3.2" itertools = "0.10.0" diff --git a/src/errors/mod.rs b/src/errors/mod.rs index 2831c945..619737ab 100644 --- a/src/errors/mod.rs +++ b/src/errors/mod.rs @@ -1,5 +1,6 @@ pub mod io; +extern crate derivative; extern crate url; use crate::build_platform::BuildError; @@ -12,6 +13,7 @@ use crate::error::{EngineError as LegacyEngineError, EngineErrorCause, EngineErr use crate::events::{EventDetails, GeneralStep, Stage, Transmitter}; use crate::io_models::QoveryIdentifier; use crate::object_storage::errors::ObjectStorageError; +use derivative::Derivative; use std::fmt::{Display, Formatter}; use thiserror::Error; use url::Url; @@ -24,13 +26,16 @@ pub enum ErrorMessageVerbosity { } /// CommandError: command error, mostly returned by third party tools. -#[derive(Clone, Debug, Error, PartialEq)] +#[derive(Derivative, Clone, Error, PartialEq)] +#[derivative(Debug)] pub struct CommandError { /// full_details: full error message, can contains unsafe text such as passwords and tokens. full_details: String, /// message_safe: error message omitting displaying any protected data such as passwords and tokens. message_safe: Option, /// env_vars: environments variables including touchy data such as secret keys. + /// env_vars field is ignored from any wild Debug printing because of it touchy data it carries. + #[derivative(Debug = "ignore")] env_vars: Option>, } @@ -2866,3 +2871,63 @@ impl Display for EngineError { f.write_str(format!("{:?}", self).as_str()) } } + +#[cfg(test)] +mod tests { + use crate::cloud_provider::Kind; + use crate::errors::{CommandError, EngineError}; + use crate::events::{EventDetails, InfrastructureStep, Stage, Transmitter}; + use crate::io_models::QoveryIdentifier; + use crate::models::scaleway::ScwRegion; + + #[test] + fn test_command_error_test_hidding_env_vars_in_debug() { + // setup: + let command_err = CommandError::new_with_env_vars( + "my raw message".to_string(), + Some("my safe message".to_string()), + Some(vec![("my_secret".to_string(), "my_secret_value".to_string())]), + ); + + // execute: + let res = format!("{:?}", command_err); + + // verify: + assert!(!res.contains("my_secret")); + assert!(!res.contains("my_secret_value")); + } + + #[test] + fn test_engine_error_test_hidding_env_vars_in_debug() { + // setup: + let command_err = CommandError::new_with_env_vars( + "my raw message".to_string(), + Some("my safe message".to_string()), + Some(vec![("my_secret".to_string(), "my_secret_value".to_string())]), + ); + let cluster_id = QoveryIdentifier::new_random(); + let engine_err = EngineError::new_unknown( + EventDetails::new( + Some(Kind::Scw), + QoveryIdentifier::new_random(), + QoveryIdentifier::new_random(), + QoveryIdentifier::new_random(), + Some(ScwRegion::Paris.as_str().to_string()), + Stage::Infrastructure(InfrastructureStep::Create), + Transmitter::Kubernetes(cluster_id.to_string(), cluster_id.to_string()), + ), + "qovery_log_message".to_string(), + "user_log_message".to_string(), + Some(command_err.clone()), + None, + None, + ); + + // execute: + let res = format!("{:?}", engine_err); + + // verify: + assert!(!res.contains("my_secret")); + assert!(!res.contains("my_secret_value")); + } +} diff --git a/src/events/mod.rs b/src/events/mod.rs index f1e472a9..5c990fe3 100644 --- a/src/events/mod.rs +++ b/src/events/mod.rs @@ -4,11 +4,13 @@ pub mod io; +extern crate derivative; extern crate url; use crate::cloud_provider::Kind; use crate::errors::{CommandError, EngineError, ErrorMessageVerbosity}; use crate::io_models::QoveryIdentifier; +use derivative::Derivative; use std::fmt::{Display, Formatter}; #[derive(Debug, Clone)] @@ -63,7 +65,8 @@ impl From for ErrorMessageVerbosity { } } -#[derive(Debug, Clone)] +#[derive(Derivative, Clone)] +#[derivative(Debug)] /// EventMessage: represents an event message. pub struct EventMessage { // Message which is known to be safe: doesn't expose any credentials nor touchy info. @@ -71,6 +74,8 @@ pub struct EventMessage { // String containing full details including touchy data (passwords and tokens). full_details: Option, // Environments variables including touchy data such as secret keys. + // env_vars field is ignored from any wild Debug printing because of it touchy data it carries. + #[derivative(Debug = "ignore")] env_vars: Option>, } @@ -578,4 +583,21 @@ mod tests { assert_eq!(expected_step_name, result); } } + + #[test] + fn test_event_message_test_hidding_env_vars_in_debug() { + // setup: + let event_message = EventMessage::new_with_env_vars( + "my safe message".to_string(), + Some("my full message".to_string()), + Some(vec![("my_secret".to_string(), "my_secret_value".to_string())]), + ); + + // execute: + let res = format!("{:?}", event_message); + + // verify: + assert!(!res.contains("my_secret")); + assert!(!res.contains("my_secret_value")); + } } diff --git a/src/transaction.rs b/src/transaction.rs index c0cbe5a5..21fc5496 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -430,7 +430,7 @@ impl<'a> Transaction<'a> { match result { Err(err) => { - warn!("infrastructure ROLLBACK STARTED! an error occurred {:?}", err); + warn!("infrastructure ROLLBACK STARTED! an error occurred {}", err); match self.rollback() { Ok(_) => { // an error occurred on infrastructure deployment BUT rolledback is OK @@ -601,17 +601,24 @@ impl Clone for Step { } } -#[derive(Debug)] +#[derive(thiserror::Error, Debug)] pub enum RollbackError { + #[error("Commit error: {0}")] CommitError(Box), + #[error("No failover environment")] NoFailoverEnvironment, + #[error("Nothing")] Nothing, } -#[derive(Debug)] +#[derive(thiserror::Error, Debug)] pub enum TransactionResult { + #[error("Ok")] Ok, + #[error("Canceled")] Canceled, + #[error("Rollback: {0}")] Rollback(EngineError), + #[error("Unrecoverable error: {0}, {1}")] UnrecoverableError(EngineError, RollbackError), }