Looks fine to me.
Thanks,
Xuelei
On 8/20/2018 10:42 AM, Jamil Nimeh wrote:
Hello all, updated webrev:
* Copyright and comment fixes
* Leaving NoDesRC4CiphSuite.java in othervm mode per Xuelei's concerns
* Changed output to use System.err so it outputs on the same stream as
SSLLogger.
http://cr.openjdk.java.net/~jnimeh/reviews/8208350/webrev.02
Thanks,
--Jamil
On 08/20/2018 09:01 AM, Xue-Lei Fan wrote:
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
