rbac: add delay_deny implementation in RBAC network filter (envoyprox…

…y#33875)

If specified, the RBAC network filter will delay the specified duration before actually closing the connection.

This implements envoyproxy#33771.

---------

Signed-off-by: Yangmin Zhu <[email protected]>
Signed-off-by: asingh-g <[email protected]>

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 <config_http_filters_rbac>`.
-// [#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;
 }

changelogs/current.yaml

@@ -164,5 +164,9 @@ new_features:
   change: |
     Prefer using IPv6 address when addresses from both families are available.
     Can be reverted by setting ``envoy.reloadable_features.prefer_ipv6_dns_on_macos`` to false.
+- area: rbac
+  change: |
+    Added :ref:`delay_deny <envoy_v3_api_msg_extensions.filters.network.rbac.v3.RBAC>` to support deny connection after
+    the configured duration.
 
 deprecated:

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,14 +123,44 @@ 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", duration.count());
+      delay_timer_ = callbacks_->connection().dispatcher().createTimer(
+          [this]() -> void { closeConnection(); });
+      ASSERT(!is_delay_denied_);
+      is_delay_denied_ = true;
+      callbacks_->connection().readDisable(true);
+      delay_timer_->enableTimer(duration);
+    } else {
+      closeConnection();
+    }
     return Network::FilterStatus::StopIteration;
   }
 
   ENVOY_LOG(debug, "no engine, allowed by default");
   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;

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,8 @@ 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 +69,7 @@ class RoleBasedAccessControlFilterConfig {
   std::unique_ptr<const Filters::Common::RBAC::RoleBasedAccessControlEngine> engine_;
   std::unique_ptr<const Filters::Common::RBAC::RoleBasedAccessControlEngine> shadow_engine_;
   const envoy::extensions::filters::network::rbac::v3::RBAC::EnforcementType enforcement_type_;
+  std::chrono::milliseconds delay_deny_ms_;
 };
 
 using RoleBasedAccessControlFilterConfigSharedPtr =
@@ -75,6 +79,7 @@ using RoleBasedAccessControlFilterConfigSharedPtr =
  * Implementation of a basic RBAC network filter.
  */
 class RoleBasedAccessControlFilter : public Network::ReadFilter,
+                                     public Network::ConnectionCallbacks,
                                      public Logger::Loggable<Logger::Id::rbac> {
 
 public:
@@ -87,8 +92,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 +109,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

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,14 @@ 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<RoleBasedAccessControlFilterConfig>(
         config, *store_.rootScope(), context_, ProtobufMessage::getStrictValidationVisitor());
-
-    filter_ = std::make_unique<RoleBasedAccessControlFilter>(config_);
-    filter_->initializeReadFilterCallbacks(callbacks_);
+    initFilter();
   }
 
   void setupMatcher(bool with_matcher = true, bool continuous = false, std::string action = "ALLOW",
@@ -163,12 +168,10 @@ class RoleBasedAccessControlNetworkFilterTest : public testing::Test {
 
     config_ = std::make_shared<RoleBasedAccessControlFilterConfig>(
         config, *store_.rootScope(), context_, ProtobufMessage::getStrictValidationVisitor());
-
-    filter_ = std::make_unique<RoleBasedAccessControlFilter>(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 +350,28 @@ 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_, readDisable(true));
+  EXPECT_CALL(callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush, _));
+
+  Event::MockTimer* delay_timer =
+      new NiceMock<Event::MockTimer>(&callbacks_.connection_.dispatcher_);
+  EXPECT_CALL(*delay_timer, enableTimer(std::chrono::milliseconds(delay_deny_duration_ms), _));
+
+  // 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();
 

test/extensions/filters/network/rbac/integration_test.cc

@@ -7,6 +7,7 @@
 
 #include "test/integration/integration.h"
 #include "test/test_common/environment.h"
+#include "test/test_common/simulated_time_system.h"
 
 #include "fmt/printf.h"
 
@@ -22,6 +23,7 @@ std::string rbac_config;
 
 class RoleBasedAccessControlNetworkFilterIntegrationTest
     : public testing::TestWithParam<Network::Address::IpVersion>,
+      public Event::TestUsingSimulatedTime,
       public BaseIntegrationTest {
 public:
   RoleBasedAccessControlNetworkFilterIntegrationTest()
@@ -133,6 +135,36 @@ 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: 5s
+)EOF");
+  IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("listener_0"));
+  ASSERT_TRUE(tcp_client->write("hello", false, false));
+  ASSERT_TRUE(tcp_client->connected());
+
+  timeSystem().advanceTimeWait(std::chrono::seconds(3));
+  ASSERT_TRUE(tcp_client->connected());
+
+  timeSystem().advanceTimeWait(std::chrono::seconds(6));
+  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(

test/integration/integration_tcp_client.h

@@ -45,6 +45,7 @@ class IntegrationTcpClient {
   ABSL_MUST_USE_RESULT AssertionResult
   write(const std::string& data, bool end_stream = false, bool verify = true,
         std::chrono::milliseconds timeout = TestUtility::DefaultTimeout);
+
   const std::string& data() { return payload_reader_->data(); }
   bool connected() const { return !disconnected_; }
   // clear up to the `count` number of bytes of received data