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.
Certificate:
[70] I would mark this volatile.
[134] the old code returned 0, seems we should preserve that even though
this is an odd error case
CheckAll:
I would move this test to test/lib/java/security and rename it to
CheckBlacklistedCerts.
You should also check that the files have the same number of entries.
UntrustedCertificates:
[84] nit: insert spaces around the "=" and "<"
--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
