From 5d731878fd0134ca15d5904450a64dab0ff577a9 Mon Sep 17 00:00:00 2001 From: Yangmin Date: Tue, 4 Sep 2018 18:57:56 -0700 Subject: [PATCH] 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 --- DEPRECATED.md | 2 ++ api/envoy/config/rbac/v2alpha/BUILD | 2 ++ api/envoy/config/rbac/v2alpha/rbac.proto | 16 ++++++--- docs/root/intro/version_history.rst | 2 ++ source/common/common/matchers.cc | 13 ++++--- source/common/common/matchers.h | 2 ++ source/extensions/filters/common/rbac/BUILD | 1 + .../filters/common/rbac/matchers.cc | 4 +-- .../extensions/filters/common/rbac/matchers.h | 6 ++-- .../filters/common/rbac/matchers_test.cc | 34 +++++++++++++------ 10 files changed, 58 insertions(+), 24 deletions(-) diff --git a/DEPRECATED.md b/DEPRECATED.md index f5bc2d245ca0..0a6f54c846cf 100644 --- a/DEPRECATED.md +++ b/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 diff --git a/api/envoy/config/rbac/v2alpha/BUILD b/api/envoy/config/rbac/v2alpha/BUILD index f24c8594ad2e..c97a2f82378d 100644 --- a/api/envoy/config/rbac/v2alpha/BUILD +++ b/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", ], ) diff --git a/api/envoy/config/rbac/v2alpha/rbac.proto b/api/envoy/config/rbac/v2alpha/rbac.proto index 3f1f3eadbcc4..c5d8f1d827fb 100644 --- a/api/envoy/config/rbac/v2alpha/rbac.proto +++ b/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 { diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 999dc5df7bb8..df98978994f8 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -66,6 +66,8 @@ Version history :ref:`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 ` 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 ` has been added. * rest-api: added ability to set the :ref:`request timeout ` for REST API requests. * router: added ability to set request/response headers at the :ref:`envoy_api_msg_route.Route` level. diff --git a/source/common/common/matchers.cc b/source/common/common/matchers.cc index 8433ae7099ca..4121b599fa8d 100644 --- a/source/common/common/matchers.cc +++ b/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; } diff --git a/source/common/common/matchers.h b/source/common/common/matchers.h index 8f7967a47096..1fd4a210517b 100644 --- a/source/common/common/matchers.h +++ b/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: diff --git a/source/extensions/filters/common/rbac/BUILD b/source/extensions/filters/common/rbac/BUILD index 5e5ee7db77de..23a8926ab5ef 100644 --- a/source/extensions/filters/common/rbac/BUILD +++ b/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", diff --git a/source/extensions/filters/common/rbac/matchers.cc b/source/extensions/filters/common/rbac/matchers.cc index dad7ee9c58b4..8a6030c21408 100644 --- a/source/extensions/filters/common/rbac/matchers.cc +++ b/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&, diff --git a/source/extensions/filters/common/rbac/matchers.h b/source/extensions/filters/common/rbac/matchers.h index 85487cc408f7..d6b2d27c901d 100644 --- a/source/extensions/filters/common/rbac/matchers.h +++ b/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(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 matcher_; }; /** diff --git a/test/extensions/filters/common/rbac/matchers_test.cc b/test/extensions/filters/common/rbac/matchers_test.cc index 8aeae16c80f3..caee42c76371 100644 --- a/test/extensions/filters/common/rbac/matchers_test.cc +++ b/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);