From 26d50f726ad407c6f7f289e4de41ae549a241bed Mon Sep 17 00:00:00 2001 From: matthewstevenson88 <52979934+matthewstevenson88@users.noreply.github.com> Date: Thu, 23 Jan 2020 12:34:43 -0800 Subject: [PATCH] Revert "Collect TLS-specific changes from PR 20568." --- .../grpcpp/security/tls_credentials_options.h | 53 +++++++++------- src/cpp/common/tls_credentials_options.cc | 62 +++++-------------- test/cpp/client/credentials_test.cc | 47 ++++---------- 3 files changed, 57 insertions(+), 105 deletions(-) diff --git a/include/grpcpp/security/tls_credentials_options.h b/include/grpcpp/security/tls_credentials_options.h index d83226865d8..b5bb7c78b7f 100644 --- a/include/grpcpp/security/tls_credentials_options.h +++ b/include/grpcpp/security/tls_credentials_options.h @@ -55,13 +55,12 @@ class TlsKeyMaterialsConfig { } int version() const { return version_; } - /** Setter for key materials that will be called by the user. Ownership of the - * arguments will not be transferred. **/ - void set_pem_root_certs(const grpc::string& pem_root_certs); + /** Setter for key materials that will be called by the user. The setter + * transfers ownership of the arguments to the config. **/ + void set_pem_root_certs(grpc::string pem_root_certs); void add_pem_key_cert_pair(const PemKeyCertPair& pem_key_cert_pair); - void set_key_materials( - const grpc::string& pem_root_certs, - const std::vector& pem_key_cert_pair_list); + void set_key_materials(grpc::string pem_root_certs, + std::vector pem_key_cert_pair_list); void set_version(int version) { version_ = version; }; private: @@ -71,36 +70,40 @@ class TlsKeyMaterialsConfig { }; /** TLS credential reload arguments, wraps grpc_tls_credential_reload_arg. It is - * used for experimental purposes for now and it is subject to change. + * used for experimental purposes for now and it is subject to change. * - * The credential reload arg contains all the info necessary to schedule/cancel - * a credential reload request. The callback function must be called after - * finishing the schedule operation. See the description of the - * grpc_tls_credential_reload_arg struct in grpc_security.h for more details. + * The credential reload arg contains all the info necessary to schedule/cancel + * a credential reload request. The callback function must be called after + * finishing the schedule operation. See the description of the + * grpc_tls_credential_reload_arg struct in grpc_security.h for more details. * **/ class TlsCredentialReloadArg { public: /** TlsCredentialReloadArg does not take ownership of the C arg that is passed - * to the constructor. One must remember to free any memory allocated to the - * C arg after using the setter functions below. **/ + * to the constructor. One must remember to free any memory allocated to the C + * arg after using the setter functions below. **/ TlsCredentialReloadArg(grpc_tls_credential_reload_arg* arg); ~TlsCredentialReloadArg(); - /** Getters for member fields. **/ + /** Getters for member fields. The callback function is not exposed. + * They return the corresponding fields of the underlying C arg. In the case + * of the key materials config, it creates a new instance of the C++ key + * materials config from the underlying C grpc_tls_key_materials_config. **/ void* cb_user_data() const; bool is_pem_key_cert_pair_list_empty() const; grpc_ssl_certificate_config_reload_status status() const; grpc::string error_details() const; - /** Setters for member fields. Ownership of the arguments will not be - * transferred. **/ + /** Setters for member fields. They modify the fields of the underlying C arg. + * The setters for the key_materials_config and the error_details allocate + * memory when modifying c_arg_, so one must remember to free c_arg_'s + * original key_materials_config or error_details after using the appropriate + * setter function. + * **/ void set_cb_user_data(void* cb_user_data); void set_pem_root_certs(const grpc::string& pem_root_certs); void add_pem_key_cert_pair( - const TlsKeyMaterialsConfig::PemKeyCertPair& pem_key_cert_pair); - void set_key_materials(const grpc::string& pem_root_certs, - std::vector - pem_key_cert_pair_list); + TlsKeyMaterialsConfig::PemKeyCertPair pem_key_cert_pair); void set_key_materials_config( const std::shared_ptr& key_materials_config); void set_status(grpc_ssl_certificate_config_reload_status status); @@ -184,7 +187,8 @@ class TlsServerAuthorizationCheckArg { TlsServerAuthorizationCheckArg(grpc_tls_server_authorization_check_arg* arg); ~TlsServerAuthorizationCheckArg(); - /** Getters for member fields. **/ + /** Getters for member fields. They return the corresponding fields of the + * underlying C arg.**/ void* cb_user_data() const; int success() const; grpc::string target_name() const; @@ -193,7 +197,12 @@ class TlsServerAuthorizationCheckArg { grpc_status_code status() const; grpc::string error_details() const; - /** Setters for member fields. **/ + /** Setters for member fields. They modify the fields of the underlying C arg. + * The setters for target_name, peer_cert, and error_details allocate memory + * when modifying c_arg_, so one must remember to free c_arg_'s original + * target_name, peer_cert, or error_details after using the appropriate setter + * function. + * **/ void set_cb_user_data(void* cb_user_data); void set_success(int success); void set_target_name(const grpc::string& target_name); diff --git a/src/cpp/common/tls_credentials_options.cc b/src/cpp/common/tls_credentials_options.cc index 1c65ea66d89..d5692c6effd 100644 --- a/src/cpp/common/tls_credentials_options.cc +++ b/src/cpp/common/tls_credentials_options.cc @@ -16,18 +16,19 @@ * */ -#include #include #include "src/core/lib/security/credentials/tls/grpc_tls_credentials_options.h" + +#include + #include "src/cpp/common/tls_credentials_options_util.h" namespace grpc_impl { namespace experimental { /** TLS key materials config API implementation **/ -void TlsKeyMaterialsConfig::set_pem_root_certs( - const grpc::string& pem_root_certs) { - pem_root_certs_ = pem_root_certs; +void TlsKeyMaterialsConfig::set_pem_root_certs(grpc::string pem_root_certs) { + pem_root_certs_ = std::move(pem_root_certs); } void TlsKeyMaterialsConfig::add_pem_key_cert_pair( @@ -36,10 +37,10 @@ void TlsKeyMaterialsConfig::add_pem_key_cert_pair( } void TlsKeyMaterialsConfig::set_key_materials( - const grpc::string& pem_root_certs, - const std::vector& pem_key_cert_pair_list) { - pem_key_cert_pair_list_ = pem_key_cert_pair_list; - pem_root_certs_ = pem_root_certs; + grpc::string pem_root_certs, + std::vector pem_key_cert_pair_list) { + pem_key_cert_pair_list_ = std::move(pem_key_cert_pair_list); + pem_root_certs_ = std::move(pem_root_certs); } /** TLS credential reload arg API implementation **/ @@ -58,6 +59,7 @@ TlsCredentialReloadArg::~TlsCredentialReloadArg() {} void* TlsCredentialReloadArg::cb_user_data() const { return c_arg_->cb_user_data; } + bool TlsCredentialReloadArg::is_pem_key_cert_pair_list_empty() const { return c_arg_->key_materials_config->pem_key_cert_pair_list().empty(); } @@ -83,46 +85,17 @@ void TlsCredentialReloadArg::set_pem_root_certs( c_arg_->key_materials_config->set_pem_root_certs(std::move(c_pem_root_certs)); } -namespace { - -::grpc_core::PemKeyCertPair ConvertToCorePemKeyCertPair( - const TlsKeyMaterialsConfig::PemKeyCertPair& pem_key_cert_pair) { +void TlsCredentialReloadArg::add_pem_key_cert_pair( + TlsKeyMaterialsConfig::PemKeyCertPair pem_key_cert_pair) { grpc_ssl_pem_key_cert_pair* ssl_pair = (grpc_ssl_pem_key_cert_pair*)gpr_malloc( sizeof(grpc_ssl_pem_key_cert_pair)); ssl_pair->private_key = gpr_strdup(pem_key_cert_pair.private_key.c_str()); ssl_pair->cert_chain = gpr_strdup(pem_key_cert_pair.cert_chain.c_str()); - return ::grpc_core::PemKeyCertPair(ssl_pair); -} - -} // namespace - -void TlsCredentialReloadArg::add_pem_key_cert_pair( - const TlsKeyMaterialsConfig::PemKeyCertPair& pem_key_cert_pair) { + ::grpc_core::PemKeyCertPair c_pem_key_cert_pair = + ::grpc_core::PemKeyCertPair(ssl_pair); c_arg_->key_materials_config->add_pem_key_cert_pair( - ConvertToCorePemKeyCertPair(pem_key_cert_pair)); -} - -void TlsCredentialReloadArg::set_key_materials( - const grpc::string& pem_root_certs, - std::vector pem_key_cert_pair_list) { - /** Initialize the |key_materials_config| field of |c_arg_|, if it has not - * already been done. **/ - if (c_arg_->key_materials_config == nullptr) { - c_arg_->key_materials_config = grpc_tls_key_materials_config_create(); - } - /** Convert |pem_key_cert_pair_list| to an inlined vector of ssl pairs. **/ - ::grpc_core::InlinedVector<::grpc_core::PemKeyCertPair, 1> - c_pem_key_cert_pair_list; - for (const auto& key_cert_pair : pem_key_cert_pair_list) { - c_pem_key_cert_pair_list.emplace_back( - ConvertToCorePemKeyCertPair(key_cert_pair)); - } - /** Populate the key materials config field of |c_arg_|. **/ - ::grpc_core::UniquePtr c_pem_root_certs( - gpr_strdup(pem_root_certs.c_str())); - c_arg_->key_materials_config->set_key_materials(std::move(c_pem_root_certs), - c_pem_key_cert_pair_list); + std::move(c_pem_key_cert_pair)); } void TlsCredentialReloadArg::set_key_materials_config( @@ -315,11 +288,6 @@ TlsCredentialsOptions::TlsCredentialsOptions( c_credentials_options_, server_verification_option); } -/** Whenever a TlsCredentialsOptions instance is created, the caller takes - * ownership of the c_credentials_options_ pointer (see e.g. the implementation - * of the TlsCredentials API in secure_credentials.cc). For this reason, the - * TlsCredentialsOptions destructor is not responsible for freeing - * c_credentials_options_. **/ TlsCredentialsOptions::~TlsCredentialsOptions() {} } // namespace experimental diff --git a/test/cpp/client/credentials_test.cc b/test/cpp/client/credentials_test.cc index 8d4922e961e..3cc0ebfb8e9 100644 --- a/test/cpp/client/credentials_test.cc +++ b/test/cpp/client/credentials_test.cc @@ -17,7 +17,6 @@ */ #include -#include #include #include @@ -54,10 +53,10 @@ static void tls_credential_reload_callback( class TestTlsCredentialReload : public TlsCredentialReloadInterface { int Schedule(TlsCredentialReloadArg* arg) override { GPR_ASSERT(arg != nullptr); - TlsKeyMaterialsConfig::PemKeyCertPair pair = {"private_key3", - "cert_chain3"}; + struct TlsKeyMaterialsConfig::PemKeyCertPair pair3 = {"private_key3", + "cert_chain3"}; arg->set_pem_root_certs("new_pem_root_certs"); - arg->add_pem_key_cert_pair(pair); + arg->add_pem_key_cert_pair(pair3); arg->set_status(GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_NEW); return 0; } @@ -101,6 +100,7 @@ class TestTlsServerAuthorizationCheck arg->set_error_details("cancelled"); } }; + } // namespace namespace grpc { @@ -293,7 +293,8 @@ TEST_F(CredentialsTest, TlsKeyMaterialsConfigCppToC) { TEST_F(CredentialsTest, TlsKeyMaterialsModifiers) { std::shared_ptr config(new TlsKeyMaterialsConfig()); - TlsKeyMaterialsConfig::PemKeyCertPair pair = {"private_key", "cert_chain"}; + struct TlsKeyMaterialsConfig::PemKeyCertPair pair = {"private_key", + "cert_chain"}; config->add_pem_key_cert_pair(pair); config->set_pem_root_certs("pem_root_certs"); EXPECT_STREQ(config->pem_root_certs().c_str(), "pem_root_certs"); @@ -311,28 +312,15 @@ typedef class ::grpc_impl::experimental::TlsCredentialReloadConfig TEST_F(CredentialsTest, TlsCredentialReloadArgCallback) { grpc_tls_credential_reload_arg* c_arg = new grpc_tls_credential_reload_arg; - c_arg->key_materials_config = grpc_tls_key_materials_config_create(); c_arg->cb = tls_credential_reload_callback; c_arg->context = nullptr; TlsCredentialReloadArg* arg = new TlsCredentialReloadArg(c_arg); - arg->set_pem_root_certs("pem_root_certs"); - TlsKeyMaterialsConfig::PemKeyCertPair pair = {"private_key", "cert_chain"}; - arg->add_pem_key_cert_pair(pair); arg->set_status(GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_NEW); arg->OnCredentialReloadDoneCallback(); EXPECT_EQ(arg->status(), GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_UNCHANGED); - EXPECT_STREQ(c_arg->key_materials_config->pem_root_certs(), "pem_root_certs"); - EXPECT_EQ(c_arg->key_materials_config->pem_key_cert_pair_list().size(), 1); - EXPECT_STREQ( - c_arg->key_materials_config->pem_key_cert_pair_list()[0].private_key(), - "private_key"); - EXPECT_STREQ( - c_arg->key_materials_config->pem_key_cert_pair_list()[0].cert_chain(), - "cert_chain"); // Cleanup. delete arg; - delete c_arg->key_materials_config; delete c_arg; } @@ -344,12 +332,15 @@ TEST_F(CredentialsTest, TlsCredentialReloadConfigSchedule) { grpc_tls_credential_reload_arg* c_arg = new grpc_tls_credential_reload_arg(); c_arg->context = nullptr; TlsCredentialReloadArg* arg = new TlsCredentialReloadArg(c_arg); + std::shared_ptr key_materials_config( + new TlsKeyMaterialsConfig()); struct TlsKeyMaterialsConfig::PemKeyCertPair pair1 = {"private_key1", "cert_chain1"}; struct TlsKeyMaterialsConfig::PemKeyCertPair pair2 = {"private_key2", "cert_chain2"}; std::vector pair_list = {pair1, pair2}; - arg->set_key_materials("pem_root_certs", pair_list); + key_materials_config->set_key_materials("pem_root_certs", pair_list); + arg->set_key_materials_config(key_materials_config); arg->set_status(GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_NEW); arg->set_error_details("error_details"); const char* error_details_before_schedule = c_arg->error_details; @@ -657,7 +648,7 @@ TEST_F(CredentialsTest, TlsCredentialsOptionsCppToC) { delete c_options; } -// This test demonstrates how the TLS credentials will be used. +// This test demonstrates how the SPIFFE credentials will be used. TEST_F(CredentialsTest, LoadTlsChannelCredentials) { std::shared_ptr test_credential_reload( new TestTlsCredentialReload()); @@ -679,22 +670,6 @@ TEST_F(CredentialsTest, LoadTlsChannelCredentials) { GPR_ASSERT(channel_credentials != nullptr); } -// This test demonstrates how the TLS credentials will be used to create -// server credentials. -TEST_F(CredentialsTest, LoadTlsServerCredentials) { - std::shared_ptr test_credential_reload( - new TestTlsCredentialReload()); - std::shared_ptr credential_reload_config( - new TlsCredentialReloadConfig(test_credential_reload)); - - TlsCredentialsOptions options = TlsCredentialsOptions( - GRPC_SSL_REQUEST_CLIENT_CERTIFICATE_AND_VERIFY, - GRPC_TLS_SERVER_VERIFICATION, nullptr, credential_reload_config, nullptr); - std::shared_ptr<::grpc_impl::ServerCredentials> server_credentials = - grpc::experimental::TlsServerCredentials(options); - GPR_ASSERT(server_credentials != nullptr); -} - TEST_F(CredentialsTest, TlsCredentialReloadConfigErrorMessages) { std::shared_ptr config( new TlsCredentialReloadConfig(nullptr));