Merge pull request #24780 from yashykt/xdssecenvvar

Protect xds security code with the environment variable "GRPC_XDS_EXPERIMENTAL_SECURITY_SUPPORT"

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;
+            }
           }
         }
       }

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 {

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",

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<TestType> {
+ 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<FakeCertificateProviderFactory>());
   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<FakeCertificateProviderFactory>());
   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<RefCountedPtr<FakeCertificateProviderFactory::Config>>(
-                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<RefCountedPtr<FakeCertificateProviderFactory::Config>>(
+            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<FakeCertificateProviderFactory>());
   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<RefCountedPtr<FakeCertificateProviderFactory::Config>>(
-                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<RefCountedPtr<FakeCertificateProviderFactory::Config>>(
+            fake_plugin.config)
+            ->value(),
+        0);
+  } else {
+    EXPECT_TRUE(bootstrap.certificate_providers().empty());
+  }
+}
+
+std::string TestTypeName(const ::testing::TestParamInfo<TestType>& info) {
+  return info.param.AsString();
 }
 
+INSTANTIATE_TEST_SUITE_P(XdsBootstrap, XdsBootstrapTest,
+                         ::testing::Values(TestType(false), TestType(true)),
+                         &TestTypeName);
+
 }  // namespace testing
 }  // namespace grpc_core