From d469897d66e91889eccbff68ed2bd303b33d96b6 Mon Sep 17 00:00:00 2001 From: Lars Francke Date: Tue, 10 Nov 2026 20:34:41 +0100 Subject: [PATCH 1/2] fix: Write secret files without following symlinks Processes in the Pod can write to the volume, because the kubelet applies the fsGroup with group write permissions, so any entry in it may have been replaced since the last publish. Secret files and their directories are now opened relative to their parent without following symlinks, and data is only written into regular files. --- CHANGELOG.md | 2 + Cargo.lock | 1 + Cargo.nix | 5 + Cargo.toml | 1 + rust/operator-binary/Cargo.toml | 1 + rust/operator-binary/src/csi_server/node.rs | 274 ++++++++++++++------ typos.toml | 2 + 7 files changed, 211 insertions(+), 75 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f13053f..2149a0e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ All notable changes to this project will be documented in this file. - In privileged mode, `NodePublishVolume` reuses an existing tmpfs mount instead of mounting another one on top, so a repeated publish (for example after a kubelet restart) no longer stacks mounts, which kept Pods in `Terminating` for minutes ([#770]). - `NodePublishVolume` truncates the secret files it rewrites, so a repeated publish no longer leaves trailing bytes of a longer previous file behind ([#770]). - 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]). +- `NodePublishVolume` no longer follows symlinks inside the volume and only writes secret data into regular files ([#775]). - 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 @@ -44,6 +45,7 @@ All notable changes to this project will be documented in this file. [#766]: https://github.com/stackabletech/secret-operator/pull/766 [#770]: https://github.com/stackabletech/secret-operator/pull/770 [#773]: https://github.com/stackabletech/secret-operator/pull/773 +[#775]: https://github.com/stackabletech/secret-operator/pull/775 ## [26.7.0] - 2026-07-21 diff --git a/Cargo.lock b/Cargo.lock index f442d6c1..731e7ab7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3921,6 +3921,7 @@ dependencies = [ "prost 0.14.4", "prost-types", "rand 0.10.3", + "rustix", "serde", "serde_json", "snafu 0.9.2", diff --git a/Cargo.nix b/Cargo.nix index 077068dd..36ee0138 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -13646,6 +13646,11 @@ rec { name = "rand"; packageId = "rand 0.10.3"; } + { + name = "rustix"; + packageId = "rustix"; + features = [ "fs" ]; + } { name = "serde"; packageId = "serde"; diff --git a/Cargo.toml b/Cargo.toml index 658a544d..e545726e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -39,6 +39,7 @@ pin-project = "1.1" prost = "0.14" prost-types = "0.14" rand = "0.10" +rustix = { version = "1.1", features = ["fs"] } serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" snafu = "0.9" diff --git a/rust/operator-binary/Cargo.toml b/rust/operator-binary/Cargo.toml index aefe94b4..e2751529 100644 --- a/rust/operator-binary/Cargo.toml +++ b/rust/operator-binary/Cargo.toml @@ -44,6 +44,7 @@ tonic-reflection.workspace = true tracing.workspace = true uuid.workspace = true rand.workspace = true +rustix.workspace = true yaml_serde.workspace = true [dev-dependencies] diff --git a/rust/operator-binary/src/csi_server/node.rs b/rust/operator-binary/src/csi_server/node.rs index 3b2567d6..393ed4bf 100644 --- a/rust/operator-binary/src/csi_server/node.rs +++ b/rust/operator-binary/src/csi_server/node.rs @@ -1,12 +1,17 @@ use std::{ - fs::Permissions, + fs::{File, Permissions}, + io::Write, os::unix::prelude::{MetadataExt, PermissionsExt}, path::{Component, Path, PathBuf}, }; use openssl::sha::Sha256; +use rustix::{ + fs::{CWD, FileType, Mode, OFlags, fstat, mkdirat, openat}, + io::Errno, +}; use serde::{Deserialize, de::IntoDeserializer}; -use snafu::{ResultExt, Snafu, ensure}; +use snafu::{OptionExt, ResultExt, Snafu, ensure}; use stackable_operator::{ builder::meta::ObjectMetaBuilder, k8s_openapi::api::core::v1::Pod, @@ -14,10 +19,6 @@ use stackable_operator::{ kvp::{AnnotationError, Annotations}, }; use sys_mount::{Mount, MountFlags, UnmountFlags, unmount}; -use tokio::{ - fs::{OpenOptions, create_dir_all}, - io::AsyncWriteExt, -}; use tonic::{Request, Response, Status}; use super::{controller::TOPOLOGY_NODE, log_if_endpoint_error}; @@ -108,6 +109,9 @@ enum PublishError { #[snafu(display("provisioned marker {path:?} is not a regular file"))] InvalidProvisionedMarker { path: PathBuf }, + #[snafu(display("secret file {path:?} exists but is not a regular file"))] + NotRegularFile { path: PathBuf }, + #[snafu(display("file path {path:?} must only contain normal components"))] InvalidComponents { path: PathBuf }, @@ -145,6 +149,7 @@ impl From for Status { PublishError::WriteFile { .. } => Status::unavailable(full_msg), PublishError::CheckProvisionedMarker { .. } => Status::unavailable(full_msg), PublishError::InvalidProvisionedMarker { .. } => Status::failed_precondition(full_msg), + PublishError::NotRegularFile { .. } => Status::failed_precondition(full_msg), PublishError::InvalidComponents { .. } => Status::unavailable(full_msg), PublishError::InvalidAbsolutePath { .. } => Status::unavailable(full_msg), PublishError::TagPod { .. } => Status::unavailable(full_msg), @@ -255,68 +260,12 @@ impl SecretProvisionerNode { .. } = selector; - let create_secret = { - let mut opts = OpenOptions::new(); - opts.create(true) - .write(true) - // A repeated publish rewrites existing files, and regenerated keys and - // certificates vary in length. Without truncating, a shorter file keeps the - // trailing bytes of the previous one. Not good. So we truncate. - .truncate(true) - // User: root/secret-operator - // Group: Controlled by Pod.securityContext.fsGroup, the actual application - // (when running as unprivileged user) - .mode(0o640); - opts - }; - for (k, v) in data .data .into_files(format, names, compat, provision_parts) .context(publish_error::FormatDataSnafu)? { - // 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 - .context(publish_error::CreateDirSnafu { - path: item_path_parent, - })?; - } - create_secret - .open(&item_path) - .await - .context(publish_error::CreateFileSnafu { path: &item_path })? - .write_all(&v) - .await - .context(publish_error::WriteFileSnafu { path: item_path })?; + write_secret_file(target_path, Path::new(&k), &v)?; } Ok(()) } @@ -447,17 +396,107 @@ async fn is_provisioned(target_path: &Path) -> Result { /// Must be called after all secret files are written. 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 })?; - Ok(()) + write_secret_file(target_path, Path::new(PROVISIONED_MARKER_FILE_NAME), &[]) +} + +/// Writes `contents` to `relative_path` beneath `root`, creating missing directories. +/// +/// `relative_path` can be chosen by users, so it must be relative and only consist of normal +/// components, otherwise the write fails. +/// +/// The Pod can write to the volume, so since the last publish it may have replaced any entry in +/// it, for example with a symlink. This function never follows a symlink, neither for the file +/// nor for the directories leading to it. Each directory is opened through a file descriptor of +/// the directory before it rather than by path, so replacing an entry after it has been opened +/// cannot redirect the write. It only writes into regular files: a FIFO or device node in place +/// of the file is an error. +fn write_secret_file( + root: &Path, + relative_path: &Path, + contents: &[u8], +) -> Result<(), PublishError> { + // Ensure that `relative_path` is indeed a relative path + ensure!( + !relative_path.has_root(), + publish_error::InvalidAbsolutePathSnafu { + path: relative_path + } + ); + + // Ensure that all components of the path are "normal" + // Examples for not-normal are things like "." and ".." + ensure!( + relative_path + .components() + .all(|c| matches!(c, Component::Normal(_))), + publish_error::InvalidComponentsSnafu { + path: relative_path + } + ); + + // This creates the directory structure needed to get to the relative directory + // making sure to not follow symlinks and so on + let directory_flags = OFlags::RDONLY | OFlags::DIRECTORY | OFlags::NOFOLLOW | OFlags::CLOEXEC; + let file_name = relative_path + .file_name() + .context(publish_error::InvalidComponentsSnafu { + path: relative_path, + })?; + + let mut path = root.to_path_buf(); + let mut directory = openat(CWD, root, directory_flags, Mode::empty()) + .map_err(std::io::Error::from) + .context(publish_error::CreateDirSnafu { path: &path })?; + for component in relative_path + .parent() + .into_iter() + .flat_map(Path::components) + { + let name = component.as_os_str(); + path.push(name); + match mkdirat(&directory, name, Mode::from_raw_mode(0o777)) { + Ok(()) | Err(Errno::EXIST) => {} + Err(err) => { + return Err(std::io::Error::from(err)) + .context(publish_error::CreateDirSnafu { path }); + } + } + directory = openat(&directory, name, directory_flags, Mode::empty()) + .map_err(std::io::Error::from) + .context(publish_error::CreateDirSnafu { path: &path })?; + } + + // And now finally we create the actual file from `contents` + path.push(file_name); + let file = openat( + &directory, + file_name, + OFlags::WRONLY + | OFlags::CREATE + // A repeated publish rewrites existing files, and regenerated keys and certificates + // vary in length, so a shorter file would keep trailing bytes of the previous one. + | OFlags::TRUNC + | OFlags::NOFOLLOW + | OFlags::NONBLOCK + | OFlags::CLOEXEC, + // User: root/secret-operator + // Group: Controlled by Pod.securityContext.fsGroup, the actual application + // (when running as unprivileged user) + Mode::from_raw_mode(0o640), + ) + .map_err(std::io::Error::from) + .context(publish_error::CreateFileSnafu { path: &path })?; + let file_type = fstat(&file) + .map(|stat| FileType::from_raw_mode(stat.st_mode)) + .map_err(std::io::Error::from) + .context(publish_error::CreateFileSnafu { path: &path })?; + ensure!( + file_type == FileType::RegularFile, + publish_error::NotRegularFileSnafu { path } + ); + File::from(file) + .write_all(contents) + .context(publish_error::WriteFileSnafu { path }) } // Most of the services are not yet implemented, most of them will never be, because they are @@ -600,7 +639,12 @@ impl Node for SecretProvisionerNode { mod tests { use std::path::Path; - use super::{PROVISIONED_MARKER_FILE_NAME, is_mount_point, is_provisioned, mark_provisioned}; + use rustix::fs::{CWD, FileType, Mode, mknodat}; + + use super::{ + PROVISIONED_MARKER_FILE_NAME, PublishError, is_mount_point, is_provisioned, + mark_provisioned, write_secret_file, + }; #[tokio::test] async fn plain_directory_is_not_mount_point() { @@ -664,4 +708,84 @@ mod tests { assert!(mark_provisioned(volume.path()).await.is_err()); assert!(!target.exists()); } + + #[test] + fn writes_nested_secret_file() { + let volume = tempfile::tempdir().expect("temporary directory can be created"); + write_secret_file(volume.path(), Path::new("nested/tls.crt"), b"certificate") + .expect("secret file can be written"); + let contents = + std::fs::read(volume.path().join("nested/tls.crt")).expect("secret file is readable"); + assert_eq!(contents, b"certificate"); + } + + #[test] + fn rewriting_truncates_secret_file() { + let volume = tempfile::tempdir().expect("temporary directory can be created"); + write_secret_file(volume.path(), Path::new("tls.key"), b"longer key") + .expect("secret file can be written"); + write_secret_file(volume.path(), Path::new("tls.key"), b"key") + .expect("secret file can be rewritten"); + let contents = + std::fs::read(volume.path().join("tls.key")).expect("secret file is readable"); + assert_eq!(contents, b"key"); + } + + #[test] + fn symlinked_secret_file_is_not_followed() { + let volume = tempfile::tempdir().expect("temporary directory can be created"); + let outside = tempfile::tempdir().expect("temporary directory can be created"); + let target = outside.path().join("victim"); + std::fs::write(&target, "untouched").expect("symlink target can be written"); + std::os::unix::fs::symlink(&target, volume.path().join("tls.crt")) + .expect("symlink can be created"); + + assert!(write_secret_file(volume.path(), Path::new("tls.crt"), b"certificate").is_err()); + let contents = std::fs::read_to_string(&target).expect("symlink target is readable"); + assert_eq!(contents, "untouched"); + } + + #[test] + fn symlinked_directory_is_not_followed() { + let volume = tempfile::tempdir().expect("temporary directory can be created"); + let outside = tempfile::tempdir().expect("temporary directory can be created"); + std::os::unix::fs::symlink(outside.path(), volume.path().join("nested")) + .expect("symlink can be created"); + + assert!( + write_secret_file(volume.path(), Path::new("nested/tls.crt"), b"certificate").is_err() + ); + assert!(!outside.path().join("tls.crt").exists()); + } + + #[test] + fn fifo_in_place_of_secret_file_is_rejected() { + let volume = tempfile::tempdir().expect("temporary directory can be created"); + mknodat( + CWD, + volume.path().join("tls.crt"), + FileType::Fifo, + Mode::from_raw_mode(0o600), + 0, + ) + .expect("FIFO can be created"); + + let result = write_secret_file(volume.path(), Path::new("tls.crt"), b"certificate"); + assert!(matches!( + result, + Err(PublishError::NotRegularFile { .. } | PublishError::CreateFile { .. }) + )); + } + + #[test] + fn secret_file_outside_volume_is_rejected() { + let root = tempfile::tempdir().expect("temporary directory can be created"); + let volume = root.path().join("volume"); + std::fs::create_dir(&volume).expect("volume directory can be created"); + + for path in ["../escaped", "/escaped", "nested/../../escaped"] { + assert!(write_secret_file(&volume, Path::new(path), b"certificate").is_err()); + } + assert!(!root.path().join("escaped").exists()); + } } diff --git a/typos.toml b/typos.toml index 7d0acd88..9d07e578 100644 --- a/typos.toml +++ b/typos.toml @@ -52,3 +52,5 @@ passin = "passin" tpe = "tpe" # Triple-DES EDE (encrypt-decrypt-encrypt) cipher type from the `des` crate. TdesEde3 = "TdesEde3" +# open(2) flag for write-only access, as `OFlags::WRONLY` in `rustix`. +WRONLY = "WRONLY" From 3a8771f832880383a960548d2b31677b3ce65733 Mon Sep 17 00:00:00 2001 From: Lars Francke Date: Tue, 10 Nov 2026 22:46:05 +0100 Subject: [PATCH 2/2] chore: Bump serde-saphyr to 2.1.1 --- Cargo.lock | 6 +++--- Cargo.nix | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 731e7ab7..d262a7eb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3490,12 +3490,12 @@ dependencies = [ [[package]] name = "serde-saphyr" -version = "2.1.0" +version = "2.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3f4dc2714fc5632417a6e979602d7044bad8f2880c63fc7217087180acec31b6" +checksum = "d43fb82d550b07e6f929a58d7d0935182602f48ddead875f5ee313c41c8f07fe" dependencies = [ "annotate-snippets", - "base64 0.23.1", + "base64 0.22.1", "encoding_rs_io", "granit-parser", "nohash-hasher", diff --git a/Cargo.nix b/Cargo.nix index 36ee0138..1b0ded86 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -12186,10 +12186,10 @@ rec { }; "serde-saphyr" = rec { crateName = "serde-saphyr"; - version = "2.1.0"; + version = "2.1.1"; edition = "2024"; crateBin = []; - sha256 = "1diixjn80w882xrgqqqci3rdifj4f0nn0yg9lqbj8qy59xqw4k9z"; + sha256 = "1zh7iwfc84z3brgqgbfyips049hq6l4pv3d557wyc1qbalnvhgyl"; libName = "serde_saphyr"; license = "MIT OR Apache-2.0"; dependencies = [ @@ -12200,7 +12200,7 @@ rec { } { name = "base64"; - packageId = "base64 0.23.1"; + packageId = "base64 0.22.1"; optional = true; } {