You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2008/08/21 01:20:42 UTC

svn commit: r687503 - in /tomcat/trunk/java/org/apache/tomcat/util/net/jsse: JSSESocketFactory.java res/LocalStrings.properties

Author: markt
Date: Wed Aug 20 16:20:42 2008
New Revision: 687503

URL: http://svn.apache.org/viewvc?rev=687503&view=rev
Log:
Improved fix for 45528 (invalid SSL config).
It is a variation on the previous patch that:
- does the check earlier
- uses an unbound socket so there is no possibility of a client connection
- uses the String manager for the error message
Note: I gave up on the alterntaive javax.crypto.Cipher suggestion as the cipher names are different and there is no easy conversion.

Modified:
    tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java
    tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties

Modified: tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java?rev=687503&r1=687502&r2=687503&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java Wed Aug 20 16:20:42 2008
@@ -26,6 +26,7 @@
 import java.net.ServerSocket;
 import java.net.Socket;
 import java.net.SocketException;
+import java.net.SocketTimeoutException;
 import java.security.KeyStore;
 import java.security.SecureRandom;
 import java.security.cert.CRL;
@@ -428,6 +429,9 @@
                 getEnabledCiphers(requestedCiphers,
                         sslProxy.getSupportedCipherSuites());
 
+            // Check the SSL config is OK
+            checkConfig();
+
         } catch(Exception e) {
             if( e instanceof IOException )
                 throw (IOException)e;
@@ -692,7 +696,7 @@
      * Configures the given SSL server socket with the requested cipher suites,
      * protocol versions, and need for client authentication
      */
-    private void initServerSocket(ServerSocket ssocket) {
+    private void initServerSocket(ServerSocket ssocket) throws IOException {
 
         SSLServerSocket socket = (SSLServerSocket) ssocket;
 
@@ -709,4 +713,33 @@
         configureClientAuth(socket);
     }
 
+    /**
+     * Checks that the cetificate is compatible with the enabled cipher suites.
+     * If we don't check now, the JIoEndpoint can enter a nasty logging loop.
+     * See bug 45528.
+     */
+    private void checkConfig() throws IOException {
+        // Create an unbound server socket
+        ServerSocket socket = sslProxy.createServerSocket();
+        initServerSocket(socket);
+
+        // Set the timeout to 1ms as all we care about is if it throws an
+        // exception on accept. 
+        socket.setSoTimeout(1);
+        try {
+            socket.accept();
+            // Will never get here - no client can connect to an unbound port
+        } catch (SSLException ssle) {
+            // SSL configuration is invalid. Possibly cert doesn't match ciphers
+            IOException ioe = new IOException(sm.getString(
+                    "jsse.invalid_ssl_conf", ssle.getMessage()));
+            ioe.initCause(ssle);
+            throw ioe;
+        } catch (SocketTimeoutException ste) {
+            // Expected if all is well - do nothing
+        } finally {
+            socket.close();
+        }
+        
+    }
 }

Modified: tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties?rev=687503&r1=687502&r2=687503&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties Wed Aug 20 16:20:42 2008
@@ -15,3 +15,4 @@
 
 jsse.alias_no_key_entry=Alias name {0} does not identify a key entry
 jsse.keystore_load_failed=Failed to load keystore type {0} with path {1} due to {2}
+jsse.invalid_ssl_conf=SSL configuration is invalid due to {0} 
\ No newline at end of file



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Re: svn commit: r687503 - in /tomcat/trunk/java/org/apache/tomcat/util/net/jsse: JSSESocketFactory.java res/LocalStrings.properties

Posted by Mark Thomas <ma...@apache.org>.
Filip Hanik - Dev Lists wrote:
> the checkConfig catches a SocketTimeoutException, but the javadoc says
> 
> public synchronized void setSoTimeout(int timeout) throws
> SocketException

Thanks for the catch. I'll update the patch to handle that.

> this still seems like a hack :)

Yep, but hopefully one getting to the point that is acceptable.

