Merge pull request #20436 from vespa-engine/vekterli/support-slash-delimited-globs-for-uri-san-matching

Support glob-style credential matching of SAN_URI certificate fields [run-systemtest]

3 files changed, 145 insertions, 52 deletions

diff --git a/vespalib/src/tests/net/tls/policy_checking_certificate_verifier/policy_checking_certificate_verifier_test.cpp b/vespalib/src/tests/net/tls/policy_checking_certificate_verifier/policy_checking_certificate_verifier_test.cpp
index e129ef2a389..812d06868fd 100644
--- a/vespalib/src/tests/net/tls/policy_checking_certificate_verifier/policy_checking_certificate_verifier_test.cpp
+++ b/vespalib/src/tests/net/tls/policy_checking_certificate_verifier/policy_checking_certificate_verifier_test.cpp
@@ -7,57 +7,93 @@
 using namespace vespalib;
 using namespace vespalib::net::tls;
 
-bool glob_matches(vespalib::stringref pattern, vespalib::stringref string_to_check) {
-    auto glob = CredentialMatchPattern::create_from_glob(pattern);
+bool dns_glob_matches(vespalib::stringref pattern, vespalib::stringref string_to_check) {
+    auto glob = CredentialMatchPattern::create_from_dns_glob(pattern);
     return glob->matches(string_to_check);
 }
 
+bool uri_glob_matches(vespalib::stringref pattern, vespalib::stringref string_to_check) {
+    auto glob = CredentialMatchPattern::create_from_uri_glob(pattern);
+    return glob->matches(string_to_check);
+}
+
+void verify_all_glob_types_match(vespalib::stringref pattern, vespalib::stringref string_to_check) {
+    EXPECT_TRUE(dns_glob_matches(pattern, string_to_check));
+    EXPECT_TRUE(uri_glob_matches(pattern, string_to_check));
+}
+
+void verify_all_glob_types_mismatch(vespalib::stringref pattern, vespalib::stringref string_to_check) {
+    EXPECT_FALSE(dns_glob_matches(pattern, string_to_check));
+    EXPECT_FALSE(uri_glob_matches(pattern, string_to_check));
+}
+
 TEST("glob without wildcards matches entire string") {
-    EXPECT_TRUE(glob_matches("foo", "foo"));
-    EXPECT_FALSE(glob_matches("foo", "fooo"));
-    EXPECT_FALSE(glob_matches("foo", "ffoo"));
+    verify_all_glob_types_match("foo", "foo");
+    verify_all_glob_types_mismatch("foo", "fooo");
+    verify_all_glob_types_mismatch("foo", "ffoo");
 }
 
 TEST("wildcard glob can match prefix") {
-    EXPECT_TRUE(glob_matches("foo*", "foo"));
-    EXPECT_TRUE(glob_matches("foo*", "foobar"));
-    EXPECT_FALSE(glob_matches("foo*", "ffoo"));
+    verify_all_glob_types_match("foo*", "foo");
+    verify_all_glob_types_match("foo*", "foobar");
+    verify_all_glob_types_mismatch("foo*", "ffoo");
 }
 
 TEST("wildcard glob can match suffix") {
-    EXPECT_TRUE(glob_matches("*foo", "foo"));
-    EXPECT_TRUE(glob_matches("*foo", "ffoo"));
-    EXPECT_FALSE(glob_matches("*foo", "fooo"));
+    verify_all_glob_types_match("*foo", "foo");
+    verify_all_glob_types_match("*foo", "ffoo");
+    verify_all_glob_types_mismatch("*foo", "fooo");
 }
 
 TEST("wildcard glob can match substring") {
-    EXPECT_TRUE(glob_matches("f*o", "fo"));
-    EXPECT_TRUE(glob_matches("f*o", "foo"));
-    EXPECT_TRUE(glob_matches("f*o", "ffoo"));
-    EXPECT_FALSE(glob_matches("f*o", "boo"));
+    verify_all_glob_types_match("f*o", "fo");
+    verify_all_glob_types_match("f*o", "foo");
+    verify_all_glob_types_match("f*o", "ffoo");
+    verify_all_glob_types_mismatch("f*o", "boo");
 }
 
-TEST("wildcard glob does not cross multiple dot delimiter boundaries") {
-    EXPECT_TRUE(glob_matches("*.bar.baz", "foo.bar.baz"));
-    EXPECT_TRUE(glob_matches("*.bar.baz", ".bar.baz"));
-    EXPECT_FALSE(glob_matches("*.bar.baz", "zoid.foo.bar.baz"));
-    EXPECT_TRUE(glob_matches("foo.*.baz", "foo.bar.baz"));
-    EXPECT_FALSE(glob_matches("foo.*.baz", "foo.bar.zoid.baz"));
+TEST("single char DNS glob matches single character") {
+    EXPECT_TRUE(dns_glob_matches("f?o", "foo"));
+    EXPECT_FALSE(dns_glob_matches("f?o", "fooo"));
+    EXPECT_FALSE(dns_glob_matches("f?o", "ffoo"));
 }
 
-TEST("single char glob matches non dot characters") {
-    EXPECT_TRUE(glob_matches("f?o", "foo"));
-    EXPECT_FALSE(glob_matches("f?o", "fooo"));
-    EXPECT_FALSE(glob_matches("f?o", "ffoo"));
-    EXPECT_FALSE(glob_matches("f?o", "f.o"));
+// Due to URIs being able to contain '?' characters as a query separator, don't use it for wildcarding.
+TEST("URI glob matching treats question mark character as literal match") {
+    EXPECT_TRUE(uri_glob_matches("f?o", "f?o"));
+    EXPECT_FALSE(uri_glob_matches("f?o", "foo"));
+    EXPECT_FALSE(uri_glob_matches("f?o", "f?oo"));
+}
+
+TEST("wildcard DNS glob does not cross multiple dot delimiter boundaries") {
+    EXPECT_TRUE(dns_glob_matches("*.bar.baz", "foo.bar.baz"));
+    EXPECT_TRUE(dns_glob_matches("*.bar.baz", ".bar.baz"));
+    EXPECT_FALSE(dns_glob_matches("*.bar.baz", "zoid.foo.bar.baz"));
+    EXPECT_TRUE(dns_glob_matches("foo.*.baz", "foo.bar.baz"));
+    EXPECT_FALSE(dns_glob_matches("foo.*.baz", "foo.bar.zoid.baz"));
+}
+
+TEST("wildcard URI glob does not cross multiple fwd slash delimiter boundaries") {
+    EXPECT_TRUE(uri_glob_matches("*/bar/baz", "foo/bar/baz"));
+    EXPECT_TRUE(uri_glob_matches("*/bar/baz", "/bar/baz"));
+    EXPECT_FALSE(uri_glob_matches("*/bar/baz", "bar/baz"));
+    EXPECT_FALSE(uri_glob_matches("*/bar/baz", "/bar/baz/"));
+    EXPECT_FALSE(uri_glob_matches("*/bar/baz", "zoid/foo/bar/baz"));
+    EXPECT_TRUE(uri_glob_matches("foo/*/baz", "foo/bar/baz"));
+    EXPECT_FALSE(uri_glob_matches("foo/*/baz", "foo/bar/zoid/baz"));
+    EXPECT_TRUE(uri_glob_matches("foo/*/baz", "foo/bar.zoid/baz")); // No special handling of dots
+}
+
+TEST("single char DNS glob matches non dot characters only") {
+    EXPECT_FALSE(dns_glob_matches("f?o", "f.o"));
 }
 
 TEST("special basic regex characters are escaped") {
-    EXPECT_TRUE(glob_matches("$[.\\^", "$[.\\^"));
+    verify_all_glob_types_match("$[.\\^", "$[.\\^");
 }
 
 TEST("special extended regex characters are ignored") {
-    EXPECT_TRUE(glob_matches("{)(+|]}", "{)(+|]}"));
+    verify_all_glob_types_match("{)(+|]}", "{)(+|]}");
 }
 
 // TODO CN + SANs
@@ -116,7 +152,7 @@ TEST("DNS SAN requirement without glob pattern is matched as exact string") {
     EXPECT_FALSE(verify(authorized, creds_with_dns_sans({{"hello.world.bar"}})));
 }
 
-TEST("DNS SAN requirement can include glob wildcards") {
+TEST("DNS SAN requirement can include glob wildcards, delimited by dot character") {
     auto authorized = authorized_peers({policy_with({required_san_dns("*.w?rld")})});
     EXPECT_TRUE(verify(authorized,  creds_with_dns_sans({{"hello.world"}})));
     EXPECT_TRUE(verify(authorized,  creds_with_dns_sans({{"greetings.w0rld"}})));
@@ -124,8 +160,8 @@ TEST("DNS SAN requirement can include glob wildcards") {
     EXPECT_FALSE(verify(authorized, creds_with_dns_sans({{"world"}})));
 }
 
-// FIXME make this RFC 2459-compliant with subdomain matching, case insensitity for host etc
-TEST("URI SAN requirement is matched as exact string in cheeky, pragmatic violation of RFC 2459") {
+// TODO consider making this RFC 2459-compliant with case insensitivity for scheme and host
+TEST("URI SAN requirement without glob pattern is matched as exact string") {
     auto authorized = authorized_peers({policy_with({required_san_uri("foo://bar.baz/zoid")})});
     EXPECT_TRUE(verify(authorized,  creds_with_uri_sans({{"foo://bar.baz/zoid"}})));
     EXPECT_FALSE(verify(authorized, creds_with_uri_sans({{"foo://bar.baz/zoi"}})));
@@ -136,6 +172,25 @@ TEST("URI SAN requirement is matched as exact string in cheeky, pragmatic violat
     EXPECT_FALSE(verify(authorized, creds_with_uri_sans({{"foo://BAR.baz/zoid"}})));
 }
 
+// TODO consider making this RFC 2459-compliant with case insensitivity for scheme and host
+TEST("URI SAN requirement can include glob wildcards, delimited by fwd slash character") {
+    auto authorized = authorized_peers({policy_with({required_san_uri("myscheme://my/*/uri")})});
+    EXPECT_TRUE(verify(authorized,  creds_with_uri_sans({{"myscheme://my/cool/uri"}})));
+    EXPECT_TRUE(verify(authorized,  creds_with_uri_sans({{"myscheme://my/really.cool/uri"}}))); // Not delimited by dots
+    EXPECT_FALSE(verify(authorized, creds_with_uri_sans({{"theirscheme://my/cool/uri"}})));
+    EXPECT_FALSE(verify(authorized, creds_with_uri_sans({{"myscheme://their/cool/uri"}})));
+    EXPECT_FALSE(verify(authorized, creds_with_uri_sans({{"myscheme://my/cool/uris"}})));
+    EXPECT_FALSE(verify(authorized, creds_with_uri_sans({{"myscheme://my/swag/uri/"}})));
+    EXPECT_FALSE(verify(authorized, creds_with_uri_sans({{"myscheme://my/uri"}})));
+}
+
+TEST("URI SAN requirement can include query part even though it's rather silly to do so") {
+    auto authorized = authorized_peers({policy_with({required_san_uri("myscheme://my/fancy/*?magic")})});
+    EXPECT_TRUE(verify(authorized,  creds_with_uri_sans({{"myscheme://my/fancy/uri?magic"}})));
+    EXPECT_TRUE(verify(authorized,  creds_with_uri_sans({{"myscheme://my/fancy/?magic"}})));
+    EXPECT_FALSE(verify(authorized, creds_with_uri_sans({{"myscheme://my/fancy/urimagic"}})));
+}
+
 TEST("multi-SAN policy requires all SANs to be present in certificate") {
     auto authorized = authorized_peers({policy_with({required_san_dns("hello.world"),
                                                      required_san_dns("foo.bar"),
@@ -157,6 +212,13 @@ TEST("wildcard DNS SAN in certificate is not treated as a wildcard match by poli
     EXPECT_FALSE(verify(authorized, creds_with_dns_sans({{"*.world"}})));
 }
 
+TEST("wildcard URI SAN in certificate is not treated as a wildcard match by policy") {
+    auto authorized = authorized_peers({policy_with({required_san_uri("hello://world")})});
+    EXPECT_FALSE(verify(authorized, creds_with_uri_sans({{"hello://*"}})));
+}
+
+// TODO this is just by coincidence since we match '*' as any other character, not because we interpret
+//  the wildcard in the SAN as anything special during matching. Consider if we need/want to handle explicitly.
 TEST("wildcard DNS SAN in certificate is still matched by wildcard policy SAN") {
     auto authorized = authorized_peers({policy_with({required_san_dns("*.world")})});
     EXPECT_TRUE(verify(authorized, creds_with_dns_sans({{"*.world"}})));
diff --git a/vespalib/src/vespa/vespalib/net/tls/peer_policies.cpp b/vespalib/src/vespa/vespalib/net/tls/peer_policies.cpp
index 149ad01b947..a476e23e6cb 100644
--- a/vespalib/src/vespa/vespalib/net/tls/peer_policies.cpp
+++ b/vespalib/src/vespa/vespalib/net/tls/peer_policies.cpp
@@ -22,23 +22,29 @@ bool is_regex_special_char(char c) noexcept {
     case '\\':
     case '+':
     case '.':
+    case '?':
+    case '*':
         return true;
     default:
         return false;
     }
 }
 
-std::string dot_separated_glob_to_regex(vespalib::stringref glob) {
+// Important: `delimiter` MUST NOT be a character that needs escaping within a regex [charset]
+template <bool SupportSingleCharMatch>
+std::string char_delimited_glob_to_regex(vespalib::stringref glob, char delimiter) {
     std::string ret = "^";
     ret.reserve(glob.size() + 2);
+    // Note: we explicitly stop matching at a delimiter boundary.
+    // This is to make path fragment matching less vulnerable to dirty tricks.
+    const std::string wildcard_pattern    = std::string("[^") + delimiter + "]*";
+    // Same applies for single chars; they should only match _within_ a delimited boundary.
+    const std::string single_char_pattern = std::string("[^") + delimiter + "]";
     for (auto c : glob) {
         if (c == '*') {
-            // Note: we explicitly stop matching at a dot separator boundary.
-            // This is to make host name matching less vulnerable to dirty tricks.
-            ret += "[^.]*";
-        } else if (c == '?') {
-            // Same applies for single chars; they should only match _within_ a dot boundary.
-            ret += "[^.]";
+            ret += wildcard_pattern;
+        } else if (c == '?' && SupportSingleCharMatch) {
+            ret += single_char_pattern;
         } else {
             if (is_regex_special_char(c)) {
                 ret += '\\';
@@ -52,14 +58,25 @@ std::string dot_separated_glob_to_regex(vespalib::stringref glob) {
 
 class RegexHostMatchPattern : public CredentialMatchPattern {
     Regex _pattern_as_regex;
-public:
-    explicit RegexHostMatchPattern(vespalib::stringref glob_pattern)
-        : _pattern_as_regex(Regex::from_pattern(dot_separated_glob_to_regex(glob_pattern)))
+    explicit RegexHostMatchPattern(std::string_view glob_pattern)
+        : _pattern_as_regex(Regex::from_pattern(glob_pattern))
     {
     }
+public:
+    RegexHostMatchPattern(RegexHostMatchPattern&&) noexcept = default;
     ~RegexHostMatchPattern() override = default;
 
-    [[nodiscard]] bool matches(vespalib::stringref str) const override {
+    RegexHostMatchPattern& operator=(RegexHostMatchPattern&&) noexcept = default;
+
+    [[nodiscard]] static RegexHostMatchPattern from_dns_glob_pattern(vespalib::stringref glob_pattern) {
+        return RegexHostMatchPattern(char_delimited_glob_to_regex<true>(glob_pattern, '.'));
+    }
+
+    [[nodiscard]] static RegexHostMatchPattern from_uri_glob_pattern(vespalib::stringref glob_pattern) {
+        return RegexHostMatchPattern(char_delimited_glob_to_regex<false>(glob_pattern, '/'));
+    }
+
+    [[nodiscard]] bool matches(vespalib::stringref str) const noexcept override {
         return _pattern_as_regex.full_match(std::string_view(str.data(), str.size()));
     }
 };
@@ -73,15 +90,19 @@ public:
     }
     ~ExactMatchPattern() override = default;
 
-    [[nodiscard]] bool matches(vespalib::stringref str) const override {
+    [[nodiscard]] bool matches(vespalib::stringref str) const noexcept override {
         return (str == _must_match_exactly);
     }
 };
 
 } // anon ns
 
-std::shared_ptr<const CredentialMatchPattern> CredentialMatchPattern::create_from_glob(vespalib::stringref glob_pattern) {
-    return std::make_shared<const RegexHostMatchPattern>(glob_pattern);
+std::shared_ptr<const CredentialMatchPattern> CredentialMatchPattern::create_from_dns_glob(vespalib::stringref glob_pattern) {
+    return std::make_shared<const RegexHostMatchPattern>(RegexHostMatchPattern::from_dns_glob_pattern(glob_pattern));
+}
+
+std::shared_ptr<const CredentialMatchPattern> CredentialMatchPattern::create_from_uri_glob(vespalib::stringref glob_pattern) {
+    return std::make_shared<const RegexHostMatchPattern>(RegexHostMatchPattern::from_uri_glob_pattern(glob_pattern));
 }
 
 std::shared_ptr<const CredentialMatchPattern> CredentialMatchPattern::create_exact_match(vespalib::stringref str) {
@@ -91,9 +112,8 @@ std::shared_ptr<const CredentialMatchPattern> CredentialMatchPattern::create_exa
 RequiredPeerCredential::RequiredPeerCredential(Field field, vespalib::string must_match_pattern)
     : _field(field),
       _original_pattern(std::move(must_match_pattern)),
-      // FIXME it's not RFC 2459-compliant to use exact-matching for URIs, but that's all we currently need.
-      _match_pattern(field == Field::SAN_URI ? CredentialMatchPattern::create_exact_match(_original_pattern)
-                                             : CredentialMatchPattern::create_from_glob(_original_pattern))
+      _match_pattern(field == Field::SAN_URI ? CredentialMatchPattern::create_from_uri_glob(_original_pattern)
+                                             : CredentialMatchPattern::create_from_dns_glob(_original_pattern))
 {
 }
 
@@ -111,11 +131,21 @@ void print_joined(std::ostream& os, const Collection& coll, const char* sep) {
         os << e;
     }
 }
+
+constexpr const char* to_string(RequiredPeerCredential::Field field) noexcept {
+    switch (field) {
+    case RequiredPeerCredential::Field::CN:      return "CN";
+    case RequiredPeerCredential::Field::SAN_DNS: return "SAN_DNS";
+    case RequiredPeerCredential::Field::SAN_URI: return "SAN_URI";
+    default: abort();
+    }
+}
+
 }
 
 std::ostream& operator<<(std::ostream& os, const RequiredPeerCredential& cred) {
     os << "RequiredPeerCredential("
-       << (cred.field() == RequiredPeerCredential::Field::CN ? "CN" : "SAN_DNS")
+       << to_string(cred.field())
        << " matches '"
        << cred.original_pattern()
        << "')";
diff --git a/vespalib/src/vespa/vespalib/net/tls/peer_policies.h b/vespalib/src/vespa/vespalib/net/tls/peer_policies.h
index c5721858518..4166efc4312 100644
--- a/vespalib/src/vespa/vespalib/net/tls/peer_policies.h
+++ b/vespalib/src/vespa/vespalib/net/tls/peer_policies.h
@@ -10,9 +10,10 @@ namespace vespalib::net::tls {
 
 struct CredentialMatchPattern {
     virtual ~CredentialMatchPattern() = default;
-    [[nodiscard]] virtual bool matches(vespalib::stringref str) const = 0;
+    [[nodiscard]] virtual bool matches(vespalib::stringref str) const noexcept = 0;
 
-    static std::shared_ptr<const CredentialMatchPattern> create_from_glob(vespalib::stringref pattern);
+    static std::shared_ptr<const CredentialMatchPattern> create_from_dns_glob(vespalib::stringref glob_pattern);
+    static std::shared_ptr<const CredentialMatchPattern> create_from_uri_glob(vespalib::stringref glob_pattern);
     static std::shared_ptr<const CredentialMatchPattern> create_exact_match(vespalib::stringref pattern);
 };
 
@@ -37,7 +38,7 @@ public:
                 && (_original_pattern == rhs._original_pattern));
     }
 
-    [[nodiscard]] bool matches(vespalib::stringref str) const {
+    [[nodiscard]] bool matches(vespalib::stringref str) const noexcept {
         return (_match_pattern && _match_pattern->matches(str));
     }