Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Superuser fastpath for indexAccessControl #78498

Merged
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
*/
public class IndicesAccessControl {

public static final IndicesAccessControl ALLOW_ALL = new IndicesAccessControl(true, Collections.emptyMap());
public static final IndicesAccessControl ALLOW_NO_INDICES = new IndicesAccessControl(true,
Collections.singletonMap(IndicesAndAliasesResolverField.NO_INDEX_PLACEHOLDER,
new IndicesAccessControl.IndexAccessControl(true, new FieldPermissions(), DocumentPermissions.allowAll())));
Expand Down Expand Up @@ -195,6 +194,12 @@ public int hashCode() {
* @return {@link IndicesAccessControl}
*/
public IndicesAccessControl limitIndicesAccessControl(IndicesAccessControl limitedByIndicesAccessControl) {
if (this instanceof AllowAllIndicesAccessControl) {
return limitedByIndicesAccessControl;
} else if (limitedByIndicesAccessControl instanceof AllowAllIndicesAccessControl) {
return this;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We seem to have 3 different (overlapping) methods for deciding whether an IndicesAccessControl object is the allow all object.

  1. instanceof AllowAllIndicesAccessControl
  2. == AllowAllIndicesAccessControl.ALLOW_ALL_INDICES_ACCESS_CONTROL
  3. isAllowAll() (implemented via method 2)

Can we standardise?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the 3rd method (it is actually not used ...) and thus remove 2 as well. Option 1 is the standard now.


final boolean granted;
if (this.granted == limitedByIndicesAccessControl.granted) {
granted = this.granted;
Expand All @@ -221,4 +226,35 @@ public String toString() {
", indexPermissions=" + indexPermissions +
'}';
}

public static IndicesAccessControl allowAll() {
return AllowAllIndicesAccessControl.INSTANCE;
}

private static class AllowAllIndicesAccessControl extends IndicesAccessControl {

private static final IndicesAccessControl INSTANCE = new AllowAllIndicesAccessControl();

private final IndexAccessControl ALLOW_ALL_INDEX_ACCESS_CONTROL = new IndexAccessControl(true, null, null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Since this is no longer static it shouldn't have an ALL_CAPS_NAME either.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


private AllowAllIndicesAccessControl() {
super(true, null);
}

@Override
public IndexAccessControl getIndexPermissions(String index) {
return ALLOW_ALL_INDEX_ACCESS_CONTROL;
Copy link
Contributor

@albertzaharovits albertzaharovits Oct 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it that we now return an IndexAccessControl instance instead of null, for the "allow all" case.

}

@Override
public boolean isGranted() {
return true;
}

@Override
public Collection<?> getDeniedIndices() {
return Set.of();
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -341,12 +341,16 @@ public Collection<String> resolveConcreteIndices() {
/**
* Authorizes the provided action against the provided indices, given the current cluster metadata
*/
public Map<String, IndicesAccessControl.IndexAccessControl> authorize(
public IndicesAccessControl authorize(
String action,
Set<String> requestedIndicesOrAliases,
Map<String, IndexAbstraction> lookup,
FieldPermissionsCache fieldPermissionsCache
) {
// Short circuit if the indicesPermission allows all access to every index
if (Arrays.stream(groups).anyMatch(Group::isTotal)) {
return IndicesAccessControl.allowAll();
}

final List<IndexResource> resources = new ArrayList<>(requestedIndicesOrAliases.size());
int totalResourceCount = 0;
Expand Down Expand Up @@ -460,6 +464,7 @@ public Map<String, IndicesAccessControl.IndexAccessControl> authorize(
}
}

boolean overallGranted = true;
Map<String, IndicesAccessControl.IndexAccessControl> indexPermissions = new HashMap<>(grantedBuilder.size());
for (Map.Entry<String, Boolean> entry : grantedBuilder.entrySet()) {
String index = entry.getKey();
Expand All @@ -479,10 +484,13 @@ public Map<String, IndicesAccessControl.IndexAccessControl> authorize(
} else {
fieldPermissions = FieldPermissions.DEFAULT;
}
if (entry.getValue() == false) {
overallGranted = false;
}
indexPermissions.put(index, new IndicesAccessControl.IndexAccessControl(entry.getValue(), fieldPermissions,
(roleQueries != null) ? DocumentPermissions.filteredBy(roleQueries) : DocumentPermissions.allowAll()));
}
return unmodifiableMap(indexPermissions);
return new IndicesAccessControl(overallGranted, unmodifiableMap(indexPermissions));
}

private boolean isConcreteRestrictedIndex(String indexPattern) {
Expand All @@ -506,7 +514,7 @@ public static class Group {
private final IndexPrivilege privilege;
private final Predicate<String> actionMatcher;
private final String[] indices;
private final Predicate<String> indexNameMatcher;
private final StringMatcher indexNameMatcher;
private final Supplier<Automaton> indexNameAutomaton;
private final FieldPermissions fieldPermissions;
private final Set<BytesReference> query;
Expand Down Expand Up @@ -574,6 +582,14 @@ public boolean allowRestrictedIndices() {
public Automaton getIndexMatcherAutomaton() {
return indexNameAutomaton.get();
}

boolean isTotal() {
return allowRestrictedIndices
&& indexNameMatcher.isTotal()
&& privilege == IndexPrivilege.ALL
&& query == null
&& false == fieldPermissions.hasFieldLevelSecurity();
}
}

private static class DocumentLevelPermissions {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ public IndicesAccessControl authorize(String action, Set<String> requestedIndice
super.authorize(action, requestedIndicesOrAliases, aliasAndIndexLookup, fieldPermissionsCache);
IndicesAccessControl limitedByIndicesAccessControl = limitedBy.authorize(action, requestedIndicesOrAliases, aliasAndIndexLookup,
fieldPermissionsCache);

return indicesAccessControl.limitIndicesAccessControl(limitedByIndicesAccessControl);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,19 +183,7 @@ public ResourcePrivilegesMap checkApplicationResourcePrivileges(final String app
public IndicesAccessControl authorize(String action, Set<String> requestedIndicesOrAliases,
Map<String, IndexAbstraction> aliasAndIndexLookup,
FieldPermissionsCache fieldPermissionsCache) {
Map<String, IndicesAccessControl.IndexAccessControl> indexPermissions = indices.authorize(
action, requestedIndicesOrAliases, aliasAndIndexLookup, fieldPermissionsCache
);

// At least one role / indices permission set need to match with all the requested indices/aliases:
boolean granted = true;
for (Map.Entry<String, IndicesAccessControl.IndexAccessControl> entry : indexPermissions.entrySet()) {
if (entry.getValue().isGranted() == false) {
granted = false;
break;
}
}
return new IndicesAccessControl(granted, indexPermissions);
return indices.authorize(action, requestedIndicesOrAliases, aliasAndIndexLookup, fieldPermissionsCache);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@ public boolean test(String s) {
return predicate.test(s);
}

public boolean isTotal() {
return predicate == ALWAYS_TRUE_PREDICATE;
}

// For testing
Predicate<String> getPredicate() {
return predicate;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@
import org.elasticsearch.xpack.core.security.action.CreateApiKeyRequest;
import org.elasticsearch.xpack.core.security.action.GetApiKeyRequest;
import org.elasticsearch.xpack.core.security.action.apikey.QueryApiKeyRequest;
import org.elasticsearch.xpack.core.security.authz.accesscontrol.IndicesAccessControl;
import org.elasticsearch.xpack.core.textstructure.action.FindStructureAction;
import org.elasticsearch.xpack.core.ml.action.FlushJobAction;
import org.elasticsearch.xpack.core.ml.action.ForecastJobAction;
Expand Down Expand Up @@ -167,7 +168,6 @@
import org.elasticsearch.xpack.core.security.action.user.PutUserAction;
import org.elasticsearch.xpack.core.security.authc.Authentication;
import org.elasticsearch.xpack.core.security.authz.RoleDescriptor;
import org.elasticsearch.xpack.core.security.authz.accesscontrol.IndicesAccessControl.IndexAccessControl;
import org.elasticsearch.xpack.core.security.authz.permission.FieldPermissionsCache;
import org.elasticsearch.xpack.core.security.authz.permission.Role;
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilege;
Expand Down Expand Up @@ -214,7 +214,6 @@
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.SortedMap;

import static org.elasticsearch.xpack.core.security.test.TestRestrictedIndices.RESTRICTED_INDICES_AUTOMATON;
Expand Down Expand Up @@ -1135,12 +1134,12 @@ private void assertMonitoringOnRestrictedIndices(Role role) {
GetSettingsAction.NAME, IndicesShardStoresAction.NAME, RecoveryAction.NAME);
for (final String indexMonitoringActionName : indexMonitoringActionNamesList) {
String asyncSearchIndex = XPackPlugin.ASYNC_RESULTS_INDEX + randomAlphaOfLengthBetween(0, 2);
final Map<String, IndexAccessControl> authzMap = role.indices().authorize(indexMonitoringActionName,
final IndicesAccessControl iac = role.indices().authorize(indexMonitoringActionName,
Sets.newHashSet(internalSecurityIndex, RestrictedIndicesNames.SECURITY_MAIN_ALIAS, asyncSearchIndex),
metadata.getIndicesLookup(), fieldPermissionsCache);
assertThat(authzMap.get(internalSecurityIndex).isGranted(), is(true));
assertThat(authzMap.get(RestrictedIndicesNames.SECURITY_MAIN_ALIAS).isGranted(), is(true));
assertThat(authzMap.get(asyncSearchIndex).isGranted(), is(true));
assertThat(iac.getIndexPermissions(internalSecurityIndex).isGranted(), is(true));
assertThat(iac.getIndexPermissions(RestrictedIndicesNames.SECURITY_MAIN_ALIAS).isGranted(), is(true));
assertThat(iac.getIndexPermissions(asyncSearchIndex).isGranted(), is(true));
}
}

Expand Down Expand Up @@ -1233,25 +1232,25 @@ public void testSuperuserRole() {

FieldPermissionsCache fieldPermissionsCache = new FieldPermissionsCache(Settings.EMPTY);
SortedMap<String, IndexAbstraction> lookup = metadata.getIndicesLookup();
Map<String, IndexAccessControl> authzMap =
IndicesAccessControl iac =
superuserRole.indices().authorize(SearchAction.NAME, Sets.newHashSet("a1", "ba"), lookup, fieldPermissionsCache);
assertThat(authzMap.get("a1").isGranted(), is(true));
assertThat(authzMap.get("b").isGranted(), is(true));
authzMap =
assertThat(iac.getIndexPermissions("a1").isGranted(), is(true));
assertThat(iac.getIndexPermissions("b").isGranted(), is(true));
iac =
superuserRole.indices().authorize(DeleteIndexAction.NAME, Sets.newHashSet("a1", "ba"), lookup, fieldPermissionsCache);
assertThat(authzMap.get("a1").isGranted(), is(true));
assertThat(authzMap.get("b").isGranted(), is(true));
authzMap = superuserRole.indices().authorize(IndexAction.NAME, Sets.newHashSet("a2", "ba"), lookup, fieldPermissionsCache);
assertThat(authzMap.get("a2").isGranted(), is(true));
assertThat(authzMap.get("b").isGranted(), is(true));
authzMap = superuserRole.indices()
assertThat(iac.getIndexPermissions("a1").isGranted(), is(true));
assertThat(iac.getIndexPermissions("b").isGranted(), is(true));
iac = superuserRole.indices().authorize(IndexAction.NAME, Sets.newHashSet("a2", "ba"), lookup, fieldPermissionsCache);
assertThat(iac.getIndexPermissions("a2").isGranted(), is(true));
assertThat(iac.getIndexPermissions("b").isGranted(), is(true));
iac = superuserRole.indices()
.authorize(UpdateSettingsAction.NAME, Sets.newHashSet("aaaaaa", "ba"), lookup, fieldPermissionsCache);
assertThat(authzMap.get("aaaaaa").isGranted(), is(true));
assertThat(authzMap.get("b").isGranted(), is(true));
authzMap = superuserRole.indices().authorize(randomFrom(IndexAction.NAME, DeleteIndexAction.NAME, SearchAction.NAME),
assertThat(iac.getIndexPermissions("aaaaaa").isGranted(), is(true));
assertThat(iac.getIndexPermissions("b").isGranted(), is(true));
iac = superuserRole.indices().authorize(randomFrom(IndexAction.NAME, DeleteIndexAction.NAME, SearchAction.NAME),
Sets.newHashSet(RestrictedIndicesNames.SECURITY_MAIN_ALIAS), lookup, fieldPermissionsCache);
assertThat(authzMap.get(RestrictedIndicesNames.SECURITY_MAIN_ALIAS).isGranted(), is(true));
assertThat(authzMap.get(internalSecurityIndex).isGranted(), is(true));
assertThat(iac.getIndexPermissions(RestrictedIndicesNames.SECURITY_MAIN_ALIAS).isGranted(), is(true));
assertThat(iac.getIndexPermissions(internalSecurityIndex).isGranted(), is(true));
assertTrue(superuserRole.indices().check(SearchAction.NAME));
assertFalse(superuserRole.indices().check("unknown"));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ private void authorizeAction(final RequestInfo requestInfo, final String request
if (ClusterPrivilegeResolver.isClusterAction(action)) {
final ActionListener<AuthorizationResult> clusterAuthzListener =
wrapPreservingContext(new AuthorizationResultListener<>(result -> {
threadContext.putTransient(INDICES_PERMISSIONS_KEY, IndicesAccessControl.ALLOW_ALL);
threadContext.putTransient(INDICES_PERMISSIONS_KEY, IndicesAccessControl.allowAll());
listener.onResponse(null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how GH throws in a main source code change in between test ones, to keep us on our toes about tests.

}, listener::onFailure, requestInfo, requestId, authzInfo), threadContext);
authzEngine.authorizeClusterAction(requestInfo, authzInfo, ActionListener.wrap(result -> {
Expand Down Expand Up @@ -512,7 +512,7 @@ private void authorizeSystemUser(final Authentication authentication, final Stri
final TransportRequest request, final ActionListener<Void> listener) {
final AuditTrail auditTrail = auditTrailService.get();
if (SystemUser.isAuthorized(action)) {
threadContext.putTransient(INDICES_PERMISSIONS_KEY, IndicesAccessControl.ALLOW_ALL);
threadContext.putTransient(INDICES_PERMISSIONS_KEY, IndicesAccessControl.allowAll());
threadContext.putTransient(AUTHORIZATION_INFO_KEY, SYSTEM_AUTHZ_INFO);
auditTrail.accessGranted(requestId, authentication, action, request, SYSTEM_AUTHZ_INFO);
listener.onResponse(null);
Expand Down
Loading