From 057c1e463557a988345ecfe14c77df8694a5f5f8 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Sun, 15 Mar 2020 19:48:23 +1100 Subject: [PATCH 1/4] Code re-structure for consistent behaviour --- .../core/security/authz/RoleDescriptor.java | 10 ++++ .../action/TransportCreateApiKeyAction.java | 7 +++ .../security/authc/ApiKeyIntegTests.java | 56 +++++++++++++++++++ .../security/authz/RoleDescriptorTests.java | 53 ++++++++++++++++++ 4 files changed, 126 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..d6ef39d472486 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 Arrays.equals(clusterPrivileges, Strings.EMPTY_ARRAY) + && Arrays.equals(configurableClusterPrivileges, ConfigurableClusterPrivileges.EMPTY_ARRAY) + && Arrays.equals(indicesPrivileges, IndicesPrivileges.NONE) + && Arrays.equals(applicationPrivileges, ApplicationResourcePrivileges.NONE) + && Arrays.equals(runAs, Strings.EMPTY_ARRAY) + && metadata.equals(Collections.emptyMap()); + } + @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..e885787bd0215 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,6 +45,12 @@ 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()) { + if (request.getRoleDescriptors() == null || request.getRoleDescriptors().isEmpty() || + false == request.getRoleDescriptors().stream().allMatch(RoleDescriptor::isEmpty)) { + listener.onFailure(new IllegalArgumentException("derived api keys can only have explicit empty role descriptor")); + } + } generator.generateApiKey(authentication, request, listener); } } 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..267dbac08d356 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 @@ -758,6 +758,62 @@ public void testApiKeyWithManageOwnPrivilegeIsAbleToInvalidateItselfButNotAnyOth assertThat(invalidateResponse.getErrors().size(), equalTo(0)); } + public void testDerivedKeys() { + 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 = "can only have explicit empty role descriptor"; + + 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()); + } + 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()); + } + } } From 835149f44560fb072c91b7d2af9ef1e80c18a885 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 16 Mar 2020 17:37:15 +1100 Subject: [PATCH 2/4] Address feedback --- .../xpack/core/security/authz/RoleDescriptor.java | 12 ++++++------ .../security/action/TransportCreateApiKeyAction.java | 12 +++++++++--- .../xpack/security/authc/ApiKeyIntegTests.java | 2 +- 3 files changed, 16 insertions(+), 10 deletions(-) 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 d6ef39d472486..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 @@ -214,12 +214,12 @@ public int hashCode() { public boolean isEmpty() { - return Arrays.equals(clusterPrivileges, Strings.EMPTY_ARRAY) - && Arrays.equals(configurableClusterPrivileges, ConfigurableClusterPrivileges.EMPTY_ARRAY) - && Arrays.equals(indicesPrivileges, IndicesPrivileges.NONE) - && Arrays.equals(applicationPrivileges, ApplicationResourcePrivileges.NONE) - && Arrays.equals(runAs, Strings.EMPTY_ARRAY) - && metadata.equals(Collections.emptyMap()); + return clusterPrivileges.length == 0 + && configurableClusterPrivileges.length == 0 + && indicesPrivileges.length == 0 + && applicationPrivileges.length == 0 + && runAs.length == 0 + && metadata.size() == 0; } @Override 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 e885787bd0215..332d2ea7a3062 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 @@ -46,12 +46,18 @@ protected void doExecute(Task task, CreateApiKeyRequest request, ActionListener< listener.onFailure(new IllegalStateException("authentication is required")); } else { if (Authentication.AuthenticationType.API_KEY == authentication.getAuthenticationType()) { - if (request.getRoleDescriptors() == null || request.getRoleDescriptors().isEmpty() || - false == request.getRoleDescriptors().stream().allMatch(RoleDescriptor::isEmpty)) { - listener.onFailure(new IllegalArgumentException("derived api keys can only have explicit empty role descriptor")); + if (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 267dbac08d356..47a5576b03086 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 @@ -777,7 +777,7 @@ public void testDerivedKeys() { (response.getId() + ":" + response.getKey().toString()).getBytes(StandardCharsets.UTF_8)); final Client clientKey1 = client().filterWithHeader(Collections.singletonMap("Authorization", "ApiKey " + base64ApiKeyKeyValue)); - final String expectedMessage = "can only have explicit empty role descriptor"; + 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()); From bb1dbccb4fd3bf298919928f3c6dcc7c7db280aa Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 16 Mar 2020 17:55:42 +1100 Subject: [PATCH 3/4] Minor code formatting --- .../action/TransportCreateApiKeyAction.java | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) 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 332d2ea7a3062..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 @@ -45,19 +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()) { - if (grantsAnyPrivileges(request)) { - listener.onFailure(new IllegalArgumentException( - "creating derived api keys requires an explicit role descriptor that is empty (has no privileges)")); - return; - } + 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); + return request.getRoleDescriptors() == null + || request.getRoleDescriptors().isEmpty() + || false == request.getRoleDescriptors().stream().allMatch(RoleDescriptor::isEmpty); } } From 224545d24dbdc85fb5c394b2daec89a706e8e179 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 16 Mar 2020 19:56:21 +1100 Subject: [PATCH 4/4] Add negative tests --- .../xpack/security/authc/ApiKeyIntegTests.java | 17 ++++++++++++++++- 1 file changed, 16 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 47a5576b03086..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,7 +760,7 @@ public void testApiKeyWithManageOwnPrivilegeIsAbleToInvalidateItselfButNotAnyOth assertThat(invalidateResponse.getErrors().size(), equalTo(0)); } - public void testDerivedKeys() { + public void testDerivedKeys() throws ExecutionException, InterruptedException { Client client = client().filterWithHeader(Collections.singletonMap("Authorization", UsernamePasswordToken.basicAuthHeaderValue(SecuritySettingsSource.TEST_SUPERUSER, SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING))); @@ -812,6 +814,19 @@ public void testDerivedKeys() { 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,