external authz: Interpret error grpc codes of the external authz serv…

…er as failure to fix failure_mode_allowed feature (envoyproxy#34951)

Fixes envoyproxy#34705

Signed-off-by: konstantin-baidin-y42 <[email protected]>

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
   // <config_http_filters_ext_authz_stats>`.

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
   // <config_http_filters_ext_authz_stats>`.

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
+    <envoy_v3_api_field_extensions.filters.http.ext_authz.v3.ExtAuthz.failure_mode_allow>`
+    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 <deprecated>`

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);

source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc

@@ -114,7 +114,11 @@ void GrpcClientImpl::onSuccess(std::unique_ptr<envoy::service::auth::v3::CheckRe
       const auto& ok_response = response->ok_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_ptr<envoy::service::auth::v3::CheckRe
       }
       authz_response->body = 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

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",
     ],

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<envoy::service::auth::v3::CheckResponse>();
-  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<ResponsePtr&>(
-                                      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<envoy::service::auth::v3::CheckResponse>();
+  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<ResponsePtr&>(
+                                      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<envoy::service::auth::v3::CheckResponse>();
+  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<ResponsePtr&>(
+                                      AuthzResponseNoAttributes(authz_response))));
+
+  client_->onSuccess(std::move(check_response), span_);
+}
+
 } // namespace ExtAuthz
 } // namespace Common
 } // namespace Filters