From ce0ab182e3ce498d97b4882900f9e90bee272d78 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Fri, 20 Nov 2020 16:32:51 -0500 Subject: [PATCH 1/4] fix grpc illegal chars Signed-off-by: Asra Ali --- .../common/grpc/async_client_manager_impl.cc | 22 +++++++ .../grpc/async_client_manager_impl_test.cc | 20 ++++++ .../server_corpus/grpc_illegal_characters | 64 +++++++++++++++++++ 3 files changed, 106 insertions(+) create mode 100644 test/server/server_corpus/grpc_illegal_characters diff --git a/source/common/grpc/async_client_manager_impl.cc b/source/common/grpc/async_client_manager_impl.cc index 5f809755f89d1..981f724c5644b 100644 --- a/source/common/grpc/async_client_manager_impl.cc +++ b/source/common/grpc/async_client_manager_impl.cc @@ -11,6 +11,20 @@ namespace Envoy { namespace Grpc { +namespace { + +// Validates a string for gRPC header key compliance. This is a subset of legal HTTP characters. +// See https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md +bool validateGrpcHeaderChars(absl::string_view key) { + for (auto ch : key) { + if (!absl::ascii_isalnum(ch) || ch == '_' || ch == '.' || ch == '-') { + return false; + } + } + return true; +} + +} // namespace AsyncClientFactoryImpl::AsyncClientFactoryImpl(Upstream::ClusterManager& cm, const envoy::config::core::v3::GrpcService& config, @@ -66,6 +80,14 @@ GoogleAsyncClientFactoryImpl::GoogleAsyncClientFactoryImpl( #else ASSERT(google_tls_slot_ != nullptr); #endif + + // Check metadata for grpc API compliance. Uppercases are lowered in the HeaderParser. + // https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md + for (const auto& header : config.initial_metadata()) { + if (!validateGrpcHeaderChars(header.key()) || !validateGrpcHeaderChars(header.value())) { + throw EnvoyException("Illegal characters in gRPC initial metadata."); + } + } } RawAsyncClientPtr GoogleAsyncClientFactoryImpl::create() { diff --git a/test/common/grpc/async_client_manager_impl_test.cc b/test/common/grpc/async_client_manager_impl_test.cc index 448f52aa6140b..da2050a6bfd94 100644 --- a/test/common/grpc/async_client_manager_impl_test.cc +++ b/test/common/grpc/async_client_manager_impl_test.cc @@ -88,6 +88,26 @@ TEST_F(AsyncClientManagerImplTest, GoogleGrpc) { #endif } +TEST_F(AsyncClientManagerImplTest, GoogleGrpcIllegalChars) { + EXPECT_CALL(scope_, createScope_("grpc.foo.")); + envoy::config::core::v3::GrpcService grpc_service; + grpc_service.mutable_google_grpc()->set_stat_prefix("foo"); + + auto& metadata = *grpc_service.mutable_initial_metadata()->Add(); + metadata.set_key("illegalcharacter;"); + metadata.set_value("value"); + +#ifdef ENVOY_GOOGLE_GRPC + EXPECT_THROW_WITH_MESSAGE( + async_client_manager_.factoryForGrpcService(grpc_service, scope_, false), EnvoyException, + "Illegal characters in gRPC initial metadata."); +#else + EXPECT_THROW_WITH_MESSAGE( + async_client_manager_.factoryForGrpcService(grpc_service, scope_, false), EnvoyException, + "Google C++ gRPC client is not linked"); +#endif +} + TEST_F(AsyncClientManagerImplTest, EnvoyGrpcUnknownOk) { envoy::config::core::v3::GrpcService grpc_service; grpc_service.mutable_envoy_grpc()->set_cluster_name("foo"); diff --git a/test/server/server_corpus/grpc_illegal_characters b/test/server/server_corpus/grpc_illegal_characters new file mode 100644 index 0000000000000..8309bb56e7d7e --- /dev/null +++ b/test/server/server_corpus/grpc_illegal_characters @@ -0,0 +1,64 @@ +cluster_manager { + load_stats_config { + api_type: GRPC + grpc_services { + google_grpc { + target_uri: "48?" + stat_prefix: "$" + } + initial_metadata { + key: "2" + value: "$$" + } + initial_metadata { + key: "2" + value: "2" + } + initial_metadata { + key: "2" + value: "$$" + } + initial_metadata { + key: "2" + value: "$$" + } + initial_metadata { + key: "2;" + value: "$$" + } + initial_metadata { + key: "1" + value: "$$" + } + initial_metadata { + key: "2" + value: "$$" + } + initial_metadata { + key: "2" + value: "$$" + } + initial_metadata { + key: "2" + value: "2" + } + initial_metadata { + key: "2" + value: "$$" + } + initial_metadata { + key: "4" + value: "$$" + } + initial_metadata { + key: "2" + value: "1" + } + initial_metadata { + key: "10" + value: "$$" + } + } + } +} +enable_dispatcher_stats: true \ No newline at end of file From 842cbdbb637d8a99789c434c80e48b00be81b422 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Fri, 20 Nov 2020 16:41:22 -0500 Subject: [PATCH 2/4] ninja Signed-off-by: Asra Ali --- .../common/grpc/async_client_manager_impl.cc | 2 +- .../grpc/async_client_manager_impl_test.cc | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/source/common/grpc/async_client_manager_impl.cc b/source/common/grpc/async_client_manager_impl.cc index 981f724c5644b..953241967b935 100644 --- a/source/common/grpc/async_client_manager_impl.cc +++ b/source/common/grpc/async_client_manager_impl.cc @@ -17,7 +17,7 @@ namespace { // See https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md bool validateGrpcHeaderChars(absl::string_view key) { for (auto ch : key) { - if (!absl::ascii_isalnum(ch) || ch == '_' || ch == '.' || ch == '-') { + if (!(absl::ascii_isalnum(ch) || ch == '_' || ch == '.' || ch == '-')) { return false; } } diff --git a/test/common/grpc/async_client_manager_impl_test.cc b/test/common/grpc/async_client_manager_impl_test.cc index da2050a6bfd94..76c7c869a3b8e 100644 --- a/test/common/grpc/async_client_manager_impl_test.cc +++ b/test/common/grpc/async_client_manager_impl_test.cc @@ -108,6 +108,24 @@ TEST_F(AsyncClientManagerImplTest, GoogleGrpcIllegalChars) { #endif } +TEST_F(AsyncClientManagerImplTest, LegalGoogleGrpcChar) { + EXPECT_CALL(scope_, createScope_("grpc.foo.")); + envoy::config::core::v3::GrpcService grpc_service; + grpc_service.mutable_google_grpc()->set_stat_prefix("foo"); + + auto& metadata = *grpc_service.mutable_initial_metadata()->Add(); + metadata.set_key("_legal-character."); + metadata.set_value("value"); + +#ifdef ENVOY_GOOGLE_GRPC + EXPECT_NE(nullptr, async_client_manager_.factoryForGrpcService(grpc_service, scope_, false)); +#else + EXPECT_THROW_WITH_MESSAGE( + async_client_manager_.factoryForGrpcService(grpc_service, scope_, false), EnvoyException, + "Google C++ gRPC client is not linked"); +#endif +} + TEST_F(AsyncClientManagerImplTest, EnvoyGrpcUnknownOk) { envoy::config::core::v3::GrpcService grpc_service; grpc_service.mutable_envoy_grpc()->set_cluster_name("foo"); From 8c59fb10c74d392207a2561b30c72c0945b0523f Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Mon, 23 Nov 2020 09:30:43 -0500 Subject: [PATCH 3/4] fix spelling Signed-off-by: Asra Ali --- source/common/grpc/async_client_manager_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/grpc/async_client_manager_impl.cc b/source/common/grpc/async_client_manager_impl.cc index 953241967b935..25749ce84bb5f 100644 --- a/source/common/grpc/async_client_manager_impl.cc +++ b/source/common/grpc/async_client_manager_impl.cc @@ -81,7 +81,7 @@ GoogleAsyncClientFactoryImpl::GoogleAsyncClientFactoryImpl( ASSERT(google_tls_slot_ != nullptr); #endif - // Check metadata for grpc API compliance. Uppercases are lowered in the HeaderParser. + // Check metadata for gRPC API compliance. Uppercased characters are lowered in the HeaderParser. // https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md for (const auto& header : config.initial_metadata()) { if (!validateGrpcHeaderChars(header.key()) || !validateGrpcHeaderChars(header.value())) { From 71dae82e07e2f3172fae1b21369ac5e8aa990d10 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Mon, 23 Nov 2020 11:37:19 -0500 Subject: [PATCH 4/4] fix Signed-off-by: Asra Ali --- source/common/grpc/async_client_manager_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/grpc/async_client_manager_impl.cc b/source/common/grpc/async_client_manager_impl.cc index 25749ce84bb5f..14866aa25d0ef 100644 --- a/source/common/grpc/async_client_manager_impl.cc +++ b/source/common/grpc/async_client_manager_impl.cc @@ -81,7 +81,7 @@ GoogleAsyncClientFactoryImpl::GoogleAsyncClientFactoryImpl( ASSERT(google_tls_slot_ != nullptr); #endif - // Check metadata for gRPC API compliance. Uppercased characters are lowered in the HeaderParser. + // Check metadata for gRPC API compliance. Uppercase characters are lowered in the HeaderParser. // https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md for (const auto& header : config.initial_metadata()) { if (!validateGrpcHeaderChars(header.key()) || !validateGrpcHeaderChars(header.value())) {