Skip to content

ext_proc: Check validity of the :status header #17596

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
snowp merged 1 commit into from
Aug 10, 2021
Merged

ext_proc: Check validity of the :status header #17596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion source/extensions/filters/http/ext_proc/ext_proc.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Copy link
Contributor

@snowp snowp Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to signal an error to the processing server if we get back an invalid status code in the response? Might make it easier for operators to understand that something is misconfigured

Copy link
Contributor Author

@gbrail gbrail Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now we don't have a mechanism to do that. That applies to attempts to set invalid headers as well as attempts to set an invalid status. I'm not sure what would be a good way -- for instance, we could send a list of error messages on the next message that we send to the processor, or we could introduce a third set of messages (request, response, and response status) to the protocol -- but I feel like either of these would make things more complicated and make it harder to write a processor.

One thing we should probably do is introduce another statistic so that there is a way to count the number of invalid responses by looking at a stat instead of requiring that people turn on debug.

Copy link
Contributor

@snowp snowp Aug 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think a statistic would be good, enabling debug logging is often not an option when debugging production systems due the log volume

Maybe follow up with that in another PR?

status_code = DefaultImmediateStatus;
}
const auto grpc_status =
response.has_grpc_status()
? absl::optional<Grpc::Status::GrpcStatus>(response.grpc_status().status())
Expand All @@ -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<Http::Code>(status_code), response.body(),
mutate_headers, grpc_status, response.details());
}
Expand Down
58 changes: 45 additions & 13 deletions source/extensions/filters/http/ext_proc/mutation_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
}
}
}
Expand Down Expand Up @@ -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
Copy link
Contributor

@snowp snowp Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are usually referred to as psuedo headers, not system headers

// were never designed to support multiple values.
bool MutationUtils::isAppendableHeader(absl::string_view key) {
return !key.empty() && key[0] != ':';
}

} // namespace ExternalProcessing
Expand Down
7 changes: 6 additions & 1 deletion source/extensions/filters/http/ext_proc/mutation_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,13 @@ class MutationUtils : public Logger::Loggable<Logger::Id::filter> {
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
Expand Down
83 changes: 81 additions & 2 deletions test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand All @@ -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
Expand All @@ -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) {
Expand Down
42 changes: 42 additions & 0 deletions test/extensions/filters/http/ext_proc/mutation_utils_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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{
Expand All @@ -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"},
Expand All @@ -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;
Expand Down