208193 - Add SSLSocket.enableTLS and SSLSocket.enableTLSDefault

Status: RESOLVED FIXED

(JSS Graveyard :: Library, enhancement)

Description

[Wan-Teh Chang]

In JSS 3.4, the SSLSocket class has the enableSSL2,
enableSSL2Default, enableSSL3, and enableSSL3Default
methods.  We should add the enableTLS and
enableTLSDefault methods for enabling or disabling
TLS.

Comment 1

[glen beasley]

reassigning bug to myself

Comment 2

[Buddy Brewer]

Attachment: patch adding SSLSocket.enableTLS and SSLSocket.enableTLSDefault

Comment 3

[Buddy Brewer]

bleah, forgot to add comment regarding the patch I submitted -- I've verified
that it functionally works against -r JSS_3_4_RTM, and that it applies without
conflict against HEAD, but I haven't functionally tested against HEAD.

Comment 4

[Wan-Teh Chang]

Comment on attachment 144856 [details] [diff] [review]
patch adding SSLSocket.enableTLS and SSLSocket.enableTLSDefault

Just curious: who is Buddy Brewer?

Please ask Glen Beasley to review this patch,
too.  Below are my review comments.

1. Should we also modify
org/mozilla/jss/ssl/SSLServerSocket.java to
add an enableTLS method to the SSLServerSocket
class?

2. org/mozilla/jss/ssl/SSLSocket.java

The existing comment in front of enableSSL3Default
incorrectly says "SSL v2".  Please fix that.

3. org/mozilla/jss/ssl/SocketBase.java

I just wanted you to make sure that the values
of these static class members (the enums) do not
need to stay the same across JSS releases.  I don't
know Java and JSS well enough to know whether this
is necessary for "binary compatibility".  My guess
is that the values of these static class members
do NOT need to stay the same across JSS releases.

The existing comment
"These must match the enums table in SSLSocket.c."
is out of date.  That enums table is in
org/mozilla/jss/ssl/common.c now.  I suggest
changing "SSLSocket.c" to "common.c" in that comment.

4. org/mozilla/jss/ssl/common.c

The existing comment
"These must match up with the constants defined in SSLSocket.java."
is also out of date (see above).  Please change
"SSLSocket.java" to "SocketBase.java".

Comment 5

[Saul Edwards]

If Buddy is still in the loop on this bug:  Buddy, how did you test your
changes?  We've been using the clients in the JSS tests directory, along with
ssltap.

In reply to Wan-Teh:

I will attach a latest after a bit more testing.

1.) Already done.
2.) Done.
3.) They aren't public, so don't worry about it.
  3.5.) Done.
4.) Done.

Comment 6

[Saul Edwards]

Attachment: Add enableTLS to the ServerSocket, clean up comments

Tested many configurations on both client and server side.  All SSL2, SSL3, and
TLS options work on both sides.

Comment 7

[glen beasley]

Comment on attachment 160676 [details] [diff] [review]
Add enableTLS to the ServerSocket, clean up comments

Patch looks good.

Comment 8

[Wan-Teh Chang]

Comment on attachment 160676 [details] [diff] [review]
Add enableTLS to the ServerSocket, clean up comments

r=wtc.

Comment 9

[Saul Edwards]

Checked in to the tip:

Checking in SSLServerSocket.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SSLServerSocket.java,v  <-- 
SSLServerSocket.java
new revision: 1.17; previous revision: 1.16
done
Checking in SSLSocket.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SSLSocket.java,v  <-- 
SSLSocket.java
new revision: 1.19; previous revision: 1.18
done
Checking in SocketBase.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SocketBase.java,v  <-- 
SocketBase.java
new revision: 1.11; previous revision: 1.10
done
Checking in common.c;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/common.c,v  <--  common.c
new revision: 1.21; previous revision: 1.20
done

Comment 10

[Julien Pierre]

Doesn't the following change to the enum value break binary compatibility ?
It seems to me the SSL_ENABLE_TLS should have been added to the end, ie. with a
value of 23 .

     /**
-     * Enums. These must match the enums table in SSLSocket.c. This is
+     * Enums. These must match the enums table in common.c. This is
      * safer than copying the values of the C constants, which are subject
      * to change, into Java code.
      */
     static final int SSL_ENABLE_SSL2 = 0;
     static final int SSL_ENABLE_SSL3 = 1;
-    static final int TCP_NODELAY = 2;
-    static final int SO_KEEPALIVE = 3;
-    static final int PR_SHUTDOWN_RCV = 4;
-    static final int PR_SHUTDOWN_SEND = 5;
-    static final int SSL_REQUIRE_CERTIFICATE = 6;
-    static final int SSL_REQUEST_CERTIFICATE = 7;
-    static final int SSL_NO_CACHE = 8;
-    static final int SSL_POLICY_DOMESTIC = 9;
-    static final int SSL_POLICY_EXPORT = 10;
-    static final int SSL_POLICY_FRANCE = 11;
+    static final int SSL_ENABLE_TLS = 2;
+    static final int TCP_NODELAY = 3;
+    static final int SO_KEEPALIVE = 4;
+    static final int PR_SHUTDOWN_RCV = 5;
+    static final int PR_SHUTDOWN_SEND = 6;
+    static final int SSL_REQUIRE_CERTIFICATE = 7;
+    static final int SSL_REQUEST_CERTIFICATE = 8;
+    static final int SSL_NO_CACHE = 9;
+    static final int SSL_POLICY_DOMESTIC = 10;
+    static final int SSL_POLICY_EXPORT = 11;
+    static final int SSL_POLICY_FRANCE = 12;

Comment 11

[Julien Pierre]

I meant a value of 12 of course, my fingers slipped.

Comment 12

[Wan-Teh Chang]

Julien, see point 3 in comment 4 and comment 5.

Comment 13

[Julien Pierre]

Sorry for not reading the whole bug. Setting status back to fixed.