From 2395bb1b866d7943813fb630d69e1fbadeb10708 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 28 May 2024 12:53:34 -0700 Subject: [PATCH] [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()`](https://github.com/grpc/grpc/blob/621aa4e5ce5a1ac93a31439adcec00e301206072/src/core/lib/security/credentials/tls/tls_credentials.cc#L85), which is called only when we call [`grpc_tls_credentials_create()`](https://github.com/grpc/grpc/blob/621aa4e5ce5a1ac93a31439adcec00e301206072/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 dac2789e104a7cdc22efbfe5be0a4da4a4ad13a1 PiperOrigin-RevId: 637993734 --- doc/grpc_xds_features.md | 2 +- .../channel_creds_registry_init.cc | 2 ++ test/cpp/end2end/xds/BUILD | 5 ++++ test/cpp/end2end/xds/xds_core_end2end_test.cc | 28 +++++++++++++++++++ test/cpp/end2end/xds/xds_end2end_test_lib.cc | 28 ++++++++++++++++++- test/cpp/end2end/xds/xds_end2end_test_lib.h | 18 +++++++++++- test/cpp/end2end/xds/xds_utils.cc | 7 ++++- test/cpp/end2end/xds/xds_utils.h | 5 +++- 8 files changed, 90 insertions(+), 5 deletions(-) diff --git a/doc/grpc_xds_features.md b/doc/grpc_xds_features.md index b6f33ea59fe..16ac26214bd 100644 --- a/doc/grpc_xds_features.md +++ b/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 | | | | diff --git a/src/core/lib/security/credentials/channel_creds_registry_init.cc b/src/core/lib/security/credentials/channel_creds_registry_init.cc index 9b7a0e04372..832daba1284 100644 --- a/src/core/lib/security/credentials/channel_creds_registry_init.cc +++ b/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()); return MakeRefCounted(std::move(options)); } diff --git a/test/cpp/end2end/xds/BUILD b/test/cpp/end2end/xds/BUILD index 15afd38eb26..2a66bca252c 100644 --- a/test/cpp/end2end/xds/BUILD +++ b/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", ], diff --git a/test/cpp/end2end/xds/xds_core_end2end_test.cc b/test/cpp/end2end/xds/xds_core_end2end_test.cc index abc1d20cf06..6b8fcd43482 100644 --- a/test/cpp/end2end/xds/xds_core_end2end_test.cc +++ b/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) diff --git a/test/cpp/end2end/xds/xds_end2end_test_lib.cc b/test/cpp/end2end/xds/xds_end2end_test_lib.cc index 08d305c2a8a..f9e884650fe 100644 --- a/test/cpp/end2end/xds/xds_end2end_test_lib.cc +++ b/test/cpp/end2end/xds/xds_end2end_test_lib.cc @@ -318,6 +318,26 @@ XdsEnd2endTest::BalancerServerThread::BalancerServerThread( }, debug_label)) {} +std::shared_ptr +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 identity_key_cert_pairs = { + {private_key, identity_cert}}; + auto certificate_provider = + std::make_shared( + 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 XdsEnd2endTest::GetBackendPorts(size_t start_index, void XdsEnd2endTest::InitClient(absl::optional 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 builder, const_cast(GRPC_ARG_FAKE_SECURITY_EXPECTED_TARGETS), const_cast(lb_expected_authority.c_str()))); } + if (!balancer_authority_override.empty()) { + xds_channel_args_to_add_.emplace_back(grpc_channel_arg_string_create( + const_cast(GRPC_ARG_DEFAULT_AUTHORITY), + const_cast(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(); diff --git a/test/cpp/end2end/xds/xds_end2end_test_lib.h b/test/cpp/end2end/xds/xds_end2end_test_lib.h index 7ac0474a9f4..c16aa278d1b 100644 --- a/test/cpp/end2end/xds/xds_end2end_test_lib.h +++ b/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, 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 Credentials() override; + private: const char* Type() override { return "Balancer"; } void RegisterAllServices(ServerBuilder* builder) override; @@ -578,7 +593,8 @@ class XdsEnd2endTest : public ::testing::TestWithParam, // All tests must call this exactly once at the start of the test. void InitClient(absl::optional 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( diff --git a/test/cpp/end2end/xds/xds_utils.cc b/test/cpp/end2end/xds/xds_utils.cc index f70ca9b9ccd..6a45d81b8a0 100644 --- a/test/cpp/end2end/xds/xds_utils.cc +++ b/test/cpp/end2end/xds/xds_utils.cc @@ -84,7 +84,7 @@ std::string XdsBootstrapBuilder::MakeXdsServersText( " \"server_uri\": \"\",\n" " \"channel_creds\": [\n" " {\n" - " \"type\": \"\"\n" + " \"type\": \"\"\n" " }\n" " ],\n" " \"server_features\": []\n" @@ -99,6 +99,11 @@ std::string XdsBootstrapBuilder::MakeXdsServersText( kXdsServerTemplate, {{"", server_uri}, {"", xds_channel_creds_type_}, + {"", + xds_channel_creds_config_.empty() + ? "" + : absl::StrCat(",\n \"config\": ", + xds_channel_creds_config_)}, {"", absl::StrJoin(server_features, ", ")}})); } return absl::StrCat(" \"xds_servers\": [\n", diff --git a/test/cpp/end2end/xds/xds_utils.h b/test/cpp/end2end/xds/xds_utils.h index 10b552ea62c..6fb0d33b341 100644 --- a/test/cpp/end2end/xds/xds_utils.h +++ b/test/cpp/end2end/xds/xds_utils.h @@ -43,8 +43,10 @@ class XdsBootstrapBuilder { servers_ = std::vector(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 servers_; std::string xds_channel_creds_type_ = "fake"; + std::string xds_channel_creds_config_; std::string client_default_listener_resource_name_template_; std::map plugins_; std::map authorities_;