Skip to content
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

http: adding response code details for downstream HTTP/1.1 codec errors #9286

Merged
merged 4 commits into from
Dec 12, 2019
Merged

http: adding response code details for downstream HTTP/1.1 codec errors #9286

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f7b3c4f
http: adding response code details for HTTP/1.1 codec errors
alyssawilk Dec 5, 2019
5bf39a3
reviewer comments, connect bugfix
alyssawilk Dec 11, 2019
47dc75a
format and comment
alyssawilk Dec 11, 2019
82e6937
Merge branch 'master' into http1_details
alyssawilk Dec 11, 2019
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
5 changes: 5 additions & 0 deletions include/envoy/http/codec.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,11 @@ class Stream {
* @return uint32_t the stream's configured buffer limits.
*/
virtual uint32_t bufferLimit() PURE;

/*
* @return string_view optionally return the reason behind codec level errors.
*/
virtual absl::string_view responseDetails() { return ""; }
Copy link
Member

Choose a reason for hiding this comment

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

Instead of returning this error this way, WDYT about just throwing this out with the CodecProtocolException? I think this would lead to clearer logic since in the HCM it can just be passed into the code that resets all of the streams?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I started with that, but it didn't work the way I wanted to, because if one H2 stream has bad headers, I only wanted that particular stream to be tagged with bad headers - the others would maybe eventually get flagged with "you shared a connection with a bad stream" but shouldn't be tagged with the error that wasn't theirs. Only the codec knows which stream causes the problem so it has to be on a per-stream basis to be tagged correctly.

Copy link
Member

Choose a reason for hiding this comment

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

OK that makes sense. Can you leave a comment trail about that thought process?

};

/**
Expand Down
4 changes: 4 additions & 0 deletions source/common/http/conn_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,10 @@ void ConnectionManagerImpl::resetAllStreams(
stream.onResetStream(StreamResetReason::ConnectionTermination, absl::string_view());
if (response_flag.has_value()) {
stream.stream_info_.setResponseFlag(response_flag.value());
if (response_flag.value() == StreamInfo::ResponseFlag::DownstreamProtocolError) {
Copy link
Contributor

Choose a reason for hiding this comment

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

micro optimization: you already checked that is has_value() above. You can use the value directly *response_flag and save a branch.

stream.stream_info_.setResponseCodeDetails(
stream.response_encoder_->getStream().responseDetails());
}
}
}
}
Expand Down
34 changes: 27 additions & 7 deletions source/common/http/http1/codec_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,17 @@ namespace Http {
namespace Http1 {
namespace {

struct Http1ResponseCodeDetailValues {
const std::string TooManyHeaders = "http1.too_many_headers";
const std::string HeadersTooLarge = "http1.headers_too_large";
const std::string HttpCodecError = "http1.codec_error";
const std::string InvalidCharacters = "http1.invalid_characters";
const std::string ConnectionHeaderSanitization = "http1.connection_header_bad";
const std::string InvalidUrl = "http1.invalid_url";
};

using Http1ResponseCodeDetails = ConstSingleton<Http1ResponseCodeDetailValues>;
Copy link
Contributor

Choose a reason for hiding this comment

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

These can all be absl::string_view instead. They're always passed to functions expecting string_view.

Also, honestly since there's no initialization order fiasco here involved.
Why not just declare all those literals as constexpr auto var = "..."?
Then you wouldn't need any allocations in the singleton and no std::string allocations either (even though many of the values here qualify for SSO)


const StringUtil::CaseUnorderedSet& caseUnorderdSetContainingUpgradeAndHttp2Settings() {
CONSTRUCT_ON_FIRST_USE(StringUtil::CaseUnorderedSet,
Http::Headers::get().ConnectionValues.Upgrade,
Expand Down Expand Up @@ -381,7 +392,7 @@ void ConnectionImpl::completeLastHeader() {
// Check if the number of headers exceeds the limit.
if (current_header_map_->size() > max_headers_count_) {
error_code_ = Http::Code::RequestHeaderFieldsTooLarge;
sendProtocolError();
sendProtocolError(Http1ResponseCodeDetails::get().TooManyHeaders);
throw CodecProtocolException("headers size exceeds limit");
}

Expand Down Expand Up @@ -442,7 +453,7 @@ void ConnectionImpl::dispatch(Buffer::Instance& data) {
size_t ConnectionImpl::dispatchSlice(const char* slice, size_t len) {
ssize_t rc = http_parser_execute(&parser_, &settings_, slice, len);
if (HTTP_PARSER_ERRNO(&parser_) != HPE_OK && HTTP_PARSER_ERRNO(&parser_) != HPE_PAUSED) {
sendProtocolError();
sendProtocolError(Http1ResponseCodeDetails::get().HttpCodecError);
throw CodecProtocolException("http/1.1 protocol error: " +
std::string(http_errno_name(HTTP_PARSER_ERRNO(&parser_))));
}
Expand Down Expand Up @@ -475,7 +486,7 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) {
if (!Http::HeaderUtility::headerIsValid(header_value)) {
ENVOY_CONN_LOG(debug, "invalid header value: {}", connection_, header_value);
error_code_ = Http::Code::BadRequest;
sendProtocolError();
sendProtocolError(Http1ResponseCodeDetails::get().InvalidCharacters);
throw CodecProtocolException("http/1.1 protocol error: header value contains invalid chars");
}
} else if (header_value.find('\0') != absl::string_view::npos) {
Expand All @@ -496,7 +507,7 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) {
if (total > (max_headers_kb_ * 1024)) {

error_code_ = Http::Code::RequestHeaderFieldsTooLarge;
sendProtocolError();
sendProtocolError(Http1ResponseCodeDetails::get().HeadersTooLarge);
throw CodecProtocolException("headers size exceeds limit");
}
}
Expand Down Expand Up @@ -543,7 +554,7 @@ int ConnectionImpl::onHeadersCompleteBase() {
ENVOY_CONN_LOG(debug, "Invalid nominated headers in Connection: {}", connection_,
header_value);
error_code_ = Http::Code::BadRequest;
sendProtocolError();
sendProtocolError(Http1ResponseCodeDetails::get().ConnectionHeaderSanitization);
}
}

Expand Down Expand Up @@ -631,7 +642,7 @@ void ServerConnectionImpl::handlePath(HeaderMapImpl& headers, unsigned int metho

Utility::Url absolute_url;
if (!absolute_url.initialize(active_request_->request_url_.getStringView())) {
sendProtocolError();
sendProtocolError(Http1ResponseCodeDetails::get().InvalidUrl);
throw CodecProtocolException("http/1.1 protocol error: invalid url in request line");
}
// RFC7230#5.7
Expand Down Expand Up @@ -738,7 +749,10 @@ void ServerConnectionImpl::onResetStream(StreamResetReason reason) {
active_request_.reset();
}

void ServerConnectionImpl::sendProtocolError() {
void ServerConnectionImpl::sendProtocolError(absl::string_view details) {
if (active_request_) {
active_request_->response_encoder_.setDetails(details);
}
// We do this here because we may get a protocol error before we have a logical stream. Higher
// layers can only operate on streams, so there is no coherent way to allow them to send an error
// "out of band." On one hand this is kind of a hack but on the other hand it normalizes HTTP/1.1
Expand Down Expand Up @@ -875,6 +889,12 @@ void ClientConnectionImpl::onResetStream(StreamResetReason reason) {
}
}

void ClientConnectionImpl::sendProtocolError(absl::string_view details) {
if (request_encoder_) {
request_encoder_->setDetails(details);
}
}

void ClientConnectionImpl::onAboveHighWatermark() {
// This should never happen without an active stream/request.
ASSERT(!pending_responses_.empty());
Expand Down
9 changes: 6 additions & 3 deletions source/common/http/http1/codec_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,10 @@ class StreamEncoderImpl : public StreamEncoder,
void resetStream(StreamResetReason reason) override;
void readDisable(bool disable) override;
uint32_t bufferLimit() override;
absl::string_view responseDetails() override { return details_; }

void isResponseToHeadRequest(bool value) { is_response_to_head_request_ = value; }
void setDetails(absl::string_view details) { details_ = details; }

protected:
StreamEncoderImpl(ConnectionImpl& connection, HeaderKeyFormatter* header_key_formatter);
Expand Down Expand Up @@ -101,6 +103,7 @@ class StreamEncoderImpl : public StreamEncoder,
bool processing_100_continue_ : 1;
bool is_response_to_head_request_ : 1;
bool is_content_length_allowed_ : 1;
absl::string_view details_;
};

/**
Expand Down Expand Up @@ -282,7 +285,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
/**
* Send a protocol error response to remote.
*/
virtual void sendProtocolError() PURE;
virtual void sendProtocolError(absl::string_view details = "") PURE;

