From 7f21ade92407c5c65d2bd83a42e752838acdde5b Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 17 Mar 2020 17:56:37 +1100 Subject: [PATCH] Explicitly require that derived API keys have no privileges (#53647) (#53648) 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 | 11 +++ .../security/authc/ApiKeyIntegTests.java | 76 +++++++++++++++++++ .../security/authz/RoleDescriptorTests.java | 53 +++++++++++++ 4 files changed, 150 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 bb738ad3f94..fc79c8f4c1b 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 @@ -217,6 +217,16 @@ public class RoleDescriptor implements ToXContentObject, Writeable { 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 72a92516e59..e96e0bdcdba 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 @@ -54,6 +54,11 @@ public final class TransportCreateApiKeyAction extends HandledTransportAction(Arrays.asList(authentication.getUser().roles())), ActionListener.wrap(roleDescriptors -> { for (RoleDescriptor rd : roleDescriptors) { @@ -69,4 +74,10 @@ public final class TransportCreateApiKeyAction extends HandledTransportAction 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_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, + () -> 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_MAIN_ALIAS).execute().get(); + PlainActionFuture getApiKeyResponseListener = new PlainActionFuture<>(); + new SecurityClient(client).getApiKey( + GetApiKeyRequest.usingApiKeyName(keyName, false), getApiKeyResponseListener); + assertEquals(0, getApiKeyResponseListener.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 aad413e2061..d5cbf4c1cbe 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 org.hamcrest.Matchers; 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 class RoleDescriptorTests extends ESTestCase { 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()); + } + } }