From 44245533c1b5843e4f7356eb006f871906becd6a Mon Sep 17 00:00:00 2001 From: Andrew Harding Date: Thu, 8 Sep 2022 12:36:24 -0600 Subject: [PATCH] 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 --- cmd/spiffe-csi-driver/main.go | 36 +++++++++-- example/config/workload.yaml | 1 + pkg/driver/driver.go | 46 ++++++++++---- pkg/driver/driver_test.go | 70 ++++++++++++++++----- pkg/logkeys/keys.go | 17 ++--- pkg/mount/mount.go | 5 ++ pkg/mount/mount_linux.go | 4 ++ pkg/mount/mount_other.go | 4 ++ test/config/spiffe-csi-test-workload-1.yaml | 3 + test/config/spiffe-csi-test-workload-2.yaml | 1 + 10 files changed, 146 insertions(+), 41 deletions(-) diff --git a/cmd/spiffe-csi-driver/main.go b/cmd/spiffe-csi-driver/main.go index 6a94a3c..0911c7e 100644 --- a/cmd/spiffe-csi-driver/main.go +++ b/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 +} diff --git a/example/config/workload.yaml b/example/config/workload.yaml index 61f5f88..68f6d87 100644 --- a/example/config/workload.yaml +++ b/example/config/workload.yaml @@ -18,3 +18,4 @@ spec: - name: spiffe-workload-api csi: driver: "csi.spiffe.io" + readOnly: true diff --git a/pkg/driver/driver.go b/pkg/driver/driver.go index a25ec22..62d3b05 100644 --- a/pkg/driver/driver.go +++ b/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") diff --git a/pkg/driver/driver_test.go b/pkg/driver/driver_test.go index 62a97ec..c960dc4 100644 --- a/pkg/driver/driver_test.go +++ b/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) + } } } diff --git a/pkg/logkeys/keys.go b/pkg/logkeys/keys.go index e5a7131..634bf8d 100644 --- a/pkg/logkeys/keys.go +++ b/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" ) diff --git a/pkg/mount/mount.go b/pkg/mount/mount.go index e3b28c0..805ac51 100644 --- a/pkg/mount/mount.go +++ b/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) diff --git a/pkg/mount/mount_linux.go b/pkg/mount/mount_linux.go index 8e7d228..07a94a7 100644 --- a/pkg/mount/mount_linux.go +++ b/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) } diff --git a/pkg/mount/mount_other.go b/pkg/mount/mount_other.go index 302b05e..9fc6fe8 100644 --- a/pkg/mount/mount_other.go +++ b/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") } diff --git a/test/config/spiffe-csi-test-workload-1.yaml b/test/config/spiffe-csi-test-workload-1.yaml index b967bc3..cc1273a 100644 --- a/test/config/spiffe-csi-test-workload-1.yaml +++ b/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 diff --git a/test/config/spiffe-csi-test-workload-2.yaml b/test/config/spiffe-csi-test-workload-2.yaml index 80c942c..c0539e2 100644 --- a/test/config/spiffe-csi-test-workload-2.yaml +++ b/test/config/spiffe-csi-test-workload-2.yaml @@ -31,3 +31,4 @@ spec: - name: spire-agent-socket csi: driver: "csi.spiffe.io" + readOnly: true