solved issue "certutil: large passwords not set" #30944 #36689

MiguelFerreira1998 · 2018-12-17T02:23:50Z

Older versions of OpenSSL couldn't support passwords for a new Certificate Authority larger than 50 characters. I check the current version of the user and i prompt a warn message if they input a password longer than 50 and they are using OpenSLL under version 1.1.0

Fixed the issue about passwords longer than 50 not working on older versions of OpenSSL. From now on, users get an alert when they try to put a password with length above 50 and they have an OpenSSL version lower than 1.1.0, giving then the opportunity ti prompt one password again.

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

elasticmachine · 2018-12-17T02:47:22Z

Pinging @elastic/es-security

(warning for all versions)

tvernum

Hi @MiguelFerreira1998 and @bedgarone

Thanks for your contribution on this issue. I've left some suggestions.

tvernum · 2018-12-17T07:16:59Z

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

-            });
-        }
+                });
+            }


It looks like all of this section is whitespace changes (extra blank lines, additional indentation) that aren't needed.

tvernum · 2018-12-17T07:21:57Z

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

+                    return body.apply(promptedValue);
+                } finally {
+                    Arrays.fill(promptedValue, (char) 0);
+                }


The withPassword method is called every time we need a password, even if it's being used to read a certificate file.
In that case we don't want to print this warning, because that would cause additional output (and an additional prompt) for simple things like reading a CA file that has a long password.

We need to only perform this check/warning if the password is being applied to a new file.
I'm OK if we want get rid of the promptYesNo and just print out a warning, but we only want to do either of them when we know the password is being used to write a file.

tvernum · 2018-12-17T07:23:27Z

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

+                }
+                else{
+                    long_size = false;
+                }


The formatting of thise code (spaces around brackets etc) is different to the way the rest of this source code is formatted. By the time we get to the end of this review process, we'll want to have it formatted in the same style as the surrounding code.

MiguelFerreira1998 · 2018-12-17T10:29:56Z

Thanks @tvernum for all the support. I will be committing what i think its a fix.

tvernum

I left some more feedback.

tvernum · 2018-12-28T03:53:39Z

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

