fix for bug 332: IceSSL: is handshake retry loop necessary?

15 files changed, 54 insertions, 123 deletions

diff --git a/cpp/CHANGES b/cpp/CHANGES
index 567f372dd3e..097f52b5d82 100644
--- a/cpp/CHANGES
+++ b/cpp/CHANGES
@@ -1,6 +1,9 @@
 Changes since version 2.1.1
 ---------------------------
 
+- Removed IceSSL's internal handshake retry loop, along with the
+  related property IceSSL.Client.Handshake.Retries.
+
 - Changed the Ice::Service class on Windows so that service failures
   do not cause the obscure message "Error1: Incorrect function".
 
diff --git a/cpp/config/PropertyNames.def b/cpp/config/PropertyNames.def
index ab564f188e3..57b97cbf074 100644
--- a/cpp/config/PropertyNames.def
+++ b/cpp/config/PropertyNames.def
@@ -325,7 +325,6 @@ IcePatch2:
 IceSSL:
     Client.CertPath
     Client.Config
-    Client.Handshake.Retries
     Client.IgnoreValidPeriod
     Client.Overrides.CACertificate
     Client.Overrides.DSA.Certificate
diff --git a/cpp/doc/Properties.sgml b/cpp/doc/Properties.sgml
index fd6304181d0..4a12b8c7b10 100644
--- a/cpp/doc/Properties.sgml
+++ b/cpp/doc/Properties.sgml
@@ -1431,27 +1431,6 @@ for the proper operation of the &IceSSL; plug-in.
 </section>
 </section>
 
-<section><title>IceSSL.Client.Handshake.Retries</title>
-<section><title>Synopsis</title>
-<synopsis>
-IceSSL.Client.Handshake.Retries=<replaceable>num</replaceable>
-</synopsis>
-</section>
-<section>
-<title>Description</title>
-<para>
-IceSSL clients attempt to perform an entire SSL handshake in the connection
-phase. When attempting this handshake, it is possible for the client to
-time out while waiting for a response from the server. This property specifies
-the number of handshake retries the client attempts before throwing a
-<literal>Ice::ConnectionFailedException</literal>. (C++ only)
-</para>
-<para>
-If not specified, the default value for this property is 10 retries.
-</para>
-</section>
-</section>
-
 <section><title>IceSSL.Client.Passphrase.Retries, IceSSL.Server.Passphrase.Retries</title>
 <section><title>Synopsis</title>
 <synopsis>
diff --git a/cpp/src/Ice/PropertyNames.cpp b/cpp/src/Ice/PropertyNames.cpp
index 97ac8cc7b62..e9bfeb811e3 100644
--- a/cpp/src/Ice/PropertyNames.cpp
+++ b/cpp/src/Ice/PropertyNames.cpp
@@ -7,7 +7,7 @@
 //
 // **********************************************************************
 
-// Generated by makeprops.py from file `../config/PropertyNames.def', Thu Jun  2 15:14:44 2005
+// Generated by makeprops.py from file `../config/PropertyNames.def', Tue Jun 14 10:24:30 2005
 
 // IMPORTANT: Do not edit this file -- any edits made here will be lost!
 
