8182879: Add warnings to keytool when using JKS and JCEKS

Reviewed-by: mullan, weijun

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

@@ -26,6 +26,8 @@
 package sun.security.tools.keytool;
 
 import java.io.*;
+import java.nio.file.Files;
+import java.nio.file.Paths;
 import java.security.CodeSigner;
 import java.security.CryptoPrimitive;
 import java.security.KeyStore;
@@ -164,7 +166,12 @@ public final class Main {
     private List<String> ids = new ArrayList<>();   // used in GENCRL
     private List<String> v3ext = new ArrayList<>();
 
-    // Warnings on weak algorithms
+    // In-place importkeystore is special.
+    // A backup is needed, and no need to prompt for deststorepass.
+    private boolean inplaceImport = false;
+    private String inplaceBackupName = null;
+
+    // Warnings on weak algorithms etc
     private List<String> weakWarnings = new ArrayList<>();
 
     private static final DisabledAlgorithmConstraints DISABLED_CHECK =
@@ -727,18 +734,34 @@ public final class Main {
                 ("New.password.must.be.at.least.6.characters"));
         }
 
+        // Set this before inplaceImport check so we can compare name.
+        if (ksfname == null) {
+            ksfname = System.getProperty("user.home") + File.separator
+                    + ".keystore";
+        }
+
+        KeyStore srcKeyStore = null;
+        if (command == IMPORTKEYSTORE) {
+            inplaceImport = inplaceImportCheck();
+            if (inplaceImport) {
+                // We load srckeystore first so we have srcstorePass that
+                // can be assigned to storePass
+                srcKeyStore = loadSourceKeyStore();
+                if (storePass == null) {
+                    storePass = srcstorePass;
+                }
+            }
+        }
+
         // Check if keystore exists.
         // If no keystore has been specified at the command line, try to use
         // the default, which is located in $HOME/.keystore.
         // If the command is "genkey", "identitydb", "import", or "printcert",
         // it is OK not to have a keystore.
