Updated to fix a problem with SSL server shutdown.

8 files changed, 279 insertions, 130 deletions

diff --git a/cpp/src/IceSSL/SslConnection.h b/cpp/src/IceSSL/SslConnection.h
index 906a4aed655..eb7aedb9e7f 100644
--- a/cpp/src/IceSSL/SslConnection.h
+++ b/cpp/src/IceSSL/SslConnection.h
@@ -29,7 +29,7 @@ public:
                const CertificateVerifierPtr&);
     virtual ~Connection();
 
-    virtual void shutdown() = 0;
+    virtual int shutdown(int timeout = 0) = 0;
 
     virtual int read(IceInternal::Buffer&, int) = 0;
     virtual int write(IceInternal::Buffer&, int) = 0;
diff --git a/cpp/src/IceSSL/SslConnectionOpenSSL.cpp b/cpp/src/IceSSL/SslConnectionOpenSSL.cpp
index f4b56da7cf9..043ccd06184 100644
--- a/cpp/src/IceSSL/SslConnectionOpenSSL.cpp
+++ b/cpp/src/IceSSL/SslConnectionOpenSSL.cpp
@@ -71,6 +71,9 @@ IceSSL::OpenSSL::Connection::Connection(const IceSSL::CertificateVerifierPtr& ce
 
     SSL_set_ex_data(sslConnection, 0, static_cast<void*>(plugin.get()));
 
+    // We always start off in a Handshake
+    _phase = Handshake;
+
     _lastError = SSL_ERROR_NONE;
 
     _initWantRead = 0;
@@ -94,39 +97,141 @@ IceSSL::OpenSSL::Connection::~Connection()
     }
 }
 
-void
-IceSSL::OpenSSL::Connection::shutdown()
+int
+IceSSL::OpenSSL::Connection::shutdown(int timeout)
 {
     if (_sslConnection == 0)
     {
-        return;
+        return 1;
     }
 
-    if (_traceLevels->security >= IceSSL::SECURITY_WARNINGS)
-    { 
-        _logger->trace(_traceLevels->securityCat, "WRN " +
-                       string("shutting down ssl connection\n") +
-                       fdToString(SSL_get_fd(_sslConnection)));
-    }
+    int retCode = 0;
 
-    int shutdown = 0;
-    int numRetries = 100;
-    int retries = -numRetries;
+    if (_initWantWrite)
+    {
+        int i = writeSelect(timeout);
 
-    do
+        if (i == 0)
+        {
+            return 0;
+        }
+
+        _initWantWrite = 0;
+    }
+    else if (_initWantRead)
     {
-        shutdown = SSL_shutdown(_sslConnection);
-        retries++;
+        int i = readSelect(timeout);
+
+        if (i == 0)
+        {
+            return 0;
+        }
+
+        _initWantRead = 0;
     }
-    while ((shutdown == 0) && (retries < 0));
 
-    if ((_traceLevels->security >= IceSSL::SECURITY_PROTOCOL) && (shutdown <= 0))
+    ERR_clear_error();
+
+    retCode = SSL_shutdown(_sslConnection);
+
+    if (retCode == 1)
+    {
+        // Shutdown successful - shut down the socket for writing.
+        ::shutdown(SSL_get_fd(_sslConnection), SHUT_WR);
+    }
+    else if (retCode == -1)
     {
-        ostringstream s;
-        s << "ssl shutdown failure encountered: code[" << shutdown << "] retries[";
-        s << (retries + numRetries) << "]\n" << fdToString(SSL_get_fd(_sslConnection));
-        _logger->trace(_traceLevels->securityCat, s.str());
+        setLastError(retCode);
+
+        // Shutdown failed due to an error.
+
+        switch (getLastError())
+        {
+            case SSL_ERROR_WANT_WRITE:
+            {
+                _initWantWrite = 1;
+                retCode = 0;
+                break;
+            }
+
+            case SSL_ERROR_WANT_READ:
+            {
+                _initWantRead = 1;
+                retCode = 0;
+                break;
+            }
+
+            case SSL_ERROR_NONE:
+            case SSL_ERROR_WANT_X509_LOOKUP:
+            {
+                // Ignore
+                retCode = 0;
+                break;
+            }
+
+            case SSL_ERROR_SYSCALL:
+            {
+                //
+                // Some error with the underlying transport.
+                //
+
+                if (interrupted())
+                {
+                    retCode = 0;
+                    break;
+                }
+
+                if (wouldBlock())
+                {
+                    readSelect(timeout);
+                    retCode = 0;
+                    break;
+                }
+
+                if (connectionLost())
+                {
+                    ConnectionLostException ex(__FILE__, __LINE__);
+                    ex.error = getSocketErrno();
+                    throw ex;
+                }
+
+                //
+                // Non-specific socket problem.
+                //
+                SocketException ex(__FILE__, __LINE__);
+                ex.error = getSocketErrno();
+                throw ex;
+            }
+
+            case SSL_ERROR_SSL:
+            {
+                //
+                // Error in the SSL library, usually a Protocol error.
+                //
+
+                ProtocolException protocolEx(__FILE__, __LINE__);
+
+                protocolEx._message = "encountered a violation of the ssl protocol during shutdown\n";
+                protocolEx._message += sslGetErrors();
+
+                throw protocolEx;
+            }
+
+            case SSL_ERROR_ZERO_RETURN:
+            {
+                //
+                // Indicates that the SSL connection has been closed. For SSLv3.0
+                // and TLSv1.0, it indicates that a closure alert was received,
+                // and thus the connection has been closed cleanly.
+                //
+
+		CloseConnectionException ex(__FILE__, __LINE__);
+		throw ex;
+            }
+        }
     }
+
+    return retCode;
 }
 
 void