@@ -255,7 +255,6 @@ const char* IceInternal::PropertyNames::IceSSLProps[] =
 {
     "IceSSL.Client.CertPath",
     "IceSSL.Client.Config",
-    "IceSSL.Client.Handshake.Retries",
     "IceSSL.Client.IgnoreValidPeriod",
     "IceSSL.Client.Overrides.CACertificate",
     "IceSSL.Client.Overrides.DSA.Certificate",
diff --git a/cpp/src/Ice/PropertyNames.h b/cpp/src/Ice/PropertyNames.h
index 68cae66c143..0ca294e8e73 100644
--- a/cpp/src/Ice/PropertyNames.h
+++ b/cpp/src/Ice/PropertyNames.h
@@ -7,7 +7,7 @@
 //
 // **********************************************************************
 
-// Generated by makeprops.py from file `../config/PropertyNames.def', Thu Jun  2 15:14:44 2005
+// Generated by makeprops.py from file `../config/PropertyNames.def', Tue Jun 14 10:24:30 2005
 
 // IMPORTANT: Do not edit this file -- any edits made here will be lost!
 
diff --git a/cpp/src/IceSSL/ClientContext.cpp b/cpp/src/IceSSL/ClientContext.cpp
index 9ea7c7b8054..eaac6778704 100644
--- a/cpp/src/IceSSL/ClientContext.cpp
+++ b/cpp/src/IceSSL/ClientContext.cpp
@@ -60,11 +60,7 @@ IceSSL::ClientContext::createTransceiver(int socket, const OpenSSLPluginIPtr& pl
     }
 
     SSL* ssl = createSSLConnection(socket);
-    SslTransceiverPtr transceiver = new SslClientTransceiver(plugin, socket, _certificateVerifier, ssl);
-
-    transceiverSetup(transceiver, timeout);
-
-    return transceiver;
+    return new SslClientTransceiver(plugin, socket, _certificateVerifier, ssl, timeout);
 }
 
 IceSSL::ClientContext::ClientContext(const TraceLevelsPtr& traceLevels, const CommunicatorPtr& communicator) :
@@ -76,5 +72,4 @@ IceSSL::ClientContext::ClientContext(const TraceLevelsPtr& traceLevels, const Co
     _dsaPublicKeyProperty  = "IceSSL.Client.Overrides.DSA.Certificate";
     _caCertificateProperty = "IceSSL.Client.Overrides.CACertificate";
     _passphraseRetriesProperty = "IceSSL.Client.Passphrase.Retries";
-    _connectionHandshakeRetries = "IceSSL.Client.Handshake.Retries";
 }
diff --git a/cpp/src/IceSSL/Context.cpp b/cpp/src/IceSSL/Context.cpp
index 726007c8a34..e23f73866ac 100644
--- a/cpp/src/IceSSL/Context.cpp
+++ b/cpp/src/IceSSL/Context.cpp
@@ -609,26 +609,6 @@ IceSSL::Context::createSSLConnection(int socket)
 }
 
 void
-IceSSL::Context::transceiverSetup(const SslTransceiverPtr& transceiver, int timeout)
-{
-    // This timeout is implemented once on the first read after hanshake.
-    //
-    // Note: This is here because of some strange issue where SSL sometimes has long pauses
-    //       during handshake which cause timeouts.  If the IceSSL timeout is less than 5s,
-    //       there is a possibility (increasing in probability as you set the timeout lower)
-    //       that the handshake will faile due to a timeout.
-    //
-    transceiver->setHandshakeReadTimeout(timeout < 5000 ? 5000 : timeout);
-
-    // Note: Likewise, because the handshake has to be completed by a single thread, we do
-    //       handshake retries in the Transceiver.  This is obviously not what we'd like, but
-    //       this is due to the way the OpenSSL library handles handshaking.
-    //
-    int retries = _communicator->getProperties()->getPropertyAsIntWithDefault(_connectionHandshakeRetries, 10);
-    transceiver->setHandshakeRetries(retries);
-}
-
-void
 IceSSL::Context::setCipherList(const string& cipherList)
 {
     assert(_sslContext != 0);
diff --git a/cpp/src/IceSSL/Context.h b/cpp/src/IceSSL/Context.h
index 889ef6146a8..6e6ccb8588c 100644
--- a/cpp/src/IceSSL/Context.h
+++ b/cpp/src/IceSSL/Context.h
@@ -79,8 +79,6 @@ protected:
 
     SSL* createSSLConnection(int);
 
-    void transceiverSetup(const SslTransceiverPtr&, int);
-
     void setCipherList(const std::string&);
 
     void setDHParams(const BaseCertificates&);
diff --git a/cpp/src/IceSSL/ServerContext.cpp b/cpp/src/IceSSL/ServerContext.cpp
index e6213a5c043..068348cf9c4 100644
--- a/cpp/src/IceSSL/ServerContext.cpp
+++ b/cpp/src/IceSSL/ServerContext.cpp
@@ -81,11 +81,7 @@ IceSSL::ServerContext::createTransceiver(int socket, const OpenSSLPluginIPtr& pl
     }
 
     SSL* ssl = createSSLConnection(socket);
-    SslTransceiverPtr transceiver = new SslServerTransceiver(plugin, socket, _certificateVerifier, ssl);
-
-    transceiverSetup(transceiver, timeout);
-
-    return transceiver;
+    return new SslServerTransceiver(plugin, socket, _certificateVerifier, ssl, timeout);
 }
 
 //
