From 69a9c7c7468e8484f439e302b4f0cd4f3784545e Mon Sep 17 00:00:00 2001 From: Konstantin Baidin <128369285+konstantin-baidin-y42@users.noreply.github.com> Date: Tue, 23 Jul 2024 19:00:48 +0200 Subject: [PATCH] external authz: Interpret error grpc codes of the external authz server as failure to fix failure_mode_allowed feature (#34951) Fixes #34705 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 | 6 ++ source/common/runtime/runtime_features.cc | 2 + .../common/ext_authz/ext_authz_grpc_impl.cc | 12 ++- .../extensions/filters/common/ext_authz/BUILD | 1 + .../ext_authz/ext_authz_grpc_impl_test.cc | 91 ++++++++++++++----- 7 files changed, 93 insertions(+), 29 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 4503db855258..fdcb91be6d49 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -15,6 +15,12 @@ bug_fixes: - area: dns change: | The DNS filter no longer returns FORMERR if a message has an ID of 0. +- area: ext_authz + change: | + 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``. removed_config_or_runtime: # *Normally occurs at the end of the* :ref:`deprecation period ` diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 3fa0167c79db..4fe38e851ebe 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_no_timer_based_rate_limit_token_bucket); RUNTIME_GUARD(envoy_reloadable_features_normalize_host_for_preresolve_dfp_dns); 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_104); RUNTIME_GUARD(envoy_reloadable_features_proxy_status_mapping_more_core_response_flags); RUNTIME_GUARD(envoy_reloadable_features_quic_fix_filter_manager_uaf); 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..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 @@ -114,7 +114,11 @@ 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 || + !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; @@ -129,6 +133,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(response->status().code())); + authz_response->status = CheckStatus::Error; + authz_response->status_code = Http::Code::Forbidden; } // OkHttpResponse.dynamic_metadata is deprecated. Until OkHttpResponse.dynamic_metadata is 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", ], 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..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 @@ -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" @@ -179,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,6 +414,72 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithAppendActions) { span_); } +// Test the client when an error code response is received. +TEST_F(ExtAuthzGrpcClientTest, AuthorizationReturnsErrorOnBadGrpcCode) { + 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::Error; + + 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(request_callbacks_, onComplete_(WhenDynamicCastTo( + AuthzResponseNoAttributes(authz_response)))); + 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