never log secrets

When running at glog level >= 5, external-provisioner logged the full
CreateVolumeRequest, including the secrets. Secrets should never be
logged at any level to avoid accidentally exposing them.

We need to filter out the secrets. With older CSI versions, that could
have been done based on the field name, which is still an option
should this get backported. With CSI 1.0, a custom field option marks
fields as secret. Using that option has the advantage that the code
will continue to work also when new secret fields get added in the
future.

For the sake of simplicity, JSON is now used as representation of the
string instead of the former compact text format from gRPC. That makes
it possible to strip values from a map with generic types, instead of
having to copy and manipulate the real generated structures.

Another option would have been to copy
https://github.com/golang/protobuf/blob/master/proto/text.go and
modify it so that it skips secret fields, but that's over 800 lines of
code.

Ultimately this new package should live in a "csi-common" repo and
also include other utility code, like logGRPC itself.

Fixes: kubernetes-csi#82, kubernetes-csi#167

pkg/controller/controller.go

@@ -51,6 +51,7 @@ import (
 	"github.com/container-storage-interface/spec/lib/go/csi"
 	csiclientset "k8s.io/csi-api/pkg/client/clientset/versioned"
 
+	"github.com/kubernetes-csi/external-provisioner/pkg/csigrpc"
 	"github.com/kubernetes-csi/external-provisioner/pkg/features"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 )
@@ -119,9 +120,9 @@ var (
 //TODO consolidate ane librarize
 func logGRPC(ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error {
 	glog.V(5).Infof("GRPC call: %s", method)
-	glog.V(5).Infof("GRPC request: %+v", req)
+	glog.V(5).Infof("GRPC request: %s", csigrpc.StripSecrets(req))
 	err := invoker(ctx, method, req, reply, cc, opts...)
-	glog.V(5).Infof("GRPC response: %+v", reply)
+	glog.V(5).Infof("GRPC response: %s", csigrpc.StripSecrets(reply))
 	glog.V(5).Infof("GRPC error: %v", err)
 	return err
 }

pkg/csigrpc/csigrpc.go

@@ -0,0 +1,135 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+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 csigrpc contains common utility code for CSI drivers and sidecar
+// apps that helps with operations related to gRPC.
+package csigrpc
+
+import (
+	"encoding/json"
+	"fmt"
+	"reflect"
+	"strings"
+
+	"github.com/container-storage-interface/spec/lib/go/csi"
+	"github.com/golang/protobuf/descriptor"
+	"github.com/golang/protobuf/proto"
+	protobuf "github.com/golang/protobuf/protoc-gen-go/descriptor"
+)
+
+// StripSecrets returns a wrapper around the original CSI gRPC message
+// which has a Stringer implementation that serializes the message
+// as one-line JSON, but without including secret information.
+// Instead of the secret value(s), the string "***stripped***" is
+// included in the result.
+//
+// StripSecrets itself is fast and therefore it is cheap to pass the
+// result to logging functions which may or may not end up serializing
+// the parameter depending on the current log level.
+func StripSecrets(msg interface{}) fmt.Stringer {
+	return &stripSecrets{msg}
+}
+
+type stripSecrets struct {
+	msg interface{}
+}
+
+func (s *stripSecrets) String() string {
+	// First convert to a generic representation. That's less efficient
+	// than using reflect directly, but easier to work with.
+	var parsed interface{}
+	b, err := json.Marshal(s.msg)
+	if err != nil {
+		return fmt.Sprintf("<<json.Marshal %T: %s>>", s.msg, err)
+	}
+	if err := json.Unmarshal(b, &parsed); err != nil {
+		return fmt.Sprintf("<<json.Unmarshal %T: %s>>", s.msg, err)
+	}
+
+	// Now remove secrets from the generic representation of the message.
+	strip(parsed, s.msg)
+
+	// Re-encoded the stripped representation and return that.
+	b, err = json.Marshal(parsed)
+	if err != nil {
+		return fmt.Sprintf("<<json.Marshal %T: %s>>", s.msg, err)
+	}
+	return string(b)
+}
+
+func strip(parsed interface{}, msg interface{}) {
+	protobufMsg, ok := msg.(descriptor.Message)
+	if !ok {
+		// Not a protobuf message, so we are done.
+		return
+	}
+
+	// The corresponding map in the parsed JSON representation.
+	parsedFields, ok := parsed.(map[string]interface{})
+	if !ok {
+		// Probably nil.
+		return
+	}
+
+	// Walk through all fields and replace those with ***stripped*** that
+	// are marked as secret. This relies on protobuf adding "json:" tags
+	// on each field where the name matches the field name in the protobuf
+	// spec (like volume_capabilities). The field.GetJsonName() method returns
+	// a different name (volumeCapabilities) which we don't use.
+	_, md := descriptor.ForMessage(protobufMsg)
+	fields := md.GetField()
+	if fields != nil {
+		for _, field := range fields {
+			ex, err := proto.GetExtension(field.Options, csi.E_CsiSecret)
+			if err == nil && ex != nil && *ex.(*bool) {
+				parsedFields[field.GetName()] = "***stripped***"
+			} else if field.GetType() == protobuf.FieldDescriptorProto_TYPE_MESSAGE {
+				// When we get here,
+				// the type name is something like ".csi.v1.CapacityRange" (leading dot!)
+				// and looking up "csi.v1.CapacityRange"
+				// returns the type of a pointer to a pointer
+				// to CapacityRange. We need a pointer to such
+				// a value for recursive stripping.
+				typeName := field.GetTypeName()
+				if strings.HasPrefix(typeName, ".") {
+					typeName = typeName[1:]
+				}
+				t := proto.MessageType(typeName)
+				if t == nil || t.Kind() != reflect.Ptr {
+					// Shouldn't happen, but
+					// better check anyway instead
+					// of panicking.
+					continue
+				}
+				v := reflect.New(t.Elem())
+
+				// Recursively strip the message(s) that
+				// the field contains.
+				i := v.Interface()
+				entry := parsedFields[field.GetName()]
+				if slice, ok := entry.([]interface{}); ok {
+					// Array of values, like VolumeCapabilities in CreateVolumeRequest.
+					for _, entry := range slice {
+						strip(entry, i)
+					}
+				} else {
+					// Single value.
+					strip(entry, i)
+				}
+			}
+		}
+	}
+}

pkg/csigrpc/csigrpc_test.go

@@ -0,0 +1,80 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+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 csigrpc
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/container-storage-interface/spec/lib/go/csi"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestStripSecrets(t *testing.T) {
+	secretName := "secret-abc"
+	secretValue := "123"
+	createVolume := &csi.CreateVolumeRequest{
+		Name: "foo",
+		VolumeCapabilities: []*csi.VolumeCapability{
+			&csi.VolumeCapability{
+				AccessType: &csi.VolumeCapability_Mount{
+					Mount: &csi.VolumeCapability_MountVolume{
+						FsType: "ext4",
+					},
+				},
+			},
+		},
+		CapacityRange: &csi.CapacityRange{
+			RequiredBytes: 1024,
+		},
+		Secrets: map[string]string{
+			secretName:   secretValue,
+			"secret-xyz": "987",
+		},
+	}
+
+	cases := []struct {
+		original, stripped interface{}
+	}{
+		{nil, "null"},
+		{1, "1"},
+		{"hello world", `"hello world"`},
+		{true, "true"},
+		{false, "false"},
+		{createVolume, `{"capacity_range":{"required_bytes":1024},"name":"foo","secrets":"***stripped***","volume_capabilities":[{"AccessType":{"Mount":{"fs_type":"ext4"}}}]}`},
+
+		// There is currently no test case that can verify
+		// that recursive stripping works, because there is no
+		// message where that is necessary. The code
+		// nevertheless implements it and it has been verified
+		// manually that it recurses properly for single and
+		// repeated values. One-of might require further work.
+	}
+
+	for _, c := range cases {
+		original := c.original
+		stripped := StripSecrets(original)
+		assert.Equal(t, c.stripped, fmt.Sprintf("%s", stripped), "unexpected result for fmt s")
+		assert.Equal(t, c.stripped, fmt.Sprintf("%v", stripped), "unexpected result for fmt v")
+		assert.Equal(t, original, c.original, "original value modified")
+	}
+
+	// The secret is hidden because StripSecrets is a struct referencing it.
+	dump := fmt.Sprintf("%#v", StripSecrets(createVolume))
+	assert.NotContains(t, secretName, dump)
+	assert.NotContains(t, secretValue, dump)
+}