diff --git a/api/envoy/service/auth/v3/external_auth.proto b/api/envoy/service/auth/v3/external_auth.proto index b627fcb314751..31adbc161b881 100644 --- a/api/envoy/service/auth/v3/external_auth.proto +++ b/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 ` 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 sends ``403 Forbidden`` HTTP status code by default. google.rpc.Status status = 1; // An message that contains HTTP response attributes. This message is diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index c8e227d34b58d..ee8f0f1319a01 100644 --- a/docs/root/version_history/current.rst +++ b/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 `. * dynamic forward proxy: fixing a validation bug where san and sni checks were not applied setting :ref:`http_protocol_options ` via :ref:`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 ` 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. diff --git a/generated_api_shadow/envoy/service/auth/v3/external_auth.proto b/generated_api_shadow/envoy/service/auth/v3/external_auth.proto index b627fcb314751..31adbc161b881 100644 --- a/generated_api_shadow/envoy/service/auth/v3/external_auth.proto +++ b/generated_api_shadow/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 ` 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 sends ``403 Forbidden`` HTTP status code by default. google.rpc.Status status = 1; // An message that contains HTTP response attributes. This message is 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 d462a5179572b..a1155ac0528dd 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 @@ -67,13 +67,17 @@ void GrpcClientImpl::onSuccess(std::unique_ptrstatus = 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(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(status_code); + } authz_response->body = response->denied_response().body(); - } else { - authz_response->status_code = Http::Code::Forbidden; } } 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 8d776a73b061f..ca45b70ca26b3 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 @@ -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(AuthzDeniedResponse(authz_response)))); + + client_->onSuccess(std::move(check_response), span_); +} + // Test the client when an unknown error occurs. TEST_F(ExtAuthzGrpcClientTest, UnknownError) { initialize();