From b885fc46158535283099676b96498b18ad4fc79f Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Thu, 29 Aug 2019 16:56:56 -0400 Subject: [PATCH] protobuf: recursively validate unknown fields. This PR unifies the recursive traversal of deprecated fields with that of unknown fields. It doesn't deal with moving to a validator visitor model for deprecation; this would be a nice cleanup that we track at https://github.com/envoyproxy/envoy/issues/8092. Risk level: Low Testing: New nested unknown field test added. Fixes #7980 Signed-off-by: Harvey Tuch --- source/common/protobuf/utility.cc | 41 +++++++++++++----------- source/common/protobuf/utility.h | 13 +++----- test/common/protobuf/utility_test.cc | 48 +++++++++++++++++++--------- test/tools/router_check/router.cc | 5 ++- 4 files changed, 64 insertions(+), 43 deletions(-) diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 4df4964b768f..b52a716f321f 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -88,21 +88,6 @@ ProtoValidationException::ProtoValidationException(const std::string& validation ENVOY_LOG_MISC(debug, "Proto validation error; throwing {}", what()); } -void MessageUtil::checkUnknownFields(const Protobuf::Message& message, - ProtobufMessage::ValidationVisitor& validation_visitor) { - const auto& unknown_fields = message.GetReflection()->GetUnknownFields(message); - // If there are no unknown fields, we're done here. - if (unknown_fields.empty()) { - return; - } - std::string error_msg; - for (int n = 0; n < unknown_fields.field_count(); ++n) { - error_msg += absl::StrCat(n > 0 ? ", " : "", unknown_fields.field(n).number()); - } - validation_visitor.onUnknownField("type " + message.GetTypeName() + " with unknown field set {" + - error_msg + "}"); -} - void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& message, ProtobufMessage::ValidationVisitor& validation_visitor) { Protobuf::util::JsonParseOptions options; @@ -159,7 +144,7 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa if (absl::EndsWith(path, FileExtensions::get().ProtoBinary)) { // Attempt to parse the binary format. if (message.ParseFromString(contents)) { - MessageUtil::checkUnknownFields(message, validation_visitor); + MessageUtil::checkForUnexpectedFields(message, validation_visitor); return; } throw EnvoyException("Unable to parse file \"" + path + "\" as a binary protobuf (type " + @@ -180,7 +165,23 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa } } -void MessageUtil::checkForDeprecation(const Protobuf::Message& message, Runtime::Loader* runtime) { +void MessageUtil::checkForUnexpectedFields(const Protobuf::Message& message, + ProtobufMessage::ValidationVisitor& validation_visitor, + Runtime::Loader* runtime) { + // Reject unknown fields. + const auto& unknown_fields = message.GetReflection()->GetUnknownFields(message); + if (!unknown_fields.empty()) { + std::string error_msg; + for (int n = 0; n < unknown_fields.field_count(); ++n) { + error_msg += absl::StrCat(n > 0 ? ", " : "", unknown_fields.field(n).number()); + } + // We use the validation visitor but have hard coded behavior below for deprecated fields. + // TODO(htuch): Unify the deprecated and unknown visitor handling behind the validation + // visitor pattern. https://github.com/envoyproxy/envoy/issues/8092. + validation_visitor.onUnknownField("type " + message.GetTypeName() + + " with unknown field set {" + error_msg + "}"); + } + const Protobuf::Descriptor* descriptor = message.GetDescriptor(); const Protobuf::Reflection* reflection = message.GetReflection(); for (int i = 0; i < descriptor->field_count(); ++i) { @@ -231,10 +232,12 @@ void MessageUtil::checkForDeprecation(const Protobuf::Message& message, Runtime: if (field->is_repeated()) { const int size = reflection->FieldSize(message, field); for (int j = 0; j < size; ++j) { - checkForDeprecation(reflection->GetRepeatedMessage(message, field, j), runtime); + checkForUnexpectedFields(reflection->GetRepeatedMessage(message, field, j), + validation_visitor, runtime); } } else { - checkForDeprecation(reflection->GetMessage(message, field), runtime); + checkForUnexpectedFields(reflection->GetMessage(message, field), validation_visitor, + runtime); } } } diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index d0451893d479..00ae4bceb66c 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -206,9 +206,6 @@ class MessageUtil { return HashUtil::xxHash64(text); } - static void checkUnknownFields(const Protobuf::Message& message, - ProtobufMessage::ValidationVisitor& validation_visitor); - static void loadFromJson(const std::string& json, Protobuf::Message& message, ProtobufMessage::ValidationVisitor& validation_visitor); static void loadFromJson(const std::string& json, ProtobufWkt::Struct& message); @@ -225,8 +222,9 @@ class MessageUtil { * in disallowed_features in runtime_features.h */ static void - checkForDeprecation(const Protobuf::Message& message, - Runtime::Loader* loader = Runtime::LoaderSingleton::getExisting()); + checkForUnexpectedFields(const Protobuf::Message& message, + ProtobufMessage::ValidationVisitor& validation_visitor, + Runtime::Loader* loader = Runtime::LoaderSingleton::getExisting()); /** * Validate protoc-gen-validate constraints on a given protobuf. @@ -238,9 +236,8 @@ class MessageUtil { template static void validate(const MessageType& message, ProtobufMessage::ValidationVisitor& validation_visitor) { - // Log warnings or throw errors if deprecated fields are in use. - checkForDeprecation(message); - checkUnknownFields(message, validation_visitor); + // Log warnings or throw errors if deprecated fields or unknown fields are in use. + checkForUnexpectedFields(message, validation_visitor); std::string err; if (!Validate(message, &err)) { diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 4371e4ecc9e8..539dfb6a28f6 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -1,8 +1,10 @@ #include +#include "envoy/api/v2/cds.pb.validate.h" #include "envoy/config/bootstrap/v2/bootstrap.pb.h" #include "envoy/config/bootstrap/v2/bootstrap.pb.validate.h" +#include "common/protobuf/message_validator_impl.h" #include "common/protobuf/protobuf.h" #include "common/protobuf/utility.h" #include "common/runtime/runtime_impl.h" @@ -145,6 +147,19 @@ TEST_F(ProtobufUtilityTest, DowncastAndValidateUnknownFields) { "unknown field set {1}) has unknown fields"); } +// Validated exception thrown when downcastAndValidate observes a nested unknown field. +TEST_F(ProtobufUtilityTest, DowncastAndValidateUnknownFieldsNested) { + envoy::config::bootstrap::v2::Bootstrap bootstrap; + auto* cluster = bootstrap.mutable_static_resources()->add_clusters(); + cluster->GetReflection()->MutableUnknownFields(cluster)->AddVarint(1, 0); + EXPECT_THROW_WITH_MESSAGE(TestUtility::validate(*cluster), EnvoyException, + "Protobuf message (type envoy.api.v2.Cluster with " + "unknown field set {1}) has unknown fields"); + EXPECT_THROW_WITH_MESSAGE(TestUtility::validate(bootstrap), EnvoyException, + "Protobuf message (type envoy.api.v2.Cluster with " + "unknown field set {1}) has unknown fields"); +} + TEST_F(ProtobufUtilityTest, LoadBinaryProtoFromFile) { envoy::config::bootstrap::v2::Bootstrap bootstrap; bootstrap.mutable_cluster_manager() @@ -494,20 +509,24 @@ class DeprecatedFieldsTest : public testing::Test { NiceMock validation_visitor_; }; +void checkForDeprecation(const Protobuf::Message& message) { + MessageUtil::checkForUnexpectedFields(message, ProtobufMessage::getStrictValidationVisitor()); +} + TEST_F(DeprecatedFieldsTest, NoCrashIfRuntimeMissing) { loader_.reset(); envoy::test::deprecation_test::Base base; base.set_not_deprecated("foo"); // Fatal checks for a non-deprecated field should cause no problem. - MessageUtil::checkForDeprecation(base); + checkForDeprecation(base); } TEST_F(DeprecatedFieldsTest, NoErrorWhenDeprecatedFieldsUnused) { envoy::test::deprecation_test::Base base; base.set_not_deprecated("foo"); // Fatal checks for a non-deprecated field should cause no problem. - MessageUtil::checkForDeprecation(base); + checkForDeprecation(base); EXPECT_EQ(0, runtime_deprecated_feature_use_.value()); } @@ -517,7 +536,7 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(IndividualFieldDeprecated)) // Non-fatal checks for a deprecated field should log rather than throw an exception. EXPECT_LOG_CONTAINS("warning", "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'", - MessageUtil::checkForDeprecation(base)); + checkForDeprecation(base)); EXPECT_EQ(1, runtime_deprecated_feature_use_.value()); } @@ -526,7 +545,7 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(IndividualFieldDisallowed)) envoy::test::deprecation_test::Base base; base.set_is_deprecated_fatal("foo"); EXPECT_THROW_WITH_REGEX( - MessageUtil::checkForDeprecation(base), ProtoValidationException, + checkForDeprecation(base), ProtoValidationException, "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated_fatal'"); } @@ -537,7 +556,7 @@ TEST_F(DeprecatedFieldsTest, // Make sure this is set up right. EXPECT_THROW_WITH_REGEX( - MessageUtil::checkForDeprecation(base), ProtoValidationException, + checkForDeprecation(base), ProtoValidationException, "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated_fatal'"); // The config will be rejected, so the feature will not be used. EXPECT_EQ(0, runtime_deprecated_feature_use_.value()); @@ -549,7 +568,7 @@ TEST_F(DeprecatedFieldsTest, // Now the same deprecation check should only trigger a warning. EXPECT_LOG_CONTAINS( "warning", "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated_fatal'", - MessageUtil::checkForDeprecation(base)); + checkForDeprecation(base)); EXPECT_EQ(1, runtime_deprecated_feature_use_.value()); } @@ -559,7 +578,7 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(DisallowViaRuntime)) { EXPECT_LOG_CONTAINS("warning", "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'", - MessageUtil::checkForDeprecation(base)); + checkForDeprecation(base)); EXPECT_EQ(1, runtime_deprecated_feature_use_.value()); // Now create a new snapshot with this feature disallowed. @@ -567,7 +586,7 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(DisallowViaRuntime)) { {{"envoy.deprecated_features.deprecated.proto:is_deprecated", " false"}}); EXPECT_THROW_WITH_REGEX( - MessageUtil::checkForDeprecation(base), ProtoValidationException, + checkForDeprecation(base), ProtoValidationException, "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'"); EXPECT_EQ(1, runtime_deprecated_feature_use_.value()); } @@ -582,7 +601,7 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(MixOfFatalAndWarnings)) { EXPECT_LOG_CONTAINS( "warning", "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'", { EXPECT_THROW_WITH_REGEX( - MessageUtil::checkForDeprecation(base), ProtoValidationException, + checkForDeprecation(base), ProtoValidationException, "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated_fatal'"); }); } @@ -593,7 +612,7 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(MessageDeprecated)) { base.mutable_deprecated_message(); EXPECT_LOG_CONTAINS( "warning", "Using deprecated option 'envoy.test.deprecation_test.Base.deprecated_message'", - MessageUtil::checkForDeprecation(base)); + checkForDeprecation(base)); EXPECT_EQ(1, runtime_deprecated_feature_use_.value()); } @@ -601,15 +620,14 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(InnerMessageDeprecated)) { envoy::test::deprecation_test::Base base; base.mutable_not_deprecated_message()->set_inner_not_deprecated("foo"); // Checks for a non-deprecated field shouldn't trigger warnings - EXPECT_LOG_NOT_CONTAINS("warning", "Using deprecated option", - MessageUtil::checkForDeprecation(base)); + EXPECT_LOG_NOT_CONTAINS("warning", "Using deprecated option", checkForDeprecation(base)); base.mutable_not_deprecated_message()->set_inner_deprecated("bar"); // Checks for a deprecated sub-message should result in a warning. EXPECT_LOG_CONTAINS( "warning", "Using deprecated option 'envoy.test.deprecation_test.Base.InnerMessage.inner_deprecated'", - MessageUtil::checkForDeprecation(base)); + checkForDeprecation(base)); } // Check that repeated sub-messages get validated. @@ -623,7 +641,7 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(SubMessageDeprecated)) { EXPECT_LOG_CONTAINS("warning", "Using deprecated option " "'envoy.test.deprecation_test.Base.InnerMessage.inner_deprecated'", - MessageUtil::checkForDeprecation(base)); + checkForDeprecation(base)); } // Check that deprecated repeated messages trigger @@ -635,7 +653,7 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(RepeatedMessageDeprecated)) EXPECT_LOG_CONTAINS("warning", "Using deprecated option " "'envoy.test.deprecation_test.Base.deprecated_repeated_message'", - MessageUtil::checkForDeprecation(base)); + checkForDeprecation(base)); } class TimestampUtilTest : public testing::Test, public ::testing::WithParamInterface {}; diff --git a/test/tools/router_check/router.cc b/test/tools/router_check/router.cc index 5eeadc7a1bb1..f6881f82640b 100644 --- a/test/tools/router_check/router.cc +++ b/test/tools/router_check/router.cc @@ -6,6 +6,7 @@ #include #include "common/network/utility.h" +#include "common/protobuf/message_validator_impl.h" #include "common/protobuf/utility.h" #include "common/stream_info/stream_info_impl.h" @@ -74,7 +75,9 @@ RouterCheckTool RouterCheckTool::create(const std::string& router_config_file, auto factory_context = std::make_unique>(); auto config = std::make_unique(route_config, *factory_context, false); if (!disableDeprecationCheck) { - MessageUtil::checkForDeprecation(route_config, &factory_context->runtime_loader_); + MessageUtil::checkForUnexpectedFields(route_config, + ProtobufMessage::getStrictValidationVisitor(), + &factory_context->runtime_loader_); } return RouterCheckTool(std::move(factory_context), std::move(config), std::move(stats),