From 1c5f53076b625c44111c1e9430dc36327a088583 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 18 Mar 2020 09:48:44 +1100 Subject: [PATCH] Explicitly require that derived API keys have no privileges (#53647) (#53650) * 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 | 9 +++ .../action/TransportCreateApiKeyAction.java | 12 +++ .../security/authc/ApiKeyIntegTests.java | 79 +++++++++++++++++++ .../security/authz/RoleDescriptorTests.java | 53 +++++++++++++ 4 files changed, 153 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 dc506881b1db1..f852560c561a3 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 @@ -216,6 +216,15 @@ public int hashCode() { return result; } + public boolean isEmpty() { + return clusterPrivileges.length == 0 + && conditionalClusterPrivileges.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 c4f521d65ad37..643f35de209bc 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 @@ -19,6 +19,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.authz.store.CompositeRolesStore; @@ -51,9 +52,20 @@ protected void doExecute(CreateApiKeyRequest request, ActionListener(Arrays.asList(authentication.getUser().roles())), ActionListener.wrap(roleDescriptors -> apiKeyService.createApiKey(authentication, request, roleDescriptors, listener), listener::onFailure)); } } + + 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 79bc3a349102f..112f3faea4af2 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 @@ -11,6 +11,8 @@ import org.elasticsearch.ElasticsearchSecurityException; import org.elasticsearch.action.DocWriteResponse; import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse; +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; @@ -52,6 +54,7 @@ import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; +import static org.elasticsearch.xpack.core.security.index.RestrictedIndicesNames.SECURITY_INDEX_NAME; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; @@ -518,6 +521,82 @@ public void testGetApiKeysForApiKeyName() throws InterruptedException, Execution verifyGetResponse(1, responses, response, Collections.singleton(responses.get(0).getId()), null); } + public void testDerivedKeys() throws ExecutionException, InterruptedException { + final Client client = client().filterWithHeader(Collections.singletonMap("Authorization", + UsernamePasswordToken.basicAuthHeaderValue(SecuritySettingsSource.TEST_SUPERUSER, + SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING))); + + final CreateApiKeyResponse response = new SecurityClient(client) + .prepareCreateApiKey() + .setName("key-1") + .setRoleDescriptors(Collections.singletonList( + new RoleDescriptor("role", new String[] { "manage_security" }, 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 SecurityClient clientKey1 = new SecurityClient( + 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, + () -> clientKey1.prepareCreateApiKey().setName("key-2").get()); + assertThat(e1.getMessage(), containsString(expectedMessage)); + + final IllegalArgumentException e2 = expectThrows(IllegalArgumentException.class, + () -> clientKey1.prepareCreateApiKey().setName("key-3") + .setRoleDescriptors(Collections.emptyList()).get()); + assertThat(e2.getMessage(), containsString(expectedMessage)); + + final IllegalArgumentException e3 = expectThrows(IllegalArgumentException.class, + () -> clientKey1.prepareCreateApiKey().setName("key-4") + .setRoleDescriptors(Collections.singletonList( + new RoleDescriptor("role", new String[] {"manage_security"}, null, null) + )).get()); + assertThat(e3.getMessage(), containsString(expectedMessage)); + + final List roleDescriptors = new ArrayList<>(); + for (int i = 0; i < randomIntBetween(2, 10); i++) { + roleDescriptors.add(new RoleDescriptor("role", null, null, null)); + } + roleDescriptors.set(randomInt(roleDescriptors.size() - 1), + new RoleDescriptor("role", new String[] {"manage_security"}, null, null)); + + final IllegalArgumentException e4 = expectThrows(IllegalArgumentException.class, + () -> clientKey1.prepareCreateApiKey().setName("key-5") + .setRoleDescriptors(roleDescriptors).get()); + assertThat(e4.getMessage(), containsString(expectedMessage)); + + final CreateApiKeyResponse key100Response = clientKey1.prepareCreateApiKey().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_INDEX_NAME).execute().get(); + PlainActionFuture getApiKeyResponseListener = new PlainActionFuture<>(); + new SecurityClient(client).getApiKey( + GetApiKeyRequest.usingApiKeyName(keyName), getApiKeyResponseListener); + assertEquals(0, getApiKeyResponseListener.get().getApiKeyInfos().length); + } + private void verifyGetResponse(int noOfApiKeys, List responses, GetApiKeyResponse response, Set validApiKeyIds, List invalidatedApiKeyIds) { 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 896fbaf7d42cd..27ffdca6f937a 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 @@ -27,7 +27,9 @@ 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; @@ -277,4 +279,55 @@ public void testParseIgnoresTransientMetadata() throws Exception { assertEquals(1, parsed.getTransientMetadata().size()); assertEquals(true, parsed.getTransientMetadata().get("enabled")); } + + 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 ConditionalClusterPrivileges.ManageApplicationPrivileges[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 ConditionalClusterPrivileges.ManageApplicationPrivileges[0] : + new ConditionalClusterPrivileges.ManageApplicationPrivileges[] { + new ConditionalClusterPrivileges.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()); + } + } }