RFR: 8270946: X509CertImpl.getFingerprint should not return the empty String

[Sean Mullan]

On Fri, 23 Jul 2021 22:18:26 GMT, Weijun Wang <weijun at 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