From e5277ab8b4986e4a57498566d9b53436b9867f31 Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Fri, 9 Aug 2024 14:34:12 -0700 Subject: [PATCH] Revert "implement per-route override for rbac stat prefixes (#35531)" This reverts commit 751217c2bf6cdf0f48f3c7016ae8dfcb49097739. --- 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, 22 insertions(+), 174 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 8f12340b7aa0..86f5204385de 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -61,10 +61,6 @@ 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 e55924cfc861..cefa97f010c4 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.cc +++ b/source/extensions/filters/http/rbac/rbac_filter.cc @@ -1,4 +1,3 @@ -#include "rbac_filter.h" #include "source/extensions/filters/http/rbac/rbac_filter.h" #include "envoy/stats/scope.h" @@ -75,29 +74,6 @@ 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 { @@ -127,9 +103,7 @@ RoleBasedAccessControlRouteSpecificFilterConfig::RoleBasedAccessControlRouteSpec const envoy::extensions::filters::http::rbac::v3::RBACPerRoute& per_route_config, Server::Configuration::ServerFactoryContext& context, ProtobufMessage::ValidationVisitor& validation_visitor) - : 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()) { + : 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 @@ -191,11 +165,10 @@ RoleBasedAccessControlFilter::decodeHeaders(Http::RequestHeaderMap& headers, boo } if (!effective_policy_id.empty()) { - *fields[config_->shadowEffectivePolicyIdField(callbacks_)].mutable_string_value() = - effective_policy_id; + *fields[config_->shadowEffectivePolicyIdField()].mutable_string_value() = effective_policy_id; } - *fields[config_->shadowEngineResultField(callbacks_)].mutable_string_value() = shadow_resp_code; + *fields[config_->shadowEngineResultField()].mutable_string_value() = shadow_resp_code; } const auto engine = config_->engine(callbacks_, Filters::Common::RBAC::EnforcementMode::Enforced); @@ -205,7 +178,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(callbacks_)].mutable_string_value() = + *fields[config_->enforcedEffectivePolicyIdField()].mutable_string_value() = effective_policy_id; } if (allowed) { @@ -215,7 +188,7 @@ RoleBasedAccessControlFilter::decodeHeaders(Http::RequestHeaderMap& headers, boo config_->stats().incPolicyAllowed(effective_policy_id); } - *fields[config_->enforcedEngineResultField(callbacks_)].mutable_string_value() = + *fields[config_->enforcedEngineResultField()].mutable_string_value() = Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().EngineResultAllowed; callbacks_->streamInfo().setDynamicMetadata("envoy.filters.http.rbac", metrics); @@ -230,7 +203,7 @@ RoleBasedAccessControlFilter::decodeHeaders(Http::RequestHeaderMap& headers, boo config_->stats().incPolicyDenied(effective_policy_id); } - *fields[config_->enforcedEngineResultField(callbacks_)].mutable_string_value() = + *fields[config_->enforcedEngineResultField()].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 bb6748691fd2..9f021cb94e98 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.h +++ b/source/extensions/filters/http/rbac/rbac_filter.h @@ -35,15 +35,10 @@ 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_; @@ -62,11 +57,23 @@ class RoleBasedAccessControlFilterConfig { ProtobufMessage::ValidationVisitor& validation_visitor); Filters::Common::RBAC::RoleBasedAccessControlFilterStats& stats() { return stats_; } - std::string shadowEffectivePolicyIdField(const Http::StreamFilterCallbacks* callbacks) const; - std::string shadowEngineResultField(const Http::StreamFilterCallbacks* callbacks) const; + 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 enforcedEffectivePolicyIdField(const Http::StreamFilterCallbacks* callbacks) const; - std::string enforcedEngineResultField(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; + } 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 3877ab4046bc..2d772457a176 100644 --- a/test/extensions/filters/http/rbac/rbac_filter_test.cc +++ b/test/extensions/filters/http/rbac/rbac_filter_test.cc @@ -576,134 +576,6 @@ 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);