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

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

@@ -60,8 +60,20 @@ message DeniedHttpResponse {
   string body = 3;
 }
 
+// TODO: Should this be in the core API?
+message QueryParameterOption {
+  // The key of a the query parameter. Must be non-empty.
+  string key = 1 [(validate.rules).string = {min_len: 1}];
+
+  // The value of the query parameter. May be empty, e.g. for a query parameter such as `foo=`.
+  string value = 2;
+
+  // Whether to remove the query parameter with the above `key`.
+  bool remove = 3;
+}
+
 // HTTP attributes for an OK response.
-// [#next-free-field: 7]
+// [#next-free-field: 8]
 message OkHttpResponse {
   option (udpa.annotations.versioning).previous_message_type =
       "envoy.service.auth.v2.OkHttpResponse";
@@ -103,6 +115,10 @@ 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 QueryParameterOption query_parameters_to_set = 7;
 }
 
 // Intended for gRPC and Network Authorization servers `only`.

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 {
@@ -81,6 +82,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::QueryParams 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,6 +57,18 @@ 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()) {
+          // TODO(esmet): It might make more sense to store query_parameters_to_set as a vector
+          // instead of a map since we will likely only ever iterate them linearly.
+          if (query_parameter.remove()) {
+            authz_response->query_parameters_to_remove.push_back(query_parameter.key());
+          } else {
+            authz_response->query_parameters_to_set[query_parameter.key()] =
+                query_parameter.value();
+          }
+        }
+      }
       if (response->ok_response().response_headers_to_add_size() > 0) {
         for (const auto& header : response->ok_response().response_headers_to_add()) {
           authz_response->response_headers_to_add.emplace_back(

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

@@ -37,6 +37,8 @@ const Response& errorResponse() {
                                             Http::HeaderVector{},
                                             Http::HeaderVector{},
                                             {{}},
+                                            {{}},
+                                            {{}},
                                             EMPTY_STRING,
                                             Http::Code::Forbidden,
                                             ProtobufWkt::Struct{}});
@@ -324,12 +326,20 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) {
 
   // Create an Ok authorization response.
   if (status_code == enumToInt(Http::Code::OK)) {
-    SuccessResponse ok{
-        message->headers(), config_->upstreamHeaderMatchers(),
-        config_->upstreamHeaderToAppendMatchers(), config_->clientHeaderOnSuccessMatchers(),
-        Response{CheckStatus::OK, Http::HeaderVector{}, Http::HeaderVector{}, Http::HeaderVector{},
-                 Http::HeaderVector{}, std::move(headers_to_remove), EMPTY_STRING, Http::Code::OK,
-                 ProtobufWkt::Struct{}}};
+    SuccessResponse ok{message->headers(), config_->upstreamHeaderMatchers(),
+                       config_->upstreamHeaderToAppendMatchers(),
+                       config_->clientHeaderOnSuccessMatchers(),
+                       Response{CheckStatus::OK,
+                                Http::HeaderVector{},
+                                Http::HeaderVector{},
+                                Http::HeaderVector{},
+                                Http::HeaderVector{},
+                                std::move(headers_to_remove),
+                                {{}},
+                                {{}},
+                                EMPTY_STRING,
+                                Http::Code::OK,
+                                ProtobufWkt::Struct{}}};
     return std::move(ok.response_);
   }
 
@@ -343,6 +353,8 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) {
                                   Http::HeaderVector{},
                                   Http::HeaderVector{},
                                   {{}},
+                                  {{}},
+                                  {{}},
                                   message->bodyAsString(),
                                   static_cast<Http::Code>(status_code),
                                   ProtobufWkt::Struct{}}};

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

@@ -213,12 +213,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();
     }
@@ -271,6 +272,50 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) {
       response_headers_to_add_ = std::move(response->response_headers_to_add);
     }
 
