Remove uniqueness constraint for API key name and make it optional (#47549)

Since we cannot guarantee the uniqueness of the API key `name` this commit removes the constraint and makes this field optional.

Closes #46646

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<Role> 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<Role> 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;

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"));

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);

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
+<2> expiration if the API keys expire

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

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);

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<RoleDescriptor> 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<RoleDescriptor> roleDescriptors, @Nullable TimeValue expiration) {
+        this.name = name;
         this.roleDescriptors = (roleDescriptors == null) ? List.of() : List.copyOf(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 = List.copyOf(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);

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestTests.java

@@ -28,20 +28,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);

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\"}"
                 + "]"
                 + "}"));
     }

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;
@@ -193,46 +192,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<RoleDescriptor> userRoles,
-                                                         ActionListener<CreateApiKeyResponse> 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<RoleDescriptor> roleDescriptorSet,
                                         ActionListener<CreateApiKeyResponse> listener) {
         final Instant created = clock.instant();

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java

@@ -7,7 +7,6 @@
 package org.elasticsearch.xpack.security.authc;
 
 import com.google.common.collect.Sets;
-
 import org.elasticsearch.ElasticsearchSecurityException;
 import org.elasticsearch.action.DocWriteResponse;
 import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse;
@@ -175,43 +174,24 @@ 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<CreateApiKeyResponse> 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)));
             final CreateApiKeyResponse response = new CreateApiKeyRequestBuilder(client).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)));
-        ElasticsearchSecurityException e = expectThrows(ElasticsearchSecurityException.class, () -> new CreateApiKeyRequestBuilder(client)
-            .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<InvalidateApiKeyResponse> listener = new PlainActionFuture<>();
-        client.execute(InvalidateApiKeyAction.INSTANCE, 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 = new CreateApiKeyRequestBuilder(client).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 {