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

http2: refactor flood checks into a separate class. #13098

Merged
merged 7 commits into from
Sep 18, 2020
Merged

http2: refactor flood checks into a separate class. #13098

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6e9b760
http2: refactor flood checks into a separate class.
yanavlasov Sep 15, 2020
c18ae70
Add comments
yanavlasov Sep 15, 2020
763593d
Fix uninitialized variable in test
yanavlasov Sep 15, 2020
f797f9a
Address comments
yanavlasov Sep 16, 2020
65fb18d
Address comments
yanavlasov Sep 17, 2020
b5eaf30
Merge branch 'master' into flood-disconnect
yanavlasov Sep 17, 2020
a15fbe4
Address comments
yanavlasov Sep 18, 2020
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
2 changes: 1 addition & 1 deletion source/common/http/conn_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data, bool
const Status status = codec_->dispatch(data);

ASSERT(!isPrematureResponseError(status));
if (isBufferFloodError(status)) {
if (isBufferFloodError(status) || isInboundFramesWithEmptyPayloadError(status)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be appropriate to ASSERT(status.ok()); on line 292, after the end of this if block?

Question is related to the ASSERT(!isPrematureResponseError(status)); on the previous line, is there a danger of us ignoring some of error status that is added in the future? Granted, this seems like a pre-existing issue.

Copy link
Contributor Author

@yanavlasov yanavlasov Sep 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I think it is a good suggestion. Fixed

handleCodecError(status.message());
return Network::FilterStatus::StopIteration;
} else if (isCodecProtocolError(status)) {
Expand Down
15 changes: 15 additions & 0 deletions source/common/http/http2/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ CODEC_LIB_DEPS = [
":codec_stats_lib",
":metadata_decoder_lib",
":metadata_encoder_lib",
":protocol_constraints_lib",
"//include/envoy/event:deferred_deletable",
"//include/envoy/event:dispatcher_interface",
"//include/envoy/http:codec_interface",
Expand Down Expand Up @@ -131,3 +132,17 @@ envoy_cc_library(
"//source/common/common:minimal_logger_lib",
],
)

envoy_cc_library(
name = "protocol_constraints_lib",
srcs = ["protocol_constraints.cc"],
hdrs = ["protocol_constraints.h"],
deps = [
":codec_stats_lib",
"//bazel/foreign_cc:nghttp2",
"//include/envoy/network:connection_interface",
"//source/common/common:assert_lib",
"//source/common/http:status_lib",
"@envoy_api//envoy/config/core/v3:pkg_cc_proto",
],
)
136 changes: 23 additions & 113 deletions source/common/http/http2/codec_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ Status ConnectionImpl::StreamImpl::onDataSourceSend(const uint8_t* framehd, size
// https://nghttp2.org/documentation/types.html#c.nghttp2_send_data_callback
static const uint64_t FRAME_HEADER_SIZE = 9;

parent_.outbound_data_frames_++;
parent_.protocol_constraints_.incrementOutboundDataFrameCount();

Buffer::OwnedImpl output;
auto status = parent_.addOutboundFrameFragment(output, framehd, FRAME_HEADER_SIZE);
Expand Down Expand Up @@ -552,16 +552,7 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stat
per_stream_buffer_limit_(http2_options.initial_stream_window_size().value()),
stream_error_on_invalid_http_messaging_(
http2_options.override_stream_error_on_invalid_http_message().value()),
max_outbound_frames_(http2_options.max_outbound_frames().value()),
frame_buffer_releasor_([this]() { releaseOutboundFrame(); }),
max_outbound_control_frames_(http2_options.max_outbound_control_frames().value()),
control_frame_buffer_releasor_([this]() { releaseOutboundControlFrame(); }),
max_consecutive_inbound_frames_with_empty_payload_(
http2_options.max_consecutive_inbound_frames_with_empty_payload().value()),
max_inbound_priority_frames_per_stream_(
http2_options.max_inbound_priority_frames_per_stream().value()),
max_inbound_window_update_frames_per_data_frame_sent_(
http2_options.max_inbound_window_update_frames_per_data_frame_sent().value()),
protocol_constraints_(stats, http2_options),
skip_encoding_empty_trailers_(Runtime::runtimeFeatureEnabled(
"envoy.reloadable_features.http2_skip_encoding_empty_trailers")),
dispatching_(false), raised_goaway_(false), pending_deferred_reset_(false) {}
Expand Down Expand Up @@ -857,39 +848,21 @@ int ConnectionImpl::onBeforeFrameSend(const nghttp2_frame* frame) {
return 0;
}

Status ConnectionImpl::incrementOutboundFrameCount(bool is_outbound_flood_monitored_control_frame) {
++outbound_frames_;
if (is_outbound_flood_monitored_control_frame) {
++outbound_control_frames_;
}
return checkOutboundQueueLimits();
}

Status ConnectionImpl::addOutboundFrameFragment(Buffer::OwnedImpl& output, const uint8_t* data,
size_t length) {
// Reset the outbound frame type (set in the onBeforeFrameSend callback) since the
// onBeforeFrameSend callback is not called for DATA frames.
bool is_outbound_flood_monitored_control_frame = false;
std::swap(is_outbound_flood_monitored_control_frame, is_outbound_flood_monitored_control_frame_);
RETURN_IF_ERROR(incrementOutboundFrameCount(is_outbound_flood_monitored_control_frame));
auto releasor =
protocol_constraints_.incrementOutboundFrameCount(is_outbound_flood_monitored_control_frame);
RETURN_IF_ERROR(checkOutboundFrameLimits());

output.add(data, length);
output.addDrainTracker(is_outbound_flood_monitored_control_frame ? control_frame_buffer_releasor_
: frame_buffer_releasor_);
output.addDrainTracker(releasor);
return okStatus();
}

void ConnectionImpl::releaseOutboundFrame() {
ASSERT(outbound_frames_ >= 1);
--outbound_frames_;
}

void ConnectionImpl::releaseOutboundControlFrame() {
ASSERT(outbound_control_frames_ >= 1);
--outbound_control_frames_;
releaseOutboundFrame();
}

StatusOr<ssize_t> ConnectionImpl::onSend(const uint8_t* data, size_t length) {
ENVOY_CONN_LOG(trace, "send data: bytes={}", connection_, length);
Buffer::OwnedImpl buffer;
Expand Down Expand Up @@ -1037,10 +1010,9 @@ Status ConnectionImpl::sendPendingFrames() {
return nghttp2_callback_status_;
}

// The frame flood error should set the nghttp2_callback_status_ error, and return at the
// statement above.
ASSERT(outbound_frames_ <= max_outbound_frames_ &&
outbound_control_frames_ <= max_outbound_control_frames_);
// Protocol constrain violations should set the nghttp2_callback_status_ error, and return at
// the statement above.
ASSERT(protocol_constraints_.status().ok());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to fail this ASSERT in the upstream direction? Should you be calling ASSERT(checkProtocolConstraintsStatus()); instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should not be anymore. Both inbound and outbound frame limits are only checked in the server codec now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should not be possible now, since the flood checks that can flip status are only done by the server codec.


return codecProtocolError(nghttp2_strerror(rc));
}
Expand Down Expand Up @@ -1420,86 +1392,24 @@ Status ServerConnectionImpl::trackInboundFrames(const nghttp2_frame_hd* hd,
ENVOY_CONN_LOG(trace, "track inbound frame type={} flags={} length={} padding_length={}",
connection_, static_cast<uint64_t>(hd->type), static_cast<uint64_t>(hd->flags),
static_cast<uint64_t>(hd->length), padding_length);
switch (hd->type) {
case NGHTTP2_HEADERS:
case NGHTTP2_CONTINUATION:
// Track new streams.
if (hd->flags & NGHTTP2_FLAG_END_HEADERS) {
inbound_streams_++;
}
FALLTHRU;
case NGHTTP2_DATA:
// Track frames with an empty payload and no end stream flag.
if (hd->length - padding_length == 0 && !(hd->flags & NGHTTP2_FLAG_END_STREAM)) {
ENVOY_CONN_LOG(trace, "frame with an empty payload and no end stream flag.", connection_);
consecutive_inbound_frames_with_empty_payload_++;
} else {
consecutive_inbound_frames_with_empty_payload_ = 0;
}
break;
case NGHTTP2_PRIORITY:
inbound_priority_frames_++;
break;
case NGHTTP2_WINDOW_UPDATE:
inbound_window_update_frames_++;
break;
default:
break;
}

return checkInboundFrameLimits(hd->stream_id);
}

Status ServerConnectionImpl::checkInboundFrameLimits(int32_t stream_id) {
ASSERT(dispatching_downstream_data_);
ConnectionImpl::StreamImpl* stream = getStream(stream_id);

if (consecutive_inbound_frames_with_empty_payload_ >
max_consecutive_inbound_frames_with_empty_payload_) {
ENVOY_CONN_LOG(trace,
"error reading frame: Too many consecutive frames with an empty payload "
"received in this HTTP/2 session.",
connection_);
if (stream) {
stream->setDetails(Http2ResponseCodeDetails::get().inbound_empty_frame_flood);
auto result = protocol_constraints_.trackInboundFrames(hd, padding_length);
if (!result.ok()) {
ENVOY_CONN_LOG(trace, "error reading frame: {} received in this HTTP/2 session.", connection_,
result.message());
if (isInboundFramesWithEmptyPayloadError(result)) {
ConnectionImpl::StreamImpl* stream = getStream(hd->stream_id);
if (stream) {
stream->setDetails(Http2ResponseCodeDetails::get().inbound_empty_frame_flood);
}
}
stats_.inbound_empty_frames_flood_.inc();
return bufferFloodError("Too many consecutive frames with an empty payload");
}

if (inbound_priority_frames_ >
static_cast<uint64_t>(max_inbound_priority_frames_per_stream_) * (1 + inbound_streams_)) {
ENVOY_CONN_LOG(trace,
"error reading frame: Too many PRIORITY frames received in this HTTP/2 session.",
connection_);
stats_.inbound_priority_frames_flood_.inc();
return bufferFloodError("Too many PRIORITY frames");
}

if (inbound_window_update_frames_ >
1 + 2 * (inbound_streams_ +
max_inbound_window_update_frames_per_data_frame_sent_ * outbound_data_frames_)) {
ENVOY_CONN_LOG(
trace,
"error reading frame: Too many WINDOW_UPDATE frames received in this HTTP/2 session.",
connection_);
stats_.inbound_window_update_frames_flood_.inc();
return bufferFloodError("Too many WINDOW_UPDATE frames");
}

return okStatus();
return result;
}

Status ServerConnectionImpl::checkOutboundQueueLimits() {
if (outbound_frames_ > max_outbound_frames_ && dispatching_downstream_data_) {
stats_.outbound_flood_.inc();
return bufferFloodError("Too many frames in the outbound queue.");
}
if (outbound_control_frames_ > max_outbound_control_frames_ && dispatching_downstream_data_) {
stats_.outbound_control_flood_.inc();
return bufferFloodError("Too many control frames in the outbound queue.");
}
return okStatus();
Status ServerConnectionImpl::checkOutboundFrameLimits() {
return dispatching_downstream_data_ ? protocol_constraints_.checkOutboundFrameLimits()
: okStatus();
}

Http::Status ServerConnectionImpl::dispatch(Buffer::Instance& data) {
Expand All @@ -1518,7 +1428,7 @@ Http::Status ServerConnectionImpl::innerDispatch(Buffer::Instance& data) {
Cleanup cleanup([this]() { dispatching_downstream_data_ = false; });

// Make sure downstream outbound queue was not flooded by the upstream frames.
RETURN_IF_ERROR(checkOutboundQueueLimits());
RETURN_IF_ERROR(checkOutboundFrameLimits());
return ConnectionImpl::innerDispatch(data);
}

Expand Down
67 changes: 6 additions & 61 deletions source/common/http/http2/codec_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "common/http/http2/codec_stats.h"
#include "common/http/http2/metadata_decoder.h"
#include "common/http/http2/metadata_encoder.h"
#include "common/http/http2/protocol_constraints.h"
#include "common/http/status.h"
#include "common/http/utility.h"

Expand Down Expand Up @@ -451,57 +452,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
// Set if the type of frame that is about to be sent is PING or SETTINGS with the ACK flag set, or
// RST_STREAM.
bool is_outbound_flood_monitored_control_frame_ = 0;
// This counter keeps track of the number of outbound frames of all types (these that were
// buffered in the underlying connection but not yet written into the socket). If this counter
// exceeds the `max_outbound_frames_' value the connection is terminated.
uint32_t outbound_frames_ = 0;
// Maximum number of outbound frames. Initialized from corresponding http2_protocol_options.
// Default value is 10000.
const uint32_t max_outbound_frames_;
const std::function<void()> frame_buffer_releasor_;
// This counter keeps track of the number of outbound frames of types PING, SETTINGS and
// RST_STREAM (these that were buffered in the underlying connection but not yet written into the
// socket). If this counter exceeds the `max_outbound_control_frames_' value the connection is
// terminated.
uint32_t outbound_control_frames_ = 0;
// Maximum number of outbound frames of types PING, SETTINGS and RST_STREAM. Initialized from
// corresponding http2_protocol_options. Default value is 1000.
const uint32_t max_outbound_control_frames_;
const std::function<void()> control_frame_buffer_releasor_;
// This counter keeps track of the number of consecutive inbound frames of types HEADERS,
// CONTINUATION and DATA with an empty payload and no end stream flag. If this counter exceeds
// the `max_consecutive_inbound_frames_with_empty_payload_` value the connection is terminated.
uint32_t consecutive_inbound_frames_with_empty_payload_ = 0;
// Maximum number of consecutive inbound frames of types HEADERS, CONTINUATION and DATA without
// a payload. Initialized from corresponding http2_protocol_options. Default value is 1.
const uint32_t max_consecutive_inbound_frames_with_empty_payload_;

// This counter keeps track of the number of inbound streams.
uint32_t inbound_streams_ = 0;
// This counter keeps track of the number of inbound PRIORITY frames. If this counter exceeds
// the value calculated using this formula:
//
// max_inbound_priority_frames_per_stream_ * (1 + inbound_streams_)
//
// the connection is terminated.
uint64_t inbound_priority_frames_ = 0;
// Maximum number of inbound PRIORITY frames per stream. Initialized from corresponding
// http2_protocol_options. Default value is 100.
const uint32_t max_inbound_priority_frames_per_stream_;

// This counter keeps track of the number of inbound WINDOW_UPDATE frames. If this counter exceeds
// the value calculated using this formula:
//
// 1 + 2 * (inbound_streams_ +
// max_inbound_window_update_frames_per_data_frame_sent_ * outbound_data_frames_)
//
// the connection is terminated.
uint64_t inbound_window_update_frames_ = 0;
// This counter keeps track of the number of outbound DATA frames.
uint64_t outbound_data_frames_ = 0;
// Maximum number of inbound WINDOW_UPDATE frames per outbound DATA frame sent. Initialized
// from corresponding http2_protocol_options. Default value is 10.
const uint32_t max_inbound_window_update_frames_per_data_frame_sent_;
ProtocolConstraints protocol_constraints_;

// For the flood mitigation to work the onSend callback must be called once for each outbound
// frame. This is what the nghttp2 library is doing, however this is not documented. The
Expand Down Expand Up @@ -535,12 +486,8 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
// Adds buffer fragment for a new outbound frame to the supplied Buffer::OwnedImpl.
// Returns Ok Status on success or error if outbound queue limits were exceeded.
Status addOutboundFrameFragment(Buffer::OwnedImpl& output, const uint8_t* data, size_t length);
virtual Status checkOutboundQueueLimits() PURE;
Status incrementOutboundFrameCount(bool is_outbound_flood_monitored_control_frame);
virtual Status checkOutboundFrameLimits() PURE;
virtual Status trackInboundFrames(const nghttp2_frame_hd* hd, uint32_t padding_length) PURE;
virtual Status checkInboundFrameLimits(int32_t stream_id) PURE;
void releaseOutboundFrame();
void releaseOutboundControlFrame();

bool dispatching_ : 1;
bool raised_goaway_ : 1;
Expand Down Expand Up @@ -576,9 +523,8 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl {
// mitigation on the downstream connections, however there is currently no mechanism for
// handling these types of errors.
// TODO(yanavlasov): add flood mitigation for upstream connections as well.
Status checkOutboundQueueLimits() override { return okStatus(); }
Status checkOutboundFrameLimits() override { return okStatus(); }
Status trackInboundFrames(const nghttp2_frame_hd*, uint32_t) override { return okStatus(); }
Status checkInboundFrameLimits(int32_t) override { return okStatus(); }

Http::ConnectionCallbacks& callbacks_;
};
Expand All @@ -601,16 +547,15 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
ConnectionCallbacks& callbacks() override { return callbacks_; }
Status onBeginHeaders(const nghttp2_frame* frame) override;
int onHeader(const nghttp2_frame* frame, HeaderString&& name, HeaderString&& value) override;
Status checkOutboundQueueLimits() override;
Status checkOutboundFrameLimits() override;
Status trackInboundFrames(const nghttp2_frame_hd* hd, uint32_t padding_length) override;
Status checkInboundFrameLimits(int32_t stream_id) override;
absl::optional<int> checkHeaderNameForUnderscores(absl::string_view header_name) override;

// Http::Connection
// The reason for overriding the dispatch method is to do flood mitigation only when
// processing data from downstream client. Doing flood mitigation when processing upstream
// responses makes clean-up tricky, which needs to be improved (see comments for the
// ClientConnectionImpl::checkOutboundQueueLimits method). The dispatch method on the
// ClientConnectionImpl::checkProtocolConstraintsStatus method). The dispatch method on the
// ServerConnectionImpl objects is called only when processing data from the downstream client in
// the ConnectionManagerImpl::onData method.
Http::Status dispatch(Buffer::Instance& data) override;
Expand Down
Loading