8182879: Add warnings to keytool when using JKS and JCEKS
Reviewed-by: vinnie, ahgross, mullan
--- a/src/java.base/share/classes/sun/security/tools/keytool/Main.java Thu Jul 06 09:43:27 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 =
@@ -846,37 +853,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);
}
}
}
@@ -900,7 +922,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) {
@@ -930,7 +952,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();
}
@@ -1167,7 +1193,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;
@@ -1298,6 +1328,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));
+ }
+ }
+ }
}
/**
@@ -1989,12 +2064,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;
@@ -2007,20 +2110,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;
@@ -2087,17 +2179,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 Jul 06 09:43:27 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
@@ -471,6 +471,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/jdk/sun/security/tools/keytool/WeakAlg.java Thu Jul 06 09:43:27 2017 -0700
+++ b/test/jdk/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
@@ -40,6 +40,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;
@@ -47,6 +48,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;
@@ -141,25 +143,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 {
@@ -525,17 +666,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();
@@ -559,19 +705,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
- + " -storetype JKS " + options);
+ + " -storetype PKCS12 " + options);
}
static OutputAnalyzer certreq(String alias, String options) {