From 6c498d6b6fca972e555665fc1328ce748e87e9fa Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Mon, 22 May 2023 17:36:04 +0000 Subject: [PATCH 1/8] ASSERT(valid()) when ext_proc filter apply header mutations. Signed-off-by: Yanjun Xiang --- .../filters/http/ext_proc/mutation_utils.cc | 11 +++++++ .../http/ext_proc/mutation_utils_test.cc | 29 +++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index 08dbcd6eedf9..35a20059b352 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -40,6 +40,11 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, const Checker& checker, Counter& rejected_mutations) { for (const auto& hdr : mutation.remove_headers()) { + if (!Envoy::Http::validHeaderString(hdr)) { + ENVOY_LOG(debug, "remove_headers mutation contain null character, may not be removed."); + rejected_mutations.inc(); + return absl::InvalidArgumentError("Invalid null character in remove_headers mutation."); + } const LowerCaseString remove_header(hdr); switch (checker.check(CheckOperation::REMOVE, remove_header, "")) { case CheckResult::OK: @@ -62,6 +67,11 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, if (!sh.has_header()) { continue; } + if (!Envoy::Http::validHeaderString(sh.header().key())) { + ENVOY_LOG(debug, "set_headers mutation contain null character, may not be appended."); + rejected_mutations.inc(); + return absl::InvalidArgumentError("Invalid null character in set_headers mutation."); + } const LowerCaseString header_name(sh.header().key()); const bool append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, false); const auto check_op = (append && !headers.get(header_name).empty()) ? CheckOperation::APPEND @@ -87,6 +97,7 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, break; case CheckResult::FAIL: ENVOY_LOG(debug, "Header {} may not be modified. Returning error", header_name); + rejected_mutations.inc(); return absl::InvalidArgumentError( absl::StrCat("Invalid attempt to modify ", static_cast(header_name))); } 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 09dc91254794..716b3e398e46 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -188,6 +188,35 @@ TEST(MutationUtils, TestNonAppendableHeaders) { EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); } +TEST(MutationUtils, TestSetHeaderWithNullCharacter) { + Http::TestRequestHeaderMapImpl headers{ + {":method", "GET"}, + {"host", "localhost:1000"}, + }; + envoy::service::ext_proc::v3::HeaderMutation mutation; + auto* s = mutation.add_set_headers(); + s->mutable_header()->set_key("x-append-this\n"); + Checker checker(HeaderMutationRules::default_instance()); + Envoy::Stats::MockCounter rejections; + EXPECT_CALL(rejections, inc()).Times(1); + EXPECT_FALSE( + MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); +} + +TEST(MutationUtils, TestRemoveHeaderWithNullCharacter) { + Http::TestRequestHeaderMapImpl headers{ + {":method", "GET"}, + {"host", "localhost:1000"}, + }; + envoy::service::ext_proc::v3::HeaderMutation mutation; + mutation.add_remove_headers("host\r"); + Checker checker(HeaderMutationRules::default_instance()); + Envoy::Stats::MockCounter rejections; + EXPECT_CALL(rejections, inc()).Times(1); + EXPECT_FALSE( + MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); +} + // Ensure that we actually replace the body TEST(MutationUtils, TestBodyMutationReplace) { Buffer::OwnedImpl buf; From 03a6a07472cd7d3a22210f4581b5057d6123545c Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Mon, 22 May 2023 17:38:26 +0000 Subject: [PATCH 2/8] fix format Signed-off-by: Yanjun Xiang --- test/extensions/filters/http/ext_proc/mutation_utils_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 716b3e398e46..e100b6c45d0d 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -198,7 +198,7 @@ TEST(MutationUtils, TestSetHeaderWithNullCharacter) { s->mutable_header()->set_key("x-append-this\n"); Checker checker(HeaderMutationRules::default_instance()); Envoy::Stats::MockCounter rejections; - EXPECT_CALL(rejections, inc()).Times(1); + EXPECT_CALL(rejections, inc()); EXPECT_FALSE( MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); } @@ -212,7 +212,7 @@ TEST(MutationUtils, TestRemoveHeaderWithNullCharacter) { mutation.add_remove_headers("host\r"); Checker checker(HeaderMutationRules::default_instance()); Envoy::Stats::MockCounter rejections; - EXPECT_CALL(rejections, inc()).Times(1); + EXPECT_CALL(rejections, inc()); EXPECT_FALSE( MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); } From c0463c0fc4a1fb70eaece9db390a02b5b36be0dc Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Mon, 22 May 2023 20:23:17 +0000 Subject: [PATCH 3/8] follow up commit Signed-off-by: Yanjun Xiang --- .../filters/http/ext_proc/ext_proc.cc | 4 +++- .../filters/http/ext_proc/mutation_utils.cc | 7 ++++--- .../http/ext_proc/ext_proc_integration_test.cc | 15 +++++++++++++++ .../http/ext_proc/mutation_utils_test.cc | 17 ++++++++++++++--- 4 files changed, 36 insertions(+), 7 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index b56f3da39cb1..987b547ba0f3 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -765,7 +765,9 @@ void Filter::sendImmediateResponse(const ImmediateResponse& response) { const auto mut_status = MutationUtils::applyHeaderMutations( response.headers(), headers, false, immediateResponseChecker().checker(), stats_.rejected_header_mutations_); - ENVOY_BUG(mut_status.ok(), "Immediate response mutations should not fail"); + if (!mut_status.ok()) { + ENVOY_LOG(error, "Immediate response mutations failed"); + } } }; diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index 35a20059b352..42190a0362be 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -41,7 +41,7 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, Counter& rejected_mutations) { for (const auto& hdr : mutation.remove_headers()) { if (!Envoy::Http::validHeaderString(hdr)) { - ENVOY_LOG(debug, "remove_headers mutation contain null character, may not be removed."); + ENVOY_LOG(debug, "remove_headers contain null character, may not be removed."); rejected_mutations.inc(); return absl::InvalidArgumentError("Invalid null character in remove_headers mutation."); } @@ -67,8 +67,9 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, if (!sh.has_header()) { continue; } - if (!Envoy::Http::validHeaderString(sh.header().key())) { - ENVOY_LOG(debug, "set_headers mutation contain null character, may not be appended."); + if (!Envoy::Http::validHeaderString(sh.header().key()) || + !Envoy::Http::validHeaderString(sh.header().value())) { + ENVOY_LOG(debug, "set_headers contain null character in key or value, may not be appended."); rejected_mutations.inc(); return absl::InvalidArgumentError("Invalid null character in set_headers mutation."); } 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 3c488aaa0e27..e75b538c885e 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 @@ -1079,6 +1079,21 @@ TEST_P(ExtProcIntegrationTest, GetAndRespondImmediately) { EXPECT_EQ("{\"reason\": \"Not authorized\"}", response->body()); } +TEST_P(ExtProcIntegrationTest, GetAndRespondImmediatelyWithNullCharacter) { + initializeConfig(); + HttpIntegrationTest::initialize(); + auto response = sendDownstreamRequest(absl::nullopt); + + processAndRespondImmediately(*grpc_upstreams_[0], true, [](ImmediateResponse& immediate) { + immediate.mutable_status()->set_code(envoy::type::v3::StatusCode::Unauthorized); + auto* hdr = immediate.mutable_headers()->add_set_headers(); + hdr->mutable_header()->set_key("x-failure-reason\n"); + hdr->mutable_header()->set_value("testing"); + }); + + verifyDownstreamResponse(*response, 401); +} + // Test the filter using the default configuration by connecting to // an ext_proc server that responds to the request_headers message // by sending back an immediate_response message after the 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 e100b6c45d0d..a43a2f1eeabf 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -193,11 +193,22 @@ TEST(MutationUtils, TestSetHeaderWithNullCharacter) { {":method", "GET"}, {"host", "localhost:1000"}, }; + Checker checker(HeaderMutationRules::default_instance()); + Envoy::Stats::MockCounter rejections; envoy::service::ext_proc::v3::HeaderMutation mutation; auto* s = mutation.add_set_headers(); + // Test header key contains null character. s->mutable_header()->set_key("x-append-this\n"); - Checker checker(HeaderMutationRules::default_instance()); - Envoy::Stats::MockCounter rejections; + s->mutable_header()->set_value("value"); + EXPECT_CALL(rejections, inc()); + EXPECT_FALSE( + MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); + + mutation.Clear(); + s = mutation.add_set_headers(); + // Test header value contains null character. + s->mutable_header()->set_key("x-append-this"); + s->mutable_header()->set_value("value\r"); EXPECT_CALL(rejections, inc()); EXPECT_FALSE( MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); @@ -209,7 +220,7 @@ TEST(MutationUtils, TestRemoveHeaderWithNullCharacter) { {"host", "localhost:1000"}, }; envoy::service::ext_proc::v3::HeaderMutation mutation; - mutation.add_remove_headers("host\r"); + mutation.add_remove_headers("host\n"); Checker checker(HeaderMutationRules::default_instance()); Envoy::Stats::MockCounter rejections; EXPECT_CALL(rejections, inc()); From a00826856f6971907943c787a8fe52eccfb9e295 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Tue, 23 May 2023 19:50:08 +0000 Subject: [PATCH 4/8] addressing comments Signed-off-by: Yanjun Xiang --- .../extensions/filters/http/ext_proc/ext_proc.cc | 2 +- .../filters/http/ext_proc/mutation_utils.cc | 15 ++++++++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 987b547ba0f3..40a3d57f53e2 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -766,7 +766,7 @@ void Filter::sendImmediateResponse(const ImmediateResponse& response) { response.headers(), headers, false, immediateResponseChecker().checker(), stats_.rejected_header_mutations_); if (!mut_status.ok()) { - ENVOY_LOG(error, "Immediate response mutations failed"); + ENVOY_LOG(debug, "Immediate response mutations failed"); } } }; diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index 42190a0362be..6ac7297004ec 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -40,10 +40,10 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, const Checker& checker, Counter& rejected_mutations) { for (const auto& hdr : mutation.remove_headers()) { - if (!Envoy::Http::validHeaderString(hdr)) { - ENVOY_LOG(debug, "remove_headers contain null character, may not be removed."); + if (!Http::HeaderUtility::headerNameIsValid(hdr)) { + ENVOY_LOG(debug, "remove_headers contain invalid character, may not be removed."); rejected_mutations.inc(); - return absl::InvalidArgumentError("Invalid null character in remove_headers mutation."); + return absl::InvalidArgumentError("Invalid character in remove_headers mutation."); } const LowerCaseString remove_header(hdr); switch (checker.check(CheckOperation::REMOVE, remove_header, "")) { @@ -67,11 +67,12 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, if (!sh.has_header()) { continue; } - if (!Envoy::Http::validHeaderString(sh.header().key()) || - !Envoy::Http::validHeaderString(sh.header().value())) { - ENVOY_LOG(debug, "set_headers contain null character in key or value, may not be appended."); + if (!Http::HeaderUtility::headerNameIsValid(sh.header().key()) || + !Http::HeaderUtility::headerValueIsValid(sh.header().value())) { + ENVOY_LOG(debug, + "set_headers contain invalid character in key or value, may not be appended."); rejected_mutations.inc(); - return absl::InvalidArgumentError("Invalid null character in set_headers mutation."); + return absl::InvalidArgumentError("Invalid character in set_headers mutation."); } const LowerCaseString header_name(sh.header().key()); const bool append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, false); From bd41972fd12c6d65de35791e5a68b083b05af530 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Wed, 24 May 2023 02:34:40 +0000 Subject: [PATCH 5/8] ENVOY_LOG_EVERY_POW_W Signed-off-by: Yanjun Xiang --- source/extensions/filters/http/ext_proc/ext_proc.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 40a3d57f53e2..80b50f761abd 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -766,7 +766,7 @@ void Filter::sendImmediateResponse(const ImmediateResponse& response) { response.headers(), headers, false, immediateResponseChecker().checker(), stats_.rejected_header_mutations_); if (!mut_status.ok()) { - ENVOY_LOG(debug, "Immediate response mutations failed"); + ENVOY_LOG_EVERY_POW_2(error, "Immediate response mutations failed"); } } }; From 0d3f4fab8a050682976e576faf58e1b416343030 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Wed, 24 May 2023 14:10:40 +0000 Subject: [PATCH 6/8] null->invalid Signed-off-by: Yanjun Xiang --- .../filters/http/ext_proc/mutation_utils_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 a43a2f1eeabf..669dcbd28b60 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -188,7 +188,7 @@ TEST(MutationUtils, TestNonAppendableHeaders) { EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); } -TEST(MutationUtils, TestSetHeaderWithNullCharacter) { +TEST(MutationUtils, TestSetHeaderWithInvalidCharacter) { Http::TestRequestHeaderMapImpl headers{ {":method", "GET"}, {"host", "localhost:1000"}, @@ -197,7 +197,7 @@ TEST(MutationUtils, TestSetHeaderWithNullCharacter) { Envoy::Stats::MockCounter rejections; envoy::service::ext_proc::v3::HeaderMutation mutation; auto* s = mutation.add_set_headers(); - // Test header key contains null character. + // Test header key contains invalid character. s->mutable_header()->set_key("x-append-this\n"); s->mutable_header()->set_value("value"); EXPECT_CALL(rejections, inc()); @@ -206,7 +206,7 @@ TEST(MutationUtils, TestSetHeaderWithNullCharacter) { mutation.Clear(); s = mutation.add_set_headers(); - // Test header value contains null character. + // Test header value contains invalid character. s->mutable_header()->set_key("x-append-this"); s->mutable_header()->set_value("value\r"); EXPECT_CALL(rejections, inc()); @@ -214,7 +214,7 @@ TEST(MutationUtils, TestSetHeaderWithNullCharacter) { MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); } -TEST(MutationUtils, TestRemoveHeaderWithNullCharacter) { +TEST(MutationUtils, TestRemoveHeaderWithInvalidCharacter) { Http::TestRequestHeaderMapImpl headers{ {":method", "GET"}, {"host", "localhost:1000"}, From 046a045ae0e4f5b09a7f9e4770a2b3d1bbe7414d Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Wed, 24 May 2023 15:18:40 +0000 Subject: [PATCH 7/8] null -> invalid integration test Signed-off-by: Yanjun Xiang --- .../filters/http/ext_proc/ext_proc_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 e75b538c885e..d137b3e5cda8 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 @@ -1079,7 +1079,7 @@ TEST_P(ExtProcIntegrationTest, GetAndRespondImmediately) { EXPECT_EQ("{\"reason\": \"Not authorized\"}", response->body()); } -TEST_P(ExtProcIntegrationTest, GetAndRespondImmediatelyWithNullCharacter) { +TEST_P(ExtProcIntegrationTest, GetAndRespondImmediatelyWithInvalidCharacter) { initializeConfig(); HttpIntegrationTest::initialize(); auto response = sendDownstreamRequest(absl::nullopt); From 3eb361967fd77751e8064c779ba6f36dd884650d Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Wed, 24 May 2023 23:53:14 +0000 Subject: [PATCH 8/8] status in message Signed-off-by: Yanjun Xiang --- source/extensions/filters/http/ext_proc/ext_proc.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 80b50f761abd..658d580ad089 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -766,7 +766,8 @@ void Filter::sendImmediateResponse(const ImmediateResponse& response) { response.headers(), headers, false, immediateResponseChecker().checker(), stats_.rejected_header_mutations_); if (!mut_status.ok()) { - ENVOY_LOG_EVERY_POW_2(error, "Immediate response mutations failed"); + ENVOY_LOG_EVERY_POW_2(error, "Immediate response mutations failed with {}", + mut_status.message()); } } };