[xDS] fix "tls" channel cred in bootstrap to actually work (#36726)

This fixes a fairly embarrassing bug and lack of testing from #33234.  Prior to this fix, attempting to use the "tls" creds type would always cause a crash.

@gtcooke94 @matthewstevenson88 Note that the root cause of this bug was that when I wrote this code, I assumed that `grpc_tls_credentials_options` had a reasonable default for the cert verifier.  But it turns out that it doesn't do that directly; instead, we are only imposing that default in [`CredentialOptionSanityCheck()`](621aa4e5ce/src/core/lib/security/credentials/tls/tls_credentials.cc (L85)), which is called only when we call [`grpc_tls_credentials_create()`](621aa4e5ce/src/core/lib/security/credentials/tls/tls_credentials.cc (L160)), not when we directly instantiate `TlsCredentials` as my code was doing.  As part of the TlsCreds API cleanup you're working on, we should fix this so that callers get the right behavior even if they are internal callers that instantiate the TlsCreds object directly rather than calling the C-core API.

Closes #36726

COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36726 from markdroth:xds_bootstrap_mtls_creds_fix dac2789e10
PiperOrigin-RevId: 637993734

doc/grpc_xds_features.md

@@ -77,5 +77,5 @@ Aggregate Cluster Behavior Fixes | [A75](https://github.com/grpc/proposal/blob/m
 Pick First | [A62](https://github.com/grpc/proposal/blob/master/A62-pick-first.md) | v1.58.0 | v1.58.1 | v1.56.0 | |
 [StringMatcher for Header Matching](https://github.com/envoyproxy/envoy/blob/3fe4b8d335fa339ef6f17325c8d31f87ade7bb1a/api/envoy/config/route/v3/route_components.proto#L2280) | [A63](https://github.com/grpc/proposal/blob/master/A63-xds-string-matcher-in-header-matching.md) | v1.56.0 | v1.53.0 | v1.56.0 | v1.9.0 |
 LRS Custom Metrics Support | [A64](https://github.com/grpc/proposal/blob/master/A64-lrs-custom-metrics.md) | v1.54.0 | | | |
-mTLS Credentials in xDS Bootstrap File | [A65](https://github.com/grpc/proposal/blob/master/A65-xds-mtls-creds-in-bootstrap.md) | v1.57.0 | | v1.61.0 | |
+mTLS Credentials in xDS Bootstrap File | [A65](https://github.com/grpc/proposal/blob/master/A65-xds-mtls-creds-in-bootstrap.md) | v1.65.0 | | v1.61.0 | |
 Stateful Session Affinity | [A55](https://github.com/grpc/proposal/blob/master/A55-xds-stateful-session-affinity.md), [A60](https://github.com/grpc/proposal/blob/master/A60-xds-stateful-session-affinity-weighted-clusters.md), [A75](https://github.com/grpc/proposal/blob/master/A75-xds-aggregate-cluster-behavior-fixes.md) | v1.61.0 | | | |

src/core/lib/security/credentials/channel_creds_registry_init.cc

@@ -96,6 +96,8 @@ class TlsChannelCredsFactory : public ChannelCredsFactory<> {
     }
     options->set_watch_root_cert(!config->ca_certificate_file().empty());
     options->set_watch_identity_pair(!config->certificate_file().empty());
+    options->set_certificate_verifier(
+        MakeRefCounted<HostNameCertificateVerifier>());
     return MakeRefCounted<TlsCredentials>(std::move(options));
   }
 

test/cpp/end2end/xds/BUILD

@@ -216,6 +216,11 @@ grpc_cc_test(
     name = "xds_core_end2end_test",
     size = "large",
     srcs = ["xds_core_end2end_test.cc"],
+    data = [
+        "//src/core/tsi/test_creds:ca.pem",
+        "//src/core/tsi/test_creds:server1.key",
+        "//src/core/tsi/test_creds:server1.pem",
+    ],
     external_deps = [
         "gtest",
     ],

test/cpp/end2end/xds/xds_core_end2end_test.cc

@@ -160,6 +160,34 @@ TEST_P(XdsClientTest, XdsStreamErrorPropagation) {
               ::testing::HasSubstr("(node ID:xds_end2end_test)"));
 }
 
+//
+// XdsServerTlsTest: xDS server using TlsCreds
+//
+
+class XdsServerTlsTest : public XdsEnd2endTest {
+ protected:
+  void SetUp() override {
+    InitClient(MakeBootstrapBuilder().SetXdsChannelCredentials(
+                   "tls", absl::StrCat("{\"ca_certificate_file\": \"",
+                                       kCaCertPath, "\"}")),
+               /*lb_expected_authority=*/"",
+               /*xds_resource_does_not_exist_timeout_ms=*/0,
+               /*balancer_authority_override=*/"foo.test.google.fr");
+  }
+};
+
+INSTANTIATE_TEST_SUITE_P(
+    XdsTest, XdsServerTlsTest,
+    ::testing::Values(XdsTestType().set_xds_server_uses_tls_creds(true)),
+    &XdsTestType::Name);
+
+TEST_P(XdsServerTlsTest, Basic) {
+  CreateAndStartBackends(1);
+  EdsResourceArgs args({{"locality0", CreateEndpointsForBackends()}});
+  balancer_->ads_service()->SetEdsResource(BuildEdsResource(args));
+  CheckRpcSendOk(DEBUG_LOCATION);
+}
+
 //
 // GlobalXdsClientTest - tests that need to run with a global XdsClient
 // (this is the default in production)

test/cpp/end2end/xds/xds_end2end_test_lib.cc

@@ -318,6 +318,26 @@ XdsEnd2endTest::BalancerServerThread::BalancerServerThread(
           },
           debug_label)) {}
 
+std::shared_ptr<ServerCredentials>
+XdsEnd2endTest::BalancerServerThread::Credentials() {
+  if (GetParam().xds_server_uses_tls_creds()) {
+    std::string identity_cert =
+        grpc_core::testing::GetFileContents(kServerCertPath);
+    std::string private_key =
+        grpc_core::testing::GetFileContents(kServerKeyPath);
+    std::vector<experimental::IdentityKeyCertPair> identity_key_cert_pairs = {
+        {private_key, identity_cert}};
+    auto certificate_provider =
+        std::make_shared<grpc::experimental::StaticDataCertificateProvider>(
+            identity_key_cert_pairs);
+    grpc::experimental::TlsServerCredentialsOptions options(
+        certificate_provider);
+    options.watch_identity_key_cert_pairs();
+    return grpc::experimental::TlsServerCredentials(options);
+  }
+  return ServerThread::Credentials();
+}
+
 void XdsEnd2endTest::BalancerServerThread::RegisterAllServices(
     ServerBuilder* builder) {
   builder->RegisterService(ads_service_.get());
@@ -490,7 +510,8 @@ std::vector<int> XdsEnd2endTest::GetBackendPorts(size_t start_index,
 
 void XdsEnd2endTest::InitClient(absl::optional<XdsBootstrapBuilder> builder,
                                 std::string lb_expected_authority,
-                                int xds_resource_does_not_exist_timeout_ms) {
+                                int xds_resource_does_not_exist_timeout_ms,
+                                std::string balancer_authority_override) {
   if (!builder.has_value()) {
     builder = MakeBootstrapBuilder();
   }
@@ -509,6 +530,11 @@ void XdsEnd2endTest::InitClient(absl::optional<XdsBootstrapBuilder> builder,
         const_cast<char*>(GRPC_ARG_FAKE_SECURITY_EXPECTED_TARGETS),
         const_cast<char*>(lb_expected_authority.c_str())));
   }
+  if (!balancer_authority_override.empty()) {
+    xds_channel_args_to_add_.emplace_back(grpc_channel_arg_string_create(
+        const_cast<char*>(GRPC_ARG_DEFAULT_AUTHORITY),
+        const_cast<char*>(balancer_authority_override.c_str())));
+  }
   xds_channel_args_.num_args = xds_channel_args_to_add_.size();
   xds_channel_args_.args = xds_channel_args_to_add_.data();
   bootstrap_ = builder->Build();

test/cpp/end2end/xds/xds_end2end_test_lib.h

@@ -115,6 +115,11 @@ class XdsTestType {
     return *this;
   }
 
+  XdsTestType& set_xds_server_uses_tls_creds(bool value) {
+    xds_server_uses_tls_creds_ = value;
+    return *this;
+  }
+
   bool enable_load_reporting() const { return enable_load_reporting_; }
   bool enable_rds_testing() const { return enable_rds_testing_; }
   bool use_xds_credentials() const { return use_xds_credentials_; }
@@ -130,6 +135,7 @@ class XdsTestType {
   rbac_audit_condition() const {
     return rbac_audit_condition_;
   }
+  bool xds_server_uses_tls_creds() const { return xds_server_uses_tls_creds_; }
 
   std::string AsString() const {
     std::string retval = "V3";
@@ -158,6 +164,9 @@ class XdsTestType {
                                  RBAC_AuditLoggingOptions_AuditCondition_Name(
                                      rbac_audit_condition_));
     }
+    if (xds_server_uses_tls_creds_) {
+      retval += "XdsServerUsesTlsCreds";
+    }
     return retval;
   }
 
@@ -178,6 +187,7 @@ class XdsTestType {
   ::envoy::config::rbac::v3::RBAC_AuditLoggingOptions_AuditCondition
       rbac_audit_condition_ = ::envoy::config::rbac::v3::
           RBAC_AuditLoggingOptions_AuditCondition_NONE;
+  bool xds_server_uses_tls_creds_ = false;
 };
 
 // A base class for xDS end-to-end tests.
@@ -412,6 +422,11 @@ class XdsEnd2endTest : public ::testing::TestWithParam<XdsTestType>,
     AdsServiceImpl* ads_service() { return ads_service_.get(); }
     LrsServiceImpl* lrs_service() { return lrs_service_.get(); }
 
+    // If XdsTestType::xds_server_uses_tls_creds() is true,
+    // returns TlsServerCredentials.
+    // Otherwise, returns fake credentials.
+    std::shared_ptr<ServerCredentials> Credentials() override;
+
    private:
     const char* Type() override { return "Balancer"; }
     void RegisterAllServices(ServerBuilder* builder) override;
@@ -578,7 +593,8 @@ class XdsEnd2endTest : public ::testing::TestWithParam<XdsTestType>,
   // All tests must call this exactly once at the start of the test.
   void InitClient(absl::optional<XdsBootstrapBuilder> builder = absl::nullopt,
                   std::string lb_expected_authority = "",
-                  int xds_resource_does_not_exist_timeout_ms = 0);
+                  int xds_resource_does_not_exist_timeout_ms = 0,
+                  std::string balancer_authority_override = "");
 
   XdsBootstrapBuilder MakeBootstrapBuilder() {
     return XdsBootstrapBuilder().SetServers(

test/cpp/end2end/xds/xds_utils.cc

@@ -84,7 +84,7 @@ std::string XdsBootstrapBuilder::MakeXdsServersText(
       "          \"server_uri\": \"<SERVER_URI>\",\n"
       "          \"channel_creds\": [\n"
       "            {\n"
-      "              \"type\": \"<SERVER_CREDS_TYPE>\"\n"
+      "              \"type\": \"<SERVER_CREDS_TYPE>\"<SERVER_CREDS_CONFIG>\n"
       "            }\n"
       "          ],\n"
       "          \"server_features\": [<SERVER_FEATURES>]\n"
@@ -99,6 +99,11 @@ std::string XdsBootstrapBuilder::MakeXdsServersText(
         kXdsServerTemplate,
         {{"<SERVER_URI>", server_uri},
          {"<SERVER_CREDS_TYPE>", xds_channel_creds_type_},
+         {"<SERVER_CREDS_CONFIG>",
+          xds_channel_creds_config_.empty()
+              ? ""
+              : absl::StrCat(",\n              \"config\": ",
+                             xds_channel_creds_config_)},
          {"<SERVER_FEATURES>", absl::StrJoin(server_features, ", ")}}));
   }
   return absl::StrCat("      \"xds_servers\": [\n",

test/cpp/end2end/xds/xds_utils.h

@@ -43,8 +43,10 @@ class XdsBootstrapBuilder {
     servers_ = std::vector<std::string>(servers.begin(), servers.end());
     return *this;
   }
-  XdsBootstrapBuilder& SetXdsChannelCredentials(const std::string& type) {
+  XdsBootstrapBuilder& SetXdsChannelCredentials(
+      const std::string& type, const std::string& config = "") {
     xds_channel_creds_type_ = type;
+    xds_channel_creds_config_ = config;
     return *this;
   }
   XdsBootstrapBuilder& SetClientDefaultListenerResourceNameTemplate(
@@ -100,6 +102,7 @@ class XdsBootstrapBuilder {
   bool ignore_resource_deletion_ = false;
   std::vector<std::string> servers_;
   std::string xds_channel_creds_type_ = "fake";
+  std::string xds_channel_creds_config_;
   std::string client_default_listener_resource_name_template_;
   std::map<std::string /*key*/, PluginInfo> plugins_;
   std::map<std::string /*authority_name*/, AuthorityInfo> authorities_;