From 9f4b31a11cc4deba7f5c542399d5ec71fab3a053 Mon Sep 17 00:00:00 2001 From: Zach Reyes <39203661+zasweq@users.noreply.github.com> Date: Thu, 19 May 2022 14:48:44 -0400 Subject: [PATCH] Added HTTP status and grpc status to POST check (#5364) * Added HTTP status and grpc status to POST check --- internal/transport/controlbuf.go | 6 +++ internal/transport/http2_server.go | 16 ++++--- internal/transport/transport_test.go | 70 +++++++++++++++++----------- 3 files changed, 58 insertions(+), 34 deletions(-) diff --git a/internal/transport/controlbuf.go b/internal/transport/controlbuf.go index 8394d252df0..244f4b081d5 100644 --- a/internal/transport/controlbuf.go +++ b/internal/transport/controlbuf.go @@ -137,6 +137,7 @@ type earlyAbortStream struct { streamID uint32 contentSubtype string status *status.Status + rst bool } func (*earlyAbortStream) isTransportResponseFrame() bool { return false } @@ -786,6 +787,11 @@ func (l *loopyWriter) earlyAbortStreamHandler(eas *earlyAbortStream) error { if err := l.writeHeader(eas.streamID, true, headerFields, nil); err != nil { return err } + if eas.rst { + if err := l.framer.fr.WriteRSTStream(eas.streamID, http2.ErrCodeNo); err != nil { + return err + } + } return nil } diff --git a/internal/transport/http2_server.go b/internal/transport/http2_server.go index 4969102f4af..45d7bd145e3 100644 --- a/internal/transport/http2_server.go +++ b/internal/transport/http2_server.go @@ -448,6 +448,7 @@ func (t *http2Server) operateHeaders(frame *http2.MetaHeadersFrame, handle func( streamID: streamID, contentSubtype: s.contentSubtype, status: status.New(codes.Internal, errMsg), + rst: !frame.StreamEnded(), }) return false } @@ -521,14 +522,16 @@ func (t *http2Server) operateHeaders(frame *http2.MetaHeadersFrame, handle func( } if httpMethod != http.MethodPost { t.mu.Unlock() + errMsg := fmt.Sprintf("http2Server.operateHeaders parsed a :method field: %v which should be POST", httpMethod) if logger.V(logLevel) { - logger.Infof("transport: http2Server.operateHeaders parsed a :method field: %v which should be POST", httpMethod) + logger.Infof("transport: %v", errMsg) } - t.controlBuf.put(&cleanupStream{ - streamID: streamID, - rst: true, - rstCode: http2.ErrCodeProtocol, - onWrite: func() {}, + t.controlBuf.put(&earlyAbortStream{ + httpStatus: 405, + streamID: streamID, + contentSubtype: s.contentSubtype, + status: status.New(codes.Internal, errMsg), + rst: !frame.StreamEnded(), }) s.cancel() return false @@ -549,6 +552,7 @@ func (t *http2Server) operateHeaders(frame *http2.MetaHeadersFrame, handle func( streamID: s.id, contentSubtype: s.contentSubtype, status: stat, + rst: !frame.StreamEnded(), }) return false } diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go index d129dbf0a3b..c1f9664ada6 100644 --- a/internal/transport/transport_test.go +++ b/internal/transport/transport_test.go @@ -1696,21 +1696,6 @@ func (s) TestHeadersCausingStreamError(t *testing.T) { values []string } }{ - // If the client sends an HTTP/2 request with a :method header with a - // value other than POST, as specified in the gRPC over HTTP/2 - // specification, the server should close the stream. - { - name: "Client Sending Wrong Method", - headers: []struct { - name string - values []string - }{ - {name: ":method", values: []string{"PUT"}}, - {name: ":path", values: []string{"foo"}}, - {name: ":authority", values: []string{"localhost"}}, - {name: "content-type", values: []string{"application/grpc"}}, - }, - }, // "Transports must consider requests containing the Connection header // as malformed" - A41 Malformed requests map to a stream error of type // PROTOCOL_ERROR. @@ -1827,25 +1812,30 @@ func (s) TestHeadersCausingStreamError(t *testing.T) { } } -// TestHeadersMultipleHosts tests that a request with multiple hosts gets -// rejected with HTTP Status 400 and gRPC status Internal, regardless of whether -// the client is speaking gRPC or not. -func (s) TestHeadersMultipleHosts(t *testing.T) { +// TestHeadersHTTPStatusGRPCStatus tests requests with certain headers get a +// certain HTTP and gRPC status back. +func (s) TestHeadersHTTPStatusGRPCStatus(t *testing.T) { tests := []struct { name string headers []struct { name string values []string } + httpStatusWant string + grpcStatusWant string + grpcMessageWant string }{ // Note: multiple authority headers are handled by the framer itself, // which will cause a stream error. Thus, it will never get to - // operateHeaders with the check in operateHeaders for possible grpc-status sent back. + // operateHeaders with the check in operateHeaders for possible + // grpc-status sent back. // multiple :authority or multiple Host headers would make the eventual // :authority ambiguous as per A41. This takes precedence even over the // fact a request is non grpc. All of these requests should be rejected - // with grpc-status Internal. + // with grpc-status Internal. Thus, requests with multiple hosts should + // get rejected with HTTP Status 400 and gRPC status Internal, + // regardless of whether the client is speaking gRPC or not. { name: "Multiple host headers non grpc", headers: []struct { @@ -1857,6 +1847,9 @@ func (s) TestHeadersMultipleHosts(t *testing.T) { {name: ":authority", values: []string{"localhost"}}, {name: "host", values: []string{"localhost", "localhost2"}}, }, + httpStatusWant: "400", + grpcStatusWant: "13", + grpcMessageWant: "both must only have 1 value as per HTTP/2 spec", }, { name: "Multiple host headers grpc", @@ -1870,6 +1863,27 @@ func (s) TestHeadersMultipleHosts(t *testing.T) { {name: "content-type", values: []string{"application/grpc"}}, {name: "host", values: []string{"localhost", "localhost2"}}, }, + httpStatusWant: "400", + grpcStatusWant: "13", + grpcMessageWant: "both must only have 1 value as per HTTP/2 spec", + }, + // If the client sends an HTTP/2 request with a :method header with a + // value other than POST, as specified in the gRPC over HTTP/2 + // specification, the server should fail the RPC. + { + name: "Client Sending Wrong Method", + headers: []struct { + name string + values []string + }{ + {name: ":method", values: []string{"PUT"}}, + {name: ":path", values: []string{"foo"}}, + {name: ":authority", values: []string{"localhost"}}, + {name: "content-type", values: []string{"application/grpc"}}, + }, + httpStatusWant: "405", + grpcStatusWant: "13", + grpcMessageWant: "which should be POST", }, } for _, test := range tests { @@ -1909,10 +1923,10 @@ func (s) TestHeadersMultipleHosts(t *testing.T) { case *http2.SettingsFrame: // Do nothing. A settings frame is expected from server preface. case *http2.MetaHeadersFrame: - var status, grpcStatus, grpcMessage string + var httpStatus, grpcStatus, grpcMessage string for _, header := range frame.Fields { if header.Name == ":status" { - status = header.Value + httpStatus = header.Value } if header.Name == "grpc-status" { grpcStatus = header.Value @@ -1921,15 +1935,15 @@ func (s) TestHeadersMultipleHosts(t *testing.T) { grpcMessage = header.Value } } - if status != "400" { - result.Send(fmt.Errorf("incorrect HTTP Status got %v, want 200", status)) + if httpStatus != test.httpStatusWant { + result.Send(fmt.Errorf("incorrect HTTP Status got %v, want %v", httpStatus, test.httpStatusWant)) return } - if grpcStatus != "13" { // grpc status code internal - result.Send(fmt.Errorf("incorrect gRPC Status got %v, want 13", grpcStatus)) + if grpcStatus != test.grpcStatusWant { // grpc status code internal + result.Send(fmt.Errorf("incorrect gRPC Status got %v, want %v", grpcStatus, test.grpcStatusWant)) return } - if !strings.Contains(grpcMessage, "both must only have 1 value as per HTTP/2 spec") { + if !strings.Contains(grpcMessage, test.grpcMessageWant) { result.Send(fmt.Errorf("incorrect gRPC message")) return }