Never encode binary metadata within the metadata map (#1188)

This change ensures consistency for the user when accessing metadata values:
they are never encoded except when sent on the wire.  Previously, they would
appear encoded to client code, but not to server code.  As such, this
represents a behavior change, but one unlikely to affect user code, as it's
unusual to inspect the metadata after setting it.

Author: dfawley

metadata/metadata.go

@@ -36,79 +36,47 @@
 package metadata // import "google.golang.org/grpc/metadata"
 
 import (
-	"encoding/base64"
 	"fmt"
 	"strings"
 
 	"golang.org/x/net/context"
 )
 
-const (
-	binHdrSuffix = "-bin"
-)
-
-// encodeKeyValue encodes key and value qualified for transmission via gRPC.
-// Transmitting binary headers violates HTTP/2 spec.
-// TODO(zhaoq): Maybe check if k is ASCII also.
-func encodeKeyValue(k, v string) (string, string) {
-	k = strings.ToLower(k)
-	if strings.HasSuffix(k, binHdrSuffix) {
-		val := base64.StdEncoding.EncodeToString([]byte(v))
-		v = string(val)
-	}
-	return k, v
-}
-
-// DecodeKeyValue returns the original key and value corresponding to the
-// encoded data in k, v.
-// If k is a binary header and v contains comma, v is split on comma before decoded,
-// and the decoded v will be joined with comma before returned.
+// DecodeKeyValue returns k, v, nil.  It is deprecated and should not be used.
 func DecodeKeyValue(k, v string) (string, string, error) {
-	if !strings.HasSuffix(k, binHdrSuffix) {
-		return k, v, nil
-	}
-	vvs := strings.Split(v, ",")
-	for i, vv := range vvs {
-		val, err := base64.StdEncoding.DecodeString(vv)
-		if err != nil {
-			return "", "", err
-		}
-		vvs[i] = string(val)
-	}
-	return k, strings.Join(vvs, ","), nil
+	return k, v, nil
 }
 
 // MD is a mapping from metadata keys to values. Users should use the following
 // two convenience functions New and Pairs to generate MD.
 type MD map[string][]string
 
 // New creates a MD from given key-value map.
-// Keys are automatically converted to lowercase. And for keys having "-bin" as suffix, their values will be applied Base64 encoding.
+// Keys are automatically converted to lowercase.
 func New(m map[string]string) MD {
 	md := MD{}
-	for k, v := range m {
-		key, val := encodeKeyValue(k, v)
+	for k, val := range m {
+		key := strings.ToLower(k)
 		md[key] = append(md[key], val)
 	}
 	return md
 }
 
 // Pairs returns an MD formed by the mapping of key, value ...
 // Pairs panics if len(kv) is odd.
-// Keys are automatically converted to lowercase. And for keys having "-bin" as suffix, their values will be appplied Base64 encoding.
+// Keys are automatically converted to lowercase.
 func Pairs(kv ...string) MD {
 	if len(kv)%2 == 1 {
 		panic(fmt.Sprintf("metadata: Pairs got the odd number of input pairs for metadata: %d", len(kv)))
 	}
 	md := MD{}
-	var k string
+	var key string
 	for i, s := range kv {
 		if i%2 == 0 {
-			k = s
+			key = strings.ToLower(s)
 			continue
 		}
-		key, val := encodeKeyValue(k, s)
-		md[key] = append(md[key], val)
+		md[key] = append(md[key], s)
 	}
 	return md
 }

metadata/metadata_test.go

@@ -38,73 +38,20 @@ import (
 	"testing"
 )
 
-const binaryValue = string(128)
-
-func TestEncodeKeyValue(t *testing.T) {
-	for _, test := range []struct {
-		// input
-		kin string
-		vin string
-		// output
-		kout string
-		vout string
-	}{
-		{"key", "abc", "key", "abc"},
-		{"KEY", "abc", "key", "abc"},
-		{"key-bin", "abc", "key-bin", "YWJj"},
-		{"key-bin", binaryValue, "key-bin", "woA="},
-	} {
-		k, v := encodeKeyValue(test.kin, test.vin)
-		if k != test.kout || !reflect.DeepEqual(v, test.vout) {
-			t.Fatalf("encodeKeyValue(%q, %q) = %q, %q, want %q, %q", test.kin, test.vin, k, v, test.kout, test.vout)
-		}
-	}
-}
-
-func TestDecodeKeyValue(t *testing.T) {
-	for _, test := range []struct {
-		// input
-		kin string
-		vin string
-		// output
-		kout string
-		vout string
-		err  error
-	}{
-		{"a", "abc", "a", "abc", nil},
-		{"key-bin", "Zm9vAGJhcg==", "key-bin", "foo\x00bar", nil},
-		{"key-bin", "woA=", "key-bin", binaryValue, nil},
-		{"a", "abc,efg", "a", "abc,efg", nil},
-		{"key-bin", "Zm9vAGJhcg==,Zm9vAGJhcg==", "key-bin", "foo\x00bar,foo\x00bar", nil},
-	} {
-		k, v, err := DecodeKeyValue(test.kin, test.vin)
-		if k != test.kout || !reflect.DeepEqual(v, test.vout) || !reflect.DeepEqual(err, test.err) {
-			t.Fatalf("DecodeKeyValue(%q, %q) = %q, %q, %v, want %q, %q, %v", test.kin, test.vin, k, v, err, test.kout, test.vout, test.err)
-		}
-	}
-}
-
 func TestPairsMD(t *testing.T) {
 	for _, test := range []struct {
 		// input
 		kv []string
 		// output
-		md   MD
-		size int
+		md MD
 	}{
-		{[]string{}, MD{}, 0},
-		{[]string{"k1", "v1", "k2-bin", binaryValue}, New(map[string]string{
-			"k1":     "v1",
-			"k2-bin": binaryValue,
-		}), 2},
+		{[]string{}, MD{}},
+		{[]string{"k1", "v1", "k1", "v2"}, MD{"k1": []string{"v1", "v2"}}},
 	} {
 		md := Pairs(test.kv...)
 		if !reflect.DeepEqual(md, test.md) {
 			t.Fatalf("Pairs(%v) = %v, want %v", test.kv, md, test.md)
 		}
-		if md.Len() != test.size {
-			t.Fatalf("Pairs(%v) generates md of size %d, want %d", test.kv, md.Len(), test.size)
-		}
 	}
 }
 

test/end2end_test.go

@@ -75,17 +75,19 @@ import (
 var (
 	// For headers:
 	testMetadata = metadata.MD{
-		"key1": []string{"value1"},
-		"key2": []string{"value2"},
+		"key1":     []string{"value1"},
+		"key2":     []string{"value2"},
+		"key3-bin": []string{"binvalue1", string([]byte{1, 2, 3})},
 	}
 	testMetadata2 = metadata.MD{
 		"key1": []string{"value12"},
 		"key2": []string{"value22"},
 	}
 	// For trailers:
 	testTrailerMetadata = metadata.MD{
-		"tkey1": []string{"trailerValue1"},
-		"tkey2": []string{"trailerValue2"},
+		"tkey1":     []string{"trailerValue1"},
+		"tkey2":     []string{"trailerValue2"},
+		"tkey3-bin": []string{"trailerbinvalue1", string([]byte{3, 2, 1})},
 	}
 	testTrailerMetadata2 = metadata.MD{
 		"tkey1": []string{"trailerValue12"},

transport/handler_server.go

@@ -111,6 +111,10 @@ func NewServerHandlerTransport(w http.ResponseWriter, r *http.Request) (ServerTr
 					v = v[:i]
 				}
 			}
+			v, err := decodeMetadataHeader(k, v)
+			if err != nil {
+				return nil, streamErrorf(codes.InvalidArgument, "malformed binary metadata: %v", err)
+			}
 			metakv = append(metakv, k, v)
 		}
 	}
@@ -207,10 +211,9 @@ func (ht *serverHandlerTransport) WriteStatus(s *Stream, st *status.Status) erro
 					continue
 				}
 				for _, v := range vv {
-					// http2 ResponseWriter mechanism to
-					// send undeclared Trailers after the
-					// headers have possibly been written.
-					h.Add(http2.TrailerPrefix+k, v)
+					// http2 ResponseWriter mechanism to send undeclared Trailers after
+					// the headers have possibly been written.
+					h.Add(http2.TrailerPrefix+k, encodeMetadataHeader(k, v))
 				}
 			}
 		}
