diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index c1b4fa807cbe5..fddeb94afa83b 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -27,6 +27,7 @@ Minor Behavior Changes logging, :ref:`auto_host_rewrite `, etc. Setting the hostname manually allows overriding the internal hostname used for such features while still allowing the original DNS resolution name to be used. +* ext_authz: routes that contain a redirect or a direct response action are now properly subject to external authorization checks by default. To restore the original behavior and skip authorization checks, disable ext_authz on the :ref:`per-route config `. * grpc_json_transcoder: filter now adheres to encoder and decoder buffer limits. Requests and responses that require buffering over the limits will be directly rejected. The behavior can be reverted by disabling runtime feature `envoy.reloadable_features.grpc_json_transcoder_adhere_to_buffer_limits`. diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index 97ca3d7782062..b0d269e6a07bf 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/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 diff --git a/source/extensions/filters/http/ext_authz/ext_authz.h b/source/extensions/filters/http/ext_authz/ext_authz.h index 9b1aca32f97af..8d47f9daafafe 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.h +++ b/source/extensions/filters/http/ext_authz/ext_authz.h @@ -268,6 +268,10 @@ class Filter : public Logger::Loggable, 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; diff --git a/test/extensions/filters/http/ext_authz/ext_authz_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_test.cc index e0f2ec53e031f..975a0039d3ed9 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_test.cc @@ -1599,9 +1599,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 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 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_));