changeset 17321:e884b64ff38e jdk-9.0.3+5

8182879: Add warnings to keytool when using JKS and JCEKS Reviewed-by: vinnie, ahgross, mullan
author weijun
date Wed, 12 Jul 2017 10:55:40 +0800
parents 922f685000b0
children 7b182ade7b8f
files src/java.base/share/classes/sun/security/tools/keytool/Main.java src/java.base/share/classes/sun/security/tools/keytool/Resources.java test/sun/security/tools/keytool/WeakAlg.java
diffstat 3 files changed, 320 insertions(+), 56 deletions(-) [+]
line wrap: on
line diff
--- a/src/java.base/share/classes/sun/security/tools/keytool/Main.java	Thu Aug 03 08:30:03 2017 -0700
+++ b/src/java.base/share/classes/sun/security/tools/keytool/Main.java	Wed Jul 12 10:55:40 2017 +0800
@@ -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;
@@ -168,7 +170,12 @@
     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 =
@@ -812,37 +819,52 @@
                 ("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) {
-                try {
-                    ksfile = new File(ksfname);
-                    // Check if keystore file is empty
-                    if (ksfile.exists() && ksfile.length() == 0) {
-                        throw new Exception(rb.getString
-                        ("Keystore.file.exists.but.is.empty.") + ksfname);
-                    }
-                    ksStream = new FileInputStream(ksfile);
-                } catch (FileNotFoundException e) {
-                    if (command != GENKEYPAIR &&
+
+        // 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
+                if (ksfile.exists() && ksfile.length() == 0) {
+                    throw new Exception(rb.getString
+                            ("Keystore.file.exists.but.is.empty.") + ksfname);
+                }
+                ksStream = new FileInputStream(ksfile);
+            } catch (FileNotFoundException e) {
+                if (command != GENKEYPAIR &&
                         command != GENSECKEY &&
                         command != IDENTITYDB &&
                         command != IMPORTCERT &&
                         command != IMPORTPASS &&
                         command != IMPORTKEYSTORE &&
                         command != PRINTCRL) {
-                        throw new Exception(rb.getString
-                                ("Keystore.file.does.not.exist.") + ksfname);
-                    }
+                    throw new Exception(rb.getString
+                            ("Keystore.file.does.not.exist.") + ksfname);
                 }
             }
         }
@@ -866,7 +888,7 @@
         // Create new keystore
         // Probe for keystore type when filename is available
         if (ksfile != null && ksStream != null && providerName == null &&
-            hasStoretypeOption == false) {
+                hasStoretypeOption == false && !inplaceImport) {
             keyStore = KeyStore.getInstance(ksfile, storePass);
         } else {
             if (providerName == null) {
@@ -896,7 +918,11 @@
              * Null stream keystores are loaded later.
              */
             if (!nullStream) {
-                keyStore.load(ksStream, storePass);
+                if (inplaceImport) {
+                    keyStore.load(null, storePass);
+                } else {
+                    keyStore.load(ksStream, storePass);
+                }
                 if (ksStream != null) {
                     ksStream.close();
                 }
@@ -1133,7 +1159,11 @@
                 }
             }
         } 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;
@@ -1264,6 +1294,51 @@
                 }
             }
         }
+
+        if (isKeyStoreRelated(command)
+                && !token && !nullStream && ksfname != null) {
+
+            // JKS storetype warning on the final result keystore
+            File f = new File(ksfname);
+            char[] pass = (storePassNew!=null) ? storePassNew : storePass;
+            if (f.exists()) {
+                // Probe for real type. A JKS can be loaded as PKCS12 because
+                // DualFormat support, vice versa.
+                keyStore = KeyStore.getInstance(f, pass);
+                String realType = keyStore.getType();
+                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 = KeyStore.getInstance(
+                            new File(inplaceBackupName), srcstorePass).getType();
+                    String format =
+                            realType.equalsIgnoreCase(realSourceStoreType) ?
+                            rb.getString("backup.keystore.warning") :
+                            rb.getString("migrate.keystore.warning");
+                    weakWarnings.add(
+                            String.format(format,
+                                    srcksfname,
+                                    realSourceStoreType,
+                                    inplaceBackupName,
+                                    realType));
+                }
+            }
+        }
     }
 
     /**
@@ -1955,12 +2030,40 @@
         }
     }
 
+    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
      * @return the src KeyStore
      */
     KeyStore loadSourceKeyStore() throws Exception {
-        boolean isPkcs11 = false;
 
         InputStream is = null;
         File srcksfile = null;
@@ -1973,20 +2076,9 @@
                 System.err.println();
                 tinyHelp();
             }
-            isPkcs11 = true;
         } else {
-            if (srcksfname != null) {
-                srcksfile = new File(srcksfname);
-                    if (srcksfile.exists() && srcksfile.length() == 0) {
-                        throw new Exception(rb.getString
-                                ("Source.keystore.file.exists.but.is.empty.") +
-                                srcksfname);
-                }
-                is = new FileInputStream(srcksfile);
-            } else {
-                throw new Exception(rb.getString
-                        ("Please.specify.srckeystore"));
-            }
+            srcksfile = new File(srcksfname);
+            is = new FileInputStream(srcksfile);
         }
 
         KeyStore store;
@@ -2053,17 +2145,32 @@
      * 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