@@ -265,6 +268,7 @@ func (ht *serverHandlerTransport) WriteHeader(s *Stream, md metadata.MD) error {
 				continue
 			}
 			for _, v := range vv {
+				v = encodeMetadataHeader(k, v)
 				h.Add(k, v)
 			}
 		}

transport/http2_client.go

@@ -437,23 +437,23 @@ func (t *http2Client) NewStream(ctx context.Context, callHdr *CallHdr) (_ *Strea
 	)
 	if md, ok := metadata.FromOutgoingContext(ctx); ok {
 		hasMD = true
-		for k, v := range md {
+		for k, vv := range md {
 			// HTTP doesn't allow you to set pseudoheaders after non pseudoheaders were set.
 			if isReservedHeader(k) {
 				continue
 			}
-			for _, entry := range v {
-				t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: entry})
+			for _, v := range vv {
+				t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: encodeMetadataHeader(k, v)})
 			}
 		}
 	}
 	if md, ok := t.md.(*metadata.MD); ok {
-		for k, v := range *md {
+		for k, vv := range *md {
 			if isReservedHeader(k) {
 				continue
 			}
-			for _, entry := range v {
-				t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: entry})
+			for _, v := range vv {
+				t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: encodeMetadataHeader(k, v)})
 			}
 		}
 	}

