From 0a054cc6eaf7eba44e503395efd85f3377a15b45 Mon Sep 17 00:00:00 2001 From: Matthew Stevenson Date: Fri, 20 Sep 2019 08:59:34 -0700 Subject: [PATCH] Implementing Yang's secound round of comments. --- .../grpcpp/security/tls_credentials_options.h | 73 +++++++++++-------- src/cpp/common/tls_credentials_options.cc | 14 ++-- test/cpp/client/credentials_test.cc | 38 ++++------ 3 files changed, 67 insertions(+), 58 deletions(-) diff --git a/include/grpcpp/security/tls_credentials_options.h b/include/grpcpp/security/tls_credentials_options.h index 8b5382cd852..92c7bab0487 100644 --- a/include/grpcpp/security/tls_credentials_options.h +++ b/include/grpcpp/security/tls_credentials_options.h @@ -112,39 +112,44 @@ class TlsCredentialReloadArg { }; /** An interface that the application derives and uses to instantiate a - * TlsCredentialReloadConfig instance. All 3 methods must be defined. **/ + * TlsCredentialReloadConfig instance. Refer to the definition of the + * grpc_tls_credential_reload_config in grpc_tls_credentials_options.h for more + * details on the expectations of the member functions of the interface. **/ struct TlsCredentialReloadInterface { - /** An application-provided callback that invokes the credential reload. **/ - virtual int Schedule(TlsCredentialReloadArg* arg) = 0; - /** An application-provided callback that cancels a credential reload request. - * **/ - virtual void Cancel(TlsCredentialReloadArg* arg) = 0; - /** An application-provided callback that cleans up any data associated to the - * interface or the config. **/ - virtual void Release() = 0; + virtual ~TlsCredentialReloadInterface() = default; + /** A callback that invokes the credential reload. **/ + virtual int Schedule(TlsCredentialReloadArg* arg) { return 1; } + /** A callback that cancels a credential reload request. **/ + virtual void Cancel(TlsCredentialReloadArg* 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. **/ + virtual void Release() {} }; /** TLS credential reloag config, wraps grpc_tls_credential_reload_config. It is * used for experimental purposes for now and it is subject to change. **/ class TlsCredentialReloadConfig { public: - /** The constructor takes ownership of the interface argument. **/ - TlsCredentialReloadConfig( - std::shared_ptr interface); + /** The config takes ownership of the credential reload interface. **/ + TlsCredentialReloadConfig(std::unique_ptr + credential_reload_interface); ~TlsCredentialReloadConfig(); int Schedule(TlsCredentialReloadArg* arg) const { - return interface_->Schedule(arg); + return credential_reload_interface_->Schedule(arg); } - void Cancel(TlsCredentialReloadArg* arg) const { interface_->Cancel(arg); } + void Cancel(TlsCredentialReloadArg* arg) const { + credential_reload_interface_->Cancel(arg); + } /** Returns a C struct for the credential reload config. **/ grpc_tls_credential_reload_config* c_config() const { return c_config_; } private: grpc_tls_credential_reload_config* c_config_; - std::shared_ptr interface_; + std::unique_ptr credential_reload_interface_; }; /** TLS server authorization check arguments, wraps @@ -195,19 +200,20 @@ class TlsServerAuthorizationCheckArg { }; /** An interface that the application derives and uses to instantiate a - * TlsServerAuthorizationCheckConfig instance. All 3 methods must be defined. + * TlsServerAuthorizationCheckConfig instance. Refer to the definition of the + * grpc_tls_server_authorization_check_config in grpc_tls_credentials_options.h + * for more details on the expectations of the member functions of the + * interface. * **/ struct TlsServerAuthorizationCheckInterface { - /** An application-provided callback that invokes the server authorization - * check. **/ - virtual int Schedule(TlsServerAuthorizationCheckArg* arg) = 0; - /** An application-provided callback that cancels a server authorization check - * request. - * **/ - virtual void Cancel(TlsServerAuthorizationCheckArg* arg) = 0; - /** An application-provided callback that cleans up any data associated to the + virtual ~TlsServerAuthorizationCheckInterface() = default; + /** A callback that invokes the server authorization check. **/ + virtual int Schedule(TlsServerAuthorizationCheckArg* arg) { return 1; } + /** A callback that cancels a server authorization check request. **/ + virtual void Cancel(TlsServerAuthorizationCheckArg* arg){}; + /** A callback that cleans up any data associated to the * interface or the config. **/ - virtual void Release() = 0; + virtual void Release(){}; }; /** TLS server authorization check config, wraps @@ -215,17 +221,19 @@ struct TlsServerAuthorizationCheckInterface { * purposes for now and it is subject to change. **/ class TlsServerAuthorizationCheckConfig { public: - /** The constructor takess ownership of the interface argument. **/ + /** The config takes ownership of the server authorization check interface. + * **/ TlsServerAuthorizationCheckConfig( - std::shared_ptr interface); + std::unique_ptr + server_authorization_check_interface); ~TlsServerAuthorizationCheckConfig(); int Schedule(TlsServerAuthorizationCheckArg* arg) const { - return interface_->Schedule(arg); + return server_authorization_check_interface_->Schedule(arg); } void Cancel(TlsServerAuthorizationCheckArg* arg) const { - interface_->Cancel(arg); + server_authorization_check_interface_->Cancel(arg); } /** Creates C struct for the server authorization check config. **/ @@ -235,7 +243,8 @@ class TlsServerAuthorizationCheckConfig { private: grpc_tls_server_authorization_check_config* c_config_; - std::shared_ptr interface_; + std::unique_ptr + server_authorization_check_interface_; }; /** TLS credentials options, wrapper for grpc_tls_credentials_options. It is @@ -271,6 +280,10 @@ class TlsCredentialsOptions { } private: + /** The cert_request_type_ flag is only relevant when the + * TlsCredentialsOptions are used to instantiate server credentials; the flag + * goes unused when creating channel credentials, and the user can set it to + * GRPC_SSL_DONT_REQUEST_CLIENT_CERTIFICATE. **/ grpc_ssl_client_certificate_request_type cert_request_type_; std::shared_ptr key_materials_config_; std::shared_ptr credential_reload_config_; diff --git a/src/cpp/common/tls_credentials_options.cc b/src/cpp/common/tls_credentials_options.cc index b5e58660233..13b400b45bf 100644 --- a/src/cpp/common/tls_credentials_options.cc +++ b/src/cpp/common/tls_credentials_options.cc @@ -87,8 +87,8 @@ void TlsCredentialReloadArg::OnCredentialReloadDoneCallback() { /** gRPC TLS credential reload config API implementation **/ TlsCredentialReloadConfig::TlsCredentialReloadConfig( - std::shared_ptr interface) - : interface_(std::move(interface)) { + std::unique_ptr credential_reload_interface) + : credential_reload_interface_(std::move(credential_reload_interface)) { c_config_ = grpc_tls_credential_reload_config_create( nullptr, &TlsCredentialReloadConfigCSchedule, &TlsCredentialReloadConfigCCancel, nullptr); @@ -96,7 +96,7 @@ TlsCredentialReloadConfig::TlsCredentialReloadConfig( } TlsCredentialReloadConfig::~TlsCredentialReloadConfig() { - interface_->Release(); + credential_reload_interface_->Release(); } /** gRPC TLS server authorization check arg API implementation **/ @@ -164,8 +164,10 @@ void TlsServerAuthorizationCheckArg::OnServerAuthorizationCheckDoneCallback() { /** gRPC TLS server authorization check config API implementation. **/ TlsServerAuthorizationCheckConfig::TlsServerAuthorizationCheckConfig( - std::shared_ptr interface) - : interface_(std::move(interface)) { + std::unique_ptr + server_authorization_check_interface) + : server_authorization_check_interface_( + std::move(server_authorization_check_interface)) { c_config_ = grpc_tls_server_authorization_check_config_create( nullptr, &TlsServerAuthorizationCheckConfigCSchedule, &TlsServerAuthorizationCheckConfigCCancel, nullptr); @@ -173,7 +175,7 @@ TlsServerAuthorizationCheckConfig::TlsServerAuthorizationCheckConfig( } TlsServerAuthorizationCheckConfig::~TlsServerAuthorizationCheckConfig() { - interface_->Release(); + server_authorization_check_interface_->Release(); } /** gRPC TLS credential options API implementation **/ diff --git a/test/cpp/client/credentials_test.cc b/test/cpp/client/credentials_test.cc index eeb073c80ab..3e3b44ede2d 100644 --- a/test/cpp/client/credentials_test.cc +++ b/test/cpp/client/credentials_test.cc @@ -74,8 +74,6 @@ class TestTlsCredentialReloadInterface : public TlsCredentialReloadInterface { arg->set_status(GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_FAIL); arg->set_error_details("cancelled"); } - - void Release() override { return; } }; static void tls_server_authorization_check_callback( @@ -109,8 +107,6 @@ class TestTlsServerAuthorizationCheckInterface arg->set_status(GRPC_STATUS_PERMISSION_DENIED); arg->set_error_details("cancelled"); } - - void Release() override { return; } }; } // namespace @@ -348,9 +344,9 @@ TEST_F(CredentialsTest, TlsCredentialReloadArgCallback) { } TEST_F(CredentialsTest, TlsCredentialReloadConfigSchedule) { - std::shared_ptr interface( + std::unique_ptr interface( new TestTlsCredentialReloadInterface()); - TlsCredentialReloadConfig config(interface); + TlsCredentialReloadConfig config(std::move(interface)); grpc_tls_credential_reload_arg c_arg; TlsCredentialReloadArg arg(&c_arg); arg.set_cb_user_data(static_cast(nullptr)); @@ -393,9 +389,9 @@ TEST_F(CredentialsTest, TlsCredentialReloadConfigSchedule) { } TEST_F(CredentialsTest, TlsCredentialReloadConfigCppToC) { - std::shared_ptr interface( + std::unique_ptr interface( new TestTlsCredentialReloadInterface()); - TlsCredentialReloadConfig config = TlsCredentialReloadConfig(interface); + TlsCredentialReloadConfig config(std::move(interface)); grpc_tls_credential_reload_arg c_arg; c_arg.cb_user_data = static_cast(nullptr); grpc_tls_key_materials_config c_key_materials; @@ -487,10 +483,9 @@ TEST_F(CredentialsTest, TlsServerAuthorizationCheckArgCallback) { } TEST_F(CredentialsTest, TlsServerAuthorizationCheckConfigSchedule) { - std::shared_ptr interface( + std::unique_ptr interface( new TestTlsServerAuthorizationCheckInterface()); - TlsServerAuthorizationCheckConfig config = - TlsServerAuthorizationCheckConfig(interface); + TlsServerAuthorizationCheckConfig config(std::move(interface)); grpc_tls_server_authorization_check_arg c_arg; TlsServerAuthorizationCheckArg arg(&c_arg); arg.set_cb_user_data(nullptr); @@ -524,10 +519,9 @@ TEST_F(CredentialsTest, TlsServerAuthorizationCheckConfigSchedule) { } TEST_F(CredentialsTest, TlsServerAuthorizationCheckConfigCppToC) { - std::shared_ptr interface( + std::unique_ptr interface( new TestTlsServerAuthorizationCheckInterface()); - TlsServerAuthorizationCheckConfig config = - TlsServerAuthorizationCheckConfig(interface); + TlsServerAuthorizationCheckConfig config(std::move(interface)); grpc_tls_server_authorization_check_arg c_arg; c_arg.cb = tls_server_authorization_check_callback; c_arg.cb_user_data = nullptr; @@ -574,17 +568,17 @@ TEST_F(CredentialsTest, TlsCredentialsOptionsCppToC) { std::vector pair_list = {pair}; key_materials_config->set_key_materials("pem_root_certs", pair_list); - std::shared_ptr credential_reload_interface( + std::unique_ptr credential_reload_interface( new TestTlsCredentialReloadInterface()); std::shared_ptr credential_reload_config( - new TlsCredentialReloadConfig(credential_reload_interface)); + new TlsCredentialReloadConfig(std::move(credential_reload_interface))); - std::shared_ptr + std::unique_ptr server_authorization_check_interface( new TestTlsServerAuthorizationCheckInterface()); std::shared_ptr server_authorization_check_config(new TlsServerAuthorizationCheckConfig( - server_authorization_check_interface)); + std::move(server_authorization_check_interface))); TlsCredentialsOptions options = TlsCredentialsOptions( GRPC_SSL_REQUEST_CLIENT_CERTIFICATE_AND_VERIFY, key_materials_config, @@ -672,17 +666,17 @@ TEST_F(CredentialsTest, TlsCredentialsOptionsCppToC) { // This test demonstrates how the SPIFFE credentials will be used. TEST_F(CredentialsTest, LoadSpiffeChannelCredentials) { - std::shared_ptr credential_reload_interface( + std::unique_ptr credential_reload_interface( new TestTlsCredentialReloadInterface()); std::shared_ptr credential_reload_config( - new TlsCredentialReloadConfig(credential_reload_interface)); + new TlsCredentialReloadConfig(std::move(credential_reload_interface))); - std::shared_ptr + std::unique_ptr server_authorization_check_interface( new TestTlsServerAuthorizationCheckInterface()); std::shared_ptr server_authorization_check_config(new TlsServerAuthorizationCheckConfig( - server_authorization_check_interface)); + std::move(server_authorization_check_interface))); TlsCredentialsOptions options = TlsCredentialsOptions( GRPC_SSL_REQUEST_CLIENT_CERTIFICATE_AND_VERIFY, nullptr,