From c20356c1baf02a1829eecaa913d986e1054ddb85 Mon Sep 17 00:00:00 2001 From: Ashitha Santhosh <55257063+ashithasantosh@users.noreply.github.com> Date: Mon, 17 May 2021 17:31:05 -0700 Subject: [PATCH] Update conditional matcher ctors and include NotMatcher. (#26256) --- .../lib/security/authorization/matchers.cc | 161 ++++++++---------- .../lib/security/authorization/matchers.h | 74 ++++---- .../lib/security/authorization/rbac_policy.cc | 126 +++++++------- .../lib/security/authorization/rbac_policy.h | 42 ++--- .../security/authorization_matchers_test.cc | 135 +++++---------- .../grpc_authorization_engine_test.cc | 24 ++- 6 files changed, 235 insertions(+), 327 deletions(-) diff --git a/src/core/lib/security/authorization/matchers.cc b/src/core/lib/security/authorization/matchers.cc index 478054210a5..6be4cd964e2 100644 --- a/src/core/lib/security/authorization/matchers.cc +++ b/src/core/lib/security/authorization/matchers.cc @@ -22,30 +22,39 @@ namespace grpc_core { std::unique_ptr AuthorizationMatcher::Create( Rbac::Permission permission) { switch (permission.type) { - case Rbac::Permission::RuleType::kAnd: - return absl::make_unique( - std::move(permission.permissions), permission.not_rule); - case Rbac::Permission::RuleType::kOr: - return absl::make_unique( - std::move(permission.permissions), permission.not_rule); + case Rbac::Permission::RuleType::kAnd: { + std::vector> matchers; + for (const auto& rule : permission.permissions) { + matchers.push_back(AuthorizationMatcher::Create(std::move(*rule))); + } + return absl::make_unique(std::move(matchers)); + } + case Rbac::Permission::RuleType::kOr: { + std::vector> matchers; + for (const auto& rule : permission.permissions) { + matchers.push_back(AuthorizationMatcher::Create(std::move(*rule))); + } + return absl::make_unique(std::move(matchers)); + } + case Rbac::Permission::RuleType::kNot: + return absl::make_unique( + AuthorizationMatcher::Create(std::move(*permission.permissions[0]))); case Rbac::Permission::RuleType::kAny: - return absl::make_unique(permission.not_rule); + return absl::make_unique(); case Rbac::Permission::RuleType::kHeader: return absl::make_unique( - std::move(permission.header_matcher), permission.not_rule); + std::move(permission.header_matcher)); case Rbac::Permission::RuleType::kPath: return absl::make_unique( - std::move(permission.string_matcher), permission.not_rule); + std::move(permission.string_matcher)); case Rbac::Permission::RuleType::kDestIp: return absl::make_unique( - IpAuthorizationMatcher::Type::kDestIp, std::move(permission.ip), - permission.not_rule); + IpAuthorizationMatcher::Type::kDestIp, std::move(permission.ip)); case Rbac::Permission::RuleType::kDestPort: - return absl::make_unique(permission.port, - permission.not_rule); + return absl::make_unique(permission.port); case Rbac::Permission::RuleType::kReqServerName: return absl::make_unique( - std::move(permission.string_matcher), permission.not_rule); + std::move(permission.string_matcher)); } return nullptr; } @@ -53,103 +62,78 @@ std::unique_ptr AuthorizationMatcher::Create( std::unique_ptr AuthorizationMatcher::Create( Rbac::Principal principal) { switch (principal.type) { - case Rbac::Principal::RuleType::kAnd: - return absl::make_unique( - std::move(principal.principals), principal.not_rule); - case Rbac::Principal::RuleType::kOr: - return absl::make_unique( - std::move(principal.principals), principal.not_rule); + case Rbac::Principal::RuleType::kAnd: { + std::vector> matchers; + for (const auto& id : principal.principals) { + matchers.push_back(AuthorizationMatcher::Create(std::move(*id))); + } + return absl::make_unique(std::move(matchers)); + } + case Rbac::Principal::RuleType::kOr: { + std::vector> matchers; + for (const auto& id : principal.principals) { + matchers.push_back(AuthorizationMatcher::Create(std::move(*id))); + } + return absl::make_unique(std::move(matchers)); + } + case Rbac::Principal::RuleType::kNot: + return absl::make_unique( + AuthorizationMatcher::Create(std::move(*principal.principals[0]))); case Rbac::Principal::RuleType::kAny: - return absl::make_unique(principal.not_rule); + return absl::make_unique(); case Rbac::Principal::RuleType::kPrincipalName: return absl::make_unique( - std::move(principal.string_matcher), principal.not_rule); + std::move(principal.string_matcher)); case Rbac::Principal::RuleType::kSourceIp: return absl::make_unique( - IpAuthorizationMatcher::Type::kSourceIp, std::move(principal.ip), - principal.not_rule); + IpAuthorizationMatcher::Type::kSourceIp, std::move(principal.ip)); case Rbac::Principal::RuleType::kDirectRemoteIp: return absl::make_unique( IpAuthorizationMatcher::Type::kDirectRemoteIp, - std::move(principal.ip), principal.not_rule); + std::move(principal.ip)); case Rbac::Principal::RuleType::kRemoteIp: return absl::make_unique( - IpAuthorizationMatcher::Type::kRemoteIp, std::move(principal.ip), - principal.not_rule); + IpAuthorizationMatcher::Type::kRemoteIp, std::move(principal.ip)); case Rbac::Principal::RuleType::kHeader: return absl::make_unique( - std::move(principal.header_matcher), principal.not_rule); + std::move(principal.header_matcher)); case Rbac::Principal::RuleType::kPath: return absl::make_unique( - std::move(principal.string_matcher), principal.not_rule); + std::move(principal.string_matcher)); } return nullptr; } -AndAuthorizationMatcher::AndAuthorizationMatcher( - std::vector> rules, bool not_rule) - : not_rule_(not_rule) { - for (auto& rule : rules) { - matchers_.push_back(AuthorizationMatcher::Create(std::move(*rule))); - } -} - -AndAuthorizationMatcher::AndAuthorizationMatcher( - std::vector> ids, bool not_rule) - : not_rule_(not_rule) { - for (const auto& id : ids) { - matchers_.push_back(AuthorizationMatcher::Create(std::move(*id))); - } -} - bool AndAuthorizationMatcher::Matches(const EvaluateArgs& args) const { - bool matches = true; for (const auto& matcher : matchers_) { if (!matcher->Matches(args)) { - matches = false; - break; + return false; } } - return matches != not_rule_; -} - -OrAuthorizationMatcher::OrAuthorizationMatcher( - std::vector> rules, bool not_rule) - : not_rule_(not_rule) { - for (const auto& rule : rules) { - matchers_.push_back(AuthorizationMatcher::Create(std::move(*rule))); - } -} - -OrAuthorizationMatcher::OrAuthorizationMatcher( - std::vector> ids, bool not_rule) - : not_rule_(not_rule) { - for (const auto& id : ids) { - matchers_.push_back(AuthorizationMatcher::Create(std::move(*id))); - } + return true; } bool OrAuthorizationMatcher::Matches(const EvaluateArgs& args) const { - bool matches = false; for (const auto& matcher : matchers_) { if (matcher->Matches(args)) { - matches = true; - break; + return true; } } - return matches != not_rule_; + return false; +} + +bool NotAuthorizationMatcher::Matches(const EvaluateArgs& args) const { + return !matcher_->Matches(args); } bool HeaderAuthorizationMatcher::Matches(const EvaluateArgs& args) const { std::string concatenated_value; - bool matches = - matcher_.Match(args.GetHeaderValue(matcher_.name(), &concatenated_value)); - return matches != not_rule_; + return matcher_.Match( + args.GetHeaderValue(matcher_.name(), &concatenated_value)); } -IpAuthorizationMatcher::IpAuthorizationMatcher(Type type, Rbac::CidrRange range, - bool not_rule) - : type_(type), prefix_len_(range.prefix_len), not_rule_(not_rule) { +IpAuthorizationMatcher::IpAuthorizationMatcher(Type type, Rbac::CidrRange range) + : type_(type), prefix_len_(range.prefix_len) { grpc_error_handle error = grpc_string_to_sockaddr(&subnet_address_, range.address_prefix.c_str(), /*port does not matter here*/ 0); @@ -176,59 +160,54 @@ bool IpAuthorizationMatcher::Matches(const EvaluateArgs& args) const { } default: { // Currently we do not support matching rules containing "remote_ip". - return not_rule_; + return false; } } - bool matches = - grpc_sockaddr_match_subnet(&address, &subnet_address_, prefix_len_); - return matches != not_rule_; + return grpc_sockaddr_match_subnet(&address, &subnet_address_, prefix_len_); } bool PortAuthorizationMatcher::Matches(const EvaluateArgs& args) const { - bool matches = (port_ == args.GetLocalPort()); - return matches != not_rule_; + return port_ == args.GetLocalPort(); } bool AuthenticatedAuthorizationMatcher::Matches( const EvaluateArgs& args) const { if (args.GetTransportSecurityType() != GRPC_SSL_TRANSPORT_SECURITY_TYPE) { // Connection is not authenticated. - return not_rule_; + return false; } if (matcher_.string_matcher().empty()) { // Allows any authenticated user. - return !not_rule_; + return true; } absl::string_view spiffe_id = args.GetSpiffeId(); if (!spiffe_id.empty()) { - return matcher_.Match(spiffe_id) != not_rule_; + return matcher_.Match(spiffe_id); } std::vector dns_sans = args.GetDnsSans(); if (!dns_sans.empty()) { for (const auto& dns : dns_sans) { if (matcher_.Match(dns)) { - return !not_rule_; + return true; } } } // TODO(ashithasantosh): Check Subject field from certificate. - return not_rule_; + return false; } bool ReqServerNameAuthorizationMatcher::Matches(const EvaluateArgs&) const { // Currently we do not support matching rules containing // "requested_server_name". - bool matches = false; - return matches != not_rule_; + return false; } bool PathAuthorizationMatcher::Matches(const EvaluateArgs& args) const { - bool matches = false; absl::string_view path = args.GetPath(); if (!path.empty()) { - matches = matcher_.Match(path); + return matcher_.Match(path); } - return matches != not_rule_; + return false; } bool PolicyAuthorizationMatcher::Matches(const EvaluateArgs& args) const { diff --git a/src/core/lib/security/authorization/matchers.h b/src/core/lib/security/authorization/matchers.h index b74bd7541ed..95ebd515837 100644 --- a/src/core/lib/security/authorization/matchers.h +++ b/src/core/lib/security/authorization/matchers.h @@ -47,46 +47,46 @@ class AuthorizationMatcher { class AlwaysAuthorizationMatcher : public AuthorizationMatcher { public: - explicit AlwaysAuthorizationMatcher(bool not_rule = false) - : not_rule_(not_rule) {} + explicit AlwaysAuthorizationMatcher() = default; - bool Matches(const EvaluateArgs&) const override { return !not_rule_; } - - private: - // Negates matching the provided permission/principal. - const bool not_rule_; + bool Matches(const EvaluateArgs&) const override { return true; } }; class AndAuthorizationMatcher : public AuthorizationMatcher { public: explicit AndAuthorizationMatcher( - std::vector> rules, - bool not_rule = false); - explicit AndAuthorizationMatcher( - std::vector> ids, bool not_rule = false); + std::vector> matchers) + : matchers_(std::move(matchers)) {} bool Matches(const EvaluateArgs& args) const override; private: std::vector> matchers_; - // Negates matching the provided permission/principal. - const bool not_rule_; }; class OrAuthorizationMatcher : public AuthorizationMatcher { public: explicit OrAuthorizationMatcher( - std::vector> rules, - bool not_rule = false); - explicit OrAuthorizationMatcher( - std::vector> ids, bool not_rule = false); + std::vector> matchers) + : matchers_(std::move(matchers)) {} bool Matches(const EvaluateArgs& args) const override; private: std::vector> matchers_; - // Negates matching the provided permission/principal. - const bool not_rule_; +}; + +// Negates matching the provided permission/principal. +class NotAuthorizationMatcher : public AuthorizationMatcher { + public: + explicit NotAuthorizationMatcher( + std::unique_ptr matcher) + : matcher_(std::move(matcher)) {} + + bool Matches(const EvaluateArgs& args) const override; + + private: + std::unique_ptr matcher_; }; // TODO(ashithasantosh): Add matcher implementation for metadata field. @@ -94,16 +94,13 @@ class OrAuthorizationMatcher : public AuthorizationMatcher { // Perform a match against HTTP headers. class HeaderAuthorizationMatcher : public AuthorizationMatcher { public: - explicit HeaderAuthorizationMatcher(HeaderMatcher matcher, - bool not_rule = false) - : matcher_(std::move(matcher)), not_rule_(not_rule) {} + explicit HeaderAuthorizationMatcher(HeaderMatcher matcher) + : matcher_(std::move(matcher)) {} bool Matches(const EvaluateArgs& args) const override; private: const HeaderMatcher matcher_; - // Negates matching the provided permission/principal. - const bool not_rule_; }; // Perform a match against IP Cidr Range. @@ -116,8 +113,7 @@ class IpAuthorizationMatcher : public AuthorizationMatcher { kRemoteIp, }; - IpAuthorizationMatcher(Type type, Rbac::CidrRange range, - bool not_rule = false); + IpAuthorizationMatcher(Type type, Rbac::CidrRange range); bool Matches(const EvaluateArgs& args) const override; @@ -126,38 +122,30 @@ class IpAuthorizationMatcher : public AuthorizationMatcher { // Subnet masked address. grpc_resolved_address subnet_address_; const uint32_t prefix_len_; - // Negates matching the provided permission/principal. - const bool not_rule_; }; // Perform a match against port number of the destination (local) address. class PortAuthorizationMatcher : public AuthorizationMatcher { public: - explicit PortAuthorizationMatcher(int port, bool not_rule = false) - : port_(port), not_rule_(not_rule) {} + explicit PortAuthorizationMatcher(int port) : port_(port) {} bool Matches(const EvaluateArgs& args) const override; private: const int port_; - // Negates matching the provided permission/principal. - const bool not_rule_; }; // Matches the principal name as described in the peer certificate. Uses URI SAN // or DNS SAN in that order, otherwise uses subject field. class AuthenticatedAuthorizationMatcher : public AuthorizationMatcher { public: - explicit AuthenticatedAuthorizationMatcher(StringMatcher auth, - bool not_rule = false) - : matcher_(std::move(auth)), not_rule_(not_rule) {} + explicit AuthenticatedAuthorizationMatcher(StringMatcher auth) + : matcher_(std::move(auth)) {} bool Matches(const EvaluateArgs& args) const override; private: const StringMatcher matcher_; - // Negates matching the provided permission/principal. - const bool not_rule_; }; // Perform a match against the request server from the client's connection @@ -165,29 +153,25 @@ class AuthenticatedAuthorizationMatcher : public AuthorizationMatcher { class ReqServerNameAuthorizationMatcher : public AuthorizationMatcher { public: explicit ReqServerNameAuthorizationMatcher( - StringMatcher requested_server_name, bool not_rule = false) - : matcher_(std::move(requested_server_name)), not_rule_(not_rule) {} + StringMatcher requested_server_name) + : matcher_(std::move(requested_server_name)) {} bool Matches(const EvaluateArgs&) const override; private: const StringMatcher matcher_; - // Negates matching the provided permission/principal. - const bool not_rule_; }; // Perform a match against the path header of HTTP request. class PathAuthorizationMatcher : public AuthorizationMatcher { public: - explicit PathAuthorizationMatcher(StringMatcher path, bool not_rule = false) - : matcher_(std::move(path)), not_rule_(not_rule) {} + explicit PathAuthorizationMatcher(StringMatcher path) + : matcher_(std::move(path)) {} bool Matches(const EvaluateArgs& args) const override; private: const StringMatcher matcher_; - // Negates matching the provided permission/principal. - const bool not_rule_; }; // Performs a match for policy field in RBAC, which is a collection of diff --git a/src/core/lib/security/authorization/rbac_policy.cc b/src/core/lib/security/authorization/rbac_policy.cc index a081c514147..a6f7ff2c8a5 100644 --- a/src/core/lib/security/authorization/rbac_policy.cc +++ b/src/core/lib/security/authorization/rbac_policy.cc @@ -77,31 +77,31 @@ std::string Rbac::CidrRange::ToString() const { Rbac::Permission::Permission( Permission::RuleType type, - std::vector> permissions, bool not_rule) - : type(type), permissions(std::move(permissions)), not_rule(not_rule) {} -Rbac::Permission::Permission(Permission::RuleType type, bool not_rule) - : type(type), not_rule(not_rule) {} + std::vector> permissions) + : type(type), permissions(std::move(permissions)) {} +Rbac::Permission::Permission(Permission::RuleType type, Permission permission) + : type(type) { + permissions.push_back( + absl::make_unique(std::move(permission))); +} +Rbac::Permission::Permission(Permission::RuleType type) : type(type) {} Rbac::Permission::Permission(Permission::RuleType type, - HeaderMatcher header_matcher, bool not_rule) - : type(type), - header_matcher(std::move(header_matcher)), - not_rule(not_rule) {} + HeaderMatcher header_matcher) + : type(type), header_matcher(std::move(header_matcher)) {} Rbac::Permission::Permission(Permission::RuleType type, - StringMatcher string_matcher, bool not_rule) - : type(type), - string_matcher(std::move(string_matcher)), - not_rule(not_rule) {} -Rbac::Permission::Permission(Permission::RuleType type, CidrRange ip, - bool not_rule) - : type(type), ip(std::move(ip)), not_rule(not_rule) {} -Rbac::Permission::Permission(Permission::RuleType type, int port, bool not_rule) - : type(type), port(port), not_rule(not_rule) {} + StringMatcher string_matcher) + : type(type), string_matcher(std::move(string_matcher)) {} +Rbac::Permission::Permission(Permission::RuleType type, CidrRange ip) + : type(type), ip(std::move(ip)) {} +Rbac::Permission::Permission(Permission::RuleType type, int port) + : type(type), port(port) {} Rbac::Permission::Permission(Rbac::Permission&& other) noexcept - : type(other.type), not_rule(other.not_rule) { + : type(other.type) { switch (type) { case RuleType::kAnd: case RuleType::kOr: + case RuleType::kNot: permissions = std::move(other.permissions); break; case RuleType::kAny: @@ -124,10 +124,10 @@ Rbac::Permission::Permission(Rbac::Permission&& other) noexcept Rbac::Permission& Rbac::Permission::operator=( Rbac::Permission&& other) noexcept { type = other.type; - not_rule = other.not_rule; switch (type) { case RuleType::kAnd: case RuleType::kOr: + case RuleType::kNot: permissions = std::move(other.permissions); break; case RuleType::kAny: @@ -156,8 +156,7 @@ std::string Rbac::Permission::ToString() const { for (const auto& permission : permissions) { contents.push_back(permission->ToString()); } - return absl::StrFormat("%sand=[%s]", not_rule ? "not " : "", - absl::StrJoin(contents, ",")); + return absl::StrFormat("and=[%s]", absl::StrJoin(contents, ",")); } case RuleType::kOr: { std::vector contents; @@ -165,25 +164,23 @@ std::string Rbac::Permission::ToString() const { for (const auto& permission : permissions) { contents.push_back(permission->ToString()); } - return absl::StrFormat("%sor=[%s]", not_rule ? "not " : "", - absl::StrJoin(contents, ",")); + return absl::StrFormat("or=[%s]", absl::StrJoin(contents, ",")); } + case RuleType::kNot: + return absl::StrFormat("not %s", permissions[0]->ToString()); case RuleType::kAny: - return absl::StrFormat("%sany", not_rule ? "not " : ""); + return "any"; case RuleType::kHeader: - return absl::StrFormat("%sheader=%s", not_rule ? "not " : "", - header_matcher.ToString()); + return absl::StrFormat("header=%s", header_matcher.ToString()); case RuleType::kPath: - return absl::StrFormat("%spath=%s", not_rule ? "not " : "", - string_matcher.ToString()); + return absl::StrFormat("path=%s", string_matcher.ToString()); case RuleType::kDestIp: - return absl::StrFormat("%sdest_ip=%s", not_rule ? "not " : "", - ip.ToString()); + return absl::StrFormat("dest_ip=%s", ip.ToString()); case RuleType::kDestPort: - return absl::StrFormat("%sdest_port=%d", not_rule ? "not " : "", port); + return absl::StrFormat("dest_port=%d", port); case RuleType::kReqServerName: - return absl::StrFormat("%srequested_server_name=%s", - not_rule ? "not " : "", string_matcher.ToString()); + return absl::StrFormat("requested_server_name=%s", + string_matcher.ToString()); default: return ""; } @@ -194,30 +191,29 @@ std::string Rbac::Permission::ToString() const { // Rbac::Principal::Principal(Principal::RuleType type, - std::vector> principals, - bool not_rule) - : type(type), principals(std::move(principals)), not_rule(not_rule) {} -Rbac::Principal::Principal(Principal::RuleType type, bool not_rule) - : type(type), not_rule(not_rule) {} + std::vector> principals) + : type(type), principals(std::move(principals)) {} +Rbac::Principal::Principal(Principal::RuleType type, Principal principal) + : type(type) { + principals.push_back( + absl::make_unique(std::move(principal))); +} +Rbac::Principal::Principal(Principal::RuleType type) : type(type) {} Rbac::Principal::Principal(Principal::RuleType type, - StringMatcher string_matcher, bool not_rule) - : type(type), - string_matcher(std::move(string_matcher)), - not_rule(not_rule) {} -Rbac::Principal::Principal(Principal::RuleType type, CidrRange ip, - bool not_rule) - : type(type), ip(std::move(ip)), not_rule(not_rule) {} + StringMatcher string_matcher) + : type(type), string_matcher(std::move(string_matcher)) {} +Rbac::Principal::Principal(Principal::RuleType type, CidrRange ip) + : type(type), ip(std::move(ip)) {} Rbac::Principal::Principal(Principal::RuleType type, - HeaderMatcher header_matcher, bool not_rule) - : type(type), - header_matcher(std::move(header_matcher)), - not_rule(not_rule) {} + HeaderMatcher header_matcher) + : type(type), header_matcher(std::move(header_matcher)) {} Rbac::Principal::Principal(Rbac::Principal&& other) noexcept - : type(other.type), not_rule(other.not_rule) { + : type(other.type) { switch (type) { case RuleType::kAnd: case RuleType::kOr: + case RuleType::kNot: principals = std::move(other.principals); break; case RuleType::kAny: @@ -236,10 +232,10 @@ Rbac::Principal::Principal(Rbac::Principal&& other) noexcept Rbac::Principal& Rbac::Principal::operator=(Rbac::Principal&& other) noexcept { type = other.type; - not_rule = other.not_rule; switch (type) { case RuleType::kAnd: case RuleType::kOr: + case RuleType::kNot: principals = std::move(other.principals); break; case RuleType::kAny: @@ -265,8 +261,7 @@ std::string Rbac::Principal::ToString() const { for (const auto& principal : principals) { contents.push_back(principal->ToString()); } - return absl::StrFormat("%sand=[%s]", not_rule ? "not " : "", - absl::StrJoin(contents, ",")); + return absl::StrFormat("and=[%s]", absl::StrJoin(contents, ",")); } case RuleType::kOr: { std::vector contents; @@ -274,29 +269,24 @@ std::string Rbac::Principal::ToString() const { for (const auto& principal : principals) { contents.push_back(principal->ToString()); } - return absl::StrFormat("%sor=[%s]", not_rule ? "not " : "", - absl::StrJoin(contents, ",")); + return absl::StrFormat("or=[%s]", absl::StrJoin(contents, ",")); } + case RuleType::kNot: + return absl::StrFormat("not %s", principals[0]->ToString()); case RuleType::kAny: - return absl::StrFormat("%sany", not_rule ? "not " : ""); + return "any"; case RuleType::kPrincipalName: - return absl::StrFormat("%sprincipal_name=%s", not_rule ? "not " : "", - string_matcher.ToString()); + return absl::StrFormat("principal_name=%s", string_matcher.ToString()); case RuleType::kSourceIp: - return absl::StrFormat("%ssource_ip=%s", not_rule ? "not " : "", - ip.ToString()); + return absl::StrFormat("source_ip=%s", ip.ToString()); case RuleType::kDirectRemoteIp: - return absl::StrFormat("%sdirect_remote_ip=%s", not_rule ? "not " : "", - ip.ToString()); + return absl::StrFormat("direct_remote_ip=%s", ip.ToString()); case RuleType::kRemoteIp: - return absl::StrFormat("%sremote_ip=%s", not_rule ? "not " : "", - ip.ToString()); + return absl::StrFormat("remote_ip=%s", ip.ToString()); case RuleType::kHeader: - return absl::StrFormat("%sheader=%s", not_rule ? "not " : "", - header_matcher.ToString()); + return absl::StrFormat("header=%s", header_matcher.ToString()); case RuleType::kPath: - return absl::StrFormat("%spath=%s", not_rule ? "not " : "", - string_matcher.ToString()); + return absl::StrFormat("path=%s", string_matcher.ToString()); default: return ""; } diff --git a/src/core/lib/security/authorization/rbac_policy.h b/src/core/lib/security/authorization/rbac_policy.h index f13232ba403..3963eb84852 100644 --- a/src/core/lib/security/authorization/rbac_policy.h +++ b/src/core/lib/security/authorization/rbac_policy.h @@ -49,6 +49,7 @@ struct Rbac { enum class RuleType { kAnd, kOr, + kNot, kAny, kHeader, kPath, @@ -60,20 +61,19 @@ struct Rbac { Permission() = default; // For kAnd/kOr RuleType. Permission(Permission::RuleType type, - std::vector> permissions, - bool not_rule = false); + std::vector> permissions); + // For kNot RuleType. + Permission(Permission::RuleType type, Permission permission); // For kAny RuleType. - explicit Permission(Permission::RuleType type, bool not_rule = false); + explicit Permission(Permission::RuleType type); // For kHeader RuleType. - Permission(Permission::RuleType type, HeaderMatcher header_matcher, - bool not_rule = false); + Permission(Permission::RuleType type, HeaderMatcher header_matcher); // For kPath/kReqServerName RuleType. - Permission(Permission::RuleType type, StringMatcher string_matcher, - bool not_rule = false); + Permission(Permission::RuleType type, StringMatcher string_matcher); // For kDestIp RuleType. - Permission(Permission::RuleType type, CidrRange ip, bool not_rule = false); + Permission(Permission::RuleType type, CidrRange ip); // For kDestPort RuleType. - Permission(Permission::RuleType type, int port, bool not_rule = false); + Permission(Permission::RuleType type, int port); Permission(Permission&& other) noexcept; Permission& operator=(Permission&& other) noexcept; @@ -85,15 +85,16 @@ struct Rbac { StringMatcher string_matcher; CidrRange ip; int port; - // For type kAnd/kOr. + // For type kAnd/kOr/kNot. For kNot type, the vector will have only one + // element. std::vector> permissions; - bool not_rule = false; }; struct Principal { enum class RuleType { kAnd, kOr, + kNot, kAny, kPrincipalName, kSourceIp, @@ -106,18 +107,17 @@ struct Rbac { Principal() = default; // For kAnd/kOr RuleType. Principal(Principal::RuleType type, - std::vector> principals, - bool not_rule = false); + std::vector> principals); + // For kNot RuleType. + Principal(Principal::RuleType type, Principal principal); // For kAny RuleType. - explicit Principal(Principal::RuleType type, bool not_rule = false); + explicit Principal(Principal::RuleType type); // For kPrincipalName/kPath RuleType. - Principal(Principal::RuleType type, StringMatcher string_matcher, - bool not_rule = false); + Principal(Principal::RuleType type, StringMatcher string_matcher); // For kSourceIp/kDirectRemoteIp/kRemoteIp RuleType. - Principal(Principal::RuleType type, CidrRange ip, bool not_rule = false); + Principal(Principal::RuleType type, CidrRange ip); // For kHeader RuleType. - Principal(Principal::RuleType type, HeaderMatcher header_matcher, - bool not_rule = false); + Principal(Principal::RuleType type, HeaderMatcher header_matcher); Principal(Principal&& other) noexcept; Principal& operator=(Principal&& other) noexcept; @@ -128,9 +128,9 @@ struct Rbac { HeaderMatcher header_matcher; StringMatcher string_matcher; CidrRange ip; - // For type kAnd/kOr. + // For type kAnd/kOr/kNot. For kNot type, the vector will have only one + // element. std::vector> principals; - bool not_rule = false; }; struct Policy { diff --git a/test/core/security/authorization_matchers_test.cc b/test/core/security/authorization_matchers_test.cc index 10aa9e707ab..766b186cfa8 100644 --- a/test/core/security/authorization_matchers_test.cc +++ b/test/core/security/authorization_matchers_test.cc @@ -36,12 +36,6 @@ TEST_F(AuthorizationMatchersTest, AlwaysAuthorizationMatcher) { EXPECT_TRUE(matcher.Matches(args)); } -TEST_F(AuthorizationMatchersTest, NotAlwaysAuthorizationMatcher) { - EvaluateArgs args = args_.MakeEvaluateArgs(); - AlwaysAuthorizationMatcher matcher(/*not_rule=*/true); - EXPECT_FALSE(matcher.Matches(args)); -} - TEST_F(AuthorizationMatchersTest, AndAuthorizationMatcherSuccessfulMatch) { args_.AddPairToMetadata("foo", "bar"); args_.SetLocalEndpoint("ipv4:255.255.255.255:123"); @@ -54,8 +48,9 @@ TEST_F(AuthorizationMatchersTest, AndAuthorizationMatcherSuccessfulMatch) { .value())); rules.push_back(absl::make_unique( Rbac::Permission::RuleType::kDestPort, /*port=*/123)); - AndAuthorizationMatcher matcher(std::move(rules)); - EXPECT_TRUE(matcher.Matches(args)); + auto matcher = AuthorizationMatcher::Create( + Rbac::Permission(Rbac::Permission::RuleType::kAnd, std::move(rules))); + EXPECT_TRUE(matcher->Matches(args)); } TEST_F(AuthorizationMatchersTest, AndAuthorizationMatcherFailedMatch) { @@ -70,23 +65,10 @@ TEST_F(AuthorizationMatchersTest, AndAuthorizationMatcherFailedMatch) { .value())); rules.push_back(absl::make_unique( Rbac::Permission::RuleType::kDestPort, /*port=*/123)); - AndAuthorizationMatcher matcher(std::move(rules)); + auto matcher = AuthorizationMatcher::Create( + Rbac::Permission(Rbac::Permission::RuleType::kAnd, std::move(rules))); // Header rule fails. Expected value "bar", got "not_bar" for key "foo". - EXPECT_FALSE(matcher.Matches(args)); -} - -TEST_F(AuthorizationMatchersTest, NotAndAuthorizationMatcher) { - args_.AddPairToMetadata(":path", "/expected/foo"); - EvaluateArgs args = args_.MakeEvaluateArgs(); - std::vector> ids; - ids.push_back(absl::make_unique( - Rbac::Permission::RuleType::kPath, - StringMatcher::Create(StringMatcher::Type::kExact, - /*matcher=*/"/expected/foo", - /*case_sensitive=*/false) - .value())); - AndAuthorizationMatcher matcher(std::move(ids), /*not_rule=*/true); - EXPECT_FALSE(matcher.Matches(args)); + EXPECT_FALSE(matcher->Matches(args)); } TEST_F(AuthorizationMatchersTest, OrAuthorizationMatcherSuccessfulMatch) { @@ -101,9 +83,10 @@ TEST_F(AuthorizationMatchersTest, OrAuthorizationMatcherSuccessfulMatch) { .value())); rules.push_back(absl::make_unique( Rbac::Permission::RuleType::kDestPort, /*port=*/456)); - OrAuthorizationMatcher matcher(std::move(rules)); + auto matcher = AuthorizationMatcher::Create( + Rbac::Permission(Rbac::Permission::RuleType::kOr, std::move(rules))); // Matches as header rule matches even though port rule fails. - EXPECT_TRUE(matcher.Matches(args)); + EXPECT_TRUE(matcher->Matches(args)); } TEST_F(AuthorizationMatchersTest, OrAuthorizationMatcherFailedMatch) { @@ -115,22 +98,36 @@ TEST_F(AuthorizationMatchersTest, OrAuthorizationMatcherFailedMatch) { HeaderMatcher::Create(/*name=*/"foo", HeaderMatcher::Type::kExact, /*matcher=*/"bar") .value())); - OrAuthorizationMatcher matcher(std::move(rules)); + auto matcher = AuthorizationMatcher::Create( + Rbac::Permission(Rbac::Permission::RuleType::kOr, std::move(rules))); // Header rule fails. Expected value "bar", got "not_bar" for key "foo". - EXPECT_FALSE(matcher.Matches(args)); + EXPECT_FALSE(matcher->Matches(args)); } -TEST_F(AuthorizationMatchersTest, NotOrAuthorizationMatcher) { - args_.AddPairToMetadata("foo", "not_bar"); +TEST_F(AuthorizationMatchersTest, NotAuthorizationMatcherSuccessfulMatch) { + args_.AddPairToMetadata(":path", "/different/foo"); EvaluateArgs args = args_.MakeEvaluateArgs(); - std::vector> rules; - rules.push_back(absl::make_unique( - Rbac::Permission::RuleType::kHeader, - HeaderMatcher::Create(/*name=*/"foo", HeaderMatcher::Type::kExact, - /*matcher=*/"bar") - .value())); - OrAuthorizationMatcher matcher(std::move(rules), /*not_rule=*/true); - EXPECT_TRUE(matcher.Matches(args)); + auto matcher = AuthorizationMatcher::Create(Rbac::Principal( + Rbac::Principal::RuleType::kNot, + Rbac::Principal(Rbac::Principal::RuleType::kPath, + StringMatcher::Create(StringMatcher::Type::kExact, + /*matcher=*/"/expected/foo", + /*case_sensitive=*/false) + .value()))); + EXPECT_TRUE(matcher->Matches(args)); +} + +TEST_F(AuthorizationMatchersTest, NotAuthorizationMatcherFailedMatch) { + args_.AddPairToMetadata(":path", "/expected/foo"); + EvaluateArgs args = args_.MakeEvaluateArgs(); + auto matcher = AuthorizationMatcher::Create(Rbac::Principal( + Rbac::Principal::RuleType::kNot, + Rbac::Principal(Rbac::Principal::RuleType::kPath, + StringMatcher::Create(StringMatcher::Type::kExact, + /*matcher=*/"/expected/foo", + /*case_sensitive=*/false) + .value()))); + EXPECT_FALSE(matcher->Matches(args)); } TEST_F(AuthorizationMatchersTest, HybridAuthorizationMatcherSuccessfulMatch) { @@ -151,8 +148,9 @@ TEST_F(AuthorizationMatchersTest, HybridAuthorizationMatcherSuccessfulMatch) { Rbac::Permission::RuleType::kAnd, std::move(sub_and_rules))); and_rules.push_back(absl::make_unique( Rbac::Permission::RuleType::kOr, std::move(std::move(sub_or_rules)))); - AndAuthorizationMatcher matcher(std::move(and_rules)); - EXPECT_TRUE(matcher.Matches(args)); + auto matcher = AuthorizationMatcher::Create( + Rbac::Permission(Rbac::Permission::RuleType::kAnd, std::move(and_rules))); + EXPECT_TRUE(matcher->Matches(args)); } TEST_F(AuthorizationMatchersTest, HybridAuthorizationMatcherFailedMatch) { @@ -178,9 +176,10 @@ TEST_F(AuthorizationMatchersTest, HybridAuthorizationMatcherFailedMatch) { Rbac::Permission::RuleType::kAnd, std::move(sub_and_rules))); and_rules.push_back(absl::make_unique( Rbac::Permission::RuleType::kOr, std::move(std::move(sub_or_rules)))); - AndAuthorizationMatcher matcher(std::move(and_rules)); + auto matcher = AuthorizationMatcher::Create( + Rbac::Permission(Rbac::Permission::RuleType::kAnd, std::move(and_rules))); // Fails as "absent_key" header was not present. - EXPECT_FALSE(matcher.Matches(args)); + EXPECT_FALSE(matcher->Matches(args)); } TEST_F(AuthorizationMatchersTest, PathAuthorizationMatcherSuccessfulMatch) { @@ -205,18 +204,6 @@ TEST_F(AuthorizationMatchersTest, PathAuthorizationMatcherFailedMatch) { EXPECT_FALSE(matcher.Matches(args)); } -TEST_F(AuthorizationMatchersTest, NotPathAuthorizationMatcher) { - args_.AddPairToMetadata(":path", "expected/path"); - EvaluateArgs args = args_.MakeEvaluateArgs(); - PathAuthorizationMatcher matcher( - StringMatcher::Create(StringMatcher::Type::kExact, - /*matcher=*/"expected/path", - /*case_sensitive=*/false) - .value(), - /*not_rule=*/true); - EXPECT_FALSE(matcher.Matches(args)); -} - TEST_F(AuthorizationMatchersTest, PathAuthorizationMatcherFailedMatchMissingPath) { EvaluateArgs args = args_.MakeEvaluateArgs(); @@ -270,17 +257,6 @@ TEST_F(AuthorizationMatchersTest, EXPECT_FALSE(matcher.Matches(args)); } -TEST_F(AuthorizationMatchersTest, NotHeaderAuthorizationMatcher) { - args_.AddPairToMetadata("key123", "foo"); - EvaluateArgs args = args_.MakeEvaluateArgs(); - HeaderAuthorizationMatcher matcher( - HeaderMatcher::Create(/*name=*/"key123", HeaderMatcher::Type::kExact, - /*matcher=*/"bar") - .value(), - /*not_rule=*/true); - EXPECT_TRUE(matcher.Matches(args)); -} - TEST_F(AuthorizationMatchersTest, IpAuthorizationMatcherLocalIpSuccessfulMatch) { args_.SetLocalEndpoint("ipv4:1.2.3.4:123"); @@ -325,16 +301,6 @@ TEST_F(AuthorizationMatchersTest, EXPECT_FALSE(matcher.Matches(args)); } -TEST_F(AuthorizationMatchersTest, NotIpAuthorizationMatcherSuccessfulMatch) { - args_.SetLocalEndpoint("ipv4:1.2.3.4:123"); - EvaluateArgs args = args_.MakeEvaluateArgs(); - IpAuthorizationMatcher matcher( - IpAuthorizationMatcher::Type::kDestIp, - Rbac::CidrRange(/*address_prefix=*/"1.7.8.9", /*prefix_len=*/8), - /*not_rule=*/true); - EXPECT_FALSE(matcher.Matches(args)); -} - TEST_F(AuthorizationMatchersTest, PortAuthorizationMatcherSuccessfulMatch) { args_.SetLocalEndpoint("ipv4:255.255.255.255:123"); EvaluateArgs args = args_.MakeEvaluateArgs(); @@ -349,13 +315,6 @@ TEST_F(AuthorizationMatchersTest, PortAuthorizationMatcherFailedMatch) { EXPECT_FALSE(matcher.Matches(args)); } -TEST_F(AuthorizationMatchersTest, NotPortAuthorizationMatcher) { - args_.SetLocalEndpoint("ipv4:255.255.255.255:123"); - EvaluateArgs args = args_.MakeEvaluateArgs(); - PortAuthorizationMatcher matcher(/*port=*/123, /*not_rule=*/true); - EXPECT_FALSE(matcher.Matches(args)); -} - TEST_F(AuthorizationMatchersTest, AuthenticatedMatcherUnAuthenticatedConnection) { EvaluateArgs args = args_.MakeEvaluateArgs(); @@ -451,18 +410,6 @@ TEST_F(AuthorizationMatchersTest, AuthenticatedMatcherFailedNothingMatches) { EXPECT_FALSE(matcher.Matches(args)); } -TEST_F(AuthorizationMatchersTest, NotAuthenticatedMatcher) { - args_.AddPropertyToAuthContext(GRPC_TRANSPORT_SECURITY_TYPE_PROPERTY_NAME, - GRPC_SSL_TRANSPORT_SECURITY_TYPE); - EvaluateArgs args = args_.MakeEvaluateArgs(); - AuthenticatedAuthorizationMatcher matcher( - StringMatcher::Create(StringMatcher::Type::kExact, /*matcher=*/"foo", - /*case_sensitive=*/false) - .value(), - /*not_rule=*/true); - EXPECT_TRUE(matcher.Matches(args)); -} - TEST_F(AuthorizationMatchersTest, PolicyAuthorizationMatcherSuccessfulMatch) { args_.AddPairToMetadata("key123", "foo"); EvaluateArgs args = args_.MakeEvaluateArgs(); diff --git a/test/core/security/grpc_authorization_engine_test.cc b/test/core/security/grpc_authorization_engine_test.cc index a2b5e11443e..e6dd4add847 100644 --- a/test/core/security/grpc_authorization_engine_test.cc +++ b/test/core/security/grpc_authorization_engine_test.cc @@ -23,8 +23,10 @@ namespace grpc_core { TEST(GrpcAuthorizationEngineTest, AllowEngineWithMatchingPolicy) { Rbac::Policy policy1( - Rbac::Permission(Rbac::Permission::RuleType::kAny, /*not_rule=*/true), - Rbac::Principal(Rbac::Principal::RuleType::kAny, /*not_rule=*/true)); + Rbac::Permission(Rbac::Permission::RuleType::kNot, + Rbac::Permission(Rbac::Permission::RuleType::kAny)), + Rbac::Principal(Rbac::Principal::RuleType::kNot, + Rbac::Principal(Rbac::Principal::RuleType::kAny))); Rbac::Policy policy2((Rbac::Permission(Rbac::Permission::RuleType::kAny)), (Rbac::Principal(Rbac::Principal::RuleType::kAny))); std::map policies; @@ -40,8 +42,10 @@ TEST(GrpcAuthorizationEngineTest, AllowEngineWithMatchingPolicy) { TEST(GrpcAuthorizationEngineTest, AllowEngineWithNoMatchingPolicy) { Rbac::Policy policy1( - Rbac::Permission(Rbac::Permission::RuleType::kAny, /*not_rule=*/true), - Rbac::Principal(Rbac::Principal::RuleType::kAny, /*not_rule=*/true)); + Rbac::Permission(Rbac::Permission::RuleType::kNot, + Rbac::Permission(Rbac::Permission::RuleType::kAny)), + Rbac::Principal(Rbac::Principal::RuleType::kNot, + Rbac::Principal(Rbac::Principal::RuleType::kAny))); std::map policies; policies["policy1"] = std::move(policy1); Rbac rbac(Rbac::Action::kAllow, std::move(policies)); @@ -62,8 +66,10 @@ TEST(GrpcAuthorizationEngineTest, AllowEngineWithEmptyPolicies) { TEST(GrpcAuthorizationEngineTest, DenyEngineWithMatchingPolicy) { Rbac::Policy policy1( - Rbac::Permission(Rbac::Permission::RuleType::kAny, /*not_rule=*/true), - Rbac::Principal(Rbac::Principal::RuleType::kAny, /*not_rule=*/true)); + Rbac::Permission(Rbac::Permission::RuleType::kNot, + Rbac::Permission(Rbac::Permission::RuleType::kAny)), + Rbac::Principal(Rbac::Principal::RuleType::kNot, + Rbac::Principal(Rbac::Principal::RuleType::kAny))); Rbac::Policy policy2((Rbac::Permission(Rbac::Permission::RuleType::kAny)), (Rbac::Principal(Rbac::Principal::RuleType::kAny))); std::map policies; @@ -79,8 +85,10 @@ TEST(GrpcAuthorizationEngineTest, DenyEngineWithMatchingPolicy) { TEST(GrpcAuthorizationEngineTest, DenyEngineWithNoMatchingPolicy) { Rbac::Policy policy1( - Rbac::Permission(Rbac::Permission::RuleType::kAny, /*not_rule=*/true), - Rbac::Principal(Rbac::Principal::RuleType::kAny, /*not_rule=*/true)); + Rbac::Permission(Rbac::Permission::RuleType::kNot, + Rbac::Permission(Rbac::Permission::RuleType::kAny)), + Rbac::Principal(Rbac::Principal::RuleType::kNot, + Rbac::Principal(Rbac::Principal::RuleType::kAny))); std::map policies; policies["policy1"] = std::move(policy1); Rbac rbac(Rbac::Action::kDeny, std::move(policies));