From afd457acdd648aafeea6a03f36c154715a5b97c0 Mon Sep 17 00:00:00 2001
From: Maxim Patlasov <mpt@redhat.com>
Date: Thu, 19 Oct 2023 19:34:06 -0700
Subject: [PATCH] Clear secrets from request for klog print in `logGRPC()`

Malicious user can put a secret in request as explained here: https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/issues/1372.
---
 pkg/gce-pd-csi-driver/utils.go      | 21 ++++++++++++++++++--
 pkg/gce-pd-csi-driver/utils_test.go | 30 +++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/pkg/gce-pd-csi-driver/utils.go b/pkg/gce-pd-csi-driver/utils.go
index d8ef8ce0b7..94db11f738 100644
--- a/pkg/gce-pd-csi-driver/utils.go
+++ b/pkg/gce-pd-csi-driver/utils.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"reflect"
 
 	csi "github.com/container-storage-interface/spec/lib/go/csi"
 	"google.golang.org/grpc"
@@ -56,14 +57,30 @@ func NewNodeServiceCapability(cap csi.NodeServiceCapability_RPC_Type) *csi.NodeS
 	}
 }
 
+// Reflect magic below simply clears Secrets map from request.
+func clearSecrets(req interface{}) interface{} {
+	v := reflect.ValueOf(&req).Elem()
+	e := reflect.New(v.Elem().Type()).Elem()
+	e.Set(v.Elem())
+	f := reflect.Indirect(e).FieldByName("Secrets")
+	if f.IsValid() && f.CanSet() && f.Kind() == reflect.Map {
+		f.Set(reflect.MakeMap(f.Type()))
+		v.Set(e)
+	}
+	return req
+}
+
 func logGRPC(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
 	if info.FullMethod == ProbeCSIFullMethod {
 		return handler(ctx, req)
 	}
-	// Note that secrets are not included in any RPC message. In the past protosanitizer and other log
+	// Note that secrets may be included in some RPC messages
+	// (https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/issues/1372),
+	// but the driver ignores them. In the past protosanitizer and other log
 	// stripping was shown to cause a significant increase of CPU usage (see
 	// https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/issues/356#issuecomment-550529004).
-	klog.V(4).Infof("%s called with request: %s", info.FullMethod, req)
+	// That is why we use hand-crafted clearSecrets() below rather than protosanitizer.
+	klog.V(4).Infof("%s called with request: %s", info.FullMethod, clearSecrets(req))
 	resp, err := handler(ctx, req)
 	if err != nil {
 		klog.Errorf("%s returned with error: %v", info.FullMethod, err.Error())
diff --git a/pkg/gce-pd-csi-driver/utils_test.go b/pkg/gce-pd-csi-driver/utils_test.go
index 2a0fd5f45b..5637b9e6d3 100644
--- a/pkg/gce-pd-csi-driver/utils_test.go
+++ b/pkg/gce-pd-csi-driver/utils_test.go
@@ -18,6 +18,7 @@ limitations under the License.
 package gceGCEDriver
 
 import (
+	"reflect"
 	"testing"
 
 	csi "github.com/container-storage-interface/spec/lib/go/csi"
@@ -291,3 +292,32 @@ func TestGetReadOnlyFromCapabilities(t *testing.T) {
 		}
 	}
 }
+
+func TestClearSecrets(t *testing.T) {
+	vc := &csi.VolumeCapability{
+		AccessType: &csi.VolumeCapability_Mount{
+			Mount: &csi.VolumeCapability_MountVolume{},
+		},
+		AccessMode: &csi.VolumeCapability_AccessMode{
+			Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
+		},
+	}
+
+	req := &csi.NodeExpandVolumeRequest{
+		VolumePath:       "/path",
+		VolumeCapability: vc,
+		Secrets: map[string]string{
+			"key": "value",
+		},
+	}
+
+	clearedReq := &csi.NodeExpandVolumeRequest{
+		VolumePath:       "/path",
+		VolumeCapability: vc,
+		Secrets:          map[string]string{},
+	}
+
+	if !reflect.DeepEqual(clearSecrets(req), clearedReq) {
+		t.Fatalf("Unexpected change: %v vs. %v", clearSecrets(req), clearedReq)
+	}
+}