On Fri, 23 Jul 2021 22:18:26 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Please review this fix to change the internal `X509CertImpl.getFingerprint`
>> method to not return "" as a fingerprint if there is an error generating
>> that fingerprint. Instead, `null` is now returned, and "" is no longer
>> cached as a valid fingerprint. Although errors generating fingerprints
>> should be very rare, this is a cleaner way to handle them.
>>
>> Also, debugging messages have been added when there is an exception. And, as
>> a memory/performance improvement, `X509CertImpl.getFingerprint` now calls
>> `X509CertImpl.getEncodedInternal` which avoids cloning the encoded bytes if
>> the `Certificate` is an instance of `X509CertImpl`.
>
> src/java.base/share/classes/sun/security/validator/SymantecTLSPolicy.java
> line 161:
>
>> 159: X509Certificate anchor = chain[chain.length-1];
>> 160: String fp = fingerprint(anchor);
>> 161: if (fp != null && FINGERPRINTS.contains(fp)) {
>
> I understand the original behavior is also bypassing the check if fingerprint
> cannot be calculated, but this sounds a little irresponsible. Same as in
> `UntrustedCertificates`.
Right, I see your point, and I agree it makes more sense in this case to fail,
so I will change it. However, if this situation were to occur because of a
rogue X509Certificate impl, it is also possible for it to return an encoding
that generates a different fingerprint.
> src/java.base/share/classes/sun/security/x509/X509CertImpl.java line 1924:
>
>> 1922: x -> getFingerprintInternal(x, debug));
>> 1923: }
>> 1924:
>
> I'm a little confused by these methods. Can you inline
> `getFingerprintInternal` and rename `getFingerprint` on line 1936 to
> `getFingerprintInternal`?
Not sure what you mean by inline. Do you mean use an anonymous inner class
instead of a lambda for the `Function` argument to
`ConcurrentHashMap.computeIfAbsent`? Note that `getFingerprintInternal` calls
`this.getEncodedInternal` so it cannot be static.
> test/jdk/sun/security/x509/X509CertImpl/GetFingerprintError.java line 53:
>
>> 51:
>> 52: // test cert with bad encoding
>> 53: X509Certificate fcert = new X509CertificateWithBadEncoding(cert);
>
> In fact, `new X509CertImpl()` satisfies your requirement perfectly, which is
> an unpopulated cert with no encoding. It might be a little weird though. You
> can continue with your choice.
Although I didn't include it as part of this change, there are a number of
other tests that override a few methods of X509Certificate and the code can be
changed to use this pattern. I'll file a follow-on issue to clean that up.
-------------
PR: https://git.openjdk.java.net/jdk/pull/4891
