Skip to content

Commit

Permalink
call same user permission check from authz service
Browse files Browse the repository at this point in the history
  • Loading branch information
@jaymode
jaymode committed Dec 10, 2018
1 parent 6a3c2fa commit 54c33bd
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 35 deletions.
3 changes: 2 additions & 1 deletion ...in/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationEngine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ void authorizeRunAs(Authentication authentication, TransportRequest request, Str
void authorizeClusterAction(Authentication authentication, TransportRequest request, String action, AuthorizationInfo authorizationInfo,
ActionListener<AuthorizationResult> listener);

boolean checkSameUserPermissions(String action, TransportRequest request, Authentication authentication);
void checkSameUserPermissions(Authentication authentication, TransportRequest request, String action,
AuthorizationInfo authorizationInfo, ActionListener<AuthorizationResult> listener);

boolean shouldAuthorizeIndexActionNameOnly(String action, TransportRequest request);

Expand Down
15 changes: 14 additions & 1 deletion ...n/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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!
Expand Down
55 changes: 31 additions & 24 deletions x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -55,8 +56,9 @@

public class RBACEngine implements AuthorizationEngine {

private static final Predicate<String> SAME_USER_PRIVILEGE = Automatons.predicate(
ChangePasswordAction.NAME, AuthenticateAction.NAME, HasPrivilegesAction.NAME, GetUserPrivilegesAction.NAME);
private static final Set<String> SAME_USER_ACTIONS = Collections.unmodifiableSet(
Sets.newHashSet(ChangePasswordAction.NAME, AuthenticateAction.NAME, HasPrivilegesAction.NAME, GetUserPrivilegesAction.NAME));
private static final Predicate<String> SAME_USER_PRIVILEGE = SAME_USER_ACTIONS::contains;
private static final Predicate<String> 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]";
Expand Down Expand Up @@ -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());
}
Expand All @@ -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<AuthorizationResult> 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
Expand Down
48 changes: 39 additions & 9 deletions ...plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -67,7 +70,10 @@ public void testSameUserPermission() {
randomAlphaOfLengthBetween(4, 12));

assertThat(request, instanceOf(UserRequest.class));
assertTrue(engine.checkSameUserPermissions(action, request, authentication));
PlainActionFuture<AuthorizationResult> future = new PlainActionFuture<>();
engine.checkSameUserPermissions(authentication, request, action, mock(AuthorizationInfo.class), future);
assertTrue(future.actionGet().isGranted());
assertTrue(future.actionGet().isAuditable());
}

public void testSameUserPermissionDoesNotAllowNonMatchingUsername() {
Expand All @@ -88,7 +94,10 @@ public void testSameUserPermissionDoesNotAllowNonMatchingUsername() {
randomAlphaOfLengthBetween(4, 12));

assertThat(request, instanceOf(UserRequest.class));
assertFalse(engine.checkSameUserPermissions(action, request, authentication));
PlainActionFuture<AuthorizationResult> 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);
Expand All @@ -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() {
Expand All @@ -122,7 +137,10 @@ public void testSameUserPermissionDoesNotAllowOtherActions() {
when(authenticatedBy.getType())
.thenReturn(randomAlphaOfLengthBetween(4, 12));

assertFalse(engine.checkSameUserPermissions(action, request, authentication));
PlainActionFuture<AuthorizationResult> future = new PlainActionFuture<>();
engine.checkSameUserPermissions(authentication, request, action, mock(AuthorizationInfo.class), future);
assertFalse(future.actionGet().isGranted());
assertTrue(future.actionGet().isAuditable());
verifyZeroInteractions(user, request, authentication);
}

Expand All @@ -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<AuthorizationResult> 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() {
Expand All @@ -163,7 +187,10 @@ public void testSameUserPermissionDoesNotAllowChangePasswordForOtherRealms() {
randomAlphaOfLengthBetween(4, 12)));

assertThat(request, instanceOf(UserRequest.class));
assertFalse(engine.checkSameUserPermissions(action, request, authentication));
PlainActionFuture<AuthorizationResult> 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();
Expand All @@ -186,7 +213,10 @@ public void testSameUserPermissionDoesNotAllowChangePasswordForLookedUpByOtherRe
randomAlphaOfLengthBetween(4, 12)));

assertThat(request, instanceOf(UserRequest.class));
assertFalse(engine.checkSameUserPermissions(action, request, authentication));
PlainActionFuture<AuthorizationResult> 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();
Expand Down

0 comments on commit 54c33bd

Please sign in to comment.