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 @@ -29,7 +29,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 @@ -179,6 +178,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 @@ -205,4 +210,38 @@ public String toString() {
", indexPermissions=" + indexPermissions +
'}';
}

public boolean isAllowAll() {
return this == AllowAllIndicesAccessControl.ALLOW_ALL_INDICES_ACCESS_CONTROL;
}

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

private static class AllowAllIndicesAccessControl extends IndicesAccessControl {

private static final IndicesAccessControl ALLOW_ALL_INDICES_ACCESS_CONTROL = new AllowAllIndicesAccessControl();
private static 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.

Can we make this a non-static field?

Since ALLOW_ALL_INDICES_ACCESS_CONTROL is a singleton, having a non-static field for ALLOW_ALL_INDEX_ACCESS_CONTROL makes no difference in terms on memory usage or execution time.
However, it makes the code much cleaner because then we can have 1 static field for the singleton INSTANCE which is the one you want to use when you need an instance of AllowAllIndicesAccessControl and the other field becomes a member field and there's no chance of ambiguity because it's clearly an internal implementation detail.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. updated as suggested.


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 @@ -118,6 +118,10 @@ public Group[] groups() {
return groups;
}

public boolean isTotal() {
return Arrays.stream(groups).anyMatch(Group::isTotal);
Copy link
Member Author

Choose a reason for hiding this comment

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

We can make this stricter to require the groups to have only one member, which is the case for superuser role and the API keys created by it.

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 think the condition is OK like this.

But this being a public method irks me a little bit.
I think we can avoid it by including this check at the beginning of the authorize method.
We get one less method to worry about for the caller, especially since the isTotal is fuzzy (isTotal can be false but the permission to actually grant all indices).

Copy link
Contributor

Choose a reason for hiding this comment

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

And we also avoid the companion isTotal public method on the Role and LimitedRole (hopefully).

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated as suggested.

}

/**
* @return A predicate that will match all the indices that this permission
* has the privilege for executing the given action on.
Expand Down Expand Up @@ -340,7 +344,6 @@ public Map<String, IndicesAccessControl.IndexAccessControl> authorize(
Map<String, IndexAbstraction> lookup,
FieldPermissionsCache fieldPermissionsCache
) {

final List<IndexResource> resources = new ArrayList<>(requestedIndicesOrAliases.size());
int totalResourceCount = 0;

Expand Down Expand Up @@ -499,7 +502,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 @@ -567,6 +570,14 @@ public boolean allowRestrictedIndices() {
public Automaton getIndexMatcherAutomaton() {
return indexNameAutomaton.get();
}

public 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 @@ -84,10 +84,14 @@ 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);
}

@Override
public boolean allowAllIndices() {
return super.allowAllIndices() && limitedBy.allowAllIndices();
}

/**
* @return A predicate that will match all the indices that this role and the limited by role has the privilege for executing the given
* action on.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,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(
final Map<String, IndicesAccessControl.IndexAccessControl> indexPermissions = indices.authorize(
action, requestedIndicesOrAliases, aliasAndIndexLookup, fieldPermissionsCache
);

Expand All @@ -194,6 +194,10 @@ public IndicesAccessControl authorize(String action, Set<String> requestedIndice
return new IndicesAccessControl(granted, indexPermissions);
}

public boolean allowAllIndices() {
return indices.isTotal();
}

@Override
public boolean equals(Object o) {
if (this == o) {
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 @@ -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 @@ -508,7 +508,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
Original file line number Diff line number Diff line change
Expand Up @@ -630,8 +630,13 @@ private IndexAuthorizationResult buildIndicesAccessControl(String action,
Set<String> indices,
Map<String, IndexAbstraction> aliasAndIndexLookup) {
final Role role = ensureRBAC(authorizationInfo).getRole();
final IndicesAccessControl accessControl = role.authorize(action, indices, aliasAndIndexLookup, fieldPermissionsCache);
return new IndexAuthorizationResult(true, accessControl);
// Fast path if the role can access all indices, e.g. superuser
if (role.allowAllIndices()) {
return new IndexAuthorizationResult(true, IndicesAccessControl.allowAll());
Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion from the other comments is to push the fast-path one level down in the role.authorize call, for slight reasons of interface clarity, but I also prefer it because at this level we avoid introducing a bypass for such an important operation as authorization (I feel that the bypass inside the authorize method is more tolerable mentally).

Copy link
Contributor

Choose a reason for hiding this comment

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

++

} else {
final IndicesAccessControl accessControl = role.authorize(action, indices, aliasAndIndexLookup, fieldPermissionsCache);
return new IndexAuthorizationResult(true, accessControl);
}
ywangd marked this conversation as resolved.
Show resolved Hide resolved
}

private static RBACAuthorizationInfo ensureRBAC(AuthorizationInfo authorizationInfo) {
Expand Down