Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add manage_own_api_key cluster privilege #45696

Merged
merged 34 commits into from
Aug 23, 2019
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
80fa13f
Add support for authentication based predicate for cluster permission
Aug 7, 2019
60f436b
Merge branch 'manage-own-api-key-privilege' into moap-authentication-…
Aug 16, 2019
1af1308
address review comments
Aug 20, 2019
8896dcd
remove unwanted code
Aug 20, 2019
4976d78
remove unwanted code change, raise a separate pr for it
Aug 20, 2019
54490a7
fix tests
Aug 20, 2019
9033996
precommit error
Aug 20, 2019
541cfad
oh precommit, ran build on another branch, fixed it
Aug 20, 2019
aa623c3
Add `manage_own_api_key` cluster privilege
Aug 13, 2019
efc2c2b
add tests
Aug 16, 2019
7db504c
fix tests
Aug 19, 2019
adc6d69
change manage own api cluster privielge
Aug 20, 2019
2497c6f
precommit error
Aug 20, 2019
6bd259a
no need to override
Aug 20, 2019
66fc5b3
Merge branch 'manage-own-api-key-privilege' into moap-authentication-…
Aug 21, 2019
508a718
address review comment
Aug 21, 2019
29cefd2
add a base class for action based permission check to enforce checks
Aug 21, 2019
0eba55f
protected instead of public
Aug 21, 2019
de88e11
Merge branch 'manage-own-api-key-privilege' into moap-authentication-…
Aug 21, 2019
69b56c6
add unit test check for creator.realm
Aug 21, 2019
a94fa92
make the condition explicit
Aug 21, 2019
d5a295c
Merge branch 'moap-authentication-based-permission-check' into moap-m…
Aug 21, 2019
73498ea
address review comments
Aug 22, 2019
2f7933c
Merge branch 'manage-own-api-key-privilege' into moap-mo-cp
Aug 22, 2019
5c9c422
test name and test correctness
Aug 22, 2019
186e599
Merge branch 'manage-own-api-key-privilege' into moap-mo-cp
Aug 22, 2019
45b0192
remove extra blank lines
Aug 22, 2019
dc512c2
check owner flag in manage own cluster privilege, add tests
Aug 22, 2019
d740034
Update x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/…
bizybot Aug 22, 2019
7a9cfb7
fix compilation error
Aug 22, 2019
569cae6
Merge branch 'manage-own-api-key-privilege' into moap-mo-cp
Aug 22, 2019
1a2a371
address review comments
Aug 23, 2019
43cf4e3
static method instead of instance method
Aug 23, 2019
1e59c70
add unsupported class name to exception
Aug 23, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ A successful call returns an object with "cluster" and "index" fields.
"manage_ingest_pipelines",
"manage_ml",
"manage_oidc",
"manage_own_api_key",
"manage_pipeline",
"manage_rollup",
"manage_saml",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,32 +158,60 @@ public interface PermissionCheck {
boolean implies(PermissionCheck otherPermissionCheck);
}

