diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc b/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc index 8a10db52222..6a49d399eb8 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc @@ -270,6 +270,17 @@ void CdsLb::ShutdownLocked() { } xds_client_.reset(DEBUG_LOCATION, "CdsLb"); } + if (xds_certificate_provider_ != nullptr) { + // Unregister root and identity distributors from xDS provider, so + // that we don't leak memory. + // TODO(yashkt): This should not be necessary. Consider changing + // the provider API to use DualRefCounted<> instead of RefCounted<>, + // so that the xDS provider can use weak refs internally. + xds_certificate_provider_->UpdateRootCertNameAndDistributor( + config_->cluster(), "", nullptr); + xds_certificate_provider_->UpdateIdentityCertNameAndDistributor( + config_->cluster(), "", nullptr); + } grpc_channel_args_destroy(args_); args_ = nullptr; } @@ -453,18 +464,16 @@ grpc_error* CdsLb::UpdateXdsCertificateProvider( xds_certificate_provider_ = nullptr; return GRPC_ERROR_NONE; } + if (xds_certificate_provider_ == nullptr) { + xds_certificate_provider_ = MakeRefCounted(); + } + // Configure root cert. absl::string_view root_provider_instance_name = cluster_data.common_tls_context.combined_validation_context .validation_context_certificate_provider_instance.instance_name; absl::string_view root_provider_cert_name = cluster_data.common_tls_context.combined_validation_context .validation_context_certificate_provider_instance.certificate_name; - absl::string_view identity_provider_instance_name = - cluster_data.common_tls_context - .tls_certificate_certificate_provider_instance.instance_name; - absl::string_view identity_provider_cert_name = - cluster_data.common_tls_context - .tls_certificate_certificate_provider_instance.certificate_name; RefCountedPtr new_root_provider; if (!root_provider_instance_name.empty()) { new_root_provider = @@ -491,6 +500,18 @@ grpc_error* CdsLb::UpdateXdsCertificateProvider( } root_certificate_provider_ = std::move(new_root_provider); } + xds_certificate_provider_->UpdateRootCertNameAndDistributor( + config_->cluster(), root_provider_cert_name, + root_certificate_provider_ == nullptr + ? nullptr + : root_certificate_provider_->distributor()); + // Configure identity cert. + absl::string_view identity_provider_instance_name = + cluster_data.common_tls_context + .tls_certificate_certificate_provider_instance.instance_name; + absl::string_view identity_provider_cert_name = + cluster_data.common_tls_context + .tls_certificate_certificate_provider_instance.certificate_name; RefCountedPtr new_identity_provider; if (!identity_provider_instance_name.empty()) { new_identity_provider = @@ -517,53 +538,17 @@ grpc_error* CdsLb::UpdateXdsCertificateProvider( } identity_certificate_provider_ = std::move(new_identity_provider); } + xds_certificate_provider_->UpdateIdentityCertNameAndDistributor( + config_->cluster(), identity_provider_cert_name, + identity_certificate_provider_ == nullptr + ? nullptr + : identity_certificate_provider_->distributor()); + // Configure SAN matchers. const std::vector& match_subject_alt_names = cluster_data.common_tls_context.combined_validation_context .default_validation_context.match_subject_alt_names; - if (!root_provider_instance_name.empty() && - !identity_provider_instance_name.empty()) { - // Using mTLS configuration - if (xds_certificate_provider_ != nullptr && - xds_certificate_provider_->ProvidesRootCerts() && - xds_certificate_provider_->ProvidesIdentityCerts()) { - xds_certificate_provider_->UpdateRootCertNameAndDistributor( - root_provider_cert_name, root_certificate_provider_->distributor()); - xds_certificate_provider_->UpdateIdentityCertNameAndDistributor( - identity_provider_cert_name, - identity_certificate_provider_->distributor()); - xds_certificate_provider_->UpdateSubjectAlternativeNameMatchers( - match_subject_alt_names); - } else { - // Existing xDS certificate provider does not have mTLS configuration. - // Create new certificate provider so that new subchannel connectors are - // created. - xds_certificate_provider_ = MakeRefCounted( - root_provider_cert_name, root_certificate_provider_->distributor(), - identity_provider_cert_name, - identity_certificate_provider_->distributor(), - match_subject_alt_names); - } - } else if (!root_provider_instance_name.empty()) { - // Using TLS configuration - if (xds_certificate_provider_ != nullptr && - xds_certificate_provider_->ProvidesRootCerts() && - !xds_certificate_provider_->ProvidesIdentityCerts()) { - xds_certificate_provider_->UpdateRootCertNameAndDistributor( - root_provider_cert_name, root_certificate_provider_->distributor()); - xds_certificate_provider_->UpdateSubjectAlternativeNameMatchers( - match_subject_alt_names); - } else { - // Existing xDS certificate provider does not have TLS configuration. - // Create new certificate provider so that new subchannel connectors are - // created. - xds_certificate_provider_ = MakeRefCounted( - root_provider_cert_name, root_certificate_provider_->distributor(), - "", nullptr, match_subject_alt_names); - } - } else { - // No configuration provided. - xds_certificate_provider_ = nullptr; - } + xds_certificate_provider_->UpdateSubjectAlternativeNameMatchers( + config_->cluster(), match_subject_alt_names); return GRPC_ERROR_NONE; } diff --git a/src/core/ext/xds/xds_certificate_provider.cc b/src/core/ext/xds/xds_certificate_provider.cc index f285b6db3a4..0386be1cf6b 100644 --- a/src/core/ext/xds/xds_certificate_provider.cc +++ b/src/core/ext/xds/xds_certificate_provider.cc @@ -37,15 +37,16 @@ class RootCertificatesWatcher // presently, the watcher is immediately deleted when // CancelTlsCertificatesWatch() is called, but that can potentially change in // the future. - explicit RootCertificatesWatcher( - RefCountedPtr parent) - : parent_(std::move(parent)) {} + RootCertificatesWatcher( + RefCountedPtr parent, + std::string cert_name) + : parent_(std::move(parent)), cert_name_(std::move(cert_name)) {} void OnCertificatesChanged(absl::optional root_certs, absl::optional /* key_cert_pairs */) override { if (root_certs.has_value()) { - parent_->SetKeyMaterials("", std::string(root_certs.value()), + parent_->SetKeyMaterials(cert_name_, std::string(root_certs.value()), absl::nullopt); } } @@ -53,7 +54,7 @@ class RootCertificatesWatcher void OnError(grpc_error* root_cert_error, grpc_error* identity_cert_error) override { if (root_cert_error != GRPC_ERROR_NONE) { - parent_->SetErrorForCert("", root_cert_error /* pass the ref */, + parent_->SetErrorForCert(cert_name_, root_cert_error /* pass the ref */, absl::nullopt); } GRPC_ERROR_UNREF(identity_cert_error); @@ -61,6 +62,7 @@ class RootCertificatesWatcher private: RefCountedPtr parent_; + std::string cert_name_; }; class IdentityCertificatesWatcher @@ -71,22 +73,23 @@ class IdentityCertificatesWatcher // presently, the watcher is immediately deleted when // CancelTlsCertificatesWatch() is called, but that can potentially change in // the future. - explicit IdentityCertificatesWatcher( - RefCountedPtr parent) - : parent_(std::move(parent)) {} + IdentityCertificatesWatcher( + RefCountedPtr parent, + std::string cert_name) + : parent_(std::move(parent)), cert_name_(std::move(cert_name)) {} void OnCertificatesChanged( absl::optional /* root_certs */, absl::optional key_cert_pairs) override { if (key_cert_pairs.has_value()) { - parent_->SetKeyMaterials("", absl::nullopt, key_cert_pairs); + parent_->SetKeyMaterials(cert_name_, absl::nullopt, key_cert_pairs); } } void OnError(grpc_error* root_cert_error, grpc_error* identity_cert_error) override { if (identity_cert_error != GRPC_ERROR_NONE) { - parent_->SetErrorForCert("", absl::nullopt, + parent_->SetErrorForCert(cert_name_, absl::nullopt, identity_cert_error /* pass the ref */); } GRPC_ERROR_UNREF(root_cert_error); @@ -94,34 +97,35 @@ class IdentityCertificatesWatcher private: RefCountedPtr parent_; + std::string cert_name_; }; } // namespace -XdsCertificateProvider::XdsCertificateProvider( - absl::string_view root_cert_name, - RefCountedPtr root_cert_distributor, - absl::string_view identity_cert_name, - RefCountedPtr identity_cert_distributor, - std::vector san_matchers) - : root_cert_name_(root_cert_name), - identity_cert_name_(identity_cert_name), - root_cert_distributor_(std::move(root_cert_distributor)), - identity_cert_distributor_(std::move(identity_cert_distributor)), - san_matchers_(std::move(san_matchers)), - distributor_(MakeRefCounted()) { - distributor_->SetWatchStatusCallback( - absl::bind_front(&XdsCertificateProvider::WatchStatusCallback, this)); +// +// XdsCertificateProvider::ClusterCertificateState +// + +XdsCertificateProvider::ClusterCertificateState::~ClusterCertificateState() { + if (root_cert_watcher_ != nullptr) { + root_cert_distributor_->CancelTlsCertificatesWatch(root_cert_watcher_); + } + if (identity_cert_watcher_ != nullptr) { + identity_cert_distributor_->CancelTlsCertificatesWatch( + identity_cert_watcher_); + } } -XdsCertificateProvider::~XdsCertificateProvider() { - distributor_->SetWatchStatusCallback(nullptr); +bool XdsCertificateProvider::ClusterCertificateState::IsSafeToRemove() const { + return !watching_root_certs_ && !watching_identity_certs_ && + root_cert_distributor_ == nullptr && + identity_cert_distributor_ == nullptr; } -void XdsCertificateProvider::UpdateRootCertNameAndDistributor( - absl::string_view root_cert_name, - RefCountedPtr root_cert_distributor) { - MutexLock lock(&mu_); +void XdsCertificateProvider::ClusterCertificateState:: + UpdateRootCertNameAndDistributor( + const std::string& cert_name, absl::string_view root_cert_name, + RefCountedPtr root_cert_distributor) { if (root_cert_name_ == root_cert_name && root_cert_distributor_ == root_cert_distributor) { return; @@ -133,10 +137,10 @@ void XdsCertificateProvider::UpdateRootCertNameAndDistributor( root_cert_distributor_->CancelTlsCertificatesWatch(root_cert_watcher_); } if (root_cert_distributor != nullptr) { - UpdateRootCertWatcher(root_cert_distributor.get()); + UpdateRootCertWatcher(cert_name, root_cert_distributor.get()); } else { root_cert_watcher_ = nullptr; - distributor_->SetErrorForCert( + xds_certificate_provider_->distributor_->SetErrorForCert( "", GRPC_ERROR_CREATE_FROM_STATIC_STRING( "No certificate provider available for root certificates"), @@ -147,10 +151,11 @@ void XdsCertificateProvider::UpdateRootCertNameAndDistributor( root_cert_distributor_ = std::move(root_cert_distributor); } -void XdsCertificateProvider::UpdateIdentityCertNameAndDistributor( - absl::string_view identity_cert_name, - RefCountedPtr identity_cert_distributor) { - MutexLock lock(&mu_); +void XdsCertificateProvider::ClusterCertificateState:: + UpdateIdentityCertNameAndDistributor( + const std::string& cert_name, absl::string_view identity_cert_name, + RefCountedPtr + identity_cert_distributor) { if (identity_cert_name_ == identity_cert_name && identity_cert_distributor_ == identity_cert_distributor) { return; @@ -163,10 +168,10 @@ void XdsCertificateProvider::UpdateIdentityCertNameAndDistributor( identity_cert_watcher_); } if (identity_cert_distributor != nullptr) { - UpdateIdentityCertWatcher(identity_cert_distributor.get()); + UpdateIdentityCertWatcher(cert_name, identity_cert_distributor.get()); } else { identity_cert_watcher_ = nullptr; - distributor_->SetErrorForCert( + xds_certificate_provider_->distributor_->SetErrorForCert( "", absl::nullopt, GRPC_ERROR_CREATE_FROM_STATIC_STRING( "No certificate provider available for identity certificates")); @@ -176,42 +181,45 @@ void XdsCertificateProvider::UpdateIdentityCertNameAndDistributor( identity_cert_distributor_ = std::move(identity_cert_distributor); } -void XdsCertificateProvider::UpdateSubjectAlternativeNameMatchers( - std::vector matchers) { - MutexLock lock(&san_matchers_mu_); - san_matchers_ = std::move(matchers); +void XdsCertificateProvider::ClusterCertificateState::UpdateRootCertWatcher( + const std::string& cert_name, + grpc_tls_certificate_distributor* root_cert_distributor) { + auto watcher = absl::make_unique( + xds_certificate_provider_->distributor_, cert_name); + root_cert_watcher_ = watcher.get(); + root_cert_distributor->WatchTlsCertificates(std::move(watcher), + root_cert_name_, absl::nullopt); } -void XdsCertificateProvider::WatchStatusCallback(std::string cert_name, - bool root_being_watched, - bool identity_being_watched) { +void XdsCertificateProvider::ClusterCertificateState::UpdateIdentityCertWatcher( + const std::string& cert_name, + grpc_tls_certificate_distributor* identity_cert_distributor) { + auto watcher = absl::make_unique( + xds_certificate_provider_->distributor_, cert_name); + identity_cert_watcher_ = watcher.get(); + identity_cert_distributor->WatchTlsCertificates( + std::move(watcher), absl::nullopt, identity_cert_name_); +} + +void XdsCertificateProvider::ClusterCertificateState::WatchStatusCallback( + const std::string& cert_name, bool root_being_watched, + bool identity_being_watched) { // We aren't specially handling the case where root_cert_distributor is same // as identity_cert_distributor. Always using two separate watchers // irrespective of the fact results in a straightforward design, and using a // single watcher does not seem to provide any benefit other than cutting down // on the number of callbacks. - MutexLock lock(&mu_); - if (!cert_name.empty()) { - grpc_error* error = GRPC_ERROR_CREATE_FROM_COPIED_STRING( - absl::StrCat("Illegal certificate name: \'", cert_name, - "\'. Should be empty.") - .c_str()); - distributor_->SetErrorForCert(cert_name, GRPC_ERROR_REF(error), - GRPC_ERROR_REF(error)); - GRPC_ERROR_UNREF(error); - return; - } if (root_being_watched && !watching_root_certs_) { // We need to start watching root certs. watching_root_certs_ = true; if (root_cert_distributor_ == nullptr) { - distributor_->SetErrorForCert( - "", + xds_certificate_provider_->distributor_->SetErrorForCert( + cert_name, GRPC_ERROR_CREATE_FROM_STATIC_STRING( "No certificate provider available for root certificates"), absl::nullopt); } else { - UpdateRootCertWatcher(root_cert_distributor_.get()); + UpdateRootCertWatcher(cert_name, root_cert_distributor_.get()); } } else if (!root_being_watched && watching_root_certs_) { // We need to cancel root certs watch. @@ -225,12 +233,12 @@ void XdsCertificateProvider::WatchStatusCallback(std::string cert_name, if (identity_being_watched && !watching_identity_certs_) { watching_identity_certs_ = true; if (identity_cert_distributor_ == nullptr) { - distributor_->SetErrorForCert( - "", absl::nullopt, + xds_certificate_provider_->distributor_->SetErrorForCert( + cert_name, absl::nullopt, GRPC_ERROR_CREATE_FROM_STATIC_STRING( "No certificate provider available for identity certificates")); } else { - UpdateIdentityCertWatcher(identity_cert_distributor_.get()); + UpdateIdentityCertWatcher(cert_name, identity_cert_distributor_.get()); } } else if (!identity_being_watched && watching_identity_certs_) { watching_identity_certs_ = false; @@ -243,20 +251,102 @@ void XdsCertificateProvider::WatchStatusCallback(std::string cert_name, } } -void XdsCertificateProvider::UpdateRootCertWatcher( - grpc_tls_certificate_distributor* root_cert_distributor) { - auto watcher = absl::make_unique(distributor()); - root_cert_watcher_ = watcher.get(); - root_cert_distributor->WatchTlsCertificates(std::move(watcher), - root_cert_name_, absl::nullopt); +// +// XdsCertificateProvider +// + +XdsCertificateProvider::XdsCertificateProvider() + : distributor_(MakeRefCounted()) { + distributor_->SetWatchStatusCallback( + absl::bind_front(&XdsCertificateProvider::WatchStatusCallback, this)); } -void XdsCertificateProvider::UpdateIdentityCertWatcher( - grpc_tls_certificate_distributor* identity_cert_distributor) { - auto watcher = absl::make_unique(distributor()); - identity_cert_watcher_ = watcher.get(); - identity_cert_distributor->WatchTlsCertificates( - std::move(watcher), absl::nullopt, identity_cert_name_); +XdsCertificateProvider::~XdsCertificateProvider() { + distributor_->SetWatchStatusCallback(nullptr); +} + +bool XdsCertificateProvider::ProvidesRootCerts(const std::string& cert_name) { + MutexLock lock(&mu_); + auto it = certificate_state_map_.find(cert_name); + if (it == certificate_state_map_.end()) return false; + return it->second->ProvidesRootCerts(); +} + +void XdsCertificateProvider::UpdateRootCertNameAndDistributor( + const std::string& cert_name, absl::string_view root_cert_name, + RefCountedPtr root_cert_distributor) { + MutexLock lock(&mu_); + auto it = certificate_state_map_.find(cert_name); + if (it == certificate_state_map_.end()) { + it = certificate_state_map_ + .emplace(cert_name, + absl::make_unique(Ref())) + .first; + } + it->second->UpdateRootCertNameAndDistributor(cert_name, root_cert_name, + root_cert_distributor); + // Delete unused entries. + if (it->second->IsSafeToRemove()) certificate_state_map_.erase(it); +} + +bool XdsCertificateProvider::ProvidesIdentityCerts( + const std::string& cert_name) { + MutexLock lock(&mu_); + auto it = certificate_state_map_.find(cert_name); + if (it == certificate_state_map_.end()) return false; + return it->second->ProvidesIdentityCerts(); +} + +void XdsCertificateProvider::UpdateIdentityCertNameAndDistributor( + const std::string& cert_name, absl::string_view identity_cert_name, + RefCountedPtr identity_cert_distributor) { + MutexLock lock(&mu_); + auto it = certificate_state_map_.find(cert_name); + if (it == certificate_state_map_.end()) { + it = certificate_state_map_ + .emplace(cert_name, + absl::make_unique(Ref())) + .first; + } + it->second->UpdateIdentityCertNameAndDistributor( + cert_name, identity_cert_name, identity_cert_distributor); + // Delete unused entries. + if (it->second->IsSafeToRemove()) certificate_state_map_.erase(it); +} + +std::vector XdsCertificateProvider::GetSanMatchers( + const std::string& cluster) { + MutexLock lock(&san_matchers_mu_); + auto it = san_matcher_map_.find(cluster); + if (it == san_matcher_map_.end()) return {}; + return it->second; +} + +void XdsCertificateProvider::UpdateSubjectAlternativeNameMatchers( + const std::string& cluster, std::vector matchers) { + MutexLock lock(&san_matchers_mu_); + if (matchers.empty()) { + san_matcher_map_.erase(cluster); + } else { + san_matcher_map_[cluster] = std::move(matchers); + } +} + +void XdsCertificateProvider::WatchStatusCallback(std::string cert_name, + bool root_being_watched, + bool identity_being_watched) { + MutexLock lock(&mu_); + auto it = certificate_state_map_.find(cert_name); + if (it == certificate_state_map_.end()) { + it = certificate_state_map_ + .emplace(cert_name, + absl::make_unique(Ref())) + .first; + } + it->second->WatchStatusCallback(cert_name, root_being_watched, + identity_being_watched); + // Delete unused entries. + if (it->second->IsSafeToRemove()) certificate_state_map_.erase(it); } namespace { diff --git a/src/core/ext/xds/xds_certificate_provider.h b/src/core/ext/xds/xds_certificate_provider.h index 4d13423a6e5..6fbedb13b55 100644 --- a/src/core/ext/xds/xds_certificate_provider.h +++ b/src/core/ext/xds/xds_certificate_provider.h @@ -31,44 +31,28 @@ namespace grpc_core { class XdsCertificateProvider : public grpc_tls_certificate_provider { public: - XdsCertificateProvider( - absl::string_view root_cert_name, - RefCountedPtr root_cert_distributor, - absl::string_view identity_cert_name, - RefCountedPtr identity_cert_distributor, - std::vector san_matchers); - + XdsCertificateProvider(); ~XdsCertificateProvider() override; - void UpdateRootCertNameAndDistributor( - absl::string_view root_cert_name, - RefCountedPtr root_cert_distributor); - void UpdateIdentityCertNameAndDistributor( - absl::string_view identity_cert_name, - RefCountedPtr - identity_cert_distributor); - void UpdateSubjectAlternativeNameMatchers( - std::vector matchers); - grpc_core::RefCountedPtr distributor() const override { return distributor_; } - bool ProvidesRootCerts() { - MutexLock lock(&mu_); - return root_cert_distributor_ != nullptr; - } + bool ProvidesRootCerts(const std::string& cert_name); + void UpdateRootCertNameAndDistributor( + const std::string& cert_name, absl::string_view root_cert_name, + RefCountedPtr root_cert_distributor); - bool ProvidesIdentityCerts() { - MutexLock lock(&mu_); - return identity_cert_distributor_ != nullptr; - } + bool ProvidesIdentityCerts(const std::string& cert_name); + void UpdateIdentityCertNameAndDistributor( + const std::string& cert_name, absl::string_view identity_cert_name, + RefCountedPtr + identity_cert_distributor); - std::vector subject_alternative_name_matchers() { - MutexLock lock(&san_matchers_mu_); - return san_matchers_; - } + std::vector GetSanMatchers(const std::string& cluster); + void UpdateSubjectAlternativeNameMatchers( + const std::string& cluster, std::vector matchers); grpc_arg MakeChannelArg() const; @@ -76,14 +60,63 @@ class XdsCertificateProvider : public grpc_tls_certificate_provider { const grpc_channel_args* args); private: + class ClusterCertificateState { + public: + explicit ClusterCertificateState( + RefCountedPtr xds_certificate_provider) + : xds_certificate_provider_(std::move(xds_certificate_provider)) {} + + ~ClusterCertificateState(); + + // Returns true if the certs aren't being watched and there are no + // distributors configured. + bool IsSafeToRemove() const; + + bool ProvidesRootCerts() const { return root_cert_distributor_ != nullptr; } + bool ProvidesIdentityCerts() const { + return identity_cert_distributor_ != nullptr; + } + + void UpdateRootCertNameAndDistributor( + const std::string& cert_name, absl::string_view root_cert_name, + RefCountedPtr root_cert_distributor); + void UpdateIdentityCertNameAndDistributor( + const std::string& cert_name, absl::string_view identity_cert_name, + RefCountedPtr + identity_cert_distributor); + + void UpdateRootCertWatcher( + const std::string& cert_name, + grpc_tls_certificate_distributor* root_cert_distributor); + void UpdateIdentityCertWatcher( + const std::string& cert_name, + grpc_tls_certificate_distributor* identity_cert_distributor); + + void WatchStatusCallback(const std::string& cert_name, + bool root_being_watched, + bool identity_being_watched); + + private: + RefCountedPtr xds_certificate_provider_; + bool watching_root_certs_ = false; + bool watching_identity_certs_ = false; + std::string root_cert_name_; + std::string identity_cert_name_; + RefCountedPtr root_cert_distributor_; + RefCountedPtr identity_cert_distributor_; + grpc_tls_certificate_distributor::TlsCertificatesWatcherInterface* + root_cert_watcher_ = nullptr; + grpc_tls_certificate_distributor::TlsCertificatesWatcherInterface* + identity_cert_watcher_ = nullptr; + }; + void WatchStatusCallback(std::string cert_name, bool root_being_watched, bool identity_being_watched); - void UpdateRootCertWatcher( - grpc_tls_certificate_distributor* root_cert_distributor); - void UpdateIdentityCertWatcher( - grpc_tls_certificate_distributor* identity_cert_distributor); Mutex mu_; + std::map> + certificate_state_map_; + // Use a separate mutex for san_matchers_ to avoid deadlocks since // san_matchers_ needs to be accessed when a handshake is being done and we // run into a possible deadlock scenario if using the same mutex. The mutex @@ -93,18 +126,10 @@ class XdsCertificateProvider : public grpc_tls_certificate_provider { // -> HandshakeManager::Add() -> SecurityHandshaker::DoHandshake() -> // subject_alternative_names_matchers() Mutex san_matchers_mu_; - bool watching_root_certs_ = false; - bool watching_identity_certs_ = false; - std::string root_cert_name_; - std::string identity_cert_name_; - RefCountedPtr root_cert_distributor_; - RefCountedPtr identity_cert_distributor_; - std::vector san_matchers_; + std::map> + san_matcher_map_; + RefCountedPtr distributor_; - grpc_tls_certificate_distributor::TlsCertificatesWatcherInterface* - root_cert_watcher_ = nullptr; - grpc_tls_certificate_distributor::TlsCertificatesWatcherInterface* - identity_cert_watcher_ = nullptr; }; } // namespace grpc_core diff --git a/src/core/lib/security/credentials/xds/xds_credentials.cc b/src/core/lib/security/credentials/xds/xds_credentials.cc index 9e00f70fda9..cca6a71ce48 100644 --- a/src/core/lib/security/credentials/xds/xds_credentials.cc +++ b/src/core/lib/security/credentials/xds/xds_credentials.cc @@ -20,6 +20,7 @@ #include "src/core/lib/security/credentials/xds/xds_credentials.h" +#include "src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h" #include "src/core/ext/xds/xds_certificate_provider.h" #include "src/core/lib/security/credentials/tls/grpc_tls_credentials_options.h" #include "src/core/lib/security/credentials/tls/tls_credentials.h" @@ -60,32 +61,44 @@ bool XdsVerifySubjectAlternativeNames( return false; } -int ServerAuthCheckSchedule(void* config_user_data, - grpc_tls_server_authorization_check_arg* arg) { - XdsCertificateProvider* xds_certificate_provider = - static_cast(config_user_data); - if (XdsVerifySubjectAlternativeNames( - arg->subject_alternative_names, arg->subject_alternative_names_size, - xds_certificate_provider->subject_alternative_name_matchers())) { - arg->success = 1; - arg->status = GRPC_STATUS_OK; - } else { - arg->success = 0; - arg->status = GRPC_STATUS_UNAUTHENTICATED; - if (arg->error_details) { - arg->error_details->set_error_details( - "SANs from certificate did not match SANs from xDS control plane"); - } +class ServerAuthCheck { + public: + ServerAuthCheck( + RefCountedPtr xds_certificate_provider, + std::string cluster_name) + : xds_certificate_provider_(std::move(xds_certificate_provider)), + cluster_name_(std::move(cluster_name)) {} + + static int Schedule(void* config_user_data, + grpc_tls_server_authorization_check_arg* arg) { + return static_cast(config_user_data)->ScheduleImpl(arg); } - return 0; /* synchronous check */ -} + static void Destroy(void* config_user_data) { + delete static_cast(config_user_data); + } -void ServerAuthCheckDestroy(void* config_user_data) { - XdsCertificateProvider* xds_certificate_provider = - static_cast(config_user_data); - xds_certificate_provider->Unref(); -} + private: + int ScheduleImpl(grpc_tls_server_authorization_check_arg* arg) { + if (XdsVerifySubjectAlternativeNames( + arg->subject_alternative_names, arg->subject_alternative_names_size, + xds_certificate_provider_->GetSanMatchers(cluster_name_))) { + arg->success = 1; + arg->status = GRPC_STATUS_OK; + } else { + arg->success = 0; + arg->status = GRPC_STATUS_UNAUTHENTICATED; + if (arg->error_details) { + arg->error_details->set_error_details( + "SANs from certificate did not match SANs from xDS control plane"); + } + } + return 0; /* synchronous check */ + } + + RefCountedPtr xds_certificate_provider_; + std::string cluster_name_; +}; } // namespace @@ -105,49 +118,79 @@ RefCountedPtr XdsCredentials::create_security_connector( RefCountedPtr call_creds, const char* target_name, const grpc_channel_args* args, grpc_channel_args** new_args) { - auto xds_certificate_provider = - XdsCertificateProvider::GetFromChannelArgs(args); + struct ChannelArgsDeleter { + const grpc_channel_args* args; + bool owned; + ~ChannelArgsDeleter() { + if (owned) grpc_channel_args_destroy(args); + } + }; + ChannelArgsDeleter temp_args{args, false}; // TODO(yashykt): This arg will no longer need to be added after b/173119596 // is fixed. grpc_arg override_arg = grpc_channel_arg_string_create( const_cast(GRPC_SSL_TARGET_NAME_OVERRIDE_ARG), const_cast(target_name)); const char* override_arg_name = GRPC_SSL_TARGET_NAME_OVERRIDE_ARG; - const grpc_channel_args* temp_args = args; if (grpc_channel_args_find(args, override_arg_name) == nullptr) { - temp_args = grpc_channel_args_copy_and_add_and_remove( + temp_args.args = grpc_channel_args_copy_and_add_and_remove( args, &override_arg_name, 1, &override_arg, 1); + temp_args.owned = true; } RefCountedPtr security_connector; + auto xds_certificate_provider = + XdsCertificateProvider::GetFromChannelArgs(args); if (xds_certificate_provider != nullptr) { - auto tls_credentials_options = - MakeRefCounted(); - tls_credentials_options->set_certificate_provider(xds_certificate_provider); - if (xds_certificate_provider->ProvidesRootCerts()) { - tls_credentials_options->set_watch_root_cert(true); - } - if (xds_certificate_provider->ProvidesIdentityCerts()) { - tls_credentials_options->set_watch_identity_pair(true); + std::string cluster_name = + grpc_channel_args_find_string(args, GRPC_ARG_XDS_CLUSTER_NAME); + GPR_ASSERT(cluster_name.data() != nullptr); + const bool watch_root = + xds_certificate_provider->ProvidesRootCerts(cluster_name); + const bool watch_identity = + xds_certificate_provider->ProvidesIdentityCerts(cluster_name); + if (watch_root || watch_identity) { + auto tls_credentials_options = + MakeRefCounted(); + tls_credentials_options->set_certificate_provider( + xds_certificate_provider); + if (watch_root) { + tls_credentials_options->set_watch_root_cert(true); + tls_credentials_options->set_root_cert_name(cluster_name); + } + if (watch_identity) { + tls_credentials_options->set_watch_identity_pair(true); + tls_credentials_options->set_identity_cert_name(cluster_name); + } + tls_credentials_options->set_server_verification_option( + GRPC_TLS_SKIP_HOSTNAME_VERIFICATION); + auto* server_auth_check = new ServerAuthCheck(xds_certificate_provider, + std::move(cluster_name)); + tls_credentials_options->set_server_authorization_check_config( + MakeRefCounted( + server_auth_check, ServerAuthCheck::Schedule, nullptr, + ServerAuthCheck::Destroy)); + // TODO(yashkt): Creating a new TlsCreds object each time we create a + // security connector means that the security connector's cmp() method + // returns unequal for each instance, which means that every time an LB + // policy updates, all the subchannels will be recreated. This is + // going to lead to a lot of connection churn. Instead, we should + // either (a) change the TLS security connector's cmp() method to be + // smarter somehow, so that it compares unequal only when the + // tls_credentials_options have changed, or (b) cache the TlsCreds + // objects in the XdsCredentials object so that we can reuse the + // same one when creating new security connectors, swapping out the + // TlsCreds object only when the tls_credentials_options change. + // Option (a) would probably be better, although it may require some + // structural changes to the security connector API. + auto tls_credentials = + MakeRefCounted(std::move(tls_credentials_options)); + return tls_credentials->create_security_connector( + std::move(call_creds), target_name, temp_args.args, new_args); } - tls_credentials_options->set_server_verification_option( - GRPC_TLS_SKIP_HOSTNAME_VERIFICATION); - tls_credentials_options->set_server_authorization_check_config( - MakeRefCounted( - xds_certificate_provider->Ref().release(), ServerAuthCheckSchedule, - nullptr, ServerAuthCheckDestroy)); - auto tls_credentials = - MakeRefCounted(std::move(tls_credentials_options)); - security_connector = tls_credentials->create_security_connector( - std::move(call_creds), target_name, temp_args, new_args); - } else { - GPR_ASSERT(fallback_credentials_ != nullptr); - security_connector = fallback_credentials_->create_security_connector( - std::move(call_creds), target_name, temp_args, new_args); - } - if (temp_args != args) { - grpc_channel_args_destroy(temp_args); } - return security_connector; + GPR_ASSERT(fallback_credentials_ != nullptr); + return fallback_credentials_->create_security_connector( + std::move(call_creds), target_name, temp_args.args, new_args); } // diff --git a/test/core/xds/xds_certificate_provider_test.cc b/test/core/xds/xds_certificate_provider_test.cc index fff1c48d47a..8f6c8f3ab49 100644 --- a/test/core/xds/xds_certificate_provider_test.cc +++ b/test/core/xds/xds_certificate_provider_test.cc @@ -105,8 +105,10 @@ TEST( MakeRefCounted(); auto identity_cert_distributor = MakeRefCounted(); - XdsCertificateProvider provider("root", root_cert_distributor, "identity", - identity_cert_distributor, {}); + XdsCertificateProvider provider; + provider.UpdateRootCertNameAndDistributor("", "root", root_cert_distributor); + provider.UpdateIdentityCertNameAndDistributor("", "identity", + identity_cert_distributor); auto* watcher = new TestCertificatesWatcher; provider.distributor()->WatchTlsCertificates( std::unique_ptr(watcher), "", ""); @@ -174,8 +176,10 @@ TEST(XdsCertificateProviderTest, MakeRefCounted(); auto identity_cert_distributor = MakeRefCounted(); - XdsCertificateProvider provider("test", root_cert_distributor, "test", - identity_cert_distributor, {}); + XdsCertificateProvider provider; + provider.UpdateRootCertNameAndDistributor("", "test", root_cert_distributor); + provider.UpdateIdentityCertNameAndDistributor("", "test", + identity_cert_distributor); auto* watcher = new TestCertificatesWatcher; provider.distributor()->WatchTlsCertificates( std::unique_ptr(watcher), "", ""); @@ -242,8 +246,9 @@ TEST(XdsCertificateProviderTest, TEST(XdsCertificateProviderTest, RootCertDistributorSameAsIdentityCertDistributorDifferentCertNames) { auto distributor = MakeRefCounted(); - XdsCertificateProvider provider("root", distributor, "identity", distributor, - {}); + XdsCertificateProvider provider; + provider.UpdateRootCertNameAndDistributor("", "root", distributor); + provider.UpdateIdentityCertNameAndDistributor("", "identity", distributor); auto* watcher = new TestCertificatesWatcher; provider.distributor()->WatchTlsCertificates( std::unique_ptr(watcher), "", ""); @@ -306,7 +311,9 @@ TEST(XdsCertificateProviderTest, TEST(XdsCertificateProviderTest, RootCertDistributorSameAsIdentityCertDistributorSameCertNames) { auto distributor = MakeRefCounted(); - XdsCertificateProvider provider("", distributor, "", distributor, {}); + XdsCertificateProvider provider; + provider.UpdateRootCertNameAndDistributor("", "", distributor); + provider.UpdateIdentityCertNameAndDistributor("", "", distributor); auto* watcher = new TestCertificatesWatcher; provider.distributor()->WatchTlsCertificates( std::unique_ptr(watcher), "", ""); @@ -369,7 +376,7 @@ TEST(XdsCertificateProviderTest, TEST(XdsCertificateProviderTest, SwapOutDistributorsMultipleTimes) { auto distributor = MakeRefCounted(); distributor->SetKeyMaterials("", kRootCert1, MakeKeyCertPairsType1()); - XdsCertificateProvider provider("", nullptr, "", nullptr, {}); + XdsCertificateProvider provider; auto* watcher = new TestCertificatesWatcher; provider.distributor()->WatchTlsCertificates( std::unique_ptr(watcher), "", ""); @@ -384,7 +391,7 @@ TEST(XdsCertificateProviderTest, SwapOutDistributorsMultipleTimes) { ::testing::HasSubstr( "No certificate provider available for identity certificates")); // Update root cert distributor. - provider.UpdateRootCertNameAndDistributor("", distributor); + provider.UpdateRootCertNameAndDistributor("", "", distributor); EXPECT_EQ(watcher->root_certs(), kRootCert1); EXPECT_EQ(watcher->key_cert_pairs(), absl::nullopt); EXPECT_EQ(watcher->root_cert_error(), GRPC_ERROR_NONE); @@ -393,7 +400,7 @@ TEST(XdsCertificateProviderTest, SwapOutDistributorsMultipleTimes) { ::testing::HasSubstr( "No certificate provider available for identity certificates")); // Update identity cert distributor - provider.UpdateIdentityCertNameAndDistributor("", distributor); + provider.UpdateIdentityCertNameAndDistributor("", "", distributor); EXPECT_EQ(watcher->root_certs(), kRootCert1); EXPECT_EQ(watcher->key_cert_pairs(), MakeKeyCertPairsType1()); EXPECT_EQ(watcher->root_cert_error(), GRPC_ERROR_NONE); @@ -421,7 +428,7 @@ TEST(XdsCertificateProviderTest, SwapOutDistributorsMultipleTimes) { EXPECT_EQ(watcher->root_cert_error(), GRPC_ERROR_NONE); EXPECT_EQ(watcher->identity_cert_error(), GRPC_ERROR_NONE); // Remove root cert provider - provider.UpdateRootCertNameAndDistributor("", nullptr); + provider.UpdateRootCertNameAndDistributor("", "", nullptr); distributor->SetKeyMaterials("", kRootCert2, MakeKeyCertPairsType2()); EXPECT_EQ(watcher->root_certs(), kRootCert1); // not updated EXPECT_EQ(watcher->key_cert_pairs(), MakeKeyCertPairsType2()); @@ -430,7 +437,7 @@ TEST(XdsCertificateProviderTest, SwapOutDistributorsMultipleTimes) { "No certificate provider available for root certificates")); EXPECT_EQ(watcher->identity_cert_error(), GRPC_ERROR_NONE); // Remove identity cert provider too - provider.UpdateIdentityCertNameAndDistributor("", nullptr); + provider.UpdateIdentityCertNameAndDistributor("", "", nullptr); distributor->SetKeyMaterials("", kRootCert1, MakeKeyCertPairsType1()); EXPECT_EQ(watcher->root_certs(), kRootCert1); EXPECT_EQ(watcher->key_cert_pairs(), MakeKeyCertPairsType2()); // not updated @@ -442,8 +449,8 @@ TEST(XdsCertificateProviderTest, SwapOutDistributorsMultipleTimes) { ::testing::HasSubstr( "No certificate provider available for identity certificates")); // Change certificate names being watched, without any certificate updates. - provider.UpdateRootCertNameAndDistributor("root", distributor); - provider.UpdateIdentityCertNameAndDistributor("identity", distributor); + provider.UpdateRootCertNameAndDistributor("", "root", distributor); + provider.UpdateIdentityCertNameAndDistributor("", "identity", distributor); EXPECT_EQ(watcher->root_certs(), kRootCert1); EXPECT_EQ(watcher->key_cert_pairs(), MakeKeyCertPairsType2()); EXPECT_THAT(grpc_error_string(watcher->root_cert_error()), @@ -467,16 +474,16 @@ TEST(XdsCertificateProviderTest, SwapOutDistributorsMultipleTimes) { MakeRefCounted(); auto identity_cert_distributor = MakeRefCounted(); - provider.UpdateRootCertNameAndDistributor("root", root_cert_distributor); - provider.UpdateIdentityCertNameAndDistributor("identity", + provider.UpdateRootCertNameAndDistributor("", "root", root_cert_distributor); + provider.UpdateIdentityCertNameAndDistributor("", "identity", identity_cert_distributor); EXPECT_EQ(watcher->root_certs(), kRootCert2); EXPECT_EQ(watcher->key_cert_pairs(), MakeKeyCertPairsType1()); EXPECT_EQ(watcher->root_cert_error(), GRPC_ERROR_NONE); EXPECT_EQ(watcher->identity_cert_error(), GRPC_ERROR_NONE); // Change certificate names without any certificate updates. - provider.UpdateRootCertNameAndDistributor("test", root_cert_distributor); - provider.UpdateIdentityCertNameAndDistributor("test", + provider.UpdateRootCertNameAndDistributor("", "test", root_cert_distributor); + provider.UpdateIdentityCertNameAndDistributor("", "test", identity_cert_distributor); EXPECT_EQ(watcher->root_certs(), kRootCert2); EXPECT_EQ(watcher->key_cert_pairs(), MakeKeyCertPairsType1()); @@ -493,15 +500,70 @@ TEST(XdsCertificateProviderTest, SwapOutDistributorsMultipleTimes) { EXPECT_EQ(watcher->identity_cert_error(), GRPC_ERROR_NONE); } -TEST(XdsCertificateProviderTest, CertificateNameNotEmpty) { - XdsCertificateProvider provider("", nullptr, "", nullptr, {}); +TEST(XdsCertificateProviderTest, MultipleCertNames) { + XdsCertificateProvider provider; + // Start watch for "test1". There are no underlying distributors for + // that cert name, so it will return an error. + auto* watcher1 = new TestCertificatesWatcher; + provider.distributor()->WatchTlsCertificates( + std::unique_ptr(watcher1), "test1", "test1"); + EXPECT_EQ(watcher1->root_certs(), absl::nullopt); + EXPECT_EQ(watcher1->key_cert_pairs(), absl::nullopt); + EXPECT_THAT(grpc_error_string(watcher1->root_cert_error()), + ::testing::HasSubstr( + "No certificate provider available for root certificates")); + EXPECT_THAT( + grpc_error_string(watcher1->identity_cert_error()), + ::testing::HasSubstr( + "No certificate provider available for identity certificates")); + // Add distributor for "test1". This will return data to the watcher. + auto cert_distributor1 = MakeRefCounted(); + cert_distributor1->SetKeyMaterials("root", kRootCert1, absl::nullopt); + cert_distributor1->SetKeyMaterials("identity", absl::nullopt, + MakeKeyCertPairsType1()); + provider.UpdateRootCertNameAndDistributor("test1", "root", cert_distributor1); + provider.UpdateIdentityCertNameAndDistributor("test1", "identity", + cert_distributor1); + EXPECT_EQ(watcher1->root_certs(), kRootCert1); + EXPECT_EQ(watcher1->key_cert_pairs(), MakeKeyCertPairsType1()); + EXPECT_EQ(watcher1->root_cert_error(), GRPC_ERROR_NONE); + EXPECT_EQ(watcher1->identity_cert_error(), GRPC_ERROR_NONE); + // Add distributor for "test2". + auto cert_distributor2 = MakeRefCounted(); + cert_distributor2->SetKeyMaterials("root2", kRootCert2, absl::nullopt); + cert_distributor2->SetKeyMaterials("identity2", absl::nullopt, + MakeKeyCertPairsType2()); + provider.UpdateRootCertNameAndDistributor("test2", "root2", + cert_distributor2); + provider.UpdateIdentityCertNameAndDistributor("test2", "identity2", + cert_distributor2); + // Add watcher for "test2". This one should return data immediately. + auto* watcher2 = new TestCertificatesWatcher; + provider.distributor()->WatchTlsCertificates( + std::unique_ptr(watcher2), "test2", "test2"); + EXPECT_EQ(watcher2->root_certs(), kRootCert2); + EXPECT_EQ(watcher2->key_cert_pairs(), MakeKeyCertPairsType2()); + EXPECT_EQ(watcher2->root_cert_error(), GRPC_ERROR_NONE); + EXPECT_EQ(watcher2->identity_cert_error(), GRPC_ERROR_NONE); + // The presence of "test2" should not affect "test1". + EXPECT_EQ(watcher1->root_certs(), kRootCert1); + EXPECT_EQ(watcher1->key_cert_pairs(), MakeKeyCertPairsType1()); + EXPECT_EQ(watcher1->root_cert_error(), GRPC_ERROR_NONE); + EXPECT_EQ(watcher1->identity_cert_error(), GRPC_ERROR_NONE); +} + +TEST(XdsCertificateProviderTest, UnknownCertName) { + XdsCertificateProvider provider; auto* watcher = new TestCertificatesWatcher; provider.distributor()->WatchTlsCertificates( std::unique_ptr(watcher), "test", "test"); EXPECT_THAT(grpc_error_string(watcher->root_cert_error()), - ::testing::HasSubstr("Illegal certificate name: \'test\'")); - EXPECT_THAT(grpc_error_string(watcher->identity_cert_error()), - ::testing::HasSubstr("Illegal certificate name: \'test\'")); + ::testing::HasSubstr( + "No certificate provider available for root certificates")); + EXPECT_THAT( + grpc_error_string(watcher->identity_cert_error()), + ::testing::HasSubstr( + "No certificate provider available for identity certificates")); } } // namespace diff --git a/test/cpp/end2end/xds_end2end_test.cc b/test/cpp/end2end/xds_end2end_test.cc index d408f345f48..323f42101e2 100644 --- a/test/cpp/end2end/xds_end2end_test.cc +++ b/test/cpp/end2end/xds_end2end_test.cc @@ -5453,7 +5453,7 @@ class XdsSecurityTest : public BasicTest { } } } - EXPECT_TRUE(num_tries < kRetryCount); + EXPECT_LT(num_tries, kRetryCount); } std::string root_cert_;