NoDesRC4CiphSuite.java
----------------------
Please move line 30-31 out of the test comment block. The two lines will be parsed as part of the run parameters.

I would prefer to use othervm mode. Otherwise, once there is a test case does not run with othervm and changes the context, this test may be not reliable any more. If this test failed, it is hard to evaluate the root cause if it is impacted by other test case.

I may prefer to use System.err for new test case as the SunJSSE default debug log now is dumped to System.err. Dumps on System.out and System.err are not synchronized. Using the same stream may make the debug log easier to read.

Thanks,
Xuelei


On 8/20/2018 7:33 AM, Jamil Nimeh wrote:
I can fix the copyright, no problem.  Good catch on the othervm - the original form of the test did set properties but it seemed better to not set them explicitly and just use the new defaults. One would not expect to ever remove DES and RC4 from the disabledAlgorithms identifier set, at least in our delivered code. It doesn't need to be run in othervm mode.  And I can comment those other two tests.

Thanks,
--Jamil

On 8/20/2018 7:19 AM, Sean Mullan wrote:
Looks good, just a few minor comments:

CustomizedCipherSuites.java

- should have both years (2016, 2018) on copyright

NoDesRC4CiphSuite.java

- does this need to be run in othervm mode? It doesn't look like you are setting any properties dynamically. Lines 30-31 should also be removed, if so.

- add comments  describing what the testEngAddDisabled method does (similar to the testEngOnlyDisabled method)

--Sean

On 8/19/18 9:06 PM, Jamil Nimeh wrote:
Hello all,

This change adds all DES cipher suites to the jdk.tls.disabledAlgorithms Security property.  This will have the effect of making all DES-based suites unavailable to SunJSSE SSLSocket and SSLEngine instances, even if explicitly enabled using calls like SSLEngine.setEnabledCipherSuites() or SSLSocket.setEnabledCipherSuites().  Users wishing to re-enable these suites for legacy purposes must first alter the jdk.tls.disabledAlgorithms property in the java.security file.

Please note that prior to this change, DES-based suites were available, but not enabled by default on SSLSocket and SSLEngine objects.  This change just makes these suites no longer available without further intervention.

This change also removes RC4_40 from this Security property as it is already superseded by the RC4 identifier.  It also cleans up a cut-and-paste bug in a couple of the RC4_40 export suites (those suites are disabled already).

Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8208350/webrev.01/
JBS: https://bugs.openjdk.java.net/browse/JDK-8208350
CSR: https://bugs.openjdk.java.net/browse/JDK-8209318

Thanks,
--Jamil

Reply via email to