http: moving authority checks to HTTP/1 codec (envoyproxy#9330)

Removing duplicate checks for the HTTP/2 path by doing authority checks in the HTTP/1 codec.
Also doing some cleanup from the setec review branch and leveraging the new HTTP/1 codec details work.

Risk Level: Medium (touches data plane, no functional changes expected)
Testing: new codec unit test. Integration test continues to validate
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

include/envoy/stream_info/stream_info.h

@@ -137,8 +137,6 @@ struct ResponseCodeDetailValues {
   // indicates that original "success" headers may have been sent downstream
   // despite the subsequent failure.
   const std::string LateUpstreamReset = "upstream_reset_after_response_started";
-  // The request was rejected due to invalid characters in the HOST/:authority header.
-  const std::string InvalidAuthority = "invalid_authority";
 };
 
 using ResponseCodeDetails = ConstSingleton<ResponseCodeDetailValues>;

source/common/http/BUILD

@@ -352,6 +352,7 @@ envoy_cc_library(
         "//source/common/common:regex_lib",
         "//source/common/common:utility_lib",
         "//source/common/protobuf:utility_lib",
+        "//source/common/runtime:runtime_lib",
         "@envoy_api//envoy/config/route/v3alpha:pkg_cc_proto",
         "@envoy_api//envoy/type/v3alpha:pkg_cc_proto",
     ],

source/common/http/conn_manager_impl.cc

@@ -774,14 +774,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
     }
   }
 
-  // Make sure the host is valid.
-  if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.strict_authority_validation") &&
-      !HeaderUtility::authorityIsValid(request_headers_->Host()->value().getStringView())) {
-    sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, "",
-                   nullptr, is_head_request_, absl::nullopt,
-                   StreamInfo::ResponseCodeDetails::get().InvalidAuthority);
-    return;
-  }
+  // Verify header sanity checks which should have been performed by the codec.
+  ASSERT(HeaderUtility::requestHeadersValid(*request_headers_).has_value() == false);
 
   // Currently we only support relative paths at the application layer. We expect the codec to have
   // broken the path into pieces if applicable. NOTE: Currently the HTTP/1.1 codec only does this

source/common/http/header_utility.cc

@@ -6,13 +6,20 @@
 #include "common/common/utility.h"
 #include "common/http/header_map_impl.h"
 #include "common/protobuf/utility.h"
+#include "common/runtime/runtime_impl.h"
 
 #include "absl/strings/match.h"
 #include "nghttp2/nghttp2.h"
 
 namespace Envoy {
 namespace Http {
 
+struct SharedResponseCodeDetailsValues {
+  const absl::string_view InvalidAuthority = "http.invalid_authority";
+};
+
+using SharedResponseCodeDetails = ConstSingleton<SharedResponseCodeDetailsValues>;
+
 // HeaderMatcher will consist of:
 //   header_match_specifier which can be any one of exact_match, regex_match, range_match,
 //   present_match, prefix_match or suffix_match.
@@ -136,13 +143,13 @@ bool HeaderUtility::matchHeaders(const HeaderMap& request_headers, const HeaderD
 }
 
 bool HeaderUtility::headerIsValid(const absl::string_view header_value) {
-  return (nghttp2_check_header_value(reinterpret_cast<const uint8_t*>(header_value.data()),
-                                     header_value.size()) != 0);
+  return nghttp2_check_header_value(reinterpret_cast<const uint8_t*>(header_value.data()),
+                                    header_value.size()) != 0;
 }
 
 bool HeaderUtility::authorityIsValid(const absl::string_view header_value) {
-  return (nghttp2_check_authority(reinterpret_cast<const uint8_t*>(header_value.data()),
-                                  header_value.size()) != 0);
+  return nghttp2_check_authority(reinterpret_cast<const uint8_t*>(header_value.data()),
+                                 header_value.size()) != 0;
 }
 
 void HeaderUtility::addHeaders(HeaderMap& headers, const HeaderMap& headers_to_add) {
@@ -164,5 +171,15 @@ bool HeaderUtility::isEnvoyInternalRequest(const HeaderMap& headers) {
          internal_request_header->value() == Headers::get().EnvoyInternalRequestValues.True;
 }
 
+absl::optional<std::reference_wrapper<const absl::string_view>>
+HeaderUtility::requestHeadersValid(const HeaderMap& headers) {
+  // Make sure the host is valid.
+  if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.strict_authority_validation") &&
+      headers.Host() && !HeaderUtility::authorityIsValid(headers.Host()->value().getStringView())) {
+    return SharedResponseCodeDetails::get().InvalidAuthority;
+  }
+  return absl::nullopt;
+}
+
 } // namespace Http
 } // namespace Envoy

