From 12230f0cbd48089832a300b4cbc60344627dd87c Mon Sep 17 00:00:00 2001 From: John Esmet Date: Tue, 31 Aug 2021 15:59:36 +0000 Subject: [PATCH 01/24] Add query_parameters_to_add Signed-off-by: John Esmet --- api/envoy/service/auth/v3/external_auth.proto | 18 ++++++- .../envoy/service/auth/v3/external_auth.proto | 18 ++++++- .../filters/common/ext_authz/ext_authz.h | 6 +++ .../common/ext_authz/ext_authz_grpc_impl.cc | 12 +++++ .../common/ext_authz/ext_authz_http_impl.cc | 24 ++++++--- .../filters/http/ext_authz/ext_authz.cc | 49 ++++++++++++++++++- 6 files changed, 117 insertions(+), 10 deletions(-) diff --git a/api/envoy/service/auth/v3/external_auth.proto b/api/envoy/service/auth/v3/external_auth.proto index b627fcb314751..3c4c196d912cb 100644 --- a/api/envoy/service/auth/v3/external_auth.proto +++ b/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, should be non-empty. + string key = 1; + + // The value of the query parameter, may be empty. + 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 ` // 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 potentially overwrite query + // string parameters on the original request before it is sent upstream. + repeated QueryParameterOption query_parameters_to_add = 7; } // Intended for gRPC and Network Authorization servers `only`. diff --git a/generated_api_shadow/envoy/service/auth/v3/external_auth.proto b/generated_api_shadow/envoy/service/auth/v3/external_auth.proto index b627fcb314751..3c4c196d912cb 100644 --- a/generated_api_shadow/envoy/service/auth/v3/external_auth.proto +++ b/generated_api_shadow/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, should be non-empty. + string key = 1; + + // The value of the query parameter, may be empty. + 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 ` // 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 potentially overwrite query + // string parameters on the original request before it is sent upstream. + repeated QueryParameterOption query_parameters_to_add = 7; } // Intended for gRPC and Network Authorization servers `only`. diff --git a/source/extensions/filters/common/ext_authz/ext_authz.h b/source/extensions/filters/common/ext_authz/ext_authz.h index 3d06846ed1123..a09e7952b2b34 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz.h +++ b/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 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 query_parameters_to_remove; // Optional http body used only on denied response. std::string body; // Optional http status used only on denied response. diff --git a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc index d462a5179572b..18b8f93c39850 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc @@ -57,6 +57,18 @@ void GrpcClientImpl::onSuccess(std::unique_ptrheaders_to_remove.push_back(Http::LowerCaseString(header)); } } + if (response->ok_response().query_parameters_to_add_size() > 0) { + for (const auto& query_parameter : response->ok_response().query_parameters_to_add()) { + // TODO(esmet): It might make more sense to store query_parameters_to_set as a vector + // instead of a map since we likely only need to lineaerly iterate them. + if (query_parameter.remove()) { + authz_response->query_parameters_to_set[query_parameter.key()] = + query_parameter.value(); + } else { + authz_response->query_parameters_to_remove.push_back(query_parameter.key()); + } + } + } 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( diff --git a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc index 4dba952fede7d..dfaaf58d58c15 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc +++ b/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(status_code), ProtobufWkt::Struct{}}}; diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index 29a6ca8be8437..4de37c088c21b 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/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 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_set.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_); } From a934913f30178ba430a5a4b4f6f2442177086956 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Mon, 6 Sep 2021 22:00:58 +0000 Subject: [PATCH 02/24] Fix format Signed-off-by: John Esmet --- api/envoy/service/auth/v3/external_auth.proto | 6 +- .../envoy/service/auth/v3/external_auth.proto | 6 +- .../filters/http/ext_authz/ext_authz.cc | 4 +- .../filters/http/ext_authz/ext_authz_test.cc | 101 ++++++++++++++++++ 4 files changed, 109 insertions(+), 8 deletions(-) diff --git a/api/envoy/service/auth/v3/external_auth.proto b/api/envoy/service/auth/v3/external_auth.proto index 3c4c196d912cb..c6973ec66b92e 100644 --- a/api/envoy/service/auth/v3/external_auth.proto +++ b/api/envoy/service/auth/v3/external_auth.proto @@ -62,10 +62,10 @@ message DeniedHttpResponse { // TODO: Should this be in the core API? message QueryParameterOption { - // The key of a the query parameter, should be non-empty. - string key = 1; + // 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. + // 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`. diff --git a/generated_api_shadow/envoy/service/auth/v3/external_auth.proto b/generated_api_shadow/envoy/service/auth/v3/external_auth.proto index 3c4c196d912cb..c6973ec66b92e 100644 --- a/generated_api_shadow/envoy/service/auth/v3/external_auth.proto +++ b/generated_api_shadow/envoy/service/auth/v3/external_auth.proto @@ -62,10 +62,10 @@ message DeniedHttpResponse { // TODO: Should this be in the core API? message QueryParameterOption { - // The key of a the query parameter, should be non-empty. - string key = 1; + // 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. + // 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`. diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index 4de37c088c21b..e71d663fe5a92 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/source/extensions/filters/http/ext_authz/ext_authz.cc @@ -286,7 +286,7 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { } } - if (!response->query_parameters_to_set.empty()) { + if (!response->query_parameters_to_remove.empty()) { if (!modified_query_parameters) { modified_query_parameters = Http::Utility::parseQueryString(request_headers_->Path()->value().getStringView()); @@ -309,7 +309,7 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { // 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); + 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); diff --git a/test/extensions/filters/http/ext_authz/ext_authz_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_test.cc index 18283f359bc95..7204c223ad057 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_test.cc @@ -70,6 +70,58 @@ template 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>& 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()) { + const Http::Utility::QueryParams query_parameters_to_add{}; + for (const auto &[key, value] : add_me) { + response.query_parameters_to_set[key] = value; + } + } + if (!remove_me.empty()) { + const std::vector query_parameters_to_remove{remove_me}; + response.query_parameters_to_remove = query_parameters_to_remove; + } + + auto response_ptr = std::make_unique(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_store_; envoy::config::bootstrap::v3::Bootstrap bootstrap_; FilterConfigSharedPtr config_; @@ -105,6 +157,7 @@ class HttpFilterTestParam : public HttpFilterTestBase> { public: void SetUp() override { initialize(""); } + }; template @@ -1776,6 +1829,54 @@ 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> 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> 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> 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> 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> 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> 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) { From cad93ec7213ccaaa30b2aa86c7d1437f1c3ca76e Mon Sep 17 00:00:00 2001 From: John Esmet Date: Mon, 6 Sep 2021 22:15:33 +0000 Subject: [PATCH 03/24] Fix format Signed-off-by: John Esmet --- .../filters/http/ext_authz/ext_authz_test.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/extensions/filters/http/ext_authz/ext_authz_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_test.cc index 7204c223ad057..5890e2cd8994b 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_test.cc @@ -88,7 +88,7 @@ template class HttpFilterTestBase : public T { if (!add_me.empty()) { const Http::Utility::QueryParams query_parameters_to_add{}; - for (const auto &[key, value] : add_me) { + for (const auto& [key, value] : add_me) { response.query_parameters_to_set[key] = value; } } @@ -101,8 +101,8 @@ template class HttpFilterTestBase : public T { EXPECT_CALL(*client_, check(_, _, _, _)) .WillOnce(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks, - const envoy::service::auth::v3::CheckRequest&, Tracing::Span&, - const StreamInfo::StreamInfo&) -> void { + 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); @@ -157,7 +157,6 @@ class HttpFilterTestParam : public HttpFilterTestBase> { public: void SetUp() override { initialize(""); } - }; template @@ -1872,7 +1871,8 @@ TEST_P(HttpFilterTestParam, ImmediateOkResponseWithOverwrittenQueryParameters) { 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> add_me{{"add-me", "9"}, {"overwrite-me", "new"}}; + const std::vector> add_me{{"add-me", "9"}, + {"overwrite-me", "new"}}; const std::string remove_me{"remove-me"}; queryParameterTest(original_path, expected_path, add_me, remove_me); } From 426a467ce2eeda35397917f2da7788250d13ea5e Mon Sep 17 00:00:00 2001 From: John Esmet Date: Mon, 6 Sep 2021 22:40:16 +0000 Subject: [PATCH 04/24] Clean up some things Signed-off-by: John Esmet --- api/envoy/service/auth/v3/external_auth.proto | 4 ++-- .../envoy/service/auth/v3/external_auth.proto | 4 ++-- .../filters/common/ext_authz/ext_authz_grpc_impl.cc | 10 +++++----- .../filters/http/ext_authz/ext_authz_test.cc | 1 - 4 files changed, 9 insertions(+), 10 deletions(-) diff --git a/api/envoy/service/auth/v3/external_auth.proto b/api/envoy/service/auth/v3/external_auth.proto index c6973ec66b92e..957826e793805 100644 --- a/api/envoy/service/auth/v3/external_auth.proto +++ b/api/envoy/service/auth/v3/external_auth.proto @@ -116,9 +116,9 @@ message OkHttpResponse { // 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 potentially overwrite query + // 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_add = 7; + repeated QueryParameterOption query_parameters_to_set = 7; } // Intended for gRPC and Network Authorization servers `only`. diff --git a/generated_api_shadow/envoy/service/auth/v3/external_auth.proto b/generated_api_shadow/envoy/service/auth/v3/external_auth.proto index c6973ec66b92e..957826e793805 100644 --- a/generated_api_shadow/envoy/service/auth/v3/external_auth.proto +++ b/generated_api_shadow/envoy/service/auth/v3/external_auth.proto @@ -116,9 +116,9 @@ message OkHttpResponse { // 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 potentially overwrite query + // 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_add = 7; + repeated QueryParameterOption query_parameters_to_set = 7; } // Intended for gRPC and Network Authorization servers `only`. diff --git a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc index 18b8f93c39850..f1ad646630891 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc @@ -57,15 +57,15 @@ void GrpcClientImpl::onSuccess(std::unique_ptrheaders_to_remove.push_back(Http::LowerCaseString(header)); } } - if (response->ok_response().query_parameters_to_add_size() > 0) { - for (const auto& query_parameter : response->ok_response().query_parameters_to_add()) { + 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 likely only need to lineaerly iterate them. + // 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(); - } else { - authz_response->query_parameters_to_remove.push_back(query_parameter.key()); } } } diff --git a/test/extensions/filters/http/ext_authz/ext_authz_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_test.cc index 5890e2cd8994b..1199f51e7f495 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_test.cc @@ -87,7 +87,6 @@ template class HttpFilterTestBase : public T { response.status = Filters::Common::ExtAuthz::CheckStatus::OK; if (!add_me.empty()) { - const Http::Utility::QueryParams query_parameters_to_add{}; for (const auto& [key, value] : add_me) { response.query_parameters_to_set[key] = value; } From 5c9c01e55668fd039d07441d620a401841866576 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Thu, 9 Sep 2021 16:41:02 +0000 Subject: [PATCH 05/24] Add a test for the grpc implementation. Signed-off-by: John Esmet --- .../ext_authz/ext_authz_grpc_impl_test.cc | 37 +++++++++++++++++++ .../filters/common/ext_authz/test_common.cc | 9 +++++ .../filters/common/ext_authz/test_common.h | 13 +++++++ 3 files changed, 59 insertions(+) diff --git a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc index 8d776a73b061f..cad925e709b7b 100644 --- a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc +++ b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc @@ -282,6 +282,43 @@ 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(); + auto status = check_response->mutable_status(); + + status->set_code(Grpc::Status::WellKnownGrpcStatus::Ok); + + const std::vector> query_parameters_to_set = { + {"add-me", "yes", false}, {"remove-me", "", true}}; + for (const auto& [key, value, remove]: 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); + query_parameter->set_remove(remove); + } + + // 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( + AuthzOkResponse(authz_response)))); + client_->onSuccess(std::move(check_response), span_); +} + } // namespace ExtAuthz } // namespace Common } // namespace Filters diff --git a/test/extensions/filters/common/ext_authz/test_common.cc b/test/extensions/filters/common/ext_authz/test_common.cc index 67429f891e2ae..88faeee30ed59 100644 --- a/test/extensions/filters/common/ext_authz/test_common.cc +++ b/test/extensions/filters/common/ext_authz/test_common.cc @@ -125,6 +125,15 @@ bool TestCommon::compareVectorOfHeaderName(const std::vector(rhs.begin(), rhs.end()); } +bool TestCommon::compareVectorOfUnorderedStrings(const std::vector& lhs, const std::vector& rhs) { + return std::set(lhs.begin(), lhs.end()) == std::set(rhs.begin(), rhs.end()); +} + +// TODO(esmet): This belongs in a QueryParams class +bool TestCommon::compareQueryParams(const Http::Utility::QueryParams& lhs, const Http::Utility::QueryParams& rhs) { + return lhs == rhs; +} + } // namespace ExtAuthz } // namespace Common } // namespace Filters diff --git a/test/extensions/filters/common/ext_authz/test_common.h b/test/extensions/filters/common/ext_authz/test_common.h index 5d1e72222713d..1c585ca6a6229 100644 --- a/test/extensions/filters/common/ext_authz/test_common.h +++ b/test/extensions/filters/common/ext_authz/test_common.h @@ -46,8 +46,11 @@ class TestCommon { static HeaderValueOptionVector makeHeaderValueOption(KeyValueOptionVector&& headers); static bool compareHeaderVector(const Http::HeaderVector& lhs, const Http::HeaderVector& rhs); + static bool compareQueryParams(const Http::Utility::QueryParams& lhs, const Http::Utility::QueryParams& rhs); static bool compareVectorOfHeaderName(const std::vector& lhs, const std::vector& rhs); + static bool compareVectorOfUnorderedStrings(const std::vector& lhs, + const std::vector& rhs); }; MATCHER_P(AuthzErrorResponse, status, "") { @@ -111,6 +114,16 @@ MATCHER_P(AuthzOkResponse, response, "") { return false; } + // Compare query_parameters_to_set. + if (!TestCommon::compareQueryParams(response.query_parameters_to_set, arg->query_parameters_to_set)) { + return false; + } + + // Compare query_parameters_to_remove. + if (!TestCommon::compareVectorOfUnorderedStrings(response.query_parameters_to_remove, arg->query_parameters_to_remove)) { + return false; + } + return TestCommon::compareVectorOfHeaderName(response.headers_to_remove, arg->headers_to_remove); } From 06a633854cdee87c1da59f9ca84218d93d3530bd Mon Sep 17 00:00:00 2001 From: John Esmet Date: Thu, 9 Sep 2021 16:44:03 +0000 Subject: [PATCH 06/24] Formatting Signed-off-by: John Esmet --- .../filters/common/ext_authz/ext_authz_grpc_impl_test.cc | 6 +++--- test/extensions/filters/common/ext_authz/test_common.cc | 9 ++++++--- test/extensions/filters/common/ext_authz/test_common.h | 9 ++++++--- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc index cad925e709b7b..71c293957ebe3 100644 --- a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc +++ b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc @@ -293,7 +293,7 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithQueryParameters) { const std::vector> query_parameters_to_set = { {"add-me", "yes", false}, {"remove-me", "", true}}; - for (const auto& [key, value, remove]: query_parameters_to_set) { + for (const auto& [key, value, remove] : 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); @@ -314,8 +314,8 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithQueryParameters) { client_->onCreateInitialMetadata(headers); EXPECT_CALL(span_, setTag(Eq("ext_authz_status"), Eq("ext_authz_ok"))); - EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo( - AuthzOkResponse(authz_response)))); + EXPECT_CALL(request_callbacks_, + onComplete_(WhenDynamicCastTo(AuthzOkResponse(authz_response)))); client_->onSuccess(std::move(check_response), span_); } diff --git a/test/extensions/filters/common/ext_authz/test_common.cc b/test/extensions/filters/common/ext_authz/test_common.cc index 88faeee30ed59..65e413c486db6 100644 --- a/test/extensions/filters/common/ext_authz/test_common.cc +++ b/test/extensions/filters/common/ext_authz/test_common.cc @@ -125,12 +125,15 @@ bool TestCommon::compareVectorOfHeaderName(const std::vector(rhs.begin(), rhs.end()); } -bool TestCommon::compareVectorOfUnorderedStrings(const std::vector& lhs, const std::vector& rhs) { - return std::set(lhs.begin(), lhs.end()) == std::set(rhs.begin(), rhs.end()); +bool TestCommon::compareVectorOfUnorderedStrings(const std::vector& lhs, + const std::vector& rhs) { + return std::set(lhs.begin(), lhs.end()) == + std::set(rhs.begin(), rhs.end()); } // TODO(esmet): This belongs in a QueryParams class -bool TestCommon::compareQueryParams(const Http::Utility::QueryParams& lhs, const Http::Utility::QueryParams& rhs) { +bool TestCommon::compareQueryParams(const Http::Utility::QueryParams& lhs, + const Http::Utility::QueryParams& rhs) { return lhs == rhs; } diff --git a/test/extensions/filters/common/ext_authz/test_common.h b/test/extensions/filters/common/ext_authz/test_common.h index 1c585ca6a6229..f213a66c89147 100644 --- a/test/extensions/filters/common/ext_authz/test_common.h +++ b/test/extensions/filters/common/ext_authz/test_common.h @@ -46,7 +46,8 @@ class TestCommon { static HeaderValueOptionVector makeHeaderValueOption(KeyValueOptionVector&& headers); static bool compareHeaderVector(const Http::HeaderVector& lhs, const Http::HeaderVector& rhs); - static bool compareQueryParams(const Http::Utility::QueryParams& lhs, const Http::Utility::QueryParams& rhs); + static bool compareQueryParams(const Http::Utility::QueryParams& lhs, + const Http::Utility::QueryParams& rhs); static bool compareVectorOfHeaderName(const std::vector& lhs, const std::vector& rhs); static bool compareVectorOfUnorderedStrings(const std::vector& lhs, @@ -115,12 +116,14 @@ MATCHER_P(AuthzOkResponse, response, "") { } // Compare query_parameters_to_set. - if (!TestCommon::compareQueryParams(response.query_parameters_to_set, arg->query_parameters_to_set)) { + if (!TestCommon::compareQueryParams(response.query_parameters_to_set, + arg->query_parameters_to_set)) { return false; } // Compare query_parameters_to_remove. - if (!TestCommon::compareVectorOfUnorderedStrings(response.query_parameters_to_remove, arg->query_parameters_to_remove)) { + if (!TestCommon::compareVectorOfUnorderedStrings(response.query_parameters_to_remove, + arg->query_parameters_to_remove)) { return false; } From b9fc09ee0c2d0e12f9f6ac83d19a71b2e29efe0b Mon Sep 17 00:00:00 2001 From: John Esmet Date: Fri, 10 Sep 2021 01:04:17 +0000 Subject: [PATCH 07/24] Add release notes Signed-off-by: John Esmet --- docs/root/version_history/current.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index c8e227d34b58d..1ca7945f8796c 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -100,12 +100,13 @@ New Features * access_log: added :ref:`METADATA` token to handle all types of metadata (DYNAMIC, CLUSTER, ROUTE). * bootstrap: added :ref:`inline_headers ` in the bootstrap to make custom inline headers bootstrap configurable. * contrib: added new :ref:`contrib images ` which contain contrib extensions. +* ext_authz: added :ref:`query_parameters_to_set ` and :ref:`query_parameters_to_remove ` for adding and removing query string parameters when using a gRPC authorization server. * grpc reverse bridge: added a new :ref:`option ` to support streaming response bodies when withholding gRPC frames from the upstream. * http: added :ref:`string_match ` in the header matcher. * http: added :ref:`x-envoy-upstream-stream-duration-ms ` that allows configuring the max stream duration via a request header. * http: added support for :ref:`max_requests_per_connection ` for both upstream and downstream connections. * http: sanitizing the referer header as documented :ref:`here `. This feature can be temporarily turned off by setting runtime guard ``envoy.reloadable_features.sanitize_http_header_referer`` to false. -* jwt_authn: added support for :ref:`Jwt Cache ` and its size can be specified by :ref:`jwt_cache_size `. +* jwt_authn: added support for :ref:`Jwt Cache ` an/Cd its size can be specified by :ref:`jwt_cache_size `. * jwt_authn: added support for extracting JWTs from request cookies using :ref:`from_cookies `. * listener: new listener metric ``downstream_cx_transport_socket_connect_timeout`` to track transport socket timeouts. * matcher: added :ref:`invert ` for inverting the match result in the metadata matcher. From 251655eb4fdc6eb7d10882696b3235179cdde9c9 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Fri, 10 Sep 2021 01:23:59 +0000 Subject: [PATCH 08/24] Fix labels in release notes Signed-off-by: John Esmet --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 1ca7945f8796c..f4f2ea1d9d1f9 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -100,7 +100,7 @@ New Features * access_log: added :ref:`METADATA` token to handle all types of metadata (DYNAMIC, CLUSTER, ROUTE). * bootstrap: added :ref:`inline_headers ` in the bootstrap to make custom inline headers bootstrap configurable. * contrib: added new :ref:`contrib images ` which contain contrib extensions. -* ext_authz: added :ref:`query_parameters_to_set ` and :ref:`query_parameters_to_remove ` for adding and removing query string parameters when using a gRPC authorization server. +* ext_authz: added :ref:`query_parameters_to_set ` and :ref:`query_parameters_to_remove ` for adding and removing query string parameters when using a gRPC authorization server. * grpc reverse bridge: added a new :ref:`option ` to support streaming response bodies when withholding gRPC frames from the upstream. * http: added :ref:`string_match ` in the header matcher. * http: added :ref:`x-envoy-upstream-stream-duration-ms ` that allows configuring the max stream duration via a request header. From 7686b3c0c97cb3137a4cdcb29ca7006727e99456 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Mon, 13 Sep 2021 15:19:48 +0000 Subject: [PATCH 09/24] Try fixing labels again Signed-off-by: John Esmet --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index f4f2ea1d9d1f9..4a63ae7be1711 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -100,7 +100,7 @@ New Features * access_log: added :ref:`METADATA` token to handle all types of metadata (DYNAMIC, CLUSTER, ROUTE). * bootstrap: added :ref:`inline_headers ` in the bootstrap to make custom inline headers bootstrap configurable. * contrib: added new :ref:`contrib images ` which contain contrib extensions. -* ext_authz: added :ref:`query_parameters_to_set ` and :ref:`query_parameters_to_remove ` for adding and removing query string parameters when using a gRPC authorization server. +* ext_authz: added :ref:`query_parameters_to_set ` and :ref:`query_parameters_to_remove ` for adding and removing query string parameters when using a gRPC authorization server. * grpc reverse bridge: added a new :ref:`option ` to support streaming response bodies when withholding gRPC frames from the upstream. * http: added :ref:`string_match ` in the header matcher. * http: added :ref:`x-envoy-upstream-stream-duration-ms ` that allows configuring the max stream duration via a request header. From 9edf4c0dfb2547e705bf784573593431ccd68f7d Mon Sep 17 00:00:00 2001 From: John Esmet Date: Mon, 13 Sep 2021 22:55:29 +0000 Subject: [PATCH 10/24] Fix one more time Signed-off-by: John Esmet --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 4a63ae7be1711..80f5f2963e4ae 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -100,7 +100,7 @@ New Features * access_log: added :ref:`METADATA` token to handle all types of metadata (DYNAMIC, CLUSTER, ROUTE). * bootstrap: added :ref:`inline_headers ` in the bootstrap to make custom inline headers bootstrap configurable. * contrib: added new :ref:`contrib images ` which contain contrib extensions. -* ext_authz: added :ref:`query_parameters_to_set ` and :ref:`query_parameters_to_remove ` for adding and removing query string parameters when using a gRPC authorization server. +* ext_authz: added :ref:`query_parameters_to_set ` and :ref:`query_parameters_to_remove ` for adding and removing query string parameters when using a gRPC authorization server. * grpc reverse bridge: added a new :ref:`option ` to support streaming response bodies when withholding gRPC frames from the upstream. * http: added :ref:`string_match ` in the header matcher. * http: added :ref:`x-envoy-upstream-stream-duration-ms ` that allows configuring the max stream duration via a request header. From b2b3dec0ec64d4f09b083e82a15598ee411fe474 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Tue, 21 Sep 2021 15:13:27 +0000 Subject: [PATCH 11/24] Add QueryParamsVector Signed-off-by: John Esmet --- envoy/http/query_params.h | 1 + source/extensions/filters/common/ext_authz/ext_authz.h | 2 +- .../filters/common/ext_authz/ext_authz_grpc_impl.cc | 5 +---- source/extensions/filters/http/ext_authz/ext_authz.cc | 2 +- test/extensions/filters/common/ext_authz/test_common.cc | 7 ++++--- test/extensions/filters/common/ext_authz/test_common.h | 8 ++++---- 6 files changed, 12 insertions(+), 13 deletions(-) diff --git a/envoy/http/query_params.h b/envoy/http/query_params.h index d30ae58b1ab36..3c81e44abb92b 100644 --- a/envoy/http/query_params.h +++ b/envoy/http/query_params.h @@ -12,6 +12,7 @@ namespace Utility { // https://github.com/apache/incubator-pagespeed-mod/blob/master/pagespeed/kernel/http/query_params.h using QueryParams = std::map; +using QueryParamsVector = std::vector>; } // namespace Utility } // namespace Http diff --git a/source/extensions/filters/common/ext_authz/ext_authz.h b/source/extensions/filters/common/ext_authz/ext_authz.h index a09e7952b2b34..28c040a013b8a 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz.h +++ b/source/extensions/filters/common/ext_authz/ext_authz.h @@ -84,7 +84,7 @@ struct Response { std::vector 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; + Http::Utility::QueryParamsVector query_parameters_to_set; // A set of query string parameters to remove from the request to the upstream server. std::vector query_parameters_to_remove; // Optional http body used only on denied response. diff --git a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc index 749b029f7aaa9..29334e4a937ad 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc @@ -59,13 +59,10 @@ void GrpcClientImpl::onSuccess(std::unique_ptrok_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(); + authz_response->query_parameters_to_set.push_back(std::pair(query_parameter.key(), query_parameter.value())); } } } diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index e71d663fe5a92..ae9c682604f44 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/source/extensions/filters/http/ext_authz/ext_authz.cc @@ -282,7 +282,7 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { 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; + (*modified_query_parameters).push_back(std::pair(key, value)); } } diff --git a/test/extensions/filters/common/ext_authz/test_common.cc b/test/extensions/filters/common/ext_authz/test_common.cc index 65e413c486db6..3714ecd1d8e01 100644 --- a/test/extensions/filters/common/ext_authz/test_common.cc +++ b/test/extensions/filters/common/ext_authz/test_common.cc @@ -132,9 +132,10 @@ bool TestCommon::compareVectorOfUnorderedStrings(const std::vector& } // TODO(esmet): This belongs in a QueryParams class -bool TestCommon::compareQueryParams(const Http::Utility::QueryParams& lhs, - const Http::Utility::QueryParams& rhs) { - return lhs == rhs; +bool TestCommon::compareQueryParamsVector(const Http::Utility::QueryParamsVector& lhs, + const Http::Utility::QueryParamsVector& rhs) { + return std::set>(lhs.begin(), lhs.end()) == + std::set>(rhs.begin(), rhs.end()); } } // namespace ExtAuthz diff --git a/test/extensions/filters/common/ext_authz/test_common.h b/test/extensions/filters/common/ext_authz/test_common.h index f213a66c89147..0b058b67d42d1 100644 --- a/test/extensions/filters/common/ext_authz/test_common.h +++ b/test/extensions/filters/common/ext_authz/test_common.h @@ -46,8 +46,8 @@ class TestCommon { static HeaderValueOptionVector makeHeaderValueOption(KeyValueOptionVector&& headers); static bool compareHeaderVector(const Http::HeaderVector& lhs, const Http::HeaderVector& rhs); - static bool compareQueryParams(const Http::Utility::QueryParams& lhs, - const Http::Utility::QueryParams& rhs); + static bool compareQueryParamsVector(const Http::Utility::QueryParamsVector& lhs, + const Http::Utility::QueryParamsVector& rhs); static bool compareVectorOfHeaderName(const std::vector& lhs, const std::vector& rhs); static bool compareVectorOfUnorderedStrings(const std::vector& lhs, @@ -116,8 +116,8 @@ MATCHER_P(AuthzOkResponse, response, "") { } // Compare query_parameters_to_set. - if (!TestCommon::compareQueryParams(response.query_parameters_to_set, - arg->query_parameters_to_set)) { + if (!TestCommon::compareQueryParamsVector(response.query_parameters_to_set, + arg->query_parameters_to_set)) { return false; } From e67a46e726ef81dfcfcc56bd3185f53c4e43888d Mon Sep 17 00:00:00 2001 From: John Esmet Date: Tue, 21 Sep 2021 15:14:54 +0000 Subject: [PATCH 12/24] Format Signed-off-by: John Esmet --- .../extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc index 29334e4a937ad..2fc996629610f 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc @@ -62,7 +62,8 @@ void GrpcClientImpl::onSuccess(std::unique_ptrquery_parameters_to_remove.push_back(query_parameter.key()); } else { - authz_response->query_parameters_to_set.push_back(std::pair(query_parameter.key(), query_parameter.value())); + authz_response->query_parameters_to_set.push_back( + std::pair(query_parameter.key(), query_parameter.value())); } } } From 95872b4a17333544d43cfa554ab514ca3a392f7e Mon Sep 17 00:00:00 2001 From: John Esmet Date: Tue, 21 Sep 2021 16:31:37 -0400 Subject: [PATCH 13/24] Refactor towards replaceQueryString, add unit tests for it and stripQueryString as well. Signed-off-by: John Esmet --- source/common/http/utility.cc | 14 ++++++ source/common/http/utility.h | 16 +++++++ .../filters/http/ext_authz/ext_authz.cc | 16 ++----- test/common/http/utility_test.cc | 43 +++++++++++++++++++ .../filters/http/ext_authz/ext_authz_test.cc | 41 +++++++----------- 5 files changed, 93 insertions(+), 37 deletions(-) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 9a803b9d8e906..42d0006faefed 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -463,6 +463,20 @@ 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); diff --git a/source/common/http/utility.h b/source/common/http/utility.h index d2ba67613c35d..186674e0ccf01 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -247,6 +247,21 @@ 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. @@ -459,6 +474,7 @@ RequestMessagePtr prepareHeaders(const envoy::config::core::v3::HttpUri& http_ur */ std::string queryParamsToString(const QueryParams& query_params); + /** * Returns string representation of StreamResetReason. */ diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index ae9c682604f44..33d757d944a10 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/source/extensions/filters/http/ext_authz/ext_authz.cc @@ -280,9 +280,7 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { 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).push_back(std::pair(key, value)); + (*modified_query_parameters)[key] = value; } } @@ -302,16 +300,10 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { // 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); + 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, using new path for request: {}", + "ext_authz filter modified query parameter(s), using new path for request: {}", *decoder_callbacks_, new_path); request_headers_->setPath(new_path); } diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 766fd52200b57..58248f028ec18 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -91,6 +91,49 @@ 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"}})); diff --git a/test/extensions/filters/http/ext_authz/ext_authz_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_test.cc index 1199f51e7f495..b725d802b15a1 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_test.cc @@ -71,8 +71,8 @@ template class HttpFilterTestBase : public T { } void queryParameterTest(const std::string& original_path, const std::string& expected_path, - const std::vector>& add_me, - const std::string& remove_me) { + const Http::Utility::QueryParamsVector& add_me, + const std::vector& remove_me) { InSequence s; // Set up all the typical headers plus a path with a query string that we'll remove later. @@ -85,16 +85,8 @@ template class HttpFilterTestBase : public T { 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 query_parameters_to_remove{remove_me}; - response.query_parameters_to_remove = query_parameters_to_remove; - } + response.query_parameters_to_set = add_me; + response.query_parameters_to_remove = remove_me; auto response_ptr = std::make_unique(response); @@ -1830,49 +1822,48 @@ TEST_P(HttpFilterTestParam, ImmediateOkResponseWithHttpAttributes) { 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> add_me{}; - const std::string remove_me{"remove-me"}; + const Http::Utility::QueryParamsVector add_me{}; + const std::vector 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> add_me{{"add-me", "123"}}; - const std::string remove_me{}; + const Http::Utility::QueryParamsVector add_me{{"add-me", "123"}}; + const std::vector 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> add_me{{"add-me", "456"}}; - const std::string remove_me{"remove-me"}; + const Http::Utility::QueryParamsVector add_me{{"add-me", "456"}}; + const std::vector 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> add_me{}; - const std::string remove_me{"remove-me"}; + const Http::Utility::QueryParamsVector add_me{}; + const std::vector 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> add_me{{"overwrite-me", "new"}}; - const std::string remove_me{}; + const Http::Utility::QueryParamsVector add_me{{"overwrite-me", "new"}}; + const std::vector 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> add_me{{"add-me", "9"}, - {"overwrite-me", "new"}}; - const std::string remove_me{"remove-me"}; + const Http::Utility::QueryParamsVector add_me{{"add-me", "9"}, {"overwrite-me", "new"}}; + const std::vector remove_me{{"remove-me"}}; queryParameterTest(original_path, expected_path, add_me, remove_me); } From 68185f1a7a47b0e5b35413109f9b341e3855c4ed Mon Sep 17 00:00:00 2001 From: John Esmet Date: Tue, 21 Sep 2021 16:38:25 -0400 Subject: [PATCH 14/24] Formatting Signed-off-by: John Esmet --- source/common/http/utility.cc | 4 +- source/common/http/utility.h | 8 ++-- .../filters/http/ext_authz/ext_authz.cc | 10 ++--- test/common/http/utility_test.cc | 40 ++++++++++++++----- 4 files changed, 38 insertions(+), 24 deletions(-) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 42d0006faefed..a8e8aef21392d 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -468,15 +468,13 @@ std::string Utility::replaceQueryString(const HeaderString& path, std::string new_path{Http::Utility::stripQueryString(path)}; if (!params.empty()) { - const auto new_query_string = - Http::Utility::queryParamsToString(params); + 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); diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 186674e0ccf01..4c0e073de5afd 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -249,18 +249,17 @@ 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); +std::string replaceQueryString(const HeaderString& path, const QueryParams& params); /** * Parse a particular value out of a cookie @@ -474,7 +473,6 @@ RequestMessagePtr prepareHeaders(const envoy::config::core::v3::HttpUri& http_ur */ std::string queryParamsToString(const QueryParams& query_params); - /** * Returns string representation of StreamResetReason. */ diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index 33d757d944a10..358406a06e7e1 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/source/extensions/filters/http/ext_authz/ext_authz.cc @@ -300,11 +300,11 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { // 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); + 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); } diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 58248f028ec18..a8fabb277c35a 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -119,19 +119,37 @@ TEST(HttpUtility, replaceQueryString) { 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"); + 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"); + 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"); + 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) { From 2d63f10ab2b839e01294d2c86de5a6ec85090f5e Mon Sep 17 00:00:00 2001 From: John Esmet Date: Tue, 21 Sep 2021 17:59:36 -0400 Subject: [PATCH 15/24] tools: fix protoprint to respect CLANG_FORMAT env var Signed-off-by: John Esmet --- tools/protoxform/protoprint.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/protoxform/protoprint.py b/tools/protoxform/protoprint.py index b30058b37a68e..45dcfea7ce612 100755 --- a/tools/protoxform/protoprint.py +++ b/tools/protoxform/protoprint.py @@ -82,8 +82,9 @@ def clang_format(contents): Returns: clang-formatted string """ + clang_format_path = os.getenv("CLANG_FORMAT", "clang-format-11") return subprocess.run( - ['clang-format', + [clang_format_path, '--style=%s' % CLANG_FORMAT_STYLE, '--assume-filename=.proto'], input=contents.encode('utf-8'), stdout=subprocess.PIPE).stdout From 08ba422646420807b12bbe6eb0e230907c27ecaa Mon Sep 17 00:00:00 2001 From: John Esmet Date: Tue, 21 Sep 2021 19:24:09 -0400 Subject: [PATCH 16/24] Fix changelog Signed-off-by: John Esmet --- docs/root/version_history/current.rst | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index b87a81fed0925..ab98debf90747 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -103,8 +103,8 @@ New Features * access_log: added :ref:`METADATA` token to handle all types of metadata (DYNAMIC, CLUSTER, ROUTE). * bootstrap: added :ref:`inline_headers ` in the bootstrap to make custom inline headers bootstrap configurable. * contrib: added new :ref:`contrib images ` which contain contrib extensions. -* ext_authz: added :ref:`query_parameters_to_set ` and :ref:`query_parameters_to_remove ` for adding and removing query string parameters when using a gRPC authorization server. * dns: added :ref:`V4_PREFERRED ` option to return V6 addresses only if V4 addresses are not available. +* ext_authz: added :ref:`query_parameters_to_set ` and :ref:`query_parameters_to_remove ` for adding and removing query string parameters when using a gRPC authorization server. * grpc reverse bridge: added a new :ref:`option ` to support streaming response bodies when withholding gRPC frames from the upstream. * http: added cluster_header in :ref:`weighted_clusters ` to allow routing to the weighted cluster specified in the request_header. * http: added :ref:`alternate_protocols_cache_options ` for enabling HTTP/3 connections to servers which advertise HTTP/3 support via `HTTP Alternative Services `_. @@ -112,7 +112,6 @@ New Features * http: added :ref:`x-envoy-upstream-stream-duration-ms ` that allows configuring the max stream duration via a request header. * http: added support for :ref:`max_requests_per_connection ` for both upstream and downstream connections. * http: sanitizing the referer header as documented :ref:`here `. This feature can be temporarily turned off by setting runtime guard ``envoy.reloadable_features.sanitize_http_header_referer`` to false. -* jwt_authn: added support for :ref:`Jwt Cache ` an/Cd its size can be specified by :ref:`jwt_cache_size `. * http: validating outgoing HTTP/2 CONNECT requests to ensure that if ``:path`` is set that ``:protocol`` is present. This behavior can be temporarily turned off by setting runtime guard ``envoy.reloadable_features.validate_connect`` to false. * jwt_authn: added support for :ref:`Jwt Cache ` and its size can be specified by :ref:`jwt_cache_size `. * jwt_authn: added support for extracting JWTs from request cookies using :ref:`from_cookies `. From 617f90e6b92d77216b770eb7a2e9e173534d2493 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Tue, 21 Sep 2021 20:14:24 -0400 Subject: [PATCH 17/24] Add query_parameters_to_remove, move QueryParameter to core API Signed-off-by: John Esmet --- api/envoy/config/core/v3/base.proto | 9 ++++++++ api/envoy/service/auth/v3/external_auth.proto | 21 +++++++------------ .../common/ext_authz/ext_authz_grpc_impl.cc | 9 ++++---- .../ext_authz/ext_authz_grpc_impl_test.cc | 11 ++++++---- 4 files changed, 28 insertions(+), 22 deletions(-) diff --git a/api/envoy/config/core/v3/base.proto b/api/envoy/config/core/v3/base.proto index d6c507b8dec9a..2259761211492 100644 --- a/api/envoy/config/core/v3/base.proto +++ b/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. + 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"; diff --git a/api/envoy/service/auth/v3/external_auth.proto b/api/envoy/service/auth/v3/external_auth.proto index 899395735f59f..95a02c63b19c5 100644 --- a/api/envoy/service/auth/v3/external_auth.proto +++ b/api/envoy/service/auth/v3/external_auth.proto @@ -60,20 +60,8 @@ 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: 8] +// [#next-free-field: 9] message OkHttpResponse { option (udpa.annotations.versioning).previous_message_type = "envoy.service.auth.v2.OkHttpResponse"; @@ -118,7 +106,12 @@ message OkHttpResponse { // 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; + 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`. diff --git a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc index 2fc996629610f..7c1454c41f7b0 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc @@ -59,12 +59,13 @@ void GrpcClientImpl::onSuccess(std::unique_ptrok_response().query_parameters_to_set_size() > 0) { for (const auto& query_parameter : response->ok_response().query_parameters_to_set()) { - if (query_parameter.remove()) { - authz_response->query_parameters_to_remove.push_back(query_parameter.key()); - } else { 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); } } if (response->ok_response().response_headers_to_add_size() > 0) { diff --git a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc index e950fe7c7b3bc..57a9fd6906753 100644 --- a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc +++ b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc @@ -324,13 +324,16 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithQueryParameters) { status->set_code(Grpc::Status::WellKnownGrpcStatus::Ok); - const std::vector> query_parameters_to_set = { - {"add-me", "yes", false}, {"remove-me", "", true}}; - for (const auto& [key, value, remove] : query_parameters_to_set) { + 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); - query_parameter->set_remove(remove); + } + + const std::vector 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. From 32786f4965372ccc6ac09554656f097e9c9db661 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Thu, 23 Sep 2021 12:08:57 -0400 Subject: [PATCH 18/24] Formatting Signed-off-by: John Esmet --- .../filters/common/ext_authz/ext_authz_grpc_impl.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc index 7c1454c41f7b0..f166fe5244e40 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.cc @@ -59,13 +59,13 @@ void GrpcClientImpl::onSuccess(std::unique_ptrok_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())); + 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); + authz_response->query_parameters_to_remove.push_back(key); } } if (response->ok_response().response_headers_to_add_size() > 0) { From 427be585b1aef5f0ebfed90e55fe34ce72c7c447 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Tue, 28 Sep 2021 16:29:54 -0400 Subject: [PATCH 19/24] Docs fixups Signed-off-by: John Esmet --- api/envoy/config/core/v3/base.proto | 2 +- docs/root/version_history/current.rst | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/api/envoy/config/core/v3/base.proto b/api/envoy/config/core/v3/base.proto index e8e26c8acdd73..efa8ec5186f46 100644 --- a/api/envoy/config/core/v3/base.proto +++ b/api/envoy/config/core/v3/base.proto @@ -298,7 +298,7 @@ message RuntimeFeatureFlag { // Query parameter name/value pair. message QueryParameter { - // The key of the query parameter. + // The key of the query parameter. Case sensitive. string key = 1 [(validate.rules).string = {min_len: 1}]; // The value of the query parameter. diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 6aa70cd7cac1d..b9c04239c0d41 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -105,7 +105,7 @@ New Features * bootstrap: added :ref:`inline_headers ` in the bootstrap to make custom inline headers bootstrap configurable. * contrib: added new :ref:`contrib images ` which contain contrib extensions. * dns: added :ref:`V4_PREFERRED ` option to return V6 addresses only if V4 addresses are not available. -* ext_authz: added :ref:`query_parameters_to_set ` and :ref:`query_parameters_to_remove ` for adding and removing query string parameters when using a gRPC authorization server. +* ext_authz: added :ref:`query_parameters_to_set ` and :ref:`query_parameters_to_remove ` for adding and removing query string parameters when using a gRPC authorization server. * ext_authz: added :ref:`dynamic_metadata_from_headers ` to support emitting dynamic metadata from headers returned by an external authorization service via HTTP. * grpc reverse bridge: added a new :ref:`option ` to support streaming response bodies when withholding gRPC frames from the upstream. * http: added cluster_header in :ref:`weighted_clusters ` to allow routing to the weighted cluster specified in the request_header. From a2674e37f1ddbbad9f697ad5e0dc5094a99b3694 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Wed, 6 Oct 2021 15:38:21 -0400 Subject: [PATCH 20/24] Fix initialization bugs Signed-off-by: John Esmet --- .../filters/common/ext_authz/ext_authz_http_impl.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc index beff67355ac7d..6ea634396894a 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc @@ -38,8 +38,8 @@ const Response& errorResponse() { Http::HeaderVector{}, Http::HeaderVector{}, {{}}, - {{}}, - {{}}, + Http::Utility::QueryParamsVector{}, + {}, EMPTY_STRING, Http::Code::Forbidden, ProtobufWkt::Struct{}}); @@ -359,8 +359,8 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) { Http::HeaderVector{}, Http::HeaderVector{}, std::move(headers_to_remove), - {{}}, - {{}}, + Http::Utility::QueryParamsVector{}, + {}, EMPTY_STRING, Http::Code::OK, ProtobufWkt::Struct{}}}; @@ -380,8 +380,8 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) { Http::HeaderVector{}, Http::HeaderVector{}, {{}}, - {{}}, - {{}}, + Http::Utility::QueryParamsVector{}, + {}, message->bodyAsString(), static_cast(status_code), ProtobufWkt::Struct{}}}; From 19383d6822402fc2e3ac2532f983fdfe2ff42f1d Mon Sep 17 00:00:00 2001 From: John Esmet Date: Wed, 6 Oct 2021 19:52:03 -0400 Subject: [PATCH 21/24] Fix bad merge on the changelog Signed-off-by: John Esmet --- docs/root/version_history/current.rst | 39 --------------------------- 1 file changed, 39 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 53908f2977c21..a5312a13c5563 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -23,46 +23,7 @@ Removed Config or Runtime New Features ------------ -* access_log: added :ref:`METADATA` token to handle all types of metadata (DYNAMIC, CLUSTER, ROUTE). -* bootstrap: added :ref:`inline_headers ` in the bootstrap to make custom inline headers bootstrap configurable. -* contrib: added new :ref:`contrib images ` which contain contrib extensions. -* dns: added :ref:`V4_PREFERRED ` option to return V6 addresses only if V4 addresses are not available. * ext_authz: added :ref:`query_parameters_to_set ` and :ref:`query_parameters_to_remove ` for adding and removing query string parameters when using a gRPC authorization server. -* ext_authz: added :ref:`dynamic_metadata_from_headers ` to support emitting dynamic metadata from headers returned by an external authorization service via HTTP. -* grpc reverse bridge: added a new :ref:`option ` to support streaming response bodies when withholding gRPC frames from the upstream. -* grpc_json_transcoder: added support to unescape '+' in query parameters to space with a new config field :ref:`query_param_unescape_plus `. -* http: added cluster_header in :ref:`weighted_clusters ` to allow routing to the weighted cluster specified in the request_header. -* http: added :ref:`alternate_protocols_cache_options ` for enabling HTTP/3 connections to servers which advertise HTTP/3 support via `HTTP Alternative Services `_ and caching the advertisements to disk. -* http: added :ref:`string_match ` in the header matcher. -* http: added :ref:`x-envoy-upstream-stream-duration-ms ` that allows configuring the max stream duration via a request header. -* http: added support for :ref:`max_requests_per_connection ` for both upstream and downstream connections. -* http: sanitizing the referer header as documented :ref:`here `. This feature can be temporarily turned off by setting runtime guard ``envoy.reloadable_features.sanitize_http_header_referer`` to false. -* http: validating outgoing HTTP/2 CONNECT requests to ensure that if ``:path`` is set that ``:protocol`` is present. This behavior can be temporarily turned off by setting runtime guard ``envoy.reloadable_features.validate_connect`` to false. -* jwt_authn: added support for :ref:`Jwt Cache ` and its size can be specified by :ref:`jwt_cache_size `. -* jwt_authn: added support for extracting JWTs from request cookies using :ref:`from_cookies `. -* jwt_authn: added support for setting the extracted headers from a successfully verified JWT using :ref:`header_in_metadata ` to dynamic metadata. -* listener: new listener metric ``downstream_cx_transport_socket_connect_timeout`` to track transport socket timeouts. -* lua: added ``header:getAtIndex()`` and ``header:getNumValues()`` methods to :ref:`header object ` for retrieving the value of a header at certain index and get the total number of values for a given header. -* matcher: added :ref:`invert ` for inverting the match result in the metadata matcher. -* overload: add a new overload action that resets streams using a lot of memory. To enable the tracking of allocated bytes in buffers that a stream is using we need to configure the minimum threshold for tracking via:ref:`buffer_factory_config `. We have an overload action ``Envoy::Server::OverloadActionNameValues::ResetStreams`` that takes advantage of the tracking to reset the most expensive stream first. -* rbac: added :ref:`destination_port_range ` for matching range of destination ports. -* rbac: added :ref:`matcher` along with extension category ``extension_category_envoy.rbac.matchers`` for custom RBAC permission matchers. Added reference implementation for matchers :ref:`envoy.rbac.matchers.upstream_ip_port `. -* route config: added :ref:`dynamic_metadata ` for routing based on dynamic metadata. -* router: added retry options predicate extensions configured via - :ref:` `. These - extensions allow modification of requests between retries at the router level. There are not - currently any built-in extensions that implement this extension point. -* router: added :ref:`per_try_idle_timeout ` timeout configuration. -* router: added an optional :ref:`override_auto_sni_header ` to support setting SNI value from an arbitrary header other than host/authority. -* sxg_filter: added filter to transform response to SXG package to :ref:`contrib images `. This can be enabled by setting :ref:`SXG ` configuration. -* thrift_proxy: added support for :ref:`mirroring requests `. -* udp: allows updating filter chain in-place through LDS, which is supported by Quic listener. Such listener config will be rejected in other connection-less UDP listener implementations. It can be reverted by ``envoy.reloadable_features.udp_listener_updates_filter_chain_in_place``. -* udp: disallow L4 filter chain in config which configures connection-less UDP listener. It can be reverted by ``envoy.reloadable_features.udp_listener_updates_filter_chain_in_place``. -* upstream: added support for :ref:`slow start mode `, which allows to progresively increase traffic for new endpoints. -* upstream: extended :ref:`Round Robin load balancer configuration ` with :ref:`slow start ` support. -* upstream: extended :ref:`Least Request load balancer configuration ` with :ref:`slow start ` support. -* windows: added a new container image based on Windows Nanoserver 2022. -* xray: request direction (``ingress`` or ``egress``) is recorded as X-Ray trace segment's annotation by name ``direction``. Deprecated ---------- From 7b529294c65444318f06faa2a2c93bcb3836d9e0 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Wed, 6 Oct 2021 22:01:26 -0400 Subject: [PATCH 22/24] Fix include in query_params.h Signed-off-by: John Esmet --- envoy/http/query_params.h | 1 + 1 file changed, 1 insertion(+) diff --git a/envoy/http/query_params.h b/envoy/http/query_params.h index 3c81e44abb92b..9cee9dc650c46 100644 --- a/envoy/http/query_params.h +++ b/envoy/http/query_params.h @@ -1,6 +1,7 @@ #pragma once #include +#include #include namespace Envoy { From 883753696fc2fc78215c8cdb4087fd0060b44c10 Mon Sep 17 00:00:00 2001 From: John Esmet Date: Wed, 6 Oct 2021 22:01:57 -0400 Subject: [PATCH 23/24] Fix order Signed-off-by: John Esmet --- envoy/http/query_params.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/envoy/http/query_params.h b/envoy/http/query_params.h index 9cee9dc650c46..e500f93ca3c86 100644 --- a/envoy/http/query_params.h +++ b/envoy/http/query_params.h @@ -1,8 +1,8 @@ #pragma once #include -#include #include +#include namespace Envoy { namespace Http { From a9d7f5a9a947aa943f64765baed65671a5dd4fcd Mon Sep 17 00:00:00 2001 From: John Esmet Date: Fri, 8 Oct 2021 10:25:20 -0400 Subject: [PATCH 24/24] More test cases Signed-off-by: John Esmet --- test/common/http/utility_test.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 023702055929c..e602f2108df0c 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -95,9 +95,11 @@ 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("/?x=1&y=2")), "/"); 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?hello=there&good=bye")), "/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");