Per RPC callback GetRequestMetadata fails when key contains colon or slash

[ThomasHabets]

 conn, err := grpc.Dial(addr,
                grpc.WithPerRPCCredentials(&perRPC{}),
[…]
type perRPC struct{}

func (*perRPC) RequireTransportSecurity() bool {
        return true
}

func (*perRPC) GetRequestMetadata(ctx context.Context, uri ...string) (map[string]string, error) {
        ret = make(map[string]string)
        ret["http:cookie"] = getCookieFromContext(ctx)
        return ret, nil
}

All RPCs fail when key in the returned may contains slash or colon. This used to work, so is it not supposed to?

Error is unhelpful rpc error: code = 13 desc =

[bufdev]

Well shoot, this might be related to #576

[iamqizhao]

@bradfitz can you check why https://github.com/golang/net/blob/4876518f9e71663000c348837735820161a42df7/http2/http2.go#L348 does not list all ascii? colon and slash should be there.

[bradfitz]

@iamqizhao, it doesn't list all ASCII because it's not a table of ASCII. It's a table for whether it's a valid token, as its name suggests. See:

// validHeaderFieldName reports whether v is a valid header field name (key).
//  RFC 7230 says:
//   header-field   = field-name ":" OWS field-value OWS
//   field-name     = token
//   token          = 1*tchar
//   tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." /
//           "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA
// Further, http2 says:
//   "Just as in HTTP/1.x, header field names are strings of ASCII
//   characters that are compared in a case-insensitive
//   fashion. However, header field names MUST be converted to
//   lowercase prior to their encoding in HTTP/2. "
func validHeaderFieldName(v string) bool {
    if len(v) == 0 {
        return false
    }
    for _, r := range v {
        if int(r) >= len(isTokenTable) || ('A' <= r && r <= 'Z') {
            return false
        }
        if !isTokenTable[byte(r)] {
            return false
        }
    }
    return true
}

[bradfitz]

All RPCs fail when key in the returned may contains slash or colon. This used to work, so is it not supposed to?

A header field name cannot contain a colon.

[iamqizhao]

I see. BTW, this is a google example on checking illegal chars on reading path does not give good error info. Can you add it? We can definitely add the checking in gRPC but it seems it should go into http2 frame code. If you are short of time, we can it to gRPC as a short term solution.

[ThomasHabets]

So the valid keys for GetRequestMetadata are whatever the underlying data transport allows in "headers or other contexts"?

This is not clear from:
https://godoc.org/google.golang.org/grpc/credentials#Credentials

And it implies a leaky abstraction, where key-values break when underlying transport changes, doesn't it?

[bradfitz]

And it implies a leaky abstraction, where key-values break when underlying transport changes, doesn't it?

That interface could definitely use some more docs saying what the keys are allowed to be. And the callers of that interface should verify the docs were obeyed and fail earlier, before it gets to the wire.

[bradfitz]

I see. BTW, this is a google example on checking illegal chars on reading path does not give good error info. Can you add it? We can definitely add the checking in gRPC but it seems it should go into http2 frame code.

It should be already. ErrorDetail would say that.

[iamqizhao]

Sorry for confusion. I meant the checking should be added to the writing path. ErrorDetail is only for reading path.

[bradfitz]

It's a little gross because hpack allows a larger set of things than http2 (which just uses hpack) allows, and a bunch of the code in gRPC and elsewhere is using hpack directly to encode things. I suppose I can make the hpack encoder assume http2 rules by default, unless you opt-in to the full hpack encoding possibilities.

[iamqizhao]

@bradfitz in that case, it seems it makes sense grpc http2 transport enforces the rule by itself instead of relying on http2 framer.

@ThomasHabets I actually agree with you regarding the abstraction. I am going to talk to the team to see how we can improve it.

[bradfitz]

@bradfitz in that case, it seems it makes sense grpc http2 transport enforces the rule by itself instead of relying on http2 framer.

gRPC doesn't look at the error return value from hpack.Encoder.WriteField anyway, so me returning an error for invalid http2 field Name values wouldn't help anyway...

transport/http2_client.go:      // HPACK encodes various headers. Note that once WriteField(...) is
transport/http2_client.go:      t.hEnc.WriteField(hpack.HeaderField{Name: ":method", Value: "POST"})
transport/http2_client.go:      t.hEnc.WriteField(hpack.HeaderField{Name: ":scheme", Value: t.scheme})
transport/http2_client.go:      t.hEnc.WriteField(hpack.HeaderField{Name: ":path", Value: callHdr.Method})
transport/http2_client.go:      t.hEnc.WriteField(hpack.HeaderField{Name: ":authority", Value: callHdr.Host})
transport/http2_client.go:      t.hEnc.WriteField(hpack.HeaderField{Name: "content-type", Value: "application/grpc"})
transport/http2_client.go:      t.hEnc.WriteField(hpack.HeaderField{Name: "user-agent", Value: t.userAgent})
transport/http2_client.go:      t.hEnc.WriteField(hpack.HeaderField{Name: "te", Value: "trailers"})
transport/http2_client.go:              t.hEnc.WriteField(hpack.HeaderField{Name: "grpc-encoding", Value: callHdr.SendCompress})
transport/http2_client.go:              t.hEnc.WriteField(hpack.HeaderField{Name: "grpc-timeout", Value: timeoutEncode(timeout)})
transport/http2_client.go:              t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: v})
transport/http2_client.go:                              t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: entry})
transport/http2_server.go:      t.hEnc.WriteField(hpack.HeaderField{Name: ":status", Value: "200"})
transport/http2_server.go:      t.hEnc.WriteField(hpack.HeaderField{Name: "content-type", Value: "application/grpc"})
transport/http2_server.go:              t.hEnc.WriteField(hpack.HeaderField{Name: "grpc-encoding", Value: s.sendCompress})
transport/http2_server.go:                      t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: entry})
transport/http2_server.go:              t.hEnc.WriteField(hpack.HeaderField{Name: ":status", Value: "200"})
transport/http2_server.go:              t.hEnc.WriteField(hpack.HeaderField{Name: "content-type", Value: "application/grpc"})
transport/http2_server.go:      t.hEnc.WriteField(
transport/http2_server.go:      t.hEnc.WriteField(hpack.HeaderField{Name: "grpc-message", Value: statusDesc})
transport/http2_server.go:                      t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: entry})
transport/http2_server.go:              t.hEnc.WriteField(hpack.HeaderField{Name: ":status", Value: "200"})
transport/http2_server.go:              t.hEnc.WriteField(hpack.HeaderField{Name: "content-type", Value: "application/grpc"})
transport/http2_server.go:                      t.hEnc.WriteField(hpack.HeaderField{Name: "grpc-encoding", Value: s.sendCompress})

[ThomasHabets]

Why shouldn't those errors be checked?

[bradfitz]

@ThomasBets, a *bytes.Buffer is documented as never returning an error on Writes. (It only returns error values to be compatible with the io.Writer etc interfaces). And *hpack.Encoder (so far) just returns errors from its underlying Writer, so it was a safe move to ignore those errors so far.