[grpc/api] Avoiding API mis-use / illegal header keys in grpc configuration #14128
Description
Avoiding illegal header keys in grpc configuration:
A grpc config contains initial metadata which can eventually be evaluated by a header parser here and then used to start a grpc stream.
However, grpc source code uses an ASSERT on an API error so that if you configure Envoy with initial metadata that contains illegal characters, or somehow the header parser allows illegal characters that grpc doesn't allow, then you end up triggering a (production) failure in grpc (https://github.com/grpc/grpc/blob/1bf2ab3ab244a7fc7df7fe1d05146842ec61da76/include/grpcpp/impl/codegen/call_op_set.h#L974, https://github.com/grpc/grpc/blob/1e08bc22244ed0788c2e0bc3aedd1158b8b23085/src/core/lib/surface/validate_metadata.cc#L61)
Test reproducer:
+TEST_F(EnvoyGoogleLessMockedAsyncClientImplTest, TestInvalidHeader) {
+ auto* google_grpc = config_.mutable_google_grpc();
+ google_grpc->set_target_uri("48?");
+ google_grpc->set_stat_prefix("$");
+
+ auto& metadata = *config_.mutable_initial_metadata()->Add();
+ metadata.set_key("illegalcolon;");
+ metadata.set_value("value");
+
+ initialize();
+
+ NiceMock<MockAsyncStreamCallbacks<helloworld::HelloReply>> grpc_callbacks;
+ AsyncStream<helloworld::HelloRequest> grpc_stream =
+ grpc_client_->start(*method_descriptor_, grpc_callbacks, Http::AsyncClient::RequestOptions());
+}
Question: where is the best place to prevent this from triggering?
(1) If we add this to an API restriction, it would impact all HeaderValues, and be against API. :/
(2) Check metadata for compliance here
(3) Check and throw if you try to create a grpc client with a bad config . Is it possible to have good metadata key/values here but then evaluate them and get illegal ones? I don't think so? I don't think there should be bad vals in stream info strings
(4) Give header parser a strict mode to ignore illegal header keys. Seems too circuitous
Stack trace:
E1120 15:09:19.873943934 1 call.cc:906] validate_metadata: {"created":"@1605884959.873852746","description":"Illegal header key","file":"external/com_github_grpc_grpc/src/core/lib/surface/validate_metadata.cc","file_line":44,"offset":1,"raw_bytes":"32 3b '2;'\u0000"}
--
| E1120 15:09:19.874028070 1 call_op_set.h:947] assertion failed: false
#2 0xabe0ddb in grpc::CoreCodegen::assert_fail(char const*, char const*, int) /proc/self/cwd/external/com_github_grpc_grpc/src/cpp/common/core_codegen.cc:237:3
--
| #3 0xa846dfe in grpc::internal::CallOpSet<grpc::internal::CallOpSendInitialMetadata, grpc::internal::CallOpSendMessage, grpc::internal::CallOpClientSendClose, grpc::internal::CallNoOp<4>, grpc::internal::CallNoOp<5>, grpc::internal::CallNoOp<6> >::ContinueFillOpsAfterInterception() /proc/self/cwd/external/com_github_grpc_grpc/include/grpcpp/impl/codegen/call_op_set.h:947:7
| #4 0xa8469c5 in grpc::internal::CallOpSet<grpc::internal::CallOpSendInitialMetadata, grpc::internal::CallOpSendMessage, grpc::internal::CallOpClientSendClose, grpc::internal::CallNoOp<4>, grpc::internal::CallNoOp<5>, grpc::internal::CallNoOp<6> >::FillOps(grpc::internal::Call*) /proc/self/cwd/external/com_github_grpc_grpc/include/grpcpp/impl/codegen/call_op_set.h:868:7
| #5 0xabb7521 in grpc_impl::Channel::PerformOpsOnCall(grpc::internal::CallOpSetInterface*, grpc::internal::Call*) /proc/self/cwd/external/com_github_grpc_grpc/src/cpp/client/channel_cc.cc:164:8
| #6 0xa84c8f9 in grpc::internal::Call::PerformOps(grpc::internal::CallOpSetInterface*) /proc/self/cwd/external/com_github_grpc_grpc/include/grpcpp/impl/codegen/call.h:69:17
| #7 0xa85ce6f in grpc_impl::ClientAsyncReaderWriter<grpc::ByteBuffer, grpc::ByteBuffer>::StartCallInternal(void*) /proc/self/cwd/external/com_github_grpc_grpc/include/grpcpp/impl/codegen/async_stream_impl.h:621:13
| #8 0xa82380d in grpc_impl::ClientAsyncReaderWriter<grpc::ByteBuffer, grpc::ByteBuffer>::StartCall(void*) /proc/self/cwd/external/com_github_grpc_grpc/include/grpcpp/impl/codegen/async_stream_impl.h:531:5
| #9 0xa821c0e in Envoy::Grpc::GoogleAsyncStreamImpl::initialize(bool) /proc/self/cwd/source/common/grpc/google_async_client_impl.cc:196:8
| #10 0xa81fe80 in Envoy::Grpc::GoogleAsyncClientImpl::startRaw(std::__1::basic_string_view<char, std::__1::char_traits<char> >, std::__1::basic_string_view<char, std::__1::char_traits<char> >, Envoy::Grpc::RawAsyncStreamCallbacks&, Envoy::Http::AsyncClient::StreamOptions const&) /proc/self/cwd/source/common/grpc/google_async_client_impl.cc:143:16