Allow fsgroup/SELinux attributes to be set (#44)

- Changes the mount to be mounted read-write on the host so that
  fsetxattr can be used by the host to change the attributes on files
  inside the mount. For security purposes, this only happens if the CSI
  volume is specified as read-only so the kubelet will mount the volume
  read-only into the containers.
- Optionally enforces that the CSI volume is marked read-only. We can't
  enforce this by default, since it would break existing deployments.
  It will be enforced in a future release.

Fixes: #42

Signed-off-by: Andrew Harding <aharding@vmware.com>

cmd/spiffe-csi-driver/main.go

@@ -15,10 +15,11 @@ import (
 )
 
 var (
-	nodeIDFlag           = flag.String("node-id", "", "Kubernetes Node ID. If unset, the node ID is obtained from the environment (i.e., -node-id-env)")
-	nodeIDEnvFlag        = flag.String("node-id-env", "MY_NODE_NAME", "Envvar from which to obtain the node ID. Overridden by -node-id.")
-	csiSocketPathFlag    = flag.String("csi-socket-path", "/spiffe-csi/csi.sock", "Path to the CSI socket")
-	workloadAPISocketDir = flag.String("workload-api-socket-dir", "", "Path to the Workload API socket directory")
+	enforceReadOnlyVolumeFlag = flag.Bool("enforce-read-only-volume", false, "If set, enforce that the CSI volume is marked read-only")
+	nodeIDFlag                = flag.String("node-id", "", "Kubernetes Node ID. If unset, the node ID is obtained from the environment (i.e., -node-id-env)")
+	nodeIDEnvFlag             = flag.String("node-id-env", "MY_NODE_NAME", "Envvar from which to obtain the node ID. Overridden by -node-id.")
+	csiSocketPathFlag         = flag.String("csi-socket-path", "/spiffe-csi/csi.sock", "Path to the CSI socket")
+	workloadAPISocketDirFlag  = flag.String("workload-api-socket-dir", "", "Path to the Workload API socket directory")
 )
 
 func main() {
@@ -39,19 +40,32 @@ func main() {
 	}
 	log = zapr.NewLogger(zapLog)
 
+	// If the enforce-read-only-volume flag was not explicitly provided,
+	// instruct the user to set it. Either way, if it is set to false, or
+	// unset, emit a log educating users that the will be enforced in the
+	// future.
+	// TODO: enforce this by default in a future release.
+	if !isFlagSet("enforce-read-only-volume") {
+		log.Error(nil, "Pass the --enforce-read-only-volume flag to enforce that the CSI volume is marked read-only")
+	}
+	if !*enforceReadOnlyVolumeFlag {
+		log.Error(nil, "Not enforcing that the CSI volume is marked read-only; this will be required in a future release")
+	}
+
 	nodeID := getNodeIDFromFlags()
 
 	log.Info("Starting.",
 		logkeys.Version, version.Version(),
 		logkeys.NodeID, nodeID,
-		logkeys.WorkloadAPISocketDir, *workloadAPISocketDir,
+		logkeys.WorkloadAPISocketDir, *workloadAPISocketDirFlag,
 		logkeys.CSISocketPath, *csiSocketPathFlag,
+		logkeys.EnforceReadOnlyVolume, *enforceReadOnlyVolumeFlag,
 	)
 
 	driver, err := driver.New(driver.Config{
 		Log:                  log,
 		NodeID:               nodeID,
-		WorkloadAPISocketDir: *workloadAPISocketDir,
+		WorkloadAPISocketDir: *workloadAPISocketDirFlag,
 	})
 	if err != nil {
 		log.Error(err, "Failed to create driver")
@@ -78,3 +92,13 @@ func getNodeIDFromFlags() string {
 	}
 	return nodeID
 }
+
+func isFlagSet(name string) bool {
+	set := false
+	flag.Visit(func(f *flag.Flag) {
+		if f.Name == name {
+			set = true
+		}
+	})
+	return set
+}

example/config/workload.yaml

@@ -18,3 +18,4 @@ spec:
   - name: spiffe-workload-api
     csi:
       driver: "csi.spiffe.io"
+      readOnly: true

pkg/driver/driver.go

@@ -22,15 +22,17 @@ const (
 var (
 	// We replace these in tests since bind mounting generally requires root.
 	bindMountRO  = mount.BindMountRO
+	bindMountRW  = mount.BindMountRW
 	unmount      = mount.Unmount
 	isMountPoint = mount.IsMountPoint
 )
 
 // Config is the configuration for the driver
 type Config struct {
-	Log                  logr.Logger
-	NodeID               string
-	WorkloadAPISocketDir string
+	Log                   logr.Logger
+	NodeID                string
+	WorkloadAPISocketDir  string
+	EnforceReadOnlyVolume bool
 }
 
 // Driver is the ephemeral-inline CSI driver implementation
@@ -38,9 +40,10 @@ type Driver struct {
 	csi.UnimplementedIdentityServer
 	csi.UnimplementedNodeServer
 
-	log                  logr.Logger
-	nodeID               string
-	workloadAPISocketDir string
+	log                   logr.Logger
+	nodeID                string
+	workloadAPISocketDir  string
+	enforceReadOnlyVolume bool
 }
 
 // New creates a new driver with the given config
@@ -52,9 +55,10 @@ func New(config Config) (*Driver, error) {
 		return nil, errors.New("workload API socket directory is required")
 	}
 	return &Driver{
-		log:                  config.Log,
-		nodeID:               config.NodeID,
-		workloadAPISocketDir: config.WorkloadAPISocketDir,
+		log:                   config.Log,
+		nodeID:                config.NodeID,
+		workloadAPISocketDir:  config.WorkloadAPISocketDir,
+		enforceReadOnlyVolume: config.EnforceReadOnlyVolume,
 	}, nil
 }
 
@@ -115,6 +119,8 @@ func (d *Driver) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolu
 		return nil, status.Error(codes.InvalidArgument, "request missing required volume capability access mode")
 	case isVolumeCapabilityAccessModeReadOnly(req.VolumeCapability.AccessMode):
 		return nil, status.Error(codes.InvalidArgument, "request volume capability access mode is not valid")
+	case d.enforceReadOnlyVolume && !req.Readonly:
+		return nil, status.Error(codes.InvalidArgument, "pod.spec.volumes[].csi.readOnly must be set to 'true'")
 	case ephemeralMode != "true":
 		return nil, status.Error(codes.InvalidArgument, "only ephemeral volumes are supported")
 	}
@@ -123,9 +129,25 @@ func (d *Driver) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolu
 	if err := os.Mkdir(req.TargetPath, 0777); err != nil && !os.IsExist(err) {
 		return nil, status.Errorf(codes.Internal, "unable to create target path %q: %v", req.TargetPath, err)
 	}
-	// Bind mount the agent socket directory (read only) to the target path
-	if err := bindMountRO(d.workloadAPISocketDir, req.TargetPath); err != nil {
-		return nil, status.Errorf(codes.Internal, "unable to mount %q: %v", req.TargetPath, err)
+
+	// Ideally the volume is writable by the host to enable, for example,
+	// manipulation of file attributes by SELinux. However, the volume MUST NOT
+	// be writable by workload containers.
+	//
+	// Therefore, if the volume is marked read-only, thus ensuring that the
+	// kubelet will mount it read-only into the workload container, bind mount
+	// the agent socket directory read-write to the target path on the host.
+	//
+	// If the volume is not marked read-only, then do a read-only mount to
+	// prevent the directory from being manipulated by the workload container.
+	if req.Readonly {
+		if err := bindMountRW(d.workloadAPISocketDir, req.TargetPath); err != nil {
+			return nil, status.Errorf(codes.Internal, "unable to mount %q read-write: %v", req.TargetPath, err)
+		}
+	} else {
+		if err := bindMountRO(d.workloadAPISocketDir, req.TargetPath); err != nil {
+			return nil, status.Errorf(codes.Internal, "unable to mount %q read-only: %v", req.TargetPath, err)
+		}
 	}
 
 	log.Info("Volume published")

pkg/driver/driver_test.go

@@ -29,7 +29,10 @@ const (
 
 func init() {
 	bindMountRO = func(src, dst string) error {
-		return writeMeta(dst, src)
+		return writeMeta(dst, src+",ro")
+	}
+	bindMountRW = func(src, dst string) error {
+		return writeMeta(dst, src+",rw")
 	}
 	unmount = func(dst string) error {
 		return os.Remove(metaPath(dst))
@@ -120,14 +123,13 @@ func TestBoilerplateRPCs(t *testing.T) {
 }
 
 func TestNodePublishVolume(t *testing.T) {
-	client, workloadAPISocketDir := startDriver(t)
-
 	for _, tt := range []struct {
-		desc            string
-		mutateReq       func(req *csi.NodePublishVolumeRequest)
-		mungeTargetPath func(t *testing.T, targetPath string)
-		expectCode      codes.Code
-		expectMsgPrefix string
+		desc                  string
+		mutateReq             func(req *csi.NodePublishVolumeRequest)
+		mungeTargetPath       func(t *testing.T, targetPath string)
+		enforceReadOnlyVolume bool
+		expectCode            codes.Code
+		expectMsgPrefix       string
 	}{
 		{
 			desc: "missing volume id",
@@ -246,6 +248,23 @@ func TestNodePublishVolume(t *testing.T) {
 			expectCode:      codes.Internal,
 			expectMsgPrefix: "unable to mount",
 		},
+		{
+			desc: "not enforcing read-only",
+			mutateReq: func(req *csi.NodePublishVolumeRequest) {
+				req.Readonly = false
+			},
+			expectCode: codes.OK,
+		},
+		{
+			desc: "enforcing read-only",
+			mutateReq: func(req *csi.NodePublishVolumeRequest) {
+				req.Readonly = false
+			},
+			enforceReadOnlyVolume: true,
+			expectCode:            codes.InvalidArgument,
+			expectMsgPrefix:       "pod.spec.volumes[].csi.readOnly must be set to 'true'",
+		},
+
 		{
 			desc:       "success",
 			expectCode: codes.OK,
@@ -262,6 +281,7 @@ func TestNodePublishVolume(t *testing.T) {
 			req := &csi.NodePublishVolumeRequest{
 				VolumeId:   "volumeID",
 				TargetPath: targetPath,
+				Readonly:   true,
 				VolumeCapability: &csi.VolumeCapability{
 					AccessType: &csi.VolumeCapability_Mount{},
 					AccessMode: &csi.VolumeCapability_AccessMode{},
@@ -273,11 +293,14 @@ func TestNodePublishVolume(t *testing.T) {
 			if tt.mutateReq != nil {
 				tt.mutateReq(req)
 			}
+
+			client, workloadAPISocketDir := startDriver(t, enforceReadOnlyVolume(tt.enforceReadOnlyVolume))
+
 			resp, err := client.NodePublishVolume(context.Background(), req)
 			requireGRPCStatusPrefix(t, err, tt.expectCode, tt.expectMsgPrefix)
 			if err == nil {
 				assert.Equal(t, &csi.NodePublishVolumeResponse{}, resp)
-				assertMounted(t, targetPath, workloadAPISocketDir)
+				assertMounted(t, targetPath, workloadAPISocketDir, req.Readonly)
 			} else {
 				assert.Nil(t, resp)
 				assertNotMounted(t, targetPath)
@@ -392,14 +415,26 @@ type client struct {
 	csi.NodeClient
 }
 
-func startDriver(t *testing.T) (client, string) {
+func enforceReadOnlyVolume(value bool) func(*Config) {
+	return func(c *Config) {
+		c.EnforceReadOnlyVolume = value
+	}
+}
+
+func startDriver(t *testing.T, opts ...func(*Config)) (client, string) {
 	workloadAPISocketDir := t.TempDir()
 
-	d, err := New(Config{
+	config := Config{
 		Log:                  logr.Discard(),
 		NodeID:               testNodeID,
 		WorkloadAPISocketDir: workloadAPISocketDir,
-	})
+	}
+
+	for _, opt := range opts {
+		opt(&config)
+	}
+
+	d, err := New(config)
 	require.NoError(t, err)
 
 	l, err := net.Listen("tcp", "localhost:0")
@@ -446,11 +481,16 @@ func startDriver(t *testing.T) (client, string) {
 	}, workloadAPISocketDir
 }
 
-func assertMounted(t *testing.T, targetPath, src string) {
+func assertMounted(t *testing.T, targetPath, src string, readOnly bool) {
 	meta, err := readMeta(targetPath)
 	if assert.NoError(t, err) {
-		// assert the src
-		assert.Equal(t, src, meta)
+		// Counterintuitively, we expect the driver to mount R/W when the
+		// volume is marked read-only. See the note in driver.go for details.
+		if readOnly {
+			assert.Equal(t, src+",rw", meta)
+		} else {
+			assert.Equal(t, src+",ro", meta)
+		}
 	}
 }
 

pkg/logkeys/keys.go

@@ -1,12 +1,13 @@
 package logkeys
 
 const (
-	CSISocketPath        = "csiSocketPath"
-	FullMethod           = "fullMethod"
-	NodeID               = "nodeID"
-	TargetPath           = "targetPath"
-	Version              = "version"
-	VolumeID             = "volumeID"
-	VolumePath           = "volumePath"
-	WorkloadAPISocketDir = "workloadAPISocketDir"
+	CSISocketPath         = "csiSocketPath"
+	EnforceReadOnlyVolume = "enforceReadOnlyVolume"
+	FullMethod            = "fullMethod"
+	NodeID                = "nodeID"
+	TargetPath            = "targetPath"
+	Version               = "version"
+	VolumeID              = "volumeID"
+	VolumePath            = "volumePath"
+	WorkloadAPISocketDir  = "workloadAPISocketDir"
 )

pkg/mount/mount.go

@@ -5,6 +5,11 @@ func BindMountRO(root, mountPoint string) error {
 	return bindMountRO(root, mountPoint)
 }
 
+// BindMountRW performs a read-write bind mount from root to mountPoint
+func BindMountRW(root, mountPoint string) error {
+	return bindMountRW(root, mountPoint)
+}
+
 // Unmount unmounts a mount
 func Unmount(mountPoint string) error {
 	return unmount(mountPoint)

pkg/mount/mount_linux.go

@@ -27,6 +27,10 @@ func bindMountRO(root, mountPoint string) error {
 	return unix.Mount(root, mountPoint, "none", msBind|msRdOnly, "")
 }
 
+func bindMountRW(root, mountPoint string) error {
+	return unix.Mount(root, mountPoint, "none", msBind, "")
+}
+
 func unmount(mountPoint string) error {
 	return unix.Unmount(mountPoint, 0)
 }

pkg/mount/mount_other.go

@@ -11,6 +11,10 @@ func bindMountRO(src, dst string) error {
 	return errors.New("unsupported on this platform")
 }
 
+func bindMountRW(src, dst string) error {
+	return errors.New("unsupported on this platform")
+}
+
 func unmount(path string) error {
 	return errors.New("unsupported on this platform")
 }

test/config/spiffe-csi-test-workload-1.yaml

@@ -31,3 +31,6 @@ spec:
       - name: spire-agent-socket
         csi:
           driver: "csi.spiffe.io"
+          # Don't set the readOnly attribute since we need to ensure existing
+          # deployments work.
+          # TODO: set readOnly once we enforce it

test/config/spiffe-csi-test-workload-2.yaml

@@ -31,3 +31,4 @@ spec:
       - name: spire-agent-socket
         csi:
           driver: "csi.spiffe.io"
+          readOnly: true