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

dns_filter: add support for wildcard name in DNS filter inline table #34588

Merged
merged 6 commits into from
Jul 9, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
10 changes: 9 additions & 1 deletion api/envoy/data/dns/v3/dns_table.proto
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,15 @@ message DnsTable {
option (udpa.annotations.versioning).previous_message_type =
"envoy.data.dns.v2alpha.DnsTable.DnsVirtualDomain";

// A domain name for which Envoy will respond to query requests
// A domain name for which Envoy will respond to query requests.
// Wildcard records are supported on the first label only, e.g. ``*.example.com`` or ``*.subdomain.example.com``.
// Names such as ``*example.com``, ``subdomain.*.example.com``, ``*subdomain.example.com``, etc
// are not valid wildcard names and asterisk will be interpreted as a literal ``*`` character.
// Wildcard records match subdomains on any levels, e.g. ``*.example.com`` will match
// ``foo.example.com``, ``bar.foo.example.com``, ``baz.bar.foo.example.com``, etc. In case there are multiple
// wildcard records, the longest wildcard match will be used, e.g. if there are wildcard records for
// ``*.example.com`` and ``*.foo.example.com`` and the query is for ``bar.foo.example.com``, the latter will be used.
// Specific records will always take precedence over wildcard records.
string name = 1 [(validate.rules).string = {min_len: 1 well_known_regex: HTTP_HEADER_NAME}];

// The configuration containing the method to determine the address of this endpoint
Expand Down
5 changes: 5 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,11 @@ new_features:
change: |
Added :ref:`disable_id_token_set_cookie <envoy_v3_api_field_extensions.filters.http.oauth2.v3.OAuth2Config.disable_id_token_set_cookie>`
to disable setting the ID Token cookie.
- area: dns_filter
change: |
Added support for wildcard resolution in :ref:`inline_dns_table
<envoy_v3_api_field_extensions.filters.udp.dns_filter.v3.DnsFilterConfig.ServerContextConfig.inline_dns_table>`
when DNS filter is working in server mode.

deprecated:
- area: tracing
Expand Down
40 changes: 25 additions & 15 deletions source/extensions/filters/udp/dns_filter/dns_filter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,11 @@ DnsFilterEnvoyConfig::DnsFilterEnvoyConfig(
for (const auto& virtual_domain : dns_table.virtual_domains()) {
AddressConstPtrVec addrs{};

const absl::string_view domain_name = virtual_domain.name();
const absl::string_view suffix = Utils::getDomainSuffix(domain_name);
ENVOY_LOG(trace, "Loading configuration for domain: {}. Suffix: {}", domain_name, suffix);
const absl::string_view virtual_domain_name =
Utils::getVirtualDomainName(virtual_domain.name());
const absl::string_view suffix = Utils::getDomainSuffix(virtual_domain_name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we getDomainSuffix(virtual_domain.name()),
or getDomainSuffix( Utils::getVirtualDomainName(virtual_domain.name()))

Can these two return different result in some cases?

Copy link
Contributor Author

@StupidScience StupidScience Jun 13, 2024

Choose a reason for hiding this comment

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

Yeah, I thought about this (getting suffix of "initial" input domain name). But with current implementation result should always be the same and for me it makes more sense to extract suffix after manipulations

ENVOY_LOG(trace, "Loading configuration for domain: {}. Suffix: {}", virtual_domain_name,
suffix);

if (virtual_domain.endpoint().has_address_list()) {
const auto& address_list = virtual_domain.endpoint().address_list().address();
Expand Down Expand Up @@ -69,7 +71,7 @@ DnsFilterEnvoyConfig::DnsFilterEnvoyConfig(
if (virtual_domains != nullptr) {
// The suffix already has a node in the trie

auto existing_endpoint_config = virtual_domains->find(domain_name);
auto existing_endpoint_config = virtual_domains->find(virtual_domain_name);
if (existing_endpoint_config != virtual_domains->end()) {
// Update the existing endpoint config with the new addresses

Expand All @@ -79,11 +81,11 @@ DnsFilterEnvoyConfig::DnsFilterEnvoyConfig(
} else {
// Add a new endpoint config for the new domain
endpoint_config.address_list = absl::make_optional<AddressConstPtrVec>(std::move(addrs));
virtual_domains->emplace(std::string(domain_name), std::move(endpoint_config));
virtual_domains->emplace(std::string(virtual_domain_name), std::move(endpoint_config));
}
} else {
endpoint_config.address_list = absl::make_optional<AddressConstPtrVec>(std::move(addrs));
addEndpointToSuffix(suffix, domain_name, endpoint_config);
addEndpointToSuffix(suffix, virtual_domain_name, endpoint_config);
}
}

Expand Down Expand Up @@ -147,20 +149,20 @@ DnsFilterEnvoyConfig::DnsFilterEnvoyConfig(
// See if there's a suffix already configured
auto virtual_domains = dns_lookup_trie_.find(suffix);
if (virtual_domains == nullptr) {
addEndpointToSuffix(suffix, domain_name, endpoint_config);
addEndpointToSuffix(suffix, virtual_domain_name, endpoint_config);
} else {
// A domain can be redirected to one cluster. If it appears multiple times, the first
// entry is the only one used
if (virtual_domains->find(domain_name) == virtual_domains->end()) {
virtual_domains->emplace(domain_name, std::move(endpoint_config));
if (virtual_domains->find(virtual_domain_name) == virtual_domains->end()) {
virtual_domains->emplace(virtual_domain_name, std::move(endpoint_config));
}
}
}

std::chrono::seconds ttl = virtual_domain.has_answer_ttl()
? std::chrono::seconds(virtual_domain.answer_ttl().seconds())
: DEFAULT_RESOLVER_TTL;
domain_ttl_.emplace(virtual_domain.name(), ttl);
domain_ttl_.emplace(virtual_domain_name, ttl);
}

forward_queries_ = config.has_client_config();
Expand Down Expand Up @@ -393,12 +395,20 @@ const DnsEndpointConfig* DnsFilter::getEndpointConfigForDomain(const absl::strin
return nullptr;
}

const auto iter = virtual_domains->find(domain);
if (iter == virtual_domains->end()) {
ENVOY_LOG(debug, "No endpoint configuration exists for [{}]", domain);
return nullptr;
// Try to find exact match at first and then look for possible wildcard match
// moving to the next label on each iteration.
size_t pos = 0;
while (pos != domain.npos) {
const auto iter = virtual_domains->find(domain.substr(pos));
if (iter != virtual_domains->end()) {
return &(iter->second);
}

pos = domain.find('.', pos + 1);
}
return &(iter->second);

ENVOY_LOG(debug, "No endpoint configuration exists for [{}]", domain);
return nullptr;
}

const DnsSrvRecord* DnsFilter::getServiceConfigForDomain(const absl::string_view domain) {
Expand Down
13 changes: 13 additions & 0 deletions source/extensions/filters/udp/dns_filter/dns_filter_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,19 @@ absl::string_view getDomainSuffix(const absl::string_view name) {
return name.substr(pos + 1);
}

absl::string_view getVirtualDomainName(const absl::string_view domain_name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we call this function getWildcardDomainName?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be incorrect. It returns not only wildcard domain names but any name.
I'm not quite happy with naming here tho. Will smth like sanitizeVirtualDomainName make intent more clear?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's okay to me then.

// We can use names started with '.' as wildcard records in virtual domain name config
// since these are not valid domain names and in this way we optimize for future search against
// them. We expect only names like *.foo.com as a valid wildcard records because any other
// wildcard usages are not considered as valid ones, i.e. **.foo.com, *foo.bar.com, foo*.bar.com
// are all invalid.
if (domain_name.starts_with("*.")) {
return domain_name.substr(1);
Copy link
Contributor

@yanjunxiang-google yanjunxiang-google Jun 12, 2024

Choose a reason for hiding this comment

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

What if the domain_name is invalid, like just "*.". This will return an empty string. Can we handle it correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If domain is *. it will return just .. You can see that in tests.
While working on these code I made few assumptions and cut the corners sometimes. Mostly because current code does not have any validation for passed domain names and in proto validations are: length >= 1 + header value regex, that's probably incorrect.
I did not want to increase the scope of my changes so decided not to cover validations. It is probably should be addressed, likely in different PR tho

}

return domain_name;
}

} // namespace Utils
} // namespace DnsFilter
} // namespace UdpFilters
Expand Down
7 changes: 7 additions & 0 deletions source/extensions/filters/udp/dns_filter/dns_filter_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,13 @@ getAddressRecordType(const Network::Address::InstanceConstSharedPtr& ipaddr);
*/
absl::string_view getDomainSuffix(const absl::string_view name);

/**
* @brief For given domain name return virtual domain name that will be used for internal
* resolution. Valid wildcard names are sanitized and converted to format to store in
* virtual domain config.
*/
absl::string_view getVirtualDomainName(const absl::string_view domain_name);

} // namespace Utils
} // namespace DnsFilter
} // namespace UdpFilters
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,13 @@ name: listener_0
- cluster_name: "cluster_0"
weight: 20
priority: 10
- name: "*.foo1.com"
endpoint:
address_list:
address:
- 10.10.0.1
- 10.10.0.2
- 10.10.0.3
)EOF",
addr->ip()->addressAsString(), addr->ip()->addressAsString(),
addr->ip()->port());
Expand Down Expand Up @@ -401,6 +408,24 @@ TEST_P(DnsFilterIntegrationTest, ClusterEndpointWithoutPortServiceRecordLookupTe

EXPECT_EQ(endpoints, ports.size());
}

TEST_P(DnsFilterIntegrationTest, WildcardLookupTest) {
setup(0);
const uint32_t port = lookupPort("listener_0");
const auto listener_address = *Network::Utility::resolveUrl(
fmt::format("tcp://{}:{}", Network::Test::getLoopbackAddressUrlString(version_), port));

Network::UdpRecvData response;
std::string query =
Utils::buildQueryForDomain("wild.foo1.com", DNS_RECORD_TYPE_A, DNS_RECORD_CLASS_IN);
requestResponseWithListenerAddress(*listener_address, query, response);

response_ctx_ = ResponseValidator::createResponseContext(response, counters_);
EXPECT_TRUE(response_ctx_->parse_status_);

EXPECT_EQ(3, response_ctx_->answers_.size());
EXPECT_EQ(DNS_RESPONSE_CODE_NO_ERROR, response_ctx_->getQueryResponseCode());
}
} // namespace
} // namespace DnsFilter
} // namespace UdpFilters
Expand Down
127 changes: 127 additions & 0 deletions test/extensions/filters/udp/dns_filter/dns_filter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2433,6 +2433,133 @@ stat_prefix: "my_prefix"
EXPECT_EQ(1, config_->stats().known_domain_queries_.value());
}

TEST_F(DnsFilterTest, WildcardName) {
InSequence s;

const std::string wildcard_virtual_domain = R"EOF(
stat_prefix: "my_prefix"
server_config:
inline_dns_table:
external_retry_count: 0
virtual_domains:
- name: "*.foobaz.com"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add some tests for invalid virtual_domain test, like ".com", ".", "com", and see whether there is any issue?

Copy link
Contributor Author

@StupidScience StupidScience Jun 13, 2024

Choose a reason for hiding this comment

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

As I mentioned above, invalid domains will be accepted by envoy and (do not) work due to lack of validations.
Just answering how it will work:

  • .com will behave exactly as *.com and I'm not sure whether it is bug or feature with the current state
  • . will be accepted. We can test if that will work as catch-all wildcard for queries ended with period like foo.bar.. Shall we?
  • com in general looks like valid record

endpoint:
address_list:
address:
- "10.0.0.1"
)EOF";
setup(wildcard_virtual_domain);

const std::list<std::string> expected_address{"10.0.0.1"};
const std::string domain("www.foobaz.com");

const std::string query =
Utils::buildQueryForDomain(domain, DNS_RECORD_TYPE_A, DNS_RECORD_CLASS_IN);
ASSERT_FALSE(query.empty());
sendQueryFromClient("10.0.0.1:1000", query);

response_ctx_ = ResponseValidator::createResponseContext(udp_response_, counters_);
EXPECT_TRUE(response_ctx_->parse_status_);
EXPECT_EQ(DNS_RESPONSE_CODE_NO_ERROR, response_ctx_->getQueryResponseCode());

for (const auto& answer : response_ctx_->answers_) {
EXPECT_EQ(answer.first, domain);
Utils::verifyAddress(expected_address, answer.second);
}

// Validate stats
EXPECT_EQ(1, config_->stats().downstream_rx_queries_.value());
EXPECT_EQ(1, config_->stats().known_domain_queries_.value());
}

TEST_F(DnsFilterTest, WildcardSubdomainPrevails) {
InSequence s;

const std::string wildcard_with_subdomain_virtual_domain = R"EOF(
stat_prefix: "my_prefix"
server_config:
inline_dns_table:
external_retry_count: 0
virtual_domains:
- name: "*.foo1.com"
endpoint:
address_list:
address:
- "10.0.0.1"
- name: "*.foo2.foo1.com"
endpoint:
address_list:
address:
- "10.0.0.2"
)EOF";
setup(wildcard_with_subdomain_virtual_domain);

const std::list<std::string> expected_address{"10.0.0.2"};
const std::string domain("www.foo2.foo1.com");

const std::string query =
Utils::buildQueryForDomain(domain, DNS_RECORD_TYPE_A, DNS_RECORD_CLASS_IN);
ASSERT_FALSE(query.empty());
sendQueryFromClient("10.0.0.1:1000", query);

response_ctx_ = ResponseValidator::createResponseContext(udp_response_, counters_);
EXPECT_TRUE(response_ctx_->parse_status_);
EXPECT_EQ(DNS_RESPONSE_CODE_NO_ERROR, response_ctx_->getQueryResponseCode());

for (const auto& answer : response_ctx_->answers_) {
EXPECT_EQ(answer.first, domain);
Utils::verifyAddress(expected_address, answer.second);
}

// Validate stats
EXPECT_EQ(1, config_->stats().downstream_rx_queries_.value());
EXPECT_EQ(1, config_->stats().known_domain_queries_.value());
}

TEST_F(DnsFilterTest, WildcardExactNamePrevails) {
InSequence s;

const std::string wildcard_and_exact_name_virtual_domain = R"EOF(
stat_prefix: "my_prefix"
server_config:
inline_dns_table:
external_retry_count: 0
virtual_domains:
- name: "*.foo1.com"
endpoint:
address_list:
address:
- "10.0.0.1"
- name: "bar.foo1.com"
endpoint:
address_list:
address:
- "10.0.0.2"
)EOF";
setup(wildcard_and_exact_name_virtual_domain);

const std::list<std::string> expected_address{"10.0.0.2"};
const std::string domain("bar.foo1.com");

const std::string query =
Utils::buildQueryForDomain(domain, DNS_RECORD_TYPE_A, DNS_RECORD_CLASS_IN);
ASSERT_FALSE(query.empty());
sendQueryFromClient("10.0.0.1:1000", query);

response_ctx_ = ResponseValidator::createResponseContext(udp_response_, counters_);
EXPECT_TRUE(response_ctx_->parse_status_);
EXPECT_EQ(DNS_RESPONSE_CODE_NO_ERROR, response_ctx_->getQueryResponseCode());

for (const auto& answer : response_ctx_->answers_) {
EXPECT_EQ(answer.first, domain);
Utils::verifyAddress(expected_address, answer.second);
}

// Validate stats
EXPECT_EQ(1, config_->stats().downstream_rx_queries_.value());
EXPECT_EQ(1, config_->stats().known_domain_queries_.value());
}

} // namespace
} // namespace DnsFilter
} // namespace UdpFilters
Expand Down
26 changes: 26 additions & 0 deletions test/extensions/filters/udp/dns_filter/dns_filter_utils_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@ TEST_F(DnsFilterUtilsTest, GetDomainSuffixTest) {
{"_ldap._tcp.Default-First-Site-Name._sites.dc._msdcs.utelsystems.local",
"utelsystems.local"},
{"primary.voip.subzero.com", "subzero.com"},
{"*.voip.subzero.com", "subzero.com"},
{"*.subzero.com", "subzero.com"},
{".subzero.com", "subzero.com"},
{"subzero.com", "subzero.com"},
{"subzero", "subzero"},
{".com", "com"},
Expand All @@ -120,6 +123,29 @@ TEST_F(DnsFilterUtilsTest, GetDomainSuffixTest) {
}
}

TEST_F(DnsFilterUtilsTest, GetVirtualDomainName) {
struct DomainSuffixTestData {
const std::string domain;
const std::string expected_name;
} suffix_data[] = {
{"_ldap._tcp.Default-First-Site-Name._sites.dc._msdcs.utelsystems.local",
"_ldap._tcp.Default-First-Site-Name._sites.dc._msdcs.utelsystems.local"},
{"primary.voip.subzero.com", "primary.voip.subzero.com"},
{"*.subzero.com", ".subzero.com"},
{"*www.subzero.com", "*www.subzero.com"},
{"subzero", "subzero"},
{".com", ".com"},
{"*.", "."},
{".", "."},
{"", ""},
};

for (auto& ptr : suffix_data) {
const absl::string_view result = Utils::getVirtualDomainName(ptr.domain);
EXPECT_EQ(ptr.expected_name, result);
}
}

} // namespace
} // namespace Utils
} // namespace DnsFilter
Expand Down