The updated webrev looks good.
--Sean
On 12/19/2015 11:06 AM, Xuelei Fan wrote:
new webrev: http://cr.openjdk.java.net/~xuelei/8133070/webrev.01/
Updates to webrev.00:
Re-org the get methods of SSLContextImpl so as to avoid repeated
initialization of instance fields.
On 12/18/2015 11:35 PM, Sean Mullan wrote:
Here are a few other other comments on the code:
SSLContextImpl:
- I noticed that SSLContext.init does not specify how it handles empty
arrays, and you have changed the code so that an empty TrustManager
array is treated like they are null - is this change in behavior a
compatibility issue at all? Could you file a follow-on issue to clarify
the behavior of SSLContext.init as to how it should handle empty arrays?
It's a bug of my update. Empty means no trust manager, which is
different from using the default trust manager.
I removed this update.
- are there any race conditions that you need to worry about now that
you have removed the synchronized blocks from
getSupportedCipherSuiteList and getDefaultCipherSuite?
May be initialized repeatedly. Should be OK as there is only one
instance for the to-be-assigned value.
The code can be more straightforward and effective. I moved the impls
into sub-classes.
- on lines 781 and 789, an empty array is created and discarded if
there is not an exception thrown. It would be better to only create the
empty array when an exception is thrown (move it into the catch block).
Updated.
Thanks,
Xuelei
--Sean
On 12/14/2015 10:08 PM, Xuelei Fan wrote:
On 12/15/2015 4:24 AM, Sean Mullan wrote:
Hi Xuelei,
For JDK 9, the EC impl is defined to be in its own module
(jdk.crypto.ec). How does it affect this fix if that module is not
available/installed?
The SunJSSE provider would not support dynamically loading of crypto
providers/modules. At present, there are two providers that supports EC
implementation, SunEC (jdk.crypto.ec) and SunPKCS11 (jdk.crypto.pkcs11).
If no EC provider, EC cipher suites would not be available to use.
That's to say, EC cipher suites would not be enabled by default, and
cannot be used for handshaking. In the fix,
JsseJce#EcAvailability.isAvailable is used to check the availability of
EC crypto impl.
Xuelei
--Sean
On 12/01/2015 07:49 AM, Xuelei Fan wrote:
Hi,
Please review the fix for JDK-8133070:
http://cr.openjdk.java.net/~xuelei/8133070/webrev.00/
In (Open)JDK 6, EC cipher suites get supported by Java. However, there
is no default EC provider in JDK 6 at that time. In order to support
third part's EC algorithm JCE provider dynamically, it is hard-coded to
check the cipher suite availability dynamically for EC algorithms in
SunJSSE provider.
Here is an example update in CipherSuite.java for the checking:
- static final boolean DYNAMIC_AVAILABILITY = false;
+ static final boolean DYNAMIC_AVAILABILITY = true;
The dynamically checking impacts the performance significantly as:
1. the check of the availability is expensive as it involves crypto
operations.
2. a cache is used to maintain the availability of bulk ciphers in
order
to mitigate the #1 performance impact. However, access and update to
the cache need to be synchronized.
3. in order to support dynamically checking, the cache may be
cleared if
a particular cipher is not found or a new SSLContext is generated. As
bring the performance impact of #1 back again.
Later, AEAD algorithm is defined by Java. The same mechanism is
used to
support AEAD ciphers.
Now, EC and GCM algorithms are supported in JDK crypto providers. The
hard-coded checking can get improved. This fix updates:
1. remove the dynamically checking of cipher suite availability.
2. remove the cipher suite availability cache accordingly (need no
synchronization accordingly)
3. other updates that impact by the availability checking.
The performance improvement is tested with the following simple case.
Run the following code 1000, 2000, 3000 times in single thread mode and
measure the millisecond for each:
---------
String[] cipherSuites =
{"TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256"};
for (int i = 0; i < loops; i++) { // loops: 1000, 2000, 3000
SSLEngine engine = SSLContext.getDefault().createSSLEngine();
engine.getEnabledCipherSuites();
engine.getSupportedCipherSuites();
}
---------
The milliseconds for each test case (less is better) look like:
loops | old | fixed
---------+---------+----------
1000 | 2736 | 735
---------+---------+----------
2000 | 3718 | 746
---------+---------+----------
3000 | 4750 | 765
---------+---------+----------
This fix improves the performance.
The existing regression testing get passed. No new regression test is
planned as this is a performance enhancement fix.
Thanks,
Xuelei
