webrev.01 of 8011402: Move blacklisting certificate logic from hard code to data
Sean Mullan
sean.mullan at oracle.com
Thu Sep 12 21:15:27 UTC 2013
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
More information about the security-dev
mailing list