Browse Source

Enable security automaton caching (#34028)

Building automatons can be costly. For the most part we cache things
that use automatons so the cost is limited.
However:
- We don't (currently) do that everywhere (e.g. we don't cache role
  mappings)
- It is sometimes necessary to clear some of those caches which can
  cause significant CPU overhead and processing delays.

This commit introduces a new cache in the Automatons class to avoid
unnecesarily recomputing automatons.
Tim Vernum 7 years ago
parent
commit
6608992523

+ 28 - 0
docs/reference/settings/security-settings.asciidoc

@@ -55,6 +55,7 @@ Enables fips mode of operation. Set this to `true` if you run this {es} instance
 `xpack.security.authc.accept_default_password`::
 In `elasticsearch.yml`, set this to `false` to disable support for the default "changeme" password.
 
+[float]
 [[password-hashing-settings]]
 ==== Password hashing settings
 `xpack.security.authc.password_hashing.algorithm`::
@@ -82,6 +83,33 @@ resource. When set to `false`, an HTTP 401 response is returned and the user
 can provide credentials with the appropriate permissions to gain
 access. Defaults to `true`.
 
+[float]
+[[security-automata-settings]]
+==== Automata Settings
+In places where {security} accepts wildcard patterns (e.g. index patterns in
+roles, group matches in the role mapping API), each pattern is compiled into
+an Automaton. The follow settings are available to control this behaviour.
+
+`xpack.security.automata.max_determinized_states`::
+The upper limit on how many automaton states may be created by a single pattern.
+This protects against too-difficult (e.g. exponentially hard) patterns.
+Defaults to `100,000`.
+
+`xpack.security.automata.cache.enabled`::
+Whether to cache the compiled automata. Compiling automata can be CPU intensive
+and may slowdown some operations. The cache reduces the frequency with which
+automata need to be compiled.
+Defaults to `true`.
+
+`xpack.security.automata.cache.size`::
+The maximum number of items to retain in the automata cache.
+Defaults to `10,000`.
+
+`xpack.security.automata.cache.ttl`::
+The length of time to retain in an item in the automata cache (based on most
+recent usage).
+Defaults to `48h` (48 hours).
+
 [float]
 [[field-document-security-settings]]
 ==== Document and field level security settings

+ 72 - 7
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/Automatons.java

@@ -9,13 +9,18 @@ import org.apache.lucene.util.automaton.Automata;
 import org.apache.lucene.util.automaton.Automaton;
 import org.apache.lucene.util.automaton.CharacterRunAutomaton;
 import org.apache.lucene.util.automaton.RegExp;
+import org.elasticsearch.common.cache.Cache;
+import org.elasticsearch.common.cache.CacheBuilder;
 import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.unit.TimeValue;
+import org.elasticsearch.common.util.set.Sets;
 
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.List;
+import java.util.concurrent.ExecutionException;
 import java.util.function.Predicate;
 
 import static org.apache.lucene.util.automaton.MinimizationOperations.minimize;
