# HG changeset patch
# User weijun
# Date 1511203449 0
# Node ID 57e2285a0e8b74c3c0db0ab8a4855cb9cb7d357a
# Parent  d2ce6ebdf7fbea99759d631293668b53ef486667
8177569: keytool should not warn if signature algorithm used in cacerts is weak
Reviewed-by: mullan

diff -r d2ce6ebdf7fb -r 57e2285a0e8b src/share/classes/sun/security/tools/keytool/Main.java
--- a/src/share/classes/sun/security/tools/keytool/Main.java	Mon Nov 20 17:44:05 2017 +0000
+++ b/src/share/classes/sun/security/tools/keytool/Main.java	Mon Nov 20 18:44:09 2017 +0000
@@ -924,6 +924,13 @@
             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();
         }
@@ -1581,9 +1588,8 @@
         if (keyPass == null) {
             keyPass = promptForKeyPass(alias, null, storePass);
         }
+        checkWeak(rb.getString("the.generated.certificate"), chain[0]);
         keyStore.setKeyEntry(alias, privKey, keyPass, chain);
-
-        checkWeak(rb.getString("the.generated.certificate"), chain[0]);
     }
 
     /**
@@ -1931,11 +1937,11 @@
         }
 
         try {
-            keyStore.setEntry(newAlias, entry, pp);
             Certificate c = srckeystore.getCertificate(alias);
             if (c != null) {
                 checkWeak("<" + newAlias + ">", c);
             }
+            keyStore.setEntry(newAlias, entry, pp);
             return 1;
         } catch (KeyStoreException kse) {
             Object[] source2 = {alias, kse.toString()};
@@ -2612,8 +2618,8 @@
         }
 
         if (noprompt) {
+            checkWeak(rb.getString("the.input"), cert);
             keyStore.setCertificateEntry(alias, cert);
-            checkWeak(rb.getString("the.input"), cert);
             return true;
         }
 
@@ -2864,6 +2870,11 @@
         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),
@@ -2872,7 +2883,7 @@
                         getCertFingerPrint("MD5", cert),
                         getCertFingerPrint("SHA1", cert),
                         getCertFingerPrint("SHA-256", cert),
-                        withWeak(cert.getSigAlgName()),
+                        sigName,
                         withWeak(pkey),
                         cert.getVersion()
                         };
@@ -2946,7 +2957,7 @@
      * 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);
         }
@@ -3298,9 +3309,9 @@
         // 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) {
@@ -4106,9 +4117,19 @@
         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));
         }
@@ -4121,7 +4142,8 @@
         }
     }
 
-    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) {
@@ -4130,15 +4152,18 @@
                 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());
         }
     }
 
diff -r d2ce6ebdf7fb -r 57e2285a0e8b test/sun/security/tools/keytool/WeakAlg.java
--- a/test/sun/security/tools/keytool/WeakAlg.java	Mon Nov 20 17:44:05 2017 +0000
+++ b/test/sun/security/tools/keytool/WeakAlg.java	Mon Nov 20 18:44:09 2017 +0000
@@ -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
@@ -38,6 +38,8 @@
 
 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;
@@ -53,6 +55,10 @@
 
 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");
@@ -74,7 +80,8 @@
                 .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);
@@ -180,7 +187,7 @@
                 .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) {
@@ -204,12 +211,40 @@
             }
         }
         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 @@
         // 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", "");
         gencert("b-c", "");