Skip to content

Commit

Permalink
Issue warning in certutil when using long passwords (#75915)
Browse files Browse the repository at this point in the history 
Older versions of OpenSSL (prior to 1.1.0) had a fixed 50 char buffer
for password input.
This means that keys (etc) encrypted with a password > 50 chars
cannot be used by old versions of OpenSSL.

This change adds warnings/prompts when creating encrypted files with
passwords longer than 50 characters in elasticsearch-certutil.

Backport of: #36689

Co-authored-by: MiguelFerreira1998 <33258304+MiguelFerreira1998@users.noreply.github.com>

Co-authored-by: MiguelFerreira1998 <33258304+MiguelFerreira1998@users.noreply.github.com>
  • Loading branch information
@tvernum @MiguelFerreira1998
tvernum and MiguelFerreira1998 committed Aug 2, 2021
1 parent 2b0d25b commit cc2ba16
Show file tree
Hide file tree
Showing 4 changed files with 225 additions and 36 deletions.
64 changes: 50 additions & 14 deletions ...ugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java
View file
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
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
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

0 comments on commit cc2ba16

Please sign in to comment.