From 894572371d4e9be8aa564035ac7e782f8ceda403 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 18 Sep 2025 13:04:59 +0200 Subject: [PATCH 1/3] fix: Prevent missing certificates --- CHANGELOG.md | 10 +++ .../src/authentication/ldap.rs | 2 - .../operator-binary/src/authentication/mod.rs | 32 ++----- .../src/authentication/oidc.rs | 6 +- rust/operator-binary/src/crd/mod.rs | 4 +- rust/operator-binary/src/crd/security.rs | 85 +++++-------------- 6 files changed, 39 insertions(+), 100 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 20bf89ce..245f0ee8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,16 @@ All notable changes to this project will be documented in this file. - Helm: Allow Pod `priorityClassName` to be configured ([#752]). +### Fixed + +- Previously we had a bug that could lead to missing certificates ([#XXX]). + + This could be the case when the Stackable PKI rotated it's CA certificate or you specified multiple + CAs in your SecretClass. + Especially the CA rotation could brake working clusters at any time. + We now correctly handle multiple certificates for both cases. + See [this GitHub issue](https://github.com/stackabletech/issues/issues/764) for details + [#752]: https://github.com/stackabletech/druid-operator/pull/752 ## [25.7.0] - 2025-07-23 diff --git a/rust/operator-binary/src/authentication/ldap.rs b/rust/operator-binary/src/authentication/ldap.rs index cae6ca72..863a4186 100644 --- a/rust/operator-binary/src/authentication/ldap.rs +++ b/rust/operator-binary/src/authentication/ldap.rs @@ -96,7 +96,6 @@ pub fn generate_runtime_properties_config( } pub fn prepare_container_commands( - auth_class_name: &String, provider: &ldap::v1alpha1::AuthenticationProvider, command: &mut Vec, ) { @@ -104,7 +103,6 @@ pub fn prepare_container_commands( command.push(add_cert_to_trust_store_cmd( &tls_ca_cert_mount_path, STACKABLE_TLS_DIR, - &format!("ldap-{}", auth_class_name), TLS_STORE_PASSWORD, )) } diff --git a/rust/operator-binary/src/authentication/mod.rs b/rust/operator-binary/src/authentication/mod.rs index dafb3779..3bb85c00 100644 --- a/rust/operator-binary/src/authentication/mod.rs +++ b/rust/operator-binary/src/authentication/mod.rs @@ -54,11 +54,9 @@ pub enum Error { pub enum DruidAuthenticationConfig { Tls {}, Ldap { - auth_class_name: String, provider: authentication::ldap::v1alpha1::AuthenticationProvider, }, Oidc { - auth_class_name: String, provider: authentication::oidc::v1alpha1::AuthenticationProvider, oidc: authentication::oidc::v1alpha1::ClientAuthenticationOptions, }, @@ -75,19 +73,10 @@ impl DruidAuthenticationConfig { None => Ok(None), Some(auth_class_resolved) => match &auth_class_resolved { AuthenticationClassResolved::Tls { .. } => Ok(Some(Self::Tls {})), - AuthenticationClassResolved::Ldap { - auth_class_name, - provider, - } => Ok(Some(Self::Ldap { - auth_class_name: auth_class_name.to_string(), + AuthenticationClassResolved::Ldap { provider, .. } => Ok(Some(Self::Ldap { provider: provider.clone(), })), - AuthenticationClassResolved::Oidc { - auth_class_name, - provider, - oidc, - } => Ok(Some(Self::Oidc { - auth_class_name: auth_class_name.to_string(), + AuthenticationClassResolved::Oidc { provider, oidc, .. } => Ok(Some(Self::Oidc { provider: provider.clone(), oidc: oidc.clone(), })), @@ -133,25 +122,16 @@ impl DruidAuthenticationConfig { pub fn main_container_commands(&self) -> Vec { let mut command = vec![]; - if let DruidAuthenticationConfig::Oidc { - auth_class_name, - provider, - .. - } = self - { - oidc::main_container_commands(auth_class_name, provider, &mut command) + if let DruidAuthenticationConfig::Oidc { provider, .. } = self { + oidc::main_container_commands(provider, &mut command) } command } pub fn prepare_container_commands(&self) -> Vec { let mut command = vec![]; - if let DruidAuthenticationConfig::Ldap { - auth_class_name, - provider, - } = self - { - ldap::prepare_container_commands(auth_class_name, provider, &mut command) + if let DruidAuthenticationConfig::Ldap { provider } = self { + ldap::prepare_container_commands(provider, &mut command) } command } diff --git a/rust/operator-binary/src/authentication/oidc.rs b/rust/operator-binary/src/authentication/oidc.rs index f61849db..22f4f909 100644 --- a/rust/operator-binary/src/authentication/oidc.rs +++ b/rust/operator-binary/src/authentication/oidc.rs @@ -107,15 +107,11 @@ pub fn generate_runtime_properties_config( } pub fn main_container_commands( - auth_class_name: &String, provider: &oidc::v1alpha1::AuthenticationProvider, command: &mut Vec, ) { if let Some(tls_ca_cert_mount_path) = provider.tls.tls_ca_cert_mount_path() { - command.push(add_cert_to_jvm_trust_store_cmd( - &tls_ca_cert_mount_path, - &format!("oidc-{}", auth_class_name), - )) + command.push(add_cert_to_jvm_trust_store_cmd(&tls_ca_cert_mount_path)) } } diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index 079c14b1..18ceae55 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -2,6 +2,7 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use indoc::formatdoc; use product_config::types::PropertyNameKind; +use security::add_cert_to_jvm_trust_store_cmd; use serde::{Deserialize, Serialize}; use snafu::{OptionExt, ResultExt, Snafu}; use stackable_operator::{ @@ -996,8 +997,7 @@ impl DruidRole { if let Some(s3) = s3 { if let Some(ca_cert_file) = s3.tls.tls_ca_cert_mount_path() { - // The alias can not clash, as we only support a single S3Connection - commands.push(format!("keytool -importcert -file {ca_cert_file} -alias stackable-s3-ca-cert -keystore {STACKABLE_TRUST_STORE} -storepass {STACKABLE_TRUST_STORE_PASSWORD} -noprompt")); + commands.push(add_cert_to_jvm_trust_store_cmd(&ca_cert_file)); } } diff --git a/rust/operator-binary/src/crd/security.rs b/rust/operator-binary/src/crd/security.rs index fa1f6565..5ecb4bfd 100644 --- a/rust/operator-binary/src/crd/security.rs +++ b/rust/operator-binary/src/crd/security.rs @@ -22,7 +22,7 @@ use stackable_operator::{ }; use crate::crd::{ - DruidRole, STACKABLE_TRUST_STORE, STACKABLE_TRUST_STORE_PASSWORD, + DruidRole, STACKABLE_TRUST_STORE_PASSWORD, authentication::{self, AuthenticationClassesResolved}, v1alpha1, }; @@ -81,15 +81,16 @@ const SERVER_HTTPS_CERT_ALIAS: &str = "druid.server.https.certAlias"; const SERVER_HTTPS_VALIDATE_HOST_NAMES: &str = "druid.server.https.validateHostnames"; const SERVER_HTTPS_KEY_MANAGER_PASSWORD: &str = "druid.server.https.keyManagerPassword"; const SERVER_HTTPS_REQUIRE_CLIENT_CERTIFICATE: &str = "druid.server.https.requireClientCertificate"; -const TLS_ALIAS_NAME: &str = "tls"; +/// The alias of the certificate in the keystore used for TLS stuff. +/// All secret-op generated keystores have one entry with the alias "1". +/// (side node: I think technically they don't have an alias and the JVm counts them, but not sure) +const TLS_ALIAS_NAME: &str = "1"; pub const AUTH_TRUST_STORE_PATH: &str = "druid.auth.basic.ssl.trustStorePath"; pub const AUTH_TRUST_STORE_TYPE: &str = "druid.auth.basic.ssl.trustStoreType"; pub const AUTH_TRUST_STORE_PASSWORD: &str = "druid.auth.basic.ssl.trustStorePassword"; // Misc TLS pub const TLS_STORE_PASSWORD: &str = "changeit"; pub const TLS_STORE_TYPE: &str = "pkcs12"; -const SYSTEM_TRUST_STORE: &str = "/etc/pki/java/cacerts"; -const SYSTEM_TRUST_STORE_PASSWORD: &str = "changeit"; // directories const STACKABLE_MOUNT_TLS_DIR: &str = "/stackable/mount_tls"; @@ -432,12 +433,13 @@ impl DruidTlsSecurity { } vec![ - // Copy system truststore to empty dir and convert to PKCS12 - import_system_truststore(STACKABLE_TLS_DIR), - // Import secret-op truststore to copied system trust store - import_truststore(STACKABLE_MOUNT_TLS_DIR, STACKABLE_TLS_DIR), - // Import / Copy secret-op keystore to empty dir and set required alias - import_keystore(STACKABLE_MOUNT_TLS_DIR, STACKABLE_TLS_DIR), + // FIXME: *Technically* we should only add the system truststore in case any webPki usage is detected, + // wether that's in S3, LDAP, OIDC, FTE or whatnot. + format!( + "cert-tools generate-pkcs12-truststore --pkcs12 '{STACKABLE_MOUNT_TLS_DIR}/truststore.p12:{TLS_STORE_PASSWORD}' --pem /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem --out {STACKABLE_TLS_DIR}/truststore.p12 --out-password '{TLS_STORE_PASSWORD}'" + ), + // We can copy the keystore as is. + format!("cp {STACKABLE_MOUNT_TLS_DIR}/keystore.p12 {STACKABLE_TLS_DIR}/keystore.p12"), ] } @@ -468,66 +470,19 @@ impl DruidTlsSecurity { } } -/// Generate a script to add a CA to a truststore +/// Generate a bash command to add a CA to a truststore pub fn add_cert_to_trust_store_cmd( - cert: &str, - trust_store_directory: &str, - alias_name: &str, + cert_file: &str, + destination_directory: &str, store_password: &str, ) -> String { + let truststore = format!("{destination_directory}/truststore.p12"); format!( - "keytool -importcert -file {cert} -keystore {trust_store_directory}/truststore.p12 -storetype pkcs12 -alias {alias_name} -storepass {store_password} -noprompt" + "cert-tools generate-pkcs12-truststore --pkcs12 {truststore}:{store_password} --pem {cert_file} --out {truststore} --out-password {store_password}" ) } -pub fn add_cert_to_jvm_trust_store_cmd(cert: &str, alias_name: &str) -> String { - format!( - "keytool -importcert -file {cert} -keystore {STACKABLE_TRUST_STORE} -storetype pkcs12 -alias {alias_name} -storepass {STACKABLE_TRUST_STORE_PASSWORD} -noprompt" - ) -} - -/// Import the system truststore to a truststore named `truststore.p12` in `destination_directory`. -fn import_system_truststore(destination_directory: &str) -> String { - let dest_truststore_path = format!("{destination_directory}/truststore.p12"); - format!( - "keytool -importkeystore -srckeystore {SYSTEM_TRUST_STORE} -srcstoretype jks -srcstorepass {SYSTEM_TRUST_STORE_PASSWORD} -destkeystore {dest_truststore_path} -deststoretype pkcs12 -deststorepass {TLS_STORE_PASSWORD} -noprompt" - ) -} - -/// Generates the shell script to import a secret operator provided truststore without password -/// into a new truststore with password in a writeable empty dir -/// -/// # Arguments -/// - `source_directory`: The directory of the source truststore. Should usually be a secret -/// operator volume mount. -/// - `destination_directory`: The directory of the destination truststore. Should usually be an -/// empty dir. -fn import_truststore(source_directory: &str, destination_directory: &str) -> String { - let source_truststore_path = format!("{source_directory}/truststore.p12"); - let dest_truststore_path = format!("{destination_directory}/truststore.p12"); - // The source directory is a secret-op mount and we do not want to write / add anything in there - // Therefore we import all the contents to a truststore in "writeable" empty dirs. - // Keytool is only barking if a password is not set for the destination truststore (which we set) - // and do provide an empty password for the source truststore coming from the secret-operator. - // Using no password will result in a warning. - // All secret-op generated truststores have one entry with alias "1". We generate a UUID for - // the destination truststore to avoid conflicts when importing multiple secret-op generated - // truststores. We do not use the UUID rust crate since this will continuously change the STS... and - // leads to never-ending reconciles. - format!( - "keytool -importkeystore -srckeystore {source_truststore_path} -srcstoretype PKCS12 -srcstorepass {TLS_STORE_PASSWORD} -srcalias 1 -destkeystore {dest_truststore_path} -deststoretype PKCS12 -deststorepass {TLS_STORE_PASSWORD} -destalias $(cat /proc/sys/kernel/random/uuid) -noprompt" - ) -} - -/// Generate a script to import a mounted keystore to an empty dir and set an alias -fn import_keystore(source_directory: &str, destination_directory: &str) -> String { - let source_keystore_path = format!("{source_directory}/keystore.p12"); - let dest_keystore_path = format!("{destination_directory}/keystore.p12"); - // The source directory is a secret-op mount and we do not want to write / add anything in there - // Therefore we import all the contents to a keystore in "writeable" empty dirs. - // Using no password will result in a warning. - // All secret-op generated keystores have one entry with alias "1". - format!( - "keytool -importkeystore -srckeystore {source_keystore_path} -srcstoretype PKCS12 -srcstorepass {TLS_STORE_PASSWORD} -srcalias 1 -destkeystore {dest_keystore_path} -deststoretype PKCS12 -deststorepass {TLS_STORE_PASSWORD} -destalias {TLS_ALIAS_NAME} -noprompt" - ) +/// Generate a bash command to add a CA to the truststore that is passed to the JVM +pub fn add_cert_to_jvm_trust_store_cmd(cert_file: &str) -> String { + add_cert_to_trust_store_cmd(cert_file, "/stackable", STACKABLE_TRUST_STORE_PASSWORD) } From 4d8c041f93e6f87412edd5f67dafeb03885d7c17 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 18 Sep 2025 13:06:56 +0200 Subject: [PATCH 2/3] changelog --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 245f0ee8..3f1d4b4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ All notable changes to this project will be documented in this file. ### Fixed -- Previously we had a bug that could lead to missing certificates ([#XXX]). +- Previously we had a bug that could lead to missing certificates ([#753]). This could be the case when the Stackable PKI rotated it's CA certificate or you specified multiple CAs in your SecretClass. @@ -19,6 +19,7 @@ All notable changes to this project will be documented in this file. See [this GitHub issue](https://github.com/stackabletech/issues/issues/764) for details [#752]: https://github.com/stackabletech/druid-operator/pull/752 +[#753]: https://github.com/stackabletech/druid-operator/pull/753 ## [25.7.0] - 2025-07-23 From ced5c180fbe9d1328052aa46c9b2bbae815c58ad Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 23 Sep 2025 16:14:29 +0200 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Andrew Kenworthy <1712947+adwk67@users.noreply.github.com> --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f1d4b4c..b51196a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,9 +12,9 @@ All notable changes to this project will be documented in this file. - Previously we had a bug that could lead to missing certificates ([#753]). - This could be the case when the Stackable PKI rotated it's CA certificate or you specified multiple + This could be the case when the Stackable PKI rotated its CA certificate or you specified multiple CAs in your SecretClass. - Especially the CA rotation could brake working clusters at any time. + Especially the CA rotation could break working clusters at any time. We now correctly handle multiple certificates for both cases. See [this GitHub issue](https://github.com/stackabletech/issues/issues/764) for details