diff options
author | JackLivio <jack@livio.io> | 2018-06-18 16:39:28 -0400 |
---|---|---|
committer | GitHub <noreply@github.com> | 2018-06-18 16:39:28 -0400 |
commit | a920e7141f0f8627cc37673f403c97899091f527 (patch) | |
tree | 9954a5b43cc1461c185bbda004fbe8999cff8536 | |
parent | abf4630471f84b8563a8acc88c6f72cd1aa71e23 (diff) | |
parent | 0a7317dda7adb28fc5a15234ab2bc09428153015 (diff) | |
download | sdl_core-a920e7141f0f8627cc37673f403c97899091f527.tar.gz |
Merge pull request #2218 from smartdevicelink/fix/fix_certificate_saving_after_ptu
Fix certificate saving after policy table update
10 files changed, 121 insertions, 194 deletions
diff --git a/src/components/connection_handler/src/connection_handler_impl.cc b/src/components/connection_handler/src/connection_handler_impl.cc index b97c6eacd4..59865ac4b2 100644 --- a/src/components/connection_handler/src/connection_handler_impl.cc +++ b/src/components/connection_handler/src/connection_handler_impl.cc @@ -467,14 +467,6 @@ void ConnectionHandlerImpl::OnSessionStartedCallback( const uint32_t session_key = KeyFromPair(connection_handle, context.new_session_id_); - uint32_t app_id = 0; - GetDataOnSessionKey( - session_key, &app_id, NULL, static_cast<DeviceHandle*>(NULL)); - if (app_id > 0) { - context.is_ptu_required_ = - !connection_handler_observer_->CheckAppIsNavi(app_id); - } - { sync_primitives::AutoLock auto_lock(start_service_context_map_lock_); start_service_context_map_[session_key] = context; diff --git a/src/components/connection_handler/test/connection_handler_impl_test.cc b/src/components/connection_handler/test/connection_handler_impl_test.cc index b231ee625f..56dbf6b9de 100644 --- a/src/components/connection_handler/test/connection_handler_impl_test.cc +++ b/src/components/connection_handler/test/connection_handler_impl_test.cc @@ -1273,9 +1273,6 @@ TEST_F(ConnectionHandlerTest, SessionStarted_WithRpc) { true, ByRef(empty))); - EXPECT_CALL(mock_connection_handler_observer, CheckAppIsNavi(_)) - .WillOnce(Return(true)); - connection_handler_->set_protocol_handler(&mock_protocol_handler_); EXPECT_CALL(mock_protocol_handler_, NotifySessionStarted(_, _)) .WillOnce(SaveArg<0>(&out_context_)); @@ -1312,8 +1309,6 @@ TEST_F(ConnectionHandlerTest, ServiceStarted_Video_SUCCESS) { session_key, true, ByRef(empty))); - EXPECT_CALL(mock_connection_handler_observer, CheckAppIsNavi(_)) - .WillOnce(Return(true)); // confirm that NotifySessionStarted() is called connection_handler_->set_protocol_handler(&mock_protocol_handler_); @@ -1354,8 +1349,6 @@ TEST_F(ConnectionHandlerTest, ServiceStarted_Video_FAILURE) { session_key, false, ByRef(empty))); - EXPECT_CALL(mock_connection_handler_observer, CheckAppIsNavi(_)) - .WillOnce(Return(true)); // confirm that NotifySessionStarted() is called connection_handler_->set_protocol_handler(&mock_protocol_handler_); @@ -1446,9 +1439,6 @@ TEST_F(ConnectionHandlerTest, ServiceStarted_Video_Multiple) { session_key1, true, ByRef(empty)))); - EXPECT_CALL(mock_connection_handler_observer, CheckAppIsNavi(_)) - .Times(2) - .WillOnce(Return(true)); // verify that connection handler will not mix up the two results SessionContext new_context_first, new_context_second; diff --git a/src/components/include/protocol_handler/session_observer.h b/src/components/include/protocol_handler/session_observer.h index 3482c6569c..242775bf25 100644 --- a/src/components/include/protocol_handler/session_observer.h +++ b/src/components/include/protocol_handler/session_observer.h @@ -66,7 +66,6 @@ struct SessionContext { uint32_t hash_id_; bool is_protected_; bool is_new_service_; - bool is_ptu_required_; /** * @brief Constructor @@ -78,8 +77,7 @@ struct SessionContext { , service_type_(protocol_handler::kInvalidServiceType) , hash_id_(0) , is_protected_(false) - , is_new_service_(false) - , is_ptu_required_(false) {} + , is_new_service_(false) {} /** * @brief Constructor @@ -105,8 +103,7 @@ struct SessionContext { , service_type_(service_type) , hash_id_(hash_id) , is_protected_(is_protected) - , is_new_service_(false) - , is_ptu_required_(false) {} + , is_new_service_(false) {} }; /** diff --git a/src/components/policy/policy_external/src/policy_manager_impl.cc b/src/components/policy/policy_external/src/policy_manager_impl.cc index 56de26af64..172e218e0e 100644 --- a/src/components/policy/policy_external/src/policy_manager_impl.cc +++ b/src/components/policy/policy_external/src/policy_manager_impl.cc @@ -1790,11 +1790,6 @@ StatusNotifier PolicyManagerImpl::AddApplication( device_consent); } else { PromoteExistedApplication(application_id, device_consent); - if (helpers::in_range(hmi_types, policy_table::AHT_NAVIGATION) && - !HasCertificate()) { - LOG4CXX_DEBUG(logger_, "Certificate does not exist, scheduling update."); - update_status_manager_.ScheduleUpdate(); - } return utils::MakeShared<utils::CallNothing>(); } } diff --git a/src/components/policy/policy_regular/src/policy_manager_impl.cc b/src/components/policy/policy_regular/src/policy_manager_impl.cc index 84d4ac853a..8687501b2f 100644 --- a/src/components/policy/policy_regular/src/policy_manager_impl.cc +++ b/src/components/policy/policy_regular/src/policy_manager_impl.cc @@ -1073,13 +1073,6 @@ StatusNotifier PolicyManagerImpl::AddApplication( device_consent); } else { PromoteExistedApplication(application_id, device_consent); - const policy_table::AppHMIType type = policy_table::AHT_NAVIGATION; - if (helpers::in_range(hmi_types, - (rpc::Enum<policy_table::AppHMIType>)type) && - !HasCertificate()) { - LOG4CXX_DEBUG(logger_, "Certificate does not exist, scheduling update."); - update_status_manager_.ScheduleUpdate(); - } return utils::MakeShared<utils::CallNothing>(); } } @@ -1157,6 +1150,10 @@ bool PolicyManagerImpl::InitPT(const std::string& file_name, if (ret) { RefreshRetrySequence(); update_status_manager_.OnPolicyInit(cache_->UpdateRequired()); + const std::string certificate_data = cache_->GetCertificate(); + if (!certificate_data.empty()) { + listener_->OnCertificateUpdated(certificate_data); + } } return ret; } diff --git a/src/components/protocol_handler/include/protocol_handler/protocol_handler_impl.h b/src/components/protocol_handler/include/protocol_handler/protocol_handler_impl.h index 4d86a78688..99f03b1c04 100644 --- a/src/components/protocol_handler/include/protocol_handler/protocol_handler_impl.h +++ b/src/components/protocol_handler/include/protocol_handler/protocol_handler_impl.h @@ -475,14 +475,6 @@ class ProtocolHandlerImpl const transport_manager::ConnectionUID connection_id) OVERRIDE; /** - * @brief OnPTUFinished the callback which signals PTU has finished - * - * @param ptu_result the result from the PTU - true if successful, - * otherwise false. - */ - void OnPTUFinished(const bool ptu_result) OVERRIDE; - - /** * @brief Notifies subscribers about message * received from mobile device. * @param message Message with already parsed header. @@ -685,12 +677,6 @@ class ProtocolHandlerImpl #ifdef ENABLE_SECURITY security_manager::SecurityManager* security_manager_; - - bool is_ptu_triggered_; - std::list<std::shared_ptr<HandshakeHandler> > ptu_pending_handlers_; - std::list<std::shared_ptr<HandshakeHandler> > handshake_handlers_; - sync_primitives::Lock ptu_handlers_lock_; - sync_primitives::Lock handshake_handlers_lock_; #endif // ENABLE_SECURITY // Thread that pumps non-parsed messages coming from mobile side. diff --git a/src/components/protocol_handler/src/protocol_handler_impl.cc b/src/components/protocol_handler/src/protocol_handler_impl.cc index 5516af81c1..86dbb604fa 100644 --- a/src/components/protocol_handler/src/protocol_handler_impl.cc +++ b/src/components/protocol_handler/src/protocol_handler_impl.cc @@ -75,7 +75,6 @@ ProtocolHandlerImpl::ProtocolHandlerImpl( , #ifdef ENABLE_SECURITY security_manager_(NULL) - , is_ptu_triggered_(false) , #endif // ENABLE_SECURITY raw_ford_messages_from_mobile_( @@ -149,7 +148,6 @@ ProtocolHandlerImpl::~ProtocolHandlerImpl() { "Not all observers have unsubscribed" " from ProtocolHandlerImpl"); } - handshake_handlers_.clear(); } void ProtocolHandlerImpl::AddProtocolObserver(ProtocolObserver* observer) { @@ -845,61 +843,6 @@ void ProtocolHandlerImpl::NotifyOnFailedHandshake() { security_manager_->NotifyListenersOnHandshakeFailed(); } -void ProtocolHandlerImpl::OnPTUFinished(const bool ptu_result) { - LOG4CXX_AUTO_TRACE(logger_); - -#ifdef ENABLE_SECURITY - sync_primitives::AutoLock lock(ptu_handlers_lock_); - - if (!is_ptu_triggered_) { - LOG4CXX_ERROR(logger_, - "PTU was not triggered by service starting. Ignored"); - return; - } - - for (auto handler : ptu_pending_handlers_) { - const bool is_cert_expired = security_manager_->IsCertificateUpdateRequired( - handler->connection_key()); - security_manager::SSLContext* ssl_context = - is_cert_expired ? NULL - : security_manager_->CreateSSLContext( - handler->connection_key(), - security_manager::SecurityManager::kUseExisting); - - if (!ssl_context) { - const std::string error("CreateSSLContext failed"); - LOG4CXX_ERROR(logger_, error); - security_manager_->SendInternalError( - handler->connection_key(), - security_manager::SecurityManager::ERROR_INTERNAL, - error); - - handler->OnHandshakeDone( - handler->connection_key(), - security_manager::SSLContext::Handshake_Result_Fail); - - continue; - } - - if (ssl_context->IsInitCompleted()) { - handler->OnHandshakeDone( - handler->connection_key(), - security_manager::SSLContext::Handshake_Result_Success); - } else { - security_manager_->AddListener(new HandshakeHandler(*handler)); - if (!ssl_context->IsHandshakePending()) { - // Start handshake process - security_manager_->StartHandshake(handler->connection_key()); - } - } - } - - LOG4CXX_DEBUG(logger_, "Handshake handlers were notified"); - ptu_pending_handlers_.clear(); - is_ptu_triggered_ = false; -#endif // ENABLE_SECURITY -} - RESULT_CODE ProtocolHandlerImpl::SendFrame(const ProtocolFramePtr packet) { LOG4CXX_AUTO_TRACE(logger_); if (!packet) { @@ -1572,40 +1515,12 @@ void ProtocolHandlerImpl::NotifySessionStarted( context, packet->protocol_version(), bson_object_bytes); - handshake_handlers_.push_back(handler); - - const bool is_certificate_empty = - security_manager_->IsPolicyCertificateDataEmpty(); - - if (context.is_ptu_required_ && is_certificate_empty) { - LOG4CXX_DEBUG(logger_, - "PTU for StartSessionHandler " - << handler.get() - << " is required and certificate data is empty"); - - sync_primitives::AutoLock lock(ptu_handlers_lock_); - if (!is_ptu_triggered_) { - LOG4CXX_DEBUG(logger_, - "PTU is not triggered yet. " - << "Starting PTU and postponing SSL handshake"); - - ptu_pending_handlers_.push_back(handler); - is_ptu_triggered_ = true; - security_manager_->NotifyOnCertificateUpdateRequired(); - security_manager_->PostponeHandshake(connection_key); - } else { - LOG4CXX_DEBUG(logger_, "PTU has been triggered. Added to pending."); - ptu_pending_handlers_.push_back(handler); - } - return; - } security_manager::SSLContext* ssl_context = - is_certificate_empty - ? NULL - : security_manager_->CreateSSLContext( - connection_key, - security_manager::SecurityManager::kUseExisting); + security_manager_->CreateSSLContext( + connection_key, + security_manager::SecurityManager::ContextCreationStrategy:: + kUseExisting); if (!ssl_context) { const std::string error("CreateSSLContext failed"); LOG4CXX_ERROR(logger_, error); diff --git a/src/components/security_manager/include/security_manager/crypto_manager_impl.h b/src/components/security_manager/include/security_manager/crypto_manager_impl.h index f4a13198da..aa3be0f430 100644 --- a/src/components/security_manager/include/security_manager/crypto_manager_impl.h +++ b/src/components/security_manager/include/security_manager/crypto_manager_impl.h @@ -155,6 +155,14 @@ class CryptoManagerImpl : public CryptoManager { bool set_certificate(const std::string& cert_data); /** + * @brief Saves new certificate data on the file system + * @param cert_data certificate data in PEM format + * @return true if new certificate data was successfully saved on the file + * system, otherwise returns false + */ + bool SaveCertificateData(const std::string& cert_data) const; + + /** * @brief Updates certificate and private key for the current SSL context * @param certificate new certificate to update * @param key new private key to update @@ -177,6 +185,24 @@ class CryptoManagerImpl : public CryptoManager { */ EVP_PKEY* LoadModulePrivateKeyFromFile(); + /** + * @brief Saves new X509 certificate data to file specified in + * CryptoManagerSettings + * @param certificate new X509 certificate data + * @return true if certificate data was saved to the file system otherwise + * returns false + */ + bool SaveModuleCertificateToFile(X509* certificate) const; + + /** + * @brief Saves new private key data to file specified in + * CryptoManagerSettings + * @param key new private key data + * @return true if private key data was saved to the file system otherwise + * returns false + */ + bool SaveModuleKeyToFile(EVP_PKEY* key) const; + const utils::SharedPtr<const CryptoManagerSettings> settings_; SSL_CTX* context_; static uint32_t instance_count_; diff --git a/src/components/security_manager/src/crypto_manager_impl.cc b/src/components/security_manager/src/crypto_manager_impl.cc index 34727fedf9..84c5db7c0e 100644 --- a/src/components/security_manager/src/crypto_manager_impl.cc +++ b/src/components/security_manager/src/crypto_manager_impl.cc @@ -221,7 +221,7 @@ bool CryptoManagerImpl::Init() { // Disable SSL2 as deprecated SSL_CTX_set_options(context_, SSL_OP_NO_SSLv2); - set_certificate(get_settings().certificate_data()); + SaveCertificateData(get_settings().certificate_data()); if (get_settings().ciphers_list().empty()) { LOG4CXX_WARN(logger_, "Empty ciphers list"); @@ -288,7 +288,7 @@ bool CryptoManagerImpl::OnCertificateUpdated(const std::string& data) { return false; } - if (!set_certificate(data)) { + if (!SaveCertificateData(data)) { LOG4CXX_ERROR(logger_, "Failed to save certificate data"); return false; } @@ -362,7 +362,8 @@ const CryptoManagerSettings& CryptoManagerImpl::get_settings() const { return *settings_; } -bool CryptoManagerImpl::set_certificate(const std::string& cert_data) { +bool CryptoManagerImpl::SaveCertificateData( + const std::string& cert_data) const { LOG4CXX_AUTO_TRACE(logger_); if (cert_data.empty()) { @@ -377,51 +378,30 @@ bool CryptoManagerImpl::set_certificate(const std::string& cert_data) { UNUSED(bio_guard) X509* cert = NULL; - PEM_read_bio_X509(bio_cert, &cert, 0, 0); - - EVP_PKEY* pkey = NULL; - if (1 == BIO_reset(bio_cert)) { - PEM_read_bio_PrivateKey(bio_cert, &pkey, 0, 0); - } else { - LOG4CXX_WARN(logger_, - "Unabled to reset BIO in order to read private key, " - << LastError()); - } - - if (NULL == cert || NULL == pkey) { - LOG4CXX_WARN(logger_, "Either certificate or key not valid."); + if (!PEM_read_bio_X509(bio_cert, &cert, 0, 0)) { + LOG4CXX_WARN(logger_, "Could not read certificate data: " << LastError()); return false; } - if (!SSL_CTX_use_certificate(context_, cert)) { - LOG4CXX_WARN(logger_, "Could not use certificate: " << LastError()); - return false; - } + utils::ScopeGuard cert_guard = utils::MakeGuard(X509_free, cert); + UNUSED(cert_guard); - if (!SSL_CTX_use_PrivateKey(context_, pkey)) { - LOG4CXX_ERROR(logger_, "Could not use key: " << LastError()); - return false; + if (1 != BIO_reset(bio_cert)) { + LOG4CXX_WARN(logger_, + "Unabled to reset BIO in order to read private key, " + << LastError()); } - if (!SSL_CTX_check_private_key(context_)) { - LOG4CXX_ERROR(logger_, "Could not use certificate: " << LastError()); + EVP_PKEY* pkey = NULL; + if (!PEM_read_bio_PrivateKey(bio_cert, &pkey, 0, 0)) { + LOG4CXX_WARN(logger_, "Could not read private key data: " << LastError()); return false; } - X509_STORE* store = SSL_CTX_get_cert_store(context_); - if (store) { - X509* extra_cert = NULL; - while ((extra_cert = PEM_read_bio_X509(bio_cert, NULL, 0, 0))) { - if (extra_cert != cert) { - LOG4CXX_DEBUG(logger_, - "Added new certificate to store: " << extra_cert); - X509_STORE_add_cert(store, extra_cert); - } - } - } + utils::ScopeGuard key_guard = utils::MakeGuard(EVP_PKEY_free, pkey); + UNUSED(key_guard); - LOG4CXX_DEBUG(logger_, "Certificate and key successfully updated"); - return true; + return SaveModuleCertificateToFile(cert) && SaveModuleKeyToFile(pkey); } bool CryptoManagerImpl::UpdateModuleCertificateData(X509* certificate, @@ -501,4 +481,59 @@ EVP_PKEY* CryptoManagerImpl::LoadModulePrivateKeyFromFile() { return module_key; } +bool CryptoManagerImpl::SaveModuleCertificateToFile(X509* certificate) const { + LOG4CXX_AUTO_TRACE(logger_); + + if (!certificate) { + LOG4CXX_WARN(logger_, "Empty certificate. Saving will be skipped"); + return false; + } + + const std::string cert_path = get_settings().module_cert_path(); + BIO* bio_cert = BIO_new_file(cert_path.c_str(), "w"); + if (!bio_cert) { + LOG4CXX_ERROR(logger_, + "Failed to open " << cert_path << " file: " << LastError()); + return false; + } + + utils::ScopeGuard bio_guard = utils::MakeGuard(BIO_free, bio_cert); + UNUSED(bio_guard); + + if (!PEM_write_bio_X509(bio_cert, certificate)) { + LOG4CXX_ERROR(logger_, + "Failed to write certificate to file: " << LastError()); + return false; + } + + return true; +} + +bool CryptoManagerImpl::SaveModuleKeyToFile(EVP_PKEY* key) const { + LOG4CXX_AUTO_TRACE(logger_); + + if (!key) { + LOG4CXX_WARN(logger_, "Empty private key. Saving will be skipped"); + return false; + } + + const std::string key_path = get_settings().module_key_path(); + BIO* bio_key = BIO_new_file(key_path.c_str(), "w"); + if (!bio_key) { + LOG4CXX_ERROR(logger_, + "Failed to open " << key_path << " file: " << LastError()); + return false; + } + + utils::ScopeGuard bio_guard = utils::MakeGuard(BIO_free, bio_key); + UNUSED(bio_guard); + + if (!PEM_write_bio_PrivateKey(bio_key, key, NULL, NULL, 0, NULL, NULL)) { + LOG4CXX_ERROR(logger_, "Failed to write key to file: " << LastError()); + return false; + } + + return true; +} + } // namespace security_manager diff --git a/src/components/security_manager/test/crypto_manager_impl_test.cc b/src/components/security_manager/test/crypto_manager_impl_test.cc index 47e63dc194..74b071793d 100644 --- a/src/components/security_manager/test/crypto_manager_impl_test.cc +++ b/src/components/security_manager/test/crypto_manager_impl_test.cc @@ -145,26 +145,20 @@ TEST_F(CryptoManagerTest, UsingBeforeInit) { } TEST_F(CryptoManagerTest, WrongInit) { - forced_protected_services_.push_back(kServiceNumber); - forced_unprotected_services_.push_back(kServiceNumber); - EXPECT_CALL(*mock_security_manager_settings_, security_manager_mode()) - .WillOnce(Return(security_manager::SERVER)); - EXPECT_CALL(*mock_security_manager_settings_, force_unprotected_service()) - .WillOnce(ReturnRef(forced_unprotected_services_)); - EXPECT_CALL(*mock_security_manager_settings_, force_protected_service()) - .WillOnce(ReturnRef(forced_protected_services_)); - EXPECT_FALSE(crypto_manager_->Init()); - forced_protected_services_.pop_back(); - forced_unprotected_services_.pop_back(); - EXPECT_NE(std::string(), crypto_manager_->LastError()); + // We have to cast (-1) to security_manager::Protocol Enum to be accepted by + // crypto_manager_->Init(...) + // Unknown protocol version + security_manager::Protocol UNKNOWN = + static_cast<security_manager::Protocol>(-1); // Unexistent cipher value const std::string invalid_cipher = "INVALID_UNKNOWN_CIPHER"; - EXPECT_CALL(*mock_security_manager_settings_, security_manager_mode()) - .WillOnce(Return(security_manager::SERVER)); - EXPECT_CALL(*mock_security_manager_settings_, force_unprotected_service()) - .WillOnce(ReturnRef(forced_unprotected_services_)); - EXPECT_CALL(*mock_security_manager_settings_, force_protected_service()) - .WillOnce(ReturnRef(forced_protected_services_)); + const security_manager::Mode mode = security_manager::SERVER; + + SetInitialValues(mode, UNKNOWN, invalid_cipher); + + EXPECT_FALSE(crypto_manager_->Init()); + EXPECT_NE(std::string(), crypto_manager_->LastError()); + EXPECT_CALL(*mock_security_manager_settings_, security_manager_protocol_name()) .WillOnce(Return(security_manager::TLSv1_2)); |