-        if (isKeyStoreRelated(command)) {
-            if (ksfname == null) {
-                ksfname = System.getProperty("user.home") + File.separator
-                    + ".keystore";
-            }
 
-            if (!nullStream) {
+        // DO NOT open the existing keystore if this is an in-place import.
+        // The keystore should be created as brand new.
+        if (isKeyStoreRelated(command) && !nullStream && !inplaceImport) {
                 try {
                     ksfile = new File(ksfname);
                     // Check if keystore file is empty
@@ -760,7 +783,6 @@ public final class Main {
                     }
                 }
             }
-        }
 
         if ((command == KEYCLONE || command == CHANGEALIAS)
                 && dest == null) {
@@ -806,7 +828,11 @@ public final class Main {
          * Null stream keystores are loaded later.
          */
         if (!nullStream) {
+            if (inplaceImport) {
+                keyStore.load(null, storePass);
+            } else {
             keyStore.load(ksStream, storePass);
+            }
             if (ksStream != null) {
                 ksStream.close();
             }
@@ -1042,7 +1068,11 @@ public final class Main {
                 }
             }
         } else if (command == IMPORTKEYSTORE) {
-            doImportKeyStore();
+            // When not in-place import, srcKeyStore is not loaded yet.
+            if (srcKeyStore == null) {
+                srcKeyStore = loadSourceKeyStore();
+            }
+            doImportKeyStore(srcKeyStore);
             kssave = true;
         } else if (command == KEYCLONE) {
             keyPassNew = newPass;
@@ -1173,6 +1203,65 @@ public final class Main {
                 }
             }
         }
+
+        if (isKeyStoreRelated(command)
+                && !token && !nullStream && ksfname != null) {
+
+            // JKS storetype warning on the final result keystore
+            File f = new File(ksfname);
+            if (f.exists()) {
+                // Read the first 4 bytes to determine
+                // if we're dealing with JKS/JCEKS type store
+                String realType = keyStoreType(f);
+                if (realType.equalsIgnoreCase("JKS")
+                    || realType.equalsIgnoreCase("JCEKS")) {
+                    boolean allCerts = true;
+                    for (String a : Collections.list(keyStore.aliases())) {
+                        if (!keyStore.entryInstanceOf(
+                                a, TrustedCertificateEntry.class)) {
+                            allCerts = false;
+                            break;
+                        }
+                    }
+                    // Don't warn for "cacerts" style keystore.
+                    if (!allCerts) {
+                        weakWarnings.add(String.format(
+                                rb.getString("jks.storetype.warning"),
+                                realType, ksfname));
+                    }
+                }
+                if (inplaceImport) {
+                    String realSourceStoreType =
+                        keyStoreType(new File(inplaceBackupName));
+                    String format =
+                            realType.equalsIgnoreCase(realSourceStoreType) ?
+                            rb.getString("backup.keystore.warning") :
+                            rb.getString("migrate.keystore.warning");
+                    weakWarnings.add(
+                            String.format(format,
+                                    srcksfname,
+                                    realSourceStoreType,
+                                    inplaceBackupName,
+                                    realType));
+                }
+            }
+        }
+    }
+
+    private String keyStoreType(File f) throws IOException {
+        int MAGIC = 0xfeedfeed;
+        int JCEKS_MAGIC = 0xcececece;
+        try (DataInputStream dis = new DataInputStream(
+            new FileInputStream(f))) {
+            int xMagic = dis.readInt();
+            if (xMagic == MAGIC) {
+                return "JKS";
+            } else if (xMagic == JCEKS_MAGIC) {
+                return "JCEKS";
+            } else {
+                return "Non JKS/JCEKS";
+            }
+        }
     }
 
     /**
@@ -1871,14 +1960,43 @@ public final class Main {
         }
     }
 
+    boolean inplaceImportCheck() throws Exception {
+        if (P11KEYSTORE.equalsIgnoreCase(srcstoretype) ||
+                KeyStoreUtil.isWindowsKeyStore(srcstoretype)) {
+            return false;
+        }
+
+        if (srcksfname != null) {
+            File srcksfile = new File(srcksfname);
+            if (srcksfile.exists() && srcksfile.length() == 0) {
+                throw new Exception(rb.getString
+                        ("Source.keystore.file.exists.but.is.empty.") +
+                        srcksfname);
+            }
+            if (srcksfile.getCanonicalFile()
+                    .equals(new File(ksfname).getCanonicalFile())) {
+                return true;
+            } else {
+                // Informational, especially if destkeystore is not
+                // provided, which default to ~/.keystore.
+                System.err.println(String.format(rb.getString(
+                        "importing.keystore.status"), srcksfname, ksfname));
+                return false;
+            }
+        } else {
+            throw new Exception(rb.getString
+                    ("Please.specify.srckeystore"));
+        }
+    }
+
     /**
      * Load the srckeystore from a stream, used in -importkeystore
      * @returns the src KeyStore
      */
     KeyStore loadSourceKeyStore() throws Exception {
-        boolean isPkcs11 = false;
 
         InputStream is = null;
+        File srcksfile = null;
 
         if (P11KEYSTORE.equalsIgnoreCase(srcstoretype) ||
                 KeyStoreUtil.isWindowsKeyStore(srcstoretype)) {
@@ -1888,20 +2006,9 @@ public final class Main {
                 System.err.println();
                 tinyHelp();
             }
-            isPkcs11 = true;
         } else {
-            if (srcksfname != null) {
-                File srcksfile = new File(srcksfname);
-                    if (srcksfile.exists() && srcksfile.length() == 0) {
-                        throw new Exception(rb.getString
-                                ("Source.keystore.file.exists.but.is.empty.") +
-                                srcksfname);
-                }
+            srcksfile = new File(srcksfname);
                 is = new FileInputStream(srcksfile);
-            } else {
-                throw new Exception(rb.getString
-                        ("Please.specify.srckeystore"));
-            }
         }
 
         KeyStore store;
@@ -1962,17 +2069,32 @@ public final class Main {
      * keep alias unchanged if no name conflict, otherwise, prompt.
      * keep keypass unchanged for keys
      */
-    private void doImportKeyStore() throws Exception {
+    private void doImportKeyStore(KeyStore srcKS) throws Exception {
 
         if (alias != null) {
-            doImportKeyStoreSingle(loadSourceKeyStore(), alias);
+            doImportKeyStoreSingle(srcKS, alias);
         } else {
             if (dest != null || srckeyPass != null) {
                 throw new Exception(rb.getString(
                         "if.alias.not.specified.destalias.and.srckeypass.must.not.be.specified"));
             }
-            doImportKeyStoreAll(loadSourceKeyStore());
+            doImportKeyStoreAll(srcKS);
+        }
+
+        if (inplaceImport) {
+            // Backup to file.old or file.old2...
+            // The keystore is not rewritten yet now.
+            for (int n = 1; /* forever */; n++) {
+                inplaceBackupName = srcksfname + ".old" + (n == 1 ? "" : n);
+                File bkFile = new File(inplaceBackupName);
+                if (!bkFile.exists()) {
+                    Files.copy(Paths.get(srcksfname), bkFile.toPath());
+                    break;
+                }
+            }
+
         }
+
         /*
          * Information display rule of -importkeystore
          * 1. inside single, shows failure

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

@@ -453,6 +453,10 @@ public class Resources extends java.util.ListResourceBundle {
         {"verified.by.s.in.s.weak", "Verified by %s in %s with a %s"},
         {"whose.sigalg.risk", "%s uses the %s signature algorithm which is considered a security risk."},
         {"whose.key.risk", "%s uses a %s which is considered a security risk."},
+        {"jks.storetype.warning", "The %1$s keystore uses a proprietary format. It is recommended to migrate to PKCS12 which is an industry standard format using \"keytool -importkeystore -srckeystore %2$s -destkeystore %2$s -deststoretype pkcs12\"."},
+        {"migrate.keystore.warning", "Migrated \"%1$s\" to %4$s. The %2$s keystore is backed up as \"%3$s\"."},
+        {"backup.keystore.warning", "The original keystore \"%1$s\" is backed up as \"%3$s\"..."},
+        {"importing.keystore.status", "Importing keystore %1$s to %2$s..."},
     };
 
 

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

@@ -23,13 +23,14 @@
 
 /*
  * @test
- * @bug 8171319 8177569
+ * @bug 8171319 8177569 8182879
  * @summary keytool should print out warnings when reading or generating
   *         cert/cert req using weak algorithms
  * @library /lib/testlibrary
  * @run main/othervm/timeout=600 -Duser.language=en -Duser.country=US WeakAlg
  */
 
+import jdk.testlibrary.Asserts;
 import jdk.testlibrary.SecurityTools;
 import jdk.testlibrary.OutputAnalyzer;
 import sun.security.tools.KeyStoreUtil;
@@ -137,25 +138,161 @@ public class WeakAlg {
         kt("-delete -alias b");
         kt("-printcrl -file b.crl")
                 .shouldContain("WARNING: not verified");
+
+        jksTypeCheck();
+
+        checkInplaceImportKeyStore();
     }
 
-    static void checkImportKeyStore() throws Exception {
+    static void jksTypeCheck() throws Exception {
 
-        saveStore();
+        rm("ks");
+        rm("ks2");
+
+        kt("-genkeypair -alias a -storetype pkcs12 -dname CN=A")
+                .shouldNotContain("Warning:");
+        kt("-list")
+                .shouldNotContain("Warning:");
+        kt("-list -storetype jks") // no warning if PKCS12 used as JKS
+                .shouldNotContain("Warning:");
+        kt("-exportcert -alias a -file a.crt")
+                .shouldNotContain("Warning:");
+
+        // warn if migrating to JKS
+        importkeystore("ks", "ks2", "-deststoretype jks")
+                .shouldContain("JKS keystore uses a proprietary format");
+
+        rm("ks");
+        rm("ks2");
+        rm("ks3");
+
+        // no warning if all certs
+        kt("-importcert -alias b -file a.crt -storetype jks -noprompt")
+                .shouldNotContain("Warning:");
+        kt("-genkeypair -alias a -dname CN=A")
+                .shouldContain("JKS keystore uses a proprietary format");
+        kt("-list")
+                .shouldContain("JKS keystore uses a proprietary format");
+        kt("-exportcert -alias a -file a.crt")
+                .shouldContain("JKS keystore uses a proprietary format");
+        kt("-printcert -file a.crt") // no warning if keystore not touched
+                .shouldNotContain("Warning:");
+        kt("-certreq -alias a -file a.req")
+               .shouldContain("JKS keystore uses a proprietary format");
+        kt("-printcertreq -file a.req") // no warning if keystore not touched
+                .shouldNotContain("Warning:");
+
+        // Earlier than JDK 9 defaults to JKS
+        importkeystore("ks", "ks2", "")
+                .shouldContain("Warning:");
+
+        importkeystore("ks", "ks3", "-deststoretype pkcs12")
+                .shouldNotContain("Warning:");
 
         rm("ks");
-        kt("-importkeystore -srckeystore ks2 -srcstorepass changeit")
+
+        kt("-genkeypair -alias a -dname CN=A -storetype jceks")
+                .shouldContain("JCEKS keystore uses a proprietary format");
+        kt("-list -storetype jceks")
+                .shouldContain("JCEKS keystore uses a proprietary format");
+        kt("-importcert -alias b -file a.crt -noprompt -storetype jceks")
+                .shouldContain("JCEKS keystore uses a proprietary format");
+        kt("-exportcert -alias a -file a.crt -storetype jceks")
+                .shouldContain("JCEKS keystore uses a proprietary format");
+        kt("-printcert -file a.crt")
+                .shouldNotContain("Warning:");
+        kt("-certreq -alias a -file a.req -storetype jceks")
+                .shouldContain("JCEKS keystore uses a proprietary format");
+        kt("-printcertreq -file a.req")
+                .shouldNotContain("Warning:");
+        kt("-genseckey -alias c -keyalg AES -keysize 128 -storetype jceks")
+                .shouldContain("JCEKS keystore uses a proprietary format");
+    }
+
+    static void checkImportKeyStore() throws Exception {
+
+        rm("ks2");
+        rm("ks3");
+
+        importkeystore("ks", "ks2", "")
                 .shouldContain("3 entries successfully imported")
                 .shouldContain("Warning")
                 .shouldMatch("<b>.*512-bit RSA key.*risk")
                 .shouldMatch("<a>.*MD5withRSA.*risk");
 
-        rm("ks");
-        kt("-importkeystore -srckeystore ks2 -srcstorepass changeit -srcalias a")
+        importkeystore("ks", "ks3", "-srcalias a")
                 .shouldContain("Warning")
                 .shouldMatch("<a>.*MD5withRSA.*risk");
+    }
 
-        reStore();
+    static void checkInplaceImportKeyStore() throws Exception {
+
+        rm("ks");
+        genkeypair("a", "");
+
+        // Same type backup
+        importkeystore("ks", "ks", "")
+                .shouldContain("Warning:")
+                .shouldMatch(".*ks.old");
+
+        importkeystore("ks", "ks", "")
+                .shouldContain("Warning:")
+                .shouldMatch(".*ks.old2");
+
+        importkeystore("ks", "ks", "-srcstoretype jks") // it knows real type
+                .shouldContain("Warning:")
+                .shouldMatch(".*ks.old3");
+
+        String cPath = new File("ks").getCanonicalPath();
+
+        importkeystore("ks", cPath, "")
+                .shouldContain("Warning:")
+                .shouldMatch(".*ks.old4");
+
+        // Migration
+        importkeystore("ks", "ks", "-deststoretype jks")
+                .shouldContain("Warning:")
+                .shouldContain("JKS keystore uses a proprietary format")
+                .shouldMatch("The original.*ks.old5");
+
+        KeyStore test_ks = KeyStore.getInstance("JKS");
+        test_ks.load(new FileInputStream(new File("ks")),
+                "changeit".toCharArray());
+        Asserts.assertEQ(
+                    test_ks.getType(), "JKS");
+
+        importkeystore("ks", "ks", "-deststoretype PKCS12")
+                .shouldContain("Warning:")
+                .shouldNotContain("proprietary format")
+                .shouldMatch("Migrated.*Non.*JKS.*ks.old6");
+
+        test_ks = KeyStore.getInstance("PKCS12");
+        test_ks.load(new FileInputStream(new File("ks")),
+                "changeit".toCharArray());
+        Asserts.assertEQ(
+                test_ks.getType(), "PKCS12");
+
+        test_ks = KeyStore.getInstance("JKS");
+        test_ks.load(new FileInputStream(new File("ks.old6")),
+                "changeit".toCharArray());
+        Asserts.assertEQ(
+                test_ks.getType(), "JKS");
+
+        // One password prompt is enough for migration
+        kt0("-importkeystore -srckeystore ks -destkeystore ks", "changeit")
+                .shouldMatch("backed.*ks.old7");
+
+        // But three if importing to a different keystore
+        rm("ks2");
+        kt0("-importkeystore -srckeystore ks -destkeystore ks2",
+                    "changeit")
+                .shouldContain("Keystore password is too short");
+
+        kt0("-importkeystore -srckeystore ks -destkeystore ks2",
+                "changeit", "changeit", "changeit")
+                .shouldContain("Importing keystore ks to ks2...")
+                .shouldNotContain("original")
+                .shouldNotContain("Migrated");
     }
 
     static void checkImport() throws Exception {
@@ -521,17 +658,22 @@ public class WeakAlg {
         }
     }
 
-    // Fast keytool execution by directly calling its main() method
     static OutputAnalyzer kt(String cmd, String... input) {
+        return kt0("-keystore ks -storepass changeit " +
+                "-keypass changeit " + cmd, input);
+    }
+
+    // Fast keytool execution by directly calling its main() method
+    static OutputAnalyzer kt0(String cmd, String... input) {
         PrintStream out = System.out;
         PrintStream err = System.err;
         InputStream ins = System.in;
         ByteArrayOutputStream bout = new ByteArrayOutputStream();
         ByteArrayOutputStream berr = new ByteArrayOutputStream();
         boolean succeed = true;
+        String sout;
+        String serr;
         try {
-            cmd = "-keystore ks -storepass changeit " +
-                    "-keypass changeit " + cmd;
             System.out.println("---------------------------------------------");
             System.out.println("$ keytool " + cmd);
             System.out.println();
@@ -555,19 +697,26 @@ public class WeakAlg {
             System.setOut(out);
             System.setErr(err);
             System.setIn(ins);
-        }
-        String sout = new String(bout.toByteArray());
-        String serr = new String(berr.toByteArray());
+            sout = new String(bout.toByteArray());
+            serr = new String(berr.toByteArray());
             System.out.println("STDOUT:\n" + sout + "\nSTDERR:\n" + serr);
+        }
         if (!succeed) {
             throw new RuntimeException();
         }
         return new OutputAnalyzer(sout, serr);
     }
 
+    static OutputAnalyzer importkeystore(String src, String dest,
+                                         String options) {
+        return kt0("-importkeystore "
+                + "-srckeystore " + src + " -destkeystore " + dest
+                + " -srcstorepass changeit -deststorepass changeit " + options);
+    }
+
     static OutputAnalyzer genkeypair(String alias, String options) {
         return kt("-genkeypair -alias " + alias + " -dname CN=" + alias
-                + " -keyalg RSA -storetype JKS " + options);
+                + " -keyalg RSA -storetype PKCS12 " + options);
     }
 
     static OutputAnalyzer certreq(String alias, String options) {

test/sun/security/tools/keytool/keyalg.sh

@@ -31,6 +31,8 @@ if [ "${TESTJAVA}" = "" ] ; then
   TESTJAVA=`dirname $JAVAC_CMD`/..
 fi
 
+TESTTOOLVMOPTS="$TESTTOOLVMOPTS -J-Duser.language=en -J-Duser.country=US"
+
 KS=ks
 KEYTOOL="$TESTJAVA/bin/keytool ${TESTTOOLVMOPTS} -keystore ks -storepass changeit -keypass changeit"