diff --git a/docs/root/configuration/http/http_conn_man/stats.rst b/docs/root/configuration/http/http_conn_man/stats.rst index 889adf2508f98..d587d91ea310f 100644 --- a/docs/root/configuration/http/http_conn_man/stats.rst +++ b/docs/root/configuration/http/http_conn_man/stats.rst @@ -131,6 +131,7 @@ All http2 statistics are rooted at *http2.* inbound_empty_frames_flood, Counter, Total number of connections terminated for exceeding the limit on consecutive inbound frames with an empty payload and no end stream flag. The limit is configured by setting the :ref:`max_consecutive_inbound_frames_with_empty_payload config setting `. inbound_priority_frames_flood, Counter, Total number of connections terminated for exceeding the limit on inbound frames of type PRIORITY. The limit is configured by setting the :ref:`max_inbound_priority_frames_per_stream config setting `. inbound_window_update_frames_flood, Counter, Total number of connections terminated for exceeding the limit on inbound frames of type WINDOW_UPDATE. The limit is configured by setting the :ref:`max_inbound_window_updateframes_per_data_frame_sent config setting `. + metadata_empty_frames, Counter, Total number of metadata frames that were received and contained empty maps. outbound_flood, Counter, Total number of connections terminated for exceeding the limit on outbound frames of all types. The limit is configured by setting the :ref:`max_outbound_frames config setting `. outbound_control_flood, Counter, "Total number of connections terminated for exceeding the limit on outbound frames of types PING, SETTINGS and RST_STREAM. The limit is configured by setting the :ref:`max_outbound_control_frames config setting `." requests_rejected_with_underscores_in_headers, Counter, Total numbers of rejected requests due to header names containing underscores. This action is configured by setting the :ref:`headers_with_underscores_action config setting `. diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc index 51196a8a8fac6..d7aa333e29cee 100644 --- a/source/common/grpc/common.cc +++ b/source/common/grpc/common.cc @@ -166,12 +166,18 @@ Common::getGrpcTimeout(const Http::RequestHeaderMap& request_headers) { const Http::HeaderEntry* header_grpc_timeout_entry = request_headers.GrpcTimeout(); std::chrono::milliseconds timeout; if (header_grpc_timeout_entry) { - uint64_t grpc_timeout; - // TODO(dnoe): Migrate to pure string_view (#6580) - std::string grpc_timeout_string(header_grpc_timeout_entry->value().getStringView()); - const char* unit = StringUtil::strtoull(grpc_timeout_string.c_str(), grpc_timeout); - if (unit != nullptr && *unit != '\0') { - switch (*unit) { + int64_t grpc_timeout; + absl::string_view timeout_entry = header_grpc_timeout_entry->value().getStringView(); + if (timeout_entry.empty()) { + // Must be of the form TimeoutValue TimeoutUnit. See + // https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#requests. + return absl::nullopt; + } + // TimeoutValue must be a positive integer of at most 8 digits. + if (absl::SimpleAtoi(timeout_entry.substr(0, timeout_entry.size() - 1), &grpc_timeout) && + grpc_timeout >= 0 && static_cast(grpc_timeout) <= MAX_GRPC_TIMEOUT_VALUE) { + const char unit = timeout_entry[timeout_entry.size() - 1]; + switch (unit) { case 'H': return std::chrono::hours(grpc_timeout); case 'M': diff --git a/source/common/grpc/common.h b/source/common/grpc/common.h index f76082610f317..dcade2ad8015a 100644 --- a/source/common/grpc/common.h +++ b/source/common/grpc/common.h @@ -178,6 +178,8 @@ class Common { private: static void checkForHeaderOnlyError(Http::ResponseMessage& http_response); + + static constexpr size_t MAX_GRPC_TIMEOUT_VALUE = 99999999; }; } // namespace Grpc diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 68adc01286039..db5f24bb1ff08 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -545,7 +545,13 @@ MetadataDecoder& ConnectionImpl::StreamImpl::getMetadataDecoder() { } void ConnectionImpl::StreamImpl::onMetadataDecoded(MetadataMapPtr&& metadata_map_ptr) { - decoder().decodeMetadata(std::move(metadata_map_ptr)); + // Empty metadata maps should not be decoded. + if (metadata_map_ptr->empty()) { + ENVOY_CONN_LOG(debug, "decode metadata called with empty map, skipping", parent_.connection_); + parent_.stats_.metadata_empty_frames_.inc(); + } else { + decoder().decodeMetadata(std::move(metadata_map_ptr)); + } } ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stats, diff --git a/source/common/http/http2/codec_impl_legacy.cc b/source/common/http/http2/codec_impl_legacy.cc index 354fb689752c4..e0c417f5d81c0 100644 --- a/source/common/http/http2/codec_impl_legacy.cc +++ b/source/common/http/http2/codec_impl_legacy.cc @@ -525,7 +525,13 @@ MetadataDecoder& ConnectionImpl::StreamImpl::getMetadataDecoder() { } void ConnectionImpl::StreamImpl::onMetadataDecoded(MetadataMapPtr&& metadata_map_ptr) { - decoder().decodeMetadata(std::move(metadata_map_ptr)); + // Empty metadata maps should not be decoded. + if (metadata_map_ptr->empty()) { + ENVOY_CONN_LOG(debug, "decode metadata called with empty map, skipping", parent_.connection_); + parent_.stats_.metadata_empty_frames_.inc(); + } else { + decoder().decodeMetadata(std::move(metadata_map_ptr)); + } } ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stats, diff --git a/source/common/http/http2/codec_stats.h b/source/common/http/http2/codec_stats.h index ba9d1592095bb..eb11291f82f4b 100644 --- a/source/common/http/http2/codec_stats.h +++ b/source/common/http/http2/codec_stats.h @@ -19,6 +19,7 @@ namespace Http2 { COUNTER(inbound_empty_frames_flood) \ COUNTER(inbound_priority_frames_flood) \ COUNTER(inbound_window_update_frames_flood) \ + COUNTER(metadata_empty_frames) \ COUNTER(outbound_control_flood) \ COUNTER(outbound_flood) \ COUNTER(requests_rejected_with_underscores_in_headers) \ diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index 8be424e42d06f..fb8ae94026e91 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -296,9 +296,10 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c if (ctx.cert_chain_ == nullptr || !SSL_CTX_use_certificate(ctx.ssl_ctx_.get(), ctx.cert_chain_.get())) { while (uint64_t err = ERR_get_error()) { - ENVOY_LOG_MISC(debug, "SSL error: {}:{}:{}:{}", err, ERR_lib_error_string(err), - ERR_func_error_string(err), ERR_GET_REASON(err), - ERR_reason_error_string(err)); + ENVOY_LOG_MISC(debug, "SSL error: {}:{}:{}:{}", err, + absl::NullSafeStringView(ERR_lib_error_string(err)), + absl::NullSafeStringView(ERR_func_error_string(err)), ERR_GET_REASON(err), + absl::NullSafeStringView(ERR_reason_error_string(err))); } throw EnvoyException( absl::StrCat("Failed to load certificate chain from ", ctx.cert_chain_file_path_)); diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index f004947630406..0550b9c4cf719 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -216,9 +216,10 @@ void SslSocket::drainErrorQueue() { if (failure_reason_.empty()) { failure_reason_ = "TLS error:"; } - failure_reason_.append(absl::StrCat(" ", err, ":", ERR_lib_error_string(err), ":", - ERR_func_error_string(err), ":", - ERR_reason_error_string(err))); + failure_reason_.append(absl::StrCat(" ", err, ":", + absl::NullSafeStringView(ERR_lib_error_string(err)), ":", + absl::NullSafeStringView(ERR_func_error_string(err)), ":", + absl::NullSafeStringView(ERR_reason_error_string(err)))); } ENVOY_CONN_LOG(debug, "{}", callbacks_->connection(), failure_reason_); if (saw_error && !saw_counted_error) { diff --git a/test/common/grpc/common_test.cc b/test/common/grpc/common_test.cc index 3f6d88dd00963..c9b2e6d113fc1 100644 --- a/test/common/grpc/common_test.cc +++ b/test/common/grpc/common_test.cc @@ -78,6 +78,9 @@ TEST(GrpcContextTest, GetGrpcTimeout) { Http::TestRequestHeaderMapImpl missing_unit{{"grpc-timeout", "123"}}; EXPECT_EQ(absl::nullopt, Common::getGrpcTimeout(missing_unit)); + Http::TestRequestHeaderMapImpl small_missing_unit{{"grpc-timeout", "1"}}; + EXPECT_EQ(absl::nullopt, Common::getGrpcTimeout(small_missing_unit)); + Http::TestRequestHeaderMapImpl illegal_unit{{"grpc-timeout", "123F"}}; EXPECT_EQ(absl::nullopt, Common::getGrpcTimeout(illegal_unit)); diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index a022f18c2cd80..9b9c8a6cba00d 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -2281,6 +2281,19 @@ TEST_F(HttpConnectionManagerImplTest, DurationTimeout) { decoder_filters_[0]->callbacks_->clusterInfo(); } + // With an invalid gRPC timeout, refreshing cached route will not use header and use stream + // duration. + latched_headers->setGrpcTimeout("6666666666666H"); + { + // 25ms used already from previous case so timer is set to be 5ms. + EXPECT_CALL(*timer, enableTimer(std::chrono::milliseconds(5), _)); + EXPECT_CALL(route_config_provider_.route_config_->route_->route_entry_, maxStreamDuration()) + .Times(2) + .WillRepeatedly(Return(std::chrono::milliseconds(30))); + decoder_filters_[0]->callbacks_->clearRouteCache(); + decoder_filters_[0]->callbacks_->clusterInfo(); + } + // Cleanup. EXPECT_CALL(*timer, disableTimer()); EXPECT_CALL(*decoder_filters_[0], onStreamComplete()); diff --git a/test/common/http/http2/http2_frame.cc b/test/common/http/http2/http2_frame.cc index b29956144b4bd..66875946cc52c 100644 --- a/test/common/http/http2/http2_frame.cc +++ b/test/common/http/http2/http2_frame.cc @@ -227,7 +227,7 @@ Http2Frame Http2Frame::makeWindowUpdateFrame(uint32_t stream_index, uint32_t inc // Note: encoder in codebase persists multiple maps, with each map representing an individual frame. Http2Frame Http2Frame::makeMetadataFrameFromMetadataMap(uint32_t stream_index, - MetadataMap& metadata_map, + const MetadataMap& metadata_map, MetadataFlags flags) { const int numberOfNameValuePairs = metadata_map.size(); absl::FixedArray nameValues(numberOfNameValuePairs); diff --git a/test/common/http/http2/http2_frame.h b/test/common/http/http2/http2_frame.h index fc585815d8503..5759c69d66859 100644 --- a/test/common/http/http2/http2_frame.h +++ b/test/common/http/http2/http2_frame.h @@ -125,7 +125,7 @@ class Http2Frame { static Http2Frame makeWindowUpdateFrame(uint32_t stream_index, uint32_t increment); static Http2Frame makeMetadataFrameFromMetadataMap(uint32_t stream_index, - MetadataMap& metadata_map, + const MetadataMap& metadata_map, MetadataFlags flags); static Http2Frame makeMalformedRequest(uint32_t stream_index); diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 54365731d78f8..cca89f70f7eb5 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -5682,6 +5682,16 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { EXPECT_EQ("5", headers.get_("x-envoy-expected-rq-timeout-ms")); EXPECT_EQ("5m", headers.get_("grpc-timeout")); } + { + NiceMock route; + EXPECT_CALL(route, maxGrpcTimeout()) + .WillRepeatedly(Return(absl::optional(10000))); + Http::TestRequestHeaderMapImpl headers{{"content-type", "application/grpc"}, + {"grpc-timeout", "6666666666666H"}}; + FilterUtility::finalTimeout(route, headers, true, true, false, false); + EXPECT_EQ("10000", headers.get_("x-envoy-expected-rq-timeout-ms")); + EXPECT_EQ("10000m", headers.get_("grpc-timeout")); + } { NiceMock route; EXPECT_CALL(route, timeout()).WillOnce(Return(std::chrono::milliseconds(10))); diff --git a/test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc b/test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc index ad7a5e0df9e7c..39cc8fade20fd 100644 --- a/test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc +++ b/test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc @@ -18,6 +18,7 @@ #include "extensions/transport_sockets/tls/context_config_impl.h" #include "extensions/transport_sockets/tls/context_manager_impl.h" +#include "extensions/transport_sockets/tls/ssl_handshaker.h" #include "test/integration/autonomous_upstream.h" #include "test/integration/integration.h" @@ -89,6 +90,36 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, SslIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); +// Test that Envoy behaves correctly when receiving an SSLAlert for an unspecified code. The codes +// are defined in the standard, and assigned codes have a string associated with them in BoringSSL, +// which is included in logs. For an unknown code, verify that no crash occurs. +TEST_P(SslIntegrationTest, UnknownSslAlert) { + initialize(); + Network::ClientConnectionPtr connection = makeSslClientConnection({}); + ConnectionStatusCallbacks callbacks; + connection->addConnectionCallbacks(callbacks); + connection->connect(); + while (!callbacks.connected()) { + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + } + + Ssl::ConnectionInfoConstSharedPtr ssl_info = connection->ssl(); + SSL* ssl = + dynamic_cast(ssl_info.get()) + ->ssl(); + ASSERT_EQ(connection->state(), Network::Connection::State::Open); + ASSERT_NE(ssl, nullptr); + SSL_send_fatal_alert(ssl, 255); + while (!callbacks.closed()) { + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + } + + const std::string counter_name = listenerStatPrefix("ssl.connection_error"); + Stats::CounterSharedPtr counter = test_server_->counter(counter_name); + test_server_->waitForCounterGe(counter_name, 1); + connection->close(Network::ConnectionCloseType::NoFlush); +} + TEST_P(SslIntegrationTest, RouterRequestAndResponseWithGiantBodyBuffer) { ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { return makeSslClientConnection({}); diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index d08a282e66a44..0ade6f704b523 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -181,6 +181,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyMetadataInResponse) { response->waitForEndStream(); ASSERT_TRUE(response->complete()); EXPECT_EQ(response->metadataMap().find(key)->second, value); + EXPECT_EQ(1, response->metadataMapsDecodedCount()); // Sends the second request. response = codec_client_->makeRequestWithBody(default_request_headers_, 10); @@ -200,6 +201,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyMetadataInResponse) { response->waitForEndStream(); ASSERT_TRUE(response->complete()); EXPECT_EQ(response->metadataMap().find(key)->second, value); + EXPECT_EQ(1, response->metadataMapsDecodedCount()); // Sends the third request. response = codec_client_->makeRequestWithBody(default_request_headers_, 10); @@ -219,6 +221,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyMetadataInResponse) { response->waitForEndStream(); ASSERT_TRUE(response->complete()); EXPECT_EQ(response->metadataMap().find(key)->second, value); + EXPECT_EQ(1, response->metadataMapsDecodedCount()); // Sends the fourth request. response = codec_client_->makeRequestWithBody(default_request_headers_, 10); @@ -239,6 +242,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyMetadataInResponse) { response->waitForEndStream(); ASSERT_TRUE(response->complete()); EXPECT_EQ(response->metadataMap().find(key)->second, value); + EXPECT_EQ(1, response->metadataMapsDecodedCount()); // Sends the fifth request. response = codec_client_->makeRequestWithBody(default_request_headers_, 10); @@ -259,6 +263,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyMetadataInResponse) { response->waitForEndStream(); ASSERT_TRUE(response->complete()); EXPECT_EQ(response->metadataMap().find(key)->second, value); + EXPECT_EQ(1, response->metadataMapsDecodedCount()); // Sends the sixth request. response = codec_client_->makeRequestWithBody(default_request_headers_, 10); @@ -309,6 +314,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyMultipleMetadata) { // Verifies multiple metadata are received by the client. response->waitForEndStream(); ASSERT_TRUE(response->complete()); + EXPECT_EQ(4, response->metadataMapsDecodedCount()); for (int i = 0; i < size; i++) { for (const auto& metadata : *multiple_vecs[i][0]) { EXPECT_EQ(response->metadataMap().find(metadata.first)->second, metadata.second); @@ -342,6 +348,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyInvalidMetadata) { // Verifies metadata is not received by the client. response->waitForEndStream(); ASSERT_TRUE(response->complete()); + EXPECT_EQ(0, response->metadataMapsDecodedCount()); EXPECT_EQ(response->metadataMap().size(), 0); } @@ -383,6 +390,7 @@ TEST_P(Http2MetadataIntegrationTest, TestResponseMetadata) { expected_metadata_keys.insert("data"); verifyExpectedMetadata(response->metadataMap(), expected_metadata_keys); EXPECT_EQ(response->keyCount("duplicate"), 2); + EXPECT_EQ(2, response->metadataMapsDecodedCount()); // Upstream responds with headers, data and trailers. response = codec_client_->makeRequestWithBody(default_request_headers_, 10); @@ -397,6 +405,7 @@ TEST_P(Http2MetadataIntegrationTest, TestResponseMetadata) { expected_metadata_keys.insert("trailers"); verifyExpectedMetadata(response->metadataMap(), expected_metadata_keys); EXPECT_EQ(response->keyCount("duplicate"), 3); + EXPECT_EQ(4, response->metadataMapsDecodedCount()); // Upstream responds with headers, 100-continue and data. response = @@ -419,6 +428,7 @@ TEST_P(Http2MetadataIntegrationTest, TestResponseMetadata) { expected_metadata_keys.insert("100-continue"); verifyExpectedMetadata(response->metadataMap(), expected_metadata_keys); EXPECT_EQ(response->keyCount("duplicate"), 4); + EXPECT_EQ(4, response->metadataMapsDecodedCount()); // Upstream responds with headers and metadata that will not be consumed. response = codec_client_->makeRequestWithBody(default_request_headers_, 10); @@ -437,6 +447,7 @@ TEST_P(Http2MetadataIntegrationTest, TestResponseMetadata) { expected_metadata_keys.insert("aaa"); expected_metadata_keys.insert("keep"); verifyExpectedMetadata(response->metadataMap(), expected_metadata_keys); + EXPECT_EQ(2, response->metadataMapsDecodedCount()); // Upstream responds with headers, data and metadata that will be consumed. response = codec_client_->makeRequestWithBody(default_request_headers_, 10); @@ -456,6 +467,7 @@ TEST_P(Http2MetadataIntegrationTest, TestResponseMetadata) { expected_metadata_keys.insert("replace"); verifyExpectedMetadata(response->metadataMap(), expected_metadata_keys); EXPECT_EQ(response->keyCount("duplicate"), 2); + EXPECT_EQ(3, response->metadataMapsDecodedCount()); } TEST_P(Http2MetadataIntegrationTest, ProxyMultipleMetadataReachSizeLimit) { diff --git a/test/integration/integration_stream_decoder.cc b/test/integration/integration_stream_decoder.cc index 6ac864cdabbb1..0b7566675317a 100644 --- a/test/integration/integration_stream_decoder.cc +++ b/test/integration/integration_stream_decoder.cc @@ -97,6 +97,7 @@ void IntegrationStreamDecoder::decodeTrailers(Http::ResponseTrailerMapPtr&& trai } void IntegrationStreamDecoder::decodeMetadata(Http::MetadataMapPtr&& metadata_map) { + metadata_maps_decoded_count_++; // Combines newly received metadata with the existing metadata. for (const auto& metadata : *metadata_map) { duplicated_metadata_key_count_[metadata.first]++; diff --git a/test/integration/integration_stream_decoder.h b/test/integration/integration_stream_decoder.h index 888a4ad266000..99486c78b7ce0 100644 --- a/test/integration/integration_stream_decoder.h +++ b/test/integration/integration_stream_decoder.h @@ -29,6 +29,7 @@ class IntegrationStreamDecoder : public Http::ResponseDecoder, public Http::Stre const Http::ResponseTrailerMapPtr& trailers() { return trailers_; } const Http::MetadataMap& metadataMap() { return *metadata_map_; } uint64_t keyCount(std::string key) { return duplicated_metadata_key_count_[key]; } + uint32_t metadataMapsDecodedCount() const { return metadata_maps_decoded_count_; } void waitForContinueHeaders(); void waitForHeaders(); // This function waits until body_ has at least size bytes in it (it might have more). clearBody() @@ -70,6 +71,7 @@ class IntegrationStreamDecoder : public Http::ResponseDecoder, public Http::Stre bool waiting_for_headers_{}; bool saw_reset_{}; Http::StreamResetReason reset_reason_{}; + uint32_t metadata_maps_decoded_count_{}; }; using IntegrationStreamDecoderPtr = std::unique_ptr; diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index 1683a3db13dc6..fa7826d0bdf83 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -288,7 +288,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSize) { // https://github.com/envoyproxy/envoy/issues/12209 // EXPECT_MEMORY_EQ(m_per_cluster, 37061); } - EXPECT_MEMORY_LE(m_per_cluster, 39350); // Round up to allow platform variations. + EXPECT_MEMORY_LE(m_per_cluster, 39400); // Round up to allow platform variations. } TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) {