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.
reassigning bug to myself
Created attachment 144856 [details] [diff] [review] patch adding SSLSocket.enableTLS and SSLSocket.enableTLSDefault
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 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".
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.
Created attachment 160676 [details] [diff] [review] 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 on attachment 160676 [details] [diff] [review] Add enableTLS to the ServerSocket, clean up comments Patch looks good.
Comment on attachment 160676 [details] [diff] [review] Add enableTLS to the ServerSocket, clean up comments r=wtc.
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
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;
I meant a value of 12 of course, my fingers slipped.
Sorry for not reading the whole bug. Setting status back to fixed.