Skip to content

Commit afd457a

Browse files
committed
Clear secrets from request for klog print in logGRPC()
Malicious user can put a secret in request as explained here: #1372.
1 parent b105d5a commit afd457a

File tree

2 files changed

+49
-2
lines changed

2 files changed

+49
-2
lines changed

Diff for: pkg/gce-pd-csi-driver/utils.go

+19-2
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"context"
2121
"errors"
2222
"fmt"
23+
"reflect"
2324

2425
csi "github.com/container-storage-interface/spec/lib/go/csi"
2526
"google.golang.org/grpc"
@@ -56,14 +57,30 @@ func NewNodeServiceCapability(cap csi.NodeServiceCapability_RPC_Type) *csi.NodeS
5657
}
5758
}
5859

60+
// Reflect magic below simply clears Secrets map from request.
61+
func clearSecrets(req interface{}) interface{} {
62+
v := reflect.ValueOf(&req).Elem()
63+
e := reflect.New(v.Elem().Type()).Elem()
64+
e.Set(v.Elem())
65+
f := reflect.Indirect(e).FieldByName("Secrets")
66+
if f.IsValid() && f.CanSet() && f.Kind() == reflect.Map {
67+
f.Set(reflect.MakeMap(f.Type()))
68+
v.Set(e)
69+
}
70+
return req
71+
}
72+
5973
func logGRPC(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
6074
if info.FullMethod == ProbeCSIFullMethod {
6175
return handler(ctx, req)
6276
}
63-
// Note that secrets are not included in any RPC message. In the past protosanitizer and other log
77+
// Note that secrets may be included in some RPC messages
78+
// (https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/issues/1372),
79+
// but the driver ignores them. In the past protosanitizer and other log
6480
// stripping was shown to cause a significant increase of CPU usage (see
6581
// https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/issues/356#issuecomment-550529004).
66-
klog.V(4).Infof("%s called with request: %s", info.FullMethod, req)
82+
// That is why we use hand-crafted clearSecrets() below rather than protosanitizer.
83+
klog.V(4).Infof("%s called with request: %s", info.FullMethod, clearSecrets(req))
6784
resp, err := handler(ctx, req)
6885
if err != nil {
6986
klog.Errorf("%s returned with error: %v", info.FullMethod, err.Error())

Diff for: pkg/gce-pd-csi-driver/utils_test.go

+30
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ limitations under the License.
1818
package gceGCEDriver
1919

2020
import (
21+
"reflect"
2122
"testing"
2223

2324
csi "github.com/container-storage-interface/spec/lib/go/csi"
@@ -291,3 +292,32 @@ func TestGetReadOnlyFromCapabilities(t *testing.T) {
291292
}
292293
}
293294
}
295+
296+
func TestClearSecrets(t *testing.T) {
297+
vc := &csi.VolumeCapability{
298+
AccessType: &csi.VolumeCapability_Mount{
299+
Mount: &csi.VolumeCapability_MountVolume{},
300+
},
301+
AccessMode: &csi.VolumeCapability_AccessMode{
302+
Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
303+
},
304+
}
305+
306+
req := &csi.NodeExpandVolumeRequest{
307+
VolumePath: "/path",
308+
VolumeCapability: vc,
309+
Secrets: map[string]string{
310+
"key": "value",
311+
},
312+
}
313+
314+
clearedReq := &csi.NodeExpandVolumeRequest{
315+
VolumePath: "/path",
316+
VolumeCapability: vc,
317+
Secrets: map[string]string{},
318+
}
319+
320+
if !reflect.DeepEqual(clearSecrets(req), clearedReq) {
321+
t.Fatalf("Unexpected change: %v vs. %v", clearSecrets(req), clearedReq)
322+
}
323+
}

0 commit comments

Comments
 (0)