From 9a2601317151d3a647902511589a92e8cb1af010 Mon Sep 17 00:00:00 2001 From: gabrik Date: Thu, 2 Nov 2023 10:40:20 +0100 Subject: [PATCH] feat(575): using a mix of secrecy + serde::skip_serializing to avoid leaks of secrets in logs Signed-off-by: gabrik --- Cargo.toml | 1 + commons/zenoh-config/Cargo.toml | 1 + commons/zenoh-config/src/lib.rs | 43 +++++++++++++++---- io/zenoh-links/zenoh-link-quic/Cargo.toml | 3 +- io/zenoh-links/zenoh-link-quic/src/lib.rs | 31 +++++++------- io/zenoh-links/zenoh-link-tls/Cargo.toml | 3 +- io/zenoh-links/zenoh-link-tls/src/lib.rs | 51 ++++++++++++----------- 7 files changed, 83 insertions(+), 50 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index cd8254efc2..a707ab390c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -128,6 +128,7 @@ rustls = { version = "0.21.5", features = ["dangerous_configuration"] } rustls-native-certs = "0.6.2" rustls-pemfile = "1.0.2" schemars = "0.8.12" +secrecy = {version = "0.8.0", features = ["serde", "alloc"]} serde = { version = "1.0.154", default-features = false, features = [ "derive", ] } # Default features are disabled due to usage in no_std crates diff --git a/commons/zenoh-config/Cargo.toml b/commons/zenoh-config/Cargo.toml index 3bf62bfdb6..f0189ff3e7 100644 --- a/commons/zenoh-config/Cargo.toml +++ b/commons/zenoh-config/Cargo.toml @@ -35,3 +35,4 @@ zenoh-core = { workspace = true } zenoh-protocol = { workspace = true } zenoh-result = { workspace = true } zenoh-util = { workspace = true } +secrecy = { workspace = true } diff --git a/commons/zenoh-config/src/lib.rs b/commons/zenoh-config/src/lib.rs index 9345118464..45b9d4a218 100644 --- a/commons/zenoh-config/src/lib.rs +++ b/commons/zenoh-config/src/lib.rs @@ -16,6 +16,7 @@ pub mod defaults; mod include; use include::recursive_include; +use secrecy::{CloneableSecret, DebugSecret, Secret, SerializableSecret, Zeroize}; use serde::{ de::{self, MapAccess, Visitor}, Deserialize, Serialize, @@ -46,6 +47,29 @@ use zenoh_protocol::{ use zenoh_result::{bail, zerror, ZResult}; use zenoh_util::LibLoader; +// Wrappers for secrecy of values +#[derive(serde::Serialize, serde::Deserialize, Debug, Clone)] +pub struct SecretString(String); + +impl Deref for SecretString { + type Target = String; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl SerializableSecret for SecretString {} +impl DebugSecret for SecretString {} +impl CloneableSecret for SecretString {} +impl Zeroize for SecretString { + fn zeroize(&mut self) { + self.0 = "".to_string(); + } +} + +pub type SecretValue = Secret; + pub type ValidationFunction = std::sync::Arc< dyn Fn( &str, @@ -294,14 +318,17 @@ validated_struct::validator! { client_private_key: Option, client_certificate: Option, server_name_verification: Option, - pub private : #[derive(Default)] - Base64Data { - root_ca_certificate_base64: Option, - server_private_key_base64: Option, - server_certificate_base64: Option, - client_private_key_base64 : Option, - client_certificate_base64 : Option, - } + // Skip serializing field because they contain secrets + #[serde(skip_serializing)] + root_ca_certificate_base64: Option, + #[serde(skip_serializing)] + server_private_key_base64: Option, + #[serde(skip_serializing)] + server_certificate_base64: Option, + #[serde(skip_serializing)] + client_private_key_base64 : Option, + #[serde(skip_serializing)] + client_certificate_base64 : Option, } , pub unixpipe: #[derive(Default)] diff --git a/io/zenoh-links/zenoh-link-quic/Cargo.toml b/io/zenoh-links/zenoh-link-quic/Cargo.toml index 20baa21f0e..1cc2d82744 100644 --- a/io/zenoh-links/zenoh-link-quic/Cargo.toml +++ b/io/zenoh-links/zenoh-link-quic/Cargo.toml @@ -42,4 +42,5 @@ zenoh-protocol = { workspace = true } zenoh-result = { workspace = true } zenoh-sync = { workspace = true } zenoh-util = { workspace = true } -base64 = { workspace = true } \ No newline at end of file +base64 = { workspace = true } +secrecy = {workspace = true } \ No newline at end of file diff --git a/io/zenoh-links/zenoh-link-quic/src/lib.rs b/io/zenoh-links/zenoh-link-quic/src/lib.rs index 5dc5e9ba5b..4f268200a2 100644 --- a/io/zenoh-links/zenoh-link-quic/src/lib.rs +++ b/io/zenoh-links/zenoh-link-quic/src/lib.rs @@ -24,6 +24,7 @@ use config::{ TLS_SERVER_CERTIFICATE_FILE, TLS_SERVER_NAME_VERIFICATION, TLS_SERVER_PRIVATE_KEY_BASE64, TLS_SERVER_PRIVATE_KEY_FILE, }; +use secrecy::ExposeSecret; use std::net::SocketAddr; use zenoh_config::Config; use zenoh_core::zconfigurable; @@ -74,10 +75,7 @@ impl ConfigurationInspector for QuicConfigurator { let c = config.transport().link().tls(); - match ( - c.root_ca_certificate(), - c.private().root_ca_certificate_base64(), - ) { + match (c.root_ca_certificate(), c.root_ca_certificate_base64()) { (Some(_), Some(_)) => { bail!("Only one between 'root_ca_certificate' and 'root_ca_certificate_base64' can be present!") } @@ -85,15 +83,15 @@ impl ConfigurationInspector for QuicConfigurator { ps.push((TLS_ROOT_CA_CERTIFICATE_FILE, ca_certificate)); } (None, Some(ca_certificate)) => { - ps.push((TLS_ROOT_CA_CERTIFICATE_BASE64, ca_certificate)); + ps.push(( + TLS_ROOT_CA_CERTIFICATE_BASE64, + ca_certificate.expose_secret(), + )); } _ => {} } - match ( - c.server_private_key(), - c.private().server_private_key_base64(), - ) { + match (c.server_private_key(), c.server_private_key_base64()) { (Some(_), Some(_)) => { bail!("Only one between 'server_private_key' and 'server_private_key_base64' can be present!") } @@ -101,15 +99,15 @@ impl ConfigurationInspector for QuicConfigurator { ps.push((TLS_SERVER_PRIVATE_KEY_FILE, server_private_key)); } (None, Some(server_private_key)) => { - ps.push((TLS_SERVER_PRIVATE_KEY_BASE64, server_private_key)); + ps.push(( + TLS_SERVER_PRIVATE_KEY_BASE64, + server_private_key.expose_secret(), + )); } _ => {} } - match ( - c.server_certificate(), - c.private().server_certificate_base64(), - ) { + match (c.server_certificate(), c.server_certificate_base64()) { (Some(_), Some(_)) => { bail!("Only one between 'server_certificate' and 'server_certificate_base64' can be present!") } @@ -117,7 +115,10 @@ impl ConfigurationInspector for QuicConfigurator { ps.push((TLS_SERVER_CERTIFICATE_FILE, server_certificate)); } (None, Some(server_certificate)) => { - ps.push((TLS_SERVER_CERTIFICATE_BASE64, server_certificate)); + ps.push(( + TLS_SERVER_CERTIFICATE_BASE64, + server_certificate.expose_secret(), + )); } _ => {} } diff --git a/io/zenoh-links/zenoh-link-tls/Cargo.toml b/io/zenoh-links/zenoh-link-tls/Cargo.toml index c397ad29d1..5d047b1160 100644 --- a/io/zenoh-links/zenoh-link-tls/Cargo.toml +++ b/io/zenoh-links/zenoh-link-tls/Cargo.toml @@ -41,4 +41,5 @@ zenoh-protocol = { workspace = true } zenoh-result = { workspace = true } zenoh-sync = { workspace = true } zenoh-util = { workspace = true } -base64 = { workspace = true } \ No newline at end of file +base64 = { workspace = true } +secrecy = {workspace = true } \ No newline at end of file diff --git a/io/zenoh-links/zenoh-link-tls/src/lib.rs b/io/zenoh-links/zenoh-link-tls/src/lib.rs index 79a17de401..9b45b5e68b 100644 --- a/io/zenoh-links/zenoh-link-tls/src/lib.rs +++ b/io/zenoh-links/zenoh-link-tls/src/lib.rs @@ -26,6 +26,7 @@ use config::{ TLS_ROOT_CA_CERTIFICATE_FILE, TLS_SERVER_CERTIFICATE_BASE64, TLS_SERVER_CERTIFICATE_FILE, TLS_SERVER_NAME_VERIFICATION, TLS_SERVER_PRIVATE_KEY_BASE_64, TLS_SERVER_PRIVATE_KEY_FILE, }; +use secrecy::ExposeSecret; use std::{convert::TryFrom, net::SocketAddr}; use zenoh_config::Config; use zenoh_core::zconfigurable; @@ -71,10 +72,7 @@ impl ConfigurationInspector for TlsConfigurator { let c = config.transport().link().tls(); - match ( - c.root_ca_certificate(), - c.private().root_ca_certificate_base64(), - ) { + match (c.root_ca_certificate(), c.root_ca_certificate_base64()) { (Some(_), Some(_)) => { bail!("Only one between 'root_ca_certificate' and 'root_ca_certificate_base64' can be present!") } @@ -82,15 +80,15 @@ impl ConfigurationInspector for TlsConfigurator { ps.push((TLS_ROOT_CA_CERTIFICATE_FILE, ca_certificate)); } (None, Some(ca_certificate)) => { - ps.push((TLS_ROOT_CA_CERTIFICATE_BASE64, ca_certificate)); + ps.push(( + TLS_ROOT_CA_CERTIFICATE_BASE64, + ca_certificate.expose_secret(), + )); } _ => {} } - match ( - c.server_private_key(), - c.private().server_private_key_base64(), - ) { + match (c.server_private_key(), c.server_private_key_base64()) { (Some(_), Some(_)) => { bail!("Only one between 'server_private_key' and 'server_private_key_base64' can be present!") } @@ -98,15 +96,15 @@ impl ConfigurationInspector for TlsConfigurator { ps.push((TLS_SERVER_PRIVATE_KEY_FILE, server_private_key)); } (None, Some(server_private_key)) => { - ps.push((TLS_SERVER_PRIVATE_KEY_BASE_64, server_private_key)); + ps.push(( + TLS_SERVER_PRIVATE_KEY_BASE_64, + server_private_key.expose_secret(), + )); } _ => {} } - match ( - c.server_certificate(), - c.private().server_certificate_base64(), - ) { + match (c.server_certificate(), c.server_certificate_base64()) { (Some(_), Some(_)) => { bail!("Only one between 'server_certificate' and 'server_certificate_base64' can be present!") } @@ -114,7 +112,10 @@ impl ConfigurationInspector for TlsConfigurator { ps.push((TLS_SERVER_CERTIFICATE_FILE, server_certificate)); } (None, Some(server_certificate)) => { - ps.push((TLS_SERVER_CERTIFICATE_BASE64, server_certificate)); + ps.push(( + TLS_SERVER_CERTIFICATE_BASE64, + server_certificate.expose_secret(), + )); } _ => {} } @@ -126,10 +127,7 @@ impl ConfigurationInspector for TlsConfigurator { }; } - match ( - c.client_private_key(), - c.private().client_private_key_base64(), - ) { + match (c.client_private_key(), c.client_private_key_base64()) { (Some(_), Some(_)) => { bail!("Only one between 'client_private_key' and 'client_private_key_base64' can be present!") } @@ -137,15 +135,15 @@ impl ConfigurationInspector for TlsConfigurator { ps.push((TLS_CLIENT_PRIVATE_KEY_FILE, client_private_key)); } (None, Some(client_private_key)) => { - ps.push((TLS_CLIENT_PRIVATE_KEY_BASE64, client_private_key)); + ps.push(( + TLS_CLIENT_PRIVATE_KEY_BASE64, + client_private_key.expose_secret(), + )); } _ => {} } - match ( - c.client_certificate(), - c.private().client_certificate_base64(), - ) { + match (c.client_certificate(), c.client_certificate_base64()) { (Some(_), Some(_)) => { bail!("Only one between 'client_certificate' and 'client_certificate_base64' can be present!") } @@ -153,7 +151,10 @@ impl ConfigurationInspector for TlsConfigurator { ps.push((TLS_CLIENT_CERTIFICATE_FILE, client_certificate)); } (None, Some(client_certificate)) => { - ps.push((TLS_CLIENT_CERTIFICATE_BASE64, client_certificate)); + ps.push(( + TLS_CLIENT_CERTIFICATE_BASE64, + client_certificate.expose_secret(), + )); } _ => {} }