On Fri, 23 Jul 2021 17:16:26 GMT, Sean Mullan <mul...@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`.

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`?

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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/4891

Reply via email to