Skip to content

Commit

Permalink
Allow API key to retrieve its own information with no API key privilege
Browse files Browse the repository at this point in the history 
Unless the API key has `manage_api_key` privilege, it cannot get its
own API key information when authenticating using API key. There can
be a use case wherein we do not wish the user authenticating using API
key to be able to invalidate or view any other API keys. This commit
solves this by adding allowing the request in case the API key id from
the `GetApiKeyRequest` matches the API key id present in the
`authentication` metadata.

Relates: elastic#40031
  • Loading branch information
Yogesh Gaikwad committed Aug 12, 2019
1 parent 9170c81 commit 145ce1f
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 19 deletions.
50 changes: 31 additions & 19 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 @@ -32,6 +32,8 @@
import org.elasticsearch.common.util.set.Sets;
import org.elasticsearch.transport.TransportActionProxy;
import org.elasticsearch.transport.TransportRequest;
import org.elasticsearch.xpack.core.security.action.GetApiKeyAction;
import org.elasticsearch.xpack.core.security.action.GetApiKeyRequest;
import org.elasticsearch.xpack.core.security.action.user.AuthenticateAction;
import org.elasticsearch.xpack.core.security.action.user.ChangePasswordAction;
import org.elasticsearch.xpack.core.security.action.user.GetUserPrivilegesAction;
Expand Down Expand Up @@ -86,7 +88,7 @@
public class RBACEngine implements AuthorizationEngine {

private static final Predicate<String> SAME_USER_PRIVILEGE = Automatons.predicate(
ChangePasswordAction.NAME, AuthenticateAction.NAME, HasPrivilegesAction.NAME, GetUserPrivilegesAction.NAME);
ChangePasswordAction.NAME, AuthenticateAction.NAME, HasPrivilegesAction.NAME, GetUserPrivilegesAction.NAME, GetApiKeyAction.NAME);
private static final String INDEX_SUB_REQUEST_PRIMARY = IndexAction.NAME + "[p]";
private static final String INDEX_SUB_REQUEST_REPLICA = IndexAction.NAME + "[r]";
private static final String DELETE_SUB_REQUEST_PRIMARY = DeleteAction.NAME + "[p]";
Expand Down Expand Up @@ -154,26 +156,36 @@ public void authorizeClusterAction(RequestInfo requestInfo, AuthorizationInfo au
boolean checkSameUserPermissions(String action, TransportRequest request, Authentication authentication) {
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";
if (request instanceof UserRequest) {
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);
}

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 if (request instanceof GetApiKeyRequest) {
GetApiKeyRequest getApiKeyRequest = (GetApiKeyRequest) request;
if (authentication.getAuthenticatedBy().getType().equals("_es_api_key")) {
// if authenticated by API key then the request must also contain same API key id
String authenticatedApiKeyId = (String) authentication.getMetadata().get("_security_api_key_id");
if (Strings.hasText(getApiKeyRequest.getApiKeyId())) {
return getApiKeyRequest.getApiKeyId().equals(authenticatedApiKeyId);
}
}
} else {
assert false : "right now only a user request or get api key request should be allowed";
return false;
}
final String username = usernames[0];
final boolean sameUsername = authentication.getUser().principal().equals(username);
if (sameUsername && ChangePasswordAction.NAME.equals(action)) {
return checkChangePasswordAction(authentication);
}

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;
}
return false;
}
Expand Down
22 changes: 22 additions & 0 deletions ...lugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import java.util.Base64;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
Expand Down Expand Up @@ -479,6 +480,23 @@ public void testGetApiKeysForApiKeyName() throws InterruptedException, Execution
verifyGetResponse(1, responses, response, Collections.singleton(responses.get(0).getId()), null);
}

