diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index dfedb67f82dec..f3c741661ee87 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -24,6 +24,7 @@ using Http::ResponseHeaderMap; using Http::ResponseTrailerMap; static const std::string kErrorPrefix = "ext_proc error"; +static const int DefaultImmediateStatus = 200; void Filter::setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) { Http::PassThroughFilter::setDecoderFilterCallbacks(callbacks); @@ -558,7 +559,11 @@ void Filter::cleanUpTimers() { } void Filter::sendImmediateResponse(const ImmediateResponse& response) { - const auto status_code = response.has_status() ? response.status().code() : 200; + auto status_code = response.has_status() ? response.status().code() : DefaultImmediateStatus; + if (!MutationUtils::isValidHttpStatus(status_code)) { + ENVOY_LOG(debug, "Ignoring attempt to set invalid HTTP status {}", status_code); + status_code = DefaultImmediateStatus; + } const auto grpc_status = response.has_grpc_status() ? absl::optional(response.grpc_status().status()) @@ -570,6 +575,7 @@ void Filter::sendImmediateResponse(const ImmediateResponse& response) { }; sent_immediate_response_ = true; + ENVOY_LOG(debug, "Sending local reply with status code {}", status_code); encoder_callbacks_->sendLocalReply(static_cast(status_code), response.body(), mutate_headers, grpc_status, response.details()); } diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index 8fa43ade130de..1079716b8a7c0 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -14,6 +14,7 @@ namespace ExternalProcessing { using Http::Headers; using Http::LowerCaseString; +using envoy::config::core::v3::HeaderValueOption; using envoy::service::ext_proc::v3alpha::BodyMutation; using envoy::service::ext_proc::v3alpha::BodyResponse; using envoy::service::ext_proc::v3alpha::CommonResponse; @@ -56,19 +57,26 @@ void MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, Http::H if (!sh.has_header()) { continue; } - if (isSettableHeader(sh.header().key(), replacing_message)) { + if (!isSettableHeader(sh, replacing_message)) { + // Log the failure to set the header here, but don't log the value in case it's + // something sensitive like the Authorization header. + ENVOY_LOG(debug, "Ignorning improper attempt to set header {}", sh.header().key()); + } else { // Make "false" the default. This is logical and matches the ext_authz // filter. However, the router handles this same protobuf and uses "true" // as the default instead. const bool append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, false); - ENVOY_LOG(trace, "Setting header {} append = {}", sh.header().key(), append); - if (append) { - headers.addCopy(LowerCaseString(sh.header().key()), sh.header().value()); + const LowerCaseString lcKey(sh.header().key()); + if (append && !headers.get(lcKey).empty() && !isAppendableHeader(lcKey)) { + ENVOY_LOG(debug, "Ignoring duplicate value for header {}", sh.header().key()); } else { - headers.setCopy(LowerCaseString(sh.header().key()), sh.header().value()); + ENVOY_LOG(trace, "Setting header {} append = {}", sh.header().key(), append); + if (append) { + headers.addCopy(lcKey, sh.header().value()); + } else { + headers.setCopy(lcKey, sh.header().value()); + } } - } else { - ENVOY_LOG(debug, "Header {} is not settable", sh.header().key()); } } } @@ -112,16 +120,40 @@ void MutationUtils::applyBodyMutations(const BodyMutation& mutation, Buffer::Ins } } +bool MutationUtils::isValidHttpStatus(int code) { return (code >= 200); } + // Ignore attempts to set certain sensitive headers that can break later processing. // We may re-enable some of these after further testing. This logic is specific // to the ext_proc filter so it is not shared with HeaderUtils. -bool MutationUtils::isSettableHeader(absl::string_view key, bool replacing_message) { +bool MutationUtils::isSettableHeader(const HeaderValueOption& header, bool replacing_message) { + const auto& key = header.header().key(); const auto& headers = Headers::get(); - return !absl::EqualsIgnoreCase(key, headers.HostLegacy.get()) && - !absl::EqualsIgnoreCase(key, headers.Host.get()) && - (!absl::EqualsIgnoreCase(key, headers.Method.get()) || replacing_message) && - !absl::EqualsIgnoreCase(key, headers.Scheme.get()) && - !absl::StartsWithIgnoreCase(key, headers.prefix()); + if (absl::EqualsIgnoreCase(key, headers.HostLegacy.get()) || + absl::EqualsIgnoreCase(key, headers.Host.get()) || + (absl::EqualsIgnoreCase(key, headers.Method.get()) && !replacing_message) || + absl::EqualsIgnoreCase(key, headers.Scheme.get()) || + absl::StartsWithIgnoreCase(key, headers.prefix())) { + return false; + } + if (absl::EqualsIgnoreCase(key, headers.Status.get())) { + const auto& value = header.header().value(); + uint32_t status; + if (!absl::SimpleAtoi(value, &status)) { + ENVOY_LOG(debug, "Invalid value {} for HTTP status code", value); + return false; + } + if (!isValidHttpStatus(status)) { + ENVOY_LOG(debug, "Invalid HTTP status code {}", status); + return false; + } + } + return true; +} + +// Ignore attempts to append a second value to any system header, as in general those +// were never designed to support multiple values. +bool MutationUtils::isAppendableHeader(absl::string_view key) { + return !key.empty() && key[0] != ':'; } } // namespace ExternalProcessing diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.h b/source/extensions/filters/http/ext_proc/mutation_utils.h index 14b28cf306e7d..f57c13793d8b5 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.h +++ b/source/extensions/filters/http/ext_proc/mutation_utils.h @@ -37,8 +37,13 @@ class MutationUtils : public Logger::Loggable { static void applyBodyMutations(const envoy::service::ext_proc::v3alpha::BodyMutation& mutation, Buffer::Instance& buffer); + // Determine if a particular HTTP status code is valid. + static bool isValidHttpStatus(int code); + private: - static bool isSettableHeader(absl::string_view key, bool replacing_message); + static bool isSettableHeader(const envoy::config::core::v3::HeaderValueOption& header, + bool replacing_message); + static bool isAppendableHeader(absl::string_view key); }; } // namespace ExternalProcessing diff --git a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc index d2ab6ca2a89ac..eae609db2a7e7 100644 --- a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc +++ b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc @@ -476,13 +476,75 @@ TEST_P(ExtProcIntegrationTest, GetAndSetHeadersOnResponse) { auto* add1 = response_mutation->add_set_headers(); add1->mutable_header()->set_key("x-response-processed"); add1->mutable_header()->set_value("1"); + auto* add2 = response_mutation->add_set_headers(); + add2->mutable_header()->set_key(":status"); + add2->mutable_header()->set_value("201"); return true; }); + verifyDownstreamResponse(*response, 201); + EXPECT_THAT(response->headers(), SingleHeaderValueIs("x-response-processed", "1")); +} + +// Test the filter using the default configuration by connecting to +// an ext_proc server that responds to the response_headers message +// but tries to set the status code to an invalid value +TEST_P(ExtProcIntegrationTest, GetAndSetHeadersOnResponseBadStatus) { + initializeConfig(); + HttpIntegrationTest::initialize(); + auto response = sendDownstreamRequest(absl::nullopt); + processRequestHeadersMessage(true, absl::nullopt); + handleUpstreamRequest(); + + processResponseHeadersMessage(false, [](const HttpHeaders&, HeadersResponse& headers_resp) { + auto* response_mutation = headers_resp.mutable_response()->mutable_header_mutation(); + auto* add1 = response_mutation->add_set_headers(); + add1->mutable_header()->set_key("x-response-processed"); + add1->mutable_header()->set_value("1"); + auto* add2 = response_mutation->add_set_headers(); + add2->mutable_header()->set_key(":status"); + add2->mutable_header()->set_value("100"); + return true; + }); + + // Invalid status code should be ignored, but the other header mutation + // should still have been processed. verifyDownstreamResponse(*response, 200); EXPECT_THAT(response->headers(), SingleHeaderValueIs("x-response-processed", "1")); } +// Test the filter using the default configuration by connecting to +// an ext_proc server that responds to the response_headers message +// but tries to set the status code to two values. The second +// attempt should be ignored. +TEST_P(ExtProcIntegrationTest, GetAndSetHeadersOnResponseTwoStatuses) { + initializeConfig(); + HttpIntegrationTest::initialize(); + auto response = sendDownstreamRequest(absl::nullopt); + processRequestHeadersMessage(true, absl::nullopt); + handleUpstreamRequest(); + + processResponseHeadersMessage(false, [](const HttpHeaders&, HeadersResponse& headers_resp) { + auto* response_mutation = headers_resp.mutable_response()->mutable_header_mutation(); + auto* add1 = response_mutation->add_set_headers(); + add1->mutable_header()->set_key("x-response-processed"); + add1->mutable_header()->set_value("1"); + auto* add2 = response_mutation->add_set_headers(); + add2->mutable_header()->set_key(":status"); + add2->mutable_header()->set_value("201"); + auto* add3 = response_mutation->add_set_headers(); + add3->mutable_header()->set_key(":status"); + add3->mutable_header()->set_value("202"); + add3->mutable_append()->set_value(true); + return true; + }); + + // Invalid status code should be ignored, but the other header mutation + // should still have been processed. + verifyDownstreamResponse(*response, 201); + EXPECT_THAT(response->headers(), SingleHeaderValueIs("x-response-processed", "1")); +} + // Test the filter using the default configuration by connecting to // an ext_proc server that responds to the response_headers message // by checking the headers and modifying the trailers @@ -789,7 +851,7 @@ TEST_P(ExtProcIntegrationTest, GetAndRespondImmediatelyOnResponse) { EXPECT_EQ("{\"reason\": \"Not authorized\"}", response->body()); } -// Test the filter with request body streaming enabled using +// Test the filter with request body buffering enabled using // an ext_proc server that responds to the request_body message // by sending back an immediate_response message TEST_P(ExtProcIntegrationTest, GetAndRespondImmediatelyOnRequestBody) { @@ -808,7 +870,7 @@ TEST_P(ExtProcIntegrationTest, GetAndRespondImmediatelyOnRequestBody) { EXPECT_EQ("{\"reason\": \"Not authorized\"}", response->body()); } -// Test the filter with body streaming enabled using +// Test the filter with body buffering enabled using // an ext_proc server that responds to the response_body message // by sending back an immediate_response message. Since this // happens after the response headers have been sent, as a result @@ -834,6 +896,23 @@ TEST_P(ExtProcIntegrationTest, GetAndRespondImmediatelyOnResponseBody) { EXPECT_EQ("{\"reason\": \"Not authorized\"}", response->body()); } +// Test the filter using an ext_proc server that responds to the request_body message +// by sending back an immediate_response message with an invalid status code. +TEST_P(ExtProcIntegrationTest, GetAndRespondImmediatelyWithBadStatus) { + initializeConfig(); + HttpIntegrationTest::initialize(); + auto response = sendDownstreamRequestWithBody("Replace this!", absl::nullopt); + processAndRespondImmediately(true, [](ImmediateResponse& immediate) { + immediate.mutable_status()->set_code(envoy::type::v3::StatusCode::Continue); + immediate.set_body("{\"reason\": \"Because\"}"); + immediate.set_details("Failed because we said so"); + }); + + // The attempt to set the status code to 100 should have been ignored. + verifyDownstreamResponse(*response, 200); + EXPECT_EQ("{\"reason\": \"Because\"}", response->body()); +} + // Test the ability of the filter to turn a GET into a POST by adding a body // and changing the method. TEST_P(ExtProcIntegrationTest, ConvertGetToPost) { diff --git a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc index abed79419dae1..c617caa299d59 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -66,6 +66,9 @@ TEST(MutationUtils, TestApplyMutations) { s->mutable_append()->set_value(false); s->mutable_header()->set_key("x-replace-this"); s->mutable_header()->set_value("no"); + s = mutation.add_set_headers(); + s->mutable_header()->set_key(":status"); + s->mutable_header()->set_value("418"); // Default of "append" is "false" and mutations // are applied in order. s = mutation.add_set_headers(); @@ -101,6 +104,15 @@ TEST(MutationUtils, TestApplyMutations) { s->mutable_header()->set_key("X-Envoy-StrangeThing"); s->mutable_header()->set_value("Yes"); + // Attempts to set the status header out of range should + // also be ignored. + s = mutation.add_set_headers(); + s->mutable_header()->set_key(":status"); + s->mutable_header()->set_value("This is not even an integer"); + s = mutation.add_set_headers(); + s->mutable_header()->set_key(":status"); + s->mutable_header()->set_value("100"); + MutationUtils::applyHeaderMutations(mutation, headers, false); Http::TestRequestHeaderMapImpl expected_headers{ @@ -109,6 +121,7 @@ TEST(MutationUtils, TestApplyMutations) { {":path", "/foo/the/bar?size=123"}, {"host", "localhost:1000"}, {":authority", "localhost:1000"}, + {":status", "418"}, {"content-type", "text/plain; encoding=UTF8"}, {"x-append-this", "1"}, {"x-append-this", "2"}, @@ -120,6 +133,35 @@ TEST(MutationUtils, TestApplyMutations) { EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); } +TEST(MutationUtils, TestNonAppendableHeaders) { + Http::TestRequestHeaderMapImpl headers; + envoy::service::ext_proc::v3alpha::HeaderMutation mutation; + auto* s = mutation.add_set_headers(); + s->mutable_append()->set_value(true); + s->mutable_header()->set_key(":path"); + s->mutable_header()->set_value("/foo"); + s = mutation.add_set_headers(); + s->mutable_header()->set_key(":status"); + s->mutable_header()->set_value("400"); + // These two should be ignored since we ignore attempts + // to set multiple values for system headers. + s = mutation.add_set_headers(); + s->mutable_append()->set_value(true); + s->mutable_header()->set_key(":path"); + s->mutable_header()->set_value("/baz"); + s = mutation.add_set_headers(); + s->mutable_append()->set_value(true); + s->mutable_header()->set_key(":status"); + s->mutable_header()->set_value("401"); + + MutationUtils::applyHeaderMutations(mutation, headers, false); + Http::TestRequestHeaderMapImpl expected_headers{ + {":path", "/foo"}, + {":status", "400"}, + }; + EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); +} + // Ensure that we actually replace the body TEST(MutationUtils, TestBodyMutationReplace) { Buffer::OwnedImpl buf;