ext_authz: support modifying and removing query string parameters when using a gRPC authorization server

api/envoy/config/core/v3/base.proto

@@ -296,6 +296,15 @@ message RuntimeFeatureFlag {
   string runtime_key = 2 [(validate.rules).string = {min_len: 1}];
 }
 
+// Query parameter name/value pair.
+message QueryParameter {
+  // The key of the query parameter. Case sensitive.
+  string key = 1 [(validate.rules).string = {min_len: 1}];
+
+  // The value of the query parameter.
+  string value = 2;
+}
+
 // Header name/value pair.
 message HeaderValue {
   option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.HeaderValue";

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

@@ -60,7 +60,7 @@ message DeniedHttpResponse {
 }
 
 // HTTP attributes for an OK response.
-// [#next-free-field: 7]
+// [#next-free-field: 9]
 message OkHttpResponse {
   option (udpa.annotations.versioning).previous_message_type =
       "envoy.service.auth.v2.OkHttpResponse";
@@ -102,6 +102,15 @@ message OkHttpResponse {
   // to the downstream client on success. Note that the :ref:`append field in HeaderValueOption <envoy_v3_api_field_config.core.v3.HeaderValueOption.append>`
   // defaults to false when used in this message.
   repeated config.core.v3.HeaderValueOption response_headers_to_add = 6;
+
+  // This field allows the authorization service to set (and overwrite) query
+  // string parameters on the original request before it is sent upstream.
+  repeated config.core.v3.QueryParameter query_parameters_to_set = 7;
+
+  // This field allows the authorization service to specify which query parameters
+  // should be removed from the original request before it is sent upstream. Each
+  // element in this list is a case-sensitive query parameter name to be removed.
+  repeated string query_parameters_to_remove = 8;
 }
 
 // Intended for gRPC and Network Authorization servers `only`.

docs/root/version_history/current.rst

@@ -24,6 +24,7 @@ Removed Config or Runtime
 
 New Features
 ------------
+* ext_authz: added :ref:`query_parameters_to_set <envoy_v3_api_field_service.auth.v3.OkHttpResponse.query_parameters_to_set>` and :ref:`query_parameters_to_remove <envoy_v3_api_field_service.auth.v3.OkHttpResponse.query_parameters_to_remove>` for adding and removing query string parameters when using a gRPC authorization server.
 * http: added support for :ref:`retriable health check status codes <envoy_v3_api_field_config.core.v3.HealthCheck.HttpHealthCheck.retriable_statuses>`.
 
 Deprecated

envoy/http/query_params.h

@@ -2,6 +2,7 @@
 
 #include <map>
 #include <string>
+#include <vector>
 
 namespace Envoy {
 namespace Http {
@@ -12,6 +13,7 @@ namespace Utility {
 // https://github.com/apache/incubator-pagespeed-mod/blob/master/pagespeed/kernel/http/query_params.h
 
 using QueryParams = std::map<std::string, std::string>;
+using QueryParamsVector = std::vector<std::pair<std::string, std::string>>;
 
 } // namespace Utility
 } // namespace Http

source/common/http/utility.cc

@@ -463,6 +463,18 @@ std::string Utility::stripQueryString(const HeaderString& path) {
                      query_offset != path_str.npos ? query_offset : path_str.size());
 }
 
+std::string Utility::replaceQueryString(const HeaderString& path,
+                                        const Utility::QueryParams& params) {
+  std::string new_path{Http::Utility::stripQueryString(path)};
+
+  if (!params.empty()) {
+    const auto new_query_string = Http::Utility::queryParamsToString(params);
+    absl::StrAppend(&new_path, new_query_string);
+  }
+
+  return new_path;
+}
+
 std::string Utility::parseCookieValue(const HeaderMap& headers, const std::string& key) {
   // TODO(wbpcode): Modify the headers parameter type to 'RequestHeaderMap'.
   return parseCookie(headers, key, Http::Headers::get().Cookie);

source/common/http/utility.h

@@ -248,6 +248,20 @@ absl::string_view findQueryStringStart(const HeaderString& path);
  */
 std::string stripQueryString(const HeaderString& path);
 
+/**
+ * Replace the query string portion of a given path with a new one.
+ *
+ * e.g. replaceQueryString("/foo?key=1", {key:2}) -> "/foo?key=2"
+ *      replaceQueryString("/bar", {hello:there}) -> "/bar?hello=there"
+ *
+ * @param path the original path that may or may not contain an existing query string
+ * @param params the new params whose string representation should be formatted onto
+ *               the `path` above
+ * @return std::string the new path whose query string has been replaced by `params` and whose path
+ *         portion from `path` remains unchanged.
+ */
+std::string replaceQueryString(const HeaderString& path, const QueryParams& params);
+
 /**
  * Parse a particular value out of a cookie
  * @param headers supplies the headers to get the cookie from.

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

@@ -12,6 +12,7 @@
 #include "envoy/tracing/http_tracer.h"
 
 #include "source/common/http/headers.h"
+#include "source/common/http/utility.h"
 #include "source/common/singleton/const_singleton.h"
 
 namespace Envoy {
@@ -84,6 +85,11 @@ struct Response {
   // A set of HTTP headers consumed by the authorization server, will be removed
   // from the request to the upstream server.
   std::vector<Envoy::Http::LowerCaseString> headers_to_remove;
+  // A set of query string parameters to be set (possibly overwritten) on the
+  // request to the upstream server.
+  Http::Utility::QueryParamsVector query_parameters_to_set;
+  // A set of query string parameters to remove from the request to the upstream server.
+  std::vector<std::string> query_parameters_to_remove;
   // Optional http body used only on denied response.
   std::string body;
   // Optional http status used only on denied response.

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

@@ -57,7 +57,17 @@ void GrpcClientImpl::onSuccess(std::unique_ptr<envoy::service::auth::v3::CheckRe
           authz_response->headers_to_remove.push_back(Http::LowerCaseString(header));
         }
       }
-
+      if (response->ok_response().query_parameters_to_set_size() > 0) {
+        for (const auto& query_parameter : response->ok_response().query_parameters_to_set()) {
+          authz_response->query_parameters_to_set.push_back(
+              std::pair(query_parameter.key(), query_parameter.value()));
+        }
+      }
+      if (response->ok_response().query_parameters_to_remove_size() > 0) {
+        for (const auto& key : response->ok_response().query_parameters_to_remove()) {
+          authz_response->query_parameters_to_remove.push_back(key);
+        }
+      }
       // These two vectors hold header overrides of encoded response headers.
       if (response->ok_response().response_headers_to_add_size() > 0) {
         for (const auto& header : response->ok_response().response_headers_to_add()) {

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

@@ -38,6 +38,8 @@ const Response& errorResponse() {
                                             Http::HeaderVector{},
                                             Http::HeaderVector{},
                                             {{}},
+                                            Http::Utility::QueryParamsVector{},
+                                            {},
                                             EMPTY_STRING,
                                             Http::Code::Forbidden,
                                             ProtobufWkt::Struct{}});
@@ -350,9 +352,17 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) {
                        config_->upstreamHeaderToAppendMatchers(),
                        config_->clientHeaderOnSuccessMatchers(),
                        config_->dynamicMetadataMatchers(),
-                       Response{CheckStatus::OK, Http::HeaderVector{}, Http::HeaderVector{},
-                                Http::HeaderVector{}, Http::HeaderVector{}, Http::HeaderVector{},
-                                std::move(headers_to_remove), EMPTY_STRING, Http::Code::OK,
+                       Response{CheckStatus::OK,
+                                Http::HeaderVector{},
+                                Http::HeaderVector{},
+                                Http::HeaderVector{},
+                                Http::HeaderVector{},
+                                Http::HeaderVector{},
+                                std::move(headers_to_remove),
+                                Http::Utility::QueryParamsVector{},
+                                {},
+                                EMPTY_STRING,
+                                Http::Code::OK,
                                 ProtobufWkt::Struct{}}};
     return std::move(ok.response_);
   }
@@ -370,6 +380,8 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) {
                                   Http::HeaderVector{},
                                   Http::HeaderVector{},
                                   {{}},
+                                  Http::Utility::QueryParamsVector{},
+                                  {},
                                   message->bodyAsString(),
                                   static_cast<Http::Code>(status_code),
                                   ProtobufWkt::Struct{}}};

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

@@ -222,12 +222,13 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) {
 
   switch (response->status) {
   case CheckStatus::OK: {
-    // Any changes to request headers can affect how the request is going to be
+    // Any changes to request headers or query parameters can affect how the request is going to be
     // routed. If we are changing the headers we also need to clear the route
     // cache.
     if (config_->clearRouteCache() &&
         (!response->headers_to_set.empty() || !response->headers_to_append.empty() ||
-         !response->headers_to_remove.empty())) {
+         !response->headers_to_remove.empty() || !response->query_parameters_to_set.empty() ||
+         !response->query_parameters_to_remove.empty())) {
       ENVOY_STREAM_LOG(debug, "ext_authz is clearing route cache", *decoder_callbacks_);
       decoder_callbacks_->clearRouteCache();
     }
@@ -286,6 +287,42 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) {
       response_headers_to_set_ = std::move(response->response_headers_to_set);
     }
 
+    absl::optional<Http::Utility::QueryParams> modified_query_parameters;
+    if (!response->query_parameters_to_set.empty()) {
+      modified_query_parameters =
+          Http::Utility::parseQueryString(request_headers_->Path()->value().getStringView());
+      ENVOY_STREAM_LOG(
+          trace, "ext_authz filter set query parameter(s) on the request:", *decoder_callbacks_);
+      for (const auto& [key, value] : response->query_parameters_to_set) {
+        ENVOY_STREAM_LOG(trace, "'{}={}'", *decoder_callbacks_, key, value);
+        (*modified_query_parameters)[key] = value;
+      }
+    }
+
+    if (!response->query_parameters_to_remove.empty()) {
+      if (!modified_query_parameters) {
+        modified_query_parameters =
+            Http::Utility::parseQueryString(request_headers_->Path()->value().getStringView());
+      }
+      ENVOY_STREAM_LOG(trace, "ext_authz filter removed query parameter(s) from the request:",
+                       *decoder_callbacks_);
+      for (const auto& key : response->query_parameters_to_remove) {
+        ENVOY_STREAM_LOG(trace, "'{}'", *decoder_callbacks_, key);
+        (*modified_query_parameters).erase(key);
+      }
+    }
+
+    // We modified the query parameters in some way, so regenerate the `path` header and set it
+    // here.
+    if (modified_query_parameters) {
+      const auto new_path = Http::Utility::replaceQueryString(request_headers_->Path()->value(),
+                                                              modified_query_parameters.value());
+      ENVOY_STREAM_LOG(
+          trace, "ext_authz filter modified query parameter(s), using new path for request: {}",
+          *decoder_callbacks_, new_path);
+      request_headers_->setPath(new_path);
+    }
+
     if (cluster_) {
       config_->incCounter(cluster_->statsScope(), config_->ext_authz_ok_);
     }

test/common/http/utility_test.cc

@@ -91,6 +91,67 @@ TEST(HttpUtility, parseQueryString) {
           "bucket%7Crq_xx%7Crq_complete%7Crq_active%7Ccx_active%29%29%7C%28server.version%29"));
 }
 
+TEST(HttpUtility, stripQueryString) {
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/")), "/");
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/?")), "/");
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/?x=1")), "/");
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/foo")), "/foo");
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/foo?")), "/foo");
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/foo?hello=there")), "/foo");
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/foo/?")), "/foo/");
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/foo/?x=1")), "/foo/");
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/foo/bar")), "/foo/bar");
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/foo/bar?")), "/foo/bar");
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/foo/bar?a=b")), "/foo/bar");
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/foo/bar?a=b&b=c")), "/foo/bar");
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/foo/bar/")), "/foo/bar/");
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/foo/bar/?")), "/foo/bar/");
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/foo/bar/?x=1")), "/foo/bar/");
+  EXPECT_EQ(Utility::stripQueryString(HeaderString("/foo/bar/?x=1&y=2")), "/foo/bar/");
+}
+
+TEST(HttpUtility, replaceQueryString) {
+  // Replace with nothing
+  EXPECT_EQ(Utility::replaceQueryString(HeaderString("/"), Utility::QueryParams()), "/");
+  EXPECT_EQ(Utility::replaceQueryString(HeaderString("/?"), Utility::QueryParams()), "/");
+  EXPECT_EQ(Utility::replaceQueryString(HeaderString("/?x=0"), Utility::QueryParams()), "/");
+  EXPECT_EQ(Utility::replaceQueryString(HeaderString("/a"), Utility::QueryParams()), "/a");
+  EXPECT_EQ(Utility::replaceQueryString(HeaderString("/a/"), Utility::QueryParams()), "/a/");
+  EXPECT_EQ(Utility::replaceQueryString(HeaderString("/a/?y=5"), Utility::QueryParams()), "/a/");
+  // Replace with x=1
+  EXPECT_EQ(Utility::replaceQueryString(HeaderString("/"), Utility::QueryParams({{"x", "1"}})),
+            "/?x=1");
+  EXPECT_EQ(Utility::replaceQueryString(HeaderString("/?"), Utility::QueryParams({{"x", "1"}})),
+            "/?x=1");
+  EXPECT_EQ(Utility::replaceQueryString(HeaderString("/?x=0"), Utility::QueryParams({{"x", "1"}})),
+            "/?x=1");
+  EXPECT_EQ(Utility::replaceQueryString(HeaderString("/a?x=0"), Utility::QueryParams({{"x", "1"}})),
+            "/a?x=1");
+  EXPECT_EQ(
+      Utility::replaceQueryString(HeaderString("/a/?x=0"), Utility::QueryParams({{"x", "1"}})),
+      "/a/?x=1");
+  // More replacements
+  EXPECT_EQ(Utility::replaceQueryString(HeaderString("/foo"),
+                                        Utility::QueryParams({{"x", "1"}, {"z", "3"}})),
+            "/foo?x=1&z=3");
+  EXPECT_EQ(Utility::replaceQueryString(HeaderString("/foo?z=2"),
+                                        Utility::QueryParams({{"x", "1"}, {"y", "5"}})),
+            "/foo?x=1&y=5");
+  EXPECT_EQ(Utility::replaceQueryString(HeaderString("/foo?y=9"),
+                                        Utility::QueryParams({{"x", "1"}, {"y", "5"}})),
+            "/foo?x=1&y=5");
+  // More path components
+  EXPECT_EQ(Utility::replaceQueryString(HeaderString("/foo/bar?"),
+                                        Utility::QueryParams({{"x", "1"}, {"y", "5"}})),
+            "/foo/bar?x=1&y=5");
+  EXPECT_EQ(Utility::replaceQueryString(HeaderString("/foo/bar?y=9&a=b"),
+                                        Utility::QueryParams({{"x", "1"}, {"y", "5"}})),
+            "/foo/bar?x=1&y=5");
+  EXPECT_EQ(Utility::replaceQueryString(HeaderString("/foo/bar?y=11&z=7"),
+                                        Utility::QueryParams({{"a", "b"}, {"x", "1"}, {"y", "5"}})),
+            "/foo/bar?a=b&x=1&y=5");
+}
+
 TEST(HttpUtility, getResponseStatus) {
   EXPECT_THROW(Utility::getResponseStatus(TestResponseHeaderMapImpl{}), CodecClientException);
   EXPECT_EQ(200U, Utility::getResponseStatus(TestResponseHeaderMapImpl{{":status", "200"}}));

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

@@ -315,6 +315,46 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithDynamicMetadata) {
   client_->onSuccess(std::move(check_response), span_);
 }
 
+// Test the client when an OK response is received with additional query string parameters.
+TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithQueryParameters) {
+  initialize();
+
+  auto check_response = std::make_unique<envoy::service::auth::v3::CheckResponse>();
+  auto status = check_response->mutable_status();
+
+  status->set_code(Grpc::Status::WellKnownGrpcStatus::Ok);
+
+  const Http::Utility::QueryParamsVector query_parameters_to_set{{"add-me", "yes"}};
+  for (const auto& [key, value] : query_parameters_to_set) {
+    auto* query_parameter = check_response->mutable_ok_response()->add_query_parameters_to_set();
+    query_parameter->set_key(key);
+    query_parameter->set_value(value);
+  }
+
+  const std::vector<std::string> query_parameters_to_remove{"remove-me"};
+  for (const auto& key : query_parameters_to_remove) {
+    check_response->mutable_ok_response()->add_query_parameters_to_remove(key);
+  }
+
+  // This is the expected authz response.
+  auto authz_response = Response{};
+  authz_response.status = CheckStatus::OK;
+  authz_response.query_parameters_to_set = {{"add-me", "yes"}};
+  authz_response.query_parameters_to_remove = {"remove-me"};
+
+  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_CALL(span_, setTag(Eq("ext_authz_status"), Eq("ext_authz_ok")));
+  EXPECT_CALL(request_callbacks_,
+              onComplete_(WhenDynamicCastTo<ResponsePtr&>(AuthzOkResponse(authz_response))));
+  client_->onSuccess(std::move(check_response), span_);
+}
+
 } // namespace ExtAuthz
 } // namespace Common
 } // namespace Filters

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

@@ -130,6 +130,19 @@ bool TestCommon::compareVectorOfHeaderName(const std::vector<Http::LowerCaseStri
          std::set<Http::LowerCaseString>(rhs.begin(), rhs.end());
 }
 
+bool TestCommon::compareVectorOfUnorderedStrings(const std::vector<std::string>& lhs,
+                                                 const std::vector<std::string>& rhs) {
+  return std::set<std::string>(lhs.begin(), lhs.end()) ==
+         std::set<std::string>(rhs.begin(), rhs.end());
+}
+
+// TODO(esmet): This belongs in a QueryParams class
+bool TestCommon::compareQueryParamsVector(const Http::Utility::QueryParamsVector& lhs,
+                                          const Http::Utility::QueryParamsVector& rhs) {
+  return std::set<std::pair<std::string, std::string>>(lhs.begin(), lhs.end()) ==
+         std::set<std::pair<std::string, std::string>>(rhs.begin(), rhs.end());
+}
+
 } // namespace ExtAuthz
 } // namespace Common
 } // namespace Filters