Clear secrets from request for klog print in logGRPC()

Malicious user can put a secret in request as explained here: kubernetes-sigs#1372.

Author: mpatlasov

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"
@@ -59,14 +60,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())

pkg/gce-pd-csi-driver/utils_test.go

@@ -18,11 +18,14 @@ limitations under the License.
 package gceGCEDriver
 
 import (
+	"context"
 	"fmt"
+	"reflect"
 	"testing"
 
 	csi "github.com/container-storage-interface/spec/lib/go/csi"
 	"github.com/google/go-cmp/cmp"
+	"google.golang.org/grpc"
 	"sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common"
 )
 
@@ -723,3 +726,53 @@ func TestValidateStoragePoolZones(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)
+	}
+}
+
+func TestLogGRPC(t *testing.T) {
+	info := &grpc.UnaryServerInfo{
+		FullMethod: "foo",
+	}
+
+	req := struct {
+		Secrets map[string]string
+	}{map[string]string{
+		"password": "pass",
+	}}
+
+	handler := func(ctx context.Context, req any) (any, error) {
+		return nil, nil
+	}
+
+	_, err := logGRPC(nil, req, info, handler)
+	if err != nil {
+		t.Fatalf("logGRPC returns error %v", err)
+	}
+}