Skip to content

Commit

Permalink
http2: removing legacy configuration options (#9783)
Browse files Browse the repository at this point in the history 
Removing some configuration knobs which were intended to be temporary runtime override knobs for high risk security fixes. They've been running in prod so far so good. Time to clean up.

Risk Level: Medium (someone could have missed they were temporary)
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Fixes #8993

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
  • Loading branch information
@alyssawilk
alyssawilk authored Jan 27, 2020
1 parent e0dd91b commit f57e280
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 92 deletions.
45 changes: 8 additions & 37 deletions source/common/http/http2/codec_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -347,57 +347,28 @@ void ConnectionImpl::StreamImpl::onMetadataDecoded(MetadataMapPtr&& metadata_map
decoder_->decodeMetadata(std::move(metadata_map_ptr));
}

namespace {

const char InvalidHttpMessagingOverrideKey[] =
"envoy.reloadable_features.http2_protocol_options.stream_error_on_invalid_http_messaging";
const char MaxOutboundFramesOverrideKey[] =
"envoy.reloadable_features.http2_protocol_options.max_outbound_frames";
const char MaxOutboundControlFramesOverrideKey[] =
"envoy.reloadable_features.http2_protocol_options.max_outbound_control_frames";
const char MaxConsecutiveInboundFramesWithEmptyPayloadOverrideKey[] =
"envoy.reloadable_features.http2_protocol_options."
"max_consecutive_inbound_frames_with_empty_payload";
const char MaxInboundPriorityFramesPerStreamOverrideKey[] =
"envoy.reloadable_features.http2_protocol_options.max_inbound_priority_frames_per_stream";
const char MaxInboundWindowUpdateFramesPerDataFrameSentOverrideKey[] =
"envoy.reloadable_features.http2_protocol_options."
"max_inbound_window_update_frames_per_data_frame_sent";

bool checkRuntimeOverride(bool config_value, const char* override_key) {
return Runtime::runtimeFeatureEnabled(override_key) ? true : config_value;
}

} // namespace

ConnectionImpl::ConnectionImpl(Network::Connection& connection, Stats::Scope& stats,
const Http2Settings& http2_settings, const uint32_t max_headers_kb,
const uint32_t max_headers_count)
: stats_{ALL_HTTP2_CODEC_STATS(POOL_COUNTER_PREFIX(stats, "http2."))}, connection_(connection),
max_headers_kb_(max_headers_kb), max_headers_count_(max_headers_count),
per_stream_buffer_limit_(http2_settings.initial_stream_window_size_),
stream_error_on_invalid_http_messaging_(checkRuntimeOverride(
http2_settings.stream_error_on_invalid_http_messaging_, InvalidHttpMessagingOverrideKey)),
flood_detected_(false),
max_outbound_frames_(
Runtime::getInteger(MaxOutboundFramesOverrideKey, http2_settings.max_outbound_frames_)),
stream_error_on_invalid_http_messaging_(
http2_settings.stream_error_on_invalid_http_messaging_),
flood_detected_(false), max_outbound_frames_(http2_settings.max_outbound_frames_),
frame_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) {
releaseOutboundFrame(fragment);
}),
max_outbound_control_frames_(Runtime::getInteger(
MaxOutboundControlFramesOverrideKey, http2_settings.max_outbound_control_frames_)),
max_outbound_control_frames_(http2_settings.max_outbound_control_frames_),
control_frame_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) {
releaseOutboundControlFrame(fragment);
}),
max_consecutive_inbound_frames_with_empty_payload_(
Runtime::getInteger(MaxConsecutiveInboundFramesWithEmptyPayloadOverrideKey,
http2_settings.max_consecutive_inbound_frames_with_empty_payload_)),
http2_settings.max_consecutive_inbound_frames_with_empty_payload_),
max_inbound_priority_frames_per_stream_(
Runtime::getInteger(MaxInboundPriorityFramesPerStreamOverrideKey,
http2_settings.max_inbound_priority_frames_per_stream_)),
max_inbound_window_update_frames_per_data_frame_sent_(Runtime::getInteger(
MaxInboundWindowUpdateFramesPerDataFrameSentOverrideKey,
http2_settings.max_inbound_window_update_frames_per_data_frame_sent_)),
http2_settings.max_inbound_priority_frames_per_stream_),
max_inbound_window_update_frames_per_data_frame_sent_(
http2_settings.max_inbound_window_update_frames_per_data_frame_sent_),
dispatching_(false), raised_goaway_(false), pending_deferred_reset_(false) {}

