Mailing-List: contact dev-help@tomcat.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Tomcat Developers List" <dev@tomcat.apache.org>
Received-SPF: pass (athena.apache.org: domain of knst.kolinko@gmail.com
 designates 66.249.92.175 as permitted sender)
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=message-id:date:from:to:subject:in-reply-to:mime-version
         :content-type:content-transfer-encoding:content-disposition
         :references;
        b=QDeaE1d87DTJGrp5Og37WpbmeHH90k5RMXlohZ2C/fzYnnr0DI7u1p09p5aQU7wfuk
         AYj7M2cp+KQb7nlhloPPx5U8MIrLB2HqvveEWCNUnvm1fJFq5qhot5zDBdns7Snxn9J7
         mMG8NlGY9ugAaWP7G2wJnG+gCgH+u14aSvhqI=
Message-ID: <427155180808201822s3385654kc30b63e631cd723e@mail.gmail.com>
Date: Thu, 21 Aug 2008 05:22:49 +0400
From: "Konstantin Kolinko" <knst.kolinko@gmail.com>
To: "Tomcat Developers List" <dev@tomcat.apache.org>
Subject: Re: svn commit: r687503 - in
 /tomcat/trunk/java/org/apache/tomcat/util/net/jsse: JSSESocketFactory.java
 res/LocalStrings.properties
In-Reply-To: <20080820232042.AB85223889BA@eris.apache.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
References: <20080820232042.AB85223889BA@eris.apache.org>

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  <markt@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