@@ -223,7 +328,7 @@ IceSSL::OpenSSL::Connection::connect()
 {
     assert(_sslConnection != 0);
 
-    ERR_clear_error();

+    ERR_clear_error();
     int result = SSL_connect(_sslConnection);
 
     setLastError(result);
@@ -236,7 +341,7 @@ IceSSL::OpenSSL::Connection::accept()
 {
     assert(_sslConnection != 0);
 
-    ERR_clear_error();

+    ERR_clear_error();
     int result = SSL_accept(_sslConnection);
 
     setLastError(result);
@@ -288,7 +393,45 @@ IceSSL::OpenSSL::Connection::initialize(int timeout)
         {
             // Perform our init(), then leave.
             IceUtil::Mutex::Lock sync(_handshakeWaitMutex);
-            retCode = init(timeout);
+
+            // Here we 'take the ball and run with it' for as long as we can
+            // get away with it.  As long as we don't encounter some error
+            // status (or completion), this thread continues to service the
+            // initialize() call.
+            while (retCode == 0)
+            {
+                switch (_phase)
+                {
+                    case Handshake :
+                    {
+                        retCode = handshake(timeout);
+                        break;
+                    }
+
+                    case Shutdown : 
+                    {
+                        retCode = shutdown(timeout);
+                        break;
+                    }
+
+                    case Connected :
+                    {
+                        retCode = SSL_is_init_finished(_sslConnection);
+
+                        if (!retCode)
+                        {
+                            // In this case, we are essentially renegotiating
+                            // the connection at the behest of the peer.
+                            _phase = Handshake;
+                            continue;
+                        }
+
+                        // Done here.
+                        return retCode;
+                    }
+                }
+            }
+
             break;
         }
     }
@@ -306,7 +449,7 @@ IceSSL::OpenSSL::Connection::pending()
 int
 IceSSL::OpenSSL::Connection::getLastError() const
 {
-    assert(_sslConnection != 0);

+    assert(_sslConnection != 0);
     return SSL_get_error(_sslConnection, _lastError);
 }
 
@@ -315,7 +458,7 @@ IceSSL::OpenSSL::Connection::sslRead(char* buffer, int bufferSize)
 {
     assert(_sslConnection != 0);
 
-    ERR_clear_error();

+    ERR_clear_error();
     int bytesRead = SSL_read(_sslConnection, buffer, bufferSize);
 
     setLastError(bytesRead);
@@ -327,7 +470,7 @@ int
 IceSSL::OpenSSL::Connection::sslWrite(char* buffer, int bufferSize)
 {
     assert(_sslConnection != 0);
-

+
     ERR_clear_error();
     int bytesWritten = SSL_write(_sslConnection, buffer, bufferSize);
 
@@ -437,7 +580,7 @@ IceSSL::OpenSSL::Connection::read(Buffer& buf, int timeout)
             continue;
         }
 
-        // initReturn must be > 0, so we're okay to try a write
+        // initReturn must be > 0, so we're okay to try a read
 
         if (!pending() && !readSelect(_readTimeout))
         {
@@ -484,7 +627,7 @@ IceSSL::OpenSSL::Connection::read(Buffer& buf, int timeout)
             case SSL_ERROR_WANT_READ:
             {
                 if (!readSelect(timeout))
-                {

+                {
                     // Timeout and wait for them to arrive.
                     throw TimeoutException(__FILE__, __LINE__);
                 }
@@ -520,12 +663,10 @@ IceSSL::OpenSSL::Connection::read(Buffer& buf, int timeout)
                         ex.error = getSocketErrno();
                         throw ex;
                     }
-                    else
-                    {
-                        SocketException ex(__FILE__, __LINE__);
-                        ex.error = getSocketErrno();
-                        throw ex;
-                    }
+
+                    SocketException ex(__FILE__, __LINE__);
+                    ex.error = getSocketErrno();
+                    throw ex;
                 }
                 else // (bytesRead == 0)
                 {
diff --git a/cpp/src/IceSSL/SslConnectionOpenSSL.h b/cpp/src/IceSSL/SslConnectionOpenSSL.h
index c71b11e8a0b..0d08f83ccbc 100644
--- a/cpp/src/IceSSL/SslConnectionOpenSSL.h
+++ b/cpp/src/IceSSL/SslConnectionOpenSSL.h
@@ -116,6 +116,13 @@ private:
 //       would always be a reference to them from the map.
 typedef std::map<SSL*, Connection*> SslConnectionMap;
 
+typedef enum
+{
+    Handshake,  // The connection is negotiating a connection with the peer.
+    Shutdown,   // The connection is in the process of shutting down.
+    Connected   // The connection is connected - communication may continue.
+} ConnectPhase;
+
 class Connection : public IceSSL::Connection
 {
 public:
@@ -125,12 +132,12 @@ public:
                const IceSSL::PluginBaseIPtr&);
     virtual ~Connection();
 
-    virtual void shutdown();
+    virtual int shutdown(int timeout = 0);
 
     virtual int read(IceInternal::Buffer&, int);
     virtual int write(IceInternal::Buffer&, int) = 0;
 
-    virtual int init(int timeout = 0) = 0;
+    virtual int handshake(int timeout = 0) = 0;
 
     void setHandshakeReadTimeout(int timeout);
 
@@ -188,6 +195,8 @@ protected:
     int _initWantWrite;
     int _handshakeReadTimeout;
     int _readTimeout;
+
+    ConnectPhase _phase;
 };
 
 }
diff --git a/cpp/src/IceSSL/SslConnectionOpenSSLClient.cpp b/cpp/src/IceSSL/SslConnectionOpenSSLClient.cpp
index 5d2e247632b..dd7c527a8b8 100644
--- a/cpp/src/IceSSL/SslConnectionOpenSSLClient.cpp
+++ b/cpp/src/IceSSL/SslConnectionOpenSSLClient.cpp
@@ -50,14 +50,8 @@ IceSSL::OpenSSL::ClientConnection::~ClientConnection()
 {
 }
 
-void
-IceSSL::OpenSSL::ClientConnection::shutdown()
-{
-    Connection::shutdown();
-}
-
 int
-IceSSL::OpenSSL::ClientConnection::init(int timeout)
+IceSSL::OpenSSL::ClientConnection::handshake(int timeout)
 {
     assert(_sslConnection != 0);
 
@@ -65,32 +59,31 @@ IceSSL::OpenSSL::ClientConnection::init(int timeout)
 
     while (!retCode)
     {
-        int i = 0;
-
         _readTimeout = timeout > _handshakeReadTimeout ? timeout : _handshakeReadTimeout;
 
         if (_initWantRead)
         {
-            i = readSelect(_readTimeout);
+            int i = readSelect(_readTimeout);
+
+            if (i == 0)
+            {
+                return 0;
+            }
+
+            _initWantRead = 0;
         }
         else if (_initWantWrite)
         {
-            i = writeSelect(timeout);
-        }
+            int i = writeSelect(timeout);
 
-        if (_initWantRead && i == 0)
-        {
-            return 0;
-        }
+            if (i == 0)
+            {
+                return 0;
+            }
 
-        if (_initWantWrite && i == 0)
-        {
-            return 0;
+            _initWantWrite = 0;
         }
 
-        _initWantRead = 0;
-        _initWantWrite = 0;
-
         int result = connect();
 
         switch (getLastError())
@@ -121,6 +114,7 @@ IceSSL::OpenSSL::ClientConnection::init(int timeout)
                 // this define here as OpenSSL doesn't refer
                 // to it as a SOCKET_ERROR (but that's what it is
                 // if you look at their code).
+
                 if(result == -1)
                 {
                     // IO Error in underlying BIO
@@ -142,12 +136,10 @@ IceSSL::OpenSSL::ClientConnection::init(int timeout)
                         ex.error = getSocketErrno();
                         throw ex;
                     }
-                    else
-                    {
-                        SocketException ex(__FILE__, __LINE__);
-                        ex.error = getSocketErrno();
-                        throw ex;
-                    }
+
+                    SocketException ex(__FILE__, __LINE__);
+                    ex.error = getSocketErrno();
+                    throw ex;
                 }
                 else    // result == 0
                 {
@@ -158,6 +150,7 @@ IceSSL::OpenSSL::ClientConnection::init(int timeout)
                     // errno isn't set in this situation, so we always use
                     // ECONNREFUSED.
                     //
+
                     ConnectFailedException ex(__FILE__, __LINE__);
 #ifdef _WIN32
                     ex.error = WSAECONNREFUSED;
@@ -198,23 +191,25 @@ IceSSL::OpenSSL::ClientConnection::init(int timeout)
                     throw protocolEx;
                 }
             }
-

-            case SSL_ERROR_ZERO_RETURN:

-            {

-                // Indicates that that the SSL Connection has been closed.

-                // But does not necessarily indicate that the underlying transport

-                // has been closed (in the case of Ice, it definitely hasn't yet).

-

-                ConnectionLostException ex(__FILE__, __LINE__);

-                ex.error = getSocketErrno();

-                throw ex;

-            }

+
+            case SSL_ERROR_ZERO_RETURN:
+            {
+                // Indicates that that the SSL Connection has been closed.
+                // But does not necessarily indicate that the underlying transport
+                // has been closed (in the case of Ice, it definitely hasn't yet).
+
+                ConnectionLostException ex(__FILE__, __LINE__);
+                ex.error = getSocketErrno();
+                throw ex;
+            }
         }
 
         retCode = SSL_is_init_finished(_sslConnection);
 
         if (retCode > 0)
         {
+            _phase = Connected;
+
             // Init finished, look at the connection information.
             showConnectionInfo();
         }
@@ -324,12 +319,10 @@ IceSSL::OpenSSL::ClientConnection::write(Buffer& buf, int timeout)
 		        ex.error = getSocketErrno();
 		        throw ex;
 	            }
