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

[Sean Mullan]

On 12/22/16 2:52 PM, Xuelei Fan wrote:
> updated: http://cr.openjdk.java.net/~xuelei/8129988/webrev.02/

I think there are still some race conditions. For example:

  264             TrustStoreDescriptor temporaryDesc = this.descriptor;
  265             KeyStore cachedKeyStore = ksRef.get();
  266             if (descriptor.equals(temporaryDesc) && 
(cachedKeyStore != null)) {
  267                 return cachedKeyStore;
  268             }

There is no locking here.

Maybe that's ok based on your explanation below, but it seems a bit 
fragile and could lead to problems that are hard to debug. Have you 
looked at the AtomicReference class? You could define a new class 
containing the descriptor, keystore, and certs and wrap that in an 
AtomicReference and then use the methods on that class to update it.
Might be worth exploring that a bit more.

--Sean


> On 12/22/2016 9:32 AM, Sean Mullan wrote:
>> On 12/20/16 3:21 PM, Xuelei Fan wrote:
>>
>>>>  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.
>>
>> But the main focus of this performance issue is for the cacerts file,
>> which is not PKCS11. So I would not use the password and other
>> non-relevant or security-sensitive attributes. A hash of the password
>> isn't sufficient against dictionary-type attacks, for example.
>>
> I see your point.  The password hash code block is removed.
>
>>>>  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.
>>
>> I think there are thread-safeness issues in the TrustStoreManager class.
>> You are not synchronizing when you read so looks like there can be
>> various race conditions. For example, this.descriptor and this.ksRef can
>> be updated by another thread in the middle of this code
>>
>>  267             TrustStoreDescriptor temporaryDesc = this.descriptor;
>>  268             if ((temporaryDesc != null) &&
>>  269                 temporaryDesc.equals(descriptor)) {
>>  270                 KeyStore ks = ksRef.get();
>>  271                 if (ks != null) {
>>  272                     return ks;
>>  273                 }
>>  274             }
>>
>> Maybe that doesn't really matter, but I'm not sure -- have you thought
>> about it?
>>
> I thought about the issue.  But I really missed to the double check
> idiom.  Updated.
>
> For performance consideration, I'm trying to mitigate the impact of
> synchronization.  Once the key store get loaded, there is a strong
> reference, and it can be used safely.  If another thread is trying to
> modify the descriptor and key store, this thread will use the existing
> key store, and another thread can use the new key store.  If two threads
> try to modify the key store for the same descriptor, I added the double
> check idiom so that the first thread will complete the update and the
> 2nd thread will use the 1st thread updated key store.  If two threads
> try to modify the key store for different descriptor, each will get a
> different key store and the 2nd thread will reset the final key store
> for future use.
>
> In general, applications would not modify the system properties.  So the
> use of the synchronized block should be very rare.  It benefits the
> performance in multiple threading computation environment.
>
> Xuelei
>
>> --Sean