G1: Add proposal for true binary metadata #19

ctiller · 2017-03-29T17:06:17Z

Please direct discussion here: https://groups.google.com/forum/#!topic/grpc-io/Ww8waYz_Nes

ejona86 · 2017-03-29T17:17:17Z

G1-true-binary-metadata.md

+
+When transmitting metadata on a connection where the peer has specified
+GRPC_ALLOW_TRUE_BINARY_METADATA, instead of encoding using base64, an
+implementation MAY instead prefix a NUL byte to the metadata and transmit the


s/metadata/metadata value/. Or maybe "header field value".

ejona86 · 2017-03-29T17:23:00Z

G1-true-binary-metadata.md

+
+Since this is a HTTP2 extension and other extensions might alias this extension
+id, it's possible that this becomes misconfigured. In that case, peers are
+required to RST_STREAM with http error PROTOCOL_ERROR. If a binary encoding was


Maybe call out that this is the normal behavior as specified in RFC7540§10.3?

ejona86 · 2017-03-29T17:23:47Z

G1-true-binary-metadata.md

+Suppose we wanted to send metadata element 'foo-bin: 0x01' (ie a single byte
+containing '1').
+
+Under base64, we'd send a http header 'foo-bin: AQ'


Add period at end of sentence, or an additional newline.

ejona86 · 2017-03-29T17:27:47Z

G1-true-binary-metadata.md

@@ -0,0 +1,89 @@
+Title


Put in the real title.

markdroth · 2017-03-29T17:58:17Z

G1-true-binary-metadata.md

+GRPC_ALLOW_TRUE_BINARY_METADATA = 0xfe03.
+
+This setting is randomly chosen (to avoid conflicts with other extensions), and
+within the experimental range of HTTP extensions.


Suggest adding a link to https://tools.ietf.org/html/rfc7540#section-11.3 here to refer readers to information about the experimental range.

PiotrSikora · 2017-04-07T22:55:24Z

G1-true-binary-metadata.md

+### New setting
+
+Expose a custom setting in our HTTP2 settings exchange:
+GRPC_ALLOW_TRUE_BINARY_METADATA = 0xfe03.


This doesn't seem gRPC-specific enough to justify GRPC_ prefix.

That it only applies to -bin headers (which are a gRPC thing) makes it gRPC-specific enough (IMO)

Why only -bin headers? If we're using 0x00 magic prefix, then I don't see a reason for this limitation.

So gRPC uses -bin to specify that a header is binary, and says that other metadata must be ascii (per http2). That -bin has semantic meaning (foo-bin is a different header than foo).

We use that as a signal to base64 encode things right now. Under this proposal we'd just switch to an alternate encoding.

Since we can't universally make all of the HTTP2 ecosystem take binary metadata, a mechanism to allow transmitting anything and dropping the -bin isn't possible for us (we need to keep interoperating - so proxies that support this would potentially need to do the base64 conversion if one side didn't support this mechanism).

If there were an extension that allowed sending binary values in metadata, we'd still end up needing a disambiguating byte for backwards compatibility (for the gRPC -bin suffixed headers), and we'd still maintain the requirement that non -bin headers must be ascii.

Are you aware of any HTTP/2 proxies that would reject requests because of "true binary" encoding of the header field? Unless it's a well-known header that must be processed by the proxy, most (all?) will just pass-through headers without ever looking at the content of the header fields.

Don't get me wrong, I'm all in for allowing binary header fields, but in order get this accepted into HTTP/2 ecosystem, this proposal should be a little more than gRPC implementation detail, that's why -bin suffix feels like an artificial limitation.

I believe that standardizing on "0x00 prefix of the header field indicates binary/raw representation" is much more reasonable, and doesn't prevent gRPC from only using it in headers with -bin suffix.

To answer this myself, RFC7540, sec. 10.3. Intermediary Encapsulation Attacks says:

Similarly, HTTP/2 allows header field values that are not valid.
While most of the values that can be encoded will not alter header
field parsing, carriage return (CR, ASCII 0xd), line feed (LF, ASCII
0xa), and the zero character (NUL, ASCII 0x0) might be exploited by
an attacker if they are translated verbatim. Any request or response
that contains a character not permitted in a header field value MUST
be treated as malformed (Section 8.1.2.6). Valid characters are
defined by the "field-content" ABNF rule in Section 3.2 of [RFC7230].

I also did some digging on the http-wg mailing list and it seems that sending binary blobs in header fields was discussed during HTTP/2 standardization, but they eventually decided against it, because apparently base64 in HPACK is "good enough".

PiotrSikora · 2017-04-07T22:59:33Z

G1-true-binary-metadata.md

+containing '1').
+
+Under base64, we'd send a http header 'foo-bin: AQ'.
+Under binary, we'd send 'foo-bin: 0x00 0x01' (ie prefixing a NUL byte and then


Why do we need NUL-byte here? Why can't we send binary header as foo-bin: 0x01?

I actually spent some time on Wednesday looking into this (for different reasons), and I couldn't find anything in the spec that forbids binary header values.

This was a limitation in SPDY, which used NUL-byte as a separator between multiple header values, but it seems that this "feature" was removed from HPACK.

https://tools.ietf.org/html/rfc7540#section-8.1 says that https://tools.ietf.org/html/rfc7230#section-3.2 is normative, which does not allow binary headers.

...which means that foo-bin: 0x00 0x01 is also illegal.

My point is that nothing in HPACK prevents it from transmitting binary data, so we could add generic SETTINGS parameter for it (for example: SETTING_ALLOW_BINARY_HEADER_FIELD) and carry it over HPACK as any other value.

But I guess 0x00 prefix is here to differentiate binary from text? If so, do we really need -bin prefix in header name?

