proto: re-implement MessageUtil::hash function to consistently hash A…

…ny recursively (envoyproxy#8231) Use TextFormat::Print with SetExpandAny(true) instead of SerializeAsString to calculate hash with Any expanded. Risk Level: Med Testing: CI, regression test Fixes envoyproxy#5744. Signed-off-by: Lizan Zhou <lizan@tetrate.io>
danzh2010 · Sep 24, 2019 · 0334a27 · 0334a27
1 parent 7689ab4
commit 0334a27
Show file tree

Hide file tree

Showing 6 changed files with 131 additions and 14 deletions.
diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc
@@ -88,6 +88,20 @@ ProtoValidationException::ProtoValidationException(const std::string& validation
   ENVOY_LOG_MISC(debug, "Proto validation error; throwing {}", what());
 }
 
+size_t MessageUtil::hash(const Protobuf::Message& message) {
+  std::string text_format;
+
+  {
+    Protobuf::TextFormat::Printer printer;
+    printer.SetExpandAny(true);
+    printer.SetUseFieldNumber(true);
+    printer.SetSingleLineMode(true);
+    printer.PrintToString(message, &text_format);
+  }
+
+  return HashUtil::xxHash64(text_format);
+}
+
 void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& message,
                                ProtobufMessage::ValidationVisitor& validation_visitor) {
   Protobuf::util::JsonParseOptions options;

diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h
@@ -200,20 +200,14 @@ class MessageUtil {
 
   using FileExtensions = ConstSingleton<FileExtensionValues>;
 
-  static std::size_t hash(const Protobuf::Message& message) {
-    // Use Protobuf::io::CodedOutputStream to force deterministic serialization, so that the same
-    // message doesn't hash to different values.
-    std::string text;
-    {
-      // For memory safety, the StringOutputStream needs to be destroyed before
-      // we read the string.
-      Protobuf::io::StringOutputStream string_stream(&text);
-      Protobuf::io::CodedOutputStream coded_stream(&string_stream);
-      coded_stream.SetSerializationDeterministic(true);
-      message.SerializeToCodedStream(&coded_stream);
-    }
-    return HashUtil::xxHash64(text);
-  }
+  /**
+   * A hash function uses Protobuf::TextFormat to force deterministic serialization recursively
+   * including known types in google.protobuf.Any. See
+   * https://github.com/protocolbuffers/protobuf/issues/5731 for the context.
+   * Using this function is discouraged, see discussion in
+   * https://github.com/envoyproxy/envoy/issues/8301.
+   */
+  static std::size_t hash(const Protobuf::Message& message);
 
   static void loadFromJson(const std::string& json, Protobuf::Message& message,
                            ProtobufMessage::ValidationVisitor& validation_visitor);

diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc
@@ -4,6 +4,7 @@
 #include "envoy/config/bootstrap/v2/bootstrap.pb.h"
 #include "envoy/config/bootstrap/v2/bootstrap.pb.validate.h"
 
+#include "common/common/base64.h"
 #include "common/protobuf/message_validator_impl.h"
 #include "common/protobuf/protobuf.h"
 #include "common/protobuf/utility.h"
@@ -118,6 +119,26 @@ TEST_F(ProtobufUtilityTest, evaluateFractionalPercent) {
 
 } // namespace ProtobufPercentHelper
 
+TEST_F(ProtobufUtilityTest, MessageUtilHash) {
+  ProtobufWkt::Struct s;
+  (*s.mutable_fields())["ab"].set_string_value("fgh");
+  (*s.mutable_fields())["cde"].set_string_value("ij");
+
+  ProtobufWkt::Any a1;
+  a1.PackFrom(s);
+  // The two base64 encoded Struct to test map is identical to the struct above, this tests whether
+  // a map is deterministically serialized and hashed.
+  ProtobufWkt::Any a2 = a1;
+  a2.set_value(Base64::decode("CgsKA2NkZRIEGgJpagoLCgJhYhIFGgNmZ2g="));
+  ProtobufWkt::Any a3 = a1;
+  a3.set_value(Base64::decode("CgsKAmFiEgUaA2ZnaAoLCgNjZGUSBBoCaWo="));
+
+  EXPECT_EQ(MessageUtil::hash(a1), MessageUtil::hash(a2));
+  EXPECT_EQ(MessageUtil::hash(a2), MessageUtil::hash(a3));
+  EXPECT_NE(0, MessageUtil::hash(a1));
+  EXPECT_NE(MessageUtil::hash(s), MessageUtil::hash(a1));
+}
+
 TEST_F(ProtobufUtilityTest, RepeatedPtrUtilDebugString) {
   Protobuf::RepeatedPtrField<ProtobufWkt::UInt32Value> repeated;
   EXPECT_EQ("[]", RepeatedPtrUtil::debugString(repeated));

diff --git a/test/common/secret/BUILD b/test/common/secret/BUILD
@@ -24,6 +24,7 @@ envoy_cc_test(
         "//test/test_common:registry_lib",
         "//test/test_common:simulated_time_system_lib",
         "//test/test_common:utility_lib",
+        "@envoy_api//envoy/config/grpc_credential/v2alpha:file_based_metadata_cc",
     ],
 )
 

diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc
@@ -3,7 +3,9 @@
 #include "envoy/admin/v2alpha/config_dump.pb.h"
 #include "envoy/api/v2/auth/cert.pb.h"
 #include "envoy/common/exception.h"
+#include "envoy/config/grpc_credential/v2alpha/file_based_metadata.pb.h"
 
+#include "common/common/base64.h"
 #include "common/common/logger.h"
 #include "common/secret/sds_api.h"
 #include "common/secret/secret_manager_impl.h"
@@ -165,6 +167,90 @@ name: "abc.com"
                             "Secret type not implemented");
 }
 
