Closed Bug 313176 Opened 19 years ago Closed 19 years ago

allow JSS to enable more options on SSLSockets such as SSL_BYPASS_PKCS11

Categories

(JSS Graveyard :: Library, defect)

4.1.1
Sun
Solaris
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: glenbeasley, Assigned: glenbeasley)

Details

Attachments

(2 files, 2 obsolete files)

 
Along with SSL_BYPASS_PKCS11 JSS should also allow enabling of 
SSL_ENABLE_FDX
SSL_V2_COMPATIBLE_HELLO
SSL_ROLLBACK_DETECTION
SSL_NO_STEP_DOWN

note, I do think we should allow for enabling the SSL_NO_LOCKS option. 
Status: NEW → ASSIGNED
Summary: allow JSS to enable ByPassPKCS11 for SSL sockets → allow JSS to enable more options on SSLSockets such as SSL_BYPASS_PKCS11
(In reply to comment #1)
> Along with SSL_BYPASS_PKCS11 JSS should also allow enabling of 
> SSL_ENABLE_FDX
> SSL_V2_COMPATIBLE_HELLO
> SSL_ROLLBACK_DETECTION
> SSL_NO_STEP_DOWN
> 
> note, I do not think we should allow for enabling the SSL_NO_LOCKS option. 

This is part 1 of the check in which is all changes except 
for the test programs in org.mozilla.jss.tests. Part 2 will 
be the JSS tests programs that test the bypass and
and I'll request Sandeep only to review the tests. 

For Part 1 of this patch I followed the orginal design with the 
earlier SSL_OPTIONS such as SSL_ENABLE_TLS.

I added enable"option" methods to SocketBase.java to be accessed 
by SSLSocket.java and SSLServerSocket.java
 
Then added enable"option"default methods to SSLSocket.java.

I did add two methods mainly for debug purposes but users 
may find the methods useful for logging or debug so I kept
them. JSS only has enable methods to set the SSLoptions on/off. 
JSS does have any support for get"option" status methods. 
No user has asked for get"options" status methods for the 
existing options, so I did not implement.  

in SSLSocket.java
+    /**
+     *
+     * @return a String listing  the Default SSLOptions for all SSLSockets.
+     */
+    static public String getDefaultSSLOptions()
and in SSLSocket.java and SSLServerSocket
+    /**
+     * @return a String listing the current SSLOptions for this SSLSocket.
+     */
+    public String getSSLOptions() {
Attachment #200573 - Flags: superreview?(wtchang)
Attachment #200573 - Flags: review?(Sandeep.Konchady)
Comment on attachment 200573 [details] [diff] [review]
 added SSL_BYPASS_PKCS11, SSL_ROLLBACK_DETECTION, SSL_NO_STEP_DOWN, SSL_ENABLE_FDX, SSL_V2_COMPATIBLE_HELL

The changes look good.  In SSLSocket.java alignment of set***() in enableByPass..(), enableRollBack..(), and enableV2Compatible..() seems to be a bit off.  Maybe tab or space issue.  Once other question, SSLSocket.c has getSSLOptionDefault(..) and SSLSocket.java has getDefaultSSLOptions(), is there a reason why these are different?
Attachment #200573 - Flags: review?(Sandeep.Konchady) → review+
Comment on attachment 200573 [details] [diff] [review]
 added SSL_BYPASS_PKCS11, SSL_ROLLBACK_DETECTION, SSL_NO_STEP_DOWN, SSL_ENABLE_FDX, SSL_V2_COMPATIBLE_HELL

This patch is basically okay.  Most of the changes I
suggest below are stylistic or even just nits.

1. In lib/jss.def, we have:

> ;+JSS_4.1 {       # JSS 4.1 release
> ;+    global:
> Java_org_mozilla_jss_ssl_SSLSocket_abortReadWrite;
> Java_org_mozilla_jss_ssl_SSLServerSocket_abortAccept;
>+Java_org_mozilla_jss_ssl_SocketBase_getSSLOption;
>+Java_org_mozilla_jss_ssl_SSLSocket_getSSLOptionDefault;
> ;+    local:
> ;+       *;
> ;+};

We should add a new JSS_4.1.1 section and add the two
new symbols there.  (I assume the next JSS release will
be JSS 4.1.1.)

2. In org/mozilla/jss/ssl/SSLServerSocket.java,

>+     * Enables the By Pass of PKCS11 for performance on this socket.

"Bypass" is one word and it doesn't need to be capitialized.

>+    public void enableByPassPKCS11(boolean enable) throws SocketException {
>+        base.enableByPassPKCS11(enable);
>+    }

I suggest that you change all instances of "ByPass" to
"Bypass" in method names or comments (including in other
files) because "bypass" is one word.  Also, since "bypass"
is a verb, this method can be simply named "bypassPKCS11".

>+     * enable rollback compatiblity for this socket.

"rollback compatibility" should be "rollback detection".
Please fix all instances of this string, including in other
files.

>+    public void enableRollBackDetection(boolean enable) throws SocketException {
>+        base.enableRollBackDetection(enable);
>+    }

This one is borderline -- I would spell it as "Rollback".
But that's based on the assumption that "rollback" is one
word, and I'm not sure if that's true.

>+    /**
>+     * enable export ciphers suites if step-down keys
>+     * are needed for this socket.

"enable" should be "disable".

>+     * It is disabled by default, unless the default has been changed 
>+     * with <code>SSLSocket.enableNoStepDownDefault</code>.
>+     */
>+    public void enableNoStepDown(boolean enable) throws SocketException {
>+        base.enableNoStepDown(enable);
>+    }

Now you see that the comment and the method's name seem
to contradict each other.  But in fact they are both correct.
This is very confusing.

Also, the option name "No step down" is unfortunate because
it is very confusing when used with "enable" and "false", as
in:

    enableNoStepDown(false);

I can't figure out whether step down is enabled or not
right away.

Bug 148452 comment 14 has a good definition of the
SSL_NO_STEP_DOWN option, much better than the comments
in ssl.h.

With that information, one possible way to fix this mess is:

    /**
     * This option is concerned with the generation of step-down
     * keys.
     * If the server cert's public key is 512 bits of less,
     * this option is ignored because step-down keys don't
     * need to be generated.
     * If the server cert's public key is more than 512 bits,
     * this option has the following effect:
     * enable=true:  generate step-down keys
     * enable=false: don't generate step-down keys; disable
     *               export cipher suites
     *
     * This option is enabled by default. ...
     */
    public void enableStepDown(boolean enable) throws SocketException {
        base.enableStepDown(enable);
    }

>+     * enable simultaneous read/write for this socket.

Add "(full duplex)" after "read/write" to motivate the
"FDX" abbreviation.

3. In org/mozilla/jss/ssl/SSLSocket.c:

>+
>+JNIEXPORT jboolean JNICALL
>+Java_org_mozilla_jss_ssl_SSLSocket_getSSLOptionDefault(JNIEnv *env, 
>+										jobject self, jint joption)
>+{
>+    PRStatus status;
>+    PRBool bOption;
>+
>+	/*get the Default option */
>+	status = SSL_OptionGetDefault(JSSL_enums[joption], &bOption);
>+    if( status != PR_SUCCESS ) {
>+        JSSL_throwSSLSocketException(env, "SSL_OptionGetDefault failed");
>+    }
>+
>+    return bOption;
>+}

PRStatus   ==> SECStatus
PR_SUCCESS ==> SECSuccess

Note that you use a tabstop of 4 in three lines.  Unless
this file already uses a tabstop of 4, you should follow
NSS's default tabstop of 8.  Alternatively, do not use
tabs unless necessary (such as in makefiles).

4. In org/mozilla/jss/ssl/SSLSocket.java:

>+    static private native boolean getSSLOptionDefault(int option)
>+        throws SocketException;

For consistency with the existing setSSLDefaultOption
method, this method should be named getSSLDefaultOption.
Also it probably should return int instead of boolean,
or have a version that return int (see the two versions
of setSSLDefaultOption).

>+    static public String getDefaultSSLOptions() {

By analogy, this method should be named getSSLDefaultOptions.

>+            buf.append("Default Options configured for all SSLSockets: ");

There is an extra space at the end of the string literal.

>+            buf.append("\nSSL_REQUIRE_CERTIFICATE"  + 
>+                (getSSLOptionDefault(SocketBase.SSL_REQUIRE_CERTIFICATE) 
>+                ? "=on" :  "=off"));

This option is not a boolean option.  It has four values,
so we can't simply print "on" and "off":

/* Values for "on" with SSL_REQUIRE_CERTIFICATE. */
#define SSL_REQUIRE_NEVER           ((PRBool)0)
#define SSL_REQUIRE_ALWAYS          ((PRBool)1)
#define SSL_REQUIRE_FIRST_HANDSHAKE ((PRBool)2)
#define SSL_REQUIRE_NO_ERROR        ((PRBool)3)

I think this is the only SSL option that we abused.
(The type of our SSL options is PRBool.)

5. In org/mozilla/jss/ssl/SocketBase.java:

>     void setSSLOption(int option, boolean on)
>         throws SocketException
>     {
>         setSSLOption(option, on ? 1 : 0);
>     }
> 
>     native void setSSLOption(int option, int on)
>         throws SocketException;
> 
>+    native boolean getSSLOption(int option)
>+        throws SocketException;

It seems that the getSSLOption method should return int
instead of boolean because the native setSSLOption method's
'on' parameter is of the int type.  It may need to be int
to handle SSL_REQUIRE_CERTIFICATE, which has four values.

>+            buf.append("Options configured for this SSLSocket: ");

There is an extra space at the end of the string literal.

>+            buf.append("\nSSL_REQUIRE_CERTIFICATE"  + 
>+                (getSSLOption( SSL_REQUIRE_CERTIFICATE) 
>+                ? "=on" :  "=off"));

Same as above, need to print the four possible values.

6. In org/mozilla/jss/ssl/common.c:

>+JNIEXPORT jboolean JNICALL
>+Java_org_mozilla_jss_ssl_SocketBase_getSSLOption(JNIEnv *env, 
>+												 jobject self, jint option)
>+{
>+    PRSocketOptionData sockOptions;
>+    JSSL_SocketData *sock = NULL;
>+    PRStatus status;
>+    PRBool bOption;
>+
>+    if( JSSL_getSockData(env, self, &sock) != PR_SUCCESS ) {
>+        goto finish;
>+    }
>+
>+	/*get the option */
>+	status = SSL_OptionGet(sock->fd, JSSL_enums[option], &bOption);
>+    if( status != PR_SUCCESS ) {
>+        JSSL_throwSSLSocketException(env, "SSL_OptionGet failed");
>+        goto finish;
>+    }
>+
>+finish:
>+    EXCEPTION_CHECK(env, sock)
>+    return bOption;
>+}

PRStatus ==> SECStatus
PR_SUCCESS ==> SECSuccess
Three lines assume tabstop of 4.
Attachment #200573 - Flags: superreview?(wtchang) → superreview-
You can also avoid the enableNoStepDown mess by not
exposing this option in JSS now.  It seems to be a
useful option though, avoiding generating unused
step-down keys for servers that don't support export
cipher suites.
Attached patch applied Wan-Teh review changes (obsolete) — Splinter Review
Attachment #200573 - Attachment is obsolete: true
Attachment #201764 - Flags: superreview?
Attachment #201764 - Flags: superreview? → superreview?(nelson)
Comment on attachment 201764 [details] [diff] [review]
applied Wan-Teh review changes



1) Minor stylistic inconsistency:  
Some java comments begin with a capital letter, like this one:
>     /**
>+     * Enables the bypass of PKCS11 for performance on this socket. 

Others begin with lower case letter, like this:
>+    /**
>+     * enable rollback detection for this socket.

Judge for yourself if that needs fixing.

2) Suggested additions to comments (do or not at your own discretion)
    
>+    /**
>+     * This option, enableStepDown, is concerned with the generation 
>+     * of step-down keys.

You might add, "which are used with export cipher suites."

>+    /**
>+     * enable simultaneous read/write (full duplex) for this socket.

Perhaps you want to add "by separate read and write threads."
The above block comments appear in several files, so be sure to 
keep the comments consistent in all files in which they appear.

3) indentation problem:  THe comment line below is indented by one 
space too many:

>+    PRBool bOption;
>+
>+     /*get the Default option */
>+    status = SSL_OptionGetDefault(JSSL_enums[joption], &bOption);

4) testing integers as booleans
All the "== 1" tests throughout these files should be "!= 0" instead
because that is how NSS interprets them.  

>+            buf.append("\nSSL_ENABLE_SSL2" + 
>+                ((getSSLDefaultOption(SocketBase.SSL_ENABLE_SSL2) == 1)
>+                ? "=on" :  "=off"));
>+            buf.append("\nSSL_ENABLE_SSL3"  + 
>+                ((getSSLDefaultOption(SocketBase.SSL_ENABLE_SSL3) == 1) 
>+                ? "=on" :  "=off"));

5) Seems that "bypass" and "rollback" got downshiften in those names.
I suspect that was unintentional.  
Please keep these names capitalized consistently.
Those capitalization problems will need to be fixed in several files.

