webrev.01 of 8011402: Move blacklisting certificate logic from hard code to data

[Sean Mullan]

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