Various SSL fixes, tests for intermedate CAs

5 files changed, 22 insertions, 13 deletions

diff --git a/cpp/src/IceSSL/OpenSSLEngine.cpp b/cpp/src/IceSSL/OpenSSLEngine.cpp
index 8dfde2f45fd..d6caa8f4e53 100644
--- a/cpp/src/IceSSL/OpenSSLEngine.cpp
+++ b/cpp/src/IceSSL/OpenSSLEngine.cpp
@@ -812,7 +812,7 @@ OpenSSLEngine::initialize()
         // Determine whether a certificate is required from the peer.
         //
         {
-            int sslVerifyMode = SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT;;
+            int sslVerifyMode = SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT;
             switch(getVerifyPeer())
             {
                 case 0:
@@ -899,7 +899,14 @@ OpenSSLEngine::verifyCallback(int ok, SSL* ssl, X509_STORE_CTX* c)
         out << "error = " << X509_verify_cert_error_string(err) << '\n';
         out << IceInternal::fdToString(SSL_get_fd(ssl));
     }
-    return ok;
+
+    //
+    // Always return 1 to prevent SSL_connect/SSL_accept from
+    // returning SSL_ERROR_SSL for verification failures. This ensure
+    // that we can raise SecurityException for verification failures
+    // rather than a ProtocolException.
+    //
+    return 1;
 }
 
 #  ifndef OPENSSL_NO_DH
diff --git a/cpp/src/IceSSL/OpenSSLTransceiverI.cpp b/cpp/src/IceSSL/OpenSSLTransceiverI.cpp
index 5edf2874221..5acd71dc8f3 100644
--- a/cpp/src/IceSSL/OpenSSLTransceiverI.cpp
+++ b/cpp/src/IceSSL/OpenSSLTransceiverI.cpp
@@ -125,7 +125,6 @@ IceSSL::TransceiverI::initialize(IceInternal::Buffer& readBuffer, IceInternal::B
 #if defined(OPENSSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER < 0x100000bfL
         sync.release();
 #endif
-
         if(ret <= 0)
         {
             switch(SSL_get_error(_ssl, ret))
diff --git a/cpp/src/IceSSL/SChannelTransceiverI.cpp b/cpp/src/IceSSL/SChannelTransceiverI.cpp
index 588d21917ef..8f79d63df1d 100644
--- a/cpp/src/IceSSL/SChannelTransceiverI.cpp
+++ b/cpp/src/IceSSL/SChannelTransceiverI.cpp
@@ -645,7 +645,7 @@ IceSSL::TransceiverI::initialize(IceInternal::Buffer& readBuffer, IceInternal::B
         SECURITY_STATUS err = QueryContextAttributes(&_ssl, SECPKG_ATTR_REMOTE_CERT_CONTEXT, &cert);
         if(err && err != SEC_E_NO_CREDENTIALS)
         {
-            throw ProtocolException(__FILE__, __LINE__, "IceSSL: certificate verification failure:" +
+            throw SecurityException(__FILE__, __LINE__, "IceSSL: certificate verification failure:" +
                                     IceUtilInternal::lastErrorToString());
         }
 
@@ -653,7 +653,7 @@ IceSSL::TransceiverI::initialize(IceInternal::Buffer& readBuffer, IceInternal::B
         {
             // Clients require server certificate if VerifyPeer > 0
             // and servers require client certificate if VerifyPeer == 2
-            throw ProtocolException(__FILE__, __LINE__, "IceSSL: certificate required:" +
+            throw SecurityException(__FILE__, __LINE__, "IceSSL: certificate required:" +
                                     IceUtilInternal::lastErrorToString());
         }
         else if(cert) // Verify the remote certificate
@@ -706,7 +706,7 @@ IceSSL::TransceiverI::initialize(IceInternal::Buffer& readBuffer, IceInternal::B
                     {
                         _instance->logger()->trace(_instance->traceCategory(), msg);
                     }
-                    throw ProtocolException(__FILE__, __LINE__, msg);
+                    throw SecurityException(__FILE__, __LINE__, msg);
                 }
             }
         }
diff --git a/cpp/src/IceSSL/SecureTransportTransceiverI.cpp b/cpp/src/IceSSL/SecureTransportTransceiverI.cpp
index c384def1f59..3feb7774e86 100644
--- a/cpp/src/IceSSL/SecureTransportTransceiverI.cpp
+++ b/cpp/src/IceSSL/SecureTransportTransceiverI.cpp
@@ -112,7 +112,7 @@ checkTrustResult(SecTrustRef trust, const SecureTransportEnginePtr& engine, cons
         //
         if((err = SecTrustSetNetworkFetchAllowed(trust, false)))
         {
-            throw ProtocolException(__FILE__, __LINE__, "IceSSL: handshake failure:\n" + errorToString(err));
+            throw SecurityException(__FILE__, __LINE__, "IceSSL: handshake failure:\n" + errorToString(err));
         }
 
         //
@@ -120,7 +120,7 @@ checkTrustResult(SecTrustRef trust, const SecureTransportEnginePtr& engine, cons
         //
         if((err = SecTrustEvaluate(trust, &trustResult)))
         {
-            throw ProtocolException(__FILE__, __LINE__, "IceSSL: handshake failure:\n" + errorToString(err));
+            throw SecurityException(__FILE__, __LINE__, "IceSSL: handshake failure:\n" + errorToString(err));
         }
     }
 
@@ -160,7 +160,7 @@ checkTrustResult(SecTrustRef trust, const SecureTransportEnginePtr& engine, cons
             {
                 instance->logger()->trace(instance->traceCategory(), msg);
             }
-            throw ProtocolException(__FILE__, __LINE__, msg);
+            throw SecurityException(__FILE__, __LINE__, msg);
         }
     }
     }
@@ -226,9 +226,12 @@ IceSSL::TransceiverI::initialize(IceInternal::Buffer& readBuffer, IceInternal::B
             err = SSLCopyPeerTrust(_ssl, &_trust);
             if(_incoming && err == errSSLBadCert && _engine->getVerifyPeer() == 1)
             {
-                // This happens in 10.10 when the client doesn't provide
-                // a certificate and the server is configured to try
-                // authenticate
+                //
+                // This is expected if the client doesn't provide a
+                // certificate (occurs since 10.10). The server is
+                // configured to verify to not require the client
+                // certificate so we ignore the failure.
+                //
                 continue;
             }
             if(err == noErr)
diff --git a/cpp/src/IceSSL/Util.cpp b/cpp/src/IceSSL/Util.cpp
index 19713f79364..f87e361c194 100644
--- a/cpp/src/IceSSL/Util.cpp
+++ b/cpp/src/IceSSL/Util.cpp
@@ -579,7 +579,7 @@ loadKeychainItems(const string& file, SecExternalItemType type, SecKeychainRef k
         throw CertificateReadException(__FILE__, __LINE__, os.str());
     }
 
-    if(type != kSecItemTypeUnknown && importType != type)
+    if(type != kSecItemTypeUnknown && importType != kSecItemTypeAggregate && importType != type)
     {
         CFRelease(items);
         ostringstream os;