New webrev:
http://cr.openjdk.java.net/~xuelei/8129988/webrev.04/
On 1/4/2017 1:27 PM, Sean Mullan wrote:
It's unfortunate that the previous code read the system properties each
time a TrustManagerFactory was initialized. I think in practice, if
applications are changing these system properties on the fly it isn't
going to work predictably. This implementation would have been much
simpler if the properties were read once, and then you could just check
the lastModified time.
But, I suppose we should err on the side of caution and maintain that
behavior.
I don't like the old behavior, which is error-prone. But for safe we
may want to keep the behavior unchanged at this stage.
A few comments:
160 // Not break, the file is inaccessbile.
Typo: inaccessible
191 Objects.equals(this.storePassword,
that.storePassword));
Let's avoid this. I don't think it's possible for an app to leverage
this to try to guess the password, but let's err on the side of caution.
If the password has changed, then the file's lastModified time would
need to be updated anyway, so you don't really need to check this.
Good. Updated.
326 this.ksRef = new WeakReference<>(ks);
ks is a local variable that goes out of scope when this method is
finished, so I don't think this will work as you expect.
Good catch! I made an update that the local key store reference will
not reserved any more.
Thanks,
Xuelei
--Sean
On 1/4/17 1:38 PM, Xuelei Fan wrote:
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
