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

[Weijun Wang]

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
>