@@ -27,14 +32,23 @@ import static org.elasticsearch.common.Strings.collectionToDelimitedString;
 
 public final class Automatons {
 
-    public static final Setting<Integer> MAX_DETERMINIZED_STATES_SETTING =
+    static final Setting<Integer> MAX_DETERMINIZED_STATES_SETTING =
         Setting.intSetting("xpack.security.automata.max_determinized_states", 100000, DEFAULT_MAX_DETERMINIZED_STATES,
             Setting.Property.NodeScope);
+
+    static final Setting<Boolean> CACHE_ENABLED =
+        Setting.boolSetting("xpack.security.automata.cache.enabled", true, Setting.Property.NodeScope);
+    static final Setting<Integer> CACHE_SIZE =
+        Setting.intSetting("xpack.security.automata.cache.size", 10_000, Setting.Property.NodeScope);
+    static final Setting<TimeValue> CACHE_TTL =
+        Setting.timeSetting("xpack.security.automata.cache.ttl", TimeValue.timeValueHours(48), Setting.Property.NodeScope);
+
     public static final Automaton EMPTY = Automata.makeEmpty();
     public static final Automaton MATCH_ALL = Automata.makeAnyString();
 
-    // this value is not final since we allow it to be set at runtime
+    // these values are not final since we allow them to be set at runtime
     private static int maxDeterminizedStates = 100000;
+    private static Cache<Object, Automaton> cache = buildCache(Settings.EMPTY);
 
     static final char WILDCARD_STRING = '*';     // String equality with support for wildcards
     static final char WILDCARD_CHAR = '?';       // Char equality with support for wildcards
@@ -57,6 +71,18 @@ public final class Automatons {
         if (patterns.isEmpty()) {
             return EMPTY;
         }
+        if (cache == null) {
+            return buildAutomaton(patterns);
+        } else {
+            try {
+                return cache.computeIfAbsent(Sets.newHashSet(patterns), ignore -> buildAutomaton(patterns));
+            } catch (ExecutionException e) {
+                throw unwrapCacheException(e);
+            }
+        }
+    }
+
+    private static Automaton buildAutomaton(Collection<String> patterns) {
         List<Automaton> automata = new ArrayList<>(patterns.size());
         for (String pattern : patterns) {
             final Automaton patternAutomaton = pattern(pattern);
@@ -69,11 +95,23 @@ public final class Automatons {
      * Builds and returns an automaton that represents the given pattern.
      */
     static Automaton pattern(String pattern) {
+        if (cache == null) {
+            return buildAutomaton(pattern);
+        } else {
+            try {
+                return cache.computeIfAbsent(pattern, ignore -> buildAutomaton(pattern));
+            } catch (ExecutionException e) {
+                throw unwrapCacheException(e);
+            }
+        }
+    }
+
+    private static Automaton buildAutomaton(String pattern) {
         if (pattern.startsWith("/")) { // it's a lucene regexp
             if (pattern.length() == 1 || !pattern.endsWith("/")) {
                 throw new IllegalArgumentException("invalid pattern [" + pattern + "]. patterns starting with '/' " +
-                        "indicate regular expression pattern and therefore must also end with '/'." +
-                        " other patterns (those that do not start with '/') will be treated as simple wildcard patterns");
+                    "indicate regular expression pattern and therefore must also end with '/'." +
+                    " other patterns (those that do not start with '/') will be treated as simple wildcard patterns");
             }
             String regex = pattern.substring(1, pattern.length() - 1);
             return new RegExp(regex).toAutomaton();
@@ -84,16 +122,25 @@ public final class Automatons {
         }
     }
 
+    private static RuntimeException unwrapCacheException(ExecutionException e) {
+        final Throwable cause = e.getCause();
+        if (cause instanceof RuntimeException) {
+            return (RuntimeException) cause;
+        } else {
+            return new RuntimeException(cause);
+        }
+    }
+
     /**
      * Builds and returns an automaton that represents the given pattern.
      */
     @SuppressWarnings("fallthrough") // explicit fallthrough at end of switch
     static Automaton wildcard(String text) {
         List<Automaton> automata = new ArrayList<>();
-        for (int i = 0; i < text.length();) {
+        for (int i = 0; i < text.length(); ) {
             final char c = text.charAt(i);
             int length = 1;
-            switch(c) {
+            switch (c) {
                 case WILDCARD_STRING:
                     automata.add(Automata.makeAnyString());
                     break;
@@ -138,8 +185,19 @@ public final class Automatons {
         return predicate(automaton, "Predicate for " + automaton);
     }
 
-    public static void updateMaxDeterminizedStates(Settings settings) {
+    public static void updateConfiguration(Settings settings) {
         maxDeterminizedStates = MAX_DETERMINIZED_STATES_SETTING.get(settings);
+        cache = buildCache(settings);
+    }
+
+    private static Cache<Object, Automaton> buildCache(Settings settings) {
+        if (CACHE_ENABLED.get(settings) == false) {
+            return null;
+        }
+        return CacheBuilder.<Object, Automaton>builder()
+            .setExpireAfterAccess(CACHE_TTL.get(settings))
+            .setMaximumWeight(CACHE_SIZE.get(settings))
+            .build();
     }
 
     // accessor for testing
@@ -161,4 +219,11 @@ public final class Automatons {
             }
         };
     }
+
+    public static void addSettings(List<Setting<?>> settingsList) {
+        settingsList.add(MAX_DETERMINIZED_STATES_SETTING);
+        settingsList.add(CACHE_ENABLED);
+        settingsList.add(CACHE_SIZE);
+        settingsList.add(CACHE_TTL);
+    }
 }

+ 82 - 25
x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/support/AutomatonsTests.java

@@ -22,6 +22,8 @@ import static org.elasticsearch.xpack.core.security.support.Automatons.patterns;
 import static org.elasticsearch.xpack.core.security.support.Automatons.predicate;
 import static org.elasticsearch.xpack.core.security.support.Automatons.wildcard;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.sameInstance;
 
 public class AutomatonsTests extends ESTestCase {
     public void testPatternsUnionOfMultiplePatterns() throws Exception {
@@ -70,29 +72,29 @@ public class AutomatonsTests extends ESTestCase {
 
     public void testPatternComplexity() {
         List<String> patterns = Arrays.asList("*", "filebeat*de-tst-chatclassification*",
-                "metricbeat*de-tst-chatclassification*",
-                "packetbeat*de-tst-chatclassification*",
-                "heartbeat*de-tst-chatclassification*",
-                "filebeat*documentationdev*",
-                "metricbeat*documentationdev*",
-                "packetbeat*documentationdev*",
-                "heartbeat*documentationdev*",
-                "filebeat*devsupport-website*",
-                "metricbeat*devsupport-website*",
-                "packetbeat*devsupport-website*",
-                "heartbeat*devsupport-website*",
-                ".kibana-tcloud",
-                ".reporting-tcloud",
-                "filebeat-app-ingress-*",
-                "filebeat-app-tcloud-*",
-                "filebeat*documentationprod*",
-                "metricbeat*documentationprod*",
-                "packetbeat*documentationprod*",
-                "heartbeat*documentationprod*",
-                "filebeat*bender-minio-test-1*",
-                "metricbeat*bender-minio-test-1*",
-                "packetbeat*bender-minio-test-1*",
-                "heartbeat*bender-minio-test-1*");
+            "metricbeat*de-tst-chatclassification*",
+            "packetbeat*de-tst-chatclassification*",
+            "heartbeat*de-tst-chatclassification*",
+            "filebeat*documentationdev*",
+            "metricbeat*documentationdev*",
+            "packetbeat*documentationdev*",
+            "heartbeat*documentationdev*",
+            "filebeat*devsupport-website*",
+            "metricbeat*devsupport-website*",
+            "packetbeat*devsupport-website*",
+            "heartbeat*devsupport-website*",
+            ".kibana-tcloud",
+            ".reporting-tcloud",
+            "filebeat-app-ingress-*",
+            "filebeat-app-tcloud-*",
+            "filebeat*documentationprod*",
+            "metricbeat*documentationprod*",
+            "packetbeat*documentationprod*",
+            "heartbeat*documentationprod*",
+            "filebeat*bender-minio-test-1*",
+            "metricbeat*bender-minio-test-1*",
+            "packetbeat*bender-minio-test-1*",
+            "heartbeat*bender-minio-test-1*");
         final Automaton automaton = Automatons.patterns(patterns);
         assertTrue(Operations.isTotal(automaton));
         assertTrue(automaton.isDeterministic());
@@ -137,7 +139,7 @@ public class AutomatonsTests extends ESTestCase {
             assertNotEquals(10000, Automatons.getMaxDeterminizedStates());
             // set to the min value
             Settings settings = Settings.builder().put(Automatons.MAX_DETERMINIZED_STATES_SETTING.getKey(), 10000).build();
-            Automatons.updateMaxDeterminizedStates(settings);
+            Automatons.updateConfiguration(settings);
             assertEquals(10000, Automatons.getMaxDeterminizedStates());
 
             final List<String> names = new ArrayList<>(1024);
@@ -147,8 +149,63 @@ public class AutomatonsTests extends ESTestCase {
             TooComplexToDeterminizeException e = expectThrows(TooComplexToDeterminizeException.class, () -> Automatons.patterns(names));
             assertThat(e.getMaxDeterminizedStates(), equalTo(10000));
         } finally {
-            Automatons.updateMaxDeterminizedStates(Settings.EMPTY);
+            Automatons.updateConfiguration(Settings.EMPTY);
             assertEquals(100000, Automatons.getMaxDeterminizedStates());
         }
     }
+
+    public void testCachingOfAutomatons() {
+        Automatons.updateConfiguration(Settings.EMPTY);
+
+        String pattern1 = randomAlphaOfLengthBetween(3, 8) + "*";
+        String pattern2 = "/" + randomAlphaOfLengthBetween(1, 2) + "*" + randomAlphaOfLengthBetween(2, 4) + "/";
+
+        final Automaton a1 = Automatons.pattern(pattern1);
+        final Automaton a2 = Automatons.pattern(pattern2);
+
+        assertThat(Automatons.pattern(pattern1), sameInstance(a1));
+        assertThat(Automatons.pattern(pattern2), sameInstance(a2));
+
+        final Automaton a3 = Automatons.patterns(pattern1, pattern2);
+        final Automaton a4 = Automatons.patterns(pattern2, pattern1);
+        assertThat(a3, sameInstance(a4));
+    }
+
+    public void testConfigurationOfCacheSize() {
+        final Settings settings = Settings.builder()
+            .put(Automatons.CACHE_SIZE.getKey(), 2)
+            .build();
+        Automatons.updateConfiguration(settings);
+
+        String pattern1 = "a";
+        String pattern2 = "b";
+        String pattern3 = "c";
+
+        final Automaton a1 = Automatons.pattern(pattern1);
+        final Automaton a2 = Automatons.pattern(pattern2);
+
+        assertThat(Automatons.pattern(pattern1), sameInstance(a1));
+        assertThat(Automatons.pattern(pattern2), sameInstance(a2));
+
+        final Automaton a3 = Automatons.pattern(pattern3);
+        assertThat(Automatons.pattern(pattern3), sameInstance(a3));
+
+        // either pattern 1 or 2 should be evicted (in theory it should be 1, but we don't care about that level of precision)
+        final Automaton a1b = Automatons.pattern(pattern1);
+        final Automaton a2b = Automatons.pattern(pattern2);
+        if (a1b == a1 && a2b == a2) {
+            fail("Expected one of the existing automatons to be evicted, but both were still cached");
+        }
+    }
+
+    public void testDisableCache() {
+        final Settings settings = Settings.builder()
+            .put(Automatons.CACHE_ENABLED.getKey(), false)
+            .build();
+        Automatons.updateConfiguration(settings);
+
+        final String pattern = randomAlphaOfLengthBetween(5, 10);
+        final Automaton automaton = Automatons.pattern(pattern);
+        assertThat(Automatons.pattern(pattern), not(sameInstance(automaton)));
+    }
 }

+ 2 - 2
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

@@ -307,7 +307,7 @@ public class Security extends Plugin implements ActionPlugin, IngestPlugin, Netw
                 new FIPS140LicenseBootstrapCheck()));
             checks.addAll(InternalRealms.getBootstrapChecks(settings, env));
             this.bootstrapChecks = Collections.unmodifiableList(checks);
-            Automatons.updateMaxDeterminizedStates(settings);
+            Automatons.updateConfiguration(settings);
         } else {
             this.bootstrapChecks = Collections.emptyList();
         }
@@ -609,7 +609,7 @@ public class Security extends Plugin implements ActionPlugin, IngestPlugin, Netw
         ReservedRealm.addSettings(settingsList);
         AuthenticationService.addSettings(settingsList);
         AuthorizationService.addSettings(settingsList);
-        settingsList.add(Automatons.MAX_DETERMINIZED_STATES_SETTING);
+        Automatons.addSettings(settingsList);
         settingsList.add(CompositeRolesStore.CACHE_SIZE_SETTING);
         settingsList.add(FieldPermissionsCache.CACHE_SIZE_SETTING);
         settingsList.add(TokenService.TOKEN_EXPIRATION);