Fix certutil http for empty password with JDK 11 and lower #55437

ywangd · 2020-04-20T05:44:23Z

This PR fix elasticseaerch-certutil http command so that it correctly accepts empty keystore password for JDK version 11 and lower.

Resovles: #55386

elasticmachine · 2020-04-20T05:44:25Z

Pinging @elastic/es-security (:Security/Security)

ywangd · 2020-04-20T05:47:29Z

...in/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java

+        final String caPassword = randomFrom("", randomAlphaOfLengthBetween(4, 16));
+        final String node1Password = randomFrom("", randomAlphaOfLengthBetween(4, 16));
+        final String node2Password = randomFrom("", randomAlphaOfLengthBetween(4, 16));
+        final String node3Password = randomFrom("", randomAlphaOfLengthBetween(4, 16));


Commands other than http do not have issue with empty password because of the withPassword(...) wrapper. These empty strings are just added here for completeness.

elasticsearch/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java

Lines 928 to 940 in c5b923a

private static <T, E extends Exception> T withPassword(String description, char[] password, Terminal terminal,

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);

}

} else {

return body.apply(password);

}

}

ywangd · 2020-04-20T05:54:33Z

.../security/cli/src/main/java/org/elasticsearch/xpack/security/cli/HttpCertificateCommand.java

@@ -864,7 +864,7 @@ private boolean askCertSigningRequest(Terminal terminal) {
            terminal.println("IT IS IMPORTANT THAT YOU REMEMBER THIS PASSWORD AND KEEP IT SECURE");
            terminal.println("");
            final char[] password = readPassword(terminal, "CA password: ", true);
-            return new CertificateTool.CAInfo(caCert, keyPair.getPrivate(), true, password.length == 0 ? null : password);
+            return new CertificateTool.CAInfo(caCert, keyPair.getPrivate(), true, password);


This change is the fix. But is there any reason this was intentionally set to null for empty char array? I wonder whether the change would introduce any subtle side effect. @tvernum

Inspecting the uses of this returned CAInfo instance, I am pretty sure this change is not impactful.

However, looking at this other use of CAInfo, there seems to be a pattern where a null value is used to signal that a password prompt is further required. I believe the original code here intended for this same behavior.

ywangd · 2020-04-21T00:36:27Z

Btw, I checked all usages of PKCS12KeyStore#engineStore and this is the only place that could get a null password.

Tag #55386 here so the issue is correctly referenced here (forgot to do when creating the PR and subsequent editing does not count).

albertzaharovits

LGTM

I would've opted for parametrized test methods, to explicitly cover the empty password test cases, instead of relying on randomization for that. So, for example, there would be private void testGenerateSingleCertificateSigningRequest(boolean withEmptyPassword), together with two public test calls.
But I think this is acceptable nonetheless.

albertzaharovits · 2020-04-21T14:09:32Z

.../security/cli/src/main/java/org/elasticsearch/xpack/security/cli/HttpCertificateCommand.java

@@ -864,7 +864,7 @@ private boolean askCertSigningRequest(Terminal terminal) {
            terminal.println("IT IS IMPORTANT THAT YOU REMEMBER THIS PASSWORD AND KEEP IT SECURE");
            terminal.println("");
            final char[] password = readPassword(terminal, "CA password: ", true);
-            return new CertificateTool.CAInfo(caCert, keyPair.getPrivate(), true, password.length == 0 ? null : password);
+            return new CertificateTool.CAInfo(caCert, keyPair.getPrivate(), true, password);


Inspecting the uses of this returned CAInfo instance, I am pretty sure this change is not impactful.

However, looking at this other use of CAInfo, there seems to be a pattern where a null value is used to signal that a password prompt is further required. I believe the original code here intended for this same behavior.

albertzaharovits · 2020-04-21T14:33:25Z

...rity/cli/src/test/java/org/elasticsearch/xpack/security/cli/HttpCertificateCommandTests.java

+    private void runForNonEmptyPattern(String pattern, Runnable runnable) {
+        if ("".equals(pattern) == false) {
+            runnable.run();
+        }
    }


My preference would have been a simple if in the code.

Sure I removed it and replaced with if check in the call sites.

albertzaharovits · 2020-04-21T14:42:20Z

...rity/cli/src/test/java/org/elasticsearch/xpack/security/cli/HttpCertificateCommandTests.java


        assertThat(kibanaYml, containsString("elasticsearch.ssl.certificateAuthorities: [ \"config/elasticsearch-ca.pem\" ]"));
        assertThat(kibanaYml, containsString("https://"));
-        shouldNotContain.forEach(s -> assertThat(kibanaYml, not(containsString(s))));
+        shouldNotContain.forEach(s -> runForNonEmptyPattern(s, () -> assertThat(kibanaYml, not(containsString(s)))));
    }


I would've not included the empty password in the list of shouldNotContain, but it's just a personal preference.

Makes sense. The list is not filtered before passed in.

albertzaharovits · 2020-04-21T14:54:21Z

I would also suggest changing the title from Support empty p12 keystore password for JDK 11 and lower to Fix certutil http empty password for JDK 11 and lower as well as the description to better reflect what this PR does not so much of how.

…assword

ywangd · 2020-04-22T00:58:00Z

Thanks @albertzaharovits I have made changes based on your comments. One thing I didn't change is having empty password has part of randomPassword() call instead of creating a separate method to test for empty password. The main reason is that both ca password and node password can be empty. So it is easier to test with randomness for better coverage.

…5437) Fix elasticseaerch-certutil http command so that it correctly accepts empty keystore password with JDK version 11 and lower.

