[Merged by Bors] - Implement unprivileged mode

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
 

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/.*$",

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

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:

rust/operator-binary/src/csi_server/node.rs

@@ -134,6 +134,7 @@ impl From<UnpublishError> 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<NodePublishVolumeRequest>,
     ) -> Result<Response<NodePublishVolumeResponse>, 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<NodeUnpublishVolumeRequest>,
     ) -> Result<Response<NodeUnpublishVolumeResponse>, 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<T, E: std::error::Error + 'static>(
+    error_msg: &str,
+    res: Result<T, E>,
+) -> Result<T, E> {
+    if let Err(err) = &res {
+        tracing::warn!(error = err as &dyn std::error::Error, "{error_msg}");
+    }
+    res
+}

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?;