Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http3: respecting header number limits #15970

Merged
merged 17 commits into from
May 3, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -112,3 +112,6 @@ All http3 details are rooted at *http3.*
http3.invalid_header_field, One of the HTTP/3 headers was invalid
http3.headers_too_large, The size of headers (or trailers) exceeded the configured limits
http3.unexpected_underscore, Envoy was configured to drop or reject requests with header keys beginning with underscores.
http3.too_many_headers, Either incoming request or response headers contained too many headers.
http3.too_many_trailers, Either incoming request or response trailers contained too many entries.

2 changes: 1 addition & 1 deletion source/common/http/codec_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& conne
auto& quic_session = dynamic_cast<Quic::EnvoyQuicClientSession&>(*connection_);
codec_ = std::make_unique<Quic::QuicHttpClientConnectionImpl>(
quic_session, *this, host->cluster().http3CodecStats(), host->cluster().http3Options(),
Http::DEFAULT_MAX_REQUEST_HEADERS_KB);
Http::DEFAULT_MAX_REQUEST_HEADERS_KB, host->cluster().maxResponseHeadersCount());
// Initialize the session after max request header size is changed in above http client
// connection creation.
quic_session.Initialize();
Expand Down
6 changes: 4 additions & 2 deletions source/common/quic/codec_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,15 @@ QuicHttpServerConnectionImpl::QuicHttpServerConnectionImpl(
EnvoyQuicServerSession& quic_session, Http::ServerConnectionCallbacks& callbacks,
Http::Http3::CodecStats& stats,
const envoy::config::core::v3::Http3ProtocolOptions& http3_options,
const uint32_t max_request_headers_kb,
const uint32_t max_request_headers_kb, const uint32_t max_request_headers_count,
envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
headers_with_underscores_action)
: QuicHttpConnectionImplBase(quic_session, stats), quic_server_session_(quic_session) {
quic_session.setCodecStats(stats);
quic_session.setHttp3Options(http3_options);
quic_session.setHeadersWithUnderscoreAction(headers_with_underscores_action);
quic_session.setHttpConnectionCallbacks(callbacks);
quic_session.setMaxIncomingHeadersCount(max_request_headers_count);
quic_session.set_max_inbound_header_list_size(max_request_headers_kb * 1024u);
}

Expand Down Expand Up @@ -69,11 +70,12 @@ QuicHttpClientConnectionImpl::QuicHttpClientConnectionImpl(
EnvoyQuicClientSession& session, Http::ConnectionCallbacks& callbacks,
Http::Http3::CodecStats& stats,
const envoy::config::core::v3::Http3ProtocolOptions& http3_options,
const uint32_t max_request_headers_kb)
const uint32_t max_request_headers_kb, const uint32_t max_response_headers_count)
: QuicHttpConnectionImplBase(session, stats), quic_client_session_(session) {
session.setCodecStats(stats);
session.setHttp3Options(http3_options);
session.setHttpConnectionCallbacks(callbacks);
session.setMaxIncomingHeadersCount(max_response_headers_count);
session.set_max_inbound_header_list_size(max_request_headers_kb * 1024);
}

Expand Down
5 changes: 3 additions & 2 deletions source/common/quic/codec_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class QuicHttpServerConnectionImpl : public QuicHttpConnectionImplBase,
EnvoyQuicServerSession& quic_session, Http::ServerConnectionCallbacks& callbacks,
Http::Http3::CodecStats& stats,
const envoy::config::core::v3::Http3ProtocolOptions& http3_options,
const uint32_t max_request_headers_kb,
const uint32_t max_request_headers_kb, const uint32_t max_request_headers_count,
envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
headers_with_underscores_action);

Expand All @@ -63,7 +63,8 @@ class QuicHttpClientConnectionImpl : public QuicHttpConnectionImplBase,
QuicHttpClientConnectionImpl(EnvoyQuicClientSession& session,
Http::ConnectionCallbacks& callbacks, Http::Http3::CodecStats& stats,
const envoy::config::core::v3::Http3ProtocolOptions& http3_options,
const uint32_t max_request_headers_kb);
const uint32_t max_request_headers_kb,
const uint32_t max_response_headers_count);

