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));