8177569: keytool should not warn if signature algorithm used in cacerts is weak
Reviewed-by: mullan
This commit is contained in:
Weijun Wang
parent
a2173b8f39
commit
837ceec9a5
@ -1025,6 +1025,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();
|
||||
}
|
||||
@ -1758,9 +1765,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);
|
||||
}
|
||||
|
||||
/**
|
||||
@ -2118,6 +2124,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.
|
||||
@ -2127,10 +2137,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()};
|
||||
@ -2814,8 +2820,8 @@ public final class Main {
|
||||
}
|
||||
|
||||
if (noprompt) {
|
||||
keyStore.setCertificateEntry(alias, cert);
|
||||
checkWeak(rb.getString("the.input"), cert);
|
||||
keyStore.setCertificateEntry(alias, cert);
|
||||
return true;
|
||||
}
|
||||
|
||||
@ -3049,6 +3055,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),
|
||||
@ -3056,7 +3067,7 @@ public final class Main {
|
||||
cert.getNotAfter().toString(),
|
||||
getCertFingerPrint("SHA-1", cert),
|
||||
getCertFingerPrint("SHA-256", cert),
|
||||
withWeak(cert.getSigAlgName()),
|
||||
sigName,
|
||||
withWeak(pkey),
|
||||
cert.getVersion()
|
||||
};
|
||||
@ -3111,7 +3122,7 @@ public final class Main {
|
||||
* or null otherwise. A label is added.
|
||||
*/
|
||||
private static Pair<String,Certificate>
|
||||
getTrustedSigner(Certificate cert, KeyStore ks) throws Exception {
|
||||
getSigner(Certificate cert, KeyStore ks) throws Exception {
|
||||
if (ks.getCertificateAlias(cert) != null) {
|
||||
return new Pair<>("", cert);
|
||||
}
|
||||
@ -3467,9 +3478,9 @@ public final class Main {
|
||||
// do we trust the cert at the top?
|
||||
Certificate topCert = replyCerts[replyCerts.length-1];
|
||||
boolean fromKeyStore = true;
|
||||
Pair<String,Certificate> root = getTrustedSigner(topCert, keyStore);
|
||||
Pair<String,Certificate> root = getSigner(topCert, keyStore);
|
||||
if (root == null && trustcacerts && caks != null) {
|
||||
root = getTrustedSigner(topCert, caks);
|
||||
root = getSigner(topCert, caks);
|
||||
fromKeyStore = false;
|
||||
}
|
||||
if (root == null) {
|
||||
@ -4301,9 +4312,19 @@ public final class Main {
|
||||
return result;
|
||||
}
|
||||
|
||||
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));
|
||||
}
|
||||
@ -4316,7 +4337,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) {
|
||||
@ -4325,15 +4347,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());
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -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 /test/lib
|
||||
@ -78,7 +78,8 @@ public class WeakAlg {
|
||||
.shouldMatch("<b>.*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);
|
||||
@ -184,7 +185,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) {
|
||||
@ -208,12 +209,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 " + KeyStoreUtil.getCacerts())
|
||||
.shouldNotContain("risk");
|
||||
kt("-list -v -keystore " + KeyStoreUtil.getCacerts())
|
||||
.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")
|
||||
@ -265,6 +294,26 @@ public class WeakAlg {
|
||||
|
||||
// install reply
|
||||
|
||||
reStore();
|
||||
certreq("c", "");
|
||||
gencert("a-c", "");
|
||||
kt("-importcert -alias c -file a-c.cert")
|
||||
.shouldContain("Warning")
|
||||
.shouldMatch("Issuer <a>.*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", "");
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user