-	            else
-	            {
-		        SocketException ex(__FILE__, __LINE__);
-		        ex.error = getSocketErrno();
-		        throw ex;
-	            }
+
+                    SocketException ex(__FILE__, __LINE__);
+		    ex.error = getSocketErrno();
+		    throw ex;
                 }
                 else if (bytesWritten > 0)
                 {
diff --git a/cpp/src/IceSSL/SslConnectionOpenSSLClient.h b/cpp/src/IceSSL/SslConnectionOpenSSLClient.h
index ba55058b185..89903e5c8a8 100644
--- a/cpp/src/IceSSL/SslConnectionOpenSSLClient.h
+++ b/cpp/src/IceSSL/SslConnectionOpenSSLClient.h
@@ -28,8 +28,7 @@ public:
                      const IceSSL::PluginBaseIPtr&);
     virtual ~ClientConnection();
 
-    virtual void shutdown();
-    virtual int init(int timeout = 0);
+    virtual int handshake(int timeout = 0);
 
     virtual int write(IceInternal::Buffer&, int);
 
diff --git a/cpp/src/IceSSL/SslConnectionOpenSSLServer.cpp b/cpp/src/IceSSL/SslConnectionOpenSSLServer.cpp
index 46d48cf833d..14bb5dc75e3 100644
--- a/cpp/src/IceSSL/SslConnectionOpenSSLServer.cpp
+++ b/cpp/src/IceSSL/SslConnectionOpenSSLServer.cpp
@@ -52,14 +52,8 @@ IceSSL::OpenSSL::ServerConnection::~ServerConnection()
 {
 }
 