source/common/http/header_utility.h

@@ -115,6 +115,14 @@ class HeaderUtility {
    * @brief a helper function to determine if the headers represent an envoy internal request
    */
   static bool isEnvoyInternalRequest(const HeaderMap& headers);
+
+  /**
+   * Determines if request headers pass Envoy validity checks.
+   * @param headers to validate
+   * @return details of the error if an error is present, otherwise absl::nullopt
+   */
+  static absl::optional<std::reference_wrapper<const absl::string_view>>
+  requestHeadersValid(const HeaderMap& headers);
 };
 } // namespace Http
 } // namespace Envoy

source/common/http/http1/codec_impl.cc

@@ -753,6 +753,14 @@ int ServerConnectionImpl::onHeadersComplete(HeaderMapImplPtr&& headers) {
 
     headers->setMethod(method_string);
 
+    // Make sure the host is valid.
+    auto details = HeaderUtility::requestHeadersValid(*headers);
+    if (details.has_value()) {
+      sendProtocolError(details.value().get());
+      throw CodecProtocolException(
+          "http/1.1 protocol error: request headers failed spec compliance checks");
+    }
+
     // Determine here whether we have a body or not. This uses the new RFC semantics where the
     // presence of content-length or chunked transfer-encoding indicates a body vs. a particular
     // method. If there is no body, we defer raising decodeHeaders() until the parser is flushed

test/common/http/header_utility_test.cc

@@ -464,11 +464,9 @@ TEST(HeaderIsValidTest, ValidHeaderValuesAreAccepted) {
   EXPECT_TRUE(HeaderUtility::headerIsValid("Some Other Value"));
 }
 
-TEST(HeaderIsValidTest, AuthIsValid) {
+TEST(HeaderIsValidTest, AuthorityIsValid) {
   EXPECT_TRUE(HeaderUtility::authorityIsValid("strangebutlegal$-%&'"));
   EXPECT_FALSE(HeaderUtility::authorityIsValid("illegal{}"));
-  // Full checks are done by Http2CodecImplTest.CheckAuthority, cross checking
-  // against nghttp2 compliance.
 }
 
 TEST(HeaderAddTest, HeaderAdd) {

test/common/http/http1/codec_impl_test.cc

@@ -666,6 +666,25 @@ TEST_F(Http1ServerConnectionImplTest, HeaderInvalidCharsRejection) {
   EXPECT_EQ("http1.invalid_characters", response_encoder->getStream().responseDetails());
 }
 
+TEST_F(Http1ServerConnectionImplTest, HeaderInvalidAuthority) {
+  TestScopedRuntime scoped_runtime;
+
+  initialize();
+
+  Http::MockStreamDecoder 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\n\r\n"));
+  EXPECT_THROW_WITH_MESSAGE(
+      codec_->dispatch(buffer), CodecProtocolException,
+      "http/1.1 protocol error: request headers failed spec compliance checks");
+  EXPECT_EQ("http.invalid_authority", response_encoder->getStream().responseDetails());
+}
+
 // Regression test for http-parser allowing embedded NULs in header values,
 // verify we reject them.
 TEST_F(Http1ServerConnectionImplTest, HeaderEmbeddedNulRejection) {

test/integration/protocol_integration_test.cc

@@ -1474,7 +1474,7 @@ TEST_P(ProtocolIntegrationTest, ConnDurationTimeoutNoHttpRequest) {
 }
 
 // Make sure that invalid authority headers get blocked at or before the HCM.
-TEST_P(DownstreamProtocolIntegrationTest, InvalidAuth) {
+TEST_P(DownstreamProtocolIntegrationTest, InvalidAuthority) {
   initialize();
   fake_upstreams_[0]->set_allow_unexpected_disconnects(true);