ext_authz: send local reply on failed response header mutations (#41844)

Commit Message: ext_authz: send local reply on failed response header
mutations
Additional Description:

Without this PR, the ext authz filter will silently (to the downstream
client) fail to add headers if it would violate the response header map
limits. This is bad because the app being proxied could rely on the
headers added by ext authz.

Instead it is better if the ext authz filter just sent a local reply the
same way ext_proc does. This way it will be clear that something went
wrong. If this behavior is undesirable, the filter can always be
configured with these check disabled.

Since we are now sending a local reply instead of an incomplete reply,
we can now do the simpler thing and just add headers and do a
post-check, which means we can also using the existing
headersWithinLimits function.

Risk Level: low
Testing: unit tested and integration tests updated
Docs Changes: config comments updated
Release Notes: changelog updated

---------

Signed-off-by: antoniovleonti <[email protected]>
Signed-off-by: Antonio V. Leonti <[email protected]>

Author: antoniovleonti

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

@@ -324,8 +324,7 @@ message ExtAuthz {
   uint32 max_denied_response_body_bytes = 30;
 
   // When set to ``true``, the filter will enforce the response header map's count and size limits
-  // by dropping headers added by the external auth service. Enabling this may break applications
-  // relying on those headers!
+  // by sending a local reply when those limits are violated.
   //
   // When set to ``false``, the filter will ignore the response header map's limits and add / set
   // all response headers as specified by the external auth service.

changelogs/current.yaml

@@ -55,8 +55,8 @@ minor_behavior_changes:
     ``envoy.reloadable_features.ext_proc_stream_close_optimization`` to ``false``.
 - area: ext_authz
   change: |
-    Check that the response header count is less than the configured limits before applying mutations, and do not
-    add new headers if not.
+    Check that the response header count and size is less than the configured limits after applying
+    mutations and send a local reply if not.
 - area: ext_authz
   change: |
     Fixed HTTP ext_authz service to properly propagate headers (such as ``set-cookie``) back to clients. The

docs/root/configuration/http/http_filters/ext_authz_filter.rst

@@ -153,6 +153,10 @@ The HTTP filter outputs statistics in the ``cluster.<route target cluster>.ext_a
   invalid, Counter, Total responses rejected due to invalid header or query parameter mutations.
   omitted_response_headers, Counter, "Total responses for which ext_authz rejected any number of
   headers due to the header map constraints."
+  request_header_limits_reached, Counter, "Total requests for which ext_authz sent a local reply
+  because it couldn't apply all header mutations"
+  response_header_limits_reached, Counter, "Total responses for which ext_authz sent a local reply
+  because it couldn't apply all header mutations"
 
 Dynamic Metadata
 ----------------

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

@@ -488,47 +488,43 @@ Http::FilterHeadersStatus Filter::encodeHeaders(Http::ResponseHeaderMap& headers
                    "set to the encoded response:",
                    *encoder_callbacks_, response_headers_to_add_.size(),
                    response_headers_to_set_.size());
-  bool omitted_response_headers = false;
   if (!response_headers_to_add_.empty()) {
     ENVOY_STREAM_LOG(
         trace, "ext_authz filter added header(s) to the encoded response:", *encoder_callbacks_);
     for (const auto& [key, value] : response_headers_to_add_) {
-      if (config_->enforceResponseHeaderLimits() && headers.size() >= headers.maxHeadersCount()) {
-        omitted_response_headers = true;
-        break;
-      }
       ENVOY_STREAM_LOG(trace, "'{}':'{}'", *encoder_callbacks_, key.get(), value);
       headers.addCopy(key, value);
+      if (config_->enforceResponseHeaderLimits() && !headersWithinLimits(headers)) {
+        responseHeaderLimitsReached();
+        return Http::FilterHeadersStatus::StopIteration;
+      }
     }
   }
 
   if (!response_headers_to_set_.empty()) {
     ENVOY_STREAM_LOG(
         trace, "ext_authz filter set header(s) to the encoded response:", *encoder_callbacks_);
     for (const auto& [key, value] : response_headers_to_set_) {
-      if (config_->enforceResponseHeaderLimits() && headers.get(key).empty() &&
-          headers.size() >= headers.maxHeadersCount()) {
-        omitted_response_headers = true;
-        // Continue because there could be other existing headers that can be set without increasing
-        // the number of headers.
-        continue;
-      }
       ENVOY_STREAM_LOG(trace, "'{}':'{}'", *encoder_callbacks_, key.get(), value);
       headers.setCopy(key, value);
+      if (config_->enforceResponseHeaderLimits() && !headersWithinLimits(headers)) {
+        responseHeaderLimitsReached();
+        return Http::FilterHeadersStatus::StopIteration;
+      }
     }
   }
 
   if (!response_headers_to_add_if_absent_.empty()) {
     for (const auto& [key, value] : response_headers_to_add_if_absent_) {
       ENVOY_STREAM_LOG(trace, "'{}':'{}'", *encoder_callbacks_, key.get(), value);
       if (auto header_entry = headers.get(key); header_entry.empty()) {
-        if (config_->enforceResponseHeaderLimits() && headers.size() >= headers.maxHeadersCount()) {
-          omitted_response_headers = true;
-          break;
-        }
         ENVOY_STREAM_LOG(trace, "ext_authz filter added header(s) to the encoded response:",
                          *encoder_callbacks_);
         headers.addCopy(key, value);
+        if (config_->enforceResponseHeaderLimits() && !headersWithinLimits(headers)) {
+          responseHeaderLimitsReached();
+          return Http::FilterHeadersStatus::StopIteration;
+        }
       }
     }
   }
@@ -540,16 +536,14 @@ Http::FilterHeadersStatus Filter::encodeHeaders(Http::ResponseHeaderMap& headers
         ENVOY_STREAM_LOG(
             trace, "ext_authz filter set header(s) to the encoded response:", *encoder_callbacks_);
         headers.setCopy(key, value);
+        if (config_->enforceResponseHeaderLimits() && !headersWithinLimits(headers)) {
+          responseHeaderLimitsReached();
+          return Http::FilterHeadersStatus::StopIteration;
+        }
       }
     }
   }
 