-void
-IceSSL::OpenSSL::ServerConnection::shutdown()
-{

-    Connection::shutdown();
-}
-
 int
-IceSSL::OpenSSL::ServerConnection::init(int timeout)
+IceSSL::OpenSSL::ServerConnection::handshake(int timeout)
 {
     assert(_sslConnection != 0);
 
@@ -67,16 +61,15 @@ IceSSL::OpenSSL::ServerConnection::init(int timeout)
     
     while (!retCode)
     {
-        int i = 0;
-
         _readTimeout = timeout > _handshakeReadTimeout ? timeout : _handshakeReadTimeout;
 
         if (_initWantWrite)
         {
-            i = writeSelect(timeout);
+            int i = writeSelect(timeout);
 
             if (i == 0)
             {
+                cerr << "-" << flush;
                 return 0;
             }
 
@@ -84,10 +77,11 @@ IceSSL::OpenSSL::ServerConnection::init(int timeout)
         }
         else
         {
-            i = readSelect(_readTimeout);
+            int i = readSelect(_readTimeout);
 
             if (i == 0)
             {
+                cerr << "-" << flush;
                 return 0;
             }
         }
@@ -102,19 +96,12 @@ IceSSL::OpenSSL::ServerConnection::init(int timeout)
 
             if (verifyError != X509_V_OK)
             {
-                CertificateVerificationException certVerEx(__FILE__, __LINE__);
-
-                certVerEx._message = "ssl certificate verification error";
-
-                string errors = sslGetErrors();
+                // Flag the connection for shutdown, let the
+                // usual initialization take care of it.
 
-                if (!errors.empty())
-                {
-                    certVerEx._message += "\n";
-                    certVerEx._message += errors;
-                }
+                _phase = Shutdown;
 
-                throw certVerEx;
+                return 0;
             }
             else
             {
@@ -169,15 +156,17 @@ IceSSL::OpenSSL::ServerConnection::init(int timeout)
                         ex.error = getSocketErrno();
                         throw ex;
                     }
-                    else
-                    {
-                        SocketException ex(__FILE__, __LINE__);
-                        ex.error = getSocketErrno();
-                        throw ex;
-                    }
+
+                    SocketException ex(__FILE__, __LINE__);
+                    ex.error = getSocketErrno();
+                    throw ex;
                 }
                 else
                 {
+                    //
+                    // NOTE: Should this be ConnectFailedException like in the Client?
+                    //
+
                     ProtocolException protocolEx(__FILE__, __LINE__);
 
                     // Protocol Error: Unexpected EOF
@@ -197,19 +186,21 @@ IceSSL::OpenSSL::ServerConnection::init(int timeout)
 
                 throw protocolEx;
             }
-

-            case SSL_ERROR_ZERO_RETURN:

-            {

-		ConnectionLostException ex(__FILE__, __LINE__);

-		ex.error = getSocketErrno();

-		throw ex;

-            }

+
+            case SSL_ERROR_ZERO_RETURN:
+            {
+		ConnectionLostException ex(__FILE__, __LINE__);
+		ex.error = getSocketErrno();
+		throw ex;
+            }
         }
 
         retCode = SSL_is_init_finished(_sslConnection);
 
         if (retCode > 0)
         {
+            _phase = Connected;
+
             // Init finished, look at the connection information.
             showConnectionInfo();
         }
