From d4a0f80c3266edb6aae75200e3b080fb091e3ed9 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Tue, 7 Jul 2020 23:26:57 +0300 Subject: [PATCH] Ensure authz role for API key is named after owner role (#59041) The composite role that is used for authz, following the authn with an API key, is an intersection of the privileges from the owner role and the key privileges defined when the key has been created. This change ensures that the `#names` property of such a role equals the `#names` property of the key owner role, thereby rectifying the value for the `user.roles` audit event field. --- .../authz/permission/LimitedRole.java | 16 ++--- .../authz/permission/LimitedRoleTests.java | 41 +++++++++++ .../xpack/security/authc/ApiKeyService.java | 22 ++++-- .../security/authc/AuthenticationService.java | 7 +- .../security/authc/ApiKeyServiceTests.java | 19 +++++ .../authz/store/CompositeRolesStoreTests.java | 70 ++++++++++++------- 6 files changed, 129 insertions(+), 46 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/LimitedRole.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/LimitedRole.java index 9114984787909..6e1b58dba6a28 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/LimitedRole.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/LimitedRole.java @@ -29,17 +29,12 @@ public final class LimitedRole extends Role { private final Role limitedBy; - LimitedRole(String[] names, ClusterPermission cluster, IndicesPermission indices, ApplicationPermission application, - RunAsPermission runAs, Role limitedBy) { - super(names, cluster, indices, application, runAs); - assert limitedBy != null : "limiting role is required"; + LimitedRole(ClusterPermission cluster, IndicesPermission indices, ApplicationPermission application, RunAsPermission runAs, + Role limitedBy) { + super(Objects.requireNonNull(limitedBy, "limiting role is required").names(), cluster, indices, application, runAs); this.limitedBy = limitedBy; } - public Role limitedBy() { - return limitedBy; - } - @Override public ClusterPermission cluster() { throw new UnsupportedOperationException("cannot retrieve cluster permission on limited role"); @@ -86,7 +81,7 @@ public Predicate allowedIndicesMatcher(String action) { @Override public Automaton allowedActionsMatcher(String index) { final Automaton allowedMatcher = super.allowedActionsMatcher(index); - final Automaton limitedByMatcher = super.allowedActionsMatcher(index); + final Automaton limitedByMatcher = limitedBy.allowedActionsMatcher(index); return Automatons.intersectAndMinimize(allowedMatcher, limitedByMatcher); } @@ -187,7 +182,6 @@ public boolean checkRunAs(String runAs) { */ public static LimitedRole createLimitedRole(Role fromRole, Role limitedByRole) { Objects.requireNonNull(limitedByRole, "limited by role is required to create limited role"); - return new LimitedRole(fromRole.names(), fromRole.cluster(), fromRole.indices(), fromRole.application(), fromRole.runAs(), - limitedByRole); + return new LimitedRole(fromRole.cluster(), fromRole.indices(), fromRole.application(), fromRole.runAs(), limitedByRole); } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/LimitedRoleTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/LimitedRoleTests.java index deb3a7484d4ff..57cc0938632e3 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/LimitedRoleTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/LimitedRoleTests.java @@ -6,9 +6,11 @@ package org.elasticsearch.xpack.core.security.authz.permission; +import org.apache.lucene.util.automaton.Automaton; import org.elasticsearch.Version; import org.elasticsearch.action.admin.indices.create.CreateIndexAction; import org.elasticsearch.action.admin.indices.delete.DeleteIndexAction; +import org.elasticsearch.action.bulk.BulkAction; import org.elasticsearch.action.search.SearchAction; import org.elasticsearch.cluster.metadata.AliasMetadata; import org.elasticsearch.cluster.metadata.IndexMetadata; @@ -24,12 +26,14 @@ import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor; import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilegeResolver; import org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege; +import org.elasticsearch.xpack.core.security.support.Automatons; import org.junit.Before; import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Set; +import java.util.function.Predicate; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; @@ -50,6 +54,7 @@ public void testRoleConstructorWithLimitedRole() { Role limitedByRole = Role.builder("limited-role").build(); Role role = LimitedRole.createLimitedRole(fromRole, limitedByRole); assertNotNull(role); + assertThat(role.names(), is(limitedByRole.names())); NullPointerException npe = expectThrows(NullPointerException.class, () -> LimitedRole.createLimitedRole(fromRole, null)); assertThat(npe.getMessage(), containsString("limited by role is required to create limited role")); @@ -200,6 +205,42 @@ public void testAllowedIndicesMatcher() { } } + public void testAllowedActionsMatcher() { + Role fromRole = Role.builder("fromRole") + .add(IndexPrivilege.WRITE, "ind*") + .add(IndexPrivilege.READ, "ind*") + .add(IndexPrivilege.READ, "other*") + .build(); + Automaton fromRoleAutomaton = fromRole.allowedActionsMatcher("index1"); + Predicate fromRolePredicate = Automatons.predicate(fromRoleAutomaton); + assertThat(fromRolePredicate.test(SearchAction.NAME), is(true)); + assertThat(fromRolePredicate.test(BulkAction.NAME), is(true)); + + Role limitedByRole = Role.builder("limitedRole") + .add(IndexPrivilege.READ, "index1", "index2") + .build(); + Automaton limitedByRoleAutomaton = limitedByRole.allowedActionsMatcher("index1"); + Predicate limitedByRolePredicated = Automatons.predicate(limitedByRoleAutomaton); + assertThat(limitedByRolePredicated.test(SearchAction.NAME), is(true)); + assertThat(limitedByRolePredicated.test(BulkAction.NAME), is(false)); + Role role = LimitedRole.createLimitedRole(fromRole, limitedByRole); + + Automaton roleAutomaton = role.allowedActionsMatcher("index1"); + Predicate rolePredicate = Automatons.predicate(roleAutomaton); + assertThat(rolePredicate.test(SearchAction.NAME), is(true)); + assertThat(rolePredicate.test(BulkAction.NAME), is(false)); + + roleAutomaton = role.allowedActionsMatcher("index2"); + rolePredicate = Automatons.predicate(roleAutomaton); + assertThat(rolePredicate.test(SearchAction.NAME), is(true)); + assertThat(rolePredicate.test(BulkAction.NAME), is(false)); + + roleAutomaton = role.allowedActionsMatcher("other"); + rolePredicate = Automatons.predicate(roleAutomaton); + assertThat(rolePredicate.test(SearchAction.NAME), is(false)); + assertThat(rolePredicate.test(BulkAction.NAME), is(false)); + } + public void testCheckClusterPrivilege() { Role fromRole = Role.builder("a-role").cluster(Collections.singleton("manage_security"), Collections.emptyList()) .build(); 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 e7c409fd8067e..4a11f34dc28bd 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 @@ -71,6 +71,7 @@ import org.elasticsearch.xpack.core.security.action.GetApiKeyResponse; import org.elasticsearch.xpack.core.security.action.InvalidateApiKeyResponse; import org.elasticsearch.xpack.core.security.authc.Authentication; +import org.elasticsearch.xpack.core.security.authc.Authentication.RealmRef; import org.elasticsearch.xpack.core.security.authc.AuthenticationResult; import org.elasticsearch.xpack.core.security.authc.support.Hasher; import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; @@ -246,7 +247,7 @@ private void createApiKeyAndIndexIt(Authentication authentication, CreateApiKeyR } /** - * package protected for testing + * package-private for testing */ XContentBuilder newDocument(SecureString apiKey, String name, Authentication authentication, Set userRoles, Instant created, Instant expiration, List keyRoles, @@ -335,6 +336,16 @@ void authenticateWithApiKeyIfPresent(ThreadContext ctx, ActionListener listener) { final String docId = credentials.getId(); @@ -531,7 +542,8 @@ CachedApiKeyHashResult getFromCache(String id) { return apiKeyAuthCache == null ? null : FutureUtils.get(apiKeyAuthCache.get(id), 0L, TimeUnit.MILLISECONDS); } - private void validateApiKeyExpiration(Map source, ApiKeyCredentials credentials, Clock clock, + // package-private for testing + void validateApiKeyExpiration(Map source, ApiKeyCredentials credentials, Clock clock, ActionListener listener) { final Long expirationEpochMilli = (Long) source.get("expiration_time"); if (expirationEpochMilli == null || Instant.ofEpochMilli(expirationEpochMilli).isAfter(clock.instant())) { @@ -624,12 +636,12 @@ public void ensureEnabled() { } } - // package private class for testing - static final class ApiKeyCredentials implements Closeable { + // public class for testing + public static final class ApiKeyCredentials implements Closeable { private final String id; private final SecureString key; - ApiKeyCredentials(String id, SecureString key) { + public ApiKeyCredentials(String id, SecureString key) { this.id = id; this.key = key; } 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 e93336133e5bf..b1b40d9aada6b 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 @@ -346,10 +346,9 @@ private void authenticateAsync() { private void checkForApiKey() { apiKeyService.authenticateWithApiKeyIfPresent(threadContext, ActionListener.wrap(authResult -> { if (authResult.isAuthenticated()) { - final User user = authResult.getUser(); - 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())); + final Authentication authentication = apiKeyService.createApiKeyAuthentication(authResult, nodeName); + this.authenticatedBy = authentication.getAuthenticatedBy(); + writeAuthToContext(authentication); } else if (authResult.getStatus() == AuthenticationResult.Status.TERMINATE) { Exception e = (authResult.getException() != null) ? authResult.getException() : Exceptions.authenticationError(authResult.getMessage()); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java index 101d95f98f24e..23357d7f8b48c 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java @@ -66,7 +66,9 @@ import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Semaphore; @@ -719,6 +721,23 @@ public void testCachedApiKeyValidationWillNotBeBlockedByUnCachedApiKey() throws assertEquals(AuthenticationResult.Status.SUCCESS, authenticationResult3.getStatus()); } + public static class Utils { + + public static Authentication createApiKeyAuthentication(ApiKeyService apiKeyService, + Authentication authentication, + Set userRoles, + List keyRoles) throws Exception { + XContentBuilder keyDocSource = apiKeyService.newDocument(new SecureString("secret".toCharArray()), "test", authentication, + userRoles, Instant.now(), Instant.now().plus(Duration.ofSeconds(3600)), keyRoles, Version.CURRENT); + Map keyDocMap = XContentHelper.convertToMap(BytesReference.bytes(keyDocSource), true, XContentType.JSON).v2(); + PlainActionFuture authenticationResultFuture = PlainActionFuture.newFuture(); + apiKeyService.validateApiKeyExpiration(keyDocMap, new ApiKeyService.ApiKeyCredentials("id", + new SecureString("pass".toCharArray())), + Clock.systemUTC(), authenticationResultFuture); + return apiKeyService.createApiKeyAuthentication(authenticationResultFuture.get(), "node01"); + } + } + private ApiKeyService createApiKeyService(Settings baseSettings) { final Settings settings = Settings.builder() .put(XPackSettings.API_KEY_SERVICE_ENABLED_SETTING.getKey(), true) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java index e7cc96df0ad82..e355c1fb4791d 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java @@ -12,9 +12,11 @@ import org.elasticsearch.action.get.GetAction; import org.elasticsearch.action.index.IndexAction; import org.elasticsearch.action.support.PlainActionFuture; +import org.elasticsearch.client.Client; import org.elasticsearch.cluster.health.ClusterHealthStatus; import org.elasticsearch.cluster.metadata.IndexMetadata; import org.elasticsearch.cluster.metadata.Metadata; +import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.io.stream.StreamOutput; @@ -63,10 +65,10 @@ import org.elasticsearch.xpack.core.security.user.XPackUser; import org.elasticsearch.xpack.security.audit.AuditUtil; import org.elasticsearch.xpack.security.authc.ApiKeyService; -import org.elasticsearch.xpack.security.authc.ApiKeyService.ApiKeyRoleDescriptors; import org.elasticsearch.xpack.security.support.SecurityIndexManager; import java.io.IOException; +import java.time.Clock; import java.time.Instant; import java.util.ArrayList; import java.util.Arrays; @@ -88,8 +90,10 @@ import static org.elasticsearch.mock.orig.Mockito.times; import static org.elasticsearch.mock.orig.Mockito.verifyNoMoreInteractions; +import static org.elasticsearch.xpack.security.authc.ApiKeyServiceTests.Utils.createApiKeyAuthentication; import static org.hamcrest.Matchers.anyOf; import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.hasSize; @@ -1009,7 +1013,7 @@ public void testGetRolesForSystemUserThrowsException() { assertEquals("the user [_system] is the system user and we should never try to get its roles", iae.getMessage()); } - public void testApiKeyAuthUsesApiKeyService() throws IOException { + public void testApiKeyAuthUsesApiKeyService() throws Exception { final FileRolesStore fileRolesStore = mock(FileRolesStore.class); doCallRealMethod().when(fileRolesStore).accept(any(Set.class), any(ActionListener.class)); final NativeRolesStore nativeRolesStore = mock(NativeRolesStore.class); @@ -1022,7 +1026,9 @@ public void testApiKeyAuthUsesApiKeyService() throws IOException { }).when(nativeRolesStore).getRoleDescriptors(isA(Set.class), any(ActionListener.class)); final ReservedRolesStore reservedRolesStore = spy(new ReservedRolesStore()); ThreadContext threadContext = new ThreadContext(SECURITY_ENABLED_SETTINGS); - ApiKeyService apiKeyService = mock(ApiKeyService.class); + ApiKeyService apiKeyService = new ApiKeyService(SECURITY_ENABLED_SETTINGS, Clock.systemUTC(), mock(Client.class), + new XPackLicenseState(SECURITY_ENABLED_SETTINGS), mock(SecurityIndexManager.class), mock(ClusterService.class), + mock(ThreadPool.class)); NativePrivilegeStore nativePrivStore = mock(NativePrivilegeStore.class); doAnswer(invocationOnMock -> { ActionListener> listener = @@ -1039,23 +1045,19 @@ public void testApiKeyAuthUsesApiKeyService() throws IOException { new XPackLicenseState(SECURITY_ENABLED_SETTINGS), cache, apiKeyService, documentSubsetBitsetCache, rds -> effectiveRoleDescriptors.set(rds)); AuditUtil.getOrGenerateRequestId(threadContext); - final Authentication authentication = new Authentication(new User("test api key user", "superuser"), - new RealmRef("_es_api_key", "_es_api_key", "node"), null, Version.CURRENT, AuthenticationType.API_KEY, Collections.emptyMap()); - doAnswer(invocationOnMock -> { - ActionListener listener = (ActionListener) invocationOnMock.getArguments()[1]; - listener.onResponse(new ApiKeyRoleDescriptors("keyId", - Collections.singletonList(ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR), null)); - return Void.TYPE; - }).when(apiKeyService).getRoleForApiKey(eq(authentication), any(ActionListener.class)); + + final Authentication authentication = createApiKeyAuthentication(apiKeyService, createAuthentication(), + Collections.singleton(new RoleDescriptor("user_role_" + randomAlphaOfLength(4), new String[]{"manage"}, null, null)), null); PlainActionFuture roleFuture = new PlainActionFuture<>(); compositeRolesStore.getRoles(authentication.getUser(), authentication, roleFuture); - roleFuture.actionGet(); + Role role = roleFuture.actionGet(); assertThat(effectiveRoleDescriptors.get(), is(nullValue())); - verify(apiKeyService).getRoleForApiKey(eq(authentication), any(ActionListener.class)); + assertThat(role.names().length, is(1)); + assertThat(role.names()[0], containsString("user_role_")); } - public void testApiKeyAuthUsesApiKeyServiceWithScopedRole() throws IOException { + public void testApiKeyAuthUsesApiKeyServiceWithScopedRole() throws Exception { final FileRolesStore fileRolesStore = mock(FileRolesStore.class); doCallRealMethod().when(fileRolesStore).accept(any(Set.class), any(ActionListener.class)); final NativeRolesStore nativeRolesStore = mock(NativeRolesStore.class); @@ -1068,7 +1070,10 @@ public void testApiKeyAuthUsesApiKeyServiceWithScopedRole() throws IOException { }).when(nativeRolesStore).getRoleDescriptors(isA(Set.class), any(ActionListener.class)); final ReservedRolesStore reservedRolesStore = spy(new ReservedRolesStore()); ThreadContext threadContext = new ThreadContext(SECURITY_ENABLED_SETTINGS); - ApiKeyService apiKeyService = mock(ApiKeyService.class); + + ApiKeyService apiKeyService = new ApiKeyService(SECURITY_ENABLED_SETTINGS, Clock.systemUTC(), mock(Client.class), + new XPackLicenseState(SECURITY_ENABLED_SETTINGS), mock(SecurityIndexManager.class), mock(ClusterService.class), + mock(ThreadPool.class)); NativePrivilegeStore nativePrivStore = mock(NativePrivilegeStore.class); doAnswer(invocationOnMock -> { ActionListener> listener = @@ -1085,23 +1090,18 @@ public void testApiKeyAuthUsesApiKeyServiceWithScopedRole() throws IOException { new XPackLicenseState(SECURITY_ENABLED_SETTINGS), cache, apiKeyService, documentSubsetBitsetCache, rds -> effectiveRoleDescriptors.set(rds)); AuditUtil.getOrGenerateRequestId(threadContext); - final Authentication authentication = new Authentication(new User("test api key user", "api_key"), - new RealmRef("_es_api_key", "_es_api_key", "node"), null, Version.CURRENT, AuthenticationType.API_KEY, Collections.emptyMap()); - doAnswer(invocationOnMock -> { - ActionListener listener = (ActionListener) invocationOnMock.getArguments()[1]; - listener.onResponse(new ApiKeyRoleDescriptors("keyId", - Collections.singletonList(new RoleDescriptor("a-role", new String[] {"all"}, null, null)), - Collections.singletonList( - new RoleDescriptor("scoped-role", new String[] {"manage_security"}, null, null)))); - return Void.TYPE; - }).when(apiKeyService).getRoleForApiKey(eq(authentication), any(ActionListener.class)); + + final Authentication authentication = createApiKeyAuthentication(apiKeyService, createAuthentication(), + Collections.singleton(new RoleDescriptor("user_role_" + randomAlphaOfLength(4), new String[]{"manage"}, null, null)), + Collections.singletonList(new RoleDescriptor("key_role_" + randomAlphaOfLength(8), new String[]{"monitor"}, null, null))); PlainActionFuture roleFuture = new PlainActionFuture<>(); compositeRolesStore.getRoles(authentication.getUser(), authentication, roleFuture); Role role = roleFuture.actionGet(); assertThat(role.checkClusterAction("cluster:admin/foo", Empty.INSTANCE, mock(Authentication.class)), is(false)); assertThat(effectiveRoleDescriptors.get(), is(nullValue())); - verify(apiKeyService).getRoleForApiKey(eq(authentication), any(ActionListener.class)); + assertThat(role.names().length, is(1)); + assertThat(role.names()[0], containsString("user_role_")); } public void testUsageStats() { @@ -1184,6 +1184,24 @@ public void testLoggingOfDeprecatedRoles() { ); } + private Authentication createAuthentication() { + final RealmRef lookedUpBy; + final User user; + if (randomBoolean()) { + user = new User("_username", randomBoolean() ? new String[]{"r1"} : + new String[]{ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR.getName()}, + new User("authenticated_username", new String[]{"r2"})); + lookedUpBy = new RealmRef("lookRealm", "up", "by"); + } else { + user = new User("_username", randomBoolean() ? new String[]{"r1"} : + new String[]{ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR.getName()}); + lookedUpBy = null; + } + return new Authentication(user, new RealmRef("authRealm", "test", "foo"), lookedUpBy, + Version.CURRENT, randomFrom(AuthenticationType.REALM, AuthenticationType.TOKEN, AuthenticationType.INTERNAL, + AuthenticationType.ANONYMOUS), Collections.emptyMap()); + } + private CompositeRolesStore buildCompositeRolesStore(Settings settings, @Nullable FileRolesStore fileRolesStore, @Nullable NativeRolesStore nativeRolesStore,