…55565) Fix elasticseaerch-certutil http command so that it correctly accepts empty keystore password with JDK version 11 and lower.

Support empty keystore password for JDK 11 and lower

0a4431d

ywangd added >bug :Security/Security Security issues without another label v8.0.0 v7.8.0 labels Apr 20, 2020

ywangd requested a review from albertzaharovits April 20, 2020 05:44

ywangd commented Apr 20, 2020

View reviewed changes

albertzaharovits approved these changes Apr 21, 2020

View reviewed changes

ywangd added 2 commits April 22, 2020 10:41

Address feedback.

ad06cd2

Merge remote-tracking branch 'origin/master' into es-55386-p12-null-p…

d0e7670

…assword

ywangd changed the title ~~Support empty p12 keystore password for JDK 11 and lower~~ Fix certutil http for empty password with JDK 11 and lower Apr 22, 2020

ywangd merged commit 00b4d3d into elastic:master Apr 22, 2020

ywangd mentioned this pull request Apr 22, 2020

PKCS12 keystore fails to create with empty password and JDK 11 or lower #55386

Closed

ywangd added a commit that referenced this pull request Apr 22, 2020

Fix certutil http for empty password with JDK 11 and lower (#55437) (#…

32e46bf

…55565) Fix elasticseaerch-certutil http command so that it correctly accepts empty keystore password with JDK version 11 and lower.

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix certutil http for empty password with JDK 11 and lower #55437

Fix certutil http for empty password with JDK 11 and lower #55437

ywangd commented Apr 20, 2020 •

edited

Loading

elasticmachine commented Apr 20, 2020

ywangd Apr 20, 2020

ywangd Apr 20, 2020 •

edited

Loading

albertzaharovits Apr 21, 2020

ywangd commented Apr 21, 2020

albertzaharovits left a comment

albertzaharovits Apr 21, 2020

albertzaharovits Apr 21, 2020

ywangd Apr 22, 2020 •

edited

Loading

albertzaharovits Apr 21, 2020

ywangd Apr 22, 2020

albertzaharovits commented Apr 21, 2020

ywangd commented Apr 22, 2020

	private static <T, E extends Exception> T withPassword(String description, char[] password, Terminal terminal,
	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);
	}
	} else {
	return body.apply(password);
	}
	}

Fix certutil http for empty password with JDK 11 and lower #55437

Fix certutil http for empty password with JDK 11 and lower #55437

Conversation

ywangd commented Apr 20, 2020 • edited Loading

elasticmachine commented Apr 20, 2020

ywangd Apr 20, 2020

Choose a reason for hiding this comment

ywangd Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

albertzaharovits Apr 21, 2020

Choose a reason for hiding this comment

ywangd commented Apr 21, 2020

albertzaharovits left a comment

Choose a reason for hiding this comment

albertzaharovits Apr 21, 2020

Choose a reason for hiding this comment

albertzaharovits Apr 21, 2020

Choose a reason for hiding this comment

ywangd Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

albertzaharovits Apr 21, 2020

Choose a reason for hiding this comment

ywangd Apr 22, 2020

Choose a reason for hiding this comment

albertzaharovits commented Apr 21, 2020

ywangd commented Apr 22, 2020

ywangd commented Apr 20, 2020 •

edited

Loading

ywangd Apr 20, 2020 •

edited

Loading

ywangd Apr 22, 2020 •

edited

Loading