Enable SNI extension with CheckCertName > 1

- Remove IceSSL.ServerNameIndication, IceSSL.CheckCertName = 2 should be used instead. - Minor style fixes
zeroc-ice · Sep 6, 2019 · f77a9b7 · f77a9b7
1 parent 59d4929
commit f77a9b7
Show file tree

Hide file tree

Showing 16 changed files with 34 additions and 54 deletions.
diff --git a/CHANGELOG-3.7.md b/CHANGELOG-3.7.md
@@ -65,12 +65,12 @@ These are the changes since Ice 3.7.2.
   the backward compatible invocation timeout -2. The invocation failed instead
   of being retried.
 
-- Add property `IceSSL.ServerNameIndication` to send the SNI extension in TLS
-  connections (defaults to 1). This is helpful, for example, if a web server in
-  front of Ice secure WebSocket (WSS) endpoints needs to decide the certificate
-  and backend based on requested hostname. This is supported by the OpenSSL,
-  macOS (SecureTransport) and Java backends; these backends did not send the
-  extension before. The C# and Windows (SChannel) backends always send the
+- Add support to enable SNI (Server Name Indication) extension in TLS outgoing
+  connections for SSL engines that make this optional, the SNI TLS extension
+  can be enabled by setting `IceSSL.CheckCertName` to a value greater than 1.
+
+  This is supported by the OpenSSL, SecureTransport and Java; these engines did
+  not send the extension before. The C# and SChannel engines always send the
   extension. The Java-Compat mapping does not support it.
 
 ## C++ Changes

diff --git a/config/PropertyNames.xml b/config/PropertyNames.xml
@@ -594,7 +594,6 @@ generated from the section label.
         <property name="ProtocolVersionMin" />
         <property name="Random" />
         <property name="SchannelStrongCrypto" />
-        <property name="ServerNameIndication" />
         <property name="Trace.Security" />
         <property name="TrustOnly" />
         <property name="TrustOnly.Client" />

diff --git a/cpp/src/Ice/PropertyNames.cpp b/cpp/src/Ice/PropertyNames.cpp
@@ -1,7 +1,7 @@
 //
 // Copyright (c) ZeroC, Inc. All rights reserved.
 //
-// Generated by makeprops.py from file ./config/PropertyNames.xml, Mon Aug 19 09:05:47 2019
+// Generated by makeprops.py from file ./config/PropertyNames.xml, Fri Sep  6 18:11:04 2019
 
 // IMPORTANT: Do not edit this file -- any edits made here will be lost!
 
@@ -1149,7 +1149,6 @@ const IceInternal::Property IceSSLPropsData[] =
     IceInternal::Property("IceSSL.ProtocolVersionMin", false, 0),
     IceInternal::Property("IceSSL.Random", false, 0),
     IceInternal::Property("IceSSL.SchannelStrongCrypto", false, 0),
-    IceInternal::Property("IceSSL.ServerNameIndication", false, 0),
     IceInternal::Property("IceSSL.Trace.Security", false, 0),
     IceInternal::Property("IceSSL.TrustOnly", false, 0),
     IceInternal::Property("IceSSL.TrustOnly.Client", false, 0),

diff --git a/cpp/src/Ice/PropertyNames.h b/cpp/src/Ice/PropertyNames.h
@@ -1,7 +1,7 @@
 //
 // Copyright (c) ZeroC, Inc. All rights reserved.
 //
-// Generated by makeprops.py from file ../config/PropertyNames.xml, Wed May 29 21:06:31 2019
+// Generated by makeprops.py from file ./config/PropertyNames.xml, Fri Sep  6 18:11:04 2019
 
 // IMPORTANT: Do not edit this file -- any edits made here will be lost!
 

diff --git a/cpp/src/IceSSL/OpenSSLTransceiverI.cpp b/cpp/src/IceSSL/OpenSSLTransceiverI.cpp
@@ -188,14 +188,14 @@ OpenSSL::TransceiverI::initialize(IceInternal::Buffer& readBuffer, IceInternal::
             SSL_set_verify(_ssl, sslVerifyMode, IceSSL_opensslVerifyCallback);
         }
 
