Skip to content

Commit

Permalink
Enable security automaton caching (#34028)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@tvernum
tvernum authored Oct 5, 2018
1 parent 1bb2a15 commit 6608992
Show file tree
Hide file tree
Showing 4 changed files with 184 additions and 34 deletions.
28 changes: 28 additions & 0 deletions docs/reference/settings/security-settings.asciidoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`::
Expand Down Expand Up @@ -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
Expand Down
79 changes: 72 additions & 7 deletions ...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/Automatons.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,18 @@
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;
Expand All @@ -27,14 +32,23 @@

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
Expand All @@ -57,6 +71,18 @@ public static Automaton patterns(Collection<String> patterns) {
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);
Expand All @@ -69,11 +95,23 @@ public static Automaton patterns(Collection<String> patterns) {
* 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();
Expand All @@ -84,16 +122,25 @@ static Automaton pattern(String pattern) {
}
}

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;
Expand Down Expand Up @@ -138,8 +185,19 @@ public static Predicate<String> predicate(Automaton automaton) {
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
Expand All @@ -161,4 +219,11 @@ public String toString() {
}
};
}

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);
}
}
107 changes: 82 additions & 25 deletions ...gin/core/src/test/java/org/elasticsearch/xpack/core/security/support/AutomatonsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
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 {
Expand Down Expand Up @@ -70,29 +72,29 @@ public void testPredicateToString() throws Exception {

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());
Expand Down Expand Up @@ -137,7 +139,7 @@ public void testSettingMaxDeterminizedStates() {
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);
Expand All @@ -147,8 +149,63 @@ public void testSettingMaxDeterminizedStates() {
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)));
}
}
4 changes: 2 additions & 2 deletions x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ public Security(Settings settings, final Path configPath) {
new FIPS140LicenseBootstrapCheck()));
checks.addAll(InternalRealms.getBootstrapChecks(settings, env));
this.bootstrapChecks = Collections.unmodifiableList(checks);
Automatons.updateMaxDeterminizedStates(settings);
Automatons.updateConfiguration(settings);
} else {
this.bootstrapChecks = Collections.emptyList();
}
Expand Down Expand Up @@ -609,7 +609,7 @@ public static List<Setting<?>> getSettings(boolean transportClientMode, List<Sec
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);
Expand Down

0 comments on commit 6608992

Please sign in to comment.