--- a/src/java.base/share/classes/sun/security/tools/keytool/Resources.java	Thu Aug 03 08:30:03 2017 -0700
+++ b/src/java.base/share/classes/sun/security/tools/keytool/Resources.java	Wed Jul 12 10:55:40 2017 +0800
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2000, 2014, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2000, 2017, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -468,6 +468,10 @@
         {"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..."},
     };
 
 
--- a/test/sun/security/tools/keytool/WeakAlg.java	Thu Aug 03 08:30:03 2017 -0700
+++ b/test/sun/security/tools/keytool/WeakAlg.java	Wed Jul 12 10:55:40 2017 +0800
@@ -23,7 +23,7 @@
 
 /*
  * @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 /test/lib
@@ -33,6 +33,7 @@
  * @run main/othervm/timeout=600 -Duser.language=en -Duser.country=US WeakAlg
  */
 
+import jdk.test.lib.Asserts;
 import jdk.test.lib.SecurityTools;
 import jdk.test.lib.process.OutputAnalyzer;
 import sun.security.tools.KeyStoreUtil;
@@ -40,6 +41,7 @@
 
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
+import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.PrintStream;
@@ -134,25 +136,164 @@
         kt("-delete -alias b");
         kt("-printcrl -file b.crl")
                 .shouldContain("WARNING: not verified");
+
+        jksTypeCheck();
+
+        checkInplaceImportKeyStore();
+    }
+
+    static void jksTypeCheck() throws Exception {
+
+        // No warning for cacerts, all certs
+        kt0("-cacerts -list -storepass changeit")
+                .shouldNotContain("Warning:");
+
+        rm("ks");
+        rm("ks2");
+
+        kt("-genkeypair -alias a -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("-list -storetype pkcs12") // warn if JKS used as PKCS12
+                .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:");
+
+        // No warning if migrating from JKS
+        importkeystore("ks", "ks2", "")
+                .shouldNotContain("Warning:");
+
+        importkeystore("ks", "ks3", "-deststoretype pkcs12")
+                .shouldNotContain("Warning:");
+
+        rm("ks");
+
+        kt("-genkeypair -alias a -dname CN=A -storetype jceks")
+                .shouldContain("JCEKS keystore uses a proprietary format");
+        kt("-list")
+                .shouldContain("JCEKS keystore uses a proprietary format");
+        kt("-importcert -alias b -file a.crt -noprompt")
+                .shouldContain("JCEKS keystore uses a proprietary format");
+        kt("-exportcert -alias a -file a.crt")
+                .shouldContain("JCEKS keystore uses a proprietary format");
+        kt("-printcert -file a.crt")
+                .shouldNotContain("Warning:");
+        kt("-certreq -alias a -file a.req")
+                .shouldContain("JCEKS keystore uses a proprietary format");
+        kt("-printcertreq -file a.req")
+                .shouldNotContain("Warning:");
+        kt("-genseckey -alias c -keyalg AES -keysize 128")
+                .shouldContain("JCEKS keystore uses a proprietary format");
     }
 
     static void checkImportKeyStore() throws Exception {
 
-        saveStore();
+        rm("ks2");
+        rm("ks3");
 
-        rm("ks");
-        kt("-importkeystore -srckeystore ks2 -srcstorepass changeit")
+        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("original.*ks.old");
+
+        importkeystore("ks", "ks", "")
+                .shouldContain("Warning:")
+                .shouldMatch("original.*ks.old2");
+
+        importkeystore("ks", "ks", "-srcstoretype jks") // it knows real type
+                .shouldContain("Warning:")
+                .shouldMatch("original.*ks.old3");
+
+        String cPath = new File("ks").getCanonicalPath();
+
+        importkeystore("ks", cPath, "")
+                .shouldContain("Warning:")
+                .shouldMatch("original.*ks.old4");
+
+        // Migration
+        importkeystore("ks", "ks", "-deststoretype jks")
+                .shouldContain("Warning:")
+                .shouldContain("JKS keystore uses a proprietary format")
+                .shouldMatch("Migrated.*JKS.*PKCS12.*ks.old5");
+
+        Asserts.assertEQ(
+                KeyStore.getInstance(
+                        new File("ks"), "changeit".toCharArray()).getType(),
+                "JKS");
+
+        importkeystore("ks", "ks", "-srcstoretype PKCS12")
+                .shouldContain("Warning:")
+                .shouldNotContain("proprietary format")
+                .shouldMatch("Migrated.*PKCS12.*JKS.*ks.old6");
+
+        Asserts.assertEQ(
+                KeyStore.getInstance(
+                        new File("ks"), "changeit".toCharArray()).getType(),
+                "PKCS12");
+
+        Asserts.assertEQ(
+                KeyStore.getInstance(
+                        new File("ks.old6"), "changeit".toCharArray()).getType(),
+                "JKS");
+
+        // One password prompt is enough for migration
+        kt0("-importkeystore -srckeystore ks -destkeystore ks", "changeit")
+                .shouldMatch("original.*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 {
@@ -518,17 +659,22 @@
         }
     }
 
+    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 kt(String cmd, String... input) {
+    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();
@@ -552,19 +698,26 @@
             System.setOut(out);
             System.setErr(err);
             System.setIn(ins);
+            sout = new String(bout.toByteArray());
+            serr = new String(berr.toByteArray());
+            System.out.println("STDOUT:\n" + sout + "\nSTDERR:\n" + serr);
         }
-        String sout = new String(bout.toByteArray());
-        String 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) {