[Audit Logging] Authz policy support for audit logging

src/core/ext/filters/rbac/rbac_service_config_parser.cc

@@ -193,6 +193,7 @@ struct RbacConfig {
       void JsonPostLoad(const Json&, const JsonArgs&, ValidationErrors* errors);
     };
 
+    std::string name;
     absl::optional<Rules> rules;
 
     Rbac TakeAsRbac();
@@ -756,15 +757,15 @@ Rbac RbacConfig::RbacPolicy::TakeAsRbac() {
   if (!rules.has_value()) {
     // No enforcing to be applied. An empty deny policy with an empty map
     // is equivalent to no enforcing.
-    // TODO(lwge): Pass the fitler name when working on this parser.
-    return Rbac(Rbac::Action::kDeny, {}, "");
+    return Rbac(name, Rbac::Action::kDeny, {});
   }
   return rules->TakeAsRbac();
 }
 
 const JsonLoaderInterface* RbacConfig::RbacPolicy::JsonLoader(const JsonArgs&) {
   static const auto* loader = JsonObjectLoader<RbacPolicy>()
                                   .OptionalField("rules", &RbacPolicy::rules)
+                                  .Field("filter_name", &RbacPolicy::name)
                                   .Finish();
   return loader;
 }

src/core/lib/security/authorization/rbac_policy.cc

@@ -29,29 +29,29 @@ namespace grpc_core {
 // Rbac
 //
 
-Rbac::Rbac(Rbac::Action action, std::map<std::string, Policy> policies,
-           absl::string_view name)
-    : action(action), policies(std::move(policies)), name(name) {}
+Rbac::Rbac(absl::string_view name, Rbac::Action action,
+           std::map<std::string, Policy> policies)
+    : name(name), action(action), policies(std::move(policies)) {}
 
 Rbac::Rbac(Rbac&& other) noexcept
-    : action(other.action),
-      audit_condition(other.audit_condition),
+    : name(std::move(other.name)),
+      action(other.action),
       policies(std::move(other.policies)),
-      logger_configs(std::move(other.logger_configs)),
-      name(std::move(other.name)) {}
+      audit_condition(other.audit_condition),
+      logger_configs(std::move(other.logger_configs)) {}
 
 Rbac& Rbac::operator=(Rbac&& other) noexcept {
+  name = std::move(other.name);
   action = other.action;
-  audit_condition = other.audit_condition;
   policies = std::move(other.policies);
+  audit_condition = other.audit_condition;
   logger_configs = std::move(other.logger_configs);
-  name = std::move(other.name);
   return *this;
 }
 
 std::string Rbac::ToString() const {
   std::vector<std::string> contents;
-  std::string condition_str;
+  absl::string_view condition_str;
   switch (audit_condition) {
     case Rbac::AuditCondition::kNone:
       condition_str = "None";
@@ -73,6 +73,10 @@ std::string Rbac::ToString() const {
     contents.push_back(absl::StrFormat("{\n  policy_name=%s\n%s\n}", p.first,
                                        p.second.ToString()));
   }
+  for (const auto& config : logger_configs) {
+    contents.push_back(absl::StrFormat("{\n  audit_logger=%s\n%s\n}",
+                                       config->name(), config->ToString()));
+  }
   contents.push_back("}");
   return absl::StrJoin(contents, "\n");
 }

src/core/lib/security/authorization/rbac_policy.h

@@ -172,22 +172,23 @@ struct Rbac {
   };
 
   Rbac() = default;
-  Rbac(Rbac::Action action, std::map<std::string, Policy> policies,
-       absl::string_view name);
+  Rbac(absl::string_view name, Rbac::Action action,
+       std::map<std::string, Policy> policies);
 
   Rbac(Rbac&& other) noexcept;
   Rbac& operator=(Rbac&& other) noexcept;
 
   std::string ToString() const;
 
-  Action action;
-  AuditCondition audit_condition;
+  // The authorization policy name or the HTTP RBAC filter name.
+  std::string name;
 
+  Action action;
   std::map<std::string, Policy> policies;
+
+  AuditCondition audit_condition;
   std::vector<std::unique_ptr<experimental::AuditLoggerFactory::Config>>
       logger_configs;
-  // The authorization policy name or the HTTP RBAC filter name.
-  std::string name;
 };
 
 }  // namespace grpc_core

src/core/lib/security/authorization/rbac_translator.cc