>+            buf.append("\nSSL_BYPASS_PKCS11"  + 
>+                ((getSSLDefaultOption(SocketBase.SSL_bypass_PKCS11) == 1) 
>+                ? "=on" :  "=off"));
>+            buf.append("\nSSL_ROLLBACK_DETECTION"  + 
>+                ((getSSLDefaultOption(SocketBase.SSL_rollback_DETECTION)  == 1)
>+                ? "=on" :  "=off"));

>     static final int SSL_POLICY_FRANCE = 12;
>+    static final int SSL_bypass_PKCS11 = 13;      
>+    static final int SSL_rollback_DETECTION = 14;
Attachment #201764 - Flags: superreview?(nelson) → review-
Attachment #201764 - Attachment is obsolete: true
Attachment #201790 - Flags: superreview?(nelson)
Comment on attachment 201790 [details] [diff] [review]
added Nelson's suggestions.

r=nelson.
I have a number of issues with the design of the table JSSL_enums in common.c and the corresponding set of "static final int"s in SocketBase.java, but fixing those is outside of the scope of this bug/patch.
Attachment #201790 - Flags: superreview?(nelson)
Attachment #201790 - Flags: superreview-
Attachment #201790 - Flags: review+
Comment on attachment 201790 [details] [diff] [review]
added Nelson's suggestions.

