On Thu, 13 Jan 2022 16:31:35 GMT, Sean Mullan <mul...@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
