ext_authz: support external authorization checks when a route contains a direct response entry

docs/root/version_history/current.rst

@@ -54,6 +54,7 @@ Bug Fixes
 
 * active http health checks: properly handles HTTP/2 GOAWAY frames from the upstream. Previously a GOAWAY frame due to a graceful listener drain could cause improper failed health checks due to streams being refused by the upstream on a connection that is going away. To revert to old GOAWAY handling behavior, set the runtime feature `envoy.reloadable_features.health_check.graceful_goaway_handling` to false.
 * buffer: tighten network connection read and write buffer high watermarks in preparation to more careful enforcement of read limits. Buffer high-watermark is now set to the exact configured value; previously it was set to value + 1.
+* ext_authz: fix a bug that skipped external authorization checks for routes that contained only a direct response.
 * fault injection: stop counting as active fault after delay elapsed. Previously fault injection filter continues to count the injected delay as an active fault even after it has elapsed. This produces incorrect output statistics and impacts the max number of consecutive faults allowed (e.g., for long-lived streams). This change decreases the active fault count when the delay fault is the only active and has gone finished.
 * grpc-web: fix local reply and non-proto-encoded gRPC response handling for small response bodies. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling` to false.
 * http: disallowing "host:" in request_headers_to_add for behavioral consistency with rejecting :authority header. This behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_host_like_authority` to false.

source/extensions/filters/http/ext_authz/ext_authz.cc

@@ -376,8 +376,16 @@ void Filter::continueDecoding() {
 }
 
 Filter::PerRouteFlags Filter::getPerRouteFlags(const Router::RouteConstSharedPtr& route) const {
-  if (route == nullptr || route->routeEntry() == nullptr) {
-    return PerRouteFlags{true /*skip_check_*/, false /*skip_request_body_buffering_*/};
+  if (route == nullptr) {
+    // If there's no route, skip authorization checks. This is an optimization to avoid work
+    // if we know the request won't be forwarded upstream anyway.
+    return PerRouteFlags::skipCheckFlags();
+  }
+
+  if (route->routeEntry() == nullptr && route->directResponseEntry() == nullptr) {
+    // If there's a route, but no route entry nor direct response entry, then skip authorization
+    // checks. This is another optimization.
+    return PerRouteFlags::skipCheckFlags();
   }
 
   const auto* specific_per_route_config =
@@ -388,7 +396,7 @@ Filter::PerRouteFlags Filter::getPerRouteFlags(const Router::RouteConstSharedPtr
                          specific_per_route_config->disableRequestBodyBuffering()};
   }
 
-  return PerRouteFlags{false /*skip_check_*/, false /*skip_request_body_buffering_*/};
+  return PerRouteFlags::defaultFlags();
 }
 
 } // namespace ExtAuthz

source/extensions/filters/http/ext_authz/ext_authz.h

@@ -268,6 +268,10 @@ class Filter : public Logger::Loggable<Logger::Id::filter>,
   struct PerRouteFlags {
     const bool skip_check_;
     const bool skip_request_body_buffering_;
+
+    static constexpr PerRouteFlags defaultFlags() { return PerRouteFlags{false, false}; }
+
+    static constexpr PerRouteFlags skipCheckFlags() { return PerRouteFlags{true, false}; }
   };
   PerRouteFlags getPerRouteFlags(const Router::RouteConstSharedPtr& route) const;
 

test/extensions/filters/http/ext_authz/ext_authz_test.cc

@@ -1589,9 +1589,42 @@ TEST_P(HttpFilterTestParam, DisabledOnRouteWithRequestBody) {
   EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers_));
 }
 
-// Test that the request continues when the filter_callbacks has no route.
-TEST_P(HttpFilterTestParam, NoRoute) {
+// Test that the request stops at decodeHeaders when filter_callbacks has a route entry.
+TEST_P(HttpFilterTestParam, StopIterationWithRouteEntry) {
+  NiceMock<Router::MockRouteEntry> route_entry;
+  prepareCheck();
+  EXPECT_CALL(*client_, check(_, _, _, _));
+  EXPECT_CALL(*filter_callbacks_.route_, routeEntry()).WillRepeatedly(Return(&route_entry));
+  EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
+            filter_->decodeHeaders(request_headers_, true));
+}
+
+// Test that the request stops at decodeHeaders when filter_callbacks has no route entry, but it
+// does have a direct response entry.
+TEST_P(HttpFilterTestParam, StopIterationWithDirectResponseEntry) {
+  NiceMock<Router::MockDirectResponseEntry> direct_response_entry;
+  prepareCheck();
+  EXPECT_CALL(*client_, check(_, _, _, _));
+  EXPECT_CALL(*filter_callbacks_.route_, routeEntry()).WillRepeatedly(Return(nullptr));
+  EXPECT_CALL(*filter_callbacks_.route_, directResponseEntry())
+      .WillOnce(Return(&direct_response_entry));
+  EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
+            filter_->decodeHeaders(request_headers_, true));
+}
+
+// Test that the request continues when filter_callbacks has no route
+TEST_P(HttpFilterTestParam, ContinueIterationWithNoRoute) {
+  EXPECT_CALL(filter_callbacks_, route()).WillRepeatedly(Return(nullptr));
+  EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false));
+  EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data_, false));
+  EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers_));
+}
+
+// Test that the request continues when filter_callbacks has no route entry / direct response entry.
+TEST_P(HttpFilterTestParam, ContinueIterationWithNoRouteEntryOrDirectResponseEntry) {
+  EXPECT_CALL(*client_, check(_, _, _, _)).Times(0);
   EXPECT_CALL(*filter_callbacks_.route_, routeEntry()).WillOnce(Return(nullptr));
+  EXPECT_CALL(*filter_callbacks_.route_, directResponseEntry()).WillOnce(Return(nullptr));
   EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false));
   EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data_, false));
   EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers_));