Skip to content

Commit

Permalink
Refactor to remove the IndexAccessControl#isGranted flag (#90141)
Browse files Browse the repository at this point in the history
This is a refactoring of the `IndicesAccessControl` class intended to
remove the `isGranted` flag from each of the individual index resources.

In almost all cases the flag is redundant (and is `true`):  * when the
request is authorized (`IndicesAccessControl#isGranted`  is `true`), the
`isGranted` flags, for all the individual index resources,  must be
`true` * when the request is unauthorized the `isGranted` flag can be
either `false` or `true` but the request is promptly rejected and so the
flag has a short lifetime (basically it is available for the phrasing of
the access denied exception).

`IndexAccessControl` instances that could _theoretically_ have had
`isGranted` as `false` are replaced by `null`. The code already handled
nulls in the place of `IndexAccessControl` instances as if they were
unauthorized.
  • Loading branch information
albertzaharovits committed Sep 29, 2022
1 parent f746450 commit c3a1cb3
Show file tree
Hide file tree
Showing 34 changed files with 366 additions and 393 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,13 @@
import org.elasticsearch.common.util.concurrent.ThreadContext.StoredContext;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.node.Node;
import org.elasticsearch.search.internal.ReaderContext;
import org.elasticsearch.xpack.core.security.authc.Authentication;
import org.elasticsearch.xpack.core.security.authc.support.AuthenticationContextSerializer;
import org.elasticsearch.xpack.core.security.authc.support.SecondaryAuthentication;
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine;
import org.elasticsearch.xpack.core.security.authz.AuthorizationServiceField;
import org.elasticsearch.xpack.core.security.authz.accesscontrol.IndicesAccessControl;
import org.elasticsearch.xpack.core.security.user.SystemUser;
import org.elasticsearch.xpack.core.security.user.User;

Expand Down Expand Up @@ -100,6 +103,27 @@ public ThreadContext getThreadContext() {
return threadContext;
}

public void putIndicesAccessControl(@Nullable IndicesAccessControl indicesAccessControl) {
if (indicesAccessControl != null) {
if (indicesAccessControl.isGranted() == false) {
throw new IllegalStateException("Unexpected unauthorized access control :" + indicesAccessControl);
}
threadContext.putTransient(AuthorizationServiceField.INDICES_PERMISSIONS_KEY, indicesAccessControl);
}
}

public void copyIndicesAccessControlToReaderContext(ReaderContext readerContext) {
IndicesAccessControl indicesAccessControl = getThreadContext().getTransient(AuthorizationServiceField.INDICES_PERMISSIONS_KEY);
assert indicesAccessControl != null : "thread context does not contain index access control";
readerContext.putInContext(AuthorizationServiceField.INDICES_PERMISSIONS_KEY, indicesAccessControl);
}

public void copyIndicesAccessControlFromReaderContext(ReaderContext readerContext) {
IndicesAccessControl scrollIndicesAccessControl = readerContext.getFromContext(AuthorizationServiceField.INDICES_PERMISSIONS_KEY);
assert scrollIndicesAccessControl != null : "scroll does not contain index access control";
getThreadContext().putTransient(AuthorizationServiceField.INDICES_PERMISSIONS_KEY, scrollIndicesAccessControl);
}

