Go does not validate metadata keys and values

[ejona86]

HTTP has restrictions on what characters may be used in headers. For example, newline and control characters are strictly prohibited. In addition, gRPC has additional restrictions and by not enforcing them grpc-go makes it much more likely users will create grpc services that can't be used with the other implementations, because they rely on invalid keys or values.

Enforcing the HTTP restrictions must be done, but it would also make sense to enforce gRPC's metadata restrictions.

https://github.com/grpc/grpc-go/blob/master/metadata/metadata.go#L63
https://github.com/grpc/grpc-go/blob/master/transport/http2_client.go#L334

[kelseyhightower]

@iamqizhao Can I take a shot at this one?

Seems like one or more backwards incompatible changes will be required to the metadata package to enforce gRPC metadata requirements. For example should the metadata.encodeKeyValue function return an error which would then need to propagate through the metadata.New function?

Ideally we could add a new function to validate function to the metadata package:

func Validate(md MD) error

[iamqizhao]

Kelsey,

Sure. Thanks a lot for the help!. I will schedule a meeting with you and let's talk.

[dfawley]

Proposal:

Return a gRPC status error (code: Internal) from grpc.Invoke or ServerStream's SetHeader, SendHeader, and SetTrailer methods before transmitting any additional data if the metadata in the request is malformed.

This avoids changing the API and doesn't require a new validation function that has to be called manually. Note that anyone relying upon sending metadata forbidden by the spec but accepted by the current version of grpc-go would be broken by this change.

[easwars]

@dfawley

Few clarifying questions before I get started on this:

1. On the client side, I need to make sure that we validate the metadata from grpc.Invoke() and ClientConn.NewStream(). Both of these call newClientStream(). Are there any other code paths I need to be aware of?

2. Once newClientStream() creates a csAttempt and calls newStream() on it, the underlying transport's method to create a stream is invoked, http2Client.NewStream(). This calls http2Client.createHeaderFields(), and here I see that some header fields added a metadata directly to the transport are also sent in the HEADERS frame. Do we need to validate these?

3. On the server side, ServerStream.SetHeader ServerStream.SendHeader and ServerStream.SetTrailer are the methods of concern. Are there more?

4. The new method for validating the headers might look like this:

package metadata

func (md MD) Validate() error { ...}

Is this the complete set of checks to be done, or are there more?

• Header names contain one or more characters from this set [0-9 a-z _ - .]
• If the header-name ends with a "-bin" suffix, the header-value could contain an arbitrary octet sequence. So no real validation required here.
• If header-name does not end with a "-bin" suffix, header-value should only contain one or more characters from the set ( %x20-%x7E ) which includes space and printable ASCII.

Thanks.

[gyuho]

@dfawley @menghanl @easwars Any updates? Even adding validate method would be great.

[dfawley]

@easwars do you have any plans to work on this soon? If not, please unassign and add a "help wanted" label.

@gyuho, this is semi-low priority for us right now, since it is only intended to protect users from their own mistakes.