From b610ffd3f8786563002bd5b6a9951d56f64a4921 Mon Sep 17 00:00:00 2001 From: dfawley Date: Fri, 28 Apr 2017 10:56:58 -0700 Subject: [PATCH] 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. --- metadata/metadata.go | 50 ++++++------------------------- metadata/metadata_test.go | 59 ++----------------------------------- test/end2end_test.go | 10 ++++--- transport/handler_server.go | 12 +++++--- transport/http2_client.go | 12 ++++---- transport/http2_server.go | 18 +++++------ transport/http_util.go | 35 ++++++++++++++++++---- transport/http_util_test.go | 44 +++++++++++++++++++++++++++ 8 files changed, 113 insertions(+), 127 deletions(-) diff --git a/metadata/metadata.go b/metadata/metadata.go index 7ca44182612f..5395ca8200d6 100644 --- a/metadata/metadata.go +++ b/metadata/metadata.go @@ -36,46 +36,15 @@ 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 @@ -83,11 +52,11 @@ func DecodeKeyValue(k, v string) (string, string, error) { 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 @@ -95,20 +64,19 @@ func New(m map[string]string) 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 } diff --git a/metadata/metadata_test.go b/metadata/metadata_test.go index fef0a0f815d0..bd982ccd888f 100644 --- a/metadata/metadata_test.go +++ b/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) - } } } diff --git a/test/end2end_test.go b/test/end2end_test.go index fd77cd7c57ba..6a36b6882e93 100644 --- a/test/end2end_test.go +++ b/test/end2end_test.go @@ -75,8 +75,9 @@ 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"}, @@ -84,8 +85,9 @@ var ( } // 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"}, diff --git a/transport/handler_server.go b/transport/handler_server.go index 28c9ce03658a..24f306babbb1 100644 --- a/transport/handler_server.go +++ b/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) } } diff --git a/transport/http2_client.go b/transport/http2_client.go index 67a79459c0a0..380fff665fbb 100644 --- a/transport/http2_client.go +++ b/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)}) } } } diff --git a/transport/http2_server.go b/transport/http2_server.go index 31fefc7bb7cc..14cd19c64c6e 100644 --- a/transport/http2_server.go +++ b/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() diff --git a/transport/http_util.go b/transport/http_util.go index 89c1525d2376..e5120ebf0079 100644 --- a/transport/http_util.go +++ b/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 diff --git a/transport/http_util_test.go b/transport/http_util_test.go index a5f8a85f7f48..8e2c488d8717 100644 --- a/transport/http_util_test.go +++ b/transport/http_util_test.go @@ -35,6 +35,7 @@ package transport import ( "fmt" + "reflect" "testing" "time" ) @@ -143,3 +144,46 @@ func TestDecodeGrpcMessage(t *testing.T) { } } } + +const binaryValue = string(128) + +func TestEncodeMetadataHeader(t *testing.T) { + for _, test := range []struct { + // input + kin string + vin string + // output + vout string + }{ + {"key", "abc", "abc"}, + {"KEY", "abc", "abc"}, + {"key-bin", "abc", "YWJj"}, + {"key-bin", binaryValue, "woA="}, + } { + v := encodeMetadataHeader(test.kin, test.vin) + if !reflect.DeepEqual(v, test.vout) { + t.Fatalf("encodeMetadataHeader(%q, %q) = %q, want %q", test.kin, test.vin, v, test.vout) + } + } +} + +func TestDecodeMetadataHeader(t *testing.T) { + for _, test := range []struct { + // input + kin string + vin string + // output + vout string + err error + }{ + {"a", "abc", "abc", nil}, + {"key-bin", "Zm9vAGJhcg==", "foo\x00bar", nil}, + {"key-bin", "woA=", binaryValue, nil}, + {"a", "abc,efg", "abc,efg", nil}, + } { + v, err := decodeMetadataHeader(test.kin, test.vin) + if !reflect.DeepEqual(v, test.vout) || !reflect.DeepEqual(err, test.err) { + t.Fatalf("decodeMetadataHeader(%q, %q) = %q, %v, want %q, %v", test.kin, test.vin, v, err, test.vout, test.err) + } + } +}