Skip to content

Commit

Permalink
filter chain manager/tls: comparing the DNS name as case insensitive (#…
Browse files Browse the repository at this point in the history
…14793)

Currently, the comparing of DNS name with filter chain matching and SANs is case sensitive.

Fixes #6199
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
  • Loading branch information
soulxu authored Feb 24, 2021
1 parent f155eaa commit 54c7445
Show file tree
Hide file tree
Showing 8 changed files with 55 additions and 26 deletions.
3 changes: 3 additions & 0 deletions docs/root/version_history/current.rst
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,15 @@ Bug Fixes
* active http health checks: properly handles HTTP/2 GOAWAY frames from the upstream. Previously a GOAWAY frame due to a graceful listener drain could cause improper failed health checks due to streams being refused by the upstream on a connection that is going away. To revert to old GOAWAY handling behavior, set the runtime feature `envoy.reloadable_features.health_check.graceful_goaway_handling` to false.
* buffer: tighten network connection read and write buffer high watermarks in preparation to more careful enforcement of read limits. Buffer high-watermark is now set to the exact configured value; previously it was set to value + 1.
* fault injection: stop counting as active fault after delay elapsed. Previously fault injection filter continues to count the injected delay as an active fault even after it has elapsed. This produces incorrect output statistics and impacts the max number of consecutive faults allowed (e.g., for long-lived streams). This change decreases the active fault count when the delay fault is the only active and has gone finished.
* filter_chain: fix filter chain matching with the server name as the case-insensitive way.
* grpc-web: fix local reply and non-proto-encoded gRPC response handling for small response bodies. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling` to false.
* grpc_http_bridge: the downstream HTTP status is now correctly set for trailers-only responses from the upstream.
* http: disallowing "host:" in request_headers_to_add for behavioral consistency with rejecting :authority header. This behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_host_like_authority` to false.
* http: reverting a behavioral change where upstream connect timeouts were temporarily treated differently from other connection failures. The change back to the original behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure` to false.
* listener: prevent crashing when an unknown listener config proto is received and debug logging is enabled.
* overload: fix a bug that can cause use-after-free when one scaled timer disables another one with the same duration.
* sni: as the server name in sni should be case-insensitive, envoy will convert the server name as lower case first before any other process inside envoy.
* tls: fix the subject alternative name of the presented certificate matches the specified matchers as the case-insensitive way when it uses DNS name.
* upstream: fix handling of moving endpoints between priorities when active health checks are enabled. Previously moving to a higher numbered priority was a NOOP, and moving to a lower numbered priority caused an abort.

Removed Config or Runtime
Expand Down
3 changes: 2 additions & 1 deletion source/common/network/listen_socket_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,8 @@ class ConnectionSocketImpl : public SocketImpl, public ConnectionSocket {
}

void setRequestedServerName(absl::string_view server_name) override {
server_name_ = std::string(server_name);
// Always keep the server_name_ as lower case.
server_name_ = absl::AsciiStrToLower(server_name);
}
absl::string_view requestedServerName() const override { return server_name_; }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -293,16 +293,19 @@ bool DefaultCertValidator::verifySubjectAltName(X509* cert,

bool DefaultCertValidator::dnsNameMatch(const absl::string_view dns_name,
const absl::string_view pattern) {
if (dns_name == pattern) {
const std::string lower_case_dns_name = absl::AsciiStrToLower(dns_name);
const std::string lower_case_pattern = absl::AsciiStrToLower(pattern);
if (lower_case_dns_name == lower_case_pattern) {
return true;
}

size_t pattern_len = pattern.length();
if (pattern_len > 1 && pattern[0] == '*' && pattern[1] == '.') {
if (dns_name.length() > pattern_len - 1) {
const size_t off = dns_name.length() - pattern_len + 1;
return dns_name.substr(0, off).find('.') == std::string::npos &&
dns_name.substr(off, pattern_len - 1) == pattern.substr(1, pattern_len - 1);
size_t pattern_len = lower_case_pattern.length();
if (pattern_len > 1 && lower_case_pattern[0] == '*' && lower_case_pattern[1] == '.') {
if (lower_case_dns_name.length() > pattern_len - 1) {
const size_t off = lower_case_dns_name.length() - pattern_len + 1;
return lower_case_dns_name.substr(0, off).find('.') == std::string::npos &&
lower_case_dns_name.substr(off, pattern_len - 1) ==
lower_case_pattern.substr(1, pattern_len - 1);
}
}

Expand Down
25 changes: 14 additions & 11 deletions source/server/filter_chain_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ void FilterChainManagerImpl::addFilterChains(
source_ips.push_back(cidr_range.asString());
}

std::vector<std::string> server_names;
// Reject partial wildcards, we don't match on them.
for (const auto& server_name : filter_chain_match.server_names()) {
if (server_name.find('*') != std::string::npos && !isWildcardServerName(server_name)) {
Expand All @@ -198,6 +199,7 @@ void FilterChainManagerImpl::addFilterChains(
"\"server_names\"",
address_->asString()));
}
server_names.push_back(absl::AsciiStrToLower(server_name));
}

// Reuse created filter chain if possible.
Expand All @@ -213,7 +215,7 @@ void FilterChainManagerImpl::addFilterChains(
addFilterChainForDestinationPorts(
destination_ports_map_,
PROTOBUF_GET_WRAPPED_OR_DEFAULT(filter_chain_match, destination_port, 0), destination_ips,
filter_chain_match.server_names(), filter_chain_match.transport_protocol(),
server_names, filter_chain_match.transport_protocol(),
filter_chain_match.application_protocols(), filter_chain_match.source_type(), source_ips,
filter_chain_match.source_ports(), filter_chain_impl);

Expand Down Expand Up @@ -262,7 +264,7 @@ void FilterChainManagerImpl::copyOrRebuildDefaultFilterChain(
void FilterChainManagerImpl::addFilterChainForDestinationPorts(
DestinationPortsMap& destination_ports_map, uint16_t destination_port,
const std::vector<std::string>& destination_ips,
const absl::Span<const std::string* const> server_names, const std::string& transport_protocol,
const absl::Span<const std::string> server_names, const std::string& transport_protocol,
const absl::Span<const std::string* const> application_protocols,
const envoy::config::listener::v3::FilterChainMatch::ConnectionSourceType source_type,
const std::vector<std::string>& source_ips,
Expand All @@ -279,7 +281,7 @@ void FilterChainManagerImpl::addFilterChainForDestinationPorts(

void FilterChainManagerImpl::addFilterChainForDestinationIPs(
DestinationIPsMap& destination_ips_map, const std::vector<std::string>& destination_ips,
const absl::Span<const std::string* const> server_names, const std::string& transport_protocol,
const absl::Span<const std::string> server_names, const std::string& transport_protocol,
const absl::Span<const std::string* const> application_protocols,
const envoy::config::listener::v3::FilterChainMatch::ConnectionSourceType source_type,
const std::vector<std::string>& source_ips,
Expand All @@ -299,8 +301,8 @@ void FilterChainManagerImpl::addFilterChainForDestinationIPs(
}

void FilterChainManagerImpl::addFilterChainForServerNames(
ServerNamesMapSharedPtr& server_names_map_ptr,
const absl::Span<const std::string* const> server_names, const std::string& transport_protocol,
ServerNamesMapSharedPtr& server_names_map_ptr, const absl::Span<const std::string> server_names,
const std::string& transport_protocol,
const absl::Span<const std::string* const> application_protocols,
const envoy::config::listener::v3::FilterChainMatch::ConnectionSourceType source_type,
const std::vector<std::string>& source_ips,
Expand All @@ -316,16 +318,16 @@ void FilterChainManagerImpl::addFilterChainForServerNames(
application_protocols, source_type, source_ips,
source_ports, filter_chain);
} else {
for (const auto& server_name_ptr : server_names) {
if (isWildcardServerName(*server_name_ptr)) {
for (const auto& server_name : server_names) {
if (isWildcardServerName(server_name)) {
// Add mapping for the wildcard domain, i.e. ".example.com" for "*.example.com".
addFilterChainForApplicationProtocols(
server_names_map[server_name_ptr->substr(1)][transport_protocol], application_protocols,
server_names_map[server_name.substr(1)][transport_protocol], application_protocols,
source_type, source_ips, source_ports, filter_chain);
} else {
addFilterChainForApplicationProtocols(
server_names_map[*server_name_ptr][transport_protocol], application_protocols,
source_type, source_ips, source_ports, filter_chain);
addFilterChainForApplicationProtocols(server_names_map[server_name][transport_protocol],
application_protocols, source_type, source_ips,
source_ports, filter_chain);
}
}
}
Expand Down Expand Up @@ -470,6 +472,7 @@ const Network::FilterChain* FilterChainManagerImpl::findFilterChainForDestinatio

const Network::FilterChain* FilterChainManagerImpl::findFilterChainForServerName(
const ServerNamesMap& server_names_map, const Network::ConnectionSocket& socket) const {
ASSERT(absl::AsciiStrToLower(socket.requestedServerName()) == socket.requestedServerName());
const std::string server_name(socket.requestedServerName());

// Match on exact server name, i.e. "www.example.com" for "www.example.com".
Expand Down
9 changes: 3 additions & 6 deletions source/server/filter_chain_manager_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -253,26 +253,23 @@ class FilterChainManagerImpl : public Network::FilterChainManager,
void addFilterChainForDestinationPorts(
DestinationPortsMap& destination_ports_map, uint16_t destination_port,
const std::vector<std::string>& destination_ips,
const absl::Span<const std::string* const> server_names,
const std::string& transport_protocol,
const absl::Span<const std::string> server_names, const std::string& transport_protocol,
const absl::Span<const std::string* const> application_protocols,
const envoy::config::listener::v3::FilterChainMatch::ConnectionSourceType source_type,
const std::vector<std::string>& source_ips,
const absl::Span<const Protobuf::uint32> source_ports,
const Network::FilterChainSharedPtr& filter_chain);
void addFilterChainForDestinationIPs(
DestinationIPsMap& destination_ips_map, const std::vector<std::string>& destination_ips,
const absl::Span<const std::string* const> server_names,
const std::string& transport_protocol,
const absl::Span<const std::string> server_names, const std::string& transport_protocol,
const absl::Span<const std::string* const> application_protocols,
const envoy::config::listener::v3::FilterChainMatch::ConnectionSourceType source_type,
const std::vector<std::string>& source_ips,
const absl::Span<const Protobuf::uint32> source_ports,
const Network::FilterChainSharedPtr& filter_chain);
void addFilterChainForServerNames(
ServerNamesMapSharedPtr& server_names_map_ptr,
const absl::Span<const std::string* const> server_names,
const std::string& transport_protocol,
const absl::Span<const std::string> server_names, const std::string& transport_protocol,
const absl::Span<const std::string* const> application_protocols,
const envoy::config::listener::v3::FilterChainMatch::ConnectionSourceType source_type,
const std::vector<std::string>& source_ips,
Expand Down
10 changes: 10 additions & 0 deletions test/common/network/listen_socket_impl_test.cc
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "envoy/common/platform.h"
#include "envoy/config/core/v3/base.pb.h"
#include "envoy/network/exception.h"
#include "envoy/network/socket.h"

#include "common/api/os_sys_calls_impl.h"
#include "common/network/io_socket_handle_impl.h"
Expand All @@ -21,6 +22,15 @@ namespace Envoy {
namespace Network {
namespace {

TEST(ConnectionSocketImplTest, LowerCaseRequestedServerName) {
absl::string_view serverName("www.EXAMPLE.com");
absl::string_view expectedServerName("www.example.com");
auto loopback_addr = Network::Test::getCanonicalLoopbackAddress(Address::IpVersion::v4);
auto conn_socket_ = ConnectionSocketImpl(Socket::Type::Stream, loopback_addr, loopback_addr);
conn_socket_.setRequestedServerName(serverName);
EXPECT_EQ(expectedServerName, conn_socket_.requestedServerName());
}

template <Network::Socket::Type Type>
class ListenSocketImplTest : public testing::TestWithParam<Address::IpVersion> {
protected:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ namespace Tls {
TEST(DefaultCertValidatorTest, TestDnsNameMatching) {
EXPECT_TRUE(DefaultCertValidator::dnsNameMatch("lyft.com", "lyft.com"));
EXPECT_TRUE(DefaultCertValidator::dnsNameMatch("a.lyft.com", "*.lyft.com"));
EXPECT_TRUE(DefaultCertValidator::dnsNameMatch("a.LYFT.com", "*.lyft.COM"));
EXPECT_FALSE(DefaultCertValidator::dnsNameMatch("a.b.lyft.com", "*.lyft.com"));
EXPECT_FALSE(DefaultCertValidator::dnsNameMatch("foo.test.com", "*.lyft.com"));
EXPECT_FALSE(DefaultCertValidator::dnsNameMatch("lyft.com", "*.lyft.com"));
Expand Down
13 changes: 12 additions & 1 deletion test/server/filter_chain_manager_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ class FilterChainManagerImplTest : public testing::Test {
mock_socket->address_provider_->setLocalAddress(local_address_);

ON_CALL(*mock_socket, requestedServerName())
.WillByDefault(Return(absl::string_view(server_name)));
.WillByDefault(Return(absl::AsciiStrToLower(server_name)));
ON_CALL(*mock_socket, detectedTransportProtocol())
.WillByDefault(Return(absl::string_view(transport_protocol)));
ON_CALL(*mock_socket, requestedApplicationProtocols())
Expand Down Expand Up @@ -149,6 +149,17 @@ TEST_F(FilterChainManagerImplTest, FilterChainMatchNothing) {
EXPECT_EQ(filter_chain, nullptr);
}

TEST_F(FilterChainManagerImplTest, FilterChainMatchCaseInSensitive) {
envoy::config::listener::v3::FilterChain new_filter_chain = filter_chain_template_;
new_filter_chain.mutable_filter_chain_match()->add_server_names("foo.EXAMPLE.com");
filter_chain_manager_.addFilterChains(
std::vector<const envoy::config::listener::v3::FilterChain*>{&new_filter_chain}, nullptr,
filter_chain_factory_builder_, filter_chain_manager_);
auto filter_chain =
findFilterChainHelper(10000, "127.0.0.1", "FOO.example.com", "tls", {}, "8.8.8.8", 111);
EXPECT_NE(filter_chain, nullptr);
}

TEST_F(FilterChainManagerImplTest, AddSingleFilterChain) {
addSingleFilterChainHelper(filter_chain_template_);
auto* filter_chain = findFilterChainHelper(10000, "127.0.0.1", "", "tls", {}, "8.8.8.8", 111);
Expand Down

0 comments on commit 54c7445

Please sign in to comment.