8177569: keytool should not warn if signature algorithm used in cacerts is weak

Reviewed-by: mullan

src/share/classes/sun/security/tools/keytool/Main.java

@@ -934,6 +934,13 @@ public final class Main {
             cf = CertificateFactory.getInstance("X509");
         }
 
+        // -trustcacerts can only be specified on -importcert.
+        // Reset it so that warnings on CA cert will remain for
+        // -printcert, etc.
+        if (command != IMPORTCERT) {
+            trustcacerts = false;
+        }
+
         if (trustcacerts) {
             caks = KeyStoreUtil.getCacertsKeyStore();
         }
@@ -1674,9 +1681,8 @@ public final class Main {
         if (keyPass == null) {
             keyPass = promptForKeyPass(alias, null, storePass);
         }
-        keyStore.setKeyEntry(alias, privKey, keyPass, chain);
-
         checkWeak(rb.getString("the.generated.certificate"), chain[0]);
+        keyStore.setKeyEntry(alias, privKey, keyPass, chain);
     }
 
     /**
@@ -2027,6 +2033,10 @@ public final class Main {
         }
 
         try {
+            Certificate c = srckeystore.getCertificate(alias);
+            if (c != null) {
+                checkWeak("<" + newAlias + ">", c);
+            }
             keyStore.setEntry(newAlias, entry, pp);
             // Place the check so that only successful imports are blocked.
             // For example, we don't block a failed SecretEntry import.
@@ -2036,10 +2046,6 @@ public final class Main {
                             "The.destination.pkcs12.keystore.has.different.storepass.and.keypass.Please.retry.with.destkeypass.specified."));
                 }
             }
-            Certificate c = srckeystore.getCertificate(alias);
-            if (c != null) {
-                checkWeak("<" + newAlias + ">", c);
-            }
             return 1;
         } catch (KeyStoreException kse) {
             Object[] source2 = {alias, kse.toString()};
@@ -2725,8 +2731,8 @@ public final class Main {
         }
 
         if (noprompt) {
-            keyStore.setCertificateEntry(alias, cert);
             checkWeak(rb.getString("the.input"), cert);
+            keyStore.setCertificateEntry(alias, cert);
             return true;
         }
 
@@ -2977,6 +2983,11 @@ public final class Main {
         MessageFormat form = new MessageFormat
                 (rb.getString(".PATTERN.printX509Cert.with.weak"));
         PublicKey pkey = cert.getPublicKey();
+        String sigName = cert.getSigAlgName();
+        // No need to warn about sigalg of a trust anchor
+        if (!isTrustedCert(cert)) {
+            sigName = withWeak(sigName);
+        }
         Object[] source = {cert.getSubjectDN().toString(),
                         cert.getIssuerDN().toString(),
                         cert.getSerialNumber().toString(16),
@@ -2985,7 +2996,7 @@ public final class Main {
                         getCertFingerPrint("MD5", cert),
                         getCertFingerPrint("SHA1", cert),
                         getCertFingerPrint("SHA-256", cert),
-                        withWeak(cert.getSigAlgName()),
+                        sigName,
                         withWeak(pkey),
                         cert.getVersion()
                         };
@@ -3059,7 +3070,7 @@ public final class Main {
      * or null otherwise. A label is added.
      */
     private static Pair<String,Certificate>
