From 7cf3c0ff1482d23494f88a0eaf13a821adc0bc47 Mon Sep 17 00:00:00 2001 From: Weijun Wang <weijun@openjdk.org> Date: Wed, 12 Jul 2017 10:55:40 +0800 Subject: [PATCH] 8182879: Add warnings to keytool when using JKS and JCEKS Reviewed-by: vinnie, ahgross, mullan --- .../sun/security/tools/keytool/Main.java | 187 ++++++++++++++---- .../sun/security/tools/keytool/Resources.java | 6 +- .../sun/security/tools/keytool/WeakAlg.java | 181 +++++++++++++++-- 3 files changed, 319 insertions(+), 55 deletions(-) diff --git a/src/java.base/share/classes/sun/security/tools/keytool/Main.java b/src/java.base/share/classes/sun/security/tools/keytool/Main.java index dac650b1451..65675435fc1 100644 --- a/src/java.base/share/classes/sun/security/tools/keytool/Main.java +++ b/src/java.base/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; @@ -168,7 +170,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 = @@ -846,37 +853,52 @@ 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) { - 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 @@ public final class Main { // 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 @@ public final class Main { * 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 @@ 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; @@ -1298,6 +1328,51 @@ public final class Main { } } } + + 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 @@ 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 * @return the src KeyStore */ KeyStore loadSourceKeyStore() throws Exception { - boolean isPkcs11 = false; InputStream is = null; File srcksfile = null; @@ -2007,20 +2110,9 @@ public final class Main { 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 @@ 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 diff --git a/src/java.base/share/classes/sun/security/tools/keytool/Resources.java b/src/java.base/share/classes/sun/security/tools/keytool/Resources.java index 9bdd95cbda0..5213b1a7903 100644 --- a/src/java.base/share/classes/sun/security/tools/keytool/Resources.java +++ b/src/java.base/share/classes/sun/security/tools/keytool/Resources.java @@ -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 @@ 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..."}, }; diff --git a/test/jdk/sun/security/tools/keytool/WeakAlg.java b/test/jdk/sun/security/tools/keytool/WeakAlg.java index c9cefb1a4a7..2a7216bc21d 100644 --- a/test/jdk/sun/security/tools/keytool/WeakAlg.java +++ b/test/jdk/sun/security/tools/keytool/WeakAlg.java @@ -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 sun.security.util.DisabledAlgorithmConstraints; 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 @@ public class WeakAlg { 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 @@ 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(); @@ -559,19 +705,26 @@ public class WeakAlg { 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) {