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

[Xuelei Fan]

Updated to use synchronized method.  Most of the time, the synchronized 
only perform the read access to the variables. The impact on performance 
should be acceptable.

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

Only the TrustStoreManager.java implementation get updated in this webrev.

Thanks,
Xuelei

On 12/23/2016 7:52 AM, Sean Mullan wrote:
> 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