// Automaton based permission check
private static class AutomatonPermissionCheck implements PermissionCheck {
/**
* Base for implementing cluster action based {@link PermissionCheck}.
* It enforces the checks at cluster action level and then hands it off to the implementations
* to enforce checks based on {@link TransportRequest} and/or {@link Authentication}.
*/
public abstract static class ActionBasedPermissionCheck implements PermissionCheck {
private final Automaton automaton;
private final Predicate<String> actionPredicate;

AutomatonPermissionCheck(final Automaton automaton) {
public ActionBasedPermissionCheck(final Automaton automaton) {
this.automaton = automaton;
this.actionPredicate = Automatons.predicate(automaton);
}

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

protected abstract boolean extendedCheck(String action, TransportRequest request, Authentication authentication);

@Override
public boolean implies(final PermissionCheck permissionCheck) {
if (permissionCheck instanceof AutomatonPermissionCheck) {
return Operations.subsetOf(((AutomatonPermissionCheck) permissionCheck).automaton, this.automaton);
public final boolean implies(final PermissionCheck permissionCheck) {
if (permissionCheck instanceof ActionBasedPermissionCheck) {
return Operations.subsetOf(((ActionBasedPermissionCheck) permissionCheck).automaton, this.automaton) &&
doImplies((ActionBasedPermissionCheck) permissionCheck);
}
return false;
}

protected abstract boolean doImplies(ActionBasedPermissionCheck permissionCheck);
}

// Automaton based permission check
private static class AutomatonPermissionCheck extends ActionBasedPermissionCheck {

AutomatonPermissionCheck(final Automaton automaton) {
super(automaton);
}

@Override
protected boolean extendedCheck(String action, TransportRequest request, Authentication authentication) {
return true;
}
bizybot marked this conversation as resolved.
Show resolved Hide resolved

@Override
protected boolean doImplies(ActionBasedPermissionCheck permissionCheck) {
return permissionCheck instanceof AutomatonPermissionCheck;
}

}

// action, request based permission check
private static class ActionRequestBasedPermissionCheck extends AutomatonPermissionCheck {
private static class ActionRequestBasedPermissionCheck extends ActionBasedPermissionCheck {
private final ClusterPrivilege clusterPrivilege;
private final Predicate<TransportRequest> requestPredicate;

Expand All @@ -195,18 +223,16 @@ private static class ActionRequestBasedPermissionCheck extends AutomatonPermissi
}

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

@Override
public boolean implies(final PermissionCheck permissionCheck) {
if (super.implies(permissionCheck)) {
if (permissionCheck instanceof ActionRequestBasedPermissionCheck) {
final ActionRequestBasedPermissionCheck otherCheck =
(ActionRequestBasedPermissionCheck) permissionCheck;
return this.clusterPrivilege.equals(otherCheck.clusterPrivilege);
}
protected boolean doImplies(final ActionBasedPermissionCheck 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 @@ -103,6 +103,8 @@ public class ClusterPrivilegeResolver {
public static final NamedClusterPrivilege MANAGE_SLM = new ActionClusterPrivilege("manage_slm", MANAGE_SLM_PATTERN);
public static final NamedClusterPrivilege READ_SLM = new ActionClusterPrivilege("read_slm", READ_SLM_PATTERN);

public static final NamedClusterPrivilege MANAGE_OWN_API_KEY = ManageOwnApiKeyClusterPrivilege.INSTANCE;

private static final Map<String, NamedClusterPrivilege> VALUES = Stream.of(
NONE,
ALL,
Expand Down Expand Up @@ -131,7 +133,8 @@ public class ClusterPrivilegeResolver {
MANAGE_ILM,
READ_ILM,
MANAGE_SLM,
READ_SLM).collect(Collectors.toUnmodifiableMap(NamedClusterPrivilege::name, Function.identity()));
READ_SLM,
MANAGE_OWN_API_KEY).collect(Collectors.toUnmodifiableMap(NamedClusterPrivilege::name, Function.identity()));

/**
* Resolves a {@link NamedClusterPrivilege} from a given name if it exists.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
/*
*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*
*/

package org.elasticsearch.xpack.core.security.authz.privilege;

import org.elasticsearch.common.Strings;
import org.elasticsearch.transport.TransportRequest;
import org.elasticsearch.xpack.core.security.action.CreateApiKeyRequest;
import org.elasticsearch.xpack.core.security.action.GetApiKeyRequest;
import org.elasticsearch.xpack.core.security.action.InvalidateApiKeyRequest;
import org.elasticsearch.xpack.core.security.authc.Authentication;
import org.elasticsearch.xpack.core.security.authz.permission.ClusterPermission;
import org.elasticsearch.xpack.core.security.support.Automatons;

/**
* Named cluster privilege for managing API keys owned by the current authenticated user.
*/
public class ManageOwnApiKeyClusterPrivilege implements NamedClusterPrivilege {
public static final ManageOwnApiKeyClusterPrivilege INSTANCE = new ManageOwnApiKeyClusterPrivilege();
private static final String PRIVILEGE_NAME = "manage_own_api_key";
private static final String API_KEY_REALM_TYPE = "_es_api_key";
private static final String API_KEY_ID_KEY = "_security_api_key_id";

private ManageOwnApiKeyClusterPrivilege() {
}

@Override
public String name() {
return PRIVILEGE_NAME;
}

@Override
public ClusterPermission.Builder buildPermission(ClusterPermission.Builder builder) {
return builder.add(this, ManageOwnClusterPermissionCheck.INSTANCE);
}

private static final class ManageOwnClusterPermissionCheck extends ClusterPermission.ActionBasedPermissionCheck {
public static final ManageOwnClusterPermissionCheck INSTANCE = new ManageOwnClusterPermissionCheck();

private ManageOwnClusterPermissionCheck() {
super(Automatons.patterns("cluster:admin/xpack/security/api_key/*"));
}

@Override
protected boolean extendedCheck(String action, TransportRequest request, Authentication authentication) {
if (request instanceof CreateApiKeyRequest) {
return true;
} else if (request instanceof GetApiKeyRequest) {
final GetApiKeyRequest getApiKeyRequest = (GetApiKeyRequest) request;
return checkIfUserIsOwnerOfApiKeys(authentication, getApiKeyRequest.getApiKeyId(), getApiKeyRequest.getUserName(),
getApiKeyRequest.getRealmName(), getApiKeyRequest.ownedByAuthenticatedUser());
} else if (request instanceof InvalidateApiKeyRequest) {
final InvalidateApiKeyRequest invalidateApiKeyRequest = (InvalidateApiKeyRequest) request;
return checkIfUserIsOwnerOfApiKeys(authentication, invalidateApiKeyRequest.getId(),
invalidateApiKeyRequest.getUserName(), invalidateApiKeyRequest.getRealmName(),
invalidateApiKeyRequest.ownedByAuthenticatedUser());
}
return false;
bizybot marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
protected boolean doImplies(ClusterPermission.ActionBasedPermissionCheck permissionCheck) {
return permissionCheck instanceof ManageOwnClusterPermissionCheck;
}

private boolean checkIfUserIsOwnerOfApiKeys(Authentication authentication, String apiKeyId, String username, String realmName,
boolean ownedByAuthenticatedUser) {
if (isCurrentAuthenticationUsingSameApiKeyIdFromRequest(authentication, apiKeyId)) {
return true;
} else {
/*
* TODO bizybot we need to think on how we can propagate appropriate error message to the end user when username, realm name
* is missing. This is similar to the problem of propagating right error messages in case of access denied.
*/
String authenticatedUserPrincipal = authentication.getUser().principal();
String authenticatedUserRealm = authentication.getAuthenticatedBy().getName();
bizybot marked this conversation as resolved.
Show resolved Hide resolved
bizybot marked this conversation as resolved.
Show resolved Hide resolved
if (authenticatedUserRealm.equals(API_KEY_REALM_TYPE)) {
bizybot marked this conversation as resolved.
Show resolved Hide resolved
// API key cannot own any other API key so deny access
return false;
} else if (ownedByAuthenticatedUser) {
return true;
} else if (Strings.hasText(username) && Strings.hasText(realmName)) {
return username.equals(authenticatedUserPrincipal) && realmName.equals(authenticatedUserRealm);
}
}
return false;
}

private boolean isCurrentAuthenticationUsingSameApiKeyIdFromRequest(Authentication authentication, String apiKeyId) {
bizybot marked this conversation as resolved.
Show resolved Hide resolved
if (authentication.getAuthenticatedBy().getType().equals(API_KEY_REALM_TYPE)) {
// API key id from authentication must match the id from request
String authenticatedApiKeyId = (String) authentication.getMetadata().get(API_KEY_ID_KEY);
if (Strings.hasText(apiKeyId)) {
return apiKeyId.equals(authenticatedApiKeyId);
}
}
return false;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ public int hashCode() {
@Override
public String toString() {
return "MockConfigurableClusterPrivilege{" +
"requestAuthnPredicate=" + requestPredicate +
"requestPredicate=" + requestPredicate +
'}';
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*
*/

package org.elasticsearch.xpack.core.security.authz.privilege;

import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.transport.TransportRequest;
import org.elasticsearch.xpack.core.security.action.GetApiKeyRequest;
import org.elasticsearch.xpack.core.security.action.InvalidateApiKeyRequest;
import org.elasticsearch.xpack.core.security.authc.Authentication;
import org.elasticsearch.xpack.core.security.authz.permission.ClusterPermission;
import org.elasticsearch.xpack.core.security.user.User;

import java.util.Map;

import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

public class ManageOwnApiKeyClusterPrivilegeTests extends ESTestCase {

public void testAuthenticationWithApiKeyAllowsAccessToApiKeyActionsWhenItIsOwner() {
final ClusterPermission clusterPermission =
ManageOwnApiKeyClusterPrivilege.INSTANCE.buildPermission(ClusterPermission.builder()).build();

final String apiKeyId = randomAlphaOfLengthBetween(4, 7);
final Authentication authentication = createMockAuthentication("joe","_es_api_key", "_es_api_key",
Map.of("_security_api_key_id", apiKeyId));
final TransportRequest getApiKeyRequest = GetApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean());
final TransportRequest invalidateApiKeyRequest = InvalidateApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean());

assertTrue(clusterPermission.check("cluster:admin/xpack/security/api_key/get", getApiKeyRequest, authentication));
assertTrue(clusterPermission.check("cluster:admin/xpack/security/api_key/invalidate", invalidateApiKeyRequest, authentication));
assertFalse(clusterPermission.check("cluster:admin/something", mock(TransportRequest.class), authentication));
}

public void testAuthenticationWithApiKeyDeniesAccessToApiKeyActionsWhenItIsNotOwner() {
final ClusterPermission clusterPermission =
ManageOwnApiKeyClusterPrivilege.INSTANCE.buildPermission(ClusterPermission.builder()).build();

final String apiKeyId = randomAlphaOfLengthBetween(4, 7);
final Authentication authentication = createMockAuthentication("joe","_es_api_key", "_es_api_key",
Map.of("_security_api_key_id", randomAlphaOfLength(7)));
final TransportRequest getApiKeyRequest = GetApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean());
final TransportRequest invalidateApiKeyRequest = InvalidateApiKeyRequest.usingApiKeyId(apiKeyId, randomBoolean());

assertFalse(clusterPermission.check("cluster:admin/xpack/security/api_key/get", getApiKeyRequest, authentication));
assertFalse(clusterPermission.check("cluster:admin/xpack/security/api_key/invalidate", invalidateApiKeyRequest, authentication));
}

public void testAuthenticationWithUserAllowsAccessToApiKeyActionsWhenItIsOwner() {
final ClusterPermission clusterPermission =
ManageOwnApiKeyClusterPrivilege.INSTANCE.buildPermission(ClusterPermission.builder()).build();

final Authentication authentication = createMockAuthentication("joe","realm1", "native", Map.of());
final TransportRequest getApiKeyRequest = randomFrom(GetApiKeyRequest.usingRealmAndUserName("realm1", "joe"),
GetApiKeyRequest.forOwnedApiKeys());
bizybot marked this conversation as resolved.
Show resolved Hide resolved
final TransportRequest invalidateApiKeyRequest = randomFrom(InvalidateApiKeyRequest.usingRealmAndUserName("realm1", "joe"),
GetApiKeyRequest.forOwnedApiKeys());
bizybot marked this conversation as resolved.
Show resolved Hide resolved

assertTrue(clusterPermission.check("cluster:admin/xpack/security/api_key/get", getApiKeyRequest, authentication));
assertTrue(clusterPermission.check("cluster:admin/xpack/security/api_key/invalidate", invalidateApiKeyRequest, authentication));
assertFalse(clusterPermission.check("cluster:admin/something", mock(TransportRequest.class), authentication));
}

public void testAuthenticationWithUserDeniesAccessToApiKeyActionsWhenItIsNotOwner() {
final ClusterPermission clusterPermission =
ManageOwnApiKeyClusterPrivilege.INSTANCE.buildPermission(ClusterPermission.builder()).build();

final Authentication authentication = createMockAuthentication("joe", "realm1", "native", Map.of());
final TransportRequest getApiKeyRequest = randomFrom(
GetApiKeyRequest.usingRealmAndUserName("realm1", randomAlphaOfLength(7)),
GetApiKeyRequest.usingRealmAndUserName(randomAlphaOfLength(5), "joe"),
new GetApiKeyRequest(randomAlphaOfLength(5), randomAlphaOfLength(7), null, null, false));
final TransportRequest invalidateApiKeyRequest = randomFrom(
InvalidateApiKeyRequest.usingRealmAndUserName("realm1", randomAlphaOfLength(7)),
InvalidateApiKeyRequest.usingRealmAndUserName(randomAlphaOfLength(5), "joe"),
new InvalidateApiKeyRequest(randomAlphaOfLength(5), randomAlphaOfLength(7), null, null, false));

assertFalse(clusterPermission.check("cluster:admin/xpack/security/api_key/get", getApiKeyRequest, authentication));
assertFalse(clusterPermission.check("cluster:admin/xpack/security/api_key/invalidate", invalidateApiKeyRequest, authentication));
}

private Authentication createMockAuthentication(String username, String realmName, String realmType, Map<String, Object> metadata) {
final User user = new User(username);
final Authentication authentication = mock(Authentication.class);
final Authentication.RealmRef authenticatedBy = mock(Authentication.RealmRef.class);
when(authentication.getUser()).thenReturn(user);
when(authentication.getAuthenticatedBy()).thenReturn(authenticatedBy);
when(authenticatedBy.getName()).thenReturn(realmName);
when(authenticatedBy.getType()).thenReturn(realmType);
when(authentication.getMetadata()).thenReturn(metadata);
return authentication;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,11 @@ protected void doExecute(Task task, GetApiKeyRequest request, ActionListener<Get
assert realm == null;
// restrict username and realm to current authenticated user.
username = authentication.getUser().principal();
realm = authentication.getAuthenticatedBy().getName();
if (authentication.getAuthenticatedBy().getType().equals(ApiKeyService.API_KEY_REALM_TYPE)) {
realm = (String) authentication.getMetadata().get(ApiKeyService.API_KEY_CREATOR_REALM);
} else {
realm = authentication.getAuthenticatedBy().getName();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this logic be a function that is used in ManageOwnApiKeyClusterPrivilege?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we are intercepting the request and passing default values to the ApiKeyService#getApiKeys when you specify owner flag as true.

ManageOwnApiKeyClusterPrivilege only performs the authz checks and should not modify the request while doing so.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, the question I am raising is that you have here a way of extracting the realm name which is different than the way of extracting the realm name in ManageOwnApiKeyClusterPrivilege. I believe they should be the same.
It works as is, but it's brittle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It never needs to extract the realm, user details when owner flag is true we return immediately in ManageOwnApiKeyClusterPrivilege#checkIfUserIsOwnerOfApiKeys.
This is a code change after Tim's comment so you may want to look at the method in ManageOwnApiKeyClusterPrivilege that does that.
Here we need to do so we can populate the realm and username before invoking ApiKeyService in case owner flag is true.

Copy link
Contributor

@tvernum tvernum Aug 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same logic is in 2 transport actions. That seems like a strong enough argument to move it to a static method on the service.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ManageOwnApiKeyClusterPrivilege is in core whereas the ApiKeyService is in security, but I have made changes to share between transport actions. Thank you.

}

apiKeyService.getApiKeys(realm, username, apiKeyName, apiKeyId, listener);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,11 @@ protected void doExecute(Task task, InvalidateApiKeyRequest request, ActionListe
assert realm == null;
// restrict username and realm to current authenticated user.
username = authentication.getUser().principal();
realm = authentication.getAuthenticatedBy().getName();
if (authentication.getAuthenticatedBy().getType().equals(ApiKeyService.API_KEY_REALM_TYPE)) {
realm = (String) authentication.getMetadata().get(ApiKeyService.API_KEY_CREATOR_REALM);
} else {
realm = authentication.getAuthenticatedBy().getName();
}
}

apiKeyService.invalidateApiKeys(realm, username, apiKeyName, apiKeyId, listener);
Expand Down
Loading