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..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", @@ -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..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; } { @@ -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"