Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support metadata on API keys #70292

Merged
merged 20 commits into from
Mar 28, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
refactor
ywangd committed Mar 11, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
commit c0a2456eae2ee2f179d7470df3a9e3dffb6345b5
Original file line number Diff line number Diff line change
@@ -16,6 +16,7 @@

import java.io.IOException;
import java.time.Instant;
import java.util.List;
import java.util.Map;
import java.util.Objects;

@@ -34,8 +35,7 @@ public void testXContent() throws IOException {
final boolean invalidated = randomBoolean();
final String username = randomAlphaOfLengthBetween(4, 10);
final String realmName = randomAlphaOfLengthBetween(3, 8);
final Map<String, Object> metadata =
randomBoolean() ? null : Map.of(randomAlphaOfLengthBetween(3, 8), randomAlphaOfLengthBetween(3, 8));
final Map<String, Object> metadata = randomMetadata();

final ApiKey apiKey = new ApiKey(name, id, creation, expiration, invalidated, username, realmName, metadata);

@@ -57,4 +57,16 @@ public void testXContent() throws IOException {
assertThat(map.get("metadata"), equalTo(Objects.requireNonNullElseGet(metadata, Map::of)));
}

@SuppressWarnings("unchecked")
public static Map<String, Object> randomMetadata() {
return randomFrom(
Map.of("application", randomAlphaOfLength(5),
"number", 1,
"numbers", List.of(1, 3, 5),
"environment", Map.of("os", "linux", "level", 42, "category", "trusted")
),
Map.of(randomAlphaOfLengthBetween(3, 8), randomAlphaOfLengthBetween(3, 8)),
Map.of(),
null);
}
}
Original file line number Diff line number Diff line change
@@ -40,6 +40,7 @@
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.xpack.core.XPackSettings;
import org.elasticsearch.xpack.core.security.action.ApiKey;
import org.elasticsearch.xpack.core.security.action.ApiKeyTests;
import org.elasticsearch.xpack.core.security.action.ClearSecurityCacheAction;
import org.elasticsearch.xpack.core.security.action.ClearSecurityCacheRequest;
import org.elasticsearch.xpack.core.security.action.ClearSecurityCacheResponse;
@@ -55,7 +56,6 @@
import org.elasticsearch.xpack.core.security.action.user.PutUserRequest;
import org.elasticsearch.xpack.core.security.action.user.PutUserResponse;
import org.elasticsearch.xpack.core.security.authz.RoleDescriptor;
import org.elasticsearch.xpack.security.rest.action.apikey.RestGetApiKeyActionTests;
import org.elasticsearch.xpack.security.transport.filter.IPFilter;
import org.junit.After;
import org.junit.Before;
@@ -177,7 +177,7 @@ public void testCreateApiKey() throws Exception {
.setName("test key")
.setExpiration(TimeValue.timeValueHours(TimeUnit.DAYS.toHours(7L)))
.setRoleDescriptors(Collections.singletonList(descriptor))
.setMetadata(RestGetApiKeyActionTests.randomMetadata())
.setMetadata(ApiKeyTests.randomMetadata())
.get();

assertEquals("test key", response.getName());
@@ -227,7 +227,7 @@ public void testMultipleApiKeysCanHaveSameName() {
Collections.singletonMap("Authorization", basicAuthHeaderValue(TEST_SUPERUSER, TEST_PASSWORD_SECURE_STRING)));
final CreateApiKeyResponse response = new CreateApiKeyRequestBuilder(client).setName(keyName).setExpiration(null)
.setRoleDescriptors(Collections.singletonList(descriptor))
.setMetadata(RestGetApiKeyActionTests.randomMetadata()).get();
.setMetadata(ApiKeyTests.randomMetadata()).get();
assertNotNull(response.getId());
assertNotNull(response.getKey());
responses.add(response);
@@ -911,7 +911,7 @@ public void testDerivedKeys() throws ExecutionException, InterruptedException {
.setName("key-1")
.setRoleDescriptors(Collections.singletonList(
new RoleDescriptor("role", new String[] { "manage_api_key" }, null, null)))
.setMetadata(RestGetApiKeyActionTests.randomMetadata())
.setMetadata(ApiKeyTests.randomMetadata())
.get();

assertEquals("key-1", response.getName());
@@ -926,7 +926,7 @@ public void testDerivedKeys() throws ExecutionException, InterruptedException {
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").setMetadata(RestGetApiKeyActionTests.randomMetadata()).get());
() -> new CreateApiKeyRequestBuilder(clientKey1).setName("key-2").setMetadata(ApiKeyTests.randomMetadata()).get());
assertThat(e1.getMessage(), containsString(expectedMessage));

final IllegalArgumentException e2 = expectThrows(IllegalArgumentException.class,
@@ -936,7 +936,7 @@ public void testDerivedKeys() throws ExecutionException, InterruptedException {

final IllegalArgumentException e3 = expectThrows(IllegalArgumentException.class,
() -> new CreateApiKeyRequestBuilder(clientKey1).setName("key-4")
.setMetadata(RestGetApiKeyActionTests.randomMetadata())
.setMetadata(ApiKeyTests.randomMetadata())
.setRoleDescriptors(Collections.singletonList(
new RoleDescriptor("role", new String[] { "manage_own_api_key" }, null, null)
)).get());
@@ -949,12 +949,12 @@ public void testDerivedKeys() throws ExecutionException, InterruptedException {

final IllegalArgumentException e4 = expectThrows(IllegalArgumentException.class,
() -> new CreateApiKeyRequestBuilder(clientKey1).setName("key-5")
.setMetadata(RestGetApiKeyActionTests.randomMetadata())
.setMetadata(ApiKeyTests.randomMetadata())
.setRoleDescriptors(roleDescriptors).get());
assertThat(e4.getMessage(), containsString(expectedMessage));

final CreateApiKeyResponse key100Response = new CreateApiKeyRequestBuilder(clientKey1).setName("key-100")
.setMetadata(RestGetApiKeyActionTests.randomMetadata())
.setMetadata(ApiKeyTests.randomMetadata())
.setRoleDescriptors(Collections.singletonList(
new RoleDescriptor("role", null, null, null)
)).get();
@@ -982,7 +982,7 @@ public void testAuthenticationReturns429WhenThreadPoolIsSaturated() throws IOExc
final CreateApiKeyResponse createApiKeyResponse = new CreateApiKeyRequestBuilder(client)
.setName("auth only key")
.setRoleDescriptors(Collections.singletonList(descriptor))
.setMetadata(RestGetApiKeyActionTests.randomMetadata())
.setMetadata(ApiKeyTests.randomMetadata())
.get();

assertNotNull(createApiKeyResponse.getId());
@@ -1137,7 +1137,7 @@ private Tuple<String, String> createApiKeyAndAuthenticateWithIt() throws IOExcep

final CreateApiKeyResponse createApiKeyResponse = new CreateApiKeyRequestBuilder(client)
.setName("test key")
.setMetadata(RestGetApiKeyActionTests.randomMetadata())
.setMetadata(ApiKeyTests.randomMetadata())
.get();
final String docId = createApiKeyResponse.getId();
final String base64ApiKeyKeyValue = Base64.getEncoder().encodeToString(
@@ -1235,7 +1235,7 @@ private Tuple<List<CreateApiKeyResponse>, List<Map<String, Object>>> createApiKe
for (int i = 0; i < noOfApiKeys; i++) {
final RoleDescriptor descriptor = new RoleDescriptor("role", clusterPrivileges, null, null);
Client client = client().filterWithHeader(headers);
final Map<String, Object> metadata = RestGetApiKeyActionTests.randomMetadata();
final Map<String, Object> metadata = ApiKeyTests.randomMetadata();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems weird. Everything else in this method is provided by the caller, but the metadata is randomly generated and returned.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about letting callers pass it in, but then realised the callers were just going to pass ApiKeyTests.randomMetadata() anyway because there is no place where the metadata needs to be more specific. So instead of changing every call sites, I just added it here as an easy change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like we accumulating debt - we don't really know what we want our test-object-creation strategy to be, so we have a weird mix.
I don't know what the answer is, but I think this is a sign we're doing something wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't really know what we want our test-object-creation strategy to be

I think this is true for the metadata field, at least for now. But it has a reasonable explanation. The metadata currently really does not do much other than being stored literally and retrieved literally. Therefore a simple random is sufficient for existing tests. We will need a better strategy when the metadata is searchable. When that happens, we will need to craft the metadata more specifically because we need to search for specific things. I think this can wait till we start working on the metadata search part.

The existing createApiKeys methods are also aligned with the above reasoning. We currently have five of them:

  1. createApiKeys(int noOfApiKeys, TimeValue expiration)
  2. createApiKeys(String user, int noOfApiKeys, TimeValue expiration, String... clusterPrivileges)
  3. createApiKeys(String owningUser, String authenticatingUser, int noOfApiKeys, TimeValue expiration, String... clusterPrivileges)
  4. createApiKeys(Map<String, String> headers, int noOfApiKeys, TimeValue expiration, String... clusterPrivileges)
  5. createApiKeys(Map<String, String> headers, int noOfApiKeys, String namePrefix, TimeValue expiration, String... clusterPrivileges)

They are listed roughly in the order of delegation (or increasing complexity), i.e. (1) calls (2), (2) call (3), etc. For the more generic variation, say (1), it basically randomly chooses most of the attributes for an API key, while the more specific variation, (5), gets to specify many of the attributes. It is also where the metadata is currently randomised because the existing tests do not care about the exact content of metadata. When we start working on the metadata search, I imagine we can add a 6th variation of createApiKeys so it takes the metadata as an argument, i.e.:
createApiKeys(Map<String, String> headers, int noOfApiKeys, String namePrefix, TimeValue expiration, Map<String, Object> metadata, String... clusterPrivileges) and current variation (5) will delegate its call to (6).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have a smell - the ApiKeyIntegTests are a bit weirdly stuck together, and would benefit from having some structured thinking about what they're trying to achieve and how best to do that.

I think that's a separate issue though.

metadatas.add(metadata);
final CreateApiKeyResponse response = new CreateApiKeyRequestBuilder(client)
.setName(namePrefix + randomAlphaOfLengthBetween(5, 9) + i).setExpiration(expiration)
Original file line number Diff line number Diff line change
@@ -44,6 +44,7 @@
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.xpack.core.XPackSettings;
import org.elasticsearch.xpack.core.security.SecurityContext;
import org.elasticsearch.xpack.core.security.action.ApiKeyTests;
import org.elasticsearch.xpack.core.security.action.CreateApiKeyRequest;
import org.elasticsearch.xpack.core.security.authc.Authentication;
import org.elasticsearch.xpack.core.security.authc.Authentication.AuthenticationType;
@@ -59,7 +60,6 @@
import org.elasticsearch.xpack.security.authc.ApiKeyService.ApiKeyRoleDescriptors;
import org.elasticsearch.xpack.security.authc.ApiKeyService.CachedApiKeyHashResult;
import org.elasticsearch.xpack.security.authz.store.NativePrivilegeStore;
import org.elasticsearch.xpack.security.rest.action.apikey.RestGetApiKeyActionTests;
import org.elasticsearch.xpack.security.support.CacheInvalidatorRegistry;
import org.elasticsearch.xpack.security.support.FeatureNotEnabledException;
import org.elasticsearch.xpack.security.support.SecurityIndexManager;
@@ -342,7 +342,7 @@ Version.CURRENT, randomFrom(AuthenticationType.REALM, AuthenticationType.TOKEN,
AuthenticationType.ANONYMOUS), Collections.emptyMap());
}
@SuppressWarnings("unchecked")
final Map<String, Object> metadata = RestGetApiKeyActionTests.randomMetadata();
final Map<String, Object> metadata = ApiKeyTests.randomMetadata();
XContentBuilder docSource = service.newDocument(new SecureString(key.toCharArray()), "test", authentication,
Collections.singleton(SUPERUSER_ROLE_DESCRIPTOR), Instant.now(), Instant.now().plus(expiry), keyRoles,
Version.CURRENT, metadata);
@@ -1090,7 +1090,7 @@ private Map<String, Object> buildApiKeySourceDoc(char[] hash) {
sourceMap.put("creator", creatorMap);
sourceMap.put("api_key_invalidated", false);
//noinspection unchecked
sourceMap.put("metadata_flattened", RestGetApiKeyActionTests.randomMetadata());
sourceMap.put("metadata_flattened", ApiKeyTests.randomMetadata());
return sourceMap;
}

@@ -1109,7 +1109,7 @@ private void mockSourceDocument(String id, Map<String, Object> sourceMap) throws

private ApiKeyDoc buildApiKeyDoc(char[] hash, long expirationTime, boolean invalidated) throws IOException {
final BytesReference metadataBytes =
XContentTestUtils.convertToXContent(RestGetApiKeyActionTests.randomMetadata(), XContentType.JSON);
XContentTestUtils.convertToXContent(ApiKeyTests.randomMetadata(), XContentType.JSON);
return new ApiKeyDoc(
"api_key",
Clock.systemUTC().instant().toEpochMilli(),
Original file line number Diff line number Diff line change
@@ -29,6 +29,7 @@
import org.elasticsearch.test.rest.FakeRestRequest;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.xpack.core.security.action.ApiKey;
import org.elasticsearch.xpack.core.security.action.ApiKeyTests;
import org.elasticsearch.xpack.core.security.action.GetApiKeyRequest;
import org.elasticsearch.xpack.core.security.action.GetApiKeyResponse;

@@ -85,7 +86,7 @@ public void sendResponse(RestResponse restResponse) {
final Instant creation = Instant.now();
final Instant expiration = randomFrom(Arrays.asList(null, Instant.now().plus(10, ChronoUnit.DAYS)));
@SuppressWarnings("unchecked")
final Map<String, Object> metadata = randomMetadata();
final Map<String, Object> metadata = ApiKeyTests.randomMetadata();
final GetApiKeyResponse getApiKeyResponseExpected = new GetApiKeyResponse(
Collections.singletonList(
new ApiKey("api-key-name-1", "api-key-id-1", creation, expiration, false, "user-x", "realm-1", metadata)));
@@ -159,9 +160,9 @@ public void sendResponse(RestResponse restResponse) {
final Instant creation = Instant.now();
final Instant expiration = randomFrom(Arrays.asList(null, Instant.now().plus(10, ChronoUnit.DAYS)));
final ApiKey apiKey1 = new ApiKey("api-key-name-1", "api-key-id-1", creation, expiration, false,
"user-x", "realm-1", randomMetadata());
"user-x", "realm-1", ApiKeyTests.randomMetadata());
final ApiKey apiKey2 = new ApiKey("api-key-name-2", "api-key-id-2", creation, expiration, false,
"user-y", "realm-1", randomMetadata());
"user-y", "realm-1", ApiKeyTests.randomMetadata());
final GetApiKeyResponse getApiKeyResponseExpectedWhenOwnerFlagIsTrue = new GetApiKeyResponse(Collections.singletonList(apiKey1));
final GetApiKeyResponse getApiKeyResponseExpectedWhenOwnerFlagIsFalse = new GetApiKeyResponse(List.of(apiKey1, apiKey2));

@@ -206,19 +207,6 @@ void doExecute(ActionType<Response> action, Request request, ActionListener<Resp

}

@SuppressWarnings("unchecked")
public static Map<String, Object> randomMetadata() {
return randomFrom(
Map.of("application", randomAlphaOfLength(5),
"number", 1,
"numbers", List.of(1, 3, 5),
"environment", Map.of("os", "linux", "level", 42, "category", "trusted")
),
Map.of(randomAlphaOfLengthBetween(3, 8), randomAlphaOfLengthBetween(3, 8)),
Map.of(),
null);
}

private static MapBuilder<String, String> mapBuilder() {
return MapBuilder.newMapBuilder();
}