http2: mix cleanups, TODOs, new tests, enforce header list size in Transport

Just re-reading the code and polishing things up.

Nothing major. Enforcing the max header list size in the Transport and
letting it be configurable (not exposed in Go 1.6, only via x/net) is
the most notable. Mostly just tests.

Change-Id: Iacbad5a0b1ba0df9296b1aecfbc8b9b83323d435
Reviewed-on: https://go-review.googlesource.com/18367
Reviewed-by: Blake Mizerany <blake.mizerany@gmail.com>

http2/server.go

@@ -6,8 +6,8 @@
 // instead, and make sure that on close we close all open
 // streams. then remove doneServing?
 
-// TODO: finish GOAWAY support. Consider each incoming frame type and
-// whether it should be ignored during a shutdown race.
+// TODO: re-audit GOAWAY support. Consider each incoming frame type and
+// whether it should be ignored during graceful shutdown.
 
 // TODO: disconnect idle clients. GFE seems to do 4 minutes. make
 // configurable?  or maximum number of idle clients and remove the
@@ -550,14 +550,10 @@ func (st *stream) onNewTrailerField(f hpack.HeaderField) {
 	sc.vlogf("got trailer field %+v", f)
 	switch {
 	case !validHeader(f.Name):
-		// TODO: change hpack signature so this can return
-		// errors?  Or stash an error somewhere on st or sc
-		// for processHeaderBlockFragment etc to pick up and
-		// return after the hpack Write/Close.  For now just
-		// ignore.
+		sc.req.invalidHeader = true
 		return
 	case strings.HasPrefix(f.Name, ":"):
-		// TODO: same TODO as above.
+		sc.req.invalidHeader = true
 		return
 	default:
 		key := sc.canonicalHeader(f.Name)
@@ -570,7 +566,6 @@ func (st *stream) onNewTrailerField(f hpack.HeaderField) {
 			if len(vv) >= tooBig {
 				sc.hpackDecoder.SetEmitEnabled(false)
 			}
-
 		}
 	}
 }
@@ -1411,6 +1406,10 @@ func (st *stream) processTrailerHeaders(f *HeadersFrame) error {
 		return ConnectionError(ErrCodeProtocol)
 	}
 	st.gotTrailerHeader = true
+	if !f.StreamEnded() {
+		return StreamError{st.id, ErrCodeProtocol}
+	}
+	sc.resetPendingRequest() // we use invalidHeader from it for trailers
 	return st.processTrailerHeaderBlockFragment(f.HeaderBlockFragment(), f.HeadersEnded())
 }
 
@@ -1485,6 +1484,12 @@ func (st *stream) processTrailerHeaderBlockFragment(frag []byte, end bool) error
 	if !end {
 		return nil
 	}
