protobuf: recursively validate unknown fields.

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);
  }
  }
  }

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 <class MessageType>
  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)) {

test/common/protobuf/utility_test.cc

@@ -1,8 +1,10 @@
 #include <unordered_set>
 
+#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<ProtobufMessage::MockValidationVisitor> 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,15 +578,15 @@ 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.
  Runtime::LoaderSingleton::getExisting()->mergeValues(
  {{"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,23 +612,22 @@ 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());
 }
 
 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<int64_t> {};

test/tools/router_check/router.cc

@@ -6,6 +6,7 @@
 #include <unordered_map>
 
 #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<NiceMock<Server::Configuration::MockFactoryContext>>();
  auto config = std::make_unique<Router::ConfigImpl>(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),