From fd9e5bcc1c6efbbd60fc04dc8efbb5b053c0c66c Mon Sep 17 00:00:00 2001 From: konstantin-baidin-y42 Date: Thu, 27 Jun 2024 12:16:08 +0200 Subject: [PATCH 01/14] Interpret error grpc codes of the extranl authz server as failure to fix failure_mode_allowed feature Signed-off-by: konstantin-baidin-y42 --- .../filters/common/ext_authz/ext_authz_grpc_impl.cc | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc index dab5d014d109..512415cd4f3e 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc @@ -114,7 +114,9 @@ void GrpcClientImpl::onSuccess(std::unique_ptrok_response(); copyOkResponseMutations(authz_response, ok_response); } - } else { + } else if (response->status().code() == Grpc::Status::WellKnownGrpcStatus::PermissionDenied || + response->status().code() == Grpc::Status::WellKnownGrpcStatus::Unauthenticated) { + // The request was explicitly forbidden by the external authz server. span.setTag(TracingConstants::get().TraceStatus, TracingConstants::get().TraceUnauthz); authz_response->status = CheckStatus::Denied; @@ -129,6 +131,12 @@ void GrpcClientImpl::onSuccess(std::unique_ptrbody = response->denied_response().body(); } + } else { + // Unexpected response from external authz server is interpreted as failure + ENVOY_LOG(trace, "CheckRequest call failed with status: {}", + Grpc::Utility::grpcStatusToString(status)); + authz_response->status = CheckStatus::Error; + authz_response->status_code = Http::Code::Forbidden; } // OkHttpResponse.dynamic_metadata is deprecated. Until OkHttpResponse.dynamic_metadata is From 65911f41e1eb8b86527074c22e0f7419dad39b33 Mon Sep 17 00:00:00 2001 From: konstantin-baidin-y42 Date: Thu, 27 Jun 2024 14:37:48 +0200 Subject: [PATCH 02/14] Add test case Signed-off-by: konstantin-baidin-y42 --- .../ext_authz/ext_authz_grpc_impl_test.cc | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc index a7832ddeda2a..4aebe0bad80d 100644 --- a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc +++ b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc @@ -436,6 +436,43 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithAppendActions) { client_->onSuccess(std::make_unique(check_response), span_); } +// Test the client when an error code response is received. +TEST_F(ExtAuthzGrpcClientTest, AuthorizationOk) { + initialize(); + + auto check_response = std::make_unique(); + auto status = check_response->mutable_status(); + + ProtobufWkt::Struct expected_dynamic_metadata; + auto* metadata_fields = expected_dynamic_metadata.mutable_fields(); + (*metadata_fields)["foo"] = ValueUtil::stringValue("ok"); + (*metadata_fields)["bar"] = ValueUtil::numberValue(1); + + // The expected dynamic metadata is set to the outer check response, hence regardless the + // check_response's http_response value (either OkHttpResponse or DeniedHttpResponse) the dynamic + // metadata is set to be equal to the check response's dynamic metadata. + check_response->mutable_dynamic_metadata()->MergeFrom(expected_dynamic_metadata); + + status->set_code(Grpc::Status::WellKnownGrpcStatus::Unavailable); + + // This is the expected authz response. + auto authz_response = Response{}; + authz_response.status = CheckStatus::OK; + + authz_response.dynamic_metadata = expected_dynamic_metadata; + + envoy::service::auth::v3::CheckRequest request; + expectCallSend(request); + client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); + + Http::TestRequestHeaderMapImpl headers; + client_->onCreateInitialMetadata(headers); + + EXPECT_CALL(span_, setTag(Eq("ext_authz_status"), Eq("ext_authz_error"))); + EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo( + AuthzResponseNoAttributes(authz_response)))); + client_->onSuccess(std::move(check_response), span_); +} } // namespace ExtAuthz } // namespace Common From b1de993007deabd7908b06425fc059fa81058c39 Mon Sep 17 00:00:00 2001 From: konstantin-baidin-y42 Date: Thu, 27 Jun 2024 14:40:44 +0200 Subject: [PATCH 03/14] fix test Signed-off-by: konstantin-baidin-y42 --- .../filters/common/ext_authz/ext_authz_grpc_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc index 4aebe0bad80d..bd558e348d7f 100644 --- a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc +++ b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc @@ -457,7 +457,7 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationOk) { // This is the expected authz response. auto authz_response = Response{}; - authz_response.status = CheckStatus::OK; + authz_response.status = CheckStatus::Error; authz_response.dynamic_metadata = expected_dynamic_metadata; From 7addb7ab38abce83d800ec365eef1ac4da89e16b Mon Sep 17 00:00:00 2001 From: konstantin-baidin-y42 Date: Thu, 27 Jun 2024 14:43:15 +0200 Subject: [PATCH 04/14] fix using status field Signed-off-by: konstantin-baidin-y42 --- .../extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc index 512415cd4f3e..a7dad8107bd7 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc @@ -134,7 +134,7 @@ void GrpcClientImpl::onSuccess(std::unique_ptrstatus_code)); authz_response->status = CheckStatus::Error; authz_response->status_code = Http::Code::Forbidden; } From 85ece5259fdd3a08a79cd84c105fa54e2f28588e Mon Sep 17 00:00:00 2001 From: konstantin-baidin-y42 Date: Thu, 27 Jun 2024 14:51:31 +0200 Subject: [PATCH 05/14] fix Signed-off-by: konstantin-baidin-y42 --- .../extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc index a7dad8107bd7..8fd00d3df789 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc @@ -134,7 +134,7 @@ void GrpcClientImpl::onSuccess(std::unique_ptrstatus_code)); + Grpc::Utility::grpcStatusToString(response->status().code())); authz_response->status = CheckStatus::Error; authz_response->status_code = Http::Code::Forbidden; } From 036233754a4c940adc61d77e8ee176b89df3de26 Mon Sep 17 00:00:00 2001 From: konstantin-baidin-y42 Date: Thu, 27 Jun 2024 15:56:51 +0200 Subject: [PATCH 06/14] fix: rename test Signed-off-by: konstantin-baidin-y42 --- .../filters/common/ext_authz/ext_authz_grpc_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc index bd558e348d7f..6f13cdca0d5b 100644 --- a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc +++ b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc @@ -437,7 +437,7 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithAppendActions) { span_); } // Test the client when an error code response is received. -TEST_F(ExtAuthzGrpcClientTest, AuthorizationOk) { +TEST_F(ExtAuthzGrpcClientTest, AuthorizationReturnsErrorOnBadGrpcCode) { initialize(); auto check_response = std::make_unique(); From 923461c3863f485c8d75ad94f929c4c0cd61ca6a Mon Sep 17 00:00:00 2001 From: konstantin-baidin-y42 Date: Thu, 27 Jun 2024 16:58:40 +0200 Subject: [PATCH 07/14] fix tests Signed-off-by: konstantin-baidin-y42 --- .../filters/common/ext_authz/ext_authz_grpc_impl_test.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc index 6f13cdca0d5b..c6b7e345840b 100644 --- a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc +++ b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc @@ -187,7 +187,7 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationDeniedGrpcUnknownStatus) { auto status = check_response->mutable_status(); status->set_code(Grpc::Status::WellKnownGrpcStatus::Unknown); auto authz_response = Response{}; - authz_response.status = CheckStatus::Denied; + authz_response.status = CheckStatus::Error; envoy::service::auth::v3::CheckRequest request; expectCallSend(request); @@ -468,7 +468,6 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationReturnsErrorOnBadGrpcCode) { Http::TestRequestHeaderMapImpl headers; client_->onCreateInitialMetadata(headers); - EXPECT_CALL(span_, setTag(Eq("ext_authz_status"), Eq("ext_authz_error"))); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo( AuthzResponseNoAttributes(authz_response)))); client_->onSuccess(std::move(check_response), span_); From 4f639b44b10d4c71b70f5ec3a4e64795dfe8682a Mon Sep 17 00:00:00 2001 From: konstantin-baidin-y42 Date: Thu, 27 Jun 2024 17:21:35 +0200 Subject: [PATCH 08/14] remove expected `ext_authz_unauthorized` tag from the test as the error status instead of unauthorized is expected Signed-off-by: konstantin-baidin-y42 --- .../filters/common/ext_authz/ext_authz_grpc_impl_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc index c6b7e345840b..4b44db3619c1 100644 --- a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc +++ b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc @@ -196,7 +196,6 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationDeniedGrpcUnknownStatus) { Http::TestRequestHeaderMapImpl headers; client_->onCreateInitialMetadata(headers); EXPECT_EQ(nullptr, headers.RequestId()); - EXPECT_CALL(span_, setTag(Eq("ext_authz_status"), Eq("ext_authz_unauthorized"))); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo( AuthzResponseNoAttributes(authz_response)))); From 958cbbbd9725791e3221481279252a1bdfc5f915 Mon Sep 17 00:00:00 2001 From: konstantin-baidin-y42 Date: Wed, 3 Jul 2024 10:13:45 +0200 Subject: [PATCH 09/14] Add runtime guard Signed-off-by: konstantin-baidin-y42 --- source/common/runtime/runtime_features.cc | 2 ++ .../filters/common/ext_authz/ext_authz_grpc_impl.cc | 4 +++- .../filters/common/ext_authz/ext_authz_grpc_impl_test.cc | 7 ++++++- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index bf096e110539..9baa70f7ca4e 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -154,6 +154,8 @@ FALSE_RUNTIME_GUARD(envoy_restart_features_xds_failover_support); // A flag to set the maximum TLS version for google_grpc client to TLS1.2, when needed for // compliance restrictions. FALSE_RUNTIME_GUARD(envoy_reloadable_features_google_grpc_disable_tls_13); +// Fixes fail-open behaviour of failure_mode_allow for external authz grpc servers. +FALSE_RUNTIME_GUARD(envoy_reloadable_features_process_ext_authz_grpc_error_codes_as_errors); // Block of non-boolean flags. Use of int flags is deprecated. Do not add more. ABSL_FLAG(uint64_t, re2_max_program_size_error_level, 100, ""); // NOLINT diff --git a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc index 8fd00d3df789..22b72b803224 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc @@ -115,7 +115,9 @@ void GrpcClientImpl::onSuccess(std::unique_ptrstatus().code() == Grpc::Status::WellKnownGrpcStatus::PermissionDenied || - response->status().code() == Grpc::Status::WellKnownGrpcStatus::Unauthenticated) { + response->status().code() == Grpc::Status::WellKnownGrpcStatus::Unauthenticated || + !Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.process_ext_authz_grpc_error_codes_as_errors")) { // The request was explicitly forbidden by the external authz server. span.setTag(TracingConstants::get().TraceStatus, TracingConstants::get().TraceUnauthz); authz_response->status = CheckStatus::Denied; diff --git a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc index 4b44db3619c1..3cf81ff0dcf3 100644 --- a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc +++ b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc @@ -187,7 +187,7 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationDeniedGrpcUnknownStatus) { auto status = check_response->mutable_status(); status->set_code(Grpc::Status::WellKnownGrpcStatus::Unknown); auto authz_response = Response{}; - authz_response.status = CheckStatus::Error; + authz_response.status = CheckStatus::Denied; envoy::service::auth::v3::CheckRequest request; expectCallSend(request); @@ -196,6 +196,7 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationDeniedGrpcUnknownStatus) { Http::TestRequestHeaderMapImpl headers; client_->onCreateInitialMetadata(headers); EXPECT_EQ(nullptr, headers.RequestId()); + EXPECT_CALL(span_, setTag(Eq("ext_authz_status"), Eq("ext_authz_unauthorized"))); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo( AuthzResponseNoAttributes(authz_response)))); @@ -437,6 +438,10 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithAppendActions) { } // Test the client when an error code response is received. TEST_F(ExtAuthzGrpcClientTest, AuthorizationReturnsErrorOnBadGrpcCode) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.process_ext_authz_grpc_error_codes_as_errors", "true"}}); + initialize(); auto check_response = std::make_unique(); From 493754625e5a9b7851470c901327528986fe03ab Mon Sep 17 00:00:00 2001 From: konstantin-baidin-y42 Date: Wed, 3 Jul 2024 10:37:21 +0200 Subject: [PATCH 10/14] Add changelog entry Signed-off-by: konstantin-baidin-y42 --- changelogs/current.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index f627913785fe..63d97138c3fb 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -203,6 +203,10 @@ bug_fixes: change: | Fixed missing :ref:`additional addresses ` for :ref:`LbEndpoint ` in config dump. +- area: ext_authz + change: | + Fixed fail-open behaviour of the failure_mode_allow config option when a grpc external authz server is used. + The behaviour can be enabled by ``envoy_reloadable_features_process_ext_authz_grpc_error_codes_as_errors``. removed_config_or_runtime: # *Normally occurs at the end of the* :ref:`deprecation period ` From 218afa17ac22d5e8797a80ba98d97d382bb667fd Mon Sep 17 00:00:00 2001 From: konstantin-baidin-y42 Date: Wed, 3 Jul 2024 11:02:07 +0200 Subject: [PATCH 11/14] include "test/test_common/test_runtime.h" Signed-off-by: konstantin-baidin-y42 --- .../filters/common/ext_authz/ext_authz_grpc_impl_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc index 3cf81ff0dcf3..867c176c1c85 100644 --- a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc +++ b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc @@ -11,6 +11,7 @@ #include "test/mocks/grpc/mocks.h" #include "test/mocks/stream_info/mocks.h" #include "test/mocks/tracing/mocks.h" +#include "test/test_common/test_runtime.h" #include "gmock/gmock.h" #include "gtest/gtest.h" From d5c2b4a3ee60846a44a33e30b6dabf2bba7acd46 Mon Sep 17 00:00:00 2001 From: konstantin-baidin-y42 Date: Wed, 3 Jul 2024 15:26:36 +0200 Subject: [PATCH 12/14] Add test_runtime_lib into ext_authz_grpc_impl_test deps Signed-off-by: konstantin-baidin-y42 --- test/extensions/filters/common/ext_authz/BUILD | 1 + 1 file changed, 1 insertion(+) diff --git a/test/extensions/filters/common/ext_authz/BUILD b/test/extensions/filters/common/ext_authz/BUILD index 931e9f56d3b2..b4d802beb974 100644 --- a/test/extensions/filters/common/ext_authz/BUILD +++ b/test/extensions/filters/common/ext_authz/BUILD @@ -36,6 +36,7 @@ envoy_cc_test( "//source/extensions/filters/common/ext_authz:ext_authz_grpc_lib", "//test/extensions/filters/common/ext_authz:ext_authz_test_common", "//test/mocks/tracing:tracing_mocks", + "//test/test_common:test_runtime_lib", "@envoy_api//envoy/service/auth/v3:pkg_cc_proto", "@envoy_api//envoy/type/v3:pkg_cc_proto", ], From a050b77c2b4508f78741d79c1d34d2f8da506707 Mon Sep 17 00:00:00 2001 From: konstantin-baidin-y42 Date: Mon, 15 Jul 2024 16:21:43 +0200 Subject: [PATCH 13/14] Address feedback on PR Signed-off-by: konstantin-baidin-y42 --- .../filter/http/ext_authz/v2/ext_authz.proto | 5 +- .../filters/http/ext_authz/v3/ext_authz.proto | 5 +- changelogs/current.yaml | 4 +- source/common/runtime/runtime_features.cc | 4 +- .../ext_authz/ext_authz_grpc_impl_test.cc | 58 ++++++++++--------- 5 files changed, 41 insertions(+), 35 deletions(-) diff --git a/api/envoy/config/filter/http/ext_authz/v2/ext_authz.proto b/api/envoy/config/filter/http/ext_authz/v2/ext_authz.proto index c79dd4a24bc9..c55938fc3cbe 100644 --- a/api/envoy/config/filter/http/ext_authz/v2/ext_authz.proto +++ b/api/envoy/config/filter/http/ext_authz/v2/ext_authz.proto @@ -40,11 +40,12 @@ message ExtAuthz { // // 1. When set to true, the filter will *accept* client request even if the communication with // the authorization service has failed, or if the authorization service has returned a HTTP 5xx - // error. + // error. In case with GRPC authorization service, only PermissionDenied (7) and Unauthenticated (16) + // status codes will *reject* client requests. And other GRPC statuses will *accept* client requests. // // 2. When set to false, ext-authz will *reject* client requests and return a *Forbidden* // response if the communication with the authorization service has failed, or if the - // authorization service has returned a HTTP 5xx error. + // authorization service has returned a HTTP 5xx error or any non-Ok GRPC status. // // Note that errors can be *always* tracked in the :ref:`stats // `. diff --git a/api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto b/api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto index 4b4e79ef1f9f..7619d7ba5097 100644 --- a/api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto +++ b/api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto @@ -56,11 +56,12 @@ message ExtAuthz { // // 1. When set to true, the filter will ``accept`` client request even if the communication with // the authorization service has failed, or if the authorization service has returned a HTTP 5xx - // error. + // error. In case with GRPC authorization service, only PermissionDenied (7) and Unauthenticated (16) + // status codes will ``reject`` client requests. And other GRPC statuses will ``accept`` client requests. // // 2. When set to false, ext-authz will ``reject`` client requests and return a ``Forbidden`` // response if the communication with the authorization service has failed, or if the - // authorization service has returned a HTTP 5xx error. + // authorization service has returned a HTTP 5xx error or any non-Ok GRPC status. // // Note that errors can be ``always`` tracked in the :ref:`stats // `. diff --git a/changelogs/current.yaml b/changelogs/current.yaml index d9d4ec6c9544..2b3c51c6a952 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -248,7 +248,9 @@ bug_fixes: Bumped the version of datadog to resolve a crashing bug in earlier versions of the library. - area: ext_authz change: | - Fixed fail-open behaviour of the failure_mode_allow config option when a grpc external authz server is used. + Fixed fail-open behaviour of the failure_mode_allow config option + https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/ext_authz/v3/ext_authz.proto + when a grpc external authz server is used. The behaviour can be enabled by ``envoy_reloadable_features_process_ext_authz_grpc_error_codes_as_errors``. removed_config_or_runtime: diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 1192e04f35fc..90b43fb03647 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -73,6 +73,8 @@ RUNTIME_GUARD(envoy_reloadable_features_no_extension_lookup_by_name); RUNTIME_GUARD(envoy_reloadable_features_normalize_host_for_preresolve_dfp_dns); RUNTIME_GUARD(envoy_reloadable_features_oauth_use_url_encoding); RUNTIME_GUARD(envoy_reloadable_features_original_dst_rely_on_idle_timeout); +// Fixes fail-open behaviour of failure_mode_allow for external authz grpc servers. +RUNTIME_GUARD(envoy_reloadable_features_process_ext_authz_grpc_error_codes_as_errors); RUNTIME_GUARD(envoy_reloadable_features_proxy_status_mapping_more_core_response_flags); RUNTIME_GUARD(envoy_reloadable_features_quic_fix_filter_manager_uaf); RUNTIME_GUARD(envoy_reloadable_features_quic_receive_ecn); @@ -158,8 +160,6 @@ FALSE_RUNTIME_GUARD(envoy_restart_features_xds_failover_support); // A flag to set the maximum TLS version for google_grpc client to TLS1.2, when needed for // compliance restrictions. FALSE_RUNTIME_GUARD(envoy_reloadable_features_google_grpc_disable_tls_13); -// Fixes fail-open behaviour of failure_mode_allow for external authz grpc servers. -FALSE_RUNTIME_GUARD(envoy_reloadable_features_process_ext_authz_grpc_error_codes_as_errors); // Block of non-boolean flags. Use of int flags is deprecated. Do not add more. ABSL_FLAG(uint64_t, re2_max_program_size_error_level, 100, ""); // NOLINT diff --git a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc index 867c176c1c85..cec883ca7506 100644 --- a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc +++ b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc @@ -180,30 +180,6 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationDenied) { client_->onSuccess(std::move(check_response), span_); } -// Test the client when a gRPC status code unknown is received from the authorization server. -TEST_F(ExtAuthzGrpcClientTest, AuthorizationDeniedGrpcUnknownStatus) { - initialize(); - - auto check_response = std::make_unique(); - auto status = check_response->mutable_status(); - status->set_code(Grpc::Status::WellKnownGrpcStatus::Unknown); - auto authz_response = Response{}; - authz_response.status = CheckStatus::Denied; - - envoy::service::auth::v3::CheckRequest request; - expectCallSend(request); - client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); - - Http::TestRequestHeaderMapImpl headers; - client_->onCreateInitialMetadata(headers); - EXPECT_EQ(nullptr, headers.RequestId()); - EXPECT_CALL(span_, setTag(Eq("ext_authz_status"), Eq("ext_authz_unauthorized"))); - EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo( - AuthzResponseNoAttributes(authz_response)))); - - client_->onSuccess(std::move(check_response), span_); -} - // Test the client when a denied response with additional HTTP attributes is received. TEST_F(ExtAuthzGrpcClientTest, AuthorizationDeniedWithAllAttributes) { initialize(); @@ -437,12 +413,9 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithAppendActions) { client_->onSuccess(std::make_unique(check_response), span_); } + // Test the client when an error code response is received. TEST_F(ExtAuthzGrpcClientTest, AuthorizationReturnsErrorOnBadGrpcCode) { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues( - {{"envoy.reloadable_features.process_ext_authz_grpc_error_codes_as_errors", "true"}}); - initialize(); auto check_response = std::make_unique(); @@ -478,6 +451,35 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationReturnsErrorOnBadGrpcCode) { client_->onSuccess(std::move(check_response), span_); } +// Test the client when an error code response is received and +// process_ext_authz_grpc_error_codes_as_errors is false. +TEST_F(ExtAuthzGrpcClientTest, AuthorizationReturnsDeniedOnUnknownGrpcCodeWhenFeatureFlagDisabled) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.process_ext_authz_grpc_error_codes_as_errors", "false"}}); + + initialize(); + + auto check_response = std::make_unique(); + auto status = check_response->mutable_status(); + status->set_code(Grpc::Status::WellKnownGrpcStatus::Unknown); + auto authz_response = Response{}; + authz_response.status = CheckStatus::Denied; + + envoy::service::auth::v3::CheckRequest request; + expectCallSend(request); + client_->check(request_callbacks_, request, Tracing::NullSpan::instance(), stream_info_); + + Http::TestRequestHeaderMapImpl headers; + client_->onCreateInitialMetadata(headers); + EXPECT_EQ(nullptr, headers.RequestId()); + EXPECT_CALL(span_, setTag(Eq("ext_authz_status"), Eq("ext_authz_unauthorized"))); + EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo( + AuthzResponseNoAttributes(authz_response)))); + + client_->onSuccess(std::move(check_response), span_); +} + } // namespace ExtAuthz } // namespace Common } // namespace Filters From 0f220b42366ce5a9196cd40d2e72037415d21dc9 Mon Sep 17 00:00:00 2001 From: konstantin-baidin-y42 Date: Wed, 17 Jul 2024 09:40:09 +0200 Subject: [PATCH 14/14] Change link Signed-off-by: konstantin-baidin-y42 --- changelogs/current.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 442823409cf1..5f36a1ecca34 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -269,8 +269,8 @@ bug_fixes: Fixed a bug where the user data will reference a dangling pointer to the Lua state and cause a crash. - area: ext_authz change: | - Fixed fail-open behaviour of the failure_mode_allow config option - https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/ext_authz/v3/ext_authz.proto + Fixed fail-open behaviour of the :ref:`failure_mode_allow config option + ` when a grpc external authz server is used. The behaviour can be enabled by ``envoy_reloadable_features_process_ext_authz_grpc_error_codes_as_errors``.