From abdc490f72fc26beec6966a61adaf9fa2ce158e3 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 17 Nov 2020 17:18:27 -0800 Subject: [PATCH] Protect xds security code with environment variable --- src/core/ext/xds/xds_api.cc | 68 ++++++----- src/core/ext/xds/xds_api.h | 5 + src/core/ext/xds/xds_bootstrap.cc | 19 +-- test/core/xds/xds_bootstrap_test.cc | 176 +++++++++++++++++++--------- 4 files changed, 176 insertions(+), 92 deletions(-) diff --git a/src/core/ext/xds/xds_api.cc b/src/core/ext/xds/xds_api.cc index 31d862d6c2d..8ec9dfe9bd9 100644 --- a/src/core/ext/xds/xds_api.cc +++ b/src/core/ext/xds/xds_api.cc @@ -99,6 +99,17 @@ bool XdsTimeoutEnabled() { return parse_succeeded && parsed_value; } +// TODO(yashykt): Check to see if xDS security is enabled. This will be +// removed once this feature is fully integration-tested and enabled by +// default. +bool XdsSecurityEnabled() { + char* value = gpr_getenv("GRPC_XDS_EXPERIMENTAL_SECURITY_SUPPORT"); + bool parsed_value; + bool parse_succeeded = gpr_parse_bool_value(value, &parsed_value); + gpr_free(value); + return parse_succeeded && parsed_value; +} + // // XdsApi::Route::Matchers::PathMatcher // @@ -1566,33 +1577,36 @@ grpc_error* CdsResponseParse( return GRPC_ERROR_CREATE_FROM_STATIC_STRING( "LB policy is not ROUND_ROBIN."); } - // Record Upstream tls context - auto* transport_socket = - envoy_config_cluster_v3_Cluster_transport_socket(cluster); - if (transport_socket != nullptr) { - absl::string_view name = UpbStringToAbsl( - envoy_config_core_v3_TransportSocket_name(transport_socket)); - if (name == "envoy.transport_sockets.tls") { - auto* typed_config = - envoy_config_core_v3_TransportSocket_typed_config(transport_socket); - if (typed_config != nullptr) { - const upb_strview encoded_upstream_tls_context = - google_protobuf_Any_value(typed_config); - auto* upstream_tls_context = - envoy_extensions_transport_sockets_tls_v3_UpstreamTlsContext_parse( - encoded_upstream_tls_context.data, - encoded_upstream_tls_context.size, arena); - if (upstream_tls_context == nullptr) { - return GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "Can't decode upstream tls context."); - } - auto* common_tls_context = - envoy_extensions_transport_sockets_tls_v3_UpstreamTlsContext_common_tls_context( - upstream_tls_context); - if (common_tls_context != nullptr) { - grpc_error* error = CommonTlsContextParse( - common_tls_context, &cds_update.common_tls_context); - if (error != GRPC_ERROR_NONE) return error; + if (XdsSecurityEnabled()) { + // Record Upstream tls context + auto* transport_socket = + envoy_config_cluster_v3_Cluster_transport_socket(cluster); + if (transport_socket != nullptr) { + absl::string_view name = UpbStringToAbsl( + envoy_config_core_v3_TransportSocket_name(transport_socket)); + if (name == "envoy.transport_sockets.tls") { + auto* typed_config = + envoy_config_core_v3_TransportSocket_typed_config( + transport_socket); + if (typed_config != nullptr) { + const upb_strview encoded_upstream_tls_context = + google_protobuf_Any_value(typed_config); + auto* upstream_tls_context = + envoy_extensions_transport_sockets_tls_v3_UpstreamTlsContext_parse( + encoded_upstream_tls_context.data, + encoded_upstream_tls_context.size, arena); + if (upstream_tls_context == nullptr) { + return GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "Can't decode upstream tls context."); + } + auto* common_tls_context = + envoy_extensions_transport_sockets_tls_v3_UpstreamTlsContext_common_tls_context( + upstream_tls_context); + if (common_tls_context != nullptr) { + grpc_error* error = CommonTlsContextParse( + common_tls_context, &cds_update.common_tls_context); + if (error != GRPC_ERROR_NONE) return error; + } } } } diff --git a/src/core/ext/xds/xds_api.h b/src/core/ext/xds/xds_api.h index 8e7b1f235a8..885dd2c6a73 100644 --- a/src/core/ext/xds/xds_api.h +++ b/src/core/ext/xds/xds_api.h @@ -39,6 +39,11 @@ namespace grpc_core { +// TODO(yashykt): Check to see if xDS security is enabled. This will be +// removed once this feature is fully integration-tested and enabled by +// default. +bool XdsSecurityEnabled(); + class XdsClient; class XdsApi { diff --git a/src/core/ext/xds/xds_bootstrap.cc b/src/core/ext/xds/xds_bootstrap.cc index 3daeb73404d..e48d9827dec 100644 --- a/src/core/ext/xds/xds_bootstrap.cc +++ b/src/core/ext/xds/xds_bootstrap.cc @@ -29,6 +29,7 @@ #include "absl/strings/string_view.h" #include "src/core/ext/xds/certificate_provider_registry.h" +#include "src/core/ext/xds/xds_api.h" #include "src/core/lib/gpr/env.h" #include "src/core/lib/gpr/string.h" #include "src/core/lib/iomgr/load_file.h" @@ -204,14 +205,16 @@ XdsBootstrap::XdsBootstrap(Json json, grpc_error** error) { if (parse_error != GRPC_ERROR_NONE) error_list.push_back(parse_error); } } - it = json.mutable_object()->find("certificate_providers"); - if (it != json.mutable_object()->end()) { - if (it->second.type() != Json::Type::OBJECT) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "\"certificate_providers\" field is not an object")); - } else { - grpc_error* parse_error = ParseCertificateProviders(&it->second); - if (parse_error != GRPC_ERROR_NONE) error_list.push_back(parse_error); + if (XdsSecurityEnabled()) { + it = json.mutable_object()->find("certificate_providers"); + if (it != json.mutable_object()->end()) { + if (it->second.type() != Json::Type::OBJECT) { + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "\"certificate_providers\" field is not an object")); + } else { + grpc_error* parse_error = ParseCertificateProviders(&it->second); + if (parse_error != GRPC_ERROR_NONE) error_list.push_back(parse_error); + } } } *error = GRPC_ERROR_CREATE_FROM_VECTOR("errors parsing xds bootstrap file", diff --git a/test/core/xds/xds_bootstrap_test.cc b/test/core/xds/xds_bootstrap_test.cc index 596d8233c17..7a7545dca1d 100644 --- a/test/core/xds/xds_bootstrap_test.cc +++ b/test/core/xds/xds_bootstrap_test.cc @@ -34,14 +34,40 @@ namespace grpc_core { namespace testing { -class XdsBootstrapTest : public ::testing::Test { +class TestType { public: - XdsBootstrapTest() { grpc_init(); } + TestType(bool parse_xds_certificate_providers) + : parse_xds_certificate_providers_(parse_xds_certificate_providers) {} + + bool parse_xds_certificate_providers() const { + return parse_xds_certificate_providers_; + } + + std::string AsString() const { + return parse_xds_certificate_providers_ + ? "WithCertificateProvidersParsing" + : "WithoutCertificateProvidersParsing"; + } + + private: + const bool parse_xds_certificate_providers_; +}; + +class XdsBootstrapTest : public ::testing::TestWithParam { + public: + XdsBootstrapTest() { + if (GetParam().parse_xds_certificate_providers()) { + gpr_setenv("GRPC_XDS_EXPERIMENTAL_SECURITY_SUPPORT", "true"); + } else { + gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_SECURITY_SUPPORT"); + } + grpc_init(); + } ~XdsBootstrapTest() override { grpc_shutdown_blocking(); } }; -TEST_F(XdsBootstrapTest, Basic) { +TEST_P(XdsBootstrapTest, Basic) { const char* json_str = "{" " \"xds_servers\": [" @@ -116,7 +142,7 @@ TEST_F(XdsBootstrapTest, Basic) { ::testing::Property(&Json::string_value, "1"))))); } -TEST_F(XdsBootstrapTest, ValidWithoutNode) { +TEST_P(XdsBootstrapTest, ValidWithoutNode) { const char* json_str = "{" " \"xds_servers\": [" @@ -136,7 +162,7 @@ TEST_F(XdsBootstrapTest, ValidWithoutNode) { EXPECT_EQ(bootstrap.node(), nullptr); } -TEST_F(XdsBootstrapTest, InsecureCreds) { +TEST_P(XdsBootstrapTest, InsecureCreds) { const char* json_str = "{" " \"xds_servers\": [" @@ -156,7 +182,7 @@ TEST_F(XdsBootstrapTest, InsecureCreds) { EXPECT_EQ(bootstrap.node(), nullptr); } -TEST_F(XdsBootstrapTest, GoogleDefaultCreds) { +TEST_P(XdsBootstrapTest, GoogleDefaultCreds) { // Generate call creds file needed by GoogleDefaultCreds. const char token_str[] = "{ \"client_id\": \"32555999999.apps.googleusercontent.com\"," @@ -192,7 +218,7 @@ TEST_F(XdsBootstrapTest, GoogleDefaultCreds) { EXPECT_EQ(bootstrap.node(), nullptr); } -TEST_F(XdsBootstrapTest, MissingChannelCreds) { +TEST_P(XdsBootstrapTest, MissingChannelCreds) { const char* json_str = "{" " \"xds_servers\": [" @@ -210,7 +236,7 @@ TEST_F(XdsBootstrapTest, MissingChannelCreds) { GRPC_ERROR_UNREF(error); } -TEST_F(XdsBootstrapTest, NoKnownChannelCreds) { +TEST_P(XdsBootstrapTest, NoKnownChannelCreds) { const char* json_str = "{" " \"xds_servers\": [" @@ -230,7 +256,7 @@ TEST_F(XdsBootstrapTest, NoKnownChannelCreds) { GRPC_ERROR_UNREF(error); } -TEST_F(XdsBootstrapTest, MissingXdsServers) { +TEST_P(XdsBootstrapTest, MissingXdsServers) { grpc_error* error = GRPC_ERROR_NONE; Json json = Json::Parse("{}", &error); ASSERT_EQ(error, GRPC_ERROR_NONE) << grpc_error_string(error); @@ -240,7 +266,7 @@ TEST_F(XdsBootstrapTest, MissingXdsServers) { GRPC_ERROR_UNREF(error); } -TEST_F(XdsBootstrapTest, TopFieldsWrongTypes) { +TEST_P(XdsBootstrapTest, TopFieldsWrongTypes) { const char* json_str = "{" " \"xds_servers\":1," @@ -252,14 +278,17 @@ TEST_F(XdsBootstrapTest, TopFieldsWrongTypes) { ASSERT_EQ(error, GRPC_ERROR_NONE) << grpc_error_string(error); XdsBootstrap bootstrap(std::move(json), &error); EXPECT_THAT(grpc_error_string(error), - ::testing::ContainsRegex( - "\"xds_servers\" field is not an array.*" - "\"node\" field is not an object.*" - "\"certificate_providers\" field is not an object")); + ::testing::ContainsRegex("\"xds_servers\" field is not an array.*" + "\"node\" field is not an object.*")); + if (GetParam().parse_xds_certificate_providers()) { + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "\"certificate_providers\" field is not an object")); + } GRPC_ERROR_UNREF(error); } -TEST_F(XdsBootstrapTest, XdsServerMissingServerUri) { +TEST_P(XdsBootstrapTest, XdsServerMissingServerUri) { const char* json_str = "{" " \"xds_servers\":[{}]" @@ -275,7 +304,7 @@ TEST_F(XdsBootstrapTest, XdsServerMissingServerUri) { GRPC_ERROR_UNREF(error); } -TEST_F(XdsBootstrapTest, XdsServerUriAndCredsWrongTypes) { +TEST_P(XdsBootstrapTest, XdsServerUriAndCredsWrongTypes) { const char* json_str = "{" " \"xds_servers\":[" @@ -298,7 +327,7 @@ TEST_F(XdsBootstrapTest, XdsServerUriAndCredsWrongTypes) { GRPC_ERROR_UNREF(error); } -TEST_F(XdsBootstrapTest, ChannelCredsFieldsWrongTypes) { +TEST_P(XdsBootstrapTest, ChannelCredsFieldsWrongTypes) { const char* json_str = "{" " \"xds_servers\":[" @@ -328,7 +357,7 @@ TEST_F(XdsBootstrapTest, ChannelCredsFieldsWrongTypes) { GRPC_ERROR_UNREF(error); } -TEST_F(XdsBootstrapTest, NodeFieldsWrongTypes) { +TEST_P(XdsBootstrapTest, NodeFieldsWrongTypes) { const char* json_str = "{" " \"node\":{" @@ -351,7 +380,7 @@ TEST_F(XdsBootstrapTest, NodeFieldsWrongTypes) { GRPC_ERROR_UNREF(error); } -TEST_F(XdsBootstrapTest, LocalityFieldsWrongType) { +TEST_P(XdsBootstrapTest, LocalityFieldsWrongType) { const char* json_str = "{" " \"node\":{" @@ -375,12 +404,13 @@ TEST_F(XdsBootstrapTest, LocalityFieldsWrongType) { GRPC_ERROR_UNREF(error); } -TEST_F(XdsBootstrapTest, CertificateProvidersElementWrongType) { +TEST_P(XdsBootstrapTest, CertificateProvidersElementWrongType) { const char* json_str = "{" " \"xds_servers\": [" " {" - " \"server_uri\": \"fake:///lb\"" + " \"server_uri\": \"fake:///lb\"," + " \"channel_creds\": [{\"type\": \"fake\"}]" " }" " ]," " \"certificate_providers\": {" @@ -391,19 +421,24 @@ TEST_F(XdsBootstrapTest, CertificateProvidersElementWrongType) { Json json = Json::Parse(json_str, &error); ASSERT_EQ(error, GRPC_ERROR_NONE) << grpc_error_string(error); XdsBootstrap bootstrap(std::move(json), &error); - EXPECT_THAT(grpc_error_string(error), - ::testing::ContainsRegex( - "errors parsing \"certificate_providers\" object.*" - "element \"plugin\" is not an object")); + if (GetParam().parse_xds_certificate_providers()) { + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "errors parsing \"certificate_providers\" object.*" + "element \"plugin\" is not an object")); + } else { + EXPECT_EQ(error, GRPC_ERROR_NONE) << grpc_error_string(error); + } GRPC_ERROR_UNREF(error); } -TEST_F(XdsBootstrapTest, CertificateProvidersPluginNameWrongType) { +TEST_P(XdsBootstrapTest, CertificateProvidersPluginNameWrongType) { const char* json_str = "{" " \"xds_servers\": [" " {" - " \"server_uri\": \"fake:///lb\"" + " \"server_uri\": \"fake:///lb\"," + " \"channel_creds\": [{\"type\": \"fake\"}]" " }" " ]," " \"certificate_providers\": {" @@ -416,11 +451,15 @@ TEST_F(XdsBootstrapTest, CertificateProvidersPluginNameWrongType) { Json json = Json::Parse(json_str, &error); ASSERT_EQ(error, GRPC_ERROR_NONE) << grpc_error_string(error); XdsBootstrap bootstrap(std::move(json), &error); - EXPECT_THAT(grpc_error_string(error), - ::testing::ContainsRegex( - "errors parsing \"certificate_providers\" object.*" - "errors parsing element \"plugin\".*" - "\"plugin_name\" field is not a string")); + if (GetParam().parse_xds_certificate_providers()) { + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "errors parsing \"certificate_providers\" object.*" + "errors parsing element \"plugin\".*" + "\"plugin_name\" field is not a string")); + } else { + EXPECT_EQ(error, GRPC_ERROR_NONE) << grpc_error_string(error); + } GRPC_ERROR_UNREF(error); } @@ -473,14 +512,15 @@ class FakeCertificateProviderFactory : public CertificateProviderFactory { } }; -TEST_F(XdsBootstrapTest, CertificateProvidersFakePluginParsingError) { +TEST_P(XdsBootstrapTest, CertificateProvidersFakePluginParsingError) { CertificateProviderRegistry::RegisterCertificateProviderFactory( absl::make_unique()); const char* json_str = "{" " \"xds_servers\": [" " {" - " \"server_uri\": \"fake:///lb\"" + " \"server_uri\": \"fake:///lb\"," + " \"channel_creds\": [{\"type\": \"fake\"}]" " }" " ]," " \"certificate_providers\": {" @@ -496,15 +536,19 @@ TEST_F(XdsBootstrapTest, CertificateProvidersFakePluginParsingError) { Json json = Json::Parse(json_str, &error); ASSERT_EQ(error, GRPC_ERROR_NONE) << grpc_error_string(error); XdsBootstrap bootstrap(std::move(json), &error); - EXPECT_THAT(grpc_error_string(error), - ::testing::ContainsRegex( - "errors parsing \"certificate_providers\" object.*" - "errors parsing element \"fake_plugin\".*" - "field:config field:value not of type number")); + if (GetParam().parse_xds_certificate_providers()) { + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "errors parsing \"certificate_providers\" object.*" + "errors parsing element \"fake_plugin\".*" + "field:config field:value not of type number")); + } else { + EXPECT_EQ(error, GRPC_ERROR_NONE) << grpc_error_string(error); + } GRPC_ERROR_UNREF(error); } -TEST_F(XdsBootstrapTest, CertificateProvidersFakePluginParsingSuccess) { +TEST_P(XdsBootstrapTest, CertificateProvidersFakePluginParsingSuccess) { CertificateProviderRegistry::RegisterCertificateProviderFactory( absl::make_unique()); const char* json_str = @@ -529,17 +573,22 @@ TEST_F(XdsBootstrapTest, CertificateProvidersFakePluginParsingSuccess) { ASSERT_EQ(error, GRPC_ERROR_NONE) << grpc_error_string(error); XdsBootstrap bootstrap(std::move(json), &error); ASSERT_TRUE(error == GRPC_ERROR_NONE) << grpc_error_string(error); - const CertificateProviderStore::PluginDefinition& fake_plugin = - bootstrap.certificate_providers().at("fake_plugin"); - ASSERT_EQ(fake_plugin.plugin_name, "fake"); - ASSERT_STREQ(fake_plugin.config->name(), "fake"); - ASSERT_EQ(static_cast>( - fake_plugin.config) - ->value(), - 10); + if (GetParam().parse_xds_certificate_providers()) { + const CertificateProviderStore::PluginDefinition& fake_plugin = + bootstrap.certificate_providers().at("fake_plugin"); + ASSERT_EQ(fake_plugin.plugin_name, "fake"); + ASSERT_STREQ(fake_plugin.config->name(), "fake"); + ASSERT_EQ( + static_cast>( + fake_plugin.config) + ->value(), + 10); + } else { + EXPECT_TRUE(bootstrap.certificate_providers().empty()); + } } -TEST_F(XdsBootstrapTest, CertificateProvidersFakePluginEmptyConfig) { +TEST_P(XdsBootstrapTest, CertificateProvidersFakePluginEmptyConfig) { CertificateProviderRegistry::RegisterCertificateProviderFactory( absl::make_unique()); const char* json_str = @@ -561,16 +610,29 @@ TEST_F(XdsBootstrapTest, CertificateProvidersFakePluginEmptyConfig) { ASSERT_EQ(error, GRPC_ERROR_NONE) << grpc_error_string(error); XdsBootstrap bootstrap(std::move(json), &error); ASSERT_TRUE(error == GRPC_ERROR_NONE) << grpc_error_string(error); - const CertificateProviderStore::PluginDefinition& fake_plugin = - bootstrap.certificate_providers().at("fake_plugin"); - ASSERT_EQ(fake_plugin.plugin_name, "fake"); - ASSERT_STREQ(fake_plugin.config->name(), "fake"); - ASSERT_EQ(static_cast>( - fake_plugin.config) - ->value(), - 0); + if (GetParam().parse_xds_certificate_providers()) { + const CertificateProviderStore::PluginDefinition& fake_plugin = + bootstrap.certificate_providers().at("fake_plugin"); + ASSERT_EQ(fake_plugin.plugin_name, "fake"); + ASSERT_STREQ(fake_plugin.config->name(), "fake"); + ASSERT_EQ( + static_cast>( + fake_plugin.config) + ->value(), + 0); + } else { + EXPECT_TRUE(bootstrap.certificate_providers().empty()); + } +} + +std::string TestTypeName(const ::testing::TestParamInfo& info) { + return info.param.AsString(); } +INSTANTIATE_TEST_SUITE_P(XdsBootstrap, XdsBootstrapTest, + ::testing::Values(TestType(false), TestType(true)), + &TestTypeName); + } // namespace testing } // namespace grpc_core