From 357769bc9cc3d66df112bc13c84f1f63b7c545dc Mon Sep 17 00:00:00 2001 From: Lars Francke Date: Fri, 6 Nov 2026 17:04:53 +0100 Subject: [PATCH 1/3] fix: Keep published secrets on repeated NodePublishVolume The kubelet calls NodePublishVolume again for volumes it already published, for example after a kubelet restart. Every call issued a new secret and moved the restarter's expiry annotation to it. A process that loaded its secret at startup kept using the old one, so the restarter acted on the wrong expiry and could restart the Pod only after the secret in use had expired. A completely published volume now contains an empty marker file, written after the secret files, and a repeated publish returns early when it exists. --- CHANGELOG.md | 2 + rust/operator-binary/src/csi_server/node.rs | 73 +++++++++++++++++++++ 2 files changed, 75 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7557f73f..d9dcaa9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ All notable changes to this project will be documented in this file. `serviceAccount.create=false` now requires `serviceAccount.name`; it used to fall back to the namespace default ServiceAccount, which lacks the operator ClusterRole ([#736]). - `CreateVolume` no longer returns gRPC codes that make external-provisioner retry indefinitely ([#743]). +- A repeated `NodePublishVolume` (for example after a kubelet restart) keeps the published secret instead of issuing a new one, so the expiry annotation keeps matching the secret the Pod uses ([#773]). - Report the certificate expiry for the `certManager` backend, so that Pods are restarted before their certificate expires and pick up the renewed one. Previously no expiry was reported at all, so a Pod kept the certificate it was given at startup and eventually ran on an expired one @@ -39,6 +40,7 @@ All notable changes to this project will be documented in this file. [#762]: https://github.com/stackabletech/secret-operator/pull/762 [#763]: https://github.com/stackabletech/secret-operator/pull/763 [#766]: https://github.com/stackabletech/secret-operator/pull/766 +[#773]: https://github.com/stackabletech/secret-operator/pull/773 ## [26.7.0] - 2026-07-21 diff --git a/rust/operator-binary/src/csi_server/node.rs b/rust/operator-binary/src/csi_server/node.rs index 822a7a06..469f6b53 100644 --- a/rust/operator-binary/src/csi_server/node.rs +++ b/rust/operator-binary/src/csi_server/node.rs @@ -93,6 +93,12 @@ enum PublishError { path: PathBuf, }, + #[snafu(display("failed to check whether published marker {path:?} exists"))] + CheckPublishedMarker { + source: std::io::Error, + path: PathBuf, + }, + #[snafu(display("file path {path:?} must only contain normal components"))] InvalidComponents { path: PathBuf }, @@ -127,6 +133,7 @@ impl From for Status { PublishError::SetDirPermissions { .. } => Status::unavailable(full_msg), PublishError::CreateFile { .. } => Status::unavailable(full_msg), PublishError::WriteFile { .. } => Status::unavailable(full_msg), + PublishError::CheckPublishedMarker { .. } => Status::unavailable(full_msg), PublishError::InvalidComponents { .. } => Status::unavailable(full_msg), PublishError::InvalidAbsolutePath { .. } => Status::unavailable(full_msg), PublishError::TagPod { .. } => Status::unavailable(full_msg), @@ -373,6 +380,32 @@ impl SecretProvisionerNode { } } +/// Empty file whose presence means that the volume has been published completely. +/// +/// It is a dotfile so that `*` globs over the volume, as in `cp /stackable/tls/* ...`, skip it. +const PUBLISHED_MARKER_FILE_NAME: &str = ".secret-operator-published"; + +async fn is_published(target_path: &Path) -> Result { + let path = target_path.join(PUBLISHED_MARKER_FILE_NAME); + tokio::fs::try_exists(&path) + .await + .context(publish_error::CheckPublishedMarkerSnafu { path }) +} + +/// Must be called after all secret files are written. +async fn mark_published(target_path: &Path) -> Result<(), PublishError> { + let path = target_path.join(PUBLISHED_MARKER_FILE_NAME); + OpenOptions::new() + .create(true) + .write(true) + .truncate(true) + .mode(0o640) + .open(&path) + .await + .context(publish_error::CreateFileSnafu { path })?; + Ok(()) +} + // Most of the services are not yet implemented, most of them will never be, because they are // not needed for this use case. // The main two services are publish_volume und unpublish_volume, which get called whenever a @@ -408,6 +441,17 @@ impl Node for SecretProvisionerNode { volume.path = %target_path.display(), "Received NodePublishVolume request" ); + // The kubelet calls NodePublishVolume again for volumes it has already published, + // for example after a kubelet restart. Issuing a new secret would move the expiry + // annotation to a secret that a running process may never load, so the restarter + // could restart the Pod only after the secret in use has expired. + if is_published(&target_path).await? { + tracing::info!( + volume.path = %target_path.display(), + "Volume is already published, keeping its secret data" + ); + return Ok(Response::new(NodePublishVolumeResponse {})); + } let selector = SecretVolumeSelector::deserialize(request.volume_context.into_deserializer()) .context(publish_error::InvalidSelectorSnafu)?; @@ -430,6 +474,7 @@ impl Node for SecretProvisionerNode { selector ) .await?; + mark_published(&target_path).await?; Ok(Response::new(NodePublishVolumeResponse {})) } .await, @@ -496,3 +541,31 @@ impl Node for SecretProvisionerNode { })) } } + +#[cfg(test)] +mod tests { + use super::{is_published, mark_published}; + + #[tokio::test] + async fn empty_volume_is_not_published() { + let volume = tempfile::tempdir().expect("temporary directory can be created"); + assert!( + !is_published(volume.path()) + .await + .expect("volume directory is readable") + ); + } + + #[tokio::test] + async fn marked_volume_is_published() { + let volume = tempfile::tempdir().expect("temporary directory can be created"); + mark_published(volume.path()) + .await + .expect("published marker can be written"); + assert!( + is_published(volume.path()) + .await + .expect("volume directory is readable") + ); + } +} From 779a0c023be55869043385417c3c46825d97e668 Mon Sep 17 00:00:00 2001 From: Lars Francke Date: Mon, 9 Nov 2026 14:30:42 +0100 Subject: [PATCH 2/3] fix: Don't follow symlinks for the provisioned marker Processes in the Pod can write to the volume, because the kubelet applies the fsGroup with group write permissions. Checking for the marker no longer follows a symlink, and any entry under its name counts as provisioned: returning early writes nothing, while reissuing on a marker that cannot be written would issue a new secret on every kubelet retry. Writing the marker refuses to follow a symlink. The marker is renamed to .stackable-secret-operator-data-provisioned to avoid collisions with files from other tools. --- rust/operator-binary/src/csi_server/node.rs | 78 +++++++++++++++------ 1 file changed, 56 insertions(+), 22 deletions(-) diff --git a/rust/operator-binary/src/csi_server/node.rs b/rust/operator-binary/src/csi_server/node.rs index adfe97e2..acbd977a 100644 --- a/rust/operator-binary/src/csi_server/node.rs +++ b/rust/operator-binary/src/csi_server/node.rs @@ -99,8 +99,8 @@ enum PublishError { path: PathBuf, }, - #[snafu(display("failed to check whether published marker {path:?} exists"))] - CheckPublishedMarker { + #[snafu(display("failed to check whether provisioned marker {path:?} exists"))] + CheckProvisionedMarker { source: std::io::Error, path: PathBuf, }, @@ -140,7 +140,7 @@ impl From for Status { PublishError::SetDirPermissions { .. } => Status::unavailable(full_msg), PublishError::CreateFile { .. } => Status::unavailable(full_msg), PublishError::WriteFile { .. } => Status::unavailable(full_msg), - PublishError::CheckPublishedMarker { .. } => Status::unavailable(full_msg), + PublishError::CheckProvisionedMarker { .. } => Status::unavailable(full_msg), PublishError::InvalidComponents { .. } => Status::unavailable(full_msg), PublishError::InvalidAbsolutePath { .. } => Status::unavailable(full_msg), PublishError::TagPod { .. } => Status::unavailable(full_msg), @@ -423,26 +423,35 @@ async fn is_mount_point(path: &Path) -> Result { } } -/// Empty file whose presence means that the volume has been published completely. +/// Empty file whose presence means that the secret data has been provisioned completely. /// /// It is a dotfile so that `*` globs over the volume, as in `cp /stackable/tls/* ...`, skip it. -const PUBLISHED_MARKER_FILE_NAME: &str = ".secret-operator-published"; - -async fn is_published(target_path: &Path) -> Result { - let path = target_path.join(PUBLISHED_MARKER_FILE_NAME); - tokio::fs::try_exists(&path) - .await - .context(publish_error::CheckPublishedMarkerSnafu { path }) +/// +/// Processes in the Pod can write to the volume, so the marker might have been replaced by a +/// symlink or another kind of file. [`is_provisioned`] counts any entry without following it. +/// Because if there's _something_ there already we won't tamper with it and [`mark_provisioned`] +/// will not overwrite it anyway. Effectively that means if someone writes anything with that name +/// then you're on your own. The operator counts it as provisioned. +const PROVISIONED_MARKER_FILE_NAME: &str = ".stackable-secret-operator-data-provisioned"; + +async fn is_provisioned(target_path: &Path) -> Result { + let path = target_path.join(PROVISIONED_MARKER_FILE_NAME); + match tokio::fs::symlink_metadata(&path).await { + Ok(_) => Ok(true), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(false), + Err(err) => Err(err).context(publish_error::CheckProvisionedMarkerSnafu { path }), + } } /// Must be called after all secret files are written. -async fn mark_published(target_path: &Path) -> Result<(), PublishError> { - let path = target_path.join(PUBLISHED_MARKER_FILE_NAME); +async fn mark_provisioned(target_path: &Path) -> Result<(), PublishError> { + let path = target_path.join(PROVISIONED_MARKER_FILE_NAME); OpenOptions::new() .create(true) .write(true) .truncate(true) .mode(0o640) + .custom_flags(libc::O_NOFOLLOW) .open(&path) .await .context(publish_error::CreateFileSnafu { path })?; @@ -488,7 +497,7 @@ impl Node for SecretProvisionerNode { // for example after a kubelet restart. Issuing a new secret would move the expiry // annotation to a secret that a running process may never load, so the restarter // could restart the Pod only after the secret in use has expired. - if is_published(&target_path).await? { + if is_provisioned(&target_path).await? { tracing::info!( volume.path = %target_path.display(), "Volume is already published, keeping its secret data" @@ -517,7 +526,7 @@ impl Node for SecretProvisionerNode { selector ) .await?; - mark_published(&target_path).await?; + mark_provisioned(&target_path).await?; Ok(Response::new(NodePublishVolumeResponse {})) } .await, @@ -589,7 +598,7 @@ impl Node for SecretProvisionerNode { mod tests { use std::path::Path; - use super::{is_mount_point, is_published, mark_published}; + use super::{PROVISIONED_MARKER_FILE_NAME, is_mount_point, is_provisioned, mark_provisioned}; #[tokio::test] async fn plain_directory_is_not_mount_point() { @@ -611,25 +620,50 @@ mod tests { } #[tokio::test] - async fn empty_volume_is_not_published() { + async fn empty_volume_is_not_provisioned() { let volume = tempfile::tempdir().expect("temporary directory can be created"); assert!( - !is_published(volume.path()) + !is_provisioned(volume.path()) .await .expect("volume directory is readable") ); } #[tokio::test] - async fn marked_volume_is_published() { + async fn marked_volume_is_provisioned() { let volume = tempfile::tempdir().expect("temporary directory can be created"); - mark_published(volume.path()) + mark_provisioned(volume.path()) .await - .expect("published marker can be written"); + .expect("provisioned marker can be written"); assert!( - is_published(volume.path()) + is_provisioned(volume.path()) .await .expect("volume directory is readable") ); } + + #[tokio::test] + async fn dangling_symlink_marker_is_provisioned() { + let volume = tempfile::tempdir().expect("temporary directory can be created"); + std::os::unix::fs::symlink( + volume.path().join("missing"), + volume.path().join(PROVISIONED_MARKER_FILE_NAME), + ) + .expect("symlink can be created"); + assert!( + is_provisioned(volume.path()) + .await + .expect("volume directory is readable") + ); + } + + #[tokio::test] + async fn marking_does_not_follow_symlink() { + let volume = tempfile::tempdir().expect("temporary directory can be created"); + let target = volume.path().join("target"); + std::os::unix::fs::symlink(&target, volume.path().join(PROVISIONED_MARKER_FILE_NAME)) + .expect("symlink can be created"); + assert!(mark_provisioned(volume.path()).await.is_err()); + assert!(!target.exists()); + } } From 3f79d8440f3379e3ea073ed1b7c14f00a87a5b83 Mon Sep 17 00:00:00 2001 From: Lars Francke Date: Tue, 10 Nov 2026 14:41:25 +0100 Subject: [PATCH 3/3] fix: Reject a provisioned marker that is not a regular file Counting any entry as provisioned hid tampering. Failing the publish instead stops before the backend is called, so a tampered marker still cannot make every kubelet retry issue a new secret, and the error shows up in the Pod's events. Only the Pod that changed its own volume is affected. --- rust/operator-binary/src/csi_server/node.rs | 22 ++++++++++----------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/rust/operator-binary/src/csi_server/node.rs b/rust/operator-binary/src/csi_server/node.rs index acbd977a..3b2567d6 100644 --- a/rust/operator-binary/src/csi_server/node.rs +++ b/rust/operator-binary/src/csi_server/node.rs @@ -105,6 +105,9 @@ enum PublishError { path: PathBuf, }, + #[snafu(display("provisioned marker {path:?} is not a regular file"))] + InvalidProvisionedMarker { path: PathBuf }, + #[snafu(display("file path {path:?} must only contain normal components"))] InvalidComponents { path: PathBuf }, @@ -141,6 +144,7 @@ impl From for Status { PublishError::CreateFile { .. } => Status::unavailable(full_msg), PublishError::WriteFile { .. } => Status::unavailable(full_msg), PublishError::CheckProvisionedMarker { .. } => Status::unavailable(full_msg), + PublishError::InvalidProvisionedMarker { .. } => Status::failed_precondition(full_msg), PublishError::InvalidComponents { .. } => Status::unavailable(full_msg), PublishError::InvalidAbsolutePath { .. } => Status::unavailable(full_msg), PublishError::TagPod { .. } => Status::unavailable(full_msg), @@ -427,17 +431,15 @@ async fn is_mount_point(path: &Path) -> Result { /// /// It is a dotfile so that `*` globs over the volume, as in `cp /stackable/tls/* ...`, skip it. /// -/// Processes in the Pod can write to the volume, so the marker might have been replaced by a -/// symlink or another kind of file. [`is_provisioned`] counts any entry without following it. -/// Because if there's _something_ there already we won't tamper with it and [`mark_provisioned`] -/// will not overwrite it anyway. Effectively that means if someone writes anything with that name -/// then you're on your own. The operator counts it as provisioned. +/// Processes in the Pod can write to the volume. +/// Anything other than a regular file is rejected const PROVISIONED_MARKER_FILE_NAME: &str = ".stackable-secret-operator-data-provisioned"; async fn is_provisioned(target_path: &Path) -> Result { let path = target_path.join(PROVISIONED_MARKER_FILE_NAME); match tokio::fs::symlink_metadata(&path).await { - Ok(_) => Ok(true), + Ok(metadata) if metadata.is_file() => Ok(true), + Ok(_) => publish_error::InvalidProvisionedMarkerSnafu { path }.fail(), Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(false), Err(err) => Err(err).context(publish_error::CheckProvisionedMarkerSnafu { path }), } @@ -643,18 +645,14 @@ mod tests { } #[tokio::test] - async fn dangling_symlink_marker_is_provisioned() { + async fn symlinked_marker_is_rejected() { let volume = tempfile::tempdir().expect("temporary directory can be created"); std::os::unix::fs::symlink( volume.path().join("missing"), volume.path().join(PROVISIONED_MARKER_FILE_NAME), ) .expect("symlink can be created"); - assert!( - is_provisioned(volume.path()) - .await - .expect("volume directory is readable") - ); + assert!(is_provisioned(volume.path()).await.is_err()); } #[tokio::test]