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

[Sean Mullan]

Here are more comments on TrustStoreManager:

  87-93: these variables can be declared private

  120                     new PrivilegedAction<TrustStoreDescriptor>() {

use <> operator

  152                                 // Not break, so the file is 
accessbile.

s/accessbile/inaccessible/

  156                                         "Not accessible trust 
store: " +

s/Not accessible/Inaccessible/

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.

  159                             } catch (Exception exp) {

Why is this needed? This is done inside a doPriv, so no SecurityExc 
should be thrown.

  198         public int hashCode() {

Seems like you might want to compute the hashcode once and store it.

  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? 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?

  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.

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
>
>   54         Set<X509Certificate> set = new HashSet<X509Certificate>();
>
> Use <> operator.
>
>   71         } catch (KeyStoreException e) {
>   72             // ignore
>   73         }
>
> This should be rare, but maybe we want to log this.
>
> * TrustStoreManager.java
>
>   65     public static KeyStore getgetTrustedKeyStore() throws Exception {
>
> s/getget/get/
>
> * TrustManagerFactoryImpl.java
>
>   85     abstract X509TrustManager getInstance(
>   86             Collection<X509Certificate> trustedCerts) throws
> KeyStoreException;
>
> It no longer makes sense for this method to throw KeyStoreException.
>
> * 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.
>
>  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.
>
> --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
>>