From e06df144559db0ffd81a5ae594c6f548dc324c93 Mon Sep 17 00:00:00 2001 From: Lars Francke Date: Wed, 11 Nov 2026 08:32:50 +0100 Subject: [PATCH] feat: Make secret volumes read-only for the Pod's fsGroup With fsGroupPolicy File, the kubelet gives the Pod's fsGroup read and write access to everything in the volume after each publish. The node service now advertises the VOLUME_MOUNT_GROUP capability, so the kubelet passes the fsGroup instead, and the volume is owned by that group with 0750 for directories and 0640 for files. The CSI spec recommends read-write access for the mount group. Read-only is deliberate: the publish path writes into the volume again on a repeated NodePublishVolume, so the Pod must not be able to change its entries. Products that write into a secret volume break with this change. --- CHANGELOG.md | 1 + rust/operator-binary/src/csi_server/node.rs | 172 +++++++++++++++++--- 2 files changed, 153 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2149a0e7..1e63d21e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ All notable changes to this project will be documented in this file. ### Changed +- BREAKING: The Pod's fsGroup only gets read access to secret volumes instead of write access. Products must not write into these volumes ([#XXX]). - Bump stackable-operator to 0.121.0 ([#730], [#735], [#759], [#763]). - All pods now set `securityContext.seccompProfile.type` to `RuntimeDefault` ([#763]). - Replace the unmaintained `serde_yaml` with `yaml_serde` ([#763]). diff --git a/rust/operator-binary/src/csi_server/node.rs b/rust/operator-binary/src/csi_server/node.rs index 393ed4bf..623cbaf7 100644 --- a/rust/operator-binary/src/csi_server/node.rs +++ b/rust/operator-binary/src/csi_server/node.rs @@ -7,7 +7,7 @@ use std::{ use openssl::sha::Sha256; use rustix::{ - fs::{CWD, FileType, Mode, OFlags, fstat, mkdirat, openat}, + fs::{CWD, FileType, Gid, Mode, OFlags, fchmod, fchown, fstat, mkdirat, openat}, io::Errno, }; use serde::{Deserialize, de::IntoDeserializer}; @@ -32,9 +32,10 @@ use crate::{ NodeExpandVolumeRequest, NodeExpandVolumeResponse, NodeGetCapabilitiesRequest, NodeGetCapabilitiesResponse, NodeGetInfoRequest, NodeGetInfoResponse, NodeGetVolumeStatsRequest, NodeGetVolumeStatsResponse, NodePublishVolumeRequest, - NodePublishVolumeResponse, NodeStageVolumeRequest, NodeStageVolumeResponse, - NodeUnpublishVolumeRequest, NodeUnpublishVolumeResponse, NodeUnstageVolumeRequest, - NodeUnstageVolumeResponse, Topology, node_server::Node, + NodePublishVolumeResponse, NodeServiceCapability, NodeStageVolumeRequest, + NodeStageVolumeResponse, NodeUnpublishVolumeRequest, NodeUnpublishVolumeResponse, + NodeUnstageVolumeRequest, NodeUnstageVolumeResponse, Topology, node_server::Node, + node_service_capability, volume_capability, }, utils::{FmtByteSlice, error_full_message}, }; @@ -112,6 +113,15 @@ enum PublishError { #[snafu(display("secret file {path:?} exists but is not a regular file"))] NotRegularFile { path: PathBuf }, + #[snafu(display("failed to set owner group and permissions of {path:?}"))] + SetOwnership { + source: std::io::Error, + path: PathBuf, + }, + + #[snafu(display("volume mount group {group:?} is not a valid group ID"))] + InvalidVolumeMountGroup { group: String }, + #[snafu(display("file path {path:?} must only contain normal components"))] InvalidComponents { path: PathBuf }, @@ -150,6 +160,8 @@ impl From for Status { PublishError::CheckProvisionedMarker { .. } => Status::unavailable(full_msg), PublishError::InvalidProvisionedMarker { .. } => Status::failed_precondition(full_msg), PublishError::NotRegularFile { .. } => Status::failed_precondition(full_msg), + PublishError::SetOwnership { .. } => Status::unavailable(full_msg), + PublishError::InvalidVolumeMountGroup { .. } => Status::invalid_argument(full_msg), PublishError::InvalidComponents { .. } => Status::unavailable(full_msg), PublishError::InvalidAbsolutePath { .. } => Status::unavailable(full_msg), PublishError::TagPod { .. } => Status::unavailable(full_msg), @@ -251,6 +263,7 @@ impl SecretProvisionerNode { target_path: &Path, data: SecretContents, selector: SecretVolumeSelector, + group: Option, ) -> Result<(), PublishError> { let SecretVolumeSelector { provision_parts, @@ -265,7 +278,7 @@ impl SecretProvisionerNode { .into_files(format, names, compat, provision_parts) .context(publish_error::FormatDataSnafu)? { - write_secret_file(target_path, Path::new(&k), &v)?; + write_secret_file(target_path, Path::new(&k), &v, group)?; } Ok(()) } @@ -395,8 +408,56 @@ 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> { - write_secret_file(target_path, Path::new(PROVISIONED_MARKER_FILE_NAME), &[]) +async fn mark_provisioned(target_path: &Path, group: Option) -> Result<(), PublishError> { + write_secret_file( + target_path, + Path::new(PROVISIONED_MARKER_FILE_NAME), + &[], + group, + ) +} + +/// Returns the Pod's fsGroup, which the kubelet passes because of the `VOLUME_MOUNT_GROUP` node +/// capability. +fn volume_mount_group( + capability: Option<&crate::grpc::csi::v1::VolumeCapability>, +) -> Result, PublishError> { + let group = capability + .and_then(|capability| match &capability.access_type { + Some(volume_capability::AccessType::Mount(mount)) => { + Some(mount.volume_mount_group.as_str()) + } + _ => None, + }) + .filter(|group| !group.is_empty()); + group + .map(|group| { + group + .parse::() + .ok() + // u32::MAX is -1, which leaves the group unchanged in chown(2). + .filter(|id| *id != u32::MAX) + .map(Gid::from_raw) + .context(publish_error::InvalidVolumeMountGroupSnafu { group }) + }) + .transpose() +} + +/// Sets `mode` on `fd`, and `group` as its group if given. +fn restrict_to_group( + fd: impl std::os::fd::AsFd, + mode: u32, + group: Option, + path: &Path, +) -> Result<(), PublishError> { + if let Some(group) = group { + fchown(&fd, None, Some(group)) + .map_err(std::io::Error::from) + .context(publish_error::SetOwnershipSnafu { path })?; + } + fchmod(&fd, Mode::from_raw_mode(mode)) + .map_err(std::io::Error::from) + .context(publish_error::SetOwnershipSnafu { path }) } /// Writes `contents` to `relative_path` beneath `root`, creating missing directories. @@ -410,10 +471,14 @@ async fn mark_provisioned(target_path: &Path) -> Result<(), PublishError> { /// 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. +/// +/// Every directory on the way gets mode `0750` and the file `0640`, both owned by `group` if given, +/// so the Pod can read the secret but not change the volume. fn write_secret_file( root: &Path, relative_path: &Path, contents: &[u8], + group: Option, ) -> Result<(), PublishError> { // Ensure that `relative_path` is indeed a relative path ensure!( @@ -447,6 +512,7 @@ fn write_secret_file( let mut directory = openat(CWD, root, directory_flags, Mode::empty()) .map_err(std::io::Error::from) .context(publish_error::CreateDirSnafu { path: &path })?; + restrict_to_group(&directory, 0o750, group, &path)?; for component in relative_path .parent() .into_iter() @@ -464,6 +530,7 @@ fn write_secret_file( directory = openat(&directory, name, directory_flags, Mode::empty()) .map_err(std::io::Error::from) .context(publish_error::CreateDirSnafu { path: &path })?; + restrict_to_group(&directory, 0o750, group, &path)?; } // And now finally we create the actual file from `contents` @@ -494,6 +561,7 @@ fn write_secret_file( file_type == FileType::RegularFile, publish_error::NotRegularFileSnafu { path } ); + restrict_to_group(&file, 0o640, group, &path)?; File::from(file) .write_all(contents) .context(publish_error::WriteFileSnafu { path }) @@ -545,6 +613,7 @@ impl Node for SecretProvisionerNode { ); return Ok(Response::new(NodePublishVolumeResponse {})); } + let group = volume_mount_group(request.volume_capability.as_ref())?; let selector = SecretVolumeSelector::deserialize(request.volume_context.into_deserializer()) .context(publish_error::InvalidSelectorSnafu)?; @@ -564,10 +633,11 @@ impl Node for SecretProvisionerNode { self.save_secret_data( &target_path, data, - selector + selector, + group, ) .await?; - mark_provisioned(&target_path).await?; + mark_provisioned(&target_path, group).await?; Ok(Response::new(NodePublishVolumeResponse {})) } .await, @@ -616,8 +686,17 @@ impl Node for SecretProvisionerNode { &self, _request: Request, ) -> Result, Status> { + // With this capability the kubelet passes the Pod's fsGroup instead of making the volume + // group-writable itself. The group only gets read access: the CSI spec recommends + // read-write, but files the Pod can replace are files the publish path cannot trust. Ok(Response::new(NodeGetCapabilitiesResponse { - capabilities: vec![], + capabilities: vec![NodeServiceCapability { + r#type: Some(node_service_capability::Type::Rpc( + node_service_capability::Rpc { + r#type: node_service_capability::rpc::Type::VolumeMountGroup.into(), + }, + )), + }], })) } @@ -678,7 +757,7 @@ mod tests { #[tokio::test] async fn marked_volume_is_provisioned() { let volume = tempfile::tempdir().expect("temporary directory can be created"); - mark_provisioned(volume.path()) + mark_provisioned(volume.path(), None) .await .expect("provisioned marker can be written"); assert!( @@ -705,15 +784,20 @@ mod tests { 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!(mark_provisioned(volume.path(), None).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"); + write_secret_file( + volume.path(), + Path::new("nested/tls.crt"), + b"certificate", + None, + ) + .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"); @@ -722,9 +806,9 @@ mod tests { #[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") + write_secret_file(volume.path(), Path::new("tls.key"), b"longer key", None) .expect("secret file can be written"); - write_secret_file(volume.path(), Path::new("tls.key"), b"key") + write_secret_file(volume.path(), Path::new("tls.key"), b"key", None) .expect("secret file can be rewritten"); let contents = std::fs::read(volume.path().join("tls.key")).expect("secret file is readable"); @@ -740,7 +824,9 @@ mod tests { 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()); + assert!( + write_secret_file(volume.path(), Path::new("tls.crt"), b"certificate", None).is_err() + ); let contents = std::fs::read_to_string(&target).expect("symlink target is readable"); assert_eq!(contents, "untouched"); } @@ -753,7 +839,13 @@ mod tests { .expect("symlink can be created"); assert!( - write_secret_file(volume.path(), Path::new("nested/tls.crt"), b"certificate").is_err() + write_secret_file( + volume.path(), + Path::new("nested/tls.crt"), + b"certificate", + None + ) + .is_err() ); assert!(!outside.path().join("tls.crt").exists()); } @@ -770,7 +862,7 @@ mod tests { ) .expect("FIFO can be created"); - let result = write_secret_file(volume.path(), Path::new("tls.crt"), b"certificate"); + let result = write_secret_file(volume.path(), Path::new("tls.crt"), b"certificate", None); assert!(matches!( result, Err(PublishError::NotRegularFile { .. } | PublishError::CreateFile { .. }) @@ -784,8 +876,48 @@ mod tests { 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!(write_secret_file(&volume, Path::new(path), b"certificate", None).is_err()); } assert!(!root.path().join("escaped").exists()); } + + #[test] + fn secret_file_is_readable_but_not_writable_by_group() { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + let volume = tempfile::tempdir().expect("temporary directory can be created"); + let group = rustix::fs::Gid::from_raw( + std::fs::metadata(volume.path()) + .expect("metadata is readable") + .gid(), + ); + let file = volume.path().join("nested/tls.crt"); + write_secret_file( + volume.path(), + Path::new("nested/tls.crt"), + b"certificate", + None, + ) + .expect("secret file can be written"); + std::fs::set_permissions(&file, std::fs::Permissions::from_mode(0o660)) + .expect("permissions can be widened"); + + write_secret_file( + volume.path(), + Path::new("nested/tls.crt"), + b"certificate", + Some(group), + ) + .expect("secret file can be rewritten"); + + for (path, mode) in [ + (volume.path().to_path_buf(), 0o750), + (volume.path().join("nested"), 0o750), + (file, 0o640), + ] { + let metadata = std::fs::metadata(&path).expect("metadata is readable"); + assert_eq!(metadata.mode() & 0o777, mode, "mode of {path:?}"); + assert_eq!(metadata.gid(), group.as_raw(), "group of {path:?}"); + } + } }