From 5c314153b1e9862f52be6db6e1dad5adccd508d3 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Mon, 24 Sep 2018 16:51:17 +1000 Subject: [PATCH 1/6] [Authz] Allow update settings action for system user When the `cluster.routing.allocation.disk.watermark.flood_stage` watermark is breached, DiskThresholdMonitor marks the indices as read only. This failed when x-pack security was present as System user does not have privilege for update settings action("indices:admin/settings/update"). This commit adds the required privilege for System user. Also added missing debug logs when access is denied to help future debugging. An assert statement is added to catch any missed privileges required for system user. Closes #33119 --- .../authz/privilege/SystemPrivilege.java | 3 +- .../authz/privilege/PrivilegeTests.java | 1 + .../security/authz/AuthorizationService.java | 7 ++- .../authz/AuthorizationServiceTests.java | 45 ++++++++++--------- 4 files changed, 34 insertions(+), 22 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java index f1527429b323e..32b16b87414a9 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java @@ -23,7 +23,8 @@ public final class SystemPrivilege extends Privilege { "indices:admin/mapping/put", // needed for recovery and shrink api "indices:admin/template/put", // needed for the TemplateUpgradeService "indices:admin/template/delete", // needed for the TemplateUpgradeService - "indices:admin/seq_no/global_checkpoint_sync*" // needed for global checkpoint syncs + "indices:admin/seq_no/global_checkpoint_sync*", // needed for global checkpoint syncs + "indices:admin/settings/update" // needed for DiskThreasholdMonitor.markIndicesReadOnly ), Automatons.patterns("internal:transport/proxy/*"))); // no proxy actions for system user! private SystemPrivilege() { diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/PrivilegeTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/PrivilegeTests.java index c4c95211d4c1c..2fc13e759775a 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/PrivilegeTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/PrivilegeTests.java @@ -126,6 +126,7 @@ public void testSystem() throws Exception { assertThat(predicate.test("indices:admin/seq_no/global_checkpoint_sync"), is(true)); assertThat(predicate.test("indices:admin/seq_no/global_checkpoint_sync[p]"), is(true)); assertThat(predicate.test("indices:admin/seq_no/global_checkpoint_sync[r]"), is(true)); + assertThat(predicate.test("indices:admin/settings/update"), is(true)); } public void testManageCcrPrivilege() { 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 642bc167f7d4a..8adb5f13bc752 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 @@ -154,7 +154,10 @@ public void authorize(Authentication authentication, String action, TransportReq auditTrail.accessGranted(authentication, action, request, new String[] { SystemUser.ROLE_NAME }); return; } - throw denial(authentication, action, request, new String[] { SystemUser.ROLE_NAME }); + auditTrail.accessDenied(authentication, action, request, new String[] { SystemUser.ROLE_NAME }); + ElasticsearchSecurityException e = denialException(authentication, action); + assert false : e.getMessage(); + throw e; } // get the roles of the authenticated user, which may be different than the effective @@ -568,9 +571,11 @@ private ElasticsearchSecurityException denialException(Authentication authentica } // check for run as if (authentication.getUser().isRunAs()) { + logger.debug("action [{}] is unauthorized for user [{}]", action, authUser.principal(), authentication.getUser().principal()); return authorizationError("action [{}] is unauthorized for user [{}] run as [{}]", action, authUser.principal(), authentication.getUser().principal()); } + logger.debug("action [{}] is unauthorized for user [{}]", action, authUser.principal()); return authorizationError("action [{}] is unauthorized for user [{}]", action, authUser.principal()); } 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 8ccac83c86f5d..ba2cd17969cfb 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 @@ -240,53 +240,58 @@ private void authorize(Authentication authentication, String action, TransportRe future.actionGet(); } - public void testActionsSystemUserIsAuthorized() { - TransportRequest request = mock(TransportRequest.class); + public void testActionsForSystemUserIsAuthorized() { + final TransportRequest request = mock(TransportRequest.class); // A failure would throw an exception - Authentication authentication = createAuthentication(SystemUser.INSTANCE); - authorize(authentication, "indices:monitor/whatever", request); - verify(auditTrail).accessGranted(authentication, "indices:monitor/whatever", request, - new String[]{SystemUser.ROLE_NAME}); - - authentication = createAuthentication(SystemUser.INSTANCE); - authorize(authentication, "internal:whatever", request); - verify(auditTrail).accessGranted(authentication, "internal:whatever", request, new String[]{SystemUser.ROLE_NAME}); + final Authentication authentication = createAuthentication(SystemUser.INSTANCE); + final String[] actions = { "indices:monitor/whatever", "internal:whatever", "cluster:monitor/whatever", "cluster:admin/reroute", + "indices:admin/mapping/put", "indices:admin/template/put", "indices:admin/seq_no/global_checkpoint_sync", + "indices:admin/settings/update" }; + for (String action : actions) { + authorize(authentication, action, request); + verify(auditTrail).accessGranted(authentication, action, request, new String[] { SystemUser.ROLE_NAME }); + } + verifyNoMoreInteractions(auditTrail); } - public void testIndicesActionsAreNotAuthorized() { + public void testIndicesActionsForSystemUserWhichAreNotAuthorized() { final TransportRequest request = mock(TransportRequest.class); final Authentication authentication = createAuthentication(SystemUser.INSTANCE); - assertThrowsAuthorizationException( + assertThrowsAssertionErrorWhenAccessDeniedToSystemUser( () -> authorize(authentication, "indices:", request), "indices:", SystemUser.INSTANCE.principal()); verify(auditTrail).accessDenied(authentication, "indices:", request, new String[]{SystemUser.ROLE_NAME}); verifyNoMoreInteractions(auditTrail); } - public void testClusterAdminActionsAreNotAuthorized() { + public void testClusterAdminActionsForSystemUserWhichAreNotAuthorized() { final TransportRequest request = mock(TransportRequest.class); final Authentication authentication = createAuthentication(SystemUser.INSTANCE); - assertThrowsAuthorizationException( + assertThrowsAssertionErrorWhenAccessDeniedToSystemUser( () -> authorize(authentication, "cluster:admin/whatever", request), "cluster:admin/whatever", SystemUser.INSTANCE.principal()); - verify(auditTrail).accessDenied(authentication, "cluster:admin/whatever", request, - new String[]{SystemUser.ROLE_NAME}); + verify(auditTrail).accessDenied(authentication, "cluster:admin/whatever", request, new String[] { SystemUser.ROLE_NAME }); verifyNoMoreInteractions(auditTrail); } - public void testClusterAdminSnapshotStatusActionIsNotAuthorized() { + public void testClusterAdminSnapshotStatusActionForSystemUserWhichIsNotAuthorized() { final TransportRequest request = mock(TransportRequest.class); final Authentication authentication = createAuthentication(SystemUser.INSTANCE); - assertThrowsAuthorizationException( + assertThrowsAssertionErrorWhenAccessDeniedToSystemUser( () -> authorize(authentication, "cluster:admin/snapshot/status", request), "cluster:admin/snapshot/status", SystemUser.INSTANCE.principal()); - verify(auditTrail).accessDenied(authentication, "cluster:admin/snapshot/status", request, - new String[]{SystemUser.ROLE_NAME}); + verify(auditTrail).accessDenied(authentication, "cluster:admin/snapshot/status", request, new String[] { SystemUser.ROLE_NAME }); verifyNoMoreInteractions(auditTrail); } + private static void assertThrowsAssertionErrorWhenAccessDeniedToSystemUser(ThrowingRunnable throwingRunnable, + String action, String user) { + AssertionError assertionError = expectThrows(AssertionError.class, throwingRunnable); + assertThat(assertionError.getMessage(), containsString("action ["+ action +"] is unauthorized for user ["+ user +"]")); + } + public void testAuthorizeUsingConditionalPrivileges() { final DeletePrivilegesRequest request = new DeletePrivilegesRequest(); final Authentication authentication = createAuthentication(new User("user1", "role1")); From f03009016618a8a1600869528f7301de8f98c0c3 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Tue, 25 Sep 2018 17:43:57 +1000 Subject: [PATCH 2/6] Correct run as debug log statement --- .../xpack/security/authz/AuthorizationService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 8adb5f13bc752..3809fc617ba04 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 @@ -571,7 +571,7 @@ private ElasticsearchSecurityException denialException(Authentication authentica } // check for run as if (authentication.getUser().isRunAs()) { - logger.debug("action [{}] is unauthorized for user [{}]", action, authUser.principal(), authentication.getUser().principal()); + logger.debug("action [{}] is unauthorized for user [{}] run as [{}]", action, authUser.principal(), authentication.getUser().principal()); return authorizationError("action [{}] is unauthorized for user [{}] run as [{}]", action, authUser.principal(), authentication.getUser().principal()); } From a118ab7deb245b680209f7913be09113f7fffb4e Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Tue, 25 Sep 2018 19:49:35 +1000 Subject: [PATCH 3/6] Fix line length --- .../xpack/security/authz/AuthorizationService.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 3809fc617ba04..662e65f68f5c9 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 @@ -571,7 +571,8 @@ private ElasticsearchSecurityException denialException(Authentication authentica } // check for run as if (authentication.getUser().isRunAs()) { - logger.debug("action [{}] is unauthorized for user [{}] run as [{}]", action, authUser.principal(), authentication.getUser().principal()); + logger.debug("action [{}] is unauthorized for user [{}] run as [{}]", action, authUser.principal(), + authentication.getUser().principal()); return authorizationError("action [{}] is unauthorized for user [{}] run as [{}]", action, authUser.principal(), authentication.getUser().principal()); } From cddd75edc3fc45d287d01a78e1902451169490c5 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Wed, 26 Sep 2018 14:05:24 +1000 Subject: [PATCH 4/6] Address review comments from Tim - Corrected the spelling mistake - Added test case - Added comment for the assert --- .../xpack/core/security/authz/privilege/SystemPrivilege.java | 2 +- .../xpack/core/security/authz/privilege/PrivilegeTests.java | 1 + .../xpack/security/authz/AuthorizationService.java | 5 +++-- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java index 32b16b87414a9..d5a5d04ddeda0 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java @@ -24,7 +24,7 @@ public final class SystemPrivilege extends Privilege { "indices:admin/template/put", // needed for the TemplateUpgradeService "indices:admin/template/delete", // needed for the TemplateUpgradeService "indices:admin/seq_no/global_checkpoint_sync*", // needed for global checkpoint syncs - "indices:admin/settings/update" // needed for DiskThreasholdMonitor.markIndicesReadOnly + "indices:admin/settings/update" // needed for DiskThresholdMonitor.markIndicesReadOnly ), Automatons.patterns("internal:transport/proxy/*"))); // no proxy actions for system user! private SystemPrivilege() { diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/PrivilegeTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/PrivilegeTests.java index 2fc13e759775a..f2bfd8d2d8e6b 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/PrivilegeTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/PrivilegeTests.java @@ -127,6 +127,7 @@ public void testSystem() throws Exception { assertThat(predicate.test("indices:admin/seq_no/global_checkpoint_sync[p]"), is(true)); assertThat(predicate.test("indices:admin/seq_no/global_checkpoint_sync[r]"), is(true)); assertThat(predicate.test("indices:admin/settings/update"), is(true)); + assertThat(predicate.test("indices:admin/settings/foo"), is(false)); } public void testManageCcrPrivilege() { 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 662e65f68f5c9..2db577729003e 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 @@ -154,8 +154,9 @@ public void authorize(Authentication authentication, String action, TransportReq auditTrail.accessGranted(authentication, action, request, new String[] { SystemUser.ROLE_NAME }); return; } - auditTrail.accessDenied(authentication, action, request, new String[] { SystemUser.ROLE_NAME }); - ElasticsearchSecurityException e = denialException(authentication, action); + ElasticsearchSecurityException e = denial(authentication, action, originalRequest, new String[] { SystemUser.ROLE_NAME }); + // If this throws AssertionError during ES tests then you probably + // need to update system user's privileges assert false : e.getMessage(); throw e; } From 6549cebdda7651aeb0c5b1fdbef504682fa53ca5 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Thu, 27 Sep 2018 12:08:31 +1000 Subject: [PATCH 5/6] Address review comments, suggestions from Jay -Removed the assertions and will create a issue for discussion. -Modified tests to verify exception. --- .../xpack/security/authz/AuthorizationService.java | 6 +----- .../security/authz/AuthorizationServiceTests.java | 12 +++--------- 2 files changed, 4 insertions(+), 14 deletions(-) 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 2db577729003e..7e79ff0f1384c 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 @@ -154,11 +154,7 @@ public void authorize(Authentication authentication, String action, TransportReq auditTrail.accessGranted(authentication, action, request, new String[] { SystemUser.ROLE_NAME }); return; } - ElasticsearchSecurityException e = denial(authentication, action, originalRequest, new String[] { SystemUser.ROLE_NAME }); - // If this throws AssertionError during ES tests then you probably - // need to update system user's privileges - assert false : e.getMessage(); - throw e; + throw denial(authentication, action, originalRequest, new String[] { SystemUser.ROLE_NAME }); } // get the roles of the authenticated user, which may be different than the effective 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 ba2cd17969cfb..47cf458e19a18 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 @@ -259,7 +259,7 @@ public void testActionsForSystemUserIsAuthorized() { public void testIndicesActionsForSystemUserWhichAreNotAuthorized() { final TransportRequest request = mock(TransportRequest.class); final Authentication authentication = createAuthentication(SystemUser.INSTANCE); - assertThrowsAssertionErrorWhenAccessDeniedToSystemUser( + assertThrowsAuthorizationException( () -> authorize(authentication, "indices:", request), "indices:", SystemUser.INSTANCE.principal()); verify(auditTrail).accessDenied(authentication, "indices:", request, new String[]{SystemUser.ROLE_NAME}); @@ -269,7 +269,7 @@ public void testIndicesActionsForSystemUserWhichAreNotAuthorized() { public void testClusterAdminActionsForSystemUserWhichAreNotAuthorized() { final TransportRequest request = mock(TransportRequest.class); final Authentication authentication = createAuthentication(SystemUser.INSTANCE); - assertThrowsAssertionErrorWhenAccessDeniedToSystemUser( + assertThrowsAuthorizationException( () -> authorize(authentication, "cluster:admin/whatever", request), "cluster:admin/whatever", SystemUser.INSTANCE.principal()); verify(auditTrail).accessDenied(authentication, "cluster:admin/whatever", request, new String[] { SystemUser.ROLE_NAME }); @@ -279,19 +279,13 @@ public void testClusterAdminActionsForSystemUserWhichAreNotAuthorized() { public void testClusterAdminSnapshotStatusActionForSystemUserWhichIsNotAuthorized() { final TransportRequest request = mock(TransportRequest.class); final Authentication authentication = createAuthentication(SystemUser.INSTANCE); - assertThrowsAssertionErrorWhenAccessDeniedToSystemUser( + assertThrowsAuthorizationException( () -> authorize(authentication, "cluster:admin/snapshot/status", request), "cluster:admin/snapshot/status", SystemUser.INSTANCE.principal()); verify(auditTrail).accessDenied(authentication, "cluster:admin/snapshot/status", request, new String[] { SystemUser.ROLE_NAME }); verifyNoMoreInteractions(auditTrail); } - private static void assertThrowsAssertionErrorWhenAccessDeniedToSystemUser(ThrowingRunnable throwingRunnable, - String action, String user) { - AssertionError assertionError = expectThrows(AssertionError.class, throwingRunnable); - assertThat(assertionError.getMessage(), containsString("action ["+ action +"] is unauthorized for user ["+ user +"]")); - } - public void testAuthorizeUsingConditionalPrivileges() { final DeletePrivilegesRequest request = new DeletePrivilegesRequest(); final Authentication authentication = createAuthentication(new User("user1", "role1")); From 27d96c875412bb654d6fd4f01ea22f840d60c333 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Mon, 1 Oct 2018 15:39:03 +1000 Subject: [PATCH 6/6] Remove unwanted change. --- .../xpack/security/authz/AuthorizationService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 7e79ff0f1384c..f9fe2b7eaa7f2 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 @@ -154,7 +154,7 @@ public void authorize(Authentication authentication, String action, TransportReq auditTrail.accessGranted(authentication, action, request, new String[] { SystemUser.ROLE_NAME }); return; } - throw denial(authentication, action, originalRequest, new String[] { SystemUser.ROLE_NAME }); + throw denial(authentication, action, request, new String[] { SystemUser.ROLE_NAME }); } // get the roles of the authenticated user, which may be different than the effective