Skip to content

Commit

Permalink
[Backport] Add more context to index access denied errors (elastic#66878
Browse files Browse the repository at this point in the history
)

Access denied messages for indices were overly brief and missed two
pieces of useful information:

1. The names of the indices for which access was denied
2. The privileges that could be used to grant that access

This change improves the access denied messages for index based
actions by adding the index and privilege names.
Privilege names are listed in order from least-privilege to
most-privileged so that the first recommended path to resolution is
also the lowest privilege change.

Backport of: elastic#60357
  • Loading branch information
tvernum authored Dec 31, 2020
1 parent 5dfaae8 commit 7cd00ba
Show file tree
Hide file tree
Showing 11 changed files with 309 additions and 46 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.util.Iterator;
import java.util.List;
import java.util.Objects;
import java.util.function.Predicate;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;

Expand Down Expand Up @@ -103,6 +104,17 @@ public static <T> T get(Iterable<T> iterable, int position) {
}
}

public static <T> int indexOf(Iterable<T> iterable, Predicate<T> predicate) {
int i = 0;
for (T element : iterable) {
if (predicate.test(element)) {
return i;
}
i++;
}
return -1;
}

public static long size(Iterable<?> iterable) {
return StreamSupport.stream(iterable.spliterator(), true).count();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@
import java.util.Iterator;
import java.util.List;
import java.util.NoSuchElementException;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static org.hamcrest.Matchers.is;
import static org.hamcrest.object.HasToString.hasToString;

public class IterablesTests extends ESTestCase {
Expand Down Expand Up @@ -86,6 +89,19 @@ public void testFlatten() {
assertEquals(1, count);
}

public void testIndexOf() {
final List<String> list = Stream.generate(() -> randomAlphaOfLengthBetween(3, 9))
.limit(randomIntBetween(10, 30))
.distinct()
.collect(Collectors.toList());
for (int i = 0; i < list.size(); i++) {
final String val = list.get(i);
assertThat(Iterables.indexOf(list, val::equals), is(i));
}
assertThat(Iterables.indexOf(list, s -> false), is(-1));
assertThat(Iterables.indexOf(list, s -> true), is(0));
}

private void test(Iterable<String> iterable) {
try {
Iterables.get(iterable, -1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

import org.elasticsearch.action.ActionListener;
import org.elasticsearch.cluster.metadata.IndexAbstraction;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.Strings;
import org.elasticsearch.transport.TransportRequest;
import org.elasticsearch.xpack.core.security.action.user.GetUserPrivilegesRequest;
import org.elasticsearch.xpack.core.security.action.user.GetUserPrivilegesResponse;
Expand Down Expand Up @@ -292,6 +294,14 @@ public boolean isAuditable() {
return auditable;
}

/**
* Returns additional context about an authorization failure, if {@link #isGranted()} is false.
*/
@Nullable
public String getFailureContext() {
return null;
}

/**
* Returns a new authorization result that is granted and auditable
*/
Expand Down Expand Up @@ -321,6 +331,19 @@ public IndexAuthorizationResult(boolean auditable, IndicesAccessControl indicesA
this.indicesAccessControl = indicesAccessControl;
}

@Override
public String getFailureContext() {
if (isGranted()) {
return null;
} else {
return getFailureDescription(indicesAccessControl.getDeniedIndices());
}
}

public static String getFailureDescription(Collection<?> deniedIndices) {
return "on indices [" + Strings.collectionToCommaDelimitedString(deniedIndices) + "]";
}

public IndicesAccessControl getIndicesAccessControl() {
return indicesAccessControl;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,12 @@
import org.elasticsearch.xpack.core.security.authz.permission.DocumentPermissions;
import org.elasticsearch.xpack.core.security.authz.permission.FieldPermissions;

import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

/**
* Encapsulates the field and document permissions per concrete index based on the current request.
Expand Down Expand Up @@ -51,6 +53,13 @@ public boolean isGranted() {
return granted;
}

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

/**
* Encapsulates the field and document permissions for an index.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,36 +14,39 @@
import org.elasticsearch.action.admin.indices.close.CloseIndexAction;
import org.elasticsearch.action.admin.indices.create.AutoCreateAction;
import org.elasticsearch.action.admin.indices.create.CreateIndexAction;
import org.elasticsearch.action.admin.indices.resolve.ResolveIndexAction;
import org.elasticsearch.xpack.core.action.CreateDataStreamAction;
import org.elasticsearch.xpack.core.action.DeleteDataStreamAction;
import org.elasticsearch.xpack.core.action.GetDataStreamAction;
import org.elasticsearch.action.admin.indices.delete.DeleteIndexAction;
import org.elasticsearch.action.admin.indices.exists.indices.IndicesExistsAction;
import org.elasticsearch.action.admin.indices.exists.types.TypesExistsAction;
import org.elasticsearch.action.admin.indices.get.GetIndexAction;
import org.elasticsearch.action.admin.indices.mapping.get.GetFieldMappingsAction;
import org.elasticsearch.action.admin.indices.mapping.get.GetMappingsAction;
import org.elasticsearch.action.admin.indices.mapping.put.AutoPutMappingAction;
import org.elasticsearch.action.admin.indices.resolve.ResolveIndexAction;
import org.elasticsearch.action.admin.indices.settings.get.GetSettingsAction;
import org.elasticsearch.action.admin.indices.validate.query.ValidateQueryAction;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.collect.MapBuilder;
import org.elasticsearch.xpack.core.action.CreateDataStreamAction;
import org.elasticsearch.xpack.core.action.DeleteDataStreamAction;
import org.elasticsearch.xpack.core.action.GetDataStreamAction;
import org.elasticsearch.xpack.core.ccr.action.ForgetFollowerAction;
import org.elasticsearch.xpack.core.ccr.action.PutFollowAction;
import org.elasticsearch.xpack.core.ccr.action.UnfollowAction;
import org.elasticsearch.xpack.core.ilm.action.ExplainLifecycleAction;
import org.elasticsearch.xpack.core.security.support.Automatons;

import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.Predicate;
import java.util.stream.Collectors;

import static org.elasticsearch.common.collect.Map.ofEntries;
import static org.elasticsearch.common.collect.Map.entry;
import static org.elasticsearch.xpack.core.security.support.Automatons.patterns;
import static org.elasticsearch.xpack.core.security.support.Automatons.unionAndMinimize;

Expand Down Expand Up @@ -99,27 +102,26 @@ public final class IndexPrivilege extends Privilege {
public static final IndexPrivilege MAINTENANCE = new IndexPrivilege("maintenance", MAINTENANCE_AUTOMATON);
public static final IndexPrivilege AUTO_CONFIGURE = new IndexPrivilege("auto_configure", AUTO_CONFIGURE_AUTOMATON);

private static final Map<String, IndexPrivilege> VALUES = MapBuilder.<String, IndexPrivilege>newMapBuilder()
.put("none", NONE)
.put("all", ALL)
.put("manage", MANAGE)
.put("create_index", CREATE_INDEX)
.put("monitor", MONITOR)
.put("read", READ)
.put("index", INDEX)
.put("delete", DELETE)
.put("write", WRITE)
.put("create", CREATE)
.put("create_doc", CREATE_DOC)
.put("delete_index", DELETE_INDEX)
.put("view_index_metadata", VIEW_METADATA)
.put("read_cross_cluster", READ_CROSS_CLUSTER)
.put("manage_follow_index", MANAGE_FOLLOW_INDEX)
.put("manage_leader_index", MANAGE_LEADER_INDEX)
.put("manage_ilm", MANAGE_ILM)
.put("maintenance", MAINTENANCE)
.put("auto_configure", AUTO_CONFIGURE)
.immutableMap();
private static final Map<String, IndexPrivilege> VALUES = sortByAccessLevel(ofEntries(
entry("none", NONE),
entry("all", ALL),
entry("manage", MANAGE),
entry("create_index", CREATE_INDEX),
entry("monitor", MONITOR),
entry("read", READ),
entry("index", INDEX),
entry("delete", DELETE),
entry("write", WRITE),
entry("create", CREATE),
entry("create_doc", CREATE_DOC),
entry("delete_index", DELETE_INDEX),
entry("view_index_metadata", VIEW_METADATA),
entry("read_cross_cluster", READ_CROSS_CLUSTER),
entry("manage_follow_index", MANAGE_FOLLOW_INDEX),
entry("manage_leader_index", MANAGE_LEADER_INDEX),
entry("manage_ilm", MANAGE_ILM),
entry("maintenance", MAINTENANCE),
entry("auto_configure", AUTO_CONFIGURE)));

public static final Predicate<String> ACTION_MATCHER = ALL.predicate();
public static final Predicate<String> CREATE_INDEX_MATCHER = CREATE_INDEX.predicate();
Expand Down Expand Up @@ -157,7 +159,7 @@ private static IndexPrivilege resolve(Set<String> name) {
if (ACTION_MATCHER.test(part)) {
actions.add(actionToPattern(part));
} else {
IndexPrivilege indexPrivilege = VALUES.get(part);
IndexPrivilege indexPrivilege = part == null ? null : VALUES.get(part);
if (indexPrivilege != null && size == 1) {
return indexPrivilege;
} else if (indexPrivilege != null) {
Expand Down Expand Up @@ -187,4 +189,16 @@ public static Set<String> names() {
return Collections.unmodifiableSet(VALUES.keySet());
}

/**
* Returns the names of privileges that grant the specified action.
* @return A collection of names, ordered (to the extent possible) from least privileged (e.g. {@link #CREATE_DOC})
* to most privileged (e.g. {@link #ALL})
* @see Privilege#sortByAccessLevel
*/
public static Collection<String> findPrivilegesThatGrant(String action) {
return Collections.unmodifiableList(VALUES.entrySet().stream()
.filter(e -> e.getValue().predicate.test(action))
.map(e -> e.getKey())
.collect(Collectors.toList()));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,16 @@
package org.elasticsearch.xpack.core.security.authz.privilege;

import org.apache.lucene.util.automaton.Automaton;
import org.apache.lucene.util.automaton.Operations;
import org.elasticsearch.xpack.core.security.support.Automatons;

import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.SortedMap;
import java.util.TreeMap;
import java.util.function.Predicate;

import static org.elasticsearch.xpack.core.security.support.Automatons.patterns;
Expand Down Expand Up @@ -74,4 +80,21 @@ public String toString() {
public Automaton getAutomaton() {
return automaton;
}

/**
* Sorts the map of privileges from least-privilege to most-privilege
*/
static <T extends Privilege> SortedMap<String, T> sortByAccessLevel(Map<String, T> privileges) {
// How many other privileges is this privilege a subset of. Those with a higher count are considered to be a lower privilege
final Map<String, Long> subsetCount = new HashMap<>(privileges.size());
privileges.forEach((name, priv) -> subsetCount.put(name,
privileges.values().stream().filter(p2 -> p2 != priv && Operations.subsetOf(priv.automaton, p2.automaton)).count())
);

final Comparator<String> compare = Comparator.<String>comparingLong(key -> subsetCount.getOrDefault(key, 0L)).reversed()
.thenComparing(Comparator.naturalOrder());
final TreeMap<String, T> tree = new TreeMap<>(compare);
tree.putAll(privileges);
return Collections.unmodifiableSortedMap(tree);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

package org.elasticsearch.xpack.core.security.authz.privilege;

import org.elasticsearch.action.admin.indices.refresh.RefreshAction;
import org.elasticsearch.action.admin.indices.shrink.ShrinkAction;
import org.elasticsearch.action.admin.indices.stats.IndicesStatsAction;
import org.elasticsearch.action.delete.DeleteAction;
import org.elasticsearch.action.index.IndexAction;
import org.elasticsearch.action.search.SearchAction;
import org.elasticsearch.action.update.UpdateAction;
import org.elasticsearch.common.util.iterable.Iterables;
import org.elasticsearch.test.ESTestCase;

import org.elasticsearch.common.collect.List;
import java.util.Set;

import static org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege.findPrivilegesThatGrant;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.lessThan;

public class IndexPrivilegeTests extends ESTestCase {

/**
* The {@link IndexPrivilege#values()} map is sorted so that privilege names that offer the _least_ access come before those that
* offer _more_ access. There is no guarantee of ordering between privileges that offer non-overlapping privileges.
*/
public void testOrderingOfPrivilegeNames() throws Exception {
final Set<String> names = IndexPrivilege.values().keySet();
final int all = Iterables.indexOf(names, "all"::equals);
final int manage = Iterables.indexOf(names, "manage"::equals);
final int monitor = Iterables.indexOf(names, "monitor"::equals);
final int read = Iterables.indexOf(names, "read"::equals);
final int write = Iterables.indexOf(names, "write"::equals);
final int index = Iterables.indexOf(names, "index"::equals);
final int create_doc = Iterables.indexOf(names, "create_doc"::equals);
final int delete = Iterables.indexOf(names, "delete"::equals);

assertThat(read, lessThan(all));
assertThat(manage, lessThan(all));
assertThat(monitor, lessThan(manage));
assertThat(write, lessThan(all));
assertThat(index, lessThan(write));
assertThat(create_doc, lessThan(index));
assertThat(delete, lessThan(write));
}

public void testFindPrivilegesThatGrant() {
assertThat(findPrivilegesThatGrant(SearchAction.NAME), equalTo(List.of("read", "all")));
assertThat(findPrivilegesThatGrant(IndexAction.NAME), equalTo(List.of("create_doc", "create", "index", "write", "all")));
assertThat(findPrivilegesThatGrant(UpdateAction.NAME), equalTo(List.of("index", "write", "all")));
assertThat(findPrivilegesThatGrant(DeleteAction.NAME), equalTo(List.of("delete", "write", "all")));
assertThat(findPrivilegesThatGrant(IndicesStatsAction.NAME), equalTo(List.of("monitor", "manage", "all")));
assertThat(findPrivilegesThatGrant(RefreshAction.NAME), equalTo(List.of("maintenance", "manage", "all")));
assertThat(findPrivilegesThatGrant(ShrinkAction.NAME), equalTo(List.of("manage", "all")));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,8 @@ public void testCanManageIndexWithNoPermissions() throws Exception {
assertThat(indexExplain.get("failed_step"), equalTo("wait-for-shard-history-leases"));
Map<String, String> stepInfo = (Map<String, String>) indexExplain.get("step_info");
assertThat(stepInfo.get("type"), equalTo("security_exception"));
assertThat(stepInfo.get("reason"), equalTo("action [indices:monitor/stats] is unauthorized for user [test_ilm]"));
assertThat(stepInfo.get("reason"), equalTo("action [indices:monitor/stats] is unauthorized for user [test_ilm]" +
" on indices [not-ilm], this action is granted by the privileges [monitor,manage,all]"));
}
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -818,7 +818,7 @@ public void testLookbackWithoutPermissions() throws Exception {
new Request("GET", NotificationsIndex.NOTIFICATIONS_INDEX + "/_search?size=1000&q=job_id:" + jobId));
String notificationsResponseAsString = EntityUtils.toString(notificationsResponse.getEntity());
assertThat(notificationsResponseAsString, containsString("\"message\":\"Datafeed is encountering errors extracting data: " +
"action [indices:data/read/search] is unauthorized for user [ml_admin_plus_data]\""));
"action [indices:data/read/search] is unauthorized for user [ml_admin_plus_data] on indices [network-data],"));
}

public void testLookbackWithPipelineBucketAgg() throws Exception {
Expand Down Expand Up @@ -966,7 +966,8 @@ public void testLookbackWithoutPermissionsAndRollup() throws Exception {
new Request("GET", NotificationsIndex.NOTIFICATIONS_INDEX + "/_search?size=1000&q=job_id:" + jobId));
String notificationsResponseAsString = EntityUtils.toString(notificationsResponse.getEntity());
assertThat(notificationsResponseAsString, containsString("\"message\":\"Datafeed is encountering errors extracting data: " +
"action [indices:data/read/xpack/rollup/search] is unauthorized for user [ml_admin_plus_data]\""));
"action [indices:data/read/xpack/rollup/search] is unauthorized for user [ml_admin_plus_data] " +
"on indices [airline-data-aggs-rollup], "));
}

public void testLookbackWithSingleBucketAgg() throws Exception {
Expand Down
Loading

0 comments on commit 7cd00ba

Please sign in to comment.