From 145ce1f636b3faf1147ae29c4335da0a7b7ae0e6 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Mon, 12 Aug 2019 16:16:56 +1000 Subject: [PATCH 1/4] Allow API key to retrieve its own information with no API key privilege 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: #40031 --- .../xpack/security/authz/RBACEngine.java | 50 ++++++++++++------- .../security/authc/ApiKeyIntegTests.java | 22 ++++++++ .../xpack/security/authz/RBACEngineTests.java | 31 ++++++++++++ 3 files changed, 84 insertions(+), 19 deletions(-) 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 df00474f6d69d..ad739ee60a55e 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 @@ -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; @@ -86,7 +88,7 @@ public class RBACEngine implements AuthorizationEngine { private static final Predicate 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]"; @@ -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; } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java index 7309dfa2225d8..211b8d06f807f 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java @@ -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; @@ -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 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 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 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 responses, GetApiKeyResponse response, Set validApiKeyIds, List invalidatedApiKeyIds) { @@ -522,4 +540,8 @@ private List 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 + "]")); + } } 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 0a431ec95f5ee..59d465eb198b3 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 @@ -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; @@ -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; @@ -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}). From f96a550c15e49d4f0a967fd0e439147bc6e867f3 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Mon, 12 Aug 2019 19:42:16 +1000 Subject: [PATCH 2/4] address review comments --- .../xpack/security/authc/ApiKeyService.java | 2 ++ .../security/authc/AuthenticationService.java | 2 +- .../xpack/security/authz/RBACEngine.java | 6 ++-- .../xpack/security/authz/RBACEngineTests.java | 28 ++++++++++++++++--- 4 files changed, 31 insertions(+), 7 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java index ec7c2b7870840..b8777b62f1f23 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java @@ -107,6 +107,8 @@ public class ApiKeyService { private static final Logger logger = LogManager.getLogger(ApiKeyService.class); private static final DeprecationLogger deprecationLogger = new DeprecationLogger(logger); public static final String API_KEY_ID_KEY = "_security_api_key_id"; + public static final String API_KEY_REALM_NAME = "_es_api_key"; + public static final String API_KEY_REALM_TYPE = "_es_api_key"; static final String API_KEY_ROLE_DESCRIPTORS_KEY = "_security_api_key_role_descriptors"; static final String API_KEY_LIMITED_ROLE_DESCRIPTORS_KEY = "_security_api_key_limited_by_role_descriptors"; diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java index 1fe3ed67f7337..1d54c3e8dddd2 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java @@ -269,7 +269,7 @@ private void checkForApiKey() { apiKeyService.authenticateWithApiKeyIfPresent(threadContext, ActionListener.wrap(authResult -> { if (authResult.isAuthenticated()) { final User user = authResult.getUser(); - authenticatedBy = new RealmRef("_es_api_key", "_es_api_key", nodeName); + authenticatedBy = new RealmRef(ApiKeyService.API_KEY_REALM_NAME, ApiKeyService.API_KEY_REALM_TYPE, nodeName); writeAuthToContext(new Authentication(user, authenticatedBy, null, Version.CURRENT, Authentication.AuthenticationType.API_KEY, authResult.getMetadata())); } else if (authResult.getStatus() == AuthenticationResult.Status.TERMINATE) { 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 ad739ee60a55e..e70c5c2261cb9 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 @@ -64,6 +64,7 @@ import org.elasticsearch.xpack.core.security.authz.privilege.Privilege; import org.elasticsearch.xpack.core.security.support.Automatons; import org.elasticsearch.xpack.core.security.user.User; +import org.elasticsearch.xpack.security.authc.ApiKeyService; import org.elasticsearch.xpack.security.authc.esnative.ReservedRealm; import org.elasticsearch.xpack.security.authz.store.CompositeRolesStore; @@ -175,9 +176,10 @@ boolean checkSameUserPermissions(String action, TransportRequest request, Authen return sameUsername; } else if (request instanceof GetApiKeyRequest) { GetApiKeyRequest getApiKeyRequest = (GetApiKeyRequest) request; - if (authentication.getAuthenticatedBy().getType().equals("_es_api_key")) { + if (authentication.getAuthenticatedBy().getType().equals(ApiKeyService.API_KEY_REALM_TYPE)) { + assert authentication.getLookedUpBy() == null : "runAs not supported for api key authentication"; // 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"); + String authenticatedApiKeyId = (String) authentication.getMetadata().get(ApiKeyService.API_KEY_ID_KEY); if (Strings.hasText(getApiKeyRequest.getApiKeyId())) { return getApiKeyRequest.getApiKeyId().equals(authenticatedApiKeyId); } 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 59d465eb198b3..fd84afea365be 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 @@ -53,6 +53,7 @@ import org.elasticsearch.xpack.core.security.authz.privilege.Privilege; import org.elasticsearch.xpack.core.security.index.RestrictedIndicesNames; import org.elasticsearch.xpack.core.security.user.User; +import org.elasticsearch.xpack.security.authc.ApiKeyService; import org.elasticsearch.xpack.security.authc.esnative.ReservedRealm; import org.elasticsearch.xpack.security.authz.RBACEngine.RBACAuthorizationInfo; import org.elasticsearch.xpack.security.authz.store.CompositeRolesStore; @@ -243,8 +244,8 @@ public void testSameUserPermissionAllowsSelfApiKeyInfoRetrievalWhenAuthenticated 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)); + when(authenticatedBy.getType()).thenReturn(ApiKeyService.API_KEY_REALM_TYPE); + when(authentication.getMetadata()).thenReturn(Map.of(ApiKeyService.API_KEY_ID_KEY, apiKeyId)); assertTrue(engine.checkSameUserPermissions(GetApiKeyAction.NAME, request, authentication)); } @@ -257,12 +258,31 @@ public void testSameUserPermissionDeniesApiKeyInfoRetrievalWhenAuthenticatedByAD 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))); + when(authenticatedBy.getType()).thenReturn(ApiKeyService.API_KEY_REALM_TYPE); + when(authentication.getMetadata()).thenReturn(Map.of(ApiKeyService.API_KEY_ID_KEY, randomAlphaOfLengthBetween(4, 7))); assertFalse(engine.checkSameUserPermissions(GetApiKeyAction.NAME, request, authentication)); } + public void testSameUserPermissionDeniesApiKeyInfoRetrievalWhenLookedupByIsPresent() { + 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); + final Authentication.RealmRef lookedupBy = mock(Authentication.RealmRef.class); + when(authentication.getUser()).thenReturn(user); + when(authentication.getAuthenticatedBy()).thenReturn(authenticatedBy); + when(authentication.getLookedUpBy()).thenReturn(lookedupBy); + when(authenticatedBy.getType()).thenReturn(ApiKeyService.API_KEY_REALM_TYPE); + when(authentication.getMetadata()).thenReturn(Map.of(ApiKeyService.API_KEY_ID_KEY, randomAlphaOfLengthBetween(4, 7))); + + final AssertionError assertionError = expectThrows(AssertionError.class, () -> engine.checkSameUserPermissions(GetApiKeyAction.NAME, + request, authentication)); + assertNotNull(assertionError); + assertThat(assertionError.getLocalizedMessage(), is("runAs not supported for api key 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}). From d2ece5963c8c823978e8cb2f361d4aee4b4f6b36 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Wed, 14 Aug 2019 10:55:21 +1000 Subject: [PATCH 3/4] address review comments --- .../java/org/elasticsearch/xpack/security/authz/RBACEngine.java | 2 ++ 1 file changed, 2 insertions(+) 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 e70c5c2261cb9..a17cca5283211 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 @@ -182,6 +182,8 @@ boolean checkSameUserPermissions(String action, TransportRequest request, Authen String authenticatedApiKeyId = (String) authentication.getMetadata().get(ApiKeyService.API_KEY_ID_KEY); if (Strings.hasText(getApiKeyRequest.getApiKeyId())) { return getApiKeyRequest.getApiKeyId().equals(authenticatedApiKeyId); + } else { + return false; } } } else { From eb86acbe387254736ef186bdffbff98c1359c796 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Wed, 14 Aug 2019 16:13:09 +1000 Subject: [PATCH 4/4] precommit fix --- .../elasticsearch/xpack/security/authc/ApiKeyIntegTests.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java index b217a0c124ae5..bec82b17c1495 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java @@ -552,7 +552,8 @@ public void testApiKeyAuthorizationApiKeyMustBeAbleToRetrieveItsOwnInformation() private void verifyGetResponse(int expectedNumberOfApiKeys, List responses, GetApiKeyResponse response, Set validApiKeyIds, List invalidatedApiKeyIds) { - verifyGetResponse(SecuritySettingsSource.TEST_SUPERUSER, expectedNumberOfApiKeys, responses, response, validApiKeyIds, invalidatedApiKeyIds); + verifyGetResponse(SecuritySettingsSource.TEST_SUPERUSER, expectedNumberOfApiKeys, responses, response, validApiKeyIds, + invalidatedApiKeyIds); } private void verifyGetResponse(String user, int expectedNumberOfApiKeys, List responses,