solved issue "certutil: large passwords not set" #30944 #36689
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -938,6 +938,9 @@ private char[] readPassword(Terminal terminal, String prompt, boolean confirm) { | |
| continue; | ||
| } | ||
| } | ||
| if (CertificateTool.checkPasswordLengthForOpenSSLCompatibility(password, terminal, confirm) == false) { | ||
| continue; | ||
| } | ||
| return password; | ||
| } else { | ||
| terminal.println(Terminal.Verbosity.SILENT, "Passwords must be plain ASCII"); | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -70,6 +71,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,8 +95,10 @@ | |
| import static org.hamcrest.Matchers.in; | ||
| import static org.hamcrest.Matchers.instanceOf; | ||
| import static org.hamcrest.Matchers.is; | ||
| import static org.hamcrest.Matchers.notNullValue; | ||
| import static org.hamcrest.Matchers.nullValue; | ||
| import static org.mockito.Matchers.any; | ||
| import static org.mockito.Matchers.eq; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
|
|
@@ -382,6 +386,72 @@ 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"); | ||
|
|
@@ -881,6 +951,44 @@ private static Path resolvePath(String path) { | |
| return PathUtils.get(path).toAbsolutePath(); | ||
| } | ||
|
|
||
| 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 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); | ||
|
|
||
| assertThat(caFile, pathExists()); | ||
tvernum marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| 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 +1015,4 @@ Path resolveOutputPath(Terminal terminal, OptionSet options, String defaultFilen | |
| return outFile; | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -141,7 +141,7 @@ public void testGenerateSingleCertificateSigningRequest() throws Exception { | |
|
|
||
| terminal.addTextInput(randomBoolean() ? "n" : ""); // don't change advanced settings | ||
|
|
||
| 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(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()); | ||
|
|
@@ -365,10 +368,20 @@ public void testGenerateMultipleCertificateWithNewCA() throws Exception { | |
| caKeySize = HttpCertificateCommand.DEFAULT_CA_KEY_SIZE; | ||
| } | ||
|
|
||
| final String caPassword = randomPassword(randomBoolean()); | ||
| // 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 | ||
| } | ||
| 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 +411,13 @@ public void testGenerateMultipleCertificateWithNewCA() throws Exception { | |
| terminal.addTextInput("n"); // no more certs | ||
|
|
||
|
|
||
| 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); | ||
| } | ||
|
There was a problem hiding this comment. This isn't really related to this PR, but I noticed it was a gap in testing. |
||
| terminal.addSecretInput(password); | ||
| if ("".equals(password) == false) { | ||
| terminal.addSecretInput(password); | ||
|
|
@@ -410,6 +429,9 @@ public void testGenerateMultipleCertificateWithNewCA() throws Exception { | |
| final OptionSet options = command.getParser().parse(new String[0]); | ||
| command.execute(terminal, options, env); | ||
|
|
||
| if (caPassword.length() > 50) { | ||
| assertThat(terminal.getOutput(), containsString("OpenSSL")); | ||
| } | ||
tvernum marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Path zipRoot = getZipRoot(outFile); | ||
|
|
||
| // Should have a CA directory with the generated CA. | ||
|
|
@@ -619,12 +641,12 @@ private List<String> randomHostNames() { | |
| return hostNames; | ||
| } | ||
|
|
||
| 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( | ||
| "", | ||
| randomAlphaOfLengthBetween(4, 8) + randomFrom('~', '*', '%', '$', '|') + randomAlphaOfLength(longPassword ? 100 : 4) | ||
| ); | ||
| } | ||
|
|
||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
This method and parameter names confused me for a bit. They made me think that the behaviour when
confirmisfalseshould bereturn falseif password is too long. But instead the default behaviour isreturn trueunless user says otherwise andconfirmis to let user have that say. Maybe:It is quite verbose, but it feels more accurate to me. I am also happy if you can come up with something better.