Skip to content

Build indicesAccessControl for selective indices #78327

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

Build indicesAccessControl for selective indices #78327

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9f7bda2
Build indicesAccessControl for selective indices
ywangd Sep 27, 2021
291253b
Merge remote-tracking branch 'origin/master' into faster-role-authorize
ywangd Sep 28, 2021
abf5f69
Update to leverage the new getAliases API
ywangd Sep 28, 2021
9050910
checkstyle
ywangd Sep 28, 2021
4605f35
check index name itself
ywangd Sep 28, 2021
9b88dac
Simple heuristic and comments.
ywangd Sep 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 79 additions & 3 deletions x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.elasticsearch.action.bulk.BulkAction;
import org.elasticsearch.action.bulk.BulkShardRequest;
import org.elasticsearch.action.delete.DeleteAction;
import org.elasticsearch.action.fieldcaps.FieldCapabilitiesIndexRequest;
import org.elasticsearch.action.get.MultiGetAction;
import org.elasticsearch.action.index.IndexAction;
import org.elasticsearch.action.search.ClearScrollAction;
Expand All @@ -35,6 +36,8 @@
import org.elasticsearch.common.regex.Regex;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.set.Sets;
import org.elasticsearch.search.fetch.ShardFetchSearchRequest;
import org.elasticsearch.search.internal.ShardSearchRequest;
import org.elasticsearch.transport.TransportActionProxy;
import org.elasticsearch.transport.TransportRequest;
import org.elasticsearch.xpack.core.async.DeleteAsyncResultAction;
Expand Down Expand Up @@ -92,6 +95,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;
Expand Down Expand Up @@ -325,7 +329,7 @@ public void authorizeIndexAction(RequestInfo requestInfo, AuthorizationInfo auth
listener.onResponse(authorizeIndexActionName(action, authorizationInfo, IndicesAccessControl.ALLOW_NO_INDICES));
} else {
listener.onResponse(buildIndicesAccessControl(
action, authorizationInfo, Sets.newHashSet(resolvedIndices.getLocal()), aliasOrIndexLookup));
action, request, authorizationInfo, Sets.newHashSet(resolvedIndices.getLocal()), aliasOrIndexLookup));
}
}, listener::onFailure));
} else {
Expand All @@ -342,7 +346,7 @@ public void authorizeIndexAction(RequestInfo requestInfo, AuthorizationInfo auth
listener.onResponse(new IndexAuthorizationResult(true, IndicesAccessControl.ALLOW_NO_INDICES));
} else {
listener.onResponse(buildIndicesAccessControl(
action, authorizationInfo, Sets.newHashSet(resolvedIndices.getLocal()), aliasOrIndexLookup));
action, request, authorizationInfo, Sets.newHashSet(resolvedIndices.getLocal()), aliasOrIndexLookup));
}
}, listener::onFailure));
} else {
Expand Down Expand Up @@ -626,14 +630,86 @@ static Set<String> resolveAuthorizedIndicesFromRole(Role role, RequestInfo reque
}

private IndexAuthorizationResult buildIndicesAccessControl(String action,
TransportRequest request,
AuthorizationInfo authorizationInfo,
Set<String> indices,
Map<String, IndexAbstraction> aliasAndIndexLookup) {
final Role role = ensureRBAC(authorizationInfo).getRole();
final IndicesAccessControl accessControl = role.authorize(action, indices, aliasAndIndexLookup, fieldPermissionsCache);

Set<String> effectiveIndices = null;
if (indices.size() > 1) {
// When there are multiple requested indices, attempt to cut down the number of indices for building indicesAccessControl
// This is possible only if the request actually targets a single concrete index.
final String targetIndexName = getSingleTargetIndexName(request);
if (targetIndexName != null) {
final IndexAbstraction targetIndexAbstraction = aliasAndIndexLookup.get(targetIndexName);
assert targetIndexAbstraction.getType() == IndexAbstraction.Type.CONCRETE_INDEX : "must be a concrete index";
// There are two methods to filter for the list of names that must have indicesAccessControl
// 1. begin with the single target index and check whether itself and all its aliases are part of the requested names
// 2. Loop through the requested names and pick those that are the target index name or any of its aliases
// Method 1 is generally better in terms of performance since it begins with a known size of 1.
// But it can be worse if there are a large number of aliases pointing to the index.
// Method 2 is safer in terms of performance because the loop is not more expensive than do nothing,
// i.e. computing indicesAccessControl for all requested names.
// The code uses a simple heuristic to choose between the two: if the total number of aliases is less
// than the number of requested names, it uses Method 1. Otherwise, it uses Method 2.
int totalAliases = 0;
totalAliases += targetIndexAbstraction.getIndices().get(0).getAliases().size();
Copy link
Contributor

@tvernum tvernum Oct 7, 2021

Choose a reason for hiding this comment

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

This bothers me.
Can we add getIndexMetadata to IndexAbstraction.Index so we don't have to rely on the implementation detail of pulling a index out of the singleton list?

Or add getNumberOfAliases to IndexAbstraction? (I assume we don't call getAliases().size() because it's not very efficient).

Or we could make IndexAbstraction.getAliases() return Collection<String> and make IndexAbstraction.Index.getAliases cheaper to call.

Actually, since we call getAliases below (in method 1) we should just call it here and keep a local copy of the result.

final IndexAbstraction.DataStream parentDataStream = targetIndexAbstraction.getParentDataStream();
if (parentDataStream != null) {
totalAliases += parentDataStream.getAliases().size();
}
if (totalAliases < indices.size()) {
// Method 1
effectiveIndices = new HashSet<>();
Copy link
Contributor

@tvernum tvernum Oct 7, 2021

Choose a reason for hiding this comment

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

Suggested change
effectiveIndices = new HashSet<>();
effectiveIndices = new HashSet<>(totalAliases + 2);
// aliases + targetIndexName + parentDataStream

if (indices.contains(targetIndexName)) {
effectiveIndices.add(targetIndexName);
}
targetIndexAbstraction.getAliases().stream().filter(indices::contains).forEach(effectiveIndices::add);
if (parentDataStream != null) {
if (indices.contains(parentDataStream.getName())) {
effectiveIndices.add(parentDataStream.getName());
}
parentDataStream.getAliases().stream().filter(indices::contains).forEach(effectiveIndices::add);
}
effectiveIndices = Set.copyOf(effectiveIndices);
} else {
// Method 2
effectiveIndices = indices.stream().filter(name -> {
if (name.equals(targetIndexName)) {
return true;
}
final IndexAbstraction indexAbstraction = aliasAndIndexLookup.get(name);
if (indexAbstraction.getType() != IndexAbstraction.Type.CONCRETE_INDEX) {
return indexAbstraction.getIndices().stream().anyMatch(im -> im.getIndex().getName().equals(targetIndexName));
}
return false;
}).collect(Collectors.toUnmodifiableSet());
Comment on lines +678 to +687
Copy link
Contributor

@tvernum tvernum Oct 7, 2021

Choose a reason for hiding this comment

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

Collectors.toUnmodifiableSet() is very inefficient. If you're trying to squeeze the milliseconds here, you're better off constructing the set by hand like in method 1.

Suggested change
effectiveIndices = indices.stream().filter(name -> {
if (name.equals(targetIndexName)) {
return true;
}
final IndexAbstraction indexAbstraction = aliasAndIndexLookup.get(name);
if (indexAbstraction.getType() != IndexAbstraction.Type.CONCRETE_INDEX) {
return indexAbstraction.getIndices().stream().anyMatch(im -> im.getIndex().getName().equals(targetIndexName));
}
return false;
}).collect(Collectors.toUnmodifiableSet());
List<String> indexNames = new ArrayList<>(indices.size());
indices.stream().filter(name -> {
if (name.equals(targetIndexName)) {
return true;
}
final IndexAbstraction indexAbstraction = aliasAndIndexLookup.get(name);
if (indexAbstraction.getType() != IndexAbstraction.Type.CONCRETE_INDEX) {
return indexAbstraction.getIndices().stream().anyMatch(im -> im.getIndex().getName().equals(targetIndexName));
}
return false;
}).forEach(effectiveIndices::add);
effectiveIndices = Set.copyOf(indexNames);

I've suggested using a list to collect the names here, because I think it's going to be more efficient (but I haven't benchmarked it).
Set.copyOf constructs a new HashSet even if the argument is already a HashSet, so you're better off just using an ArrayList for raw efficiency while collecting and then paying the uniqueness cost once during copyOf.

}
}
}
// Fallback to the requested indices if optimisation is not possible or necessary
if (effectiveIndices == null) {
effectiveIndices = indices;
}

final IndicesAccessControl accessControl =
role.authorize(action, Set.copyOf(effectiveIndices), aliasAndIndexLookup, fieldPermissionsCache);
Copy link
Contributor

@tvernum tvernum Oct 7, 2021

Choose a reason for hiding this comment

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

If we're going to introduce a copyOf here, then we should change the caller to use Set.of rather than Sets.newHashSet and reduce the amount of copying that happens.

return new IndexAuthorizationResult(true, accessControl);
}

// An allowlist of requests that have only a single concrete target index regardless of the original indices
private String getSingleTargetIndexName(TransportRequest request) {
if (request instanceof ShardSearchRequest) {
return ((ShardSearchRequest) request).shardId().getIndexName();
} else if (request instanceof FieldCapabilitiesIndexRequest) {
return ((FieldCapabilitiesIndexRequest) request).index();
} else if (request instanceof ShardFetchSearchRequest) {
return ((ShardFetchSearchRequest) request).getShardSearchRequest().shardId().getIndexName();
}
return null;
Copy link
Contributor

@tvernum tvernum Oct 7, 2021

Choose a reason for hiding this comment

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

Can we implement something in core that unifies these? It seems like

public interface ShardIndicesRequest extends IndicesRequest {
    public ShardId shardId();
}

would be helpful.

}

private static RBACAuthorizationInfo ensureRBAC(AuthorizationInfo authorizationInfo) {
if (authorizationInfo instanceof RBACAuthorizationInfo == false) {
throw new IllegalArgumentException("unsupported authorization info:" + authorizationInfo.getClass().getSimpleName());
Expand Down