diff --git a/grpc.def b/grpc.def index 1a82b377c68..c3f8af10860 100644 --- a/grpc.def +++ b/grpc.def @@ -157,6 +157,8 @@ EXPORTS grpc_tls_certificate_provider_file_watcher_create grpc_tls_certificate_provider_release grpc_tls_credentials_options_create + grpc_tls_credentials_options_copy + grpc_tls_credentials_options_destroy grpc_tls_credentials_options_set_certificate_provider grpc_tls_credentials_options_watch_root_certs grpc_tls_credentials_options_set_root_cert_name diff --git a/include/grpc/grpc_security.h b/include/grpc/grpc_security.h index 986683d4051..54dc862d260 100644 --- a/include/grpc/grpc_security.h +++ b/include/grpc/grpc_security.h @@ -815,6 +815,22 @@ GRPCAPI void grpc_tls_certificate_provider_release( */ GRPCAPI grpc_tls_credentials_options* grpc_tls_credentials_options_create(void); +/** + * EXPERIMENTAL API - Subject to change + * + * Copies a grpc_tls_credentials_options. + */ +GRPCAPI grpc_tls_credentials_options* grpc_tls_credentials_options_copy( + grpc_tls_credentials_options* options); + +/** + * EXPERIMENTAL API - Subject to change + * + * Destroys a grpc_tls_credentials_options. + */ +GRPCAPI void grpc_tls_credentials_options_destroy( + grpc_tls_credentials_options* options); + /** * EXPERIMENTAL API - Subject to change * diff --git a/include/grpcpp/security/tls_credentials_options.h b/include/grpcpp/security/tls_credentials_options.h index 7f8ff150910..341d3a857bf 100644 --- a/include/grpcpp/security/tls_credentials_options.h +++ b/include/grpcpp/security/tls_credentials_options.h @@ -44,6 +44,7 @@ class TlsCredentialsOptions { // @param certificate_provider the provider which fetches TLS credentials that // will be used in the TLS handshake TlsCredentialsOptions(); + ~TlsCredentialsOptions(); // ---- Setters for member fields ---- // Sets the certificate provider used to store root certs and identity certs. void set_certificate_provider( @@ -108,8 +109,14 @@ class TlsCredentialsOptions { void set_crl_provider(std::shared_ptr crl_provider); // ----- Getters for member fields ---- - // Get the internal c options. This function shall be used only internally. - grpc_tls_credentials_options* c_credentials_options() const { + // Returns a deep copy of the internal c options. The caller takes ownership + // of the returned pointer. This function shall be used only internally. + grpc_tls_credentials_options* c_credentials_options() const; + + protected: + // Returns the internal c options. The caller does not take ownership of the + // returned pointer. + grpc_tls_credentials_options* mutable_c_credentials_options() { return c_credentials_options_; } diff --git a/src/core/lib/security/credentials/tls/grpc_tls_credentials_options.cc b/src/core/lib/security/credentials/tls/grpc_tls_credentials_options.cc index 5e314700428..3c090011f31 100644 --- a/src/core/lib/security/credentials/tls/grpc_tls_credentials_options.cc +++ b/src/core/lib/security/credentials/tls/grpc_tls_credentials_options.cc @@ -38,6 +38,17 @@ grpc_tls_credentials_options* grpc_tls_credentials_options_create() { return new grpc_tls_credentials_options(); } +grpc_tls_credentials_options* grpc_tls_credentials_options_copy( + grpc_tls_credentials_options* options) { + GPR_ASSERT(options != nullptr); + return new grpc_tls_credentials_options(*options); +} + +void grpc_tls_credentials_options_destroy( + grpc_tls_credentials_options* options) { + delete options; +} + void grpc_tls_credentials_options_set_cert_request_type( grpc_tls_credentials_options* options, grpc_ssl_client_certificate_request_type type) { @@ -136,5 +147,5 @@ void grpc_tls_credentials_options_set_crl_provider( grpc_tls_credentials_options* options, std::shared_ptr provider) { GPR_ASSERT(options != nullptr); - options->set_crl_provider(std::move(provider)); + options->set_crl_provider(provider); } diff --git a/src/core/lib/security/credentials/tls/grpc_tls_credentials_options.h b/src/core/lib/security/credentials/tls/grpc_tls_credentials_options.h index 0994fb2ff91..6f56905ae32 100644 --- a/src/core/lib/security/credentials/tls/grpc_tls_credentials_options.h +++ b/src/core/lib/security/credentials/tls/grpc_tls_credentials_options.h @@ -39,6 +39,7 @@ struct grpc_tls_credentials_options : public grpc_core::RefCounted { public: + grpc_tls_credentials_options() = default; ~grpc_tls_credentials_options() override = default; // Getters for member fields. @@ -105,6 +106,23 @@ struct grpc_tls_credentials_options send_client_ca_list_ == other.send_client_ca_list_; } + grpc_tls_credentials_options(grpc_tls_credentials_options& other) : + cert_request_type_(other.cert_request_type_), + verify_server_cert_(other.verify_server_cert_), + min_tls_version_(other.min_tls_version_), + max_tls_version_(other.max_tls_version_), + certificate_verifier_(other.certificate_verifier_), + check_call_host_(other.check_call_host_), + certificate_provider_(other.certificate_provider_), + watch_root_cert_(other.watch_root_cert_), + root_cert_name_(other.root_cert_name_), + watch_identity_pair_(other.watch_identity_pair_), + identity_cert_name_(other.identity_cert_name_), + tls_session_key_log_file_path_(other.tls_session_key_log_file_path_), + crl_directory_(other.crl_directory_), + crl_provider_(other.crl_provider_), + send_client_ca_list_(other.send_client_ca_list_) {} + private: grpc_ssl_client_certificate_request_type cert_request_type_ = GRPC_SSL_DONT_REQUEST_CLIENT_CERTIFICATE; bool verify_server_cert_ = true; diff --git a/src/cpp/common/tls_credentials_options.cc b/src/cpp/common/tls_credentials_options.cc index 8336b69ffb8..6e907704652 100644 --- a/src/cpp/common/tls_credentials_options.cc +++ b/src/cpp/common/tls_credentials_options.cc @@ -18,7 +18,6 @@ #include #include -#include #include #include @@ -36,9 +35,13 @@ TlsCredentialsOptions::TlsCredentialsOptions() { c_credentials_options_ = grpc_tls_credentials_options_create(); } +TlsCredentialsOptions::~TlsCredentialsOptions() { + grpc_tls_credentials_options_destroy(c_credentials_options_); +} + void TlsCredentialsOptions::set_certificate_provider( std::shared_ptr certificate_provider) { - certificate_provider_ = std::move(certificate_provider); + certificate_provider_ = certificate_provider; if (certificate_provider_ != nullptr) { grpc_tls_credentials_options_set_certificate_provider( c_credentials_options_, certificate_provider_->c_provider()); @@ -48,7 +51,7 @@ void TlsCredentialsOptions::set_certificate_provider( void TlsCredentialsOptions::set_crl_provider( std::shared_ptr crl_provider) { grpc_tls_credentials_options_set_crl_provider(c_credentials_options_, - std::move(crl_provider)); + crl_provider); } void TlsCredentialsOptions::watch_root_certs() { @@ -85,22 +88,27 @@ void TlsCredentialsOptions::set_tls_session_key_log_file_path( void TlsCredentialsOptions::set_certificate_verifier( std::shared_ptr certificate_verifier) { - certificate_verifier_ = std::move(certificate_verifier); + certificate_verifier_ = certificate_verifier; if (certificate_verifier_ != nullptr) { grpc_tls_credentials_options_set_certificate_verifier( c_credentials_options_, certificate_verifier_->c_verifier()); } } +grpc_tls_credentials_options* TlsCredentialsOptions::c_credentials_options() + const { + return grpc_tls_credentials_options_copy(c_credentials_options_); +} + void TlsCredentialsOptions::set_check_call_host(bool check_call_host) { - grpc_tls_credentials_options* options = c_credentials_options(); + grpc_tls_credentials_options* options = mutable_c_credentials_options(); GPR_ASSERT(options != nullptr); grpc_tls_credentials_options_set_check_call_host(options, check_call_host); } void TlsChannelCredentialsOptions::set_verify_server_certs( bool verify_server_certs) { - grpc_tls_credentials_options* options = c_credentials_options(); + grpc_tls_credentials_options* options = mutable_c_credentials_options(); GPR_ASSERT(options != nullptr); grpc_tls_credentials_options_set_verify_server_cert(options, verify_server_certs); @@ -108,7 +116,7 @@ void TlsChannelCredentialsOptions::set_verify_server_certs( void TlsServerCredentialsOptions::set_cert_request_type( grpc_ssl_client_certificate_request_type cert_request_type) { - grpc_tls_credentials_options* options = c_credentials_options(); + grpc_tls_credentials_options* options = mutable_c_credentials_options(); GPR_ASSERT(options != nullptr); grpc_tls_credentials_options_set_cert_request_type(options, cert_request_type); @@ -116,7 +124,7 @@ void TlsServerCredentialsOptions::set_cert_request_type( void TlsServerCredentialsOptions::set_send_client_ca_list( bool send_client_ca_list) { - grpc_tls_credentials_options* options = c_credentials_options(); + grpc_tls_credentials_options* options = mutable_c_credentials_options(); GPR_ASSERT(options != nullptr); grpc_tls_credentials_options_set_send_client_ca_list(options, send_client_ca_list); diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.c b/src/ruby/ext/grpc/rb_grpc_imports.generated.c index 36bd6808a05..641c9cb97cc 100644 --- a/src/ruby/ext/grpc/rb_grpc_imports.generated.c +++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.c @@ -180,6 +180,8 @@ grpc_tls_certificate_provider_static_data_create_type grpc_tls_certificate_provi grpc_tls_certificate_provider_file_watcher_create_type grpc_tls_certificate_provider_file_watcher_create_import; grpc_tls_certificate_provider_release_type grpc_tls_certificate_provider_release_import; grpc_tls_credentials_options_create_type grpc_tls_credentials_options_create_import; +grpc_tls_credentials_options_copy_type grpc_tls_credentials_options_copy_import; +grpc_tls_credentials_options_destroy_type grpc_tls_credentials_options_destroy_import; grpc_tls_credentials_options_set_certificate_provider_type grpc_tls_credentials_options_set_certificate_provider_import; grpc_tls_credentials_options_watch_root_certs_type grpc_tls_credentials_options_watch_root_certs_import; grpc_tls_credentials_options_set_root_cert_name_type grpc_tls_credentials_options_set_root_cert_name_import; @@ -467,6 +469,8 @@ void grpc_rb_load_imports(HMODULE library) { grpc_tls_certificate_provider_file_watcher_create_import = (grpc_tls_certificate_provider_file_watcher_create_type) GetProcAddress(library, "grpc_tls_certificate_provider_file_watcher_create"); grpc_tls_certificate_provider_release_import = (grpc_tls_certificate_provider_release_type) GetProcAddress(library, "grpc_tls_certificate_provider_release"); grpc_tls_credentials_options_create_import = (grpc_tls_credentials_options_create_type) GetProcAddress(library, "grpc_tls_credentials_options_create"); + grpc_tls_credentials_options_copy_import = (grpc_tls_credentials_options_copy_type) GetProcAddress(library, "grpc_tls_credentials_options_copy"); + grpc_tls_credentials_options_destroy_import = (grpc_tls_credentials_options_destroy_type) GetProcAddress(library, "grpc_tls_credentials_options_destroy"); grpc_tls_credentials_options_set_certificate_provider_import = (grpc_tls_credentials_options_set_certificate_provider_type) GetProcAddress(library, "grpc_tls_credentials_options_set_certificate_provider"); grpc_tls_credentials_options_watch_root_certs_import = (grpc_tls_credentials_options_watch_root_certs_type) GetProcAddress(library, "grpc_tls_credentials_options_watch_root_certs"); grpc_tls_credentials_options_set_root_cert_name_import = (grpc_tls_credentials_options_set_root_cert_name_type) GetProcAddress(library, "grpc_tls_credentials_options_set_root_cert_name"); diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.h b/src/ruby/ext/grpc/rb_grpc_imports.generated.h index cce517daad2..55da803ef6a 100644 --- a/src/ruby/ext/grpc/rb_grpc_imports.generated.h +++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.h @@ -515,6 +515,12 @@ extern grpc_tls_certificate_provider_release_type grpc_tls_certificate_provider_ typedef grpc_tls_credentials_options*(*grpc_tls_credentials_options_create_type)(void); extern grpc_tls_credentials_options_create_type grpc_tls_credentials_options_create_import; #define grpc_tls_credentials_options_create grpc_tls_credentials_options_create_import +typedef grpc_tls_credentials_options*(*grpc_tls_credentials_options_copy_type)(grpc_tls_credentials_options* options); +extern grpc_tls_credentials_options_copy_type grpc_tls_credentials_options_copy_import; +#define grpc_tls_credentials_options_copy grpc_tls_credentials_options_copy_import +typedef void(*grpc_tls_credentials_options_destroy_type)(grpc_tls_credentials_options* options); +extern grpc_tls_credentials_options_destroy_type grpc_tls_credentials_options_destroy_import; +#define grpc_tls_credentials_options_destroy grpc_tls_credentials_options_destroy_import typedef void(*grpc_tls_credentials_options_set_certificate_provider_type)(grpc_tls_credentials_options* options, grpc_tls_certificate_provider* provider); extern grpc_tls_credentials_options_set_certificate_provider_type grpc_tls_credentials_options_set_certificate_provider_import; #define grpc_tls_credentials_options_set_certificate_provider grpc_tls_credentials_options_set_certificate_provider_import diff --git a/test/cpp/client/credentials_test.cc b/test/cpp/client/credentials_test.cc index 1c833921fd3..a6b1645e891 100644 --- a/test/cpp/client/credentials_test.cc +++ b/test/cpp/client/credentials_test.cc @@ -47,10 +47,14 @@ constexpr const char* kIdentityCertName = "identity_cert_name"; constexpr const char* kIdentityCertPrivateKey = "identity_private_key"; constexpr const char* kIdentityCertContents = "identity_cert_contents"; +using ::grpc::experimental::CreateStaticCrlProvider; using ::grpc::experimental::ExternalCertificateVerifier; using ::grpc::experimental::FileWatcherCertificateProvider; using ::grpc::experimental::HostNameCertificateVerifier; +using ::grpc::experimental::NoOpCertificateVerifier; using ::grpc::experimental::StaticDataCertificateProvider; +using ::grpc::experimental::TlsChannelCredentialsOptions; +using ::grpc::experimental::TlsCredentialsOptions; } // namespace @@ -419,8 +423,82 @@ TEST(CredentialsTest, TlsChannelCredentialsWithCrlProviderAndDirectory) { GPR_ASSERT(channel_credentials.get() != nullptr); } -// TODO(gtcooke94) - Add test to make sure Tls*CredentialsOptions does not leak -// when not moved into TlsCredentials +TEST(CredentialsTest, TlsCredentialsOptionsDoesNotLeak) { + TlsCredentialsOptions options; + (void)options; +} + +TEST(CredentialsTest, MultipleOptionsOneCertificateProviderDoesNotLeak) { + auto provider = std::make_shared("root-pem"); + TlsCredentialsOptions options_1; + options_1.set_certificate_provider(provider); + TlsCredentialsOptions options_2; + options_2.set_certificate_provider(provider); +} + +TEST(CredentialsTest, MultipleOptionsOneCertificateVerifierDoesNotLeak) { + auto verifier = std::make_shared(); + TlsCredentialsOptions options_1; + options_1.set_certificate_verifier(verifier); + TlsCredentialsOptions options_2; + options_2.set_certificate_verifier(verifier); +} + +TEST(CredentialsTest, MultipleOptionsOneCrlProviderDoesNotLeak) { + auto crl_provider = CreateStaticCrlProvider(/*crls=*/{}); + EXPECT_TRUE(crl_provider.ok()); + TlsCredentialsOptions options_1; + options_1.set_crl_provider(*crl_provider); + TlsCredentialsOptions options_2; + options_2.set_crl_provider(*crl_provider); +} + +TEST(CredentialsTest, TlsChannelCredentialsDoesNotLeak) { + TlsChannelCredentialsOptions options; + auto channel_creds = TlsCredentials(options); + EXPECT_NE(channel_creds, nullptr); +} + +TEST(CredentialsTest, MultipleChannelCredentialsSameOptionsDoesNotLeak) { + TlsChannelCredentialsOptions options; + auto channel_creds_1 = TlsCredentials(options); + EXPECT_NE(channel_creds_1, nullptr); + auto channel_creds_2 = TlsCredentials(options); + EXPECT_NE(channel_creds_2, nullptr); +} + +TEST(CredentialsTest, + MultipleChannelCredentialsOneCertificateProviderDoesNotLeak) { + TlsChannelCredentialsOptions options; + auto provider = std::make_shared("root-pem"); + options.set_certificate_provider(provider); + auto channel_creds_1 = TlsCredentials(options); + EXPECT_NE(channel_creds_1, nullptr); + auto channel_creds_2 = TlsCredentials(options); + EXPECT_NE(channel_creds_2, nullptr); +} + +TEST(CredentialsTest, + MultipleChannelCredentialsOneCertificateVerifierDoesNotLeak) { + TlsChannelCredentialsOptions options; + auto verifier = std::make_shared(); + options.set_certificate_verifier(verifier); + auto channel_creds_1 = TlsCredentials(options); + EXPECT_NE(channel_creds_1, nullptr); + auto channel_creds_2 = TlsCredentials(options); + EXPECT_NE(channel_creds_2, nullptr); +} + +TEST(CredentialsTest, MultipleChannelCredentialsOneCrlProviderDoesNotLeak) { + TlsChannelCredentialsOptions options; + auto provider = CreateStaticCrlProvider(/*crls=*/{}); + EXPECT_TRUE(provider.ok()); + options.set_crl_provider(*provider); + auto channel_creds_1 = TlsCredentials(options); + EXPECT_NE(channel_creds_1, nullptr); + auto channel_creds_2 = TlsCredentials(options); + EXPECT_NE(channel_creds_2, nullptr); +} } // namespace } // namespace testing diff --git a/test/cpp/server/credentials_test.cc b/test/cpp/server/credentials_test.cc index da7bed0673d..8c61d3ce378 100644 --- a/test/cpp/server/credentials_test.cc +++ b/test/cpp/server/credentials_test.cc @@ -41,9 +41,13 @@ constexpr const char* kIdentityCertName = "identity_cert_name"; constexpr const char* kIdentityCertPrivateKey = "identity_private_key"; constexpr const char* kIdentityCertContents = "identity_cert_contents"; +using ::grpc::experimental::CreateStaticCrlProvider; using ::grpc::experimental::ExternalCertificateVerifier; using ::grpc::experimental::FileWatcherCertificateProvider; +using ::grpc::experimental::NoOpCertificateVerifier; using ::grpc::experimental::StaticDataCertificateProvider; +using ::grpc::experimental::TlsServerCredentials; +using ::grpc::experimental::TlsServerCredentialsOptions; } // namespace @@ -200,8 +204,78 @@ TEST(CredentialsTest, TlsServerCredentialsWithCrlProviderAndDirectory) { GPR_ASSERT(server_credentials != nullptr); } -// TODO(gtcooke94) - Add test to make sure Tls*CredentialsOptions does not leak -// when not moved into TlsCredentials +TEST(CredentialsTest, TlsCredentialsOptionsDoesNotLeak) { + auto provider = std::make_shared("root-pem"); + TlsServerCredentialsOptions options(provider); + (void)options; +} + +TEST(CredentialsTest, MultipleOptionsOneCertificateProviderDoesNotLeak) { + auto provider = std::make_shared("root-pem"); + TlsServerCredentialsOptions options_1(provider); + (void)options_1; + TlsServerCredentialsOptions options_2(provider); + (void)options_2; +} + +TEST(CredentialsTest, MultipleOptionsOneCertificateVerifierDoesNotLeak) { + auto provider = std::make_shared("root-pem"); + auto verifier = std::make_shared(); + TlsServerCredentialsOptions options_1(provider); + options_1.set_certificate_verifier(verifier); + TlsServerCredentialsOptions options_2(provider); + options_2.set_certificate_verifier(verifier); +} + +TEST(CredentialsTest, MultipleOptionsOneCrlProviderDoesNotLeak) { + auto provider = std::make_shared("root-pem"); + auto crl_provider = CreateStaticCrlProvider(/*crls=*/{}); + EXPECT_TRUE(crl_provider.ok()); + TlsServerCredentialsOptions options_1(provider); + options_1.set_crl_provider(*crl_provider); + TlsServerCredentialsOptions options_2(provider); + options_2.set_crl_provider(*crl_provider); +} + +TEST(CredentialsTest, TlsServerCredentialsDoesNotLeak) { + auto provider = std::make_shared("root-pem"); + TlsServerCredentialsOptions options(provider); + auto server_creds = TlsServerCredentials(options); + EXPECT_NE(server_creds, nullptr); +} + +TEST(CredentialsTest, MultipleServerCredentialsOneOptionsDoesNotLeak) { + auto provider = std::make_shared("root-pem"); + TlsServerCredentialsOptions options(provider); + auto server_creds_1 = TlsServerCredentials(options); + EXPECT_NE(server_creds_1, nullptr); + auto server_creds_2 = TlsServerCredentials(options); + EXPECT_NE(server_creds_2, nullptr); +} + +TEST(CredentialsTest, + MultipleServerCredentialsOneCertificateVerifierDoesNotLeak) { + auto provider = std::make_shared("root-pem"); + TlsServerCredentialsOptions options(provider); + auto verifier = std::make_shared(); + options.set_certificate_verifier(verifier); + auto server_creds_1 = TlsServerCredentials(options); + EXPECT_NE(server_creds_1, nullptr); + auto server_creds_2 = TlsServerCredentials(options); + EXPECT_NE(server_creds_2, nullptr); +} + +TEST(CredentialsTest, MultipleServerCredentialsOneCrlProviderDoesNotLeak) { + auto provider = std::make_shared("root-pem"); + TlsServerCredentialsOptions options(provider); + auto crl_provider = CreateStaticCrlProvider(/*crls=*/{}); + EXPECT_TRUE(crl_provider.ok()); + options.set_crl_provider(*crl_provider); + auto server_creds_1 = TlsServerCredentials(options); + EXPECT_NE(server_creds_1, nullptr); + auto server_creds_2 = TlsServerCredentials(options); + EXPECT_NE(server_creds_2, nullptr); +} } // namespace } // namespace testing diff --git a/tools/codegen/core/gen_grpc_tls_credentials_options.py b/tools/codegen/core/gen_grpc_tls_credentials_options.py index 7180fa7aa1e..a7e97f0e214 100755 --- a/tools/codegen/core/gen_grpc_tls_credentials_options.py +++ b/tools/codegen/core/gen_grpc_tls_credentials_options.py @@ -319,6 +319,7 @@ print( struct grpc_tls_credentials_options : public grpc_core::RefCounted { public: + grpc_tls_credentials_options() = default; ~grpc_tls_credentials_options() override = default; """, file=H, @@ -394,6 +395,25 @@ for i in range(len(_DATA_MEMBERS)): operator_equal_content += " &&\n" print(operator_equal_content + ";\n }", file=H) +# Write out copy constructor +print( + "\n grpc_tls_credentials_options(grpc_tls_credentials_options& other) :", + file=H, +) +operator_equal_content = " " +for i in range(len(_DATA_MEMBERS)): + if i != 0: + operator_equal_content += " " + if i == len(_DATA_MEMBERS) - 1: + operator_equal_content += ( + _DATA_MEMBERS[i].name + "_(other." + _DATA_MEMBERS[i].name + "_)" + ) + else: + operator_equal_content += ( + _DATA_MEMBERS[i].name + "_(other." + _DATA_MEMBERS[i].name + "_),\n" + ) +print(operator_equal_content + " {}", file=H) + # Print out data member declarations print("\n private:", file=H) for data_member in _DATA_MEMBERS: