Mercurial > hg > openjdk > bsd-port > jdk
changeset 8806:f468fe784056
8182879: Add warnings to keytool when using JKS and JCEKS
Reviewed-by: mullan, weijun
| author | coffeys |
|---|---|
| date | Tue, 21 Nov 2017 23:14:09 +0000 |
| parents | 50dc930e07d6 |
| children | b531b441e3c3 |
| files | src/share/classes/sun/security/tools/keytool/Main.java src/share/classes/sun/security/tools/keytool/Resources.java test/sun/security/tools/keytool/WeakAlg.java test/sun/security/tools/keytool/keyalg.sh |
| diffstat | 4 files changed, 317 insertions(+), 40 deletions(-) [+] |
line wrap: on
line diff
--- a/src/share/classes/sun/security/tools/keytool/Main.java Thu Aug 03 00:20:39 2017 -0700 +++ b/src/share/classes/sun/security/tools/keytool/Main.java Tue Nov 21 23:14:09 2017 +0000 @@ -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; @@ -162,7 +164,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 = @@ -719,18 +726,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 @@ -751,7 +774,6 @@ } } } - } if ((command == KEYCLONE || command == CHANGEALIAS) && dest == null) { @@ -797,7 +819,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(); } @@ -1025,7 +1051,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; @@ -1156,6 +1186,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"; + } + } } /** @@ -1778,14 +1867,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)) { @@ -1795,20 +1913,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; @@ -1869,17 +1976,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 || destKeyPass != null) { throw new Exception(rb.getString( "if.alias.not.specified.destalias.srckeypass.and.destkeypass.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/share/classes/sun/security/tools/keytool/Resources.java Thu Aug 03 00:20:39 2017 -0700 +++ b/src/share/classes/sun/security/tools/keytool/Resources.java Tue Nov 21 23:14:09 2017 +0000 @@ -442,6 +442,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 00:20:39 2017 -0700 +++ b/test/sun/security/tools/keytool/WeakAlg.java Tue Nov 21 23:14:09 2017 +0000 @@ -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 /lib/testlibrary @@ -31,6 +31,7 @@ * @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; @@ -136,25 +137,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 { @@ -520,17 +657,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(); @@ -554,19 +696,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) {
--- a/test/sun/security/tools/keytool/keyalg.sh Thu Aug 03 00:20:39 2017 -0700 +++ b/test/sun/security/tools/keytool/keyalg.sh Tue Nov 21 23:14:09 2017 +0000 @@ -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"