From 751217c2bf6cdf0f48f3c7016ae8dfcb49097739 Mon Sep 17 00:00:00 2001 From: "Antonio V. Leonti" <53806445+antoniovleonti@users.noreply.github.com> Date: Thu, 8 Aug 2024 09:23:44 -0400 Subject: [PATCH] implement per-route override for rbac stat prefixes (#35531) Currently, if you only have stat prefixes configured in per-route config and not the base config, the stat prefix will be empty. this PR addresses that. --------- Signed-off-by: antoniovleonti --- changelogs/current.yaml | 4 + .../filters/http/rbac/rbac_filter.cc | 39 +++++- .../filters/http/rbac/rbac_filter.h | 25 ++-- .../filters/http/rbac/rbac_filter_test.cc | 128 ++++++++++++++++++ 4 files changed, 174 insertions(+), 22 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 5019a19f60ab..050c0abf0d6c 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -58,6 +58,10 @@ bug_fixes: change: | Add runtime guard for timeout error code 504 Gateway Timeout that is returned to downstream. If runtime flag ``envoy.reloadable_features.ext_proc_timeout_error`` is set to false, old error code 500 Internal Server Error will be returned. +- area: rbac + change: | + RBAC will now allow stat prefixes configured in per-route config to override the base config's + stat prefix. removed_config_or_runtime: # *Normally occurs at the end of the* :ref:`deprecation period ` diff --git a/source/extensions/filters/http/rbac/rbac_filter.cc b/source/extensions/filters/http/rbac/rbac_filter.cc index cefa97f010c4..e55924cfc861 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.cc +++ b/source/extensions/filters/http/rbac/rbac_filter.cc @@ -1,3 +1,4 @@ +#include "rbac_filter.h" #include "source/extensions/filters/http/rbac/rbac_filter.h" #include "envoy/stats/scope.h" @@ -74,6 +75,29 @@ RoleBasedAccessControlFilterConfig::RoleBasedAccessControlFilterConfig( shadow_engine_(Filters::Common::RBAC::createShadowEngine( proto_config, context, validation_visitor, action_validation_visitor_)) {} +#define DEFINE_DYNAMIC_METADATA_STAT_KEY_GETTER(GETTER_NAME, PREFIX, ROUTE_LOCAL_PREFIX_OVERRIDE, \ + DYNAMIC_METADATA_KEY) \ + std::string RoleBasedAccessControlFilterConfig::GETTER_NAME( \ + const Http::StreamFilterCallbacks* callbacks) const { \ + const auto* route_local = Http::Utility::resolveMostSpecificPerFilterConfig< \ + RoleBasedAccessControlRouteSpecificFilterConfig>(callbacks); \ + std::string prefix = PREFIX; \ + if (route_local && !route_local->ROUTE_LOCAL_PREFIX_OVERRIDE().empty()) { \ + prefix = route_local->ROUTE_LOCAL_PREFIX_OVERRIDE(); \ + } \ + return prefix + \ + Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().DYNAMIC_METADATA_KEY; \ + } + +DEFINE_DYNAMIC_METADATA_STAT_KEY_GETTER(shadowEffectivePolicyIdField, shadow_rules_stat_prefix_, + shadowRulesStatPrefix, ShadowEffectivePolicyIdField) +DEFINE_DYNAMIC_METADATA_STAT_KEY_GETTER(shadowEngineResultField, shadow_rules_stat_prefix_, + shadowRulesStatPrefix, ShadowEngineResultField) +DEFINE_DYNAMIC_METADATA_STAT_KEY_GETTER(enforcedEffectivePolicyIdField, rules_stat_prefix_, + rulesStatPrefix, EnforcedEffectivePolicyIdField) +DEFINE_DYNAMIC_METADATA_STAT_KEY_GETTER(enforcedEngineResultField, rules_stat_prefix_, + rulesStatPrefix, EnforcedEngineResultField) + const Filters::Common::RBAC::RoleBasedAccessControlEngine* RoleBasedAccessControlFilterConfig::engine(const Http::StreamFilterCallbacks* callbacks, Filters::Common::RBAC::EnforcementMode mode) const { @@ -103,7 +127,9 @@ RoleBasedAccessControlRouteSpecificFilterConfig::RoleBasedAccessControlRouteSpec const envoy::extensions::filters::http::rbac::v3::RBACPerRoute& per_route_config, Server::Configuration::ServerFactoryContext& context, ProtobufMessage::ValidationVisitor& validation_visitor) - : per_rule_stats_(per_route_config.rbac().track_per_rule_stats()) { + : rules_stat_prefix_(per_route_config.rbac().rules_stat_prefix()), + shadow_rules_stat_prefix_(per_route_config.rbac().shadow_rules_stat_prefix()), + per_rule_stats_(per_route_config.rbac().track_per_rule_stats()) { // Moved from member initializer to ctor body to overcome clang false warning about memory // leak (clang-analyzer-cplusplus.NewDeleteLeaks,-warnings-as-errors). // Potentially https://lists.llvm.org/pipermail/llvm-bugs/2018-July/066769.html @@ -165,10 +191,11 @@ RoleBasedAccessControlFilter::decodeHeaders(Http::RequestHeaderMap& headers, boo } if (!effective_policy_id.empty()) { - *fields[config_->shadowEffectivePolicyIdField()].mutable_string_value() = effective_policy_id; + *fields[config_->shadowEffectivePolicyIdField(callbacks_)].mutable_string_value() = + effective_policy_id; } - *fields[config_->shadowEngineResultField()].mutable_string_value() = shadow_resp_code; + *fields[config_->shadowEngineResultField(callbacks_)].mutable_string_value() = shadow_resp_code; } const auto engine = config_->engine(callbacks_, Filters::Common::RBAC::EnforcementMode::Enforced); @@ -178,7 +205,7 @@ RoleBasedAccessControlFilter::decodeHeaders(Http::RequestHeaderMap& headers, boo callbacks_->streamInfo(), &effective_policy_id); const std::string log_policy_id = effective_policy_id.empty() ? "none" : effective_policy_id; if (!effective_policy_id.empty()) { - *fields[config_->enforcedEffectivePolicyIdField()].mutable_string_value() = + *fields[config_->enforcedEffectivePolicyIdField(callbacks_)].mutable_string_value() = effective_policy_id; } if (allowed) { @@ -188,7 +215,7 @@ RoleBasedAccessControlFilter::decodeHeaders(Http::RequestHeaderMap& headers, boo config_->stats().incPolicyAllowed(effective_policy_id); } - *fields[config_->enforcedEngineResultField()].mutable_string_value() = + *fields[config_->enforcedEngineResultField(callbacks_)].mutable_string_value() = Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().EngineResultAllowed; callbacks_->streamInfo().setDynamicMetadata("envoy.filters.http.rbac", metrics); @@ -203,7 +230,7 @@ RoleBasedAccessControlFilter::decodeHeaders(Http::RequestHeaderMap& headers, boo config_->stats().incPolicyDenied(effective_policy_id); } - *fields[config_->enforcedEngineResultField()].mutable_string_value() = + *fields[config_->enforcedEngineResultField(callbacks_)].mutable_string_value() = Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().EngineResultDenied; callbacks_->streamInfo().setDynamicMetadata("envoy.filters.http.rbac", metrics); diff --git a/source/extensions/filters/http/rbac/rbac_filter.h b/source/extensions/filters/http/rbac/rbac_filter.h index 9f021cb94e98..bb6748691fd2 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.h +++ b/source/extensions/filters/http/rbac/rbac_filter.h @@ -35,10 +35,15 @@ class RoleBasedAccessControlRouteSpecificFilterConfig : public Router::RouteSpec return mode == Filters::Common::RBAC::EnforcementMode::Enforced ? engine_.get() : shadow_engine_.get(); } + std::string rulesStatPrefix() const { return rules_stat_prefix_; } + + std::string shadowRulesStatPrefix() const { return shadow_rules_stat_prefix_; } bool perRuleStatsEnabled() const { return per_rule_stats_; } private: + const std::string rules_stat_prefix_; + const std::string shadow_rules_stat_prefix_; ActionValidationVisitor action_validation_visitor_; std::unique_ptr engine_; std::unique_ptr shadow_engine_; @@ -57,23 +62,11 @@ class RoleBasedAccessControlFilterConfig { ProtobufMessage::ValidationVisitor& validation_visitor); Filters::Common::RBAC::RoleBasedAccessControlFilterStats& stats() { return stats_; } - std::string shadowEffectivePolicyIdField() const { - return shadow_rules_stat_prefix_ + - Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().ShadowEffectivePolicyIdField; - } - std::string shadowEngineResultField() const { - return shadow_rules_stat_prefix_ + - Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().ShadowEngineResultField; - } + std::string shadowEffectivePolicyIdField(const Http::StreamFilterCallbacks* callbacks) const; + std::string shadowEngineResultField(const Http::StreamFilterCallbacks* callbacks) const; - std::string enforcedEffectivePolicyIdField() const { - return rules_stat_prefix_ + Filters::Common::RBAC::DynamicMetadataKeysSingleton::get() - .EnforcedEffectivePolicyIdField; - } - std::string enforcedEngineResultField() const { - return rules_stat_prefix_ + - Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().EnforcedEngineResultField; - } + std::string enforcedEffectivePolicyIdField(const Http::StreamFilterCallbacks* callbacks) const; + std::string enforcedEngineResultField(const Http::StreamFilterCallbacks* callbacks) const; const Filters::Common::RBAC::RoleBasedAccessControlEngine* engine(const Http::StreamFilterCallbacks* callbacks, diff --git a/test/extensions/filters/http/rbac/rbac_filter_test.cc b/test/extensions/filters/http/rbac/rbac_filter_test.cc index 2d772457a176..3877ab4046bc 100644 --- a/test/extensions/filters/http/rbac/rbac_filter_test.cc +++ b/test/extensions/filters/http/rbac/rbac_filter_test.cc @@ -576,6 +576,134 @@ TEST_F(RoleBasedAccessControlFilterTest, RouteLocalOverride) { checkAccessLogMetadata(LogResult::Undecided); } +TEST_F(RoleBasedAccessControlFilterTest, RouteLocalOverrideDynamicMetadataStats) { + setupPolicy(envoy::config::rbac::v3::RBAC::DENY, "original_rbac_rules_prefix_"); + + setDestinationPort(123); + setMetadata(); + + // Set up an allow route_config that overrides the deny policy. + envoy::extensions::filters::http::rbac::v3::RBACPerRoute route_config; + route_config.mutable_rbac()->mutable_rules()->set_action(envoy::config::rbac::v3::RBAC::ALLOW); + route_config.mutable_rbac()->mutable_shadow_rules()->set_action( + envoy::config::rbac::v3::RBAC::ALLOW); + route_config.mutable_rbac()->set_rules_stat_prefix("override_rules_stat_prefix_"); + route_config.mutable_rbac()->set_shadow_rules_stat_prefix("override_shadow_rules_stat_prefix_"); + + envoy::config::rbac::v3::Policy policy; + auto policy_rules = policy.add_permissions()->mutable_or_rules(); + policy_rules->add_rules()->set_destination_port(123); + policy.add_principals()->set_any(true); + + envoy::config::rbac::v3::Policy shadow_policy; + auto shadow_policy_rules = shadow_policy.add_permissions()->mutable_or_rules(); + shadow_policy_rules->add_rules()->set_destination_port(123); + shadow_policy.add_principals()->set_any(true); + + (*route_config.mutable_rbac()->mutable_rules()->mutable_policies())["foobar"] = policy; + (*route_config.mutable_rbac()->mutable_shadow_rules()->mutable_policies())["foobar"] = + shadow_policy; + NiceMock factory_context; + NiceMock engine{route_config.rbac().rules(), factory_context}; + NiceMock per_route_config_{route_config, + context_}; + + EXPECT_CALL(engine, handleAction(_, _, _, _)).WillRepeatedly(Return(true)); + EXPECT_CALL(per_route_config_, engine()).WillRepeatedly(ReturnRef(engine)); + + EXPECT_CALL(*callbacks_.route_, mostSpecificPerFilterConfig(_)) + .WillRepeatedly(Return(&per_route_config_)); + + // Filter iteration should continue since the route-specific policy is ALLOW + // and there are enforced and shadow rules. + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(headers_, true)); + ASSERT_TRUE(req_info_.dynamicMetadata().filter_metadata().contains("envoy.filters.http.rbac")); + auto filter_meta = req_info_.dynamicMetadata().filter_metadata().at("envoy.filters.http.rbac"); + + // We expect the route-specific rules and prefix to be used for the enforced + // engine and the shadow rules and prefix to be used for the shadow engine. + ASSERT_TRUE(filter_meta.fields().contains("override_rules_stat_prefix_enforced_engine_result")); + EXPECT_EQ( + "allowed", + filter_meta.fields().at("override_rules_stat_prefix_enforced_engine_result").string_value()); + ASSERT_TRUE( + filter_meta.fields().contains("override_rules_stat_prefix_enforced_effective_policy_id")); + EXPECT_EQ("foobar", filter_meta.fields() + .at("override_rules_stat_prefix_enforced_effective_policy_id") + .string_value()); + + ASSERT_TRUE( + filter_meta.fields().contains("override_shadow_rules_stat_prefix_shadow_engine_result")); + EXPECT_EQ("allowed", filter_meta.fields() + .at("override_shadow_rules_stat_prefix_shadow_engine_result") + .string_value()); + ASSERT_TRUE(filter_meta.fields().contains( + "override_shadow_rules_stat_prefix_shadow_effective_policy_id")); + EXPECT_EQ("foobar", filter_meta.fields() + .at("override_shadow_rules_stat_prefix_shadow_effective_policy_id") + .string_value()); +} + +TEST_F(RoleBasedAccessControlFilterTest, NoRouteLocalOverrideDynamicMetadataStatsEmpty) { + setupPolicy(envoy::config::rbac::v3::RBAC::DENY, "rules_stat_prefix_"); + + setDestinationPort(123); + setMetadata(); + + // Set up an allow route_config that overrides the deny policy. But do not set up stat prefixes. + envoy::extensions::filters::http::rbac::v3::RBACPerRoute route_config; + route_config.mutable_rbac()->mutable_rules()->set_action(envoy::config::rbac::v3::RBAC::ALLOW); + route_config.mutable_rbac()->mutable_shadow_rules()->set_action( + envoy::config::rbac::v3::RBAC::ALLOW); + + envoy::config::rbac::v3::Policy policy; + auto policy_rules = policy.add_permissions()->mutable_or_rules(); + policy_rules->add_rules()->set_destination_port(123); + policy.add_principals()->set_any(true); + + envoy::config::rbac::v3::Policy shadow_policy; + auto shadow_policy_rules = shadow_policy.add_permissions()->mutable_or_rules(); + shadow_policy_rules->add_rules()->set_destination_port(123); + shadow_policy.add_principals()->set_any(true); + + (*route_config.mutable_rbac()->mutable_rules()->mutable_policies())["foobar"] = policy; + (*route_config.mutable_rbac()->mutable_shadow_rules()->mutable_policies())["foobar"] = + shadow_policy; + NiceMock factory_context; + NiceMock engine{route_config.rbac().rules(), factory_context}; + NiceMock per_route_config_{route_config, + context_}; + + EXPECT_CALL(engine, handleAction(_, _, _, _)).WillRepeatedly(Return(true)); + EXPECT_CALL(per_route_config_, engine()).WillRepeatedly(ReturnRef(engine)); + + EXPECT_CALL(*callbacks_.route_, mostSpecificPerFilterConfig(_)) + .WillRepeatedly(Return(&per_route_config_)); + + // Filter iteration should continue since the route-specific policy is ALLOW and there are + // enforced and shadow rules. + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(headers_, true)); + ASSERT_TRUE(req_info_.dynamicMetadata().filter_metadata().contains("envoy.filters.http.rbac")); + auto filter_meta = req_info_.dynamicMetadata().filter_metadata().at("envoy.filters.http.rbac"); + + // We expect the base rules and prefix to be used since no route-specific stat was set up. + ASSERT_TRUE(filter_meta.fields().contains("rules_stat_prefix_enforced_engine_result")); + EXPECT_EQ("allowed", + filter_meta.fields().at("rules_stat_prefix_enforced_engine_result").string_value()); + ASSERT_TRUE(filter_meta.fields().contains("rules_stat_prefix_enforced_effective_policy_id")); + EXPECT_EQ( + "foobar", + filter_meta.fields().at("rules_stat_prefix_enforced_effective_policy_id").string_value()); + + ASSERT_TRUE(filter_meta.fields().contains("shadow_rules_prefix_shadow_engine_result")); + EXPECT_EQ("allowed", + filter_meta.fields().at("shadow_rules_prefix_shadow_engine_result").string_value()); + ASSERT_TRUE(filter_meta.fields().contains("shadow_rules_prefix_shadow_effective_policy_id")); + EXPECT_EQ( + "foobar", + filter_meta.fields().at("shadow_rules_prefix_shadow_effective_policy_id").string_value()); +} + TEST_F(RoleBasedAccessControlFilterTest, RouteLocalOverrideWithPerRuleStats) { setupPolicy(envoy::config::rbac::v3::RBAC::ALLOW);