From 940dde4a0cba3157f980a5547697d753ab693b7e Mon Sep 17 00:00:00 2001 From: dervoeti Date: Mon, 18 May 2026 15:02:27 +0200 Subject: [PATCH 1/2] fix: Refuse to overwrite foreign objects from TrustStore reconciler --- CHANGELOG.md | 4 +- .../src/truststore_controller.rs | 171 ++++++++++++++++++ 2 files changed, 174 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad70d71c..bd3fe4d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,10 +15,12 @@ All notable changes to this project will be documented in this file. ### Fixed -- Redact the user-provided PKCS#12 keystore password in operator logs. ([#706]). +- Redact the user-provided PKCS#12 keystore password in operator logs ([#706]). +- Refuse to overwrite pre-existing ConfigMaps or Secrets that are not owned by the reconciling TrustStore ([#707]). [#693]: https://github.com/stackabletech/secret-operator/pull/693 [#706]: https://github.com/stackabletech/secret-operator/pull/706 +[#707]: https://github.com/stackabletech/secret-operator/pull/707 ## [26.3.0] - 2026-03-16 diff --git a/rust/operator-binary/src/truststore_controller.rs b/rust/operator-binary/src/truststore_controller.rs index 430d8220..d4cd5f33 100644 --- a/rust/operator-binary/src/truststore_controller.rs +++ b/rust/operator-binary/src/truststore_controller.rs @@ -8,6 +8,7 @@ use stackable_operator::{ k8s_openapi::{ ByteString, api::core::v1::{ConfigMap, Secret}, + apimachinery::pkg::apis::meta::v1::OwnerReference, }, kube::{ Resource, @@ -220,6 +221,27 @@ pub enum Error { source: stackable_operator::client::Error, secret: ObjectRef, }, + + #[snafu(display( + "failed to look up pre-existing target {object} before applying the TrustStore" + ))] + GetExistingTarget { + source: stackable_operator::client::Error, + object: ObjectRef, + }, + + #[snafu(display( + "refusing to overwrite pre-existing {object} that is not owned by this TrustStore" + ))] + RefuseToOverwriteForeignTarget { + object: ObjectRef, + }, + + #[snafu(display("TrustStore has no associated UID"))] + NoTrustStoreUid, + + #[snafu(display("TrustStore has no associated name"))] + NoTrustStoreName, } type Result = std::result::Result; @@ -239,10 +261,28 @@ impl ReconcilerError for Error { Error::BuildOwnerReference { .. } => None, Error::ApplyTrustStoreConfigMap { config_map, .. } => Some(config_map.clone().erase()), Error::ApplyTrustStoreSecret { secret, .. } => Some(secret.clone().erase()), + Error::GetExistingTarget { object, .. } => Some(object.clone()), + Error::RefuseToOverwriteForeignTarget { object } => Some(object.clone()), + Error::NoTrustStoreUid => None, + Error::NoTrustStoreName => None, } } } +/// Returns `true` if `existing_owners` contain a controller `OwnerReference` that points to the +/// TrustStore with the given UID. Used to ensure we only overwrite output ConfigMaps and Secrets +/// that we previously created ourselves; refusing otherwise prevents the TrustStore primitive +/// from being abused to clobber foreign same-named objects (e.g. `kube-root-ca.crt`) via the +/// operator's elevated cluster-wide write permissions. +fn is_owned_by_truststore(existing_owners: &[OwnerReference], truststore_uid: &str) -> bool { + existing_owners.iter().any(|owner| { + owner.controller == Some(true) + && owner.kind == "TrustStore" + && owner.api_version.starts_with("secrets.stackable.tech/") + && owner.uid == truststore_uid + }) +} + struct Ctx { client: stackable_operator::client::Client, } @@ -311,6 +351,18 @@ async fn reconcile( .context(BuildOwnerReferenceSnafu)? .build(); + let truststore_name = truststore + .metadata + .name + .as_deref() + .context(NoTrustStoreNameSnafu)?; + let truststore_namespace = selector.namespace.as_str(); + let truststore_uid = truststore + .metadata + .uid + .as_deref() + .context(NoTrustStoreUidSnafu)?; + match truststore.spec.target_kind { v1alpha1::TrustStoreOutputType::ConfigMap => { let trust_cm = ConfigMap { @@ -319,6 +371,22 @@ async fn reconcile( binary_data: Some(binary_data), ..Default::default() }; + let existing = ctx + .client + .get_opt::(truststore_name, truststore_namespace) + .await + .with_context(|_| GetExistingTargetSnafu { + object: ObjectRef::from_obj(&trust_cm).erase(), + })?; + if let Some(existing) = existing { + let owners = existing.metadata.owner_references.as_deref().unwrap_or(&[]); + if !is_owned_by_truststore(owners, truststore_uid) { + return RefuseToOverwriteForeignTargetSnafu { + object: ObjectRef::from_obj(&trust_cm).erase(), + } + .fail(); + } + } ctx.client .apply_patch(CONTROLLER_NAME, &trust_cm, &trust_cm) .await @@ -333,6 +401,22 @@ async fn reconcile( data: Some(binary_data), ..Default::default() }; + let existing = ctx + .client + .get_opt::(truststore_name, truststore_namespace) + .await + .with_context(|_| GetExistingTargetSnafu { + object: ObjectRef::from_obj(&trust_secret).erase(), + })?; + if let Some(existing) = existing { + let owners = existing.metadata.owner_references.as_deref().unwrap_or(&[]); + if !is_owned_by_truststore(owners, truststore_uid) { + return RefuseToOverwriteForeignTargetSnafu { + object: ObjectRef::from_obj(&trust_secret).erase(), + } + .fail(); + } + } ctx.client .apply_patch(CONTROLLER_NAME, &trust_secret, &trust_secret) .await @@ -352,3 +436,90 @@ fn error_policy( ) -> controller::Action { controller::Action::requeue(Duration::from_secs(5)) } + +#[cfg(test)] +mod tests { + use super::*; + + fn owner_ref( + api_version: &str, + kind: &str, + uid: &str, + controller: Option, + ) -> OwnerReference { + OwnerReference { + api_version: api_version.to_string(), + kind: kind.to_string(), + name: "some-name".to_string(), + uid: uid.to_string(), + controller, + block_owner_deletion: None, + } + } + + #[test] + fn empty_owner_refs_are_rejected() { + assert!(!is_owned_by_truststore(&[], "my-uid")); + } + + #[test] + fn matching_controller_owner_ref_is_accepted() { + let owners = [owner_ref( + "secrets.stackable.tech/v1alpha1", + "TrustStore", + "my-uid", + Some(true), + )]; + assert!(is_owned_by_truststore(&owners, "my-uid")); + } + + #[test] + fn non_controller_owner_ref_is_rejected() { + let owners = [owner_ref( + "secrets.stackable.tech/v1alpha1", + "TrustStore", + "my-uid", + Some(false), + )]; + assert!(!is_owned_by_truststore(&owners, "my-uid")); + } + + #[test] + fn different_uid_is_rejected() { + let owners = [owner_ref( + "secrets.stackable.tech/v1alpha1", + "TrustStore", + "other-uid", + Some(true), + )]; + assert!(!is_owned_by_truststore(&owners, "my-uid")); + } + + #[test] + fn different_kind_is_rejected() { + let owners = [owner_ref("v1", "ConfigMap", "my-uid", Some(true))]; + assert!(!is_owned_by_truststore(&owners, "my-uid")); + } + + #[test] + fn foreign_api_group_is_rejected() { + let owners = [owner_ref( + "evil.example.com/v1", + "TrustStore", + "my-uid", + Some(true), + )]; + assert!(!is_owned_by_truststore(&owners, "my-uid")); + } + + #[test] + fn unrelated_controller_owner_ref_is_rejected() { + let owners = [owner_ref( + "apps/v1", + "Deployment", + "some-deployment-uid", + Some(true), + )]; + assert!(!is_owned_by_truststore(&owners, "my-uid")); + } +} From d39f8c70c91360f80a8e403dc387258ddfbaf92a Mon Sep 17 00:00:00 2001 From: dervoeti Date: Mon, 18 May 2026 16:39:15 +0200 Subject: [PATCH 2/2] fix: address review feedback --- .../src/truststore_controller.rs | 99 +++++++++++-------- 1 file changed, 58 insertions(+), 41 deletions(-) diff --git a/rust/operator-binary/src/truststore_controller.rs b/rust/operator-binary/src/truststore_controller.rs index d4cd5f33..4a4a6b39 100644 --- a/rust/operator-binary/src/truststore_controller.rs +++ b/rust/operator-binary/src/truststore_controller.rs @@ -199,6 +199,12 @@ pub enum Error { #[snafu(display("TrustStore has no associated Namespace"))] NoTrustStoreNamespace, + #[snafu(display("TrustStore has no associated name"))] + NoTrustStoreName, + + #[snafu(display("TrustStore has no associated UID"))] + NoTrustStoreUid, + #[snafu(display("failed to convert trust data into desired format"))] FormatData { source: format::IntoFilesError, @@ -236,12 +242,6 @@ pub enum Error { RefuseToOverwriteForeignTarget { object: ObjectRef, }, - - #[snafu(display("TrustStore has no associated UID"))] - NoTrustStoreUid, - - #[snafu(display("TrustStore has no associated name"))] - NoTrustStoreName, } type Result = std::result::Result; @@ -257,14 +257,14 @@ impl ReconcilerError for Error { Error::InitBackend { secret_class, .. } => Some(secret_class.clone().erase()), Error::BackendGetTrustData { source } => source.secondary_object(), Error::NoTrustStoreNamespace => None, + Error::NoTrustStoreName => None, + Error::NoTrustStoreUid => None, Error::FormatData { secret_class, .. } => Some(secret_class.clone().erase()), Error::BuildOwnerReference { .. } => None, Error::ApplyTrustStoreConfigMap { config_map, .. } => Some(config_map.clone().erase()), Error::ApplyTrustStoreSecret { secret, .. } => Some(secret.clone().erase()), Error::GetExistingTarget { object, .. } => Some(object.clone()), Error::RefuseToOverwriteForeignTarget { object } => Some(object.clone()), - Error::NoTrustStoreUid => None, - Error::NoTrustStoreName => None, } } } @@ -275,14 +275,47 @@ impl ReconcilerError for Error { /// from being abused to clobber foreign same-named objects (e.g. `kube-root-ca.crt`) via the /// operator's elevated cluster-wide write permissions. fn is_owned_by_truststore(existing_owners: &[OwnerReference], truststore_uid: &str) -> bool { + let truststore_kind = ::kind(&()); existing_owners.iter().any(|owner| { owner.controller == Some(true) - && owner.kind == "TrustStore" + && owner.kind == truststore_kind && owner.api_version.starts_with("secrets.stackable.tech/") && owner.uid == truststore_uid }) } +/// Looks up a pre-existing object with the same name/namespace as the TrustStore output and fails +/// if it exists but is not controlled by this TrustStore. See [`is_owned_by_truststore`] for the +/// security rationale. +async fn ensure_existing_target_is_not_foreign( + client: &stackable_operator::client::Client, + name: &str, + namespace: &str, + truststore_uid: &str, + object_ref: ObjectRef, +) -> Result<()> +where + K: stackable_operator::kube::Resource + + stackable_operator::client::GetApi + + Clone + + std::fmt::Debug + + serde::de::DeserializeOwned, +{ + let existing = client + .get_opt::(name, namespace) + .await + .with_context(|_| GetExistingTargetSnafu { + object: object_ref.clone(), + })?; + if let Some(existing) = existing { + let owners = existing.meta().owner_references.as_deref().unwrap_or(&[]); + if !is_owned_by_truststore(owners, truststore_uid) { + return RefuseToOverwriteForeignTargetSnafu { object: object_ref }.fail(); + } + } + Ok(()) +} + struct Ctx { client: stackable_operator::client::Client, } @@ -371,22 +404,14 @@ async fn reconcile( binary_data: Some(binary_data), ..Default::default() }; - let existing = ctx - .client - .get_opt::(truststore_name, truststore_namespace) - .await - .with_context(|_| GetExistingTargetSnafu { - object: ObjectRef::from_obj(&trust_cm).erase(), - })?; - if let Some(existing) = existing { - let owners = existing.metadata.owner_references.as_deref().unwrap_or(&[]); - if !is_owned_by_truststore(owners, truststore_uid) { - return RefuseToOverwriteForeignTargetSnafu { - object: ObjectRef::from_obj(&trust_cm).erase(), - } - .fail(); - } - } + ensure_existing_target_is_not_foreign::( + &ctx.client, + truststore_name, + truststore_namespace, + truststore_uid, + ObjectRef::from_obj(&trust_cm).erase(), + ) + .await?; ctx.client .apply_patch(CONTROLLER_NAME, &trust_cm, &trust_cm) .await @@ -401,22 +426,14 @@ async fn reconcile( data: Some(binary_data), ..Default::default() }; - let existing = ctx - .client - .get_opt::(truststore_name, truststore_namespace) - .await - .with_context(|_| GetExistingTargetSnafu { - object: ObjectRef::from_obj(&trust_secret).erase(), - })?; - if let Some(existing) = existing { - let owners = existing.metadata.owner_references.as_deref().unwrap_or(&[]); - if !is_owned_by_truststore(owners, truststore_uid) { - return RefuseToOverwriteForeignTargetSnafu { - object: ObjectRef::from_obj(&trust_secret).erase(), - } - .fail(); - } - } + ensure_existing_target_is_not_foreign::( + &ctx.client, + truststore_name, + truststore_namespace, + truststore_uid, + ObjectRef::from_obj(&trust_secret).erase(), + ) + .await?; ctx.client .apply_patch(CONTROLLER_NAME, &trust_secret, &trust_secret) .await