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: support Http3Options for downstream #15753

Merged
merged 20 commits into from
Apr 6, 2021
Merged
Show file tree
Hide file tree
Changes from 12 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
12 changes: 9 additions & 3 deletions api/envoy/config/core/v3/protocol.proto
Original file line number Diff line number Diff line change
Expand Up @@ -418,9 +418,15 @@ message GrpcProtocolOptions {

// [#not-implemented-hide:]
//
// A message which allows using HTTP/3 as an upstream protocol.
//
// Eventually this will include configuration for tuning HTTP/3.
// A message which allows using HTTP/3.
message Http3ProtocolOptions {
QuicProtocolOptions quic_protocol_options = 1;

// Allows invalid HTTP messaging and headers. When this option is disabled (default), then
// the whole HTTP/3 connection is terminated upon receiving invalid HEADERS frame. However,
// when this option is enabled, only the offending stream is terminated.
//
// If set, this overrides any HCM :ref:`stream_error_on_invalid_http_messaging
// <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.stream_error_on_invalid_http_message>`.
google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 2;
}
12 changes: 9 additions & 3 deletions api/envoy/config/core/v4alpha/protocol.proto

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
// HTTP connection manager :ref:`configuration overview <config_http_conn_man>`.
// [#extension: envoy.filters.network.http_connection_manager]

// [#next-free-field: 44]
// [#next-free-field: 45]
message HttpConnectionManager {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager";
Expand Down Expand Up @@ -325,6 +325,10 @@ message HttpConnectionManager {
config.core.v3.Http2ProtocolOptions http2_protocol_options = 9
[(udpa.annotations.security).configure_for_untrusted_downstream = true];

// Additional HTTP/3 settings that are passed directly to the HTTP/3 codec.
// [#not-implemented-hide:]
config.core.v3.Http3ProtocolOptions http3_protocol_options = 44;

// An optional override that the connection manager will write to the server
// header in responses. If not set, the default is *envoy*.
string server_name = 10
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -99,3 +99,16 @@ All http2 details are rooted at *http2.*
http2.unexpected_underscore, Envoy was configured to drop requests with header keys beginning with underscores.
http2.unknown.nghttp2.error, An unknown error was encountered by nghttp2
http2.violation.of.messaging.rule, The stream was in violation of a HTTP/2 messaging rule.

Http3 details
~~~~~~~~~~~~~

All http3 details are rooted at *http3.*

.. csv-table::
:header: Name, Description
:widths: 1, 2

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.
12 changes: 9 additions & 3 deletions generated_api_shadow/envoy/config/core/v3/protocol.proto

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 9 additions & 3 deletions generated_api_shadow/envoy/config/core/v4alpha/protocol.proto

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions include/envoy/http/codec.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ namespace Http2 {
struct CodecStats;
}

namespace Http3 {
struct CodecStats;
}

// Legacy default value of 60K is safely under both codec default limits.
static constexpr uint32_t DEFAULT_MAX_REQUEST_HEADERS_KB = 60;
// Default maximum number of headers.
Expand Down
5 changes: 5 additions & 0 deletions include/envoy/upstream/upstream.h
Original file line number Diff line number Diff line change
Expand Up @@ -995,6 +995,11 @@ class ClusterInfo {
*/
virtual Http::Http2::CodecStats& http2CodecStats() const PURE;

/**
* @return the Http2 Codec Stats.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @return the Http2 Codec Stats.
* @return the Http3 Codec Stats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

*/
virtual Http::Http3::CodecStats& http3CodecStats() const PURE;

protected:
/**
* Invoked by extensionProtocolOptionsTyped.
Expand Down
1 change: 1 addition & 0 deletions source/common/http/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,7 @@ envoy_cc_library(
"//source/common/common:utility_lib",
"//source/common/protobuf:utility_lib",
"//source/common/runtime:runtime_features_lib",
"@envoy_api//envoy/config/core/v3:pkg_cc_proto",
"@envoy_api//envoy/config/route/v3:pkg_cc_proto",
"@envoy_api//envoy/type/v3:pkg_cc_proto",
],
Expand Down
4 changes: 3 additions & 1 deletion source/common/http/codec_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,9 @@ CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& conne
case Type::HTTP3: {
#ifdef ENVOY_ENABLE_QUIC
codec_ = std::make_unique<Quic::QuicHttpClientConnectionImpl>(
dynamic_cast<Quic::EnvoyQuicClientSession&>(*connection_), *this);
dynamic_cast<Quic::EnvoyQuicClientSession&>(*connection_), *this,
host->cluster().http3CodecStats(), host->cluster().http3Options(),
Http::DEFAULT_MAX_REQUEST_HEADERS_KB);
break;
#else
// Should be blocked by configuration checking at an earlier point.
Expand Down
52 changes: 52 additions & 0 deletions source/common/http/header_utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -333,5 +333,57 @@ bool HeaderUtility::isModifiableHeader(absl::string_view header) {
!absl::EqualsIgnoreCase(header, Headers::get().HostLegacy.get()));
}

HeaderUtility::HeaderValidationResult HeaderUtility::checkHeaderNameForUnderscores(
Copy link
Member

Choose a reason for hiding this comment

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

It kills me that this code is now copied into a third place when we already effectively have it in both the H1 and H2 codec. This is really scary and security sensitive stuff and I feel we shouldn't be making the problem worse (it's in the opposite direction of #10646). Is there any work we can do to share some of this code now? Maybe we can do a different PR to add this helper and call it somehow from both other codecs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to touch any code in production in this PR because that needs feature protection. Once this PR lands, the other two codec can switch to this utility function.

Copy link
Member

Choose a reason for hiding this comment

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

OK fair enough. Can you at least add comments and put TODOs in the other codec places which reference this code and the tracking issue for merging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @yanavlasov might be a good starter project for one of the envoy-sec folks

const std::string& header_name,
envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
headers_with_underscores_action,
Stats::Counter& dropped_headers_with_underscores,
Stats::Counter& requests_rejected_with_underscores_in_headers) {
if (headers_with_underscores_action == envoy::config::core::v3::HttpProtocolOptions::ALLOW ||
!HeaderUtility::headerNameContainsUnderscore(header_name)) {
return HeaderValidationResult::ACCEPT;
}
if (headers_with_underscores_action ==
envoy::config::core::v3::HttpProtocolOptions::DROP_HEADER) {
ENVOY_LOG_MISC(debug, "Dropping header with invalid characters in its name: {}", header_name);
dropped_headers_with_underscores.inc();
return HeaderValidationResult::DROP;
}
ENVOY_LOG_MISC(debug, "Rejecting request due to header name with underscores: {}", header_name);
requests_rejected_with_underscores_in_headers.inc();
return HeaderUtility::HeaderValidationResult::REJECT;
}

HeaderUtility::HeaderValidationResult
HeaderUtility::validateContentLength(absl::string_view header_value,
bool override_stream_error_on_invalid_http_message,
bool& should_close_connection) {
should_close_connection = false;
std::vector<absl::string_view> values = absl::StrSplit(header_value, ',');
absl::optional<uint64_t> content_length;
for (const absl::string_view& value : values) {
uint64_t new_value;
if (!absl::SimpleAtoi(value, &new_value) ||
!std::all_of(value.begin(), value.end(), absl::ascii_isdigit)) {
ENVOY_LOG_MISC(debug, "Content length was either unparseable or negative");
should_close_connection = !override_stream_error_on_invalid_http_message;
return HeaderValidationResult::REJECT;
}
if (!content_length.has_value()) {
content_length = new_value;
continue;
}
if (new_value != content_length.value()) {
ENVOY_LOG_MISC(
debug,
"Parsed content length {} is inconsistent with previously detected content length {}",
new_value, content_length.value());
should_close_connection = !override_stream_error_on_invalid_http_message;
return HeaderValidationResult::REJECT;
}
}
return HeaderValidationResult::ACCEPT;
}

} // namespace Http
} // namespace Envoy
31 changes: 31 additions & 0 deletions source/common/http/header_utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include <vector>

#include "envoy/common/regex.h"
#include "envoy/config/core/v3/protocol.pb.h"
#include "envoy/config/route/v3/route_components.pb.h"
#include "envoy/http/header_map.h"
#include "envoy/http/protocol.h"
Expand Down Expand Up @@ -209,6 +210,36 @@ class HeaderUtility {
* may not be modified.
*/
static bool isModifiableHeader(absl::string_view header);

enum class HeaderValidationResult {
ACCEPT = 0,
DROP,
REJECT,
};

/**
* Check if the given |header_name| has underscore.
* Return HeaderValidationResult and populate the given counters based on
* |headers_with_underscores_action|.
*/
static HeaderValidationResult checkHeaderNameForUnderscores(
const std::string& header_name,
envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
headers_with_underscores_action,
Stats::Counter& dropped_headers_with_underscores,
Stats::Counter& requests_rejected_with_underscores_in_headers);

/**
* Check if |header_value| represents valid value for HTTP content-length
* header.
* Return HeaderValidationResult and populate |should_close_connection|
* according to |override_stream_error_on_invalid_http_message|.
*/
static HeaderValidationResult
validateContentLength(absl::string_view header_value,
bool override_stream_error_on_invalid_http_message,
bool& should_close_connection);
};

} // namespace Http
} // namespace Envoy
10 changes: 10 additions & 0 deletions source/common/http/http3/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,13 @@ envoy_cc_library(
name = "quic_client_connection_factory_lib",
hdrs = ["quic_client_connection_factory.h"],
)

envoy_cc_library(
name = "codec_stats_lib",
hdrs = ["codec_stats.h"],
deps = [
"//include/envoy/stats:stats_interface",
"//include/envoy/stats:stats_macros",
"//source/common/common:thread_lib",
],
)
44 changes: 44 additions & 0 deletions source/common/http/http3/codec_stats.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
#pragma once

#include "envoy/stats/scope.h"
#include "envoy/stats/stats_macros.h"

#include "common/common/thread.h"

namespace Envoy {
namespace Http {
namespace Http3 {

/**
* All stats for the HTTP/3 codec. @see stats_macros.h
* TODO(danzh) populate all of them in codec.
*/
#define ALL_HTTP3_CODEC_STATS(COUNTER, GAUGE) \
COUNTER(dropped_headers_with_underscores) \
COUNTER(header_overflow) \
COUNTER(requests_rejected_with_underscores_in_headers) \
COUNTER(rx_messaging_error) \
COUNTER(rx_reset) \
COUNTER(trailers) \
COUNTER(tx_reset) \
GAUGE(streams_active, Accumulate)

/**
* Wrapper struct for the HTTP/3 codec stats. @see stats_macros.h
*/
struct CodecStats {
using AtomicPtr = Thread::AtomicPtr<CodecStats, Thread::AtomicPtrAllocMode::DeleteOnDestruct>;

static CodecStats& atomicGet(AtomicPtr& ptr, Stats::Scope& scope) {
return *ptr.get([&scope]() -> CodecStats* {
return new CodecStats{ALL_HTTP3_CODEC_STATS(POOL_COUNTER_PREFIX(scope, "http3."),
POOL_GAUGE_PREFIX(scope, "http3."))};
});
}

ALL_HTTP3_CODEC_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT)
};

} // namespace Http3
} // namespace Http
} // namespace Envoy
Loading