/**
* Called when output_buffer_ or the underlying connection go from below a low watermark to over
Expand Down Expand Up @@ -355,7 +358,7 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
void onBody(const char* data, size_t length) override;
void onMessageComplete() override;
void onResetStream(StreamResetReason reason) override;
void sendProtocolError() override;
void sendProtocolError(absl::string_view details) override;
void onAboveHighWatermark() override;
void onBelowLowWatermark() override;

Expand Down Expand Up @@ -395,7 +398,7 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl {
void onBody(const char* data, size_t length) override;
void onMessageComplete() override;
void onResetStream(StreamResetReason reason) override;
void sendProtocolError() override {}
void sendProtocolError(absl::string_view details) override;
void onAboveHighWatermark() override;
void onBelowLowWatermark() override;

Expand Down
1 change: 1 addition & 0 deletions test/common/http/http1/codec_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1349,6 +1349,7 @@ TEST_F(Http1ServerConnectionImplTest, LargeRequestHeadersSplitRejected) {
// the 60th 1kb header should induce overflow
buffer = Buffer::OwnedImpl(fmt::format("big: {}\r\n", long_string));
EXPECT_THROW_WITH_MESSAGE(codec_->dispatch(buffer), EnvoyException, "headers size exceeds limit");
EXPECT_EQ("http1.headers_too_large", response_encoder->getStream().responseDetails());
Copy link
Member

Choose a reason for hiding this comment

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

Can we get tests that cover each of the new details?

}

// Tests that the 101th request header causes overflow with the default max number of request
Expand Down
2 changes: 2 additions & 0 deletions test/integration/integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,11 +336,13 @@ TEST_P(IntegrationTest, BadFirstline) {
}

TEST_P(IntegrationTest, MissingDelimiter) {
useAccessLog("%RESPONSE_CODE_DETAILS%");
initialize();
std::string response;
sendRawHttpAndWaitForResponse(lookupPort("http"),
"GET / HTTP/1.1\r\nHost: host\r\nfoo bar\r\n\r\n", &response);
EXPECT_EQ("HTTP/1.1 400 Bad Request\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", response);
EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("http1.codec_error"));
}

TEST_P(IntegrationTest, InvalidCharacterInFirstline) {
Expand Down