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

[Weijun Wang]

Webrev updated to version 02 at

   http://cr.openjdk.java.net/~weijun/8011402/webrev.02/

Changes since webrev.01:

1. Makefiles:
    - new build logic outside "ifndef OPENJDK"
    - Add a sed check to make sure open and close list use same algorithm

2. Fingerprint calculation moved into X509CertImpl using a 
ConcurrrentHashMap, although we only use one algorithm now.

3. Certificate::hashCode is now 0 if it's not a X509Cert

4. Cleanup comments in blacklisted.certs.pem, only subject/issuer/serial 
remain

5. Test moved to lib/security and check more.

I didn't change Certificate's private hash field to volatile.

Thanks
Max


On 9/13/13 8:29 AM, Weijun Wang wrote:
>
>
> 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
>>