Support wider range of application names (#31752)

tvernum · web-flow · commit 4e1031b77620 · 2018-07-05T16:59:08.000+10:00
This extends the validation for application names to allow an optional
suffix of "-" (or "_") followed by any number of "filename safe
characters" (excluding '*')

The purpose of this is to support multiple kibana instances against a
single ES cluster where the name of each kibana application is
"kibana-${kibana-index}", assuming some reasonable limits on the
Kibana index name.

The change also retricts the wildcard handling of application names
to only support a trailing wildcard: e.g `*`, `kibana-*`, etc.
diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/ApplicationPrivilege.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/ApplicationPrivilege.java
@@ -29,8 +29,8 @@
  */
 public final class ApplicationPrivilege extends Privilege {
 
-    private static final Pattern VALID_APPLICATION = Pattern.compile("^[a-z][A-Za-z0-9_-]{2,}$");
-    private static final Pattern VALID_APPLICATION_OR_WILDCARD = Pattern.compile("^[a-z*][A-Za-z0-9_*-]*");
+    private static final Pattern VALID_APPLICATION_PREFIX = Pattern.compile("^[a-z][A-Za-z0-9]*$");
+    private static final Pattern WHITESPACE = Pattern.compile("[\\v\\h]");
     private static final Pattern VALID_NAME = Pattern.compile("^[a-z][a-zA-Z0-9_.-]*$");
 
     /**
@@ -69,7 +69,7 @@ String[] getPatterns() {
      * @throws IllegalArgumentException if the name is not valid
      */
     public static void validateApplicationName(String application) {
-        validateApplicationName(application, VALID_APPLICATION);
+        validateApplicationName(application, false);
     }
 
     /**
@@ -78,13 +78,61 @@ public static void validateApplicationName(String application) {
      * @throws IllegalArgumentException if the name is not valid
      */
     public static void validateApplicationNameOrWildcard(String application) {
-        validateApplicationName(application, VALID_APPLICATION_OR_WILDCARD);
+        validateApplicationName(application, true);
     }
 
-    private static void validateApplicationName(String application, Pattern pattern) {
-        if (pattern.matcher(application).matches() == false) {
-            throw new IllegalArgumentException("Application names must match the pattern " + pattern.pattern()
-                + " (but was '" + application + "')");
+    /**
+     * Validates that an application name matches the following rules:
+     * - consist of a "prefix", optionally followed by either "-" or "_" and a suffix
+     * - the prefix must begin with a lowercase ASCII letter
+     * - the prefix only contain ASCII letter or digits
+     * - the prefix must be at least 3 characters long
+     * - the suffix must only contain {@link Strings#validFileName valid filename} characters
+     * - no part of the name may contain whitespace
+     * If {@code allowWildcard} is true, then the names that end with a '*', and would match a valid
+     * application name are also accepted.
+     */
+    private static void validateApplicationName(String application, boolean allowWildcard) {
+        if (Strings.isEmpty(application)) {
+            throw new IllegalArgumentException("Application names cannot be blank");
+        }
+        final int asterisk = application.indexOf('*');
+        if (asterisk != -1) {
+            if (allowWildcard == false) {
+                throw new IllegalArgumentException("Application names may not contain '*' (found '" + application + "')");
+            }
+            if(application.equals("*")) {
+                // this is allowed and short-circuiting here makes the later validation simpler
+                return;
+            }
+            if (asterisk != application.length() - 1) {
+                throw new IllegalArgumentException("Application name patterns only support trailing wildcards (found '" + application
+                    + "')");
+            }
+        }
+        if (WHITESPACE.matcher(application).find()) {
+            throw new IllegalArgumentException("Application names may not contain whitespace (found '" + application + "')");
+        }
+
+        final String[] parts = application.split("[_-]", 2);
+        String prefix = parts[0];
+        if (prefix.endsWith("*")) {
+            prefix = prefix.substring(0, prefix.length() - 1);
+        }
+        if (VALID_APPLICATION_PREFIX.matcher(prefix).matches() == false) {
+            throw new IllegalArgumentException("An application name prefix must match the pattern " + VALID_APPLICATION_PREFIX.pattern()
+                + " (found '" + prefix + "')");
+        }
+        if (prefix.length() < 3 && asterisk == -1) {
+            throw new IllegalArgumentException("An application name prefix must be at least 3 characters long (found '" + prefix + "')");
+        }
+
+        if (parts.length > 1) {
+            final String suffix = parts[1];
+            if (Strings.validFileName(suffix) == false) {
+                throw new IllegalArgumentException("An application name suffix may not contain any of the characters '" +
+                    Strings.collectionToDelimitedString(Strings.INVALID_FILENAME_CHARS, "") + "' (found '" + suffix + "')");
+            }
         }
     }
 
diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/privilege/PutPrivilegesRequestTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/privilege/PutPrivilegesRequestTests.java
@@ -49,7 +49,7 @@ public void testSerialization() throws IOException {
     public void testValidation() {
         // wildcard app name
         final ApplicationPrivilegeDescriptor wildcardApp = descriptor("*", "all", "*");
-        assertValidationFailure(request(wildcardApp), "Application names must match");
+        assertValidationFailure(request(wildcardApp), "Application names may not contain");
 
         // invalid priv names
         final ApplicationPrivilegeDescriptor spaceName = descriptor("app", "r e a d", "read/*");
@@ -68,7 +68,8 @@ public void testValidation() {
 
         // mixed
         assertValidationFailure(request(wildcardApp, numericName, reservedMetadata, badAction),
-            "Application names must match", "Application privilege names must match", "metadata keys may not start", "must contain one of");
+            "Application names may not contain", "Application privilege names must match", "metadata keys may not start",
+            "must contain one of");
     }
 
     private ApplicationPrivilegeDescriptor descriptor(String application, String name, String... actions) {
diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/role/PutRoleRequestTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/role/PutRoleRequestTests.java
@@ -35,9 +35,9 @@ public void testValidationOfApplicationPrivileges() {
         // Fail
         assertValidationError("privilege names and actions must match the pattern",
             buildRequestWithApplicationPrivilege("app", new String[]{"in valid"}, new String[]{"*"}));
-        assertValidationError("Application names must match the pattern",
+        assertValidationError("An application name prefix must match the pattern",
             buildRequestWithApplicationPrivilege("000", new String[]{"all"}, new String[]{"*"}));
-        assertValidationError("Application names must match the pattern",
+        assertValidationError("An application name prefix must match the pattern",
             buildRequestWithApplicationPrivilege("%*", new String[]{"all"}, new String[]{"*"}));
     }
 
diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/user/HasPrivilegesRequestTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/user/HasPrivilegesRequestTests.java
@@ -76,8 +76,7 @@ public void testValidateNoWildcardApplicationPrivileges() {
         });
         final ActionRequestValidationException exception = request.validate();
         assertThat(exception, notNullValue());
-        assertThat(exception.validationErrors(), hasItem("Application names must match the pattern ^[a-z][A-Za-z0-9_-]{2,}$" +
-            " (but was '*')"));
+        assertThat(exception.validationErrors(), hasItem("Application names may not contain '*' (found '*')"));
     }
 
     private HasPrivilegesRequest serializeAndDeserialize(HasPrivilegesRequest original, Version version) throws IOException {
diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/ApplicationPrivilegeTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/ApplicationPrivilegeTests.java
@@ -5,6 +5,7 @@
  */
 package org.elasticsearch.xpack.core.security.authz.privilege;
 
+import junit.framework.AssertionFailedError;
 import org.apache.lucene.util.automaton.CharacterRunAutomaton;
 import org.elasticsearch.common.util.set.Sets;
 import org.elasticsearch.test.ESTestCase;
@@ -17,6 +18,7 @@
 import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
+import java.util.function.Supplier;
 
 import static org.elasticsearch.common.Strings.collectionToCommaDelimitedString;
 import static org.hamcrest.Matchers.arrayContainingInAnyOrder;
@@ -27,62 +29,61 @@
 public class ApplicationPrivilegeTests extends ESTestCase {
 
     public void testValidationOfApplicationName() {
-        // too short
-        assertValidationFailure("Application names", () -> ApplicationPrivilege.validateApplicationName("ap"));
-        // must start with lowercase
-        assertValidationFailure("Application names", () -> ApplicationPrivilege.validateApplicationName("App"));
-        // must start with letter
-        assertValidationFailure("Application names", () -> ApplicationPrivilege.validateApplicationName("1app"));
-        // cannot contain special characters
-        assertValidationFailure("Application names",
-            () -> ApplicationPrivilege.validateApplicationName("app" + randomFrom(":;$#%()+=/'.,".toCharArray())));
+        final String specialCharacters = ":;$#%()+='.{}[]!@^&'";
+        final Supplier<Character> specialCharacter = () -> specialCharacters.charAt(randomInt(specialCharacters.length() - 1));
+
+        assertValidationFailure("a p p", "application name", () -> ApplicationPrivilege.validateApplicationName("a p p"));
+        assertValidationFailure("ap", "application name", () -> ApplicationPrivilege.validateApplicationName("ap"));
+        for (String app : Arrays.asList(
+            "App",// must start with lowercase
+            "1app",  // must start with letter
+            "app" + specialCharacter.get() // cannot contain special characters unless preceded by a "-" or "_"
+        )) {
+            assertValidationFailure(app, "application name", () -> ApplicationPrivilege.validateApplicationName(app));
+            assertValidationFailure(app, "application name", () -> ApplicationPrivilege.validateApplicationNameOrWildcard(app));
+        }
 
         // no wildcards
-        assertValidationFailure("Application names", () -> ApplicationPrivilege.validateApplicationName("app*"));
+        assertValidationFailure("app*", "application names", () -> ApplicationPrivilege.validateApplicationName("app*"));
         // no special characters with wildcards
-        assertValidationFailure("Application names",
-            () -> ApplicationPrivilege.validateApplicationNameOrWildcard("app" + randomFrom((":;$#%()+=/'.,").toCharArray()) + "*"));
+        final String appNameWithSpecialCharAndWildcard = "app" + specialCharacter.get() + "*";
+        assertValidationFailure(appNameWithSpecialCharAndWildcard, "application name",
+            () -> ApplicationPrivilege.validateApplicationNameOrWildcard(appNameWithSpecialCharAndWildcard));
 
+        String appNameWithSpecialChars = "myapp" + randomFrom('-', '_');
+        for (int i = randomIntBetween(1, 12); i > 0; i--) {
+            appNameWithSpecialChars = appNameWithSpecialChars + specialCharacter.get();
+        }
         // these should all be OK
-        assertNoException(() -> ApplicationPrivilege.validateApplicationName("app"));
-        assertNoException(() -> ApplicationPrivilege.validateApplicationName("app1"));
-        assertNoException(() -> ApplicationPrivilege.validateApplicationName("myApp"));
-        assertNoException(() -> ApplicationPrivilege.validateApplicationName("my-App"));
-        assertNoException(() -> ApplicationPrivilege.validateApplicationName("my_App"));
-        assertNoException(() -> ApplicationPrivilege.validateApplicationNameOrWildcard("app*"));
+        for (String app : Arrays.asList("app", "app1", "myApp", "myApp-:;$#%()+='.", "myApp_:;$#%()+='.", appNameWithSpecialChars)) {
+            assertNoException(app, () -> ApplicationPrivilege.validateApplicationName(app));
+            assertNoException(app, () -> ApplicationPrivilege.validateApplicationNameOrWildcard(app));
+        }
     }
 
     public void testValidationOfPrivilegeName() {
         // must start with lowercase
-        assertValidationFailure("privilege names", () -> ApplicationPrivilege.validatePrivilegeName("Read"));
+        assertValidationFailure("Read", "privilege names", () -> ApplicationPrivilege.validatePrivilegeName("Read"));
         // must start with letter
-        assertValidationFailure("privilege names", () -> ApplicationPrivilege.validatePrivilegeName("1read"));
+        assertValidationFailure("1read", "privilege names", () -> ApplicationPrivilege.validatePrivilegeName("1read"));
         // cannot contain special characters
-        final String withSpecialChar = "read" + randomFrom(":;$#%()+=/',".toCharArray());
-        assertValidationFailure("privilege names", () -> ApplicationPrivilege.validatePrivilegeName(withSpecialChar));
+        final String specialChars = ":;$#%()+=/',";
+        final String withSpecialChar = "read" + specialChars.charAt(randomInt(specialChars.length()-1));
+        assertValidationFailure(withSpecialChar, "privilege names", () -> ApplicationPrivilege.validatePrivilegeName(withSpecialChar));
 
         // these should all be OK
-        assertNoException(() -> ApplicationPrivilege.validatePrivilegeName("read"));
-        assertNoException(() -> ApplicationPrivilege.validatePrivilegeName("read1"));
-        assertNoException(() -> ApplicationPrivilege.validatePrivilegeName("readData"));
-        assertNoException(() -> ApplicationPrivilege.validatePrivilegeName("read-data"));
-        assertNoException(() -> ApplicationPrivilege.validatePrivilegeName("read.data"));
-        assertNoException(() -> ApplicationPrivilege.validatePrivilegeName("read_data"));
-
-        assertValidationFailure("privilege names and action", () -> ApplicationPrivilege.validatePrivilegeOrActionName("r e a d"));
-        assertValidationFailure("privilege names and action", () -> ApplicationPrivilege.validatePrivilegeOrActionName("read\n"));
-        assertValidationFailure("privilege names and action", () -> ApplicationPrivilege.validatePrivilegeOrActionName("copy®"));
-
-        assertNoException(() -> ApplicationPrivilege.validatePrivilegeOrActionName("read"));
-        assertNoException(() -> ApplicationPrivilege.validatePrivilegeOrActionName("read1"));
-        assertNoException(() -> ApplicationPrivilege.validatePrivilegeOrActionName("readData"));
-        assertNoException(() -> ApplicationPrivilege.validatePrivilegeOrActionName("read-data"));
-        assertNoException(() -> ApplicationPrivilege.validatePrivilegeOrActionName("read.data"));
-        assertNoException(() -> ApplicationPrivilege.validatePrivilegeOrActionName("read_data"));
-        assertNoException(() -> ApplicationPrivilege.validatePrivilegeOrActionName("read:*"));
-        assertNoException(() -> ApplicationPrivilege.validatePrivilegeOrActionName("read/*"));
-        assertNoException(() -> ApplicationPrivilege.validatePrivilegeOrActionName("read/a_b.c-d+e%f#(g)"));
+        for (String priv : Arrays.asList("read", "read1", "readData", "read-data", "read.data", "read_data")) {
+            assertNoException(priv, () -> ApplicationPrivilege.validatePrivilegeName(priv));
+            assertNoException(priv, () -> ApplicationPrivilege.validatePrivilegeOrActionName(priv));
+        }
+
+        for (String priv : Arrays.asList("r e a d", "read\n", "copy®")) {
+            assertValidationFailure(priv, "privilege names and action", () -> ApplicationPrivilege.validatePrivilegeOrActionName(priv));
+        }
 
+        for (String priv : Arrays.asList("read:*", "read/*", "read/a_b.c-d+e%f#(g)")) {
+            assertNoException(priv, () -> ApplicationPrivilege.validatePrivilegeOrActionName(priv));
+        }
     }
 
     public void testGetPrivilegeByName() {
@@ -144,17 +145,22 @@ private String getPrivilegeName(ApplicationPrivilege privilege) {
         }
     }
 
-    private void assertValidationFailure(String messageContent, ThrowingRunnable body) {
-        final IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, body);
-        assertThat(exception.getMessage(), containsString(messageContent));
+    private void assertValidationFailure(String reason,String messageContent, ThrowingRunnable body) {
+        final IllegalArgumentException exception;
+        try {
+            exception = expectThrows(IllegalArgumentException.class, body);
+            assertThat(exception.getMessage().toLowerCase(Locale.ROOT), containsString(messageContent.toLowerCase(Locale.ROOT)));
+        } catch (AssertionFailedError e) {
+            fail(reason + " - " + e.getMessage());
+        }
     }
 
-    private void assertNoException(ThrowingRunnable body) {
+    private void assertNoException(String reason, ThrowingRunnable body) {
         try {
             body.run();
             // pass
         } catch (Throwable e) {
-            Assert.fail("Expected no exception, but got: " + e);
+            Assert.fail(reason + " - Expected no exception, but got: " + e);
         }
     }
 

Original file line number	Diff line number	Diff line change
`@@ -76,8 +76,7 @@ public void testValidateNoWildcardApplicationPrivileges() {`
`76`	`76`	`});`
`77`	`77`	`final ActionRequestValidationException exception = request.validate();`
`78`	`78`	`assertThat(exception, notNullValue());`
`79`		`- assertThat(exception.validationErrors(), hasItem("Application names must match the pattern ^[a-z][A-Za-z0-9_-]{2,}$" +`
`80`		`- " (but was '*')"));`
	`79`	`+ assertThat(exception.validationErrors(), hasItem("Application names may not contain '' (found '')"));`
`81`	`80`	`}`
`82`	`81`
`83`	`82`	`private HasPrivilegesRequest serializeAndDeserialize(HasPrivilegesRequest original, Version version) throws IOException {`