From ffcf4f477eb29a16b6b7f45281adf6c702ec7c7b Mon Sep 17 00:00:00 2001 From: Zhen Lian Date: Thu, 23 Jan 2020 12:28:03 -0800 Subject: [PATCH 1/2] [TLS Lib Clean-up] Add hostname check For tls library --- .../tls/tls_security_connector.cc | 23 ++++++++++++ .../tls/tls_security_connector.h | 6 +++- test/core/security/BUILD | 2 ++ .../security/tls_security_connector_test.cc | 36 +++++++++++++++++-- 4 files changed, 63 insertions(+), 4 deletions(-) diff --git a/src/core/lib/security/security_connector/tls/tls_security_connector.cc b/src/core/lib/security/security_connector/tls/tls_security_connector.cc index 3cd83ae1a80..64311141f3c 100644 --- a/src/core/lib/security/security_connector/tls/tls_security_connector.cc +++ b/src/core/lib/security/security_connector/tls/tls_security_connector.cc @@ -112,6 +112,18 @@ grpc_status_code TlsFetchKeyMaterials( return status; } +grpc_error* TlsCheckPeer(const char* peer_name, const tsi_peer* peer) { + /* Check the peer name if specified. */ + if (peer_name != nullptr && !grpc_ssl_host_matches_name(peer, peer_name)) { + char* msg; + gpr_asprintf(&msg, "Peer name %s is not in peer certificate", peer_name); + grpc_error* error = GRPC_ERROR_CREATE_FROM_COPIED_STRING(msg); + gpr_free(msg); + return error; + } + return GRPC_ERROR_NONE; +} + TlsChannelSecurityConnector::TlsChannelSecurityConnector( grpc_core::RefCountedPtr channel_creds, grpc_core::RefCountedPtr request_metadata_creds, @@ -180,6 +192,17 @@ void TlsChannelSecurityConnector::check_peer( grpc_ssl_peer_to_auth_context(&peer, GRPC_TLS_TRANSPORT_SECURITY_TYPE); const TlsCredentials* creds = static_cast(channel_creds()); + if (creds->options().server_verification_option() == + GRPC_TLS_SERVER_VERIFICATION) { + /* Do the default host name check if specifying the target name. */ + error = TlsCheckPeer(target_name, &peer); + if (error != GRPC_ERROR_NONE) { + grpc_core::ExecCtx::Run(DEBUG_LOCATION, on_peer_checked, error); + tsi_peer_destruct(&peer); + return; + } + } + /* Do the custom server authorization check, if specified by the user. */ const grpc_tls_server_authorization_check_config* config = creds->options().server_authorization_check_config(); /* If server authorization config is not null, use it to perform diff --git a/src/core/lib/security/security_connector/tls/tls_security_connector.h b/src/core/lib/security/security_connector/tls/tls_security_connector.h index 825ffe7b1f7..bdabed2ffc8 100644 --- a/src/core/lib/security/security_connector/tls/tls_security_connector.h +++ b/src/core/lib/security/security_connector/tls/tls_security_connector.h @@ -144,13 +144,17 @@ class TlsServerSecurityConnector final : public grpc_server_security_connector { grpc_core::RefCountedPtr key_materials_config_; }; -// Exposed for testing only. +// ---- Functions below are exposed for testing only ----------------------- grpc_status_code TlsFetchKeyMaterials( const grpc_core::RefCountedPtr& key_materials_config, const grpc_tls_credentials_options& options, bool server_config, grpc_ssl_certificate_config_reload_status* status); +// TlsCheckPeer checks if |peer_name| matches the identity information +// contained in |peer|. This is AKA hostname check. +grpc_error* TlsCheckPeer(const char* peer_name, const tsi_peer* peer); + } // namespace grpc_core #endif /* GRPC_CORE_LIB_SECURITY_SECURITY_CONNECTOR_TLS_TLS_SECURITY_CONNECTOR_H \ diff --git a/test/core/security/BUILD b/test/core/security/BUILD index 25fd11e0ca0..509c0adb55a 100644 --- a/test/core/security/BUILD +++ b/test/core/security/BUILD @@ -269,6 +269,8 @@ grpc_cc_test( "//:gpr", "//:grpc", "//:grpc_secure", + "//:tsi", + "//:tsi_interface", "//test/core/end2end:ssl_test_data", "//test/core/util:grpc_test_util", ], diff --git a/test/core/security/tls_security_connector_test.cc b/test/core/security/tls_security_connector_test.cc index 94a10ceb5cc..e7aba673edf 100644 --- a/test/core/security/tls_security_connector_test.cc +++ b/test/core/security/tls_security_connector_test.cc @@ -16,16 +16,17 @@ * */ -#include -#include +#include "src/core/lib/security/security_connector/tls/tls_security_connector.h" #include #include #include #include #include +#include +#include -#include "src/core/lib/security/security_connector/tls/tls_security_connector.h" +#include "src/core/tsi/transport_security.h" #include "test/core/end2end/data/ssl_test_data.h" #include "test/core/util/test_config.h" @@ -254,6 +255,35 @@ TEST_F(TlsSecurityConnectorTest, CreateChannelSecurityConnectorFailInit) { EXPECT_EQ(connector, nullptr); } +TEST_F(TlsSecurityConnectorTest, TlsCheckPeerSuccess) { + const char* target_name = "foo.test.google.fr"; + tsi_peer peer; + GPR_ASSERT(tsi_construct_peer(1, &peer) == TSI_OK); + GPR_ASSERT(tsi_construct_string_peer_property_from_cstring( + TSI_X509_SUBJECT_ALTERNATIVE_NAME_PEER_PROPERTY, target_name, + &peer.properties[0]) == TSI_OK); + grpc_error* error = grpc_core::TlsCheckPeer(target_name, &peer); + tsi_peer_destruct(&peer); + EXPECT_EQ(error, GRPC_ERROR_NONE); + GRPC_ERROR_UNREF(error); + options_->Unref(); +} + +TEST_F(TlsSecurityConnectorTest, TlsCheckPeerFail) { + const char* target_name = "foo.test.google.fr"; + const char* another_name = "bar.test.google.fr"; + tsi_peer peer; + GPR_ASSERT(tsi_construct_peer(1, &peer) == TSI_OK); + GPR_ASSERT(tsi_construct_string_peer_property_from_cstring( + TSI_X509_SUBJECT_ALTERNATIVE_NAME_PEER_PROPERTY, another_name, + &peer.properties[0]) == TSI_OK); + grpc_error* error = grpc_core::TlsCheckPeer(target_name, &peer); + tsi_peer_destruct(&peer); + EXPECT_NE(error, GRPC_ERROR_NONE); + GRPC_ERROR_UNREF(error); + options_->Unref(); +} + TEST_F(TlsSecurityConnectorTest, CreateServerSecurityConnectorSuccess) { SetOptions(SUCCESS); auto cred = std::unique_ptr( From 9ded19e24d015a2c7e1962ff8990539c3168d1eb Mon Sep 17 00:00:00 2001 From: Zhen Lian Date: Tue, 28 Jan 2020 14:06:01 -0800 Subject: [PATCH 2/2] fix first round of comments --- .../credentials/tls/grpc_tls_credentials_options.h | 3 ++- .../security_connector/tls/tls_security_connector.cc | 4 ++-- .../security_connector/tls/tls_security_connector.h | 4 ++-- test/core/end2end/fixtures/h2_tls.cc | 11 +++++------ test/core/security/tls_security_connector_test.cc | 8 ++++---- 5 files changed, 15 insertions(+), 15 deletions(-) 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 ca82a294928..a9e2495eadc 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 @@ -273,7 +273,8 @@ struct grpc_tls_credentials_options private: grpc_ssl_client_certificate_request_type cert_request_type_; - grpc_tls_server_verification_option server_verification_option_; + grpc_tls_server_verification_option server_verification_option_ = + GRPC_TLS_SERVER_VERIFICATION; grpc_core::RefCountedPtr key_materials_config_; grpc_core::RefCountedPtr credential_reload_config_; diff --git a/src/core/lib/security/security_connector/tls/tls_security_connector.cc b/src/core/lib/security/security_connector/tls/tls_security_connector.cc index 64311141f3c..e2be8cab57f 100644 --- a/src/core/lib/security/security_connector/tls/tls_security_connector.cc +++ b/src/core/lib/security/security_connector/tls/tls_security_connector.cc @@ -112,7 +112,7 @@ grpc_status_code TlsFetchKeyMaterials( return status; } -grpc_error* TlsCheckPeer(const char* peer_name, const tsi_peer* peer) { +grpc_error* TlsCheckHostName(const char* peer_name, const tsi_peer* peer) { /* Check the peer name if specified. */ if (peer_name != nullptr && !grpc_ssl_host_matches_name(peer, peer_name)) { char* msg; @@ -195,7 +195,7 @@ void TlsChannelSecurityConnector::check_peer( if (creds->options().server_verification_option() == GRPC_TLS_SERVER_VERIFICATION) { /* Do the default host name check if specifying the target name. */ - error = TlsCheckPeer(target_name, &peer); + error = TlsCheckHostName(target_name, &peer); if (error != GRPC_ERROR_NONE) { grpc_core::ExecCtx::Run(DEBUG_LOCATION, on_peer_checked, error); tsi_peer_destruct(&peer); diff --git a/src/core/lib/security/security_connector/tls/tls_security_connector.h b/src/core/lib/security/security_connector/tls/tls_security_connector.h index bdabed2ffc8..85c69768975 100644 --- a/src/core/lib/security/security_connector/tls/tls_security_connector.h +++ b/src/core/lib/security/security_connector/tls/tls_security_connector.h @@ -151,9 +151,9 @@ grpc_status_code TlsFetchKeyMaterials( const grpc_tls_credentials_options& options, bool server_config, grpc_ssl_certificate_config_reload_status* status); -// TlsCheckPeer checks if |peer_name| matches the identity information +// TlsCheckHostName checks if |peer_name| matches the identity information // contained in |peer|. This is AKA hostname check. -grpc_error* TlsCheckPeer(const char* peer_name, const tsi_peer* peer); +grpc_error* TlsCheckHostName(const char* peer_name, const tsi_peer* peer); } // namespace grpc_core diff --git a/test/core/end2end/fixtures/h2_tls.cc b/test/core/end2end/fixtures/h2_tls.cc index 28b8d549838..17123afa276 100644 --- a/test/core/end2end/fixtures/h2_tls.cc +++ b/test/core/end2end/fixtures/h2_tls.cc @@ -16,16 +16,13 @@ * */ -#include "test/core/end2end/end2end_tests.h" - -#include -#include - #include #include #include - #include +#include +#include + #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/gpr/env.h" #include "src/core/lib/gpr/string.h" @@ -37,6 +34,7 @@ #include "src/core/lib/security/credentials/tls/grpc_tls_credentials_options.h" #include "src/core/lib/security/security_connector/ssl_utils_config.h" #include "test/core/end2end/data/ssl_test_data.h" +#include "test/core/end2end/end2end_tests.h" #include "test/core/util/port.h" #include "test/core/util/test_config.h" @@ -193,6 +191,7 @@ static int server_cred_reload_sync(void* /*config_user_data*/, static grpc_channel_credentials* create_tls_channel_credentials( fullstack_secure_fixture_data* ffd) { grpc_tls_credentials_options* options = grpc_tls_credentials_options_create(); + options->set_server_verification_option(GRPC_TLS_SERVER_VERIFICATION); /* Set credential reload config. */ grpc_tls_credential_reload_config* reload_config = grpc_tls_credential_reload_config_create(nullptr, client_cred_reload_sync, diff --git a/test/core/security/tls_security_connector_test.cc b/test/core/security/tls_security_connector_test.cc index e7aba673edf..13761a8e055 100644 --- a/test/core/security/tls_security_connector_test.cc +++ b/test/core/security/tls_security_connector_test.cc @@ -255,21 +255,21 @@ TEST_F(TlsSecurityConnectorTest, CreateChannelSecurityConnectorFailInit) { EXPECT_EQ(connector, nullptr); } -TEST_F(TlsSecurityConnectorTest, TlsCheckPeerSuccess) { +TEST_F(TlsSecurityConnectorTest, TlsCheckHostNameSuccess) { const char* target_name = "foo.test.google.fr"; tsi_peer peer; GPR_ASSERT(tsi_construct_peer(1, &peer) == TSI_OK); GPR_ASSERT(tsi_construct_string_peer_property_from_cstring( TSI_X509_SUBJECT_ALTERNATIVE_NAME_PEER_PROPERTY, target_name, &peer.properties[0]) == TSI_OK); - grpc_error* error = grpc_core::TlsCheckPeer(target_name, &peer); + grpc_error* error = grpc_core::TlsCheckHostName(target_name, &peer); tsi_peer_destruct(&peer); EXPECT_EQ(error, GRPC_ERROR_NONE); GRPC_ERROR_UNREF(error); options_->Unref(); } -TEST_F(TlsSecurityConnectorTest, TlsCheckPeerFail) { +TEST_F(TlsSecurityConnectorTest, TlsCheckHostNameFail) { const char* target_name = "foo.test.google.fr"; const char* another_name = "bar.test.google.fr"; tsi_peer peer; @@ -277,7 +277,7 @@ TEST_F(TlsSecurityConnectorTest, TlsCheckPeerFail) { GPR_ASSERT(tsi_construct_string_peer_property_from_cstring( TSI_X509_SUBJECT_ALTERNATIVE_NAME_PEER_PROPERTY, another_name, &peer.properties[0]) == TSI_OK); - grpc_error* error = grpc_core::TlsCheckPeer(target_name, &peer); + grpc_error* error = grpc_core::TlsCheckHostName(target_name, &peer); tsi_peer_destruct(&peer); EXPECT_NE(error, GRPC_ERROR_NONE); GRPC_ERROR_UNREF(error);