From 8e09d8745db06ff0c6f374cc7be99712ac3f9924 Mon Sep 17 00:00:00 2001 From: Matthew Stevenson Date: Fri, 20 Sep 2019 16:01:10 -0700 Subject: [PATCH] Implementing further comments by Yang. --- .../grpcpp/security/tls_credentials_options.h | 18 +-- src/cpp/common/tls_credentials_options.cc | 16 ++- .../common/tls_credentials_options_util.cc | 16 +-- test/cpp/client/credentials_test.cc | 117 +++++++++--------- 4 files changed, 87 insertions(+), 80 deletions(-) diff --git a/include/grpcpp/security/tls_credentials_options.h b/include/grpcpp/security/tls_credentials_options.h index 92c7bab0487..71472fd6b4e 100644 --- a/include/grpcpp/security/tls_credentials_options.h +++ b/include/grpcpp/security/tls_credentials_options.h @@ -118,9 +118,9 @@ class TlsCredentialReloadArg { struct TlsCredentialReloadInterface { virtual ~TlsCredentialReloadInterface() = default; /** A callback that invokes the credential reload. **/ - virtual int Schedule(TlsCredentialReloadArg* arg) { return 1; } + virtual int Schedule(std::unique_ptr& arg) = 0; /** A callback that cancels a credential reload request. **/ - virtual void Cancel(TlsCredentialReloadArg* arg) {} + virtual void Cancel(std::unique_ptr& arg) {} /** A callback that cleans up any data associated to the * interface or the config. It will be called when the config is no longer * using the interface. **/ @@ -136,11 +136,11 @@ class TlsCredentialReloadConfig { credential_reload_interface); ~TlsCredentialReloadConfig(); - int Schedule(TlsCredentialReloadArg* arg) const { + int Schedule(std::unique_ptr& arg) const { return credential_reload_interface_->Schedule(arg); } - void Cancel(TlsCredentialReloadArg* arg) const { + void Cancel(std::unique_ptr& arg) const { credential_reload_interface_->Cancel(arg); } @@ -208,12 +208,12 @@ class TlsServerAuthorizationCheckArg { struct TlsServerAuthorizationCheckInterface { virtual ~TlsServerAuthorizationCheckInterface() = default; /** A callback that invokes the server authorization check. **/ - virtual int Schedule(TlsServerAuthorizationCheckArg* arg) { return 1; } + virtual int Schedule(std::unique_ptr& arg) = 0; /** A callback that cancels a server authorization check request. **/ - virtual void Cancel(TlsServerAuthorizationCheckArg* arg){}; + virtual void Cancel(std::unique_ptr& arg) {} /** A callback that cleans up any data associated to the * interface or the config. **/ - virtual void Release(){}; + virtual void Release() {} }; /** TLS server authorization check config, wraps @@ -228,11 +228,11 @@ class TlsServerAuthorizationCheckConfig { server_authorization_check_interface); ~TlsServerAuthorizationCheckConfig(); - int Schedule(TlsServerAuthorizationCheckArg* arg) const { + int Schedule(std::unique_ptr& arg) const { return server_authorization_check_interface_->Schedule(arg); } - void Cancel(TlsServerAuthorizationCheckArg* arg) const { + void Cancel(std::unique_ptr& arg) const { server_authorization_check_interface_->Cancel(arg); } diff --git a/src/cpp/common/tls_credentials_options.cc b/src/cpp/common/tls_credentials_options.cc index 13b400b45bf..3199317d030 100644 --- a/src/cpp/common/tls_credentials_options.cc +++ b/src/cpp/common/tls_credentials_options.cc @@ -96,7 +96,9 @@ TlsCredentialReloadConfig::TlsCredentialReloadConfig( } TlsCredentialReloadConfig::~TlsCredentialReloadConfig() { - credential_reload_interface_->Release(); + if (credential_reload_interface_ != nullptr) { + credential_reload_interface_->Release(); + } } /** gRPC TLS server authorization check arg API implementation **/ @@ -175,7 +177,9 @@ TlsServerAuthorizationCheckConfig::TlsServerAuthorizationCheckConfig( } TlsServerAuthorizationCheckConfig::~TlsServerAuthorizationCheckConfig() { - server_authorization_check_interface_->Release(); + if (server_authorization_check_interface_ != nullptr) { + server_authorization_check_interface_->Release(); + } } /** gRPC TLS credential options API implementation **/ @@ -193,9 +197,11 @@ TlsCredentialsOptions::TlsCredentialsOptions( c_credentials_options_ = grpc_tls_credentials_options_create(); grpc_tls_credentials_options_set_cert_request_type(c_credentials_options_, cert_request_type_); - grpc_tls_credentials_options_set_key_materials_config( - c_credentials_options_, - ConvertToCKeyMaterialsConfig(key_materials_config_)); + if (key_materials_config_ != nullptr) { + grpc_tls_credentials_options_set_key_materials_config( + c_credentials_options_, + ConvertToCKeyMaterialsConfig(key_materials_config_)); + } if (credential_reload_config_ != nullptr) { grpc_tls_credentials_options_set_credential_reload_config( c_credentials_options_, credential_reload_config_->c_config()); diff --git a/src/cpp/common/tls_credentials_options_util.cc b/src/cpp/common/tls_credentials_options_util.cc index 680aad162e4..90c2d73849d 100644 --- a/src/cpp/common/tls_credentials_options_util.cc +++ b/src/cpp/common/tls_credentials_options_util.cc @@ -92,8 +92,8 @@ int TlsCredentialReloadConfigCSchedule(void* config_user_data, } TlsCredentialReloadConfig* cpp_config = static_cast(arg->config->context()); - TlsCredentialReloadArg cpp_arg(arg); - return cpp_config->Schedule(&cpp_arg); + std::unique_ptr cpp_arg(new TlsCredentialReloadArg(arg)); + return cpp_config->Schedule(cpp_arg); } void TlsCredentialReloadConfigCCancel(void* config_user_data, @@ -105,8 +105,8 @@ void TlsCredentialReloadConfigCCancel(void* config_user_data, } TlsCredentialReloadConfig* cpp_config = static_cast(arg->config->context()); - TlsCredentialReloadArg cpp_arg(arg); - cpp_config->Cancel(&cpp_arg); + std::unique_ptr cpp_arg(new TlsCredentialReloadArg(arg)); + cpp_config->Cancel(cpp_arg); } /** The C schedule and cancel functions for the server authorization check @@ -122,8 +122,8 @@ int TlsServerAuthorizationCheckConfigCSchedule( } TlsServerAuthorizationCheckConfig* cpp_config = static_cast(arg->config->context()); - TlsServerAuthorizationCheckArg cpp_arg(arg); - return cpp_config->Schedule(&cpp_arg); + std::unique_ptr cpp_arg(new TlsServerAuthorizationCheckArg(arg)); + return cpp_config->Schedule(cpp_arg); } void TlsServerAuthorizationCheckConfigCCancel( @@ -136,8 +136,8 @@ void TlsServerAuthorizationCheckConfigCCancel( } TlsServerAuthorizationCheckConfig* cpp_config = static_cast(arg->config->context()); - TlsServerAuthorizationCheckArg cpp_arg(arg); - cpp_config->Cancel(&cpp_arg); + std::unique_ptr cpp_arg(new TlsServerAuthorizationCheckArg(arg)); + cpp_config->Cancel(cpp_arg); } } // namespace experimental diff --git a/test/cpp/client/credentials_test.cc b/test/cpp/client/credentials_test.cc index 3e3b44ede2d..026212532b1 100644 --- a/test/cpp/client/credentials_test.cc +++ b/test/cpp/client/credentials_test.cc @@ -50,8 +50,8 @@ static void tls_credential_reload_callback( arg->status = GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_UNCHANGED; } -class TestTlsCredentialReloadInterface : public TlsCredentialReloadInterface { - int Schedule(TlsCredentialReloadArg* arg) override { +class TestTlsCredentialReload : public TlsCredentialReloadInterface { + int Schedule(std::unique_ptr& arg) override { GPR_ASSERT(arg != nullptr); struct TlsKeyMaterialsConfig::PemKeyCertPair pair3 = {"private_key3", "cert_chain3"}; @@ -69,7 +69,7 @@ class TestTlsCredentialReloadInterface : public TlsCredentialReloadInterface { return 0; } - void Cancel(TlsCredentialReloadArg* arg) override { + void Cancel(std::unique_ptr& arg) override { GPR_ASSERT(arg != nullptr); arg->set_status(GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_FAIL); arg->set_error_details("cancelled"); @@ -88,9 +88,9 @@ static void tls_server_authorization_check_callback( arg->error_details = gpr_strdup("callback_error_details"); } -class TestTlsServerAuthorizationCheckInterface +class TestTlsServerAuthorizationCheck : public TlsServerAuthorizationCheckInterface { - int Schedule(TlsServerAuthorizationCheckArg* arg) override { + int Schedule(std::unique_ptr& arg) override { GPR_ASSERT(arg != nullptr); grpc::string cb_user_data = "cb_user_data"; arg->set_cb_user_data(static_cast(gpr_strdup(cb_user_data.c_str()))); @@ -102,7 +102,7 @@ class TestTlsServerAuthorizationCheckInterface return 1; } - void Cancel(TlsServerAuthorizationCheckArg* arg) override { + void Cancel(std::unique_ptr& arg) override { GPR_ASSERT(arg != nullptr); arg->set_status(GRPC_STATUS_PERMISSION_DENIED); arg->set_error_details("cancelled"); @@ -344,12 +344,12 @@ TEST_F(CredentialsTest, TlsCredentialReloadArgCallback) { } TEST_F(CredentialsTest, TlsCredentialReloadConfigSchedule) { - std::unique_ptr interface( - new TestTlsCredentialReloadInterface()); - TlsCredentialReloadConfig config(std::move(interface)); + std::unique_ptr test_credential_reload( + new TestTlsCredentialReload()); + TlsCredentialReloadConfig config(std::move(test_credential_reload)); grpc_tls_credential_reload_arg c_arg; - TlsCredentialReloadArg arg(&c_arg); - arg.set_cb_user_data(static_cast(nullptr)); + std::unique_ptr arg( new TlsCredentialReloadArg(&c_arg)); + arg->set_cb_user_data(static_cast(nullptr)); std::shared_ptr key_materials_config( new TlsKeyMaterialsConfig()); struct TlsKeyMaterialsConfig::PemKeyCertPair pair1 = {"private_key1", @@ -358,19 +358,20 @@ TEST_F(CredentialsTest, TlsCredentialReloadConfigSchedule) { "cert_chain2"}; std::vector pair_list = {pair1, pair2}; 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"); + arg->set_key_materials_config(key_materials_config); + arg->set_status(GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_NEW); + arg->set_error_details("error_details"); grpc_tls_key_materials_config* key_materials_config_before_schedule = c_arg.key_materials_config; const char* error_details_before_schedule = c_arg.error_details; - int schedule_output = config.Schedule(&arg); + int schedule_output = config.Schedule(arg); + GPR_ASSERT(arg != nullptr); EXPECT_EQ(schedule_output, 0); - EXPECT_EQ(arg.cb_user_data(), nullptr); - EXPECT_STREQ(arg.key_materials_config()->pem_root_certs().c_str(), + EXPECT_EQ(arg->cb_user_data(), nullptr); + EXPECT_STREQ(arg->key_materials_config()->pem_root_certs().c_str(), "new_pem_root_certs"); - pair_list = arg.key_materials_config()->pem_key_cert_pair_list(); + pair_list = arg->key_materials_config()->pem_key_cert_pair_list(); EXPECT_EQ(static_cast(pair_list.size()), 3); EXPECT_STREQ(pair_list[0].private_key.c_str(), "private_key01"); EXPECT_STREQ(pair_list[0].cert_chain.c_str(), "cert_chain01"); @@ -378,8 +379,8 @@ TEST_F(CredentialsTest, TlsCredentialReloadConfigSchedule) { EXPECT_STREQ(pair_list[1].cert_chain.c_str(), "cert_chain2"); EXPECT_STREQ(pair_list[2].private_key.c_str(), "private_key3"); EXPECT_STREQ(pair_list[2].cert_chain.c_str(), "cert_chain3"); - EXPECT_EQ(arg.status(), GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_NEW); - EXPECT_STREQ(arg.error_details().c_str(), "error_details"); + EXPECT_EQ(arg->status(), GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_NEW); + EXPECT_STREQ(arg->error_details().c_str(), "error_details"); // Cleanup. gpr_free(const_cast(error_details_before_schedule)); @@ -389,9 +390,9 @@ TEST_F(CredentialsTest, TlsCredentialReloadConfigSchedule) { } TEST_F(CredentialsTest, TlsCredentialReloadConfigCppToC) { - std::unique_ptr interface( - new TestTlsCredentialReloadInterface()); - TlsCredentialReloadConfig config(std::move(interface)); + std::unique_ptr test_credential_reload( + new TestTlsCredentialReload()); + TlsCredentialReloadConfig config(std::move(test_credential_reload)); grpc_tls_credential_reload_arg c_arg; c_arg.cb_user_data = static_cast(nullptr); grpc_tls_key_materials_config c_key_materials; @@ -483,32 +484,33 @@ TEST_F(CredentialsTest, TlsServerAuthorizationCheckArgCallback) { } TEST_F(CredentialsTest, TlsServerAuthorizationCheckConfigSchedule) { - std::unique_ptr interface( - new TestTlsServerAuthorizationCheckInterface()); - TlsServerAuthorizationCheckConfig config(std::move(interface)); + std::unique_ptr + test_server_authorization_check(new TestTlsServerAuthorizationCheck()); + TlsServerAuthorizationCheckConfig config( + std::move(test_server_authorization_check)); grpc_tls_server_authorization_check_arg c_arg; - TlsServerAuthorizationCheckArg arg(&c_arg); - arg.set_cb_user_data(nullptr); - arg.set_success(0); - arg.set_target_name("target_name"); - arg.set_peer_cert("peer_cert"); - arg.set_status(GRPC_STATUS_PERMISSION_DENIED); - arg.set_error_details("error_details"); + std::unique_ptr arg(new TlsServerAuthorizationCheckArg(&c_arg)); + arg->set_cb_user_data(nullptr); + arg->set_success(0); + arg->set_target_name("target_name"); + arg->set_peer_cert("peer_cert"); + arg->set_status(GRPC_STATUS_PERMISSION_DENIED); + arg->set_error_details("error_details"); const char* target_name_before_schedule = c_arg.target_name; const char* peer_cert_before_schedule = c_arg.peer_cert; const char* error_details_before_schedule = c_arg.error_details; - int schedule_output = config.Schedule(&arg); + int schedule_output = config.Schedule(arg); EXPECT_EQ(schedule_output, 1); - EXPECT_STREQ(static_cast(arg.cb_user_data()), "cb_user_data"); - EXPECT_EQ(arg.success(), 1); - EXPECT_STREQ(arg.target_name().c_str(), "sync_target_name"); - EXPECT_STREQ(arg.peer_cert().c_str(), "sync_peer_cert"); - EXPECT_EQ(arg.status(), GRPC_STATUS_OK); - EXPECT_STREQ(arg.error_details().c_str(), "sync_error_details"); + EXPECT_STREQ(static_cast(arg->cb_user_data()), "cb_user_data"); + EXPECT_EQ(arg->success(), 1); + EXPECT_STREQ(arg->target_name().c_str(), "sync_target_name"); + EXPECT_STREQ(arg->peer_cert().c_str(), "sync_peer_cert"); + EXPECT_EQ(arg->status(), GRPC_STATUS_OK); + EXPECT_STREQ(arg->error_details().c_str(), "sync_error_details"); // Cleanup. - gpr_free(arg.cb_user_data()); + gpr_free(arg->cb_user_data()); gpr_free(const_cast(target_name_before_schedule)); gpr_free(const_cast(peer_cert_before_schedule)); gpr_free(const_cast(error_details_before_schedule)); @@ -519,9 +521,10 @@ TEST_F(CredentialsTest, TlsServerAuthorizationCheckConfigSchedule) { } TEST_F(CredentialsTest, TlsServerAuthorizationCheckConfigCppToC) { - std::unique_ptr interface( - new TestTlsServerAuthorizationCheckInterface()); - TlsServerAuthorizationCheckConfig config(std::move(interface)); + std::unique_ptr + test_server_authorization_check(new TestTlsServerAuthorizationCheck()); + TlsServerAuthorizationCheckConfig config( + std::move(test_server_authorization_check)); grpc_tls_server_authorization_check_arg c_arg; c_arg.cb = tls_server_authorization_check_callback; c_arg.cb_user_data = nullptr; @@ -568,17 +571,16 @@ TEST_F(CredentialsTest, TlsCredentialsOptionsCppToC) { std::vector pair_list = {pair}; key_materials_config->set_key_materials("pem_root_certs", pair_list); - std::unique_ptr credential_reload_interface( - new TestTlsCredentialReloadInterface()); + std::unique_ptr test_credential_reload( + new TestTlsCredentialReload()); std::shared_ptr credential_reload_config( - new TlsCredentialReloadConfig(std::move(credential_reload_interface))); + new TlsCredentialReloadConfig(std::move(test_credential_reload))); - std::unique_ptr - server_authorization_check_interface( - new TestTlsServerAuthorizationCheckInterface()); + std::unique_ptr + test_server_authorization_check(new TestTlsServerAuthorizationCheck()); std::shared_ptr server_authorization_check_config(new TlsServerAuthorizationCheckConfig( - std::move(server_authorization_check_interface))); + std::move(test_server_authorization_check))); TlsCredentialsOptions options = TlsCredentialsOptions( GRPC_SSL_REQUEST_CLIENT_CERTIFICATE_AND_VERIFY, key_materials_config, @@ -666,17 +668,16 @@ TEST_F(CredentialsTest, TlsCredentialsOptionsCppToC) { // This test demonstrates how the SPIFFE credentials will be used. TEST_F(CredentialsTest, LoadSpiffeChannelCredentials) { - std::unique_ptr credential_reload_interface( - new TestTlsCredentialReloadInterface()); + std::unique_ptr test_credential_reload( + new TestTlsCredentialReload()); std::shared_ptr credential_reload_config( - new TlsCredentialReloadConfig(std::move(credential_reload_interface))); + new TlsCredentialReloadConfig(std::move(test_credential_reload))); - std::unique_ptr - server_authorization_check_interface( - new TestTlsServerAuthorizationCheckInterface()); + std::unique_ptr + test_server_authorization_check(new TestTlsServerAuthorizationCheck()); std::shared_ptr server_authorization_check_config(new TlsServerAuthorizationCheckConfig( - std::move(server_authorization_check_interface))); + std::move(test_server_authorization_check))); TlsCredentialsOptions options = TlsCredentialsOptions( GRPC_SSL_REQUEST_CLIENT_CERTIFICATE_AND_VERIFY, nullptr,