On 9/13/13 5:15 AM, Sean Mullan wrote:
Ok, I suggested you use a WeakHashMap but now I'm a little concerned
this could become a bottleneck if every certificate check has to lock
the map. Hmm. Maybe we should go back to the previous code, that also
had some concurrency issues but it was only per certificate, and wasn't
too bad since the hash would always be the same (maybe we could just
mark the fingerprint variable volatile). I think this is worse because
the lock needs to be obtained when any certificate is checked. Also,
have you thought about computing the fingerprint as you read in the
bytes of the certificate? This means every certificate object (at least
our own implementation) would have the fingerprint calculated already,
but since you are calculating the hash as you are reading in the bytes,
the performance impact might not be much at all. However one issue is
that you don't know what algorithm to use, unless you read in the
blacklist file. We could just assume SHA-256 for now.
I'll think about it later. Some CPU bugs on the plate now.
Maybe I can keep a small Map<Algorithm,fingerprint> in each certificate,
and as you suggested, pre-fill the SHA-256 value at read time.
ConcurrentMap should have a method called getOrCalculate that takes a
lambda that could calucalte the value lazily.
Certificate:
[70] I would mark this volatile.
Aha, I copied the logic from String and thought it must be the safest.
[134] the old code returned 0, seems we should preserve that even though
this is an odd error case
Do you think I should also revert the calculation from
"Arrays.hashCode(X509CertImpl.getEncodedInternal(this))" to the old for
loop?
CheckAll:
I would move this test to test/lib/java/security and rename it to
CheckBlacklistedCerts.
Certainly.
You should also check that the files have the same number of entries.
In my assumption, open and closed could have dups. I'll compare the Set
size then.
UntrustedCertificates:
[84] nit: insert spaces around the "=" and "<"
OK.
Thanks
Max
--Sean
On 09/11/2013 09:32 AM, Weijun Wang wrote:
Slightly updated at the same location.
Added different algorithm check in the 2 makefiles.
Thanks
Max
On 9/11/13 3:57 PM, Weijun Wang wrote:
Hi Sean and Erik
An updated webrev is at
http://cr.openjdk.java.net/~weijun/8011402/webrev.01/
Changes since the last webrev:
- Some makefile changes
* wildcard on closed file
* make sure the file's first line is always "Algorithm="
- Move fingerprint cache for cert from X509CertImpl to
UntrustedCertificates
- Cache hash for Certificate
- log blacklist parsing error in UntrustedCertificates
- A new test
Thanks
Max
On 9/6/13 9:30 PM, Weijun Wang wrote:
Hi Sean
Please review the code changes at
8011402: Move blacklisting certificate logic from hard code to data
http://cr.openjdk.java.net/~weijun/8011402/webrev.00/
Hard coded blacklisted certificates are moved out of the class file and
now inside a data file. Furthermore, only their fingerprints are
released in the JRE. The makefile covers blacklist files in both open
and closed repo.
No regression test, cleanup.
*build-dev*, I am not an export of Makefile, and I have some questions:
1. I create a new macro (or function?) called cat-files. Its only
difference from install-file is that it needs to deal with two inputs.
Do we already have a similar macro somewhere?
2. cat-files is defined inside CopyFiles.gmk right beside its usage. Do
you think it's better to define it in a common file?
3. Most important: it only works if both $(BLACKLISTED_CERTS_SRC_OPEN)
and $(BLACKLISTED_CERTS_SRC_CLOSED) already exists. Currently there is
no closed blacklist, but I still have to create an empty file there.
Otherwise, there will be
make[2]: *** No rule to make target
`/space/repos/jdk8/tl/jdk/src/closed/share/lib/security/blacklisted.certs',
needed by
`/space/repos/jdk8/tl/build/macosx-x86_64-normal-server-release/jdk/lib/security/blacklisted.certs'.
Stop.
Is there a way to make it work without adding that empty file?
Thanks
Max
