8247960: jarsigner says "signer errors" for some normal warnings when -strict is set

8250586: jarsigner refactoring in displayMessagesAndResult() method

Reviewed-by: weijun

src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java

@@ -1059,177 +1059,155 @@ public class Main {
         boolean signerNotExpired = expireDate == null
                 || expireDate.after(new Date());
 
-        if (badKeyUsage || badExtendedKeyUsage || badNetscapeCertType ||
+        if (badKeyUsage) {
-                notYetValidCert || chainNotValidated || hasExpiredCert ||
+            errors.add(isSigning
-                hasUnsignedEntry || signerSelfSigned || (legacyAlg != 0) ||
+                    ? rb.getString("The.signer.certificate.s.KeyUsage.extension.doesn.t.allow.code.signing.")
-                (disabledAlg != 0) || aliasNotInStore || notSignedByAlias ||
+                    : rb.getString("This.jar.contains.entries.whose.signer.certificate.s.KeyUsage.extension.doesn.t.allow.code.signing."));
-                tsaChainNotValidated ||
-                (hasExpiredTsaCert && !signerNotExpired)) {
-
-            if (strict) {
-                result = isSigning
-                        ? rb.getString("jar.signed.with.signer.errors.")
-                        : rb.getString("jar.verified.with.signer.errors.");
-            } else {
-                result = isSigning
-                        ? rb.getString("jar.signed.")
-                        : rb.getString("jar.verified.");
-            }
-
-            if (badKeyUsage) {
-                errors.add(isSigning
-                        ? rb.getString("The.signer.certificate.s.KeyUsage.extension.doesn.t.allow.code.signing.")
-                        : rb.getString("This.jar.contains.entries.whose.signer.certificate.s.KeyUsage.extension.doesn.t.allow.code.signing."));
-            }
-
-            if (badExtendedKeyUsage) {
-                errors.add(isSigning
-                        ? rb.getString("The.signer.certificate.s.ExtendedKeyUsage.extension.doesn.t.allow.code.signing.")
-                        : rb.getString("This.jar.contains.entries.whose.signer.certificate.s.ExtendedKeyUsage.extension.doesn.t.allow.code.signing."));
-            }
-
-            if (badNetscapeCertType) {
-                errors.add(isSigning
-                        ? rb.getString("The.signer.certificate.s.NetscapeCertType.extension.doesn.t.allow.code.signing.")
-                        : rb.getString("This.jar.contains.entries.whose.signer.certificate.s.NetscapeCertType.extension.doesn.t.allow.code.signing."));
-            }
-
-            // only in verifying
-            if (hasUnsignedEntry) {
-                errors.add(rb.getString(
-                        "This.jar.contains.unsigned.entries.which.have.not.been.integrity.checked."));
-            }
-            if (hasExpiredCert) {
-                errors.add(isSigning
-                        ? rb.getString("The.signer.certificate.has.expired.")
-                        : rb.getString("This.jar.contains.entries.whose.signer.certificate.has.expired."));
-            }
-            if (notYetValidCert) {
-                errors.add(isSigning
-                        ? rb.getString("The.signer.certificate.is.not.yet.valid.")
-                        : rb.getString("This.jar.contains.entries.whose.signer.certificate.is.not.yet.valid."));
-            }
-
-            if (chainNotValidated) {
-                errors.add(String.format(isSigning
-                                ? rb.getString("The.signer.s.certificate.chain.is.invalid.reason.1")
-                                : rb.getString("This.jar.contains.entries.whose.certificate.chain.is.invalid.reason.1"),
-                        chainNotValidatedReason.getLocalizedMessage()));
-            }
-
-            if (hasExpiredTsaCert) {
-                errors.add(rb.getString("The.timestamp.has.expired."));
-            }
-            if (tsaChainNotValidated) {
-                errors.add(String.format(isSigning
-                                ? rb.getString("The.tsa.certificate.chain.is.invalid.reason.1")
-                                : rb.getString("This.jar.contains.entries.whose.tsa.certificate.chain.is.invalid.reason.1"),
-                        tsaChainNotValidatedReason.getLocalizedMessage()));
-            }
-
-            // only in verifying
-            if (notSignedByAlias) {
-                errors.add(
-                        rb.getString("This.jar.contains.signed.entries.which.is.not.signed.by.the.specified.alias.es."));
-            }
-
-            // only in verifying
-            if (aliasNotInStore) {
-                errors.add(rb.getString("This.jar.contains.signed.entries.that.s.not.signed.by.alias.in.this.keystore."));
-            }
-
-            if (signerSelfSigned) {
-                errors.add(isSigning
-                        ? rb.getString("The.signer.s.certificate.is.self.signed.")
-                        : rb.getString("This.jar.contains.entries.whose.signer.certificate.is.self.signed."));
-            }
-
-            if (isSigning) {
-                if ((legacyAlg & 1) == 1) {
-                    warnings.add(String.format(
-                            rb.getString("The.1.algorithm.specified.for.the.2.option.is.considered.a.security.risk..This.algorithm.will.be.disabled.in.a.future.update."),
-                            digestalg, "-digestalg"));
-                }
-
-                if ((disabledAlg & 1) == 1) {
-                    errors.add(String.format(
-                            rb.getString("The.1.algorithm.specified.for.the.2.option.is.considered.a.security.risk.and.is.disabled."),
-                            digestalg, "-digestalg"));
-                }
-
-                if ((legacyAlg & 2) == 2) {
-                    warnings.add(String.format(
-                            rb.getString("The.1.algorithm.specified.for.the.2.option.is.considered.a.security.risk..This.algorithm.will.be.disabled.in.a.future.update."),
-                            sigalg, "-sigalg"));
-                }
-                if ((disabledAlg & 2) == 2) {
-                    errors.add(String.format(
-                            rb.getString("The.1.algorithm.specified.for.the.2.option.is.considered.a.security.risk.and.is.disabled."),
-                            sigalg, "-sigalg"));
-                }
-
-                if ((legacyAlg & 4) == 4) {
-                    warnings.add(String.format(
-                            rb.getString("The.1.algorithm.specified.for.the.2.option.is.considered.a.security.risk..This.algorithm.will.be.disabled.in.a.future.update."),
-                            tSADigestAlg, "-tsadigestalg"));
-                }
-                if ((disabledAlg & 4) == 4) {
-                    errors.add(String.format(
-                            rb.getString("The.1.algorithm.specified.for.the.2.option.is.considered.a.security.risk.and.is.disabled."),
-                            tSADigestAlg, "-tsadigestalg"));
-                }
-
-                if ((legacyAlg & 8) == 8) {
-                    warnings.add(String.format(
-                            rb.getString("The.1.signing.key.has.a.keysize.of.2.which.is.considered.a.security.risk..This.key.size.will.be.disabled.in.a.future.update."),
-                            privateKey.getAlgorithm(), KeyUtil.getKeySize(privateKey)));
-                }
-                if ((disabledAlg & 8) == 8) {
-                    errors.add(String.format(
-                            rb.getString("The.1.signing.key.has.a.keysize.of.2.which.is.considered.a.security.risk.and.is.disabled."),
-                            privateKey.getAlgorithm(), KeyUtil.getKeySize(privateKey)));
-                }
-            } else {
-                if ((legacyAlg & 1) != 0) {
-                    warnings.add(String.format(
-                            rb.getString("The.digest.algorithm.1.is.considered.a.security.risk..This.algorithm.will.be.disabled.in.a.future.update."),
-                            legacyDigestAlg));
-                }
-
-                if ((legacyAlg & 2) == 2) {
-                     warnings.add(String.format(
-                             rb.getString("The.signature.algorithm.1.is.considered.a.security.risk..This.algorithm.will.be.disabled.in.a.future.update."),
-                             legacySigAlg));
-                }
-
-                if ((legacyAlg & 4) != 0) {
-                    warnings.add(String.format(
-                            rb.getString("The.timestamp.digest.algorithm.1.is.considered.a.security.risk..This.algorithm.will.be.disabled.in.a.future.update."),
-                            legacyTsaDigestAlg));
-                }
-
-                if ((legacyAlg & 8) == 8) {
-                    warnings.add(String.format(
-                            rb.getString("The.1.signing.key.has.a.keysize.of.2.which.is.considered.a.security.risk..This.key.size.will.be.disabled.in.a.future.update."),
-                            weakPublicKey.getAlgorithm(), KeyUtil.getKeySize(weakPublicKey)));
-                }
-            }
-        } else {
-            result = isSigning ? rb.getString("jar.signed.") : rb.getString("jar.verified.");
         }
 
+        if (badExtendedKeyUsage) {
+            errors.add(isSigning
+                    ? rb.getString("The.signer.certificate.s.ExtendedKeyUsage.extension.doesn.t.allow.code.signing.")
+                    : rb.getString("This.jar.contains.entries.whose.signer.certificate.s.ExtendedKeyUsage.extension.doesn.t.allow.code.signing."));
+        }
+
+        if (badNetscapeCertType) {
+            errors.add(isSigning
+                    ? rb.getString("The.signer.certificate.s.NetscapeCertType.extension.doesn.t.allow.code.signing.")
+                    : rb.getString("This.jar.contains.entries.whose.signer.certificate.s.NetscapeCertType.extension.doesn.t.allow.code.signing."));
+        }
+
+        // only in verifying
+        if (hasUnsignedEntry) {
+            errors.add(rb.getString(
+                    "This.jar.contains.unsigned.entries.which.have.not.been.integrity.checked."));
+        }
+
+        if (hasExpiredCert) {
+            errors.add(isSigning
+                    ? rb.getString("The.signer.certificate.has.expired.")
+                    : rb.getString("This.jar.contains.entries.whose.signer.certificate.has.expired."));
+        }
+
+        if (notYetValidCert) {
+            errors.add(isSigning
+                    ? rb.getString("The.signer.certificate.is.not.yet.valid.")
+                    : rb.getString("This.jar.contains.entries.whose.signer.certificate.is.not.yet.valid."));
+        }
+
+        if (chainNotValidated) {
+            errors.add(String.format(isSigning
+                            ? rb.getString("The.signer.s.certificate.chain.is.invalid.reason.1")
+                            : rb.getString("This.jar.contains.entries.whose.certificate.chain.is.invalid.reason.1"),
+                    chainNotValidatedReason.getLocalizedMessage()));
+        }
+
+        if (tsaChainNotValidated) {
+            errors.add(String.format(isSigning
+                            ? rb.getString("The.tsa.certificate.chain.is.invalid.reason.1")
+                            : rb.getString("This.jar.contains.entries.whose.tsa.certificate.chain.is.invalid.reason.1"),
+                    tsaChainNotValidatedReason.getLocalizedMessage()));
+        }
+
+        // only in verifying
+        if (notSignedByAlias) {
+            errors.add(
+                    rb.getString("This.jar.contains.signed.entries.which.is.not.signed.by.the.specified.alias.es."));
+        }
+
+        // only in verifying
+        if (aliasNotInStore) {
+            errors.add(rb.getString("This.jar.contains.signed.entries.that.s.not.signed.by.alias.in.this.keystore."));
+        }
+
+        if (signerSelfSigned) {
+            errors.add(isSigning
+                    ? rb.getString("The.signer.s.certificate.is.self.signed.")
+                    : rb.getString("This.jar.contains.entries.whose.signer.certificate.is.self.signed."));
+        }
+
+        if (isSigning) {
+            if ((legacyAlg & 1) == 1) {
+                warnings.add(String.format(
+                        rb.getString("The.1.algorithm.specified.for.the.2.option.is.considered.a.security.risk..This.algorithm.will.be.disabled.in.a.future.update."),
+                        digestalg, "-digestalg"));
+            }
+
+            if ((disabledAlg & 1) == 1) {
+                errors.add(String.format(
+                        rb.getString("The.1.algorithm.specified.for.the.2.option.is.considered.a.security.risk.and.is.disabled."),
+                        digestalg, "-digestalg"));
+            }
+
+            if ((legacyAlg & 2) == 2) {
+                warnings.add(String.format(
+                        rb.getString("The.1.algorithm.specified.for.the.2.option.is.considered.a.security.risk..This.algorithm.will.be.disabled.in.a.future.update."),
+                        sigalg, "-sigalg"));
+            }
+
+            if ((disabledAlg & 2) == 2) {
+                errors.add(String.format(
+                        rb.getString("The.1.algorithm.specified.for.the.2.option.is.considered.a.security.risk.and.is.disabled."),
+                        sigalg, "-sigalg"));
+            }
+
+            if ((legacyAlg & 4) == 4) {
+                warnings.add(String.format(
+                        rb.getString("The.1.algorithm.specified.for.the.2.option.is.considered.a.security.risk..This.algorithm.will.be.disabled.in.a.future.update."),
+                        tSADigestAlg, "-tsadigestalg"));
+            }
+
+            if ((disabledAlg & 4) == 4) {
+                errors.add(String.format(
+                        rb.getString("The.1.algorithm.specified.for.the.2.option.is.considered.a.security.risk.and.is.disabled."),
+                        tSADigestAlg, "-tsadigestalg"));
+            }
+
+            if ((legacyAlg & 8) == 8) {
+                warnings.add(String.format(
+                        rb.getString("The.1.signing.key.has.a.keysize.of.2.which.is.considered.a.security.risk..This.key.size.will.be.disabled.in.a.future.update."),
+                        privateKey.getAlgorithm(), KeyUtil.getKeySize(privateKey)));
+            }
+
+            if ((disabledAlg & 8) == 8) {
+                errors.add(String.format(
+                        rb.getString("The.1.signing.key.has.a.keysize.of.2.which.is.considered.a.security.risk.and.is.disabled."),
+                        privateKey.getAlgorithm(), KeyUtil.getKeySize(privateKey)));
+            }
+        } else {
+            if ((legacyAlg & 1) != 0) {
+                warnings.add(String.format(
+                        rb.getString("The.digest.algorithm.1.is.considered.a.security.risk..This.algorithm.will.be.disabled.in.a.future.update."),
+                        legacyDigestAlg));
+            }
+
+            if ((legacyAlg & 2) == 2) {
+                warnings.add(String.format(
+                        rb.getString("The.signature.algorithm.1.is.considered.a.security.risk..This.algorithm.will.be.disabled.in.a.future.update."),
+                        legacySigAlg));
+            }
+
+            if ((legacyAlg & 4) != 0) {
+                warnings.add(String.format(
+                        rb.getString("The.timestamp.digest.algorithm.1.is.considered.a.security.risk..This.algorithm.will.be.disabled.in.a.future.update."),
+                        legacyTsaDigestAlg));
+            }
+
+            if ((legacyAlg & 8) == 8) {
+                warnings.add(String.format(
+                        rb.getString("The.1.signing.key.has.a.keysize.of.2.which.is.considered.a.security.risk..This.key.size.will.be.disabled.in.a.future.update."),
+                        weakPublicKey.getAlgorithm(), KeyUtil.getKeySize(weakPublicKey)));
+            }
+        }
+
+        // This check must be placed after all other "errors.add()" calls were done.
         if (hasExpiredTsaCert) {
             // No need to warn about expiring if already expired
             hasExpiringTsaCert = false;
-        }
+            // If there are already another errors, we just say it expired.
-
+            if (!signerNotExpired || !errors.isEmpty()) {
-        if (hasExpiringCert ||
+                errors.add(rb.getString("The.timestamp.has.expired."));
-                (hasExpiringTsaCert  && expireDate != null) ||
+            } else if (signerNotExpired) {
-                (noTimestamp && expireDate != null) ||
-                (hasExpiredTsaCert && signerNotExpired) ||
-                permsDetected) {
-
-            if (hasExpiredTsaCert && signerNotExpired) {
                 if (expireDate != null) {
                     warnings.add(String.format(
                             rb.getString("The.timestamp.expired.1.but.usable.2"),
@@ -1239,37 +1217,51 @@ public class Main {
                 // Reset the flag so exit code is 0
                 hasExpiredTsaCert = false;
             }
-            if (hasExpiringCert) {
+        }
-                warnings.add(isSigning
+
-                        ? rb.getString("The.signer.certificate.will.expire.within.six.months.")
+        if (hasExpiringCert) {
-                        : rb.getString("This.jar.contains.entries.whose.signer.certificate.will.expire.within.six.months."));
+            warnings.add(isSigning
-            }
+                    ? rb.getString("The.signer.certificate.will.expire.within.six.months.")
-            if (hasExpiringTsaCert && expireDate != null) {
+                    : rb.getString("This.jar.contains.entries.whose.signer.certificate.will.expire.within.six.months."));
-                if (expireDate.after(tsaExpireDate)) {
+        }
-                    warnings.add(String.format(rb.getString(
+
-                            "The.timestamp.will.expire.within.one.year.on.1.but.2"), tsaExpireDate, expireDate));
+        if (hasExpiringTsaCert && expireDate != null) {
-                } else {
+            if (expireDate.after(tsaExpireDate)) {
-                    warnings.add(String.format(rb.getString(
+                warnings.add(String.format(rb.getString(
-                            "The.timestamp.will.expire.within.one.year.on.1"), tsaExpireDate));
+                        "The.timestamp.will.expire.within.one.year.on.1.but.2"), tsaExpireDate, expireDate));
-                }
+            } else {
-            }
+                warnings.add(String.format(rb.getString(
-            if (noTimestamp && expireDate != null) {
+                        "The.timestamp.will.expire.within.one.year.on.1"), tsaExpireDate));
-                if (hasTimestampBlock) {
-                    warnings.add(String.format(isSigning
-                            ? rb.getString("invalid.timestamp.signing")
-                            : rb.getString("bad.timestamp.verifying"), expireDate));
-                } else {
-                    warnings.add(String.format(isSigning
-                            ? rb.getString("no.timestamp.signing")
-                            : rb.getString("no.timestamp.verifying"), expireDate));
-                }
-            }
-            if (permsDetected) {
-                warnings.add(rb.getString("posix.attributes.detected"));
             }
         }
 
+        if (noTimestamp && expireDate != null) {
+            if (hasTimestampBlock) {
+                warnings.add(String.format(isSigning
+                        ? rb.getString("invalid.timestamp.signing")
+                        : rb.getString("bad.timestamp.verifying"), expireDate));
+            } else {
+                warnings.add(String.format(isSigning
+                        ? rb.getString("no.timestamp.signing")
+                        : rb.getString("no.timestamp.verifying"), expireDate));
+            }
+        }
+
+        if (permsDetected) {
+            warnings.add(rb.getString("posix.attributes.detected"));
+        }
+
+        if ((strict) && (!errors.isEmpty())) {
+            result = isSigning
+                    ? rb.getString("jar.signed.with.signer.errors.")
+                    : rb.getString("jar.verified.with.signer.errors.");
+        } else {
+            result = isSigning
+                    ? rb.getString("jar.signed.")
+                    : rb.getString("jar.verified.");
+        }
         System.out.println(result);
+
         if (strict) {
             if (!errors.isEmpty()) {
                 System.out.println();
@@ -1289,6 +1281,7 @@ public class Main {
                 warnings.forEach(System.out::println);
             }
         }
+
         if (!isSigning && (!errors.isEmpty() || !warnings.isEmpty())) {
             if (! (verbose != null && showcerts)) {
                 System.out.println();

test/jdk/sun/security/tools/jarsigner/TimestampCheck.java

@@ -57,7 +57,7 @@ import sun.security.timestamp.TimestampToken;
 
 /*
  * @test
- * @bug 6543842 6543440 6939248 8009636 8024302 8163304 8169911 8180289 8172404
+ * @bug 6543842 6543440 6939248 8009636 8024302 8163304 8169911 8180289 8172404 8247960
  * @summary checking response of timestamp
  * @modules java.base/sun.security.pkcs
  *          java.base/sun.security.timestamp
@@ -293,23 +293,27 @@ public class TimestampCheck {
                 signVerbose(null, "unsigned.jar", "sha1alg.jar", "signer",
                         "-strict", "-digestalg", "SHA-1")
                         .shouldHaveExitValue(0)
-                        .shouldContain("jar signed, with signer errors")
+                        .shouldContain("jar signed")
+                        .shouldNotContain("with signer errors")
                         .shouldMatch("SHA-1.*-digestalg.*will be disabled");
                 verify("sha1alg.jar", "-strict")
                         .shouldHaveExitValue(0)
-                        .shouldContain("jar verified, with signer errors")
+                        .shouldContain("jar verified")
+                        .shouldNotContain("with signer errors")
                         .shouldContain("SHA-1 digest algorithm is considered a security risk")
                         .shouldContain("This algorithm will be disabled in a future update")
                         .shouldNotContain("is disabled");
 
                 sign("sha1tsaalg", "-tsadigestalg", "SHA-1", "-strict")
                         .shouldHaveExitValue(0)
-                        .shouldContain("jar signed, with signer errors")
+                        .shouldContain("jar signed")
+                        .shouldNotContain("with signer errors")
                         .shouldMatch("SHA-1.*-tsadigestalg.*will be disabled")
                         .shouldNotContain("is disabled");
                 verify("sha1tsaalg.jar", "-strict")
                         .shouldHaveExitValue(0)
-                        .shouldContain("jar verified, with signer errors")
+                        .shouldContain("jar verified")
+                        .shouldNotContain("with signer errors")
                         .shouldContain("SHA-1 timestamp digest algorithm is considered a security risk")
                         .shouldNotContain("is disabled");