From b67863e67a3be7a7ee761672ecb102fbecbaa28c Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 17 Mar 2020 16:34:00 +1100 Subject: [PATCH] Explicitly require that derived API keys have no privileges (#53647) The current implicit behaviour is that when an API keys is used to create another API key, the child key is created without any privilege. This implicit behaviour is surprising and is a source of confusion for users. This change makes that behaviour explicit. --- .../core/security/authz/RoleDescriptor.java | 10 +++ .../action/TransportCreateApiKeyAction.java | 12 ++++ .../security/authc/ApiKeyIntegTests.java | 71 +++++++++++++++++++ .../security/authz/RoleDescriptorTests.java | 53 ++++++++++++++ 4 files changed, 146 insertions(+) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptor.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptor.java index 832e9c9365e94..fed249544f9ac 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptor.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptor.java @@ -212,6 +212,16 @@ public int hashCode() { return result; } + + public boolean isEmpty() { + return clusterPrivileges.length == 0 + && configurableClusterPrivileges.length == 0 + && indicesPrivileges.length == 0 + && applicationPrivileges.length == 0 + && runAs.length == 0 + && metadata.size() == 0; + } + @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { return toXContent(builder, params, false); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportCreateApiKeyAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportCreateApiKeyAction.java index 730affc7ea953..2f5de387b6b73 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportCreateApiKeyAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportCreateApiKeyAction.java @@ -18,6 +18,7 @@ import org.elasticsearch.xpack.core.security.action.CreateApiKeyRequest; import org.elasticsearch.xpack.core.security.action.CreateApiKeyResponse; import org.elasticsearch.xpack.core.security.authc.Authentication; +import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; import org.elasticsearch.xpack.security.authc.ApiKeyService; import org.elasticsearch.xpack.security.authc.support.ApiKeyGenerator; import org.elasticsearch.xpack.security.authz.store.CompositeRolesStore; @@ -44,7 +45,18 @@ protected void doExecute(Task task, CreateApiKeyRequest request, ActionListener< if (authentication == null) { listener.onFailure(new IllegalStateException("authentication is required")); } else { + if (Authentication.AuthenticationType.API_KEY == authentication.getAuthenticationType() && grantsAnyPrivileges(request)) { + listener.onFailure(new IllegalArgumentException( + "creating derived api keys requires an explicit role descriptor that is empty (has no privileges)")); + return; + } generator.generateApiKey(authentication, request, listener); } } + + private boolean grantsAnyPrivileges(CreateApiKeyRequest request) { + return request.getRoleDescriptors() == null + || request.getRoleDescriptors().isEmpty() + || false == request.getRoleDescriptors().stream().allMatch(RoleDescriptor::isEmpty); + } } 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 9c695ac1ae194..4587395eabd6b 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 @@ -9,6 +9,8 @@ import com.google.common.collect.Sets; import org.elasticsearch.ElasticsearchSecurityException; import org.elasticsearch.action.DocWriteResponse; +import org.elasticsearch.action.admin.indices.refresh.RefreshAction; +import org.elasticsearch.action.admin.indices.refresh.RefreshRequestBuilder; import org.elasticsearch.action.admin.indices.refresh.RefreshResponse; import org.elasticsearch.action.support.PlainActionFuture; import org.elasticsearch.action.support.WriteRequest; @@ -758,6 +760,75 @@ public void testApiKeyWithManageOwnPrivilegeIsAbleToInvalidateItselfButNotAnyOth assertThat(invalidateResponse.getErrors().size(), equalTo(0)); } + public void testDerivedKeys() throws ExecutionException, InterruptedException { + Client client = client().filterWithHeader(Collections.singletonMap("Authorization", + UsernamePasswordToken.basicAuthHeaderValue(SecuritySettingsSource.TEST_SUPERUSER, + SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING))); + final CreateApiKeyResponse response = new CreateApiKeyRequestBuilder(client) + .setName("key-1") + .setRoleDescriptors(Collections.singletonList( + new RoleDescriptor("role", new String[] { "manage_api_key" }, null, null))) + .get(); + + assertEquals("key-1", response.getName()); + assertNotNull(response.getId()); + assertNotNull(response.getKey()); + + // use the first ApiKey for authorized action + final String base64ApiKeyKeyValue = Base64.getEncoder().encodeToString( + (response.getId() + ":" + response.getKey().toString()).getBytes(StandardCharsets.UTF_8)); + final Client clientKey1 = client().filterWithHeader(Collections.singletonMap("Authorization", "ApiKey " + base64ApiKeyKeyValue)); + + final String expectedMessage = "creating derived api keys requires an explicit role descriptor that is empty"; + + final IllegalArgumentException e1 = expectThrows(IllegalArgumentException.class, + () -> new CreateApiKeyRequestBuilder(clientKey1).setName("key-2").get()); + assertThat(e1.getMessage(), containsString(expectedMessage)); + + final IllegalArgumentException e2 = expectThrows(IllegalArgumentException.class, + () -> new CreateApiKeyRequestBuilder(clientKey1).setName("key-3") + .setRoleDescriptors(Collections.emptyList()).get()); + assertThat(e2.getMessage(), containsString(expectedMessage)); + + final IllegalArgumentException e3 = expectThrows(IllegalArgumentException.class, + () -> new CreateApiKeyRequestBuilder(clientKey1).setName("key-4") + .setRoleDescriptors(Collections.singletonList( + new RoleDescriptor("role", new String[] {"manage_own_api_key"}, null, null) + )).get()); + assertThat(e3.getMessage(), containsString(expectedMessage)); + + final List roleDescriptors = randomList(2, 10, + () -> new RoleDescriptor("role", null, null, null)); + roleDescriptors.set(randomInt(roleDescriptors.size() - 1), + new RoleDescriptor("role", new String[] {"manage_own_api_key"}, null, null)); + + final IllegalArgumentException e4 = expectThrows(IllegalArgumentException.class, + () -> new CreateApiKeyRequestBuilder(clientKey1).setName("key-5") + .setRoleDescriptors(roleDescriptors).get()); + assertThat(e4.getMessage(), containsString(expectedMessage)); + + final CreateApiKeyResponse key100Response = new CreateApiKeyRequestBuilder(clientKey1).setName("key-100") + .setRoleDescriptors(Collections.singletonList( + new RoleDescriptor("role", null, null, null) + )).get(); + assertEquals("key-100", key100Response.getName()); + assertNotNull(key100Response.getId()); + assertNotNull(key100Response.getKey()); + + // Check at the end to allow sometime for the operation to happen. Since an erroneous creation is + // asynchronous so that the document is not available immediately. + assertApiKeyNotCreated(client, "key-2"); + assertApiKeyNotCreated(client, "key-3"); + assertApiKeyNotCreated(client, "key-4"); + assertApiKeyNotCreated(client, "key-5"); + } + + private void assertApiKeyNotCreated(Client client, String keyName) throws ExecutionException, InterruptedException { + new RefreshRequestBuilder(client, RefreshAction.INSTANCE).setIndices(SECURITY_MAIN_ALIAS).execute().get(); + assertEquals(0, client.execute(GetApiKeyAction.INSTANCE, + GetApiKeyRequest.usingApiKeyName(keyName, false)).get().getApiKeyInfos().length); + } + private void verifyGetResponse(int expectedNumberOfApiKeys, List responses, GetApiKeyResponse response, Set validApiKeyIds, List invalidatedApiKeyIds) { verifyGetResponse(SecuritySettingsSource.TEST_SUPERUSER, expectedNumberOfApiKeys, responses, response, validApiKeyIds, diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RoleDescriptorTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RoleDescriptorTests.java index 66161ee27ec19..b8661168c3232 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RoleDescriptorTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RoleDescriptorTests.java @@ -32,7 +32,9 @@ import java.io.IOException; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.LinkedHashSet; +import java.util.List; import java.util.Map; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; @@ -325,4 +327,55 @@ public void testParseIndicesPrivilegesFailsWhenExceptFieldsAreNotSubsetOfGranted assertThat(epe, TestMatchers.throwableWithMessage(containsString("f2"))); assertThat(epe, TestMatchers.throwableWithMessage(containsString("f3"))); } + + public void testIsEmpty() { + assertTrue(new RoleDescriptor( + randomAlphaOfLengthBetween(1, 10), null, null, null, null, null, null, null) + .isEmpty()); + + assertTrue(new RoleDescriptor( + randomAlphaOfLengthBetween(1, 10), + new String[0], + new RoleDescriptor.IndicesPrivileges[0], + new RoleDescriptor.ApplicationResourcePrivileges[0], + new ConfigurableClusterPrivilege[0], + new String[0], + new HashMap<>(), + new HashMap<>()) + .isEmpty()); + + final List booleans = Arrays.asList( + randomBoolean(), + randomBoolean(), + randomBoolean(), + randomBoolean(), + randomBoolean(), + randomBoolean()); + + final RoleDescriptor roleDescriptor = new RoleDescriptor( + randomAlphaOfLengthBetween(1, 10), + booleans.get(0) ? new String[0] : new String[] { "foo" }, + booleans.get(1) ? + new RoleDescriptor.IndicesPrivileges[0] : + new RoleDescriptor.IndicesPrivileges[] { + RoleDescriptor.IndicesPrivileges.builder().indices("idx").privileges("foo").build() }, + booleans.get(2) ? + new RoleDescriptor.ApplicationResourcePrivileges[0] : + new RoleDescriptor.ApplicationResourcePrivileges[] { + RoleDescriptor.ApplicationResourcePrivileges.builder() + .application("app").privileges("foo").resources("res").build() }, + booleans.get(3) ? + new ConfigurableClusterPrivilege[0] : + new ConfigurableClusterPrivilege[] { + new ConfigurableClusterPrivileges.ManageApplicationPrivileges(Collections.singleton("foo")) }, + booleans.get(4) ? new String[0] : new String[] { "foo" }, + booleans.get(5) ? new HashMap<>() : Collections.singletonMap("foo", "bar"), + Collections.singletonMap("foo", "bar")); + + if (booleans.stream().anyMatch(e -> e.equals(false))) { + assertFalse(roleDescriptor.isEmpty()); + } else { + assertTrue(roleDescriptor.isEmpty()); + } + } }