Mark



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Re: svn commit: r687503 - in /tomcat/trunk/java/org/apache/tomcat/util/net/jsse: JSSESocketFactory.java res/LocalStrings.properties

Posted by Filip Hanik - Dev Lists <de...@hanik.com>.
the checkConfig catches a SocketTimeoutException, but the javadoc says

public synchronized void setSoTimeout(int timeout) throws SocketException <http://addison-wesley.de/Service/Krueger/javadocs/java.net.SocketException.html#_top_>

Enable/disable SO_TIMEOUT with the specified timeout, in milliseconds. 
With this option set to a non-zero timeout, a call to accept() for this 
ServerSocket will block for only this amount of time. If the timeout 
expires, a *java.io.InterruptedIOExceptio**n* is raised, though the 
ServerSocket is still valid.

this still seems like a hack :)
Filip




markt@apache.org wrote:
> Author: markt
> Date: Wed Aug 20 16:20:42 2008
> New Revision: 687503
>
> URL: http://svn.apache.org/viewvc?rev=687503&view=rev
> Log:
> Improved fix for 45528 (invalid SSL config).
> It is a variation on the previous patch that:
> - does the check earlier
> - uses an unbound socket so there is no possibility of a client connection
> - uses the String manager for the error message
> Note: I gave up on the alterntaive javax.crypto.Cipher suggestion as the cipher names are different and there is no easy conversion.
>
> Modified:
>     tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java
>     tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties
>
> Modified: tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java?rev=687503&r1=687502&r2=687503&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java (original)
> +++ tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java Wed Aug 20 16:20:42 2008
> @@ -26,6 +26,7 @@
>  import java.net.ServerSocket;
>  import java.net.Socket;
>  import java.net.SocketException;
> +import java.net.SocketTimeoutException;
>  import java.security.KeyStore;
>  import java.security.SecureRandom;
>  import java.security.cert.CRL;
> @@ -428,6 +429,9 @@
>                  getEnabledCiphers(requestedCiphers,
>                          sslProxy.getSupportedCipherSuites());
>  
> +            // Check the SSL config is OK
> +            checkConfig();
> +
>          } catch(Exception e) {
>              if( e instanceof IOException )
>                  throw (IOException)e;
> @@ -692,7 +696,7 @@
>       * Configures the given SSL server socket with the requested cipher suites,
>       * protocol versions, and need for client authentication
>       */
> -    private void initServerSocket(ServerSocket ssocket) {
> +    private void initServerSocket(ServerSocket ssocket) throws IOException {
>  
>          SSLServerSocket socket = (SSLServerSocket) ssocket;
>  
> @@ -709,4 +713,33 @@
>          configureClientAuth(socket);
>      }
>  
> +    /**
> +     * Checks that the cetificate is compatible with the enabled cipher suites.
> +     * If we don't check now, the JIoEndpoint can enter a nasty logging loop.
> +     * See bug 45528.
> +     */
> +    private void checkConfig() throws IOException {
> +        // Create an unbound server socket
> +        ServerSocket socket = sslProxy.createServerSocket();
> +        initServerSocket(socket);
> +
> +        // Set the timeout to 1ms as all we care about is if it throws an
> +        // exception on accept. 
> +        socket.setSoTimeout(1);
> +        try {
> +            socket.accept();
> +            // Will never get here - no client can connect to an unbound port
> +        } catch (SSLException ssle) {
> +            // SSL configuration is invalid. Possibly cert doesn't match ciphers
> +            IOException ioe = new IOException(sm.getString(
> +                    "jsse.invalid_ssl_conf", ssle.getMessage()));
> +            ioe.initCause(ssle);
> +            throw ioe;
> +        } catch (SocketTimeoutException ste) {
> +            // Expected if all is well - do nothing
> +        } finally {
> +            socket.close();
> +        }
> +        
> +    }
>  }
>
> Modified: tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties?rev=687503&r1=687502&r2=687503&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties (original)
> +++ tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties Wed Aug 20 16:20:42 2008
> @@ -15,3 +15,4 @@
>  
>  jsse.alias_no_key_entry=Alias name {0} does not identify a key entry
>  jsse.keystore_load_failed=Failed to load keystore type {0} with path {1} due to {2}
> +jsse.invalid_ssl_conf=SSL configuration is invalid due to {0} 
> \ No newline at end of file
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
>   


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Re: svn commit: r687503 - in /tomcat/trunk/java/org/apache/tomcat/util/net/jsse: JSSESocketFactory.java res/LocalStrings.properties

Posted by Filip Hanik - Dev Lists <de...@hanik.com>.
figured it out, you close the socket again

Filip

Filip Hanik - Dev Lists wrote:
> +        socket.setSoTimeout(1);
>
> does this ever get reset?
>
> In JioEndpoint.java I see
>        //if( serverTimeout >= 0 )
>        //    serverSocket.setSoTimeout( serverTimeout );
> It's commented out
>
> and I have a hard time finding where it would be set to a more normal 
> value, instead of 1 millisecond for the server socket
>
> Filip
>
>
>
> markt@apache.org wrote:
>> Author: markt
>> Date: Wed Aug 20 16:20:42 2008
>> New Revision: 687503
>>
>> URL: http://svn.apache.org/viewvc?rev=687503&view=rev
>> Log:
>> Improved fix for 45528 (invalid SSL config).
>> It is a variation on the previous patch that:
>> - does the check earlier
>> - uses an unbound socket so there is no possibility of a client 
>> connection
>> - uses the String manager for the error message
>> Note: I gave up on the alterntaive javax.crypto.Cipher suggestion as 
>> the cipher names are different and there is no easy conversion.
>>
>> Modified:
>>     
>> tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java
>>     
>> tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties 
>>
>>
>> Modified: 
>> tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java
>> URL: 
>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java?rev=687503&r1=687502&r2=687503&view=diff 
>>
>> ============================================================================== 
>>
>> --- 
>> tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java 
>> (original)
>> +++ 
>> tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java 
>> Wed Aug 20 16:20:42 2008
>> @@ -26,6 +26,7 @@
>>  import java.net.ServerSocket;
>>  import java.net.Socket;
>>  import java.net.SocketException;
>> +import java.net.SocketTimeoutException;
>>  import java.security.KeyStore;
>>  import java.security.SecureRandom;
>>  import java.security.cert.CRL;
>> @@ -428,6 +429,9 @@
>>                  getEnabledCiphers(requestedCiphers,
>>                          sslProxy.getSupportedCipherSuites());
>>  
>> +            // Check the SSL config is OK
>> +            checkConfig();
>> +
>>          } catch(Exception e) {
>>              if( e instanceof IOException )
>>                  throw (IOException)e;
>> @@ -692,7 +696,7 @@
>>       * Configures the given SSL server socket with the requested 
>> cipher suites,
>>       * protocol versions, and need for client authentication
>>       */
>> -    private void initServerSocket(ServerSocket ssocket) {
>> +    private void initServerSocket(ServerSocket ssocket) throws 
>> IOException {
>>  
>>          SSLServerSocket socket = (SSLServerSocket) ssocket;
>>  
>> @@ -709,4 +713,33 @@
>>          configureClientAuth(socket);
>>      }
>>  
>> +    /**
>> +     * Checks that the cetificate is compatible with the enabled 
>> cipher suites.
>> +     * If we don't check now, the JIoEndpoint can enter a nasty 
>> logging loop.
>> +     * See bug 45528.
>> +     */
>> +    private void checkConfig() throws IOException {
>> +        // Create an unbound server socket
>> +        ServerSocket socket = sslProxy.createServerSocket();
>> +        initServerSocket(socket);
>> +
>> +        // Set the timeout to 1ms as all we care about is if it 
>> throws an
>> +        // exception on accept. +        socket.setSoTimeout(1);
>> +        try {
>> +            socket.accept();
>> +            // Will never get here - no client can connect to an 
>> unbound port
>> +        } catch (SSLException ssle) {
>> +            // SSL configuration is invalid. Possibly cert doesn't 
>> match ciphers
>> +            IOException ioe = new IOException(sm.getString(
>> +                    "jsse.invalid_ssl_conf", ssle.getMessage()));
>> +            ioe.initCause(ssle);
>> +            throw ioe;
>> +        } catch (SocketTimeoutException ste) {
>> +            // Expected if all is well - do nothing
>> +        } finally {
>> +            socket.close();
>> +        }
>> +        +    }
>>  }
>>
>> Modified: 
>> tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties 
>>
>> URL: 
>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties?rev=687503&r1=687502&r2=687503&view=diff 
>>
>> ============================================================================== 
>>
>> --- 
>> tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties 
>> (original)
>> +++ 
>> tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties 
>> Wed Aug 20 16:20:42 2008
>> @@ -15,3 +15,4 @@
>>  
>>  jsse.alias_no_key_entry=Alias name {0} does not identify a key entry
>>  jsse.keystore_load_failed=Failed to load keystore type {0} with path 
>> {1} due to {2}
>> +jsse.invalid_ssl_conf=SSL configuration is invalid due to {0} \ No 
>> newline at end of file
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>>
>>   
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Re: svn commit: r687503 - in /tomcat/trunk/java/org/apache/tomcat/util/net/jsse: JSSESocketFactory.java res/LocalStrings.properties

Posted by Filip Hanik - Dev Lists <de...@hanik.com>.
+        socket.setSoTimeout(1);

does this ever get reset?

In JioEndpoint.java I see
        //if( serverTimeout >= 0 )
        //    serverSocket.setSoTimeout( serverTimeout );
It's commented out

and I have a hard time finding where it would be set to a more normal value, instead of 1 millisecond for the server socket

Filip



markt@apache.org wrote:
> Author: markt
> Date: Wed Aug 20 16:20:42 2008
> New Revision: 687503
>
> URL: http://svn.apache.org/viewvc?rev=687503&view=rev
> Log:
> Improved fix for 45528 (invalid SSL config).
> It is a variation on the previous patch that:
> - does the check earlier
> - uses an unbound socket so there is no possibility of a client connection
> - uses the String manager for the error message
> Note: I gave up on the alterntaive javax.crypto.Cipher suggestion as the cipher names are different and there is no easy conversion.
>
> Modified:
>     tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java
>     tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties
>
> Modified: tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java?rev=687503&r1=687502&r2=687503&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java (original)
> +++ tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java Wed Aug 20 16:20:42 2008
> @@ -26,6 +26,7 @@
>  import java.net.ServerSocket;
>  import java.net.Socket;
>  import java.net.SocketException;
> +import java.net.SocketTimeoutException;
>  import java.security.KeyStore;
>  import java.security.SecureRandom;
>  import java.security.cert.CRL;
> @@ -428,6 +429,9 @@
>                  getEnabledCiphers(requestedCiphers,
>                          sslProxy.getSupportedCipherSuites());
>  
> +            // Check the SSL config is OK
> +            checkConfig();
> +
>          } catch(Exception e) {
>              if( e instanceof IOException )
>                  throw (IOException)e;
> @@ -692,7 +696,7 @@
>       * Configures the given SSL server socket with the requested cipher suites,
>       * protocol versions, and need for client authentication
>       */
> -    private void initServerSocket(ServerSocket ssocket) {
> +    private void initServerSocket(ServerSocket ssocket) throws IOException {
>  
>          SSLServerSocket socket = (SSLServerSocket) ssocket;
>  
> @@ -709,4 +713,33 @@
>          configureClientAuth(socket);
>      }
>  
> +    /**
> +     * Checks that the cetificate is compatible with the enabled cipher suites.
> +     * If we don't check now, the JIoEndpoint can enter a nasty logging loop.
> +     * See bug 45528.
> +     */
> +    private void checkConfig() throws IOException {
> +        // Create an unbound server socket
> +        ServerSocket socket = sslProxy.createServerSocket();
> +        initServerSocket(socket);
> +
> +        // Set the timeout to 1ms as all we care about is if it throws an
> +        // exception on accept. 
> +        socket.setSoTimeout(1);
> +        try {
> +            socket.accept();
> +            // Will never get here - no client can connect to an unbound port
> +        } catch (SSLException ssle) {
> +            // SSL configuration is invalid. Possibly cert doesn't match ciphers
> +            IOException ioe = new IOException(sm.getString(
> +                    "jsse.invalid_ssl_conf", ssle.getMessage()));
> +            ioe.initCause(ssle);
> +            throw ioe;
> +        } catch (SocketTimeoutException ste) {
> +            // Expected if all is well - do nothing
> +        } finally {
> +            socket.close();
> +        }
> +        
> +    }
>  }
>
> Modified: tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties?rev=687503&r1=687502&r2=687503&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties (original)
> +++ tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties Wed Aug 20 16:20:42 2008
> @@ -15,3 +15,4 @@
>  
>  jsse.alias_no_key_entry=Alias name {0} does not identify a key entry
>  jsse.keystore_load_failed=Failed to load keystore type {0} with path {1} due to {2}
> +jsse.invalid_ssl_conf=SSL configuration is invalid due to {0} 
> \ No newline at end of file
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
>   


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Re: svn commit: r687503 - in /tomcat/trunk/java/org/apache/tomcat/util/net/jsse: JSSESocketFactory.java res/LocalStrings.properties

Posted by Mark Thomas <ma...@apache.org>.
Konstantin Kolinko wrote:
> Hi,
> 
> Several comments:
> 1. There are two glitches, that got carried over from the previous
> version of the patch:
> 
> a)
>> -    private void initServerSocket(ServerSocket ssocket) {
>> +    private void initServerSocket(ServerSocket ssocket) throws IOException {
> 
> There is no need to declare throwing an IOException here.
Thanks for the catch. I missed that whilst I was moving stuff around.

> b)
>> +     * Checks that the cetificate is compatible with the enabled cipher suites.
> 
> s/cetificate/certificate/
Fixed.

> 2. I do not understand how serverSocket.accept() can succeed for an
> unbound socket. It bugs me.
> 
> from ServerSocket#accept() of jdk 1.5.0_12:
> 
> 	if (!isBound())
> 	    throw new SocketException("Socket is not bound yet");
> 
> It seems that the specific implementation, SSLServerSocketImpl,
> bypasses the check (overwrites the accept() method not calling
> super and not reimplementing the check), but it looks more like
> a bug in this specific JDK implementation than a design decision.

It bypasses the isClosed() test too. If these tests were added, the result
would be an IOException.

As long as the cipher check happens first it will be fine. Thinking about
it logically, the cipher check has to be first as the socket can't start
accepting connections if the SSL config is invalid. Therefore, I am happy
relying on the cipher check happening before any other check.

I beginning to think that the code should catch SSLException forthe case
where the config is bad and Exception for eveything else.

That has the advantage that *if* for some reason checks were done before
the cipher check we would revert to the previous behaviour.

> Also, in this operation the server port is checked through the
> security manager for an "accept" permission. Some configurations
> might need adjusting.

And there is another reason to catch Exception.

Mark



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Re: svn commit: r687503 - in /tomcat/trunk/java/org/apache/tomcat/util/net/jsse: JSSESocketFactory.java res/LocalStrings.properties

Posted by Konstantin Kolinko <kn...@gmail.com>.
Hi,

Several comments:
1. There are two glitches, that got carried over from the previous
version of the patch:

a)
> -    private void initServerSocket(ServerSocket ssocket) {
> +    private void initServerSocket(ServerSocket ssocket) throws IOException {

There is no need to declare throwing an IOException here.

b)
> +     * Checks that the cetificate is compatible with the enabled cipher suites.

s/cetificate/certificate/

2. I do not understand how serverSocket.accept() can succeed for an
unbound socket. It bugs me.

from ServerSocket#accept() of jdk 1.5.0_12:

	if (!isBound())
	    throw new SocketException("Socket is not bound yet");

It seems that the specific implementation, SSLServerSocketImpl,
bypasses the check (overwrites the accept() method not calling
super and not reimplementing the check), but it looks more like
a bug in this specific JDK implementation than a design decision.

Also, in this operation the server port is checked through the
security manager for an "accept" permission. Some configurations
might need adjusting.


Best regards,
Konstantin Kolinko

2008/8/21  <ma...@apache.org>:
> Author: markt
> Date: Wed Aug 20 16:20:42 2008
> New Revision: 687503
>
> URL: http://svn.apache.org/viewvc?rev=687503&view=rev
> Log:
> Improved fix for 45528 (invalid SSL config).
> It is a variation on the previous patch that:
> - does the check earlier
> - uses an unbound socket so there is no possibility of a client connection
> - uses the String manager for the error message
> Note: I gave up on the alterntaive javax.crypto.Cipher suggestion as the cipher names are different and there is no easy conversion.
>
> Modified:
>    tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java
>    tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties
>
> Modified: tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java?rev=687503&r1=687502&r2=687503&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java (original)
> +++ tomcat/trunk/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java Wed Aug 20 16:20:42 2008
> @@ -26,6 +26,7 @@
>  import java.net.ServerSocket;
>  import java.net.Socket;
>  import java.net.SocketException;
> +import java.net.SocketTimeoutException;
>  import java.security.KeyStore;
>  import java.security.SecureRandom;
>  import java.security.cert.CRL;
> @@ -428,6 +429,9 @@
>                 getEnabledCiphers(requestedCiphers,
>                         sslProxy.getSupportedCipherSuites());
>
> +            // Check the SSL config is OK
> +            checkConfig();
> +
>         } catch(Exception e) {
>             if( e instanceof IOException )
>                 throw (IOException)e;
> @@ -692,7 +696,7 @@
>      * Configures the given SSL server socket with the requested cipher suites,
>      * protocol versions, and need for client authentication
>      */
> -    private void initServerSocket(ServerSocket ssocket) {
> +    private void initServerSocket(ServerSocket ssocket) throws IOException {
>
>         SSLServerSocket socket = (SSLServerSocket) ssocket;
>
> @@ -709,4 +713,33 @@
>         configureClientAuth(socket);
>     }
>
> +    /**
> +     * Checks that the cetificate is compatible with the enabled cipher suites.
> +     * If we don't check now, the JIoEndpoint can enter a nasty logging loop.
> +     * See bug 45528.
> +     */
> +    private void checkConfig() throws IOException {
> +        // Create an unbound server socket
> +        ServerSocket socket = sslProxy.createServerSocket();
> +        initServerSocket(socket);
> +
> +        // Set the timeout to 1ms as all we care about is if it throws an
> +        // exception on accept.
> +        socket.setSoTimeout(1);
> +        try {
> +            socket.accept();
> +            // Will never get here - no client can connect to an unbound port
> +        } catch (SSLException ssle) {
> +            // SSL configuration is invalid. Possibly cert doesn't match ciphers
> +            IOException ioe = new IOException(sm.getString(
> +                    "jsse.invalid_ssl_conf", ssle.getMessage()));
> +            ioe.initCause(ssle);
> +            throw ioe;
> +        } catch (SocketTimeoutException ste) {
> +            // Expected if all is well - do nothing
> +        } finally {
> +            socket.close();
> +        }
> +
> +    }
>  }
>
> Modified: tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties?rev=687503&r1=687502&r2=687503&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties (original)
> +++ tomcat/trunk/java/org/apache/tomcat/util/net/jsse/res/LocalStrings.properties Wed Aug 20 16:20:42 2008
> @@ -15,3 +15,4 @@
>
>  jsse.alias_no_key_entry=Alias name {0} does not identify a key entry
>  jsse.keystore_load_failed=Failed to load keystore type {0} with path {1} due to {2}
> +jsse.invalid_ssl_conf=SSL configuration is invalid due to {0}
> \ No newline at end of file
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org