-                Arrays.fill(promptedValue, (char) 0);
+            while(long_size){
+                if(promptedValue.length > MAX_SIZE_PW_BUFFER_OLDER_SSL){
+                    if(terminal.promptYesNo("Your password exceeds a length of 50 characters. Versions of OpenSSL older than 1.1.0 will not be able to use this certificate. Do you want to continue?", true)){


Having through about this a bit more, I think the prompt is going to be annoying rather than helpful.
I think we'd be better off just printing out a warning message, and continuing on.

Sorry for messing things around like this, but sometimes things become clearer during the review cycle.

tvernum · 2018-12-28T03:54:23Z

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

@@ -381,7 +381,7 @@ CAInfo generateCA(Terminal terminal, OptionSet options) throws Exception {
                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",false, null, terminal, p -> new CAInfo(caCert, keyPair.getPrivate(), true, p.clone()));


This one should be true.
We want to check the password length any time we're generating a new private key.

tvernum · 2018-12-28T03:55:32Z

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

@@ -527,7 +527,8 @@ 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", false, info.password, terminal, caPassword -> {


I think this needs to be true as well.

tvernum · 2018-12-28T03:57:04Z

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

@@ -794,7 +797,8 @@ 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, false, outputPassword, terminal, password -> {


I think this one needs to be true

tvernum · 2018-12-28T04:00:20Z

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

@@ -923,15 +927,29 @@ private static PEMEncryptor getEncrypter(char[] password) {
        return new JcePEMEncryptorBuilder("DES-EDE3-CBC").setProvider(BC_PROV).build(password);
    }

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


Rather than long_size, it would be better to name this parameter something like forNewKey so it describes the logical purpose of the parameter.

tvernum · 2021-07-30T04:16:20Z

@elasticmachine test this please

tvernum · 2021-07-30T04:16:59Z

@ywangd Can you do a quick review of my additions to this PR?

tvernum · 2021-07-30T04:18:20Z

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

+            String wrongPassword = randomAlphaOfLengthBetween(8, 20);
+            terminal.addSecretInput(wrongPassword);
+            terminal.addSecretInput("__" + wrongPassword);
+        }


This isn't really related to this PR, but I noticed it was a gap in testing.

ywangd

LGTM. But the PR description seems to be off:

I check the current version of the user and i prompt a warn message if they input a password longer than 50 and they are using OpenSLL under version 1.1.0

Only the password length is checked and warning is issued regardless. There is no check for OpenSSL version and in fact it is not viable to do so. If my understand is correct, I suggest we update the description before merge.

ywangd · 2021-07-30T04:51:36Z

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

+                "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."


Suggested change

"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."

"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."

Why? They're constants, so we don't need the braces in order to see where the variable part is substituted in.

I thought the braces are for emphasis. But if they are just for variables, then you are right that we don't need them here.

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

ywangd · 2021-07-30T05:05:59Z

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

@@ -924,16 +929,43 @@ 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,
+    static boolean checkPasswordLengthForOpenSSLCompatibility(char[] password, Terminal terminal, boolean confirm) {


Nit:
This method and parameter names confused me for a bit. They made me think that the behaviour when confirm is false should be return false if password is too long. But instead the default behaviour is return true unless user says otherwise and confirm is to let user have that say. Maybe:

Suggested change

static boolean checkPasswordLengthForOpenSSLCompatibility(char[] password, Terminal terminal, boolean confirm) {

static boolean maybeWarnAndConfirmPasswordLengthForOpenSSLCompatibility(char[] password, Terminal terminal, boolean confirmRequired) {

It is quite verbose, but it feels more accurate to me. I am also happy if you can come up with something better.

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

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

tvernum · 2021-07-30T06:48:24Z

@elasticmachine test this please 🙏

tvernum · 2021-08-02T04:54:00Z

Resolves #30944

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: elastic#36689 Co-authored-by: MiguelFerreira1998 <[email protected]>

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 <[email protected]> Co-authored-by: MiguelFerreira1998 <[email protected]>

MiguelFerreira1998 added 3 commits December 13, 2018 19:48

Update CONTRIBUTING.md

9f2be48

mistake

c0943be

MiguelFerreira1998 closed this Dec 17, 2018

MiguelFerreira1998 reopened this Dec 17, 2018

tvernum reviewed Dec 17, 2018

View reviewed changes

.../plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java Outdated Show resolved Hide resolved

tvernum self-requested a review December 17, 2018 02:47

tvernum added >enhancement v7.0.0 :Security/TLS SSL/TLS, Certificates v6.7.0 labels Dec 17, 2018

Removed check OpenSSL version

de70a40

(warning for all versions)

tvernum reviewed Dec 17, 2018

View reviewed changes

Fixed prompt message

1da475e

tvernum reviewed Dec 28, 2018

View reviewed changes

jasontedor added v8.0.0 and removed v7.0.0 labels Feb 6, 2019

danielmitterdorfer added v7.2.0 and removed v6.7.0 labels Feb 7, 2019

jakelandis added v7.3.0 and removed v7.2.0 labels Jun 17, 2019

jpountz added v7.4.0 and removed v7.3.0 labels Jul 5, 2019

colings86 added v7.5.0 and removed v7.4.0 labels Aug 30, 2019

jimczi added v7.6.0 and removed v7.5.0 labels Nov 12, 2019

$@polyfractal$ polyfractal added the v7.7.0 label Jan 15, 2020

pugnascotia added v7.10.0 and removed v7.9.0 labels Jul 15, 2020

andreidan added v7.11.0 and removed v7.10.0 labels Oct 7, 2020

pugnascotia added v7.12.0 and removed v7.11.0 labels Dec 16, 2020

williamrandolph added v7.13.0 and removed v7.12.0 labels Feb 18, 2021

pugnascotia added v7.14.0 and removed v7.13.0 labels Apr 21, 2021

mark-vieira added v7.15.0 and removed v7.14.0 labels Jun 30, 2021

tvernum added 2 commits July 30, 2021 12:41

Merge branch 'master' into MiguelFerreira1998/master

2e58e7a

Rework checking of password length and add tests

6fe7e60

tvernum requested a review from ywangd July 30, 2021 04:17

tvernum reviewed Jul 30, 2021

View reviewed changes

ywangd approved these changes Jul 30, 2021

View reviewed changes

tvernum self-assigned this Jul 30, 2021

Address feedback

b885cae

tvernum merged commit 6ea0ca4 into elastic:master Aug 2, 2021

tvernum mentioned this pull request Aug 2, 2021

certutil: large passwords not set #30944

Closed

tvernum mentioned this pull request Aug 2, 2021

Issue warning in certutil when using long passwords #75915

Merged

mark-vieira added v8.0.0-alpha1 and removed v8.0.0 labels Aug 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

solved issue "certutil: large passwords not set" #30944 #36689

solved issue "certutil: large passwords not set" #30944 #36689

MiguelFerreira1998 commented Dec 17, 2018

elasticmachine commented Dec 17, 2018

tvernum left a comment

tvernum Dec 17, 2018

tvernum Dec 17, 2018

tvernum Dec 17, 2018

MiguelFerreira1998 commented Dec 17, 2018

tvernum left a comment

tvernum Dec 28, 2018

tvernum Dec 28, 2018

tvernum Dec 28, 2018

tvernum Dec 28, 2018

tvernum Dec 28, 2018

tvernum commented Jul 30, 2021

tvernum commented Jul 30, 2021

tvernum Jul 30, 2021

ywangd left a comment

ywangd Jul 30, 2021

tvernum Jul 30, 2021

ywangd Jul 30, 2021

ywangd Jul 30, 2021

tvernum commented Jul 30, 2021

tvernum commented Aug 2, 2021

	static boolean checkPasswordLengthForOpenSSLCompatibility(char[] password, Terminal terminal, boolean confirm) {
	static boolean maybeWarnAndConfirmPasswordLengthForOpenSSLCompatibility(char[] password, Terminal terminal, boolean confirmRequired) {

solved issue "certutil: large passwords not set" #30944 #36689

solved issue "certutil: large passwords not set" #30944 #36689

Conversation

MiguelFerreira1998 commented Dec 17, 2018

elasticmachine commented Dec 17, 2018

tvernum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelFerreira1998 commented Dec 17, 2018

tvernum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvernum commented Jul 30, 2021

tvernum commented Jul 30, 2021

Choose a reason for hiding this comment

ywangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvernum commented Jul 30, 2021

tvernum commented Aug 2, 2021