From 993391c980c043f435318ba2568acbd7b72fa064 Mon Sep 17 00:00:00 2001 From: Prankul <84079249+prankulmahajan@users.noreply.github.com> Date: Tue, 27 Aug 2024 15:10:25 +0530 Subject: [PATCH] Run `livenessprobe` as non-root + Run all possible sidecars as nonRootGroup (#65) * Create CSI socket file using non-root user Signed-off-by: Prankul <84079249+prankulmahajan@users.noreply.github.com> --------- Signed-off-by: Prankul <84079249+prankulmahajan@users.noreply.github.com> --- .../kubernetes/manifests/config-map.yaml | 2 +- .../manifests/controller-server.yaml | 8 + .../kubernetes/manifests/node-server.yaml | 21 +- pkg/ibmcsidriver/fileOps.go | 85 ++++++++ pkg/ibmcsidriver/fileOps_test.go | 125 ++++++++++++ .../fake_socket_permission.go | 188 ++++++++++++++++++ pkg/ibmcsidriver/server.go | 11 + 7 files changed, 436 insertions(+), 4 deletions(-) create mode 100644 pkg/ibmcsidriver/fileOps.go create mode 100644 pkg/ibmcsidriver/fileOps_test.go create mode 100644 pkg/ibmcsidriver/ibmcsidriverfakes/fake_socket_permission.go diff --git a/deploy/kubernetes/driver/kubernetes/manifests/config-map.yaml b/deploy/kubernetes/driver/kubernetes/manifests/config-map.yaml index 0471f15c..819d0266 100644 --- a/deploy/kubernetes/driver/kubernetes/manifests/config-map.yaml +++ b/deploy/kubernetes/driver/kubernetes/manifests/config-map.yaml @@ -16,4 +16,4 @@ data: VPC_RETRY_INTERVAL: "60" # This is max retry Gap in seconds even considering for exponential retry VPC_API_VERSION: "2021-04-20" VPC_API_GENERATION: "1" - IKS_ENABLED: "True" \ No newline at end of file + IKS_ENABLED: "True" diff --git a/deploy/kubernetes/driver/kubernetes/manifests/controller-server.yaml b/deploy/kubernetes/driver/kubernetes/manifests/controller-server.yaml index aa2a91e5..529faeed 100644 --- a/deploy/kubernetes/driver/kubernetes/manifests/controller-server.yaml +++ b/deploy/kubernetes/driver/kubernetes/manifests/controller-server.yaml @@ -25,6 +25,7 @@ spec: securityContext: runAsNonRoot: true runAsUser: 2121 + runAsGroup: 2121 containers: - name: csi-provisioner image: MUSTPATCHWITHKUSTOMIZE @@ -187,6 +188,13 @@ spec: configMapKeyRef: name: ibm-vpc-file-csi-configmap key: SIDECAR_ENDPOINT + resources: + limits: + cpu: 12m + memory: 20Mi + requests: + cpu: 3m + memory: 5Mi volumeMounts: - mountPath: /sidecardir name: socket-dir diff --git a/deploy/kubernetes/driver/kubernetes/manifests/node-server.yaml b/deploy/kubernetes/driver/kubernetes/manifests/node-server.yaml index a968a566..e246ee06 100644 --- a/deploy/kubernetes/driver/kubernetes/manifests/node-server.yaml +++ b/deploy/kubernetes/driver/kubernetes/manifests/node-server.yaml @@ -31,6 +31,7 @@ spec: securityContext: runAsNonRoot: false runAsUser: 0 + runAsGroup: 0 privileged: false args: - "--v=5" @@ -64,6 +65,7 @@ spec: securityContext: runAsNonRoot: false runAsUser: 0 + runAsGroup: 0 privileged: true image: MUSTPATCHWITHKUSTOMIZE imagePullPolicy: Always @@ -91,6 +93,10 @@ spec: fieldPath: spec.nodeName - name: SOCKET_PATH value: "/var/lib/ibmshare.sock" + - name: IS_NODE_SERVER + value: "true" + - name: SIDECAR_GROUP_ID + value: "2121" resources: limits: cpu: 200m @@ -139,11 +145,15 @@ spec: - mountPath: /tmp/mount-helper name: mh-logs - name: liveness-probe - image: MUSTPATCHWITHKUSTOMIZE securityContext: - runAsNonRoot: false - runAsUser: 0 + runAsNonRoot: true + runAsUser: 2121 + runAsGroup: 2121 privileged: false + seLinuxOptions: # seLinux label is set as a precaution for accessing csi socket + type: spc_t + level: s0 + image: MUSTPATCHWITHKUSTOMIZE args: - "--csi-address=$(CSI_ADDRESS)" env: @@ -163,6 +173,11 @@ spec: - name: plugin-dir mountPath: /csi - name: storage-secret-sidecar + securityContext: + runAsNonRoot: true + runAsUser: 2121 + runAsGroup: 2121 + privileged: false image: MUSTPATCHWITHKUSTOMIZE imagePullPolicy: Always args: diff --git a/pkg/ibmcsidriver/fileOps.go b/pkg/ibmcsidriver/fileOps.go new file mode 100644 index 00000000..3d79bf3f --- /dev/null +++ b/pkg/ibmcsidriver/fileOps.go @@ -0,0 +1,85 @@ +/** + * + * Copyright 2024- IBM Inc. All rights reserved + * SPDX-License-Identifier: Apache2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// Package ibmcsidriver ... +package ibmcsidriver + +//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -generate + +import ( + "os" + "strconv" + + "go.uber.org/zap" +) + +const ( + filePermission = 0660 +) + +//counterfeiter:generate . socketPermission + +// socketPermission represents file system operations +type socketPermission interface { + Chown(name string, uid, gid int) error + Chmod(name string, mode os.FileMode) error +} + +// realSocketPermission implements socketPermission +type opsSocketPermission struct{} + +func (f *opsSocketPermission) Chown(name string, uid, gid int) error { + return os.Chown(name, uid, gid) +} + +func (f *opsSocketPermission) Chmod(name string, mode os.FileMode) error { + return os.Chmod(name, mode) +} + +// setupSidecar updates owner/group and permission of the file given(addr) +func setupSidecar(addr string, ops socketPermission, logger *zap.Logger) error { + groupSt := os.Getenv("SIDECAR_GROUP_ID") + + logger.Info("Setting owner and permissions of csi socket file. SIDECAR_GROUP_ID env must match the 'livenessprobe' sidecar container groupID for csi socket connection.") + + // If env is not set, set default to 0 + if groupSt == "" { + logger.Warn("Unable to fetch SIDECAR_GROUP_ID environment variable. Sidecar container(s) might fail...") + groupSt = "0" + } + + group, err := strconv.Atoi(groupSt) + if err != nil { + return err + } + + // Change group of csi socket to non-root user for enabling the csi sidecar + if err := ops.Chown(addr, -1, group); err != nil { + return err + } + + // Modify permissions of csi socket + // Only the users and the group owners will have read/write access to csi socket + if err := ops.Chmod(addr, filePermission); err != nil { + return err + } + + logger.Info("Successfully set owner and permissions of csi socket file.") + + return nil +} diff --git a/pkg/ibmcsidriver/fileOps_test.go b/pkg/ibmcsidriver/fileOps_test.go new file mode 100644 index 00000000..20326a05 --- /dev/null +++ b/pkg/ibmcsidriver/fileOps_test.go @@ -0,0 +1,125 @@ +/** + * + * Copyright 2024- IBM Inc. All rights reserved + * SPDX-License-Identifier: Apache2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package ibmcsidriver + +import ( + "errors" + "os" + "testing" + + "github.com/IBM/ibm-vpc-file-csi-driver/pkg/ibmcsidriver/ibmcsidriverfakes" + "github.com/stretchr/testify/assert" +) + +func TestSetupSidecar(t *testing.T) { + tests := []struct { + name string + groupID string + expectedErr bool + chownErr error + chmodErr error + expectedChownCalls int + expectedChmodCalls int + expectedGroupID int + }{ + { + name: "ValidGroupID", + groupID: "2121", + expectedErr: false, + chownErr: nil, + chmodErr: nil, + expectedChownCalls: 1, + expectedChmodCalls: 1, + expectedGroupID: 2121, + }, + { + name: "EmptyGroupID", + groupID: "", + expectedErr: false, + chownErr: nil, + chmodErr: nil, + expectedChownCalls: 1, + expectedChmodCalls: 1, + expectedGroupID: 0, // Default to 0 if SIDECAR_GROUP_ID is empty + }, + { + name: "ChownError", + groupID: "1000", + expectedErr: true, + chownErr: errors.New("chown error"), + chmodErr: nil, + expectedChownCalls: 1, + expectedChmodCalls: 0, // No chmod expected if chown fails + expectedGroupID: 1000, + }, + { + name: "ChmodError", + groupID: "1000", + expectedErr: true, + chownErr: nil, + chmodErr: errors.New("chmod error"), + expectedChownCalls: 1, + expectedChmodCalls: 1, + expectedGroupID: 1000, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Set SIDECAR_GROUP_ID environment variable + if tc.groupID != "" { + os.Setenv("SIDECAR_GROUP_ID", tc.groupID) + } else { + os.Unsetenv("SIDECAR_GROUP_ID") + } + defer os.Unsetenv("SIDECAR_GROUP_ID") + + // Create the fake object generated by counterfeiter + fakeSocketPermission := new(ibmcsidriverfakes.FakeSocketPermission) + + // Set return values for chown and chmod methods + fakeSocketPermission.ChownReturns(tc.chownErr) + fakeSocketPermission.ChmodReturns(tc.chmodErr) + + // Creating test logger + logger, teardown := GetTestLogger(t) + defer teardown() + + // Call the function under test + err := setupSidecar("/path/to/socket", fakeSocketPermission, logger) + + // Verify the result + if tc.expectedErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + + // Verify the number of times Chown and Chmod were called + assert.Equal(t, tc.expectedChownCalls, fakeSocketPermission.ChownCallCount()) + assert.Equal(t, tc.expectedChmodCalls, fakeSocketPermission.ChmodCallCount()) + + // Verify the group ID passed to chown + if tc.expectedChownCalls > 0 { + _, _, actualGroupID := fakeSocketPermission.ChownArgsForCall(0) + assert.Equal(t, tc.expectedGroupID, actualGroupID) + } + }) + } +} diff --git a/pkg/ibmcsidriver/ibmcsidriverfakes/fake_socket_permission.go b/pkg/ibmcsidriver/ibmcsidriverfakes/fake_socket_permission.go new file mode 100644 index 00000000..35b0aa69 --- /dev/null +++ b/pkg/ibmcsidriver/ibmcsidriverfakes/fake_socket_permission.go @@ -0,0 +1,188 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package ibmcsidriverfakes + +import ( + "io/fs" + "sync" +) + +type FakeSocketPermission struct { + ChmodStub func(string, fs.FileMode) error + chmodMutex sync.RWMutex + chmodArgsForCall []struct { + arg1 string + arg2 fs.FileMode + } + chmodReturns struct { + result1 error + } + chmodReturnsOnCall map[int]struct { + result1 error + } + ChownStub func(string, int, int) error + chownMutex sync.RWMutex + chownArgsForCall []struct { + arg1 string + arg2 int + arg3 int + } + chownReturns struct { + result1 error + } + chownReturnsOnCall map[int]struct { + result1 error + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeSocketPermission) Chmod(arg1 string, arg2 fs.FileMode) error { + fake.chmodMutex.Lock() + ret, specificReturn := fake.chmodReturnsOnCall[len(fake.chmodArgsForCall)] + fake.chmodArgsForCall = append(fake.chmodArgsForCall, struct { + arg1 string + arg2 fs.FileMode + }{arg1, arg2}) + stub := fake.ChmodStub + fakeReturns := fake.chmodReturns + fake.recordInvocation("Chmod", []interface{}{arg1, arg2}) + fake.chmodMutex.Unlock() + if stub != nil { + return stub(arg1, arg2) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeSocketPermission) ChmodCallCount() int { + fake.chmodMutex.RLock() + defer fake.chmodMutex.RUnlock() + return len(fake.chmodArgsForCall) +} + +func (fake *FakeSocketPermission) ChmodCalls(stub func(string, fs.FileMode) error) { + fake.chmodMutex.Lock() + defer fake.chmodMutex.Unlock() + fake.ChmodStub = stub +} + +func (fake *FakeSocketPermission) ChmodArgsForCall(i int) (string, fs.FileMode) { + fake.chmodMutex.RLock() + defer fake.chmodMutex.RUnlock() + argsForCall := fake.chmodArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *FakeSocketPermission) ChmodReturns(result1 error) { + fake.chmodMutex.Lock() + defer fake.chmodMutex.Unlock() + fake.ChmodStub = nil + fake.chmodReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeSocketPermission) ChmodReturnsOnCall(i int, result1 error) { + fake.chmodMutex.Lock() + defer fake.chmodMutex.Unlock() + fake.ChmodStub = nil + if fake.chmodReturnsOnCall == nil { + fake.chmodReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.chmodReturnsOnCall[i] = struct { + result1 error + }{result1} +} + +func (fake *FakeSocketPermission) Chown(arg1 string, arg2 int, arg3 int) error { + fake.chownMutex.Lock() + ret, specificReturn := fake.chownReturnsOnCall[len(fake.chownArgsForCall)] + fake.chownArgsForCall = append(fake.chownArgsForCall, struct { + arg1 string + arg2 int + arg3 int + }{arg1, arg2, arg3}) + stub := fake.ChownStub + fakeReturns := fake.chownReturns + fake.recordInvocation("Chown", []interface{}{arg1, arg2, arg3}) + fake.chownMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeSocketPermission) ChownCallCount() int { + fake.chownMutex.RLock() + defer fake.chownMutex.RUnlock() + return len(fake.chownArgsForCall) +} + +func (fake *FakeSocketPermission) ChownCalls(stub func(string, int, int) error) { + fake.chownMutex.Lock() + defer fake.chownMutex.Unlock() + fake.ChownStub = stub +} + +func (fake *FakeSocketPermission) ChownArgsForCall(i int) (string, int, int) { + fake.chownMutex.RLock() + defer fake.chownMutex.RUnlock() + argsForCall := fake.chownArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 +} + +func (fake *FakeSocketPermission) ChownReturns(result1 error) { + fake.chownMutex.Lock() + defer fake.chownMutex.Unlock() + fake.ChownStub = nil + fake.chownReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeSocketPermission) ChownReturnsOnCall(i int, result1 error) { + fake.chownMutex.Lock() + defer fake.chownMutex.Unlock() + fake.ChownStub = nil + if fake.chownReturnsOnCall == nil { + fake.chownReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.chownReturnsOnCall[i] = struct { + result1 error + }{result1} +} + +func (fake *FakeSocketPermission) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.chmodMutex.RLock() + defer fake.chmodMutex.RUnlock() + fake.chownMutex.RLock() + defer fake.chownMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeSocketPermission) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} diff --git a/pkg/ibmcsidriver/server.go b/pkg/ibmcsidriver/server.go index df23b0b6..aa11aa6e 100644 --- a/pkg/ibmcsidriver/server.go +++ b/pkg/ibmcsidriver/server.go @@ -114,6 +114,7 @@ func (s *nonBlockingGRPCServer) Setup(endpoint string, ids csi.IdentityServer, c s.logger.Info("Start listening GRPC Server", zap.Reflect("Scheme", u.Scheme), zap.Reflect("Addr", addr)) + // Create listener listener, err := net.Listen(u.Scheme, addr) if err != nil { msg := "failed to listen GRPC Server" @@ -121,6 +122,16 @@ func (s *nonBlockingGRPCServer) Setup(endpoint string, ids csi.IdentityServer, c return nil, errors.New(msg) } + // In case of nodeSerer container, setup desired csi socket permissions and user/group. + // This is required for running `livenessprobe` container as non-root user/group + if os.Getenv("IS_NODE_SERVER") == "true" { + fileops := &opsSocketPermission{} + if err := setupSidecar(addr, fileops, s.logger); err != nil { + s.logger.Error("setupSidecar failed.", zap.Error(err)) + return nil, err + } + } + server := grpc.NewServer(opts...) s.server = server