Add SSLSocket.enableTLS and SSLSocket.enableTLSDefault

RESOLVED FIXED

Status

JSS
Library
--
enhancement
RESOLVED FIXED
15 years ago
11 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Saul Edwards)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

15 years ago
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

15 years ago
reassigning bug to myself
Assignee: nicolson → glen.beasley

Updated

15 years ago
Status: NEW → ASSIGNED

Comment 2

14 years ago
Created attachment 144856 [details] [diff] [review]
patch adding SSLSocket.enableTLS and SSLSocket.enableTLSDefault

Comment 3

14 years ago
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.
(Assignee)

Updated

13 years ago
Attachment #144856 - Flags: superreview?(wchang0222)
(Reporter)

Comment 4

13 years ago
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".
(Assignee)

Updated

13 years ago
Attachment #144856 - Flags: superreview?(wchang0222)
(Assignee)

Comment 5

13 years ago
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.
(Assignee)

Comment 6

13 years ago
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.
Attachment #144856 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #160676 - Flags: superreview?(wchang0222)
Attachment #160676 - Flags: review?(glen.beasley)

Comment 7

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

Patch looks good.
Attachment #160676 - Flags: review?(glen.beasley) → review+

Updated

13 years ago
Assignee: glen.beasley → saul.edwards.bugs
Status: ASSIGNED → NEW
Version: 3.4 → 4.0
(Reporter)

Comment 8

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

r=wtc.
Attachment #160676 - Flags: superreview?(wchang0222) → superreview+
(Assignee)

Comment 9

13 years ago
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
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 10

13 years ago
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

13 years ago
I meant a value of 12 of course, my fingers slipped.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 12

13 years ago
Julien, see point 3 in comment 4 and comment 5.

Comment 13

13 years ago
Sorry for not reading the whole bug. Setting status back to fixed.
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED

Updated

11 years ago
Duplicate of this bug: 325615
You need to log in before you can comment on or make changes to this bug.