RFR: 8273236: keytool does not accurately warn about algorithms that are disabled but have additional constraints [v2]

[Hai-May Chao]

On Thu, 13 Jan 2022 16:31:35 GMT, Sean Mullan <mullan at openjdk.org> wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Update with review comments
>
> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 187:
> 
>> 185:     private List<String> weakWarnings = new ArrayList<>();
>> 186: 
>> 187:     Set<X509Certificate> trustedCerts = new HashSet<>();
> 
> Make private.

Fixed.

> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1129:
> 
>> 1127:         }
>> 1128: 
>> 1129:         buildTrustedCerts();
> 
> Can we reuse the keystore loaded by `buildTrustedCerts()` instead of reloading it again on line 1138?

No change. This is because `caks` global variable can only be initialized with cacerts keystore  when the `trustcacerts` option is specified; otherwise if has to be kept null. `buildTrustedCerts`() is always executed.

> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1558:
> 
>> 1556:         checkWeakConstraint(rb.getString("the.issuer"),
>> 1557:                 keyStore.getCertificateChain(alias));
>> 1558:         cpcp = new CertPathConstraintsParameters(cert, null, null, null);
> 
> You could also specify the variant parameter if the certificate contains an EKU extension. The mapping from EKU to variant would be:
> 
> - anyExtendedKeyUsage -> VAR_GENERIC
> - serverAuth -> VAR_TLS_SERVER
> - clientAuth -> VAR_TLS_CLIENT
> - codeSigning -> VAR_CODE_SIGNING
> - emailProtection -> none defined, so just use null or VAR_GENERIC
> - timeStamping -> VAR_TSA_SERVER
> - OCSPSigning -> none defined, so just use null or VAR_GENERIC

Fixed.

> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1706:
> 
>> 1704:         }
>> 1705:         dumpCert(cert, out);
>> 1706:         CertPathConstraintsParameters cpcp =
> 
> Here you could also specify the variant if the cert contains an EKU extension. Same comment applies to other places where you are checking the algorithm constraints of a certificate.

Fixed.

> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 2198:
> 
>> 2196:                         ("Certificate.chain.length.") + chain.length);
>> 2197: 
>> 2198:                     X509Certificate[] xcerts = convertCerts(chain);
> 
> I think you can just cast to an `X509Certificate[]` instead of reparsing all the certificates, i.e.:
> 
> `X509Certificate[] xcerts = (X509Certificate[]) chain;`

I got `java.lang.ClassCastException` when casting `Certificate[]` array form here. However, I should cast Certificate object directly instead of reparsing the certificate, and made the change to do so.

> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 2259:
> 
>> 2257:             }
>> 2258:             cpcp = new CertPathConstraintsParameters((X509Certificate)cert,
>> 2259:                     null,null, null);
> 
> Nit - add space between `null,null`.

Fixed.

> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 5039:
> 
>> 5037:                         trustedCerts.add((X509Certificate)caks.getCertificate(a));
>> 5038:                     } catch (Exception e2) {
>> 5039:                         // ignore, when a SecretkeyEntry does not include a cert
> 
> Not sure I understand this comment, as these should not be SecretKeyEntries. You should still ignore it but this should not throw an Exception unless the KeyStore was not loaded/initialized properly (which normally won't occur).

Updated with the suggested comments.

> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 5048:
> 
>> 5046:     }
>> 5047: 
>> 5048:     private TrustAnchor findTrustAnchor(List<X509Certificate> chain) {
> 
> I would consider having an initial check that returns `null` if `chain.isEmpty()`. Not sure if that is a valid scenario, but it would avoid an `IndexOOBException` just in case.

Added the initial check.

> src/java.base/share/classes/sun/security/tools/keytool/Resources.java line 486:
> 
>> 484:         {"verified.by.s.in.s.weak", "Verified by %1$s in %2$s with a %3$s"},
>> 485:         {"whose.sigalg.disabled", "%1$s uses the %2$s signature algorithm which is considered a security risk and is disabled."},
>> 486:         {"whose.sigalg.usagesignedjar", "%1$s uses the %2$s signature algorithm which is considered a security risk and cannot be used to sign JARs after 2019-01-01."},
> 
> Instead of hard-coding "2019-01-01", we should extract this date from the `denyAfter` attribute of the `jdk.certpath.disabledAlgorithms` security property and pass it in as a parameter.

Fixed.

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

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