diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index 941375ee0624..a0f4f7a743b6 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/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; diff --git a/source/common/http/BUILD b/source/common/http/BUILD index ff4e126389cb..dfd6e9b468c2 100644 --- a/source/common/http/BUILD +++ b/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", ], diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index f0a8c10b2a4c..ca3eae934670 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/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 diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index 3f200b090ce3..c37ecfe28d06 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -6,6 +6,7 @@ #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" @@ -13,6 +14,12 @@ namespace Envoy { namespace Http { +struct SharedResponseCodeDetailsValues { + const absl::string_view InvalidAuthority = "http.invalid_authority"; +}; + +using SharedResponseCodeDetails = ConstSingleton; + // 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(header_value.data()), - header_value.size()) != 0); + return nghttp2_check_header_value(reinterpret_cast(header_value.data()), + header_value.size()) != 0; } bool HeaderUtility::authorityIsValid(const absl::string_view header_value) { - return (nghttp2_check_authority(reinterpret_cast(header_value.data()), - header_value.size()) != 0); + return nghttp2_check_authority(reinterpret_cast(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> +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 diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index e0d7916117bc..21acfcd546af 100644 --- a/source/common/http/header_utility.h +++ b/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> + requestHeadersValid(const HeaderMap& headers); }; } // namespace Http } // namespace Envoy diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 09481c28a078..5faf8b1ee1e6 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/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 diff --git a/test/common/http/header_utility_test.cc b/test/common/http/header_utility_test.cc index d66d953b0f54..73ca40944f75 100644 --- a/test/common/http/header_utility_test.cc +++ b/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) { diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index c36c2467148f..d1ad84afb984 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/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) { diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 599acf5fbfd0..4616df5fee25 100644 --- a/test/integration/protocol_integration_test.cc +++ b/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);