From 04fabc2089b7474737c333694e1ccc214077ad86 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 4 Aug 2020 11:44:32 +1000 Subject: [PATCH 1/2] API key name should always be required for creation (#59836) The name is now required when creating or granting API keys. --- .../client/security/CreateApiKeyRequest.java | 2 +- .../security/action/CreateApiKeyRequest.java | 15 ++++++++------ .../action/CreateApiKeyRequestTests.java | 13 ++++++------ .../xpack/security/apikey/ApiKeyRestIT.java | 20 +++++++++++++++++++ .../security/authc/ApiKeyIntegTests.java | 10 ++++++++++ 5 files changed, 47 insertions(+), 13 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 ef866d9b08eb5..07b97871d1b95 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 @@ -46,7 +46,7 @@ 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(@Nullable String name, List roles, @Nullable TimeValue expiration, + public CreateApiKeyRequest(String name, List roles, @Nullable TimeValue expiration, @Nullable final RefreshPolicy refreshPolicy) { this.name = name; this.roles = Objects.requireNonNull(roles, "roles may not be null"); 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 9559016baa3db..2bbcdcb580fab 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 @@ -11,6 +11,7 @@ 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,7 +44,7 @@ public CreateApiKeyRequest() {} * @param roleDescriptors list of {@link RoleDescriptor}s * @param expiration to specify expiration for the API key */ - public CreateApiKeyRequest(@Nullable String name, @Nullable List roleDescriptors, @Nullable TimeValue expiration) { + public CreateApiKeyRequest(String name, @Nullable List roleDescriptors, @Nullable TimeValue expiration) { this.name = name; this.roleDescriptors = (roleDescriptors == null) ? Collections.emptyList() : Collections.unmodifiableList(roleDescriptors); this.expiration = expiration; @@ -65,7 +66,7 @@ public String getName() { return name; } - public void setName(@Nullable String name) { + public void setName(String name) { this.name = name; } @@ -96,15 +97,17 @@ public void setRefreshPolicy(WriteRequest.RefreshPolicy refreshPolicy) { @Override public ActionRequestValidationException validate() { ActionRequestValidationException validationException = null; - if (name != null) { + if (Strings.isNullOrEmpty(name)) { + validationException = addValidationError("api key name is required", validationException); + } else { if (name.length() > 256) { - validationException = addValidationError("name may not be more than 256 characters long", validationException); + validationException = addValidationError("api key name may not be more than 256 characters long", validationException); } if (name.equals(name.trim()) == false) { - validationException = addValidationError("name may not begin or end with whitespace", validationException); + validationException = addValidationError("api key name may not begin or end with whitespace", validationException); } if (name.startsWith("_")) { - validationException = addValidationError("name may not begin with an underscore", validationException); + validationException = addValidationError("api key name may not begin with an underscore", validationException); } } return validationException; 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 09b51ab36dcd1..e096e4b8164fc 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,7 +29,8 @@ public void testNameValidation() { CreateApiKeyRequest request = new CreateApiKeyRequest(); ActionRequestValidationException ve = request.validate(); - assertNull(ve); + assertThat(ve.validationErrors().size(), is(1)); + assertThat(ve.validationErrors().get(0), containsString("api key name is required")); request.setName(name); ve = request.validate(); @@ -39,25 +40,25 @@ public void testNameValidation() { ve = request.validate(); assertNotNull(ve); assertThat(ve.validationErrors().size(), is(1)); - assertThat(ve.validationErrors().get(0), containsString("name may not be more than 256 characters long")); + assertThat(ve.validationErrors().get(0), containsString("api key name may not be more than 256 characters long")); request.setName(" leading space"); ve = request.validate(); assertNotNull(ve); assertThat(ve.validationErrors().size(), is(1)); - assertThat(ve.validationErrors().get(0), containsString("name may not begin or end with whitespace")); + assertThat(ve.validationErrors().get(0), containsString("api key name may not begin or end with whitespace")); request.setName("trailing space "); ve = request.validate(); assertNotNull(ve); assertThat(ve.validationErrors().size(), is(1)); - assertThat(ve.validationErrors().get(0), containsString("name may not begin or end with whitespace")); + assertThat(ve.validationErrors().get(0), containsString("api key name may not begin or end with whitespace")); request.setName(" leading and trailing space "); ve = request.validate(); assertNotNull(ve); assertThat(ve.validationErrors().size(), is(1)); - assertThat(ve.validationErrors().get(0), containsString("name may not begin or end with whitespace")); + assertThat(ve.validationErrors().get(0), containsString("api key name may not begin or end with whitespace")); request.setName("inner space"); ve = request.validate(); @@ -67,7 +68,7 @@ public void testNameValidation() { ve = request.validate(); assertNotNull(ve); assertThat(ve.validationErrors().size(), is(1)); - assertThat(ve.validationErrors().get(0), containsString("name may not begin with an underscore")); + assertThat(ve.validationErrors().get(0), containsString("api key name may not begin with an underscore")); } public void testSerialization() throws IOException { diff --git a/x-pack/plugin/security/qa/security-trial/src/test/java/org/elasticsearch/xpack/security/apikey/ApiKeyRestIT.java b/x-pack/plugin/security/qa/security-trial/src/test/java/org/elasticsearch/xpack/security/apikey/ApiKeyRestIT.java index df993f3180ed2..388582c725be6 100644 --- a/x-pack/plugin/security/qa/security-trial/src/test/java/org/elasticsearch/xpack/security/apikey/ApiKeyRestIT.java +++ b/x-pack/plugin/security/qa/security-trial/src/test/java/org/elasticsearch/xpack/security/apikey/ApiKeyRestIT.java @@ -9,6 +9,7 @@ import org.elasticsearch.client.Request; import org.elasticsearch.client.RequestOptions; import org.elasticsearch.client.Response; +import org.elasticsearch.client.ResponseException; import org.elasticsearch.client.security.support.ApiKey; import org.elasticsearch.common.collect.MapBuilder; import org.elasticsearch.common.collect.Tuple; @@ -27,6 +28,7 @@ import java.util.HashMap; import java.util.Map; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThanOrEqualTo; import static org.hamcrest.Matchers.instanceOf; @@ -114,5 +116,23 @@ public void testGrantApiKeyForOtherUserWithAccessToken() throws IOException { assertThat(apiKey.getExpiration(), greaterThanOrEqualTo(minExpiry)); assertThat(apiKey.getExpiration(), lessThanOrEqualTo(maxExpiry)); } + + public void testGrantApiKeyWithoutApiKeyNameWillFail() throws IOException { + Request request = new Request("POST", "_security/api_key/grant"); + request.setOptions(RequestOptions.DEFAULT.toBuilder().addHeader("Authorization", + UsernamePasswordToken.basicAuthHeaderValue(SYSTEM_USER, SYSTEM_USER_PASSWORD))); + final Map requestBody = Map.ofEntries( + Map.entry("grant_type", "password"), + Map.entry("username", END_USER), + Map.entry("password", END_USER_PASSWORD.toString()) + ); + request.setJsonEntity(XContentTestUtils.convertToXContent(requestBody, XContentType.JSON).utf8ToString()); + + final ResponseException e = + expectThrows(ResponseException.class, () -> client().performRequest(request)); + + assertEquals(400, e.getResponse().getStatusLine().getStatusCode()); + assertThat(e.getMessage(), containsString("api key name is required")); + } } 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 ef3c4914353ba..e71da05a399cc 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 @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.security.authc; import org.elasticsearch.ElasticsearchSecurityException; +import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.DocWriteResponse; import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse; import org.elasticsearch.action.admin.cluster.node.info.NodeInfo; @@ -223,6 +224,15 @@ public void testMultipleApiKeysCanHaveSameName() { } } + public void testCreateApiKeyWithoutNameWillFail() { + Client client = client().filterWithHeader(Collections.singletonMap("Authorization", + UsernamePasswordToken.basicAuthHeaderValue(SecuritySettingsSource.TEST_SUPERUSER, + SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING))); + final ActionRequestValidationException e = + expectThrows(ActionRequestValidationException.class, () -> new CreateApiKeyRequestBuilder(client).get()); + assertThat(e.getMessage(), containsString("api key name is required")); + } + public void testInvalidateApiKeysForRealm() throws InterruptedException, ExecutionException { int noOfApiKeys = randomIntBetween(3, 5); List responses = createApiKeys(noOfApiKeys, null); From 9c7c2ca7d4698945cfa451c631f3faf218e7a9aa Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 4 Aug 2020 11:57:02 +1000 Subject: [PATCH 2/2] Fix backports --- .../elasticsearch/xpack/security/apikey/ApiKeyRestIT.java | 8 ++++---- .../xpack/security/authc/ApiKeyIntegTests.java | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/x-pack/plugin/security/qa/security-trial/src/test/java/org/elasticsearch/xpack/security/apikey/ApiKeyRestIT.java b/x-pack/plugin/security/qa/security-trial/src/test/java/org/elasticsearch/xpack/security/apikey/ApiKeyRestIT.java index 388582c725be6..11b88338c43f4 100644 --- a/x-pack/plugin/security/qa/security-trial/src/test/java/org/elasticsearch/xpack/security/apikey/ApiKeyRestIT.java +++ b/x-pack/plugin/security/qa/security-trial/src/test/java/org/elasticsearch/xpack/security/apikey/ApiKeyRestIT.java @@ -121,10 +121,10 @@ public void testGrantApiKeyWithoutApiKeyNameWillFail() throws IOException { Request request = new Request("POST", "_security/api_key/grant"); request.setOptions(RequestOptions.DEFAULT.toBuilder().addHeader("Authorization", UsernamePasswordToken.basicAuthHeaderValue(SYSTEM_USER, SYSTEM_USER_PASSWORD))); - final Map requestBody = Map.ofEntries( - Map.entry("grant_type", "password"), - Map.entry("username", END_USER), - Map.entry("password", END_USER_PASSWORD.toString()) + final Map requestBody = org.elasticsearch.common.collect.Map.ofEntries( + org.elasticsearch.common.collect.Map.entry("grant_type", "password"), + org.elasticsearch.common.collect.Map.entry("username", END_USER), + org.elasticsearch.common.collect.Map.entry("password", END_USER_PASSWORD.toString()) ); request.setJsonEntity(XContentTestUtils.convertToXContent(requestBody, XContentType.JSON).utf8ToString()); 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 e71da05a399cc..ec8c662bf049a 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 @@ -228,8 +228,9 @@ public void testCreateApiKeyWithoutNameWillFail() { Client client = client().filterWithHeader(Collections.singletonMap("Authorization", UsernamePasswordToken.basicAuthHeaderValue(SecuritySettingsSource.TEST_SUPERUSER, SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING))); + SecurityClient securityClient = new SecurityClient(client); final ActionRequestValidationException e = - expectThrows(ActionRequestValidationException.class, () -> new CreateApiKeyRequestBuilder(client).get()); + expectThrows(ActionRequestValidationException.class, () -> securityClient.prepareCreateApiKey().get()); assertThat(e.getMessage(), containsString("api key name is required")); }