// Http::ClientConnection
Http::RequestEncoder& newStream(Http::ResponseDecoder& response_decoder) override;
Expand Down
21 changes: 15 additions & 6 deletions source/common/quic/envoy_quic_client_stream.cc
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,8 @@ void EnvoyQuicClientStream::OnInitialHeadersComplete(bool fin, size_t frame_len,
}
quic::QuicSpdyStream::OnInitialHeadersComplete(fin, frame_len, header_list);
if (!headers_decompressed() || header_list.empty()) {
onStreamError(!http3_options_.override_stream_error_on_invalid_http_message().value());
onStreamError(!http3_options_.override_stream_error_on_invalid_http_message().value(),
quic::QUIC_BAD_APPLICATION_PAYLOAD);
return;
}

Expand All @@ -138,16 +139,18 @@ void EnvoyQuicClientStream::OnInitialHeadersComplete(bool fin, size_t frame_len,
end_stream_decoded_ = true;
}
std::unique_ptr<Http::ResponseHeaderMapImpl> headers =
quicHeadersToEnvoyHeaders<Http::ResponseHeaderMapImpl>(header_list, *this);
quicHeadersToEnvoyHeaders<Http::ResponseHeaderMapImpl>(
header_list, *this, filterManagerConnection()->maxIncomingHeadersCount(), details_);
if (headers == nullptr) {
onStreamError(close_connection_upon_invalid_header_);
onStreamError(close_connection_upon_invalid_header_, quic::QUIC_STREAM_EXCESSIVE_LOAD);
return;
}
const absl::optional<uint64_t> optional_status =
Http::Utility::getResponseStatusNoThrow(*headers);
if (!optional_status.has_value()) {
details_ = Http3ResponseCodeDetailValues::invalid_http_header;
onStreamError(!http3_options_.override_stream_error_on_invalid_http_message().value());
onStreamError(!http3_options_.override_stream_error_on_invalid_http_message().value(),
quic::QUIC_BAD_APPLICATION_PAYLOAD);
return;
}
const uint64_t status = optional_status.value();
Expand Down Expand Up @@ -244,6 +247,11 @@ void EnvoyQuicClientStream::maybeDecodeTrailers() {
if (sequencer()->IsClosed() && !FinishedReadingTrailers()) {
// Only decode trailers after finishing decoding body.
end_stream_decoded_ = true;
if (received_trailers().size() > filterManagerConnection()->maxIncomingHeadersCount()) {
details_ = Http3ResponseCodeDetailValues::too_many_trailers;
onStreamError(close_connection_upon_invalid_header_, quic::QUIC_STREAM_EXCESSIVE_LOAD);
return;
}
response_decoder_->decodeTrailers(
spdyHeaderBlockToEnvoyHeaders<Http::ResponseTrailerMapImpl>(received_trailers()));
MarkTrailersConsumed();
Expand Down Expand Up @@ -304,7 +312,8 @@ QuicFilterManagerConnectionImpl* EnvoyQuicClientStream::filterManagerConnection(
return dynamic_cast<QuicFilterManagerConnectionImpl*>(session());
}

void EnvoyQuicClientStream::onStreamError(absl::optional<bool> should_close_connection) {
void EnvoyQuicClientStream::onStreamError(absl::optional<bool> should_close_connection,
quic::QuicRstStreamErrorCode rst_code) {
if (details_.empty()) {
details_ = Http3ResponseCodeDetailValues::invalid_http_header;
}
Expand All @@ -318,7 +327,7 @@ void EnvoyQuicClientStream::onStreamError(absl::optional<bool> should_close_conn
if (close_connection_upon_invalid_header) {
stream_delegate()->OnStreamError(quic::QUIC_HTTP_FRAME_ERROR, "Invalid headers");
} else {
Reset(quic::QUIC_BAD_APPLICATION_PAYLOAD);
Reset(rst_code);
}
}

Expand Down
3 changes: 2 additions & 1 deletion source/common/quic/envoy_quic_client_stream.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@ class EnvoyQuicClientStream : public quic::QuicSpdyClientStream,

// Either reset the stream or close the connection according to
// should_close_connection and configured http3 options.
void onStreamError(absl::optional<bool> should_close_connection);
void onStreamError(absl::optional<bool> should_close_connection,
quic::QuicRstStreamErrorCode rst_code);

Http::ResponseDecoder* response_decoder_{nullptr};

Expand Down
8 changes: 7 additions & 1 deletion source/common/quic/envoy_quic_server_stream.cc
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,8 @@ void EnvoyQuicServerStream::OnInitialHeadersComplete(bool fin, size_t frame_len,
end_stream_decoded_ = true;
}
std::unique_ptr<Http::RequestHeaderMapImpl> headers =
quicHeadersToEnvoyHeaders<Http::RequestHeaderMapImpl>(header_list, *this);
quicHeadersToEnvoyHeaders<Http::RequestHeaderMapImpl>(
header_list, *this, filterManagerConnection()->maxIncomingHeadersCount(), details_);
if (headers == nullptr) {
onStreamError(close_connection_upon_invalid_header_);
return;
Expand Down Expand Up @@ -249,6 +250,11 @@ void EnvoyQuicServerStream::maybeDecodeTrailers() {
if (sequencer()->IsClosed() && !FinishedReadingTrailers()) {
// Only decode trailers after finishing decoding body.
end_stream_decoded_ = true;
if (received_trailers().size() > filterManagerConnection()->maxIncomingHeadersCount()) {
details_ = Http3ResponseCodeDetailValues::too_many_trailers;
onStreamError(close_connection_upon_invalid_header_);
return;
}
Comment on lines +253 to +257
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for client stream?

request_decoder_->decodeTrailers(
spdyHeaderBlockToEnvoyHeaders<Http::RequestTrailerMapImpl>(received_trailers()));
MarkTrailersConsumed();
Expand Down
17 changes: 0 additions & 17 deletions source/common/quic/envoy_quic_stream.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,23 +13,6 @@
namespace Envoy {
namespace Quic {

// Changes or additions to details should be reflected in
// docs/root/configuration/http/http_conn_man/response_code_details_details.rst
class Http3ResponseCodeDetailValues {
public:
// Invalid HTTP header field was received and stream is going to be
// closed.
static constexpr absl::string_view invalid_http_header = "http3.invalid_header_field";
// The size of headers (or trailers) exceeded the configured limits.
static constexpr absl::string_view headers_too_large = "http3.headers_too_large";
// Envoy was configured to drop requests with header keys beginning with underscores.
static constexpr absl::string_view invalid_underscore = "http3.unexpected_underscore";
// The peer refused the stream.
static constexpr absl::string_view remote_refused = "http3.remote_refuse";
// The peer reset the stream.
static constexpr absl::string_view remote_reset = "http3.remote_reset";
};

// Base class for EnvoyQuicServer|ClientStream.
class EnvoyQuicStream : public virtual Http::StreamEncoder,
public Http::Stream,
Expand Down
32 changes: 30 additions & 2 deletions source/common/quic/envoy_quic_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,27 @@
namespace Envoy {
namespace Quic {

// Changes or additions to details should be reflected in
// docs/root/configuration/http/http_conn_man/response_code_details.rst
class Http3ResponseCodeDetailValues {
public:
// Invalid HTTP header field was received and stream is going to be
// closed.
static constexpr absl::string_view invalid_http_header = "http3.invalid_header_field";
// The size of headers (or trailers) exceeded the configured limits.
static constexpr absl::string_view headers_too_large = "http3.headers_too_large";
// Envoy was configured to drop requests with header keys beginning with underscores.
static constexpr absl::string_view invalid_underscore = "http3.unexpected_underscore";
// The peer refused the stream.
static constexpr absl::string_view remote_refused = "http3.remote_refuse";
// The peer reset the stream.
static constexpr absl::string_view remote_reset = "http3.remote_reset";
// Too many trailers were sent.
static constexpr absl::string_view too_many_trailers = "http3.too_many_trailers";
// Too many headers were sent.
static constexpr absl::string_view too_many_headers = "http3.too_many_headers";
};

// TODO(danzh): this is called on each write. Consider to return an address instance on the stack if
// the heap allocation is too expensive.
Network::Address::InstanceConstSharedPtr
Expand All @@ -48,14 +69,21 @@ class HeaderValidator {

// The returned header map has all keys in lower case.
template <class T>
std::unique_ptr<T> quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_list,
HeaderValidator& validator) {
std::unique_ptr<T>
quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_list, HeaderValidator& validator,
uint32_t max_headers_allowed, absl::string_view& details) {
Copy link
Contributor

@danzh2010 danzh2010 Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it because QuicHeaderList doesn't expose size() interface that we need max_headers_allowed to be passed into this function?

I slightly prefer checking it in QUICHE and have something like OnHeadersTooLarge() or simply add QuicHeaderList::size() instead of iterating over the header list again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% prefer checking in QUICHE and rejecting early, vs rejecting late in the game, but I think this is still better than nothing because it protects Envoy filters against too many headers. What do you think about landing this as-is and moving where we enforce once someone on quic-dev adds the knob we'd need?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM.

auto headers = T::create();
for (const auto& entry : header_list) {
if (max_headers_allowed == 0) {
details = Http3ResponseCodeDetailValues::too_many_headers;
return nullptr;
}
max_headers_allowed--;
Http::HeaderUtility::HeaderValidationResult result =
validator.validateHeader(entry.first, entry.second);
switch (result) {
case Http::HeaderUtility::HeaderValidationResult::REJECT:
// The validator sets the details to Http3ResponseCodeDetailValues::invalid_underscore
return nullptr;
case Http::HeaderUtility::HeaderValidationResult::DROP:
continue;
Expand Down
7 changes: 7 additions & 0 deletions source/common/quic/quic_filter_manager_connection_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,12 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase,
codec_stats_ = std::reference_wrapper<Http::Http3::CodecStats>(stats);
}

uint32_t maxIncomingHeadersCount() { return max_headers_count_; }

void setMaxIncomingHeadersCount(uint32_t max_headers_count) {
max_headers_count_ = max_headers_count;
}

protected:
// Propagate connection close to network_connection_callbacks_.
void onConnectionCloseEvent(const quic::QuicConnectionCloseFrame& frame,
Expand Down Expand Up @@ -179,6 +185,7 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase,
StreamInfo::StreamInfoImpl stream_info_;
std::string transport_failure_reason_;
uint32_t bytes_to_send_{0};
uint32_t max_headers_count_{std::numeric_limits<uint32_t>::max()};
// Keeps the buffer state of the connection, and react upon the changes of how many bytes are
// buffered cross all streams' send buffer. The state is evaluated and may be changed upon each
// stream write. QUICHE doesn't buffer data in connection, all the data is buffered in stream's
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,7 @@ HttpConnectionManagerConfig::createCodec(Network::Connection& connection,
return std::make_unique<Quic::QuicHttpServerConnectionImpl>(
dynamic_cast<Quic::EnvoyQuicServerSession&>(connection), callbacks,
Http::Http3::CodecStats::atomicGet(http3_codec_stats_, context_.scope()), http3_options_,
maxRequestHeadersKb(), headersWithUnderscoresAction());
maxRequestHeadersKb(), maxRequestHeadersCount(), headersWithUnderscoresAction());
#else
// Should be blocked by configuration checking at an earlier point.
NOT_REACHED_GCOVR_EXCL_LINE;
Expand Down
2 changes: 1 addition & 1 deletion test/common/quic/envoy_quic_client_session_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParam<bool> {
stats_({ALL_HTTP3_CODEC_STATS(POOL_COUNTER_PREFIX(scope_, "http3."),
POOL_GAUGE_PREFIX(scope_, "http3."))}),
http_connection_(envoy_quic_session_, http_connection_callbacks_, stats_, http3_options_,
64 * 1024) {
64 * 1024, 100) {
EXPECT_EQ(time_system_.systemTime(), envoy_quic_session_.streamInfo().startTime());
EXPECT_EQ(EMPTY_STRING, envoy_quic_session_.nextProtocol());
EXPECT_EQ(Http::Protocol::Http3, http_connection_.protocol());
Expand Down
2 changes: 1 addition & 1 deletion test/common/quic/envoy_quic_server_session_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ class EnvoyQuicServerSessionTest : public testing::TestWithParam<bool> {
EXPECT_CALL(*read_filter_, onNewConnection()).WillOnce(Invoke([this]() {
// Create ServerConnection instance and setup callbacks for it.
http_connection_ = std::make_unique<QuicHttpServerConnectionImpl>(
envoy_quic_session_, http_connection_callbacks_, stats_, http3_options_, 64 * 1024,
envoy_quic_session_, http_connection_callbacks_, stats_, http3_options_, 64 * 1024, 100,
envoy::config::core::v3::HttpProtocolOptions::ALLOW);
EXPECT_EQ(Http::Protocol::Http3, http_connection_->protocol());
// Stop iteration to avoid calling getRead/WriteBuffer().
Expand Down
7 changes: 4 additions & 3 deletions test/common/quic/envoy_quic_utils_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,9 @@ TEST(EnvoyQuicUtilsTest, HeadersConversion) {
}
return Http::HeaderUtility::HeaderValidationResult::ACCEPT;
});
absl::string_view details;
auto envoy_headers2 =
quicHeadersToEnvoyHeaders<Http::RequestHeaderMapImpl>(quic_headers, validator);
quicHeadersToEnvoyHeaders<Http::RequestHeaderMapImpl>(quic_headers, validator, 100, details);
EXPECT_EQ(*envoy_headers, *envoy_headers2);

quic::QuicHeaderList quic_headers2;
Expand All @@ -105,8 +106,8 @@ TEST(EnvoyQuicUtilsTest, HeadersConversion) {
}
return Http::HeaderUtility::HeaderValidationResult::ACCEPT;
});
EXPECT_EQ(nullptr,
quicHeadersToEnvoyHeaders<Http::RequestHeaderMapImpl>(quic_headers2, validator));
EXPECT_EQ(nullptr, quicHeadersToEnvoyHeaders<Http::RequestHeaderMapImpl>(quic_headers2, validator,
100, details));
}

} // namespace Quic
Expand Down
3 changes: 2 additions & 1 deletion test/integration/fake_upstream.cc
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,8 @@ FakeHttpConnection::FakeHttpConnection(
Http::Http3::CodecStats& stats = fake_upstream.http3CodecStats();
codec_ = std::make_unique<Quic::QuicHttpServerConnectionImpl>(
dynamic_cast<Quic::EnvoyQuicServerSession&>(shared_connection_.connection()), *this, stats,
fake_upstream.http3Options(), max_request_headers_kb, headers_with_underscores_action);
fake_upstream.http3Options(), max_request_headers_kb, max_request_headers_count,
headers_with_underscores_action);
#else
ASSERT(false, "running a QUIC integration test without compiling QUIC");
#endif
Expand Down
2 changes: 1 addition & 1 deletion test/integration/http_integration.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1251,7 +1251,7 @@ void HttpIntegrationTest::testManyRequestHeaders(std::chrono::milliseconds time)
// This test uses an Http::HeaderMapImpl instead of an Http::TestHeaderMapImpl to avoid
// time-consuming asserts when using a large number of headers.
setMaxRequestHeadersKb(96);
setMaxRequestHeadersCount(10005);
setMaxRequestHeadersCount(10010);

config_helper_.addConfigModifier(
[&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
Expand Down
6 changes: 3 additions & 3 deletions test/integration/multiplexed_upstream_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,6 @@ TEST_P(Http2UpstreamIntegrationTest, ManyLargeSimultaneousRequestWithRandomBacku
}

TEST_P(Http2UpstreamIntegrationTest, UpstreamConnectionCloseWithManyStreams) {
EXCLUDE_UPSTREAM_HTTP3; // Times out waiting for reset.
config_helper_.setBufferLimits(1024, 1024); // Set buffer limits upstream and downstream.
const uint32_t num_requests = 20;
std::vector<Http::RequestEncoder*> encoders;
Expand Down Expand Up @@ -369,6 +368,9 @@ TEST_P(Http2UpstreamIntegrationTest, UpstreamConnectionCloseWithManyStreams) {
ASSERT_TRUE(upstream_requests[i]->waitForEndStream(*dispatcher_));
upstream_requests[i]->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, false);
upstream_requests[i]->encodeData(100, false);
// Make sure at least the headers go through, to ensure stream reset rather
// than disconnect.
responses[i]->waitForHeaders();
}
// Close the connection.
ASSERT_TRUE(fake_upstream_connection_->close());
Expand Down Expand Up @@ -442,8 +444,6 @@ name: router
// Tests the default limit for the number of response headers is 100. Results in a stream reset if
// exceeds.
TEST_P(Http2UpstreamIntegrationTest, TestManyResponseHeadersRejected) {
EXCLUDE_UPSTREAM_HTTP3; // no 503.

// Default limit for response headers is 100.
initialize();
codec_client_ = makeHttpConnection(lookupPort("http"));
Expand Down
Loading