Fix Handshake Flow issues (#3441)

* Fix issue with External PTU timeout

When attempting to start an encrypted service, the mobile proxy would not receive a response if the PTU timed out. This is because the retry count is based on the number of OnSystemRequests received from the HMI. Since that number would never exceed the number of retries, `IsAllowedRetryCountExceeded` would never return false. This will now return false after the final retry times out.

* Add error code check in ProcessInternalError

If the proxy responds with NOT_SUPPORTED or SSL_INVALID_DATA, Core will mark the handshake as failed.

4 files changed, 30 insertions, 27 deletions

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 082d4a37ba..12ab1224fd 100644
--- a/src/components/policy/policy_external/src/policy_manager_impl.cc
+++ b/src/components/policy/policy_external/src/policy_manager_impl.cc
@@ -1368,7 +1368,7 @@ bool PolicyManagerImpl::IsAllowedRetryCountExceeded() const {
   LOG4CXX_AUTO_TRACE(logger_);
   sync_primitives::AutoLock auto_lock(retry_sequence_lock_);
 
-  return retry_sequence_index_ > retry_sequence_seconds_.size();
+  return retry_sequence_index_ >= retry_sequence_seconds_.size();
 }
 
 void PolicyManagerImpl::IncrementRetryIndex() {
diff --git a/src/components/security_manager/include/security_manager/security_manager_impl.h b/src/components/security_manager/include/security_manager/security_manager_impl.h
index ee00e0774a..6e5fb08c20 100644
--- a/src/components/security_manager/include/security_manager/security_manager_impl.h
+++ b/src/components/security_manager/include/security_manager/security_manager_impl.h
@@ -241,12 +241,12 @@ class SecurityManagerImpl : public SecurityManager,
    * \brief Parse SecurityMessage as HandshakeData request
    * \param inMessage SecurityMessage with binary data of handshake
    */
-  bool ProccessHandshakeData(const SecurityMessage& inMessage);
+  bool ProcessHandshakeData(const SecurityMessage& inMessage);
   /**
    * \brief Parse InternalError from mobile side
    * \param inMessage SecurityMessage with binary data of handshake
    */
-  bool ProccessInternalError(const SecurityMessage& inMessage);
+  bool ProcessInternalError(const SecurityMessage& inMessage);
 
   /**
    * \brief Sends security query
diff --git a/src/components/security_manager/src/security_manager_impl.cc b/src/components/security_manager/src/security_manager_impl.cc
index a24e48289d..5ae8c35084 100644
--- a/src/components/security_manager/src/security_manager_impl.cc
+++ b/src/components/security_manager/src/security_manager_impl.cc
@@ -128,12 +128,12 @@ void SecurityManagerImpl::Handle(const SecurityMessage message) {
   }
   switch (message->get_header().query_id) {
     case SecurityQuery::SEND_HANDSHAKE_DATA:
-      if (!ProccessHandshakeData(message)) {
-        LOG4CXX_ERROR(logger_, "Proccess HandshakeData failed");
+      if (!ProcessHandshakeData(message)) {
+        LOG4CXX_ERROR(logger_, "Process HandshakeData failed");
       }
       break;
     case SecurityQuery::SEND_INTERNAL_ERROR:
-      if (!ProccessInternalError(message)) {
+      if (!ProcessInternalError(message)) {
         LOG4CXX_ERROR(logger_, "Processing income InternalError failed");
       }
       break;
@@ -515,7 +515,7 @@ bool SecurityManagerImpl::IsPolicyCertificateDataEmpty() {
   return false;
 }
 
-bool SecurityManagerImpl::ProccessHandshakeData(
+bool SecurityManagerImpl::ProcessHandshakeData(
     const SecurityMessage& inMessage) {
   LOG4CXX_INFO(logger_, "SendHandshakeData processing");
   DCHECK(inMessage);
@@ -556,11 +556,11 @@ bool SecurityManagerImpl::ProccessHandshakeData(
                                   &out_data_size);
   if (handshake_result == SSLContext::Handshake_Result_AbnormalFail) {
     // Do not return handshake data on AbnormalFail or null returned values
-    const std::string erorr_text(sslContext->LastError());
+    const std::string error_text(sslContext->LastError());
     LOG4CXX_ERROR(logger_,
-                  "SendHandshakeData: Handshake failed: " << erorr_text);
+                  "SendHandshakeData: Handshake failed: " << error_text);
     SendInternalError(
-        connection_key, ERROR_SSL_INVALID_DATA, erorr_text, seqNumber);
+        connection_key, ERROR_SSL_INVALID_DATA, error_text, seqNumber);
     NotifyListenersOnHandshakeDone(connection_key,
                                    SSLContext::Handshake_Result_Fail);
     // no handshake data to send
@@ -584,25 +584,29 @@ bool SecurityManagerImpl::ProccessHandshakeData(
   return true;
 }
 
-bool SecurityManagerImpl::ProccessInternalError(
+bool SecurityManagerImpl::ProcessInternalError(
     const SecurityMessage& inMessage) {
-  LOG4CXX_INFO(logger_,
-               "Received InternalError with Json message"
-                   << inMessage->get_json_message());
-  Json::Value root;
   std::string str = inMessage->get_json_message();
+  const uint32_t connection_key = inMessage->get_connection_key();
+  LOG4CXX_INFO(logger_, "Received InternalError with Json message" << str);
+  Json::Value root;
   utils::JsonReader reader;
 
   if (!reader.parse(str, &root)) {
     LOG4CXX_DEBUG(logger_, "Json parsing fails.");
     return false;
   }
+  uint8_t id = root[kErrId].asInt();
   LOG4CXX_DEBUG(logger_,
-                "Received InternalError id "
-                    << root[kErrId].asString()
-                    << ", text: " << root[kErrText].asString());
+                "Received InternalError id " << std::to_string(id) << ", text: "
+                                             << root[kErrText].asString());
+  if (ERROR_SSL_INVALID_DATA == id || ERROR_NOT_SUPPORTED == id) {
+    NotifyListenersOnHandshakeDone(connection_key,
+                                   SSLContext::Handshake_Result_Fail);
+  }
   return true;
 }
+
 void SecurityManagerImpl::SendHandshakeBinData(const uint32_t connection_key,
                                                const uint8_t* const data,
                                                const size_t data_size,
@@ -619,11 +623,11 @@ void SecurityManagerImpl::SendHandshakeBinData(const uint32_t connection_key,
 
 void SecurityManagerImpl::SendInternalError(const uint32_t connection_key,
                                             const uint8_t& error_id,
-                                            const std::string& erorr_text,
+                                            const std::string& error_text,
                                             const uint32_t seq_number) {
   Json::Value value;
   value[kErrId] = error_id;
-  value[kErrText] = erorr_text;
+  value[kErrText] = error_text;
   const std::string error_str = value.toStyledString();
   SecurityQuery::QueryHeader header(
       SecurityQuery::NOTIFICATION,
@@ -642,7 +646,7 @@ void SecurityManagerImpl::SendInternalError(const uint32_t connection_key,
   SendQuery(query, connection_key);
   LOG4CXX_DEBUG(logger_,
                 "Sent Internal error id " << static_cast<int>(error_id)
-                                          << " : \"" << erorr_text << "\".");
+                                          << " : \"" << error_text << "\".");
 }
 
 void SecurityManagerImpl::SendQuery(const SecurityQuery& query,
diff --git a/src/components/security_manager/test/security_manager_test.cc b/src/components/security_manager/test/security_manager_test.cc
index 7906ae7006..0cbe204f38 100644
--- a/src/components/security_manager/test/security_manager_test.cc
+++ b/src/components/security_manager/test/security_manager_test.cc
@@ -571,7 +571,7 @@ TEST_F(SecurityManagerTest, StartHandshake_SSLInternalError) {
  * Shall send InternallError on
  * getting SEND_HANDSHAKE_DATA with NULL data
  */
-TEST_F(SecurityManagerTest, ProccessHandshakeData_WrongDataSize) {
+TEST_F(SecurityManagerTest, ProcessHandshakeData_WrongDataSize) {
   SetMockCryptoManager();
   uint32_t connection_id = 0;
   uint8_t session_id = 0;
@@ -600,8 +600,7 @@ TEST_F(SecurityManagerTest, ProccessHandshakeData_WrongDataSize) {
  * getting SEND_HANDSHAKE_DATA from mobile side
  * for service which is not protected
  */
-TEST_F(SecurityManagerTest,
-       DISABLED_ProccessHandshakeData_ServiceNotProtected) {
+TEST_F(SecurityManagerTest, DISABLED_ProcessHandshakeData_ServiceNotProtected) {
   SetMockCryptoManager();
   // Expect InternalError with ERROR_ID
   uint32_t connection_id = 0;
@@ -649,7 +648,7 @@ TEST_F(SecurityManagerTest,
  * SEND_HANDSHAKE_DATA from mobile side with invalid handshake
  * data (DoHandshakeStep return NULL pointer)
  */
-TEST_F(SecurityManagerTest, ProccessHandshakeData_InvalidData) {
+TEST_F(SecurityManagerTest, ProcessHandshakeData_InvalidData) {
   SetMockCryptoManager();
 
   // Count handshake calls
@@ -732,7 +731,7 @@ TEST_F(SecurityManagerTest, ProccessHandshakeData_InvalidData) {
  * Shall send HandshakeData on getting SEND_HANDSHAKE_DATA from mobile side
  * with correct handshake data Check Fail and sussecc states
  */
-TEST_F(SecurityManagerTest, ProccessHandshakeData_Answer) {
+TEST_F(SecurityManagerTest, ProcessHandshakeData_Answer) {
   SetMockCryptoManager();
   // Count handshake calls
   const int handshake_emulates = 2;
@@ -808,7 +807,7 @@ TEST_F(SecurityManagerTest, ProccessHandshakeData_Answer) {
  * and return handshake data
  * Check Fail and sussecc states
  */
-TEST_F(SecurityManagerTest, ProccessHandshakeData_HandshakeFinished) {
+TEST_F(SecurityManagerTest, ProcessHandshakeData_HandshakeFinished) {
   SetMockCryptoManager();
   // Count handshake calls
   const int handshake_emulates = 6;