ext_authz: Use 403 as default for denied response

api/envoy/service/auth/v3/external_auth.proto

@@ -46,9 +46,9 @@ message DeniedHttpResponse {
   option (udpa.annotations.versioning).previous_message_type =
       "envoy.service.auth.v2.DeniedHttpResponse";
 
-  // This field allows the authorization service to send a HTTP response status
-  // code to the downstream client other than 403 (Forbidden).
-  type.v3.HttpStatus status = 1 [(validate.rules).message = {required: true}];
+  // This field allows the authorization service to send an HTTP response status code to the
+  // downstream client. If not set, Envoy sends ``403 Forbidden`` HTTP status code by default.
+  type.v3.HttpStatus status = 1;
 
   // This field allows the authorization service to send HTTP response headers
   // to the downstream client. Note that the :ref:`append field in HeaderValueOption <envoy_v3_api_field_config.core.v3.HeaderValueOption.append>` defaults to
@@ -110,7 +110,9 @@ message CheckResponse {
   option (udpa.annotations.versioning).previous_message_type =
       "envoy.service.auth.v2.CheckResponse";
 
-  // Status `OK` allows the request. Any other status indicates the request should be denied.
+  // Status `OK` allows the request. Any other status indicates the request should be denied, and
+  // for HTTP filter, if not overridden by :ref:`denied HTTP response status <envoy_v3_api_field_service.auth.v3.DeniedHttpResponse.status>`
+  // Envoy sends ``403 Forbidden`` HTTP status code by default.
   google.rpc.Status status = 1;
 
   // An message that contains HTTP response attributes. This message is

docs/root/version_history/current.rst

@@ -78,7 +78,8 @@ Bug Fixes
 * cluster: finish cluster warming even if hosts are removed before health check initialization. This only affected clusters with :ref:`ignore_health_on_host_removal <envoy_v3_api_field_config.cluster.v3.Cluster.ignore_health_on_host_removal>`.
 * dynamic forward proxy: fixing a validation bug where san and sni checks were not applied setting :ref:`http_protocol_options <envoy_v3_api_msg_extensions.upstreams.http.v3.HttpProtocolOptions>` via :ref:`typed_extension_protocol_options <envoy_v3_api_field_config.cluster.v3.Cluster.typed_extension_protocol_options>`.
 * ext_authz: fix the ext_authz filter to correctly merge multiple same headers using the ',' as separator in the check request to the external authorization service.
-* ext_authz: the network ext_authz filter now correctly sets dynamic metdata returned by the authorization service for non-OK responses. This behavior now matches the http ext_authz filter.
+* ext_authz: fix the HTTP ext_authz filter to respond with ``403 Forbidden`` when a gRPC auth server sends a denied check response with an empty HTTP status code.
+* ext_authz: the network ext_authz filter now correctly sets dynamic metadata returned by the authorization service for non-OK responses. This behavior now matches the http ext_authz filter.
 * hcm: remove deprecation for :ref:`xff_num_trusted_hops <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.xff_num_trusted_hops>` and forbid mixing ip detection extensions with old related knobs.
 * http: limit use of deferred resets in the http2 codec to server-side connections. Use of deferred reset for client connections can result in incorrect behavior and performance problems.
 * listener: fixed an issue on Windows where connections are not handled by all worker threads.

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

@@ -67,13 +67,17 @@ void GrpcClientImpl::onSuccess(std::unique_ptr<envoy::service::auth::v3::CheckRe
   } else {
     span.setTag(TracingConstants::get().TraceStatus, TracingConstants::get().TraceUnauthz);
     authz_response->status = CheckStatus::Denied;
+
+    // The default HTTP status code for denied response is 403 Forbidden.
+    authz_response->status_code = Http::Code::Forbidden;
     if (response->has_denied_response()) {
       toAuthzResponseHeader(authz_response, response->denied_response().headers());
-      authz_response->status_code =
-          static_cast<Http::Code>(response->denied_response().status().code());
+
+      const uint32_t status_code = response->denied_response().status().code();
+      if (status_code > 0) {
+        authz_response->status_code = static_cast<Http::Code>(status_code);
+      }
       authz_response->body = response->denied_response().body();
-    } else {
-      authz_response->status_code = Http::Code::Forbidden;
     }
   }
 

test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc

@@ -205,6 +205,39 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationDeniedWithAllAttributes) {
   client_->onSuccess(std::move(check_response), span_);
 }
 
+// Test the client when a denied response with unknown HTTP status code (i.e. if
+// DeniedResponse.status is not set by the auth server implementation). The response sent to client
+// is set with the default HTTP status code for denied response (403 Forbidden).
+TEST_F(ExtAuthzGrpcClientTest, AuthorizationDeniedWithEmptyDeniedResponseStatus) {
+  initialize();
+
+  const std::string expected_body{"test"};
+  const auto expected_headers =
+      TestCommon::makeHeaderValueOption({{"foo", "bar", false}, {"foobar", "bar", true}});
+  const auto expected_downstream_headers = TestCommon::makeHeaderValueOption({});
+  auto check_response = TestCommon::makeCheckResponse(
+      Grpc::Status::WellKnownGrpcStatus::PermissionDenied, envoy::type::v3::Empty, expected_body,
+      expected_headers, expected_downstream_headers);
+  // When the check response gives unknown denied response HTTP status code, the filter sets the
+  // response HTTP status code with 403 Forbidden (default).
+  auto authz_response =
+      TestCommon::makeAuthzResponse(CheckStatus::Denied, Http::Code::Forbidden, expected_body,
+                                    expected_headers, expected_downstream_headers);
+
+  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&>(AuthzDeniedResponse(authz_response))));
+
+  client_->onSuccess(std::move(check_response), span_);
+}
+
 // Test the client when an unknown error occurs.
 TEST_F(ExtAuthzGrpcClientTest, UnknownError) {
   initialize();