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@3c923d736b

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<List<CertificateInformation>, CertInfoParseContext> PARSER = new ObjectParser<>("certgen");
     static {
         ConstructingObjectParser<CertificateInformation, CertInfoParseContext> instanceParser =
                 new ConstructingObjectParser<>("instances",
-                        a -> new CertificateInformation((String) a[0], (List<String>) a[1], (List<String>) a[2]));
+                        a -> new CertificateInformation((String) a[0], (String) (a[1] == null ? a[0] : a[1]),
+                                (List<String>) a[2], (List<String>) 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<String> ipList = Arrays.asList(Strings.splitStringByCommaToArray(ipAddresses));
                 List<String> dnsList = Arrays.asList(Strings.splitStringByCommaToArray(dnsNames));
 
-                CertificateInformation information = new CertificateInformation(name, ipList, dnsList);
+                CertificateInformation information = new CertificateInformation(name, filename, ipList, dnsList);
                 List<String> 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<String> ipAddresses;
         final List<String> dnsNames;
 
-        CertificateInformation(String name, List<String> ipAddresses, List<String> dnsNames) {
+        CertificateInformation(String name, String filename, List<String> ipAddresses, List<String> dnsNames) {
-            this.name = Name.fromUserProvidedName(name);
+            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));
+            boolean validFilename = isValidFilename(filename);
-            if (filename == null) {
+            if (validFilename == false) {
-                return new Name(name, principal, null, "could not convert [" + name + "] to a valid filename");
+                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) {
+        static boolean isValidFilename(String name) {
-            StringBuilder builder = new StringBuilder();
+            return ALLOWED_FILENAME_CHAR_PATTERN.matcher(Strings.cleanPath(name)).matches()
-            Matcher matcher = ALLOWED_FILENAME_CHAR_PATTERN.matcher(name);
+                    && name.startsWith(".") == false;
-            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;
         }
     }
 

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<String, Map<String, String>> 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);
+        assertThat(name.error, containsString("valid filename"));
-        assertEquals(userProvidedName.substring(0, CertificateTool.MAX_FILENAME_LENGTH), name.filename);
-        assertEquals("CN=" + userProvidedName, name.x500Principal.getName());
 
         // 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;
     }

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"