From 2ce2ab8a1e147866f025c86f785ac0d745e363d0 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 6 Jun 2024 13:23:19 +0000 Subject: [PATCH] runtime: rejecting invalid yaml Signed-off-by: Alyssa Wilk --- changelogs/current.yaml | 6 ++ source/common/runtime/runtime_features.cc | 1 + source/common/runtime/runtime_impl.cc | 12 +-- test/common/runtime/runtime_impl_test.cc | 98 +++++++++++++---------- 4 files changed, 69 insertions(+), 48 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 75038cddb5422..b0fa99a54b56b 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -20,6 +20,12 @@ behavior_changes: change: | Passes HTTP/2 DATA frames through a different codec API. This behavior can be temporarily disabled by setting the runtime feature ``envoy.reloadable_features.http2_use_visitor_for_data`` to false. +- area: runtime + change: | + Rejecting invalid yaml. This has been an ENVOY_BUG linked to https://github.com/envoyproxy/envoy/issues/27434 + for over a year with no hard-blockers so should be Ok. This behavior can be temporarily disabled by setting + the runtime feature ``envoy.reloadable_features.reject_invalid_yaml`` to false but the runtime guard must be + parsed before any invalid yaml. - area: proxy_protocol change: | Populate typed metadata by default in proxy protocol listener. Typed metadata can be consumed as diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 91ecb569ee5f2..bf95648fd8f82 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -84,6 +84,7 @@ RUNTIME_GUARD(envoy_reloadable_features_quic_receive_ecn); // @danzh2010 or @RyanTheOptimist before removing. RUNTIME_GUARD(envoy_reloadable_features_quic_send_server_preferred_address_to_all_clients); RUNTIME_GUARD(envoy_reloadable_features_quic_upstream_reads_fixed_number_packets); +RUNTIME_GUARD(envoy_reloadable_features_reject_invalid_yaml); RUNTIME_GUARD(envoy_reloadable_features_sanitize_te); RUNTIME_GUARD(envoy_reloadable_features_send_header_raw_value); RUNTIME_GUARD(envoy_reloadable_features_send_local_reply_when_no_buffer_and_upstream_request); diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 253ea9710471b..adc3e45d29c45 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -386,11 +386,13 @@ SnapshotImpl::Entry SnapshotImpl::createEntry(const ProtobufWkt::Value& value, break; case ProtobufWkt::Value::kStringValue: parseEntryDoubleValue(entry); - if (parseEntryBooleanValue(entry)) { - error_message = kBoolError; - } - if (parseEntryFractionalPercentValue(entry)) { - error_message = kFractionError; + if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.reject_invalid_yaml")) { + if (parseEntryBooleanValue(entry)) { + error_message = kBoolError; + } + if (parseEntryFractionalPercentValue(entry)) { + error_message = kFractionError; + } } default: break; diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index fa6415809cdec..0823513c4d7ae 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/test/common/runtime/runtime_impl_test.cc @@ -1,6 +1,7 @@ #include #include +#include "absl/flags/declare.h" #include "envoy/config/bootstrap/v3/bootstrap.pb.h" #include "envoy/config/core/v3/config_source.pb.h" #include "envoy/service/discovery/v3/discovery.pb.h" @@ -31,6 +32,7 @@ #ifdef ENVOY_ENABLE_QUIC #include "quiche/common/platform/api/quiche_flags.h" #endif +ABSL_DECLARE_FLAG(bool, envoy_reloadable_features_reject_invalid_yaml); using testing::_; using testing::Invoke; @@ -548,6 +550,7 @@ class StaticLoaderImplTest : public LoaderImplTest { THROW_IF_NOT_OK(loader.status()); loader_ = std::move(loader.value()); } + void testAllTheThings(bool allow_invalid_yaml); ProtobufWkt::Struct base_; }; @@ -604,8 +607,8 @@ TEST_F(StaticLoaderImplTest, ProtoParsingInvalidField) { EXPECT_THROW_WITH_MESSAGE(setup(), EnvoyException, "Invalid runtime entry value for file0"); } -// Validate proto parsing sanity. -TEST_F(StaticLoaderImplTest, ProtoParsing) { +void StaticLoaderImplTest::testAllTheThings(bool allow_invalid_yaml) { + // Validate proto parsing sanity. base_ = TestUtility::parseYaml(R"EOF( file1: hello override file2: world @@ -758,48 +761,57 @@ TEST_F(StaticLoaderImplTest, ProtoParsing) { ProtobufWkt::Value empty_value; const_cast(dynamic_cast(loader_->snapshot())) .createEntry(empty_value, "", error); + if (allow_invalid_yaml) { + // Make sure the hacky fractional percent function works. + ProtobufWkt::Value fractional_value; + fractional_value.set_string_value(" numerator: 11 "); + auto entry = const_cast(dynamic_cast(loader_->snapshot())) + .createEntry(fractional_value, "", error); + ASSERT_TRUE(entry.fractional_percent_value_.has_value()); + EXPECT_EQ(entry.fractional_percent_value_->denominator(), + envoy::type::v3::FractionalPercent::HUNDRED); + EXPECT_EQ(entry.fractional_percent_value_->numerator(), 11); + + // Make sure the hacky percent function works with numerator and denominator + fractional_value.set_string_value("{\"numerator\": 10000, \"denominator\": \"TEN_THOUSAND\"}"); + entry = const_cast(dynamic_cast(loader_->snapshot())) + .createEntry(fractional_value, "", error); + ASSERT_TRUE(entry.fractional_percent_value_.has_value()); + EXPECT_EQ(entry.fractional_percent_value_->denominator(), + envoy::type::v3::FractionalPercent::TEN_THOUSAND); + EXPECT_EQ(entry.fractional_percent_value_->numerator(), 10000); + + // Make sure the hacky fractional percent function works with million + fractional_value.set_string_value("{\"numerator\": 10000, \"denominator\": \"MILLION\"}"); + entry = const_cast(dynamic_cast(loader_->snapshot())) + .createEntry(fractional_value, "", error); + ASSERT_TRUE(entry.fractional_percent_value_.has_value()); + EXPECT_EQ(entry.fractional_percent_value_->denominator(), + envoy::type::v3::FractionalPercent::MILLION); + EXPECT_EQ(entry.fractional_percent_value_->numerator(), 10000); + + // Test atoi failure for the hacky fractional percent value function. + fractional_value.set_string_value(" numerator: 1.1 "); + entry = const_cast(dynamic_cast(loader_->snapshot())) + .createEntry(fractional_value, "", error); + ASSERT_FALSE(entry.fractional_percent_value_.has_value()); + + // Test legacy malformed boolean support + ProtobufWkt::Value boolean_value; + boolean_value.set_string_value("FaLsE"); + entry = const_cast(dynamic_cast(loader_->snapshot())) + .createEntry(boolean_value, "", error); + ASSERT_TRUE(entry.bool_value_.has_value()); + ASSERT_FALSE(entry.bool_value_.value()); + } +} + +TEST_F(StaticLoaderImplTest, ProtoParsing) { testAllTheThings(false); } + +TEST_F(StaticLoaderImplTest, ProtoParsingLegacy) { + absl::SetFlag(&FLAGS_envoy_reloadable_features_reject_invalid_yaml, false); - // Make sure the hacky fractional percent function works. - ProtobufWkt::Value fractional_value; - fractional_value.set_string_value(" numerator: 11 "); - auto entry = const_cast(dynamic_cast(loader_->snapshot())) - .createEntry(fractional_value, "", error); - ASSERT_TRUE(entry.fractional_percent_value_.has_value()); - EXPECT_EQ(entry.fractional_percent_value_->denominator(), - envoy::type::v3::FractionalPercent::HUNDRED); - EXPECT_EQ(entry.fractional_percent_value_->numerator(), 11); - - // Make sure the hacky percent function works with numerator and denominator - fractional_value.set_string_value("{\"numerator\": 10000, \"denominator\": \"TEN_THOUSAND\"}"); - entry = const_cast(dynamic_cast(loader_->snapshot())) - .createEntry(fractional_value, "", error); - ASSERT_TRUE(entry.fractional_percent_value_.has_value()); - EXPECT_EQ(entry.fractional_percent_value_->denominator(), - envoy::type::v3::FractionalPercent::TEN_THOUSAND); - EXPECT_EQ(entry.fractional_percent_value_->numerator(), 10000); - - // Make sure the hacky fractional percent function works with million - fractional_value.set_string_value("{\"numerator\": 10000, \"denominator\": \"MILLION\"}"); - entry = const_cast(dynamic_cast(loader_->snapshot())) - .createEntry(fractional_value, "", error); - ASSERT_TRUE(entry.fractional_percent_value_.has_value()); - EXPECT_EQ(entry.fractional_percent_value_->denominator(), - envoy::type::v3::FractionalPercent::MILLION); - EXPECT_EQ(entry.fractional_percent_value_->numerator(), 10000); - - // Test atoi failure for the hacky fractional percent value function. - fractional_value.set_string_value(" numerator: 1.1 "); - entry = const_cast(dynamic_cast(loader_->snapshot())) - .createEntry(fractional_value, "", error); - ASSERT_FALSE(entry.fractional_percent_value_.has_value()); - - // Test legacy malformed boolean support - ProtobufWkt::Value boolean_value; - boolean_value.set_string_value("FaLsE"); - entry = const_cast(dynamic_cast(loader_->snapshot())) - .createEntry(boolean_value, "", error); - ASSERT_TRUE(entry.bool_value_.has_value()); - ASSERT_FALSE(entry.bool_value_.value()); + testAllTheThings(true); } TEST_F(StaticLoaderImplTest, InvalidNumerator) {