Enforce read-only volumes (#48)

After further consideration, the warning logs and flag for backcompat feel a little ineffective. The logs are not a very actionable place to do deprecation warnings. They are just tucked away too deep for anyone to notice. Further, since this will be the only change between 0.1.0 and 0.2.0, if enforcement is problematic, operators can just not upgrade (or downgrade if they don't read the changelog before upgrading). Signed-off-by: Andrew Harding <aharding@vmware.com>
spiffe · Sep 9, 2022 · a1d5ec7 · a1d5ec7
1 parent 4424553
commit a1d5ec7
Show file tree

Hide file tree

Showing 7 changed files with 45 additions and 128 deletions.
diff --git a/cmd/spiffe-csi-driver/main.go b/cmd/spiffe-csi-driver/main.go
@@ -15,11 +15,10 @@ import (
 )
 
 var (
-	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")
+	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() {
@@ -40,26 +39,13 @@ 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, *workloadAPISocketDirFlag,
 		logkeys.CSISocketPath, *csiSocketPathFlag,
-		logkeys.EnforceReadOnlyVolume, *enforceReadOnlyVolumeFlag,
 	)
 
 	driver, err := driver.New(driver.Config{
@@ -92,13 +78,3 @@ 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
-}
diff --git a/pkg/driver/driver.go b/pkg/driver/driver.go
@@ -21,29 +21,26 @@ 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
-	EnforceReadOnlyVolume bool
+	Log                  logr.Logger
+	NodeID               string
+	WorkloadAPISocketDir string
 }
 
 // Driver is the ephemeral-inline CSI driver implementation
 type Driver struct {
 	csi.UnimplementedIdentityServer
 	csi.UnimplementedNodeServer
 
-	log                   logr.Logger
-	nodeID                string
-	workloadAPISocketDir  string
-	enforceReadOnlyVolume bool
+	log                  logr.Logger
+	nodeID               string
+	workloadAPISocketDir string
 }
 
 // New creates a new driver with the given config
@@ -55,10 +52,9 @@ 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,
-		enforceReadOnlyVolume: config.EnforceReadOnlyVolume,
+		log:                  config.Log,
+		nodeID:               config.NodeID,
+		workloadAPISocketDir: config.WorkloadAPISocketDir,
 	}, nil
 }
 
@@ -119,7 +115,7 @@ 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:
+	case !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")
@@ -132,22 +128,11 @@ func (d *Driver) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolu
 
 	// 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)
-		}
+	// be writable by workload containers. We enforce that the CSI volume is
+	// marked read-only above, instructing the kubelet to mount it read-only
+	// into containers, while we mount the volume read-write to the host.
+	if err := bindMountRW(d.workloadAPISocketDir, req.TargetPath); err != nil {
+		return nil, status.Errorf(codes.Internal, "unable to mount %q: %v", req.TargetPath, err)
 	}
 
 	log.Info("Volume published")

