ext_authz: Allow headers propagation with allow_debugging_failures

Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>

api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto

@@ -26,7 +26,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
 // External Authorization :ref:`configuration overview <config_http_filters_ext_authz>`.
 // [#extension: envoy.filters.http.ext_authz]
 
-// [#next-free-field: 18]
+// [#next-free-field: 19]
 message ExtAuthz {
   option (udpa.annotations.versioning).previous_message_type =
       "envoy.config.filter.http.ext_authz.v2.ExtAuthz";
@@ -63,6 +63,16 @@ message ExtAuthz {
   // <config_http_filters_ext_authz_stats>`.
   bool failure_mode_allow = 2;
 
+  //  This changes the filter's behaviour on errors:
+  //
+  //  1. When set to true, the filter would retain the status code received from the upstream and
+  //  would also propagate the response body and headers back to the client (if available) for
+  //  further debugging.
+  //
+  //  2. When set to false, the filter would return a 403 or `status_on_error` (if set) and would
+  //  not propagate any response body or headers back to the client. This is the default behavior.
+  bool allow_debugging_failures = 18;
+
   // Enables filter to buffer the client request body and send it within the authorization request.
   // A ``x-envoy-auth-partial-body: false|true`` metadata header will be added to the authorization
   // request message indicating if the body data is partial.

changelogs/current.yaml

@@ -297,6 +297,10 @@ new_features:
     added support to allowlist headers included in the check request to gRPC authorization server (previously only available for HTTP authorization server).
     Pre-existing field :ref:`allowed_headers <envoy_v3_api_field_extensions.filters.http.ext_authz.v3.AuthorizationRequest.allowed_headers>` is deprecated in favour
     of the new field :ref:`allowed_headers <envoy_v3_api_field_extensions.filters.http.ext_authz.v3.ExtAuthz.allowed_headers>`.
+- area: ext_authz
+  change: |
+    added a new field :ref:`allow_debugging_failures <envoy_v3_api_field_extensions.filters.http.ext_authz.v3.ExtAuthz.allow_debugging_failures>` to control whether or
+    not to propagate the upstream headers back in case of 5XX failures. By default this flag is set to false in order to maintain the existing behavior.
 - area: attributes
   change: |
     added :ref:`attributes <arch_overview_attributes>` for looking up xDS configuration information.

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

@@ -114,6 +114,7 @@ ClientConfig::ClientConfig(const envoy::extensions::filters::http::ext_authz::v3
           config.http_service().authorization_response().allowed_upstream_headers())),
       upstream_header_to_append_matchers_(toUpstreamMatchers(
           config.http_service().authorization_response().allowed_upstream_headers_to_append())),
+      failure_debugging_allowed_(config.allow_debugging_failures()),
       cluster_name_(config.http_service().server_uri().cluster()), timeout_(timeout),
       path_prefix_(path_prefix),
       tracing_name_(fmt::format("async {} egress", config.http_service().server_uri().cluster())),
@@ -275,7 +276,7 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) {
   // Set an error status if the call to the authorization server returns any of the 5xx HTTP error
   // codes. A Forbidden response is sent to the client if the filter has not been configured with
   // failure_mode_allow.
-  if (Http::CodeUtility::is5xx(status_code)) {
+  if (Http::CodeUtility::is5xx(status_code) && !config_->failureDebuggingAllowed()) {
     return std::make_unique<Response>(errorResponse());
   }
 
@@ -326,6 +327,29 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) {
     return std::move(ok.response_);
   }
 
+  // Checks whether the status is 5XX and creates an Error authorization response with headers
+  // status, body, and received from upstream.
+  if (Http::CodeUtility::is5xx(status_code)) {
+    SuccessResponse error{message->headers(),
+                          config_->clientHeaderMatchers(),
+                          config_->upstreamHeaderToAppendMatchers(),
+                          config_->clientHeaderOnSuccessMatchers(),
+                          config_->dynamicMetadataMatchers(),
+                          Response{CheckStatus::Error,
+                                   Http::HeaderVector{},
+                                   Http::HeaderVector{},
+                                   Http::HeaderVector{},
+                                   Http::HeaderVector{},
+                                   Http::HeaderVector{},
+                                   {{}},
+                                   Http::Utility::QueryParamsVector{},
+                                   {},
+                                   message->bodyAsString(),
+                                   static_cast<Http::Code>(status_code),
+                                   ProtobufWkt::Struct{}}};
+    return std::move(error.response_);
+  }
+
   // Create a Denied authorization response.
   SuccessResponse denied{message->headers(),
                          config_->clientHeaderMatchers(),

source/extensions/filters/common/ext_authz/ext_authz_http_impl.h

@@ -76,6 +76,13 @@ class ClientConfig {
     return upstream_header_to_append_matchers_;
   }
 
+  /**
+   * Checks whether or not to send back an errorResponse() after seeing 5xx. If this is set by
+   * the user then we should preserve the headers, body, and status received from upstream and
+   * propagate it back to the client for further debugging.
+   */
+  bool failureDebuggingAllowed() const { return failure_debugging_allowed_; }
+
   /**
    * Returns the name used for tracing.
    */
@@ -101,6 +108,7 @@ class ClientConfig {
   const MatcherSharedPtr to_dynamic_metadata_matchers_;
   const MatcherSharedPtr upstream_header_matchers_;
   const MatcherSharedPtr upstream_header_to_append_matchers_;
+  const bool failure_debugging_allowed_;
   const std::string cluster_name_;
   const std::chrono::milliseconds timeout_;
   const std::string path_prefix_;

source/extensions/filters/http/ext_authz/ext_authz.cc

@@ -418,9 +418,52 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) {
           *decoder_callbacks_, enumToInt(config_->statusOnError()));
       decoder_callbacks_->streamInfo().setResponseFlag(
           StreamInfo::ResponseFlag::UnauthorizedExternalService);
-      decoder_callbacks_->sendLocalReply(
-          config_->statusOnError(), EMPTY_STRING, nullptr, absl::nullopt,
-          Filters::Common::ExtAuthz::ResponseCodeDetails::get().AuthzError);
+      if (config_->failureDebuggingAllowed()) {
+        ENVOY_STREAM_LOG(trace,
+                         "ext_authz filter has failure debugging allowed. Propagating response "
+                         "body and headers back to the client.",
+                         *decoder_callbacks_);
+        stats_.failure_debugging_allowed_.inc();
+        if (cluster_) {
+          config_->incCounter(cluster_->statsScope(),
+                              config_->ext_authz_failure_debugging_allowed_);
+          Http::CodeStats::ResponseStatInfo info{config_->scope(),
+                                                 cluster_->statsScope(),
+                                                 empty_stat_name,
+                                                 enumToInt(response->status_code),
+                                                 true,
+                                                 empty_stat_name,
+                                                 empty_stat_name,
+                                                 empty_stat_name,
+                                                 empty_stat_name,
+                                                 empty_stat_name,
+                                                 false};
+          config_->httpContext().codeStats().chargeResponseStat(info, false);
+        }
+        decoder_callbacks_->sendLocalReply(
+            response->status_code, response->body,
+            [&headers = response->headers_to_set,
+             &callbacks = *decoder_callbacks_](Http::HeaderMap& response_headers) -> void {
+              ENVOY_STREAM_LOG(
+                  trace, "ext_authz filter added header(s) to the local response:", callbacks);
+              // Firstly, remove all headers requested by the ext_authz filter, to ensure that they
+              // will override existing headers.
+              for (const auto& header : headers) {
+                response_headers.remove(header.first);
+              }
+              // Then set all the requested headers, allowing the same header to be set multiple
+              // times, e.g. `Set-Cookie`.
+              for (const auto& header : headers) {
+                ENVOY_STREAM_LOG(trace, " '{}':'{}'", callbacks, header.first.get(), header.second);
+                response_headers.addCopy(header.first, header.second);
+              }
+            },
+            absl::nullopt, Filters::Common::ExtAuthz::ResponseCodeDetails::get().AuthzError);
+      } else {
+        decoder_callbacks_->sendLocalReply(
+            config_->statusOnError(), EMPTY_STRING, nullptr, absl::nullopt,
+            Filters::Common::ExtAuthz::ResponseCodeDetails::get().AuthzError);
+      }
     }
     break;
   }

