Revert "rbac: add delay_deny implementation in RBAC network filter (e…

…nvoyproxy#33875)"

This reverts commit bf65ad3.

Signed-off-by: Ryan Northey <[email protected]>

api/envoy/extensions/filters/network/rbac/v3/rbac.proto

@@ -4,8 +4,6 @@ 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";
 
@@ -28,7 +26,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: 9]
+// [#next-free-field: 8]
 message RBAC {
   option (udpa.annotations.versioning).previous_message_type =
       "envoy.config.filter.network.rbac.v2.RBAC";
@@ -89,10 +87,4 @@ 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;
 }

source/extensions/filters/network/rbac/rbac_filter.cc

@@ -63,14 +63,9 @@ 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()),
-      delay_deny_ms_(PROTOBUF_GET_MS_OR_DEFAULT(proto_config, delay_deny, 0)) {}
+      enforcement_type_(proto_config.enforcement_type()) {}
 
 Network::FilterStatus RoleBasedAccessControlFilter::onData(Buffer::Instance&, bool) {
-  if (is_delay_denied_) {
-    return Network::FilterStatus::StopIteration;
-  }
-
   ENVOY_LOG(
       debug,
       "checking connection: requestedServerName: {}, sourceIP: {}, directRemoteIP: {},"
@@ -123,44 +118,14 @@ Network::FilterStatus RoleBasedAccessControlFilter::onData(Buffer::Instance&, bo
   } else if (engine_result_ == Deny) {
     callbacks_->connection().streamInfo().setConnectionTerminationDetails(
         Filters::Common::RBAC::responseDetail(log_policy_id));
-
-    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();
-    }
+    callbacks_->connection().close(Network::ConnectionCloseType::NoFlush, "rbac_deny_close");
     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,6 +1,5 @@
 #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"
@@ -59,8 +58,6 @@ 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_;
@@ -69,7 +66,6 @@ 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 =
@@ -79,7 +75,6 @@ using RoleBasedAccessControlFilterConfigSharedPtr =
  * Implementation of a basic RBAC network filter.
  */
 class RoleBasedAccessControlFilter : public Network::ReadFilter,
-                                     public Network::ConnectionCallbacks,
                                      public Logger::Loggable<Logger::Id::rbac> {
 
 public:
@@ -92,14 +87,8 @@ 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:
@@ -109,10 +98,6 @@ 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,7 +10,6 @@
 #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"
 
@@ -30,8 +29,7 @@ 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,
-              int64_t delay_deny_duration_ms = 0) {
+              envoy::config::rbac::v3::RBAC::Action action = envoy::config::rbac::v3::RBAC::ALLOW) {
 
     envoy::extensions::filters::network::rbac::v3::RBAC config;
     config.set_stat_prefix("tcp.");
@@ -60,14 +58,11 @@ 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());
-    initFilter();
+
+    filter_ = std::make_unique<RoleBasedAccessControlFilter>(config_);
+    filter_->initializeReadFilterCallbacks(callbacks_);
   }
 
   void setupMatcher(bool with_matcher = true, bool continuous = false, std::string action = "ALLOW",
@@ -168,10 +163,12 @@ class RoleBasedAccessControlNetworkFilterTest : public testing::Test {
 
     config_ = std::make_shared<RoleBasedAccessControlFilterConfig>(
         config, *store_.rootScope(), context_, ProtobufMessage::getStrictValidationVisitor());
-    initFilter();
+
+    filter_ = std::make_unique<RoleBasedAccessControlFilter>(config_);
+    filter_->initializeReadFilterCallbacks(callbacks_);
   }
 
-  void initFilter() {
+  RoleBasedAccessControlNetworkFilterTest() {
     EXPECT_CALL(callbacks_, connection()).WillRepeatedly(ReturnRef(callbacks_.connection_));
     EXPECT_CALL(callbacks_.connection_, streamInfo()).WillRepeatedly(ReturnRef(stream_info_));
 
@@ -350,28 +347,6 @@ 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,7 +7,6 @@
 
 #include "test/integration/integration.h"
 #include "test/test_common/environment.h"
-#include "test/test_common/simulated_time_system.h"
 
 #include "fmt/printf.h"
 
@@ -23,7 +22,6 @@ std::string rbac_config;
 
 class RoleBasedAccessControlNetworkFilterIntegrationTest
     : public testing::TestWithParam<Network::Address::IpVersion>,
-      public Event::TestUsingSimulatedTime,
       public BaseIntegrationTest {
 public:
   RoleBasedAccessControlNetworkFilterIntegrationTest()
@@ -135,36 +133,6 @@ 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,7 +45,6 @@ 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