-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Changes from 5 commits
b0c2594
b014814
914f56e
8a35041
1e3dbdf
120aeaf
175260b
2bc6593
800cf1e
69b3ed3
95737ac
7f055b1
a0b0a62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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()))); | ||
|
@@ -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; | ||
} | ||
|
||
final boolean granted; | ||
if (this.granted == limitedByIndicesAccessControl.granted) { | ||
granted = this.granted; | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make this a non-static field? Since There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like it that we now return an |
||
} | ||
|
||
@Override | ||
public boolean isGranted() { | ||
return true; | ||
} | ||
|
||
@Override | ||
public Collection<?> getDeniedIndices() { | ||
return Set.of(); | ||
} | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -118,6 +118,10 @@ public Group[] groups() { | |
return groups; | ||
} | ||
|
||
public boolean isTotal() { | ||
return Arrays.stream(groups).anyMatch(Group::isTotal); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And we also avoid the companion There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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; | ||
|
||
|
@@ -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; | ||
|
@@ -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 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like how GH throws in a |
||
}, listener::onFailure, requestInfo, requestId, authzInfo), threadContext); | ||
authzEngine.authorizeClusterAction(requestInfo, authzInfo, ActionListener.wrap(result -> { | ||
|
@@ -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); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,6 +92,7 @@ | |
import java.util.Set; | ||
import java.util.TreeSet; | ||
import java.util.function.Predicate; | ||
import java.util.stream.Collectors; | ||
|
||
import static org.elasticsearch.common.Strings.arrayToCommaDelimitedString; | ||
import static org.elasticsearch.xpack.security.action.user.TransportHasPrivilegesAction.getApplicationNames; | ||
|
@@ -595,11 +596,23 @@ GetUserPrivilegesResponse buildUserPrivilegesResponseObject(Role userRole) { | |
} | ||
|
||
static Set<String> resolveAuthorizedIndicesFromRole(Role role, RequestInfo requestInfo, Map<String, IndexAbstraction> lookup) { | ||
Predicate<IndexAbstraction> predicate = role.allowedIndicesMatcher(requestInfo.getAction()); | ||
final Predicate<IndexAbstraction> predicate = role.allowedIndicesMatcher(requestInfo.getAction()); | ||
|
||
// do not include data streams for actions that do not operate on data streams | ||
TransportRequest request = requestInfo.getRequest(); | ||
final boolean includeDataStreams = (request instanceof IndicesRequest) && ((IndicesRequest) request).includeDataStreams(); | ||
if (predicate == Role.MATCH_ALL_INDICES_MATCHER) { | ||
if (includeDataStreams) { | ||
return lookup.values().stream() | ||
.map(IndexAbstraction::getName) | ||
.collect(Collectors.toUnmodifiableSet()); | ||
} else { | ||
return lookup.values().stream() | ||
.filter(indexAbstraction -> indexAbstraction.getType() != IndexAbstraction.Type.DATA_STREAM) | ||
.map(IndexAbstraction::getName) | ||
.collect(Collectors.toUnmodifiableSet()); | ||
} | ||
} | ||
ywangd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Set<String> indicesAndAliases = new HashSet<>(); | ||
// TODO: can this be done smarter? I think there are usually more indices/aliases in the cluster then indices defined a roles? | ||
|
@@ -630,8 +643,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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
There was a problem hiding this comment.
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.instanceof AllowAllIndicesAccessControl
== AllowAllIndicesAccessControl.ALLOW_ALL_INDICES_ACCESS_CONTROL
isAllowAll()
(implemented via method 2)Can we standardise?
There was a problem hiding this comment.
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.