-bin is not going away so long as there are clients, servers, and intermediaries that do not support binary headers.

PiotrSikora · 2017-04-07T23:30:03Z

G1-true-binary-metadata.md

+containing '1').
+
+Under base64, we'd send a http header 'foo-bin: AQ'.
+Under binary, we'd send 'foo-bin: 0x00 0x01' (ie prefixing a NUL byte and then


...which means that foo-bin: 0x00 0x01 is also illegal.

My point is that nothing in HPACK prevents it from transmitting binary data, so we could add generic SETTINGS parameter for it (for example: SETTING_ALLOW_BINARY_HEADER_FIELD) and carry it over HPACK as any other value.

But I guess 0x00 prefix is here to differentiate binary from text? If so, do we really need -bin prefix in header name?

PiotrSikora · 2017-04-07T23:30:55Z

G1-true-binary-metadata.md

+### New setting
+
+Expose a custom setting in our HTTP2 settings exchange:
+GRPC_ALLOW_TRUE_BINARY_METADATA = 0xfe03.


Why only -bin headers? If we're using 0x00 magic prefix, then I don't see a reason for this limitation.

PiotrSikora · 2017-04-07T23:35:00Z

G1-true-binary-metadata.md

+peers MAY use a 'true binary' encoding (described below), instead of the current
+base64 encoding for -bin metadata.
+
+Implementations SHOULD transmit this setting only once, and as part of the first


Since SETTINGS apply only to a connection and not end-to-end, what about intermediaries? Are proxies supposed to re-encode header field to base64 if they received binary header field but are talking to a backend that doesn't support it?

That's right.

... or they can just not advertise support for this feature.

PiotrSikora · 2017-04-08T02:59:02Z

Per PROTOCOL-HTTP2.md:

Duplicate header names may have their values joined with "," as the delimiter and be considered semantically equivalent. Implementations must split Binary-Headers on "," before decoding the Base64-encoded values.

However, this proposal doesn't cover this case, and there is no way for implementations to detect whether , is a delimiter or just happens to be a part of binary blob.

PiotrSikora · 2017-04-09T02:55:48Z

G1-true-binary-metadata.md

+On receipt, we transparently reverse the transformation.
+
+This transformation is costly in terms of CPU, and there exist use cases where
+this transformation can become the CPU bottleneck for gRPC.


Could you add some data here (i.e. metadata to payload ratio, metadata size, how much CPU does base64 encoding, base64 decoding, Huffman encoding and Huffman decoding each take)? It makes a lot of difference whether we're talking about 2% or 50% of total CPU processing time

ejona86 · 2017-04-10T15:53:57Z

@ctiller, let's prohibits concatenating raw-binary-encoded headers that is normally possible per RFC7230§3.2.2?

ctiller · 2017-04-10T15:56:55Z

That SGTM.

markdroth · 2017-08-25T16:29:53Z

Should we merge this? It's already implemented in C-core, and there's been more than enough time for discussion.

Add proposal for true binary metadata

6011282

ctiller assigned ejona86 and a11r Mar 29, 2017

googlebot added the cla: yes label Mar 29, 2017

Add discussion link

4cb902c

ejona86 approved these changes Mar 29, 2017

View reviewed changes

ejona86 reviewed Mar 29, 2017

View reviewed changes

markdroth approved these changes Mar 29, 2017

View reviewed changes

ctiller changed the title ~~Add proposal for true binary metadata~~ G1: Add proposal for true binary metadata Mar 31, 2017

Review feedback

80ebf33

ctiller mentioned this pull request Apr 4, 2017

True Binary Encoding grpc/grpc#10473

Merged

PiotrSikora reviewed Apr 7, 2017

View reviewed changes

PiotrSikora reviewed Apr 9, 2017

View reviewed changes

ejona86 mentioned this pull request May 24, 2017

OkHttp is incompatible with newer C-core binaries grpc/grpc-java#3032

Closed

a11r approved these changes Aug 25, 2017

View reviewed changes

a11r merged commit 4de5b08 into grpc:master Aug 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

G1: Add proposal for true binary metadata #19

G1: Add proposal for true binary metadata #19

ctiller commented Mar 29, 2017 •

edited

Loading

ejona86 Mar 29, 2017

ctiller Apr 4, 2017

ejona86 Mar 29, 2017

ctiller Apr 4, 2017

ejona86 Mar 29, 2017

ctiller Apr 4, 2017

ejona86 Mar 29, 2017

ctiller Apr 4, 2017

markdroth Mar 29, 2017

ctiller Apr 4, 2017

PiotrSikora Apr 7, 2017

ctiller Apr 7, 2017

PiotrSikora Apr 7, 2017

ctiller Apr 7, 2017

PiotrSikora Apr 8, 2017

PiotrSikora Apr 9, 2017

PiotrSikora Apr 7, 2017

ctiller Apr 7, 2017

PiotrSikora Apr 7, 2017

ctiller Apr 7, 2017

PiotrSikora Apr 7, 2017

PiotrSikora Apr 7, 2017

PiotrSikora Apr 7, 2017

ctiller Apr 7, 2017

ctiller Apr 7, 2017

PiotrSikora commented Apr 8, 2017

PiotrSikora Apr 9, 2017

ejona86 commented Apr 10, 2017

ctiller commented Apr 10, 2017

markdroth commented Aug 25, 2017

G1: Add proposal for true binary metadata #19

G1: Add proposal for true binary metadata #19

Conversation

ctiller commented Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PiotrSikora commented Apr 8, 2017

Choose a reason for hiding this comment

ejona86 commented Apr 10, 2017

ctiller commented Apr 10, 2017

markdroth commented Aug 25, 2017

ctiller commented Mar 29, 2017 •

edited

Loading