fix that sr, didn't mean to mark it sr-
Attachment #201790 - Flags: superreview-
/cvsroot/mozilla/security/jss/lib/jss.def,v  <--  jss.def
new revision: 1.29; previous revision: 1.28
done
Checking in SSLServerSocket.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SSLServerSocket.java,v  <--  SSLServerSocket.java
new revision: 1.21; previous revision: 1.20
done
Checking in SSLSocket.c;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SSLSocket.c,v  <--  SSLSocket.c
new revision: 1.23; previous revision: 1.22
done
Checking in SSLSocket.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SSLSocket.java,v  <--  SSLSocket.java
new revision: 1.23; previous revision: 1.22
done
Checking in SocketBase.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SocketBase.java,v  <--  SocketBase.java
new revision: 1.13; previous revision: 1.12
done
Checking in common.c;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/common.c,v  <--  common.c
new revision: 1.24; previous revision: 1.23
done

Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
This fix requires NSS 3.11 due to the bypassPKCS11, therefore I am reopening the bug and changing the version of JSS to 4.2 instead of 4.1.1. 
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch JSS 4.2 versionSplinter Review
Attachment #201868 - Flags: superreview?(Sandeep.Konchady)
Comment on attachment 201868 [details] [diff] [review]
JSS 4.2 version

