Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing GRPC initial metadata validation #16414

Merged
merged 3 commits into from
May 20, 2021
Merged

Fixing GRPC initial metadata validation #16414

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1486058
Fixing GRPC initial metadata validation
May 18, 2021
999a495
Merge remote-tracking branch 'github/main'
May 19, 2021
a06b664
Merge remote-tracking branch 'github/main'
May 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions source/common/grpc/async_client_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@
#include "envoy/config/core/v3/grpc_service.pb.h"
#include "envoy/stats/scope.h"

#include "common/common/base64.h"
#include "common/grpc/async_client_impl.h"

#include "absl/strings/match.h"

#ifdef ENVOY_GOOGLE_GRPC
#include "common/grpc/google_async_client_impl.h"
#endif
Expand All @@ -24,6 +27,15 @@ bool validateGrpcHeaderChars(absl::string_view key) {
return true;
}

bool validateGrpcCompatibleAsciiHeaderValue(absl::string_view h_value) {
for (auto ch : h_value) {
if (ch < 0x20 || ch > 0x7e) {
return false;
}
}
return true;
}

} // namespace

AsyncClientFactoryImpl::AsyncClientFactoryImpl(Upstream::ClusterManager& cm,
Expand Down Expand Up @@ -84,8 +96,18 @@ GoogleAsyncClientFactoryImpl::GoogleAsyncClientFactoryImpl(
// 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())) {
throw EnvoyException("Illegal characters in gRPC initial metadata.");
// Validate key
if (!validateGrpcHeaderChars(header.key())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm reading the spec right, header names can be lower-case ascii chars, numericals, _, -, and .. validateGrpcHeaderChars will return true if the parameter is an upper-case char (try running python -c 'print("A".isalnum())'. Abseil's kPropertyBits is generated using python script, according to comments in ascii.c. kPropertyBits is then used to verify if a character is an alpha-numeric one),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'm not sure - but according to this: https://github.com/grpc/grpc-go/blob/master/Documentation/grpc-metadata.md#creating-a-new-metadata it sounds like you can define initial metadata to be both lower and upercase but it will be converted to lowercase. If you think it's better to have a stricter check I can do so but I'm not sure it's the correct intent in this case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, HTTP/2 always sends header names as lower-case on the wire:

https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2

throw EnvoyException(
fmt::format("Illegal characters in gRPC initial metadata header key: {}.", header.key()));
}

// Validate value
// Binary base64 encoded - handled by the GRPC library
if (!::absl::EndsWith(header.key(), "-bin") &&
!validateGrpcCompatibleAsciiHeaderValue(header.value())) {
throw EnvoyException(fmt::format(
"Illegal ASCII value for gRPC initial metadata header key: {}.", header.key()));
}
}
}
Expand Down
24 changes: 22 additions & 2 deletions test/common/grpc/async_client_manager_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ TEST_F(AsyncClientManagerImplTest, GoogleGrpc) {
#endif
}

TEST_F(AsyncClientManagerImplTest, GoogleGrpcIllegalChars) {
TEST_F(AsyncClientManagerImplTest, GoogleGrpcIllegalCharsInKey) {
EXPECT_CALL(scope_, createScope_("grpc.foo."));
envoy::config::core::v3::GrpcService grpc_service;
grpc_service.mutable_google_grpc()->set_stat_prefix("foo");
Expand All @@ -100,7 +100,7 @@ TEST_F(AsyncClientManagerImplTest, GoogleGrpcIllegalChars) {
#ifdef ENVOY_GOOGLE_GRPC
EXPECT_THROW_WITH_MESSAGE(
async_client_manager_.factoryForGrpcService(grpc_service, scope_, false), EnvoyException,
"Illegal characters in gRPC initial metadata.");
"Illegal characters in gRPC initial metadata header key: illegalcharacter;.");
#else
EXPECT_THROW_WITH_MESSAGE(
async_client_manager_.factoryForGrpcService(grpc_service, scope_, false), EnvoyException,
Expand All @@ -126,6 +126,26 @@ TEST_F(AsyncClientManagerImplTest, LegalGoogleGrpcChar) {
#endif
}

TEST_F(AsyncClientManagerImplTest, GoogleGrpcIllegalCharsInValue) {
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-key");
metadata.set_value("NonAsciValue.भारत");

#ifdef ENVOY_GOOGLE_GRPC
EXPECT_THROW_WITH_MESSAGE(
async_client_manager_.factoryForGrpcService(grpc_service, scope_, false), EnvoyException,
"Illegal ASCII value for gRPC initial metadata header key: legal-key.");
#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");
Expand Down