outh2: passthrough of authorization header when filter is matched (en…

…voyproxy#23489)

Authorization header sanitation altered headers before the passthrough evaluation.
Therefore, move of passthrough header matching to the beginning if the header evaluation,
so the unaltered request is passed through.
Introduces a new metric: oauth_passthrough counter.

Signed-off-by: Timo Haas <haastimo@gmx.de>

changelogs/current.yaml

@@ -8,6 +8,9 @@ minor_behavior_changes:
 - area: cache_filter
   change: |
     add a completion callback to updateHeaders interface. Any external cache implementations will need to update to match this new interface. See changes to simple_http_cache in PR#23666 for example.
+- area: oauth2
+  change: |
+    Requests which match the passthrough header now have their own metric ``oauth_passthrough`` and aren't included in ``oauth_success`` anymore.
 
 bug_fixes:
 # *Changes expected to improve the state of the world and are unlikely to have negative effects*
@@ -17,6 +20,9 @@ bug_fixes:
 - area: router
   change: |
     fixed a bug that incorrectly rewrote the path when using ``regex_rewrite`` for redirects matched on prefix.
+- area: oauth2
+  change: |
+    fixed a bug when passthrough header was matched, envoy would always remove the authorization header. This behavioral change can be temporarily reverted by setting runtime guard ``envoy.reloadable_features.oauth_header_passthrough_fix`` to false.
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

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

@@ -229,7 +229,7 @@ sending the user to the configured auth endpoint.
 
 :ref:`pass_through_matcher <envoy_v3_api_field_extensions.filters.http.oauth2.v3.OAuth2Config.pass_through_matcher>` provides
 an interface for users to provide specific header matching criteria such that, when applicable, the OAuth flow is entirely skipped.
-When this occurs, the ``oauth_success`` metric is still incremented.
+When this occurs, the ``oauth_passthrough`` metric is incremented but ``success`` is not.
 
 Generally, allowlisting is inadvisable from a security standpoint.
 
@@ -243,5 +243,6 @@ The OAuth2 filter outputs statistics in the *<stat_prefix>.* namespace.
   :widths: 1, 1, 2
 
   oauth_failure, Counter, Total requests that were denied.
+  oauth_passthrough, Counter, Total request that matched a passthrough header.
   oauth_success, Counter, Total requests that were allowed.
   oauth_unauthorization_rq, Counter, Total unauthorized requests.

source/common/runtime/runtime_features.cc

@@ -56,6 +56,7 @@ RUNTIME_GUARD(envoy_reloadable_features_local_ratelimit_match_all_descriptors);
 RUNTIME_GUARD(envoy_reloadable_features_lua_respond_with_send_local_reply);
 RUNTIME_GUARD(envoy_reloadable_features_no_delay_close_for_upgrades);
 RUNTIME_GUARD(envoy_reloadable_features_no_extension_lookup_by_name);
+RUNTIME_GUARD(envoy_reloadable_features_oauth_header_passthrough_fix);
 RUNTIME_GUARD(envoy_reloadable_features_original_dst_rely_on_idle_timeout);
 RUNTIME_GUARD(envoy_reloadable_features_postpone_h3_client_connect_to_next_loop);
 RUNTIME_GUARD(envoy_reloadable_features_quic_defer_send_in_response_to_packet);

source/extensions/filters/http/oauth2/filter.cc

@@ -19,6 +19,7 @@
 #include "source/common/http/message_impl.h"
 #include "source/common/http/utility.h"
 #include "source/common/protobuf/utility.h"
+#include "source/common/runtime/runtime_features.h"
 
 #include "absl/strings/escaping.h"
 #include "absl/strings/match.h"
@@ -221,12 +222,25 @@ const std::string& OAuth2Filter::bearerPrefix() const {
 
 /**
  * primary cases:
- * 1) user is signing out
- * 2) /_oauth redirect
- * 3) user is authorized
- * 4) user is unauthorized
+ * 1) pass through header is matching
+ * 2) user is signing out
+ * 3) /_oauth redirect
+ * 4) user is authorized
+ * 5) user is unauthorized
  */
 Http::FilterHeadersStatus OAuth2Filter::decodeHeaders(Http::RequestHeaderMap& headers, bool) {
+  // Skip Filter and continue chain if a Passthrough header is matching
+  // Must be done before the sanitation of the authorization header,
+  // otherwise the authorization header might be altered or removed
+  if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.oauth_header_passthrough_fix")) {
+    for (const auto& matcher : config_->passThroughMatchers()) {
+      if (matcher.matchesHeaders(headers)) {
+        config_->stats().oauth_passthrough_.inc();
+        return Http::FilterHeadersStatus::Continue;
+      }
+    }
+  }
+
   // Sanitize the Authorization header, since we have no way to validate its content. Also,
   // if token forwarding is enabled, this header will be set based on what is on the HMAC cookie
   // before forwarding the request upstream.
@@ -376,13 +390,13 @@ bool OAuth2Filter::canSkipOAuth(Http::RequestHeaderMap& headers) const {
     }
     return true;
   }
-
-  for (const auto& matcher : config_->passThroughMatchers()) {
-    if (matcher.matchesHeaders(headers)) {
-      return true;
+  if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.oauth_header_passthrough_fix")) {
+    for (const auto& matcher : config_->passThroughMatchers()) {
+      if (matcher.matchesHeaders(headers)) {
+        return true;
+      }
     }
   }
-
   return false;
 }
 

source/extensions/filters/http/oauth2/filter.h

@@ -85,6 +85,7 @@ class SDSSecretReader : public SecretReader {
 #define ALL_OAUTH_FILTER_STATS(COUNTER)                                                            \
   COUNTER(oauth_unauthorized_rq)                                                                   \
   COUNTER(oauth_failure)                                                                           \
+  COUNTER(oauth_passthrough)                                                                       \
   COUNTER(oauth_success)
 
 /**

test/extensions/filters/http/oauth2/BUILD

@@ -50,6 +50,7 @@ envoy_extension_cc_test(
         "//test/integration:http_integration_lib",
         "//test/mocks/server:server_mocks",
         "//test/mocks/upstream:upstream_mocks",
+        "//test/test_common:test_runtime_lib",
         "@envoy_api//envoy/extensions/filters/http/oauth2/v3:pkg_cc_proto",
     ],
 )

test/extensions/filters/http/oauth2/filter_test.cc

@@ -16,6 +16,7 @@
 #include "test/mocks/http/mocks.h"
 #include "test/mocks/server/mocks.h"
 #include "test/mocks/upstream/mocks.h"
+#include "test/test_common/test_runtime.h"
 #include "test/test_common/utility.h"
 
 #include "gmock/gmock.h"
@@ -573,11 +574,45 @@ TEST_F(OAuth2Test, OAuthOptionsRequestAndContinue) {
       {Http::Headers::get().Host.get(), "traffic.example.com"},
       {Http::Headers::get().Path.get(), "/anypath"},
       {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Options},
+      {Http::CustomHeaders::get().Authorization.get(), "Bearer xyz-header-token"}};
+
+  Http::TestRequestHeaderMapImpl expected_headers{
+      {Http::Headers::get().Host.get(), "traffic.example.com"},
+      {Http::Headers::get().Path.get(), "/anypath"},
+      {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Options},
+      {Http::CustomHeaders::get().Authorization.get(), "Bearer xyz-header-token"}};
+
+  EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false));
+  EXPECT_EQ(request_headers, expected_headers);
+  EXPECT_EQ(scope_.counterFromString("test.oauth_failure").value(), 0);
+  EXPECT_EQ(scope_.counterFromString("test.oauth_passthrough").value(), 1);
+  EXPECT_EQ(scope_.counterFromString("test.oauth_success").value(), 0);
+}
+
+TEST_F(OAuth2Test, OAuthOptionsRequestAndContinue_oauth_header_passthrough_fix) {
+  TestScopedRuntime scoped_runtime;
+  scoped_runtime.mergeValues({
+      {"envoy.reloadable_features.oauth_header_passthrough_fix", "false"},
+  });
+  Http::TestRequestHeaderMapImpl request_headers{
+      {Http::Headers::get().Host.get(), "traffic.example.com"},
+      {Http::Headers::get().Path.get(), "/anypath"},
+      {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Options},
+      {Http::CustomHeaders::get().Authorization.get(), "Bearer xyz-header-token"}};
+
+  Http::TestRequestHeaderMapImpl expected_headers{
+      {Http::Headers::get().Host.get(), "traffic.example.com"},
+      {Http::Headers::get().Path.get(), "/anypath"},
+      {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Options},
   };
 
   EXPECT_CALL(*validator_, setParams(_, _));
   EXPECT_CALL(*validator_, isValid()).WillOnce(Return(false));
+
   EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false));
+  EXPECT_EQ(request_headers, expected_headers);
+  EXPECT_EQ(scope_.counterFromString("test.oauth_failure").value(), 0);
+  EXPECT_EQ(scope_.counterFromString("test.oauth_success").value(), 0);
 }
 
 // Validates the behavior of the cookie validator.