protosanitizer: handle secrets in oneof and maps, gRPC-style output

protosanitizer/protosanitizer.go

@@ -19,20 +19,25 @@ limitations under the License.
 package protosanitizer
 
 import (
+	"bytes"
 	"encoding/json"
 	"fmt"
+	"io"
 	"reflect"
+	"regexp"
+	"sort"
 	"strings"
 
 	"github.com/golang/protobuf/descriptor"
 	"github.com/golang/protobuf/proto"
 	protobuf "github.com/golang/protobuf/protoc-gen-go/descriptor"
 	protobufdescriptor "github.com/golang/protobuf/protoc-gen-go/descriptor"
+	"github.com/pkg/errors"
 )
 
 // 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.
+// similar to gRPC, but without including secret information.
 // Instead of the secret value(s), the string "***stripped***" is
 // included in the result.
 //
@@ -53,6 +58,12 @@ func StripSecretsCSI03(msg interface{}) fmt.Stringer {
 	return &stripSecrets{msg, isCSI03Secret}
 }
 
+type stripped struct{}
+
+func (s stripped) String() string {
+	return "***stripped***"
+}
+
 type stripSecrets struct {
 	msg interface{}
 
@@ -72,28 +83,91 @@ func (s *stripSecrets) String() string {
 	}
 
 	// Now remove secrets from the generic representation of the message.
-	s.strip(parsed, s.msg)
+	if err := s.strip(parsed, s.msg); err != nil {
+		return fmt.Sprintf("<<error: %s>>", err)
+	}
 
 	// 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)
+	var buf bytes.Buffer
+	marshal(&buf, parsed)
+	return buf.String()
+}
+
+// marshall is mimicking the output of the compact text marshaller
+// from https://github.com/golang/protobuf/blob/master/proto/text.go. It
+// can't be exactly the same because it works on the result of json.Unmarshal
+// into generic types (map, array, int/string/bool).
+func marshal(out io.Writer, parsed interface{}) {
+	switch parsed := parsed.(type) {
+	case map[string]interface{}:
+		out.Write([]byte("<"))
+		var keys []string
+		for k := range parsed {
+			keys = append(keys, k)
+		}
+		// Ensure consistent alphabetic ordering.
+		// Ordering by field number would be nicer, but we don't
+		// have that information here.
+		sort.Strings(keys)
+		for i, k := range keys {
+			if isFieldName(k) {
+				// No quotation marks round simple
+				// strings that are likely to be field
+				// names. We can't be sure 100%,
+				// because both structs and real maps
+				// end up as maps after
+				// json.Unmarshal.
+				out.Write([]byte(k))
+			} else {
+				out.Write([]byte(fmt.Sprintf("%q", k)))
+			}
+			out.Write([]byte(":"))
+			marshal(out, parsed[k])
+			// Avoid redundant space after last element.
+			if i+1 < len(keys) {
+				out.Write([]byte(" "))
+			}
+		}
+		out.Write([]byte(">"))
+	case []interface{}:
+		// gRPC uses < for repeating elements. We use
+		// [ ] because it is a bit more readable.
+		out.Write([]byte("["))
+		for i, v := range parsed {
+			marshal(out, v)
+			// Avoid redundant space after last element.
+			if i+1 < len(parsed) {
+				out.Write([]byte(" "))
+			}
+		}
+		out.Write([]byte("]"))
+	case string:
+		fmt.Fprintf(out, "%q", parsed)
+	default:
+		fmt.Fprint(out, parsed)
 	}
-	return string(b)
 }
 
