From 54c33bd54c79b6bb736cc11fea1730123a5521e8 Mon Sep 17 00:00:00 2001 From: jaymode Date: Mon, 10 Dec 2018 09:30:25 -0700 Subject: [PATCH] call same user permission check from authz service --- .../security/authz/AuthorizationEngine.java | 3 +- .../security/authz/AuthorizationService.java | 15 ++++- .../xpack/security/authz/RBACEngine.java | 55 +++++++++++-------- .../xpack/security/authz/RBACEngineTests.java | 48 +++++++++++++--- 4 files changed, 86 insertions(+), 35 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationEngine.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationEngine.java index 4a151d9062d6e..4058d7f385c92 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationEngine.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationEngine.java @@ -29,7 +29,8 @@ void authorizeRunAs(Authentication authentication, TransportRequest request, Str void authorizeClusterAction(Authentication authentication, TransportRequest request, String action, AuthorizationInfo authorizationInfo, ActionListener listener); - boolean checkSameUserPermissions(String action, TransportRequest request, Authentication authentication); + void checkSameUserPermissions(Authentication authentication, TransportRequest request, String action, + AuthorizationInfo authorizationInfo, ActionListener listener); boolean shouldAuthorizeIndexActionNameOnly(String action, TransportRequest request); 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 130132fe62ad3..e879bab3db4f4 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 @@ -202,7 +202,20 @@ private void authorizePostRunAs(final Authentication authentication, final Strin putTransientIfNonExisting(AuthorizationServiceField.INDICES_PERMISSIONS_KEY, IndicesAccessControl.ALLOW_ALL); listener.onResponse(null); } else { - listener.onFailure(denial(requestId, authentication, action, unwrappedRequest, authzInfo)); + authzEngine.checkSameUserPermissions(authentication, unwrappedRequest, action, authzInfo, + wrapPreservingContext(ActionListener.wrap(sameUserResult -> { + if (sameUserResult.isGranted()) { + if (sameUserResult.isAuditable()) { + auditTrail.accessGranted(requestId, authentication, action, unwrappedRequest, authzInfo); + } + putTransientIfNonExisting(AuthorizationServiceField.INDICES_PERMISSIONS_KEY, + IndicesAccessControl.ALLOW_ALL); + listener.onResponse(null); + } else { + listener.onFailure(denial(requestId, authentication, action, unwrappedRequest, authzInfo)); + } + }, e -> listener.onFailure(denial(requestId, authentication, action, unwrappedRequest, authzInfo))), + threadContext)); } }, e -> { // TODO need a failure handler better than this! diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java index 02406ce02e16a..02bf841a85a43 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java @@ -17,7 +17,9 @@ import org.elasticsearch.action.search.MultiSearchAction; import org.elasticsearch.action.termvectors.MultiTermVectorsAction; import org.elasticsearch.cluster.metadata.AliasOrIndex; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.transport.TransportRequest; import org.elasticsearch.xpack.core.security.action.user.AuthenticateAction; import org.elasticsearch.xpack.core.security.action.user.ChangePasswordAction; @@ -31,7 +33,6 @@ import org.elasticsearch.xpack.core.security.authz.permission.Role; import org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege; import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore; -import org.elasticsearch.xpack.core.security.support.Automatons; import org.elasticsearch.xpack.core.security.user.AnonymousUser; import org.elasticsearch.xpack.core.security.user.SystemUser; import org.elasticsearch.xpack.core.security.user.User; @@ -55,8 +56,9 @@ public class RBACEngine implements AuthorizationEngine { - private static final Predicate SAME_USER_PRIVILEGE = Automatons.predicate( - ChangePasswordAction.NAME, AuthenticateAction.NAME, HasPrivilegesAction.NAME, GetUserPrivilegesAction.NAME); + private static final Set SAME_USER_ACTIONS = Collections.unmodifiableSet( + Sets.newHashSet(ChangePasswordAction.NAME, AuthenticateAction.NAME, HasPrivilegesAction.NAME, GetUserPrivilegesAction.NAME)); + private static final Predicate SAME_USER_PRIVILEGE = SAME_USER_ACTIONS::contains; private static final Predicate MONITOR_INDEX_PREDICATE = IndexPrivilege.MONITOR.predicate(); private static final String INDEX_SUB_REQUEST_PRIMARY = IndexAction.NAME + "[p]"; private static final String INDEX_SUB_REQUEST_REPLICA = IndexAction.NAME + "[r]"; @@ -146,8 +148,6 @@ public void authorizeClusterAction(Authentication authentication, TransportReque final Role role = ((RBACAuthorizationInfo) authorizationInfo).getRole(); if (role.cluster().check(action, request)) { listener.onResponse(AuthorizationResult.granted()); - } else if (checkSameUserPermissions(action, request, authentication)) { - listener.onResponse(AuthorizationResult.granted()); } else { listener.onResponse(AuthorizationResult.deny()); } @@ -158,31 +158,38 @@ public void authorizeClusterAction(Authentication authentication, TransportReque } @Override - public boolean checkSameUserPermissions(String action, TransportRequest request, Authentication authentication) { + public void checkSameUserPermissions(Authentication authentication, TransportRequest request, String action, + AuthorizationInfo authorizationInfo, ActionListener listener) { final boolean actionAllowed = SAME_USER_PRIVILEGE.test(action); if (actionAllowed) { if (request instanceof UserRequest == false) { assert false : "right now only a user request should be allowed"; - return false; - } - UserRequest userRequest = (UserRequest) request; - String[] usernames = userRequest.usernames(); - if (usernames == null || usernames.length != 1 || usernames[0] == null) { - assert false : "this role should only be used for actions to apply to a single user"; - return false; - } - final String username = usernames[0]; - final boolean sameUsername = authentication.getUser().principal().equals(username); - if (sameUsername && ChangePasswordAction.NAME.equals(action)) { - return checkChangePasswordAction(authentication); + listener.onResponse(AuthorizationResult.deny()); + } else { + UserRequest userRequest = (UserRequest) request; + String[] usernames = userRequest.usernames(); + if (usernames == null || usernames.length != 1 || usernames[0] == null) { + assert false : "should only be used for actions to apply to a single user, but this applies to [" + + Strings.arrayToCommaDelimitedString(usernames) + "]"; + listener.onResponse(AuthorizationResult.deny()); + } else { + final String username = usernames[0]; + final boolean sameUsername = authentication.getUser().principal().equals(username); + if (sameUsername) { + if (ChangePasswordAction.NAME.equals(action)) { + final boolean allowed = checkChangePasswordAction(authentication); + listener.onResponse(allowed ? AuthorizationResult.granted() : AuthorizationResult.deny()); + } else { + listener.onResponse(AuthorizationResult.granted()); + } + } else { + listener.onResponse(AuthorizationResult.deny()); + } + } } - - assert AuthenticateAction.NAME.equals(action) || HasPrivilegesAction.NAME.equals(action) - || GetUserPrivilegesAction.NAME.equals(action) || sameUsername == false - : "Action '" + action + "' should not be possible when sameUsername=" + sameUsername; - return sameUsername; + } else { + listener.onResponse(AuthorizationResult.deny()); } - return false; } @Override diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java index 42f9dd35fb29d..85d3f328dc3e6 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java @@ -9,6 +9,7 @@ import org.elasticsearch.action.admin.cluster.health.ClusterHealthAction; import org.elasticsearch.action.admin.cluster.state.ClusterStateAction; import org.elasticsearch.action.admin.cluster.stats.ClusterStatsAction; +import org.elasticsearch.action.support.PlainActionFuture; import org.elasticsearch.client.Client; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.license.GetLicenseAction; @@ -31,6 +32,8 @@ import org.elasticsearch.xpack.core.security.user.AnonymousUser; import org.elasticsearch.xpack.core.security.user.User; import org.elasticsearch.xpack.security.authc.esnative.ReservedRealm; +import org.elasticsearch.xpack.security.authz.AuthorizationEngine.AuthorizationInfo; +import org.elasticsearch.xpack.security.authz.AuthorizationEngine.AuthorizationResult; import org.elasticsearch.xpack.security.authz.store.CompositeRolesStore; import org.junit.Before; @@ -67,7 +70,10 @@ public void testSameUserPermission() { randomAlphaOfLengthBetween(4, 12)); assertThat(request, instanceOf(UserRequest.class)); - assertTrue(engine.checkSameUserPermissions(action, request, authentication)); + PlainActionFuture future = new PlainActionFuture<>(); + engine.checkSameUserPermissions(authentication, request, action, mock(AuthorizationInfo.class), future); + assertTrue(future.actionGet().isGranted()); + assertTrue(future.actionGet().isAuditable()); } public void testSameUserPermissionDoesNotAllowNonMatchingUsername() { @@ -88,7 +94,10 @@ public void testSameUserPermissionDoesNotAllowNonMatchingUsername() { randomAlphaOfLengthBetween(4, 12)); assertThat(request, instanceOf(UserRequest.class)); - assertFalse(engine.checkSameUserPermissions(action, request, authentication)); + PlainActionFuture future = new PlainActionFuture<>(); + engine.checkSameUserPermissions(authentication, request, action, mock(AuthorizationInfo.class), future); + assertFalse(future.actionGet().isGranted()); + assertTrue(future.actionGet().isAuditable()); when(authentication.getUser()).thenReturn(user); final Authentication.RealmRef lookedUpBy = mock(Authentication.RealmRef.class); @@ -97,14 +106,20 @@ public void testSameUserPermissionDoesNotAllowNonMatchingUsername() { .thenReturn(changePasswordRequest ? randomFrom(ReservedRealm.TYPE, NativeRealmSettings.TYPE) : randomAlphaOfLengthBetween(4, 12)); // this should still fail since the username is still different - assertFalse(engine.checkSameUserPermissions(action, request, authentication)); + future = new PlainActionFuture<>(); + engine.checkSameUserPermissions(authentication, request, action, mock(AuthorizationInfo.class), future); + assertFalse(future.actionGet().isGranted()); + assertTrue(future.actionGet().isAuditable()); if (request instanceof ChangePasswordRequest) { ((ChangePasswordRequest) request).username("joe"); } else { ((AuthenticateRequest) request).username("joe"); } - assertTrue(engine.checkSameUserPermissions(action, request, authentication)); + future = new PlainActionFuture<>(); + engine.checkSameUserPermissions(authentication, request, action, mock(AuthorizationInfo.class), future); + assertTrue(future.actionGet().isGranted()); + assertTrue(future.actionGet().isAuditable()); } public void testSameUserPermissionDoesNotAllowOtherActions() { @@ -122,7 +137,10 @@ public void testSameUserPermissionDoesNotAllowOtherActions() { when(authenticatedBy.getType()) .thenReturn(randomAlphaOfLengthBetween(4, 12)); - assertFalse(engine.checkSameUserPermissions(action, request, authentication)); + PlainActionFuture future = new PlainActionFuture<>(); + engine.checkSameUserPermissions(authentication, request, action, mock(AuthorizationInfo.class), future); + assertFalse(future.actionGet().isGranted()); + assertTrue(future.actionGet().isAuditable()); verifyZeroInteractions(user, request, authentication); } @@ -144,10 +162,16 @@ public void testSameUserPermissionRunAsChecksAuthenticatedBy() { when(lookedUpBy.getType()) .thenReturn(changePasswordRequest ? randomFrom(ReservedRealm.TYPE, NativeRealmSettings.TYPE) : randomAlphaOfLengthBetween(4, 12)); - assertTrue(engine.checkSameUserPermissions(action, request, authentication)); + PlainActionFuture future = new PlainActionFuture<>(); + engine.checkSameUserPermissions(authentication, request, action, mock(AuthorizationInfo.class), future); + assertTrue(future.actionGet().isGranted()); + assertTrue(future.actionGet().isAuditable()); when(authentication.getUser()).thenReturn(authUser); - assertFalse(engine.checkSameUserPermissions(action, request, authentication)); + future = new PlainActionFuture<>(); + engine.checkSameUserPermissions(authentication, request, action, mock(AuthorizationInfo.class), future); + assertFalse(future.actionGet().isGranted()); + assertTrue(future.actionGet().isAuditable()); } public void testSameUserPermissionDoesNotAllowChangePasswordForOtherRealms() { @@ -163,7 +187,10 @@ public void testSameUserPermissionDoesNotAllowChangePasswordForOtherRealms() { randomAlphaOfLengthBetween(4, 12))); assertThat(request, instanceOf(UserRequest.class)); - assertFalse(engine.checkSameUserPermissions(action, request, authentication)); + PlainActionFuture future = new PlainActionFuture<>(); + engine.checkSameUserPermissions(authentication, request, action, mock(AuthorizationInfo.class), future); + assertFalse(future.actionGet().isGranted()); + assertTrue(future.actionGet().isAuditable()); verify(authenticatedBy).getType(); verify(authentication).getAuthenticatedBy(); verify(authentication, times(2)).getUser(); @@ -186,7 +213,10 @@ public void testSameUserPermissionDoesNotAllowChangePasswordForLookedUpByOtherRe randomAlphaOfLengthBetween(4, 12))); assertThat(request, instanceOf(UserRequest.class)); - assertFalse(engine.checkSameUserPermissions(action, request, authentication)); + PlainActionFuture future = new PlainActionFuture<>(); + engine.checkSameUserPermissions(authentication, request, action, mock(AuthorizationInfo.class), future); + assertFalse(future.actionGet().isGranted()); + assertTrue(future.actionGet().isAuditable()); verify(authentication).getLookedUpBy(); verify(authentication, times(2)).getUser(); verify(lookedUpBy).getType();