Forráskód Böngészése

Better error message for invalid regex in index names (#91575)

When invalid regex is used in index names when defining privileges. The
error thrown by the underlying lucene atuomaton library does not include
information about the offending patterns. This PR improves the error
reporting by wrapping the exception and attaching the original patterns
associated to it.
Yang Wang 2 éve
szülő
commit
b3d7feb9a0

+ 16 - 5
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/StringMatcher.java

@@ -173,12 +173,23 @@ public class StringMatcher implements Predicate<String> {
                 return Automatons.predicate(patterns);
             } catch (TooComplexToDeterminizeException e) {
                 LOGGER.debug("Pattern automaton [{}] is too complex", patterns);
-                String description = Strings.collectionToCommaDelimitedString(patterns);
-                if (description.length() > 80) {
-                    description = Strings.cleanTruncate(description, 80) + "...";
-                }
-                throw new ElasticsearchSecurityException("The set of patterns [{}] is too complex to evaluate", e, description);
+                throw new ElasticsearchSecurityException(
+                    "The set of patterns [{}] is too complex to evaluate",
+                    e,
+                    getPatternsDescription(patterns)
+                );
+            } catch (IllegalArgumentException e) {
+                LOGGER.debug("Pattern automaton [{}] is invalid", patterns);
+                throw new ElasticsearchSecurityException("The set of patterns [{}] is invalid", e, getPatternsDescription(patterns));
+            }
+        }
+
+        private static String getPatternsDescription(Collection<String> patterns) {
+            String description = Strings.collectionToCommaDelimitedString(patterns);
+            if (description.length() > 80) {
+                description = Strings.cleanTruncate(description, 80) + "...";
             }
+            return description;
         }
     }
 }

+ 27 - 0
x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/support/StringMatcherTests.java

@@ -7,6 +7,8 @@
 
 package org.elasticsearch.xpack.core.security.support;
 
+import org.elasticsearch.ElasticsearchSecurityException;
+import org.elasticsearch.common.Strings;
 import org.elasticsearch.test.ESTestCase;
 
 import java.util.List;
@@ -15,7 +17,9 @@ import java.util.function.Supplier;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.isA;
 import static org.hamcrest.Matchers.sameInstance;
 
 public class StringMatcherTests extends ESTestCase {
@@ -180,6 +184,29 @@ public class StringMatcherTests extends ESTestCase {
         assertThat(m.toString(), equalTo(text2 + "|" + text3 + "|" + text4.substring(0, 59) + "...|" + text5.substring(0, 59) + "..."));
     }
 
+    public void testInvalidRegexPatterns() {
+        final List<String> invalidPatterns = randomNonEmptySubsetOf(
+            List.of(
+                "/~(([.]|ilm-history-).*/",  // missing closing bracket
+                "/~(([.]|ilm-history-).*", // missing ending slash,
+                "/[0-9/", // missing closing square bracket
+                "/a{0,3/", // missing closing curly bracket
+                "/[]/", // empty character class
+                "/a{}/" // empty number of occurrences
+            )
+        );
+        final ElasticsearchSecurityException e = expectThrows(
+            ElasticsearchSecurityException.class,
+            () -> StringMatcher.of(invalidPatterns)
+        );
+
+        assertThat(
+            e.getMessage(),
+            containsString("The set of patterns [" + Strings.collectionToCommaDelimitedString(invalidPatterns) + "] is invalid")
+        );
+        assertThat(e.getCause(), isA(IllegalArgumentException.class));
+    }
+
     private void assertMatch(StringMatcher matcher, String str) {
         if (matcher.test(str) == false) {
             fail(String.format(Locale.ROOT, "Matcher [%s] failed to match [%s] but should", matcher, str));

+ 27 - 0
x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java

@@ -9,6 +9,7 @@ package org.elasticsearch.xpack.security.authz.store;
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import org.elasticsearch.ElasticsearchSecurityException;
 import org.elasticsearch.Version;
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.action.admin.cluster.node.stats.NodesStatsAction;
@@ -441,6 +442,32 @@ public class CompositeRolesStoreTests extends ESTestCase {
         tryFailOnNonSuperuserRole(compositeRolesStore, throwableWithMessage(containsString("No privileges for you!")));
     }
 
+    public void testErrorForInvalidIndexNameRegex() {
+        final RoleDescriptor roleDescriptor = new RoleDescriptor(
+            "_mock_role",
+            null,
+            new IndicesPrivileges[] {
+                // invalid regex missing closing bracket
+                IndicesPrivileges.builder().indices("/~(([.]|ilm-history-).*/").privileges("read").build() },
+            null
+        );
+
+        final Consumer<ActionListener<RoleRetrievalResult>> rolesHandler = callback -> callback.onResponse(
+            RoleRetrievalResult.success(Set.of(roleDescriptor))
+        );
+        final Consumer<ActionListener<Collection<ApplicationPrivilegeDescriptor>>> privilegesHandler = callback -> callback.onResponse(
+            List.of()
+        );
+        final CompositeRolesStore compositeRolesStore = setupRolesStore(rolesHandler, privilegesHandler);
+
+        final PlainActionFuture<Role> future = new PlainActionFuture<>();
+        getRoleForRoleNames(compositeRolesStore, Set.of("_mock_role"), future);
+
+        final ElasticsearchSecurityException e = expectThrows(ElasticsearchSecurityException.class, future::actionGet);
+        assertThat(e.getMessage(), containsString("The set of patterns [/~(([.]|ilm-history-).*/] is invalid"));
+        assertThat(e.getCause().getClass(), is(IllegalArgumentException.class));
+    }
+
     private CompositeRolesStore setupRolesStore(
         Consumer<ActionListener<RoleRetrievalResult>> rolesHandler,
         Consumer<ActionListener<Collection<ApplicationPrivilegeDescriptor>>> privilegesHandler