diff --git a/pkg/driver/driver_test.go b/pkg/driver/driver_test.go
@@ -28,11 +28,8 @@ const (
 )
 
 func init() {
-	bindMountRO = func(src, dst string) error {
-		return writeMeta(dst, src+",ro")
-	}
 	bindMountRW = func(src, dst string) error {
-		return writeMeta(dst, src+",rw")
+		return writeMeta(dst, src)
 	}
 	unmount = func(dst string) error {
 		return os.Remove(metaPath(dst))
@@ -124,12 +121,11 @@ func TestBoilerplateRPCs(t *testing.T) {
 
 func TestNodePublishVolume(t *testing.T) {
 	for _, tt := range []struct {
-		desc                  string
-		mutateReq             func(req *csi.NodePublishVolumeRequest)
-		mungeTargetPath       func(t *testing.T, targetPath string)
-		enforceReadOnlyVolume bool
-		expectCode            codes.Code
-		expectMsgPrefix       string
+		desc            string
+		mutateReq       func(req *csi.NodePublishVolumeRequest)
+		mungeTargetPath func(t *testing.T, targetPath string)
+		expectCode      codes.Code
+		expectMsgPrefix string
 	}{
 		{
 			desc: "missing volume id",
@@ -249,22 +245,13 @@ func TestNodePublishVolume(t *testing.T) {
 			expectMsgPrefix: "unable to mount",
 		},
 		{
-			desc: "not enforcing read-only",
+			desc: "enforcing read-only volumes",
 			mutateReq: func(req *csi.NodePublishVolumeRequest) {
 				req.Readonly = false
 			},
-			expectCode: codes.OK,
+			expectCode:      codes.InvalidArgument,
+			expectMsgPrefix: "pod.spec.volumes[].csi.readOnly must be set to 'true'",
 		},
-		{
-			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,
@@ -294,13 +281,13 @@ func TestNodePublishVolume(t *testing.T) {
 				tt.mutateReq(req)
 			}
 
-			client, workloadAPISocketDir := startDriver(t, enforceReadOnlyVolume(tt.enforceReadOnlyVolume))
+			client, workloadAPISocketDir := startDriver(t)
 
 			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, req.Readonly)
+				assertMounted(t, targetPath, workloadAPISocketDir)
 			} else {
 				assert.Nil(t, resp)
 				assertNotMounted(t, targetPath)
@@ -415,26 +402,14 @@ type client struct {
 	csi.NodeClient
 }
 
-func enforceReadOnlyVolume(value bool) func(*Config) {
-	return func(c *Config) {
-		c.EnforceReadOnlyVolume = value
-	}
-}
-
-func startDriver(t *testing.T, opts ...func(*Config)) (client, string) {
+func startDriver(t *testing.T) (client, string) {
 	workloadAPISocketDir := t.TempDir()
 
-	config := Config{
+	d, err := New(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")
@@ -481,16 +456,10 @@ func startDriver(t *testing.T, opts ...func(*Config)) (client, string) {
 	}, workloadAPISocketDir
 }
 
-func assertMounted(t *testing.T, targetPath, src string, readOnly bool) {
+func assertMounted(t *testing.T, targetPath, src string) {
 	meta, err := readMeta(targetPath)
 	if assert.NoError(t, err) {
-		// 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)
-		}
+		assert.Equal(t, src, meta)
 	}
 }
 

diff --git a/pkg/logkeys/keys.go b/pkg/logkeys/keys.go
@@ -1,13 +1,12 @@
 package logkeys
 
 const (
-	CSISocketPath         = "csiSocketPath"
-	EnforceReadOnlyVolume = "enforceReadOnlyVolume"
-	FullMethod            = "fullMethod"
-	NodeID                = "nodeID"
-	TargetPath            = "targetPath"
-	Version               = "version"
-	VolumeID              = "volumeID"
-	VolumePath            = "volumePath"
-	WorkloadAPISocketDir  = "workloadAPISocketDir"
+	CSISocketPath        = "csiSocketPath"
+	FullMethod           = "fullMethod"
+	NodeID               = "nodeID"
+	TargetPath           = "targetPath"
+	Version              = "version"
+	VolumeID             = "volumeID"
+	VolumePath           = "volumePath"
+	WorkloadAPISocketDir = "workloadAPISocketDir"
 )
diff --git a/pkg/mount/mount.go b/pkg/mount/mount.go
@@ -1,10 +1,5 @@
 package mount
 
-// BindMountRO performs a read-only bind mount from root to mountPoint
-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)

diff --git a/pkg/mount/mount_linux.go b/pkg/mount/mount_linux.go
@@ -12,8 +12,7 @@ import (
 )
 
 const (
-	msRdOnly uintptr = 1    // LINUX MS_RDONLY
-	msBind   uintptr = 4096 // LINUX MS_BIND
+	msBind uintptr = 4096 // LINUX MS_BIND
 )
 
 var (
@@ -23,10 +22,6 @@ var (
 	procMountInfo = "/proc/self/mountinfo"
 )
 
-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, "")
 }

diff --git a/test/config/spiffe-csi-test-workload-1.yaml b/test/config/spiffe-csi-test-workload-1.yaml
@@ -31,6 +31,4 @@ 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
+          readOnly: true