[Audit Logging] Audit logging config translation by rbac service config parser

include/grpc/grpc_audit_logging.h

@@ -62,6 +62,7 @@ class AuditContext {
 class AuditLogger {
  public:
   virtual ~AuditLogger() = default;
+  virtual absl::string_view name() const = 0;
   virtual void Log(const AuditContext& audit_context) = 0;
 };
 

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

@@ -200,7 +200,7 @@ struct RbacConfig {
       int action;
       std::map<std::string, Policy> policies;
       // Defaults to 0 since its json field is optional.
-      Rbac::AuditCondition audit_condition;
+      Rbac::AuditCondition audit_condition = Rbac::AuditCondition::kNone;
       std::vector<std::unique_ptr<AuditLoggerFactory::Config>> logger_configs;
 
       Rules() = default;
@@ -800,32 +800,25 @@ void RbacConfig::RbacPolicy::Rules::JsonPostLoad(const Json& json,
     errors->AddError("unknown action");
   }
   // Parse and validate audit_condition field.
-  auto it = json.object().find("audit_condition");
-  int condition = static_cast<int>(Rbac::AuditCondition::kNone);
-  if (it != json.object().end()) {
-    if (it->second.type() != Json::Type::kNumber) {
-      ValidationErrors::ScopedField field(errors, ".audit_condition");
-      errors->AddError("is not a number");
-    } else {
-      GPR_ASSERT(absl::SimpleAtoi(it->second.string(), &condition));
-    }
-  }
-  switch (condition) {
-    case static_cast<int>(Rbac::AuditCondition::kNone):
-    case static_cast<int>(Rbac::AuditCondition::kOnAllow):
-    case static_cast<int>(Rbac::AuditCondition::kOnDeny):
-    case static_cast<int>(Rbac::AuditCondition::kOnDenyAndAllow):
-      break;
-    default: {
-      ValidationErrors::ScopedField field(errors, ".audit_condition");
-      errors->AddError("unknown audit condition");
+  auto condition = LoadJsonObjectField<int>(json.object(), args,
+                                            "audit_condition", errors, false);
+  if (condition.has_value()) {
+    switch (*condition) {
+      case static_cast<int>(Rbac::AuditCondition::kNone):
+      case static_cast<int>(Rbac::AuditCondition::kOnAllow):
+      case static_cast<int>(Rbac::AuditCondition::kOnDeny):
+      case static_cast<int>(Rbac::AuditCondition::kOnDenyAndAllow):
+        audit_condition = static_cast<Rbac::AuditCondition>(*condition);
+        break;
+      default: {
+        ValidationErrors::ScopedField field(errors, ".audit_condition");
+        errors->AddError("unknown audit condition");
+      }
     }
   }
-  audit_condition = static_cast<Rbac::AuditCondition>(condition);
-  if (json.object().find("audit_loggers") == json.object().end()) return;
   // Parse and validate audit logger configs.
   auto configs = LoadJsonObjectField<std::vector<AuditLogger>>(
-      json.object(), args, "audit_loggers", errors);
+      json.object(), args, "audit_loggers", errors, false);
   if (configs.has_value()) {
     for (size_t i = 0; i < configs->size(); ++i) {
       auto& logger = (*configs)[i];

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

@@ -60,7 +60,9 @@ class GrpcAuthorizationEngine : public AuthorizationEngine {
   Rbac::AuditCondition audit_condition() const { return audit_condition_; }
 
   // Required only for testing purpose.
-  size_t num_audit_loggers() const { return audit_loggers_.size(); }
+  const std::vector<std::unique_ptr<AuditLogger>>& audit_loggers() const {
+    return audit_loggers_;
+  }
 
   // Evaluates incoming request against RBAC policy and makes a decision to
   // whether allow/deny this request.

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

@@ -32,6 +32,7 @@ namespace experimental {
 class StdoutAuditLogger : public AuditLogger {
  public:
   StdoutAuditLogger() = default;
+  absl::string_view name() const override { return "stdout_logger"; }
   void Log(const AuditContext&) override;
 };
 

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

@@ -20,6 +20,7 @@
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
 #include "absl/strings/string_view.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 #include <grpc/grpc.h>
@@ -44,9 +45,12 @@ using experimental::AuditLoggerFactory;
 using experimental::AuditLoggerRegistry;
 using experimental::RegisterAuditLoggerFactory;
 
+constexpr absl::string_view kLoggerName = "test_logger";
+
 class TestAuditLogger : public AuditLogger {
  public:
   TestAuditLogger() = default;
+  absl::string_view name() const override { return kLoggerName; }
   void Log(const AuditContext&) override {}
 };
 
@@ -55,7 +59,7 @@ class TestAuditLoggerFactory : public AuditLoggerFactory {
   class Config : public AuditLoggerFactory::Config {
    public:
     Config(const Json& json) : config_(JsonDump(json)) {}
-    absl::string_view name() const override { return "test_logger"; }
+    absl::string_view name() const override { return kLoggerName; }
     std::string ToString() const override { return config_; }
 
    private:
@@ -64,7 +68,7 @@ class TestAuditLoggerFactory : public AuditLoggerFactory {
 
   TestAuditLoggerFactory(std::map<absl::string_view, std::string>* configs)
       : configs_(configs) {}
-  absl::string_view name() const override { return "test_logger"; }
+  absl::string_view name() const override { return kLoggerName; }
   absl::StatusOr<std::unique_ptr<AuditLoggerFactory::Config>>
   ParseAuditLoggerConfig(const Json& json) override {
     // Invalidate configs with "bad" field in it.
@@ -738,10 +742,13 @@ TEST_F(RbacServiceConfigParsingTest, AuditConditionOnDenyWithMultipleLoggers) {
   ASSERT_NE(parsed_rbac_config->authorization_engine(0), nullptr);
   EXPECT_EQ(parsed_rbac_config->authorization_engine(0)->audit_condition(),
             Rbac::AuditCondition::kOnDeny);
-  EXPECT_EQ(parsed_rbac_config->authorization_engine(0)->num_audit_loggers(),
-            2);
-  ASSERT_NE(logger_configs_.find("test_logger"), logger_configs_.end());
-  EXPECT_EQ(logger_configs_.find("test_logger")->second, "{\"foo\":\"bar\"}");
+  const auto& loggers =
+      parsed_rbac_config->authorization_engine(0)->audit_loggers();
+  ASSERT_EQ(loggers.size(), 2);
+  EXPECT_EQ(loggers[0]->name(), "stdout_logger");
+  EXPECT_EQ(loggers[1]->name(), kLoggerName);
+  EXPECT_THAT(logger_configs_, ::testing::ElementsAre(::testing::Pair(
+                                   "test_logger", "{\"foo\":\"bar\"}")));
 }
 
 TEST_F(RbacServiceConfigParsingTest, BadAuditLoggerConfig) {
@@ -775,6 +782,37 @@ TEST_F(RbacServiceConfigParsingTest, BadAuditLoggerConfig) {
       << service_config.status();
 }
 
+TEST_F(RbacServiceConfigParsingTest, UnknownAuditLoggerConfig) {
+  const char* test_json =
+      "{\n"
+      "  \"methodConfig\": [ {\n"
+      "    \"name\": [\n"
+      "      {}\n"
+      "    ],\n"
+      "    \"rbacPolicy\": [ {\n"
+      "      \"filter_name\": \"rbac\",\n"
+      "      \"rules\":{\n"
+      "        \"action\":1,\n"
+      "        \"audit_condition\":1,\n"
+      "        \"audit_loggers\":[ \n"
+      "          {\n"
+      "            \"unknown_logger\": {}\n"
+      "          }\n"
+      "        ]\n"
+      "      }\n"
+      "    } ]\n"
+      "  } ]\n"
+      "}";
+  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].rules.audit_loggers[0] "
+            "error:audit logger factory for unknown_logger does not exist]")
+      << service_config.status();
+}
+
 TEST_F(RbacServiceConfigParsingTest, BadAuditConditionAndLoggersTypes) {
   const char* test_json =
       "{\n"

test/core/security/grpc_audit_logging_test.cc

@@ -54,6 +54,7 @@ namespace {
 
 class TestAuditLogger : public AuditLogger {
  public:
+  absl::string_view name() const override { return "test_logger"; }
   void Log(const AuditContext&) override {}
 };
 

test/core/security/grpc_authorization_engine_test.cc

@@ -65,6 +65,7 @@ class TestAuditLogger : public AuditLogger {
       std::vector<std::unique_ptr<TestAuditContext>>* contexts)
       : contexts_(contexts) {}
 
+  absl::string_view name() const override { return kLoggerName; }
   void Log(const AuditContext& context) override {
     contexts_->push_back(std::make_unique<TestAuditContext>(context));
   }