+    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);
+        // TODO(esmet): Sanitize key/value and/or declare the security posture that we trust the
+        // authorization server.
+        (*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) {
+      std::string new_path;
+      const auto path_without_query =
+          Http::Utility::stripQueryString(request_headers_->Path()->value());
+      // TODO: These two lines should probably be abstracted as
+      // Http::Utility::formatPathAndQueryParams
+      const auto new_query_string =
+          Http::Utility::queryParamsToString(modified_query_parameters.value());
+      absl::StrAppend(&new_path, path_without_query, new_query_string);
+      ENVOY_STREAM_LOG(trace,
+                       "ext_authz filter modified query parameter, 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/extensions/filters/http/ext_authz/ext_authz_test.cc

@@ -70,6 +70,57 @@ template <class T> class HttpFilterTestBase : public T {
     connection_.stream_info_.downstream_connection_info_provider_->setLocalAddress(addr_);
   }
 
+  void queryParameterTest(const std::string& original_path, const std::string& expected_path,
+                          const std::vector<std::pair<std::string, std::string>>& add_me,
+                          const std::string& remove_me) {
+    InSequence s;
+
+    // Set up all the typical headers plus a path with a query string that we'll remove later.
+    request_headers_.addCopy(Http::Headers::get().Host, "example.com");
+    request_headers_.addCopy(Http::Headers::get().Method, "GET");
+    request_headers_.addCopy(Http::Headers::get().Path, original_path);
+    request_headers_.addCopy(Http::Headers::get().Scheme, "https");
+
+    prepareCheck();
+
+    Filters::Common::ExtAuthz::Response response{};
+    response.status = Filters::Common::ExtAuthz::CheckStatus::OK;
+
+    if (!add_me.empty()) {
+      for (const auto& [key, value] : add_me) {
+        response.query_parameters_to_set[key] = value;
+      }
+    }
+    if (!remove_me.empty()) {
+      const std::vector<std::string> query_parameters_to_remove{remove_me};
+      response.query_parameters_to_remove = query_parameters_to_remove;
+    }
+
+    auto response_ptr = std::make_unique<Filters::Common::ExtAuthz::Response>(response);
+
+    EXPECT_CALL(*client_, check(_, _, _, _))
+        .WillOnce(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks,
+                             const envoy::service::auth::v3::CheckRequest&, Tracing::Span&,
+                             const StreamInfo::StreamInfo&) -> void {
+          callbacks.onComplete(std::move(response_ptr));
+        }));
+    EXPECT_CALL(filter_callbacks_, continueDecoding()).Times(0);
+    EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false));
+    EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data_, false));
+    EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers_));
+    EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers_));
+    EXPECT_EQ(request_headers_.getPathValue(), expected_path);
+
+    Buffer::OwnedImpl response_data{};
+    Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}};
+    Http::TestResponseTrailerMapImpl response_trailers{};
+    Http::MetadataMap response_metadata{};
+    EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, false));
+    EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->encodeData(response_data, false));
+    EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->encodeTrailers(response_trailers));
+    EXPECT_EQ(Http::FilterMetadataStatus::Continue, filter_->encodeMetadata(response_metadata));
+  }
+
   NiceMock<Stats::MockIsolatedStatsStore> stats_store_;
   envoy::config::bootstrap::v3::Bootstrap bootstrap_;
   FilterConfigSharedPtr config_;
@@ -1776,6 +1827,55 @@ TEST_P(HttpFilterTestParam, ImmediateOkResponseWithHttpAttributes) {
   EXPECT_EQ(response_headers.get_("cookie"), "flavor=gingerbread");
 }
 
+TEST_P(HttpFilterTestParam, ImmediateOkResponseWithUnmodifiedQueryParameters) {
+  const std::string original_path{"/users?leave-me=alone"};
+  const std::string expected_path{"/users?leave-me=alone"};
+  const std::vector<std::pair<std::string, std::string>> add_me{};
+  const std::string remove_me{"remove-me"};
+  queryParameterTest(original_path, expected_path, add_me, remove_me);
+}
+
+TEST_P(HttpFilterTestParam, ImmediateOkResponseWithAddedQueryParameters) {
+  const std::string original_path{"/users"};
+  const std::string expected_path{"/users?add-me=123"};
+  const std::vector<std::pair<std::string, std::string>> add_me{{"add-me", "123"}};
+  const std::string remove_me{};
+  queryParameterTest(original_path, expected_path, add_me, remove_me);
+}
+
+TEST_P(HttpFilterTestParam, ImmediateOkResponseWithAddedAndRemovedQueryParameters) {
+  const std::string original_path{"/users?remove-me=123"};
+  const std::string expected_path{"/users?add-me=456"};
+  const std::vector<std::pair<std::string, std::string>> add_me{{"add-me", "456"}};
+  const std::string remove_me{"remove-me"};
+  queryParameterTest(original_path, expected_path, add_me, remove_me);
+}
+
+TEST_P(HttpFilterTestParam, ImmediateOkResponseWithRemovedQueryParameters) {
+  const std::string original_path{"/users?remove-me=definitely"};
+  const std::string expected_path{"/users"};
+  const std::vector<std::pair<std::string, std::string>> add_me{};
+  const std::string remove_me{"remove-me"};
+  queryParameterTest(original_path, expected_path, add_me, remove_me);
+}
+
+TEST_P(HttpFilterTestParam, ImmediateOkResponseWithOverwrittenQueryParameters) {
+  const std::string original_path{"/users?overwrite-me=original"};
+  const std::string expected_path{"/users?overwrite-me=new"};
+  const std::vector<std::pair<std::string, std::string>> add_me{{"overwrite-me", "new"}};
+  const std::string remove_me{};
+  queryParameterTest(original_path, expected_path, add_me, remove_me);
+}
+
+TEST_P(HttpFilterTestParam, ImmediateOkResponseWithManyModifiedQueryParameters) {
+  const std::string original_path{"/users?remove-me=1&overwrite-me=2&leave-me=3"};
+  const std::string expected_path{"/users?add-me=9&leave-me=3&overwrite-me=new"};
+  const std::vector<std::pair<std::string, std::string>> add_me{{"add-me", "9"},
+                                                                {"overwrite-me", "new"}};
+  const std::string remove_me{"remove-me"};
+  queryParameterTest(original_path, expected_path, add_me, remove_me);
+}
+
 // Test that an synchronous denied response from the authorization service, on the call stack,
 // results in request not continuing.
 TEST_P(HttpFilterTestParam, ImmediateDeniedResponse) {