diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 929a7038eac53..81ef7c6c3eec2 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -42,6 +42,7 @@ Bug Fixes * access log: fix ``%UPSTREAM_CLUSTER%`` when used in http upstream access logs. Previously, it was always logging as an unset value. * aws request signer: fix the AWS Request Signer extension to correctly normalize the path and query string to be signed according to AWS' guidelines, so that the hash on the server side matches. See `AWS SigV4 documentaion `_. * cluster: delete pools when they're idle to fix unbounded memory use when using PROXY protocol upstream with tcp_proxy. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.conn_pool_delete_when_idle`` runtime guard to false. +* ext_authz: fixed skipping authentication when returning either a direct response or a redirect. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.http_ext_authz_do_not_skip_direct_response_and_redirect`` runtime guard to false. * hcm: remove deprecation for :ref:`xff_num_trusted_hops ` and forbid mixing ip detection extensions with old related knobs. * listener: fixed an issue on Windows where connections are not handled by all worker threads. * xray: fix the AWS X-Ray tracer bug where span's error, fault and throttle information was not reported properly as per the `AWS X-Ray documentation `_. Before this fix, server error was reported under 'annotations' section of the segment data. diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index cf2840e6ac9fc..3810cec7d29e4 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -1012,15 +1012,9 @@ void RouteEntryImplBase::validateClusters( void RouteEntryImplBase::traversePerFilterConfig( const std::string& filter_name, std::function cb) const { - const Router::RouteEntry* route_entry = routeEntry(); - - // TODO(soulxu): This has similar bug with https://github.com/envoyproxy/envoy/issues/17377 - // it should be fixed. - if (route_entry != nullptr) { - auto maybe_vhost_config = vhost_.perFilterConfig(filter_name); - if (maybe_vhost_config != nullptr) { - cb(*maybe_vhost_config); - } + auto maybe_vhost_config = vhost_.perFilterConfig(filter_name); + if (maybe_vhost_config != nullptr) { + cb(*maybe_vhost_config); } auto maybe_route_config = per_filter_configs_.get(filter_name); diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index d08b6a1a9aec6..aa8fea21b68a7 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -72,6 +72,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.health_check.immediate_failure_exclude_from_cluster", "envoy.reloadable_features.http2_consume_stream_refused_errors", "envoy.reloadable_features.http2_skip_encoding_empty_trailers", + "envoy.reloadable_features.http_ext_authz_do_not_skip_direct_response_and_redirect", "envoy.reloadable_features.http_transport_failure_reason_in_body", "envoy.reloadable_features.improved_stream_limit_handling", "envoy.reloadable_features.internal_redirects_with_body", diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index 3af51ba77efd0..29a6ca8be8437 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/source/extensions/filters/http/ext_authz/ext_authz.cc @@ -374,7 +374,10 @@ void Filter::continueDecoding() { } Filter::PerRouteFlags Filter::getPerRouteFlags(const Router::RouteConstSharedPtr& route) const { - if (route == nullptr || route->routeEntry() == nullptr) { + if (route == nullptr || + (!Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.http_ext_authz_do_not_skip_direct_response_and_redirect") && + route->routeEntry() == nullptr)) { return PerRouteFlags{true /*skip_check_*/, false /*skip_request_body_buffering_*/}; } diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 9728a64a208d7..995accac44b4c 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -7988,6 +7988,32 @@ TEST_F(PerFilterConfigsTest, RouteLocalTypedConfig) { checkEach(yaml, 123, expected_traveled_config); } +TEST_F(PerFilterConfigsTest, RouteLocalTypedConfigWithDirectResponse) { + const std::string yaml = R"EOF( +virtual_hosts: + - name: bar + domains: ["*"] + routes: + - match: { prefix: "/" } + direct_response: + status: 200 + typed_per_filter_config: + test.filter: + "@type": type.googleapis.com/google.protobuf.Timestamp + value: + seconds: 123 + typed_per_filter_config: + test.filter: + "@type": type.googleapis.com/google.protobuf.Struct + value: + seconds: 456 +)EOF"; + + factory_context_.cluster_manager_.initializeClusters({"baz"}, {}); + absl::InlinedVector expected_traveled_config({456, 123}); + checkEach(yaml, 123, expected_traveled_config); +} + TEST_F(PerFilterConfigsTest, DEPRECATED_FEATURE_TEST(WeightedClusterConfig)) { TestDeprecatedV2Api _deprecated_v2_api; const std::string yaml = R"EOF( diff --git a/test/extensions/filters/http/ext_authz/BUILD b/test/extensions/filters/http/ext_authz/BUILD index 3d52bfc4150f5..6076c5d7b3cfe 100644 --- a/test/extensions/filters/http/ext_authz/BUILD +++ b/test/extensions/filters/http/ext_authz/BUILD @@ -32,6 +32,7 @@ envoy_extension_cc_test( "//test/mocks/runtime:runtime_mocks", "//test/mocks/tracing:tracing_mocks", "//test/mocks/upstream:cluster_manager_mocks", + "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/extensions/filters/http/ext_authz/v3:pkg_cc_proto", diff --git a/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc index 64348bf33a470..32e4e54dbbe6d 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc @@ -700,6 +700,47 @@ TEST_P(ExtAuthzHttpIntegrationTest, DefaultCaseSensitiveStringMatcher) { ASSERT_TRUE(header_entry.empty()); } +TEST_P(ExtAuthzHttpIntegrationTest, DirectReponse) { + config_helper_.addConfigModifier( + [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) { + auto* virtual_hosts = hcm.mutable_route_config()->mutable_virtual_hosts(0); + virtual_hosts->mutable_routes(0)->clear_route(); + envoy::config::route::v3::Route* route = virtual_hosts->mutable_routes(0); + route->mutable_direct_response()->set_status(204); + }); + + initializeConfig(); + HttpIntegrationTest::initialize(); + initiateClientConnection(); + waitForExtAuthzRequest(); + + ASSERT_TRUE(response_->waitForEndStream()); + EXPECT_TRUE(response_->complete()); + EXPECT_EQ("204", response_->headers().Status()->value().getStringView()); +} + +TEST_P(ExtAuthzHttpIntegrationTest, RedirectResponse) { + config_helper_.addConfigModifier( + [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) { + auto* virtual_hosts = hcm.mutable_route_config()->mutable_virtual_hosts(0); + virtual_hosts->mutable_routes(0)->clear_route(); + envoy::config::route::v3::Route* route = virtual_hosts->mutable_routes(0); + route->mutable_redirect()->set_path_redirect("/redirect"); + }); + + initializeConfig(); + HttpIntegrationTest::initialize(); + initiateClientConnection(); + waitForExtAuthzRequest(); + + ASSERT_TRUE(response_->waitForEndStream()); + EXPECT_TRUE(response_->complete()); + EXPECT_EQ("301", response_->headers().Status()->value().getStringView()); + EXPECT_EQ("http://host/redirect", response_->headers().getLocationValue()); +} + class ExtAuthzLocalReplyIntegrationTest : public HttpIntegrationTest, public TestWithParam { public: 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 d207941a77cf0..980e239311572 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_test.cc @@ -26,6 +26,7 @@ #include "test/mocks/tracing/mocks.h" #include "test/mocks/upstream/cluster_manager.h" #include "test/test_common/printers.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" @@ -1603,14 +1604,30 @@ 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 that authentication will do when the filter_callbacks has no route.(both +// direct response and redirect have no route) TEST_P(HttpFilterTestParam, NoRoute) { - EXPECT_CALL(*filter_callbacks_.route_, routeEntry()).WillOnce(Return(nullptr)); - EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); + EXPECT_CALL(*filter_callbacks_.route_, routeEntry()).WillRepeatedly(Return(nullptr)); + prepareCheck(); + EXPECT_CALL(*client_, check(_, _, _, _)); + EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark, + 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 authentication will be skipped when the filter_callbacks has no route(both +// direct response and redirect have no route) when the runtime flag +// `envoy.reloadable_features.http_ext_authz_do_not_skip_direct_response_and_redirect` is false. +TEST_P(HttpFilterTestParam, NoRouteWithSkipAuth) { + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.http_ext_authz_do_not_skip_direct_response_and_redirect", + "false"}}); + EXPECT_CALL(*filter_callbacks_.route_, routeEntry()).WillOnce(Return(nullptr)); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); +} + // Test that the request is stopped till there is an OK response back after which it continues on. TEST_P(HttpFilterTestParam, OkResponse) { InSequence s;