Skip to content

Commit

Permalink
Issue warning in certutil when using long passwords
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.

Co-authored-by: Tim Vernum <[email protected]>
  • Loading branch information
@MiguelFerreira1998 @tvernum
MiguelFerreira1998 and tvernum authored Aug 2, 2021
1 parent 2dcc337 commit 6ea0ca4
Show file tree
Hide file tree
Showing 4 changed files with 212 additions and 49 deletions.
62 changes: 49 additions & 13 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 @@ -108,6 +108,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 @@ -349,9 +354,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 @@ -391,10 +395,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 @@ -541,9 +546,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 @@ -784,7 +789,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 @@ -924,16 +929,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 @@ -938,6 +938,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
151 changes: 121 additions & 30 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 @@ -39,14 +39,14 @@
import org.elasticsearch.env.TestEnvironment;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.SecuritySettingsSourceField;
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.hamcrest.Matchers;
import org.junit.After;
import org.junit.BeforeClass;

Expand All @@ -70,6 +70,7 @@
import java.security.KeyPair;
import java.security.KeyStore;
import java.security.Principal;
import java.security.PrivateKey;
import java.security.cert.Certificate;
import java.security.cert.X509Certificate;
import java.security.interfaces.RSAKey;
Expand All @@ -93,6 +94,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;
import static org.mockito.Matchers.any;
import static org.mockito.Mockito.mock;
Expand Down Expand Up @@ -361,7 +364,7 @@ public void testGeneratingSignedPemCertificates() throws Exception {
GeneralNames.fromExtensions(x509CertHolder.getExtensions(), Extension.subjectAlternativeName);
assertSubjAltNames(subjAltNames, certInfo);
}
assertThat(p12, Matchers.not(pathExists()));
assertThat(p12, not(pathExists()));
}
}
}
Expand All @@ -382,6 +385,75 @@ public void testErrorMessageOnInvalidKeepCaOption() {
assertThat(e.getMessage(), containsString("Generating certificates without providing a CA is no longer supported"));
}

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(List.of(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 @@ -520,7 +592,7 @@ public void testNameValues() throws Exception {
public void testCreateCaAndMultipleInstances() throws Exception {
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 @@ -689,7 +761,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 @@ -881,6 +953,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 All @@ -907,30 +1024,4 @@ Path resolveOutputPath(Terminal terminal, OptionSet options, String defaultFilen
return outFile;
}
}

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

final CertificateAuthorityCommand caCommand = new CertificateAuthorityCommand() {
@Override
Path resolveOutputPath(Terminal terminal, OptionSet options, String defaultFilename) {
// Needed to work within the security manager
return 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);

assertThat(caFile, pathExists());

return caPassword;
}
}
Loading

0 comments on commit 6ea0ca4

Please sign in to comment.