@@ -345,14 +345,14 @@ absl::StatusOr<Rbac> ParseDenyRulesArray(const Json& json,
                                          absl::string_view name) {
   auto policies_or = ParseRulesArray(json);
   if (!policies_or.ok()) return policies_or.status();
-  return Rbac(Rbac::Action::kDeny, std::move(policies_or.value()), name);
+  return Rbac(name, Rbac::Action::kDeny, std::move(policies_or.value()));
 }
 
 absl::StatusOr<Rbac> ParseAllowRulesArray(const Json& json,
                                           absl::string_view name) {
   auto policies_or = ParseRulesArray(json);
   if (!policies_or.ok()) return policies_or.status();
-  return Rbac(Rbac::Action::kAllow, std::move(policies_or.value()), name);
+  return Rbac(name, Rbac::Action::kAllow, std::move(policies_or.value()));
 }
 
 absl::StatusOr<std::unique_ptr<experimental::AuditLoggerFactory::Config>>
@@ -374,10 +374,8 @@ ParseAuditLogger(const Json& json, size_t pos) {
   auto it = json.object().find("is_optional");
   if (it != json.object().end()) {
     switch (it->second.type()) {
-      case Json::Type::kTrue:
-        is_optional = true;
-        break;
-      case Json::Type::kFalse:
+      case Json::Type::kBoolean:
+        is_optional = it->second.boolean();
         break;
       default:
         return absl::InvalidArgumentError(absl::StrFormat(
@@ -394,7 +392,8 @@ ParseAuditLogger(const Json& json, size_t pos) {
         absl::StrFormat("\"audit_loggers[%d].name\" is not a string.", pos));
   }
   absl::string_view name = it->second.string();
-  Json config = Json::Object();
+  // The config defaults to an empty object.
+  Json config = Json::FromObject(Json::Object());
   it = json.object().find("config");
   if (it != json.object().end()) {
     if (it->second.type() != Json::Type::kObject) {
@@ -420,7 +419,8 @@ ParseAuditLogger(const Json& json, size_t pos) {
   return result;
 }
 
-absl::Status ParseAuditLoggingOptions(RbacPolicies& rbacs, const Json& json) {
+absl::Status ParseAuditLoggingOptions(const Json& json, RbacPolicies* rbacs) {
+  GPR_ASSERT(rbacs != nullptr);
   for (auto it = json.object().begin(); it != json.object().end(); ++it) {
     if (it->first == "audit_condition") {
       if (it->second.type() != Json::Type::kString) {
@@ -445,10 +445,10 @@ absl::Status ParseAuditLoggingOptions(RbacPolicies& rbacs, const Json& json) {
         return absl::InvalidArgumentError(absl::StrFormat(
             "Unsupported \"audit_condition\" value %s.", condition));
       }
-      if (rbacs.deny_policy != absl::nullopt) {
-        rbacs.deny_policy->audit_condition = deny_condition;
+      if (rbacs->deny_policy.has_value()) {
+        rbacs->deny_policy->audit_condition = deny_condition;
       }
-      rbacs.allow_policy.audit_condition = allow_condition;
+      rbacs->allow_policy.audit_condition = allow_condition;
     } else if (it->first == "audit_loggers") {
       if (it->second.type() != Json::Type::kArray) {
         return absl::InvalidArgumentError("\"audit_loggers\" is not an array.");
@@ -463,19 +463,19 @@ absl::Status ParseAuditLoggingOptions(RbacPolicies& rbacs, const Json& json) {
         // return ok when marked as optional.
         if (result.value() != nullptr) {
           // Only move the logger config over if audit condition is not NONE.
-          if (rbacs.allow_policy.audit_condition !=
+          if (rbacs->allow_policy.audit_condition !=
               Rbac::AuditCondition::kNone) {
-            rbacs.allow_policy.logger_configs.push_back(
+            rbacs->allow_policy.logger_configs.push_back(
                 std::move(result.value()));
           }
-          if (rbacs.deny_policy != absl::nullopt &&
-              rbacs.deny_policy->audit_condition !=
+          if (rbacs->deny_policy.has_value() &&
+              rbacs->deny_policy->audit_condition !=
                   Rbac::AuditCondition::kNone) {
             // Parse again since it returns unique_ptr, but result should be ok
             // this time.
             auto result = ParseAuditLogger(loggers.at(i), i);
             GPR_ASSERT(result.ok());
-            rbacs.deny_policy->logger_configs.push_back(
+            rbacs->deny_policy->logger_configs.push_back(
                 std::move(result.value()));
           }
         }
@@ -553,7 +553,7 @@ absl::StatusOr<RbacPolicies> GenerateRbacPolicies(
       return absl::InvalidArgumentError(
           "\"audit_logging_options\" is not an object.");
     }
-    absl::Status status = ParseAuditLoggingOptions(rbacs, it->second);
+    absl::Status status = ParseAuditLoggingOptions(it->second, &rbacs);
     if (!status.ok()) {
       return status;
     }

test/core/ext/filters/rbac/rbac_service_config_parser_test.cc

@@ -30,7 +30,7 @@ namespace grpc_core {
 namespace testing {
 namespace {
 
-// Test basic parsing of RBAC policy
+// Filter name is required in RBAC policy.
 TEST(RbacServiceConfigParsingTest, EmptyRbacPolicy) {
   const char* test_json =
       "{\n"
@@ -44,6 +44,27 @@ TEST(RbacServiceConfigParsingTest, EmptyRbacPolicy) {
       "}";
   ChannelArgs args = ChannelArgs().Set(GRPC_ARG_PARSE_RBAC_METHOD_CONFIG, 1);
   auto service_config = ServiceConfigImpl::Create(args, test_json);
+  EXPECT_EQ(service_config.status().code(), absl::StatusCode::kInvalidArgument);
+  EXPECT_EQ(service_config.status().message(),
+            "errors validating service config: ["
+            "field:methodConfig[0].rbacPolicy[0].filter_name error:field not "
+            "present]")
+      << service_config.status();
+}
+
+// Test basic parsing of RBAC policy
+TEST(RbacServiceConfigParsingTest, RbacPolicyWithoutRules) {
+  const char* test_json =
+      "{\n"
+      "  \"methodConfig\": [ {\n"
+      "    \"name\": [\n"
+      "      {}\n"
+      "    ],\n"
+      "    \"rbacPolicy\": [ {\"filter_name\": \"rbac\"} ]\n"
+      "  } ]\n"
+      "}";
+  ChannelArgs args = ChannelArgs().Set(GRPC_ARG_PARSE_RBAC_METHOD_CONFIG, 1);
+  auto service_config = ServiceConfigImpl::Create(args, test_json);
   ASSERT_TRUE(service_config.ok()) << service_config.status();
   const auto* vector_ptr =
       (*service_config)->GetMethodParsedConfigVector(grpc_empty_slice());
@@ -110,7 +131,11 @@ TEST(RbacServiceConfigParsingTest, MultipleRbacPolicies) {
       "    \"name\": [\n"
       "      {}\n"
       "    ],\n"
-      "    \"rbacPolicy\": [ {}, {}, {} ]"
+      "    \"rbacPolicy\": [\n"
+      "      { \"filter_name\": \"rbac-1\" },\n"
+      "      { \"filter_name\": \"rbac-2\" },\n"
+      "      { \"filter_name\": \"rbac-3\" }\n"
+      "    ]"
       "  } ]\n"
       "}";
   ChannelArgs args = ChannelArgs().Set(GRPC_ARG_PARSE_RBAC_METHOD_CONFIG, 1);
@@ -156,7 +181,7 @@ TEST(RbacServiceConfigParsingTest, BadRulesType) {
       "    \"name\": [\n"
       "      {}\n"
       "    ],\n"
-      "    \"rbacPolicy\": [{\"rules\":1}]"
+      "    \"rbacPolicy\": [{\"filter_name\": \"rbac\", \"rules\":1}]\n"
       "  } ]\n"
       "}";
   ChannelArgs args = ChannelArgs().Set(GRPC_ARG_PARSE_RBAC_METHOD_CONFIG, 1);
@@ -176,6 +201,7 @@ TEST(RbacServiceConfigParsingTest, BadActionAndPolicyType) {
       "      {}\n"
       "    ],\n"
       "    \"rbacPolicy\": [{\n"
+      "      \"filter_name\": \"rbac\",\n"
       "      \"rules\":{\n"
       "        \"action\":{},\n"
       "        \"policies\":123\n"
@@ -203,6 +229,7 @@ TEST(RbacServiceConfigParsingTest, MissingPermissionAndPrincipals) {
       "      {}\n"
       "    ],\n"
       "    \"rbacPolicy\": [{\n"
+      "      \"filter_name\": \"rbac\",\n"
       "      \"rules\":{\n"
       "        \"action\":1,\n"
       "        \"policies\":{\n"
@@ -233,6 +260,7 @@ TEST(RbacServiceConfigParsingTest, EmptyPrincipalAndPermission) {
       "      {}\n"
       "    ],\n"
       "    \"rbacPolicy\": [{\n"
+      "      \"filter_name\": \"rbac\",\n"
       "      \"rules\":{\n"
       "        \"action\":1,\n"
       "        \"policies\":{\n"
@@ -265,6 +293,7 @@ TEST(RbacServiceConfigParsingTest, VariousPermissionsAndPrincipalsTypes) {
       "      {}\n"
       "    ],\n"
       "    \"rbacPolicy\": [{\n"
+      "      \"filter_name\": \"rbac\",\n"
       "      \"rules\":{\n"
       "        \"action\":1,\n"
       "        \"policies\":{\n"
@@ -322,6 +351,7 @@ TEST(RbacServiceConfigParsingTest, VariousPermissionsAndPrincipalsBadTypes) {
       "      {}\n"
       "    ],\n"
       "    \"rbacPolicy\": [{\n"
+      "      \"filter_name\": \"rbac\",\n"
       "      \"rules\":{\n"
       "        \"action\":1,\n"
       "        \"policies\":{\n"
@@ -416,6 +446,7 @@ TEST(RbacServiceConfigParsingTest, HeaderMatcherVariousTypes) {
       "      {}\n"
       "    ],\n"
       "    \"rbacPolicy\": [{\n"
+      "      \"filter_name\": \"rbac\",\n"
       "      \"rules\":{\n"
       "        \"action\":1,\n"
       "        \"policies\":{\n"
@@ -460,6 +491,7 @@ TEST(RbacServiceConfigParsingTest, HeaderMatcherBadTypes) {
       "      {}\n"
       "    ],\n"
       "    \"rbacPolicy\": [{\n"
+      "      \"filter_name\": \"rbac\",\n"
       "      \"rules\":{\n"
       "        \"action\":1,\n"
       "        \"policies\":{\n"
@@ -516,6 +548,7 @@ TEST(RbacServiceConfigParsingTest, StringMatcherVariousTypes) {
       "      {}\n"
       "    ],\n"
       "    \"rbacPolicy\": [{\n"
+      "      \"filter_name\": \"rbac\",\n"
       "      \"rules\":{\n"
       "        \"action\":1,\n"
       "        \"policies\":{\n"
@@ -557,6 +590,7 @@ TEST(RbacServiceConfigParsingTest, StringMatcherBadTypes) {
       "      {}\n"
       "    ],\n"
       "    \"rbacPolicy\": [{\n"
+      "      \"filter_name\": \"rbac\",\n"
       "      \"rules\":{\n"
       "        \"action\":1,\n"
       "        \"policies\":{\n"

test/core/security/grpc_authorization_engine_test.cc

@@ -31,7 +31,7 @@ TEST(GrpcAuthorizationEngineTest, AllowEngineWithMatchingPolicy) {
   std::map<std::string, Rbac::Policy> policies;
   policies["policy1"] = std::move(policy1);
   policies["policy2"] = std::move(policy2);
-  Rbac rbac(Rbac::Action::kAllow, std::move(policies), "authz");
+  Rbac rbac("authz", Rbac::Action::kAllow, std::move(policies));
   GrpcAuthorizationEngine engine(std::move(rbac));
   AuthorizationEngine::Decision decision =
       engine.Evaluate(EvaluateArgs(nullptr, nullptr));
@@ -46,7 +46,7 @@ TEST(GrpcAuthorizationEngineTest, AllowEngineWithNoMatchingPolicy) {
       Rbac::Principal::MakeNotPrincipal(Rbac::Principal::MakeAnyPrincipal()));
   std::map<std::string, Rbac::Policy> policies;
   policies["policy1"] = std::move(policy1);
-  Rbac rbac(Rbac::Action::kAllow, std::move(policies), "authz");
+  Rbac rbac("authz", Rbac::Action::kAllow, std::move(policies));
   GrpcAuthorizationEngine engine(std::move(rbac));
   AuthorizationEngine::Decision decision =
       engine.Evaluate(EvaluateArgs(nullptr, nullptr));
@@ -72,7 +72,7 @@ TEST(GrpcAuthorizationEngineTest, DenyEngineWithMatchingPolicy) {
   std::map<std::string, Rbac::Policy> policies;
   policies["policy1"] = std::move(policy1);
   policies["policy2"] = std::move(policy2);
-  Rbac rbac(Rbac::Action::kDeny, std::move(policies), "authz");
+  Rbac rbac("authz", Rbac::Action::kDeny, std::move(policies));
   GrpcAuthorizationEngine engine(std::move(rbac));
   AuthorizationEngine::Decision decision =
       engine.Evaluate(EvaluateArgs(nullptr, nullptr));
@@ -87,7 +87,7 @@ TEST(GrpcAuthorizationEngineTest, DenyEngineWithNoMatchingPolicy) {
       Rbac::Principal::MakeNotPrincipal(Rbac::Principal::MakeAnyPrincipal()));
   std::map<std::string, Rbac::Policy> policies;
   policies["policy1"] = std::move(policy1);
-  Rbac rbac(Rbac::Action::kDeny, std::move(policies), "authz");
+  Rbac rbac("authz", Rbac::Action::kDeny, std::move(policies));
   GrpcAuthorizationEngine engine(std::move(rbac));
   AuthorizationEngine::Decision decision =
       engine.Evaluate(EvaluateArgs(nullptr, nullptr));