From 6ea0ca498612279833cbfa827906497fd14cc145 Mon Sep 17 00:00:00 2001 From: MiguelFerreira1998 <33258304+MiguelFerreira1998@users.noreply.github.com> Date: Mon, 2 Aug 2021 05:50:52 +0100 Subject: [PATCH] Issue warning in certutil when using long passwords 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 --- .../xpack/security/cli/CertificateTool.java | 62 +++++-- .../security/cli/HttpCertificateCommand.java | 3 + .../security/cli/CertificateToolTests.java | 151 ++++++++++++++---- .../cli/HttpCertificateCommandTests.java | 45 +++++- 4 files changed, 212 insertions(+), 49 deletions(-) diff --git a/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java b/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java index a579fc909ee1d..c021a845f695c 100644 --- a/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java +++ b/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java @@ -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. */ @@ -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 keys = withPassword("CA (" + path + ")", passwordOption, - terminal, password -> CertParsingUtils.readPkcs12KeyPairs(path, password, a -> password)); + Map 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 [" + @@ -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); } @@ -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); } @@ -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; }); @@ -924,16 +929,47 @@ static PEMEncryptor getEncrypter(char[] password) { return new JcePEMEncryptorBuilder("AES-128-CBC").setProvider(BC_PROV).build(password); } - private static 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 and 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 withPassword(String description, char[] password, Terminal terminal, boolean checkLength, CheckedFunction 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); } } diff --git a/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/HttpCertificateCommand.java b/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/HttpCertificateCommand.java index 8e0679dd951aa..c25f4e2a77ff8 100644 --- a/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/HttpCertificateCommand.java +++ b/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/HttpCertificateCommand.java @@ -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"); diff --git a/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java b/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java index ac3818d72c19a..3eb96f6edd3e3 100644 --- a/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java +++ b/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java @@ -39,6 +39,7 @@ 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; @@ -46,7 +47,6 @@ 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; @@ -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; @@ -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; @@ -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())); } } } @@ -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-pass", longPassword, + (expectPrompt ? "-pass" : "-pass=" + longPassword), + "-out", "", + "-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"); @@ -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"); @@ -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()); @@ -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 @@ -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; - } } diff --git a/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/HttpCertificateCommandTests.java b/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/HttpCertificateCommandTests.java index cf57a45a7752c..99c4e350b1ee1 100644 --- a/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/HttpCertificateCommandTests.java +++ b/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/HttpCertificateCommandTests.java @@ -141,7 +141,7 @@ public void testGenerateSingleCertificateSigningRequest() throws Exception { terminal.addTextInput(randomBoolean() ? "n" : ""); // don't change advanced settings - final String password = randomPassword(); + final String password = randomPassword(false); terminal.addSecretInput(password); if ("".equals(password) == false) { terminal.addSecretInput(password); @@ -268,10 +268,13 @@ public void testGenerateSingleCertificateWithExistingCA() throws Exception { terminal.addTextInput(randomBoolean() ? "n" : ""); // don't change advanced settings - final String password = randomPassword(); + final String password = randomPassword(randomBoolean()); terminal.addSecretInput(password); if ("".equals(password) == false) { terminal.addSecretInput(password); + if (password.length() > 50) { + terminal.addTextInput("y"); // Accept OpenSSL issue + } } // confirm terminal.addTextInput(outFile.toString()); @@ -280,6 +283,12 @@ public void testGenerateSingleCertificateWithExistingCA() throws Exception { final OptionSet options = command.getParser().parse(new String[0]); command.execute(terminal, options, env); + if (password.length() > 50) { + assertThat(terminal.getOutput(), containsString("OpenSSL")); + } else { + assertThat(terminal.getOutput(), not(containsString("OpenSSL"))); + } + Path zipRoot = getZipRoot(outFile); assertThat(zipRoot.resolve("elasticsearch"), isDirectory()); @@ -365,10 +374,22 @@ public void testGenerateMultipleCertificateWithNewCA() throws Exception { caKeySize = HttpCertificateCommand.DEFAULT_CA_KEY_SIZE; } - final String caPassword = randomPassword(); + final String caPassword = randomPassword(randomBoolean()); + boolean expectLongPasswordWarning = caPassword.length() > 50; + // randomly enter a long password here, and then say "no" on the warning prompt + if (randomBoolean()) { + String longPassword = randomAlphaOfLengthBetween(60, 120); + terminal.addSecretInput(longPassword); + terminal.addSecretInput(longPassword); + terminal.addTextInput("n"); // Change our mind + expectLongPasswordWarning = true; + } terminal.addSecretInput(caPassword); if ("".equals(caPassword) == false) { terminal.addSecretInput(caPassword); + if (caPassword.length() > 50) { + terminal.addTextInput("y"); // Acknowledge possible OpenSSL issue + } } // confirm final int certYears = randomIntBetween(1, 8); @@ -398,7 +419,13 @@ public void testGenerateMultipleCertificateWithNewCA() throws Exception { terminal.addTextInput("n"); // no more certs - final String password = randomPassword(); + final String password = randomPassword(false); + // randomly enter an incorrect password here which will fail the "enter twice" check and prompt to try again + if (randomBoolean()) { + String wrongPassword = randomAlphaOfLengthBetween(8, 20); + terminal.addSecretInput(wrongPassword); + terminal.addSecretInput("__" + wrongPassword); + } terminal.addSecretInput(password); if ("".equals(password) == false) { terminal.addSecretInput(password); @@ -410,6 +437,12 @@ public void testGenerateMultipleCertificateWithNewCA() throws Exception { final OptionSet options = command.getParser().parse(new String[0]); command.execute(terminal, options, env); + if (expectLongPasswordWarning) { + assertThat(terminal.getOutput(), containsString("OpenSSL")); + } else { + assertThat(terminal.getOutput(), not(containsString("OpenSSL"))); + } + Path zipRoot = getZipRoot(outFile); // Should have a CA directory with the generated CA. @@ -619,12 +652,12 @@ private List randomHostNames() { return hostNames; } - private String randomPassword() { + private String randomPassword(boolean longPassword) { // We want to assert that this password doesn't end up in any output files, so we need to make sure we // don't randomly generate a real word. return randomFrom( "", - randomAlphaOfLength(4) + randomFrom('~', '*', '%', '$', '|') + randomAlphaOfLength(4) + randomAlphaOfLengthBetween(4, 8) + randomFrom('~', '*', '%', '$', '|') + randomAlphaOfLength(longPassword ? 100 : 4) ); }