diff --git a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/ssl/CertificateTool.java b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/ssl/CertificateTool.java index df1d35b4717..43c19994bad 100644 --- a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/ssl/CertificateTool.java +++ b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/ssl/CertificateTool.java @@ -54,7 +54,6 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.atomic.AtomicReference; -import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.zip.ZipEntry; import java.util.zip.ZipOutputStream; @@ -68,16 +67,20 @@ public class CertificateTool extends SettingCommand { private static final String DESCRIPTION = "Simplifies certificate creation for use with the Elastic Stack"; private static final String DEFAULT_CSR_FILE = "csr-bundle.zip"; private static final String DEFAULT_CERT_FILE = "certificate-bundle.zip"; - private static final Pattern ALLOWED_FILENAME_CHAR_PATTERN = Pattern.compile("([a-zA-Z0-9!@#$%^&{}\\[\\]()_+\\-=,.~'` ]+)"); - private static final int DEFAULT_KEY_SIZE = 2048; private static final int FILE_EXTENSION_LENGTH = 4; static final int MAX_FILENAME_LENGTH = 255 - FILE_EXTENSION_LENGTH; + private static final Pattern ALLOWED_FILENAME_CHAR_PATTERN = + Pattern.compile("[a-zA-Z0-9!@#$%^&{}\\[\\]()_+\\-=,.~'` ]{1," + MAX_FILENAME_LENGTH + "}"); + private static final int DEFAULT_KEY_SIZE = 2048; + private static final ObjectParser, CertInfoParseContext> PARSER = new ObjectParser<>("certgen"); static { ConstructingObjectParser instanceParser = new ConstructingObjectParser<>("instances", - a -> new CertificateInformation((String) a[0], (List) a[1], (List) a[2])); + a -> new CertificateInformation((String) a[0], (String) (a[1] == null ? a[0] : a[1]), + (List) a[2], (List) a[3])); instanceParser.declareString(ConstructingObjectParser.constructorArg(), new ParseField("name")); + instanceParser.declareString(ConstructingObjectParser.optionalConstructorArg(), new ParseField("filename")); instanceParser.declareStringArray(ConstructingObjectParser.optionalConstructorArg(), new ParseField("ip")); instanceParser.declareStringArray(ConstructingObjectParser.optionalConstructorArg(), new ParseField("dns")); @@ -189,12 +192,18 @@ public class CertificateTool extends SettingCommand { while (done == false) { String name = terminal.readText("Enter instance name: "); if (name.isEmpty() == false) { + final boolean isNameValidFilename = Name.isValidFilename(name); + String filename = terminal.readText("Enter name for directories and files " + (isNameValidFilename ? "[" + name + "]" : "") + + ": " ); + if (filename.isEmpty() && isNameValidFilename) { + filename = name; + } String ipAddresses = terminal.readText("Enter IP Addresses for instance (comma-separated if more than one) []: "); String dnsNames = terminal.readText("Enter DNS names for instance (comma-separated if more than one) []: "); List ipList = Arrays.asList(Strings.splitStringByCommaToArray(ipAddresses)); List dnsList = Arrays.asList(Strings.splitStringByCommaToArray(dnsNames)); - CertificateInformation information = new CertificateInformation(name, ipList, dnsList); + CertificateInformation information = new CertificateInformation(name, filename, ipList, dnsList); List validationErrors = information.validate(); if (validationErrors.isEmpty()) { if (map.containsKey(name)) { @@ -415,6 +424,11 @@ public class CertificateTool extends SettingCommand { terminal.println(" * The minimum required value for each instance is a name. This can simply be the"); terminal.println(" hostname, which will be used as the Common Name of the certificate. A full"); terminal.println(" distinguished name may also be used."); + terminal.println(" * A filename value may be required for each instance. This is necessary when the"); + terminal.println(" name would result in an invalid file or directory name. The name provided here"); + terminal.println(" is used as the directory name (within the zip) and the prefix for the key and"); + terminal.println(" certificate files. The filename is required if you are prompted and the name"); + terminal.println(" is not displayed in the prompt."); terminal.println(" * IP addresses and DNS names are optional. Multiple values can be specified as a"); terminal.println(" comma separated string. If no IP addresses or DNS names are provided, you may"); terminal.println(" disable hostname verification in your SSL configuration."); @@ -507,8 +521,8 @@ public class CertificateTool extends SettingCommand { final List ipAddresses; final List dnsNames; - CertificateInformation(String name, List ipAddresses, List dnsNames) { - this.name = Name.fromUserProvidedName(name); + CertificateInformation(String name, String filename, List ipAddresses, List dnsNames) { + this.name = Name.fromUserProvidedName(name, filename); this.ipAddresses = ipAddresses == null ? Collections.emptyList() : ipAddresses; this.dnsNames = dnsNames == null ? Collections.emptyList() : dnsNames; } @@ -546,7 +560,7 @@ public class CertificateTool extends SettingCommand { this.error = error; } - static Name fromUserProvidedName(String name) { + static Name fromUserProvidedName(String name, String filename) { if ("ca".equals(name)) { return new Name(name, null, null, "[ca] may not be used as an instance name"); } @@ -564,28 +578,16 @@ public class CertificateTool extends SettingCommand { return new Name(name, null, null, error); } - String filename = attemptToConvertFilename(Strings.cleanPath(name)); - if (filename == null) { - return new Name(name, principal, null, "could not convert [" + name + "] to a valid filename"); + boolean validFilename = isValidFilename(filename); + if (validFilename == false) { + return new Name(name, principal, null, "[" + filename + "] is not a valid filename"); } - return new Name(name, principal, filename, null); + return new Name(name, principal, Strings.cleanPath(filename), null); } - static String attemptToConvertFilename(String name) { - StringBuilder builder = new StringBuilder(); - Matcher matcher = ALLOWED_FILENAME_CHAR_PATTERN.matcher(name); - while (matcher.find()) { - builder.append(matcher.group(1)); - } - - if (builder.length() > MAX_FILENAME_LENGTH) { - return builder.substring(0, MAX_FILENAME_LENGTH); - } - - if (builder.length() > 0) { - return builder.toString(); - } - return null; + static boolean isValidFilename(String name) { + return ALLOWED_FILENAME_CHAR_PATTERN.matcher(Strings.cleanPath(name)).matches() + && name.startsWith(".") == false; } } diff --git a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/ssl/CertificateToolTests.java b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/ssl/CertificateToolTests.java index 6225d70d510..4ae4aabc22e 100644 --- a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/ssl/CertificateToolTests.java +++ b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/ssl/CertificateToolTests.java @@ -106,6 +106,7 @@ public class CertificateToolTests extends ESTestCase { MockTerminal terminal = new MockTerminal(); for (Entry> entry : instanceInput.entrySet()) { terminal.addTextInput(entry.getKey()); + terminal.addTextInput(""); terminal.addTextInput(entry.getValue().get("ip")); terminal.addTextInput(entry.getValue().get("dns")); count++; @@ -145,18 +146,22 @@ public class CertificateToolTests extends ESTestCase { CertificateInformation certInfo = certInfosMap.get("node1"); assertEquals(Collections.singletonList("127.0.0.1"), certInfo.ipAddresses); assertEquals(Collections.singletonList("localhost"), certInfo.dnsNames); + assertEquals("node1", certInfo.name.filename); certInfo = certInfosMap.get("node2"); assertEquals(Collections.singletonList("::1"), certInfo.ipAddresses); assertEquals(Collections.emptyList(), certInfo.dnsNames); + assertEquals("node2", certInfo.name.filename); certInfo = certInfosMap.get("node3"); assertEquals(Collections.emptyList(), certInfo.ipAddresses); assertEquals(Collections.emptyList(), certInfo.dnsNames); + assertEquals("node3", certInfo.name.filename); certInfo = certInfosMap.get("CN=different value"); assertEquals(Collections.emptyList(), certInfo.ipAddresses); assertEquals(Collections.singletonList("node4.mydomain.com"), certInfo.dnsNames); + assertEquals("different file", certInfo.name.filename); } public void testGeneratingCsr() throws Exception { @@ -309,22 +314,20 @@ public class CertificateToolTests extends ESTestCase { public void testNameValues() throws Exception { // good name - Name name = Name.fromUserProvidedName("my instance"); + Name name = Name.fromUserProvidedName("my instance", "my instance"); assertEquals("my instance", name.originalName); assertNull(name.error); assertEquals("CN=my instance", name.x500Principal.getName()); - assertEquals("my instance", name.originalName); + assertEquals("my instance", name.filename); // too long String userProvidedName = randomAsciiOfLength(CertificateTool.MAX_FILENAME_LENGTH + 1); - name = Name.fromUserProvidedName(userProvidedName); + name = Name.fromUserProvidedName(userProvidedName, userProvidedName); assertEquals(userProvidedName, name.originalName); - assertNull(name.error); - assertEquals(userProvidedName.substring(0, CertificateTool.MAX_FILENAME_LENGTH), name.filename); - assertEquals("CN=" + userProvidedName, name.x500Principal.getName()); + assertThat(name.error, containsString("valid filename")); // too short - name = Name.fromUserProvidedName(""); + name = Name.fromUserProvidedName("", ""); assertEquals("", name.originalName); assertThat(name.error, containsString("valid filename")); assertEquals("CN=", name.x500Principal.getName()); @@ -332,7 +335,7 @@ public class CertificateToolTests extends ESTestCase { // invalid characters only userProvidedName = "<>|<>*|?\"\\"; - name = Name.fromUserProvidedName(userProvidedName); + name = Name.fromUserProvidedName(userProvidedName, userProvidedName); assertEquals(userProvidedName, name.originalName); assertThat(name.error, containsString("valid DN")); assertNull(name.x500Principal); @@ -340,19 +343,25 @@ public class CertificateToolTests extends ESTestCase { // invalid for file but DN ok userProvidedName = "*"; - name = Name.fromUserProvidedName(userProvidedName); + name = Name.fromUserProvidedName(userProvidedName, userProvidedName); assertEquals(userProvidedName, name.originalName); assertThat(name.error, containsString("valid filename")); assertEquals("CN=" + userProvidedName, name.x500Principal.getName()); assertNull(name.filename); - // invalid with valid chars + // invalid with valid chars for filename userProvidedName = "*.mydomain.com"; - name = Name.fromUserProvidedName(userProvidedName); + name = Name.fromUserProvidedName(userProvidedName, userProvidedName); assertEquals(userProvidedName, name.originalName); - assertNull(name.error); + assertThat(name.error, containsString("valid filename")); + assertEquals("CN=" + userProvidedName, name.x500Principal.getName()); + + // valid but could create hidden file/dir so it is not allowed + userProvidedName = ".mydomain.com"; + name = Name.fromUserProvidedName(userProvidedName, userProvidedName); + assertEquals(userProvidedName, name.originalName); + assertThat(name.error, containsString("valid filename")); assertEquals("CN=" + userProvidedName, name.x500Principal.getName()); - assertEquals(".mydomain.com", name.filename); } private PKCS10CertificationRequest readCertificateRequest(Path path) throws Exception { @@ -399,7 +408,7 @@ public class CertificateToolTests extends ESTestCase { boolean valid; do { name = randomAsciiOfLengthBetween(1, 32); - valid = Name.fromUserProvidedName(name).error == null; + valid = Name.fromUserProvidedName(name, name).error == null; } while (valid == false); return name; } diff --git a/elasticsearch/x-pack/security/src/test/resources/org/elasticsearch/xpack/security/ssl/instances.yml b/elasticsearch/x-pack/security/src/test/resources/org/elasticsearch/xpack/security/ssl/instances.yml index 0324743ebb6..c6ad9682079 100644 --- a/elasticsearch/x-pack/security/src/test/resources/org/elasticsearch/xpack/security/ssl/instances.yml +++ b/elasticsearch/x-pack/security/src/test/resources/org/elasticsearch/xpack/security/ssl/instances.yml @@ -4,8 +4,11 @@ instances: - "127.0.0.1" dns: "localhost" - name: "node2" + filename: "node2" ip: "::1" - name: "node3" + filename: "node3" - name: "CN=different value" + filename: "different file" dns: - "node4.mydomain.com"