public void testApiKeyAuthorizationApiKeyMustBeAbleToRetrieveItsOwnInformation() throws InterruptedException, ExecutionException {
List<CreateApiKeyResponse> responses = createApiKeys(2, null);
final String base64ApiKeyKeyValue = Base64.getEncoder().encodeToString(
(responses.get(0).getId() + ":" + responses.get(0).getKey().toString()).getBytes(StandardCharsets.UTF_8));
Client client = client().filterWithHeader(Map.of("Authorization", "ApiKey " + base64ApiKeyKeyValue));
PlainActionFuture<GetApiKeyResponse> listener = new PlainActionFuture<>();
client.execute(GetApiKeyAction.INSTANCE, GetApiKeyRequest.usingApiKeyId(responses.get(0).getId(), false), listener);
GetApiKeyResponse response = listener.get();
verifyGetResponse(1, responses, response, Collections.singleton(responses.get(0).getId()), null);

final PlainActionFuture<GetApiKeyResponse> failureListener = new PlainActionFuture<>();
// for any other API key id, it must deny access
client.execute(GetApiKeyAction.INSTANCE, GetApiKeyRequest.usingApiKeyId(responses.get(1).getId(), false), failureListener);
ElasticsearchSecurityException ese = expectThrows(ElasticsearchSecurityException.class, () -> failureListener.actionGet());
assertErrorMessage(ese, "cluster:admin/xpack/security/api_key/get", SecuritySettingsSource.TEST_SUPERUSER);
}

private void verifyGetResponse(int noOfApiKeys, List<CreateApiKeyResponse> responses, GetApiKeyResponse response,
Set<String> validApiKeyIds,
List<String> invalidatedApiKeyIds) {
Expand Down Expand Up @@ -522,4 +540,8 @@ private List<CreateApiKeyResponse> createApiKeys(int noOfApiKeys, TimeValue expi
assertThat(responses.size(), is(noOfApiKeys));
return responses;
}

private void assertErrorMessage(final ElasticsearchSecurityException ese, String action, String userName) {
assertThat(ese.getMessage(), is("action [" + action + "] is unauthorized for user [" + userName + "]"));
}
}
31 changes: 31 additions & 0 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 @@ -21,6 +21,8 @@
import org.elasticsearch.license.GetLicenseAction;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.transport.TransportRequest;
import org.elasticsearch.xpack.core.security.action.GetApiKeyAction;
import org.elasticsearch.xpack.core.security.action.GetApiKeyRequest;
import org.elasticsearch.xpack.core.security.action.user.AuthenticateAction;
import org.elasticsearch.xpack.core.security.action.user.AuthenticateRequest;
import org.elasticsearch.xpack.core.security.action.user.AuthenticateRequestBuilder;
Expand Down Expand Up @@ -63,6 +65,7 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;

import static java.util.Collections.emptyMap;
Expand Down Expand Up @@ -232,6 +235,34 @@ public void testSameUserPermissionDoesNotAllowChangePasswordForLookedUpByOtherRe
verifyNoMoreInteractions(authentication, lookedUpBy, authenticatedBy);
}

public void testSameUserPermissionAllowsSelfApiKeyInfoRetrievalWhenAuthenticatedByApiKey() {
final User user = new User("joe");
final String apiKeyId = randomAlphaOfLengthBetween(4, 7);
final TransportRequest request = GetApiKeyRequest.usingApiKeyId(apiKeyId, false);
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.getType()).thenReturn("_es_api_key");
when(authentication.getMetadata()).thenReturn(Map.of("_security_api_key_id", apiKeyId));

assertTrue(engine.checkSameUserPermissions(GetApiKeyAction.NAME, request, authentication));
}

public void testSameUserPermissionDeniesApiKeyInfoRetrievalWhenAuthenticatedByADifferentApiKey() {
final User user = new User("joe");
final String apiKeyId = randomAlphaOfLengthBetween(4, 7);
final TransportRequest request = GetApiKeyRequest.usingApiKeyId(apiKeyId, false);
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.getType()).thenReturn("_es_api_key");
when(authentication.getMetadata()).thenReturn(Map.of("_security_api_key_id", randomAlphaOfLengthBetween(4, 7)));

assertFalse(engine.checkSameUserPermissions(GetApiKeyAction.NAME, request, authentication));
}

/**
* This tests that action names in the request are considered "matched" by the relevant named privilege
* (in this case that {@link DeleteAction} and {@link IndexAction} are satisfied by {@link IndexPrivilege#WRITE}).
Expand Down

0 comments on commit 145ce1f

Please sign in to comment.