ConnectionImpl::~ConnectionImpl() { nghttp2_session_del(session_); }
Expand Down
2 changes: 0 additions & 2 deletions source/common/runtime/runtime_features.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,6 @@ constexpr const char* runtime_features[] = {
constexpr const char* disabled_runtime_features[] = {
// Sentinel and test flag.
"envoy.reloadable_features.test_feature_false",
// Should be removed as part of https://github.com/envoyproxy/envoy/issues/8993
"envoy.reloadable_features.http2_protocol_options.stream_error_on_invalid_http_messaging",
};

RuntimeFeatures::RuntimeFeatures() {
Expand Down
14 changes: 1 addition & 13 deletions source/common/runtime/runtime_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,18 +34,6 @@ namespace Envoy {
namespace Runtime {
namespace {

// Before ASSERTS were added to ensure runtime features were restricted to
// booleans, several integer features were added. isLegacyFeatures exempts
// existing integer features from the checks until they can be cleaned up.
// This includes
// envoy.reloadable_features.max_[request|response]_headers_count from
// include/envoy/http/codec.h as well as the http2_protocol_options overrides in
// source/common/http/http2/codec_impl.cc
bool isLegacyFeature(absl::string_view feature) {
return absl::StartsWith(feature, "envoy.reloadable_features.http2_protocol_options.") ||
absl::StartsWith(feature, "envoy.reloadable_features.max_re");
}

bool isRuntimeFeature(absl::string_view feature) {
return RuntimeFeaturesDefaults::get().enabledByDefault(feature) ||
RuntimeFeaturesDefaults::get().existsButDisabled(feature);
Expand Down Expand Up @@ -288,7 +276,7 @@ bool SnapshotImpl::featureEnabled(const std::string& key,
}

uint64_t SnapshotImpl::getInteger(const std::string& key, uint64_t default_value) const {
ASSERT(isLegacyFeature(key) || !isRuntimeFeature(key));
ASSERT(!isRuntimeFeature(key));
auto entry = values_.find(key);
if (entry == values_.end() || !entry->second.uint_value_) {
return default_value;
Expand Down
45 changes: 5 additions & 40 deletions test/common/http/http2/codec_impl_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
#include "test/mocks/protobuf/mocks.h"
#include "test/mocks/thread_local/mocks.h"
#include "test/test_common/printers.h"
#include "test/test_common/test_runtime.h"
#include "test/test_common/utility.h"

#include "codec_impl_test_util.h"
Expand Down Expand Up @@ -234,10 +233,6 @@ class Http2CodecImplTest : public ::testing::TestWithParam<Http2SettingsTestPara
data.add(emptyDataFrame.data(), emptyDataFrame.size());
}
}

// Make sure the test fixture has a fake runtime, for the tests which use
// Runtime::LoaderSingleton::getExisting()->mergeValues(...)
TestScopedRuntime scoped_runtime_;
};

TEST_P(Http2CodecImplTest, ShutdownNotice) {
Expand Down Expand Up @@ -481,25 +476,6 @@ TEST_P(Http2CodecImplTest, InvalidHeadersFrameAllowed) {
server_wrapper_.dispatch(Buffer::OwnedImpl(), *server_);
}

TEST_P(Http2CodecImplTest, InvalidHeadersFrameOverridden) {
Runtime::LoaderSingleton::getExisting()->mergeValues(
{{"envoy.reloadable_features.http2_protocol_options.stream_error_on_invalid_http_messaging",
"true"}});
initialize();

MockStreamCallbacks request_callbacks;
request_encoder_->getStream().addCallbacks(request_callbacks);

ON_CALL(client_connection_, write(_, _))
.WillByDefault(
Invoke([&](Buffer::Instance& data, bool) -> void { server_wrapper_.buffer_.add(data); }));

request_encoder_->encodeHeaders(TestHeaderMapImpl{}, true);
EXPECT_CALL(server_stream_callbacks_, onResetStream(StreamResetReason::LocalReset, _));
EXPECT_CALL(request_callbacks, onResetStream(StreamResetReason::RemoteReset, _));
server_wrapper_.dispatch(Buffer::OwnedImpl(), *server_);
}

TEST_P(Http2CodecImplTest, TrailingHeaders) {
initialize();

Expand Down Expand Up @@ -1305,9 +1281,7 @@ TEST_P(Http2CodecImplTest, PingFlood) {

// Verify that codec allows PING flood when mitigation is disabled
TEST_P(Http2CodecImplTest, PingFloodMitigationDisabled) {
Runtime::LoaderSingleton::getExisting()->mergeValues(
{{"envoy.reloadable_features.http2_protocol_options.max_outbound_control_frames",
"2147483647"}});
max_outbound_control_frames_ = 2147483647;
initialize();

TestHeaderMapImpl request_headers;
Expand Down Expand Up @@ -1432,8 +1406,7 @@ TEST_P(Http2CodecImplTest, ResponseDataFlood) {

// Verify that codec allows outbound DATA flood when mitigation is disabled
TEST_P(Http2CodecImplTest, ResponseDataFloodMitigationDisabled) {
Runtime::LoaderSingleton::getExisting()->mergeValues(
{{"envoy.reloadable_features.http2_protocol_options.max_outbound_frames", "2147483647"}});
max_outbound_control_frames_ = 2147483647;
initialize();

TestHeaderMapImpl request_headers;
Expand Down Expand Up @@ -1540,9 +1513,7 @@ TEST_P(Http2CodecImplTest, PriorityFlood) {
}

TEST_P(Http2CodecImplTest, PriorityFloodOverride) {
Runtime::LoaderSingleton::getExisting()->mergeValues(
{{"envoy.reloadable_features.http2_protocol_options.max_inbound_priority_frames_per_stream",
"2147483647"}});
max_inbound_priority_frames_per_stream_ = 2147483647;

priorityFlood();
EXPECT_NO_THROW(client_->sendPendingFrames());
Expand All @@ -1554,10 +1525,7 @@ TEST_P(Http2CodecImplTest, WindowUpdateFlood) {
}

TEST_P(Http2CodecImplTest, WindowUpdateFloodOverride) {
Runtime::LoaderSingleton::getExisting()->mergeValues(
{{"envoy.reloadable_features.http2_protocol_options.max_inbound_window_update_frames_per_"
"data_frame_sent",
"2147483647"}});
max_inbound_window_update_frames_per_data_frame_sent_ = 2147483647;
windowUpdateFlood();
EXPECT_NO_THROW(client_->sendPendingFrames());
}
Expand All @@ -1570,10 +1538,7 @@ TEST_P(Http2CodecImplTest, EmptyDataFlood) {
}

TEST_P(Http2CodecImplTest, EmptyDataFloodOverride) {
Runtime::LoaderSingleton::getExisting()->mergeValues(
{{"envoy.reloadable_features.http2_protocol_options.max_consecutive_inbound_frames_with_"
"empty_payload",
"2147483647"}});
max_consecutive_inbound_frames_with_empty_payload_ = 2147483647;
Buffer::OwnedImpl data;
emptyDataFlood(data);
EXPECT_CALL(request_decoder_, decodeData(_, false))
Expand Down

0 comments on commit f57e280

Please sign in to comment.