Skip to content

Commit

Permalink
Explicitly require that derived API keys have no privileges (#53647)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ywangd authored Mar 17, 2020
1 parent 3608be6 commit b67863e
Show file tree
Hide file tree
Showing 4 changed files with 146 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<RoleDescriptor> 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<CreateApiKeyResponse> responses,
GetApiKeyResponse response, Set<String> validApiKeyIds, List<String> invalidatedApiKeyIds) {
verifyGetResponse(SecuritySettingsSource.TEST_SUPERUSER, expectedNumberOfApiKeys, responses, response, validApiKeyIds,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<Boolean> 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());
}
}
}

0 comments on commit b67863e

Please sign in to comment.