Re: RFR: 8273236: keytool does not accurately warn about algorithms that are disabled but have additional constraints [v2]
> `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. Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision: Update with review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/7039/files - new: https://git.openjdk.java.net/jdk/pull/7039/files/10e73e50..a05728a7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7039&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7039&range=00-01 Stats: 60 lines in 3 files changed: 20 ins; 11 del; 29 mod Patch: https://git.openjdk.java.net/jdk/pull/7039.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7039/head:pull/7039 PR: https://git.openjdk.java.net/jdk/pull/7039
Re: RFR: 8273236: keytool does not accurately warn about algorithms that are disabled but have additional constraints [v2]
On Thu, 13 Jan 2022 16:31:35 GMT, Sean Mullan 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 weakWarnings = new ArrayList<>();
>> 186:
>> 187: Set 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 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
Re: RFR: 8273236: keytool does not accurately warn about algorithms that are disabled but have additional constraints [v2]
On Fri, 21 Jan 2022 03:27:44 GMT, Hai-May Chao wrote: >> 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. I was thinking `buildTrustedCerts` could return the cacerts `KeyStore`, and you could assign that instead to `caks` if `trustcacerts` is true. - PR: https://git.openjdk.java.net/jdk/pull/7039
Re: RFR: 8273236: keytool does not accurately warn about algorithms that are disabled but have additional constraints [v2]
On Fri, 21 Jan 2022 03:34:24 GMT, Hai-May Chao 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.
>
> 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 4908:
> 4906: if (eMessage.contains("denyAfter constraint check
> failed") &&
> 4907: e.getReason() ==
> BasicReason.ALGORITHM_CONSTRAINED) {
> 4908: String separator = "java.security: ";
Did you consider extracting the date from the security property? Ex:
`Security.getProperty("jdk.certpath.disabledAlgorithms")`? I think that would
be a better solution instead of parsing the exception message, which might
change in the future.
-
PR: https://git.openjdk.java.net/jdk/pull/7039
Re: RFR: 8273236: keytool does not accurately warn about algorithms that are disabled but have additional constraints [v2]
On Mon, 24 Jan 2022 16:12:25 GMT, Sean Mullan 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 4908:
>
>> 4906: if (eMessage.contains("denyAfter constraint check
>> failed") &&
>> 4907: e.getReason() ==
>> BasicReason.ALGORITHM_CONSTRAINED) {
>> 4908: String separator = "java.security: ";
>
> Did you consider extracting the date from the security property? Ex:
> `Security.getProperty("jdk.certpath.disabledAlgorithms")`? I think that would
> be a better solution instead of parsing the exception message, which might
> change in the future.
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/7039
Re: RFR: 8273236: keytool does not accurately warn about algorithms that are disabled but have additional constraints [v2]
On Mon, 24 Jan 2022 21:17:42 GMT, Hai-May Chao wrote:
>> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 4908:
>>
>>> 4906: if (eMessage.contains("denyAfter constraint check
>>> failed") &&
>>> 4907: e.getReason() ==
>>> BasicReason.ALGORITHM_CONSTRAINED) {
>>> 4908: String separator = "java.security: ";
>>
>> Did you consider extracting the date from the security property? Ex:
>> `Security.getProperty("jdk.certpath.disabledAlgorithms")`? I think that
>> would be a better solution instead of parsing the exception message, which
>> might change in the future.
>
> Fixed.
After further thought, I'm now not sure my suggestion is any better (sorry for
the rework). It is possible that there could be more than one `denyAfter`
constraint, and in that case, you would need to also match on the algorithm
that the constraint applies to, and that gets pretty complicated.
So, I now think your previous fix is probably better, even though it means we
are depending on the syntax of the exception message. To avoid that from
causing issues in the future, I would enhance your regression test to fail if
the exception message changes in the future such that the denyAfter date cannot
be parsed and is not what is expected.
-
PR: https://git.openjdk.java.net/jdk/pull/7039
