protobuf: recursively validate unknown fields. (#8094)

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 #8092. Risk level: Low Testing: New nested unknown field test added. Fixes #7980 Signed-off-by: Harvey Tuch <[email protected]>
envoyproxy · Sep 2, 2019 · dad0f2e · dad0f2e
1 parent 678bf8c
commit dad0f2e
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 43 deletions.
diff --git 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
@@ -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)) {

diff --git a/test/common/protobuf/utility_test.cc b/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> {};

diff --git a/test/tools/router_check/router.cc b/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),