From 0d9217b4915bbede50ebb7c43f98fda17aaf9a83 Mon Sep 17 00:00:00 2001
From: Subodh Iyengar <subodh@fb.com>
Date: Thu, 28 Apr 2016 12:10:12 -0700
Subject: [PATCH] Better exception types to SSLexception

Summary:
Better enum exception types for SSLException.
These enum types are fairly high level, and more
info can be gleaned from the error.what()

Reviewed By: knekritz

Differential Revision: D3234501

fb-gh-sync-id: 7cd4fbccd7f4367354dc3bd1fe4cd480d58d6173
fbshipit-source-id: 7cd4fbccd7f4367354dc3bd1fe4cd480d58d6173
---
 folly/io/async/ssl/SSLErrors.cpp           | 35 ++++++++++++++--------
 folly/io/async/ssl/SSLErrors.h             | 20 ++-----------
 folly/io/async/test/AsyncSSLSocketTest.cpp |  8 ++---
 3 files changed, 30 insertions(+), 33 deletions(-)

diff --git a/folly/io/async/ssl/SSLErrors.cpp b/folly/io/async/ssl/SSLErrors.cpp
index 94550f5d..7bbb78c3 100644
--- a/folly/io/async/ssl/SSLErrors.cpp
+++ b/folly/io/async/ssl/SSLErrors.cpp
@@ -29,14 +29,14 @@ std::string decodeOpenSSLError(
     int sslOperationReturnValue) {
   if (sslError == SSL_ERROR_SYSCALL && errError == 0) {
     if (sslOperationReturnValue == 0) {
-      return "SSL_ERROR_SYSCALL: EOF";
+      return "Connection EOF";
     } else {
       // In this case errno is set, AsyncSocketException will add it.
-      return "SSL_ERROR_SYSCALL";
+      return "Network error";
     }
   } else if (sslError == SSL_ERROR_ZERO_RETURN) {
     // This signifies a TLS closure alert.
-    return "SSL_ERROR_ZERO_RETURN";
+    return "SSL connection closed normally";
   } else {
     std::array<char, 256> buf;
     std::string msg(ERR_error_string(errError, buf.data()));
@@ -56,9 +56,14 @@ const StringPiece getSSLErrorString(SSLError error) {
     case SSLError::EARLY_WRITE:
       ret = "Attempt to write before SSL connection established";
       break;
-    case SSLError::OPENSSL_ERR:
-      // decodeOpenSSLError should be used for this type.
-      ret = "OPENSSL error";
+    case SSLError::SSL_ERROR:
+      ret = "SSL error";
+      break;
+    case SSLError::NETWORK_ERROR:
+      ret = "Network error";
+      break;
+    case SSLError::EOF_ERROR:
+      ret = "SSL connection closed normally";
       break;
   }
   return ret;
@@ -68,17 +73,23 @@ const StringPiece getSSLErrorString(SSLError error) {
 namespace folly {
 
 SSLException::SSLException(
-    int sslError,
+    int sslErr,
     unsigned long errError,
     int sslOperationReturnValue,
     int errno_copy)
     : AsyncSocketException(
           AsyncSocketException::SSL_ERROR,
-          decodeOpenSSLError(sslError, errError, sslOperationReturnValue),
-          sslError == SSL_ERROR_SYSCALL ? errno_copy : 0),
-      sslError(SSLError::OPENSSL_ERR),
-      opensslSSLError(sslError),
-      opensslErr(errError) {}
+          decodeOpenSSLError(sslErr, errError, sslOperationReturnValue),
+          sslErr == SSL_ERROR_SYSCALL ? errno_copy : 0) {
+  if (sslErr == SSL_ERROR_ZERO_RETURN) {
+    sslError = SSLError::EOF_ERROR;
+  } else if (sslErr == SSL_ERROR_SYSCALL) {
+    sslError = SSLError::NETWORK_ERROR;
+  } else {
+    // Conservatively assume that this is an SSL error
+    sslError = SSLError::SSL_ERROR;
+  }
+}
 
 SSLException::SSLException(SSLError error)
     : AsyncSocketException(
diff --git a/folly/io/async/ssl/SSLErrors.h b/folly/io/async/ssl/SSLErrors.h
index ad7a5de4..5bd37fde 100644
--- a/folly/io/async/ssl/SSLErrors.h
+++ b/folly/io/async/ssl/SSLErrors.h
@@ -24,11 +24,9 @@ enum class SSLError {
   CLIENT_RENEGOTIATION, // A client tried to renegotiate with this server
   INVALID_RENEGOTIATION, // We attempted to start a renegotiation.
   EARLY_WRITE, // Wrote before SSL connection established.
-  // An openssl error type. The openssl specific methods should be used
-  // to find the real error type.
-  // This exists for compatibility until all error types can be move to proper
-  // errors.
-  OPENSSL_ERR,
+  SSL_ERROR, // An error related to SSL
+  NETWORK_ERROR, // An error related to the network.
+  EOF_ERROR, // The peer terminated the connection correctly.
 };
 
 class SSLException : public folly::AsyncSocketException {
@@ -45,19 +43,7 @@ class SSLException : public folly::AsyncSocketException {
     return sslError;
   }
 
-  // These methods exist for compatibility until there are proper exceptions
-  // for all ssl error types.
-  int getOpensslSSLError() const {
-    return opensslSSLError;
-  }
-
-  unsigned long getOpensslErr() const {
-    return opensslErr;
-  }
-
  private:
   SSLError sslError;
-  int opensslSSLError;
-  unsigned long opensslErr;
 };
 }
diff --git a/folly/io/async/test/AsyncSSLSocketTest.cpp b/folly/io/async/test/AsyncSSLSocketTest.cpp
index bd0242aa..9feed78e 100644
--- a/folly/io/async/test/AsyncSSLSocketTest.cpp
+++ b/folly/io/async/test/AsyncSSLSocketTest.cpp
@@ -1609,8 +1609,8 @@ TEST(AsyncSSLSocketTest, ConnResetErrorString) {
   socket->closeWithReset();
 
   handshakeCallback.waitForHandshake();
-  EXPECT_NE(handshakeCallback.errorString_.find("SSL_ERROR_SYSCALL"),
-            std::string::npos);
+  EXPECT_NE(
+      handshakeCallback.errorString_.find("Network error"), std::string::npos);
   EXPECT_NE(handshakeCallback.errorString_.find("104"), std::string::npos);
 }
 
@@ -1630,8 +1630,8 @@ TEST(AsyncSSLSocketTest, ConnEOFErrorString) {
   socket->close();
 
   handshakeCallback.waitForHandshake();
-  EXPECT_NE(handshakeCallback.errorString_.find("SSL_ERROR_SYSCALL"),
-            std::string::npos);
+  EXPECT_NE(
+      handshakeCallback.errorString_.find("Connection EOF"), std::string::npos);
   EXPECT_NE(handshakeCallback.errorString_.find("EOF"), std::string::npos);
 }
 
-- 
2.34.1