feat: Add format-specific annotations to override secret file names (#572)

Techassi · web-flow · commit 53945eaf6878 · 2025-03-31T15:10:31.000Z
* feat: Add format-specific annotations to override secret file names

* docs: Add annotations to volume page

* test: Add custom secret name tests

* feat: Add check against path traversal

* refactor: Flatten struct, use moved value

* refactor: Flatten compat struct, use moved value

* test: Add unit test for selector deserialization

* refactor: Adjust path traversal checks

Path::canonicalize will return an error if the path does not exist.
The path we are checking obviously doesn't exist yet, because we want
to prevent path traversals and the file at that path will only exist
after we are done with the check. So using canonicalize does not work
in this use-case.

* test: Add custom CA name

* chore: Use quoted paths in error messages

* refactor: Ensure all components of a path a normal

* chore: Add changelog entry
diff --git 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
@@ -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
@@ -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<String>,
 
-    /// 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<String>,
+    /// 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 <https://docs.stackable.tech/home/nightly/concepts/duration>.
@@ -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<String, String> {
+        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::<MapDeserializer<'_, _, serde::de::value::Error>>(
+                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::<MapDeserializer<'_, _, serde::de::value::Error>>(
+                map.into_deserializer(),
+            )
+            .unwrap();
+    }
+}
diff --git 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<PublishError> 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<SecretFormat>,
-        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
@@ -16,7 +16,7 @@ use crate::format::utils::split_pem_certificates;
 pub fn convert(
     from: WellKnownSecretData,
     to: SecretFormat,
-    compat: &CompatibilityOptions,
+    compat: CompatibilityOptions,
 ) -> Result<WellKnownSecretData, ConvertError> {
     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
@@ -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<SecretFormat>,
-        compat: &CompatibilityOptions,
+        names: NamingOptions,
+        compat: CompatibilityOptions,
     ) -> Result<SecretFiles, IntoFilesError> {
         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
diff --git a/tests/templates/kuttl/tls/consumer.yaml.j2 b/tests/templates/kuttl/tls/consumer.yaml.j2
diff --git a/tests/test-definition.yaml b/tests/test-definition.yaml
