Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Aug 2, 2021
Merged

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

Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9f2be48
Update CONTRIBUTING.md
MiguelFerreira1998 Dec 13, 2018
51cfd77
fixed certutil: large passwords not set #30944
MiguelFerreira1998 Dec 16, 2018
c0943be
mistake
MiguelFerreira1998 Dec 17, 2018
de70a40
Removed check OpenSSL version
MiguelFerreira1998 Dec 17, 2018
1da475e
Fixed prompt message
MiguelFerreira1998 Dec 17, 2018
2e58e7a
Merge branch 'master' into MiguelFerreira1998/master
tvernum Jul 30, 2021
6fe7e60
Rework checking of password length and add tests
tvernum Jul 30, 2021
b885cae
Address feedback
tvernum Jul 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 38 additions & 20 deletions ...ugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ public class CertificateTool extends LoggingAwareMultiCommand {
private static final String DEFAULT_CA_ZIP = "elastic-stack-ca.zip";
private static final String DEFAULT_CA_P12 = "elastic-stack-ca.p12";
private static final BouncyCastleProvider BC_PROV = new BouncyCastleProvider();
private static final Integer MAX_SIZE_PW_BUFFER_OLDER_SSL = 50;

static final String DEFAULT_CERT_NAME = "instance";

Expand Down Expand Up @@ -336,8 +337,7 @@ 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,
Map<Certificate, Key> keys = withPassword("CA (" + path + ")", false, passwordOption,
terminal, password -> CertParsingUtils.readPkcs12KeyPairs(path, password, a -> password));

if (keys.size() != 1) {
Expand Down Expand Up @@ -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()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
return new CAInfo(caCert, keyPair.getPrivate(), true, null);
}
Expand Down Expand Up @@ -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 -> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be true as well.

writePkcs12(fileName, outputStream, "ca", info.certAndKey, null, caPassword, null);
return null;
});
Expand All @@ -546,17 +547,19 @@ 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 -> {
if (isAscii(p12Password)) {
pkcs12.setKeyEntry(alias, pair.key, p12Password, new Certificate[]{pair.cert});
if (caCert != null) {
pkcs12.setCertificateEntry("ca", caCert);


withPassword(fileName, true, password, terminal, p12Password -> {
if (isAscii(p12Password)) {
pkcs12.setKeyEntry(alias, pair.key, p12Password, new Certificate[]{pair.cert});
if (caCert != null) {
pkcs12.setCertificateEntry("ca", caCert);
}
pkcs12.store(output, p12Password);
return null;
} else {
throw new UserException(ExitCodes.CONFIG, "PKCS#12 passwords must be plain ASCII");
}
pkcs12.store(output, p12Password);
return null;
} else {
throw new UserException(ExitCodes.CONFIG, "PKCS#12 passwords must be plain ASCII");
}
});
}
}
Expand Down Expand Up @@ -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 -> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one needs to be true

pemWriter.writeObject(pair.key, getEncrypter(password));
return null;
});
Expand Down Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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(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)){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

long_size = false;

} else {
promptedValue = terminal.readSecret("Enter password for " + description + " : ");
if(promptedValue.length <= MAX_SIZE_PW_BUFFER_OLDER_SSL)
long_size = false;
}
} else {
long_size = false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
try {
return body.apply(promptedValue);
} finally {
Arrays.fill(promptedValue, (char) 0);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
return body.apply(password);
}
Expand Down