-            getTrustedSigner(Certificate cert, KeyStore ks) throws Exception {
+            getSigner(Certificate cert, KeyStore ks) throws Exception {
         if (ks.getCertificateAlias(cert) != null) {
             return new Pair<>("", cert);
         }
@@ -3410,9 +3421,9 @@ public final class Main {
         // do we trust the cert at the top?
         Certificate topCert = replyCerts[replyCerts.length-1];
         boolean fromKeyStore = true;
-        Pair<String,Certificate> root = getTrustedSigner(topCert, keyStore);
+        Pair<String,Certificate> root = getSigner(topCert, keyStore);
         if (root == null && trustcacerts && caks != null) {
-            root = getTrustedSigner(topCert, caks);
+            root = getSigner(topCert, caks);
             fromKeyStore = false;
         }
         if (root == null) {
@@ -4218,9 +4229,19 @@ public final class Main {
         return ext;
     }
 
+    private boolean isTrustedCert(Certificate cert) throws KeyStoreException {
+        if (caks != null && caks.getCertificateAlias(cert) != null) {
+            return true;
+        } else {
+            String inKS = keyStore.getCertificateAlias(cert);
+            return inKS != null && keyStore.isCertificateEntry(inKS);
+        }
+    }
+
     private void checkWeak(String label, String sigAlg, Key key) {
 
-        if (!DISABLED_CHECK.permits(SIG_PRIMITIVE_SET, sigAlg, null)) {
+        if (sigAlg != null && !DISABLED_CHECK.permits(
+                SIG_PRIMITIVE_SET, sigAlg, null)) {
             weakWarnings.add(String.format(
                     rb.getString("whose.sigalg.risk"), label, sigAlg));
         }
@@ -4233,7 +4254,8 @@ public final class Main {
         }
     }
 
-    private void checkWeak(String label, Certificate[] certs) {
+    private void checkWeak(String label, Certificate[] certs)
+            throws KeyStoreException {
         for (int i = 0; i < certs.length; i++) {
             Certificate cert = certs[i];
             if (cert instanceof X509Certificate) {
@@ -4242,15 +4264,18 @@ public final class Main {
                 if (certs.length > 1) {
                     fullLabel = oneInMany(label, i, certs.length);
                 }
-                checkWeak(fullLabel, xc.getSigAlgName(), xc.getPublicKey());
+                checkWeak(fullLabel, xc);
             }
         }
     }
 
-    private void checkWeak(String label, Certificate cert) {
+    private void checkWeak(String label, Certificate cert)
+            throws KeyStoreException {
         if (cert instanceof X509Certificate) {
             X509Certificate xc = (X509Certificate)cert;
-            checkWeak(label, xc.getSigAlgName(), xc.getPublicKey());
+            // No need to check the sigalg of a trust anchor
+            String sigAlg = isTrustedCert(cert) ? null : xc.getSigAlgName();
+            checkWeak(label, sigAlg, xc.getPublicKey());
         }
     }
 

test/sun/security/tools/keytool/WeakAlg.java

@@ -23,7 +23,7 @@
 
 /*
  * @test
- * @bug 8171319
+ * @bug 8171319 8177569
  * @summary keytool should print out warnings when reading or generating
   *         cert/cert req using weak algorithms
  * @library /lib/testlibrary
@@ -37,6 +37,8 @@ import sun.security.util.DisabledAlgorithmConstraints;
 
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.PrintStream;
@@ -54,6 +56,10 @@ import java.util.stream.Stream;
 
 public class WeakAlg {
 
+    static String sep = File.separator;
+    static String cacerts_location = System.getProperty("java.home") +
+        sep + "lib" + sep + "security" + sep + "cacerts";
+
     public static void main(String[] args) throws Throwable {
 
         rm("ks");
@@ -75,7 +81,8 @@ public class WeakAlg {
                 .shouldMatch("<b>.*512-bit RSA key.*risk")
                 .shouldContain("512-bit RSA key (weak)");
 
-        // Multiple warnings for multiple cert in -printcert or -list or -exportcert
+        // Multiple warnings for multiple cert in -printcert
+        // or -list or -exportcert
 
         // -certreq, -printcertreq, -gencert
         checkCertReq("a", "", null);
@@ -181,7 +188,7 @@ public class WeakAlg {
                 .shouldMatch("The input.*MD5withRSA.*risk")
                 .shouldNotContain("[no]");
 
-        // cert is self-signed cacerts
+        // JDK-8177569: no warning for sigalg of trusted cert
         String weakSigAlgCA = null;
         KeyStore ks = KeyStoreUtil.getCacertsKeyStore();
         if (ks != null) {
@@ -205,12 +212,40 @@ public class WeakAlg {
             }
         }
         if (weakSigAlgCA != null) {
+            // The following 2 commands still have a warning on why not using
+            // the -cacerts option directly.
+            kt("-list -keystore " + cacerts_location)
+                    .shouldNotContain("risk");
+            kt("-list -v -keystore " + cacerts_location)
+                    .shouldNotContain("risk");
+
+            // -printcert will always show warnings
+            kt("-printcert -file ca.cert")
+                    .shouldContain("name: " + weakSigAlgCA + " (weak)")
+                    .shouldContain("Warning")
+                    .shouldMatch("The certificate.*" + weakSigAlgCA + ".*risk");
+            kt("-printcert -file ca.cert -trustcacerts") // -trustcacerts useless
+                    .shouldContain("name: " + weakSigAlgCA + " (weak)")
+                    .shouldContain("Warning")
+                    .shouldMatch("The certificate.*" + weakSigAlgCA + ".*risk");
+
+            // Importing with -trustcacerts ignore CA cert's sig alg
             kt("-delete -alias d");
             kt("-importcert -alias d -trustcacerts -file ca.cert", "no")
                     .shouldContain("Certificate already exists in system-wide CA")
+                    .shouldNotContain("risk")
+                    .shouldContain("Do you still want to add it to your own keystore?");
+            kt("-importcert -alias d -trustcacerts -file ca.cert -noprompt")
+                    .shouldNotContain("risk")
+                    .shouldNotContain("[no]");
+
+            // but not without -trustcacerts
+            kt("-delete -alias d");
+            kt("-importcert -alias d -file ca.cert", "no")
+                    .shouldContain("name: " + weakSigAlgCA + " (weak)")
                     .shouldContain("Warning")
                     .shouldMatch("The input.*" + weakSigAlgCA + ".*risk")
-                    .shouldContain("Do you still want to add it to your own keystore?");
+                    .shouldContain("Trust this certificate?");
             kt("-importcert -alias d -file ca.cert -noprompt")
                     .shouldContain("Warning")
                     .shouldMatch("The input.*" + weakSigAlgCA + ".*risk")
@@ -262,6 +297,26 @@ public class WeakAlg {
 
         // install reply
 
+        reStore();
+        certreq("c", "");
+        gencert("a-c", "");
+        kt("-importcert -alias c -file a-c.cert")
+                .shouldContain("Warning")
+                .shouldMatch("Issuer <a>.*MD5withRSA.*risk");
+
+        // JDK-8177569: no warning for sigalg of trusted cert
+        reStore();
+        // Change a into a TrustedCertEntry
+        kt("-exportcert -alias a -file a.cert");
+        kt("-delete -alias a");
+        kt("-importcert -alias a -file a.cert -noprompt");
+        kt("-list -alias a -v")
+                .shouldNotContain("weak")
+                .shouldNotContain("Warning");
+        // This time a is trusted and no warning on its weak sig alg
+        kt("-importcert -alias c -file a-c.cert")
+                .shouldNotContain("Warning");
+
         reStore();
 
         gencert("a-b", "");