Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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,16 @@ 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

Copy link
Copy Markdown
Contributor Author

@maheshkurund maheshkurund May 15, 2021

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 the incoming request contains a host/authority header that includes a port number,
// setting this option will strip a trailing dot, if present, from the host section,
// leaving the port 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.

1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ New Features
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
19 changes: 19 additions & 0 deletions source/common/http/header_utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,25 @@ 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.
// 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.
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
41 changes: 41 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,47 @@ TEST_F(HttpConnectionManagerImplTest, RouteShouldUseNormalizedHost) {
filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose);
}

// Observe that we strip the trailing dot.
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, HostWithoutTrailingDot) {
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