@@ -309,12 +300,10 @@ IceSSL::OpenSSL::ServerConnection::write(Buffer& buf, int timeout)
                         ex.error = getSocketErrno();
                         throw ex;
 	            }
-	            else
-	            {
-		        SocketException ex(__FILE__, __LINE__);
-		        ex.error = getSocketErrno();
-		        throw ex;
-	            }
+
+                    SocketException ex(__FILE__, __LINE__);
+		    ex.error = getSocketErrno();
+		    throw ex;
                 }
                 else
                 {
diff --git a/cpp/src/IceSSL/SslConnectionOpenSSLServer.h b/cpp/src/IceSSL/SslConnectionOpenSSLServer.h
index f228775eb74..fb990670f68 100644
--- a/cpp/src/IceSSL/SslConnectionOpenSSLServer.h
+++ b/cpp/src/IceSSL/SslConnectionOpenSSLServer.h
@@ -28,8 +28,7 @@ public:
                      const IceSSL::PluginBaseIPtr&);
     virtual ~ServerConnection();
 
-    virtual void shutdown();
-    virtual int init(int timeout = 0);
+    virtual int handshake(int timeout = 0);
 
     virtual int write(IceInternal::Buffer&, int);
 
diff --git a/cpp/src/IceSSL/SslTransceiver.cpp b/cpp/src/IceSSL/SslTransceiver.cpp
index ebf5508e280..64dc4b002d7 100644
--- a/cpp/src/IceSSL/SslTransceiver.cpp
+++ b/cpp/src/IceSSL/SslTransceiver.cpp
@@ -40,7 +40,17 @@ IceSSL::SslTransceiver::close()
 
     SOCKET fd = _fd;
     _fd = INVALID_SOCKET;
-    _sslConnection->shutdown();
+
+    int shutdown = 0;
+    int numRetries = 100;
+    int retries = -numRetries;
+    do
+    {
+        shutdown = _sslConnection->shutdown();
+        retries++;
+    }
+    while ((shutdown == 0) && (retries < 0));
+
     ::shutdown(fd, SHUT_RDWR); // helps to unblock threads in recv()
     closeSocket(fd);
 }
@@ -55,7 +65,16 @@ IceSSL::SslTransceiver::shutdown()
 	_logger->trace(_traceLevels->networkCat, s.str());
     }
 
-    _sslConnection->shutdown();
+    int shutdown = 0;
+    int numRetries = 100;
+    int retries = -numRetries;
+    do
+    {
+        shutdown = _sslConnection->shutdown();
+        retries++;
+    }
+    while ((shutdown == 0) && (retries < 0));
+
     ::shutdown(_fd, SHUT_WR); // Shutdown socket for writing
 }