http1: do not add transfer-encoding: chunked to 1xx/204 responses (en…

…voyproxy#11458)

Follow-on to envoyproxy#10811. The previous change would strip the
transfer-encoding: chunked header if returned by an upstream
during upgrade, but the http/1.1 codec adds the header back.
RFC 7230, Section 3.3.1 requires that no such header appear
in a 1xx or 204 response.

Use runtime feature "envoy.reloadable_features.strict_1xx_and_204_response_headers"
to disable.

Risk Level: medium
Testing: updated tests
Docs Changes: n/a
Release Notes: updated

Signed-off-by: Stephan Zuercher <[email protected]>

docs/root/version_history/current.rst

@@ -20,6 +20,7 @@ Minor Behavior Changes
   Can be reverted temporarily by setting runtime feature `envoy.reloadable_features.fix_upgrade_response` to false.
 * http: stopped overwriting `date` response headers. Responses without a `date` header will still have the header properly set. This behavior can be temporarily reverted by setting `envoy.reloadable_features.preserve_upstream_date` to false.
 * http: stopped adding a synthetic path to CONNECT requests, meaning unconfigured CONNECT requests will now return 404 instead of 403. This behavior can be temporarily reverted by setting `envoy.reloadable_features.stop_faking_paths` to false.
+* http: stopped allowing upstream 1xx or 204 responses with Transfer-Encoding or non-zero Content-Length headers. Content-Length of 0 is allowed, but stripped. This behavior can be temporarily reverted by setting `envoy.reloadable_features.strict_1xx_and_204_response_headers` to false.
 * router: allow retries of streaming or incomplete requests. This removes stat `rq_retry_skipped_request_not_complete`.
 * router: allow retries by default when upstream responds with :ref:`x-envoy-overloaded <config_http_filters_router_x-envoy-overloaded_set>`.
 

source/common/http/http1/codec_impl.cc

@@ -35,6 +35,8 @@ struct Http1ResponseCodeDetailValues {
   const absl::string_view InvalidUrl = "http1.invalid_url";
   const absl::string_view InvalidTransferEncoding = "http1.invalid_transfer_encoding";
   const absl::string_view BodyDisallowed = "http1.body_disallowed";
+  const absl::string_view TransferEncodingNotAllowed = "http1.transfer_encoding_not_allowed";
+  const absl::string_view ContentLengthNotAllowed = "http1.content_length_not_allowed";
 };
 
 struct Http1HeaderTypesValues {
@@ -69,7 +71,7 @@ StreamEncoderImpl::StreamEncoderImpl(ConnectionImpl& connection,
                                      HeaderKeyFormatter* header_key_formatter)
     : connection_(connection), disable_chunk_encoding_(false), chunk_encoding_(true),
       processing_100_continue_(false), is_response_to_head_request_(false),
-      is_response_to_connect_request_(false), is_content_length_allowed_(true),
+      is_response_to_connect_request_(false), is_1xx_(false), is_204_(false),
       header_key_formatter_(header_key_formatter) {
   if (connection_.connection().aboveHighWatermark()) {
     runHighWatermarkCallbacks();
@@ -159,15 +161,22 @@ void StreamEncoderImpl::encodeHeadersBase(const RequestOrResponseHeaderMap& head
       // response to a HEAD request.
       // For 204s and 1xx where content length is disallowed, don't append the content length but
       // also don't chunk encode.
-      if (is_content_length_allowed_) {
+      if (!is_1xx_ && !is_204_) {
         encodeFormattedHeader(Headers::get().ContentLength.get(), "0");
       }
       chunk_encoding_ = false;
     } else if (connection_.protocol() == Protocol::Http10) {
       chunk_encoding_ = false;
+    } else if (connection_.strict1xxAnd204Headers() && (is_1xx_ || is_204_)) {
+      // For 1xx and 204 responses, do not send the chunked encoding header or enable chunked
+      // encoding: https://tools.ietf.org/html/rfc7230#section-3.3.1
+      chunk_encoding_ = false;
+
+      // Assert 1xx (may have content) OR 204 and end stream.
+      ASSERT(is_1xx_ || end_stream);
     } else {
       // For responses to connect requests, do not send the chunked encoding header:
-      // https://tools.ietf.org/html/rfc7231#section-4.3.6
+      // https://tools.ietf.org/html/rfc7231#section-4.3.6.
       if (!is_response_to_connect_request_) {
         encodeFormattedHeader(Headers::get().TransferEncoding.get(),
                               Headers::get().TransferEncodingValues.Chunked);
@@ -352,14 +361,12 @@ void ResponseEncoderImpl::encodeHeaders(const ResponseHeaderMap& headers, bool e
   connection_.addCharToBuffer('\r');
   connection_.addCharToBuffer('\n');
 
-  if (numeric_status == 204 || numeric_status < 200) {
-    // Per https://tools.ietf.org/html/rfc7230#section-3.3.2
-    setIsContentLengthAllowed(false);
-  } else {
-    // Make sure that if we encodeHeaders(100) then encodeHeaders(200) that we
-    // set is_content_length_allowed_ back to true.
-    setIsContentLengthAllowed(true);
-  }
+  // Enabling handling of https://tools.ietf.org/html/rfc7230#section-3.3.1 and
+  // https://tools.ietf.org/html/rfc7230#section-3.3.2. Also resets these flags
+  // if a 100 Continue is followed by another status.
+  setIs1xx(numeric_status < 200);
+  setIs204(numeric_status == 204);
+
   if (numeric_status >= 300) {
     // Don't do special CONNECT logic if the CONNECT was rejected.
     is_response_to_connect_request_ = false;
@@ -459,6 +466,8 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stat
       enable_trailers_(enable_trailers),
       reject_unsupported_transfer_encodings_(Runtime::runtimeFeatureEnabled(
           "envoy.reloadable_features.reject_unsupported_transfer_encodings")),
+      strict_1xx_and_204_headers_(Runtime::runtimeFeatureEnabled(
+          "envoy.reloadable_features.strict_1xx_and_204_response_headers")),
       output_buffer_([&]() -> void { this->onBelowLowWatermark(); },
                      [&]() -> void { this->onAboveHighWatermark(); },
                      []() -> void { /* TODO(adisuissa): Handle overflow watermark */ }),
@@ -1078,6 +1087,25 @@ int ClientConnectionImpl::onHeadersComplete() {
       }
     }
 
+    if (strict_1xx_and_204_headers_ && (parser_.status_code < 200 || parser_.status_code == 204)) {
+      if (headers->TransferEncoding()) {
+        sendProtocolError(Http1ResponseCodeDetails::get().TransferEncodingNotAllowed);
+        throw CodecProtocolException(
+            "http/1.1 protocol error: transfer encoding not allowed in 1xx or 204");
+      }
+
+      if (headers->ContentLength()) {
+        // Report a protocol error for non-zero Content-Length, but paper over zero Content-Length.
+        if (headers->ContentLength()->value().getStringView() != "0") {
+          sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed);
+          throw CodecProtocolException(
+              "http/1.1 protocol error: content length not allowed in 1xx or 204");
+        }
+
+        headers->removeContentLength();
+      }
+    }
+
     if (parser_.status_code == 100) {
       // http-parser treats 100 continue headers as their own complete response.
       // Swallow the spurious onMessageComplete and continue processing.

source/common/http/http1/codec_impl.h

@@ -95,7 +95,8 @@ class StreamEncoderImpl : public virtual StreamEncoder,
 
 protected:
   StreamEncoderImpl(ConnectionImpl& connection, HeaderKeyFormatter* header_key_formatter);
-  void setIsContentLengthAllowed(bool value) { is_content_length_allowed_ = value; }
+  void setIs1xx(bool value) { is_1xx_ = value; }
+  void setIs204(bool value) { is_204_ = value; }
   void encodeHeadersBase(const RequestOrResponseHeaderMap& headers, bool end_stream);
   void encodeTrailersBase(const HeaderMap& headers);
 
@@ -109,7 +110,8 @@ class StreamEncoderImpl : public virtual StreamEncoder,
   bool processing_100_continue_ : 1;
   bool is_response_to_head_request_ : 1;
   bool is_response_to_connect_request_ : 1;
-  bool is_content_length_allowed_ : 1;
+  bool is_1xx_ : 1;
+  bool is_204_ : 1;
 
 private:
   /**
@@ -236,6 +238,8 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
   void onUnderlyingConnectionAboveWriteBufferHighWatermark() override { onAboveHighWatermark(); }
   void onUnderlyingConnectionBelowWriteBufferLowWatermark() override { onBelowLowWatermark(); }
 
+  bool strict1xxAnd204Headers() { return strict_1xx_and_204_headers_; }
+
 protected:
   ConnectionImpl(Network::Connection& connection, CodecStats& stats, http_parser_type type,
                  uint32_t max_headers_kb, const uint32_t max_headers_count,
@@ -261,6 +265,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
   const bool connection_header_sanitization_ : 1;
   const bool enable_trailers_ : 1;
   const bool reject_unsupported_transfer_encodings_ : 1;
+  const bool strict_1xx_and_204_headers_ : 1;
 
 private:
   enum class HeaderParsingState { Field, Value, Done };

source/common/runtime/runtime_features.cc

@@ -67,8 +67,9 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.fix_upgrade_response",
     "envoy.reloadable_features.fixed_connection_close",
     "envoy.reloadable_features.listener_in_place_filterchain_update",
-    "envoy.reloadable_features.stop_faking_paths",
     "envoy.reloadable_features.preserve_upstream_date",
+    "envoy.reloadable_features.stop_faking_paths",
+    "envoy.reloadable_features.strict_1xx_and_204_response_headers",
 };
 
 // This is a section for officially sanctioned runtime features which are too

test/common/http/http1/codec_impl_test.cc

@@ -1659,6 +1659,31 @@ TEST_F(Http1ServerConnectionImplTest, UpgradeRequestWithNoBody) {
   EXPECT_TRUE(status.ok());
 }
 
+// Test that 101 upgrade responses do not contain content-length or transfer-encoding headers.
+TEST_F(Http1ServerConnectionImplTest, UpgradeRequestResponseHeaders) {
+  initialize();
+
+  NiceMock<MockRequestDecoder> decoder;
+  Http::ResponseEncoder* response_encoder = nullptr;
+  EXPECT_CALL(callbacks_, newStream(_, _))
+      .WillOnce(Invoke([&](ResponseEncoder& encoder, bool) -> RequestDecoder& {
+        response_encoder = &encoder;
+        return decoder;
+      }));
+
+  Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\nConnection: upgrade\r\nUpgrade: foo\r\n\r\n");
+  auto status = codec_->dispatch(buffer);
+  EXPECT_TRUE(status.ok());
+  EXPECT_EQ(0U, buffer.length());
+
+  std::string output;
+  ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output));
+
+  TestResponseHeaderMapImpl headers{{":status", "101"}};
+  response_encoder->encodeHeaders(headers, false);
+  EXPECT_EQ("HTTP/1.1 101 Switching Protocols\r\n\r\n", output);
+}
+
 TEST_F(Http1ServerConnectionImplTest, ConnectRequestNoContentLength) {
   initialize();
 
@@ -1956,11 +1981,116 @@ TEST_F(Http1ClientConnectionImplTest, 204Response) {
   request_encoder.encodeHeaders(headers, true);
 
   EXPECT_CALL(response_decoder, decodeHeaders_(_, true));
-  Buffer::OwnedImpl response("HTTP/1.1 204 OK\r\nContent-Length: 20\r\n\r\n");
+  Buffer::OwnedImpl response("HTTP/1.1 204 OK\r\n\r\n");
   auto status = codec_->dispatch(response);
   EXPECT_TRUE(status.ok());
 }
 
+// 204 No Content with Content-Length is barred by RFC 7230, Section 3.3.2.
+TEST_F(Http1ClientConnectionImplTest, 204ResponseContentLengthNotAllowed) {
+  // By default, content-length is barred.
+  {
+    initialize();
+
+    NiceMock<MockResponseDecoder> response_decoder;
+    Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder);
+    TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}};
+    request_encoder.encodeHeaders(headers, true);
+
+    Buffer::OwnedImpl response("HTTP/1.1 204 OK\r\nContent-Length: 20\r\n\r\n");
+    auto status = codec_->dispatch(response);
+    EXPECT_FALSE(status.ok());
+  }
+
+  // Test with feature disabled: content-length allowed.
+  {
+    TestScopedRuntime scoped_runtime;
+    Runtime::LoaderSingleton::getExisting()->mergeValues(
+        {{"envoy.reloadable_features.strict_1xx_and_204_response_headers", "false"}});
+
+    initialize();
+
+    NiceMock<MockResponseDecoder> response_decoder;
+    Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder);
+    TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}};
+    request_encoder.encodeHeaders(headers, true);
+
+    Buffer::OwnedImpl response("HTTP/1.1 204 OK\r\nContent-Length: 20\r\n\r\n");
+    auto status = codec_->dispatch(response);
+    EXPECT_TRUE(status.ok());
+  }
+}
+
+// 204 No Content with Content-Length: 0 is technically barred by RFC 7230, Section 3.3.2, but we
+// allow it.
+TEST_F(Http1ClientConnectionImplTest, 204ResponseWithContentLength0) {
+  {
+    initialize();
+
+    NiceMock<MockResponseDecoder> response_decoder;
+    Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder);
+    TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}};
+    request_encoder.encodeHeaders(headers, true);
+
+    EXPECT_CALL(response_decoder, decodeHeaders_(_, true));
+    Buffer::OwnedImpl response("HTTP/1.1 204 OK\r\nContent-Length: 0\r\n\r\n");
+    auto status = codec_->dispatch(response);
+    EXPECT_TRUE(status.ok());
+  }
+
+  // Test with feature disabled: content-length allowed.
+  {
+    TestScopedRuntime scoped_runtime;
+    Runtime::LoaderSingleton::getExisting()->mergeValues(
+        {{"envoy.reloadable_features.strict_1xx_and_204_response_headers", "false"}});
+
+    NiceMock<MockResponseDecoder> response_decoder;
+    Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder);
+    TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}};
+    request_encoder.encodeHeaders(headers, true);
+
+    EXPECT_CALL(response_decoder, decodeHeaders_(_, true));
+    Buffer::OwnedImpl response("HTTP/1.1 204 OK\r\nContent-Length: 0\r\n\r\n");
+    auto status = codec_->dispatch(response);
+    EXPECT_TRUE(status.ok());
+  }
+}
+
+// 204 No Content with Transfer-Encoding headers is barred by RFC 7230, Section 3.3.1.
+TEST_F(Http1ClientConnectionImplTest, 204ResponseTransferEncodingNotAllowed) {
+  // By default, transfer-encoding is barred.
+  {
+    initialize();
+
+    NiceMock<MockResponseDecoder> response_decoder;
+    Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder);
+    TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}};
+    request_encoder.encodeHeaders(headers, true);
+
+    Buffer::OwnedImpl response("HTTP/1.1 204 OK\r\nTransfer-Encoding: chunked\r\n\r\n");
+    auto status = codec_->dispatch(response);
+    EXPECT_FALSE(status.ok());
+  }
+
+  // Test with feature disabled: transfer-encoding allowed.
+  {
+    TestScopedRuntime scoped_runtime;
+    Runtime::LoaderSingleton::getExisting()->mergeValues(
+        {{"envoy.reloadable_features.strict_1xx_and_204_response_headers", "false"}});
+
+    initialize();
+
+    NiceMock<MockResponseDecoder> response_decoder;
+    Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder);
+    TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}};
+    request_encoder.encodeHeaders(headers, true);
+
+    Buffer::OwnedImpl response("HTTP/1.1 204 OK\r\nTransfer-Encoding: chunked\r\n\r\n");
+    auto status = codec_->dispatch(response);
+    EXPECT_TRUE(status.ok());
+  }
+}
+
 TEST_F(Http1ClientConnectionImplTest, 100Response) {
   initialize();
 
@@ -1976,11 +2106,48 @@ TEST_F(Http1ClientConnectionImplTest, 100Response) {
 
   EXPECT_CALL(response_decoder, decodeHeaders_(_, false));
   EXPECT_CALL(response_decoder, decodeData(_, _)).Times(0);
-  Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\nContent-Length: 20\r\n\r\n");
+  Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\n\r\n");
   status = codec_->dispatch(response);
   EXPECT_TRUE(status.ok());
 }
 
+// 101 Switching Protocol with Transfer-Encoding headers is barred by RFC 7230, Section 3.3.1.
+TEST_F(Http1ClientConnectionImplTest, 101ResponseTransferEncodingNotAllowed) {
+  // By default, transfer-encoding is barred.
+  {
+    initialize();
+
+    NiceMock<MockResponseDecoder> response_decoder;
+    Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder);
+    TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}};
+    request_encoder.encodeHeaders(headers, true);
+
+    Buffer::OwnedImpl response(
+        "HTTP/1.1 101 Switching Protocols\r\nTransfer-Encoding: chunked\r\n\r\n");
+    auto status = codec_->dispatch(response);
+    EXPECT_FALSE(status.ok());
+  }
+
+  // Test with feature disabled: transfer-encoding allowed.
+  {
+    TestScopedRuntime scoped_runtime;
+    Runtime::LoaderSingleton::getExisting()->mergeValues(
+        {{"envoy.reloadable_features.strict_1xx_and_204_response_headers", "false"}});
+
+    initialize();
+
+    NiceMock<MockResponseDecoder> response_decoder;
+    Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder);
+    TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}};
+    request_encoder.encodeHeaders(headers, true);
+
+    Buffer::OwnedImpl response(
+        "HTTP/1.1 101 Switching Protocols\r\nTransfer-Encoding: chunked\r\n\r\n");
+    auto status = codec_->dispatch(response);
+    EXPECT_TRUE(status.ok());
+  }
+}
+
 TEST_F(Http1ClientConnectionImplTest, BadEncodeParams) {
   initialize();
 

test/integration/websocket_integration_test.cc

@@ -100,6 +100,8 @@ void WebsocketIntegrationTest::validateUpgradeResponseHeaders(
   proxied_response_headers.removeDate();
   proxied_response_headers.removeServer();
 
+  ASSERT_TRUE(proxied_response_headers.TransferEncoding() == nullptr);
+
   commonValidate(proxied_response_headers, original_response_headers);
 
   EXPECT_THAT(&proxied_response_headers, HeaderMapEqualIgnoreOrder(&original_response_headers));
@@ -419,9 +421,6 @@ TEST_P(WebsocketIntegrationTest, BidirectionalChunkedData) {
   if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) {
     ASSERT_TRUE(upstream_request_->headers().TransferEncoding() != nullptr);
   }
-  if (downstreamProtocol() == Http::CodecClient::Type::HTTP1) {
-    ASSERT_TRUE(response_->headers().TransferEncoding() != nullptr);
-  }
 
   // Send both a chunked request body and "websocket" payload.
   std::string request_payload = "3\r\n123\r\n0\r\n\r\nSomeWebsocketRequestPayload";