Skip to content
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
7c0674d
Remove trailing host dot from host/authority header.
maheshkurund Mar 19, 2021
d2e05b7
Fixed formatting.
maheshkurund Mar 19, 2021
595157c
Added missing variable entry.
maheshkurund Mar 19, 2021
4945289
Added unit test to check dot striping after RequestDecoder::decodeHea…
maheshkurund Mar 22, 2021
74f247f
Fixed test failure caused because of missing cleanup.
maheshkurund Mar 22, 2021
bb62fda
Merge branch 'main' into remove_trailing_host_dot
maheshkurund Apr 8, 2021
c3d1057
Fixed proto field number
maheshkurund Apr 8, 2021
4b9c804
Merge remote-tracking branch 'upstream/main' into remove_trailing_hos…
maheshkurund Apr 22, 2021
468ce45
Fixed tests.
maheshkurund Apr 22, 2021
3a4f449
Fixed tests.
maheshkurund Apr 23, 2021
40a1a44
Fixed test cleanup.
maheshkurund Apr 23, 2021
8995b52
Merge upstream/main
maheshkurund Apr 30, 2021
baed449
Addressed review comment.
maheshkurund Apr 30, 2021
58b3759
Merge branch 'main' into remove_trailing_host_dot
maheshkurund May 10, 2021
e910496
Fixed docs.
maheshkurund May 11, 2021
dca8659
Merge main into remove_trailing_host_dot
maheshkurund May 12, 2021
fffc458
Merge main branch into remove_trailing_host_dot
maheshkurund May 18, 2021
d3cd808
Addressed review comments.
maheshkurund May 18, 2021
ef66621
Merge branch 'main' into remove_trailing_host_dot
maheshkurund May 20, 2021
c744100
Addressed review comments.
maheshkurund May 20, 2021
ec186fd
Merge branch 'main' into remove_trailing_host_dot
maheshkurund May 20, 2021
c94357b
Merge branch 'main' into remove_trailing_host_dot
maheshkurund May 21, 2021
d117486
Addressed review comments.
maheshkurund May 21, 2021
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 @@ -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: 47]
// [#next-free-field: 48]
message HttpConnectionManager {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager";
Expand Down Expand Up @@ -702,6 +702,15 @@ message HttpConnectionManager {
// <envoy_v3_api_msg_extensions.filters.network.http_connection_manager.v3.PathNormalizationOptions>`
// for details.
PathNormalizationOptions path_normalization_options = 43;

// Determines if trailing dot of the host should be removed from host/authority header before any
// processing of request by HTTP filters or routing.
// This affects the upstream host header.
// Without setting this option, incoming requests with host `example.com.` will not match against
// route with :ref:`domains<envoy_v3_api_field_config.route.v3.VirtualHost.domains>` match set to `example.com`. Defaults to `false`.
Comment on lines +706 to +710

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should probably include something about how this handles the host:port case

@maheshkurund maheshkurund May 15, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will following comment helps here in case of host:port?

In case host/authority header value has port part with host, on setting this option it removes trailing dot from host keeping port value as is i.e host example.com.:443 will become example.com:443.

// When incoming requests with host/authority header has port part with host, on setting this option it removes trailing dot from host keeping port

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe When the incoming request contains a host/authority header includes a port number, setting this option will strip trailing dots from the host section, leaving the port as is (e.g. ...?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks, will update.

// value as is (e.g. host value `example.com.:443` will be updated to `example.com:443`).
bool strip_trailing_host_dot = 47;
}

// The configuration to customize local reply returned by Envoy.
Expand Down

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

2 changes: 2 additions & 0 deletions docs/root/version_history/current.rst
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,13 @@ Removed Config or Runtime

New Features
------------

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remove this newline

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

will remove.

* crash support: restore crash context when continuing to processing requests or responses as a result of an asynchronous callback that invokes a filter directly. This is unlike the call stacks that go through the various network layers, to eventually reach the filter. For a concrete example see: ``Envoy::Extensions::HttpFilters::Cache::CacheFilter::getHeaders`` which posts a callback on the dispatcher that will invoke the filter directly.
* http: a new field `is_optional` is added to `extensions.filters.network.http_connection_manager.v3.HttpFilter`. When
value is `true`, the unsupported http filter will be ignored by envoy. This is also same with unsupported http filter
in the typed per filter config. For more information, please reference
:ref:`HttpFilter <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpFilter.is_optional>`.
* http: added :ref:`stripping trailing host dot from host header<envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.strip_trailing_host_dot>` support.
* http: added support for :ref:`original IP detection extensions<envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.original_ip_detection_extensions>`.
Two initial extensions were added, the :ref:`custom header <envoy_v3_api_msg_extensions.http.original_ip_detection.custom_header.v3.CustomHeaderConfig>` extension and the
:ref:`xff <envoy_v3_api_msg_extensions.http.original_ip_detection.xff.v3.XffConfig>` extension.
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.

6 changes: 6 additions & 0 deletions source/common/http/conn_manager_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,12 @@ class ConnectionManagerConfig {
*/
virtual const std::vector<OriginalIPDetectionSharedPtr>&
originalIpDetectionExtensions() const PURE;

/**
* @return if the HttpConnectionManager should remove trailing host dot from host/authority
* header.
*/
virtual bool shouldStripTrailingHostDot() const PURE;
};
} // namespace Http
} // namespace Envoy
3 changes: 3 additions & 0 deletions source/common/http/conn_manager_utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,9 @@ ConnectionManagerUtility::maybeNormalizePath(RequestHeaderMap& request_headers,
absl::optional<uint32_t>
ConnectionManagerUtility::maybeNormalizeHost(RequestHeaderMap& request_headers,
const ConnectionManagerConfig& config, uint32_t port) {
if (config.shouldStripTrailingHostDot()) {
HeaderUtility::stripTrailingHostDot(request_headers);
}
if (config.stripPortType() == Http::StripPortType::Any) {
return HeaderUtility::stripPortFromHost(request_headers, absl::nullopt);
} else if (config.stripPortType() == Http::StripPortType::MatchingHost) {
Expand Down
21 changes: 21 additions & 0 deletions source/common/http/header_utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,27 @@ bool HeaderUtility::isEnvoyInternalRequest(const RequestHeaderMap& headers) {
internal_request_header->value() == Headers::get().EnvoyInternalRequestValues.True;
}

void HeaderUtility::stripTrailingHostDot(RequestHeaderMap& headers) {
auto host = headers.getHostValue();
// If the host ends in a period, remove it.
auto dot_index = host.rfind('.');
if (dot_index == std::string::npos) {
return;
} else if (dot_index == (host.size() - 1)) {
host.remove_suffix(1);
headers.setHost(host);
return;
}
// If the dot is just before a colon, it must be preceding the port number.
// Because although the host may contain a colon via an IPv6 bracketed
// address, and although that IPv6 address may also contain dots when
// embedding an address per RFC 4291 2.2.3. In this case the dot will never

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a bit tricky to read, maybe something like IPv6 addresses may contain colons or dots, but the dot will never directly precede the colon, so this check should be sufficient to detect a trailing port number.?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sure, will update it.

// directly precede the colon like it would in foo.com.:123.
if (host[dot_index + 1] == ':') {
headers.setHost(absl::StrCat(host.substr(0, dot_index), host.substr(dot_index + 1)));
}
}

absl::optional<uint32_t> HeaderUtility::stripPortFromHost(RequestHeaderMap& headers,
absl::optional<uint32_t> listener_port) {
if (headers.getMethodValue() == Http::Headers::get().MethodValues.Connect &&
Expand Down
5 changes: 5 additions & 0 deletions source/common/http/header_utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,11 @@ class HeaderUtility {
static bool shouldCloseConnection(Http::Protocol protocol,
const RequestOrResponseHeaderMap& headers);

/**
* @brief Remove the trailing host dot from host/authority header.
*/
static void stripTrailingHostDot(RequestHeaderMap& headers);

/**
* @brief Remove the port part from host/authority header if it is equal to provided port.
* @return absl::optional<uint32_t> containing the port, if removed, else absl::nullopt.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
headers_with_underscores_action_(
config.common_http_protocol_options().headers_with_underscores_action()),
local_reply_(LocalReply::Factory::create(config.local_reply_config(), context)),
path_with_escaped_slashes_action_(getPathWithEscapedSlashesAction(config, context)) {
path_with_escaped_slashes_action_(getPathWithEscapedSlashesAction(config, context)),
strip_trailing_host_dot_(config.strip_trailing_host_dot()) {
// If idle_timeout_ was not configured in common_http_protocol_options, use value in deprecated
// idle_timeout field.
// TODO(asraa): Remove when idle_timeout is removed.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,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 shouldStripTrailingHostDot() const override { return strip_trailing_host_dot_; }
Http::StripPortType stripPortType() const override { return strip_port_type_; }
envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
headersWithUnderscoresAction() const override {
Expand Down Expand Up @@ -283,6 +284,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
static const uint64_t RequestHeaderTimeoutMs = 0;
const envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager::
PathWithEscapedSlashesAction path_with_escaped_slashes_action_;
const bool strip_trailing_host_dot_;
};

/**
Expand Down
1 change: 1 addition & 0 deletions source/server/admin/admin.h
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,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 shouldStripTrailingHostDot() const override { return false; }
Http::StripPortType stripPortType() const override { return Http::StripPortType::None; }
envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
headersWithUnderscoresAction() const override {
Expand Down
1 change: 1 addition & 0 deletions test/common/http/conn_manager_impl_fuzz_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ class FuzzConfig : public ConnectionManagerConfig {
const Http::Http1Settings& http1Settings() const override { return http1_settings_; }
bool shouldNormalizePath() const override { return false; }
bool shouldMergeSlashes() const override { return false; }
bool shouldStripTrailingHostDot() const override { return false; }
Http::StripPortType stripPortType() const override { return Http::StripPortType::None; }
envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
headersWithUnderscoresAction() const override {
Expand Down
21 changes: 21 additions & 0 deletions test/common/http/conn_manager_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1194,6 +1194,27 @@ TEST_F(HttpConnectionManagerImplTest, RouteShouldUseNormalizedHost) {
filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose);
}

// Observe host header w/o trailing dot in host when trailing dot removal is configured.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This makes it sound like the input doesn't have a trailing dot, maybe Observe tat we strip the trailing dot ...?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks, will change.

TEST_F(HttpConnectionManagerImplTest, StripTrailingHostDot) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mind adding a test where we set this config option but pass through a header without a trailing dot?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure. will add.

setup(false, "");
// Enable removal of host's trailing dot.
strip_trailing_host_dot_ = true;
const std::string original_host = "host.";
const std::string updated_host = "host";
// Set up the codec.
Buffer::OwnedImpl fake_input("1234");
conn_manager_->createCodec(fake_input);
// Create a new stream.
decoder_ = &conn_manager_->newStream(response_encoder_);
RequestHeaderMapPtr headers{new TestRequestHeaderMapImpl{
{":authority", original_host}, {":path", "/"}, {":method", "GET"}}};
RequestHeaderMap* updated_headers = headers.get();
decoder_->decodeHeaders(std::move(headers), true);
EXPECT_EQ(updated_host, updated_headers->getHostValue());
// Clean up.
filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose);
}

TEST_F(HttpConnectionManagerImplTest, DateHeaderNotPresent) {
setup(false, "");
setUpEncoderAndDecoder(false, false);
Expand Down
2 changes: 2 additions & 0 deletions test/common/http/conn_manager_impl_test_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan
const Http::Http1Settings& http1Settings() const override { return http1_settings_; }
bool shouldNormalizePath() const override { return normalize_path_; }
bool shouldMergeSlashes() const override { return merge_slashes_; }
bool shouldStripTrailingHostDot() const override { return strip_trailing_host_dot_; }
Http::StripPortType stripPortType() const override { return strip_port_type_; }
const RequestIDExtensionSharedPtr& requestIDExtension() override { return request_id_extension_; }
envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
Expand Down Expand Up @@ -230,6 +231,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan
PathWithEscapedSlashesAction path_with_escaped_slashes_action_{
envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager::
KEEP_UNCHANGED};
bool strip_trailing_host_dot_ = false;
};

} // namespace Http
Expand Down
45 changes: 45 additions & 0 deletions test/common/http/conn_manager_utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1502,6 +1502,51 @@ TEST_F(ConnectionManagerUtilityTest, RemovePort) {
EXPECT_EQ(header_map_none.getHostValue(), "host:9999");
}

// maybeNormalizeHost() removes trailing dot of host from host header.
TEST_F(ConnectionManagerUtilityTest, RemoveTrailingDot) {
ON_CALL(config_, shouldStripTrailingHostDot()).WillByDefault(Return(true));
TestRequestHeaderMapImpl original_headers;
original_headers.setHost("host.");

TestRequestHeaderMapImpl header_map(original_headers);
ConnectionManagerUtility::maybeNormalizeHost(header_map, config_, 0);
EXPECT_EQ(header_map.getHostValue(), "host");

ON_CALL(config_, stripPortType()).WillByDefault(Return(Http::StripPortType::None));
ON_CALL(config_, shouldStripTrailingHostDot()).WillByDefault(Return(true));
TestRequestHeaderMapImpl original_headers_with_port;
original_headers_with_port.setHost("host.:443");

TestRequestHeaderMapImpl header_map_with_port(original_headers_with_port);
ConnectionManagerUtility::maybeNormalizeHost(header_map_with_port, config_, 443);
EXPECT_EQ(header_map_with_port.getHostValue(), "host:443");

ON_CALL(config_, stripPortType()).WillByDefault(Return(Http::StripPortType::MatchingHost));
ON_CALL(config_, shouldStripTrailingHostDot()).WillByDefault(Return(true));
TestRequestHeaderMapImpl original_headers_strip_port;
original_headers_strip_port.setHost("host.:443");

TestRequestHeaderMapImpl header_map_strip_port(original_headers_strip_port);
ConnectionManagerUtility::maybeNormalizeHost(header_map_strip_port, config_, 443);
EXPECT_EQ(header_map_strip_port.getHostValue(), "host");

ON_CALL(config_, shouldStripTrailingHostDot()).WillByDefault(Return(true));
TestRequestHeaderMapImpl original_headers_no_dot;
original_headers_no_dot.setHost("host");

TestRequestHeaderMapImpl header_map_no_dot(original_headers_no_dot);
ConnectionManagerUtility::maybeNormalizeHost(header_map_no_dot, config_, 0);
EXPECT_EQ(header_map_no_dot.getHostValue(), "host");

ON_CALL(config_, shouldStripTrailingHostDot()).WillByDefault(Return(false));
TestRequestHeaderMapImpl original_headers_none;
original_headers_none.setHost("host.");

TestRequestHeaderMapImpl header_map_none(original_headers_none);
ConnectionManagerUtility::maybeNormalizeHost(header_map_none, config_, 0);
EXPECT_EQ(header_map_none.getHostValue(), "host.");
}

// maybeNormalizePath() does not touch escaped slashes when configured to KEEP_UNCHANGED.
TEST_F(ConnectionManagerUtilityTest, KeepEscapedSlashesWhenConfigured) {
ON_CALL(config_, pathWithEscapedSlashesAction())
Expand Down
28 changes: 28 additions & 0 deletions test/common/http/header_utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,34 @@ TEST_F(HeaderUtilityTest, RemovePortsFromHostConnectLegacy) {
}
}

// Host's trailing dot from host header get removed
TEST_F(HeaderUtilityTest, RemoveTrailingDotFromHost) {
const std::vector<std::pair<std::string, std::string>> host_headers{
{"localhost", "localhost"}, // w/o dot
{"localhost.", "localhost"}, // name w/ dot
{"", ""}, // empty
{"192.168.1.1", "192.168.1.1"}, // ipv4
{"abc.com", "abc.com"}, // dns w/o dot
{"abc.com.", "abc.com"}, // dns w/ dot
{"abc.com:443", "abc.com:443"}, // dns port w/o dot
{"abc.com.:443", "abc.com:443"}, // dns port w/ dot
{"[fc00::1]", "[fc00::1]"}, // ipv6
{":", ":"}, // malformed string #1
{"]:", "]:"}, // malformed string #2
{":abc", ":abc"}, // malformed string #3
{".", ""}, // malformed string #4
{"..", "."}, // malformed string #5
{".123", ".123"}, // malformed string #6
{".:.", ".:"} // malformed string #7
};

for (const auto& host_pair : host_headers) {
auto& host_header = hostHeaderEntry(host_pair.first);
HeaderUtility::stripTrailingHostDot(headers_);
EXPECT_EQ(host_header.value().getStringView(), host_pair.second);
}
}

TEST(GetAllOfHeaderAsStringTest, All) {
const LowerCaseString test_header("test");
{
Expand Down
Loading