source/extensions/filters/http/ext_authz/ext_authz.h

@@ -39,7 +39,8 @@ namespace ExtAuthz {
   COUNTER(denied)                                                                                  \
   COUNTER(error)                                                                                   \
   COUNTER(disabled)                                                                                \
-  COUNTER(failure_mode_allowed)
+  COUNTER(failure_mode_allowed)                                                                    \
+  COUNTER(failure_debugging_allowed)
 
 /**
  * Wrapper struct for ext_authz filter stats. @see stats_macros.h
@@ -60,6 +61,7 @@ class FilterConfig {
                const std::string& stats_prefix, envoy::config::bootstrap::v3::Bootstrap& bootstrap)
       : allow_partial_message_(config.with_request_body().allow_partial_message()),
         failure_mode_allow_(config.failure_mode_allow()),
+        failure_debugging_allowed_(config.allow_debugging_failures()),
         clear_route_cache_(config.clear_route_cache()),
         max_request_bytes_(config.with_request_body().max_request_bytes()),
         pack_as_bytes_(config.with_request_body().pack_as_bytes()),
@@ -88,7 +90,9 @@ class FilterConfig {
         ext_authz_denied_(pool_.add(createPoolStatName(config.stat_prefix(), "denied"))),
         ext_authz_error_(pool_.add(createPoolStatName(config.stat_prefix(), "error"))),
         ext_authz_failure_mode_allowed_(
-            pool_.add(createPoolStatName(config.stat_prefix(), "failure_mode_allowed"))) {
+            pool_.add(createPoolStatName(config.stat_prefix(), "failure_mode_allowed"))),
+        ext_authz_failure_debugging_allowed_(
+            pool_.add(createPoolStatName(config.stat_prefix(), "failure_debugging_allowed"))) {
     auto labels_key_it =
         bootstrap.node().metadata().fields().find(config.bootstrap_metadata_labels_key());
     if (labels_key_it != bootstrap.node().metadata().fields().end()) {
@@ -127,6 +131,8 @@ class FilterConfig {
 
   bool failureModeAllow() const { return failure_mode_allow_; }
 
+  bool failureDebuggingAllowed() const { return failure_debugging_allowed_; }
+
   bool clearRouteCache() const { return clear_route_cache_; }
 
   uint32_t maxRequestBytes() const { return max_request_bytes_; }
@@ -198,6 +204,7 @@ class FilterConfig {
 
   const bool allow_partial_message_;
   const bool failure_mode_allow_;
+  const bool failure_debugging_allowed_;
   const bool clear_route_cache_;
   const uint32_t max_request_bytes_;
   const bool pack_as_bytes_;
@@ -231,6 +238,7 @@ class FilterConfig {
   const Stats::StatName ext_authz_denied_;
   const Stats::StatName ext_authz_error_;
   const Stats::StatName ext_authz_failure_mode_allowed_;
+  const Stats::StatName ext_authz_failure_debugging_allowed_;
 };
 
 using FilterConfigSharedPtr = std::shared_ptr<FilterConfig>;

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

@@ -179,6 +179,40 @@ class ExtAuthzHttpClientTest : public testing::Test {
   NiceMock<StreamInfo::MockStreamInfo> stream_info_;
 };
 
+// Verify that when a call to authorization server returns a 5xx and `allow_debugging_failures` is
+// set then we preserve the original status code, headers, etc.
+TEST_F(ExtAuthzHttpClientTest, AuthorizationRequest5xxNoError) {
+  const std::string yaml = R"EOF(
+    http_service:
+      server_uri:
+        uri: "ext_authz:9000"
+        cluster: "ext_authz"
+        timeout: 0.25s
+      authorization_response:
+        dynamic_metadata_from_headers:
+          patterns:
+          - prefix: "X-Metadata-"
+            ignore_case: true
+    allow_debugging_failures: true
+    )EOF";
+
+  initialize(yaml);
+
+  const auto expected_body = std::string{"test"};
+  const auto expected_headers = TestCommon::makeHeaderValueOption(
+      {{":status", "500", false}, {"foo", "bar", false}, {"honey", "bee", false}});
+  const auto authz_response = TestCommon::makeAuthzResponse(
+      CheckStatus::Error, Http::Code::InternalServerError, expected_body, expected_headers);
+
+  envoy::service::auth::v3::CheckRequest request;
+  client_->check(request_callbacks_, request, parent_span_, stream_info_);
+
+  EXPECT_CALL(request_callbacks_,
+              onComplete_(WhenDynamicCastTo<ResponsePtr&>(AuthzDeniedResponse(authz_response))));
+  client_->onSuccess(async_request_,
+                     TestCommon::makeMessageResponse(expected_headers, expected_body));
+}
+
 // Test HTTP client config default values.
 TEST_F(ExtAuthzHttpClientTest, ClientConfig) {
   const Http::LowerCaseString foo{"foo"};

test/extensions/filters/http/ext_authz/ext_authz_test.cc

@@ -430,6 +430,137 @@ TEST_F(HttpFilterTest, ImmediateErrorOpen) {
   EXPECT_EQ(1U, config_->stats().failure_mode_allowed_.value());
 }
 
+// Test when allow_debugging_failures is set then the original returned headers are propagated back
+// to the client.
+TEST_F(HttpFilterTest, ResponseHeadersGetPropagatedBackInFailureDebuggingMode) {
+  InSequence s;
+
+  initialize(R"EOF(
+  transport_api_version: V3
+  http_service:
+    server_uri:
+      uri: "http://localhost:8080"
+      cluster: "ext_authz_server"
+      timeout: "60s"
+  allow_debugging_failures: true
+  )EOF");
+  prepareCheck();
+  EXPECT_CALL(*client_, check(_, _, _, _))
+      .WillOnce(
+          Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks,
+                     const envoy::service::auth::v3::CheckRequest&, Tracing::Span&,
+                     const StreamInfo::StreamInfo&) -> void { request_callbacks_ = &callbacks; }));
+
+  EXPECT_CALL(decoder_filter_callbacks_.stream_info_,
+              setResponseFlag(Envoy::StreamInfo::ResponseFlag::UnauthorizedExternalService));
+  EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
+            filter_->decodeHeaders(request_headers_, false));
+
+  Http::TestResponseHeaderMapImpl response_headers{{":status", "500"},
+                                                   {"content-length", "20"},
+                                                   {"content-type", "text/plain"},
+                                                   {"honey", "bee"}};
+
+  EXPECT_CALL(decoder_filter_callbacks_, continueDecoding()).Times(0);
+  EXPECT_CALL(decoder_filter_callbacks_,
+              encodeHeaders_(HeaderMapEqualRef(&response_headers), false))
+      .WillOnce(Invoke([&](const Http::ResponseHeaderMap& headers, bool) -> void {
+        EXPECT_EQ(headers.getStatusValue(),
+                  std::to_string(enumToInt(Http::Code::InternalServerError)));
+      }));
+  EXPECT_CALL(decoder_filter_callbacks_, encodeData(_, true))
+      .WillOnce(Invoke([&](Buffer::Instance& data, bool) {
+        EXPECT_EQ(data.toString(), "something-went-wrong");
+      }));
+
+  Filters::Common::ExtAuthz::Response response{};
+  response.status = Filters::Common::ExtAuthz::CheckStatus::Error;
+  response.status_code = Http::Code::InternalServerError;
+  response.body = std::string{"something-went-wrong"};
+  response.headers_to_set = Http::HeaderVector{{Http::LowerCaseString{"honey"}, "bee"}};
+  request_callbacks_->onComplete(std::make_unique<Filters::Common::ExtAuthz::Response>(response));
+
+  // Verify that both `failure_debugging_allowed` and `error` counters get incremented.
+  EXPECT_EQ(1U, decoder_filter_callbacks_.clusterInfo()
+                    ->statsScope()
+                    .counterFromString("ext_authz.error")
+                    .value());
+  EXPECT_EQ(1U, decoder_filter_callbacks_.clusterInfo()
+                    ->statsScope()
+                    .counterFromString("ext_authz.failure_debugging_allowed")
+                    .value());
+  EXPECT_EQ(1U, config_->stats().error_.value());
+  EXPECT_EQ(1U, config_->stats().failure_debugging_allowed_.value());
+  EXPECT_EQ(1U, decoder_filter_callbacks_.clusterInfo()
+                    ->statsScope()
+                    .counterFromString("upstream_rq_5xx")
+                    .value());
+  EXPECT_EQ(1U, decoder_filter_callbacks_.clusterInfo()
+                    ->statsScope()
+                    .counterFromString("upstream_rq_500")
+                    .value());
+}
+
+// Test when allow_debugging_failures is not set then the headers sent from upstream are not
+// propagated back to the client.
+TEST_F(HttpFilterTest, ResponseHeadersDoNotGetPropagatedWhenNotFailureDebuggingMode) {
+  InSequence s;
+
+  initialize(R"EOF(
+  transport_api_version: V3
+  http_service:
+    server_uri:
+      uri: "http://localhost:8080"
+      cluster: "ext_authz_server"
+      timeout: "60s"
+  allow_debugging_failures: false
+  )EOF");
+  prepareCheck();
+  EXPECT_CALL(*client_, check(_, _, _, _))
+      .WillOnce(
+          Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks,
+                     const envoy::service::auth::v3::CheckRequest&, Tracing::Span&,
+                     const StreamInfo::StreamInfo&) -> void { request_callbacks_ = &callbacks; }));
+
+  EXPECT_CALL(decoder_filter_callbacks_.stream_info_,
+              setResponseFlag(Envoy::StreamInfo::ResponseFlag::UnauthorizedExternalService));
+  EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
+            filter_->decodeHeaders(request_headers_, false));
+
+  Http::TestResponseHeaderMapImpl response_headers{{":status", "403"}};
+  Buffer::OwnedImpl response_data{};
+  EXPECT_CALL(decoder_filter_callbacks_, continueDecoding()).Times(0);
+  EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, false));
+  EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(response_data, false));
+
+  Filters::Common::ExtAuthz::Response response{};
+  response.status = Filters::Common::ExtAuthz::CheckStatus::Error;
+  response.status_code = Http::Code::InternalServerError;
+  response.body = std::string{"something-went-wrong"};
+  response.headers_to_set = Http::HeaderVector{{Http::LowerCaseString{"honey"}, "bee"}};
+  request_callbacks_->onComplete(std::make_unique<Filters::Common::ExtAuthz::Response>(response));
+
+  // Verify that both `failure_debugging_allowed` and `error` counters get incremented.
+  EXPECT_EQ(1U, decoder_filter_callbacks_.clusterInfo()
+                    ->statsScope()
+                    .counterFromString("ext_authz.error")
+                    .value());
+  EXPECT_EQ(0U, decoder_filter_callbacks_.clusterInfo()
+                    ->statsScope()
+                    .counterFromString("ext_authz.failure_debugging_allowed")
+                    .value());
+  EXPECT_EQ(1U, config_->stats().error_.value());
+  EXPECT_EQ(0U, config_->stats().failure_debugging_allowed_.value());
+  EXPECT_EQ(0U, decoder_filter_callbacks_.clusterInfo()
+                    ->statsScope()
+                    .counterFromString("upstream_rq_5xx")
+                    .value());
+  EXPECT_EQ(0U, decoder_filter_callbacks_.clusterInfo()
+                    ->statsScope()
+                    .counterFromString("upstream_rq_500")
+                    .value());
+}
+
 // Check a bad configuration results in validation exception.
 TEST_F(HttpFilterTest, BadConfig) {
   const std::string filter_config = R"EOF(