Skip to content

Commit

Permalink
Add support for authentication based predicate for cluster permission (
Browse files Browse the repository at this point in the history
…#45431)

Currently, cluster permission checks whether a cluster action is
permitted and optionally in the context of a request. There are
scenarios where we would want to check whether the cluster action
is permitted, optionally in the context of a request and current
authentication. For example, management of API keys is only
restricted to the API keys owned by the current user. In this case,
along with the cluster action and API key request, the check
needs to perform whether the currently authenticated user is indeed
allowed to operate only on owned API keys.
With this commit, we are introducing one more context of the current
authentication that can be considered during permission evaluation.

Relates: #40031
  • Loading branch information
bizybot authored Aug 22, 2019
1 parent a07f319 commit 5661e98
Show file tree
Hide file tree
Showing 14 changed files with 436 additions and 377 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
import org.apache.lucene.util.automaton.Automaton;
import org.apache.lucene.util.automaton.Operations;
import org.elasticsearch.transport.TransportRequest;
import org.elasticsearch.xpack.core.security.authc.Authentication;
import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilege;
import org.elasticsearch.xpack.core.security.authz.privilege.ConfigurableClusterPrivilege;
import org.elasticsearch.xpack.core.security.support.Automatons;

import java.util.ArrayList;
Expand All @@ -34,14 +34,16 @@ private ClusterPermission(final Set<ClusterPrivilege> clusterPrivileges,
}

/**
* Checks permission to a cluster action for a given request.
* Checks permission to a cluster action for a given request in the context of given
* authentication.
*
* @param action cluster action
* @param request {@link TransportRequest}
* @param authentication {@link Authentication}
* @return {@code true} if the access is allowed else returns {@code false}
*/
public boolean check(final String action, final TransportRequest request) {
return checks.stream().anyMatch(permission -> permission.check(action, request));
public boolean check(final String action, final TransportRequest request, final Authentication authentication) {
return checks.stream().anyMatch(permission -> permission.check(action, request, authentication));
}

/**
Expand Down Expand Up @@ -80,21 +82,15 @@ public static class Builder {
public Builder add(final ClusterPrivilege clusterPrivilege, final Set<String> allowedActionPatterns,
final Set<String> excludeActionPatterns) {
this.clusterPrivileges.add(clusterPrivilege);
if (allowedActionPatterns.isEmpty() && excludeActionPatterns.isEmpty()) {
this.actionAutomatons.add(Automatons.EMPTY);
} else {
final Automaton allowedAutomaton = Automatons.patterns(allowedActionPatterns);
final Automaton excludedAutomaton = Automatons.patterns(excludeActionPatterns);
this.actionAutomatons.add(Automatons.minusAndMinimize(allowedAutomaton, excludedAutomaton));
}
final Automaton actionAutomaton = createAutomaton(allowedActionPatterns, excludeActionPatterns);
this.actionAutomatons.add(actionAutomaton);
return this;
}

public Builder add(final ConfigurableClusterPrivilege configurableClusterPrivilege, final Predicate<String> actionPredicate,
public Builder add(final ClusterPrivilege clusterPrivilege, final Set<String> allowedActionPatterns,
final Predicate<TransportRequest> requestPredicate) {
return add(configurableClusterPrivilege, new ActionRequestPredicatePermissionCheck(configurableClusterPrivilege,
actionPredicate,
requestPredicate));
final Automaton actionAutomaton = createAutomaton(allowedActionPatterns, Set.of());
return add(clusterPrivilege, new ActionRequestBasedPermissionCheck(clusterPrivilege, actionAutomaton, requestPredicate));
}

public Builder add(final ClusterPrivilege clusterPrivilege, final PermissionCheck permissionCheck) {
Expand All @@ -116,6 +112,21 @@ public ClusterPermission build() {
}
return new ClusterPermission(this.clusterPrivileges, checks);
}

private static Automaton createAutomaton(Set<String> allowedActionPatterns, Set<String> excludeActionPatterns) {
allowedActionPatterns = (allowedActionPatterns == null) ? Set.of() : allowedActionPatterns;
excludeActionPatterns = (excludeActionPatterns == null) ? Set.of() : excludeActionPatterns;

if (allowedActionPatterns.isEmpty()) {
return Automatons.EMPTY;
} else if (excludeActionPatterns.isEmpty()) {
return Automatons.patterns(allowedActionPatterns);
} else {
final Automaton allowedAutomaton = Automatons.patterns(allowedActionPatterns);
final Automaton excludedAutomaton = Automatons.patterns(excludeActionPatterns);
return Automatons.minusAndMinimize(allowedAutomaton, excludedAutomaton);
}
}
}

/**
Expand All @@ -124,13 +135,15 @@ public ClusterPermission build() {
*/
public interface PermissionCheck {
/**
* Checks permission to a cluster action for a given request.
* Checks permission to a cluster action for a given request in the context of given
* authentication.
*
* @param action action name
* @param request {@link TransportRequest}
* @param authentication {@link Authentication}
* @return {@code true} if the specified action for given request is allowed else returns {@code false}
*/
boolean check(String action, TransportRequest request);
boolean check(String action, TransportRequest request, Authentication authentication);

/**
* Checks whether specified {@link PermissionCheck} is implied by this {@link PermissionCheck}.<br>
Expand All @@ -156,7 +169,7 @@ private static class AutomatonPermissionCheck implements PermissionCheck {
}

@Override
public boolean check(final String action, final TransportRequest request) {
public boolean check(final String action, final TransportRequest request, final Authentication authentication) {
return actionPredicate.test(action);
}

Expand All @@ -169,29 +182,31 @@ public boolean implies(final PermissionCheck permissionCheck) {
}
}

// action and request based permission check
private static class ActionRequestPredicatePermissionCheck implements PermissionCheck {
// action, request based permission check
private static class ActionRequestBasedPermissionCheck extends AutomatonPermissionCheck {
private final ClusterPrivilege clusterPrivilege;
final Predicate<String> actionPredicate;
final Predicate<TransportRequest> requestPredicate;
private final Predicate<TransportRequest> requestPredicate;

ActionRequestPredicatePermissionCheck(final ClusterPrivilege clusterPrivilege, final Predicate<String> actionPredicate,
final Predicate<TransportRequest> requestPredicate) {
this.clusterPrivilege = clusterPrivilege;
this.actionPredicate = actionPredicate;
ActionRequestBasedPermissionCheck(ClusterPrivilege clusterPrivilege, final Automaton automaton,
final Predicate<TransportRequest> requestPredicate) {
super(automaton);
this.requestPredicate = requestPredicate;
this.clusterPrivilege = clusterPrivilege;
}

@Override
public boolean check(final String action, final TransportRequest request) {
return actionPredicate.test(action) && requestPredicate.test(request);
public boolean check(final String action, final TransportRequest request, final Authentication authentication) {
return super.check(action, request, authentication) && requestPredicate.test(request);
}

@Override
public boolean implies(final PermissionCheck permissionCheck) {
if (permissionCheck instanceof ActionRequestPredicatePermissionCheck) {
final ActionRequestPredicatePermissionCheck otherCheck = (ActionRequestPredicatePermissionCheck) permissionCheck;
return this.clusterPrivilege.equals(otherCheck.clusterPrivilege);
if (super.implies(permissionCheck)) {
if (permissionCheck instanceof ActionRequestBasedPermissionCheck) {
final ActionRequestBasedPermissionCheck otherCheck =
(ActionRequestBasedPermissionCheck) permissionCheck;
return this.clusterPrivilege.equals(otherCheck.clusterPrivilege);
}
}
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.apache.lucene.util.automaton.Automaton;
import org.elasticsearch.cluster.metadata.AliasOrIndex;
import org.elasticsearch.transport.TransportRequest;
import org.elasticsearch.xpack.core.security.authc.Authentication;
import org.elasticsearch.xpack.core.security.authz.accesscontrol.IndicesAccessControl;
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor;
import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilege;
Expand Down Expand Up @@ -122,15 +123,18 @@ public ResourcePrivilegesMap checkIndicesPrivileges(Set<String> checkForIndexPat
}

/**
* Check if cluster permissions allow for the given action, also checks whether the limited by role allows the given actions
* Check if cluster permissions allow for the given action,
* also checks whether the limited by role allows the given actions in the context of given
* authentication.
*
* @param action cluster action
* @param request {@link TransportRequest}
* @param authentication {@link Authentication}
* @return {@code true} if action is allowed else returns {@code false}
*/
@Override
public boolean checkClusterAction(String action, TransportRequest request) {
return super.checkClusterAction(action, request) && limitedBy.checkClusterAction(action, request);
public boolean checkClusterAction(String action, TransportRequest request, Authentication authentication) {
return super.checkClusterAction(action, request, authentication) && limitedBy.checkClusterAction(action, request, authentication);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.elasticsearch.common.collect.Tuple;
import org.elasticsearch.common.util.set.Sets;
import org.elasticsearch.transport.TransportRequest;
import org.elasticsearch.xpack.core.security.authc.Authentication;
import org.elasticsearch.xpack.core.security.authz.RoleDescriptor;
import org.elasticsearch.xpack.core.security.authz.accesscontrol.IndicesAccessControl;
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilege;
Expand Down Expand Up @@ -121,14 +122,16 @@ public ResourcePrivilegesMap checkIndicesPrivileges(Set<String> checkForIndexPat
}

/**
* Check if cluster permissions allow for the given action
* Check if cluster permissions allow for the given action in the context of given
* authentication.
*
* @param action cluster action
* @param request {@link TransportRequest}
* @param authentication {@link Authentication}
* @return {@code true} if action is allowed else returns {@code false}
*/
public boolean checkClusterAction(String action, TransportRequest request) {
return cluster.check(action, request);
public boolean checkClusterAction(String action, TransportRequest request, Authentication authentication) {
return cluster.check(action, request, authentication);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,6 @@ private static void expectFieldName(XContentParser parser, ParseField... fields)
* of applications (identified by a wildcard-aware application-name).
*/
public static class ManageApplicationPrivileges implements ConfigurableClusterPrivilege {

private static final Predicate<String> ACTION_PREDICATE = Automatons.predicate("cluster:admin/xpack/security/privilege/*");
public static final String WRITEABLE_NAME = "manage-application-privileges";

private final Set<String> applicationNames;
Expand All @@ -145,6 +143,7 @@ public ManageApplicationPrivileges(Set<String> applicationNames) {
}
return false;
};

}

@Override
Expand Down Expand Up @@ -215,7 +214,7 @@ public int hashCode() {

@Override
public ClusterPermission.Builder buildPermission(final ClusterPermission.Builder builder) {
return builder.add(this, ACTION_PREDICATE, requestPredicate);
return builder.add(this, Set.of("cluster:admin/xpack/security/privilege/*"), requestPredicate);
}

private interface Fields {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,11 @@
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.transport.TransportRequest;
import org.elasticsearch.xpack.core.security.authc.Authentication;
import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilege;
import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilegeResolver;
import org.elasticsearch.xpack.core.security.authz.privilege.ConfigurableClusterPrivilege;
import org.elasticsearch.xpack.core.security.support.Automatons;
import org.junit.Before;
import org.mockito.Mockito;

import java.io.IOException;
import java.util.Objects;
Expand All @@ -26,9 +25,11 @@

import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.is;
import static org.mockito.Mockito.mock;

public class ClusterPermissionTests extends ESTestCase {
private TransportRequest mockTransportRequest = Mockito.mock(TransportRequest.class);
private TransportRequest mockTransportRequest;
private Authentication mockAuthentication;
private ClusterPrivilege cpThatDoesNothing = new ClusterPrivilege() {
@Override
public ClusterPermission.Builder buildPermission(ClusterPermission.Builder builder) {
Expand All @@ -38,7 +39,8 @@ public ClusterPermission.Builder buildPermission(ClusterPermission.Builder build

@Before
public void setup() {
mockTransportRequest = Mockito.mock(TransportRequest.class);
mockTransportRequest = mock(TransportRequest.class);
mockAuthentication = mock(Authentication.class);
}

public void testClusterPermissionBuilder() {
Expand Down Expand Up @@ -78,46 +80,52 @@ public void testClusterPermissionCheck() {
builder = mockConfigurableClusterPrivilege2.buildPermission(builder);
final ClusterPermission clusterPermission = builder.build();

assertThat(clusterPermission.check("cluster:admin/xpack/security/token/invalidate", mockTransportRequest), is(true));
assertThat(clusterPermission.check("cluster:admin/ilm/stop", mockTransportRequest), is(true));
assertThat(clusterPermission.check("cluster:admin/xpack/security/privilege/get", mockTransportRequest), is(true));
assertThat(clusterPermission.check("cluster:admin/snapshot/status", mockTransportRequest), is(false));
assertThat(clusterPermission.check("cluster:admin/xpack/security/token/invalidate", mockTransportRequest, mockAuthentication),
is(true));
assertThat(clusterPermission.check("cluster:admin/ilm/stop", mockTransportRequest, mockAuthentication), is(true));
assertThat(clusterPermission.check("cluster:admin/xpack/security/privilege/get", mockTransportRequest, mockAuthentication),
is(true));
assertThat(clusterPermission.check("cluster:admin/snapshot/status", mockTransportRequest, mockAuthentication), is(false));
}

public void testClusterPermissionCheckWithEmptyActionPatterns() {
final ClusterPermission.Builder builder = ClusterPermission.builder();
builder.add(cpThatDoesNothing, Set.of(), Set.of());
final ClusterPermission clusterPermission = builder.build();

assertThat(clusterPermission.check("cluster:admin/ilm/start", mockTransportRequest), is(false));
assertThat(clusterPermission.check("cluster:admin/xpack/security/token/invalidate", mockTransportRequest), is(false));
assertThat(clusterPermission.check("cluster:admin/ilm/start", mockTransportRequest, mockAuthentication), is(false));
assertThat(clusterPermission.check("cluster:admin/xpack/security/token/invalidate", mockTransportRequest, mockAuthentication),
is(false));
}

public void testClusterPermissionCheckWithExcludeOnlyActionPatterns() {
final ClusterPermission.Builder builder = ClusterPermission.builder();
builder.add(cpThatDoesNothing, Set.of(), Set.of("cluster:some/thing/to/exclude"));
final ClusterPermission clusterPermission = builder.build();

assertThat(clusterPermission.check("cluster:admin/ilm/start", mockTransportRequest), is(false));
assertThat(clusterPermission.check("cluster:admin/xpack/security/token/invalidate", mockTransportRequest), is(false));
assertThat(clusterPermission.check("cluster:admin/ilm/start", mockTransportRequest, mockAuthentication), is(false));
assertThat(clusterPermission.check("cluster:admin/xpack/security/token/invalidate", mockTransportRequest, mockAuthentication),
is(false));
}

public void testClusterPermissionCheckWithActionPatterns() {
final ClusterPermission.Builder builder = ClusterPermission.builder();
builder.add(cpThatDoesNothing, Set.of("cluster:admin/*"), Set.of("cluster:admin/ilm/*"));
final ClusterPermission clusterPermission = builder.build();

assertThat(clusterPermission.check("cluster:admin/ilm/start", mockTransportRequest), is(false));
assertThat(clusterPermission.check("cluster:admin/xpack/security/token/invalidate", mockTransportRequest), is(true));
assertThat(clusterPermission.check("cluster:admin/ilm/start", mockTransportRequest, mockAuthentication), is(false));
assertThat(clusterPermission.check("cluster:admin/xpack/security/token/invalidate", mockTransportRequest, mockAuthentication),
is(true));
}

public void testClusterPermissionCheckWithActionPatternsAndNoExludePatterns() {
final ClusterPermission.Builder builder = ClusterPermission.builder();
builder.add(cpThatDoesNothing, Set.of("cluster:admin/*"), Set.of());
final ClusterPermission clusterPermission = builder.build();

assertThat(clusterPermission.check("cluster:admin/ilm/start", mockTransportRequest), is(true));
assertThat(clusterPermission.check("cluster:admin/xpack/security/token/invalidate", mockTransportRequest), is(true));
assertThat(clusterPermission.check("cluster:admin/ilm/start", mockTransportRequest, mockAuthentication), is(true));
assertThat(clusterPermission.check("cluster:admin/xpack/security/token/invalidate", mockTransportRequest, mockAuthentication),
is(true));
}

public void testNoneClusterPermissionIsImpliedByNone() {
Expand Down Expand Up @@ -223,7 +231,6 @@ public void testClusterPermissionSubsetIsImpliedByAllClusterPermission() {
}

private static class MockConfigurableClusterPrivilege implements ConfigurableClusterPrivilege {
static final Predicate<String> ACTION_PREDICATE = Automatons.predicate("cluster:admin/xpack/security/privilege/*");
private Predicate<TransportRequest> requestPredicate;

MockConfigurableClusterPrivilege(Predicate<TransportRequest> requestPredicate) {
Expand Down Expand Up @@ -269,13 +276,13 @@ public int hashCode() {
@Override
public String toString() {
return "MockConfigurableClusterPrivilege{" +
"requestPredicate=" + requestPredicate +
"requestAuthnPredicate=" + requestPredicate +
'}';
}

@Override
public ClusterPermission.Builder buildPermission(ClusterPermission.Builder builder) {
return builder.add(this, ACTION_PREDICATE, requestPredicate);
return builder.add(this, Set.of("cluster:admin/xpack/security/privilege/*"), requestPredicate);
}
}
}
Loading

0 comments on commit 5661e98

Please sign in to comment.