Skip to content

Commit

Permalink
Allow query caching by default again (#33328)
Browse files Browse the repository at this point in the history 
With the introduction of the default distribution, it means that by
default the query cache is wrapped in the security implementation of the
query cache. This cache does not allow caching if the request does not
carry indices permissions. Yet, this will not happen if authorization is
not allowed, which it is not by default. This means that with the
introduction of the default distribution, query caching was disabled by
default! This commit addresses this by checking if authorization is
allowed and if not, delegating to the default indices query
cache. Otherwise, we proceed as before with security. Additionally, we
clear the cache on license state changes.
  • Loading branch information
@jasontedor
jasontedor committed Sep 4, 2018
1 parent 4ff1b1d commit bfa9199
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 6 deletions.
11 changes: 6 additions & 5 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 @@ -123,9 +123,6 @@
import org.elasticsearch.xpack.core.security.authz.accesscontrol.SecurityIndexSearcherWrapper;
import org.elasticsearch.xpack.core.security.authz.permission.FieldPermissions;
import org.elasticsearch.xpack.core.security.authz.permission.FieldPermissionsCache;
import org.elasticsearch.xpack.security.action.privilege.TransportDeletePrivilegesAction;
import org.elasticsearch.xpack.security.action.privilege.TransportGetPrivilegesAction;
import org.elasticsearch.xpack.security.action.privilege.TransportPutPrivilegesAction;
import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore;
import org.elasticsearch.xpack.core.security.index.IndexAuditTrailField;
import org.elasticsearch.xpack.core.security.support.Automatons;
Expand All @@ -145,6 +142,9 @@
import org.elasticsearch.xpack.security.action.interceptor.ResizeRequestInterceptor;
import org.elasticsearch.xpack.security.action.interceptor.SearchRequestInterceptor;
import org.elasticsearch.xpack.security.action.interceptor.UpdateRequestInterceptor;
import org.elasticsearch.xpack.security.action.privilege.TransportDeletePrivilegesAction;
import org.elasticsearch.xpack.security.action.privilege.TransportGetPrivilegesAction;
import org.elasticsearch.xpack.security.action.privilege.TransportPutPrivilegesAction;
import org.elasticsearch.xpack.security.action.realm.TransportClearRealmCacheAction;
import org.elasticsearch.xpack.security.action.role.TransportClearRolesCacheAction;
import org.elasticsearch.xpack.security.action.role.TransportDeleteRoleAction;
Expand Down Expand Up @@ -183,8 +183,8 @@
import org.elasticsearch.xpack.security.authz.SecuritySearchOperationListener;
import org.elasticsearch.xpack.security.authz.accesscontrol.OptOutQueryCache;
import org.elasticsearch.xpack.security.authz.store.CompositeRolesStore;
import org.elasticsearch.xpack.security.authz.store.NativePrivilegeStore;
import org.elasticsearch.xpack.security.authz.store.FileRolesStore;
import org.elasticsearch.xpack.security.authz.store.NativePrivilegeStore;
import org.elasticsearch.xpack.security.authz.store.NativeRolesStore;
import org.elasticsearch.xpack.security.ingest.SetSecurityUserProcessor;
import org.elasticsearch.xpack.security.rest.SecurityRestFilter;
Expand Down Expand Up @@ -698,7 +698,8 @@ public void onIndexModule(IndexModule module) {
* This impl. disabled the query cache if field level security is used for a particular request. If we wouldn't do
* forcefully overwrite the query cache implementation then we leave the system vulnerable to leakages of data to
* unauthorized users. */
module.forceQueryCacheProvider((settings, cache) -> new OptOutQueryCache(settings, cache, threadContext.get()));
module.forceQueryCacheProvider(
(settings, cache) -> new OptOutQueryCache(settings, cache, threadContext.get(), getLicenseState()));
}

// in order to prevent scroll ids from being maliciously crafted and/or guessed, a listener is added that
Expand Down
18 changes: 17 additions & 1 deletion .../src/main/java/org/elasticsearch/xpack/security/authz/accesscontrol/OptOutQueryCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

package org.elasticsearch.xpack.security.authz.accesscontrol;

import org.apache.lucene.search.QueryCachingPolicy;
Expand All @@ -13,6 +14,7 @@
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.index.cache.query.QueryCache;
import org.elasticsearch.indices.IndicesQueryCache;
import org.elasticsearch.license.XPackLicenseState;
import org.elasticsearch.xpack.core.security.authz.AuthorizationServiceField;
import org.elasticsearch.xpack.core.security.authz.accesscontrol.IndicesAccessControl;

Expand All @@ -29,12 +31,19 @@ public final class OptOutQueryCache extends AbstractIndexComponent implements Qu
private final IndicesQueryCache indicesQueryCache;
private final ThreadContext context;
private final String indexName;
private final XPackLicenseState licenseState;

public OptOutQueryCache(IndexSettings indexSettings, IndicesQueryCache indicesQueryCache, ThreadContext context) {
public OptOutQueryCache(
final IndexSettings indexSettings,
final IndicesQueryCache indicesQueryCache,
final ThreadContext context,
final XPackLicenseState licenseState) {
super(indexSettings);
this.indicesQueryCache = indicesQueryCache;
this.context = Objects.requireNonNull(context, "threadContext must not be null");
this.indexName = indexSettings.getIndex().getName();
this.licenseState = Objects.requireNonNull(licenseState, "licenseState");
licenseState.addListener(() -> this.clear("license state changed"));
}

@Override
Expand All @@ -50,6 +59,12 @@ public void clear(String reason) {

@Override
public Weight doCache(Weight weight, QueryCachingPolicy policy) {
// TODO: this is not concurrently safe since the license state can change between reads
if (licenseState.isSecurityEnabled() == false || licenseState.isAuthAllowed() == false) {
logger.debug("not opting out of the query cache; authorization is not allowed");
return indicesQueryCache.doCache(weight, policy);
}

IndicesAccessControl indicesAccessControl = context.getTransient(
AuthorizationServiceField.INDICES_PERMISSIONS_KEY);
if (indicesAccessControl == null) {
Expand Down Expand Up @@ -96,4 +111,5 @@ static boolean cachingIsSafe(Weight weight, IndicesAccessControl.IndexAccessCont
// we can cache, all fields are ok
return true;
}

}
97 changes: 97 additions & 0 deletions ...test/java/org/elasticsearch/xpack/security/authz/accesscontrol/OptOutQueryCacheTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,19 @@
import org.apache.lucene.search.BooleanClause;
import org.apache.lucene.search.BooleanQuery;
import org.apache.lucene.search.IndexSearcher;
import org.apache.lucene.search.QueryCachingPolicy;
import org.apache.lucene.search.TermQuery;
import org.apache.lucene.search.Weight;
import org.apache.lucene.store.Directory;
import org.elasticsearch.Version;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.indices.IndicesQueryCache;
import org.elasticsearch.license.XPackLicenseState;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xpack.core.security.authz.AuthorizationServiceField;
import org.elasticsearch.xpack.core.security.authz.accesscontrol.IndicesAccessControl;
import org.elasticsearch.xpack.core.security.authz.permission.FieldPermissions;
import org.elasticsearch.xpack.core.security.authz.permission.FieldPermissionsDefinition;
Expand All @@ -24,6 +33,12 @@
import java.io.IOException;
import java.util.HashSet;

import static org.mockito.Matchers.same;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;

/** Simple tests for opt out query cache*/
public class OptOutQueryCacheTests extends ESTestCase {
IndexSearcher searcher;
Expand Down Expand Up @@ -107,6 +122,88 @@ public void testOptOutQueryCacheSafetyCheck() throws IOException {
assertFalse(OptOutQueryCache.cachingIsSafe(weight, permissions));
}

public void testOptOutQueryCacheSecurityIsNotEnabled() {
final Settings.Builder settings = Settings.builder()
.put("index.version.created", Version.CURRENT)
.put("index.number_of_shards", 1)
.put("index.number_of_replicas", 0);
final IndexMetaData indexMetaData = IndexMetaData.builder("index").settings(settings).build();
final IndexSettings indexSettings = new IndexSettings(indexMetaData, Settings.EMPTY);
final IndicesQueryCache indicesQueryCache = mock(IndicesQueryCache.class);
final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
final XPackLicenseState licenseState = mock(XPackLicenseState.class);
when(licenseState.isSecurityEnabled()).thenReturn(false);
when(licenseState.isAuthAllowed()).thenReturn(randomBoolean());
final OptOutQueryCache cache = new OptOutQueryCache(indexSettings, indicesQueryCache, threadContext, licenseState);
final Weight weight = mock(Weight.class);
final QueryCachingPolicy policy = mock(QueryCachingPolicy.class);
cache.doCache(weight, policy);
verify(indicesQueryCache).doCache(same(weight), same(policy));
}

public void testOptOutQueryCacheAuthIsNotAllowed() {
final Settings.Builder settings = Settings.builder()
.put("index.version.created", Version.CURRENT)
.put("index.number_of_shards", 1)
.put("index.number_of_replicas", 0);
final IndexMetaData indexMetaData = IndexMetaData.builder("index").settings(settings).build();
final IndexSettings indexSettings = new IndexSettings(indexMetaData, Settings.EMPTY);
final IndicesQueryCache indicesQueryCache = mock(IndicesQueryCache.class);
final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
final XPackLicenseState licenseState = mock(XPackLicenseState.class);
when(licenseState.isSecurityEnabled()).thenReturn(randomBoolean());
when(licenseState.isAuthAllowed()).thenReturn(false);
final OptOutQueryCache cache = new OptOutQueryCache(indexSettings, indicesQueryCache, threadContext, licenseState);
final Weight weight = mock(Weight.class);
final QueryCachingPolicy policy = mock(QueryCachingPolicy.class);
cache.doCache(weight, policy);
verify(indicesQueryCache).doCache(same(weight), same(policy));
}

public void testOptOutQueryCacheNoIndicesPermissions() {
final Settings.Builder settings = Settings.builder()
.put("index.version.created", Version.CURRENT)
.put("index.number_of_shards", 1)
.put("index.number_of_replicas", 0);
final IndexMetaData indexMetaData = IndexMetaData.builder("index").settings(settings).build();
final IndexSettings indexSettings = new IndexSettings(indexMetaData, Settings.EMPTY);
final IndicesQueryCache indicesQueryCache = mock(IndicesQueryCache.class);
final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
final XPackLicenseState licenseState = mock(XPackLicenseState.class);
when(licenseState.isSecurityEnabled()).thenReturn(true);
when(licenseState.isAuthAllowed()).thenReturn(true);
final OptOutQueryCache cache = new OptOutQueryCache(indexSettings, indicesQueryCache, threadContext, licenseState);
final Weight weight = mock(Weight.class);
final QueryCachingPolicy policy = mock(QueryCachingPolicy.class);
final Weight w = cache.doCache(weight, policy);
assertSame(w, weight);
verifyNoMoreInteractions(indicesQueryCache);
}

public void testOptOutQueryCacheIndexDoesNotHaveFieldLevelSecurity() {
final Settings.Builder settings = Settings.builder()
.put("index.version.created", Version.CURRENT)
.put("index.number_of_shards", 1)
.put("index.number_of_replicas", 0);
final IndexMetaData indexMetaData = IndexMetaData.builder("index").settings(settings).build();
final IndexSettings indexSettings = new IndexSettings(indexMetaData, Settings.EMPTY);
final IndicesQueryCache indicesQueryCache = mock(IndicesQueryCache.class);
final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
final IndicesAccessControl.IndexAccessControl indexAccessControl = mock(IndicesAccessControl.IndexAccessControl.class);
when(indexAccessControl.getFieldPermissions()).thenReturn(new FieldPermissions());
final IndicesAccessControl indicesAccessControl = mock(IndicesAccessControl.class);
when(indicesAccessControl.getIndexPermissions("index")).thenReturn(indexAccessControl);
threadContext.putTransient(AuthorizationServiceField.INDICES_PERMISSIONS_KEY, indicesAccessControl);
final XPackLicenseState licenseState = mock(XPackLicenseState.class);
when(licenseState.isSecurityEnabled()).thenReturn(true);
when(licenseState.isAuthAllowed()).thenReturn(true);
final OptOutQueryCache cache = new OptOutQueryCache(indexSettings, indicesQueryCache, threadContext, licenseState);
final Weight weight = mock(Weight.class);
final QueryCachingPolicy policy = mock(QueryCachingPolicy.class);
cache.doCache(weight, policy);
verify(indicesQueryCache).doCache(same(weight), same(policy));
}

private static FieldPermissionsDefinition fieldPermissionDef(String[] granted, String[] denied) {
return new FieldPermissionsDefinition(granted, denied);
}
Expand Down

0 comments on commit bfa9199

Please sign in to comment.