@@ -101,7 +97,6 @@ IceSSL::ServerContext::ServerContext(const TraceLevelsPtr& traceLevels, const Co
     _dsaPublicKeyProperty  = "IceSSL.Server.Overrides.DSA.Certificate";
     _caCertificateProperty = "IceSSL.Server.Overrides.CACertificate";
     _passphraseRetriesProperty = "IceSSL.Server.Passphrase.Retries";
-    _connectionHandshakeRetries = "IceSSL.Server.Handshake.Retries";
 }
 
 void
diff --git a/cpp/src/IceSSL/SslClientTransceiver.cpp b/cpp/src/IceSSL/SslClientTransceiver.cpp
index e287c16d70b..f10eb44bcba 100644
--- a/cpp/src/IceSSL/SslClientTransceiver.cpp
+++ b/cpp/src/IceSSL/SslClientTransceiver.cpp
@@ -188,11 +188,9 @@ IceSSL::SslClientTransceiver::handshake(int timeout)
 
     while(!retCode)
     {
-        _readTimeout = timeout > _handshakeReadTimeout ? timeout : _handshakeReadTimeout;
-
         if(_initWantRead)
         {
-            int i = readSelect(_readTimeout);
+            int i = readSelect(timeout);
 
             if(i == 0)
             {
@@ -262,7 +260,7 @@ IceSSL::SslClientTransceiver::handshake(int timeout)
 
                     if(wouldBlock())
                     {
-                        readSelect(_readTimeout);
+                        readSelect(timeout);
                         break;
                     }
 
@@ -395,8 +393,9 @@ IceSSL::SslClientTransceiver::showConnectionInfo()
 IceSSL::SslClientTransceiver::SslClientTransceiver(const OpenSSLPluginIPtr& plugin,
                                                    SOCKET fd,
                                                    const CertificateVerifierPtr& certVerifier,
-                                                   SSL* sslConnection) :
-    SslTransceiver(plugin, fd, certVerifier, sslConnection)
+                                                   SSL* sslConnection,
+						   int timeout) :
+    SslTransceiver(plugin, fd, certVerifier, sslConnection, timeout)
 {
     // Set the Connect Connection state for this connection.
     SSL_set_connect_state(_sslConnection);
diff --git a/cpp/src/IceSSL/SslClientTransceiver.h b/cpp/src/IceSSL/SslClientTransceiver.h
index 037db6e607e..fa392f467f0 100644
--- a/cpp/src/IceSSL/SslClientTransceiver.h
+++ b/cpp/src/IceSSL/SslClientTransceiver.h
@@ -28,7 +28,7 @@ protected:
 
     virtual void showConnectionInfo();
 
-    SslClientTransceiver(const OpenSSLPluginIPtr&, SOCKET, const CertificateVerifierPtr&, SSL*);
+    SslClientTransceiver(const OpenSSLPluginIPtr&, SOCKET, const CertificateVerifierPtr&, SSL*, int);
     friend class ClientContext;
 };
 
diff --git a/cpp/src/IceSSL/SslServerTransceiver.cpp b/cpp/src/IceSSL/SslServerTransceiver.cpp
index 4acbd506500..f34ba6744a8 100644
--- a/cpp/src/IceSSL/SslServerTransceiver.cpp
+++ b/cpp/src/IceSSL/SslServerTransceiver.cpp
@@ -175,8 +175,6 @@ IceSSL::SslServerTransceiver::handshake(int timeout)
     
     while(!retCode)
     {
-        _readTimeout = timeout > _handshakeReadTimeout ? timeout : _handshakeReadTimeout;
-
         if(_initWantWrite)
         {
             int i = writeSelect(timeout);
@@ -190,7 +188,7 @@ IceSSL::SslServerTransceiver::handshake(int timeout)
         }
         else
         {
-            int i = readSelect(_readTimeout);
+            int i = readSelect(timeout);
 
             if(i == 0)
             {
@@ -265,7 +263,7 @@ IceSSL::SslServerTransceiver::handshake(int timeout)
 
                     if(wouldBlock())
                     {
-                        readSelect(_readTimeout);
+                        readSelect(timeout);
                         break;
                     }
 
@@ -363,8 +361,9 @@ IceSSL::SslServerTransceiver::showConnectionInfo()
 IceSSL::SslServerTransceiver::SslServerTransceiver(const OpenSSLPluginIPtr& plugin,
                                                    SOCKET fd,
                                                    const IceSSL::CertificateVerifierPtr& certVerifier,
-                                                   SSL* sslConnection) :
-    SslTransceiver(plugin, fd, certVerifier, sslConnection)
+                                                   SSL* sslConnection,
+						   int timeout) :
+    SslTransceiver(plugin, fd, certVerifier, sslConnection, timeout)
 {
     // Set the Accept Connection state for this connection.
     SSL_set_accept_state(sslConnection);
diff --git a/cpp/src/IceSSL/SslServerTransceiver.h b/cpp/src/IceSSL/SslServerTransceiver.h
index cea983c59c9..a1ae934d589 100644
--- a/cpp/src/IceSSL/SslServerTransceiver.h
+++ b/cpp/src/IceSSL/SslServerTransceiver.h
@@ -26,7 +26,7 @@ protected:
 
     virtual void showConnectionInfo();
 
-    SslServerTransceiver(const OpenSSLPluginIPtr&, SOCKET, const CertificateVerifierPtr&, SSL*);
+    SslServerTransceiver(const OpenSSLPluginIPtr&, SOCKET, const CertificateVerifierPtr&, SSL*, int);
     friend class ServerContext;
 };
 
diff --git a/cpp/src/IceSSL/SslTransceiver.cpp b/cpp/src/IceSSL/SslTransceiver.cpp
index b0ae47f270c..d0343372fb9 100644
--- a/cpp/src/IceSSL/SslTransceiver.cpp
+++ b/cpp/src/IceSSL/SslTransceiver.cpp
@@ -347,59 +347,49 @@ IceSSL::SslTransceiver::toString() const
 void
 IceSSL::SslTransceiver::forceHandshake()
 {
-    int retryCount = 0;
-
-    while(retryCount < _handshakeRetries)
+    try
     {
-	++retryCount;
-
-        try
-        {
-            if(handshake(_handshakeReadTimeout) > 0)
-            {
-                // Handshake complete.
-                break;
-            }
-        }
-        catch(TimeoutException)
-        {
-	    // Do nothing.
-        }
+	if(handshake(_readTimeout) > 0)
+	{
+	    return; // Handshake complete.
+	}
+    }
+    catch(const TimeoutException&)
+    {
+	// Fall through.
     }
 
-    if(retryCount >= _handshakeRetries)
+    if(_traceLevels->security >= IceSSL::SECURITY_WARNINGS)
     {
-        if(_traceLevels->security >= IceSSL::SECURITY_WARNINGS)
-        {
- 	    Trace out(_logger, _traceLevels->securityCat);
-            out << "Handshake retry maximum reached.\n" << toString();
-        }
+	Trace out(_logger, _traceLevels->securityCat);
+	if(_readTimeout >= 0)
+	{
+	    out << "Timeout occurred during SSL handshake.\n" << toString();
+	}
+	else
+	{
+	    out << "Failure occurred during SSL handshake.\n" << toString();
+	}
+    }
 
-        close();
+    close();
 
-        // If the handshake fails, we consider the connection as refused.
-        ConnectionRefusedException ex(__FILE__, __LINE__);
+    if(_readTimeout >= 0)
+    {
+	throw ConnectTimeoutException(__FILE__, __LINE__);
+    }
+    else
+    {
+	ConnectionRefusedException ex(__FILE__, __LINE__);
 #ifdef _WIN32
-        ex.error = WSAECONNREFUSED;
+	ex.error = WSAECONNREFUSED;
 #else
-        ex.error = ECONNREFUSED;
+	ex.error = ECONNREFUSED;
 #endif
-        throw ex;
+	throw ex;
     }
 }
 
-void
-IceSSL::SslTransceiver::setHandshakeReadTimeout(int timeout)
-{
-    _handshakeReadTimeout = timeout;
-}
-
-void
-IceSSL::SslTransceiver::setHandshakeRetries(int retries)
-{
-    _handshakeRetries = retries;
-}
-
 IceSSL::SslTransceiverPtr
 IceSSL::SslTransceiver::getTransceiver(SSL* sslPtr)
 {
@@ -1057,8 +1047,10 @@ IceSSL::SslTransceiver::showClientCAList(BIO* bio, const char* connType)
 IceSSL::SslTransceiver::SslTransceiver(const OpenSSLPluginIPtr& plugin,
                                        SOCKET fd,
 		                       const CertificateVerifierPtr& certificateVerifier,
-                                       SSL* sslConnection) :
+                                       SSL* sslConnection,
+				       int timeout) :
     _sslConnection(sslConnection),
+    _readTimeout(timeout),
     _plugin(plugin),
     _traceLevels(plugin->getTraceLevels()),
     _logger(plugin->getLogger()),
@@ -1081,9 +1073,6 @@ IceSSL::SslTransceiver::SslTransceiver(const OpenSSLPluginIPtr& plugin,
     _initWantRead = 0;
     _initWantWrite = 0;
 
-    _handshakeReadTimeout = 0;
-    _handshakeRetries = 0;
-
     // Set up the SSL to be able to refer back to our connection object.
     addTransceiver(_sslConnection, this);
 }
diff --git a/cpp/src/IceSSL/SslTransceiver.h b/cpp/src/IceSSL/SslTransceiver.h
index 549d78bb1e7..8d875451235 100644
--- a/cpp/src/IceSSL/SslTransceiver.h
+++ b/cpp/src/IceSSL/SslTransceiver.h
@@ -139,8 +139,6 @@ public:
 
     void forceHandshake();
     virtual int handshake(int timeout = 0) = 0;
-    void setHandshakeReadTimeout(int);
-    void setHandshakeRetries(int);
     static SslTransceiverPtr getTransceiver(SSL*);
 
     // Callback from OpenSSL for purposes of certificate verification
@@ -195,13 +193,11 @@ protected:
     SafeFlag _handshakeFlag;
     int _initWantRead;
     int _initWantWrite;
-    int _handshakeReadTimeout;
-    int _handshakeRetries;
     int _readTimeout;
 
     ConnectPhase _phase;
  
-    SslTransceiver(const OpenSSLPluginIPtr&, SOCKET, const IceSSL::CertificateVerifierPtr&, SSL*);
+    SslTransceiver(const OpenSSLPluginIPtr&, SOCKET, const IceSSL::CertificateVerifierPtr&, SSL*, int);
     virtual ~SslTransceiver();
 
     const OpenSSLPluginIPtr _plugin;