fix: validate SPCPS targetPaths match Pod UIDs

Consolidated duplicate GetMountedFiles and GetPodUIDFromTargetPath as well. Apply suggestions from code review Co-authored-by: Rita Zhang <[email protected]> Co-authored-by: Anish Ramasekar <[email protected]> fix: include a check on volume name matches the targetPath k8sutil: refactor common func SPCVolume
kubernetes-sigs · Nov 2, 2020 · c2cbb19 · c2cbb19
1 parent 69c2fd5
commit c2cbb19
Show file tree

Hide file tree

Showing 13 changed files with 645 additions and 133 deletions.
diff --git a/controllers/secretproviderclasspodstatus_controller.go b/controllers/secretproviderclasspodstatus_controller.go
@@ -19,7 +19,6 @@ package controllers
 import (
 	"context"
 	"fmt"
-	"regexp"
 	"strings"
 	"sync"
 	"time"
@@ -35,6 +34,7 @@ import (
 	"sigs.k8s.io/secrets-store-csi-driver/apis/v1alpha1"
 	"sigs.k8s.io/secrets-store-csi-driver/pkg/client/clientset/versioned/scheme"
 	"sigs.k8s.io/secrets-store-csi-driver/pkg/util/fileutil"
+	"sigs.k8s.io/secrets-store-csi-driver/pkg/util/k8sutil"
 	"sigs.k8s.io/secrets-store-csi-driver/pkg/util/secretutil"
 
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -227,18 +227,34 @@ func (r *SecretProviderClassPodStatusReconciler) Reconcile(req ctrl.Request) (ct
 		return ctrl.Result{}, nil
 	}
 
-	// podObjectReference is an object reference to the pod that spc pod status
-	// is created for. The object reference is created with minimal required fields
-	// name, namespace and UID. By doing this we can skip an additional client call
-	// to fetch the pod object
-	podObjectReference, err := getPodObjectReference(spcPodStatus)
-	if err != nil {
-		logger.Errorf("failed to get pod object reference, error: %+v", err)
+	// Obtain the full pod metadata. An object reference is needed for sending
+	// events and the UID is helpful for validating the SPCPS TargetPath.
+	pod := &v1.Pod{}
+	if err := r.reader.Get(ctx, client.ObjectKey{Namespace: req.Namespace, Name: spcPodStatus.Status.PodName}, pod); err != nil {
+		logger.Errorf("failed to get pod %s/%s, err: %+v", req.Namespace, spcPodStatus.Status.PodName, err)
+		if apierrors.IsNotFound(err) {
+			return ctrl.Result{RequeueAfter: 5 * time.Second}, nil
+		}
+		return ctrl.Result{}, err
+	}
+
+	// determine which pod volume this is associated with
+	podVol := k8sutil.SPCVolume(pod, spc.Name)
+	if podVol == nil {
+		return ctrl.Result{}, fmt.Errorf("failed to find secret provider class pod status volume for pod %s/%s", req.Namespace, spcPodStatus.Status.PodName)
+	}
+
+	// validate TargetPath
+	if fileutil.GetPodUIDFromTargetPath(spcPodStatus.Status.TargetPath) != string(pod.UID) {
+		return ctrl.Result{}, fmt.Errorf("secret provider class pod status targetPath did not match pod UID for pod %s/%s", req.Namespace, spcPodStatus.Status.PodName)
+	}
+	if fileutil.GetVolumeNameFromTargetPath(spcPodStatus.Status.TargetPath) != podVol.Name {
+		return ctrl.Result{}, fmt.Errorf("secret provider class pod status volume name did not match pod Volume for pod %s/%s", req.Namespace, spcPodStatus.Status.PodName)
 	}
 
 	files, err := fileutil.GetMountedFiles(spcPodStatus.Status.TargetPath)
 	if err != nil {
-		r.generateEvent(podObjectReference, corev1.EventTypeWarning, secretCreationFailedReason, fmt.Sprintf("failed to get mounted files, err: %+v", err))
+		r.generateEvent(pod, corev1.EventTypeWarning, secretCreationFailedReason, fmt.Sprintf("failed to get mounted files, err: %+v", err))
 		logger.Errorf("failed to get mounted files, err: %+v", err)
 		return ctrl.Result{RequeueAfter: 10 * time.Second}, err
 	}
@@ -265,7 +281,7 @@ func (r *SecretProviderClassPodStatusReconciler) Reconcile(req ctrl.Request) (ct
 
 			datamap := make(map[string][]byte)
 			if datamap, err = secretutil.GetSecretData(secretObj.Data, secretType, files); err != nil {
-				r.generateEvent(podObjectReference, corev1.EventTypeWarning, secretCreationFailedReason, fmt.Sprintf("failed to get data in spc %s/%s for secret %s, err: %+v", req.Namespace, spcName, secretName, err))
+				r.generateEvent(pod, corev1.EventTypeWarning, secretCreationFailedReason, fmt.Sprintf("failed to get data in spc %s/%s for secret %s, err: %+v", req.Namespace, spcName, secretName, err))
 				log.Errorf("failed to get data in spc %s/%s for secret %s, err: %+v", req.Namespace, spcName, secretName, err)
 				errs = append(errs, fmt.Errorf("failed to get data in spc %s/%s for secret %s, err: %+v", req.Namespace, spcName, secretName, err))
 				continue
@@ -297,7 +313,7 @@ func (r *SecretProviderClassPodStatusReconciler) Reconcile(req ctrl.Request) (ct
 				Factor:   1.0,
 				Jitter:   0.1,
 			}, f); err != nil {
-				r.generateEvent(podObjectReference, corev1.EventTypeWarning, secretCreationFailedReason, err.Error())
+				r.generateEvent(pod, corev1.EventTypeWarning, secretCreationFailedReason, err.Error())
 				return ctrl.Result{RequeueAfter: 5 * time.Second}, err
 			}
 		}
@@ -401,31 +417,6 @@ func (r *SecretProviderClassPodStatusReconciler) secretExists(ctx context.Contex
 	return false, err
 }
 
-// getPodObjectReference returns a v1.ObjectReference for the pod object
-func getPodObjectReference(spcPodStatus v1alpha1.SecretProviderClassPodStatus) (*v1.ObjectReference, error) {
-	podName := spcPodStatus.Status.PodName
-	podNamespace := spcPodStatus.Namespace
-	podUID := getPodUIDFromTargetPath(spcPodStatus.Status.TargetPath)
-	if podUID == "" {
-		return nil, fmt.Errorf("failed to get pod UID from target path")
-	}
-	return &v1.ObjectReference{
-		Name:      podName,
-		Namespace: podNamespace,
-		UID:       types.UID(podUID),
-	}, nil
-}
-
-// getPodUIDFromTargetPath returns podUID from targetPath
-func getPodUIDFromTargetPath(targetPath string) string {
-	re := regexp.MustCompile(`[\\|\/]+pods[\\|\/]+(.+?)[\\|\/]+volumes`)
-	match := re.FindStringSubmatch(targetPath)
-	if len(match) < 2 {
-		return ""
-	}
-	return match[1]
-}
-
 // generateEvent generates an event
 func (r *SecretProviderClassPodStatusReconciler) generateEvent(obj runtime.Object, eventType, reason, message string) {
 	if obj != nil {

diff --git a/controllers/secretproviderclasspodstatus_controller_test.go b/controllers/secretproviderclasspodstatus_controller_test.go
@@ -172,49 +172,6 @@ func TestCreateK8sSecret(t *testing.T) {
 	g.Expect(secret.Name).To(Equal("my-secret2"))
 }
 
-func TestGetPodUIDFromTargetPath(t *testing.T) {
-	g := NewWithT(t)
-
-	cases := []struct {
-		targetPath     string
-		expectedPodUID string
-	}{
-		{
-			targetPath:     "/var/lib/kubelet/pods/7e7686a1-56c4-4c67-a6fd-4656ac484f0a/volumes/",
-			expectedPodUID: "7e7686a1-56c4-4c67-a6fd-4656ac484f0a",
-		},
-		{
-			targetPath:     `c:\var\lib\kubelet\pods\d4fd876f-bdb3-11e9-a369-0a5d188d99c0\volumes`,
-			expectedPodUID: "d4fd876f-bdb3-11e9-a369-0a5d188d99c0",
-		},
-		{
-			targetPath:     `c:\\var\\lib\\kubelet\\pods\\d4fd876f-bdb3-11e9-a369-0a5d188d9934\\volumes`,
-			expectedPodUID: "d4fd876f-bdb3-11e9-a369-0a5d188d9934",
-		},
-		{
-			targetPath:     "/var/lib/",
-			expectedPodUID: "",
-		},
-		{
-			targetPath:     "/var/lib/kubelet/pods",
-			expectedPodUID: "",
-		},
-		{
-			targetPath:     "/opt/new/var/lib/kubelet/pods/456457fc-d980-4191-b5eb-daf70c4ff7c1/volumes/kubernetes.io~csi/secrets-store-inline/mount",
-			expectedPodUID: "456457fc-d980-4191-b5eb-daf70c4ff7c1",
-		},
-		{
-			targetPath:     "data/kubelet/pods/456457fc-d980-4191-b5eb-daf70c4ff7c1/volumes/kubernetes.io~csi/secrets-store-inline/mount",
-			expectedPodUID: "456457fc-d980-4191-b5eb-daf70c4ff7c1",
-		},
-	}
-
-	for _, tc := range cases {
-		actualPodUID := getPodUIDFromTargetPath(tc.targetPath)
-		g.Expect(actualPodUID).To(Equal(tc.expectedPodUID))
-	}
-}
-
 func TestGenerateEvent(t *testing.T) {
 	g := NewWithT(t)
 

diff --git a/go.mod b/go.mod
@@ -6,6 +6,7 @@ require (
 	github.com/blang/semver v3.5.0+incompatible
 	github.com/container-storage-interface/spec v1.0.0
 	github.com/golang/protobuf v1.4.2
+	github.com/google/go-cmp v0.5.0
 	github.com/kubernetes-csi/csi-lib-utils v0.6.1
 	github.com/kubernetes-csi/csi-test v1.1.0
 	github.com/onsi/gomega v1.8.1

diff --git a/pkg/errors/errors.go b/pkg/errors/errors.go
@@ -40,4 +40,9 @@ const (
 	// NodePublishSecretRefNotFound error
 	// #nosec G101 (Ref: https://github.com/securego/gosec/issues/295)
 	NodePublishSecretRefNotFound = "NodePublishSecretRefNotFound"
+	// UnexpectedTargetPath error
+	// Indicated SecretProviderClassPodStatus status.targetPath is an invalid value.
+	UnexpectedTargetPath = "UnexpectedTargetPath"
+	// PodVolumeNotFound error
+	PodVolumeNotFound = "PodVolumeNotFound"
 )
diff --git a/pkg/rotation/reconciler.go b/pkg/rotation/reconciler.go
@@ -30,6 +30,7 @@ import (
 
 	internalerrors "sigs.k8s.io/secrets-store-csi-driver/pkg/errors"
 	"sigs.k8s.io/secrets-store-csi-driver/pkg/util/fileutil"
+	"sigs.k8s.io/secrets-store-csi-driver/pkg/util/k8sutil"
 	"sigs.k8s.io/secrets-store-csi-driver/pkg/util/secretutil"
 
 	"k8s.io/client-go/tools/cache"
@@ -196,6 +197,23 @@ func (r *Reconciler) reconcile(ctx context.Context, spcps *v1alpha1.SecretProvid
 		return fmt.Errorf("failed to get pod %s/%s, err: %+v", podNamespace, podName, err)
 	}
 
+	// determine which pod volume this is associated with
+	podVol := k8sutil.SPCVolume(pod, spc.Name)
+	if podVol == nil {
+		errorReason = internalerrors.PodVolumeNotFound
+		return fmt.Errorf("could not find secret provider class pod status volume for pod %s/%s", podNamespace, podName)
+	}
+
+	// validate TargetPath
+	if fileutil.GetPodUIDFromTargetPath(spcps.Status.TargetPath) != string(pod.UID) {
+		errorReason = internalerrors.UnexpectedTargetPath
+		return fmt.Errorf("secret provider class pod status targetPath did not match pod UID for pod %s/%s", podNamespace, podName)
+	}
+	if fileutil.GetVolumeNameFromTargetPath(spcps.Status.TargetPath) != podVol.Name {
+		errorReason = internalerrors.UnexpectedTargetPath
+		return fmt.Errorf("secret provider class pod status volume name did not match pod Volume for pod %s/%s", podNamespace, podName)
+	}
+
 	parameters := make(map[string]string)
 	if spc.Spec.Parameters != nil {
 		parameters = spc.Spec.Parameters
@@ -217,20 +235,7 @@ func (r *Reconciler) reconcile(ctx context.Context, spcps *v1alpha1.SecretProvid
 
 	// check if the volume pertaining to the current spc is using nodePublishSecretRef for
 	// accessing external secrets store
-	var nodePublishSecretRef *v1.LocalObjectReference
-	for _, vol := range pod.Spec.Volumes {
-		if vol.CSI == nil {
-			continue
-		}
-		if vol.CSI.Driver != "secrets-store.csi.k8s.io" {
-			continue
-		}
-		if vol.CSI.VolumeAttributes["secretProviderClass"] != spc.Name {
-			continue
-		}
-		nodePublishSecretRef = vol.CSI.NodePublishSecretRef
-		break
-	}
+	nodePublishSecretRef := podVol.CSI.NodePublishSecretRef
 
 	var secretsJSON []byte
 	nodePublishSecretData := make(map[string]string)