-  if (omitted_response_headers) {
-    ENVOY_LOG_EVERY_POW_2(
-        warn, "Some ext_authz response headers weren't added because the header map was full.");
-    stats_.omitted_response_headers_.inc();
-  }
-
   return Http::FilterHeadersStatus::Continue;
 }
 
@@ -1052,6 +1046,20 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) {
   }
 }
 
+void Filter::responseHeaderLimitsReached() {
+  const Http::Code status = Http::Code::InternalServerError;
+  ENVOY_LOG_EVERY_POW_2(warn,
+                        "ext_authz filter couldn't add all response header mutations. "
+                        "Sending local reply with response status code: {}",
+                        enumToInt(status));
+  stats_.response_header_limits_reached_.inc();
+  encoder_callbacks_->streamInfo().setResponseFlag(
+      StreamInfo::CoreResponseFlag::UnauthorizedExternalService);
+  encoder_callbacks_->sendLocalReply(
+      status, EMPTY_STRING, nullptr, absl::nullopt,
+      Filters::Common::ExtAuthz::ResponseCodeDetails::get().AuthzInvalid);
+}
+
 void Filter::rejectResponse() {
   const Http::Code status = Http::Code::InternalServerError;
   ENVOY_STREAM_LOG(trace, "ext_authz filter invalidated the response. Response status code: {}",

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

@@ -48,7 +48,8 @@ namespace ExtAuthz {
   COUNTER(ignored_dynamic_metadata)                                                                \
   COUNTER(filter_state_name_collision)                                                             \
   COUNTER(omitted_response_headers)                                                                \
-  COUNTER(request_header_limits_reached)
+  COUNTER(request_header_limits_reached)                                                           \
+  COUNTER(response_header_limits_reached)
 
 /**
  * Wrapper struct for ext_authz filter stats. @see stats_macros.h
@@ -425,6 +426,8 @@ class Filter : public Logger::Loggable<Logger::Id::ext_authz>,
   validateAndCheckDecoderHeaderMutation(Filters::Common::MutationRules::CheckOperation operation,
                                         absl::string_view key, absl::string_view value) const;
 
+  void responseHeaderLimitsReached();
+
   // Called when the filter is configured to reject invalid responses & the authz response contains
   // invalid header or query parameters. Sends a local response with the configured rejection status
   // code.

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

@@ -2131,12 +2131,7 @@ TEST_P(ExtAuthzGrpcIntegrationTest, EncodeHeadersToAddExceedsLimit) {
 
   ASSERT_TRUE(response_->waitForEndStream());
   EXPECT_TRUE(response_->complete());
-  EXPECT_EQ("200", response_->headers().getStatusValue());
-  // NB: Something else adds headers to the response between the ext_authz filter and the downstream
-  // client so the header count is greater than you might expect (100), but we can at least be sure
-  // that all the ext_authz response headers didn't get added.
-  EXPECT_LT(response_->headers().size(), 120);
-  EXPECT_TRUE(response_->headers().get(Http::LowerCaseString("new-header-99")).empty());
+  EXPECT_EQ("500", response_->headers().getStatusValue());
   cleanup();
 }
 
@@ -2180,16 +2175,7 @@ TEST_P(ExtAuthzGrpcIntegrationTest, EncodeHeadersToSetExceedsLimit) {
   ASSERT_TRUE(response_->waitForEndStream());
 
   EXPECT_TRUE(response_->complete());
-  EXPECT_EQ("200", response_->headers().getStatusValue());
-
-  EXPECT_LT(response_->headers().size(), 120);
-  // The last new headers shouldn't get added to the header map, but the
-  // existing upstream header will be overridden.
-  EXPECT_TRUE(response_->headers().get(Http::LowerCaseString("new-header-99")).empty());
-  EXPECT_EQ("new-value", response_->headers()
-                             .get(Http::LowerCaseString("upstream-header"))[0]
-                             ->value()
-                             .getStringView());
+  EXPECT_EQ("500", response_->headers().getStatusValue());
   cleanup();
 }
 
@@ -2231,10 +2217,7 @@ TEST_P(ExtAuthzGrpcIntegrationTest, EncodeHeadersToAppendIfAbsentExceedsLimit) {
   ASSERT_TRUE(response_->waitForEndStream());
 
   EXPECT_TRUE(response_->complete());
-  EXPECT_EQ("200", response_->headers().getStatusValue());
-
-  EXPECT_LT(response_->headers().size(), 120);
-  EXPECT_TRUE(response_->headers().get(Http::LowerCaseString("new-header-99")).empty());
+  EXPECT_EQ("500", response_->headers().getStatusValue());
   cleanup();
 }