diff --git a/CHANGELOG.md b/CHANGELOG.md index a3b36303..0182347a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,18 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Added + +- Add format-specific annotations to override secret file names ([#572]). The following new + annotations are available: + - `secrets.stackable.tech/format.tls-pkcs12.keystore-name` + - `secrets.stackable.tech/format.tls-pkcs12.truststore-name` + - `secrets.stackable.tech/format.tls-pem.cert-name` + - `secrets.stackable.tech/format.tls-pem.key-name` + - `secrets.stackable.tech/format.tls-pem.ca-name` + +[#572]: https://github.com/stackabletech/secret-operator/pull/572 + ## [25.3.0] - 2025-03-21 ### Removed diff --git a/docs/modules/secret-operator/pages/volume.adoc b/docs/modules/secret-operator/pages/volume.adoc index 1a6becee..38d7608e 100644 --- a/docs/modules/secret-operator/pages/volume.adoc +++ b/docs/modules/secret-operator/pages/volume.adoc @@ -50,6 +50,61 @@ The xref:secretclass.adoc#format[format] that the secret should be written as. This can be either the default output format of the xref:secretclass.adoc#backend[backend], or a format that it defines a conversion into. +=== `secrets.stackable.tech/format.tls-pkcs12.keystore-name` + +*Required*: false + +*Default value*: `keystore.p12` + +*Backends*: xref:secretclass.adoc#backend-autotls[] + +An alternative name for the keystore file. +Has no effect if the `format` is not `tls-pkcs12`. + +=== `secrets.stackable.tech/format.tls-pkcs12.truststore-name` + +*Required*: false + +*Default value*: `truststore.p12` + +*Backends*: xref:secretclass.adoc#backend-autotls[] + +An alternative name for the truststore file. +Has no effect if the `format` is not `tls-pkcs12`. + +=== `secrets.stackable.tech/format.tls-pem.cert-name` + +*Required*: false + +*Default value*: `tls.crt` + +*Backends*: xref:secretclass.adoc#backend-autotls[] + +An alternative name for TLS PEM certificate. +Has no effect if the `format` is not `tls-pem`. + +=== `secrets.stackable.tech/format.tls-pem.key-name` + +*Required*: false + +*Default value*: `tls.key` + +*Backends*: xref:secretclass.adoc#backend-autotls[] + +An alternative name for TLS PEM certificate key. +Has no effect if the `format` is not `tls-pem`. + +=== `secrets.stackable.tech/format.tls-pem.ca-name` + +*Required*: false + +*Default value*: `ca.crt` + +*Backends*: xref:secretclass.adoc#backend-autotls[] + +An alternative name for TLS PEM certificate authority. +Has no effect if the `format` is not `tls-pem`. + === `secrets.stackable.tech/backend.autotls.cert.lifetime` *Required*: false diff --git a/rust/operator-binary/src/backend/mod.rs b/rust/operator-binary/src/backend/mod.rs index 91de3932..61ddb815 100644 --- a/rust/operator-binary/src/backend/mod.rs +++ b/rust/operator-binary/src/backend/mod.rs @@ -25,7 +25,10 @@ use stackable_operator::{ pub use tls::TlsGenerate; use self::pod_info::SchedulingPodInfo; -use crate::format::{SecretData, SecretFormat}; +use crate::format::{ + well_known::{CompatibilityOptions, NamingOptions}, + SecretData, SecretFormat, +}; /// Configuration provided by the `Volume` selecting what secret data should be provided /// @@ -85,16 +88,13 @@ pub struct SecretVolumeSelector { )] pub kerberos_service_names: Vec, - /// The password used to encrypt the TLS PKCS#12 keystore - /// - /// Required for some applications that misbehave with blank keystore passwords (such as Hadoop). - /// Has no effect if `format` is not `tls-pkcs12`. - #[serde( - rename = "secrets.stackable.tech/format.compatibility.tls-pkcs12.password", - deserialize_with = "SecretVolumeSelector::deserialize_some", - default - )] - pub compat_tls_pkcs12_password: Option, + /// Compatibility options used by (legacy) applications. + #[serde(flatten)] + pub compat: CompatibilityOptions, + + /// The (custom) filenames used by secrets. + #[serde(flatten)] + pub names: NamingOptions, /// The TLS cert lifetime (when using the [`tls`] backend). /// The format is documented in . @@ -297,3 +297,55 @@ impl SecretBackendError for Infallible { match *self {} } } + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use serde::de::{value::MapDeserializer, IntoDeserializer}; + + use super::*; + + fn required_fields_map() -> HashMap { + HashMap::from([ + ( + "secrets.stackable.tech/class".to_owned(), + "my-class".to_owned(), + ), + ( + "csi.storage.k8s.io/pod.name".to_owned(), + "my-pod".to_owned(), + ), + ( + "csi.storage.k8s.io/pod.namespace".to_owned(), + "my-namespace".to_owned(), + ), + ]) + } + + #[test] + fn deserialize_selector() { + let map = required_fields_map(); + + let _ = + SecretVolumeSelector::deserialize::>( + map.into_deserializer(), + ) + .unwrap(); + } + + #[test] + fn deserialize_selector_compat() { + let mut map = required_fields_map(); + map.insert( + "secrets.stackable.tech/format.compatibility.tls-pkcs12.password".to_owned(), + "supersecret".to_owned(), + ); + + let _ = + SecretVolumeSelector::deserialize::>( + map.into_deserializer(), + ) + .unwrap(); + } +} diff --git a/rust/operator-binary/src/csi_server/node.rs b/rust/operator-binary/src/csi_server/node.rs index f847e8c8..43740255 100644 --- a/rust/operator-binary/src/csi_server/node.rs +++ b/rust/operator-binary/src/csi_server/node.rs @@ -1,12 +1,12 @@ use std::{ fs::Permissions, os::unix::prelude::PermissionsExt, - path::{Path, PathBuf}, + path::{Component, Path, PathBuf}, }; use openssl::sha::Sha256; use serde::{de::IntoDeserializer, Deserialize}; -use snafu::{ResultExt, Snafu}; +use snafu::{ensure, ResultExt, Snafu}; use stackable_operator::{ builder::meta::ObjectMetaBuilder, k8s_openapi::api::core::v1::Pod, @@ -23,9 +23,15 @@ use tonic::{Request, Response, Status}; use super::controller::TOPOLOGY_NODE; use crate::{ backend::{ - self, pod_info, pod_info::PodInfo, SecretBackendError, SecretContents, SecretVolumeSelector, + self, + pod_info::{self, PodInfo}, + SecretBackendError, SecretContents, SecretVolumeSelector, + }, + format::{ + self, + well_known::{CompatibilityOptions, NamingOptions}, + SecretFormat, }, - format::{self, well_known::CompatibilityOptions, SecretFormat}, grpc::csi::v1::{ node_server::Node, NodeExpandVolumeRequest, NodeExpandVolumeResponse, NodeGetCapabilitiesRequest, NodeGetCapabilitiesResponse, NodeGetInfoRequest, @@ -59,13 +65,13 @@ enum PublishError { #[snafu(display("backend failed to get secret data"))] BackendGetSecretData { source: backend::dynamic::DynError }, - #[snafu(display("failed to create secret parent dir {}", path.display()))] + #[snafu(display("failed to create secret parent dir {path:?}"))] CreateDir { source: std::io::Error, path: PathBuf, }, - #[snafu(display("failed to mount volume mount directory {}", path.display()))] + #[snafu(display("failed to mount volume mount directory {path:?}"))] Mount { source: std::io::Error, path: PathBuf, @@ -74,24 +80,30 @@ enum PublishError { #[snafu(display("failed to convert secret data into desired format"))] FormatData { source: format::IntoFilesError }, - #[snafu(display("failed to set volume permissions for {}", path.display()))] + #[snafu(display("failed to set volume permissions for {path:?}"))] SetDirPermissions { source: std::io::Error, path: PathBuf, }, - #[snafu(display("failed to create secret file {}", path.display()))] + #[snafu(display("failed to create secret file {path:?}"))] CreateFile { source: std::io::Error, path: PathBuf, }, - #[snafu(display("failed to write secret file {}", path.display()))] + #[snafu(display("failed to write secret file {path:?}"))] WriteFile { source: std::io::Error, path: PathBuf, }, + #[snafu(display("file path {path:?} must only contain normal components"))] + InvalidComponents { path: PathBuf }, + + #[snafu(display("file path {path:?} must not be absolute"))] + InvalidAbsolutePath { path: PathBuf }, + #[snafu(display("failed to tag pod with expiry metadata"))] TagPod { source: stackable_operator::client::Error, @@ -120,6 +132,8 @@ impl From for Status { PublishError::SetDirPermissions { .. } => Status::unavailable(full_msg), PublishError::CreateFile { .. } => Status::unavailable(full_msg), PublishError::WriteFile { .. } => Status::unavailable(full_msg), + PublishError::InvalidComponents { .. } => Status::unavailable(full_msg), + PublishError::InvalidAbsolutePath { .. } => Status::unavailable(full_msg), PublishError::TagPod { .. } => Status::unavailable(full_msg), PublishError::BuildAnnotation { .. } => Status::unavailable(full_msg), } @@ -209,7 +223,8 @@ impl SecretProvisionerNode { target_path: &Path, data: SecretContents, format: Option, - compat: &CompatibilityOptions, + names: NamingOptions, + compat: CompatibilityOptions, ) -> Result<(), PublishError> { let create_secret = { let mut opts = OpenOptions::new(); @@ -223,10 +238,37 @@ impl SecretProvisionerNode { }; for (k, v) in data .data - .into_files(format, compat) + .into_files(format, names, compat) .context(publish_error::FormatDataSnafu)? { - let item_path = target_path.join(k); + // The following few lines of code do some basic checks against + // unwanted path traversals. In the future, we want to leverage + // capability based filesystem operations (openat) to prevent these + // traversals. + + // First, let's turn the (potentially custom) file path into a path. + let file_path = PathBuf::from(k); + + // Next, ensure the path is not absolute (does not contain root), + // because joining an absolute path with a different path will + // replace the exiting path entirely. + ensure!( + !file_path.has_root(), + publish_error::InvalidAbsolutePathSnafu { path: &file_path } + ); + + // Ensure that the file path only contains normal components. This + // prevents any path traversals up the path using '..'. + ensure!( + file_path + .components() + .all(|c| matches!(c, Component::Normal(_))), + publish_error::InvalidComponentsSnafu { path: &file_path } + ); + + // Now, we can join the base and file path + let item_path = target_path.join(file_path); + if let Some(item_path_parent) = item_path.parent() { create_dir_all(item_path_parent) .await @@ -383,10 +425,10 @@ impl Node for SecretProvisionerNode { self.save_secret_data( &target_path, data, + // NOTE (@Techassi): At this point, we might want to pass the whole selector instead selector.format, - &CompatibilityOptions { - tls_pkcs12_password: selector.compat_tls_pkcs12_password, - }, + selector.names, + selector.compat, ) .await?; Ok(Response::new(NodePublishVolumeResponse {})) diff --git a/rust/operator-binary/src/format/convert.rs b/rust/operator-binary/src/format/convert.rs index 40a9ee6b..fb4e5cc2 100644 --- a/rust/operator-binary/src/format/convert.rs +++ b/rust/operator-binary/src/format/convert.rs @@ -16,7 +16,7 @@ use crate::format::utils::split_pem_certificates; pub fn convert( from: WellKnownSecretData, to: SecretFormat, - compat: &CompatibilityOptions, + compat: CompatibilityOptions, ) -> Result { match (from, to) { // Converting into the current format is always a no-op diff --git a/rust/operator-binary/src/format/mod.rs b/rust/operator-binary/src/format/mod.rs index d35eeeea..9995a5f2 100644 --- a/rust/operator-binary/src/format/mod.rs +++ b/rust/operator-binary/src/format/mod.rs @@ -7,6 +7,7 @@ pub use self::{ convert::ConvertError, well_known::{FromFilesError as ParseError, SecretFormat, WellKnownSecretData}, }; +use crate::format::well_known::NamingOptions; mod convert; mod utils; @@ -30,13 +31,14 @@ impl SecretData { pub fn into_files( self, format: Option, - compat: &CompatibilityOptions, + names: NamingOptions, + compat: CompatibilityOptions, ) -> Result { if let Some(format) = format { - Ok(self.parse()?.convert_to(format, compat)?.into_files()) + Ok(self.parse()?.convert_to(format, compat)?.into_files(names)) } else { Ok(match self { - SecretData::WellKnown(data) => data.into_files(), + SecretData::WellKnown(data) => data.into_files(names), SecretData::Unknown(files) => files, }) } diff --git a/rust/operator-binary/src/format/well_known.rs b/rust/operator-binary/src/format/well_known.rs index 0e3c0e0d..c56e3eb2 100644 --- a/rust/operator-binary/src/format/well_known.rs +++ b/rust/operator-binary/src/format/well_known.rs @@ -46,24 +46,24 @@ pub enum WellKnownSecretData { } impl WellKnownSecretData { - pub fn into_files(self) -> SecretFiles { + pub fn into_files(self, names: NamingOptions) -> SecretFiles { match self { WellKnownSecretData::TlsPem(TlsPem { certificate_pem, key_pem, ca_pem, }) => [ - (FILE_PEM_CERT_CERT.to_string(), certificate_pem), - (FILE_PEM_CERT_KEY.to_string(), key_pem), - (FILE_PEM_CERT_CA.to_string(), ca_pem), + (names.tls_pem_cert_name, certificate_pem), + (names.tls_pem_key_name, key_pem), + (names.tls_pem_ca_name, ca_pem), ] .into(), WellKnownSecretData::TlsPkcs12(TlsPkcs12 { keystore, truststore, }) => [ - (FILE_PKCS12_CERT_KEYSTORE.to_string(), keystore), - (FILE_PKCS12_CERT_TRUSTSTORE.to_string(), truststore), + (names.tls_pkcs12_keystore_name, keystore), + (names.tls_pkcs12_truststore_name, truststore), ] .into(), WellKnownSecretData::Kerberos(Kerberos { keytab, krb5_conf }) => [ @@ -109,7 +109,7 @@ impl WellKnownSecretData { pub fn convert_to( self, to: SecretFormat, - compat: &CompatibilityOptions, + compat: CompatibilityOptions, ) -> Result { convert::convert(self, to, compat) } @@ -118,11 +118,91 @@ impl WellKnownSecretData { /// Options that some (legacy) applications require to ensure compatibility. /// /// The expectation is that this will be unset the vast majority of the time. -#[derive(Default)] +#[derive(Debug, Default, Deserialize)] pub struct CompatibilityOptions { + /// The password used to encrypt the TLS PKCS#12 keystore + /// + /// Required for some applications that misbehave with blank keystore passwords (such as Hadoop). + /// Has no effect if `format` is not `tls-pkcs12`. + #[serde( + rename = "secrets.stackable.tech/format.compatibility.tls-pkcs12.password", + default + )] pub tls_pkcs12_password: Option, } +/// Options to customize the well-known format file names. +/// +/// The fields will either contain the default value or the custom user-provided one. This is also +/// the reason why the fields are not wrapped in [`Option`]. +#[derive(Debug, Deserialize)] +pub struct NamingOptions { + /// An alternative name used for the TLS PKCS#12 keystore file. + /// + /// Has no effect if the `format` is not `tls-pkcs12`. + #[serde( + rename = "secrets.stackable.tech/format.tls-pkcs12.keystore-name", + default = "default_pkcs12_keystore_name" + )] + pub tls_pkcs12_keystore_name: String, + + /// An alternative name used for the TLS PKCS#12 keystore file. + /// + /// Has no effect if the `format` is not `tls-pkcs12`. + #[serde( + rename = "secrets.stackable.tech/format.tls-pkcs12.truststore-name", + default = "default_pkcs12_truststore_name" + )] + pub tls_pkcs12_truststore_name: String, + + /// An alternative name used for the TLS PEM certificate. + /// + /// Has no effect if the `format` is not `tls-pem`. + #[serde( + rename = "secrets.stackable.tech/format.tls-pem.cert-name", + default = "default_tls_pem_cert_name" + )] + pub tls_pem_cert_name: String, + + /// An alternative name used for the TLS PEM certificate key. + /// + /// Has no effect if the `format` is not `tls-pem`. + #[serde( + rename = "secrets.stackable.tech/format.tls-pem.key-name", + default = "default_tls_pem_key_name" + )] + pub tls_pem_key_name: String, + + /// An alternative name used for the TLS PEM certificate authority. + /// + /// Has no effect if the `format` is not `tls-pem`. + #[serde( + rename = "secrets.stackable.tech/format.tls-pem.ca-name", + default = "default_tls_pem_ca_name" + )] + pub tls_pem_ca_name: String, +} + +fn default_pkcs12_keystore_name() -> String { + FILE_PKCS12_CERT_KEYSTORE.to_owned() +} + +fn default_pkcs12_truststore_name() -> String { + FILE_PKCS12_CERT_TRUSTSTORE.to_owned() +} + +fn default_tls_pem_cert_name() -> String { + FILE_PEM_CERT_CERT.to_owned() +} + +fn default_tls_pem_key_name() -> String { + FILE_PEM_CERT_KEY.to_owned() +} + +fn default_tls_pem_ca_name() -> String { + FILE_PEM_CERT_CA.to_owned() +} + #[derive(Snafu, Debug)] #[snafu(module)] pub enum FromFilesError { diff --git a/tests/templates/kuttl/tls/consumer.yaml b/tests/templates/kuttl/tls/consumer.yaml.j2 similarity index 72% rename from tests/templates/kuttl/tls/consumer.yaml rename to tests/templates/kuttl/tls/consumer.yaml.j2 index d00d4863..4c51e340 100644 --- a/tests/templates/kuttl/tls/consumer.yaml +++ b/tests/templates/kuttl/tls/consumer.yaml.j2 @@ -15,14 +15,22 @@ spec: args: - -c - | +{% if test_scenario['values']['custom-secret-names'] %} + CERT_NAME=custom-tls.crt + CA_NAME=custom-ca.crt +{% else %} + CERT_NAME=tls.crt + CA_NAME=ca.crt +{% endif %} + - | set -euo pipefail ls -la /stackable/tls-3d ls -la /stackable/tls-42h - cat /stackable/tls-3d/tls.crt | openssl x509 -noout -text - cat /stackable/tls-42h/tls.crt | openssl x509 -noout -text + cat "/stackable/tls-3d/$CERT_NAME" | openssl x509 -noout -text + cat "/stackable/tls-42h/$CERT_NAME" | openssl x509 -noout -text - notAfter=`cat /stackable/tls-3d/tls.crt | openssl x509 -noout -enddate| sed -e 's#notAfter=##'` + notAfter=$(cat /stackable/tls-3d/$CERT_NAME | openssl x509 -noout -enddate| sed -e 's#notAfter=##') notAfterDate=`date -d "${notAfter}" '+%s'` nowDate=`date '+%s'` diff="$((${notAfterDate}-${nowDate}))" @@ -30,7 +38,7 @@ spec: if test "${diff}" -lt "$((57*3600))"; then echo "Cert had a lifetime of less than 57 hours!" && exit 1; fi if test "${diff}" -gt "$((72*3600))"; then echo "Cert had a lifetime of greater than 72 hours!" && exit 1; fi - notAfter=`cat /stackable/tls-42h/tls.crt | openssl x509 -noout -enddate| sed -e 's#notAfter=##'` + notAfter=$(cat /stackable/tls-42h/$CERT_NAME | openssl x509 -noout -enddate| sed -e 's#notAfter=##') notAfterDate=`date -d "${notAfter}" '+%s'` nowDate=`date '+%s'` diff="$((${notAfterDate}-${nowDate}))" @@ -38,15 +46,15 @@ spec: if test "${diff}" -lt "$((33*3600))"; then echo "Cert had a lifetime of less than 33 hours!" && exit 1; fi if test "${diff}" -gt "$((42*3600))"; then echo "Cert had a lifetime of greater than 42 hours!" && exit 1; fi - cat /stackable/tls-3d/tls.crt | openssl x509 -noout -text | grep "DNS:my-tls-service.$NAMESPACE.svc.cluster.local" - cat /stackable/tls-42h/tls.crt | openssl x509 -noout -text | grep "DNS:my-tls-service.$NAMESPACE.svc.cluster.local" + cat "/stackable/tls-3d/$CERT_NAME" | openssl x509 -noout -text | grep "DNS:my-tls-service.$NAMESPACE.svc.cluster.local" + cat "/stackable/tls-42h/$CERT_NAME" | openssl x509 -noout -text | grep "DNS:my-tls-service.$NAMESPACE.svc.cluster.local" function assert_trusted_roots_contain { subject=$1 while openssl x509 -subject -noout; do :; done \ - < /stackable/tls-3d/ca.crt \ + < "/stackable/tls-3d/$CA_NAME" \ | grep --line-regexp "subject=CN *= *$subject" } @@ -73,6 +81,10 @@ spec: secrets.stackable.tech/class: tls-$NAMESPACE secrets.stackable.tech/scope: node,pod,service=my-tls-service secrets.stackable.tech/backend.autotls.cert.lifetime: 3d +{% if test_scenario['values']['custom-secret-names'] %} + secrets.stackable.tech/format.tls-pem.cert-name: custom-tls.crt + secrets.stackable.tech/format.tls-pem.ca-name: custom-ca.crt +{% endif %} spec: storageClassName: secrets.stackable.tech accessModes: @@ -88,6 +100,10 @@ spec: secrets.stackable.tech/class: tls-$NAMESPACE-42h secrets.stackable.tech/scope: node,pod,service=my-tls-service secrets.stackable.tech/backend.autotls.cert.lifetime: 31d +{% if test_scenario['values']['custom-secret-names'] %} + secrets.stackable.tech/format.tls-pem.cert-name: custom-tls.crt + secrets.stackable.tech/format.tls-pem.ca-name: custom-ca.crt +{% endif %} spec: storageClassName: secrets.stackable.tech accessModes: diff --git a/tests/test-definition.yaml b/tests/test-definition.yaml index 91cdc540..892153e2 100644 --- a/tests/test-definition.yaml +++ b/tests/test-definition.yaml @@ -15,6 +15,10 @@ dimensions: - 2048 - 3072 # - 4096 + - name: custom-secret-names + values: + - false + - true tests: - name: kerberos dimensions: @@ -30,6 +34,7 @@ tests: - openshift - name: tls dimensions: + - custom-secret-names - rsa-key-length - openshift - name: cert-manager-tls