+
+	rp := &sc.req
+	if rp.invalidHeader {
+		return StreamError{rp.stream.id, ErrCodeProtocol}
+	}
+
 	err := sc.hpackDecoder.Close()
 	st.endStream()
 	if err != nil {
@@ -1624,7 +1629,6 @@ func (sc *serverConn) newWriterAndRequest() (*responseWriter, *http.Request, err
 		requestURI = rp.authority // mimic HTTP/1 server behavior
 	} else {
 		var err error
-		// TODO: handle asterisk '*' requests + test
 		url_, err = url.ParseRequestURI(rp.path)
 		if err != nil {
 			return nil, nil, StreamError{rp.stream.id, ErrCodeProtocol}

http2/transport.go

@@ -75,10 +75,29 @@ type Transport struct {
 	// uncompressed.
 	DisableCompression bool
 
+	// MaxHeaderListSize is the http2 SETTINGS_MAX_HEADER_LIST_SIZE to
+	// send in the initial settings frame. It is how many bytes
+	// of response headers are allow. Unlike the http2 spec, zero here
+	// means to use a default limit (currently 10MB). If you actually
+	// want to advertise an ulimited value to the peer, Transport
+	// interprets the highest possible value here (0xffffffff or 1<<32-1)
+	// to mean no limit.
+	MaxHeaderListSize uint32
+
 	connPoolOnce  sync.Once
 	connPoolOrDef ClientConnPool // non-nil version of ConnPool
 }
 
+func (t *Transport) maxHeaderListSize() uint32 {
+	if t.MaxHeaderListSize == 0 {
+		return 10 << 20
+	}
+	if t.MaxHeaderListSize == 0xffffffff {
+		return 0
+	}
+	return t.MaxHeaderListSize
+}
+
 func (t *Transport) disableCompression() bool {
 	if t.DisableCompression {
 		return true
@@ -371,6 +390,9 @@ func (t *Transport) NewClientConn(c net.Conn) (*ClientConn, error) {
 	cc.bw = bufio.NewWriter(stickyErrWriter{c, &cc.werr})
 	cc.br = bufio.NewReader(c)
 	cc.fr = NewFramer(cc.bw, cc.br)
+
+	// TODO: SetMaxDynamicTableSize, SetMaxDynamicTableSizeLimit on
+	// henc in response to SETTINGS frames?
 	cc.henc = hpack.NewEncoder(&cc.hbuf)
 
 	type connectionStater interface {
@@ -381,10 +403,14 @@ func (t *Transport) NewClientConn(c net.Conn) (*ClientConn, error) {
 		cc.tlsState = &state
 	}
 
-	cc.fr.WriteSettings(
+	initialSettings := []Setting{
 		Setting{ID: SettingEnablePush, Val: 0},
 		Setting{ID: SettingInitialWindowSize, Val: transportDefaultStreamFlow},
-	)
+	}
+	if max := t.maxHeaderListSize(); max != 0 {
+		initialSettings = append(initialSettings, Setting{ID: SettingMaxHeaderListSize, Val: max})
+	}
+	cc.fr.WriteSettings(initialSettings...)
 	cc.fr.WriteWindowUpdate(0, transportDefaultConnFlow)
 	cc.inflow.add(transportDefaultConnFlow + initialWindowSize)
 	cc.bw.Flush()
@@ -413,7 +439,7 @@ func (t *Transport) NewClientConn(c net.Conn) (*ClientConn, error) {
 		case SettingInitialWindowSize:
 			cc.initialWindowSize = s.Val
 		default:
-			// TODO(bradfitz): handle more
+			// TODO(bradfitz): handle more; at least SETTINGS_HEADER_TABLE_SIZE?
 			t.vlogf("Unhandled Setting: %v", s)
 		}
 		return nil
@@ -805,7 +831,6 @@ func (e *badStringError) Error() string { return fmt.Sprintf("%s %q", e.what, e.
 func (cc *ClientConn) encodeHeaders(req *http.Request, addGzipHeader bool, trailers string) []byte {
 	cc.hbuf.Reset()
 
-	// TODO(bradfitz): figure out :authority-vs-Host stuff between http2 and Go
 	host := req.Host
 	if host == "" {
 		host = req.URL.Host
@@ -816,7 +841,7 @@ func (cc *ClientConn) encodeHeaders(req *http.Request, addGzipHeader bool, trail
 	// target URI (the path-absolute production and optionally a '?' character
 	// followed by the query production (see Sections 3.3 and 3.4 of
 	// [RFC3986]).
-	cc.writeHeader(":authority", host) // probably not right for all sites
+	cc.writeHeader(":authority", host)
 	cc.writeHeader(":method", req.Method)
 	if req.Method != "CONNECT" {
 		cc.writeHeader(":path", req.URL.RequestURI())
@@ -927,6 +952,7 @@ type clientConnReadLoop struct {
 	sawRegHeader         bool  // saw non-pseudo header
 	reqMalformed         error // non-nil once known to be malformed
 	lastHeaderEndsStream bool
+	headerListSize       int64 // actually uint32, but easier math this way
 }
 
 // readLoop runs in its own goroutine and reads and dispatches frames.
@@ -935,7 +961,6 @@ func (cc *ClientConn) readLoop() {
 		cc:        cc,
 		activeRes: make(map[uint32]*clientStream),
 	}
-	// TODO: figure out henc size
 	rl.hdec = hpack.NewDecoder(initialHeaderTableSize, rl.onNewHeaderField)
 
 	defer rl.cleanup()
@@ -1023,11 +1048,13 @@ func (rl *clientConnReadLoop) processHeaders(f *HeadersFrame) error {
 	rl.sawRegHeader = false
 	rl.reqMalformed = nil
 	rl.lastHeaderEndsStream = f.StreamEnded()
+	rl.headerListSize = 0
 	rl.nextRes = &http.Response{
 		Proto:      "HTTP/2.0",
 		ProtoMajor: 2,
 		Header:     make(http.Header),
 	}
+	rl.hdec.SetEmitEnabled(true)
 	return rl.processHeaderBlockFragment(f.HeaderBlockFragment(), f.StreamID, f.HeadersEnded())
 }
 
@@ -1049,7 +1076,7 @@ func (rl *clientConnReadLoop) processHeaderBlockFragment(frag []byte, streamID u
 		return nil
 	}
 	if cs.pastHeaders {
-		rl.hdec.SetEmitFunc(cs.onNewTrailerField)
+		rl.hdec.SetEmitFunc(func(f hpack.HeaderField) { rl.onNewTrailerField(cs, f) })
 	} else {
 		rl.hdec.SetEmitFunc(rl.onNewHeaderField)
 	}
@@ -1250,10 +1277,18 @@ func (rl *clientConnReadLoop) processData(f *DataFrame) error {
 	return nil
 }
 
+var errInvalidTrailers = errors.New("http2: invalid trailers")
+
 func (rl *clientConnReadLoop) endStream(cs *clientStream) {
 	// TODO: check that any declared content-length matches, like
 	// server.go's (*stream).endStream method.
-	cs.bufPipe.closeWithErrorAndCode(io.EOF, cs.copyTrailers)
+	err := io.EOF
+	code := cs.copyTrailers
+	if rl.reqMalformed != nil {
+		err = rl.reqMalformed
+		code = nil
+	}
+	cs.bufPipe.closeWithErrorAndCode(err, code)
 	delete(rl.activeRes, cs.ID)
 }
 
@@ -1292,7 +1327,7 @@ func (rl *clientConnReadLoop) processSettings(f *SettingsFrame) error {
 			// window size and this one.
 			cc.initialWindowSize = s.Val
 		default:
-			// TODO(bradfitz): handle more settings?
+			// TODO(bradfitz): handle more settings? SETTINGS_HEADER_TABLE_SIZE probably.
 			cc.vlogf("Unhandled Setting: %v", s)
 		}
 		return nil
@@ -1378,6 +1413,43 @@ func (cc *ClientConn) writeStreamReset(streamID uint32, code ErrCode, err error)
 	cc.wmu.Unlock()
 }
 
+var (
+	errResponseHeaderListSize = errors.New("http2: response header list larger than advertised limit")
+	errInvalidHeaderKey       = errors.New("http2: invalid header key")
+	errPseudoTrailers         = errors.New("http2: invalid pseudo header in trailers")
+)
+
+func (rl *clientConnReadLoop) checkHeaderField(f hpack.HeaderField) bool {
+	if rl.reqMalformed != nil {
+		return false
+	}
+
+	const headerFieldOverhead = 32 // per spec
+	rl.headerListSize += int64(len(f.Name)) + int64(len(f.Value)) + headerFieldOverhead
+	if max := rl.cc.t.maxHeaderListSize(); max != 0 && rl.headerListSize > int64(max) {
+		rl.hdec.SetEmitEnabled(false)
+		rl.reqMalformed = errResponseHeaderListSize
+		return false
+	}
+
+	if !validHeader(f.Name) {
+		rl.reqMalformed = errInvalidHeaderKey
+		return false
+	}
+
+	isPseudo := strings.HasPrefix(f.Name, ":")
+	if isPseudo {
+		if rl.sawRegHeader {
+			rl.reqMalformed = errors.New("http2: invalid pseudo header after regular header")
+			return false
+		}
+	} else {
+		rl.sawRegHeader = true
+	}
+
+	return true
+}
+
 // onNewHeaderField runs on the readLoop goroutine whenever a new
 // hpack header field is decoded.
 func (rl *clientConnReadLoop) onNewHeaderField(f hpack.HeaderField) {
@@ -1385,13 +1457,13 @@ func (rl *clientConnReadLoop) onNewHeaderField(f hpack.HeaderField) {
 	if VerboseLogs {
 		cc.logf("Header field: %+v", f)
 	}
-	// TODO: enforce max header list size like server.
+
+	if !rl.checkHeaderField(f) {
+		return
+	}
+
 	isPseudo := strings.HasPrefix(f.Name, ":")
 	if isPseudo {
-		if rl.sawRegHeader {
-			rl.reqMalformed = errors.New("http2: invalid pseudo header after regular header")
-			return
-		}
 		switch f.Name {
 		case ":status":
 			code, err := strconv.Atoi(f.Value)
@@ -1407,40 +1479,43 @@ func (rl *clientConnReadLoop) onNewHeaderField(f hpack.HeaderField) {
 			// document."
 			rl.reqMalformed = fmt.Errorf("http2: unknown response pseudo header %q", f.Name)
 		}
-	} else {
-		rl.sawRegHeader = true
-		key := http.CanonicalHeaderKey(f.Name)
-		if key == "Trailer" {
-			t := rl.nextRes.Trailer
-			if t == nil {
-				t = make(http.Header)
-				rl.nextRes.Trailer = t
-			}
-			foreachHeaderElement(f.Value, func(v string) {
-				t[http.CanonicalHeaderKey(v)] = nil
-			})
-		} else {
-			rl.nextRes.Header.Add(key, f.Value)
+		return
+	}
+
+	key := http.CanonicalHeaderKey(f.Name)
+	if key == "Trailer" {
+		t := rl.nextRes.Trailer
+		if t == nil {
+			t = make(http.Header)
+			rl.nextRes.Trailer = t
 		}
+		foreachHeaderElement(f.Value, func(v string) {
+			t[http.CanonicalHeaderKey(v)] = nil
+		})
+	} else {
+		rl.nextRes.Header.Add(key, f.Value)
 	}
 }
 
-func (cs *clientStream) onNewTrailerField(f hpack.HeaderField) {
-	isPseudo := strings.HasPrefix(f.Name, ":")
-	if isPseudo {
-		// TODO: Bogus. report an error later when we close their body.
-		// drop for now.
+func (rl *clientConnReadLoop) onNewTrailerField(cs *clientStream, f hpack.HeaderField) {
+	if !rl.checkHeaderField(f) {
 		return
 	}
+	if strings.HasPrefix(f.Name, ":") {
+		// Pseudo-header fields MUST NOT appear in
+		// trailers. Endpoints MUST treat a request or
+		// response that contains undefined or invalid
+		// pseudo-header fields as malformed.
+		rl.reqMalformed = errPseudoTrailers
+		return
+	}
+
 	key := http.CanonicalHeaderKey(f.Name)
 	if _, ok := cs.resTrailer[key]; ok {
 		if cs.trailer == nil {
 			cs.trailer = make(http.Header)
 		}
-		const tooBig = 1000 // TODO: arbitrary; use max header list size limits
-		if cur := cs.trailer[key]; len(cur) < tooBig {
-			cs.trailer[key] = append(cur, f.Value)
-		}
+		cs.trailer[key] = append(cs.trailer[key], f.Value)
 	}
 }
 

http2/transport_test.go

@@ -1002,3 +1002,169 @@ func testTransportResPattern(t *testing.T, expect100Continue, resHeader headerTy
 	}
 	ct.run()
 }
+
+func TestTransportInvalidTrailer_Pseudo1(t *testing.T) {
+	testTransportInvalidTrailer_Pseudo(t, oneHeader)
+}
+func TestTransportInvalidTrailer_Pseudo2(t *testing.T) {
+	testTransportInvalidTrailer_Pseudo(t, splitHeader)
+}
+func testTransportInvalidTrailer_Pseudo(t *testing.T, trailers headerType) {
+	testInvalidTrailer(t, trailers, errPseudoTrailers, func(enc *hpack.Encoder) {
+		enc.WriteField(hpack.HeaderField{Name: ":colon", Value: "foo"})
+		enc.WriteField(hpack.HeaderField{Name: "foo", Value: "bar"})
+	})
+}
+
+func TestTransportInvalidTrailer_Capital1(t *testing.T) {
+	testTransportInvalidTrailer_Capital(t, oneHeader)
+}
+func TestTransportInvalidTrailer_Capital2(t *testing.T) {
+	testTransportInvalidTrailer_Capital(t, splitHeader)
+}
+func testTransportInvalidTrailer_Capital(t *testing.T, trailers headerType) {
+	testInvalidTrailer(t, trailers, errInvalidHeaderKey, func(enc *hpack.Encoder) {
+		enc.WriteField(hpack.HeaderField{Name: "foo", Value: "bar"})
+		enc.WriteField(hpack.HeaderField{Name: "Capital", Value: "bad"})
+	})
+}
+
+func testInvalidTrailer(t *testing.T, trailers headerType, wantErr error, writeTrailer func(*hpack.Encoder)) {
+	ct := newClientTester(t)
+	ct.client = func() error {
+		req, _ := http.NewRequest("GET", "https://dummy.tld/", nil)
+		res, err := ct.tr.RoundTrip(req)
+		if err != nil {
+			return fmt.Errorf("RoundTrip: %v", err)
+		}
+		defer res.Body.Close()
+		if res.StatusCode != 200 {
+			return fmt.Errorf("status code = %v; want 200", res.StatusCode)
+		}
+		slurp, err := ioutil.ReadAll(res.Body)
+		if err != wantErr {
+			return fmt.Errorf("res.Body ReadAll error = %q, %v; want %v", slurp, err, wantErr)
+		}
+		if len(slurp) > 0 {
+			return fmt.Errorf("body = %q; want nothing", slurp)
+		}
+		return nil
+	}
+	ct.server = func() error {
+		ct.greet()
+		var buf bytes.Buffer
+		enc := hpack.NewEncoder(&buf)
+
+		for {
+			f, err := ct.fr.ReadFrame()
+			if err != nil {
+				return err
+			}
+			switch f := f.(type) {
+			case *HeadersFrame:
+				var endStream bool
+				send := func(mode headerType) {
+					hbf := buf.Bytes()
+					switch mode {
+					case oneHeader:
+						ct.fr.WriteHeaders(HeadersFrameParam{
+							StreamID:      f.StreamID,
+							EndHeaders:    true,
+							EndStream:     endStream,
+							BlockFragment: hbf,
+						})
+					case splitHeader:
+						if len(hbf) < 2 {
+							panic("too small")
+						}
+						ct.fr.WriteHeaders(HeadersFrameParam{
+							StreamID:      f.StreamID,
+							EndHeaders:    false,
+							EndStream:     endStream,
+							BlockFragment: hbf[:1],
+						})
+						ct.fr.WriteContinuation(f.StreamID, true, hbf[1:])
+					default:
+						panic("bogus mode")
+					}
+				}
+				// Response headers (1+ frames; 1 or 2 in this test, but never 0)
+				{
+					buf.Reset()
+					enc.WriteField(hpack.HeaderField{Name: ":status", Value: "200"})
+					enc.WriteField(hpack.HeaderField{Name: "trailer", Value: "declared"})
+					endStream = false
+					send(oneHeader)
+				}
+				// Trailers:
+				{
+					endStream = true
+					buf.Reset()
+					writeTrailer(enc)
+					send(trailers)
+				}
+				return nil
+			}
+		}
+	}
+	ct.run()
+}
+
+func TestTransportChecksResponseHeaderListSize(t *testing.T) {
+	ct := newClientTester(t)
+	ct.client = func() error {
+		req, _ := http.NewRequest("GET", "https://dummy.tld/", nil)
+		res, err := ct.tr.RoundTrip(req)
+		if err != errResponseHeaderListSize {
+			if res != nil {
+				res.Body.Close()
+			}
+			size := int64(0)
+			for k, vv := range res.Header {
+				for _, v := range vv {
+					size += int64(len(k)) + int64(len(v)) + 32
+				}
+			}
+			return fmt.Errorf("RoundTrip Error = %v (and %d bytes of response headers); want errResponseHeaderListSize", err, size)
+		}
+		return nil
+	}
+	ct.server = func() error {
+		ct.greet()
+		var buf bytes.Buffer
+		enc := hpack.NewEncoder(&buf)
+
+		for {
+			f, err := ct.fr.ReadFrame()
+			if err != nil {
+				return err
+			}
+			switch f := f.(type) {
+			case *HeadersFrame:
+				enc.WriteField(hpack.HeaderField{Name: ":status", Value: "200"})
+				large := strings.Repeat("a", 1<<10)
+				for i := 0; i < 5042; i++ {
+					enc.WriteField(hpack.HeaderField{Name: large, Value: large})
+				}
+				if size, want := buf.Len(), 6329; size != want {
+					// Note: this number might change if
+					// our hpack implementation
+					// changes. That's fine. This is
+					// just a sanity check that our
+					// response can fit in a single
+					// header block fragment frame.
+					return fmt.Errorf("encoding over 10MB of duplicate keypairs took %d bytes; expected %d", size, want)
+				}
+				ct.fr.WriteHeaders(HeadersFrameParam{
+					StreamID:      f.StreamID,
+					EndHeaders:    true,
+					EndStream:     true,
+					BlockFragment: buf.Bytes(),
+				})
+				return nil
+			}
+		}
+	}
+	ct.run()
+
+}