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

http: add support for skipping any port from host header #14491

Merged
merged 14 commits into from Jan 4, 2021
Expand Up @@ -35,7 +35,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: 42]
// [#next-free-field: 43]
message HttpConnectionManager {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager";
Expand Down Expand Up @@ -551,7 +551,20 @@ message HttpConnectionManager {
// Without setting this option, incoming requests with host `example:443` will not match against
// route with :ref:`domains<envoy_api_field_config.route.v3.VirtualHost.domains>` match set to `example`. Defaults to `false`. Note that port removal is not part
// of `HTTP spec <https://tools.ietf.org/html/rfc3986>`_ and is provided for convenience.
bool strip_matching_host_port = 39;
// Only one of `strip_matching_host_port` or `strip_any_host_port` can be set.
bool strip_matching_host_port = 39
[(udpa.annotations.field_migrate).oneof_promotion = "strip_port_mode"];

oneof strip_port_mode {
// Determines if the port part should be removed from host/authority header before any processing
// of request by HTTP filters or routing. The port would be removed only if request method is not CONNECT.
// This affects the upstream host header as well.
// Without setting this option, incoming requests with host `example:443` will not match against
// route with :ref:`domains<envoy_api_field_config.route.v3.VirtualHost.domains>` match set to `example`. Defaults to `false`. Note that port removal is not part
// of `HTTP spec <https://tools.ietf.org/html/rfc3986>`_ and is provided for convenience.
// Only one of `strip_matching_host_port` or `strip_any_host_port` can be set.
bool strip_any_host_port = 42;
}

// Governs Envoy's behavior when receiving invalid HTTP from downstream.
// If this option is false (default), Envoy will err on the conservative side handling HTTP
Expand Down

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

1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
Expand Up @@ -73,6 +73,7 @@ New Features
* health_check: added option to use :ref:`no_traffic_healthy_interval <envoy_v3_api_field_config.core.v3.HealthCheck.no_traffic_healthy_interval>` which allows a different no traffic interval when the host is healthy.
* http: added HCM :ref:`timeout config field <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.request_headers_timeout>` to control how long a downstream has to finish sending headers before the stream is cancelled.
* http: added frame flood and abuse checks to the upstream HTTP/2 codec. This check is off by default and can be enabled by setting the `envoy.reloadable_features.upstream_http2_flood_checks` runtime key to true.
* http: added :ref:`stripping any port from host header <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.strip_any_host_port>` support.
* http: clusters now support selecting HTTP/1 or HTTP/2 based on ALPN, configurable via :ref:`alpn_config <envoy_v3_api_field_extensions.upstreams.http.v3.HttpProtocolOptions.auto_config>` in the :ref:`http_protocol_options <envoy_v3_api_msg_extensions.upstreams.http.v3.HttpProtocolOptions>` message.
* jwt_authn: added support for :ref:`per-route config <envoy_v3_api_msg_extensions.filters.http.jwt_authn.v3.PerRouteConfig>`.
* kill_request: added new :ref:`HTTP kill request filter <config_http_filters_kill_request>`.
Expand Down

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.

16 changes: 14 additions & 2 deletions source/common/http/conn_manager_config.h
Expand Up @@ -167,6 +167,18 @@ enum class ForwardClientCertType {
*/
enum class ClientCertDetailsType { Cert, Chain, Subject, URI, DNS };

/**
* Type that indicates how port should be stripped from Host header.
*/
enum class StripPortType {
// Removes the port from host/authority header only if the port matches with the listener port.
MatchingHost,
// Removes any port from host/authority header.
Any,
// Keeps the port in host/authority header as is.
None
};

/**
* Configuration for what addresses should be considered internal beyond the defaults.
*/
Expand Down Expand Up @@ -439,9 +451,9 @@ class ConnectionManagerConfig {
virtual bool shouldMergeSlashes() const PURE;

/**
* @return if the HttpConnectionManager should remove the port from host/authority header
* @return port strip type from host/authority header.
*/
virtual bool shouldStripMatchingPort() const PURE;
virtual StripPortType stripPortType() const PURE;

/**
* @return the action HttpConnectionManager should take when receiving client request
Expand Down
5 changes: 4 additions & 1 deletion source/common/http/conn_manager_utility.cc
Expand Up @@ -8,6 +8,7 @@

#include "common/common/empty_string.h"
#include "common/common/utility.h"
#include "common/http/conn_manager_config.h"
#include "common/http/header_utility.h"
#include "common/http/headers.h"
#include "common/http/http1/codec_impl.h"
Expand Down Expand Up @@ -427,7 +428,9 @@ bool ConnectionManagerUtility::maybeNormalizePath(RequestHeaderMap& request_head
void ConnectionManagerUtility::maybeNormalizeHost(RequestHeaderMap& request_headers,
const ConnectionManagerConfig& config,
uint32_t port) {
if (config.shouldStripMatchingPort()) {
if (config.stripPortType() == Http::StripPortType::Any) {
HeaderUtility::stripPortFromHost(request_headers, absl::nullopt);
} else if (config.stripPortType() == Http::StripPortType::MatchingHost) {
HeaderUtility::stripPortFromHost(request_headers, port);
}
}
Expand Down
8 changes: 5 additions & 3 deletions source/common/http/header_utility.cc
Expand Up @@ -216,7 +216,8 @@ bool HeaderUtility::isEnvoyInternalRequest(const RequestHeaderMap& headers) {
internal_request_header->value() == Headers::get().EnvoyInternalRequestValues.True;
}

void HeaderUtility::stripPortFromHost(RequestHeaderMap& headers, uint32_t listener_port) {
void HeaderUtility::stripPortFromHost(RequestHeaderMap& headers,
absl::optional<uint32_t> listener_port) {

if (headers.getMethodValue() == Http::Headers::get().MethodValues.Connect) {
// According to RFC 2817 Connect method should have port part in host header.
Expand All @@ -239,8 +240,9 @@ void HeaderUtility::stripPortFromHost(RequestHeaderMap& headers, uint32_t listen
if (!absl::SimpleAtoi(port_str, &port)) {
return;
}
if (port != listener_port) {
// We would strip ports only if they are the same, as local port of the listener.
if (listener_port.has_value() && port != listener_port) {
// We would strip ports only if it is specified and they are the same, as local port of the
// listener.
return;
}
const absl::string_view host = original_host.substr(0, port_start);
Expand Down
5 changes: 3 additions & 2 deletions source/common/http/header_utility.h
Expand Up @@ -174,9 +174,10 @@ class HeaderUtility {
const RequestOrResponseHeaderMap& headers);

/**
* @brief Remove the port part from host/authority header if it is equal to provided port
* @brief Remove the port part from host/authority header if it is equal to provided port or
* provided port is 0.
Copy link
Member

Choose a reason for hiding this comment

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

needs updating

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry. Missed it. Fixed now.

*/
static void stripPortFromHost(RequestHeaderMap& headers, uint32_t listener_port);
static void stripPortFromHost(RequestHeaderMap& headers, absl::optional<uint32_t> listener_port);

/* Does a common header check ensuring required headers are present.
* Required request headers include :method header, :path for non-CONNECT requests, and
Expand Down
Expand Up @@ -19,6 +19,7 @@
#include "common/common/fmt.h"
#include "common/config/utility.h"
#include "common/filter/http/filter_config_discovery_impl.h"
#include "common/http/conn_manager_config.h"
#include "common/http/conn_manager_utility.h"
#include "common/http/default_server_string.h"
#include "common/http/http1/codec_impl.h"
Expand Down Expand Up @@ -248,7 +249,6 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
0))),
#endif
merge_slashes_(config.merge_slashes()),
strip_matching_port_(config.strip_matching_host_port()),
headers_with_underscores_action_(
config.common_http_protocol_options().headers_with_underscores_action()),
local_reply_(LocalReply::Factory::create(config.local_reply_config(), context)) {
Expand All @@ -264,8 +264,16 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
idle_timeout_ = absl::nullopt;
}

// If we are provided a different request_id_extension implementation to use try and create a new
// instance of it, otherwise use default one.
if (config.strip_any_host_port()) {
strip_port_type_ = Http::StripPortType::Any;
} else if (config.strip_matching_host_port()) {
Comment on lines +272 to +274
Copy link
Member

Choose a reason for hiding this comment

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

Given that defaults are false, can we check if both are true and throw an error? I think this is safe to do and will approximate the future oneof?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. We can do that. Changed and added a test. PTAL

strip_port_type_ = Http::StripPortType::MatchingHost;
} else {
strip_port_type_ = Http::StripPortType::None;
}

// If we are provided a different request_id_extension implementation to use try and create a
// new instance of it, otherwise use default one.
if (config.request_id_extension().has_typed_config()) {
request_id_extension_ =
Http::RequestIDExtensionFactory::fromProto(config.request_id_extension(), context_);
Expand Down
Expand Up @@ -18,6 +18,7 @@
#include "envoy/tracing/http_tracer_manager.h"

#include "common/common/logger.h"
#include "common/http/conn_manager_config.h"
#include "common/http/conn_manager_impl.h"
#include "common/http/date_provider_impl.h"
#include "common/http/http1/codec_stats.h"
Expand Down Expand Up @@ -169,7 +170,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
const Http::Http1Settings& http1Settings() const override { return http1_settings_; }
bool shouldNormalizePath() const override { return normalize_path_; }
bool shouldMergeSlashes() const override { return merge_slashes_; }
bool shouldStripMatchingPort() const override { return strip_matching_port_; }
Http::StripPortType stripPortType() const override { return strip_port_type_; }
envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
headersWithUnderscoresAction() const override {
return headers_with_underscores_action_;
Expand Down Expand Up @@ -250,7 +251,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
std::chrono::milliseconds delayed_close_timeout_;
const bool normalize_path_;
const bool merge_slashes_;
const bool strip_matching_port_;
Http::StripPortType strip_port_type_;
const envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
headers_with_underscores_action_;
const LocalReply::LocalReplyPtr local_reply_;
Expand Down
3 changes: 2 additions & 1 deletion source/server/admin/admin.h
Expand Up @@ -23,6 +23,7 @@
#include "common/common/empty_string.h"
#include "common/common/logger.h"
#include "common/common/macros.h"
#include "common/http/conn_manager_config.h"
#include "common/http/conn_manager_impl.h"
#include "common/http/date_provider_impl.h"
#include "common/http/default_server_string.h"
Expand Down Expand Up @@ -173,7 +174,7 @@ class AdminImpl : public Admin,
const Http::Http1Settings& http1Settings() const override { return http1_settings_; }
bool shouldNormalizePath() const override { return true; }
bool shouldMergeSlashes() const override { return true; }
bool shouldStripMatchingPort() const override { return false; }
Http::StripPortType stripPortType() const override { return Http::StripPortType::None; }
envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
headersWithUnderscoresAction() const override {
return envoy::config::core::v3::HttpProtocolOptions::ALLOW;
Expand Down