diff --git a/src/share/classes/sun/security/tools/keytool/Main.java b/src/share/classes/sun/security/tools/keytool/Main.java index e21493e69d6eb9f6ca6361f044764ac98028886d..a712d67b0c46c0b5b00e1606042ab7b3e27ffc0b 100644 --- a/src/share/classes/sun/security/tools/keytool/Main.java +++ b/src/share/classes/sun/security/tools/keytool/Main.java @@ -935,6 +935,13 @@ public final class Main { cf = CertificateFactory.getInstance("X509"); } + // -trustcacerts can only be specified on -importcert. + // Reset it so that warnings on CA cert will remain for + // -printcert, etc. + if (command != IMPORTCERT) { + trustcacerts = false; + } + if (trustcacerts) { caks = KeyStoreUtil.getCacertsKeyStore(); } @@ -1676,9 +1683,8 @@ public final class Main { if (keyPass == null) { keyPass = promptForKeyPass(alias, null, storePass); } - keyStore.setKeyEntry(alias, privKey, keyPass, chain); - checkWeak(rb.getString("the.generated.certificate"), chain[0]); + keyStore.setKeyEntry(alias, privKey, keyPass, chain); } /** @@ -2029,6 +2035,10 @@ public final class Main { } try { + Certificate c = srckeystore.getCertificate(alias); + if (c != null) { + checkWeak("<" + newAlias + ">", c); + } keyStore.setEntry(newAlias, entry, pp); // Place the check so that only successful imports are blocked. // For example, we don't block a failed SecretEntry import. @@ -2038,10 +2048,6 @@ public final class Main { "The.destination.pkcs12.keystore.has.different.storepass.and.keypass.Please.retry.with.destkeypass.specified.")); } } - Certificate c = srckeystore.getCertificate(alias); - if (c != null) { - checkWeak("<" + newAlias + ">", c); - } return 1; } catch (KeyStoreException kse) { Object[] source2 = {alias, kse.toString()}; @@ -2727,8 +2733,8 @@ public final class Main { } if (noprompt) { - keyStore.setCertificateEntry(alias, cert); checkWeak(rb.getString("the.input"), cert); + keyStore.setCertificateEntry(alias, cert); return true; } @@ -2979,6 +2985,11 @@ public final class Main { MessageFormat form = new MessageFormat (rb.getString(".PATTERN.printX509Cert.with.weak")); PublicKey pkey = cert.getPublicKey(); + String sigName = cert.getSigAlgName(); + // No need to warn about sigalg of a trust anchor + if (!isTrustedCert(cert)) { + sigName = withWeak(sigName); + } Object[] source = {cert.getSubjectDN().toString(), cert.getIssuerDN().toString(), cert.getSerialNumber().toString(16), @@ -2987,7 +2998,7 @@ public final class Main { getCertFingerPrint("MD5", cert), getCertFingerPrint("SHA1", cert), getCertFingerPrint("SHA-256", cert), - withWeak(cert.getSigAlgName()), + sigName, withWeak(pkey), cert.getVersion() }; @@ -3061,7 +3072,7 @@ public final class Main { * or null otherwise. A label is added. */ private static Pair - getTrustedSigner(Certificate cert, KeyStore ks) throws Exception { + getSigner(Certificate cert, KeyStore ks) throws Exception { if (ks.getCertificateAlias(cert) != null) { return new Pair<>("", cert); } @@ -3412,9 +3423,9 @@ public final class Main { // do we trust the cert at the top? Certificate topCert = replyCerts[replyCerts.length-1]; boolean fromKeyStore = true; - Pair root = getTrustedSigner(topCert, keyStore); + Pair root = getSigner(topCert, keyStore); if (root == null && trustcacerts && caks != null) { - root = getTrustedSigner(topCert, caks); + root = getSigner(topCert, caks); fromKeyStore = false; } if (root == null) { @@ -4220,9 +4231,19 @@ public final class Main { return ext; } + private boolean isTrustedCert(Certificate cert) throws KeyStoreException { + if (caks != null && caks.getCertificateAlias(cert) != null) { + return true; + } else { + String inKS = keyStore.getCertificateAlias(cert); + return inKS != null && keyStore.isCertificateEntry(inKS); + } + } + private void checkWeak(String label, String sigAlg, Key key) { - if (!DISABLED_CHECK.permits(SIG_PRIMITIVE_SET, sigAlg, null)) { + if (sigAlg != null && !DISABLED_CHECK.permits( + SIG_PRIMITIVE_SET, sigAlg, null)) { weakWarnings.add(String.format( rb.getString("whose.sigalg.risk"), label, sigAlg)); } @@ -4235,7 +4256,8 @@ public final class Main { } } - private void checkWeak(String label, Certificate[] certs) { + private void checkWeak(String label, Certificate[] certs) + throws KeyStoreException { for (int i = 0; i < certs.length; i++) { Certificate cert = certs[i]; if (cert instanceof X509Certificate) { @@ -4244,15 +4266,18 @@ public final class Main { if (certs.length > 1) { fullLabel = oneInMany(label, i, certs.length); } - checkWeak(fullLabel, xc.getSigAlgName(), xc.getPublicKey()); + checkWeak(fullLabel, xc); } } } - private void checkWeak(String label, Certificate cert) { + private void checkWeak(String label, Certificate cert) + throws KeyStoreException { if (cert instanceof X509Certificate) { X509Certificate xc = (X509Certificate)cert; - checkWeak(label, xc.getSigAlgName(), xc.getPublicKey()); + // No need to check the sigalg of a trust anchor + String sigAlg = isTrustedCert(cert) ? null : xc.getSigAlgName(); + checkWeak(label, sigAlg, xc.getPublicKey()); } } diff --git a/test/sun/security/tools/keytool/WeakAlg.java b/test/sun/security/tools/keytool/WeakAlg.java index ef1c6192a57764dcecd7795308f6642345c46fcf..f5315b34d05ce5736062dc1b3bfb22a53bea49a7 100644 --- a/test/sun/security/tools/keytool/WeakAlg.java +++ b/test/sun/security/tools/keytool/WeakAlg.java @@ -23,7 +23,7 @@ /* * @test - * @bug 8171319 + * @bug 8171319 8177569 * @summary keytool should print out warnings when reading or generating * cert/cert req using weak algorithms * @library /lib/testlibrary @@ -37,6 +37,8 @@ import sun.security.util.DisabledAlgorithmConstraints; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.FileInputStream; import java.io.IOException; import java.io.InputStream; import java.io.PrintStream; @@ -54,6 +56,10 @@ import java.util.stream.Stream; public class WeakAlg { + static String sep = File.separator; + static String cacerts_location = System.getProperty("java.home") + + sep + "lib" + sep + "security" + sep + "cacerts"; + public static void main(String[] args) throws Throwable { rm("ks"); @@ -75,7 +81,8 @@ public class WeakAlg { .shouldMatch(".*512-bit RSA key.*risk") .shouldContain("512-bit RSA key (weak)"); - // Multiple warnings for multiple cert in -printcert or -list or -exportcert + // Multiple warnings for multiple cert in -printcert + // or -list or -exportcert // -certreq, -printcertreq, -gencert checkCertReq("a", "", null); @@ -181,7 +188,7 @@ public class WeakAlg { .shouldMatch("The input.*MD5withRSA.*risk") .shouldNotContain("[no]"); - // cert is self-signed cacerts + // JDK-8177569: no warning for sigalg of trusted cert String weakSigAlgCA = null; KeyStore ks = KeyStoreUtil.getCacertsKeyStore(); if (ks != null) { @@ -205,12 +212,40 @@ public class WeakAlg { } } if (weakSigAlgCA != null) { + // The following 2 commands still have a warning on why not using + // the -cacerts option directly. + kt("-list -keystore " + cacerts_location) + .shouldNotContain("risk"); + kt("-list -v -keystore " + cacerts_location) + .shouldNotContain("risk"); + + // -printcert will always show warnings + kt("-printcert -file ca.cert") + .shouldContain("name: " + weakSigAlgCA + " (weak)") + .shouldContain("Warning") + .shouldMatch("The certificate.*" + weakSigAlgCA + ".*risk"); + kt("-printcert -file ca.cert -trustcacerts") // -trustcacerts useless + .shouldContain("name: " + weakSigAlgCA + " (weak)") + .shouldContain("Warning") + .shouldMatch("The certificate.*" + weakSigAlgCA + ".*risk"); + + // Importing with -trustcacerts ignore CA cert's sig alg kt("-delete -alias d"); kt("-importcert -alias d -trustcacerts -file ca.cert", "no") .shouldContain("Certificate already exists in system-wide CA") + .shouldNotContain("risk") + .shouldContain("Do you still want to add it to your own keystore?"); + kt("-importcert -alias d -trustcacerts -file ca.cert -noprompt") + .shouldNotContain("risk") + .shouldNotContain("[no]"); + + // but not without -trustcacerts + kt("-delete -alias d"); + kt("-importcert -alias d -file ca.cert", "no") + .shouldContain("name: " + weakSigAlgCA + " (weak)") .shouldContain("Warning") .shouldMatch("The input.*" + weakSigAlgCA + ".*risk") - .shouldContain("Do you still want to add it to your own keystore?"); + .shouldContain("Trust this certificate?"); kt("-importcert -alias d -file ca.cert -noprompt") .shouldContain("Warning") .shouldMatch("The input.*" + weakSigAlgCA + ".*risk") @@ -262,6 +297,26 @@ public class WeakAlg { // install reply + reStore(); + certreq("c", ""); + gencert("a-c", ""); + kt("-importcert -alias c -file a-c.cert") + .shouldContain("Warning") + .shouldMatch("Issuer .*MD5withRSA.*risk"); + + // JDK-8177569: no warning for sigalg of trusted cert + reStore(); + // Change a into a TrustedCertEntry + kt("-exportcert -alias a -file a.cert"); + kt("-delete -alias a"); + kt("-importcert -alias a -file a.cert -noprompt"); + kt("-list -alias a -v") + .shouldNotContain("weak") + .shouldNotContain("Warning"); + // This time a is trusted and no warning on its weak sig alg + kt("-importcert -alias c -file a-c.cert") + .shouldNotContain("Warning"); + reStore(); gencert("a-b", "");