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