rbac: update the authenticated.user to a StringMatcher. (#4250)

This PR added a new principal_name of type StringMatcher to rbac Authenticated and mark the existing user field as deprecated. This gives us more flexibility to express more matching rules against peer certificate.

Risk Level: Low
Testing: Added unit tests

Signed-off-by: Yangmin Zhu <ymzhu@google.com>

DEPRECATED.md

@@ -29,6 +29,8 @@ A logged warning is expected for each deprecated item that is in deprecation win
 * Setting hosts via `hosts` field in `Cluster` is deprecated. Use `load_assignment` instead.
 * Use of `response_headers_to_*` and `request_headers_to_add` are deprecated at the `RouteAction`
   level. Please use the configuration options at the `Route` level.
+* Use of the string `user` field in `Authenticated` in [rbac.proto](https://github.com/envoyproxy/envoy/blob/master/api/envoy/config/rbac/v2alpha/rbac.proto)
+  is deprecated in favor of the new `StringMatcher` based `principal_name` field.
 
 ## Version 1.7.0
 

api/envoy/config/rbac/v2alpha/BUILD

@@ -10,6 +10,7 @@ api_proto_library_internal(
         "//envoy/api/v2/core:address",
         "//envoy/api/v2/route",
         "//envoy/type/matcher:metadata",
+        "//envoy/type/matcher:string",
     ],
 )
 
@@ -20,5 +21,6 @@ api_go_proto_library(
         "//envoy/api/v2/core:address_go_proto",
         "//envoy/api/v2/route:route_go_proto",
         "//envoy/type/matcher:metadata_go_proto",
+        "//envoy/type/matcher:string_go_proto",
     ],
 )

api/envoy/config/rbac/v2alpha/rbac.proto

@@ -4,6 +4,7 @@ import "validate/validate.proto";
 import "envoy/api/v2/core/address.proto";
 import "envoy/api/v2/route/route.proto";
 import "envoy/type/matcher/metadata.proto";
+import "envoy/type/matcher/string.proto";
 
 package envoy.config.rbac.v2alpha;
 option go_package = "v2alpha";
@@ -30,8 +31,12 @@ option go_package = "v2alpha";
 //       permissions:
 //         - any: true
 //       principals:
-//         - authenticated: { name: "cluster.local/ns/default/sa/admin" }
-//         - authenticated: { name: "cluster.local/ns/default/sa/superuser" }
+//         - authenticated:
+//             principal_name:
+//               exact: "cluster.local/ns/default/sa/admin"
+//         - authenticated:
+//             principal_name:
+//               exact: "cluster.local/ns/default/sa/superuser"
 //     "product-viewer":
 //       permissions:
 //           - and_rules:
@@ -135,9 +140,12 @@ message Principal {
 
   // Authentication attributes for a downstream.
   message Authenticated {
-    // The name of the principal. If set, the URI SAN is used from the certificate, otherwise the
+    reserved 1;
+    reserved "name";
+
+    // The name of the principal. If set, The URI SAN is used from the certificate, otherwise the
     // subject field is used. If unset, it applies to any user that is authenticated.
-    string name = 1;
+    envoy.type.matcher.StringMatcher principal_name = 2;
   }
 
   oneof identifier {

docs/root/intro/version_history.rst

@@ -66,6 +66,8 @@ Version history
   :ref:`use_data_plane_proto<envoy_api_field_config.ratelimit.v2.RateLimitServiceConfig.use_data_plane_proto>`
   boolean flag in the ratelimit configuration.
   Support for the legacy proto :repo:`source/common/ratelimit/ratelimit.proto` is deprecated and will be removed at the start of the 1.9.0 release cycle.
+* rbac config: added a :ref:`principal_name <envoy_api_field_config.rbac.v2alpha.Principal.Authenticated.principal_name>` field and
+  removed the old `name` field to give more flexibility for matching certificate identity.
 * rbac network filter: a :ref:`role-based access control network filter <config_network_filters_rbac>` has been added.
 * rest-api: added ability to set the :ref:`request timeout <envoy_api_field_core.ApiConfigSource.request_timeout>` for REST API requests.
 * router: added ability to set request/response headers at the :ref:`envoy_api_msg_route.Route` level.

source/common/common/matchers.cc

@@ -61,16 +61,19 @@ bool StringMatcher::match(const ProtobufWkt::Value& value) const {
     return false;
   }
 
-  const std::string& s = value.string_value();
+  return match(value.string_value());
+}
+
+bool StringMatcher::match(const std::string& value) const {
   switch (matcher_.match_pattern_case()) {
   case envoy::type::matcher::StringMatcher::kExact:
-    return matcher_.exact() == s;
+    return matcher_.exact() == value;
   case envoy::type::matcher::StringMatcher::kPrefix:
-    return absl::StartsWith(s, matcher_.prefix());
+    return absl::StartsWith(value, matcher_.prefix());
   case envoy::type::matcher::StringMatcher::kSuffix:
-    return absl::EndsWith(s, matcher_.suffix());
+    return absl::EndsWith(value, matcher_.suffix());
   case envoy::type::matcher::StringMatcher::kRegex:
-    return std::regex_match(s, regex_);
+    return std::regex_match(value, regex_);
   default:
     NOT_REACHED_GCOVR_EXCL_LINE;
   }

source/common/common/matchers.h

@@ -78,6 +78,8 @@ class StringMatcher : public ValueMatcher {
     }
   }
 
+  bool match(const std::string& value) const;
+
   bool match(const ProtobufWkt::Value& value) const override;
 
 private:

source/extensions/filters/common/rbac/BUILD

@@ -24,6 +24,7 @@ envoy_cc_library(
     name = "matchers_lib",
     srcs = ["matchers.cc"],
     hdrs = ["matchers.h"],
+    external_deps = ["abseil_optional"],
     deps = [
         "//include/envoy/http:header_map_interface",
         "//include/envoy/network:connection_interface",

source/extensions/filters/common/rbac/matchers.cc

@@ -135,14 +135,14 @@ bool AuthenticatedMatcher::matches(const Network::Connection& connection,
   const auto* ssl = connection.ssl();
   if (!ssl) { // connection was not authenticated
     return false;
-  } else if (name_.empty()) { // matcher allows any subject
+  } else if (!matcher_.has_value()) { // matcher allows any subject
     return true;
   }
 
   std::string principal = ssl->uriSanPeerCertificate();
   principal = principal.empty() ? ssl->subjectPeerCertificate() : principal;
 
-  return principal == name_;
+  return matcher_.value().match(principal);
 }
 
 bool MetadataMatcher::matches(const Network::Connection&, const Envoy::Http::HeaderMap&,

source/extensions/filters/common/rbac/matchers.h

@@ -163,13 +163,15 @@ class PortMatcher : public Matcher {
 class AuthenticatedMatcher : public Matcher {
 public:
   AuthenticatedMatcher(const envoy::config::rbac::v2alpha::Principal_Authenticated& auth)
-      : name_(auth.name()) {}
+      : matcher_(auth.has_principal_name()
+                     ? absl::make_optional<Matchers::StringMatcher>(auth.principal_name())
+                     : absl::nullopt) {}
 
   bool matches(const Network::Connection& connection, const Envoy::Http::HeaderMap& headers,
                const envoy::api::v2::core::Metadata&) const override;
 
 private:
-  const std::string name_;
+  const absl::optional<Matchers::StringMatcher> matcher_;
 };
 
 /**

test/extensions/filters/common/rbac/matchers_test.cc

@@ -192,32 +192,44 @@ TEST(AuthenticatedMatcher, uriSanPeerCertificate) {
   Envoy::Network::MockConnection conn;
   Envoy::Ssl::MockConnection ssl;
 
-  EXPECT_CALL(ssl, uriSanPeerCertificate()).WillOnce(Return("foo"));
-  EXPECT_CALL(Const(conn), ssl()).WillOnce(Return(&ssl));
+  EXPECT_CALL(ssl, uriSanPeerCertificate()).WillRepeatedly(Return("foo"));
+  EXPECT_CALL(Const(conn), ssl()).WillRepeatedly(Return(&ssl));
 
   envoy::config::rbac::v2alpha::Principal_Authenticated auth;
-  auth.set_name("foo");
+  auth.mutable_principal_name()->set_exact("foo");
   checkMatcher(AuthenticatedMatcher(auth), true, conn);
+
+  auth.mutable_principal_name()->set_exact("bar");
+  checkMatcher(AuthenticatedMatcher(auth), false, conn);
 }
 
 TEST(AuthenticatedMatcher, subjectPeerCertificate) {
   Envoy::Network::MockConnection conn;
   Envoy::Ssl::MockConnection ssl;
 
-  EXPECT_CALL(ssl, uriSanPeerCertificate()).WillOnce(Return(""));
-  EXPECT_CALL(ssl, subjectPeerCertificate()).WillOnce(Return("bar"));
-  EXPECT_CALL(Const(conn), ssl()).WillOnce(Return(&ssl));
+  EXPECT_CALL(ssl, uriSanPeerCertificate()).WillRepeatedly(Return(""));
+  EXPECT_CALL(ssl, subjectPeerCertificate()).WillRepeatedly(Return("bar"));
+  EXPECT_CALL(Const(conn), ssl()).WillRepeatedly(Return(&ssl));
 
   envoy::config::rbac::v2alpha::Principal_Authenticated auth;
-  auth.set_name("bar");
+  auth.mutable_principal_name()->set_exact("bar");
   checkMatcher(AuthenticatedMatcher(auth), true, conn);
+
+  auth.mutable_principal_name()->set_exact("foo");
+  checkMatcher(AuthenticatedMatcher(auth), false, conn);
 }
 
 TEST(AuthenticatedMatcher, AnySSLSubject) {
   Envoy::Network::MockConnection conn;
   Envoy::Ssl::MockConnection ssl;
-  EXPECT_CALL(Const(conn), ssl()).WillOnce(Return(&ssl));
-  checkMatcher(AuthenticatedMatcher({}), true, conn);
+  EXPECT_CALL(ssl, uriSanPeerCertificate()).WillRepeatedly(Return("foo"));
+  EXPECT_CALL(Const(conn), ssl()).WillRepeatedly(Return(&ssl));
+
+  envoy::config::rbac::v2alpha::Principal_Authenticated auth;
+  checkMatcher(AuthenticatedMatcher(auth), true, conn);
+
+  auth.mutable_principal_name()->set_regex(".*");
+  checkMatcher(AuthenticatedMatcher(auth), true, conn);
 }
 
 TEST(AuthenticatedMatcher, NoSSL) {
@@ -251,8 +263,8 @@ TEST(PolicyMatcher, PolicyMatcher) {
   envoy::config::rbac::v2alpha::Policy policy;
   policy.add_permissions()->set_destination_port(123);
   policy.add_permissions()->set_destination_port(456);
-  policy.add_principals()->mutable_authenticated()->set_name("foo");
-  policy.add_principals()->mutable_authenticated()->set_name("bar");
+  policy.add_principals()->mutable_authenticated()->mutable_principal_name()->set_exact("foo");
+  policy.add_principals()->mutable_authenticated()->mutable_principal_name()->set_exact("bar");
 
   RBAC::PolicyMatcher matcher(policy);