From 0d70c632fbc75b25aed3dcb727f5d3fb1990d298 Mon Sep 17 00:00:00 2001 From: Luwei Ge Date: Thu, 18 May 2023 14:27:26 -0700 Subject: [PATCH] [Audit Logging] Second attempt: rbac service config parsing with audit logging (#33183) This is basically the same as #33145 except that the ctor `Rules()` cannot be default but have to explicitly set a default audit condition. --- include/grpc/grpc_audit_logging.h | 1 + src/core/BUILD | 1 + .../rbac/rbac_service_config_parser.cc | 106 ++++++- .../authorization/grpc_authorization_engine.h | 8 + .../security/authorization/stdout_logger.h | 1 + .../rbac/rbac_service_config_parser_test.cc | 300 +++++++++++++++++- test/core/security/grpc_audit_logging_test.cc | 1 + .../grpc_authorization_engine_test.cc | 1 + 8 files changed, 394 insertions(+), 25 deletions(-) diff --git a/include/grpc/grpc_audit_logging.h b/include/grpc/grpc_audit_logging.h index a63d324710c..0fda1043952 100644 --- a/include/grpc/grpc_audit_logging.h +++ b/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; }; diff --git a/src/core/BUILD b/src/core/BUILD index 7a79c76cf19..3000838876a 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -3697,6 +3697,7 @@ grpc_cc_library( "channel_fwd", "closure", "error", + "grpc_audit_logging", "grpc_authorization_base", "grpc_matchers", "grpc_rbac_engine", diff --git a/src/core/ext/filters/rbac/rbac_service_config_parser.cc b/src/core/ext/filters/rbac/rbac_service_config_parser.cc index 89786f1b7ac..91f0ed7e4f8 100644 --- a/src/core/ext/filters/rbac/rbac_service_config_parser.cc +++ b/src/core/ext/filters/rbac/rbac_service_config_parser.cc @@ -20,21 +20,29 @@ #include #include +#include #include #include "absl/status/status.h" #include "absl/status/statusor.h" +#include "absl/strings/str_cat.h" #include "absl/types/optional.h" +#include + #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/json/json_args.h" #include "src/core/lib/json/json_object_loader.h" #include "src/core/lib/matchers/matchers.h" +#include "src/core/lib/security/authorization/audit_logging.h" namespace grpc_core { namespace { +using experimental::AuditLoggerFactory; +using experimental::AuditLoggerRegistry; + // RbacConfig: one or more RbacPolicy structs struct RbacConfig { // RbacPolicy: optional Rules @@ -179,16 +187,35 @@ struct RbacConfig { static const JsonLoaderInterface* JsonLoader(const JsonArgs&); }; + // AuditLogger: the name of logger and its config in json + struct AuditLogger { + std::string name; + Json::Object config; + + AuditLogger() = default; + AuditLogger(const AuditLogger&) = delete; + AuditLogger& operator=(const AuditLogger&) = delete; + AuditLogger(AuditLogger&&) = default; + AuditLogger& operator=(AuditLogger&&) = default; + + static const JsonLoaderInterface* JsonLoader(const JsonArgs&); + void JsonPostLoad(const Json&, const JsonArgs&, + ValidationErrors* errors); + }; + int action; std::map policies; + // Defaults to kNone since its json field is optional. + Rbac::AuditCondition audit_condition = Rbac::AuditCondition::kNone; + std::vector> logger_configs; - Rules() = default; + Rules() {} Rules(const Rules&) = delete; Rules& operator=(const Rules&) = delete; Rules(Rules&&) = default; Rules& operator=(Rules&&) = default; - Rbac TakeAsRbac(); + Rbac TakeAsRbac(std::string name); static const JsonLoaderInterface* JsonLoader(const JsonArgs&); void JsonPostLoad(const Json&, const JsonArgs&, ValidationErrors* errors); }; @@ -716,24 +743,51 @@ const JsonLoaderInterface* RbacConfig::RbacPolicy::Rules::Policy::JsonLoader( return loader; } +// +// RbacConfig::RbacPolicy::Rules::AuditLogger +// + +const JsonLoaderInterface* +RbacConfig::RbacPolicy::Rules::AuditLogger::JsonLoader(const JsonArgs&) { + // All fields handled in JsonPostLoad(). + static const auto* loader = JsonObjectLoader().Finish(); + return loader; +} + +void RbacConfig::RbacPolicy::Rules::AuditLogger::JsonPostLoad( + const Json& json, const JsonArgs& args, ValidationErrors* errors) { + // Should have exactly one field as the logger name. + if (json.object().size() != 1) { + errors->AddError("audit logger should have exactly one field"); + return; + } + name = json.object().begin()->first; + auto config_or = + LoadJsonObjectField(json.object(), args, name, errors); + if (config_or.has_value()) { + config = std::move(*config_or); + } +} + // // RbacConfig::RbacPolicy::Rules // -Rbac RbacConfig::RbacPolicy::Rules::TakeAsRbac() { +Rbac RbacConfig::RbacPolicy::Rules::TakeAsRbac(std::string name) { Rbac rbac; - // TODO(lwge): This is to fix msan failure for now. Add proper conversion once - // audit logging support is added. - rbac.audit_condition = Rbac::AuditCondition::kNone; + rbac.name = std::move(name); rbac.action = static_cast(action); + rbac.audit_condition = audit_condition; for (auto& p : policies) { rbac.policies.emplace(p.first, p.second.TakeAsRbacPolicy()); } + rbac.logger_configs = std::move(logger_configs); return rbac; } const JsonLoaderInterface* RbacConfig::RbacPolicy::Rules::JsonLoader( const JsonArgs&) { + // Audit logger configs handled in post load. static const auto* loader = JsonObjectLoader() .Field("action", &Rules::action) .OptionalField("policies", &Rules::policies) @@ -741,7 +795,8 @@ const JsonLoaderInterface* RbacConfig::RbacPolicy::Rules::JsonLoader( return loader; } -void RbacConfig::RbacPolicy::Rules::JsonPostLoad(const Json&, const JsonArgs&, +void RbacConfig::RbacPolicy::Rules::JsonPostLoad(const Json& json, + const JsonArgs& args, ValidationErrors* errors) { // Validate action field. auto rbac_action = static_cast(action); @@ -750,6 +805,40 @@ void RbacConfig::RbacPolicy::Rules::JsonPostLoad(const Json&, const JsonArgs&, ValidationErrors::ScopedField field(errors, ".action"); errors->AddError("unknown action"); } + // Parse and validate audit_condition field. + auto condition = LoadJsonObjectField(json.object(), args, + "audit_condition", errors, false); + if (condition.has_value()) { + switch (*condition) { + case static_cast(Rbac::AuditCondition::kNone): + case static_cast(Rbac::AuditCondition::kOnAllow): + case static_cast(Rbac::AuditCondition::kOnDeny): + case static_cast(Rbac::AuditCondition::kOnDenyAndAllow): + audit_condition = static_cast(*condition); + break; + default: { + ValidationErrors::ScopedField field(errors, ".audit_condition"); + errors->AddError("unknown audit condition"); + } + } + } + // Parse and validate audit logger configs. + auto configs = LoadJsonObjectField>( + json.object(), args, "audit_loggers", errors, false); + if (configs.has_value()) { + for (size_t i = 0; i < configs->size(); ++i) { + auto& logger = (*configs)[i]; + auto config = AuditLoggerRegistry::ParseConfig( + logger.name, Json::FromObject(std::move(logger.config))); + if (!config.ok()) { + ValidationErrors::ScopedField field( + errors, absl::StrCat(".audit_loggers[", i, "]")); + errors->AddError(config.status().message()); + continue; + } + logger_configs.push_back(std::move(*config)); + } + } } // @@ -762,8 +851,7 @@ Rbac RbacConfig::RbacPolicy::TakeAsRbac() { // is equivalent to no enforcing. return Rbac(std::move(name), Rbac::Action::kDeny, {}); } - // TODO(lwge): This also needs to take the name. - return rules->TakeAsRbac(); + return rules->TakeAsRbac(std::move(name)); } const JsonLoaderInterface* RbacConfig::RbacPolicy::JsonLoader(const JsonArgs&) { diff --git a/src/core/lib/security/authorization/grpc_authorization_engine.h b/src/core/lib/security/authorization/grpc_authorization_engine.h index 085d9c71d1a..f769aa6c15b 100644 --- a/src/core/lib/security/authorization/grpc_authorization_engine.h +++ b/src/core/lib/security/authorization/grpc_authorization_engine.h @@ -56,6 +56,14 @@ class GrpcAuthorizationEngine : public AuthorizationEngine { // Required only for testing purpose. size_t num_policies() const { return policies_.size(); } + // Required only for testing purpose. + Rbac::AuditCondition audit_condition() const { return audit_condition_; } + + // Required only for testing purpose. + const std::vector>& audit_loggers() const { + return audit_loggers_; + } + // Evaluates incoming request against RBAC policy and makes a decision to // whether allow/deny this request. Decision Evaluate(const EvaluateArgs& args) const override; diff --git a/src/core/lib/security/authorization/stdout_logger.h b/src/core/lib/security/authorization/stdout_logger.h index 96f04628d7a..4f37b7311da 100644 --- a/src/core/lib/security/authorization/stdout_logger.h +++ b/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; }; diff --git a/test/core/ext/filters/rbac/rbac_service_config_parser_test.cc b/test/core/ext/filters/rbac/rbac_service_config_parser_test.cc index 5c9126496d8..78374d0ff58 100644 --- a/test/core/ext/filters/rbac/rbac_service_config_parser_test.cc +++ b/test/core/ext/filters/rbac/rbac_service_config_parser_test.cc @@ -14,14 +14,23 @@ #include "src/core/ext/filters/rbac/rbac_service_config_parser.h" +#include +#include +#include + #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 +#include #include #include "src/core/lib/gprpp/ref_counted_ptr.h" +#include "src/core/lib/json/json_writer.h" +#include "src/core/lib/security/authorization/audit_logging.h" #include "src/core/lib/service_config/service_config.h" #include "src/core/lib/service_config/service_config_impl.h" #include "test/core/util/test_config.h" @@ -30,8 +39,68 @@ namespace grpc_core { namespace testing { namespace { +using experimental::AuditContext; +using experimental::AuditLogger; +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 {} +}; + +class TestAuditLoggerFactory : public AuditLoggerFactory { + public: + class Config : public AuditLoggerFactory::Config { + public: + explicit Config(const Json& json) : config_(JsonDump(json)) {} + absl::string_view name() const override { return kLoggerName; } + std::string ToString() const override { return config_; } + + private: + std::string config_; + }; + + explicit TestAuditLoggerFactory( + std::map* configs) + : configs_(configs) {} + absl::string_view name() const override { return kLoggerName; } + absl::StatusOr> + ParseAuditLoggerConfig(const Json& json) override { + // Invalidate configs with "bad" field in it. + if (json.object().find("bad") != json.object().end()) { + return absl::InvalidArgumentError("bad logger config"); + } + return std::make_unique(json); + } + std::unique_ptr CreateAuditLogger( + std::unique_ptr config) override { + // Only insert entry to the map when logger is created. + configs_->emplace(name(), config->ToString()); + return std::make_unique(); + } + + private: + std::map* configs_; +}; + +class RbacServiceConfigParsingTest : public ::testing::Test { + protected: + void SetUp() override { + RegisterAuditLoggerFactory( + std::make_unique(&logger_configs_)); + } + void TearDown() override { AuditLoggerRegistry::TestOnlyResetRegistry(); } + std::map logger_configs_; +}; + // Filter name is required in RBAC policy. -TEST(RbacServiceConfigParsingTest, EmptyRbacPolicy) { +TEST_F(RbacServiceConfigParsingTest, EmptyRbacPolicy) { const char* test_json = "{\n" " \"methodConfig\": [ {\n" @@ -53,7 +122,7 @@ TEST(RbacServiceConfigParsingTest, EmptyRbacPolicy) { } // Test basic parsing of RBAC policy -TEST(RbacServiceConfigParsingTest, RbacPolicyWithoutRules) { +TEST_F(RbacServiceConfigParsingTest, RbacPolicyWithoutRules) { const char* test_json = "{\n" " \"methodConfig\": [ {\n" @@ -80,7 +149,7 @@ TEST(RbacServiceConfigParsingTest, RbacPolicyWithoutRules) { // Test that RBAC policies are not parsed if the channel arg // GRPC_ARG_PARSE_RBAC_METHOD_CONFIG is not present -TEST(RbacServiceConfigParsingTest, MissingChannelArg) { +TEST_F(RbacServiceConfigParsingTest, MissingChannelArg) { const char* test_json = "{\n" " \"methodConfig\": [ {\n" @@ -102,7 +171,7 @@ TEST(RbacServiceConfigParsingTest, MissingChannelArg) { } // Test an empty rbacPolicy array -TEST(RbacServiceConfigParsingTest, EmptyRbacPolicyArray) { +TEST_F(RbacServiceConfigParsingTest, EmptyRbacPolicyArray) { const char* test_json = "{\n" " \"methodConfig\": [ {\n" @@ -124,7 +193,7 @@ TEST(RbacServiceConfigParsingTest, EmptyRbacPolicyArray) { } // Test presence of multiple RBAC policies in the array -TEST(RbacServiceConfigParsingTest, MultipleRbacPolicies) { +TEST_F(RbacServiceConfigParsingTest, MultipleRbacPolicies) { const char* test_json = "{\n" " \"methodConfig\": [ {\n" @@ -155,7 +224,7 @@ TEST(RbacServiceConfigParsingTest, MultipleRbacPolicies) { } } -TEST(RbacServiceConfigParsingTest, BadRbacPolicyType) { +TEST_F(RbacServiceConfigParsingTest, BadRbacPolicyType) { const char* test_json = "{\n" " \"methodConfig\": [ {\n" @@ -174,7 +243,7 @@ TEST(RbacServiceConfigParsingTest, BadRbacPolicyType) { << service_config.status(); } -TEST(RbacServiceConfigParsingTest, BadRulesType) { +TEST_F(RbacServiceConfigParsingTest, BadRulesType) { const char* test_json = "{\n" " \"methodConfig\": [ {\n" @@ -193,7 +262,7 @@ TEST(RbacServiceConfigParsingTest, BadRulesType) { << service_config.status(); } -TEST(RbacServiceConfigParsingTest, BadActionAndPolicyType) { +TEST_F(RbacServiceConfigParsingTest, BadActionAndPolicyType) { const char* test_json = "{\n" " \"methodConfig\": [ {\n" @@ -221,7 +290,7 @@ TEST(RbacServiceConfigParsingTest, BadActionAndPolicyType) { << service_config.status(); } -TEST(RbacServiceConfigParsingTest, MissingPermissionAndPrincipals) { +TEST_F(RbacServiceConfigParsingTest, MissingPermissionAndPrincipals) { const char* test_json = "{\n" " \"methodConfig\": [ {\n" @@ -252,7 +321,7 @@ TEST(RbacServiceConfigParsingTest, MissingPermissionAndPrincipals) { << service_config.status(); } -TEST(RbacServiceConfigParsingTest, EmptyPrincipalAndPermission) { +TEST_F(RbacServiceConfigParsingTest, EmptyPrincipalAndPermission) { const char* test_json = "{\n" " \"methodConfig\": [ {\n" @@ -285,7 +354,7 @@ TEST(RbacServiceConfigParsingTest, EmptyPrincipalAndPermission) { << service_config.status(); } -TEST(RbacServiceConfigParsingTest, VariousPermissionsAndPrincipalsTypes) { +TEST_F(RbacServiceConfigParsingTest, VariousPermissionsAndPrincipalsTypes) { const char* test_json = "{\n" " \"methodConfig\": [ {\n" @@ -343,7 +412,7 @@ TEST(RbacServiceConfigParsingTest, VariousPermissionsAndPrincipalsTypes) { EXPECT_EQ(parsed_rbac_config->authorization_engine(0)->num_policies(), 1); } -TEST(RbacServiceConfigParsingTest, VariousPermissionsAndPrincipalsBadTypes) { +TEST_F(RbacServiceConfigParsingTest, VariousPermissionsAndPrincipalsBadTypes) { const char* test_json = "{\n" " \"methodConfig\": [ {\n" @@ -438,7 +507,7 @@ TEST(RbacServiceConfigParsingTest, VariousPermissionsAndPrincipalsBadTypes) { << service_config.status(); } -TEST(RbacServiceConfigParsingTest, HeaderMatcherVariousTypes) { +TEST_F(RbacServiceConfigParsingTest, HeaderMatcherVariousTypes) { const char* test_json = "{\n" " \"methodConfig\": [ {\n" @@ -483,7 +552,7 @@ TEST(RbacServiceConfigParsingTest, HeaderMatcherVariousTypes) { EXPECT_EQ(parsed_rbac_config->authorization_engine(0)->num_policies(), 1); } -TEST(RbacServiceConfigParsingTest, HeaderMatcherBadTypes) { +TEST_F(RbacServiceConfigParsingTest, HeaderMatcherBadTypes) { const char* test_json = "{\n" " \"methodConfig\": [ {\n" @@ -540,7 +609,7 @@ TEST(RbacServiceConfigParsingTest, HeaderMatcherBadTypes) { << service_config.status(); } -TEST(RbacServiceConfigParsingTest, StringMatcherVariousTypes) { +TEST_F(RbacServiceConfigParsingTest, StringMatcherVariousTypes) { const char* test_json = "{\n" " \"methodConfig\": [ {\n" @@ -582,7 +651,7 @@ TEST(RbacServiceConfigParsingTest, StringMatcherVariousTypes) { EXPECT_EQ(parsed_rbac_config->authorization_engine(0)->num_policies(), 1); } -TEST(RbacServiceConfigParsingTest, StringMatcherBadTypes) { +TEST_F(RbacServiceConfigParsingTest, StringMatcherBadTypes) { const char* test_json = "{\n" " \"methodConfig\": [ {\n" @@ -638,6 +707,205 @@ TEST(RbacServiceConfigParsingTest, StringMatcherBadTypes) { << service_config.status(); } +TEST_F(RbacServiceConfigParsingTest, AuditConditionOnDenyWithMultipleLoggers) { + 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" + " \"stdout_logger\": {}\n" + " },\n" + " {\n" + " \"test_logger\": {\"foo\": \"bar\"}\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); + ASSERT_TRUE(service_config.status().ok()); + const auto* vector_ptr = + (*service_config)->GetMethodParsedConfigVector(grpc_empty_slice()); + ASSERT_NE(vector_ptr, nullptr); + auto* parsed_rbac_config = static_cast( + ((*vector_ptr)[RbacServiceConfigParser::ParserIndex()]).get()); + ASSERT_NE(parsed_rbac_config, nullptr); + ASSERT_NE(parsed_rbac_config->authorization_engine(0), nullptr); + EXPECT_EQ(parsed_rbac_config->authorization_engine(0)->audit_condition(), + Rbac::AuditCondition::kOnDeny); + EXPECT_THAT(parsed_rbac_config->authorization_engine(0)->audit_loggers(), + ::testing::ElementsAre(::testing::Pointee(::testing::Property( + &AuditLogger::name, "stdout_logger")), + ::testing::Pointee(::testing::Property( + &AuditLogger::name, kLoggerName)))); + EXPECT_THAT(logger_configs_, ::testing::ElementsAre(::testing::Pair( + "test_logger", "{\"foo\":\"bar\"}"))); +} + +TEST_F(RbacServiceConfigParsingTest, BadAuditLoggerConfig) { + 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" + " \"test_logger\": {\"bad\": \"bar\"}\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:bad logger config]") + << 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" + " \"methodConfig\": [ {\n" + " \"name\": [\n" + " {}\n" + " ],\n" + " \"rbacPolicy\": [ {\n" + " \"filter_name\": \"rbac\",\n" + " \"rules\":{\n" + " \"action\":1,\n" + " \"audit_condition\":{},\n" + " \"audit_loggers\":{}\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_condition " + "error:is not a number; " + "field:methodConfig[0].rbacPolicy[0].rules.audit_loggers " + "error:is not an array]") + << service_config.status(); +} + +TEST_F(RbacServiceConfigParsingTest, BadAuditConditionEnum) { + const char* test_json = + "{\n" + " \"methodConfig\": [ {\n" + " \"name\": [\n" + " {}\n" + " ],\n" + " \"rbacPolicy\": [ {\n" + " \"filter_name\": \"rbac\",\n" + " \"rules\":{\n" + " \"action\":1,\n" + " \"audit_condition\":100\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_condition " + "error:unknown audit condition]") + << service_config.status(); +} + +TEST_F(RbacServiceConfigParsingTest, BadAuditLoggerObject) { + 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" + " \"stdout_logger\": {},\n" + " \"foo\": {}\n" + " },\n" + " {\n" + " \"stdout_logger\": 123\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 should have exactly one field; " + "field:methodConfig[0].rbacPolicy[0].rules.audit_loggers[1].stdout_" + "logger error:is not an object]") + << service_config.status(); +} + } // namespace } // namespace testing } // namespace grpc_core diff --git a/test/core/security/grpc_audit_logging_test.cc b/test/core/security/grpc_audit_logging_test.cc index fd8fc4b90fd..be89611b550 100644 --- a/test/core/security/grpc_audit_logging_test.cc +++ b/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 {} }; diff --git a/test/core/security/grpc_authorization_engine_test.cc b/test/core/security/grpc_authorization_engine_test.cc index 4cbf6b15883..5b0b9b710cf 100644 --- a/test/core/security/grpc_authorization_engine_test.cc +++ b/test/core/security/grpc_authorization_engine_test.cc @@ -65,6 +65,7 @@ class TestAuditLogger : public AuditLogger { std::vector>* contexts) : contexts_(contexts) {} + absl::string_view name() const override { return kLoggerName; } void Log(const AuditContext& context) override { contexts_->push_back(std::make_unique(context)); }