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