/**
* Sets the user forcefully to the provided user. There must not be an existing user in the ThreadContext otherwise an exception
* will be thrown. This method is package private for testing.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.ActionRequestValidationException;
import org.elasticsearch.action.IndicesRequest;
import org.elasticsearch.cluster.metadata.IndexAbstraction;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.io.stream.StreamInput;
Expand All @@ -31,6 +32,7 @@
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.stream.Collectors;

import static org.elasticsearch.action.ValidateActions.addValidationError;

Expand Down Expand Up @@ -491,6 +493,14 @@ public String toString() {
+ originatingAuthorizationContext
+ "]}";
}

@Nullable
public static String[] indices(TransportRequest transportRequest) {
if (transportRequest instanceof final IndicesRequest indicesRequest) {
return indicesRequest.indices();
}
return null;
}
}

/**
Expand Down Expand Up @@ -526,7 +536,7 @@ public boolean isAuditable() {
* Returns additional context about an authorization failure, if {@link #isGranted()} is false.
*/
@Nullable
public String getFailureContext(RestrictedIndices restrictedIndices) {
public String getFailureContext(RequestInfo requestInfo, RestrictedIndices restrictedIndices) {
return null;
}

Expand Down Expand Up @@ -560,11 +570,22 @@ public IndexAuthorizationResult(boolean auditable, IndicesAccessControl indicesA
}

@Override
public String getFailureContext(RestrictedIndices restrictedIndices) {
public String getFailureContext(RequestInfo requestInfo, RestrictedIndices restrictedIndices) {
if (isGranted()) {
return null;
} else {
return getFailureDescription(indicesAccessControl.getDeniedIndices(), restrictedIndices);
assert indicesAccessControl != null;
String[] indices = RequestInfo.indices(requestInfo.getRequest());
if (indices == null
|| indices.length == 0
|| Arrays.equals(IndicesAndAliasesResolverField.NO_INDICES_OR_ALIASES_ARRAY, indices)) {
return null;
}
Set<String> deniedIndices = Arrays.asList(indices)
.stream()
.filter(index -> false == indicesAccessControl.hasIndexPermissions(index))
.collect(Collectors.toSet());
return getFailureDescription(deniedIndices, restrictedIndices);
}
}

Expand All @@ -591,6 +612,7 @@ public static String getFailureDescription(Collection<String> deniedIndices, Res
return message.toString();
}

@Nullable
public IndicesAccessControl getIndicesAccessControl() {
return indicesAccessControl;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,15 @@
*/
package org.elasticsearch.xpack.core.security.authz;

import java.util.Arrays;
import java.util.List;

public final class IndicesAndAliasesResolverField {
// placeholder used in the security plugin to indicate that the request is authorized knowing that it will yield an empty response
public static final String NO_INDEX_PLACEHOLDER = "-*";
// `*,-*` what we replace indices and aliases with if we need Elasticsearch to return empty responses without throwing exception
public static final String[] NO_INDICES_OR_ALIASES_ARRAY = new String[] { "*", "-*" };
public static final List<String> NO_INDICES_OR_ALIASES_LIST = Arrays.asList(NO_INDICES_OR_ALIASES_ARRAY);

private IndicesAndAliasesResolverField() {}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,12 @@
import org.elasticsearch.xpack.core.security.support.CacheKey;

import java.io.IOException;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.function.Predicate;
import java.util.stream.Collectors;

/**
* Encapsulates the field and document permissions per concrete index based on the current request.
Expand All @@ -33,10 +31,7 @@ public class IndicesAccessControl {

public static final IndicesAccessControl ALLOW_NO_INDICES = new IndicesAccessControl(
true,
Collections.singletonMap(
IndicesAndAliasesResolverField.NO_INDEX_PLACEHOLDER,
new IndicesAccessControl.IndexAccessControl(true, new FieldPermissions(), DocumentPermissions.allowAll())
)
Collections.singletonMap(IndicesAndAliasesResolverField.NO_INDEX_PLACEHOLDER, IndexAccessControl.ALLOW_ALL)
);
public static final IndicesAccessControl DENIED = new IndicesAccessControl(false, Collections.emptyMap());

Expand All @@ -57,21 +52,17 @@ public IndexAccessControl getIndexPermissions(String index) {
return indexPermissions.get(index);
}

public boolean hasIndexPermissions(String index) {
return getIndexPermissions(index) != null;
}

/**
* @return Whether any role / permission group is allowed to access all indices.
*/
public boolean isGranted() {
return granted;
}

public Collection<String> getDeniedIndices() {
return this.indexPermissions.entrySet()
.stream()
.filter(e -> e.getValue().granted == false)
.map(Map.Entry::getKey)
.collect(Collectors.toUnmodifiableSet());
}

public DlsFlsUsage getFieldAndDocumentLevelSecurityUsage() {
boolean hasFls = false;
boolean hasDls = false;
Expand Down Expand Up @@ -154,23 +145,16 @@ public boolean hasDocumentLevelSecurity() {
*/
public static class IndexAccessControl implements CacheKey {

private final boolean granted;
public static final IndexAccessControl ALLOW_ALL = new IndexAccessControl(null, null);

private final FieldPermissions fieldPermissions;
private final DocumentPermissions documentPermissions;

public IndexAccessControl(boolean granted, FieldPermissions fieldPermissions, DocumentPermissions documentPermissions) {
this.granted = granted;
public IndexAccessControl(FieldPermissions fieldPermissions, DocumentPermissions documentPermissions) {
this.fieldPermissions = (fieldPermissions == null) ? FieldPermissions.DEFAULT : fieldPermissions;
this.documentPermissions = (documentPermissions == null) ? DocumentPermissions.allowAll() : documentPermissions;
}

/**
* @return Whether any role / permission group is allowed to this index.
*/
public boolean isGranted() {
return granted;
}

/**
* @return The allowed fields for this index permissions.
*/
Expand All @@ -182,7 +166,6 @@ public FieldPermissions getFieldPermissions() {
* @return The allowed documents expressed as a query for this index permission. If <code>null</code> is returned
* then this means that there are no document level restrictions
*/
@Nullable
public DocumentPermissions getDocumentPermissions() {
return documentPermissions;
}
Expand All @@ -199,31 +182,18 @@ public DocumentPermissions getDocumentPermissions() {
* @see DocumentPermissions#limitDocumentPermissions(DocumentPermissions)
*/
public IndexAccessControl limitIndexAccessControl(IndexAccessControl limitedByIndexAccessControl) {
final boolean isGranted;
if (this.granted == limitedByIndexAccessControl.granted) {
isGranted = this.granted;
} else {
isGranted = false;
}
FieldPermissions constrainedFieldPermissions = getFieldPermissions().limitFieldPermissions(
limitedByIndexAccessControl.fieldPermissions
);
DocumentPermissions constrainedDocumentPermissions = getDocumentPermissions().limitDocumentPermissions(
limitedByIndexAccessControl.getDocumentPermissions()
);
return new IndexAccessControl(isGranted, constrainedFieldPermissions, constrainedDocumentPermissions);
return new IndexAccessControl(constrainedFieldPermissions, constrainedDocumentPermissions);
}

@Override
public String toString() {
return "IndexAccessControl{"
+ "granted="
+ granted
+ ", fieldPermissions="
+ fieldPermissions
+ ", documentPermissions="
+ documentPermissions
+ '}';
return "IndexAccessControl{" + "fieldPermissions=" + fieldPermissions + ", documentPermissions=" + documentPermissions + '}';
}

@Override
Expand All @@ -247,14 +217,12 @@ public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
IndexAccessControl that = (IndexAccessControl) o;
return granted == that.granted
&& Objects.equals(fieldPermissions, that.fieldPermissions)
&& Objects.equals(documentPermissions, that.documentPermissions);
return Objects.equals(fieldPermissions, that.fieldPermissions) && Objects.equals(documentPermissions, that.documentPermissions);
}

@Override
public int hashCode() {
return Objects.hash(granted, fieldPermissions, documentPermissions);
return Objects.hash(fieldPermissions, documentPermissions);
}
}

Expand Down Expand Up @@ -304,15 +272,13 @@ private static class AllowAllIndicesAccessControl extends IndicesAccessControl {

private static final IndicesAccessControl INSTANCE = new AllowAllIndicesAccessControl();

private final IndexAccessControl allowAllIndexAccessControl = new IndexAccessControl(true, null, null);

private AllowAllIndicesAccessControl() {
super(true, Map.of());
}

@Override
public IndexAccessControl getIndexPermissions(String index) {
return allowAllIndexAccessControl;
return IndexAccessControl.ALLOW_ALL;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ public DirectoryReader apply(final DirectoryReader reader) {

try {
final IndicesAccessControl indicesAccessControl = getIndicesAccessControl();
assert indicesAccessControl.isGranted();

ShardId shardId = ShardUtils.extractShardId(reader);
if (shardId == null) {
Expand All @@ -88,7 +89,7 @@ public DirectoryReader apply(final DirectoryReader reader) {

DirectoryReader wrappedReader = reader;
DocumentPermissions documentPermissions = permissions.getDocumentPermissions();
if (documentPermissions != null && documentPermissions.hasDocumentLevelPermissions()) {
if (documentPermissions.hasDocumentLevelPermissions()) {
BooleanQuery filterQuery = documentPermissions.filter(getUser(), scriptService, shardId, searchExecutionContextProvider);
if (filterQuery != null) {
wrappedReader = DocumentSubsetReader.wrap(wrappedReader, bitsetCache, new ConstantScoreQuery(filterQuery));
Expand Down

0 comments on commit c3a1cb3

Please sign in to comment.