From 240f07b15b20f4bcbef442b8cc6ab7d40bc51c85 Mon Sep 17 00:00:00 2001 From: Yangmin Zhu Date: Tue, 30 Apr 2024 10:55:12 -0700 Subject: [PATCH] rbac: add delay_deny implementation in RBAC network filter If specified, the RBAC network filter will delay the specified duration before actually closing the connection. This implements https://github.com/envoyproxy/envoy/issues/33771. Signed-off-by: Yangmin Zhu --- .../filters/network/rbac/v3/rbac.proto | 10 ++++- changelogs/current.yaml | 4 ++ .../filters/network/rbac/rbac_filter.cc | 37 +++++++++++++++++- .../filters/network/rbac/rbac_filter.h | 17 +++++++++ .../filters/network/rbac/filter_test.cc | 38 +++++++++++++++---- .../filters/network/rbac/integration_test.cc | 24 ++++++++++++ 6 files changed, 119 insertions(+), 11 deletions(-) diff --git a/api/envoy/extensions/filters/network/rbac/v3/rbac.proto b/api/envoy/extensions/filters/network/rbac/v3/rbac.proto index 823e18277d1f..16b092a50dd0 100644 --- a/api/envoy/extensions/filters/network/rbac/v3/rbac.proto +++ b/api/envoy/extensions/filters/network/rbac/v3/rbac.proto @@ -4,6 +4,8 @@ package envoy.extensions.filters.network.rbac.v3; import "envoy/config/rbac/v3/rbac.proto"; +import "google/protobuf/duration.proto"; + import "xds/annotations/v3/status.proto"; import "xds/type/matcher/v3/matcher.proto"; @@ -26,7 +28,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // // Header should not be used in rules/shadow_rules in RBAC network filter as // this information is only available in :ref:`RBAC http filter `. -// [#next-free-field: 8] +// [#next-free-field: 9] message RBAC { option (udpa.annotations.versioning).previous_message_type = "envoy.config.filter.network.rbac.v2.RBAC"; @@ -87,4 +89,10 @@ message RBAC { // every payload (e.g., Mongo, MySQL, Kafka) set the enforcement type to // CONTINUOUS to enforce RBAC policies on every message boundary. EnforcementType enforcement_type = 4; + + // Delay the specified duration before closing the connection when the policy evaluation + // result is DENY. If this is not present, the connection will be closed immediately. + // This is useful to provide a better protection for Envoy against clients that retries + // aggressively when the connection is rejected by the RBAC filter. + google.protobuf.Duration delay_deny = 8; } diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 9e3610fa6b0b..1d0056c62b24 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -62,5 +62,9 @@ new_features: change: | Added :ref:`Filter State Input ` for matching http input based on filter state objects. +- area: rbac + change: | + Added :ref:`delay_deny ` to support deny connection after + the configured duration. deprecated: diff --git a/source/extensions/filters/network/rbac/rbac_filter.cc b/source/extensions/filters/network/rbac/rbac_filter.cc index 7d07de0f6d18..ac0ef63aae64 100644 --- a/source/extensions/filters/network/rbac/rbac_filter.cc +++ b/source/extensions/filters/network/rbac/rbac_filter.cc @@ -63,9 +63,14 @@ RoleBasedAccessControlFilterConfig::RoleBasedAccessControlFilterConfig( action_validation_visitor_)), shadow_engine_(Filters::Common::RBAC::createShadowEngine( proto_config, context, validation_visitor, action_validation_visitor_)), - enforcement_type_(proto_config.enforcement_type()) {} + enforcement_type_(proto_config.enforcement_type()), + delay_deny_ms_(PROTOBUF_GET_MS_OR_DEFAULT(proto_config, delay_deny, 0)) {} Network::FilterStatus RoleBasedAccessControlFilter::onData(Buffer::Instance&, bool) { + if (is_delay_denied_) { + return Network::FilterStatus::StopIteration; + } + ENVOY_LOG( debug, "checking connection: requestedServerName: {}, sourceIP: {}, directRemoteIP: {}," @@ -118,7 +123,18 @@ Network::FilterStatus RoleBasedAccessControlFilter::onData(Buffer::Instance&, bo } else if (engine_result_ == Deny) { callbacks_->connection().streamInfo().setConnectionTerminationDetails( Filters::Common::RBAC::responseDetail(log_policy_id)); - callbacks_->connection().close(Network::ConnectionCloseType::NoFlush, "rbac_deny_close"); + + std::chrono::milliseconds duration = config_->delayDenyMs(); + if (duration > std::chrono::milliseconds(0)) { + ENVOY_LOG(debug, "connection will be delay denied in {}ms", std::to_string(duration.count())); + delay_timer_ = callbacks_->connection().dispatcher().createTimer([this]() -> void { + closeConnection(); + }); + is_delay_denied_ = true; + delay_timer_->enableTimer(duration); + } else { + closeConnection(); + } return Network::FilterStatus::StopIteration; } @@ -126,6 +142,23 @@ Network::FilterStatus RoleBasedAccessControlFilter::onData(Buffer::Instance&, bo return Network::FilterStatus::Continue; } +void RoleBasedAccessControlFilter::closeConnection() { + callbacks_->connection().close(Network::ConnectionCloseType::NoFlush, "rbac_deny_close"); +} + +void RoleBasedAccessControlFilter::resetTimerState() { + if (delay_timer_) { + delay_timer_->disableTimer(); + delay_timer_.reset(); + } +} + +void RoleBasedAccessControlFilter::onEvent(Network::ConnectionEvent event) { + if (event == Network::ConnectionEvent::RemoteClose || event == Network::ConnectionEvent::LocalClose) { + resetTimerState(); + } +} + void RoleBasedAccessControlFilter::setDynamicMetadata(std::string shadow_engine_result, std::string shadow_policy_id) { ProtobufWkt::Struct metrics; diff --git a/source/extensions/filters/network/rbac/rbac_filter.h b/source/extensions/filters/network/rbac/rbac_filter.h index a534f7c88010..805aae89fd3a 100644 --- a/source/extensions/filters/network/rbac/rbac_filter.h +++ b/source/extensions/filters/network/rbac/rbac_filter.h @@ -1,5 +1,6 @@ #pragma once +#include "envoy/event/timer.h" #include "envoy/extensions/filters/network/rbac/v3/rbac.pb.h" #include "envoy/network/connection.h" #include "envoy/network/filter.h" @@ -58,6 +59,10 @@ class RoleBasedAccessControlFilterConfig { return enforcement_type_; } + std::chrono::milliseconds delayDenyMs() const { + return delay_deny_ms_; + } + private: Filters::Common::RBAC::RoleBasedAccessControlFilterStats stats_; const std::string shadow_rules_stat_prefix_; @@ -66,6 +71,7 @@ class RoleBasedAccessControlFilterConfig { std::unique_ptr engine_; std::unique_ptr shadow_engine_; const envoy::extensions::filters::network::rbac::v3::RBAC::EnforcementType enforcement_type_; + std::chrono::milliseconds delay_deny_ms_; }; using RoleBasedAccessControlFilterConfigSharedPtr = @@ -75,6 +81,7 @@ using RoleBasedAccessControlFilterConfigSharedPtr = * Implementation of a basic RBAC network filter. */ class RoleBasedAccessControlFilter : public Network::ReadFilter, + public Network::ConnectionCallbacks, public Logger::Loggable { public: @@ -87,8 +94,14 @@ class RoleBasedAccessControlFilter : public Network::ReadFilter, Network::FilterStatus onNewConnection() override { return Network::FilterStatus::Continue; }; void initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callbacks) override { callbacks_ = &callbacks; + callbacks_->connection().addConnectionCallbacks(*this); } + // Network::ConnectionCallbacks + void onEvent(Network::ConnectionEvent event) override; + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} + void setDynamicMetadata(std::string shadow_engine_result, std::string shadow_policy_id); private: @@ -98,6 +111,10 @@ class RoleBasedAccessControlFilter : public Network::ReadFilter, EngineResult shadow_engine_result_{Unknown}; Result checkEngine(Filters::Common::RBAC::EnforcementMode mode); + void closeConnection(); + void resetTimerState(); + Event::TimerPtr delay_timer_{nullptr}; + bool is_delay_denied_{false}; }; } // namespace RBACFilter diff --git a/test/extensions/filters/network/rbac/filter_test.cc b/test/extensions/filters/network/rbac/filter_test.cc index 78b4d05d32ea..f18f2fc1b81d 100644 --- a/test/extensions/filters/network/rbac/filter_test.cc +++ b/test/extensions/filters/network/rbac/filter_test.cc @@ -10,6 +10,7 @@ #include "source/extensions/filters/network/rbac/rbac_filter.h" #include "source/extensions/filters/network/well_known_names.h" +#include "test/mocks/event/mocks.h" #include "test/mocks/network/mocks.h" #include "test/mocks/server/factory_context.h" @@ -29,7 +30,8 @@ class RoleBasedAccessControlNetworkFilterTest : public testing::Test { public: void setupPolicy(bool with_policy = true, bool continuous = false, - envoy::config::rbac::v3::RBAC::Action action = envoy::config::rbac::v3::RBAC::ALLOW) { + envoy::config::rbac::v3::RBAC::Action action = envoy::config::rbac::v3::RBAC::ALLOW, + int64_t delay_deny_duration_ms = 0) { envoy::extensions::filters::network::rbac::v3::RBAC config; config.set_stat_prefix("tcp."); @@ -58,11 +60,13 @@ class RoleBasedAccessControlNetworkFilterTest : public testing::Test { config.set_enforcement_type(envoy::extensions::filters::network::rbac::v3::RBAC::CONTINUOUS); } + if (delay_deny_duration_ms > 0) { + (*config.mutable_delay_deny()) = ProtobufUtil::TimeUtil::MillisecondsToDuration(delay_deny_duration_ms); + } + config_ = std::make_shared( config, *store_.rootScope(), context_, ProtobufMessage::getStrictValidationVisitor()); - - filter_ = std::make_unique(config_); - filter_->initializeReadFilterCallbacks(callbacks_); + initFilter(); } void setupMatcher(bool with_matcher = true, bool continuous = false, std::string action = "ALLOW", @@ -163,12 +167,10 @@ class RoleBasedAccessControlNetworkFilterTest : public testing::Test { config_ = std::make_shared( config, *store_.rootScope(), context_, ProtobufMessage::getStrictValidationVisitor()); - - filter_ = std::make_unique(config_); - filter_->initializeReadFilterCallbacks(callbacks_); + initFilter(); } - RoleBasedAccessControlNetworkFilterTest() { + void initFilter() { EXPECT_CALL(callbacks_, connection()).WillRepeatedly(ReturnRef(callbacks_.connection_)); EXPECT_CALL(callbacks_.connection_, streamInfo()).WillRepeatedly(ReturnRef(stream_info_)); @@ -347,6 +349,26 @@ TEST_F(RoleBasedAccessControlNetworkFilterTest, Denied) { filter_meta.fields().at("shadow_rules_prefix_shadow_engine_result").string_value()); } +TEST_F(RoleBasedAccessControlNetworkFilterTest, DelayDenied) { + int64_t delay_deny_duration_ms = 500; + setupPolicy(true, false, envoy::config::rbac::v3::RBAC::ALLOW, delay_deny_duration_ms); + setDestinationPort(789); + + // Only call close() once since the connection is delay denied. + EXPECT_CALL(callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush, _)).Times(1); + + Event::MockTimer* delay_timer = new NiceMock(&callbacks_.connection_.dispatcher_); + EXPECT_CALL(*delay_timer, enableTimer(std::chrono::milliseconds(delay_deny_duration_ms), _)).Times(1); + + // Call onData() twice, should only increase stats once. + EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data_, false)); + EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onData(data_, false)); + EXPECT_EQ(0U, config_->stats().allowed_.value()); + EXPECT_EQ(1U, config_->stats().denied_.value()); + + delay_timer->invokeCallback(); +} + TEST_F(RoleBasedAccessControlNetworkFilterTest, MatcherAllowedWithOneTimeEnforcement) { setupMatcher(); diff --git a/test/extensions/filters/network/rbac/integration_test.cc b/test/extensions/filters/network/rbac/integration_test.cc index f8018eaff5db..ae8562b96c7c 100644 --- a/test/extensions/filters/network/rbac/integration_test.cc +++ b/test/extensions/filters/network/rbac/integration_test.cc @@ -133,6 +133,30 @@ name: rbac EXPECT_EQ(0U, test_server_->counter("tcp.rbac.shadow_denied")->value()); } +TEST_P(RoleBasedAccessControlNetworkFilterIntegrationTest, DelayDenied) { + initializeFilter(R"EOF( +name: rbac +typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.network.rbac.v3.RBAC + stat_prefix: tcp. + rules: + policies: + "deny_all": + permissions: + - any: true + principals: + - not_id: + any: true + delay_deny: 0.5s +)EOF"); + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("listener_0")); + ASSERT_TRUE(tcp_client->write("hello", false, false)); + tcp_client->waitForDisconnect(); + + EXPECT_EQ(0U, test_server_->counter("tcp.rbac.allowed")->value()); + EXPECT_EQ(1U, test_server_->counter("tcp.rbac.denied")->value()); +} + TEST_P(RoleBasedAccessControlNetworkFilterIntegrationTest, DeniedWithDenyAction) { useListenerAccessLog("%CONNECTION_TERMINATION_DETAILS%"); initializeFilter(R"EOF(