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..4a4a6b39 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, @@ -198,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, @@ -220,6 +227,21 @@ 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, + }, } type Result = std::result::Result; @@ -235,12 +257,63 @@ 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()), + } + } +} + +/// 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 { + let truststore_kind = ::kind(&()); + existing_owners.iter().any(|owner| { + owner.controller == Some(true) + && 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 { @@ -311,6 +384,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 +404,14 @@ async fn reconcile( binary_data: Some(binary_data), ..Default::default() }; + 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 @@ -333,6 +426,14 @@ async fn reconcile( data: Some(binary_data), ..Default::default() }; + 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 @@ -352,3 +453,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")); + } +}