The changes look good.
Attachment #201868 - Flags: superreview?(Sandeep.Konchady) → superreview+
updated to JSS 4.2. 

Checking in lib/jss.def;
/cvsroot/mozilla/security/jss/lib/jss.def,v  <--  jss.def
new revision: 1.30; previous revision: 1.29
done
Checking in lib/manifest.mn;
/cvsroot/mozilla/security/jss/lib/manifest.mn,v  <--  manifest.mn
new revision: 1.8; previous revision: 1.7
done
Checking in org/mozilla/jss/CryptoManager.c;
/cvsroot/mozilla/security/jss/org/mozilla/jss/CryptoManager.c,v  <--  CryptoManager.c
new revision: 1.15; previous revision: 1.14
done
Checking in org/mozilla/jss/CryptoManager.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/CryptoManager.java,v  <--  CryptoManager.java
new revision: 1.38; previous revision: 1.37
done
Checking in org/mozilla/jss/JSSProvider.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/JSSProvider.java,v  <--  JSSProvider.java
new revision: 1.17; previous revision: 1.16
done
Checking in org/mozilla/jss/util/jssver.h;
/cvsroot/mozilla/security/jss/org/mozilla/jss/util/jssver.h,v  <--  jssver.h
new revision: 1.25; previous revision: 1.24
done
Status: REOPENED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: