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

[Erik Joelsson]

Build change looks ok.

/Erik

On 2013-09-17 13:07, Weijun Wang wrote:
> 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
>>>