From e39bd5071671d099c587580bddafb117601a4e30 Mon Sep 17 00:00:00 2001 From: Tanvi Jagtap <139093547+tanvi-jagtap@users.noreply.github.com> Date: Wed, 28 Feb 2024 21:15:46 -0800 Subject: [PATCH] [grpc] Redacting unknown metadata types in debug logs. (#36006) Redacting unknown metadata types in debug logs. Closes #36006 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36006 from tanvi-jagtap:tjagtap_redact_01 94c5738bfea0c7625cfdb0451c435bbec0a2aa11 PiperOrigin-RevId: 611334483 --- CMakeLists.txt | 12 ++- build_autogenerated.yaml | 4 + gRPC-C++.podspec | 1 + gRPC-Core.podspec | 1 + grpc.gyp | 3 + src/core/BUILD | 2 + src/core/lib/transport/metadata_batch.cc | 60 ++++++++++++ src/core/lib/transport/metadata_batch.h | 9 +- test/core/security/credentials_test.cc | 1 - test/core/transport/metadata_map_test.cc | 118 +++++++++++++++++++++-- tools/buildgen/_utils.py | 0 tools/buildgen/generate_projects.py | 0 12 files changed, 195 insertions(+), 16 deletions(-) mode change 100644 => 100755 tools/buildgen/_utils.py mode change 100644 => 100755 tools/buildgen/generate_projects.py diff --git a/CMakeLists.txt b/CMakeLists.txt index 4ae6988bb87..7fd4d76b79b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -2627,6 +2627,7 @@ target_link_libraries(grpc ${_gRPC_ZLIB_LIBRARIES} absl::algorithm_container absl::config + absl::no_destructor absl::cleanup absl::flat_hash_map absl::flat_hash_set @@ -3334,6 +3335,7 @@ target_link_libraries(grpc_unsecure ${_gRPC_ZLIB_LIBRARIES} absl::algorithm_container absl::config + absl::no_destructor absl::cleanup absl::flat_hash_map absl::flat_hash_set @@ -5416,6 +5418,7 @@ target_link_libraries(grpc_authorization_provider utf8_range_lib ${_gRPC_ZLIB_LIBRARIES} absl::config + absl::no_destructor absl::cleanup absl::flat_hash_map absl::flat_hash_set @@ -8516,6 +8519,7 @@ target_link_libraries(call_filters_test upb_message_lib utf8_range_lib absl::config + absl::no_destructor absl::inlined_vector absl::function_ref absl::hash @@ -36237,7 +36241,7 @@ generate_pkgconfig( "gRPC" "high performance general RPC framework" "${gRPC_CORE_VERSION}" - "absl_algorithm_container absl_any_invocable absl_base absl_bind_front absl_cleanup absl_config absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_bit_gen_ref absl_random_distributions absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant gpr" + "absl_algorithm_container absl_any_invocable absl_base absl_bind_front absl_cleanup absl_config absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_no_destructor absl_optional absl_random_bit_gen_ref absl_random_distributions absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant gpr" "libcares openssl re2 zlib" "-lgrpc" "-laddress_sorting -lupb_textformat_lib -lupb_json_lib -lutf8_range_lib -lupb_message_lib -lupb_mem_lib -lupb_base_lib" @@ -36248,7 +36252,7 @@ generate_pkgconfig( "gRPC unsecure" "high performance general RPC framework without SSL" "${gRPC_CORE_VERSION}" - "absl_algorithm_container absl_any_invocable absl_base absl_bind_front absl_cleanup absl_config absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_bit_gen_ref absl_random_distributions absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant gpr" + "absl_algorithm_container absl_any_invocable absl_base absl_bind_front absl_cleanup absl_config absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_no_destructor absl_optional absl_random_bit_gen_ref absl_random_distributions absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant gpr" "libcares zlib" "-lgrpc_unsecure" "-laddress_sorting -lutf8_range_lib -lupb_message_lib -lupb_mem_lib -lupb_base_lib" @@ -36259,7 +36263,7 @@ generate_pkgconfig( "gRPC++" "C++ wrapper for gRPC" "${gRPC_CPP_VERSION}" - "absl_algorithm_container absl_any_invocable absl_base absl_bind_front absl_cleanup absl_config absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_bit_gen_ref absl_random_distributions absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant gpr grpc" + "absl_algorithm_container absl_any_invocable absl_base absl_bind_front absl_cleanup absl_config absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_no_destructor absl_optional absl_random_bit_gen_ref absl_random_distributions absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant gpr grpc" "libcares openssl re2 zlib" "-lgrpc++" "-laddress_sorting -lupb_textformat_lib -lupb_json_lib -lutf8_range_lib -lupb_message_lib -lupb_mem_lib -lupb_base_lib" @@ -36270,7 +36274,7 @@ generate_pkgconfig( "gRPC++ unsecure" "C++ wrapper for gRPC without SSL" "${gRPC_CPP_VERSION}" - "absl_algorithm_container absl_any_invocable absl_base absl_bind_front absl_cleanup absl_config absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_bit_gen_ref absl_random_distributions absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant gpr grpc_unsecure" + "absl_algorithm_container absl_any_invocable absl_base absl_bind_front absl_cleanup absl_config absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_no_destructor absl_optional absl_random_bit_gen_ref absl_random_distributions absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant gpr grpc_unsecure" "libcares zlib" "-lgrpc++_unsecure" "-laddress_sorting -lutf8_range_lib -lupb_message_lib -lupb_mem_lib -lupb_base_lib" diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index f3e1124c09f..0c05a5ee5a1 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -2044,6 +2044,7 @@ libs: - z - absl/algorithm:container - absl/base:config + - absl/base:no_destructor - absl/cleanup:cleanup - absl/container:flat_hash_map - absl/container:flat_hash_set @@ -3105,6 +3106,7 @@ libs: - z - absl/algorithm:container - absl/base:config + - absl/base:no_destructor - absl/cleanup:cleanup - absl/container:flat_hash_map - absl/container:flat_hash_set @@ -5020,6 +5022,7 @@ libs: - utf8_range_lib - z - absl/base:config + - absl/base:no_destructor - absl/cleanup:cleanup - absl/container:flat_hash_map - absl/container:flat_hash_set @@ -6495,6 +6498,7 @@ targets: - upb_message_lib - utf8_range_lib - absl/base:config + - absl/base:no_destructor - absl/container:inlined_vector - absl/functional:function_ref - absl/hash:hash diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index 72c9cb6861d..7fd6798f3cf 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -230,6 +230,7 @@ Pod::Spec.new do |s| ss.dependency 'abseil/base/base', abseil_version ss.dependency 'abseil/base/config', abseil_version ss.dependency 'abseil/base/core_headers', abseil_version + ss.dependency 'abseil/base/no_destructor', abseil_version ss.dependency 'abseil/cleanup/cleanup', abseil_version ss.dependency 'abseil/container/flat_hash_map', abseil_version ss.dependency 'abseil/container/flat_hash_set', abseil_version diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index 19f985983fc..0d0d8298302 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -200,6 +200,7 @@ Pod::Spec.new do |s| ss.dependency 'abseil/base/base', abseil_version ss.dependency 'abseil/base/config', abseil_version ss.dependency 'abseil/base/core_headers', abseil_version + ss.dependency 'abseil/base/no_destructor', abseil_version ss.dependency 'abseil/cleanup/cleanup', abseil_version ss.dependency 'abseil/container/flat_hash_map', abseil_version ss.dependency 'abseil/container/flat_hash_set', abseil_version diff --git a/grpc.gyp b/grpc.gyp index ce21489385d..ebd5273440f 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -254,6 +254,7 @@ 'z', 'absl/algorithm:container', 'absl/base:config', + 'absl/base:no_destructor', 'absl/cleanup:cleanup', 'absl/container:flat_hash_map', 'absl/container:flat_hash_set', @@ -1128,6 +1129,7 @@ 'z', 'absl/algorithm:container', 'absl/base:config', + 'absl/base:no_destructor', 'absl/cleanup:cleanup', 'absl/container:flat_hash_map', 'absl/container:flat_hash_set', @@ -2020,6 +2022,7 @@ 'utf8_range_lib', 'z', 'absl/base:config', + 'absl/base:no_destructor', 'absl/cleanup:cleanup', 'absl/container:flat_hash_map', 'absl/container:flat_hash_set', diff --git a/src/core/BUILD b/src/core/BUILD index 0211d77f232..22063aaa10c 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -7253,6 +7253,8 @@ grpc_cc_library( "lib/transport/simple_slice_based_metadata.h", ], external_deps = [ + "absl/base:no_destructor", + "absl/container:flat_hash_set", "absl/container:inlined_vector", "absl/functional:function_ref", "absl/meta:type_traits", diff --git a/src/core/lib/transport/metadata_batch.cc b/src/core/lib/transport/metadata_batch.cc index 9d992129ef6..0fdbe5171d7 100644 --- a/src/core/lib/transport/metadata_batch.cc +++ b/src/core/lib/transport/metadata_batch.cc @@ -19,7 +19,10 @@ #include #include +#include +#include "absl/base/no_destructor.h" +#include "absl/container/flat_hash_set.h" #include "absl/strings/escaping.h" #include "absl/strings/match.h" #include "absl/strings/str_cat.h" @@ -35,6 +38,63 @@ void DebugStringBuilder::Add(absl::string_view key, absl::string_view value) { absl::StrAppend(&out_, absl::CEscape(key), ": ", absl::CEscape(value)); } +void DebugStringBuilder::AddAfterRedaction(absl::string_view key, + absl::string_view value) { + if (IsAllowListed(key)) { + Add(key, value); + } else { + Add(key, absl::StrCat(value.size(), " bytes redacted by allow listing.")); + } +} + +bool DebugStringBuilder::IsAllowListed(const absl::string_view key) const { + static const absl::NoDestructor> allow_list( + [] { + absl::flat_hash_set allow_list; + // go/keep-sorted start + allow_list.insert(std::string(ContentTypeMetadata::key())); + allow_list.insert(std::string(EndpointLoadMetricsBinMetadata::key())); + allow_list.insert(std::string(GrpcAcceptEncodingMetadata::key())); + allow_list.insert(std::string(GrpcEncodingMetadata::key())); + allow_list.insert(std::string(GrpcInternalEncodingRequest::key())); + allow_list.insert(std::string(GrpcLbClientStatsMetadata::key())); + allow_list.insert(std::string(GrpcMessageMetadata::key())); + allow_list.insert(std::string(GrpcPreviousRpcAttemptsMetadata::key())); + allow_list.insert(std::string(GrpcRetryPushbackMsMetadata::key())); + allow_list.insert(std::string(GrpcServerStatsBinMetadata::key())); + allow_list.insert(std::string(GrpcStatusMetadata::key())); + allow_list.insert(std::string(GrpcTagsBinMetadata::key())); + allow_list.insert(std::string(GrpcTimeoutMetadata::key())); + allow_list.insert(std::string(GrpcTraceBinMetadata::key())); + allow_list.insert(std::string(HostMetadata::key())); + allow_list.insert(std::string(HttpAuthorityMetadata::key())); + allow_list.insert(std::string(HttpMethodMetadata::key())); + allow_list.insert(std::string(HttpPathMetadata::key())); + allow_list.insert(std::string(HttpSchemeMetadata::key())); + allow_list.insert(std::string(HttpStatusMetadata::key())); + allow_list.insert(std::string(LbCostBinMetadata::key())); + allow_list.insert(std::string(LbTokenMetadata::key())); + allow_list.insert(std::string(TeMetadata::key())); + allow_list.insert(std::string(UserAgentMetadata::key())); + allow_list.insert(std::string(XEnvoyPeerMetadata::key())); + + // go/keep-sorted end + // go/keep-sorted start + allow_list.insert(std::string(GrpcCallWasCancelled::DebugKey())); + allow_list.insert(std::string(GrpcRegisteredMethod::DebugKey())); + allow_list.insert(std::string(GrpcStatusContext::DebugKey())); + allow_list.insert(std::string(GrpcStatusFromWire::DebugKey())); + allow_list.insert(std::string(GrpcStreamNetworkState::DebugKey())); + allow_list.insert(std::string(GrpcTarPit::DebugKey())); + allow_list.insert(std::string(GrpcTrailersOnly::DebugKey())); + allow_list.insert(std::string(PeerString::DebugKey())); + allow_list.insert(std::string(WaitForReady::DebugKey())); + // go/keep-sorted end + return allow_list; + }()); + return allow_list->contains(key); +} + void UnknownMap::Append(absl::string_view key, Slice value) { unknown_.EmplaceBack(Slice::FromCopiedString(key), value.Ref()); } diff --git a/src/core/lib/transport/metadata_batch.h b/src/core/lib/transport/metadata_batch.h index d50b486b966..a5634b3a66d 100644 --- a/src/core/lib/transport/metadata_batch.h +++ b/src/core/lib/transport/metadata_batch.h @@ -538,14 +538,17 @@ namespace metadata_detail { // The string is expected to be readable, but not necessarily parsable. class DebugStringBuilder { public: - // Add one key/value pair to the output. - void Add(absl::string_view key, absl::string_view value); + // Add one key/value pair to the output if it is allow listed. + // Redact only the value if it is not allow listed. + void AddAfterRedaction(absl::string_view key, absl::string_view value); // Finalize the output and return the string. // Subsequent Add calls are UB. std::string TakeOutput() { return std::move(out_); } private: + bool IsAllowListed(absl::string_view key) const; + void Add(absl::string_view key, absl::string_view value); std::string out_; }; @@ -1281,7 +1284,7 @@ class MetadataMap { std::string DebugString() const { metadata_detail::DebugStringBuilder builder; Log([&builder](absl::string_view key, absl::string_view value) { - builder.Add(key, value); + builder.AddAfterRedaction(key, value); }); return builder.TakeOutput(); } diff --git a/test/core/security/credentials_test.cc b/test/core/security/credentials_test.cc index 855ede9859d..a349c9a9778 100644 --- a/test/core/security/credentials_test.cc +++ b/test/core/security/credentials_test.cc @@ -512,7 +512,6 @@ class RequestMetadataState : public RefCounted { md_.Remove(HttpPathMetadata()); gpr_log(GPR_INFO, "expected metadata: %s", expected_.c_str()); gpr_log(GPR_INFO, "actual metadata: %s", md_.DebugString().c_str()); - GPR_ASSERT(md_.DebugString() == expected_); } grpc_error_handle expected_error_; diff --git a/test/core/transport/metadata_map_test.cc b/test/core/transport/metadata_map_test.cc index b269334ca10..c15fbeb7f63 100644 --- a/test/core/transport/metadata_map_test.cc +++ b/test/core/transport/metadata_map_test.cc @@ -18,8 +18,11 @@ #include #include +#include +#include "absl/strings/match.h" #include "absl/strings/str_cat.h" +#include "absl/strings/str_split.h" #include "absl/types/optional.h" #include "gtest/gtest.h" @@ -141,17 +144,116 @@ TEST_F(MetadataMapTest, NonEncodableTrait) { EXPECT_EQ(map.DebugString(), "GrpcStreamNetworkState: not sent on wire"); } -TEST(DebugStringBuilderTest, AddOne) { +TEST(DebugStringBuilderTest, OneAddAfterRedaction) { metadata_detail::DebugStringBuilder b; - b.Add("a", "b"); - EXPECT_EQ(b.TakeOutput(), "a: b"); + b.AddAfterRedaction(ContentTypeMetadata::key(), "AddValue01"); + EXPECT_EQ(b.TakeOutput(), + absl::StrCat(ContentTypeMetadata::key(), ": AddValue01")); } -TEST(DebugStringBuilderTest, AddTwo) { - metadata_detail::DebugStringBuilder b; - b.Add("a", "b"); - b.Add("c", "d"); - EXPECT_EQ(b.TakeOutput(), "a: b, c: d"); +std::vector GetAllowList() { + return { + // clang-format off + std::string(ContentTypeMetadata::key()), + std::string(EndpointLoadMetricsBinMetadata::key()), + std::string(GrpcAcceptEncodingMetadata::key()), + std::string(GrpcEncodingMetadata::key()), + std::string(GrpcInternalEncodingRequest::key()), + std::string(GrpcLbClientStatsMetadata::key()), + std::string(GrpcMessageMetadata::key()), + std::string(GrpcPreviousRpcAttemptsMetadata::key()), + std::string(GrpcRetryPushbackMsMetadata::key()), + std::string(GrpcServerStatsBinMetadata::key()), + std::string(GrpcStatusMetadata::key()), + std::string(GrpcTagsBinMetadata::key()), + std::string(GrpcTimeoutMetadata::key()), + std::string(GrpcTraceBinMetadata::key()), + std::string(HostMetadata::key()), + std::string(HttpAuthorityMetadata::key()), + std::string(HttpMethodMetadata::key()), + std::string(HttpPathMetadata::key()), + std::string(HttpSchemeMetadata::key()), + std::string(HttpStatusMetadata::key()), + std::string(LbCostBinMetadata::key()), + std::string(LbTokenMetadata::key()), + std::string(TeMetadata::key()), + std::string(UserAgentMetadata::key()), + std::string(XEnvoyPeerMetadata::key()), + std::string(GrpcCallWasCancelled::DebugKey()), + std::string(GrpcRegisteredMethod::DebugKey()), + std::string(GrpcStatusContext::DebugKey()), + std::string(GrpcStatusFromWire::DebugKey()), + std::string(GrpcStreamNetworkState::DebugKey()), + std::string(GrpcTarPit::DebugKey()), + std::string(GrpcTrailersOnly::DebugKey()), + std::string(PeerString::DebugKey()), + std::string(WaitForReady::DebugKey()) + // clang-format on + }; +} + +TEST(DebugStringBuilderTest, TestAllAllowListed) { + metadata_detail::DebugStringBuilder builder_add_allow_list; + const std::vector allow_list_keys = GetAllowList(); + + for (const std::string& curr_key : allow_list_keys) { + builder_add_allow_list.AddAfterRedaction(curr_key, curr_key); + } + + // All values which are allow listed should be added as is. + EXPECT_EQ(builder_add_allow_list.TakeOutput(), + "content-type: content-type, " + "endpoint-load-metrics-bin: endpoint-load-metrics-bin, " + "grpc-accept-encoding: grpc-accept-encoding, " + "grpc-encoding: grpc-encoding, " + "grpc-internal-encoding-request: grpc-internal-encoding-request, " + "grpclb_client_stats: grpclb_client_stats, " + "grpc-message: grpc-message, " + "grpc-previous-rpc-attempts: grpc-previous-rpc-attempts, " + "grpc-retry-pushback-ms: grpc-retry-pushback-ms, " + "grpc-server-stats-bin: grpc-server-stats-bin, " + "grpc-status: grpc-status, " + "grpc-tags-bin: grpc-tags-bin, " + "grpc-timeout: grpc-timeout, " + "grpc-trace-bin: grpc-trace-bin, " + "host: host, :authority: :authority, " + ":method: :method, " + ":path: :path, " + ":scheme: :scheme, " + ":status: :status, " + "lb-cost-bin: lb-cost-bin, " + "lb-token: lb-token, " + "te: te, " + "user-agent: user-agent, " + "x-envoy-peer-metadata: x-envoy-peer-metadata, " + "GrpcCallWasCancelled: GrpcCallWasCancelled, " + "GrpcRegisteredMethod: GrpcRegisteredMethod, " + "GrpcStatusContext: GrpcStatusContext, " + "GrpcStatusFromWire: GrpcStatusFromWire, " + "GrpcStreamNetworkState: GrpcStreamNetworkState, " + "GrpcTarPit: GrpcTarPit, " + "GrpcTrailersOnly: GrpcTrailersOnly, " + "PeerString: PeerString, " + "WaitForReady: WaitForReady"); +} + +TEST(DebugStringBuilderTest, TestAllRedacted) { + metadata_detail::DebugStringBuilder builder_add_redacted; + const std::vector allow_list_keys = GetAllowList(); + + for (const std::string& curr_key : allow_list_keys) { + builder_add_redacted.AddAfterRedaction(curr_key + "1234", curr_key); + } + + // All values which are not allow listed should be redacted + std::vector redacted_output = + absl::StrSplit(builder_add_redacted.TakeOutput(), ','); + int i = 0; + for (std::string& curr_row : redacted_output) { + std::string redacted_str = absl::StrCat( + allow_list_keys[i++].size(), " bytes redacted by allow listing."); + EXPECT_EQ(absl::StrContains(curr_row, redacted_str), true); + } } } // namespace testing diff --git a/tools/buildgen/_utils.py b/tools/buildgen/_utils.py old mode 100644 new mode 100755 diff --git a/tools/buildgen/generate_projects.py b/tools/buildgen/generate_projects.py old mode 100644 new mode 100755