diff --git a/CHANGELOG.md b/CHANGELOG.md index 27eb53a8..50581218 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ All notable changes to this project will be documented in this file. ### Added - Added `kerberosKeytab` provisioner backend ([#99]). +- Added experimental unprivileged mode ([#252]). ### Changed @@ -20,6 +21,7 @@ All notable changes to this project will be documented in this file. [#99]: https://github.com/stackabletech/secret-operator/pull/99 [#231]: https://github.com/stackabletech/secret-operator/pull/231 [#232]: https://github.com/stackabletech/secret-operator/pull/232 +[#252]: https://github.com/stackabletech/secret-operator/pull/252 ## [23.1.0] - 2023-01-23 diff --git a/Tiltfile b/Tiltfile index f97967be..d4658051 100644 --- a/Tiltfile +++ b/Tiltfile @@ -29,6 +29,8 @@ helm_crds, helm_non_crds = filter_yaml( name=operator_name, set=[ 'image.repository=' + registry + '/' + operator_name, + # Uncomment to enable unprivileged mode + 'securityContext.privileged=false', ], ), api_version = "^apiextensions\\.k8s\\.io/.*$", diff --git a/deploy/helm/secret-operator/templates/daemonset.yaml b/deploy/helm/secret-operator/templates/daemonset.yaml index 3755f429..2465774f 100644 --- a/deploy/helm/secret-operator/templates/daemonset.yaml +++ b/deploy/helm/secret-operator/templates/daemonset.yaml @@ -41,12 +41,16 @@ spec: fieldRef: apiVersion: v1 fieldPath: spec.nodeName + - name: PRIVILEGED + value: {{ .Values.securityContext.privileged | quote }} volumeMounts: - name: csi mountPath: /csi - name: mountpoint mountPath: {{ .Values.kubeletDir }}/pods + {{- if .Values.securityContext.privileged }} mountPropagation: Bidirectional + {{- end }} - name: tmp mountPath: /tmp - name: external-provisioner diff --git a/deploy/helm/secret-operator/values.yaml b/deploy/helm/secret-operator/values.yaml index 2ae23b50..6be3290f 100644 --- a/deploy/helm/secret-operator/values.yaml +++ b/deploy/helm/secret-operator/values.yaml @@ -35,6 +35,9 @@ podSecurityContext: {} securityContext: # secret-operator requires root permissions runAsUser: 0 + # It is strongly recommended to run secret-operator as a privileged container, since + # it enables additional protections for the secret contents. + # Unprivileged mode is EXPERIMENTAL and requires manual migration for an existing cluster. privileged: true # capabilities: # drop: diff --git a/rust/operator-binary/src/csi_server/node.rs b/rust/operator-binary/src/csi_server/node.rs index 9c389416..9438e236 100644 --- a/rust/operator-binary/src/csi_server/node.rs +++ b/rust/operator-binary/src/csi_server/node.rs @@ -134,6 +134,7 @@ impl From for Status { pub struct SecretProvisionerNode { pub client: stackable_operator::client::Client, pub node_name: String, + pub privileged: bool, } impl SecretProvisionerNode { @@ -158,14 +159,18 @@ impl SecretProvisionerNode { _ => return Err(err).context(publish_error::CreateDirSnafu { path: target_path }), }, } - Mount::new( - "", - target_path, - "tmpfs", - MountFlags::NODEV | MountFlags::NOEXEC | MountFlags::NOSUID, - None, - ) - .context(publish_error::MountSnafu { path: target_path })?; + if self.privileged { + Mount::new( + "", + target_path, + "tmpfs", + MountFlags::NODEV | MountFlags::NOEXEC | MountFlags::NOSUID, + None, + ) + .context(publish_error::MountSnafu { path: target_path })?; + } else { + tracing::info!("Running in unprivileged mode, not creating mount for secret volume"); + } // User: root/secret-operator // Group: Controlled by Pod.securityContext.fsGroup, the actual application // (when running as unprivileged user) @@ -260,23 +265,37 @@ impl SecretProvisionerNode { } async fn clean_secret_dir(&self, target_path: &Path) -> Result<(), UnpublishError> { - match unmount(target_path, UnmountFlags::empty()) { - Ok(_) => {} - Err(err) => match err.kind() { - std::io::ErrorKind::NotFound => { - tracing::warn!(volume.path = %target_path.display(), "Tried to delete volume path that does not exist, assuming it was already deleted"); - return Ok(()); - } - std::io::ErrorKind::InvalidInput => { - tracing::warn!(volume.path = %target_path.display(), "Tried to unmount volume path that is not mounted, trying to delete it anyway"); - } - _ => return Err(err).context(unpublish_error::UnmountSnafu { path: target_path }), - }, - }; - tokio::fs::remove_dir(&target_path) - .await - .context(unpublish_error::DeleteSnafu { path: target_path })?; - Ok(()) + // unmount() fails unconditionally with PermissionDenied when running in an unprivileged container, + // even if it wouldn't be sensible to even try anyway (such as when there is no volume mount). + if self.privileged { + match unmount(target_path, UnmountFlags::empty()) { + Ok(_) => {} + Err(err) => match err.kind() { + std::io::ErrorKind::NotFound => { + tracing::warn!(volume.path = %target_path.display(), "Tried to unmount volume path that does not exist, assuming it was already deleted"); + return Ok(()); + } + std::io::ErrorKind::InvalidInput => { + tracing::warn!(volume.path = %target_path.display(), "Tried to unmount volume path that is not mounted, trying to delete it anyway"); + } + _ => { + return Err(err) + .context(unpublish_error::UnmountSnafu { path: target_path }) + } + }, + }; + } + // There is no mount in unprivileged mode, so we need to remove all contents in that case. + // This may still apply to privileged mode, in case users are migrating from unprivileged to privileged mode. + match tokio::fs::remove_dir_all(&target_path).await { + Ok(_) => Ok(()), + // We already catch this above when running in privileged mode, but in unprivileged mode this is still possible + Err(err) if err.kind() == std::io::ErrorKind::NotFound => { + tracing::warn!(volume.path = %target_path.display(), "Tried to delete volume path that does not exist, assuming it was already deleted"); + Ok(()) + } + Err(err) => Err(err).context(unpublish_error::DeleteSnafu { path: target_path }), + } } } @@ -306,28 +325,34 @@ impl Node for SecretProvisionerNode { &self, request: Request, ) -> Result, Status> { - let request = request.into_inner(); - let target_path = PathBuf::from(request.target_path); - tracing::info!( - volume.path = %target_path.display(), - "Received NodePublishVolume request" - ); - let selector = - SecretVolumeSelector::deserialize(request.volume_context.into_deserializer()) - .context(publish_error::InvalidSelectorSnafu)?; - let pod_info = self.get_pod_info(&selector).await?; - let backend = backend::dynamic::from_selector(&self.client, &selector) - .await - .context(publish_error::InitBackendSnafu)?; - let data = backend - .get_secret_data(&selector, pod_info) - .await - .context(publish_error::BackendGetSecretDataSnafu)?; - self.tag_pod(&self.client, &request.volume_id, &selector, &data) - .await?; - self.prepare_secret_dir(&target_path).await?; - self.save_secret_data(&target_path, &data).await?; - Ok(Response::new(NodePublishVolumeResponse {})) + log_if_endpoint_error( + "failed to publish volume", + async move { + let request = request.into_inner(); + let target_path = PathBuf::from(request.target_path); + tracing::info!( + volume.path = %target_path.display(), + "Received NodePublishVolume request" + ); + let selector = + SecretVolumeSelector::deserialize(request.volume_context.into_deserializer()) + .context(publish_error::InvalidSelectorSnafu)?; + let pod_info = self.get_pod_info(&selector).await?; + let backend = backend::dynamic::from_selector(&self.client, &selector) + .await + .context(publish_error::InitBackendSnafu)?; + let data = backend + .get_secret_data(&selector, pod_info) + .await + .context(publish_error::BackendGetSecretDataSnafu)?; + self.tag_pod(&self.client, &request.volume_id, &selector, &data) + .await?; + self.prepare_secret_dir(&target_path).await?; + self.save_secret_data(&target_path, &data).await?; + Ok(Response::new(NodePublishVolumeResponse {})) + } + .await, + ) } // Called when a pod is terminated that contained a volume created by this provider. @@ -338,14 +363,20 @@ impl Node for SecretProvisionerNode { &self, request: Request, ) -> Result, Status> { - let request = request.into_inner(); - let target_path = PathBuf::from(request.target_path); - tracing::info!( - volume.path = %target_path.display(), - "Received NodeUnpublishVolume request" - ); - self.clean_secret_dir(&target_path).await?; - Ok(Response::new(NodeUnpublishVolumeResponse {})) + log_if_endpoint_error( + "Failed to unpublish volume", + async move { + let request = request.into_inner(); + let target_path = PathBuf::from(request.target_path); + tracing::info!( + volume.path = %target_path.display(), + "Received NodeUnpublishVolume request" + ); + self.clean_secret_dir(&target_path).await?; + Ok(Response::new(NodeUnpublishVolumeResponse {})) + } + .await, + ) } async fn node_get_volume_stats( @@ -384,3 +415,13 @@ impl Node for SecretProvisionerNode { })) } } + +fn log_if_endpoint_error( + error_msg: &str, + res: Result, +) -> Result { + if let Err(err) = &res { + tracing::warn!(error = err as &dyn std::error::Error, "{error_msg}"); + } + res +} diff --git a/rust/operator-binary/src/main.rs b/rust/operator-binary/src/main.rs index 3a1832e8..2c23893d 100644 --- a/rust/operator-binary/src/main.rs +++ b/rust/operator-binary/src/main.rs @@ -1,3 +1,4 @@ +use anyhow::Context; use clap::{crate_description, crate_version, Parser}; use csi_server::{ controller::SecretProvisionerController, identity::SecretProvisionerIdentity, @@ -35,6 +36,14 @@ struct SecretOperatorRun { csi_endpoint: PathBuf, #[clap(long, env)] node_name: String, + /// Unprivileged mode disables any features that require running secret-operator in a privileged container. + /// + /// Currently, this means that: + /// - Secret volumes will be stored on disk, rather than in a ramdisk + /// + /// Unprivileged mode is EXPERIMENTAL and heavily discouraged, since it increases the risk of leaking secrets. + #[clap(long, env)] + privileged: bool, /// Tracing log collector system #[arg(long, env, default_value_t, value_enum)] pub tracing_target: TracingTarget, @@ -56,6 +65,7 @@ async fn main() -> anyhow::Result<()> { csi_endpoint, node_name, tracing_target, + privileged, }) => { stackable_operator::logging::initialize_logging( "SECRET_PROVISIONER_LOG", @@ -92,10 +102,16 @@ async fn main() -> anyhow::Result<()> { .add_service(ControllerServer::new(SecretProvisionerController { client: client.clone(), })) - .add_service(NodeServer::new(SecretProvisionerNode { client, node_name })) + .add_service(NodeServer::new(SecretProvisionerNode { + client, + node_name, + privileged, + })) .serve_with_incoming_shutdown( - UnixListenerStream::new(uds_bind_private(csi_endpoint)?) - .map_ok(TonicUnixStream), + UnixListenerStream::new( + uds_bind_private(csi_endpoint).context("failed to bind CSI listener")?, + ) + .map_ok(TonicUnixStream), sigterm.recv().map(|_| ()), ) .await?;