transport: set and respect HTTP/2 SETTINGS_MAX_HEADER_LIST_SIZE #2084

lyuxuan · 2018-05-16T21:42:16Z

Add DialOption(WithMaxHeaderListSize) and ServerOption(MaxHeaderListSize) to set the max header list size that peer should respect. Configuring these two options results in sending the SETTINGS_MAX_HEADER_LIST_SIZE setting inside the initial settings frame. And at the same time, the framer is configured to omit header fields after peer's header list size hitting the limit.
Note: framer uses the max header list size to configure hpack decoder max string length( link). While a stream exceeding the header list size limit leads to reset stream, exceeding the max string length for hpack decoder will lead to connection close.
Issues:
- 8KB defualt limit will potentially break some users, however, other languages implementation all enforce this limit. Our current implicit setting is 16MB (by golang http2 package), and I am setting that explicitly now.
- As suggested here, https://http2.github.io/http2-spec/#rfc.section.10.5.1, it is better to send a HTTP 431 code rather than a reset stream, as peer will know better about what is happening.
- A potential behavior change for user who sends >16MB metadata: before we return truncated metadata silently, now we will fail the RPC. It's a bug fix.

MakMukhi · 2018-05-29T17:26:24Z

clientconn.go

+// header list that the client is prepared to accept.
+func WithMaxHeaderListSize(s uint32) DialOption {
+	return func(o *dialOptions) {
+		o.copts.MaxHeaderListSize = new(uint32)


How about if we just take address of s, since it's already a copy?

MakMukhi · 2018-05-29T17:26:58Z

server.go

+// of header list that the server is prepared to accept.
+func MaxHeaderListSize(s uint32) ServerOption {
+	return func(o *options) {
+		o.maxHeaderListSize = new(uint32)


Same thing here.

MakMukhi · 2018-05-29T17:29:57Z

test/end2end_test.go

+func TestServerMaxHeaderListSizeClientUserViolation(t *testing.T) {
+	defer leakcheck.Check(t)
+	for _, e := range listTestEnv() {
+		if e.httpHandler == true {


if e.httpHandler {

MakMukhi · 2018-05-29T18:13:07Z

transport/http2_client.go

@@ -41,6 +41,10 @@ import (
 	"google.golang.org/grpc/status"
 )

+const (
+	defaultClientMaxHeaderListSize = uint32(16 << 20)
+)


Should we put all connection-level defaults together? They all live here right now https://github.com/grpc/grpc-go/blob/master/transport/flowcontrol.go#L50 but I don't think some of them belong in a file called flowcontrol.go

Yeah, I was considering whether the default should be inside flowcontol.go, and decided that probably not. How about I go ahead and make a defaults.go to have all the defaults in one place?

MakMukhi · 2018-05-29T19:53:50Z

transport/http2_client.go

+		hdrFrame := it.(*headerFrame)
+		var sz int64
+		for _, f := range hdrFrame.hf {
+			if sz += int64(f.Size()); sz > int64(*t.maxSendHeaderListSize) {


Should we update createHeaderFields to record the total size so that we don't have to loop over all fields again?

It will benefit users that their peers enforce a header list size limit, but will add overhead to the case that peers don't have a limit.

MakMukhi · 2018-05-29T20:03:31Z

transport/http2_server.go

-			}
-			return
+	state := decodeState{serverSide: true}
+	if err := state.decodeResponseHeader(frame); err != nil {


decodeResponseHeader is supposed to be used on the client side. Perhaps, rename the function to something more appropriate since it's being used on the server as well.

Alternatively, add the if frame.Truncated check right at the beginning of this function itself.

Changed to decodeHeader.
Tried to move if frame.Truncated, but it seems to only save an declaration for a decodeState variable, but complicates the code substantially, e.g. add redundant code for error handling, have to specialize for server and client.

MakMukhi · 2018-05-29T20:06:23Z

transport/http_util.go

+	// frame.Truncated is set to true when framer detects that the current header
+	// list size hits MaxHeaderListSize limit.
+	if frame.Truncated {
+		return streamErrorf(codes.Internal, "peer header list size exceeded limit")


Is it better to return an error or merely log it and still try and parse the header that we got?

The header would be incomplete and I am not sure what will happen if we return an incomplete header to grpc and the user.

MakMukhi · 2018-05-29T20:08:01Z

transport/http2_server.go

@@ -714,7 +756,7 @@ func (t *http2Server) WriteHeader(s *Stream, md metadata.MD) error {
 			headerFields = append(headerFields, hpack.HeaderField{Name: k, Value: encodeMetadataHeader(k, v)})
 		}
 	}
-	t.controlBuf.put(&headerFrame{
+	success, err := t.controlBuf.executeAndPut(t.checkForHeaderListSize, &headerFrame{


Should we also do this check while writing trailer?

Yes, I think we should. I was under the wrong impression that WriteHeader is the centralized space for both headers and trailers.

MakMukhi · 2018-05-29T20:10:39Z

transport/http2_server.go

+	if t.maxSendHeaderListSize == nil {
+		return true
+	}
+	hdrFrame := it.(*headerFrame)


Same thing; perhaps the headerFrame should have a field to record size.

Adding a size field would add overhead to non limit case.

MakMukhi · 2018-05-29T20:18:22Z

transport/http2_server.go

+	if config.MaxHeaderListSize != nil {
+		maxHeaderListSize = *config.MaxHeaderListSize
+	}
+	framer := newFramer(conn, writeBufSize, readBufSize, maxHeaderListSize)


Only relevant to the server side: Should Framer.MaxHeaderListSize be set only after fist settings' ack is received?
The other side might start with an unlimited MaxHeaderListSize(https://tools.ietf.org/html/rfc7540#section-6.5.2) and start sending header frames before our settings frame is received by it.

Yes, I would like to do that in a separate PR, as things like max concurrent streams may also need this behavior.

Offline discussion decides that at current stage, enforcement after ack will not be necessary and add complexity to the code.

MakMukhi · 2018-06-22T23:16:18Z

transport/controlbuf.go

+		c.mu.Unlock()
+		return false, c.err
+	}
+	if f != nil {


executeAndPut has this check because put calls it with a nil argument.

lyuxuan force-pushed the header_list_size branch from efaa8cc to a385c2a Compare May 16, 2018 21:43

menghanl assigned MakMukhi May 17, 2018

dfawley requested a review from MakMukhi May 24, 2018 20:54

MakMukhi suggested changes May 29, 2018

View reviewed changes

MakMukhi assigned lyuxuan and unassigned MakMukhi May 29, 2018

lyuxuan force-pushed the header_list_size branch from a385c2a to aa9cd7b Compare June 1, 2018 00:29

lyuxuan force-pushed the header_list_size branch from 91aa1ba to c7e89b1 Compare June 21, 2018 17:36

lyuxuan assigned MakMukhi and unassigned lyuxuan Jun 21, 2018

lyuxuan force-pushed the header_list_size branch from c7e89b1 to d2a7132 Compare June 22, 2018 21:37

MakMukhi approved these changes Jun 22, 2018

View reviewed changes

dfawley assigned lyuxuan and unassigned lyuxuan and MakMukhi Jun 28, 2018

lyuxuan force-pushed the header_list_size branch 3 times, most recently from bd73ad3 to 6eb6e04 Compare July 2, 2018 20:55

lyuxuan added 5 commits July 3, 2018 16:13

Set and respect HTTP/2 SETTINGS_MAX_HEADER_LIST_SIZE

8223ab1

use let framer handle max header list size

2804e69

update

24f9798

fix Mak's review

c1fc17c

minor: delete check for f != nil

f2ab724

lyuxuan force-pushed the header_list_size branch from 6eb6e04 to f2ab724 Compare July 3, 2018 23:14

lyuxuan merged commit 264daa2 into grpc:master Jul 9, 2018

dfawley changed the title ~~Set and respect HTTP/2 SETTINGS_MAX_HEADER_LIST_SIZE~~ transport: set and respect HTTP/2 SETTINGS_MAX_HEADER_LIST_SIZE Jul 12, 2018

dfawley added the Type: Feature New features or improvements in behavior label Jul 12, 2018

dfawley added this to the 1.14 Release milestone Jul 12, 2018

lock bot locked as resolved and limited conversation to collaborators Jan 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transport: set and respect HTTP/2 SETTINGS_MAX_HEADER_LIST_SIZE #2084

transport: set and respect HTTP/2 SETTINGS_MAX_HEADER_LIST_SIZE #2084

lyuxuan commented May 16, 2018 •

edited

Loading

MakMukhi May 29, 2018

lyuxuan May 31, 2018

MakMukhi May 29, 2018

lyuxuan May 31, 2018

MakMukhi May 29, 2018

lyuxuan May 31, 2018

MakMukhi May 29, 2018

lyuxuan May 31, 2018

MakMukhi May 29, 2018

lyuxuan May 31, 2018

MakMukhi May 29, 2018

MakMukhi May 29, 2018

lyuxuan Jun 1, 2018

MakMukhi May 29, 2018

lyuxuan May 31, 2018

MakMukhi May 29, 2018

lyuxuan May 31, 2018

MakMukhi May 29, 2018

lyuxuan May 31, 2018

MakMukhi May 29, 2018

lyuxuan May 31, 2018

lyuxuan Jun 21, 2018

MakMukhi Jun 22, 2018

transport: set and respect HTTP/2 SETTINGS_MAX_HEADER_LIST_SIZE #2084

transport: set and respect HTTP/2 SETTINGS_MAX_HEADER_LIST_SIZE #2084

Conversation

lyuxuan commented May 16, 2018 • 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

lyuxuan commented May 16, 2018 •

edited

Loading