http: adding response code details for downstream HTTP/1.1 codec erro…

…rs (#9286)

Also adding a fix to Connect header rejection where the codec sent a protocol error without throwing an exception.

Risk Level: Medium (minimal tweaks to data plane code)
Testing: new unit tests, integration test
Docs Changes: n/a
Release Notes: n/a
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

include/envoy/http/codec.h

@@ -203,6 +203,15 @@ class Stream {
    * @return uint32_t the stream's configured buffer limits.
    */
   virtual uint32_t bufferLimit() PURE;
+
+  /*
+   * @return string_view optionally return the reason behind codec level errors.
+   *
+   * This information is communicated via direct accessor rather than passed with the
+   * CodecProtocolException so that the error can be associated only with the problematic stream and
+   * not associated with every stream on the connection.
+   */
+  virtual absl::string_view responseDetails() { return ""; }
 };
 
 /**

source/common/http/conn_manager_impl.cc

@@ -380,6 +380,10 @@ void ConnectionManagerImpl::resetAllStreams(
     stream.onResetStream(StreamResetReason::ConnectionTermination, absl::string_view());
     if (response_flag.has_value()) {
       stream.stream_info_.setResponseFlag(response_flag.value());
+      if (*response_flag == StreamInfo::ResponseFlag::DownstreamProtocolError) {
+        stream.stream_info_.setResponseCodeDetails(
+            stream.response_encoder_->getStream().responseDetails());
+      }
     }
   }
 }

source/common/http/http1/codec_impl.cc

@@ -24,6 +24,17 @@ namespace Http {
 namespace Http1 {
 namespace {
 
+struct Http1ResponseCodeDetailValues {
+  const absl::string_view TooManyHeaders = "http1.too_many_headers";
+  const absl::string_view HeadersTooLarge = "http1.headers_too_large";
+  const absl::string_view HttpCodecError = "http1.codec_error";
+  const absl::string_view InvalidCharacters = "http1.invalid_characters";
+  const absl::string_view ConnectionHeaderSanitization = "http1.connection_header_rejected";
+  const absl::string_view InvalidUrl = "http1.invalid_url";
+};
+
+using Http1ResponseCodeDetails = ConstSingleton<Http1ResponseCodeDetailValues>;
+
 const StringUtil::CaseUnorderedSet& caseUnorderdSetContainingUpgradeAndHttp2Settings() {
   CONSTRUCT_ON_FIRST_USE(StringUtil::CaseUnorderedSet,
                          Http::Headers::get().ConnectionValues.Upgrade,
@@ -381,7 +392,7 @@ void ConnectionImpl::completeLastHeader() {
   // Check if the number of headers exceeds the limit.
   if (current_header_map_->size() > max_headers_count_) {
     error_code_ = Http::Code::RequestHeaderFieldsTooLarge;
-    sendProtocolError();
+    sendProtocolError(Http1ResponseCodeDetails::get().TooManyHeaders);
     throw CodecProtocolException("headers size exceeds limit");
   }
 
@@ -442,7 +453,7 @@ void ConnectionImpl::dispatch(Buffer::Instance& data) {
 size_t ConnectionImpl::dispatchSlice(const char* slice, size_t len) {
   ssize_t rc = http_parser_execute(&parser_, &settings_, slice, len);
   if (HTTP_PARSER_ERRNO(&parser_) != HPE_OK && HTTP_PARSER_ERRNO(&parser_) != HPE_PAUSED) {
-    sendProtocolError();
+    sendProtocolError(Http1ResponseCodeDetails::get().HttpCodecError);
     throw CodecProtocolException("http/1.1 protocol error: " +
                                  std::string(http_errno_name(HTTP_PARSER_ERRNO(&parser_))));
   }
@@ -477,7 +488,7 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) {
     if (!Http::HeaderUtility::headerIsValid(header_value)) {
       ENVOY_CONN_LOG(debug, "invalid header value: {}", connection_, header_value);
       error_code_ = Http::Code::BadRequest;
-      sendProtocolError();
+      sendProtocolError(Http1ResponseCodeDetails::get().InvalidCharacters);
       throw CodecProtocolException("http/1.1 protocol error: header value contains invalid chars");
     }
   } else if (header_value.find('\0') != absl::string_view::npos) {
@@ -496,7 +507,7 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) {
   if (total > (max_headers_kb_ * 1024)) {
 
     error_code_ = Http::Code::RequestHeaderFieldsTooLarge;
-    sendProtocolError();
+    sendProtocolError(Http1ResponseCodeDetails::get().HeadersTooLarge);
     throw CodecProtocolException("headers size exceeds limit");
   }
 }
@@ -539,7 +550,8 @@ int ConnectionImpl::onHeadersCompleteBase() {
       ENVOY_CONN_LOG(debug, "Invalid nominated headers in Connection: {}", connection_,
                      header_value);
       error_code_ = Http::Code::BadRequest;
-      sendProtocolError();
+      sendProtocolError(Http1ResponseCodeDetails::get().ConnectionHeaderSanitization);
+      throw CodecProtocolException("Invalid nominated headers in Connection.");
     }
   }
 
@@ -627,7 +639,7 @@ void ServerConnectionImpl::handlePath(HeaderMapImpl& headers, unsigned int metho
 
   Utility::Url absolute_url;
   if (!absolute_url.initialize(active_request_->request_url_.getStringView())) {
-    sendProtocolError();
+    sendProtocolError(Http1ResponseCodeDetails::get().InvalidUrl);
     throw CodecProtocolException("http/1.1 protocol error: invalid url in request line");
   }
   // RFC7230#5.7
@@ -734,7 +746,10 @@ void ServerConnectionImpl::onResetStream(StreamResetReason reason) {
   active_request_.reset();
 }
 
-void ServerConnectionImpl::sendProtocolError() {
+void ServerConnectionImpl::sendProtocolError(absl::string_view details) {
+  if (active_request_) {
+    active_request_->response_encoder_.setDetails(details);
+  }
   // We do this here because we may get a protocol error before we have a logical stream. Higher
   // layers can only operate on streams, so there is no coherent way to allow them to send an error
   // "out of band." On one hand this is kind of a hack but on the other hand it normalizes HTTP/1.1
@@ -871,6 +886,12 @@ void ClientConnectionImpl::onResetStream(StreamResetReason reason) {
   }
 }
 
+void ClientConnectionImpl::sendProtocolError(absl::string_view details) {
+  if (request_encoder_) {
+    request_encoder_->setDetails(details);
+  }
+}
+
 void ClientConnectionImpl::onAboveHighWatermark() {
   // This should never happen without an active stream/request.
   ASSERT(!pending_responses_.empty());

source/common/http/http1/codec_impl.h

@@ -60,8 +60,10 @@ class StreamEncoderImpl : public StreamEncoder,
   void resetStream(StreamResetReason reason) override;
   void readDisable(bool disable) override;
   uint32_t bufferLimit() override;
+  absl::string_view responseDetails() override { return details_; }
 
   void isResponseToHeadRequest(bool value) { is_response_to_head_request_ = value; }
+  void setDetails(absl::string_view details) { details_ = details; }
 
 protected:
   StreamEncoderImpl(ConnectionImpl& connection, HeaderKeyFormatter* header_key_formatter);
@@ -101,6 +103,7 @@ class StreamEncoderImpl : public StreamEncoder,
   bool processing_100_continue_ : 1;
   bool is_response_to_head_request_ : 1;
   bool is_content_length_allowed_ : 1;
+  absl::string_view details_;
 };
 
 /**
@@ -282,7 +285,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
   /**
    * Send a protocol error response to remote.
    */
-  virtual void sendProtocolError() PURE;
+  virtual void sendProtocolError(absl::string_view details = "") PURE;
 
   /**
    * Called when output_buffer_ or the underlying connection go from below a low watermark to over
@@ -355,7 +358,7 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
   void onBody(const char* data, size_t length) override;
   void onMessageComplete() override;
   void onResetStream(StreamResetReason reason) override;
-  void sendProtocolError() override;
+  void sendProtocolError(absl::string_view details) override;
   void onAboveHighWatermark() override;
   void onBelowLowWatermark() override;
 
@@ -395,7 +398,7 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl {
   void onBody(const char* data, size_t length) override;
   void onMessageComplete() override;
   void onResetStream(StreamResetReason reason) override;
-  void sendProtocolError() override {}
+  void sendProtocolError(absl::string_view details) override;
   void onAboveHighWatermark() override;
   void onBelowLowWatermark() override;
 

test/common/http/http1/codec_impl_test.cc

@@ -57,8 +57,9 @@ class Http1ServerConnectionImplTest : public testing::Test {
 
   void expectHeadersTest(Protocol p, bool allow_absolute_url, Buffer::OwnedImpl& buffer,
                          TestHeaderMapImpl& expected_headers);
-  void expect400(Protocol p, bool allow_absolute_url, Buffer::OwnedImpl& buffer);
-  void testRequestHeadersExceedLimit(std::string header_string);
+  void expect400(Protocol p, bool allow_absolute_url, Buffer::OwnedImpl& buffer,
+                 absl::string_view details = "");
+  void testRequestHeadersExceedLimit(std::string header_string, absl::string_view details = "");
   void testRequestHeadersAccepted(std::string header_string);
 
   // Send the request, and validate the received request headers.
@@ -87,7 +88,8 @@ class Http1ServerConnectionImplTest : public testing::Test {
 };
 
 void Http1ServerConnectionImplTest::expect400(Protocol p, bool allow_absolute_url,
-                                              Buffer::OwnedImpl& buffer) {
+                                              Buffer::OwnedImpl& buffer,
+                                              absl::string_view details) {
   InSequence sequence;
 
   std::string output;
@@ -101,11 +103,19 @@ void Http1ServerConnectionImplTest::expect400(Protocol p, bool allow_absolute_ur
   }
 
   Http::MockStreamDecoder decoder;
-  EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder));
+  Http::StreamEncoder* response_encoder = nullptr;
+  EXPECT_CALL(callbacks_, newStream(_, _))
+      .WillOnce(Invoke([&](Http::StreamEncoder& encoder, bool) -> Http::StreamDecoder& {
+        response_encoder = &encoder;
+        return decoder;
+      }));
 
   EXPECT_THROW(codec_->dispatch(buffer), CodecProtocolException);
   EXPECT_EQ("HTTP/1.1 400 Bad Request\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", output);
   EXPECT_EQ(p, codec_->protocol());
+  if (!details.empty()) {
+    EXPECT_EQ(details, response_encoder->getStream().responseDetails());
+  }
 }
 
 void Http1ServerConnectionImplTest::expectHeadersTest(Protocol p, bool allow_absolute_url,
@@ -130,7 +140,8 @@ void Http1ServerConnectionImplTest::expectHeadersTest(Protocol p, bool allow_abs
   EXPECT_EQ(p, codec_->protocol());
 }
 
-void Http1ServerConnectionImplTest::testRequestHeadersExceedLimit(std::string header_string) {
+void Http1ServerConnectionImplTest::testRequestHeadersExceedLimit(std::string header_string,
+                                                                  absl::string_view details) {
   initialize();
 
   std::string exception_reason;
@@ -146,6 +157,9 @@ void Http1ServerConnectionImplTest::testRequestHeadersExceedLimit(std::string he
   codec_->dispatch(buffer);
   buffer = Buffer::OwnedImpl(header_string + "\r\n");
   EXPECT_THROW_WITH_MESSAGE(codec_->dispatch(buffer), EnvoyException, "headers size exceeds limit");
+  if (!details.empty()) {
+    EXPECT_EQ(details, response_encoder->getStream().responseDetails());
+  }
 }
 
 void Http1ServerConnectionImplTest::testRequestHeadersAccepted(std::string header_string) {
@@ -323,7 +337,7 @@ TEST_F(Http1ServerConnectionImplTest, Http11InvalidRequest) {
 
   // Invalid because www.somewhere.com is not an absolute path nor an absolute url
   Buffer::OwnedImpl buffer("GET www.somewhere.com HTTP/1.1\r\nHost: bah\r\n\r\n");
-  expect400(Protocol::Http11, true, buffer);
+  expect400(Protocol::Http11, true, buffer, "http1.codec_error");
 }
 
 TEST_F(Http1ServerConnectionImplTest, Http11AbsolutePathNoSlash) {
@@ -339,7 +353,7 @@ TEST_F(Http1ServerConnectionImplTest, Http11AbsolutePathBad) {
   initialize();
 
   Buffer::OwnedImpl buffer("GET * HTTP/1.1\r\nHost: bah\r\n\r\n");
-  expect400(Protocol::Http11, true, buffer);
+  expect400(Protocol::Http11, true, buffer, "http1.invalid_url");
 }
 
 TEST_F(Http1ServerConnectionImplTest, Http11AbsolutePortTooLarge) {
@@ -349,6 +363,14 @@ TEST_F(Http1ServerConnectionImplTest, Http11AbsolutePortTooLarge) {
   expect400(Protocol::Http11, true, buffer);
 }
 
+TEST_F(Http1ServerConnectionImplTest, SketchyConnectionHeader) {
+  initialize();
+
+  Buffer::OwnedImpl buffer(
+      "GET / HTTP/1.1\r\nHost: bah\r\nConnection: a,b,c,d,e,f,g,h,i,j,k,l,m\r\n\r\n");
+  expect400(Protocol::Http11, true, buffer, "http1.connection_header_rejected");
+}
+
 TEST_F(Http1ServerConnectionImplTest, Http11RelativeOnly) {
   initialize();
 
@@ -474,12 +496,17 @@ TEST_F(Http1ServerConnectionImplTest, HeaderInvalidCharsRejection) {
   initialize();
 
   Http::MockStreamDecoder decoder;
-  EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder));
-
+  Http::StreamEncoder* response_encoder = nullptr;
+  EXPECT_CALL(callbacks_, newStream(_, _))
+      .WillOnce(Invoke([&](Http::StreamEncoder& encoder, bool) -> Http::StreamDecoder& {
+        response_encoder = &encoder;
+        return decoder;
+      }));
   Buffer::OwnedImpl buffer(
       absl::StrCat("GET / HTTP/1.1\r\nHOST: h.com\r\nfoo: ", std::string(1, 3), "\r\n"));
   EXPECT_THROW_WITH_MESSAGE(codec_->dispatch(buffer), CodecProtocolException,
                             "http/1.1 protocol error: header value contains invalid chars");
+  EXPECT_EQ("http1.invalid_characters", response_encoder->getStream().responseDetails());
 }
 
 // Regression test for http-parser allowing embedded NULs in header values,
@@ -1402,7 +1429,7 @@ TEST_F(Http1ServerConnectionImplTest, LargeRequestHeadersRejected) {
 // Tests that the default limit for the number of request headers is 100.
 TEST_F(Http1ServerConnectionImplTest, ManyRequestHeadersRejected) {
   // Send a request with 101 headers.
-  testRequestHeadersExceedLimit(createHeaderFragment(101));
+  testRequestHeadersExceedLimit(createHeaderFragment(101), "http1.too_many_headers");
 }
 
 TEST_F(Http1ServerConnectionImplTest, LargeRequestHeadersSplitRejected) {
@@ -1428,6 +1455,7 @@ TEST_F(Http1ServerConnectionImplTest, LargeRequestHeadersSplitRejected) {
   // the 60th 1kb header should induce overflow
   buffer = Buffer::OwnedImpl(fmt::format("big: {}\r\n", long_string));
   EXPECT_THROW_WITH_MESSAGE(codec_->dispatch(buffer), EnvoyException, "headers size exceeds limit");
+  EXPECT_EQ("http1.headers_too_large", response_encoder->getStream().responseDetails());
 }
 
 // Tests that the 101th request header causes overflow with the default max number of request

test/integration/integration_test.cc

@@ -336,11 +336,13 @@ TEST_P(IntegrationTest, BadFirstline) {
 }
 
 TEST_P(IntegrationTest, MissingDelimiter) {
+  useAccessLog("%RESPONSE_CODE_DETAILS%");
   initialize();
   std::string response;
   sendRawHttpAndWaitForResponse(lookupPort("http"),
                                 "GET / HTTP/1.1\r\nHost: host\r\nfoo bar\r\n\r\n", &response);
   EXPECT_EQ("HTTP/1.1 400 Bad Request\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", response);
+  EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("http1.codec_error"));
 }
 
 TEST_P(IntegrationTest, InvalidCharacterInFirstline) {