-        // Server name indication
-        if (!_incoming && _engine->getServerNameIndication() && !_host.empty() && !IceInternal::isIpAddress(_host))
+        //
+        // Enable SNI
+        //
+        if(!_incoming && _engine->getServerNameIndication() && !_host.empty() && !IceInternal::isIpAddress(_host))
         {
-            if (!SSL_set_tlsext_host_name(_ssl, _host.c_str()))
+            if(!SSL_set_tlsext_host_name(_ssl, _host.c_str()))
             {
-                ostringstream ostr;
-                ostr << "IceSSL: failed to set SNI host " << _host << " with SSL_set_tlsext_host_name";
-                throw SecurityException(__FILE__, __LINE__, ostr.str());
+                throw SecurityException(__FILE__, __LINE__, "IceSSL: setting SNI host failed `" + _host + "'");
             }
         }
     }

diff --git a/cpp/src/IceSSL/SChannelEngine.cpp b/cpp/src/IceSSL/SChannelEngine.cpp
@@ -1145,8 +1145,6 @@ SChannel::SSLEngine::getCipherName(ALG_ID cipher) const
 CredHandle
 SChannel::SSLEngine::newCredentialsHandle(bool incoming)
 {
-    // SNI support: SChannel will send the TLS extension even if SCH_CRED_SNI_CREDENTIAL (undocumented!)
-    // is not specified. Therefore, the IceSSL.ServerNameIndication property is not used here.
     SCHANNEL_CRED cred;
     memset(&cred, 0, sizeof(cred));
     cred.dwVersion = SCHANNEL_CRED_VERSION;

diff --git a/cpp/src/IceSSL/SSLEngine.cpp b/cpp/src/IceSSL/SSLEngine.cpp
@@ -110,10 +110,10 @@ IceSSL::SSLEngine::initialize()
     _checkCertName = properties->getPropertyAsIntWithDefault(propPrefix + "CheckCertName", 0) > 0;
 
     //
-    // ServerNameIndication determines whether the SNI extension applies to client connections,
+    // CheckCertName > 1 enables SNI, the SNI extension applies to client connections,
     // indicating the hostname to the server (must be DNS hostname, not an IP address).
     //
-    _serverNameIndication = properties->getPropertyAsIntWithDefault(propPrefix + "ServerNameIndication", 1) > 0;
+    _serverNameIndication = properties->getPropertyAsIntWithDefault(propPrefix + "CheckCertName", 0) > 1;
 
     //
     // VerifyDepthMax establishes the maximum length of a peer's certificate

diff --git a/cpp/src/IceSSL/SecureTransportTransceiverI.cpp b/cpp/src/IceSSL/SecureTransportTransceiverI.cpp
@@ -243,13 +243,15 @@ IceSSL::SecureTransport::TransceiverI::initialize(IceInternal::Buffer& readBuffe
                                     sslErrorToString(err));
         }
 
-        if (!_incoming && _engine->getServerNameIndication() && !_host.empty() && !IceInternal::isIpAddress(_host))
+        //
+        // Enable SNI
+        //
+        if(!_incoming && _engine->getServerNameIndication() && !_host.empty() && !IceInternal::isIpAddress(_host))
         {
-            if ((err = SSLSetPeerDomainName(_ssl.get(), _host.data(), _host.length())))
+            if((err = SSLSetPeerDomainName(_ssl.get(), _host.data(), _host.length())))
             {
-                ostringstream ostr;
-                ostr << "IceSSL: failed to set SNI host " << _host << " with SSLSetPeerDomainName\n" << sslErrorToString(err);
-                throw SecurityException(__FILE__, __LINE__, ostr.str());
+                throw SecurityException(__FILE__, __LINE__, "IceSSL: setting SNI host failed `" + _host + "'\n" +
+                                        sslErrorToString(err));
             }
         }
     }

