diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 50a95b419974..30f9b3cec5e3 100644 --- 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 index a05587338e26..745fb93d76d7 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -191,20 +191,14 @@ class MessageUtil { using FileExtensions = ConstSingleton; - 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 checkUnknownFields(const Protobuf::Message& message, ProtobufMessage::ValidationVisitor& validation_visitor) { diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 04fb200fc075..319ba5b91943 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -3,6 +3,8 @@ #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" #include "common/runtime/runtime_impl.h" @@ -114,6 +116,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 repeated; EXPECT_EQ("[]", RepeatedPtrUtil::debugString(repeated)); diff --git a/test/common/secret/BUILD b/test/common/secret/BUILD index 19712797f54a..a2e85abcef8f 100644 --- 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 index b3a6105fb11f..788254d124ac 100644 --- 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 secret_manager(new SecretManagerImpl(config_tracker_)); + + NiceMock secret_context; + + NiceMock local_info; + NiceMock dispatcher; + NiceMock random; + Stats::IsolatedStoreImpl stats; + NiceMock init_manager; + NiceMock 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 secret_manager(new SecretManagerImpl(config_tracker_)); diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt index 3f935e236cef..edeac04258da 100644 --- a/tools/spelling_dictionary.txt +++ b/tools/spelling_dictionary.txt @@ -414,6 +414,7 @@ decrypting dedup dedupe deduplicate +deduplicates deflateInit deletable deleter