+// Validate that secret manager deduplicates dynamic TLS certificate secret provider.
+// Regression test of https://github.com/envoyproxy/envoy/issues/5744
+TEST_F(SecretManagerImplTest, DeduplicateDynamicTlsCertificateSecretProvider) {
+  Server::MockInstance server;
+  std::unique_ptr<SecretManager> secret_manager(new SecretManagerImpl(config_tracker_));
+
+  NiceMock<Server::Configuration::MockTransportSocketFactoryContext> secret_context;
+
+  NiceMock<LocalInfo::MockLocalInfo> local_info;
+  NiceMock<Event::MockDispatcher> dispatcher;
+  NiceMock<Runtime::MockRandomGenerator> random;
+  Stats::IsolatedStoreImpl stats;
+  NiceMock<Init::MockManager> init_manager;
+  NiceMock<Init::ExpectableWatcherImpl> init_watcher;
+  Init::TargetHandlePtr init_target_handle;
+  EXPECT_CALL(init_manager, add(_))
+      .WillRepeatedly(Invoke([&init_target_handle](const Init::Target& target) {
+        init_target_handle = target.createHandle("test");
+      }));
+  EXPECT_CALL(secret_context, stats()).WillRepeatedly(ReturnRef(stats));
+  EXPECT_CALL(secret_context, initManager()).WillRepeatedly(Return(&init_manager));
+  EXPECT_CALL(secret_context, dispatcher()).WillRepeatedly(ReturnRef(dispatcher));
+  EXPECT_CALL(secret_context, localInfo()).WillRepeatedly(ReturnRef(local_info));
+
+  envoy::api::v2::core::ConfigSource config_source;
+  TestUtility::loadFromYaml(R"(
+api_config_source:
+  api_type: GRPC
+  grpc_services:
+  - google_grpc:
+      call_credentials:
+      - from_plugin:
+          name: envoy.grpc_credentials.file_based_metadata
+          typed_config:
+            "@type": type.googleapis.com/envoy.config.grpc_credential.v2alpha.FileBasedMetadataConfig
+      stat_prefix: sdsstat
+      credentials_factory_name: envoy.grpc_credentials.file_based_metadata
+  )",
+                            config_source);
+  config_source.mutable_api_config_source()
+      ->mutable_grpc_services(0)
+      ->mutable_google_grpc()
+      ->mutable_call_credentials(0)
+      ->mutable_from_plugin()
+      ->mutable_typed_config()
+      ->set_value(Base64::decode("CjUKMy92YXIvcnVuL3NlY3JldHMva3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3Vud"
+                                 "C90b2tlbhILeC10b2tlbi1iaW4="));
+  auto secret_provider1 =
+      secret_manager->findOrCreateTlsCertificateProvider(config_source, "abc.com", secret_context);
+
+  // The base64 encoded proto binary is identical to the one above, but in different field order.
+  // It is also identical to the YAML below.
+  config_source.mutable_api_config_source()
+      ->mutable_grpc_services(0)
+      ->mutable_google_grpc()
+      ->mutable_call_credentials(0)
+      ->mutable_from_plugin()
+      ->mutable_typed_config()
+      ->set_value(Base64::decode("Egt4LXRva2VuLWJpbgo1CjMvdmFyL3J1bi9zZWNyZXRzL2t1YmVybmV0ZXMuaW8vc"
+                                 "2VydmljZWFjY291bnQvdG9rZW4="));
+  auto secret_provider2 =
+      secret_manager->findOrCreateTlsCertificateProvider(config_source, "abc.com", secret_context);
+
+  envoy::config::grpc_credential::v2alpha::FileBasedMetadataConfig file_based_metadata_config;
+  TestUtility::loadFromYaml(R"(
+header_key: x-token-bin
+secret_data:
+  filename: "/var/run/secrets/kubernetes.io/serviceaccount/token"
+  )",
+                            file_based_metadata_config);
+  config_source.mutable_api_config_source()
+      ->mutable_grpc_services(0)
+      ->mutable_google_grpc()
+      ->mutable_call_credentials(0)
+      ->mutable_from_plugin()
+      ->mutable_typed_config()
+      ->PackFrom(file_based_metadata_config);
+  auto secret_provider3 =
+      secret_manager->findOrCreateTlsCertificateProvider(config_source, "abc.com", secret_context);
+
+  EXPECT_EQ(secret_provider1, secret_provider2);
+  EXPECT_EQ(secret_provider2, secret_provider3);
+}
+
 TEST_F(SecretManagerImplTest, SdsDynamicSecretUpdateSuccess) {
   Server::MockInstance server;
   std::unique_ptr<SecretManager> secret_manager(new SecretManagerImpl(config_tracker_));

diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt
@@ -427,6 +427,7 @@ decls
 dedup
 dedupe
 deduplicate
+deduplicates
 deflateInit
 deletable
 deleter