diff --git a/cpp/src/IceSSL/UWPTransceiverI.cpp b/cpp/src/IceSSL/UWPTransceiverI.cpp
@@ -105,10 +105,6 @@ UWP::TransceiverI::getNativeInfo()
 IceInternal::SocketOperation
 UWP::TransceiverI::initialize(IceInternal::Buffer& readBuffer, IceInternal::Buffer& writeBuffer)
 {
-    // SNI support (TODO): not tested for UWP. The assumption is that `UpgradeToSslAsync`
-    // (https://docs.microsoft.com/en-us/uwp/api/windows.networking.sockets.streamsocket.upgradetosslasync)
-    // might always send the extension.
-
     if(!_connected)
     {
         IceInternal::SocketOperation status = _delegate->initialize(readBuffer, writeBuffer);

diff --git a/csharp/src/Ice/PropertyNames.cs b/csharp/src/Ice/PropertyNames.cs
@@ -1,7 +1,7 @@
 //
 // Copyright (c) ZeroC, Inc. All rights reserved.
 //
-// Generated by makeprops.py from file ./config/PropertyNames.xml, Mon Aug 19 09:05:47 2019
+// Generated by makeprops.py from file ./config/PropertyNames.xml, Fri Sep  6 18:11:04 2019
 
 // IMPORTANT: Do not edit this file -- any edits made here will be lost!
 
@@ -1118,7 +1118,6 @@ public sealed class PropertyNames
              new Property(@"^IceSSL\.ProtocolVersionMin$", false, null),
              new Property(@"^IceSSL\.Random$", false, null),
              new Property(@"^IceSSL\.SchannelStrongCrypto$", false, null),
-             new Property(@"^IceSSL\.ServerNameIndication$", false, null),
              new Property(@"^IceSSL\.Trace\.Security$", false, null),
              new Property(@"^IceSSL\.TrustOnly$", false, null),
              new Property(@"^IceSSL\.TrustOnly\.Client$", false, null),

diff --git a/csharp/src/IceSSL/TransceiverI.cs b/csharp/src/IceSSL/TransceiverI.cs
@@ -400,10 +400,6 @@ private bool startAuthenticate(IceInternal.AsyncCallback callback, object state)
                     //
                     // Client authentication.
                     //
-                    // SNI support: SslStream always sends the TLS extension (not configurable). See
-                    // https://github.com/dotnet/corefx/issues/17427 for older versions which don't
-                    // have it yet. Therefore, the IceSSL.ServerNameIndication property is not used here.
-                    //
                     _writeResult = _sslStream.BeginAuthenticateAsClient(_host,
                                                                         _instance.certs(),
                                                                         _instance.protocols(),

diff --git a/java-compat/src/Ice/src/main/java/IceInternal/PropertyNames.java b/java-compat/src/Ice/src/main/java/IceInternal/PropertyNames.java
@@ -1,7 +1,7 @@
 //
 // Copyright (c) ZeroC, Inc. All rights reserved.
 //
-// Generated by makeprops.py from file ./config/PropertyNames.xml, Mon Aug 19 09:05:47 2019
+// Generated by makeprops.py from file ./config/PropertyNames.xml, Fri Sep  6 18:11:04 2019
 
 // IMPORTANT: Do not edit this file -- any edits made here will be lost!
 
@@ -1118,7 +1118,6 @@ public final class PropertyNames
         new Property("IceSSL\\.ProtocolVersionMin", false, null),
         new Property("IceSSL\\.Random", false, null),
         new Property("IceSSL\\.SchannelStrongCrypto", false, null),
-        new Property("IceSSL\\.ServerNameIndication", false, null),
         new Property("IceSSL\\.Trace\\.Security", false, null),
         new Property("IceSSL\\.TrustOnly", false, null),
         new Property("IceSSL\\.TrustOnly\\.Client", false, null),

diff --git a/java-compat/src/Ice/src/main/java/IceSSL/SSLEngine.java b/java-compat/src/Ice/src/main/java/IceSSL/SSLEngine.java
@@ -872,10 +872,6 @@ else if(_verifyPeer == 1)
             }
         }
 
-        // SNI support: Java prior to version 8 do not have this feature in the SSLParameters
-        // class (https://docs.oracle.com/javase/7/docs/api/javax/net/ssl/SSLParameters.html).
-        // Therefore, Ice java-compat does not support SNI.
-
         try
         {
             engine.beginHandshake();

diff --git a/java/src/Ice/src/main/java/com/zeroc/IceInternal/PropertyNames.java b/java/src/Ice/src/main/java/com/zeroc/IceInternal/PropertyNames.java
@@ -1,7 +1,7 @@
 //
 // Copyright (c) ZeroC, Inc. All rights reserved.
 //
-// Generated by makeprops.py from file ./config/PropertyNames.xml, Mon Aug 19 09:05:47 2019
+// Generated by makeprops.py from file ./config/PropertyNames.xml, Fri Sep  6 18:11:04 2019
 
 // IMPORTANT: Do not edit this file -- any edits made here will be lost!
 
@@ -1118,7 +1118,6 @@ public final class PropertyNames
         new Property("IceSSL\\.ProtocolVersionMin", false, null),
         new Property("IceSSL\\.Random", false, null),
         new Property("IceSSL\\.SchannelStrongCrypto", false, null),
-        new Property("IceSSL\\.ServerNameIndication", false, null),
         new Property("IceSSL\\.Trace\\.Security", false, null),
         new Property("IceSSL\\.TrustOnly", false, null),
         new Property("IceSSL\\.TrustOnly\\.Client", false, null),

diff --git a/java/src/IceSSL/src/main/java/com/zeroc/IceSSL/SSLEngine.java b/java/src/IceSSL/src/main/java/com/zeroc/IceSSL/SSLEngine.java
@@ -90,10 +90,10 @@ else if(s.equals("TLS1_3") || s.equals("TLSV1_3"))
         _checkCertName = properties.getPropertyAsIntWithDefault(prefix + "CheckCertName", 0) > 0;
 
         //
-        // ServerNameIndication determines whether the SNI extension applies to client connections,
+        // CheckCertName > 1 enables SNI, the SNI extension applies to client connections,
         // indicating the hostname to the server (must be DNS hostname, not an IP address).
         //
-        _serverNameIndication = properties.getPropertyAsIntWithDefault(prefix + "ServerNameIndication", 1) > 0;
+        _serverNameIndication = properties.getPropertyAsIntWithDefault(prefix + "CheckCertName", 0) > 1;
 
         //
         // VerifyDepthMax establishes the maximum length of a peer's certificate
@@ -886,25 +886,22 @@ else if(_verifyPeer == 1)
         }
 
         // Server name indication
-        if (!incoming && _serverNameIndication)
+        if(!incoming && _serverNameIndication)
         {
             SNIHostName serverName = null;
             try
             {
                 serverName = new SNIHostName(host);
-            }
-            catch(IllegalArgumentException ex)
-            {
-                // Invalid SNI hostname, ignore because it might be an IP
-            }
-            if (serverName != null)
-            {
                 SSLParameters sslParams = engine.getSSLParameters();
                 List<SNIServerName> serverNames = new ArrayList<>();
                 serverNames.add(serverName);
                 sslParams.setServerNames(serverNames);
                 engine.setSSLParameters(sslParams);
             }
+            catch(IllegalArgumentException ex)
+            {
+                // Invalid SNI hostname, ignore because it might be an IP
+            }
         }
 
         try

diff --git a/js/src/Ice/PropertyNames.js b/js/src/Ice/PropertyNames.js
@@ -1,7 +1,7 @@
 //
 // Copyright (c) ZeroC, Inc. All rights reserved.
 //
-// Generated by makeprops.py from file ../config/PropertyNames.xml, Wed May 29 21:06:31 2019
+// Generated by makeprops.py from file ./config/PropertyNames.xml, Fri Sep  6 18:11:04 2019
 
 // IMPORTANT: Do not edit this file -- any edits made here will be lost!