From ac209c142c92a6ef67a350242ac851577faf306f Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad <902768+bizybot@users.noreply.github.com> Date: Sat, 12 Oct 2019 22:22:16 +1100 Subject: [PATCH] Remove uniqueness constraint for API key name and make it optional (#47549) (#47959) Since we cannot guarantee the uniqueness of the API key `name` this commit removes the constraint and makes this field optional. Closes #46646 --- .../client/security/CreateApiKeyRequest.java | 10 ++--- .../client/security/support/ApiKey.java | 4 +- .../security/GetApiKeyResponseTests.java | 4 +- .../security/create-api-key.asciidoc | 6 +-- .../security/create-api-keys.asciidoc | 2 +- .../xpack/core/security/action/ApiKey.java | 13 +++++- .../security/action/CreateApiKeyRequest.java | 34 ++++++++-------- .../action/CreateApiKeyRequestTests.java | 10 +---- .../action/GetApiKeyResponseTests.java | 11 +++-- .../xpack/security/authc/ApiKeyService.java | 39 +----------------- .../security/authc/ApiKeyIntegTests.java | 40 +++++-------------- 11 files changed, 60 insertions(+), 113 deletions(-) diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/security/CreateApiKeyRequest.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/security/CreateApiKeyRequest.java index ad5f0a9ba2c..ef866d9b08e 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/security/CreateApiKeyRequest.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/security/CreateApiKeyRequest.java @@ -22,7 +22,6 @@ package org.elasticsearch.client.security; import org.elasticsearch.client.Validatable; import org.elasticsearch.client.security.user.privileges.Role; import org.elasticsearch.common.Nullable; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -47,12 +46,9 @@ public final class CreateApiKeyRequest implements Validatable, ToXContentObject * @param roles list of {@link Role}s * @param expiration to specify expiration for the API key */ - public CreateApiKeyRequest(String name, List roles, @Nullable TimeValue expiration, @Nullable final RefreshPolicy refreshPolicy) { - if (Strings.hasText(name)) { - this.name = name; - } else { - throw new IllegalArgumentException("name must not be null or empty"); - } + public CreateApiKeyRequest(@Nullable String name, List roles, @Nullable TimeValue expiration, + @Nullable final RefreshPolicy refreshPolicy) { + this.name = name; this.roles = Objects.requireNonNull(roles, "roles may not be null"); this.expiration = expiration; this.refreshPolicy = (refreshPolicy == null) ? RefreshPolicy.getDefault() : refreshPolicy; diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/security/support/ApiKey.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/security/support/ApiKey.java index d021628f750..d7065a311a5 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/security/support/ApiKey.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/security/support/ApiKey.java @@ -21,6 +21,7 @@ package org.elasticsearch.client.security.support; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.xcontent.ConstructingObjectParser; +import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentParser; import java.io.IOException; @@ -131,7 +132,8 @@ public final class ApiKey { (args[3] == null) ? null : Instant.ofEpochMilli((Long) args[3]), (Boolean) args[4], (String) args[5], (String) args[6]); }); static { - PARSER.declareString(constructorArg(), new ParseField("name")); + PARSER.declareField(optionalConstructorArg(), (p, c) -> p.textOrNull(), new ParseField("name"), + ObjectParser.ValueType.STRING_OR_NULL); PARSER.declareString(constructorArg(), new ParseField("id")); PARSER.declareLong(constructorArg(), new ParseField("creation")); PARSER.declareLong(optionalConstructorArg(), new ParseField("expiration")); diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/security/GetApiKeyResponseTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/security/GetApiKeyResponseTests.java index 7aa92e4f212..4541f09a09d 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/security/GetApiKeyResponseTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/security/GetApiKeyResponseTests.java @@ -40,7 +40,9 @@ public class GetApiKeyResponseTests extends ESTestCase { "user-a", "realm-x"); ApiKey apiKeyInfo2 = createApiKeyInfo("name2", "id-2", Instant.ofEpochMilli(100000L), Instant.ofEpochMilli(10000000L), true, "user-b", "realm-y"); - GetApiKeyResponse response = new GetApiKeyResponse(Arrays.asList(apiKeyInfo1, apiKeyInfo2)); + ApiKey apiKeyInfo3 = createApiKeyInfo(null, "id-3", Instant.ofEpochMilli(100000L), null, true, + "user-c", "realm-z"); + GetApiKeyResponse response = new GetApiKeyResponse(Arrays.asList(apiKeyInfo1, apiKeyInfo2, apiKeyInfo3)); final XContentType xContentType = randomFrom(XContentType.values()); final XContentBuilder builder = XContentFactory.contentBuilder(xContentType); toXContent(response, builder); diff --git a/docs/java-rest/high-level/security/create-api-key.asciidoc b/docs/java-rest/high-level/security/create-api-key.asciidoc index 8a77f11484d..497c0fb35e4 100644 --- a/docs/java-rest/high-level/security/create-api-key.asciidoc +++ b/docs/java-rest/high-level/security/create-api-key.asciidoc @@ -12,8 +12,8 @@ API Key can be created using this API. [id="{upid}-{api}-request"] ==== Create API Key Request -A +{request}+ contains name for the API key, -list of role descriptors to define permissions and +A +{request}+ contains an optional name for the API key, +an optional list of role descriptors to define permissions and optional expiration for the generated API key. If expiration is not provided then by default the API keys do not expire. @@ -37,4 +37,4 @@ expiration. include-tagged::{doc-tests-file}[{api}-response] -------------------------------------------------- <1> the API key that can be used to authenticate to Elasticsearch. -<2> expiration if the API keys expire \ No newline at end of file +<2> expiration if the API keys expire diff --git a/x-pack/docs/en/rest-api/security/create-api-keys.asciidoc b/x-pack/docs/en/rest-api/security/create-api-keys.asciidoc index 708ddf46702..09d027eb7c3 100644 --- a/x-pack/docs/en/rest-api/security/create-api-keys.asciidoc +++ b/x-pack/docs/en/rest-api/security/create-api-keys.asciidoc @@ -44,7 +44,7 @@ service. The following parameters can be specified in the body of a POST or PUT request: `name`:: -(string) Specifies the name for this API key. +(Optional, string) Specifies the name for this API key. `role_descriptors`:: (Optional, array-of-role-descriptor) An array of role descriptors for this API diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/ApiKey.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/ApiKey.java index bfe9f523062..214881f34ea 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/ApiKey.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/ApiKey.java @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.core.security.action; +import org.elasticsearch.Version; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -49,7 +50,11 @@ public final class ApiKey implements ToXContentObject, Writeable { } public ApiKey(StreamInput in) throws IOException { - this.name = in.readString(); + if (in.getVersion().onOrAfter(Version.V_7_5_0)) { + this.name = in.readOptionalString(); + } else { + this.name = in.readString(); + } this.id = in.readString(); this.creation = in.readInstant(); this.expiration = in.readOptionalInstant(); @@ -103,7 +108,11 @@ public final class ApiKey implements ToXContentObject, Writeable { @Override public void writeTo(StreamOutput out) throws IOException { - out.writeString(name); + if (out.getVersion().onOrAfter(Version.V_7_5_0)) { + out.writeOptionalString(name); + } else { + out.writeString(name); + } out.writeString(id); out.writeInstant(creation); out.writeOptionalInstant(expiration); diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java index 2b03e8875b6..9559016baa3 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java @@ -6,11 +6,11 @@ package org.elasticsearch.xpack.core.security.action; +import org.elasticsearch.Version; import org.elasticsearch.action.ActionRequest; import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.support.WriteRequest; import org.elasticsearch.common.Nullable; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.unit.TimeValue; @@ -43,19 +43,19 @@ public final class CreateApiKeyRequest extends ActionRequest { * @param roleDescriptors list of {@link RoleDescriptor}s * @param expiration to specify expiration for the API key */ - public CreateApiKeyRequest(String name, @Nullable List roleDescriptors, @Nullable TimeValue expiration) { - if (Strings.hasText(name)) { - this.name = name; - } else { - throw new IllegalArgumentException("name must not be null or empty"); - } + public CreateApiKeyRequest(@Nullable String name, @Nullable List roleDescriptors, @Nullable TimeValue expiration) { + this.name = name; this.roleDescriptors = (roleDescriptors == null) ? Collections.emptyList() : Collections.unmodifiableList(roleDescriptors); this.expiration = expiration; } public CreateApiKeyRequest(StreamInput in) throws IOException { super(in); - this.name = in.readString(); + if (in.getVersion().onOrAfter(Version.V_7_5_0)) { + this.name = in.readOptionalString(); + } else { + this.name = in.readString(); + } this.expiration = in.readOptionalTimeValue(); this.roleDescriptors = Collections.unmodifiableList(in.readList(RoleDescriptor::new)); this.refreshPolicy = WriteRequest.RefreshPolicy.readFrom(in); @@ -65,12 +65,8 @@ public final class CreateApiKeyRequest extends ActionRequest { return name; } - public void setName(String name) { - if (Strings.hasText(name)) { - this.name = name; - } else { - throw new IllegalArgumentException("name must not be null or empty"); - } + public void setName(@Nullable String name) { + this.name = name; } public TimeValue getExpiration() { @@ -100,9 +96,7 @@ public final class CreateApiKeyRequest extends ActionRequest { @Override public ActionRequestValidationException validate() { ActionRequestValidationException validationException = null; - if (Strings.isNullOrEmpty(name)) { - validationException = addValidationError("name is required", validationException); - } else { + if (name != null) { if (name.length() > 256) { validationException = addValidationError("name may not be more than 256 characters long", validationException); } @@ -119,7 +113,11 @@ public final class CreateApiKeyRequest extends ActionRequest { @Override public void writeTo(StreamOutput out) throws IOException { super.writeTo(out); - out.writeString(name); + if (out.getVersion().onOrAfter(Version.V_7_5_0)) { + out.writeOptionalString(name); + } else { + out.writeString(name); + } out.writeOptionalTimeValue(expiration); out.writeList(roleDescriptors); refreshPolicy.writeTo(out); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestTests.java index 22ab7aa601c..09b51ab36dc 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestTests.java @@ -29,20 +29,12 @@ public class CreateApiKeyRequestTests extends ESTestCase { CreateApiKeyRequest request = new CreateApiKeyRequest(); ActionRequestValidationException ve = request.validate(); - assertNotNull(ve); - assertThat(ve.validationErrors().size(), is(1)); - assertThat(ve.validationErrors().get(0), containsString("name is required")); + assertNull(ve); request.setName(name); ve = request.validate(); assertNull(ve); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> request.setName("")); - assertThat(e.getMessage(), containsString("name must not be null or empty")); - - e = expectThrows(IllegalArgumentException.class, () -> request.setName(null)); - assertThat(e.getMessage(), containsString("name must not be null or empty")); - request.setName(randomAlphaOfLength(257)); ve = request.validate(); assertNotNull(ve); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/GetApiKeyResponseTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/GetApiKeyResponseTests.java index c278c135eda..4f82ac12230 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/GetApiKeyResponseTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/GetApiKeyResponseTests.java @@ -24,8 +24,9 @@ import static org.hamcrest.Matchers.equalTo; public class GetApiKeyResponseTests extends ESTestCase { public void testSerialization() throws IOException { + boolean withApiKeyName = randomBoolean(); boolean withExpiration = randomBoolean(); - ApiKey apiKeyInfo = createApiKeyInfo(randomAlphaOfLength(4), randomAlphaOfLength(5), Instant.now(), + ApiKey apiKeyInfo = createApiKeyInfo((withApiKeyName) ? randomAlphaOfLength(4) : null, randomAlphaOfLength(5), Instant.now(), (withExpiration) ? Instant.now() : null, false, randomAlphaOfLength(4), randomAlphaOfLength(5)); GetApiKeyResponse response = new GetApiKeyResponse(Collections.singletonList(apiKeyInfo)); try (BytesStreamOutput output = new BytesStreamOutput()) { @@ -42,7 +43,9 @@ public class GetApiKeyResponseTests extends ESTestCase { "user-a", "realm-x"); ApiKey apiKeyInfo2 = createApiKeyInfo("name2", "id-2", Instant.ofEpochMilli(100000L), Instant.ofEpochMilli(10000000L), true, "user-b", "realm-y"); - GetApiKeyResponse response = new GetApiKeyResponse(Arrays.asList(apiKeyInfo1, apiKeyInfo2)); + ApiKey apiKeyInfo3 = createApiKeyInfo(null, "id-3", Instant.ofEpochMilli(100000L), null, true, + "user-c", "realm-z"); + GetApiKeyResponse response = new GetApiKeyResponse(Arrays.asList(apiKeyInfo1, apiKeyInfo2, apiKeyInfo3)); XContentBuilder builder = XContentFactory.jsonBuilder(); response.toXContent(builder, ToXContent.EMPTY_PARAMS); assertThat(Strings.toString(builder), equalTo( @@ -51,7 +54,9 @@ public class GetApiKeyResponseTests extends ESTestCase { + "{\"id\":\"id-1\",\"name\":\"name1\",\"creation\":100000,\"expiration\":10000000,\"invalidated\":false," + "\"username\":\"user-a\",\"realm\":\"realm-x\"}," + "{\"id\":\"id-2\",\"name\":\"name2\",\"creation\":100000,\"expiration\":10000000,\"invalidated\":true," - + "\"username\":\"user-b\",\"realm\":\"realm-y\"}" + + "\"username\":\"user-b\",\"realm\":\"realm-y\"}," + + "{\"id\":\"id-3\",\"name\":null,\"creation\":100000,\"invalidated\":true," + + "\"username\":\"user-c\",\"realm\":\"realm-z\"}" + "]" + "}")); } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java index 08380393043..21494eb3e3f 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java @@ -22,7 +22,6 @@ import org.elasticsearch.action.get.GetRequest; import org.elasticsearch.action.get.GetResponse; import org.elasticsearch.action.index.IndexAction; import org.elasticsearch.action.index.IndexRequest; -import org.elasticsearch.action.search.SearchAction; import org.elasticsearch.action.search.SearchRequest; import org.elasticsearch.action.support.WriteRequest.RefreshPolicy; import org.elasticsearch.action.update.UpdateRequest; @@ -194,46 +193,10 @@ public class ApiKeyService { if (authentication == null) { listener.onFailure(new IllegalArgumentException("authentication must be provided")); } else { - /* - * Check if requested API key name already exists to avoid duplicate key names, - * this check is best effort as there could be two nodes executing search and - * then index concurrently allowing a duplicate name. - */ - checkDuplicateApiKeyNameAndCreateApiKey(authentication, request, userRoles, listener); + createApiKeyAndIndexIt(authentication, request, userRoles, listener); } } - private void checkDuplicateApiKeyNameAndCreateApiKey(Authentication authentication, CreateApiKeyRequest request, - Set userRoles, - ActionListener listener) { - final BoolQueryBuilder boolQuery = QueryBuilders.boolQuery() - .filter(QueryBuilders.termQuery("doc_type", "api_key")) - .filter(QueryBuilders.termQuery("name", request.getName())) - .filter(QueryBuilders.termQuery("api_key_invalidated", false)); - final BoolQueryBuilder expiredQuery = QueryBuilders.boolQuery() - .should(QueryBuilders.rangeQuery("expiration_time").lte(Instant.now().toEpochMilli())) - .should(QueryBuilders.boolQuery().mustNot(QueryBuilders.existsQuery("expiration_time"))); - boolQuery.filter(expiredQuery); - - final SearchRequest searchRequest = client.prepareSearch(SECURITY_MAIN_ALIAS) - .setQuery(boolQuery) - .setVersion(false) - .setSize(1) - .request(); - securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> - executeAsyncWithOrigin(client, SECURITY_ORIGIN, SearchAction.INSTANCE, searchRequest, - ActionListener.wrap( - indexResponse -> { - if (indexResponse.getHits().getTotalHits().value > 0) { - listener.onFailure(traceLog("create api key", new ElasticsearchSecurityException( - "Error creating api key as api key with name [{}] already exists", request.getName()))); - } else { - createApiKeyAndIndexIt(authentication, request, userRoles, listener); - } - }, - listener::onFailure))); - } - private void createApiKeyAndIndexIt(Authentication authentication, CreateApiKeyRequest request, Set roleDescriptorSet, ActionListener listener) { final Instant created = clock.instant(); 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 3f0b4a2ce24..46ff06822d2 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 @@ -172,45 +172,25 @@ public class ApiKeyIntegTests extends SecurityIntegTestCase { assertThat(e.status(), is(RestStatus.FORBIDDEN)); } - public void testCreateApiKeyFailsWhenApiKeyWithSameNameAlreadyExists() throws InterruptedException, ExecutionException { + public void testMultipleApiKeysCanHaveSameName() { String keyName = randomAlphaOfLength(5); + int noOfApiKeys = randomIntBetween(2, 5); List responses = new ArrayList<>(); - { - final RoleDescriptor descriptor = new RoleDescriptor("role", new String[] { "monitor" }, null, null); + for (int i = 0; i < noOfApiKeys; i++) { + final RoleDescriptor descriptor = new RoleDescriptor("role", new String[]{"monitor"}, null, null); Client client = client().filterWithHeader(Collections.singletonMap("Authorization", UsernamePasswordToken - .basicAuthHeaderValue(SecuritySettingsSource.TEST_SUPERUSER, SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING))); + .basicAuthHeaderValue(SecuritySettingsSource.TEST_SUPERUSER, SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING))); SecurityClient securityClient = new SecurityClient(client); final CreateApiKeyResponse response = securityClient.prepareCreateApiKey().setName(keyName).setExpiration(null) - .setRoleDescriptors(Collections.singletonList(descriptor)).get(); + .setRoleDescriptors(Collections.singletonList(descriptor)).get(); assertNotNull(response.getId()); assertNotNull(response.getKey()); responses.add(response); } - - final RoleDescriptor descriptor = new RoleDescriptor("role", new String[] { "monitor" }, null, null); - Client client = client().filterWithHeader(Collections.singletonMap("Authorization", - UsernamePasswordToken.basicAuthHeaderValue(SecuritySettingsSource.TEST_SUPERUSER, - SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING))); - SecurityClient securityClient = new SecurityClient(client); - ElasticsearchSecurityException e = expectThrows(ElasticsearchSecurityException.class, () -> securityClient.prepareCreateApiKey() - .setName(keyName) - .setExpiration(TimeValue.timeValueHours(TimeUnit.DAYS.toHours(7L))) - .setRoleDescriptors(Collections.singletonList(descriptor)) - .get()); - assertThat(e.getMessage(), equalTo("Error creating api key as api key with name ["+keyName+"] already exists")); - - // Now invalidate the API key - PlainActionFuture listener = new PlainActionFuture<>(); - securityClient.invalidateApiKey(InvalidateApiKeyRequest.usingApiKeyName(keyName, false), listener); - InvalidateApiKeyResponse invalidateResponse = listener.get(); - verifyInvalidateResponse(1, responses, invalidateResponse); - - // try to create API key with same name, should succeed now - CreateApiKeyResponse createResponse = securityClient.prepareCreateApiKey().setName(keyName) - .setExpiration(TimeValue.timeValueHours(TimeUnit.DAYS.toHours(7L))) - .setRoleDescriptors(Collections.singletonList(descriptor)).get(); - assertNotNull(createResponse.getId()); - assertNotNull(createResponse.getKey()); + assertThat(responses.size(), is(noOfApiKeys)); + for (int i = 0; i < noOfApiKeys; i++) { + assertThat(responses.get(i).getName(), is(keyName)); + } } public void testInvalidateApiKeysForRealm() throws InterruptedException, ExecutionException {