transport/http2_server.go

@@ -644,13 +644,13 @@ func (t *http2Server) WriteHeader(s *Stream, md metadata.MD) error {
 	if s.sendCompress != "" {
 		t.hEnc.WriteField(hpack.HeaderField{Name: "grpc-encoding", Value: s.sendCompress})
 	}
-	for k, v := range md {
+	for k, vv := range md {
 		if isReservedHeader(k) {
 			// Clients don't tolerate reading restricted headers after some non restricted ones were sent.
 			continue
 		}
-		for _, entry := range v {
-			t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: entry})
+		for _, v := range vv {
+			t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: encodeMetadataHeader(k, v)})
 		}
 	}
 	bufLen := t.hBuf.Len()
@@ -713,21 +713,17 @@ func (t *http2Server) WriteStatus(s *Stream, st *status.Status) error {
 			panic(err)
 		}
 
-		for k, v := range metadata.New(map[string]string{"grpc-status-details-bin": (string)(stBytes)}) {
-			for _, entry := range v {
-				t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: entry})
-			}
-		}
+		t.hEnc.WriteField(hpack.HeaderField{Name: "grpc-status-details-bin", Value: encodeBinHeader(stBytes)})
 	}
 
 	// Attach the trailer metadata.
-	for k, v := range s.trailer {
+	for k, vv := range s.trailer {
 		// Clients don't tolerate reading restricted headers after some non restricted ones were sent.
 		if isReservedHeader(k) {
 			continue
 		}
-		for _, entry := range v {
-			t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: entry})
+		for _, v := range vv {
+			t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: encodeMetadataHeader(k, v)})
 		}
 	}
 	bufLen := t.hBuf.Len()

transport/http_util.go

@@ -36,6 +36,7 @@ package transport
 import (
 	"bufio"
 	"bytes"
+	"encoding/base64"
 	"fmt"
 	"io"
 	"net"
@@ -50,7 +51,6 @@ import (
 	spb "google.golang.org/genproto/googleapis/rpc/status"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/grpclog"
-	"google.golang.org/grpc/metadata"
 	"google.golang.org/grpc/status"
 )
 
@@ -164,6 +164,31 @@ func (d *decodeState) status() *status.Status {
 	return d.statusGen
 }
 
+const binHdrSuffix = "-bin"
+
+func encodeBinHeader(v []byte) string {
+	return base64.StdEncoding.EncodeToString(v)
+}
+
+func decodeBinHeader(v string) ([]byte, error) {
+	return base64.StdEncoding.DecodeString(v)
+}
+
+func encodeMetadataHeader(k, v string) string {
+	if strings.HasSuffix(k, binHdrSuffix) {
+		return encodeBinHeader(([]byte)(v))
+	}
+	return v
+}
+
+func decodeMetadataHeader(k, v string) (string, error) {
+	if strings.HasSuffix(k, binHdrSuffix) {
+		b, err := decodeBinHeader(v)
+		return string(b), err
+	}
+	return v, nil
+}
+
 func (d *decodeState) processHeaderField(f hpack.HeaderField) error {
 	switch f.Name {
 	case "content-type":
@@ -181,12 +206,12 @@ func (d *decodeState) processHeaderField(f hpack.HeaderField) error {
 	case "grpc-message":
 		d.rawStatusMsg = decodeGrpcMessage(f.Value)
 	case "grpc-status-details-bin":
-		_, v, err := metadata.DecodeKeyValue("grpc-status-details-bin", f.Value)
+		v, err := decodeBinHeader(f.Value)
 		if err != nil {
 			return streamErrorf(codes.Internal, "transport: malformed grpc-status-details-bin: %v", err)
 		}
 		s := &spb.Status{}
-		if err := proto.Unmarshal([]byte(v), s); err != nil {
+		if err := proto.Unmarshal(v, s); err != nil {
 			return streamErrorf(codes.Internal, "transport: malformed grpc-status-details-bin: %v", err)
 		}
 		d.statusGen = status.FromProto(s)
@@ -203,12 +228,12 @@ func (d *decodeState) processHeaderField(f hpack.HeaderField) error {
 			if d.mdata == nil {
 				d.mdata = make(map[string][]string)
 			}
-			k, v, err := metadata.DecodeKeyValue(f.Name, f.Value)
+			v, err := decodeMetadataHeader(f.Name, f.Value)
 			if err != nil {
 				grpclog.Printf("Failed to decode (%q, %q): %v", f.Name, f.Value, err)
 				return nil
 			}
-			d.mdata[k] = append(d.mdata[k], v)
+			d.mdata[f.Name] = append(d.mdata[f.Name], v)
 		}
 	}
 	return nil