Code Review Request JDK-8129988 JSSE should create a single instance of the cacerts KeyStore

[Xuelei Fan]

New: http://cr.openjdk.java.net/~xuelei/8129988/webrev.01/

On 12/19/2016 12:35 PM, Sean Mullan wrote:
> Here are more comments on TrustStoreManager:
>
>  87-93: these variables can be declared private
>
>  120                     new PrivilegedAction<TrustStoreDescriptor>() {
>
> use <> operator
>
Updated.

>  152                                 // Not break, so the file is
> accessbile.
>
> s/accessbile/inaccessible/
>
>  156                                         "Not accessible trust
> store: " +
>
> s/Not accessible/Inaccessible/
>
updated.

> We probably should avoid dumping the filename since the pathname may
> contain sensitive info and can end up in logs. I would probably print
> the property name instead as it still contains enough info to debug the
> problem. Same comment on lines 163-4.
>
Good point.  Updated to use property name.

>  159                             } catch (Exception exp) {
>
> Why is this needed? This is done inside a doPriv, so no SecurityExc
> should be thrown.
>
Right, this block is useless.  Removed.

>  198         public int hashCode() {
>
> Seems like you might want to compute the hashcode once and store it.
>
The hashCode() method is not actually used in the context.  The code is 
Saving a cache needs additional space and synchronization.  We can make 
the improvement later if the method is used.

>  213             if (storePassword != null && !storePassword.isEmpty()) {
>  214                 MessageDigest md =
> JsseJce.getMessageDigest("SHA-256");
>  215                 result = 31 * result +
>  216 Arrays.hashCode(md.digest(storePassword.getBytes()));
>  217             }
>
> Why are you hashing the password here? Are you afraid this could be
> leaked or guessed somehow?
Yes.  The hash code of the password part can be computed.  I was 
wondering the String.hashCode() may not have sufficient strength.

> I would just leave the password out of the
> hashcode and equals. It doesn't matter, it's still the same file, right?
> I'm not sure if the type or provider matter either. Don't you just care
> about the name of the file and the modification time?
>
For file type key store, the file and the modification time should be 
sufficient.  But for non-file (PKCS11) key store, the provider and 
password may be sensible.  The basic idea is that, if one of the system 
property get updated, the key store should be reloaded.  Checking every 
property update makes the code more straightforward.

>  268             if ((temporaryDesc != null) &&
>
> Why would a null descriptor ever be ok? Shouldn't you just let this
> throw NPE? Same comment on line 301.
>
The temporaryDesc is initialized as null.  A singleton service 
(TrustStoreManage.tam) is used and lazy loaded.  Null means the 
descriptor has not been assigned.

> That's all my comments for now. I'll wait until you post another update
> to finish my review.
>
> --Sean
>
>
> On 12/16/16 11:14 AM, Sean Mullan wrote:
>> Hi Xuelei,
>>
>> First cut of a review at this. I still need to review TrustStoreManager,
>> so will get back to you later on that. Looks very good so far.
>>
>> * KeyStores.java
>>
>> Do an 'hg rename' on this file, so you can see the diffs between this
>> and the new name (TrustStoreUtil), and you preserve the history.
>>
>> * TrustStoreUtil.java
>>
'hg rename' is used.   Looks like webrev cannot identify rename of files.

>>   54         Set<X509Certificate> set = new HashSet<X509Certificate>();
>>
>> Use <> operator.
>>
Updated.

>>   71         } catch (KeyStoreException e) {
>>   72             // ignore
>>   73         }
>>
>> This should be rare, but maybe we want to log this.
>>
Not sure log it in JSSE or PKIC.  I added a comment, may add the debug 
log in the future.

>> * TrustStoreManager.java
>>
>>   65     public static KeyStore getgetTrustedKeyStore() throws
>> Exception {
>>
>> s/getget/get/
>>
Ooops.  Updated.

>> * TrustManagerFactoryImpl.java
>>
>>   85     abstract X509TrustManager getInstance(
>>   86             Collection<X509Certificate> trustedCerts) throws
>> KeyStoreException;
>>
>> It no longer makes sense for this method to throw KeyStoreException.
>>
good point!  Updated.

>> * X509TrustManagerImpl.java
>>
>>   71     X509TrustManagerImpl(String validatorType,
>>   72             Collection<X509Certificate> trustedCerts) throws
>> KeyStoreException {
>>
>> No longer can throw KeyStoreException so it can be removed from throws
>> clause.
>>
Updated.

>>  83         if (debug != null && Debug.isOn("trustmanager")) {
>>  84             showTrustedCerts();
>>
>> Not sure why you made this change (and on line 99) because
>> showTrustedCerts is still only called if debug is enabled.
>>
When I read the code, I'm not sure what's the purpose of 
showTrustedCerts() until I rollover to showTrustedCerts implementation. 
I was wondering, this update may make the code easier to read.

Thanks,
Xuelei

>> --Sean
>>
>> On 11/26/16 7:46 PM, Xuelei Fan wrote:
>>> Hi,
>>>
>>> Please review the performance enhancement update:
>>>
>>>    http://cr.openjdk.java.net/~xuelei/8129988/webrev.00/
>>>
>>> In SunJSSE provider, there are two ways to use the default trust store
>>> (lib/security/cacerts), using the default SSLContext instance or using
>>> the default trust manager.
>>>
>>> The default SSLContext holds a strong reference to a collection of
>>> trusted certificates in cacerts in static mode.  The default trust
>>> manager reads the cacerts file and creates a KeyStore and parses the
>>> certificates each time.
>>>
>>> With the growth of cacerts, the loading and cache of trusted certificate
>>> is not performance friendly.
>>>
>>> In this fix, I'm trying to find a balance between CPU and memory: reuse
>>> the loaded trusted certificates if possible and release the unused
>>> trusted certificates if necessary.
>>>
>>> Thanks,
>>> Xuelei
>>>