-func (s *stripSecrets) strip(parsed interface{}, msg interface{}) {
+// isFieldName returns true for strings that start with a-zA-Z and
+// are followed by those, digits or underscore.
+func isFieldName(str string) bool {
+	return fieldNameRe.MatchString(str)
+}
+
+var fieldNameRe = regexp.MustCompile(`[a-zA-Z][a-zA-Z0-9_]*`)
+
+func (s *stripSecrets) strip(parsed interface{}, msg interface{}) error {
 	protobufMsg, ok := msg.(descriptor.Message)
 	if !ok {
-		// Not a protobuf message, so we are done.
-		return
+		// Not a protobuf message, nothing to strip.
+		return nil
 	}
 
 	// The corresponding map in the parsed JSON representation.
 	parsedFields, ok := parsed.(map[string]interface{})
 	if !ok {
-		// Probably nil.
-		return
+		// Probably nil, nothing to strip.
+		return nil
 	}
 
 	// Walk through all fields and replace those with ***stripped*** that
@@ -108,44 +182,130 @@ func (s *stripSecrets) strip(parsed interface{}, msg interface{}) {
 			if s.isSecretField(field) {
 				// Overwrite only if already set.
 				if _, ok := parsedFields[field.GetName()]; ok {
-					parsedFields[field.GetName()] = "***stripped***"
+					parsedFields[field.GetName()] = stripped{}
+				}
+				continue
+			}
+
+			// Not stripped. Decide whether we need to
+			// recursively strip the message(s) that the
+			// field contains.
+
+			if field.GetType() != protobuf.FieldDescriptorProto_TYPE_MESSAGE {
+				// No need to recurse into plain types.
+				continue
+			}
+
+			// 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)
+			// Shouldn't happen, but better check
+			// anyway instead of panicking.
+			if t == nil {
+				return errors.Errorf("%s: unknown type", typeName)
+			}
+			var v reflect.Value
+			switch t.Kind() {
+			case reflect.Map:
+				ptrType := t.Elem()
+				if ptrType.Kind() != reflect.Ptr {
+					// map to plain type, nothing to recurse into
+					continue
+				}
+				v = reflect.New(t.Elem().Elem())
+			case reflect.Ptr:
+				v = reflect.New(t.Elem())
+			default:
+				return errors.Errorf("%s: has no elements", typeName)
+			}
+			i := v.Interface()
+
+			if field.OneofIndex != nil {
+				// A oneof field doesn't have json tags in the generated .pb.go.
+				// Therefore the parsedFields is different and we need to recurse differently:
+				// - the entry is named like the Go field (upper first character, no underscores)
+				// - it contains a map with the name of the individual candidates, again
+				//   with Go naming
+				// Example for proto field "volume" inside "VolumeContentSource":
+				// map[string]interface {} [
+				//    "Type": map[string]interface {} [
+				//       "Volume": *(*"interface {}")(0xc4201c0988),
+				//    ],
+				// ]
+				if len(md.OneofDecl) <= int(*field.OneofIndex) {
+					// Shouldn't happen, bail out.
+					return errors.Errorf("invalid oneof index in %v", field)
+				}
+				oneof := md.OneofDecl[int(*field.OneofIndex)]
+				if oneof.Name == nil {
+					return errors.Errorf("invalid oneof for %s, no name: %v", typeName, oneof)
+				}
+				jsonName := upperFirst(*oneof.Name)
+				entry, ok := parsedFields[jsonName]
+				if !ok || entry == nil {
+					// Not set.
+					continue
+				}
+				oneofMap, ok := entry.(map[string]interface{})
+				if !ok {
+					return errors.Errorf("unexpected type %T in JSON for %s", entry, typeName)
 				}
-			} 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:]
+				if field.JsonName == nil {
+					return errors.Errorf("invalid field %v, no name", field)
 				}
-				t := proto.MessageType(typeName)
-				if t == nil || t.Kind() != reflect.Ptr {
-					// Shouldn't happen, but
-					// better check anyway instead
-					// of panicking.
+				entry, ok = oneofMap[upperFirst(*field.JsonName)]
+				if !ok {
+					// Oneof does not contain this particular field.
 					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 {
-						s.strip(entry, i)
+				// Finally recurse into a particular oneof value.
+				if err := s.strip(entry, i); err != nil {
+					return errors.Wrap(err, typeName)
+				}
+				continue
+			}
+
+			entry := parsedFields[field.GetName()]
+			if slice, ok := entry.([]interface{}); ok {
+				// Array of values, like VolumeCapabilities in CreateVolumeRequest.
+				for _, entry := range slice {
+					if err := s.strip(entry, i); err != nil {
+						return errors.Wrap(err, typeName)
+					}
+				}
+				continue
+			}
+			if mapping, ok := entry.(map[string]interface{}); ok {
+				// All maps in protobuf are string to something maps in JSON.
+				for _, entry := range mapping {
+					if err := s.strip(entry, i); err != nil {
+						return errors.Wrap(err, typeName)
 					}
-				} else {
-					// Single value.
-					s.strip(entry, i)
 				}
 			}
+
+			// Single value.
+			if err := s.strip(entry, i); err != nil {
+				return errors.Wrap(err, typeName)
+			}
 		}
 	}
+	return nil
+}
+
+func upperFirst(str string) string {
+	if len(str) >= 2 {
+		return strings.ToUpper(str[:1]) + str[1:]
+	}
+	return strings.ToUpper(str)
 }
 
 // isCSI1Secret uses the csi.E_CsiSecret extension from CSI 1.0 to

protosanitizer/protosanitizer_test.go

@@ -147,12 +147,12 @@ func TestStripSecrets(t *testing.T) {
 	}
 
 	cases := []testcase{
-		{nil, "null"},
+		{nil, "<nil>"},
 		{1, "1"},
 		{"hello world", `"hello world"`},
 		{true, "true"},
 		{false, "false"},
-		{&csi.CreateVolumeRequest{}, `{}`},
+		{&csi.CreateVolumeRequest{}, `<>`},
 		// Test case from https://github.com/kubernetes-csi/csi-lib-utils/pull/1#pullrequestreview-180126394.
 		{&csi.CreateVolumeRequest{
 			Name: "test-volume",
@@ -177,17 +177,11 @@ func TestStripSecrets(t *testing.T) {
 			Parameters:                map[string]string{"param1": "param1", "param2": "param2"},
 			VolumeContentSource:       &csi.VolumeContentSource{},
 			AccessibilityRequirements: &csi.TopologyRequirement{},
-		}, `{"accessibility_requirements":{},"capacity_range":{"limit_bytes":1024,"required_bytes":1024},"name":"test-volume","parameters":{"param1":"param1","param2":"param2"},"secrets":"***stripped***","volume_capabilities":[{"AccessType":{"Mount":{"fs_type":"ext4","mount_flags":["flag1","flag2","flag3"]}},"access_mode":{"mode":5}}],"volume_content_source":{"Type":null}}`},
-		{createVolume, `{"accessibility_requirements":{"requisite":[{"segments":{"foo":"bar","x":"y"}},{"segments":{"a":"b"}}]},"capacity_range":{"required_bytes":1024},"name":"foo","secrets":"***stripped***","volume_capabilities":[{"AccessType":{"Mount":{"fs_type":"ext4"}}}]}`},
-		{createVolumeCSI03, `{"accessibility_requirements":{"requisite":[{"segments":{"foo":"bar","x":"y"}},{"segments":{"a":"b"}}]},"capacity_range":{"required_bytes":1024},"controller_create_secrets":"***stripped***","name":"foo","volume_capabilities":[{"AccessType":{"Mount":{"fs_type":"ext4"}}}]}`},
-		{&csitest.CreateVolumeRequest{}, `{}`},
+		}, `<accessibility_requirements:<> capacity_range:<limit_bytes:1024 required_bytes:1024> name:"test-volume" parameters:<param1:"param1" param2:"param2"> secrets:***stripped*** volume_capabilities:[<AccessType:<Mount:<fs_type:"ext4" mount_flags:["flag1" "flag2" "flag3"]>> access_mode:<mode:5>>] volume_content_source:<Type:<nil>>>`},
+		{createVolume, `<accessibility_requirements:<requisite:[<segments:<foo:"bar" x:"y">> <segments:<a:"b">>]> capacity_range:<required_bytes:1024> name:"foo" secrets:***stripped*** volume_capabilities:[<AccessType:<Mount:<fs_type:"ext4">>>]>`},
+		{createVolumeCSI03, `<accessibility_requirements:<requisite:[<segments:<foo:"bar" x:"y">> <segments:<a:"b">>]> capacity_range:<required_bytes:1024> controller_create_secrets:***stripped*** name:"foo" volume_capabilities:[<AccessType:<Mount:<fs_type:"ext4">>>]>`},
 		{createVolumeFuture,
-			// Secrets are *not* removed from all fields yet. This will have to be fixed one way or another
-			// before the CSI spec can start using secrets there (currently it doesn't).
-			// The test is still useful because it shows that also complicated fields get serialized.
-			// `{"capacity_range":{"required_bytes":1024},"maybe_secret_map":{"1":{"AccessType":null,"array_secret":"***stripped***"},"2":{"AccessType":null,"array_secret":"***stripped***"}},"name":"foo","new_secret_int":"***stripped***","seecreets":"***stripped***","volume_capabilities":[{"AccessType":{"Mount":{"fs_type":"ext4"}},"array_secret":"***stripped***"},{"AccessType":null,"array_secret":"***stripped***"}],"volume_content_source":{"Type":{"Volume":{"oneof_secret_field":"***stripped***","volume_id":"abc"}},"nested_secret_field":"***stripped***"}}`,
-			`{"capacity_range":{"required_bytes":1024},"maybe_secret_map":{"1":{"AccessType":null,"array_secret":"aaa"},"2":{"AccessType":null,"array_secret":"bbb"}},"name":"foo","new_secret_int":"***stripped***","seecreets":"***stripped***","volume_capabilities":[{"AccessType":{"Mount":{"fs_type":"ext4"}},"array_secret":"***stripped***"},{"AccessType":null,"array_secret":"***stripped***"}],"volume_content_source":{"Type":{"Volume":{"oneof_secret_field":"hello","volume_id":"abc"}},"nested_secret_field":"***stripped***"}}`,
-		},
+			`<capacity_range:<required_bytes:1024> maybe_secret_map:<"1":<AccessType:<nil> array_secret:***stripped***> "2":<AccessType:<nil> array_secret:***stripped***>> name:"foo" new_secret_int:***stripped*** seecreets:***stripped*** volume_capabilities:[<AccessType:<Mount:<fs_type:"ext4">> array_secret:***stripped***> <AccessType:<nil> array_secret:***stripped***>] volume_content_source:<Type:<Volume:<oneof_secret_field:***stripped*** volume_id:"abc">> nested_secret_field:***stripped***>>`},
 	}
 
 	// Message from revised spec as received by a sidecar based on the current spec.
@@ -197,7 +191,7 @@ func TestStripSecrets(t *testing.T) {
 	if assert.NoError(t, err, "marshall future message") &&
 		assert.NoError(t, proto.Unmarshal(data, unknownFields), "unmarshal with unknown fields") {
 		cases = append(cases, testcase{unknownFields,
-			`{"capacity_range":{"required_bytes":1024},"name":"foo","secrets":"***stripped***","volume_capabilities":[{"AccessType":{"Mount":{"fs_type":"ext4"}}},{"AccessType":null}],"volume_content_source":{"Type":{"Volume":{"volume_id":"abc"}}}}`,
+			`<capacity_range:<required_bytes:1024> name:"foo" secrets:***stripped*** volume_capabilities:[<AccessType:<Mount:<fs_type:"ext4">>> <AccessType:<nil>>] volume_content_source:<Type:<Volume:<volume_id:"abc">>>>`,
 		})
 	}