From 6e4a69550d8d83fb0f2c66e805424a3232285708 Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Thu, 11 Feb 2021 09:39:56 +1100 Subject: [PATCH 1/3] Include role names in access denied errors This commit adds a User's list of current role names to access denied error messages to aid in diagnostics. This allows an administrator to know whether the correct course of action is to add another role to the user (e.g. by fixing incorrect role mappings) or by modifying a role to add more privileges. Resolves: #42166 --- .../xpack/security/PermissionsIT.java | 1 + .../security/authz/AuthorizationService.java | 4 +++ .../authz/AuthorizationServiceTests.java | 29 +++++++++++++------ .../test/api_key/11_invalidation.yml | 6 ++-- 4 files changed, 29 insertions(+), 11 deletions(-) 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 acd0809410fce..94d39d2b8dc8a 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 @@ -146,6 +146,7 @@ public void testCanManageIndexWithNoPermissions() throws Exception { assertThat(stepInfo.get("type"), equalTo("security_exception")); assertThat(stepInfo.get("reason"), equalTo("action [indices:monitor/stats] is unauthorized" + " for user [test_ilm]" + + " with roles [ilm]" + " on indices [not-ilm]," + " this action is granted by the index privileges [monitor,manage,all]")); } 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 86e907187514a..4e81f7f3260c5 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 @@ -28,6 +28,7 @@ import org.elasticsearch.cluster.metadata.Metadata; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.Nullable; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; @@ -630,6 +631,9 @@ private ElasticsearchSecurityException denialException(Authentication authentica final String apiKeyId = (String) authentication.getMetadata().get(ApiKeyService.API_KEY_ID_KEY); assert apiKeyId != null : "api key id must be present in the metadata"; userText = "API key id [" + apiKeyId + "] of " + userText; + } else { + // Don't print roles for API keys because they're not meaningful + userText = userText + " with roles [" + Strings.arrayToCommaDelimitedString(authentication.getUser().roles()) + "]"; } String message = "action [" + action + "] is unauthorized for " + userText; 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 546625da6e30d..4949b12b4947d 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 @@ -677,8 +677,11 @@ public void testUnknownRoleCausesDenial() throws IOException { ElasticsearchSecurityException securityException = expectThrows(ElasticsearchSecurityException.class, () -> authorize(authentication, action, request)); - assertThat(securityException, - throwableWithMessage(containsString("[" + action + "] is unauthorized for user [test user] on indices ["))); + assertThat(securityException, throwableWithMessage(containsString( + "[" + action + "] is unauthorized" + + " for user [test user]" + + " with roles [non-existent-role]" + + " on indices ["))); assertThat(securityException, throwableWithMessage(containsString("this action is granted by the index privileges [read,all]"))); verify(auditTrail).accessDenied(eq(requestId), eq(authentication), eq(action), eq(request), authzInfoRoles(Role.EMPTY.names())); @@ -716,8 +719,11 @@ public void testThatRoleWithNoIndicesIsDenied() throws IOException { ElasticsearchSecurityException securityException = expectThrows(ElasticsearchSecurityException.class, () -> authorize(authentication, action, request)); - assertThat(securityException, - throwableWithMessage(containsString("[" + action + "] is unauthorized for user [test user] on indices ["))); + assertThat(securityException, throwableWithMessage(containsString( + "[" + action + "] is unauthorized" + + " for user [test user]" + + " with roles [no_indices]" + + " on indices ["))); assertThat(securityException, throwableWithMessage(containsString("this action is granted by the index privileges [read,all]"))); verify(auditTrail).accessDenied(eq(requestId), eq(authentication), eq(action), eq(request), @@ -888,14 +894,16 @@ public void testCreateIndexWithAlias() throws IOException { } public void testDenialErrorMessagesForSearchAction() throws IOException { - RoleDescriptor role = new RoleDescriptor("some_indices_" + randomAlphaOfLengthBetween(3, 6), null, new IndicesPrivileges[]{ + RoleDescriptor indexRole = 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()); + RoleDescriptor emptyRole = new RoleDescriptor("empty_role_" + randomAlphaOfLengthBetween(1,4), null, null, null); + User user = new User(randomAlphaOfLengthBetween(6, 8), indexRole.getName(), emptyRole.getName()); final Authentication authentication = createAuthentication(user); - roleMap.put(role.getName(), role); + roleMap.put(indexRole.getName(), indexRole); + roleMap.put(emptyRole.getName(), emptyRole); AuditUtil.getOrGenerateRequestId(threadContext); @@ -903,8 +911,11 @@ public void testDenialErrorMessagesForSearchAction() throws IOException { 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( + "[" + SearchAction.NAME + "] is unauthorized" + + " for user [" + user.principal() + "]" + + " with roles [" + indexRole.getName() + "," + emptyRole.getName() + "]" + + " on indices ["))); assertThat(securityException, throwableWithMessage(containsString("write-3"))); assertThat(securityException, throwableWithMessage(containsString("other-4"))); assertThat(securityException, throwableWithMessage(not(containsString("all-1")))); diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/api_key/11_invalidation.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/api_key/11_invalidation.yml index 6d11f3eabfcb3..52ff03ac6e46e 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/api_key/11_invalidation.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/api_key/11_invalidation.yml @@ -126,7 +126,8 @@ teardown: "username": "api_key_manager" } - match: { "error.type": "security_exception" } - - match: { "error.reason": "action [cluster:admin/xpack/security/api_key/invalidate] is unauthorized for user [api_key_user_1], this action is granted by the cluster privileges [manage_api_key,manage_security,all]" } + - match: + "error.reason": "action [cluster:admin/xpack/security/api_key/invalidate] is unauthorized for user [api_key_user_1] with roles [user_role], this action is granted by the cluster privileges [manage_api_key,manage_security,all]" - do: headers: @@ -189,7 +190,8 @@ teardown: "realm_name": "default_native" } - match: { "error.type": "security_exception" } - - match: { "error.reason": "action [cluster:admin/xpack/security/api_key/invalidate] is unauthorized for user [api_key_user_1], this action is granted by the cluster privileges [manage_api_key,manage_security,all]" } + - match: + "error.reason": "action [cluster:admin/xpack/security/api_key/invalidate] is unauthorized for user [api_key_user_1] with roles [user_role], this action is granted by the cluster privileges [manage_api_key,manage_security,all]" - do: headers: From 78feea3cd5e851efc293084a3019eaeb017a5aab Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Mon, 22 Feb 2021 15:35:26 +1100 Subject: [PATCH 2/3] Fix ML test --- .../xpack/ml/integration/DatafeedJobsRestIT.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) 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 58f5423e7d29d..9719c6d8608b7 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 @@ -819,7 +819,10 @@ 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] on indices [network-data]")); + "action [indices:data/read/search] is unauthorized" + + " for user [ml_admin_plus_data]" + + " with roles [machine_learning_admin,test_data_access]" + + " on indices [network-data]")); } public void testLookbackWithPipelineBucketAgg() throws Exception { @@ -967,8 +970,10 @@ 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] " + - "on indices [airline-data-aggs-rollup]")); + "action [indices:data/read/xpack/rollup/search] is unauthorized" + + " for user [ml_admin_plus_data] " + + " with roles [machine_learning_admin,test_data_access]" + + " on indices [airline-data-aggs-rollup]")); } public void testLookbackWithSingleBucketAgg() throws Exception { From 28fe0d2bad4854dce99bb36e2863f8da0101e84c Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Tue, 23 Feb 2021 18:04:55 +1100 Subject: [PATCH 3/3] Remove extra space in test assertion --- .../elasticsearch/xpack/ml/integration/DatafeedJobsRestIT.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 9719c6d8608b7..10b7e9d925d70 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 @@ -971,7 +971,7 @@ public void testLookbackWithoutPermissionsAndRollup() throws Exception { 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] " + + " for user [ml_admin_plus_data]" + " with roles [machine_learning_admin,test_data_access]" + " on indices [airline-data-aggs-rollup]")); }