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

[Sean Mullan]

On Wed, 12 Jan 2022 02:15:45 GMT, Hai-May Chao <hchao at openjdk.org> wrote:

> `keytool` currently uses a simpler scheme in `DisabledAlgorithmConstraints` class when performing algorithm constraints checks. This change is to enhance `keytool` to make use of the new methods `DisabledAlgorithmConstraints.permits` with `CertPathConstraintsParameters` and `checkKey` parameters. For the keyusage in the EE certificate of a certificate chains, set the variant accordingly when calling `CertPathConstraintsParameters` constructor.

Some comments so far. Will continue reviewing later.

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.

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?

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

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.

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).

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

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