diff --git a/source/extensions/filters/common/ext_authz/ext_authz.h b/source/extensions/filters/common/ext_authz/ext_authz.h index 4a09ad5dd3a9e..80831badf9fe7 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz.h +++ b/source/extensions/filters/common/ext_authz/ext_authz.h @@ -110,6 +110,8 @@ struct Response { // "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{}; + // 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 // from the request to the upstream server. std::vector headers_to_remove{}; 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 2190493375bb3..cc2858b0454f7 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 @@ -43,7 +43,6 @@ void copyOkResponseMutations(ResponsePtr& response, } } else { switch (header.append_action()) { - PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; case Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: response->response_headers_to_add.emplace_back(header.header().key(), header.header().value()); @@ -60,6 +59,9 @@ void copyOkResponseMutations(ResponsePtr& response, response->response_headers_to_set.emplace_back(header.header().key(), header.header().value()); break; + default: + response->saw_invalid_append_actions = true; + break; } } } 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 170ee16ef84a8..0c9587fe119eb 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 @@ -41,6 +41,7 @@ const Response& errorResponse() { UnsafeHeaderVector{}, UnsafeHeaderVector{}, UnsafeHeaderVector{}, + false, {{}}, Http::Utility::QueryParamsVector{}, {}, @@ -385,6 +386,7 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) { UnsafeHeaderVector{}, UnsafeHeaderVector{}, UnsafeHeaderVector{}, + false, std::move(headers_to_remove), Http::Utility::QueryParamsVector{}, {}, @@ -408,6 +410,7 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) { UnsafeHeaderVector{}, UnsafeHeaderVector{}, UnsafeHeaderVector{}, + false, {{}}, Http::Utility::QueryParamsVector{}, {}, diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index 7fd7abf65f365..8984cb37f0ebc 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/source/extensions/filters/http/ext_authz/ext_authz.cc @@ -601,6 +601,17 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { updateLoggingInfo(response->grpc_status); + if (response->saw_invalid_append_actions) { + if (config_->validateMutations()) { + ENVOY_STREAM_LOG(trace, "Rejecting response with invalid header append action.", + *decoder_callbacks_); + rejectResponse(); + return; + } + ENVOY_STREAM_LOG(trace, "Ignoring response headers with invalid header append action.", + *decoder_callbacks_); + } + if (!response->dynamic_metadata.fields().empty()) { if (!config_->enableDynamicMetadataIngestion()) { ENVOY_STREAM_LOG(trace, 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 59c629a50b7a1..75d89e50439cb 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 @@ -445,6 +445,10 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithAppendActions) { 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); @@ -458,6 +462,7 @@ TEST_F(ExtAuthzGrpcClientTest, AuthorizationOkWithAppendActions) { UnsafeHeaderVector{{"add-if-absent", "add-if-absent-value"}}, .response_headers_to_overwrite_if_exists = UnsafeHeaderVector{{"overwrite-if-exists", "overwrite-if-exists-value"}}, + .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/test_common.h b/test/extensions/filters/common/ext_authz/test_common.h index 762d37d614344..19532ddbad6e0 100644 --- a/test/extensions/filters/common/ext_authz/test_common.h +++ b/test/extensions/filters/common/ext_authz/test_common.h @@ -133,6 +133,10 @@ MATCHER_P(AuthzOkResponse, response, "") { return false; } + if (response.saw_invalid_append_actions != arg->saw_invalid_append_actions) { + return false; + } + if (!TestCommon::compareQueryParamsVector(response.query_parameters_to_set, arg->query_parameters_to_set)) { return false; diff --git a/test/extensions/filters/http/ext_authz/BUILD b/test/extensions/filters/http/ext_authz/BUILD index 33e055d352187..843387cba9bd8 100644 --- a/test/extensions/filters/http/ext_authz/BUILD +++ b/test/extensions/filters/http/ext_authz/BUILD @@ -88,6 +88,7 @@ envoy_extension_cc_test( "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/config/listener/v3:pkg_cc_proto", "@envoy_api//envoy/extensions/filters/http/ext_authz/v3:pkg_cc_proto", "@envoy_api//envoy/service/auth/v3:pkg_cc_proto", 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 9eb2348bdbafd..9bce69d8cffc4 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 @@ -1,4 +1,5 @@ #include "envoy/config/bootstrap/v3/bootstrap.pb.h" +#include "envoy/config/core/v3/base.pb.h" #include "envoy/config/listener/v3/listener_components.pb.h" #include "envoy/extensions/filters/http/ext_authz/v3/ext_authz.pb.h" #include "envoy/service/auth/v3/external_auth.pb.h" @@ -457,7 +458,8 @@ class ExtAuthzGrpcIntegrationTest const Headers& response_headers_to_append, const Headers& response_headers_to_set, const Headers& response_headers_to_append_if_absent, - const Headers& response_headers_to_set_if_exists = {}) { + const Headers& response_headers_to_set_if_exists = {}, + bool add_sentinel_header_append_action = false) { ext_authz_request_->startGrpcStream(); envoy::service::auth::v3::CheckResponse check_response; check_response.mutable_status()->set_code(Grpc::Status::WellKnownGrpcStatus::Ok); @@ -518,6 +520,20 @@ class ExtAuthzGrpcIntegrationTest ENVOY_LOG_MISC(trace, "sendExtAuthzResponse: set response_header_to_add {}={}", key, value); } + if (add_sentinel_header_append_action) { + auto* entry = check_response.mutable_ok_response()->mutable_response_headers_to_add()->Add(); + entry->set_append_action( + static_cast( + std::numeric_limits::max())); + const auto key = std::string("invalid-append-action"); + const auto value = std::string("invalid-append-action-value"); + entry->mutable_header()->set_key(key); + entry->mutable_header()->set_value(value); + ENVOY_LOG_MISC(trace, + "sendExtAuthzResponse: set response header with invalid append action {}={}", + key, value); + } + for (const auto& response_header_to_set : response_headers_to_set) { auto* entry = check_response.mutable_ok_response()->mutable_response_headers_to_add()->Add(); const auto key = std::string(response_header_to_set.first); @@ -1294,6 +1310,49 @@ TEST_P(ExtAuthzGrpcIntegrationTest, ValidateMutations) { cleanup(); } +TEST_P(ExtAuthzGrpcIntegrationTest, ValidateMutationsSentinelAppendAction) { + GrpcInitializeConfigOpts opts; + opts.validate_mutations = true; + initializeConfig(opts); + + // Use h1, set up the test. + setDownstreamProtocol(Http::CodecType::HTTP1); + HttpIntegrationTest::initialize(); + + // Start a client connection and request. + initiateClientConnection(0); + waitForExtAuthzRequest(expectedCheckRequest(Http::CodecType::HTTP1)); + sendExtAuthzResponse({}, {}, {}, {}, {}, {}, {}, {}, {}, + /*add_sentinel_header_append_action=*/true); + + ASSERT_TRUE(response_->waitForEndStream()); + EXPECT_TRUE(response_->complete()); + EXPECT_EQ("500", response_->headers().getStatusValue()); + test_server_->waitForCounterEq("cluster.cluster_0.ext_authz.invalid", 1); + + cleanup(); +} + +// Ignore invalid header append actions when validate_mutations is false. +TEST_P(ExtAuthzGrpcIntegrationTest, NoValidateMutationsSentinelAppendAction) { + GrpcInitializeConfigOpts opts; + opts.validate_mutations = false; + initializeConfig(opts); + + // Use h1, set up the test. + setDownstreamProtocol(Http::CodecType::HTTP1); + HttpIntegrationTest::initialize(); + + // Start a client connection and request. + initiateClientConnection(0); + + waitForExtAuthzRequest(expectedCheckRequest(Http::CodecType::HTTP1)); + sendExtAuthzResponse({}, {}, {}, {}, {}, {}, {}, {}, {}, + /*add_sentinel_header_append_action=*/true); + waitForSuccessfulUpstreamResponse("200"); + cleanup(); +} + TEST_P(ExtAuthzGrpcIntegrationTest, TimeoutFailOpen) { GrpcInitializeConfigOpts init_opts; init_opts.stats_expect_response_bytes = false; 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 c6b342ba95016..871f6eeb7be99 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_test.cc @@ -679,6 +679,13 @@ TEST_F(InvalidMutationTest, BasicInvalidKey) { testResponse(response); } +TEST_F(InvalidMutationTest, InvalidHeaderAppendAction) { + Filters::Common::ExtAuthz::Response response; + response.status = Filters::Common::ExtAuthz::CheckStatus::OK; + response.saw_invalid_append_actions = true; + testResponse(response); +} + struct DecoderHeaderMutationRulesTestOpts { absl::optional rules; bool expect_reject_response = false;