Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue warning in certutil when using long passwords #75915

Merged
merged 1 commit into from
Aug 2, 2021
Merged

Issue warning in certutil when using long passwords #75915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 50 additions & 14 deletions ...ugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,11 @@ public class CertificateTool extends LoggingAwareMultiCommand {
Pattern.compile("[a-zA-Z0-9!@#$%^&{}\\[\\]()_+\\-=,.~'` ]{1," + MAX_FILENAME_LENGTH + "}");
private static final int DEFAULT_KEY_SIZE = 2048;

// Older versions of OpenSSL had a max internal password length.
// We issue warnings when writing files with passwords that would not be usable in those versions of OpenSSL.
static final String OLD_OPENSSL_VERSION = "1.1.0";
static final int MAX_PASSWORD_OLD_OPENSSL = 50;

/**
* Wraps the certgen object parser.
*/
Expand Down Expand Up @@ -342,9 +347,8 @@ CAInfo getCAInfo(Terminal terminal, OptionSet options, Environment env) throws E
private CAInfo loadPkcs12CA(Terminal terminal, OptionSet options, Environment env) throws Exception {
Path path = resolvePath(options, caPkcs12PathSpec);
char[] passwordOption = getChars(caPasswordSpec.value(options));

Map<Certificate, Key> keys = withPassword("CA (" + path + ")", passwordOption,
terminal, password -> CertParsingUtils.readPkcs12KeyPairs(path, password, a -> password));
Map<Certificate, Key> keys = withPassword("CA (" + path + ")", passwordOption, terminal, false,
password -> CertParsingUtils.readPkcs12KeyPairs(path, password, a -> password));

if (keys.size() != 1) {
throw new IllegalArgumentException("expected a single key in file [" + path.toAbsolutePath() + "] but found [" +
Expand Down Expand Up @@ -384,10 +388,11 @@ CAInfo generateCA(Terminal terminal, OptionSet options) throws Exception {

if (options.hasArgument(caPasswordSpec)) {
char[] password = getChars(caPasswordSpec.value(options));
checkAndConfirmPasswordLengthForOpenSSLCompatibility(password, terminal, false);
return new CAInfo(caCert, keyPair.getPrivate(), true, password);
}
if (options.has(caPasswordSpec)) {
return withPassword("CA Private key", null, terminal, p -> new CAInfo(caCert, keyPair.getPrivate(), true, p.clone()));
return withPassword("CA Private key", null, terminal, true, p -> new CAInfo(caCert, keyPair.getPrivate(), true, p.clone()));
}
return new CAInfo(caCert, keyPair.getPrivate(), true, null);
}
Expand Down Expand Up @@ -533,7 +538,7 @@ static void writeCAInfo(ZipOutputStream outputStream, CAInfo info, Terminal term
final String dirName = createCaDirectory(outputStream);
final String fileName = dirName + "ca.p12";
outputStream.putNextEntry(new ZipEntry(fileName));
withPassword("Generated CA", info.password, terminal, caPassword -> {
withPassword("Generated CA", info.password, terminal, false, caPassword -> {
writePkcs12(fileName, outputStream, "ca", info.certAndKey, null, caPassword, null);
return null;
});
Expand All @@ -552,9 +557,9 @@ static void writePkcs12(String fileName, OutputStream output, String alias, Cert
char[] password, Terminal terminal) throws Exception {
final KeyStore pkcs12 = KeyStore.getInstance("PKCS12");
pkcs12.load(null);
withPassword(fileName, password, terminal, p12Password -> {
withPassword(fileName, password, terminal, true, p12Password -> {
if (isAscii(p12Password)) {
pkcs12.setKeyEntry(alias, pair.key, p12Password, new Certificate[]{pair.cert});
pkcs12.setKeyEntry(alias, pair.key, p12Password, new Certificate[] { pair.cert });
if (caCert != null) {
pkcs12.setCertificateEntry("ca", caCert);
}
Expand Down Expand Up @@ -809,7 +814,7 @@ void generateAndWriteSignedCertificates(Path output, boolean writeZipFile, Optio
final String keyFileName = entryBase + ".key";
outputStream.putNextEntry(new ZipEntry(keyFileName));
if (usePassword) {
withPassword(keyFileName, outputPassword, terminal, password -> {
withPassword(keyFileName, outputPassword, terminal, true, password -> {
pemWriter.writeObject(pair.key, getEncrypter(password));
return null;
});
Expand Down Expand Up @@ -949,16 +954,47 @@ static PEMEncryptor getEncrypter(char[] password) {
return new JcePEMEncryptorBuilder("AES-128-CBC").setProvider(BC_PROV).build(password);
}

private static <T, E extends Exception> T withPassword(String description, char[] password, Terminal terminal,
/**
* Checks whether the supplied password exceeds the maximum length supported by older OpenSSL versions.
* A warning message is printed to the terminal if the password is too long. If {@code confirm} is true, then the user
* (via the terminal) is asked to confirm whether to continue with the potentially problematic password.
* @return {@code false} if the password is too long <em>and</em> the user elects to reject it, otherwise {@code true}.
*/
static boolean checkAndConfirmPasswordLengthForOpenSSLCompatibility(char[] password, Terminal terminal, boolean confirm) {
if (password.length > MAX_PASSWORD_OLD_OPENSSL) {
terminal.println(
Verbosity.SILENT,
"Warning: Your password exceeds "
+ MAX_PASSWORD_OLD_OPENSSL
+ " characters. Versions of OpenSSL older than "
+ OLD_OPENSSL_VERSION
+ " may not be able to read this file."
);
if (confirm) {
return terminal.promptYesNo("Do you want to continue?", true);
}
}
return true;
}

private static <T, E extends Exception> T withPassword(String description, char[] password, Terminal terminal, boolean checkLength,
CheckedFunction<char[], T, E> body) throws E {
if (password == null) {
char[] promptedValue = terminal.readSecret("Enter password for " + description + " : ");
try {
return body.apply(promptedValue);
} finally {
Arrays.fill(promptedValue, (char) 0);
while (true) {
char[] promptedValue = terminal.readSecret("Enter password for " + description + " : ");
if (checkLength && checkAndConfirmPasswordLengthForOpenSSLCompatibility(promptedValue, terminal, true) == false) {
continue;
}
try {
return body.apply(promptedValue);
} finally {
Arrays.fill(promptedValue, (char) 0);
}
}
} else {
if (checkLength) {
checkAndConfirmPasswordLengthForOpenSSLCompatibility(password, terminal, false);
}
return body.apply(password);
}
}
Expand Down
3 changes: 3 additions & 0 deletions ...curity/cli/src/main/java/org/elasticsearch/xpack/security/cli/HttpCertificateCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -944,6 +944,9 @@ private char[] readPassword(Terminal terminal, String prompt, boolean confirm) {
continue;
}
}
if (CertificateTool.checkAndConfirmPasswordLengthForOpenSSLCompatibility(password, terminal, confirm) == false) {
continue;
}
return password;
} else {
terminal.println(Terminal.Verbosity.SILENT, "Passwords must be plain ASCII");
Expand Down
149 changes: 133 additions & 16 deletions ...security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@
*/
package org.elasticsearch.xpack.security.cli;

import com.google.common.jimfs.Configuration;
import com.google.common.jimfs.Jimfs;
import joptsimple.OptionSet;
import joptsimple.OptionSpec;

import com.google.common.jimfs.Configuration;
import com.google.common.jimfs.Jimfs;

import org.bouncycastle.asn1.ASN1ObjectIdentifier;
import org.bouncycastle.asn1.ASN1Sequence;
import org.bouncycastle.asn1.ASN1String;
Expand All @@ -26,39 +28,33 @@
import org.bouncycastle.openssl.PEMEncryptedKeyPair;
import org.bouncycastle.openssl.PEMParser;
import org.bouncycastle.pkcs.PKCS10CertificationRequest;
import org.elasticsearch.jdk.JavaVersion;
import org.elasticsearch.cli.MockTerminal;
import org.elasticsearch.cli.Terminal;
import org.elasticsearch.cli.UserException;
import org.elasticsearch.common.Strings;
import org.elasticsearch.core.SuppressForbidden;
import org.elasticsearch.core.PathUtils;
import org.elasticsearch.common.network.NetworkAddress;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.CollectionUtils;
import org.elasticsearch.core.PathUtils;
import org.elasticsearch.core.SuppressForbidden;
import org.elasticsearch.core.internal.io.IOUtils;
import org.elasticsearch.env.Environment;
import org.elasticsearch.env.TestEnvironment;
import org.elasticsearch.jdk.JavaVersion;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.SecuritySettingsSourceField;
import org.elasticsearch.xpack.core.ssl.CertParsingUtils;
import org.elasticsearch.xpack.core.ssl.PemUtils;
import org.elasticsearch.xpack.security.cli.CertificateTool.CAInfo;
import org.elasticsearch.xpack.security.cli.CertificateTool.CertificateAuthorityCommand;
import org.elasticsearch.xpack.security.cli.CertificateTool.CertificateCommand;
import org.elasticsearch.xpack.security.cli.CertificateTool.CertificateInformation;
import org.elasticsearch.xpack.security.cli.CertificateTool.GenerateCertificateCommand;
import org.elasticsearch.xpack.security.cli.CertificateTool.Name;
import org.elasticsearch.xpack.core.ssl.CertParsingUtils;
import org.elasticsearch.xpack.core.ssl.PemUtils;
import org.hamcrest.Matchers;
import org.junit.After;
import org.junit.BeforeClass;
import org.mockito.Mockito;

import javax.net.ssl.KeyManagerFactory;
import javax.net.ssl.TrustManagerFactory;
import javax.net.ssl.X509ExtendedKeyManager;
import javax.net.ssl.X509ExtendedTrustManager;
import javax.security.auth.x500.X500Principal;
import java.io.IOException;
import java.io.InputStream;
import java.io.Reader;
Expand Down Expand Up @@ -90,6 +86,11 @@
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.Function;
import java.util.stream.Collectors;
import javax.net.ssl.KeyManagerFactory;
import javax.net.ssl.TrustManagerFactory;
import javax.net.ssl.X509ExtendedKeyManager;
import javax.net.ssl.X509ExtendedTrustManager;
import javax.security.auth.x500.X500Principal;

import static org.elasticsearch.test.FileMatchers.pathExists;
import static org.hamcrest.Matchers.arrayWithSize;
Expand All @@ -98,6 +99,8 @@
import static org.hamcrest.Matchers.in;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;

/**
Expand Down Expand Up @@ -410,11 +413,80 @@ public void testGeneratingSignedPemCertificates() throws Exception {
GeneralNames.fromExtensions(x509CertHolder.getExtensions(), Extension.subjectAlternativeName);
assertSubjAltNames(subjAltNames, certInfo);
}
assertThat(p12, Matchers.not(pathExists()));
assertThat(p12, not(pathExists()));
}
}
}

public void testHandleLongPasswords() throws Exception {
final Path tempDir = initTempDir();

final MockTerminal terminal = new MockTerminal();
Environment env = TestEnvironment.newEnvironment(Settings.builder().put("path.home", tempDir).build());

final Path caFile = tempDir.resolve("ca.p12");
final Path pemZipFile = tempDir.resolve("cert.zip").toAbsolutePath();

final String longPassword = randomAlphaOfLengthBetween(51, 256);

boolean expectPrompt = randomBoolean();
final CertificateAuthorityCommand caCommand = new PathAwareCertificateAuthorityCommand(caFile);
final OptionSet gen1Options = caCommand.getParser()
.parse("-ca-dn", "CN=Test-Ca", (expectPrompt ? "-pass" : "-pass=" + longPassword), "-out", caFile.toString());

if (expectPrompt) {
terminal.addSecretInput(longPassword);
terminal.addTextInput("y"); // Yes, really use it
}
caCommand.execute(terminal, gen1Options, env);
assertThat(terminal.getOutput(), containsString("50 characters"));
assertThat(terminal.getOutput(), containsString("OpenSSL"));
assertThat(terminal.getOutput(), containsString("1.1.0"));

terminal.reset();
final GenerateCertificateCommand genCommand = new PathAwareGenerateCertificateCommand(caFile, pemZipFile);
final OptionSet gen2Options = genCommand.getParser().parse(
"-ca", "<ca>",
"-ca-pass", longPassword,
(expectPrompt ? "-pass" : "-pass=" + longPassword),
"-out", "<node2>",
"-name", "cert",
"-pem"
);

if (expectPrompt) {
terminal.addSecretInput(longPassword);
terminal.addTextInput("n"); // No, don't really use it
terminal.addSecretInput(longPassword);
terminal.addTextInput("y"); // This time, yes we will use it
}
genCommand.execute(terminal, gen2Options, env);
assertThat(terminal.getOutput(), containsString("50 characters"));
assertThat(terminal.getOutput(), containsString("OpenSSL"));
assertThat(terminal.getOutput(), containsString("1.1.0"));

assertThat(pemZipFile, pathExists());

final KeyStore caKeyStore = CertParsingUtils.readKeyStore(caFile, "PKCS12", longPassword.toCharArray());
Certificate caCert = caKeyStore.getCertificate("ca");
assertThat(caCert, notNullValue());

FileSystem zip = FileSystems.newFileSystem(new URI("jar:" + pemZipFile.toUri()), Collections.emptyMap());
Path zipRoot = zip.getPath("/");

final Path keyPath = zipRoot.resolve("cert/cert.key");
final PrivateKey key = PemUtils.readPrivateKey(keyPath, () -> longPassword.toCharArray());
assertThat(key, notNullValue());

final Path certPath = zipRoot.resolve("cert/cert.crt");
final Certificate[] certificates = CertParsingUtils.readCertificates(Collections.singletonList(certPath));
assertThat(certificates, arrayWithSize(1));
assertThat(
((X509Certificate) certificates[0]).getIssuerX500Principal(),
equalTo(((X509Certificate) caCert).getSubjectX500Principal())
);
}

public void testGetCAInfo() throws Exception {
Environment env = TestEnvironment.newEnvironment(Settings.builder().put("path.home", createTempDir()).build());
Path testNodeCertPath = getDataPath("/org/elasticsearch/xpack/security/cli/testnode.crt");
Expand Down Expand Up @@ -563,7 +635,7 @@ public void testCreateCaAndMultipleInstances() throws Exception {
JavaVersion.current().compareTo(JavaVersion.parse("8")) == 0);
final Path tempDir = initTempDir();

final Terminal terminal = new MockTerminal();
final MockTerminal terminal = new MockTerminal();
Environment env = TestEnvironment.newEnvironment(Settings.builder().put("path.home", tempDir).build());

final Path caFile = tempDir.resolve("ca.p12");
Expand Down Expand Up @@ -771,7 +843,7 @@ public void testTrustBetweenPEMandPKCS12() throws Exception {
Path zip2Root = zip2FS.getPath("/");

final Path ca2 = zip2Root.resolve("ca/ca.p12");
assertThat(ca2, Matchers.not(pathExists()));
assertThat(ca2, not(pathExists()));

final Path node2Cert = zip2Root.resolve("node02/node02.crt");
assertThat(node2Cert, pathExists());
Expand Down Expand Up @@ -962,6 +1034,51 @@ private static Path resolvePath(String path) {
return PathUtils.get(path).toAbsolutePath();
}

private String generateCA(Path caFile, MockTerminal terminal, Environment env) throws Exception {
final int caKeySize = randomIntBetween(4, 8) * 512;
final int days = randomIntBetween(7, 1500);
final String caPassword = randomFrom("", randomAlphaOfLengthBetween(4, 80));

final CertificateAuthorityCommand caCommand = new PathAwareCertificateAuthorityCommand(caFile);
final OptionSet caOptions = caCommand.getParser().parse(
"-ca-dn", "CN=My ElasticSearch Cluster",
"-pass", caPassword,
"-out", caFile.toString(),
"-keysize", String.valueOf(caKeySize),
"-days", String.valueOf(days)
);
caCommand.execute(terminal, caOptions, env);

// Check output for OpenSSL compatibility version
if (caPassword.length() > 50) {
assertThat(terminal.getOutput(), containsString("OpenSSL"));
} else {
assertThat(terminal.getOutput(), not(containsString("OpenSSL")));
}

assertThat(caFile, pathExists());

return caPassword;
}

/**
* Converting jimfs Paths into strings and back to paths doesn't work with the security manager.
* This class works around that by sticking with the original path objects
*/
private class PathAwareCertificateAuthorityCommand extends CertificateAuthorityCommand {
private final Path caFile;

private PathAwareCertificateAuthorityCommand(Path caFile) {
this.caFile = caFile;
}

@Override
Path resolveOutputPath(Terminal terminal, OptionSet options, String defaultFilename) {
// Needed to work within the security manager
return caFile;
}
}

/**
* Converting jimfs Paths into strings and back to paths doesn't work with the security manager.
* This class works around that by sticking with the original path objects
Expand Down
Loading