diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 2ab9a004225ab..6dbe0915e7b71 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -22,6 +22,12 @@ minor_behavior_changes: bug_fixes: # *Changes expected to improve the state of the world and are unlikely to have negative effects* +- area: ext_authz + change: | + Added support for the ``append_action`` enum in gRPC ExtAuthz ``OkHttpResponse.headers`` for upstream request header + mutations. Previously, only the deprecated ``append`` boolean was checked. Now ``APPEND_IF_EXISTS_OR_ADD``, + ``ADD_IF_ABSENT``, ``OVERWRITE_IF_EXISTS``, and ``OVERWRITE_IF_EXISTS_OR_ADD`` actions are fully supported, + providing parity with ``response_headers_to_add`` handling. - area: http change: | Fixed a potential file descriptor leak where HTTP/1.1 connections with zombie streams (waiting for codec completion) diff --git a/source/extensions/filters/common/ext_authz/ext_authz.h b/source/extensions/filters/common/ext_authz/ext_authz.h index 208cbe12b6973..126663be8fbb6 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz.h +++ b/source/extensions/filters/common/ext_authz/ext_authz.h @@ -78,6 +78,22 @@ enum class CheckStatus { using UnsafeHeader = std::pair; using UnsafeHeaderVector = std::vector; + +using HeaderAppendAction = envoy::config::core::v3::HeaderValueOption::HeaderAppendAction; +using HeaderValueOption = envoy::config::core::v3::HeaderValueOption; + +// A single header mutation with its action type, preserving order from the proto. +struct HeaderMutation { + std::string key; + std::string value; + HeaderAppendAction append_action; + // True if this mutation came from the deprecated 'append' boolean field. + // When true and append_action is APPEND_IF_EXISTS_OR_ADD, use appendCopy() instead + // of addCopy() for backward compatibility. + bool from_deprecated_append{false}; +}; +using HeaderMutationVector = std::vector; + /** * Authorization response object for a RequestCallback. * @@ -87,30 +103,16 @@ using UnsafeHeaderVector = std::vector; struct Response { // Call status. CheckStatus status; - // A set of HTTP headers returned by the authorization server, that will be optionally appended - // to the request to the upstream server. - UnsafeHeaderVector headers_to_append{}; - // A set of HTTP headers returned by the authorization server, will be optionally set - // (using "setCopy") to the request to the upstream server. - UnsafeHeaderVector headers_to_set{}; - // A set of HTTP headers returned by the authorization server, will be optionally added - // (using "addCopy") to the request to the upstream server. - UnsafeHeaderVector headers_to_add{}; - // A set of HTTP headers returned by the authorization server, will be optionally added - // (using "addCopy") to the response sent back to the downstream client on OK auth - // responses. - UnsafeHeaderVector response_headers_to_add{}; - // A set of HTTP headers returned by the authorization server, will be optionally set (using - // "setCopy") to the response sent back to the downstream client on OK auth responses. - UnsafeHeaderVector response_headers_to_set{}; - // A set of HTTP headers returned by the authorization server, will be optionally added - // (using "addCopy") to the response sent back to the downstream client on OK auth - // responses only if the headers were not returned from the authz server. - UnsafeHeaderVector response_headers_to_add_if_absent{}; - // A set of HTTP headers returned by the authorization server, will be optionally set (using - // "setCopy") to the response sent back to the downstream client on OK auth responses - // only if the headers were returned from the authz server. - UnsafeHeaderVector response_headers_to_overwrite_if_exists{}; + // Ordered list of header mutations for upstream request headers on OK responses. + // Mutations are applied in the order they appear in this vector. + HeaderMutationVector request_header_mutations{}; + // Ordered list of header mutations for downstream response headers on OK responses. + // Mutations are applied in the order they appear in this vector. + HeaderMutationVector response_header_mutations{}; + // Ordered list of header mutations for local reply headers on Denied/Error responses. + // These headers are sent back to the downstream client as part of the local reply. + // Mutations are applied in the order they appear in this vector. + HeaderMutationVector local_response_header_mutations{}; // Whether the authorization server returned any headers with an invalid append action type. bool saw_invalid_append_actions{false}; // A set of HTTP headers consumed by the authorization server, will be removed 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 aa8e0a1d96858..630675e254e9d 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 @@ -16,66 +16,85 @@ namespace Filters { namespace Common { namespace ExtAuthz { -void copyHeaderFieldIntoResponse( - ResponsePtr& response, - const Protobuf::RepeatedPtrField& headers) { - for (const auto& header : headers) { - if (header.append().value()) { - response->headers_to_append.emplace_back(header.header().key(), header.header().value()); - } else { - response->headers_to_set.emplace_back(header.header().key(), header.header().value()); - } +namespace { + +// Result of parsing a HeaderValueOption's append action. +struct AppendActionResult { + HeaderAppendAction action; + bool from_deprecated_append; +}; + +// Converts proto HeaderValueOption to HeaderAppendAction. +// Handles the deprecated append boolean for backward compatibility. +// Returns the action and whether it came from the deprecated field. +// Sets saw_invalid if the action is unknown. +AppendActionResult getAppendAction(envoy::config::core::v3::HeaderValueOption& header, + bool& saw_invalid_append_actions) { + if (header.has_append()) { + // Handle deprecated append boolean for backward compatibility. + // When from_deprecated_append is true and action is APPEND_IF_EXISTS_OR_ADD, + // the filter will use appendCopy() instead of addCopy(). + return {header.append().value() ? HeaderValueOption::APPEND_IF_EXISTS_OR_ADD + : HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD, + true}; + } + + if (!envoy::config::core::v3::HeaderValueOption::HeaderAppendAction_IsValid( + header.append_action())) { + saw_invalid_append_actions = true; + return {HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD, false}; } + + return {header.append_action(), false}; } -void copyOkResponseMutations(ResponsePtr& response, - const envoy::service::auth::v3::OkHttpResponse& ok_response) { - copyHeaderFieldIntoResponse(response, ok_response.headers()); - - for (const auto& header : ok_response.response_headers_to_add()) { - if (header.has_append()) { - if (header.append().value()) { - response->response_headers_to_add.emplace_back(header.header().key(), - header.header().value()); - } else { - response->response_headers_to_set.emplace_back(header.header().key(), - header.header().value()); - } - } else { - switch (header.append_action()) { - case Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: - response->response_headers_to_add.emplace_back(header.header().key(), - header.header().value()); - break; - case Router::HeaderValueOption::ADD_IF_ABSENT: - response->response_headers_to_add_if_absent.emplace_back(header.header().key(), - header.header().value()); - break; - case Router::HeaderValueOption::OVERWRITE_IF_EXISTS: - response->response_headers_to_overwrite_if_exists.emplace_back(header.header().key(), - header.header().value()); - break; - case Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD: - response->response_headers_to_set.emplace_back(header.header().key(), - header.header().value()); - break; - default: - response->saw_invalid_append_actions = true; - break; - } - } +// Moves header mutations from proto to the response vector, preserving order. +// This is more efficient than copying strings since the proto data is not reused. +void moveHeaderMutations( + Protobuf::RepeatedPtrField& headers, + HeaderMutationVector& mutations, bool& saw_invalid_append_actions) { + mutations.reserve(mutations.size() + headers.size()); + for (auto& header : headers) { + AppendActionResult result = getAppendAction(header, saw_invalid_append_actions); + mutations.push_back({std::move(*header.mutable_header()->mutable_key()), + std::move(*header.mutable_header()->mutable_value()), result.action, + result.from_deprecated_append}); } +} - response->headers_to_remove = std::vector{ok_response.headers_to_remove().begin(), - ok_response.headers_to_remove().end()}; +// Moves strings from proto repeated field to vector without copying. +std::vector +moveStringsFromProto(Protobuf::RepeatedPtrField& proto_strings) { + std::vector result; + result.reserve(proto_strings.size()); + for (auto& str : proto_strings) { + result.push_back(std::move(str)); + } + return result; +} + +} // namespace + +void copyOkResponseMutations(ResponsePtr& response, + envoy::service::auth::v3::OkHttpResponse* ok_response) { + // Move upstream request header mutations. + moveHeaderMutations(*ok_response->mutable_headers(), response->request_header_mutations, + response->saw_invalid_append_actions); + + // Move downstream response header mutations. + moveHeaderMutations(*ok_response->mutable_response_headers_to_add(), + response->response_header_mutations, response->saw_invalid_append_actions); - for (const auto& query_parameter : ok_response.query_parameters_to_set()) { + // Move headers_to_remove to avoid copying strings. + response->headers_to_remove = moveStringsFromProto(*ok_response->mutable_headers_to_remove()); + + for (const auto& query_parameter : ok_response->query_parameters_to_set()) { response->query_parameters_to_set.emplace_back(query_parameter.key(), query_parameter.value()); } + // Move query_parameters_to_remove to avoid copying strings. response->query_parameters_to_remove = - std::vector{ok_response.query_parameters_to_remove().begin(), - ok_response.query_parameters_to_remove().end()}; + moveStringsFromProto(*ok_response->mutable_query_parameters_to_remove()); } GrpcClientImpl::GrpcClientImpl(const Grpc::RawAsyncClientSharedPtr& async_client, @@ -114,8 +133,7 @@ void GrpcClientImpl::onSuccess(std::unique_ptrstatus = CheckStatus::OK; if (response->has_ok_response()) { - const auto& ok_response = response->ok_response(); - copyOkResponseMutations(authz_response, ok_response); + copyOkResponseMutations(authz_response, response->mutable_ok_response()); } } else if (response->has_error_response()) { // If error_response is present, treat it as an error and not denial. @@ -124,14 +142,17 @@ void GrpcClientImpl::onSuccess(std::unique_ptrerror_response(); - copyHeaderFieldIntoResponse(authz_response, error_response.headers()); + auto* error_response = response->mutable_error_response(); + // Move headers to local_response_header_mutations for local reply. + moveHeaderMutations(*error_response->mutable_headers(), + authz_response->local_response_header_mutations, + authz_response->saw_invalid_append_actions); - const uint32_t status_code = error_response.status().code(); + const uint32_t status_code = error_response->status().code(); if (status_code > 0) { authz_response->status_code = static_cast(status_code); } - authz_response->body = error_response.body(); + authz_response->body = error_response->body(); } else { span.setTag(TracingConstants::get().TraceStatus, TracingConstants::get().TraceUnauthz); authz_response->status = CheckStatus::Denied; @@ -139,13 +160,17 @@ void GrpcClientImpl::onSuccess(std::unique_ptrstatus_code = Http::Code::Forbidden; if (response->has_denied_response()) { - copyHeaderFieldIntoResponse(authz_response, response->denied_response().headers()); + auto* denied_response = response->mutable_denied_response(); + // Move headers to local_response_header_mutations for local reply. + moveHeaderMutations(*denied_response->mutable_headers(), + authz_response->local_response_header_mutations, + authz_response->saw_invalid_append_actions); - const uint32_t status_code = response->denied_response().status().code(); + const uint32_t status_code = denied_response->status().code(); if (status_code > 0) { authz_response->status_code = static_cast(status_code); } - authz_response->body = response->denied_response().body(); + authz_response->body = denied_response->body(); } } @@ -156,20 +181,17 @@ void GrpcClientImpl::onSuccess(std::unique_ptrdynamic_metadata = response->dynamic_metadata(); } - callbacks_->onComplete(std::move(authz_response)); callbacks_ = nullptr; } void GrpcClientImpl::onFailure(Grpc::Status::GrpcStatus status, const std::string&, Tracing::Span&) { - ENVOY_LOG(trace, "CheckRequest call failed with status: {}", - Grpc::Utility::grpcStatusToString(status)); - ASSERT(status != Grpc::Status::WellKnownGrpcStatus::Ok); - Response response{}; - response.status = CheckStatus::Error; - response.grpc_status = status; - callbacks_->onComplete(std::make_unique(response)); + ENVOY_LOG(trace, "ExtAuthz upstream failure: {}", status); + ResponsePtr authz_response = std::make_unique(Response{}); + authz_response->status = CheckStatus::Error; + authz_response->grpc_status = status; + callbacks_->onComplete(std::move(authz_response)); callbacks_ = nullptr; } 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 32d4e5ec9a3e9..4268af9834cf9 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 @@ -36,15 +36,11 @@ const Http::HeaderMap& lengthZeroHeader() { // Static response used for creating authorization ERROR responses. const Response& errorResponse() { CONSTRUCT_ON_FIRST_USE(Response, Response{CheckStatus::Error, - UnsafeHeaderVector{}, - UnsafeHeaderVector{}, - UnsafeHeaderVector{}, - UnsafeHeaderVector{}, - UnsafeHeaderVector{}, - UnsafeHeaderVector{}, - UnsafeHeaderVector{}, + HeaderMutationVector{}, + HeaderMutationVector{}, + HeaderMutationVector{}, false, - {{}}, + {}, Http::Utility::QueryParamsVector{}, {}, EMPTY_STRING, @@ -69,26 +65,39 @@ struct SuccessResponse { response_matchers_(response_matchers), to_dynamic_metadata_matchers_(dynamic_metadata_matchers), response_(std::make_unique(response)) { - headers_.iterate([this](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { - // UpstreamHeaderMatcher - if (matchers_->matches(header.key().getStringView())) { - response_->headers_to_set.emplace_back(header.key().getStringView(), - header.value().getStringView()); + const bool is_ok_response = response_->status == CheckStatus::OK; + headers_.iterate([this, + is_ok_response](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + // UpstreamHeaderMatcher only applies to OK responses for upstream request headers. + if (is_ok_response && matchers_->matches(header.key().getStringView())) { + response_->request_header_mutations.push_back( + {std::string(header.key().getStringView()), std::string(header.value().getStringView()), + HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); } - if (append_matchers_->matches(header.key().getStringView())) { + if (is_ok_response && append_matchers_->matches(header.key().getStringView())) { // If there is an existing matching key in the current headers, the new entry will be // appended with the same key. For example, given {"key": "value1"} headers, if there is // a matching "key" from the authorization response headers {"key": "value2"}, the // request to upstream server will have two entries for "key": {"key": "value1", "key": // "value2"}. - response_->headers_to_add.emplace_back(header.key().getStringView(), - header.value().getStringView()); + response_->request_header_mutations.push_back({std::string(header.key().getStringView()), + std::string(header.value().getStringView()), + HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); } if (response_matchers_->matches(header.key().getStringView())) { - // For HTTP implementation, the response headers from the auth server will, by default, be - // appended (using addCopy) to the encoded response headers. - response_->response_headers_to_add.emplace_back(header.key().getStringView(), - header.value().getStringView()); + // For OK responses, these headers are appended to the encoded response headers. + // For Denied responses, these headers go to the local reply. + if (is_ok_response) { + response_->response_header_mutations.push_back( + {std::string(header.key().getStringView()), + std::string(header.value().getStringView()), + HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + } else { + response_->local_response_header_mutations.push_back( + {std::string(header.key().getStringView()), + std::string(header.value().getStringView()), + HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + } } if (to_dynamic_metadata_matchers_->matches(header.key().getStringView())) { const std::string key{header.key().getStringView()}; @@ -423,13 +432,9 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) { config_->clientHeaderOnSuccessMatchers(), config_->dynamicMetadataMatchers(), Response{CheckStatus::OK, - UnsafeHeaderVector{}, - UnsafeHeaderVector{}, - UnsafeHeaderVector{}, - UnsafeHeaderVector{}, - UnsafeHeaderVector{}, - UnsafeHeaderVector{}, - UnsafeHeaderVector{}, + HeaderMutationVector{}, + HeaderMutationVector{}, + HeaderMutationVector{}, false, std::move(headers_to_remove), Http::Utility::QueryParamsVector{}, @@ -441,24 +446,20 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) { } // Create a Denied authorization response. - // For denied responses, only headers_to_set which is populated by the first matcher is - // used by the ext_authz filter when sending the local reply. The headers_to_add and - // response_headers_to_add fields are not used, so we pass a never-matching matcher. + // For denied responses, headers matching clientHeaderMatchers go to + // local_response_header_mutations for the local reply. The upstream request headers (matchers_, + // append_matchers_) are not used, so we pass never-matching matchers for those positions. SuccessResponse denied{message->headers(), - config_->clientHeaderMatchers(), neverMatchingMatcher(), neverMatchingMatcher(), + config_->clientHeaderMatchers(), config_->dynamicMetadataMatchers(), Response{CheckStatus::Denied, - UnsafeHeaderVector{}, - UnsafeHeaderVector{}, - UnsafeHeaderVector{}, - UnsafeHeaderVector{}, - UnsafeHeaderVector{}, - UnsafeHeaderVector{}, - UnsafeHeaderVector{}, + HeaderMutationVector{}, + HeaderMutationVector{}, + HeaderMutationVector{}, false, - {{}}, + {}, Http::Utility::QueryParamsVector{}, {}, message->bodyAsString(), diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index 1264eee722099..836e1e5486a73 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/source/extensions/filters/http/ext_authz/ext_authz.cc @@ -25,6 +25,8 @@ namespace { constexpr uint32_t kDefaultPerRouteTimeoutMs = 200; using MetadataProto = ::envoy::config::core::v3::Metadata; +using Filters::Common::ExtAuthz::HeaderAppendAction; +using Filters::Common::ExtAuthz::HeaderMutation; using Filters::Common::MutationRules::CheckOperation; using Filters::Common::MutationRules::CheckResult; @@ -72,6 +74,77 @@ bool headersWithinLimits(const Http::HeaderMap& headers) { headers.byteSize() <= headers.maxHeadersKb() * 1024; } +// Applies a single header mutation to the given header map based on its append_action. +// Returns true if the mutation was applied, false if skipped due to conditional action. +// When is_request_header is true, deprecated APPEND_IF_EXISTS_OR_ADD only appends to +// existing headers (backward compatibility). For response headers, it always appends. +bool applyHeaderMutation(Http::HeaderMap& headers, const HeaderMutation& mutation, + bool is_request_header = false) { + const Http::LowerCaseString key(mutation.key); + switch (mutation.append_action) { + case Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD: + headers.setCopy(key, mutation.value); + return true; + case Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: + // For the deprecated 'append' boolean field: + // - Request headers: only append to existing headers for backward compatibility. + // - Response headers: use appendCopy() which appends or adds new headers. + // For the new append_action enum: use addCopy() to create duplicate header entries. + if (mutation.from_deprecated_append) { + if (is_request_header) { + if (headers.get(key).empty()) { + return false; // Skip: request header doesn't exist. + } + } + headers.appendCopy(key, mutation.value); + } else { + headers.addCopy(key, mutation.value); + } + return true; + case Filters::Common::ExtAuthz::HeaderValueOption::ADD_IF_ABSENT: + if (headers.get(key).empty()) { + headers.addCopy(key, mutation.value); + return true; + } + return false; // Skip: header already exists. + case Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS: + if (!headers.get(key).empty()) { + headers.setCopy(key, mutation.value); + return true; + } + return false; // Skip: header doesn't exist. + default: + headers.setCopy(key, mutation.value); + return true; + } +} + +// Determines the CheckOperation for a given HeaderAppendAction. +CheckOperation getCheckOperationForAction(Filters::Common::ExtAuthz::HeaderAppendAction action) { + switch (action) { + case Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: + return CheckOperation::APPEND; + default: + return CheckOperation::SET; + } +} + +// Returns a human-readable name for a HeaderAppendAction. +absl::string_view getActionName(Filters::Common::ExtAuthz::HeaderAppendAction action) { + switch (action) { + case Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD: + return "set"; + case Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: + return "add"; + case Filters::Common::ExtAuthz::HeaderValueOption::ADD_IF_ABSENT: + return "add if absent"; + case Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS: + return "overwrite if exists"; + default: + return "unknown"; + } +} + } // namespace FilterConfig::FilterConfig(const envoy::extensions::filters::http::ext_authz::v3::ExtAuthz& config, @@ -483,63 +556,18 @@ Http::Filter1xxHeadersStatus Filter::encode1xxHeaders(Http::ResponseHeaderMap&) Http::FilterHeadersStatus Filter::encodeHeaders(Http::ResponseHeaderMap& headers, bool) { ENVOY_STREAM_LOG(trace, - "ext_authz filter has {} response header(s) to add and {} response header(s) to " - "set to the encoded response:", - *encoder_callbacks_, response_headers_to_add_.size(), - response_headers_to_set_.size()); - if (!response_headers_to_add_.empty()) { - ENVOY_STREAM_LOG( - trace, "ext_authz filter added header(s) to the encoded response:", *encoder_callbacks_); - for (const auto& [key, value] : response_headers_to_add_) { - ENVOY_STREAM_LOG(trace, "'{}':'{}'", *encoder_callbacks_, key.get(), value); - headers.addCopy(key, value); - if (config_->enforceResponseHeaderLimits() && !headersWithinLimits(headers)) { - responseHeaderLimitsReached(); - return Http::FilterHeadersStatus::StopIteration; - } - } - } + "ext_authz filter has {} response header mutation(s) to apply to the encoded " + "response:", + *encoder_callbacks_, response_header_mutations_.size()); - if (!response_headers_to_set_.empty()) { - ENVOY_STREAM_LOG( - trace, "ext_authz filter set header(s) to the encoded response:", *encoder_callbacks_); - for (const auto& [key, value] : response_headers_to_set_) { - ENVOY_STREAM_LOG(trace, "'{}':'{}'", *encoder_callbacks_, key.get(), value); - headers.setCopy(key, value); - if (config_->enforceResponseHeaderLimits() && !headersWithinLimits(headers)) { - responseHeaderLimitsReached(); - return Http::FilterHeadersStatus::StopIteration; - } - } - } - - if (!response_headers_to_add_if_absent_.empty()) { - for (const auto& [key, value] : response_headers_to_add_if_absent_) { - ENVOY_STREAM_LOG(trace, "'{}':'{}'", *encoder_callbacks_, key.get(), value); - if (auto header_entry = headers.get(key); header_entry.empty()) { - ENVOY_STREAM_LOG(trace, "ext_authz filter added header(s) to the encoded response:", - *encoder_callbacks_); - headers.addCopy(key, value); - if (config_->enforceResponseHeaderLimits() && !headersWithinLimits(headers)) { - responseHeaderLimitsReached(); - return Http::FilterHeadersStatus::StopIteration; - } - } - } - } + for (const auto& mutation : response_header_mutations_) { + ENVOY_STREAM_LOG(trace, "{} '{}':'{}'", *encoder_callbacks_, + getActionName(mutation.append_action), mutation.key, mutation.value); + applyHeaderMutation(headers, mutation); - if (!response_headers_to_overwrite_if_exists_.empty()) { - for (const auto& [key, value] : response_headers_to_overwrite_if_exists_) { - ENVOY_STREAM_LOG(trace, "'{}':'{}'", *encoder_callbacks_, key, value); - if (auto header_entry = headers.get(key); !header_entry.empty()) { - ENVOY_STREAM_LOG( - trace, "ext_authz filter set header(s) to the encoded response:", *encoder_callbacks_); - headers.setCopy(key, value); - if (config_->enforceResponseHeaderLimits() && !headersWithinLimits(headers)) { - responseHeaderLimitsReached(); - return Http::FilterHeadersStatus::StopIteration; - } - } + if (config_->enforceResponseHeaderLimits() && !headersWithinLimits(headers)) { + responseHeaderLimitsReached(); + return Http::FilterHeadersStatus::StopIteration; } } @@ -667,111 +695,54 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { switch (response->status) { case CheckStatus::OK: { // 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. + // 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->query_parameters_to_set.empty() || + (!response->request_header_mutations.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_->downstreamCallbacks()->clearRouteCache(); } - ENVOY_STREAM_LOG(trace, - "ext_authz filter added header(s) to the request:", *decoder_callbacks_); - for (const auto& [key, value] : response->headers_to_set) { - CheckResult check_result = validateAndCheckDecoderHeaderMutation( - Filters::Common::MutationRules::CheckOperation::SET, key, value); + ENVOY_STREAM_LOG(trace, "ext_authz filter processing {} request header mutation(s):", + *decoder_callbacks_, response->request_header_mutations.size()); + + // Process request header mutations in order. + for (const auto& mutation : response->request_header_mutations) { + const std::string& key = mutation.key; + const std::string& value = mutation.value; + CheckOperation check_op = getCheckOperationForAction(mutation.append_action); + absl::string_view op_name = getActionName(mutation.append_action); + + CheckResult check_result = validateAndCheckDecoderHeaderMutation(check_op, key, value); switch (check_result) { - case CheckResult::OK: - ENVOY_STREAM_LOG(trace, "'{}':'{}'", *decoder_callbacks_, key, value); - request_headers_->setCopy(Http::LowerCaseString(key), value); - if (!headersWithinLimits(*request_headers_)) { - stats_.request_header_limits_reached_.inc(); - rejectResponse(); - return; + case CheckResult::OK: { + if (applyHeaderMutation(*request_headers_, mutation, /*is_request_header=*/true)) { + ENVOY_STREAM_LOG(trace, "{} '{}':'{}'", *decoder_callbacks_, op_name, key, value); } - break; - case CheckResult::IGNORE: - ENVOY_STREAM_LOG(trace, "Ignoring invalid header to set '{}':'{}'.", *decoder_callbacks_, - key, value); - break; - case CheckResult::FAIL: - ENVOY_STREAM_LOG(trace, "Rejecting invalid header to set '{}':'{}'.", *decoder_callbacks_, - key, value); - rejectResponse(); - return; - } - } - for (const auto& [key, value] : response->headers_to_add) { - CheckResult check_result = validateAndCheckDecoderHeaderMutation( - Filters::Common::MutationRules::CheckOperation::SET, key, value); - switch (check_result) { - case CheckResult::OK: - ENVOY_STREAM_LOG(trace, "'{}':'{}'", *decoder_callbacks_, key, value); - request_headers_->addCopy(Http::LowerCaseString(key), value); + if (!headersWithinLimits(*request_headers_)) { stats_.request_header_limits_reached_.inc(); rejectResponse(); return; } break; - case CheckResult::IGNORE: - ENVOY_STREAM_LOG(trace, "Ignoring invalid header to add '{}':'{}'.", *decoder_callbacks_, - key, value); - break; - case CheckResult::FAIL: - ENVOY_STREAM_LOG(trace, "Rejecting invalid header to add '{}':'{}'.", *decoder_callbacks_, - key, value); - rejectResponse(); - return; - } - } - for (const auto& [key, value] : response->headers_to_append) { - CheckResult check_result = validateAndCheckDecoderHeaderMutation( - Filters::Common::MutationRules::CheckOperation::APPEND, key, value); - switch (check_result) { - case CheckResult::OK: { - ENVOY_STREAM_LOG(trace, "'{}':'{}'", *decoder_callbacks_, key, value); - Http::LowerCaseString lowercase_key(key); - const auto header_to_modify = request_headers_->get(lowercase_key); - // TODO(dio): Add a flag to allow appending non-existent headers, without setting it - // first (via `headers_to_add`). For example, given: - // 1. Original headers {"original": "true"} - // 2. Response headers from the authorization servers {{"append": "1"}, {"append": - // "2"}} - // - // Currently it is not possible to add {{"append": "1"}, {"append": "2"}} (the - // intended combined headers: {{"original": "true"}, {"append": "1"}, {"append": - // "2"}}) to the request to upstream server by only sets `headers_to_append`. - if (!header_to_modify.empty()) { - ENVOY_STREAM_LOG(trace, "'{}':'{}'", *decoder_callbacks_, key, value); - // The current behavior of appending is by combining entries with the same key, - // into one entry. The value of that combined entry is separated by ",". - // TODO(dio): Consider to use addCopy instead. - request_headers_->appendCopy(lowercase_key, value); - if (!headersWithinLimits(*request_headers_)) { - stats_.request_header_limits_reached_.inc(); - rejectResponse(); - return; - } - } - break; } case CheckResult::IGNORE: - ENVOY_STREAM_LOG(trace, "Ignoring invalid header to append '{}':'{}'.", *decoder_callbacks_, - key, value); + ENVOY_STREAM_LOG(trace, "Ignoring invalid header to {} '{}':'{}'.", *decoder_callbacks_, + op_name, key, value); break; case CheckResult::FAIL: - ENVOY_STREAM_LOG(trace, "Rejecting invalid header to append '{}':'{}'.", - *decoder_callbacks_, key, value); + ENVOY_STREAM_LOG(trace, "Rejecting invalid header to {} '{}':'{}'.", *decoder_callbacks_, + op_name, key, value); rejectResponse(); return; } } ENVOY_STREAM_LOG(trace, - "ext_authz filter removed header(s) from the request:", *decoder_callbacks_); + "ext_authz filter processing {} header removal(s):", *decoder_callbacks_, + response->headers_to_remove.size()); for (const auto& key : response->headers_to_remove) { // If the response contains an invalid header to remove, it's the same as trying to remove a // header that doesn't exist, so just ignore it. @@ -792,7 +763,7 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { switch (config_->checkDecoderHeaderMutation(CheckOperation::REMOVE, lowercase_key, EMPTY_STRING)) { case CheckResult::OK: - ENVOY_STREAM_LOG(trace, "'{}'", *decoder_callbacks_, key); + ENVOY_STREAM_LOG(trace, "Remove '{}'", *decoder_callbacks_, key); request_headers_->remove(lowercase_key); if (!headersWithinLimits(*request_headers_)) { stats_.request_header_limits_reached_.inc(); @@ -812,71 +783,21 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { } } - if (!response->response_headers_to_add.empty()) { - ENVOY_STREAM_LOG(trace, "ext_authz filter saving {} header(s) to add to the response:", - *decoder_callbacks_, response->response_headers_to_add.size()); - for (const auto& [key, value] : response->response_headers_to_add) { - if (config_->validateMutations() && (!Http::HeaderUtility::headerNameIsValid(key) || - !Http::HeaderUtility::headerValueIsValid(value))) { - ENVOY_STREAM_LOG(trace, "Rejected invalid header '{}':'{}'.", *decoder_callbacks_, key, - value); - rejectResponse(); - return; - } - } - response_headers_to_add_ = Http::HeaderVector{response->response_headers_to_add.begin(), - response->response_headers_to_add.end()}; - } - - if (!response->response_headers_to_set.empty()) { - ENVOY_STREAM_LOG(trace, "ext_authz filter saving {} header(s) to set to the response:", - *decoder_callbacks_, response->response_headers_to_set.size()); - for (const auto& [key, value] : response->response_headers_to_set) { - if (config_->validateMutations() && (!Http::HeaderUtility::headerNameIsValid(key) || - !Http::HeaderUtility::headerValueIsValid(value))) { - ENVOY_STREAM_LOG(trace, "Rejected invalid header '{}':'{}'.", *decoder_callbacks_, key, - value); - rejectResponse(); - return; - } - } - response_headers_to_set_ = Http::HeaderVector{response->response_headers_to_set.begin(), - response->response_headers_to_set.end()}; - } - - if (!response->response_headers_to_add_if_absent.empty()) { - ENVOY_STREAM_LOG(trace, "ext_authz filter saving {} header(s) to add to the response:", - *decoder_callbacks_, response->response_headers_to_add_if_absent.size()); - for (const auto& [key, value] : response->response_headers_to_add_if_absent) { - if (config_->validateMutations() && (!Http::HeaderUtility::headerNameIsValid(key) || - !Http::HeaderUtility::headerValueIsValid(value))) { - ENVOY_STREAM_LOG(trace, "Rejected invalid header '{}':'{}'.", *decoder_callbacks_, key, - value); - rejectResponse(); - return; - } - } - response_headers_to_add_if_absent_ = - Http::HeaderVector{response->response_headers_to_add_if_absent.begin(), - response->response_headers_to_add_if_absent.end()}; - } - - if (!response->response_headers_to_overwrite_if_exists.empty()) { - ENVOY_STREAM_LOG(trace, "ext_authz filter saving {} header(s) to set to the response:", - *decoder_callbacks_, - response->response_headers_to_overwrite_if_exists.size()); - for (const auto& [key, value] : response->response_headers_to_overwrite_if_exists) { - if (config_->validateMutations() && (!Http::HeaderUtility::headerNameIsValid(key) || - !Http::HeaderUtility::headerValueIsValid(value))) { - ENVOY_STREAM_LOG(trace, "Rejected invalid header '{}':'{}'.", *decoder_callbacks_, key, - value); + // Validate and save response header mutations for later application during encoding. + if (!response->response_header_mutations.empty()) { + ENVOY_STREAM_LOG(trace, "ext_authz filter saving {} response header mutation(s):", + *decoder_callbacks_, response->response_header_mutations.size()); + for (const auto& mutation : response->response_header_mutations) { + if (config_->validateMutations() && + (!Http::HeaderUtility::headerNameIsValid(mutation.key) || + !Http::HeaderUtility::headerValueIsValid(mutation.value))) { + ENVOY_STREAM_LOG(trace, "Rejected invalid header '{}':'{}'.", *decoder_callbacks_, + mutation.key, mutation.value); rejectResponse(); return; } } - response_headers_to_overwrite_if_exists_ = - Http::HeaderVector{response->response_headers_to_overwrite_if_exists.begin(), - response->response_headers_to_overwrite_if_exists.end()}; + response_header_mutations_ = std::move(response->response_header_mutations); } absl::optional modified_query_parameters; @@ -959,13 +880,13 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { } } - // Check headers are valid. + // Validate all local response header mutations. if (config_->validateMutations()) { - for (const auto& [key, value] : response->headers_to_set) { - if (!Http::HeaderUtility::headerNameIsValid(key) || - !Http::HeaderUtility::headerValueIsValid(value)) { - ENVOY_STREAM_LOG(trace, "Rejected invalid header '{}':'{}'.", *decoder_callbacks_, key, - value); + for (const auto& mutation : response->local_response_header_mutations) { + if (!Http::HeaderUtility::headerNameIsValid(mutation.key) || + !Http::HeaderUtility::headerValueIsValid(mutation.value)) { + ENVOY_STREAM_LOG(trace, "Rejected invalid header '{}':'{}'.", *decoder_callbacks_, + mutation.key, mutation.value); rejectResponse(); return; } @@ -980,33 +901,35 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { response->body.resize(config_->maxDeniedResponseBodyBytes()); } - // setResponseFlag must be called before sendLocalReply + // setResponseFlag must be called before sendLocalReply. decoder_callbacks_->streamInfo().setResponseFlag( StreamInfo::CoreResponseFlag::UnauthorizedExternalService); + + // Move mutations to local variables to avoid dangling references in the lambda. + auto local_mutations = std::move(response->local_response_header_mutations); + FilterConfig* cfg = config_.get(); + ExtAuthzFilterStats& filter_stats = stats_; + decoder_callbacks_->sendLocalReply( response->status_code, response->body, - [&headers = response->headers_to_set, &callbacks = *decoder_callbacks_, - this](Http::HeaderMap& response_headers) -> void { - ENVOY_STREAM_LOG(trace, - "ext_authz filter added header(s) to the local response:", callbacks); - // Firstly, remove all headers requested by the ext_authz filter, to ensure that they will - // override existing headers. - for (const auto& [key, _] : headers) { - response_headers.remove(Http::LowerCaseString(key)); - } - // Then set all of the requested headers, allowing the same header to be set multiple - // times, e.g. `Set-Cookie`. - for (const auto& [key, value] : headers) { - if (config_->enforceResponseHeaderLimits() && + [&local_mutations, cfg, &filter_stats](Http::HeaderMap& response_headers) -> void { + ENVOY_LOG(trace, "ext_authz filter added header(s) to the local response:"); + + // Process mutations in order, applying each based on its append_action. + for (const auto& mutation : local_mutations) { + // Check header limits before adding. + if (cfg->enforceResponseHeaderLimits() && response_headers.size() >= response_headers.maxHeadersCount()) { - stats_.omitted_response_headers_.inc(); + filter_stats.omitted_response_headers_.inc(); ENVOY_LOG_EVERY_POW_2( warn, "Some ext_authz response headers weren't added because the header map was full."); break; } - ENVOY_STREAM_LOG(trace, " '{}':'{}'", callbacks, key, value); - response_headers.addCopy(Http::LowerCaseString(key), value); + + ENVOY_LOG(trace, " {} '{}':'{}'", getActionName(mutation.append_action), mutation.key, + mutation.value); + applyHeaderMutation(response_headers, mutation); } }, absl::nullopt, Filters::Common::ExtAuthz::ResponseCodeDetails::get().AuthzDenied); @@ -1055,12 +978,31 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { decoder_callbacks_->streamInfo().setResponseFlag( StreamInfo::CoreResponseFlag::UnauthorizedExternalService); + // Move mutations to local variables to avoid dangling references in the lambda. + auto local_mutations = std::move(response->local_response_header_mutations); + FilterConfig* cfg = config_.get(); + ExtAuthzFilterStats& filter_stats = stats_; + decoder_callbacks_->sendLocalReply( status_code, response->body, - [&headers_to_set = response->headers_to_set, - &headers_to_append = response->headers_to_append, - this](Http::HeaderMap& response_headers) -> void { - addErrorResponseHeaders(response_headers, headers_to_set, headers_to_append); + [&local_mutations, cfg, &filter_stats](Http::HeaderMap& response_headers) -> void { + ENVOY_LOG(trace, "ext_authz filter added header(s) to the error response:"); + + // Process mutations in order, applying each based on its append_action. + for (const auto& mutation : local_mutations) { + // Check header limits before adding. + if (cfg->enforceResponseHeaderLimits() && + response_headers.size() >= response_headers.maxHeadersCount()) { + filter_stats.omitted_response_headers_.inc(); + ENVOY_LOG_EVERY_POW_2(warn, "Some ext_authz error response headers weren't added " + "because the header map was full."); + break; + } + + ENVOY_LOG(trace, " {} '{}':'{}'", getActionName(mutation.append_action), mutation.key, + mutation.value); + applyHeaderMutation(response_headers, mutation); + } }, absl::nullopt, Filters::Common::ExtAuthz::ResponseCodeDetails::get().AuthzError); } @@ -1143,38 +1085,18 @@ bool Filter::validateAndClearInvalidErrorResponseAttributes( return true; } - // Validate headers_to_set. - for (const auto& [key, value] : response->headers_to_set) { - if (!Http::HeaderUtility::headerNameIsValid(key) || - !Http::HeaderUtility::headerValueIsValid(value)) { - ENVOY_STREAM_LOG(trace, "Rejected invalid error header '{}':'{}'.", *decoder_callbacks_, key, - value); - ENVOY_STREAM_LOG(info, - "Custom error response from ext_authz will be ignored due to invalid " - "header. Falling back to generic error response.", - *decoder_callbacks_); - // Fall back to generic error by clearing all custom attributes. - response->headers_to_set.clear(); - response->headers_to_append.clear(); - response->body.clear(); - response->status_code = static_cast(0); // Clear custom status. - return false; - } - } - - // Validate headers_to_append. - for (const auto& [key, value] : response->headers_to_append) { - if (!Http::HeaderUtility::headerNameIsValid(key) || - !Http::HeaderUtility::headerValueIsValid(value)) { - ENVOY_STREAM_LOG(trace, "Rejected invalid error header '{}':'{}'.", *decoder_callbacks_, key, - value); + // Validate all local response header mutations. + for (const auto& mutation : response->local_response_header_mutations) { + if (!Http::HeaderUtility::headerNameIsValid(mutation.key) || + !Http::HeaderUtility::headerValueIsValid(mutation.value)) { + ENVOY_STREAM_LOG(trace, "Rejected invalid error header '{}':'{}'.", *decoder_callbacks_, + mutation.key, mutation.value); ENVOY_STREAM_LOG(info, "Custom error response from ext_authz will be ignored due to invalid " "header. Falling back to generic error response.", *decoder_callbacks_); // Fall back to generic error by clearing all custom attributes. - response->headers_to_set.clear(); - response->headers_to_append.clear(); + response->local_response_header_mutations.clear(); response->body.clear(); response->status_code = static_cast(0); // Clear custom status. return false; @@ -1184,46 +1106,6 @@ bool Filter::validateAndClearInvalidErrorResponseAttributes( return true; } -bool Filter::canAddResponseHeader(Http::HeaderMap& response_headers) { - if (config_->enforceResponseHeaderLimits() && - response_headers.size() >= response_headers.maxHeadersCount()) { - stats_.omitted_response_headers_.inc(); - ENVOY_LOG_EVERY_POW_2(warn, "Some ext_authz error response headers weren't added because the " - "header map was full."); - return false; - } - return true; -} - -void Filter::addErrorResponseHeaders( - Http::HeaderMap& response_headers, - const std::vector>& headers_to_set, - const std::vector>& headers_to_append) { - ENVOY_STREAM_LOG(trace, - "ext_authz filter added header(s) to the error response:", *decoder_callbacks_); - - // First, handle headers_to_set which should override existing headers. - for (const auto& [key, _] : headers_to_set) { - response_headers.remove(Http::LowerCaseString(key)); - } - for (const auto& [key, value] : headers_to_set) { - if (!canAddResponseHeader(response_headers)) { - break; - } - ENVOY_STREAM_LOG(trace, " '{}':'{}'", *decoder_callbacks_, key, value); - response_headers.addCopy(Http::LowerCaseString(key), value); - } - - // Then, handle headers_to_append which should append to existing headers. - for (const auto& [key, value] : headers_to_append) { - if (!canAddResponseHeader(response_headers)) { - break; - } - ENVOY_STREAM_LOG(trace, " '{}':'{}'", *decoder_callbacks_, key, value); - response_headers.addCopy(Http::LowerCaseString(key), value); - } -} - } // namespace ExtAuthz } // namespace HttpFilters } // namespace Extensions diff --git a/source/extensions/filters/http/ext_authz/ext_authz.h b/source/extensions/filters/http/ext_authz/ext_authz.h index 12411e8935e13..24b69f43f0d9f 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.h +++ b/source/extensions/filters/http/ext_authz/ext_authz.h @@ -438,17 +438,6 @@ class Filter : public Logger::Loggable, bool validateAndClearInvalidErrorResponseAttributes(Filters::Common::ExtAuthz::ResponsePtr& response); - // Helper to check if we can add more headers to the response, respecting header limits. - // Returns true if we can add more headers, false if the limit has been reached. - bool canAddResponseHeader(Http::HeaderMap& response_headers); - - // Helper to add error response headers (both set and append) to the response header map, - // respecting enforceResponseHeaderLimits(). - void addErrorResponseHeaders( - Http::HeaderMap& response_headers, - const std::vector>& headers_to_set, - const std::vector>& headers_to_append); - // Create a new gRPC client for per-route gRPC service configuration. Filters::Common::ExtAuthz::ClientPtr createPerRouteGrpcClient(const envoy::config::core::v3::GrpcService& grpc_service); @@ -489,10 +478,8 @@ class Filter : public Logger::Loggable, Http::StreamDecoderFilterCallbacks* decoder_callbacks_{}; Http::StreamEncoderFilterCallbacks* encoder_callbacks_{}; Http::RequestHeaderMap* request_headers_; - Http::HeaderVector response_headers_to_add_{}; - Http::HeaderVector response_headers_to_set_{}; - Http::HeaderVector response_headers_to_add_if_absent_{}; - Http::HeaderVector response_headers_to_overwrite_if_exists_{}; + // Ordered list of response header mutations to apply during encoding. + Filters::Common::ExtAuthz::HeaderMutationVector response_header_mutations_{}; State state_{State::NotStarted}; FilterReturn filter_return_{FilterReturn::ContinueDecoding}; Upstream::ClusterInfoConstSharedPtr cluster_; 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 595a947a73014..7fe4d2097eb4c 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 @@ -557,16 +557,89 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithAppendActions) { )EOF", check_response); + // Response header mutations are processed in order. auto expected_authz_response = Response{ .status = CheckStatus::OK, - .response_headers_to_add = - UnsafeHeaderVector{{"append-if-exists-or-add", "append-if-exists-or-add-value"}}, - .response_headers_to_set = - UnsafeHeaderVector{{"overwrite-if-exists-or-add", "overwrite-if-exists-or-add-value"}}, - .response_headers_to_add_if_absent = - UnsafeHeaderVector{{"add-if-absent", "add-if-absent-value"}}, - .response_headers_to_overwrite_if_exists = - UnsafeHeaderVector{{"overwrite-if-exists", "overwrite-if-exists-value"}}, + .response_header_mutations = + HeaderMutationVector{ + {"append-if-exists-or-add", "append-if-exists-or-add-value", + HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}, + {"add-if-absent", "add-if-absent-value", HeaderValueOption::ADD_IF_ABSENT}, + {"overwrite-if-exists", "overwrite-if-exists-value", + HeaderValueOption::OVERWRITE_IF_EXISTS}, + {"overwrite-if-exists-or-add", "overwrite-if-exists-or-add-value", + HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}, + // Invalid actions default to OVERWRITE_IF_EXISTS_OR_ADD. + {"invalid-append-action", "invalid-append-action-value", + HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}, + }, + .saw_invalid_append_actions = true, + .status_code = Http::Code::OK, + .grpc_status = Grpc::Status::WellKnownGrpcStatus::Ok, + }; + + 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(expected_authz_response)))); + client_->onSuccess(std::make_unique(check_response), + span_); +} + +TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithUpstreamHeadersAppendActions) { + initialize(); + + envoy::service::auth::v3::CheckResponse check_response; + TestUtility::loadFromYaml(R"EOF( +status: + code: 0 +ok_response: + headers: + - header: + key: append-if-exists-or-add + value: append-if-exists-or-add-value + append_action: APPEND_IF_EXISTS_OR_ADD + - header: + key: add-if-absent + value: add-if-absent-value + append_action: ADD_IF_ABSENT + - header: + key: overwrite-if-exists + value: overwrite-if-exists-value + append_action: OVERWRITE_IF_EXISTS + - header: + key: overwrite-if-exists-or-add + value: overwrite-if-exists-or-add-value + append_action: OVERWRITE_IF_EXISTS_OR_ADD + - header: + key: invalid-append-action + value: invalid-append-action-value + append_action: 404 +)EOF", + check_response); + + // Request header mutations are processed in order. + auto expected_authz_response = Response{ + .status = CheckStatus::OK, + .request_header_mutations = + HeaderMutationVector{ + {"append-if-exists-or-add", "append-if-exists-or-add-value", + HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}, + {"add-if-absent", "add-if-absent-value", HeaderValueOption::ADD_IF_ABSENT}, + {"overwrite-if-exists", "overwrite-if-exists-value", + HeaderValueOption::OVERWRITE_IF_EXISTS}, + {"overwrite-if-exists-or-add", "overwrite-if-exists-or-add-value", + HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}, + // Invalid actions default to OVERWRITE_IF_EXISTS_OR_ADD. + {"invalid-append-action", "invalid-append-action-value", + HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}, + }, .saw_invalid_append_actions = true, .status_code = Http::Code::OK, .grpc_status = Grpc::Status::WellKnownGrpcStatus::Ok, diff --git a/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc index a959eb9c4bbb6..d64b503931ebd 100644 --- a/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc +++ b/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc @@ -404,10 +404,15 @@ using HeaderValuePair = std::paircheck(request_callbacks_, request, parent_span_, stream_info_); @@ -578,11 +605,28 @@ TEST_F(ExtAuthzHttpClientTest, AuthorizationDeniedWithAllAttributes) { // allowed_client_headers is configured. TEST_F(ExtAuthzHttpClientTest, AuthorizationDeniedAndAllowedClientHeaders) { const auto expected_body = std::string{"test"}; - const auto authz_response = TestCommon::makeAuthzResponse( - CheckStatus::Denied, Http::Code::Unauthorized, expected_body, - TestCommon::makeHeaderValueOption( - {{"x-foo", "bar", false}, {":status", "401", false}, {"foo", "bar", false}}), - TestCommon::makeHeaderValueOption({})); + // Manually construct expected response because HTTP impl processes headers differently. + // Response headers from auth server: :method, x-foo, :status, foo + // - For denied responses, upstreamHeaderMatchers are not applied. + // - x-foo, :status, foo match clientHeaderMatchers (X- prefix, default :status, Foo exact) + // → local_response_header_mutations with Add. + // - :method doesn't match any matcher. + // Note that the header iteration order may differ from insertion order. Pseudo-headers + // like :status are typically iterated first. + Response authz_response; + authz_response.status = CheckStatus::Denied; + authz_response.status_code = Http::Code::Unauthorized; + authz_response.body = expected_body; + // :status matches default client header matcher → local_response_header_mutations. + authz_response.local_response_header_mutations.push_back( + {":status", "401", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + // x-foo matches clientHeaderMatchers (X- prefix) → local_response_header_mutations. + authz_response.local_response_header_mutations.push_back( + {"x-foo", "bar", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + // foo matches clientHeaderMatchers (Foo exact) → local_response_header_mutations. + authz_response.local_response_header_mutations.push_back( + {"foo", "bar", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + // For denied responses, headers_to_remove is empty (parsing only happens for OK responses). envoy::service::auth::v3::CheckRequest request; client_->check(request_callbacks_, request, parent_span_, stream_info_); @@ -756,8 +800,10 @@ TEST_F(ExtAuthzHttpClientTest, SetCookieHeaderOnSuccess) { {"x-custom-header", "custom-value", false}}); Response expected_response = TestCommon::makeAuthzResponse(CheckStatus::OK, Http::Code::OK); - expected_response.response_headers_to_add = {{"set-cookie", "session=abc123"}, - {"x-custom-header", "custom-value"}}; + expected_response.response_header_mutations.push_back( + {"set-cookie", "session=abc123", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + expected_response.response_header_mutations.push_back( + {"x-custom-header", "custom-value", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); envoy::service::auth::v3::CheckRequest request; client_->check(request_callbacks_, request, parent_span_, stream_info_); @@ -795,12 +841,15 @@ TEST_F(ExtAuthzHttpClientTest, SetCookieHeaderOnDenied) { Response expected_response = TestCommon::makeAuthzResponse(CheckStatus::Denied, Http::Code::Forbidden, expected_body); - // For denied responses, headers matching allowed_client_headers populate headers_to_set - // which is used by the ext_authz filter for the local reply. - // Note: :status is included because toClientMatchers adds default matchers for Status, - // Content-Length, WWW-Authenticate, and Location when allowed_client_headers is non-empty. - expected_response.headers_to_set = { - {":status", "403"}, {"set-cookie", "error=invalid"}, {"x-auth-error", "invalid_token"}}; + // For denied responses, headers matching allowed_client_headers go to + // local_response_header_mutations. Note that :status is always matched by the default client + // header matchers and is iterated first. Pseudo-headers are iterated before regular headers. + expected_response.local_response_header_mutations.push_back( + {":status", "403", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + expected_response.local_response_header_mutations.push_back( + {"set-cookie", "error=invalid", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + expected_response.local_response_header_mutations.push_back( + {"x-auth-error", "invalid_token", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); envoy::service::auth::v3::CheckRequest request; client_->check(request_callbacks_, request, parent_span_, stream_info_); @@ -835,8 +884,10 @@ TEST_F(ExtAuthzHttpClientTest, MultipleSetCookieHeadersOnSuccess) { message_response->headers().addCopy(Http::LowerCaseString{"set-cookie"}, "user=john"); Response expected_response = TestCommon::makeAuthzResponse(CheckStatus::OK, Http::Code::OK); - expected_response.response_headers_to_add = {{"set-cookie", "session=abc123"}, - {"set-cookie", "user=john"}}; + expected_response.response_header_mutations.push_back( + {"set-cookie", "session=abc123", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + expected_response.response_header_mutations.push_back( + {"set-cookie", "user=john", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); envoy::service::auth::v3::CheckRequest request; client_->check(request_callbacks_, request, parent_span_, stream_info_); diff --git a/test/extensions/filters/common/ext_authz/test_common.cc b/test/extensions/filters/common/ext_authz/test_common.cc index 029f0e2925015..e9d781664ac16 100644 --- a/test/extensions/filters/common/ext_authz/test_common.cc +++ b/test/extensions/filters/common/ext_authz/test_common.cc @@ -16,6 +16,23 @@ namespace Filters { namespace Common { namespace ExtAuthz { +namespace { +const char* headerAppendActionToString(HeaderAppendAction action) { + switch (action) { + case HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: + return "APPEND_IF_EXISTS_OR_ADD"; + case HeaderValueOption::ADD_IF_ABSENT: + return "ADD_IF_ABSENT"; + case HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD: + return "OVERWRITE_IF_EXISTS_OR_ADD"; + case HeaderValueOption::OVERWRITE_IF_EXISTS: + return "OVERWRITE_IF_EXISTS"; + default: + return "Unknown"; + } +} +} // namespace + // NOLINTNEXTLINE(readability-identifier-naming) void PrintTo(const ResponsePtr& ptr, std::ostream* os) { if (ptr != nullptr) { @@ -27,13 +44,22 @@ void PrintTo(const ResponsePtr& ptr, std::ostream* os) { // NOLINTNEXTLINE(readability-identifier-naming) void PrintTo(const Response& response, std::ostream* os) { - (*os) << "\n{\n check_status: " << int(response.status) - << "\n headers_to_append: " << PrintToString(response.headers_to_append) - << "\n headers_to_set: " << PrintToString(response.headers_to_set) - << "\n headers_to_add: " << PrintToString(response.headers_to_add) - << "\n response_headers_to_add: " << PrintToString(response.response_headers_to_add) - << "\n response_headers_to_set: " << PrintToString(response.response_headers_to_set) - << "\n headers_to_remove: " << PrintToString(response.headers_to_remove) + (*os) << "\n{\n check_status: " << int(response.status) << "\n request_header_mutations: ["; + for (const auto& mutation : response.request_header_mutations) { + (*os) << "\n {key: " << mutation.key << ", value: " << mutation.value + << ", append_action: " << headerAppendActionToString(mutation.append_action) << "}"; + } + (*os) << "\n ]\n response_header_mutations: ["; + for (const auto& mutation : response.response_header_mutations) { + (*os) << "\n {key: " << mutation.key << ", value: " << mutation.value + << ", append_action: " << headerAppendActionToString(mutation.append_action) << "}"; + } + (*os) << "\n ]\n local_response_header_mutations: ["; + for (const auto& mutation : response.local_response_header_mutations) { + (*os) << "\n {key: " << mutation.key << ", value: " << mutation.value + << ", append_action: " << headerAppendActionToString(mutation.append_action) << "}"; + } + (*os) << "\n ]\n headers_to_remove: " << PrintToString(response.headers_to_remove) << "\n query_parameters_to_set: " << PrintToString(response.query_parameters_to_set) << "\n query_parameters_to_remove: " << PrintToString(response.query_parameters_to_remove) << "\n body: " << response.body << "\n status_code: " << int(response.status_code) @@ -127,24 +153,46 @@ TestCommon::makeAuthzResponse(CheckStatus status, Http::Code status_code, const authz_response.body = body; } if (!headers.empty()) { - for (auto& header : headers) { - if (header.append().value()) { - authz_response.headers_to_append.emplace_back(header.header().key(), - header.header().value()); + for (const auto& header : headers) { + HeaderAppendAction action; + bool from_deprecated_append = false; + if (header.has_append()) { + // Match gRPC impl behavior: append=true → APPEND_IF_EXISTS_OR_ADD, + // append=false → OVERWRITE_IF_EXISTS_OR_ADD. + action = header.append().value() ? HeaderValueOption::APPEND_IF_EXISTS_OR_ADD + : HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD; + from_deprecated_append = true; + } else { + // Default to OVERWRITE_IF_EXISTS_OR_ADD for backward compatibility. + action = HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD; + } + if (status == Filters::Common::ExtAuthz::CheckStatus::OK) { + // OK response: headers go to request_header_mutations for upstream request. + authz_response.request_header_mutations.push_back( + {header.header().key(), header.header().value(), action, from_deprecated_append}); } else { - authz_response.headers_to_set.emplace_back(header.header().key(), header.header().value()); + // Denied/Error response: headers go to local_response_header_mutations for local reply. + authz_response.local_response_header_mutations.push_back( + {header.header().key(), header.header().value(), action, from_deprecated_append}); } } } if (!downstream_headers.empty()) { - for (auto& header : downstream_headers) { - if (header.append().value()) { - authz_response.response_headers_to_add.emplace_back(header.header().key(), - header.header().value()); + for (const auto& header : downstream_headers) { + HeaderAppendAction action; + bool from_deprecated_append = false; + if (header.has_append()) { + // Match gRPC impl behavior: append=true → APPEND_IF_EXISTS_OR_ADD, + // append=false → OVERWRITE_IF_EXISTS_OR_ADD. + action = header.append().value() ? HeaderValueOption::APPEND_IF_EXISTS_OR_ADD + : HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD; + from_deprecated_append = true; } else { - authz_response.response_headers_to_set.emplace_back(header.header().key(), - header.header().value()); + // Default to APPEND_IF_EXISTS_OR_ADD for response headers (backward compatible with Add). + action = HeaderValueOption::APPEND_IF_EXISTS_OR_ADD; } + authz_response.response_header_mutations.push_back( + {header.header().key(), header.header().value(), action, from_deprecated_append}); } } @@ -176,9 +224,19 @@ Http::ResponseMessagePtr TestCommon::makeMessageResponse(const HeaderValueOption return response; }; -bool TestCommon::compareHeaderVector(const UnsafeHeaderVector& lhs, const UnsafeHeaderVector& rhs) { - return std::set(lhs.begin(), lhs.end()) == - std::set(rhs.begin(), rhs.end()); +bool TestCommon::compareHeaderMutationVector(const HeaderMutationVector& lhs, + const HeaderMutationVector& rhs) { + if (lhs.size() != rhs.size()) { + return false; + } + // Compare in order since order matters for header mutations. + for (size_t i = 0; i < lhs.size(); ++i) { + if (lhs[i].key != rhs[i].key || lhs[i].value != rhs[i].value || + lhs[i].append_action != rhs[i].append_action) { + return false; + } + } + return true; } bool TestCommon::compareVectorOfHeaderName(const std::vector& lhs, diff --git a/test/extensions/filters/common/ext_authz/test_common.h b/test/extensions/filters/common/ext_authz/test_common.h index 0a9e15e08fa1c..dbb553289edb4 100644 --- a/test/extensions/filters/common/ext_authz/test_common.h +++ b/test/extensions/filters/common/ext_authz/test_common.h @@ -57,7 +57,8 @@ class TestCommon { static HeaderValueOptionVector makeHeaderValueOption(KeyValueOptionVector&& headers); - static bool compareHeaderVector(const UnsafeHeaderVector& lhs, const UnsafeHeaderVector& rhs); + static bool compareHeaderMutationVector(const HeaderMutationVector& lhs, + const HeaderMutationVector& rhs); static bool compareQueryParamsVector(const Http::Utility::QueryParamsVector& lhs, const Http::Utility::QueryParamsVector& rhs); static bool compareVectorOfHeaderName(const std::vector& lhs, @@ -67,9 +68,8 @@ class TestCommon { }; MATCHER_P(AuthzErrorResponse, response, "") { - // For gRPC transport errors (onFailure), these fields should be empty. - // For error_response, headers_to_set and body can be populated. - if (!arg->headers_to_append.empty()) { + // For gRPC transport errors (onFailure), request_header_mutations should be empty. + if (!arg->request_header_mutations.empty()) { return false; } // Status code can be custom for error_response or Forbidden for transport errors. @@ -89,8 +89,9 @@ MATCHER_P(AuthzErrorResponseWithAttributes, response, "") { if (arg->body.compare(response.body)) { return false; } - // Compare headers_to_set. - return TestCommon::compareHeaderVector(response.headers_to_set, arg->headers_to_set); + // Compare local_response_header_mutations for error responses. + return TestCommon::compareHeaderMutationVector(response.local_response_header_mutations, + arg->local_response_header_mutations); } MATCHER_P(AuthzResponseNoAttributes, response, "") { @@ -122,12 +123,13 @@ MATCHER_P(AuthzDeniedResponse, response, "") { if (arg->body.compare(response.body)) { return false; } - // Compare headers_to_set (used by ext_authz filter for denied local reply). - if (!TestCommon::compareHeaderVector(response.headers_to_set, arg->headers_to_set)) { + // Compare local_response_header_mutations (used for denied local reply). + if (!TestCommon::compareHeaderMutationVector(response.local_response_header_mutations, + arg->local_response_header_mutations)) { return false; } - // Compare headers_to_add. - return TestCommon::compareHeaderVector(response.headers_to_add, arg->headers_to_add); + // Compare headers_to_remove. + return TestCommon::compareVectorOfHeaderName(response.headers_to_remove, arg->headers_to_remove); } MATCHER_P(AuthzOkResponse, response, "") { @@ -139,21 +141,13 @@ MATCHER_P(AuthzOkResponse, response, "") { return false; } - if (!TestCommon::compareHeaderVector(response.headers_to_append, arg->headers_to_append)) { + if (!TestCommon::compareHeaderMutationVector(response.request_header_mutations, + arg->request_header_mutations)) { return false; } - if (!TestCommon::compareHeaderVector(response.headers_to_add, arg->headers_to_add)) { - return false; - } - - if (!TestCommon::compareHeaderVector(response.response_headers_to_add, - arg->response_headers_to_add)) { - return false; - } - - if (!TestCommon::compareHeaderVector(response.response_headers_to_set, - arg->response_headers_to_set)) { + if (!TestCommon::compareHeaderMutationVector(response.response_header_mutations, + arg->response_header_mutations)) { return false; } diff --git a/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc index 688bd64df3c35..70c649b7851a2 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc @@ -478,14 +478,17 @@ class ExtAuthzGrpcIntegrationTest const Headers& response_headers_to_set, const Headers& response_headers_to_append_if_absent, const Headers& response_headers_to_set_if_exists = {}, - bool add_sentinel_header_append_action = false) { + bool add_sentinel_header_append_action = false, + const Headers& headers_to_add_if_absent = {}, + const Headers& headers_to_overwrite_if_exists = {}, + const Headers& headers_to_overwrite_if_exists_or_add = {}) { ext_authz_request_->startGrpcStream(); envoy::service::auth::v3::CheckResponse check_response; check_response.mutable_status()->set_code(Grpc::Status::WellKnownGrpcStatus::Ok); for (const auto& header_to_add : headers_to_add) { auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); - entry->mutable_append()->set_value(false); + entry->set_append_action(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); entry->mutable_header()->set_key(header_to_add.first); entry->mutable_header()->set_value(header_to_add.second); } @@ -497,6 +500,27 @@ class ExtAuthzGrpcIntegrationTest entry->mutable_header()->set_value(header_to_append.second); } + for (const auto& header : headers_to_add_if_absent) { + auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); + entry->set_append_action(Router::HeaderValueOption::ADD_IF_ABSENT); + entry->mutable_header()->set_key(header.first); + entry->mutable_header()->set_value(header.second); + } + + for (const auto& header : headers_to_overwrite_if_exists) { + auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); + entry->set_append_action(Router::HeaderValueOption::OVERWRITE_IF_EXISTS); + entry->mutable_header()->set_key(header.first); + entry->mutable_header()->set_value(header.second); + } + + for (const auto& header : headers_to_overwrite_if_exists_or_add) { + auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); + entry->set_append_action(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + entry->mutable_header()->set_key(header.first); + entry->mutable_header()->set_value(header.second); + } + for (const auto& header_to_remove : headers_to_remove) { auto* entry = check_response.mutable_ok_response()->mutable_headers_to_remove(); entry->Add(std::string(header_to_remove.first)); @@ -673,6 +697,82 @@ class ExtAuthzGrpcIntegrationTest cleanup(); } + void expectCheckRequestWithBodyWithAppendActions( + Http::CodecType downstream_protocol, uint64_t request_size, const Headers& headers_to_add, + const Headers& headers_to_append, const Headers& headers_to_add_if_absent, + const Headers& headers_to_overwrite_if_exists, + const Headers& headers_to_overwrite_if_exists_or_add) { + initializeConfig(); + setDownstreamProtocol(downstream_protocol); + HttpIntegrationTest::initialize(); + + // Headers that we will send in the request to test overwrite/append logic. + Headers request_headers; + request_headers.reserve(headers_to_add.size() + headers_to_append.size()); + for (const auto& h : headers_to_add) { + request_headers.push_back(h); + } + for (const auto& h : headers_to_append) { + request_headers.push_back(h); + } + + // We initiate connection with headers_to_add and headers_to_append as initial headers. + initiateClientConnection(request_size, request_headers, Headers{}, Headers{}); + waitForExtAuthzRequest(expectedCheckRequest(downstream_protocol)); + + // Send response with new append actions. + sendExtAuthzResponse({}, {}, {}, {}, {}, {}, {}, {}, {}, false, headers_to_add_if_absent, + headers_to_overwrite_if_exists, headers_to_overwrite_if_exists_or_add); + + waitForSuccessfulUpstreamResponse("200"); + + // Verify headers manually. + for (const auto& h : headers_to_add_if_absent) { + // Check if it existed in the original request. + bool existed = false; + std::string original_value; + for (const auto& req_h : request_headers) { + if (req_h.first == h.first) { + existed = true; + original_value = req_h.second; + break; + } + } + + if (existed) { + // If it existed, it should NOT be changed. + EXPECT_THAT(upstream_request_->headers(), ContainsHeader(h.first, original_value)); + } else { + // If it didn't exist, it SHOULD be added. + EXPECT_THAT(upstream_request_->headers(), ContainsHeader(h.first, h.second)); + } + } + for (const auto& h : headers_to_overwrite_if_exists) { + // If the header existed, it should be overwritten. If not, it shouldn't exist unless + // it was added by other means. In this test helper we assume simple cases. + // We need to check if it was in the initial request to know expectation. + bool existed = false; + for (const auto& req_h : request_headers) { + if (req_h.first == h.first) + existed = true; + } + + if (existed) { + // Header existed, so it should be overwritten with the new value. + EXPECT_THAT(upstream_request_->headers(), ContainsHeader(h.first, h.second)); + } else { + // Header did not exist, so OverwriteIfExists should NOT add it. + EXPECT_TRUE(upstream_request_->headers().get(Http::LowerCaseString{h.first}).empty()) + << "Header '" << h.first << "' should not have been added by OverwriteIfExists"; + } + } + for (const auto& h : headers_to_overwrite_if_exists_or_add) { + EXPECT_THAT(upstream_request_->headers(), ContainsHeader(h.first, h.second)); + } + + cleanup(); + } + void expectFilterDisableCheck(bool deny_at_disable, bool disable_with_metadata, const std::string& expected_status) { GrpcInitializeConfigOpts opts; @@ -1084,6 +1184,21 @@ TEST_P(ExtAuthzGrpcIntegrationTest, SendHeadersToAddAndToAppendToUpstream) { {"multiple", "multiple-second"}}); } +TEST_P(ExtAuthzGrpcIntegrationTest, UpstreamHeadersAppendActions) { + expectCheckRequestWithBodyWithAppendActions( + Http::CodecType::HTTP1, 4, + /*headers_to_add=*/ + Headers{{"header-to-add-if-absent", "original-value"}, + {"header-to-overwrite-if-exists", "original-value"}}, // Initial headers + /*headers_to_append=*/Headers{}, + /*headers_to_add_if_absent=*/ + Headers{{"header-to-add-if-absent", "new-value"}, {"new-header-if-absent", "new-value"}}, + /*headers_to_overwrite_if_exists=*/ + Headers{{"header-to-overwrite-if-exists", "new-value"}, + {"non-existent-header", "should-not-be-added"}}, + /*headers_to_overwrite_if_exists_or_add=*/Headers{{"header-to-set", "set-value"}}); +} + TEST_P(ExtAuthzGrpcIntegrationTest, AllowAtDisable) { expectFilterDisableCheck(/*deny_at_disable=*/false, /*disable_with_metadata=*/false, "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 c1473abe0c4e8..8b1ac50dd8dfe 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_test.cc @@ -97,6 +97,23 @@ MATCHER_P(HasTimeout, expected_timeout_ms, "") { return true; } +// Type aliases for convenience. +using HeaderAppendAction = Filters::Common::ExtAuthz::HeaderAppendAction; +using HeaderValueOption = Filters::Common::ExtAuthz::HeaderValueOption; +using HeaderMutation = Filters::Common::ExtAuthz::HeaderMutation; + +// Helper to create a HeaderMutation for request headers. +HeaderMutation createRequestHeaderMutation(const std::string& key, const std::string& value, + HeaderAppendAction append_action) { + return HeaderMutation{key, value, append_action}; +} + +// Helper to create a HeaderMutation for response headers. +HeaderMutation createResponseHeaderMutation(const std::string& key, const std::string& value, + HeaderAppendAction append_action) { + return HeaderMutation{key, value, append_action}; +} + constexpr char FilterConfigName[] = "ext_authz_filter"; template class HttpFilterTestBase : public T { @@ -554,54 +571,70 @@ TEST_P(InvalidMutationParamTest, InvalidMutationFields) { INSTANTIATE_TEST_SUITE_P( InvalidMutationScenarios, InvalidMutationParamTest, testing::Values( - // Invalid key tests + // Invalid key tests for request header mutations. std::make_tuple( - "HeadersToSetKey", + "RequestHeadersSetKey", [](Filters::Common::ExtAuthz::Response& r) { - r.headers_to_set = {{InvalidMutationTest::invalid_key_, "bar"}}; + r.request_header_mutations.push_back( + {InvalidMutationTest::invalid_key_, "bar", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); }, Filters::Common::ExtAuthz::CheckStatus::OK), std::make_tuple( - "HeadersToAddKey", + "RequestHeadersAddKey", [](Filters::Common::ExtAuthz::Response& r) { - r.headers_to_add = {{InvalidMutationTest::invalid_key_, "bar"}}; + r.request_header_mutations.push_back( + {InvalidMutationTest::invalid_key_, "bar", + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); }, Filters::Common::ExtAuthz::CheckStatus::OK), std::make_tuple( - "HeadersToSetKeyDenied", + "LocalResponseHeadersSetKeyDenied", [](Filters::Common::ExtAuthz::Response& r) { - r.headers_to_set = {{InvalidMutationTest::invalid_key_, "bar"}}; + r.local_response_header_mutations.push_back( + {InvalidMutationTest::invalid_key_, "bar", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); }, Filters::Common::ExtAuthz::CheckStatus::Denied), std::make_tuple( - "HeadersToAppendKey", + "RequestHeadersAppendKey", [](Filters::Common::ExtAuthz::Response& r) { - r.headers_to_append = {{InvalidMutationTest::invalid_key_, "bar"}}; + r.request_header_mutations.push_back( + {InvalidMutationTest::invalid_key_, "bar", + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); }, Filters::Common::ExtAuthz::CheckStatus::OK), + // Invalid key tests for response header mutations. std::make_tuple( - "ResponseHeadersToAddKey", + "ResponseHeadersAddKey", [](Filters::Common::ExtAuthz::Response& r) { - r.response_headers_to_set = {{InvalidMutationTest::invalid_key_, "bar"}}; + r.response_header_mutations.push_back( + {InvalidMutationTest::invalid_key_, "bar", + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); }, Filters::Common::ExtAuthz::CheckStatus::OK), std::make_tuple( - "ResponseHeadersToSetKey", + "ResponseHeadersSetKey", [](Filters::Common::ExtAuthz::Response& r) { - r.response_headers_to_set = {{InvalidMutationTest::invalid_key_, "bar"}}; + r.response_header_mutations.push_back( + {InvalidMutationTest::invalid_key_, "bar", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); }, Filters::Common::ExtAuthz::CheckStatus::OK), std::make_tuple( - "ResponseHeadersToAddIfAbsentKey", + "ResponseHeadersAddIfAbsentKey", [](Filters::Common::ExtAuthz::Response& r) { - r.response_headers_to_add_if_absent = {{InvalidMutationTest::invalid_key_, "bar"}}; + r.response_header_mutations.push_back( + {InvalidMutationTest::invalid_key_, "bar", + Filters::Common::ExtAuthz::HeaderValueOption::ADD_IF_ABSENT}); }, Filters::Common::ExtAuthz::CheckStatus::OK), std::make_tuple( - "ResponseHeadersToOverwriteIfExistsKey", + "ResponseHeadersOverwriteIfExistsKey", [](Filters::Common::ExtAuthz::Response& r) { - r.response_headers_to_overwrite_if_exists = { - {InvalidMutationTest::invalid_key_, "bar"}}; + r.response_header_mutations.push_back( + {InvalidMutationTest::invalid_key_, "bar", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS}); }, Filters::Common::ExtAuthz::CheckStatus::OK), std::make_tuple( @@ -610,55 +643,70 @@ INSTANTIATE_TEST_SUITE_P( r.query_parameters_to_set = {{"f o o", "bar"}}; }, Filters::Common::ExtAuthz::CheckStatus::OK), - // Invalid value tests + // Invalid value tests for request header mutations. std::make_tuple( - "HeadersToSetValue", + "RequestHeadersSetValue", [](Filters::Common::ExtAuthz::Response& r) { - r.headers_to_set = {{"foo", InvalidMutationTest::getInvalidValue()}}; + r.request_header_mutations.push_back( + {"foo", InvalidMutationTest::getInvalidValue(), + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); }, Filters::Common::ExtAuthz::CheckStatus::OK), std::make_tuple( - "HeadersToAddValue", + "RequestHeadersAddValue", [](Filters::Common::ExtAuthz::Response& r) { - r.headers_to_add = {{"foo", InvalidMutationTest::getInvalidValue()}}; + r.request_header_mutations.push_back( + {"foo", InvalidMutationTest::getInvalidValue(), + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); }, Filters::Common::ExtAuthz::CheckStatus::OK), std::make_tuple( - "HeadersToSetValueDenied", + "LocalResponseHeadersSetValueDenied", [](Filters::Common::ExtAuthz::Response& r) { - r.headers_to_set = {{"foo", InvalidMutationTest::getInvalidValue()}}; + r.local_response_header_mutations.push_back( + {"foo", InvalidMutationTest::getInvalidValue(), + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); }, Filters::Common::ExtAuthz::CheckStatus::Denied), std::make_tuple( - "HeadersToAppendValue", + "RequestHeadersAppendValue", [](Filters::Common::ExtAuthz::Response& r) { - r.headers_to_append = {{"foo", InvalidMutationTest::getInvalidValue()}}; + r.request_header_mutations.push_back( + {"foo", InvalidMutationTest::getInvalidValue(), + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); }, Filters::Common::ExtAuthz::CheckStatus::OK), + // Invalid value tests for response header mutations. std::make_tuple( - "ResponseHeadersToAddValue", + "ResponseHeadersAddValue", [](Filters::Common::ExtAuthz::Response& r) { - r.response_headers_to_set = {{"foo", InvalidMutationTest::getInvalidValue()}}; + r.response_header_mutations.push_back( + {"foo", InvalidMutationTest::getInvalidValue(), + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); }, Filters::Common::ExtAuthz::CheckStatus::OK), std::make_tuple( - "ResponseHeadersToSetValue", + "ResponseHeadersSetValue", [](Filters::Common::ExtAuthz::Response& r) { - r.response_headers_to_set = {{"foo", InvalidMutationTest::getInvalidValue()}}; + r.response_header_mutations.push_back( + {"foo", InvalidMutationTest::getInvalidValue(), + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); }, Filters::Common::ExtAuthz::CheckStatus::OK), std::make_tuple( - "ResponseHeadersToAddIfAbsentValue", + "ResponseHeadersAddIfAbsentValue", [](Filters::Common::ExtAuthz::Response& r) { - r.response_headers_to_add_if_absent = { - {"foo", InvalidMutationTest::getInvalidValue()}}; + r.response_header_mutations.push_back( + {"foo", InvalidMutationTest::getInvalidValue(), + Filters::Common::ExtAuthz::HeaderValueOption::ADD_IF_ABSENT}); }, Filters::Common::ExtAuthz::CheckStatus::OK), std::make_tuple( - "ResponseHeadersToOverwriteIfExistsValue", + "ResponseHeadersOverwriteIfExistsValue", [](Filters::Common::ExtAuthz::Response& r) { - r.response_headers_to_overwrite_if_exists = { - {"foo", InvalidMutationTest::getInvalidValue()}}; + r.response_header_mutations.push_back( + {"foo", InvalidMutationTest::getInvalidValue(), + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS}); }, Filters::Common::ExtAuthz::CheckStatus::OK), std::make_tuple( @@ -675,7 +723,9 @@ INSTANTIATE_TEST_SUITE_P( TEST_F(InvalidMutationTest, BasicInvalidKey) { Filters::Common::ExtAuthz::Response response; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.headers_to_set = {{invalid_key_, "bar"}}; + response.request_header_mutations.push_back( + {invalid_key_, "bar", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); testResponse(response); } @@ -689,12 +739,13 @@ TEST_F(InvalidMutationTest, InvalidHeaderAppendAction) { struct DecoderHeaderMutationRulesTestOpts { absl::optional rules; bool expect_reject_response = false; - Filters::Common::ExtAuthz::UnsafeHeaderVector allowed_headers_to_add; - Filters::Common::ExtAuthz::UnsafeHeaderVector disallowed_headers_to_add; - Filters::Common::ExtAuthz::UnsafeHeaderVector allowed_headers_to_append; - Filters::Common::ExtAuthz::UnsafeHeaderVector disallowed_headers_to_append; - Filters::Common::ExtAuthz::UnsafeHeaderVector allowed_headers_to_set; - Filters::Common::ExtAuthz::UnsafeHeaderVector disallowed_headers_to_set; + // Key-value pairs for headers, will be converted to HeaderMutationVector with appropriate action. + std::vector> allowed_headers_to_add; + std::vector> disallowed_headers_to_add; + std::vector> allowed_headers_to_append; + std::vector> disallowed_headers_to_append; + std::vector> allowed_headers_to_set; + std::vector> disallowed_headers_to_set; std::vector allowed_headers_to_remove; std::vector disallowed_headers_to_remove; }; @@ -797,12 +848,19 @@ class DecoderHeaderMutationRulesTest } for (const auto& [key, value] : opts.allowed_headers_to_append) { - EXPECT_EQ(request_headers_.get_(Http::LowerCaseString(key)), - absl::StrCat("will be appended to,", value)) + // APPEND_IF_EXISTS_OR_ADD uses addCopy() which creates duplicate entries. + // Check that both the original and appended values exist. + auto headers = request_headers_.get(Http::LowerCaseString(key)); + ASSERT_EQ(headers.size(), 2) << "(key: '" << key << "')"; + EXPECT_EQ(headers[0]->value().getStringView(), "will be appended to") << "(key: '" << key << "')"; + EXPECT_EQ(headers[1]->value().getStringView(), value) << "(key: '" << key << "')"; } for (const auto& [key, value] : opts.disallowed_headers_to_append) { - EXPECT_EQ(request_headers_.get_(Http::LowerCaseString(key)), "will not be appended to") + // Disallowed headers should not have the appended value. + auto headers = request_headers_.get(Http::LowerCaseString(key)); + ASSERT_EQ(headers.size(), 1) << "(key: '" << key << "')"; + EXPECT_EQ(headers[0]->value().getStringView(), "will not be appended to") << "(key: '" << key << "')"; } @@ -822,19 +880,22 @@ class DecoderHeaderMutationRulesTest for (const auto& vec : {opts.allowed_headers_to_add, opts.disallowed_headers_to_add}) { for (const auto& [key, value] : vec) { - response.headers_to_add.emplace_back(key, value); + response.request_header_mutations.push_back( + {key, value, Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); } } for (const auto& vec : {opts.allowed_headers_to_set, opts.disallowed_headers_to_set}) { for (const auto& [key, value] : vec) { - response.headers_to_set.emplace_back(key, value); + response.request_header_mutations.push_back( + {key, value, Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); } } for (const auto& vec : {opts.allowed_headers_to_append, opts.disallowed_headers_to_append}) { for (const auto& [key, value] : vec) { - response.headers_to_append.emplace_back(key, value); + response.request_header_mutations.push_back( + {key, value, Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); } } @@ -1280,8 +1341,13 @@ TEST_F(HttpFilterTest, ErrorResponseWithCustomAttributes) { response.status = Filters::Common::ExtAuthz::CheckStatus::Error; response.status_code = Http::Code::InternalServerError; response.body = "{\"error\": \"auth service unavailable\"}"; - response.headers_to_set.emplace_back("x-error-code", "AUTH_SERVICE_ERROR"); - response.headers_to_set.emplace_back("x-error-message", "Internal auth service error"); + // For error responses, headers go to local_response_header_mutations. + response.local_response_header_mutations.push_back( + {"x-error-code", "AUTH_SERVICE_ERROR", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + response.local_response_header_mutations.push_back( + {"x-error-message", "Internal auth service error", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); request_callbacks_->onComplete(std::make_unique(response)); EXPECT_EQ(1U, config_->stats().error_.value()); EXPECT_EQ("ext_authz_error", decoder_filter_callbacks_.details()); @@ -1318,7 +1384,10 @@ TEST_F(HttpFilterTest, ErrorResponseWithFailureModeAllow) { response.status = Filters::Common::ExtAuthz::CheckStatus::Error; response.status_code = Http::Code::InternalServerError; response.body = "{\"error\": \"auth service unavailable\"}"; - response.headers_to_set.emplace_back("x-error-code", "AUTH_SERVICE_ERROR"); + // For error responses, headers go to local_response_header_mutations. + response.local_response_header_mutations.push_back( + {"x-error-code", "AUTH_SERVICE_ERROR", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); request_callbacks_->onComplete(std::make_unique(response)); EXPECT_EQ(1U, config_->stats().error_.value()); EXPECT_EQ(1U, config_->stats().failure_mode_allowed_.value()); @@ -1361,8 +1430,10 @@ TEST_F(HttpFilterTest, ErrorResponseWithInvalidHeaders) { response.status = Filters::Common::ExtAuthz::CheckStatus::Error; response.status_code = Http::Code::InternalServerError; response.body = "{\"error\": \"test\"}"; - // Add an invalid header with newlines. - response.headers_to_set.emplace_back("invalid\n\nheader", "value"); + // Add an invalid header with newlines. For error responses, use local_response_header_mutations. + response.local_response_header_mutations.push_back( + {"invalid\n\nheader", "value", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); request_callbacks_->onComplete(std::make_unique(response)); EXPECT_EQ(1U, config_->stats().error_.value()); } @@ -1403,10 +1474,15 @@ TEST_F(HttpFilterTest, ErrorResponseWithInvalidHeadersInAppend) { response.status = Filters::Common::ExtAuthz::CheckStatus::Error; response.status_code = Http::Code::ServiceUnavailable; response.body = "{\"error\": \"service error\"}"; - // Add valid header in headers_to_set. - response.headers_to_set.emplace_back("x-valid-header", "valid-value"); - // Add invalid header with newlines in headers_to_append. - response.headers_to_append.emplace_back("x-bad\nheader", "value"); + // For error responses, use local_response_header_mutations. + // Add valid header. + response.local_response_header_mutations.push_back( + {"x-valid-header", "valid-value", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + // Add invalid header with newlines. + response.local_response_header_mutations.push_back( + {"x-bad\nheader", "value", + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); request_callbacks_->onComplete(std::make_unique(response)); EXPECT_EQ(1U, config_->stats().error_.value()); } @@ -1446,8 +1522,11 @@ TEST_F(HttpFilterTest, ErrorResponseWithInvalidHeaderValue) { response.status = Filters::Common::ExtAuthz::CheckStatus::Error; response.status_code = Http::Code::InternalServerError; response.body = "{\"error\": \"test\"}"; - // Add header with invalid value (contains NULL byte). - response.headers_to_append.emplace_back("x-error-header", std::string("bad\0value", 9)); + // Add header with invalid value (contains NULL byte). For error responses, use + // local_response_header_mutations. + response.local_response_header_mutations.push_back( + {"x-error-header", std::string("bad\0value", 9), + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); request_callbacks_->onComplete(std::make_unique(response)); EXPECT_EQ(1U, config_->stats().error_.value()); } @@ -1492,8 +1571,11 @@ TEST_F(HttpFilterTest, ErrorResponseHeaderLimitsEnforced) { response.status_code = Http::Code::InternalServerError; response.body = "{\"error\": \"auth service error\"}"; // Try to add many headers to test the limit enforcement. + // For error responses, use local_response_header_mutations. for (size_t i = 0; i < 200; ++i) { - response.headers_to_set.emplace_back(fmt::format("x-error-header-{}", i), "value"); + response.local_response_header_mutations.push_back( + {fmt::format("x-error-header-{}", i), "value", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); } request_callbacks_->onComplete(std::make_unique(response)); EXPECT_EQ(1U, config_->stats().error_.value()); @@ -1517,13 +1599,23 @@ TEST_F(HttpFilterTest, ErrorResponseHeaderLimitsEnforcedWithMock) { response.status = Filters::Common::ExtAuthz::CheckStatus::Error; response.status_code = Http::Code::InternalServerError; response.body = "{\"error\": \"service error\"}"; - // Add 5 headers to set. - response.headers_to_set.push_back({"x-error-1", "value1"}); - response.headers_to_set.push_back({"x-error-2", "value2"}); - response.headers_to_set.push_back({"x-error-3", "value3"}); + // Add 5 headers to set. For error responses, use local_response_header_mutations. + response.local_response_header_mutations.push_back( + {"x-error-1", "value1", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + response.local_response_header_mutations.push_back( + {"x-error-2", "value2", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + response.local_response_header_mutations.push_back( + {"x-error-3", "value3", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); // Add 2 headers to append. - response.headers_to_append.push_back({"x-append-1", "value1"}); - response.headers_to_append.push_back({"x-append-2", "value2"}); + response.local_response_header_mutations.push_back( + {"x-append-1", "value1", + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + response.local_response_header_mutations.push_back( + {"x-append-2", "value2", + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); prepareCheck(); @@ -1561,7 +1653,7 @@ TEST_F(HttpFilterTest, ErrorResponseHeaderLimitsEnforcedWithMock) { EXPECT_GT(config_->stats().omitted_response_headers_.value(), 0); } -// Test that error response headers are limited in headers_to_append when the limit is hit. +// Test that error response headers are limited when header count limit is hit. TEST_F(HttpFilterTest, ErrorResponseHeaderLimitsEnforcedInAppend) { InSequence s; @@ -1577,12 +1669,16 @@ TEST_F(HttpFilterTest, ErrorResponseHeaderLimitsEnforcedInAppend) { response.status = Filters::Common::ExtAuthz::CheckStatus::Error; response.status_code = Http::Code::ServiceUnavailable; response.body = "{\"error\": \"unavailable\"}"; - // Add only 2 headers to set, so we have room to test append limit. - response.headers_to_set.push_back({"x-error-1", "value1"}); - // Add many headers to append to trigger the limit in the append loop. - response.headers_to_append.push_back({"x-append-1", "value1"}); - response.headers_to_append.push_back({"x-append-2", "value2"}); - response.headers_to_append.push_back({"x-append-3", "value3"}); + // Add headers via Set (which adds new headers) to trigger the limit. + response.local_response_header_mutations.push_back( + {"x-error-1", "value1", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + response.local_response_header_mutations.push_back( + {"x-error-2", "value2", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + response.local_response_header_mutations.push_back( + {"x-error-3", "value3", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); prepareCheck(); @@ -1598,24 +1694,23 @@ TEST_F(HttpFilterTest, ErrorResponseHeaderLimitsEnforcedInAppend) { Invoke([&](Http::Code, absl::string_view, std::function modify_headers, const absl::optional, absl::string_view) -> void { - // Create a ResponseHeaderMap with max_headers_count=2 to trigger limit in append loop. + // Create a ResponseHeaderMap with max_headers_count=2 to trigger limit. Http::TestResponseHeaderMapImpl response_headers({}, 99999, /*max_headers_count=*/2); if (modify_headers) { modify_headers(response_headers); } - // With a limit of 2, we should have 1 from set + 1 from append. + // With a limit of 2, we should have first 2 headers. EXPECT_EQ(response_headers.size(), 2); EXPECT_TRUE(response_headers.has("x-error-1")); - EXPECT_TRUE(response_headers.has("x-append-1")); + EXPECT_TRUE(response_headers.has("x-error-2")); // The rest should be omitted due to the limit. - EXPECT_FALSE(response_headers.has("x-append-2")); - EXPECT_FALSE(response_headers.has("x-append-3")); + EXPECT_FALSE(response_headers.has("x-error-3")); })); EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark, filter_->decodeHeaders(request_headers_, true)); EXPECT_EQ(1U, config_->stats().error_.value()); - // Verify that omitted_response_headers_ stat was incremented in the append loop. + // Verify that omitted_response_headers_ stat was incremented. EXPECT_GT(config_->stats().omitted_response_headers_.value(), 0); } @@ -1649,7 +1744,10 @@ TEST_F(HttpFilterTest, ErrorResponseBodySizeLimit) { response.status_code = Http::Code::InternalServerError; // Body is longer than 10 bytes, should be truncated. response.body = "This is a very long error message that exceeds the limit"; - response.headers_to_set.emplace_back("x-error-code", "ERROR"); + // For error responses, headers go to local_response_header_mutations. + response.local_response_header_mutations.push_back( + {"x-error-code", "ERROR", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); request_callbacks_->onComplete(std::make_unique(response)); EXPECT_EQ(1U, config_->stats().error_.value()); } @@ -1691,7 +1789,7 @@ TEST_F(HttpFilterTest, ErrorResponseEmptyAttributes) { EXPECT_EQ(1U, config_->stats().error_.value()); } -// Test error response with headers_to_append. +// Test error response with multiple header mutation types. TEST_F(HttpFilterTest, ErrorResponseWithAppendHeaders) { InSequence s; @@ -1718,20 +1816,24 @@ TEST_F(HttpFilterTest, ErrorResponseWithAppendHeaders) { .WillOnce(Invoke([&](const Http::ResponseHeaderMap& headers, bool) -> void { EXPECT_EQ(headers.getStatusValue(), std::to_string(enumToInt(Http::Code::InternalServerError))); - // Verify both headers_to_set and headers_to_append are present. + // Verify both headers_to_set and headers_to_add are present. EXPECT_EQ(headers.get(Http::LowerCaseString("x-error-set"))[0]->value().getStringView(), "set-value"); - EXPECT_EQ(headers.get(Http::LowerCaseString("x-error-append"))[0]->value().getStringView(), - "append-value"); + EXPECT_EQ(headers.get(Http::LowerCaseString("x-error-add"))[0]->value().getStringView(), + "add-value"); })); Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::Error; response.status_code = Http::Code::InternalServerError; response.body = "{\"error\": \"auth service error\"}"; - // Add both set and append headers. - response.headers_to_set.emplace_back("x-error-set", "set-value"); - response.headers_to_append.emplace_back("x-error-append", "append-value"); + // Add headers with Set and Add actions for local reply. + response.local_response_header_mutations.push_back( + {"x-error-set", "set-value", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + response.local_response_header_mutations.push_back( + {"x-error-add", "add-value", + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); request_callbacks_->onComplete(std::make_unique(response)); EXPECT_EQ(1U, config_->stats().error_.value()); } @@ -2340,8 +2442,10 @@ TEST_F(HttpFilterTest, ClearCache) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.headers_to_append = {{"foo", "bar"}}; - response.headers_to_set = {{"bar", "foo"}}; + response.request_header_mutations.push_back( + createRequestHeaderMutation("foo", "bar", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); + response.request_header_mutations.push_back( + createRequestHeaderMutation("bar", "foo", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); response.headers_to_remove = std::vector{"remove-me"}; request_callbacks_->onComplete(std::make_unique(response)); EXPECT_EQ(1U, decoder_filter_callbacks_.clusterInfo() @@ -2385,7 +2489,8 @@ TEST_F(HttpFilterTest, ClearCacheRouteHeadersToAppendOnly) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.headers_to_append = {{"foo", "bar"}}; + response.request_header_mutations.push_back( + createRequestHeaderMutation("foo", "bar", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); request_callbacks_->onComplete(std::make_unique(response)); EXPECT_EQ(1U, decoder_filter_callbacks_.clusterInfo() ->statsScope() @@ -2428,7 +2533,8 @@ TEST_F(HttpFilterTest, ClearCacheRouteHeadersToAddOnly) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.headers_to_set = {{"foo", "bar"}}; + response.request_header_mutations.push_back( + createRequestHeaderMutation("foo", "bar", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); request_callbacks_->onComplete(std::make_unique(response)); EXPECT_EQ(1U, decoder_filter_callbacks_.clusterInfo() ->statsScope() @@ -2649,7 +2755,10 @@ TEST_F(RequestHeaderLimitTest, HeadersToSetCount) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.headers_to_set = {{"foo", "bar"}, {"foo2", "bar2"}}; + response.request_header_mutations.push_back( + createRequestHeaderMutation("foo", "bar", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); + response.request_header_mutations.push_back( + createRequestHeaderMutation("foo2", "bar2", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); runTest(request_headers, response); } @@ -2665,7 +2774,10 @@ TEST_F(RequestHeaderLimitTest, HeadersToSetSize) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.headers_to_set = {{"foo", "bar"}, {"foo2", std::string(9999, 'a')}}; + response.request_header_mutations.push_back( + createRequestHeaderMutation("foo", "bar", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); + response.request_header_mutations.push_back(createRequestHeaderMutation( + "foo2", std::string(9999, 'a'), HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); runTest(request_headers, response); } @@ -2683,7 +2795,8 @@ TEST_F(RequestHeaderLimitTest, HeadersToAppendSize) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.headers_to_append = {{"foo", std::string(9999, 'a')}}; + response.request_header_mutations.push_back(createRequestHeaderMutation( + "foo", std::string(9999, 'a'), HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); runTest(request_headers, response); } @@ -2699,7 +2812,10 @@ TEST_F(RequestHeaderLimitTest, HeadersToAddCount) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.headers_to_add = {{"foo", "bar"}, {"foo2", "bar2"}}; + response.request_header_mutations.push_back( + createRequestHeaderMutation("foo", "bar", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); + response.request_header_mutations.push_back( + createRequestHeaderMutation("foo2", "bar2", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); runTest(request_headers, response); } @@ -2715,7 +2831,8 @@ TEST_F(RequestHeaderLimitTest, HeadersToAddSize) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.headers_to_add = {{"foo2", std::string(9999, 'a')}}; + response.request_header_mutations.push_back(createRequestHeaderMutation( + "foo2", std::string(9999, 'a'), HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); runTest(request_headers, response); } @@ -2753,7 +2870,8 @@ TEST_F(HttpFilterTest, DownstreamRequestFailsOnHeaderSizeLimit) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; // A very large header that will cause the request headers to exceed their limit. - response.headers_to_set = {{"too-big", std::string(10 * 1024, 'a')}}; + response.request_header_mutations.push_back(createRequestHeaderMutation( + "too-big", std::string(10 * 1024, 'a'), HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); // Now the test should fail, since we expect the downstream request to fail. EXPECT_CALL(decoder_filter_callbacks_.stream_info_, @@ -2840,8 +2958,10 @@ TEST_F(HttpFilterTest, NoClearCacheRouteConfig) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.headers_to_append = {{"foo", "bar"}}; - response.headers_to_set = {{"bar", "foo"}}; + response.request_header_mutations.push_back( + createRequestHeaderMutation("foo", "bar", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); + response.request_header_mutations.push_back( + createRequestHeaderMutation("bar", "foo", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); request_callbacks_->onComplete(std::make_unique(response)); EXPECT_EQ(1U, decoder_filter_callbacks_.clusterInfo() ->statsScope() @@ -2866,7 +2986,9 @@ TEST_F(HttpFilterTest, NoClearCacheRouteDeniedResponse) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::Denied; response.status_code = Http::Code::Unauthorized; - response.headers_to_set = {{"foo", "bar"}}; + // For denied responses, headers go to local_response_header_mutations. + response.local_response_header_mutations.push_back( + {"foo", "bar", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); auto response_ptr = std::make_unique(response); EXPECT_CALL(*client_, check(_, _, testing::A(), _)) @@ -3969,7 +4091,9 @@ TEST_P(HttpFilterTestParam, ImmediateDeniedResponseWithHttpAttributes) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::Denied; response.status_code = Http::Code::Unauthorized; - response.headers_to_set = {{"foo", "bar"}}; + // For denied responses, headers go to local_response_header_mutations. + response.local_response_header_mutations.push_back( + {"foo", "bar", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); response.body = std::string{"baz"}; auto response_ptr = std::make_unique(response); @@ -4016,14 +4140,21 @@ TEST_P(HttpFilterTestParam, ImmediateOkResponseWithHttpAttributes) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.headers_to_append = {{request_header_key.get(), "bar"}}; - response.headers_to_set = {{key_to_add.get(), "foo"}, {key_to_override.get(), "bar"}}; + response.request_header_mutations.push_back(createRequestHeaderMutation( + request_header_key.get(), "bar", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); + response.request_header_mutations.push_back(createRequestHeaderMutation( + key_to_add.get(), "foo", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); + response.request_header_mutations.push_back(createRequestHeaderMutation( + key_to_override.get(), "bar", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); response.headers_to_remove = {key_to_remove.get()}; // This cookie will be appended to the encoded headers. - response.response_headers_to_add = {{"set-cookie", "cookie2=gingerbread"}}; + response.response_header_mutations.push_back(createResponseHeaderMutation( + "set-cookie", "cookie2=gingerbread", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); // This "should-be-overridden" header value from the auth server will override the // "should-be-overridden" entry from the upstream server. - response.response_headers_to_set = {{"should-be-overridden", "finally-set-by-auth-server"}}; + response.response_header_mutations.push_back( + createResponseHeaderMutation("should-be-overridden", "finally-set-by-auth-server", + HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); auto response_ptr = std::make_unique(response); @@ -4038,7 +4169,11 @@ TEST_P(HttpFilterTestParam, ImmediateOkResponseWithHttpAttributes) { 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_.get_(request_header_key), "foo,bar"); + // APPEND_IF_EXISTS_OR_ADD uses addCopy() which creates duplicate entries. + auto baz_headers = request_headers_.get(request_header_key); + ASSERT_EQ(baz_headers.size(), 2); + EXPECT_EQ(baz_headers[0]->value().getStringView(), "foo"); + EXPECT_EQ(baz_headers[1]->value().getStringView(), "bar"); EXPECT_EQ(request_headers_.get_(key_to_add), "foo"); EXPECT_EQ(request_headers_.get_(key_to_override), "bar"); EXPECT_EQ(request_headers_.has(key_to_remove), false); @@ -4054,11 +4189,11 @@ TEST_P(HttpFilterTestParam, ImmediateOkResponseWithHttpAttributes) { 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)); - EXPECT_EQ(Http::HeaderUtility::getAllOfHeaderAsString(response_headers, - Http::LowerCaseString{"set-cookie"}) - .result() - .value(), - "cookie1=snickerdoodle,cookie2=gingerbread"); + // Response headers also use addCopy() for APPEND_IF_EXISTS_OR_ADD, creating duplicate entries. + auto set_cookie_headers = response_headers.get(Http::LowerCaseString{"set-cookie"}); + ASSERT_EQ(set_cookie_headers.size(), 2); + EXPECT_EQ(set_cookie_headers[0]->value().getStringView(), "cookie1=snickerdoodle"); + EXPECT_EQ(set_cookie_headers[1]->value().getStringView(), "cookie2=gingerbread"); EXPECT_EQ(response_headers.get_("should-be-overridden"), "finally-set-by-auth-server"); } @@ -4069,9 +4204,10 @@ TEST_P(HttpFilterTestParam, OkWithResponseHeadersAndAppendActions) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.response_headers_to_add_if_absent = {{"header-to-add-if-absent", "new-value"}}; - response.response_headers_to_overwrite_if_exists = { - {"header-to-overwrite-if-exists", "new-value"}}; + response.response_header_mutations.push_back(createResponseHeaderMutation( + "header-to-add-if-absent", "new-value", HeaderValueOption::ADD_IF_ABSENT)); + response.response_header_mutations.push_back(createResponseHeaderMutation( + "header-to-overwrite-if-exists", "new-value", HeaderValueOption::OVERWRITE_IF_EXISTS)); auto response_ptr = std::make_unique(response); @@ -4100,6 +4236,50 @@ TEST_P(HttpFilterTestParam, OkWithResponseHeadersAndAppendActions) { EXPECT_EQ(response_headers.get_("header-to-overwrite-if-exists"), "new-value"); } +// Covers Append action in encodeHeaders when the header exists and when it does not. +TEST_P(HttpFilterTestParam, OkResponseHeadersAppendActionsAppendAndAdd) { + InSequence s; + + prepareCheck(); + + Filters::Common::ExtAuthz::Response response{}; + response.status = Filters::Common::ExtAuthz::CheckStatus::OK; + response.response_header_mutations.push_back(createResponseHeaderMutation( + "append-existing", "appended", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); + response.response_header_mutations.push_back(createResponseHeaderMutation( + "append-new", "added", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); + + 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(decoder_filter_callbacks_, continueDecoding()).Times(0); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); + + Buffer::OwnedImpl response_data{}; + Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}, + {"append-existing", "initial"}}; + 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)); + + // APPEND_IF_EXISTS_OR_ADD uses addCopy() which creates duplicate entries. + auto append_existing_headers = response_headers.get(Http::LowerCaseString{"append-existing"}); + ASSERT_EQ(append_existing_headers.size(), 2); + EXPECT_EQ(append_existing_headers[0]->value().getStringView(), "initial"); + EXPECT_EQ(append_existing_headers[1]->value().getStringView(), "appended"); + // Append to non-existing should add the header. + EXPECT_EQ(response_headers.get_("append-new"), "added"); +} + TEST_P(HttpFilterTestParam, OkWithResponseHeadersAndAppendActionsDoNotTakeEffect) { InSequence s; @@ -4107,9 +4287,10 @@ TEST_P(HttpFilterTestParam, OkWithResponseHeadersAndAppendActionsDoNotTakeEffect Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.response_headers_to_add_if_absent = {{"header-to-add-if-absent", "new-value"}}; - response.response_headers_to_overwrite_if_exists = { - {"header-to-overwrite-if-exists", "new-value"}}; + response.response_header_mutations.push_back(createResponseHeaderMutation( + "header-to-add-if-absent", "new-value", HeaderValueOption::ADD_IF_ABSENT)); + response.response_header_mutations.push_back(createResponseHeaderMutation( + "header-to-overwrite-if-exists", "new-value", HeaderValueOption::OVERWRITE_IF_EXISTS)); auto response_ptr = std::make_unique(response); @@ -4146,6 +4327,40 @@ TEST_P(HttpFilterTestParam, ImmediateOkResponseWithUnmodifiedQueryParameters) { queryParameterTest(original_path, expected_path, add_me, remove_me); } +// Validate that invalid header removals are ignored when validate_mutations is enabled. +TEST_P(HttpFilterTestParam, OkIgnoresInvalidHeaderRemovalWhenValidated) { + InSequence s; + + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_authz_server" + validate_mutations: true + )EOF"); + + prepareCheck(); + + // Add a header that should remain because the removal key is invalid. + request_headers_.addCopy("keep-me", "yes"); + + Filters::Common::ExtAuthz::Response response{}; + response.status = Filters::Common::ExtAuthz::CheckStatus::OK; + response.headers_to_remove.push_back("invalid\nheader"); + + 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(decoder_filter_callbacks_, continueDecoding()).Times(0); + + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); + EXPECT_EQ(request_headers_.get_("keep-me"), "yes"); +} + TEST_P(HttpFilterTestParam, ImmediateOkResponseWithRepeatedUnmodifiedQueryParameters) { const std::string original_path{"/users?leave-me=alone&leave-me=in-peace"}; const std::string expected_path{"/users?leave-me=alone&leave-me=in-peace"}; @@ -4349,7 +4564,11 @@ TEST_P(HttpFilterTestParam, DestroyResponseBeforeSendLocalReply) { response.status = Filters::Common::ExtAuthz::CheckStatus::Denied; response.status_code = Http::Code::Forbidden; response.body = std::string{"foo"}; - response.headers_to_set = {{"foo", "bar"}, {"bar", "foo"}}; + // For denied responses, headers go to local_response_header_mutations. + response.local_response_header_mutations.push_back( + createRequestHeaderMutation("foo", "bar", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); + response.local_response_header_mutations.push_back( + createRequestHeaderMutation("bar", "foo", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); Filters::Common::ExtAuthz::ResponsePtr response_ptr = std::make_unique(response); @@ -4406,11 +4625,18 @@ TEST_P(HttpFilterTestParam, OverrideEncodingHeaders) { response.status = Filters::Common::ExtAuthz::CheckStatus::Denied; response.status_code = Http::Code::Forbidden; response.body = std::string{"foo"}; - response.headers_to_set = {{"foo", "bar"}, - {"bar", "foo"}, - {"set-cookie", "cookie1=value"}, - {"set-cookie", "cookie2=value"}, - {"accept-encoding", "gzip,deflate"}}; + // For denied responses, headers go to local_response_header_mutations. + // Use Add action for set-cookie headers to allow multiple values. + response.local_response_header_mutations.push_back( + createRequestHeaderMutation("foo", "bar", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); + response.local_response_header_mutations.push_back( + createRequestHeaderMutation("bar", "foo", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); + response.local_response_header_mutations.push_back(createRequestHeaderMutation( + "set-cookie", "cookie1=value", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); + response.local_response_header_mutations.push_back(createRequestHeaderMutation( + "set-cookie", "cookie2=value", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); + response.local_response_header_mutations.push_back(createRequestHeaderMutation( + "accept-encoding", "gzip,deflate", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); Filters::Common::ExtAuthz::ResponsePtr response_ptr = std::make_unique(response); @@ -4502,7 +4728,8 @@ TEST_F(HttpFilterTest, EmitDynamicMetadata) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.headers_to_set = {{"foo", "bar"}}; + response.request_header_mutations.push_back( + createRequestHeaderMutation("foo", "bar", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); (*response.dynamic_metadata.mutable_fields())["ext_authz_duration"] = ext_authz_duration_value; initializeMetadata(response); @@ -4547,7 +4774,8 @@ TEST_F(HttpFilterTest, EmitDynamicMetadataWhenDenied) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::Denied; response.status_code = Http::Code::Unauthorized; - response.headers_to_set = {{"foo", "bar"}}; + response.request_header_mutations.push_back( + createRequestHeaderMutation("foo", "bar", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); initializeMetadata(response); @@ -5854,7 +6082,8 @@ TEST_F(HttpFilterTest, HttpClientPerRouteOverride) { TEST_F(InvalidMutationTest, InvalidResponseHeadersToAddName) { Filters::Common::ExtAuthz::Response r; r.status = Filters::Common::ExtAuthz::CheckStatus::OK; - r.response_headers_to_add = {{"invalid header name", "value"}}; + r.response_header_mutations.push_back(createResponseHeaderMutation( + "invalid header name", "value", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); testResponse(r); } @@ -5862,7 +6091,8 @@ TEST_F(InvalidMutationTest, InvalidResponseHeadersToAddName) { TEST_F(InvalidMutationTest, InvalidResponseHeadersToAddValue) { Filters::Common::ExtAuthz::Response r; r.status = Filters::Common::ExtAuthz::CheckStatus::OK; - r.response_headers_to_add = {{"valid-name", getInvalidValue()}}; + r.response_header_mutations.push_back(createResponseHeaderMutation( + "valid-name", getInvalidValue(), HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); testResponse(r); } @@ -5976,9 +6206,12 @@ class ResponseHeaderLimitTest : public HttpFilterTest { TEST_F(ResponseHeaderLimitTest, EncodeHeadersToAddExceedsCountLimit) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.response_headers_to_add.push_back({"key1", "value1"}); - response.response_headers_to_add.push_back({"key2", "value2"}); - response.response_headers_to_add.push_back({"key3", "value3"}); + response.response_header_mutations.push_back( + {"key1", "value1", Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + response.response_header_mutations.push_back( + {"key2", "value2", Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + response.response_header_mutations.push_back( + {"key3", "value3", Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); Http::TestResponseHeaderMapImpl response_headers( {{":status", "200"}, {"existing-header", "value"}}, /*max_headers_kb=*/99999, @@ -5990,9 +6223,13 @@ TEST_F(ResponseHeaderLimitTest, EncodeHeadersToAddExceedsCountLimit) { TEST_F(ResponseHeaderLimitTest, EncodeHeadersToAddExceedsSizeLimit) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.response_headers_to_add.push_back({"key1", "value1"}); - response.response_headers_to_add.push_back({"key2", "value2"}); - response.response_headers_to_add.push_back({"key3", std::string(9999, 'a')}); + response.response_header_mutations.push_back( + {"key1", "value1", Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + response.response_header_mutations.push_back( + {"key2", "value2", Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + response.response_header_mutations.push_back( + {"key3", std::string(9999, 'a'), + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); Http::TestResponseHeaderMapImpl response_headers( {{":status", "200"}, {"existing-header", "value"}}, /*max_headers_kb=*/1, @@ -6006,9 +6243,15 @@ TEST_F(ResponseHeaderLimitTest, EncodeHeadersToAddExceedsSizeLimit) { TEST_F(ResponseHeaderLimitTest, EncodeHeadersToSetExceedsCountLimit) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.response_headers_to_set.push_back({"existing-header-to-overwrite", "new-value"}); - response.response_headers_to_set.push_back({"new-header-to-add", "value"}); - response.response_headers_to_set.push_back({"another-new-header", "value"}); + response.response_header_mutations.push_back( + {"existing-header-to-overwrite", "new-value", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + response.response_header_mutations.push_back( + {"new-header-to-add", "value", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + response.response_header_mutations.push_back( + {"another-new-header", "value", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); Http::TestResponseHeaderMapImpl response_headers( {{":status", "200"}, {"existing-header-to-overwrite", "old-value"}}, /*max_headers_kb=*/99999, @@ -6020,10 +6263,13 @@ TEST_F(ResponseHeaderLimitTest, EncodeHeadersToSetExceedsCountLimit) { TEST_F(ResponseHeaderLimitTest, EncodeHeadersToSetExceedsSizeLimit) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.response_headers_to_set.push_back( - {"existing-header-to-overwrite", std::string(9999, 'a')}); - response.response_headers_to_set.push_back({"new-header-to-add", "value"}); - response.response_headers_to_set.push_back({"another-new-header", "value"}); + response.response_header_mutations.push_back( + createResponseHeaderMutation("existing-header-to-overwrite", std::string(9999, 'a'), + HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); + response.response_header_mutations.push_back(createResponseHeaderMutation( + "new-header-to-add", "value", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); + response.response_header_mutations.push_back(createResponseHeaderMutation( + "another-new-header", "value", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); Http::TestResponseHeaderMapImpl response_headers( {{":status", "200"}, {"existing-header-to-overwrite", "old-value"}}, /*max_headers_kb=*/1, @@ -6037,9 +6283,12 @@ TEST_F(ResponseHeaderLimitTest, EncodeHeadersToSetExceedsSizeLimit) { TEST_F(ResponseHeaderLimitTest, EncodeHeadersToAddIfAbsentExceedsCountLimit) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.response_headers_to_add_if_absent.push_back({"key1", "value1"}); - response.response_headers_to_add_if_absent.push_back({"key2", "value2"}); - response.response_headers_to_add_if_absent.push_back({"existing-header", "value"}); + response.response_header_mutations.push_back( + {"key1", "value1", Filters::Common::ExtAuthz::HeaderValueOption::ADD_IF_ABSENT}); + response.response_header_mutations.push_back( + {"key2", "value2", Filters::Common::ExtAuthz::HeaderValueOption::ADD_IF_ABSENT}); + response.response_header_mutations.push_back( + {"existing-header", "value", Filters::Common::ExtAuthz::HeaderValueOption::ADD_IF_ABSENT}); Http::TestResponseHeaderMapImpl response_headers( {{":status", "200"}, {"existing-header", "value"}}, /*max_headers_kb=*/99999, @@ -6051,7 +6300,8 @@ TEST_F(ResponseHeaderLimitTest, EncodeHeadersToAddIfAbsentExceedsCountLimit) { TEST_F(ResponseHeaderLimitTest, EncodeHeadersToAddIfAbsentExceedsSizeLimit) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.response_headers_to_add_if_absent.push_back({"foo", std::string(9999, 'a')}); + response.response_header_mutations.push_back( + {"foo", std::string(9999, 'a'), Filters::Common::ExtAuthz::HeaderValueOption::ADD_IF_ABSENT}); Http::TestResponseHeaderMapImpl response_headers( {{":status", "200"}, {"existing-header", "value"}}, /*max_headers_kb=*/1, @@ -6075,10 +6325,14 @@ TEST_F(HttpFilterTest, EncodeHeadersLimitDisabledByDefault) { // any one of these headers would be rejected on the basis of their size, they collectively would // be rejected due to the resulting header count. const std::string big_value(9999, 'a'); - response.response_headers_to_add.push_back({"add", big_value}); - response.response_headers_to_set.push_back({"set", big_value}); - response.response_headers_to_add_if_absent.push_back({"add-if-absent", big_value}); - response.response_headers_to_overwrite_if_exists.push_back({"overwrite-if-exists", big_value}); + response.response_header_mutations.push_back( + createResponseHeaderMutation("add", big_value, HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); + response.response_header_mutations.push_back(createResponseHeaderMutation( + "set", big_value, HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); + response.response_header_mutations.push_back( + createResponseHeaderMutation("add-if-absent", big_value, HeaderValueOption::ADD_IF_ABSENT)); + response.response_header_mutations.push_back(createResponseHeaderMutation( + "overwrite-if-exists", big_value, HeaderValueOption::OVERWRITE_IF_EXISTS)); prepareCheck(); @@ -6108,9 +6362,11 @@ TEST_F(HttpFilterTest, EncodeHeadersLimitDisabledByDefault) { TEST_F(ResponseHeaderLimitTest, EncodeHeadersToOverwriteIfExistsExceedsSizeLimit) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.response_headers_to_overwrite_if_exists.push_back( - {"existing-header-to-overwrite", std::string(9999, 'a')}); - response.response_headers_to_overwrite_if_exists.push_back({"non-existing-header", "value"}); + response.response_header_mutations.push_back( + createResponseHeaderMutation("existing-header-to-overwrite", std::string(9999, 'a'), + HeaderValueOption::OVERWRITE_IF_EXISTS)); + response.response_header_mutations.push_back(createResponseHeaderMutation( + "non-existing-header", "value", HeaderValueOption::OVERWRITE_IF_EXISTS)); Http::TestResponseHeaderMapImpl response_headers({{":status", "200"}, {"existing-header", "value"}, @@ -6136,9 +6392,13 @@ TEST_F(HttpFilterTest, DeniedResponseLocalReplyExceedsLimit) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::Denied; response.status_code = Http::Code::Unauthorized; - response.headers_to_set.push_back({"key1", "value1"}); - response.headers_to_set.push_back({"key2", "value2"}); - response.headers_to_set.push_back({"key3", "value3"}); + // For denied responses, headers go to local_response_header_mutations. + response.local_response_header_mutations.push_back( + {"key1", "value1", Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + response.local_response_header_mutations.push_back( + {"key2", "value2", Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + response.local_response_header_mutations.push_back( + {"key3", "value3", Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); prepareCheck(); @@ -6182,9 +6442,13 @@ TEST_F(HttpFilterTest, DeniedResponseLocalReplyExceedsLimitDisabled) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::Denied; response.status_code = Http::Code::Unauthorized; - response.headers_to_set.push_back({"key1", "value1"}); - response.headers_to_set.push_back({"key2", "value2"}); - response.headers_to_set.push_back({"key3", "value3"}); + // For denied responses, headers go to local_response_header_mutations. + response.local_response_header_mutations.push_back( + {"key1", "value1", Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + response.local_response_header_mutations.push_back( + {"key2", "value2", Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + response.local_response_header_mutations.push_back( + {"key3", "value3", Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); prepareCheck(); @@ -6247,8 +6511,10 @@ TEST_F(HttpFilterTest, SetCookieHeaderOnSuccessfulAuthorization) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.response_headers_to_add = {{"set-cookie", "session=abc123"}, - {"x-custom-header", "custom-value"}}; + response.response_header_mutations.push_back(createResponseHeaderMutation( + "set-cookie", "session=abc123", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); + response.response_header_mutations.push_back(createResponseHeaderMutation( + "x-custom-header", "custom-value", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); request_callbacks_->onComplete(std::make_unique(response)); EXPECT_EQ(1U, config_->stats().ok_.value()); @@ -6304,8 +6570,12 @@ TEST_F(HttpFilterTest, SetCookieHeaderOnDeniedAuthorization) { response.status = Filters::Common::ExtAuthz::CheckStatus::Denied; response.status_code = Http::Code::Forbidden; response.body = "Unauthorized"; - response.headers_to_set = {{"set-cookie", "error=invalid"}, - {"www-authenticate", "Bearer realm=\"example\""}}; + // For denied responses, headers go to local_response_header_mutations. + response.local_response_header_mutations.push_back(createRequestHeaderMutation( + "set-cookie", "error=invalid", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); + response.local_response_header_mutations.push_back( + createRequestHeaderMutation("www-authenticate", "Bearer realm=\"example\"", + HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)); request_callbacks_->onComplete(std::make_unique(response)); EXPECT_EQ(1U, config_->stats().denied_.value()); @@ -6340,13 +6610,509 @@ TEST_F(HttpFilterTest, MultipleSetCookieHeadersOnSuccess) { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.response_headers_to_add = {{"set-cookie", "session=abc123"}, - {"set-cookie", "user=john"}}; + response.response_header_mutations.push_back(createResponseHeaderMutation( + "set-cookie", "session=abc123", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); + response.response_header_mutations.push_back(createResponseHeaderMutation( + "set-cookie", "user=john", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD)); request_callbacks_->onComplete(std::make_unique(response)); EXPECT_EQ(1U, config_->stats().ok_.value()); } +TEST_P(HttpFilterTestParam, RequestHeadersAppendActions) { + prepareCheck(); + request_headers_.addCopy("append-if-exists-or-add", "initial"); + request_headers_.addCopy("overwrite-if-exists", "initial"); + request_headers_.addCopy("overwrite-if-exists-or-add", "initial"); + + EXPECT_CALL(*client_, check(_, _, _, _)) + .WillOnce(Invoke([](Filters::Common::ExtAuthz::RequestCallbacks& callbacks, + const envoy::service::auth::v3::CheckRequest&, Tracing::Span&, + const StreamInfo::StreamInfo&) -> void { + Filters::Common::ExtAuthz::Response response{}; + response.status = Filters::Common::ExtAuthz::CheckStatus::OK; + + response.request_header_mutations.push_back( + {"append-if-exists-or-add", "added", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + response.request_header_mutations.push_back( + {"new-header", "added", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + response.request_header_mutations.push_back( + {"add-if-absent", "added", HeaderValueOption::ADD_IF_ABSENT}); + response.request_header_mutations.push_back( + {"append-if-exists-or-add", "ignored", HeaderValueOption::ADD_IF_ABSENT}); + response.request_header_mutations.push_back( + {"overwrite-if-exists", "overwritten", HeaderValueOption::OVERWRITE_IF_EXISTS}); + response.request_header_mutations.push_back( + {"new-header-2", "ignored", HeaderValueOption::OVERWRITE_IF_EXISTS}); + response.request_header_mutations.push_back( + {"overwrite-if-exists-or-add", "overwritten", + HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + response.request_header_mutations.push_back( + {"new-header-3", "set", HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + + callbacks.onComplete(std::make_unique(response)); + })); + + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); + + // Check append-if-exists-or-add. + auto entries = request_headers_.get(Http::LowerCaseString("append-if-exists-or-add")); + EXPECT_EQ(2, entries.size()); + EXPECT_EQ("initial", entries[0]->value().getStringView()); + EXPECT_EQ("added", entries[1]->value().getStringView()); + + // Check headers_to_add for "new-header". + entries = request_headers_.get(Http::LowerCaseString("new-header")); + EXPECT_EQ(1, entries.size()); + EXPECT_EQ("added", entries[0]->value().getStringView()); + + // Check headers_to_add_if_absent for "add-if-absent". + entries = request_headers_.get(Http::LowerCaseString("add-if-absent")); + EXPECT_EQ(1, entries.size()); + EXPECT_EQ("added", entries[0]->value().getStringView()); + + // Check headers_to_add_if_absent for "append-if-exists-or-add". It should be ignored. + entries = request_headers_.get(Http::LowerCaseString("append-if-exists-or-add")); + EXPECT_EQ(2, entries.size()); + + // Check headers_to_overwrite_if_exists for "overwrite-if-exists". + entries = request_headers_.get(Http::LowerCaseString("overwrite-if-exists")); + EXPECT_EQ(1, entries.size()); + EXPECT_EQ("overwritten", entries[0]->value().getStringView()); + + // Check headers_to_overwrite_if_exists for "new-header-2". It should be ignored. + entries = request_headers_.get(Http::LowerCaseString("new-header-2")); + EXPECT_TRUE(entries.empty()); + + // Check headers_to_set for "overwrite-if-exists-or-add". + entries = request_headers_.get(Http::LowerCaseString("overwrite-if-exists-or-add")); + EXPECT_EQ(1, entries.size()); + EXPECT_EQ("overwritten", entries[0]->value().getStringView()); + + // Check headers_to_set for "new-header-3". + entries = request_headers_.get(Http::LowerCaseString("new-header-3")); + EXPECT_EQ(1, entries.size()); + EXPECT_EQ("set", entries[0]->value().getStringView()); + + EXPECT_EQ(1U, config_->stats().ok_.value()); +} + +TEST_F(HttpFilterTest, ErrorResponseAddsHeadersToAdd) { + InSequence s; + + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_authz_server" + failure_mode_allow: false + )EOF"); + + ON_CALL(decoder_filter_callbacks_, connection()) + .WillByDefault(Return(OptRef{connection_})); + connection_.stream_info_.downstream_connection_info_provider_->setRemoteAddress(addr_); + connection_.stream_info_.downstream_connection_info_provider_->setLocalAddress(addr_); + + EXPECT_CALL(*client_, check(_, _, _, _)) + .WillOnce( + Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks, + const envoy::service::auth::v3::CheckRequest&, Tracing::Span&, + const StreamInfo::StreamInfo&) -> void { request_callbacks_ = &callbacks; })); + + EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark, + filter_->decodeHeaders(request_headers_, false)); + EXPECT_CALL(decoder_filter_callbacks_, continueDecoding()).Times(0); + + EXPECT_CALL(decoder_filter_callbacks_, encodeHeaders_(_, _)) + .WillOnce(Invoke([&](const Http::ResponseHeaderMap& headers, bool) -> void { + EXPECT_EQ(headers.getStatusValue(), std::to_string(enumToInt(Http::Code::Forbidden))); + const auto added = headers.get(Http::LowerCaseString("x-error-added")); + ASSERT_FALSE(added.empty()); + EXPECT_EQ(added[0]->value().getStringView(), "value"); + })); + + Filters::Common::ExtAuthz::Response response{}; + response.status = Filters::Common::ExtAuthz::CheckStatus::Error; + response.status_code = Http::Code::Forbidden; + // For error responses, headers go to local_response_header_mutations. + response.local_response_header_mutations.push_back( + {"x-error-added", "value", + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + request_callbacks_->onComplete(std::make_unique(response)); + + EXPECT_EQ(1U, config_->stats().error_.value()); +} + +TEST_F(HttpFilterTest, DeniedResponseAddsHeadersToAdd) { + InSequence s; + + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_authz_server" + )EOF"); + + ON_CALL(decoder_filter_callbacks_, connection()) + .WillByDefault(Return(OptRef{connection_})); + connection_.stream_info_.downstream_connection_info_provider_->setRemoteAddress(addr_); + connection_.stream_info_.downstream_connection_info_provider_->setLocalAddress(addr_); + + EXPECT_CALL(*client_, check(_, _, _, _)) + .WillOnce( + Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks, + const envoy::service::auth::v3::CheckRequest&, Tracing::Span&, + const StreamInfo::StreamInfo&) -> void { request_callbacks_ = &callbacks; })); + + EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark, + filter_->decodeHeaders(request_headers_, false)); + EXPECT_CALL(decoder_filter_callbacks_, continueDecoding()).Times(0); + + EXPECT_CALL(decoder_filter_callbacks_, encodeHeaders_(_, _)) + .WillOnce(Invoke([&](const Http::ResponseHeaderMap& headers, bool) -> void { + EXPECT_EQ(headers.getStatusValue(), std::to_string(enumToInt(Http::Code::Forbidden))); + const auto added = headers.get(Http::LowerCaseString("x-denied-added")); + ASSERT_FALSE(added.empty()); + EXPECT_EQ(added[0]->value().getStringView(), "value"); + })); + + Filters::Common::ExtAuthz::Response response{}; + response.status = Filters::Common::ExtAuthz::CheckStatus::Denied; + response.status_code = Http::Code::Forbidden; + // For denied responses, headers go to local_response_header_mutations. + response.local_response_header_mutations.push_back( + {"x-denied-added", "value", + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + request_callbacks_->onComplete(std::make_unique(response)); + + EXPECT_EQ(1U, config_->stats().denied_.value()); +} + +TEST_F(HttpFilterTest, DeniedResponseInvalidHeadersToAddRejectedWithValidation) { + InSequence s; + + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_authz_server" + validate_mutations: true + )EOF"); + + ON_CALL(decoder_filter_callbacks_, connection()) + .WillByDefault(Return(OptRef{connection_})); + connection_.stream_info_.downstream_connection_info_provider_->setRemoteAddress(addr_); + connection_.stream_info_.downstream_connection_info_provider_->setLocalAddress(addr_); + + EXPECT_CALL(*client_, check(_, _, _, _)) + .WillOnce( + Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks, + const envoy::service::auth::v3::CheckRequest&, Tracing::Span&, + const StreamInfo::StreamInfo&) -> void { request_callbacks_ = &callbacks; })); + + EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark, + filter_->decodeHeaders(request_headers_, false)); + EXPECT_CALL(decoder_filter_callbacks_, continueDecoding()).Times(0); + + // Invalid headers_to_add should cause a rejection. + EXPECT_CALL(decoder_filter_callbacks_, encodeHeaders_(_, _)) + .WillOnce(Invoke([&](const Http::ResponseHeaderMap& headers, bool) -> void { + // Should get 500 Internal Server Error from rejection. + EXPECT_EQ(headers.getStatusValue(), + std::to_string(enumToInt(Http::Code::InternalServerError))); + })); + + Filters::Common::ExtAuthz::Response response{}; + response.status = Filters::Common::ExtAuthz::CheckStatus::Denied; + response.status_code = Http::Code::Forbidden; + // For denied responses, headers go to local_response_header_mutations. + response.local_response_header_mutations.push_back( + {"invalid\nheader", "value", + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + request_callbacks_->onComplete(std::make_unique(response)); + + // Invalid stat should be incremented. + EXPECT_EQ(1U, config_->stats().invalid_.value()); +} + +// Tests that all append_action types are correctly applied to denied response headers. +TEST_F(HttpFilterTest, DeniedResponseAppendActionsOnLocalReply) { + InSequence s; + + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_authz_server" + )EOF"); + + ON_CALL(decoder_filter_callbacks_, connection()) + .WillByDefault(Return(OptRef{connection_})); + connection_.stream_info_.downstream_connection_info_provider_->setRemoteAddress(addr_); + connection_.stream_info_.downstream_connection_info_provider_->setLocalAddress(addr_); + + EXPECT_CALL(*client_, check(_, _, _, _)) + .WillOnce( + Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks, + const envoy::service::auth::v3::CheckRequest&, Tracing::Span&, + const StreamInfo::StreamInfo&) -> void { request_callbacks_ = &callbacks; })); + + EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark, + filter_->decodeHeaders(request_headers_, false)); + EXPECT_CALL(decoder_filter_callbacks_, continueDecoding()).Times(0); + + EXPECT_CALL(decoder_filter_callbacks_, encodeHeaders_(_, _)) + .WillOnce(Invoke([&](Http::ResponseHeaderMap& headers, bool) -> void { + EXPECT_EQ(headers.getStatusValue(), std::to_string(enumToInt(Http::Code::Forbidden))); + + // Set action should set header and replace existing. + auto set_header = headers.get(Http::LowerCaseString("x-set-header")); + ASSERT_EQ(set_header.size(), 1); + EXPECT_EQ(set_header[0]->value().getStringView(), "set-value"); + + // Add action should add header. + auto add_header = headers.get(Http::LowerCaseString("x-add-header")); + ASSERT_EQ(add_header.size(), 1); + EXPECT_EQ(add_header[0]->value().getStringView(), "add-value"); + + // Append action on non-existing header should add i.e. APPEND_IF_EXISTS_OR_ADD behavior. + auto append_new = headers.get(Http::LowerCaseString("x-append-new")); + ASSERT_EQ(append_new.size(), 1); + EXPECT_EQ(append_new[0]->value().getStringView(), "append-new-value"); + + // APPEND_IF_EXISTS_OR_ADD on existing header creates duplicate entries (addCopy). + // First Set creates "initial", then Add creates a second entry "appended". + auto append_existing = headers.get(Http::LowerCaseString("x-append-existing")); + ASSERT_EQ(append_existing.size(), 2); + EXPECT_EQ(append_existing[0]->value().getStringView(), "initial"); + EXPECT_EQ(append_existing[1]->value().getStringView(), "appended"); + + // AddIfAbsent action on non-existing header should add. + auto add_if_absent_new = headers.get(Http::LowerCaseString("x-add-if-absent-new")); + ASSERT_EQ(add_if_absent_new.size(), 1); + EXPECT_EQ(add_if_absent_new[0]->value().getStringView(), "added"); + + // AddIfAbsent action on existing header should not add as header was set earlier. + auto add_if_absent_existing = + headers.get(Http::LowerCaseString("x-add-if-absent-existing")); + ASSERT_EQ(add_if_absent_existing.size(), 1); + EXPECT_EQ(add_if_absent_existing[0]->value().getStringView(), "original"); + + // OverwriteIfExists action on non-existing header should not add. + auto overwrite_non_existing = + headers.get(Http::LowerCaseString("x-overwrite-non-existing")); + EXPECT_TRUE(overwrite_non_existing.empty()); + + // OverwriteIfExists action on existing header should overwrite. + auto overwrite_existing = headers.get(Http::LowerCaseString("x-overwrite-existing")); + ASSERT_EQ(overwrite_existing.size(), 1); + EXPECT_EQ(overwrite_existing[0]->value().getStringView(), "overwritten"); + })); + + Filters::Common::ExtAuthz::Response response{}; + response.status = Filters::Common::ExtAuthz::CheckStatus::Denied; + response.status_code = Http::Code::Forbidden; + + // Set up mutations in order to test all action types. + // For denied responses, headers go to local_response_header_mutations. + // 1. Set action. + response.local_response_header_mutations.push_back( + {"x-set-header", "set-value", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + + // 2. Add action. + response.local_response_header_mutations.push_back( + {"x-add-header", "add-value", + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + + // 3. Append action on non-existing header should add i.e. APPEND_IF_EXISTS_OR_ADD behavior. + response.local_response_header_mutations.push_back( + {"x-append-new", "append-new-value", + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + + // 4. Set up a header, then append to it. + response.local_response_header_mutations.push_back( + {"x-append-existing", "initial", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + response.local_response_header_mutations.push_back( + {"x-append-existing", "appended", + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + + // 5. AddIfAbsent on non-existing header should add. + response.local_response_header_mutations.push_back( + {"x-add-if-absent-new", "added", + Filters::Common::ExtAuthz::HeaderValueOption::ADD_IF_ABSENT}); + + // 6. Set up a header, then try AddIfAbsent should not add as header was set earlier. + response.local_response_header_mutations.push_back( + {"x-add-if-absent-existing", "original", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + response.local_response_header_mutations.push_back( + {"x-add-if-absent-existing", "should-not-be-added", + Filters::Common::ExtAuthz::HeaderValueOption::ADD_IF_ABSENT}); + + // 7. OverwriteIfExists on non-existing header should not add. + response.local_response_header_mutations.push_back( + {"x-overwrite-non-existing", "should-not-appear", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS}); + + // 8. Set up a header, then OverwriteIfExists should overwrite. + response.local_response_header_mutations.push_back( + {"x-overwrite-existing", "original", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + response.local_response_header_mutations.push_back( + {"x-overwrite-existing", "overwritten", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS}); + + request_callbacks_->onComplete(std::make_unique(response)); + + EXPECT_EQ(1U, config_->stats().denied_.value()); +} + +// Tests that all append_action types are correctly applied to error response headers. +TEST_F(HttpFilterTest, ErrorResponseAppendActionsOnLocalReply) { + InSequence s; + + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_authz_server" + failure_mode_allow: false + )EOF"); + + ON_CALL(decoder_filter_callbacks_, connection()) + .WillByDefault(Return(OptRef{connection_})); + connection_.stream_info_.downstream_connection_info_provider_->setRemoteAddress(addr_); + connection_.stream_info_.downstream_connection_info_provider_->setLocalAddress(addr_); + + EXPECT_CALL(*client_, check(_, _, _, _)) + .WillOnce( + Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks, + const envoy::service::auth::v3::CheckRequest&, Tracing::Span&, + const StreamInfo::StreamInfo&) -> void { request_callbacks_ = &callbacks; })); + + EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark, + filter_->decodeHeaders(request_headers_, false)); + EXPECT_CALL(decoder_filter_callbacks_, continueDecoding()).Times(0); + + EXPECT_CALL(decoder_filter_callbacks_, encodeHeaders_(_, _)) + .WillOnce(Invoke([&](Http::ResponseHeaderMap& headers, bool) -> void { + EXPECT_EQ(headers.getStatusValue(), + std::to_string(enumToInt(Http::Code::ServiceUnavailable))); + + // Set action should set header and replace existing. + auto set_header = headers.get(Http::LowerCaseString("x-error-set")); + ASSERT_EQ(set_header.size(), 1); + EXPECT_EQ(set_header[0]->value().getStringView(), "set-value"); + + // Add action should add header. + auto add_header = headers.get(Http::LowerCaseString("x-error-add")); + ASSERT_EQ(add_header.size(), 1); + EXPECT_EQ(add_header[0]->value().getStringView(), "add-value"); + + // Append action on non-existing header should add i.e. APPEND_IF_EXISTS_OR_ADD behavior. + auto append_new = headers.get(Http::LowerCaseString("x-error-append-new")); + ASSERT_EQ(append_new.size(), 1); + EXPECT_EQ(append_new[0]->value().getStringView(), "append-new-value"); + + // APPEND_IF_EXISTS_OR_ADD on existing header creates duplicate entries (addCopy). + auto append_existing = headers.get(Http::LowerCaseString("x-error-append-existing")); + ASSERT_EQ(append_existing.size(), 2); + EXPECT_EQ(append_existing[0]->value().getStringView(), "initial"); + EXPECT_EQ(append_existing[1]->value().getStringView(), "appended"); + + // AddIfAbsent on non-existing header should add. + auto add_if_absent_new = headers.get(Http::LowerCaseString("x-error-add-if-absent-new")); + ASSERT_EQ(add_if_absent_new.size(), 1); + EXPECT_EQ(add_if_absent_new[0]->value().getStringView(), "added"); + + // AddIfAbsent on existing header should not add as header was set earlier. + auto add_if_absent_existing = + headers.get(Http::LowerCaseString("x-error-add-if-absent-existing")); + ASSERT_EQ(add_if_absent_existing.size(), 1); + EXPECT_EQ(add_if_absent_existing[0]->value().getStringView(), "original"); + + // OverwriteIfExists on non-existing header should not add. + auto overwrite_non_existing = + headers.get(Http::LowerCaseString("x-error-overwrite-non-existing")); + EXPECT_TRUE(overwrite_non_existing.empty()); + + // OverwriteIfExists on existing header should overwrite. + auto overwrite_existing = headers.get(Http::LowerCaseString("x-error-overwrite-existing")); + ASSERT_EQ(overwrite_existing.size(), 1); + EXPECT_EQ(overwrite_existing[0]->value().getStringView(), "overwritten"); + })); + + Filters::Common::ExtAuthz::Response response{}; + response.status = Filters::Common::ExtAuthz::CheckStatus::Error; + response.status_code = Http::Code::ServiceUnavailable; + + // Set up mutations in order to test all action types. + // For error responses, headers go to local_response_header_mutations. + // 1. Set action. + response.local_response_header_mutations.push_back( + {"x-error-set", "set-value", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + + // 2. Add action. + response.local_response_header_mutations.push_back( + {"x-error-add", "add-value", + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + + // 3. Append action on non-existing header should add i.e. APPEND_IF_EXISTS_OR_ADD behavior. + response.local_response_header_mutations.push_back( + {"x-error-append-new", "append-new-value", + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + + // 4. Set up a header, then append to it. + response.local_response_header_mutations.push_back( + {"x-error-append-existing", "initial", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + response.local_response_header_mutations.push_back( + {"x-error-append-existing", "appended", + Filters::Common::ExtAuthz::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD}); + + // 5. AddIfAbsent on non-existing header should add. + response.local_response_header_mutations.push_back( + {"x-error-add-if-absent-new", "added", + Filters::Common::ExtAuthz::HeaderValueOption::ADD_IF_ABSENT}); + + // 6. Set up a header, then try AddIfAbsent should not add as header was set earlier. + response.local_response_header_mutations.push_back( + {"x-error-add-if-absent-existing", "original", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + response.local_response_header_mutations.push_back( + {"x-error-add-if-absent-existing", "should-not-be-added", + Filters::Common::ExtAuthz::HeaderValueOption::ADD_IF_ABSENT}); + + // 7. OverwriteIfExists on non-existing header should not add. + response.local_response_header_mutations.push_back( + {"x-error-overwrite-non-existing", "should-not-appear", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS}); + + // 8. Set up a header, then OverwriteIfExists should overwrite. + response.local_response_header_mutations.push_back( + {"x-error-overwrite-existing", "original", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); + response.local_response_header_mutations.push_back( + {"x-error-overwrite-existing", "overwritten", + Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS}); + + request_callbacks_->onComplete(std::make_unique(response)); + + EXPECT_EQ(1U, config_->stats().error_.value()); +} + +// encode1xxHeaders should always continue without mutation. +TEST_F(HttpFilterTest, Encode1xxHeadersContinue) { + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_authz_server" + )EOF"); + + Http::TestResponseHeaderMapImpl headers{{":status", "103"}}; + EXPECT_EQ(Http::Filter1xxHeadersStatus::Continue, filter_->encode1xxHeaders(headers)); + // No mutations should have been applied. + EXPECT_EQ(headers.getStatusValue(), "103"); +} + } // namespace } // namespace ExtAuthz } // namespace HttpFilters diff --git a/test/extensions/filters/network/ext_authz/ext_authz_test.cc b/test/extensions/filters/network/ext_authz/ext_authz_test.cc index 37c1e0e65d2b1..8aa2fa3c23fa8 100644 --- a/test/extensions/filters/network/ext_authz/ext_authz_test.cc +++ b/test/extensions/filters/network/ext_authz/ext_authz_test.cc @@ -97,7 +97,8 @@ class ExtAuthzFilterTest : public testing::Test { Filters::Common::ExtAuthz::Response response{}; response.status = Filters::Common::ExtAuthz::CheckStatus::OK; - response.headers_to_set = Filters::Common::ExtAuthz::UnsafeHeaderVector{{"foo", "bar"}}; + response.request_header_mutations.push_back( + {"foo", "bar", Filters::Common::ExtAuthz::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD}); auto* fields = response.dynamic_metadata.mutable_fields(); (*fields)["foo"] = ValueUtil::stringValue("ok");