8182879: Add warnings to keytool when using JKS and JCEKS
Reviewed-by: mullan, weijun
diff --git a/jdk/src/share/classes/sun/security/tools/keytool/Main.java b/jdk/src/share/classes/sun/security/tools/keytool/Main.java
index a712d67..e3fdf8c 100644
--- a/jdk/src/share/classes/sun/security/tools/keytool/Main.java
+++ b/jdk/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;
@@ -165,7 +167,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 =
@@ -728,18 +735,34 @@
("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
@@ -761,7 +784,6 @@
}
}
}
- }
if ((command == KEYCLONE || command == CHANGEALIAS)
&& dest == null) {
@@ -807,7 +829,11 @@
* Null stream keystores are loaded later.
*/
if (!nullStream) {
+ if (inplaceImport) {
+ keyStore.load(null, storePass);
+ } else {
keyStore.load(ksStream, storePass);
+ }
if (ksStream != null) {
ksStream.close();
}
@@ -1043,7 +1069,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;
@@ -1174,6 +1204,65 @@
}
}
}
+
+ 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";
+ }
+ }
}
/**
@@ -1873,14 +1962,43 @@
}
}
+ 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)) {
@@ -1890,20 +2008,9 @@
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;
@@ -1964,17 +2071,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
diff --git a/jdk/src/share/classes/sun/security/tools/keytool/Resources.java b/jdk/src/share/classes/sun/security/tools/keytool/Resources.java
index b6b3e6e..0c4bebb 100644
--- a/jdk/src/share/classes/sun/security/tools/keytool/Resources.java
+++ b/jdk/src/share/classes/sun/security/tools/keytool/Resources.java
@@ -453,6 +453,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..."},
};
diff --git a/jdk/test/sun/security/tools/keytool/WeakAlg.java b/jdk/test/sun/security/tools/keytool/WeakAlg.java
index f5315b3..e46f5a7 100644
--- a/jdk/test/sun/security/tools/keytool/WeakAlg.java
+++ b/jdk/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 @@
kt("-delete -alias b");
kt("-printcrl -file b.crl")
.shouldContain("WARNING: not verified");
+
+ jksTypeCheck();
+
+ checkInplaceImportKeyStore();
+ }
+
+ static void jksTypeCheck() throws Exception {
+
+ 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("-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 {
- 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(".*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 @@
}
}
- // 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 @@
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) {
diff --git a/jdk/test/sun/security/tools/keytool/keyalg.sh b/jdk/test/sun/security/tools/keytool/keyalg.sh
index fa3cfcdf..05d5243 100644
--- a/jdk/test/sun/security/tools/keytool/keyalg.sh
+++ b/jdk/test/sun/security/tools/keytool/keyalg.sh
@@ -31,6 +31,8 @@
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"