From 7cd00baf686500ce0d03a131b802865da719e9a0 Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Thu, 31 Dec 2020 18:32:38 +1100 Subject: [PATCH] [Backport] Add more context to index access denied errors (#66878) 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: #60357 --- .../common/util/iterable/Iterables.java | 12 +++ .../common/util/iterable/IterablesTests.java | 16 ++++ .../security/authz/AuthorizationEngine.java | 23 ++++++ .../accesscontrol/IndicesAccessControl.java | 9 +++ .../authz/privilege/IndexPrivilege.java | 68 +++++++++------- .../security/authz/privilege/Privilege.java | 23 ++++++ .../authz/privilege/IndexPrivilegeTests.java | 62 ++++++++++++++ .../xpack/security/PermissionsIT.java | 3 +- .../ml/integration/DatafeedJobsRestIT.java | 5 +- .../security/authz/AuthorizationService.java | 54 +++++++++---- .../authz/AuthorizationServiceTests.java | 80 +++++++++++++++++++ 11 files changed, 309 insertions(+), 46 deletions(-) create mode 100644 x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilegeTests.java diff --git a/server/src/main/java/org/elasticsearch/common/util/iterable/Iterables.java b/server/src/main/java/org/elasticsearch/common/util/iterable/Iterables.java index 6d1dab4a9d010..e7c002438c576 100644 --- a/server/src/main/java/org/elasticsearch/common/util/iterable/Iterables.java +++ b/server/src/main/java/org/elasticsearch/common/util/iterable/Iterables.java @@ -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; @@ -103,6 +104,17 @@ public static T get(Iterable iterable, int position) { } } + public static int indexOf(Iterable iterable, Predicate 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(); } diff --git a/server/src/test/java/org/elasticsearch/common/util/iterable/IterablesTests.java b/server/src/test/java/org/elasticsearch/common/util/iterable/IterablesTests.java index 6501c7caa1d64..1d536a3c01d2c 100644 --- a/server/src/test/java/org/elasticsearch/common/util/iterable/IterablesTests.java +++ b/server/src/test/java/org/elasticsearch/common/util/iterable/IterablesTests.java @@ -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 { @@ -86,6 +89,19 @@ public void testFlatten() { assertEquals(1, count); } + public void testIndexOf() { + final List 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 iterable) { try { Iterables.get(iterable, -1); diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/AuthorizationEngine.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/AuthorizationEngine.java index 8b2ae8ec14d7a..275dbdef3ef0a 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/AuthorizationEngine.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/AuthorizationEngine.java @@ -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; @@ -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 */ @@ -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; } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/IndicesAccessControl.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/IndicesAccessControl.java index 604508f95c5af..697df38bd07dc 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/IndicesAccessControl.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/IndicesAccessControl.java @@ -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. @@ -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. */ diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilege.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilege.java index 18768a521185e..f7747b5daa0b6 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilege.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilege.java @@ -14,10 +14,6 @@ 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; @@ -25,10 +21,13 @@ 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; @@ -36,6 +35,7 @@ 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; @@ -43,7 +43,10 @@ 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; @@ -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 VALUES = MapBuilder.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 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 ACTION_MATCHER = ALL.predicate(); public static final Predicate CREATE_INDEX_MATCHER = CREATE_INDEX.predicate(); @@ -157,7 +159,7 @@ private static IndexPrivilege resolve(Set 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) { @@ -187,4 +189,16 @@ public static Set 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 findPrivilegesThatGrant(String action) { + return Collections.unmodifiableList(VALUES.entrySet().stream() + .filter(e -> e.getValue().predicate.test(action)) + .map(e -> e.getKey()) + .collect(Collectors.toList())); + } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/Privilege.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/Privilege.java index 54db92dacae88..2f1eb0728f129 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/Privilege.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/Privilege.java @@ -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; @@ -74,4 +80,21 @@ public String toString() { public Automaton getAutomaton() { return automaton; } + + /** + * Sorts the map of privileges from least-privilege to most-privilege + */ + static SortedMap sortByAccessLevel(Map 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 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 compare = Comparator.comparingLong(key -> subsetCount.getOrDefault(key, 0L)).reversed() + .thenComparing(Comparator.naturalOrder()); + final TreeMap tree = new TreeMap<>(compare); + tree.putAll(privileges); + return Collections.unmodifiableSortedMap(tree); + } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilegeTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilegeTests.java new file mode 100644 index 0000000000000..28e9e230af5be --- /dev/null +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilegeTests.java @@ -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 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"))); + } + +} diff --git a/x-pack/plugin/ilm/qa/with-security/src/javaRestTest/java/org/elasticsearch/xpack/security/PermissionsIT.java b/x-pack/plugin/ilm/qa/with-security/src/javaRestTest/java/org/elasticsearch/xpack/security/PermissionsIT.java index 3ec7ea39f6619..d57384dbaff9c 100644 --- a/x-pack/plugin/ilm/qa/with-security/src/javaRestTest/java/org/elasticsearch/xpack/security/PermissionsIT.java +++ b/x-pack/plugin/ilm/qa/with-security/src/javaRestTest/java/org/elasticsearch/xpack/security/PermissionsIT.java @@ -143,7 +143,8 @@ public void testCanManageIndexWithNoPermissions() throws Exception { assertThat(indexExplain.get("failed_step"), equalTo("wait-for-shard-history-leases")); Map stepInfo = (Map) 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]")); } }); } diff --git a/x-pack/plugin/ml/qa/native-multi-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/ml/integration/DatafeedJobsRestIT.java b/x-pack/plugin/ml/qa/native-multi-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/ml/integration/DatafeedJobsRestIT.java index 24d985ff120a1..2e478cbeb3651 100644 --- a/x-pack/plugin/ml/qa/native-multi-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/ml/integration/DatafeedJobsRestIT.java +++ b/x-pack/plugin/ml/qa/native-multi-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/ml/integration/DatafeedJobsRestIT.java @@ -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 { @@ -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 { diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java index a4b731eef6be2..cc51cc84ef5d9 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java @@ -85,7 +85,9 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Consumer; +import static java.util.Collections.singletonList; import static org.elasticsearch.action.support.ContextPreservingActionListener.wrapPreservingContext; +import static org.elasticsearch.common.Strings.collectionToCommaDelimitedString; import static org.elasticsearch.xpack.core.security.SecurityField.setting; import static org.elasticsearch.xpack.core.security.authz.AuthorizationServiceField.ACTION_SCOPE_AUTHORIZATION_KEYS; import static org.elasticsearch.xpack.core.security.authz.AuthorizationServiceField.AUTHORIZATION_INFO_KEY; @@ -276,7 +278,7 @@ private void authorizeAction(final RequestInfo requestInfo, final String request listener.onResponse(null); }, listener::onFailure, requestInfo, requestId, authzInfo), threadContext); authzEngine.authorizeClusterAction(requestInfo, authzInfo, clusterAuthzListener); - } else if (IndexPrivilege.ACTION_MATCHER.test(action)) { + } else if (isIndexAction(action)) { final Metadata metadata = clusterService.state().metadata(); final AsyncSupplier> authorizedIndicesSupplier = new CachingAsyncSupplier<>(authzIndicesListener -> authzEngine.loadAuthorizedIndices(requestInfo, authzInfo, metadata.getIndicesLookup(), @@ -541,7 +543,8 @@ private void authorizeBulkItems(RequestInfo requestInfo, AuthorizationInfo authz if (indexAccessControl == null || indexAccessControl.isGranted() == false) { auditTrail.explicitIndexAccessEvent(requestId, AuditLevel.ACCESS_DENIED, authentication, itemAction, resolvedIndex, item.getClass().getSimpleName(), request.remoteAddress(), authzInfo); - item.abort(resolvedIndex, denialException(authentication, itemAction, null)); + item.abort(resolvedIndex, denialException(authentication, itemAction, + IndexAuthorizationResult.getFailureDescription(singletonList(resolvedIndex)), null)); } else if (audit.get()) { auditTrail.explicitIndexAccessEvent(requestId, AuditLevel.ACCESS_GRANTED, authentication, itemAction, resolvedIndex, item.getClass().getSimpleName(), request.remoteAddress(), authzInfo); @@ -561,8 +564,8 @@ private void authorizeBulkItems(RequestInfo requestInfo, AuthorizationInfo authz groupedActionListener.onResponse(new Tuple<>(bulkItemAction, indexAuthorizationResult)), groupedActionListener::onFailure)); }); - }, listener::onFailure)); }, listener::onFailure)); + }, listener::onFailure)); } private static IllegalArgumentException illegalArgument(String message) { @@ -570,6 +573,10 @@ private static IllegalArgumentException illegalArgument(String message) { return new IllegalArgumentException(message); } + private static boolean isIndexAction(String action) { + return IndexPrivilege.ACTION_MATCHER.test(action); + } + private static String getAction(BulkItemRequest item) { final DocWriteRequest docWriteRequest = item.request(); switch (docWriteRequest.opType()) { @@ -598,6 +605,11 @@ private void putTransientIfNonExisting(String key, Object value) { } private ElasticsearchSecurityException denialException(Authentication authentication, String action, Exception cause) { + return denialException(authentication, action, null, cause); + } + + private ElasticsearchSecurityException denialException(Authentication authentication, String action, @Nullable String context, + Exception cause) { final User authUser = authentication.getUser().authenticatedUser(); // Special case for anonymous user if (isAnonymousEnabled && anonymousUser.equals(authUser)) { @@ -605,23 +617,33 @@ private ElasticsearchSecurityException denialException(Authentication authentica return authcFailureHandler.authenticationRequired(action, threadContext); } } + + String userText = "user [" + authUser.principal() + "]"; // check for run as if (authentication.getUser().isRunAs()) { - logger.debug("action [{}] is unauthorized for user [{}] run as [{}]", action, authUser.principal(), - authentication.getUser().principal()); - return authorizationError("action [{}] is unauthorized for user [{}] run as [{}]", cause, action, authUser.principal(), - authentication.getUser().principal()); + userText = userText + " run as [" + authentication.getUser().principal() + "]"; } // check for authentication by API key if (AuthenticationType.API_KEY == authentication.getAuthenticationType()) { final String apiKeyId = (String) authentication.getMetadata().get(ApiKeyService.API_KEY_ID_KEY); assert apiKeyId != null : "api key id must be present in the metadata"; - logger.debug("action [{}] is unauthorized for API key id [{}] of user [{}]", action, apiKeyId, authUser.principal()); - return authorizationError("action [{}] is unauthorized for API key id [{}] of user [{}]", cause, action, apiKeyId, - authUser.principal()); + userText = "API key id [" + apiKeyId + "] of " + userText; } - logger.debug("action [{}] is unauthorized for user [{}]", action, authUser.principal()); - return authorizationError("action [{}] is unauthorized for user [{}]", cause, action, authUser.principal()); + + String message = "action [" + action + "] is unauthorized for " + userText; + if (context != null) { + message = message + " " + context; + } + + if(isIndexAction(action)) { + final Collection privileges = IndexPrivilege.findPrivilegesThatGrant(action); + if (privileges != null && privileges.size() > 0) { + message = message + ", this action is granted by the privileges [" + collectionToCommaDelimitedString(privileges) + "]"; + } + } + + logger.debug(message); + return authorizationError(message, cause); } private class AuthorizationResultListener implements ActionListener { @@ -654,21 +676,21 @@ public void onResponse(T result) { failureConsumer.accept(e); } } else { - handleFailure(result.isAuditable(), null); + handleFailure(result.isAuditable(), result.getFailureContext(), null); } } @Override public void onFailure(Exception e) { - handleFailure(true, e); + handleFailure(true, null, e); } - private void handleFailure(boolean audit, @Nullable Exception e) { + private void handleFailure(boolean audit, @Nullable String context, @Nullable Exception e) { if (audit) { auditTrailService.get().accessDenied(requestId, requestInfo.getAuthentication(), requestInfo.getAction(), requestInfo.getRequest(), authzInfo); } - failureConsumer.accept(denialException(requestInfo.getAuthentication(), requestInfo.getAction(), e)); + failureConsumer.accept(denialException(requestInfo.getAuthentication(), requestInfo.getAction(), context, e)); } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java index b06152d13b35e..a4eb1515581d8 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java @@ -44,6 +44,9 @@ import org.elasticsearch.action.bulk.BulkItemRequest; import org.elasticsearch.action.bulk.BulkRequest; import org.elasticsearch.action.bulk.BulkShardRequest; +import org.elasticsearch.action.bulk.BulkShardResponse; +import org.elasticsearch.action.bulk.MappingUpdatePerformer; +import org.elasticsearch.action.bulk.TransportShardBulkAction; import org.elasticsearch.action.delete.DeleteAction; import org.elasticsearch.action.delete.DeleteRequest; import org.elasticsearch.action.get.GetAction; @@ -65,11 +68,13 @@ import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.action.support.PlainActionFuture; import org.elasticsearch.action.support.WriteRequest; +import org.elasticsearch.action.support.replication.TransportReplicationAction; import org.elasticsearch.action.termvectors.MultiTermVectorsAction; import org.elasticsearch.action.termvectors.MultiTermVectorsRequest; import org.elasticsearch.action.termvectors.TermVectorsAction; import org.elasticsearch.action.termvectors.TermVectorsRequest; import org.elasticsearch.action.update.UpdateAction; +import org.elasticsearch.action.update.UpdateHelper; import org.elasticsearch.action.update.UpdateRequest; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.metadata.AliasMetadata; @@ -91,9 +96,11 @@ import org.elasticsearch.common.util.concurrent.ThreadContext.StoredContext; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.index.IndexNotFoundException; +import org.elasticsearch.index.shard.IndexShard; import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.license.XPackLicenseState.Feature; +import org.elasticsearch.script.ScriptService; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportActionProxy; @@ -165,13 +172,17 @@ import java.util.Set; import java.util.UUID; import java.util.concurrent.CountDownLatch; +import java.util.function.Consumer; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Predicate; import static java.util.Arrays.asList; +import static java.util.Collections.singletonMap; import static org.elasticsearch.test.SecurityTestsUtils.assertAuthenticationException; import static org.elasticsearch.test.SecurityTestsUtils.assertThrowsAuthorizationException; import static org.elasticsearch.test.SecurityTestsUtils.assertThrowsAuthorizationExceptionRunAs; +import static org.elasticsearch.xpack.security.audit.logfile.LoggingAuditTrail.PRINCIPAL_ROLES_FIELD_NAME; +import static org.elasticsearch.test.TestMatchers.throwableWithMessage; import static org.elasticsearch.xpack.core.security.authz.AuthorizationServiceField.AUTHORIZATION_INFO_KEY; import static org.elasticsearch.xpack.core.security.authz.AuthorizationServiceField.INDICES_PERMISSIONS_KEY; import static org.elasticsearch.xpack.core.security.authz.AuthorizationServiceField.ORIGINATING_ACTION_KEY; @@ -179,6 +190,7 @@ import static org.elasticsearch.xpack.core.security.index.RestrictedIndicesNames.SECURITY_MAIN_ALIAS; import static org.elasticsearch.xpack.security.audit.logfile.LoggingAuditTrail.PRINCIPAL_ROLES_FIELD_NAME; import static org.hamcrest.Matchers.arrayContainingInAnyOrder; +import static org.hamcrest.Matchers.arrayWithSize; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.endsWith; import static org.hamcrest.Matchers.equalTo; @@ -588,6 +600,7 @@ public void testUserWithNoRolesCannotSql() throws IOException { authzInfoRoles(Role.EMPTY.names())); verifyNoMoreInteractions(auditTrail); } + /** * Verifies that the behaviour tested in {@link #testUserWithNoRolesCanPerformRemoteSearch} * does not work for requests that are not remote-index-capable. @@ -868,6 +881,73 @@ public void testCreateIndexWithAlias() throws IOException { verify(state, times(1)).metadata(); } + public void testDenialErrorMessagesForSearchAction() throws IOException { + RoleDescriptor role = new RoleDescriptor("some_indices_" + randomAlphaOfLengthBetween(3, 6), null, new IndicesPrivileges[]{ + IndicesPrivileges.builder().indices("all*").privileges("all").build(), + IndicesPrivileges.builder().indices("read*").privileges("read").build(), + IndicesPrivileges.builder().indices("write*").privileges("write").build() + }, null); + User user = new User(randomAlphaOfLengthBetween(6, 8), role.getName()); + final Authentication authentication = createAuthentication(user); + roleMap.put(role.getName(), role); + + AuditUtil.getOrGenerateRequestId(threadContext); + + TransportRequest request = new SearchRequest("all-1", "read-2", "write-3", "other-4"); + + ElasticsearchSecurityException securityException = expectThrows(ElasticsearchSecurityException.class, + () -> authorize(authentication, SearchAction.NAME, request)); + assertThat(securityException, throwableWithMessage( + containsString("[" + SearchAction.NAME + "] is unauthorized for user [" + user.principal() + "] on indices ["))); + assertThat(securityException, throwableWithMessage(containsString("write-3"))); + assertThat(securityException, throwableWithMessage(containsString("other-4"))); + assertThat(securityException, throwableWithMessage(not(containsString("all-1")))); + assertThat(securityException, throwableWithMessage(not(containsString("read-2")))); + assertThat(securityException, throwableWithMessage(containsString(", this action is granted by the privileges [read,all]"))); + } + + public void testDenialErrorMessagesForBulkIngest() throws Exception { + final String index = randomAlphaOfLengthBetween(5, 12); + RoleDescriptor role = new RoleDescriptor("some_indices_" + randomAlphaOfLengthBetween(3, 6), null, new IndicesPrivileges[]{ + IndicesPrivileges.builder().indices(index).privileges(BulkAction.NAME).build() + }, null); + User user = new User(randomAlphaOfLengthBetween(6, 8), role.getName()); + final Authentication authentication = createAuthentication(user); + roleMap.put(role.getName(), role); + + AuditUtil.getOrGenerateRequestId(threadContext); + + final BulkShardRequest request = new BulkShardRequest( + new ShardId(index, randomAlphaOfLength(24), 1), + WriteRequest.RefreshPolicy.NONE, + new BulkItemRequest[]{ + new BulkItemRequest(0, + new IndexRequest(index).id("doc-1").opType(DocWriteRequest.OpType.CREATE).source(singletonMap("field", "value"))), + new BulkItemRequest(1, + new IndexRequest(index).id("doc-2").opType(DocWriteRequest.OpType.INDEX).source(singletonMap("field", "value"))), + new BulkItemRequest(2, new DeleteRequest(index, "doc-3")) + }); + + authorize(authentication, TransportShardBulkAction.ACTION_NAME, request); + + MappingUpdatePerformer mappingUpdater = (m, s, t, l) -> l.onResponse(null); + Consumer> waitForMappingUpdate = l -> l.onResponse(null); + PlainActionFuture> future = new PlainActionFuture<>(); + IndexShard indexShard = mock(IndexShard.class); + TransportShardBulkAction.performOnPrimary(request, indexShard, new UpdateHelper(mock(ScriptService.class)), + System::currentTimeMillis, mappingUpdater, waitForMappingUpdate, future, threadPool, ThreadPool.Names.WRITE); + + TransportReplicationAction.PrimaryResult result = future.get(); + BulkShardResponse response = result.finalResponseIfSuccessful; + assertThat(response, notNullValue()); + assertThat(response.getResponses(), arrayWithSize(3)); + assertThat(response.getResponses()[0].getFailureMessage(), containsString("unauthorized for user [" + user.principal() + "]")); + assertThat(response.getResponses()[0].getFailureMessage(), containsString("on indices [" + index + "]")); + assertThat(response.getResponses()[0].getFailureMessage(), containsString("[create_doc,create,index,write,all]") ); + assertThat(response.getResponses()[1].getFailureMessage(), containsString("[create,index,write,all]") ); + assertThat(response.getResponses()[2].getFailureMessage(), containsString("[delete,write,all]") ); + } + public void testDenialForAnonymousUser() throws IOException { TransportRequest request = new GetIndexRequest().indices("b"); ClusterState state = mockEmptyMetadata();