From b0c4ed04bbafa6c3f3ec5d29274306da9938a934 Mon Sep 17 00:00:00 2001
From: Sebastian Bernauer <sebastian.bernauer@stackable.de>
Date: Tue, 27 Aug 2024 09:43:06 +0200
Subject: [PATCH] fix: Don't print credentials to STDOUT during startup (#634)

* fix: Don't print credentials to STDOUT during startup

* changelog

* fix test
---
 CHANGELOG.md                                             | 4 +++-
 rust/operator-binary/src/authentication/mod.rs           | 4 ++--
 rust/operator-binary/src/authentication/password/ldap.rs | 3 +++
 rust/operator-binary/src/catalog/config.rs               | 8 +++++++-
 rust/operator-binary/src/command.rs                      | 5 ++++-
 5 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4d706215..d44dda94 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,10 +12,12 @@ All notable changes to this project will be documented in this file.
 
 ### Fixed
 
-- Do not ignore envOverrides ([#633]):
+- Don't ignore envOverrides ([#633]).
+- Don't print credentials to STDOUT during startup. Ideally we should use [config-utils](https://github.com/stackabletech/config-utils), but that's not easy (see [here](https://github.com/stackabletech/trino-operator/tree/fix/secret-printing)) ([#634]).
 
 [#631]: https://github.com/stackabletech/trino-operator/pull/631
 [#633]: https://github.com/stackabletech/trino-operator/pull/633
+[#634]: https://github.com/stackabletech/trino-operator/pull/634
 
 ## [24.7.0] - 2024-07-24
 
diff --git a/rust/operator-binary/src/authentication/mod.rs b/rust/operator-binary/src/authentication/mod.rs
index 63d9d1bf..56f4a962 100644
--- a/rust/operator-binary/src/authentication/mod.rs
+++ b/rust/operator-binary/src/authentication/mod.rs
@@ -825,7 +825,7 @@ mod tests {
             0
         );
 
-        // we expect 4 entries because of 2x user:password bind credential env export
+        // We expect 8 entries because of "set +x", "set -x" and 2x user:password bind credential env export
         assert_eq!(
             auth_config_with_ldap_bind
                 .commands(
@@ -833,7 +833,7 @@ mod tests {
                     &stackable_trino_crd::Container::Trino
                 )
                 .len(),
-            4
+            8
         );
     }
 
diff --git a/rust/operator-binary/src/authentication/password/ldap.rs b/rust/operator-binary/src/authentication/password/ldap.rs
index b6aa6c3c..e4052af9 100644
--- a/rust/operator-binary/src/authentication/password/ldap.rs
+++ b/rust/operator-binary/src/authentication/password/ldap.rs
@@ -115,6 +115,8 @@ impl LdapAuthenticator {
         let mut commands = vec![];
 
         if let Some((user_path, pw_path)) = self.ldap.bind_credentials_mount_paths() {
+            // Don't print secret contents!
+            commands.push("set +x".to_string());
             commands.push(format!(
                 "export {user}=$(cat {user_path})",
                 user = self.build_bind_credentials_env_var(LDAP_USER_ENV)
@@ -123,6 +125,7 @@ impl LdapAuthenticator {
                 "export {pw}=$(cat {pw_path})",
                 pw = self.build_bind_credentials_env_var(LDAP_PASSWORD_ENV)
             ));
+            commands.push("set -x".to_string());
         }
 
         commands
diff --git a/rust/operator-binary/src/catalog/config.rs b/rust/operator-binary/src/catalog/config.rs
index c1eb47dc..0e6e349c 100644
--- a/rust/operator-binary/src/catalog/config.rs
+++ b/rust/operator-binary/src/catalog/config.rs
@@ -13,18 +13,24 @@ use super::{FromTrinoCatalogError, ToCatalogConfig};
 pub struct CatalogConfig {
     /// Name of the catalog
     pub name: String,
+
     /// Properties of the catalog
     pub properties: BTreeMap<String, String>,
+
     /// List of EnvVar that will be added to every Trino container
     pub env_bindings: Vec<EnvVar>,
+
     /// Env-Vars that should be exported.
     /// The value will be read from the file specified.
-    /// You can think of it like `export <key>=$(cat <value>)`
+    /// You can think of it like `export <key>="$(cat <value>)"`
     pub load_env_from_files: BTreeMap<String, String>,
+
     /// Additional commands that needs to be executed before starting Trino
     pub init_container_extra_start_commands: Vec<String>,
+
     /// Volumes that need to be added to the pod (e.g. for S3 credentials)
     pub volumes: Vec<Volume>,
+
     /// Volume mounts that need to be added to the Trino container (e.g. for S3 credentials)
     pub volume_mounts: Vec<VolumeMount>,
 }
diff --git a/rust/operator-binary/src/command.rs b/rust/operator-binary/src/command.rs
index 278c7a0d..48fa93c4 100644
--- a/rust/operator-binary/src/command.rs
+++ b/rust/operator-binary/src/command.rs
@@ -103,11 +103,14 @@ pub fn container_trino_args(
     args.extend(authentication_config.commands(&TrinoRole::Coordinator, &Container::Trino));
 
     // Add the commands that are needed to set up the catalogs
+    // Don't print secret contents!
+    args.push("set +x".to_string());
     catalogs.iter().for_each(|catalog| {
         for (env_name, file) in &catalog.load_env_from_files {
-            args.push(format!("export {env_name}=$(cat {file})"));
+            args.push(format!("export {env_name}=\"$(cat {file})\""));
         }
     });
+    args.push("set -x".to_string());
 
     // Start command
     args.push(format!(