From 9c76211393931756ef31d27dc90d502cfc573fc2 Mon Sep 17 00:00:00 2001 From: jaymode Date: Tue, 19 Jul 2016 14:23:57 -0400 Subject: [PATCH] security: do not use hidden filenames when generating certs This commit changes how we get the file and directory name for certificates in the tool. The tool now prompts the user for the filename. If the provided instance name will result in a valid filename, this is provided as a default. Otherwise the user must provide a valid filename. Closes elastic/elasticsearch#2854 Original commit: elastic/x-pack-elasticsearch@3c923d736b585be8f46d74b79b54d0d91b3eda7f --- .../xpack/security/ssl/CertificateTool.java | 56 ++++++++++--------- .../security/ssl/CertificateToolTests.java | 37 +++++++----- .../xpack/security/ssl/instances.yml | 3 + 3 files changed, 55 insertions(+), 41 deletions(-) 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"