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)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: glenbeasley, Assigned: glenbeasley)
Details
Attachments
(2 files, 2 obsolete files)
21.40 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
4.50 KB,
patch
|
Sandeep.Konchady
:
superreview+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•19 years ago
|
||
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
Assignee | ||
Comment 2•19 years ago
|
||
(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.
Assignee | ||
Comment 3•19 years ago
|
||
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 4•19 years ago
|
||
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 5•19 years ago
|
||
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-
Comment 6•19 years ago
|
||
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.
Assignee | ||
Comment 7•19 years ago
|
||
Attachment #200573 -
Attachment is obsolete: true
Attachment #201764 -
Flags: superreview?
Assignee | ||
Updated•19 years ago
|
Attachment #201764 -
Flags: superreview? → superreview?(nelson)
Comment 8•19 years ago
|
||
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-
Assignee | ||
Comment 9•19 years ago
|
||
Attachment #201764 -
Attachment is obsolete: true
Attachment #201790 -
Flags: superreview?(nelson)
Comment 10•19 years ago
|
||
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 11•19 years ago
|
||
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-
Assignee | ||
Comment 12•19 years ago
|
||
/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
Assignee | ||
Comment 13•19 years ago
|
||
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 → ---
Assignee | ||
Comment 14•19 years ago
|
||
Attachment #201868 -
Flags: superreview?(Sandeep.Konchady)
Comment 15•19 years ago
|
||
Comment on attachment 201868 [details] [diff] [review] JSS 4.2 version The changes look good.
Attachment #201868 -
Flags: superreview?(Sandeep.Konchady) → superreview+
Assignee | ||
Comment 16•19 years ago
|
||
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 ago → 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•