From a0c0800fc7f510202ccba53babc67b2230361b59 Mon Sep 17 00:00:00 2001 From: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> Date: Fri, 28 Jan 2022 05:14:07 +0000 Subject: [PATCH 01/10] SAN with wildcard after string match dns name Signed-off-by: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> --- .../transport_sockets/tls/utility.cc | 70 +++++++++++++++++++ .../transport_sockets/tls/utility.h | 12 +++- .../transport_sockets/tls/utility_test.cc | 16 ++++- 3 files changed, 95 insertions(+), 3 deletions(-) diff --git a/source/extensions/transport_sockets/tls/utility.cc b/source/extensions/transport_sockets/tls/utility.cc index e2b220c9468ed..a52091b1f1b98 100644 --- a/source/extensions/transport_sockets/tls/utility.cc +++ b/source/extensions/transport_sockets/tls/utility.cc @@ -3,6 +3,7 @@ #include "source/common/common/assert.h" #include "source/common/common/empty_string.h" #include "source/common/common/safe_memcpy.h" +#include "source/common/common/utility.h" #include "source/common/network/address_impl.h" #include "source/common/protobuf/utility.h" @@ -71,13 +72,60 @@ Envoy::Ssl::CertificateDetailsPtr Utility::certificateDetails(X509* cert, const return certificate_details; } +bool Utility::labelWildcardMatch(absl::string_view dns_label, absl::string_view pattern) { + if (pattern.empty()) { + return dns_label.empty(); + } + + // Only valid if wildcard character appear once + if (std::count(pattern.begin(), pattern.end(), '*') == 1) { + constexpr char glob = '*'; + // Check the special case of a single * pattern, as it's common. + if (pattern.size() == 1 && pattern[0] == glob) { + return true; + } + size_t i = 0, p = 0, i_star = dns_label.size(), p_star = 0; + while (i < dns_label.size()) { + if (p < pattern.size() && + absl::ascii_tolower(dns_label[i]) == absl::ascii_tolower(pattern[p])) { + ++i; + ++p; + } else if (p < pattern.size() && pattern[p] == glob) { + i_star = i; + p_star = p++; + } else if (i_star != dns_label.size()) { + i = ++i_star; + p = p_star + 1; + } else { + return false; + } + } + + while (p < pattern.size() && pattern[p] == glob) { + ++p; + } + + return p == pattern.size() && i == dns_label.size(); + } + + return false; +} + bool Utility::dnsNameMatch(absl::string_view dns_name, absl::string_view pattern) { + // A-label ACE prefix https://www.rfc-editor.org/rfc/rfc5890#section-2.3.2.5 + constexpr absl::string_view ACE_prefix = "xn--"; 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; } + // https://www.rfc-editor.org/rfc/rfc6125#section-6.4.3 part #2 + // If the wildcard character is the only character of the left-most + // label in the presented identifier, the client SHOULD NOT compare + // against anything but the left-most label of the reference identifier + // (e.g., *.example.com would match foo.example.com but + // not bar.foo.example.com or example.com) 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) { @@ -88,6 +136,28 @@ bool Utility::dnsNameMatch(absl::string_view dns_name, absl::string_view pattern } } + // https://www.rfc-editor.org/rfc/rfc6125#section-6.4.3 part #3 + // Match a presented identifier in which the wildcard character is not the only + // character of the label and don't match patter if the wildcard character is + // embedded within an A-label (A-label always starts with the ACE prefix "xn--") + // (e.g., baz*.example.net and *baz.example.net and b*z.example.net would + // be taken to match baz1.example.net and foobaz.example.net and + // buzz.example.net, respectively) + size_t pattern_left_label_len = lower_case_pattern.find('.'); + size_t dns_name_left_label_len = lower_case_dns_name.find('.'); + // Only the left-most label in the pattern contains wildcard '*' and is not an A-label + if ((pattern_left_label_len != std::string::npos) && + (dns_name_left_label_len != std::string::npos) && + (lower_case_pattern.find('*') != std::string::npos) && + (lower_case_pattern.find('*') < pattern_left_label_len) && + (lower_case_pattern.substr(pattern_left_label_len).find('*') == std::string::npos) && + (!absl::StartsWith(lower_case_pattern.substr(0, pattern_left_label_len), ACE_prefix))) { + return labelWildcardMatch(lower_case_dns_name.substr(0, dns_name_left_label_len), + lower_case_pattern.substr(0, pattern_left_label_len)) && + (lower_case_dns_name.substr(dns_name_left_label_len) == + lower_case_pattern.substr(pattern_left_label_len)); + } + return false; } diff --git a/source/extensions/transport_sockets/tls/utility.h b/source/extensions/transport_sockets/tls/utility.h index e38589e30d887..abb71dedf751c 100644 --- a/source/extensions/transport_sockets/tls/utility.h +++ b/source/extensions/transport_sockets/tls/utility.h @@ -21,13 +21,23 @@ Envoy::Ssl::CertificateDetailsPtr certificateDetails(X509* cert, const std::stri TimeSource& time_source); /** - * Determines whether the given name matches 'pattern' which may optionally begin with a wildcard. + * Determines whether the given name matches 'pattern' which may optionally begin with a wildcard + * or even if names may contain the wildcard in between. * @param dns_name the DNS name to match * @param pattern the pattern to match against (*.example.com) * @return true if the san matches pattern */ bool dnsNameMatch(absl::string_view dns_name, absl::string_view pattern); +/** + * Determines whether the given DNS label matches 'pattern' which may contain a wildcard in between. + * e.g., "baz*" and "*baz" and "b*z" would match "baz1" and "foobaz" and "buzz", respectively. + * @param dns_label the DNS name label to match + * @param pattern the pattern to match against + * @return true if the dns_label matches pattern + */ +bool labelWildcardMatch(absl::string_view dns_label, absl::string_view pattern); + /** * Retrieves the serial number of a certificate. * @param cert the certificate diff --git a/test/extensions/transport_sockets/tls/utility_test.cc b/test/extensions/transport_sockets/tls/utility_test.cc index 22aef56c88ecf..26f48b1077700 100644 --- a/test/extensions/transport_sockets/tls/utility_test.cc +++ b/test/extensions/transport_sockets/tls/utility_test.cc @@ -29,12 +29,24 @@ TEST(UtilityTest, TestDnsNameMatching) { EXPECT_TRUE(Utility::dnsNameMatch("lyft.com", "lyft.com")); EXPECT_TRUE(Utility::dnsNameMatch("a.lyft.com", "*.lyft.com")); EXPECT_TRUE(Utility::dnsNameMatch("a.LYFT.com", "*.lyft.COM")); + EXPECT_TRUE(Utility::dnsNameMatch(".lyft.com", "*.lyft.com")); + EXPECT_TRUE(Utility::dnsNameMatch("lyft.com", "*yft.com")); + EXPECT_TRUE(Utility::dnsNameMatch("LYFT.com", "*yft.com")); + EXPECT_TRUE(Utility::dnsNameMatch("lyft.com", "*lyft.com")); + EXPECT_TRUE(Utility::dnsNameMatch("lyft.com", "lyf*.com")); + EXPECT_TRUE(Utility::dnsNameMatch("lyft.com", "lyft*.com")); + EXPECT_TRUE(Utility::dnsNameMatch("lyft.com", "l*ft.com")); + EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "l*ft.co")); + EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "ly?t.com")); + EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "lf*t.com")); + EXPECT_FALSE(Utility::dnsNameMatch(".lyft.com", "*lyft.com")); + EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "**lyft.com")); + EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "lyft**.com")); + EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "ly**ft.com")); EXPECT_FALSE(Utility::dnsNameMatch("a.b.lyft.com", "*.lyft.com")); EXPECT_FALSE(Utility::dnsNameMatch("foo.test.com", "*.lyft.com")); EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "*.lyft.com")); EXPECT_FALSE(Utility::dnsNameMatch("alyft.com", "*.lyft.com")); - EXPECT_FALSE(Utility::dnsNameMatch("alyft.com", "*lyft.com")); - EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "*lyft.com")); EXPECT_FALSE(Utility::dnsNameMatch("", "*lyft.com")); EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "")); } From 49c931039b3b8a9f4cca4c9d7c5fb24e751e0ccb Mon Sep 17 00:00:00 2001 From: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> Date: Fri, 28 Jan 2022 05:42:03 +0000 Subject: [PATCH 02/10] include bug description in release notes Signed-off-by: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> --- docs/root/version_history/current.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index f315cd6dc2510..3b120e5ccfcd2 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -22,6 +22,7 @@ Bug Fixes * data plane: fixing error handling where writing to a socket failed while under the stack of processing. This should genreally affect HTTP/3. This behavioral change can be reverted by setting ``envoy.reloadable_features.allow_upstream_inline_write`` to false. * eds: fix the eds cluster update by allowing update on the locality of the cluster endpoints. This behavioral change can be temporarily reverted by setting runtime guard ``envoy.reloadable_features.support_locality_update_on_eds_cluster_endpoints`` to false. +* tls: fix a bug while matching a certificate SAN with an exact value in ``match_typed_subject_alt_names`` of a listener where wildcard ``*`` character is not the only character of the dns label. Example, ``baz*.example.net`` and ``*baz.example.net`` and ``b*z.example.net`` will match ``baz1.example.net`` and ``foobaz.example.net`` and ``buzz.example.net``, respectively. * xray: fix the AWS X-Ray tracer extension to not sample the trace if ``sampled=`` keyword is not present in the header ``x-amzn-trace-id``. Removed Config or Runtime From 8c49d297df4c4b5766f54ecee1e818a6d7df09d1 Mon Sep 17 00:00:00 2001 From: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> Date: Fri, 28 Jan 2022 05:50:52 +0000 Subject: [PATCH 03/10] remove unwanted header Signed-off-by: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> --- source/extensions/transport_sockets/tls/utility.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/source/extensions/transport_sockets/tls/utility.cc b/source/extensions/transport_sockets/tls/utility.cc index a52091b1f1b98..82c8c589e6ec5 100644 --- a/source/extensions/transport_sockets/tls/utility.cc +++ b/source/extensions/transport_sockets/tls/utility.cc @@ -3,7 +3,6 @@ #include "source/common/common/assert.h" #include "source/common/common/empty_string.h" #include "source/common/common/safe_memcpy.h" -#include "source/common/common/utility.h" #include "source/common/network/address_impl.h" #include "source/common/protobuf/utility.h" From 9c8049d5d90c9af0463062f6b7b87ca41f8221f3 Mon Sep 17 00:00:00 2001 From: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> Date: Fri, 28 Jan 2022 05:57:53 +0000 Subject: [PATCH 04/10] update function description Signed-off-by: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> --- source/extensions/transport_sockets/tls/utility.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/source/extensions/transport_sockets/tls/utility.h b/source/extensions/transport_sockets/tls/utility.h index abb71dedf751c..09094f7ba21ec 100644 --- a/source/extensions/transport_sockets/tls/utility.h +++ b/source/extensions/transport_sockets/tls/utility.h @@ -22,16 +22,17 @@ Envoy::Ssl::CertificateDetailsPtr certificateDetails(X509* cert, const std::stri /** * Determines whether the given name matches 'pattern' which may optionally begin with a wildcard - * or even if names may contain the wildcard in between. + * or contain a wildcard inside the pattern's first label. * @param dns_name the DNS name to match - * @param pattern the pattern to match against (*.example.com) + * @param pattern the pattern to match against (*.example.com) or (test*.example.com) * @return true if the san matches pattern */ bool dnsNameMatch(absl::string_view dns_name, absl::string_view pattern); /** - * Determines whether the given DNS label matches 'pattern' which may contain a wildcard in between. - * e.g., "baz*" and "*baz" and "b*z" would match "baz1" and "foobaz" and "buzz", respectively. + * Determines whether the given DNS label matches 'pattern' which may contain a wildcard. e.g., + * patterns "baz*" and "*baz" and "b*z" would match DNS labels "baz1" and "foobaz" and "buzz", + * respectively. * @param dns_label the DNS name label to match * @param pattern the pattern to match against * @return true if the dns_label matches pattern From a41b38e9eea53958419d0cdc1fec875be0d31ced Mon Sep 17 00:00:00 2001 From: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> Date: Fri, 28 Jan 2022 06:12:33 +0000 Subject: [PATCH 05/10] more coverage of unit tests Signed-off-by: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> --- test/extensions/transport_sockets/tls/utility_test.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/extensions/transport_sockets/tls/utility_test.cc b/test/extensions/transport_sockets/tls/utility_test.cc index 26f48b1077700..9ab9eef23a273 100644 --- a/test/extensions/transport_sockets/tls/utility_test.cc +++ b/test/extensions/transport_sockets/tls/utility_test.cc @@ -36,6 +36,7 @@ TEST(UtilityTest, TestDnsNameMatching) { EXPECT_TRUE(Utility::dnsNameMatch("lyft.com", "lyf*.com")); EXPECT_TRUE(Utility::dnsNameMatch("lyft.com", "lyft*.com")); EXPECT_TRUE(Utility::dnsNameMatch("lyft.com", "l*ft.com")); + EXPECT_TRUE(Utility::dnsNameMatch("test.lyft.com", "t*.lyft.com")); EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "l*ft.co")); EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "ly?t.com")); EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "lf*t.com")); @@ -43,6 +44,8 @@ TEST(UtilityTest, TestDnsNameMatching) { EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "**lyft.com")); EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "lyft**.com")); EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "ly**ft.com")); + EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "lyft.c*m")); + EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "*yft.c*m")); EXPECT_FALSE(Utility::dnsNameMatch("a.b.lyft.com", "*.lyft.com")); EXPECT_FALSE(Utility::dnsNameMatch("foo.test.com", "*.lyft.com")); EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "*.lyft.com")); From e7e689eeeb8faf43180518c4cfd043ea6e712205 Mon Sep 17 00:00:00 2001 From: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> Date: Tue, 1 Feb 2022 23:55:03 +0000 Subject: [PATCH 06/10] make use of strSplit and combine label matching cases Signed-off-by: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> --- .../transport_sockets/tls/utility.cc | 50 ++++++------------- .../transport_sockets/tls/utility.h | 5 +- 2 files changed, 17 insertions(+), 38 deletions(-) diff --git a/source/extensions/transport_sockets/tls/utility.cc b/source/extensions/transport_sockets/tls/utility.cc index 82c8c589e6ec5..d4f2bea8cc256 100644 --- a/source/extensions/transport_sockets/tls/utility.cc +++ b/source/extensions/transport_sockets/tls/utility.cc @@ -85,8 +85,7 @@ bool Utility::labelWildcardMatch(absl::string_view dns_label, absl::string_view } size_t i = 0, p = 0, i_star = dns_label.size(), p_star = 0; while (i < dns_label.size()) { - if (p < pattern.size() && - absl::ascii_tolower(dns_label[i]) == absl::ascii_tolower(pattern[p])) { + if (p < pattern.size() && dns_label[i] == pattern[p]) { ++i; ++p; } else if (p < pattern.size() && pattern[p] == glob) { @@ -119,42 +118,21 @@ bool Utility::dnsNameMatch(absl::string_view dns_name, absl::string_view pattern return true; } - // https://www.rfc-editor.org/rfc/rfc6125#section-6.4.3 part #2 - // If the wildcard character is the only character of the left-most - // label in the presented identifier, the client SHOULD NOT compare - // against anything but the left-most label of the reference identifier - // (e.g., *.example.com would match foo.example.com but - // not bar.foo.example.com or example.com) - 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); - } - } + std::vector split_pattern = + absl::StrSplit(lower_case_pattern, absl::MaxSplits('.', 1)); + std::vector split_dns_name = + absl::StrSplit(lower_case_dns_name, absl::MaxSplits('.', 1)); - // https://www.rfc-editor.org/rfc/rfc6125#section-6.4.3 part #3 - // Match a presented identifier in which the wildcard character is not the only - // character of the label and don't match patter if the wildcard character is - // embedded within an A-label (A-label always starts with the ACE prefix "xn--") - // (e.g., baz*.example.net and *baz.example.net and b*z.example.net would - // be taken to match baz1.example.net and foobaz.example.net and - // buzz.example.net, respectively) - size_t pattern_left_label_len = lower_case_pattern.find('.'); - size_t dns_name_left_label_len = lower_case_dns_name.find('.'); + // dns name and pattern should contain more than 1 label to match + if (split_pattern.size() < 2 || split_dns_name.size() < 2) { + return false; + } // Only the left-most label in the pattern contains wildcard '*' and is not an A-label - if ((pattern_left_label_len != std::string::npos) && - (dns_name_left_label_len != std::string::npos) && - (lower_case_pattern.find('*') != std::string::npos) && - (lower_case_pattern.find('*') < pattern_left_label_len) && - (lower_case_pattern.substr(pattern_left_label_len).find('*') == std::string::npos) && - (!absl::StartsWith(lower_case_pattern.substr(0, pattern_left_label_len), ACE_prefix))) { - return labelWildcardMatch(lower_case_dns_name.substr(0, dns_name_left_label_len), - lower_case_pattern.substr(0, pattern_left_label_len)) && - (lower_case_dns_name.substr(dns_name_left_label_len) == - lower_case_pattern.substr(pattern_left_label_len)); + if ((split_pattern[0].find('*') != std::string::npos) && + (split_pattern[1].find('*') == std::string::npos) && + (!absl::StartsWith(split_pattern[0], ACE_prefix))) { + return labelWildcardMatch(split_dns_name[0], split_pattern[0]) && + (split_dns_name[1] == split_pattern[1]); } return false; diff --git a/source/extensions/transport_sockets/tls/utility.h b/source/extensions/transport_sockets/tls/utility.h index 09094f7ba21ec..0bab75d1d8c0b 100644 --- a/source/extensions/transport_sockets/tls/utility.h +++ b/source/extensions/transport_sockets/tls/utility.h @@ -23,6 +23,7 @@ Envoy::Ssl::CertificateDetailsPtr certificateDetails(X509* cert, const std::stri /** * Determines whether the given name matches 'pattern' which may optionally begin with a wildcard * or contain a wildcard inside the pattern's first label. + * https://www.rfc-editor.org/rfc/rfc6125#section-6.4.3 * @param dns_name the DNS name to match * @param pattern the pattern to match against (*.example.com) or (test*.example.com) * @return true if the san matches pattern @@ -33,8 +34,8 @@ bool dnsNameMatch(absl::string_view dns_name, absl::string_view pattern); * Determines whether the given DNS label matches 'pattern' which may contain a wildcard. e.g., * patterns "baz*" and "*baz" and "b*z" would match DNS labels "baz1" and "foobaz" and "buzz", * respectively. - * @param dns_label the DNS name label to match - * @param pattern the pattern to match against + * @param dns_label the DNS name label to match in lower case + * @param pattern the pattern to match against in lower case * @return true if the dns_label matches pattern */ bool labelWildcardMatch(absl::string_view dns_label, absl::string_view pattern); From 49c4f4dd0da7f6afe67cacea9dcc309b11be5931 Mon Sep 17 00:00:00 2001 From: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> Date: Wed, 2 Feb 2022 02:21:08 +0000 Subject: [PATCH 07/10] simplify wildcard comparison Signed-off-by: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> --- .../transport_sockets/tls/utility.cc | 31 ++++--------------- .../transport_sockets/tls/utility_test.cc | 1 + 2 files changed, 7 insertions(+), 25 deletions(-) diff --git a/source/extensions/transport_sockets/tls/utility.cc b/source/extensions/transport_sockets/tls/utility.cc index d4f2bea8cc256..79b664f24559d 100644 --- a/source/extensions/transport_sockets/tls/utility.cc +++ b/source/extensions/transport_sockets/tls/utility.cc @@ -75,7 +75,6 @@ bool Utility::labelWildcardMatch(absl::string_view dns_label, absl::string_view if (pattern.empty()) { return dns_label.empty(); } - // Only valid if wildcard character appear once if (std::count(pattern.begin(), pattern.end(), '*') == 1) { constexpr char glob = '*'; @@ -83,29 +82,11 @@ bool Utility::labelWildcardMatch(absl::string_view dns_label, absl::string_view if (pattern.size() == 1 && pattern[0] == glob) { return true; } - size_t i = 0, p = 0, i_star = dns_label.size(), p_star = 0; - while (i < dns_label.size()) { - if (p < pattern.size() && dns_label[i] == pattern[p]) { - ++i; - ++p; - } else if (p < pattern.size() && pattern[p] == glob) { - i_star = i; - p_star = p++; - } else if (i_star != dns_label.size()) { - i = ++i_star; - p = p_star + 1; - } else { - return false; - } - } - - while (p < pattern.size() && pattern[p] == glob) { - ++p; - } - - return p == pattern.size() && i == dns_label.size(); + std::vector split_pattern = absl::StrSplit(pattern, glob); + return (pattern.size() <= dns_label.size() + 1) && + absl::StartsWith(dns_label, split_pattern[0]) && + absl::EndsWith(dns_label, split_pattern[1]); } - return false; } @@ -131,8 +112,8 @@ bool Utility::dnsNameMatch(absl::string_view dns_name, absl::string_view pattern if ((split_pattern[0].find('*') != std::string::npos) && (split_pattern[1].find('*') == std::string::npos) && (!absl::StartsWith(split_pattern[0], ACE_prefix))) { - return labelWildcardMatch(split_dns_name[0], split_pattern[0]) && - (split_dns_name[1] == split_pattern[1]); + return (split_dns_name[1] == split_pattern[1]) && + labelWildcardMatch(split_dns_name[0], split_pattern[0]); } return false; diff --git a/test/extensions/transport_sockets/tls/utility_test.cc b/test/extensions/transport_sockets/tls/utility_test.cc index 9ab9eef23a273..53e6c5735cd7d 100644 --- a/test/extensions/transport_sockets/tls/utility_test.cc +++ b/test/extensions/transport_sockets/tls/utility_test.cc @@ -37,6 +37,7 @@ TEST(UtilityTest, TestDnsNameMatching) { EXPECT_TRUE(Utility::dnsNameMatch("lyft.com", "lyft*.com")); EXPECT_TRUE(Utility::dnsNameMatch("lyft.com", "l*ft.com")); EXPECT_TRUE(Utility::dnsNameMatch("test.lyft.com", "t*.lyft.com")); + EXPECT_FALSE(Utility::dnsNameMatch("t.lyft.com", "t*t.lyft.com")); EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "l*ft.co")); EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "ly?t.com")); EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "lf*t.com")); From 22e94fbbeea1b5ac64323f4c627da5663fb6894f Mon Sep 17 00:00:00 2001 From: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> Date: Wed, 2 Feb 2022 18:12:39 +0000 Subject: [PATCH 08/10] review comments -> code improvements Signed-off-by: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> --- .../extensions/transport_sockets/tls/utility.cc | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/source/extensions/transport_sockets/tls/utility.cc b/source/extensions/transport_sockets/tls/utility.cc index 79b664f24559d..d0d038f58287b 100644 --- a/source/extensions/transport_sockets/tls/utility.cc +++ b/source/extensions/transport_sockets/tls/utility.cc @@ -72,16 +72,13 @@ Envoy::Ssl::CertificateDetailsPtr Utility::certificateDetails(X509* cert, const } bool Utility::labelWildcardMatch(absl::string_view dns_label, absl::string_view pattern) { - if (pattern.empty()) { - return dns_label.empty(); + constexpr char glob = '*'; + if (pattern.size() == 1 && pattern[0] == glob) { + return true; } // Only valid if wildcard character appear once - if (std::count(pattern.begin(), pattern.end(), '*') == 1) { - constexpr char glob = '*'; + if (std::count(pattern.begin(), pattern.end(), glob) == 1) { // Check the special case of a single * pattern, as it's common. - if (pattern.size() == 1 && pattern[0] == glob) { - return true; - } std::vector split_pattern = absl::StrSplit(pattern, glob); return (pattern.size() <= dns_label.size() + 1) && absl::StartsWith(dns_label, split_pattern[0]) && @@ -109,8 +106,8 @@ bool Utility::dnsNameMatch(absl::string_view dns_name, absl::string_view pattern return false; } // Only the left-most label in the pattern contains wildcard '*' and is not an A-label - if ((split_pattern[0].find('*') != std::string::npos) && - (split_pattern[1].find('*') == std::string::npos) && + if ((split_pattern[0].find('*') != absl::string_view::npos) && + (split_pattern[1].find('*') == absl::string_view::npos) && (!absl::StartsWith(split_pattern[0], ACE_prefix))) { return (split_dns_name[1] == split_pattern[1]) && labelWildcardMatch(split_dns_name[0], split_pattern[0]); From 4be2194841fff01c2d14a08d13d6ebca37ea29a1 Mon Sep 17 00:00:00 2001 From: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> Date: Wed, 2 Feb 2022 18:15:58 +0000 Subject: [PATCH 09/10] move code comment Signed-off-by: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> --- source/extensions/transport_sockets/tls/utility.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/transport_sockets/tls/utility.cc b/source/extensions/transport_sockets/tls/utility.cc index d0d038f58287b..115cbe6d9af87 100644 --- a/source/extensions/transport_sockets/tls/utility.cc +++ b/source/extensions/transport_sockets/tls/utility.cc @@ -73,12 +73,12 @@ Envoy::Ssl::CertificateDetailsPtr Utility::certificateDetails(X509* cert, const bool Utility::labelWildcardMatch(absl::string_view dns_label, absl::string_view pattern) { constexpr char glob = '*'; + // Check the special case of a single * pattern, as it's common. if (pattern.size() == 1 && pattern[0] == glob) { return true; } // Only valid if wildcard character appear once if (std::count(pattern.begin(), pattern.end(), glob) == 1) { - // Check the special case of a single * pattern, as it's common. std::vector split_pattern = absl::StrSplit(pattern, glob); return (pattern.size() <= dns_label.size() + 1) && absl::StartsWith(dns_label, split_pattern[0]) && From 027ae207fafba00357a671eea04aae7e7461455a Mon Sep 17 00:00:00 2001 From: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> Date: Thu, 3 Feb 2022 23:09:27 +0000 Subject: [PATCH 10/10] more unit test coverage Signed-off-by: Sunil Narasimhamurthy <13044744+suniltheta@users.noreply.github.com> --- source/extensions/transport_sockets/tls/utility.cc | 8 ++++---- source/extensions/transport_sockets/tls/utility.h | 2 +- test/extensions/transport_sockets/tls/utility_test.cc | 4 +++- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/source/extensions/transport_sockets/tls/utility.cc b/source/extensions/transport_sockets/tls/utility.cc index 115cbe6d9af87..3c89b60139306 100644 --- a/source/extensions/transport_sockets/tls/utility.cc +++ b/source/extensions/transport_sockets/tls/utility.cc @@ -77,7 +77,7 @@ bool Utility::labelWildcardMatch(absl::string_view dns_label, absl::string_view if (pattern.size() == 1 && pattern[0] == glob) { return true; } - // Only valid if wildcard character appear once + // Only valid if wildcard character appear once. if (std::count(pattern.begin(), pattern.end(), glob) == 1) { std::vector split_pattern = absl::StrSplit(pattern, glob); return (pattern.size() <= dns_label.size() + 1) && @@ -88,7 +88,7 @@ bool Utility::labelWildcardMatch(absl::string_view dns_label, absl::string_view } bool Utility::dnsNameMatch(absl::string_view dns_name, absl::string_view pattern) { - // A-label ACE prefix https://www.rfc-editor.org/rfc/rfc5890#section-2.3.2.5 + // A-label ACE prefix https://www.rfc-editor.org/rfc/rfc5890#section-2.3.2.5. constexpr absl::string_view ACE_prefix = "xn--"; const std::string lower_case_dns_name = absl::AsciiStrToLower(dns_name); const std::string lower_case_pattern = absl::AsciiStrToLower(pattern); @@ -101,11 +101,11 @@ bool Utility::dnsNameMatch(absl::string_view dns_name, absl::string_view pattern std::vector split_dns_name = absl::StrSplit(lower_case_dns_name, absl::MaxSplits('.', 1)); - // dns name and pattern should contain more than 1 label to match + // dns name and pattern should contain more than 1 label to match. if (split_pattern.size() < 2 || split_dns_name.size() < 2) { return false; } - // Only the left-most label in the pattern contains wildcard '*' and is not an A-label + // Only the left-most label in the pattern contains wildcard '*' and is not an A-label. if ((split_pattern[0].find('*') != absl::string_view::npos) && (split_pattern[1].find('*') == absl::string_view::npos) && (!absl::StartsWith(split_pattern[0], ACE_prefix))) { diff --git a/source/extensions/transport_sockets/tls/utility.h b/source/extensions/transport_sockets/tls/utility.h index 0bab75d1d8c0b..c1fe2a1d212f4 100644 --- a/source/extensions/transport_sockets/tls/utility.h +++ b/source/extensions/transport_sockets/tls/utility.h @@ -23,7 +23,7 @@ Envoy::Ssl::CertificateDetailsPtr certificateDetails(X509* cert, const std::stri /** * Determines whether the given name matches 'pattern' which may optionally begin with a wildcard * or contain a wildcard inside the pattern's first label. - * https://www.rfc-editor.org/rfc/rfc6125#section-6.4.3 + * See: https://www.rfc-editor.org/rfc/rfc6125#section-6.4.3. * @param dns_name the DNS name to match * @param pattern the pattern to match against (*.example.com) or (test*.example.com) * @return true if the san matches pattern diff --git a/test/extensions/transport_sockets/tls/utility_test.cc b/test/extensions/transport_sockets/tls/utility_test.cc index 53e6c5735cd7d..b01d4a6e1d0fd 100644 --- a/test/extensions/transport_sockets/tls/utility_test.cc +++ b/test/extensions/transport_sockets/tls/utility_test.cc @@ -29,14 +29,15 @@ TEST(UtilityTest, TestDnsNameMatching) { EXPECT_TRUE(Utility::dnsNameMatch("lyft.com", "lyft.com")); EXPECT_TRUE(Utility::dnsNameMatch("a.lyft.com", "*.lyft.com")); EXPECT_TRUE(Utility::dnsNameMatch("a.LYFT.com", "*.lyft.COM")); - EXPECT_TRUE(Utility::dnsNameMatch(".lyft.com", "*.lyft.com")); EXPECT_TRUE(Utility::dnsNameMatch("lyft.com", "*yft.com")); EXPECT_TRUE(Utility::dnsNameMatch("LYFT.com", "*yft.com")); EXPECT_TRUE(Utility::dnsNameMatch("lyft.com", "*lyft.com")); EXPECT_TRUE(Utility::dnsNameMatch("lyft.com", "lyf*.com")); EXPECT_TRUE(Utility::dnsNameMatch("lyft.com", "lyft*.com")); EXPECT_TRUE(Utility::dnsNameMatch("lyft.com", "l*ft.com")); + EXPECT_TRUE(Utility::dnsNameMatch("t.lyft.com", "t*.lyft.com")); EXPECT_TRUE(Utility::dnsNameMatch("test.lyft.com", "t*.lyft.com")); + EXPECT_TRUE(Utility::dnsNameMatch("l-lots-of-stuff-ft.com", "l*ft.com")); EXPECT_FALSE(Utility::dnsNameMatch("t.lyft.com", "t*t.lyft.com")); EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "l*ft.co")); EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "ly?t.com")); @@ -47,6 +48,7 @@ TEST(UtilityTest, TestDnsNameMatching) { EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "ly**ft.com")); EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "lyft.c*m")); EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "*yft.c*m")); + EXPECT_FALSE(Utility::dnsNameMatch("test.lyft.com.extra", "*.lyft.com")); EXPECT_FALSE(Utility::dnsNameMatch("a.b.lyft.com", "*.lyft.com")); EXPECT_FALSE(Utility::dnsNameMatch("foo.test.com", "*.lyft.com")); EXPECT_FALSE(Utility::dnsNameMatch("lyft.com", "*.lyft.com"));