diff --git a/docs/changelog/86728.yaml b/docs/changelog/86728.yaml new file mode 100644 index 0000000000000..b36a3d5dfea30 --- /dev/null +++ b/docs/changelog/86728.yaml @@ -0,0 +1,6 @@ +pr: 86728 +summary: Make user and role name constraint consistent with max document ID +area: Security +type: bug +issues: + - 66020 diff --git a/server/src/main/java/org/elasticsearch/action/index/IndexRequest.java b/server/src/main/java/org/elasticsearch/action/index/IndexRequest.java index 93da92c0c066d..52a4c52f8ef95 100644 --- a/server/src/main/java/org/elasticsearch/action/index/IndexRequest.java +++ b/server/src/main/java/org/elasticsearch/action/index/IndexRequest.java @@ -76,6 +76,11 @@ public class IndexRequest extends ReplicatedWriteRequest implement */ static final int MAX_SOURCE_LENGTH_IN_TOSTRING = 2048; + /** + * Maximal allowed length (in bytes) of the document ID. + */ + public static final int MAX_DOCUMENT_ID_LENGTH_IN_BYTES = 512; + private static final ShardId NO_SHARD_ID = null; private String id; @@ -218,9 +223,14 @@ public ActionRequestValidationException validate() { validationException = DocWriteRequest.validateSeqNoBasedCASParams(this, validationException); - if (id != null && id.getBytes(StandardCharsets.UTF_8).length > 512) { + if (id != null && id.getBytes(StandardCharsets.UTF_8).length > MAX_DOCUMENT_ID_LENGTH_IN_BYTES) { validationException = addValidationError( - "id [" + id + "] is too long, must be no longer than 512 bytes but was: " + id.getBytes(StandardCharsets.UTF_8).length, + "id [" + + id + + "] is too long, must be no longer than " + + MAX_DOCUMENT_ID_LENGTH_IN_BYTES + + " bytes but was: " + + id.getBytes(StandardCharsets.UTF_8).length, validationException ); } @@ -381,7 +391,7 @@ public IndexRequest source(Map source, XContentType contentType) thro /** * Sets the document source to index. * - * Note, its preferable to either set it using {@link #source(org.elasticsearch.common.xcontent.XContentBuilder)} + * Note, its preferable to either set it using {@link #source(org.elasticsearch.xcontent.XContentBuilder)} * or using the {@link #source(byte[], XContentType)}. */ public IndexRequest source(String source, XContentType xContentType) { diff --git a/x-pack/docs/en/rest-api/security/create-users.asciidoc b/x-pack/docs/en/rest-api/security/create-users.asciidoc index 22b6bcaeaa605..428df1102329c 100644 --- a/x-pack/docs/en/rest-api/security/create-users.asciidoc +++ b/x-pack/docs/en/rest-api/security/create-users.asciidoc @@ -41,7 +41,7 @@ For more information about the native realm, see + -- [[username-validation]] -NOTE: Usernames must be at least 1 and no more than 1024 characters. They can +NOTE: Usernames must be at least 1 and no more than 507 characters. They can contain alphanumeric characters (`a-z`, `A-Z`, `0-9`), spaces, punctuation, and printable symbols in the {wikipedia}/Basic_Latin_(Unicode_block)[Basic Latin (ASCII) block]. Leading or trailing whitespace is not allowed. diff --git a/x-pack/docs/en/security/authorization/managing-roles.asciidoc b/x-pack/docs/en/security/authorization/managing-roles.asciidoc index 49f068ad7bfd1..92eb4fb0654b7 100644 --- a/x-pack/docs/en/security/authorization/managing-roles.asciidoc +++ b/x-pack/docs/en/security/authorization/managing-roles.asciidoc @@ -33,7 +33,7 @@ A role is defined by the following JSON structure: <5> A list of application privilege entries. This field is optional. [[valid-role-name]] -NOTE: Role names must be at least 1 and no more than 1024 characters. They can +NOTE: Role names must be at least 1 and no more than 507 characters. They can contain alphanumeric characters (`a-z`, `A-Z`, `0-9`), spaces, punctuation, and printable symbols in the {wikipedia}/Basic_Latin_(Unicode_block)[Basic Latin (ASCII) block]. Leading or trailing whitespace is not allowed. diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/role/PutRoleRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/role/PutRoleRequest.java index a16a3c578ddf4..734d2f32d669c 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/role/PutRoleRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/role/PutRoleRequest.java @@ -17,6 +17,8 @@ import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; import org.elasticsearch.xpack.core.security.authz.privilege.ConfigurableClusterPrivilege; import org.elasticsearch.xpack.core.security.authz.privilege.ConfigurableClusterPrivileges; +import org.elasticsearch.xpack.core.security.support.NativeRealmValidationUtil; +import org.elasticsearch.xpack.core.security.support.Validation; import java.io.IOException; import java.util.ArrayList; @@ -25,6 +27,8 @@ import java.util.List; import java.util.Map; +import static org.elasticsearch.action.ValidateActions.addValidationError; + /** * Request object for adding a role to the security index */ @@ -59,7 +63,12 @@ public PutRoleRequest() {} @Override public ActionRequestValidationException validate() { - return RoleDescriptorRequestValidator.validate(roleDescriptor()); + ActionRequestValidationException validationException = null; + Validation.Error error = NativeRealmValidationUtil.validateRoleName(this.name, false); + if (error != null) { + validationException = addValidationError(error.toString(), validationException); + } + return RoleDescriptorRequestValidator.validate(roleDescriptor(), validationException); } public void name(String name) { diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/SetEnabledRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/SetEnabledRequest.java index 5ea40848270ec..7b5019498751c 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/SetEnabledRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/SetEnabledRequest.java @@ -12,8 +12,8 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.xpack.core.security.support.NativeRealmValidationUtil; import org.elasticsearch.xpack.core.security.support.Validation.Error; -import org.elasticsearch.xpack.core.security.support.Validation.Users; import java.io.IOException; @@ -40,7 +40,7 @@ public SetEnabledRequest(StreamInput in) throws IOException { @Override public ActionRequestValidationException validate() { ActionRequestValidationException validationException = null; - Error error = Users.validateUsername(username, true, Settings.EMPTY); + Error error = NativeRealmValidationUtil.validateUsername(username, true, Settings.EMPTY); if (error != null) { validationException = addValidationError(error.toString(), validationException); } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/NativeRealmValidationUtil.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/NativeRealmValidationUtil.java new file mode 100644 index 0000000000000..8064a55086db2 --- /dev/null +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/NativeRealmValidationUtil.java @@ -0,0 +1,42 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.core.security.support; + +import org.elasticsearch.common.settings.Settings; + +/** + * Provides utility methods for validating user and role names stored in the native realm. + * Note: This class is a wrapper of the {@link Validation} which allow names to have {@link #MAX_NAME_LENGTH}. + */ +public final class NativeRealmValidationUtil { + + /** + * In the native realm, the maximal user and role name lenght is influenced by the maximal document ID + * (see {@link org.elasticsearch.action.index.IndexRequest#MAX_DOCUMENT_ID_LENGTH_IN_BYTES}). + * Because the document IDs are prefixed by either {@code user-} or {@code role-}, + * the actual possible max length is 507 chars. + * + * Note: If we choose (in the future) to allow more than 507 chars we should + * either consider increasing the max allowed size for document ID or + * consider hashing names longer than 507 in order to fit into document ID. + */ + public static final int MAX_NAME_LENGTH = 507; + + public static Validation.Error validateUsername(String username, boolean allowReserved, Settings settings) { + return Validation.Users.validateUsername(username, allowReserved, settings, MAX_NAME_LENGTH); + } + + public static Validation.Error validateRoleName(String roleName, boolean allowReserved) { + return Validation.Roles.validateRoleName(roleName, allowReserved, MAX_NAME_LENGTH); + } + + private NativeRealmValidationUtil() { + throw new IllegalAccessError("Not allowed!"); + } + +} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/Validation.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/Validation.java index 36fe9f6ade005..3c482b82075fc 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/Validation.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/Validation.java @@ -20,7 +20,7 @@ public final class Validation { static final int MIN_NAME_LENGTH = 1; static final int MAX_NAME_LENGTH = 1024; - static final Set VALID_NAME_CHARS = Set.of( + public static final Set VALID_NAME_CHARS = Set.of( ' ', '!', '"', @@ -120,9 +120,7 @@ public final class Validation { private static final String INVALID_NAME_MESSAGE = "%1s names must be at least " + MIN_NAME_LENGTH - + " and no more than " - + MAX_NAME_LENGTH - + " characters. " + + " and no more than %s characters. " + "They can contain alphanumeric characters (a-z, A-Z, 0-9), spaces, punctuation, and printable symbols in the " + "Basic Latin (ASCII) block. Leading or trailing whitespace is not allowed."; @@ -132,8 +130,10 @@ public final class Validation { + "and at most 256 characters that are alphanumeric (A-Z, a-z, 0-9) or hyphen (-) or underscore (_). " + "It must not begin with an underscore (_)."; - private static boolean isValidUserOrRoleName(String name) { - if (name.length() < MIN_NAME_LENGTH || name.length() > MAX_NAME_LENGTH) { + private static boolean isValidUserOrRoleName(String name, int maxLength) { + assert maxLength >= MIN_NAME_LENGTH : "maxLength cannot be less than " + MIN_NAME_LENGTH; + + if (name.length() < MIN_NAME_LENGTH || name.length() > maxLength) { return false; } @@ -178,8 +178,15 @@ public static final class Users { * @return {@code null} if valid */ public static Error validateUsername(String username, boolean allowReserved, Settings settings) { - if (isValidUserOrRoleName(username) == false) { - return new Error(String.format(Locale.ROOT, INVALID_NAME_MESSAGE, "User")); + return validateUsername(username, allowReserved, settings, MAX_NAME_LENGTH); + } + + static Error validateUsername(String username, boolean allowReserved, Settings settings, int maxLength) { + if (username == null) { + return new Error("username is missing"); + } + if (isValidUserOrRoleName(username, maxLength) == false) { + return new Error(String.format(Locale.ROOT, INVALID_NAME_MESSAGE, "User", maxLength)); } if (allowReserved == false && ClientReservedRealm.isReserved(username, settings)) { return new Error("Username [" + username + "] is reserved and may not be used."); @@ -197,13 +204,16 @@ public static Error validatePassword(SecureString password) { public static final class Roles { - public static Error validateRoleName(String roleName) { - return validateRoleName(roleName, false); + public static Error validateRoleName(String roleName, boolean allowReserved) { + return validateRoleName(roleName, allowReserved, MAX_NAME_LENGTH); } - public static Error validateRoleName(String roleName, boolean allowReserved) { - if (isValidUserOrRoleName(roleName) == false) { - return new Error(String.format(Locale.ROOT, INVALID_NAME_MESSAGE, "Role")); + static Error validateRoleName(String roleName, boolean allowReserved, int maxLength) { + if (roleName == null) { + return new Error("role name is missing"); + } + if (isValidUserOrRoleName(roleName, maxLength) == false) { + return new Error(String.format(Locale.ROOT, INVALID_NAME_MESSAGE, "Role", maxLength)); } if (allowReserved == false && ReservedRolesStore.isReserved(roleName)) { return new Error("Role [" + roleName + "] is reserved and may not be used."); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/role/PutRoleRequestTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/role/PutRoleRequestTests.java index 9ed8c2e2e99ca..7f371661691e4 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/role/PutRoleRequestTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/role/PutRoleRequestTests.java @@ -23,6 +23,7 @@ import org.elasticsearch.xpack.core.security.authz.RoleDescriptor.ApplicationResourcePrivileges; import org.elasticsearch.xpack.core.security.authz.privilege.ConfigurableClusterPrivilege; import org.elasticsearch.xpack.core.security.authz.privilege.ConfigurableClusterPrivileges; +import org.elasticsearch.xpack.core.security.support.NativeRealmValidationUtil; import java.io.IOException; import java.util.Arrays; @@ -49,6 +50,17 @@ public void testValidationErrorWithUnknownClusterPrivilegeName() { assertValidationError("unknown cluster privilege [" + unknownClusterPrivilegeName.toLowerCase(Locale.ROOT) + "]", request); } + public void testValidationErrorWithTooLongRoleName() { + final PutRoleRequest request = new PutRoleRequest(); + request.name( + randomAlphaOfLengthBetween(NativeRealmValidationUtil.MAX_NAME_LENGTH + 1, NativeRealmValidationUtil.MAX_NAME_LENGTH * 2) + ); + request.cluster("manage_security"); + + // Fail + assertValidationError("Role names must be at least 1 and no more than " + NativeRealmValidationUtil.MAX_NAME_LENGTH, request); + } + public void testValidationSuccessWithCorrectClusterPrivilegeName() { final PutRoleRequest request = new PutRoleRequest(); request.name(randomAlphaOfLengthBetween(4, 9)); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/support/ValidationTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/support/ValidationTests.java index cb0de4f46af5a..2f9cb65bc59fb 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/support/ValidationTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/support/ValidationTests.java @@ -133,6 +133,8 @@ public class ValidationTests extends ESTestCase { '\r' ); + private static final int CUSTOM_MAX_NAME_LENGTH = 507; + public void testUsernameValid() throws Exception { int length = randomIntBetween(Validation.MIN_NAME_LENGTH, Validation.MAX_NAME_LENGTH); String name = new String(generateValidName(length)); @@ -147,7 +149,7 @@ public void testUsernameReserved() { } public void testUsernameInvalidLength() throws Exception { - int length = frequently() ? randomIntBetween(Validation.MAX_NAME_LENGTH + 1, 2048) : 0; + int length = frequently() ? randomIntBetween(Validation.MAX_NAME_LENGTH + 1, Validation.MAX_NAME_LENGTH * 2) : 0; char[] name = new char[length]; if (length > 0) { name = generateValidName(length); @@ -167,25 +169,15 @@ public void testUsernameInvalidWhitespace() throws Exception { assertThat(Users.validateUsername(name, false, Settings.EMPTY), notNullValue()); } - public void testUsersValidatePassword() throws Exception { - SecureString passwd = new SecureString(randomAlphaOfLength(randomIntBetween(0, 20)).toCharArray()); - logger.info("{}[{}]", passwd, passwd.length()); - if (passwd.length() >= 6) { - assertThat(Users.validatePassword(passwd), nullValue()); - } else { - assertThat(Users.validatePassword(passwd), notNullValue()); - } - } - public void testRoleNameValid() throws Exception { int length = randomIntBetween(Validation.MIN_NAME_LENGTH, Validation.MAX_NAME_LENGTH); String name = new String(generateValidName(length)); - assertThat(Roles.validateRoleName(name), nullValue()); + assertThat(Roles.validateRoleName(name, false), nullValue()); } public void testRoleNameReserved() { final String rolename = randomFrom(ReservedRolesStore.names()); - final Error error = Roles.validateRoleName(rolename); + final Error error = Roles.validateRoleName(rolename, false, Validation.MAX_NAME_LENGTH); assertNotNull(error); assertThat(error.toString(), containsString("is reserved")); @@ -194,7 +186,7 @@ public void testRoleNameReserved() { } public void testRoleNameInvalidLength() throws Exception { - int length = frequently() ? randomIntBetween(Validation.MAX_NAME_LENGTH + 1, 2048) : 0; + int length = frequently() ? randomIntBetween(Validation.MAX_NAME_LENGTH + 1, Validation.MAX_NAME_LENGTH * 2) : 0; char[] name = new char[length]; if (length > 0) { name = generateValidName(length); @@ -214,6 +206,87 @@ public void testRoleNameInvalidWhitespace() throws Exception { assertThat(Roles.validateRoleName(name, false), notNullValue()); } + public void testUsernameValidWithCustomLength() { + int length = randomIntBetween(Validation.MIN_NAME_LENGTH, CUSTOM_MAX_NAME_LENGTH); + String name = new String(generateValidName(length)); + assertThat(Users.validateUsername(name, false, Settings.EMPTY, CUSTOM_MAX_NAME_LENGTH), nullValue()); + } + + public void testUsernameReservedWithCustomLength() { + final String username = randomFrom(ElasticUser.NAME, KibanaUser.NAME); + final Error error = Users.validateUsername(username, false, Settings.EMPTY, CUSTOM_MAX_NAME_LENGTH); + assertNotNull(error); + assertThat(error.toString(), containsString("is reserved")); + } + + public void testUsernameInvalidLengthWithCustomLength() { + int length = frequently() ? randomIntBetween(CUSTOM_MAX_NAME_LENGTH + 1, CUSTOM_MAX_NAME_LENGTH * 2) : 0; + char[] name = new char[length]; + if (length > 0) { + name = generateValidName(length); + } + assertThat(Users.validateUsername(new String(name), false, Settings.EMPTY, CUSTOM_MAX_NAME_LENGTH), notNullValue()); + } + + public void testUsernameInvalidCharactersWithCustomLength() { + int length = randomIntBetween(Validation.MIN_NAME_LENGTH, CUSTOM_MAX_NAME_LENGTH); + String name = new String(generateNameInvalidCharacters(length)); + assertThat(Users.validateUsername(name, false, Settings.EMPTY, CUSTOM_MAX_NAME_LENGTH), notNullValue()); + } + + public void testUsernameInvalidWhitespaceWithCustomLength() { + int length = randomIntBetween(Validation.MIN_NAME_LENGTH, CUSTOM_MAX_NAME_LENGTH); + String name = new String(generateNameInvalidWhitespace(length)); + assertThat(Users.validateUsername(name, false, Settings.EMPTY, CUSTOM_MAX_NAME_LENGTH), notNullValue()); + } + + public void testUsersValidatePassword() { + SecureString passwd = new SecureString(randomAlphaOfLength(randomIntBetween(0, 20)).toCharArray()); + logger.info("{}[{}]", passwd, passwd.length()); + if (passwd.length() >= 6) { + assertThat(Users.validatePassword(passwd), nullValue()); + } else { + assertThat(Users.validatePassword(passwd), notNullValue()); + } + } + + public void testRoleNameValidWithCustomLength() { + int length = randomIntBetween(Validation.MIN_NAME_LENGTH, CUSTOM_MAX_NAME_LENGTH); + String name = new String(generateValidName(length)); + assertThat(Roles.validateRoleName(name, false, CUSTOM_MAX_NAME_LENGTH), nullValue()); + } + + public void testRoleNameReservedWithCustomLength() { + final String rolename = randomFrom(ReservedRolesStore.names()); + final Error error = Roles.validateRoleName(rolename, false, CUSTOM_MAX_NAME_LENGTH); + assertNotNull(error); + assertThat(error.toString(), containsString("is reserved")); + + final Error allowed = Roles.validateRoleName(rolename, true, CUSTOM_MAX_NAME_LENGTH); + assertNull(allowed); + } + + public void testRoleNameInvalidLengthWithCustomLength() throws Exception { + int length = frequently() ? randomIntBetween(CUSTOM_MAX_NAME_LENGTH + 1, CUSTOM_MAX_NAME_LENGTH * 2) : 0; + char[] name = new char[length]; + if (length > 0) { + name = generateValidName(length); + } + assertThat(Roles.validateRoleName(new String(name), false, CUSTOM_MAX_NAME_LENGTH), notNullValue()); + } + + public void testRoleNameInvalidCharactersWithCustomLength() throws Exception { + int length = randomIntBetween(Validation.MIN_NAME_LENGTH, CUSTOM_MAX_NAME_LENGTH); + String name = new String(generateNameInvalidCharacters(length)); + assertThat(Roles.validateRoleName(name, false, CUSTOM_MAX_NAME_LENGTH), notNullValue()); + } + + public void testRoleNameInvalidWhitespaceWithCustomLength() throws Exception { + int length = randomIntBetween(Validation.MIN_NAME_LENGTH, CUSTOM_MAX_NAME_LENGTH); + String name = new String(generateNameInvalidWhitespace(length)); + assertThat(Roles.validateRoleName(name, false, CUSTOM_MAX_NAME_LENGTH), notNullValue()); + } + public void testIsValidServiceAccountTokenName() { final String tokenName1 = ValidationTests.randomTokenName(); assertThat(Validation.isValidServiceAccountTokenName(tokenName1), is(true)); diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java index 9986effa002ee..a5acaffdc547c 100644 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java @@ -756,10 +756,10 @@ public void testOperationsOnReservedRoles() throws Exception { IllegalArgumentException.class, () -> preparePutRole(name).cluster("monitor").get() ); - assertThat(exception.getMessage(), containsString("role [" + name + "] is reserved")); + assertThat(exception.getMessage(), containsString("Role [" + name + "] is reserved and may not be used")); exception = expectThrows(IllegalArgumentException.class, () -> new DeleteRoleRequestBuilder(client()).name(name).get()); - assertThat(exception.getMessage(), containsString("role [" + name + "] is reserved")); + assertThat(exception.getMessage(), containsString("role [" + name + "] is reserved and cannot be deleted")); // get role is allowed GetRolesResponse response = new GetRolesRequestBuilder(client()).names(name).get(); diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authz/SnapshotUserRoleIntegTests.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authz/SnapshotUserRoleIntegTests.java index 67e3323f5b9ca..713728065abdb 100644 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authz/SnapshotUserRoleIntegTests.java +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authz/SnapshotUserRoleIntegTests.java @@ -109,7 +109,7 @@ public void testSnapshotUserRoleIsReserved() { ResponseException.class, () -> securityClient.putRole(new RoleDescriptor("snapshot_user", new String[] { "all" }, null, new String[] { "*" })) ); - assertThat(e.getMessage(), containsString("role [snapshot_user] is reserved and cannot be modified")); + assertThat(e.getMessage(), containsString("Role [snapshot_user] is reserved and may not be used")); e = expectThrows(ResponseException.class, () -> securityClient.deleteRole("snapshot_user")); assertThat(e.getMessage(), containsString("role [snapshot_user] is reserved and cannot be deleted")); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleAction.java index df80f4ccae142..6144e8d71879f 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleAction.java @@ -8,6 +8,7 @@ import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.ActionListener; +import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.HandledTransportAction; import org.elasticsearch.common.inject.Inject; @@ -17,7 +18,6 @@ import org.elasticsearch.xpack.core.security.action.role.PutRoleAction; import org.elasticsearch.xpack.core.security.action.role.PutRoleRequest; import org.elasticsearch.xpack.core.security.action.role.PutRoleResponse; -import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore; import org.elasticsearch.xpack.core.security.authz.support.DLSRoleQueryValidator; import org.elasticsearch.xpack.security.authz.store.NativeRolesStore; @@ -40,26 +40,31 @@ public TransportPutRoleAction( @Override protected void doExecute(Task task, final PutRoleRequest request, final ActionListener listener) { - final String name = request.roleDescriptor().getName(); - if (ReservedRolesStore.isReserved(name)) { - listener.onFailure(new IllegalArgumentException("role [" + name + "] is reserved and cannot be modified.")); - return; + final Exception validationException = validateRequest(request); + if (validationException != null) { + listener.onFailure(validationException); + } else { + rolesStore.putRole(request, request.roleDescriptor(), listener.delegateFailure((l, created) -> { + if (created) { + logger.info("added role [{}]", request.name()); + } else { + logger.info("updated role [{}]", request.name()); + } + l.onResponse(new PutRoleResponse(created)); + })); } + } + private Exception validateRequest(final PutRoleRequest request) { + ActionRequestValidationException validationException = request.validate(); + if (validationException != null) { + return validationException; + } try { DLSRoleQueryValidator.validateQueryField(request.roleDescriptor().getIndicesPrivileges(), xContentRegistry); } catch (ElasticsearchException | IllegalArgumentException e) { - listener.onFailure(e); - return; + return e; } - - rolesStore.putRole(request, request.roleDescriptor(), listener.delegateFailure((l, created) -> { - if (created) { - logger.info("added role [{}]", request.name()); - } else { - logger.info("updated role [{}]", request.name()); - } - l.onResponse(new PutRoleResponse(created)); - })); + return null; } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportPutUserAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportPutUserAction.java index bc3e78bdec1f4..fe93cc9de4c9a 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportPutUserAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportPutUserAction.java @@ -19,6 +19,7 @@ import org.elasticsearch.xpack.core.security.action.user.PutUserRequest; import org.elasticsearch.xpack.core.security.action.user.PutUserResponse; import org.elasticsearch.xpack.core.security.authc.esnative.ClientReservedRealm; +import org.elasticsearch.xpack.core.security.support.NativeRealmValidationUtil; import org.elasticsearch.xpack.core.security.support.Validation; import org.elasticsearch.xpack.core.security.user.AnonymousUser; import org.elasticsearch.xpack.security.authc.esnative.NativeUsersStore; @@ -84,7 +85,7 @@ private ActionRequestValidationException validateRequest(PutUserRequest request) ); } } else { - Validation.Error usernameError = Validation.Users.validateUsername(username, true, settings); + Validation.Error usernameError = NativeRealmValidationUtil.validateUsername(username, true, settings); if (usernameError != null) { validationException = addValidationError(usernameError.toString(), validationException); } @@ -92,7 +93,7 @@ private ActionRequestValidationException validateRequest(PutUserRequest request) if (request.roles() != null) { for (String role : request.roles()) { - Validation.Error roleNameError = Validation.Roles.validateRoleName(role, true); + Validation.Error roleNameError = NativeRealmValidationUtil.validateRoleName(role, true); if (roleNameError != null) { validationException = addValidationError(roleNameError.toString(), validationException); } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java index 22d6079f38523..47018eb95776f 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java @@ -297,7 +297,7 @@ static RoleDescriptor parseRoleDescriptor( token = parser.nextToken(); if (token == XContentParser.Token.FIELD_NAME) { roleName = parser.currentName(); - Validation.Error validationError = Validation.Roles.validateRoleName(roleName); + Validation.Error validationError = Validation.Roles.validateRoleName(roleName, false); if (validationError != null) { logger.error( "invalid role definition [{}] in roles file [{}]. invalid role name - {}. skipping role... ", diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java index d10bf701a872e..0f0ff65adc241 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java @@ -45,6 +45,7 @@ import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; import org.elasticsearch.xpack.core.security.authz.RoleDescriptor.IndicesPrivileges; import org.elasticsearch.xpack.core.security.authz.store.RoleRetrievalResult; +import org.elasticsearch.xpack.core.security.support.NativeRealmValidationUtil; import org.elasticsearch.xpack.security.support.SecurityIndexManager; import java.io.IOException; @@ -217,6 +218,9 @@ public void putRole(final PutRoleRequest request, final RoleDescriptor role, fin // pkg-private for testing void innerPutRole(final PutRoleRequest request, final RoleDescriptor role, final ActionListener listener) { + final String roleName = role.getName(); + assert NativeRealmValidationUtil.validateRoleName(roleName, false) == null : "Role name was invalid or reserved: " + roleName; + securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> { final XContentBuilder xContentBuilder; try { @@ -226,7 +230,7 @@ void innerPutRole(final PutRoleRequest request, final RoleDescriptor role, final return; } final IndexRequest indexRequest = client.prepareIndex(SECURITY_MAIN_ALIAS) - .setId(getIdForRole(role.getName())) + .setId(getIdForRole(roleName)) .setSource(xContentBuilder) .setRefreshPolicy(request.getRefreshPolicy()) .request(); @@ -239,12 +243,12 @@ void innerPutRole(final PutRoleRequest request, final RoleDescriptor role, final public void onResponse(IndexResponse indexResponse) { final boolean created = indexResponse.getResult() == DocWriteResponse.Result.CREATED; logger.trace("Created role: [{}]", indexRequest); - clearRoleCache(role.getName(), listener, created); + clearRoleCache(roleName, listener, created); } @Override public void onFailure(Exception e) { - logger.error(() -> "failed to put role [" + request.name() + "]", e); + logger.error(() -> "failed to put role [" + roleName + "]", e); listener.onFailure(e); } }, diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java index 35109f5889426..a4d05f3a4d1ab 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java @@ -121,7 +121,7 @@ public void onFailure(Exception e) { assertThat(responseRef.get(), is(nullValue())); assertThat(throwableRef.get(), is(instanceOf(IllegalArgumentException.class))); - assertThat(throwableRef.get().getMessage(), containsString("is reserved and cannot be modified")); + assertThat(throwableRef.get().getMessage(), containsString("is reserved and may not be used")); verifyNoMoreInteractions(rolesStore); } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportPutUserActionTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportPutUserActionTests.java index aa215700bed12..110252856ae5b 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportPutUserActionTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportPutUserActionTests.java @@ -25,6 +25,8 @@ import org.elasticsearch.xpack.core.security.action.user.PutUserResponse; import org.elasticsearch.xpack.core.security.authc.AuthenticationTestHelper; import org.elasticsearch.xpack.core.security.authc.support.Hasher; +import org.elasticsearch.xpack.core.security.support.NativeRealmValidationUtil; +import org.elasticsearch.xpack.core.security.support.Validation; import org.elasticsearch.xpack.core.security.user.AnonymousUser; import org.elasticsearch.xpack.core.security.user.User; import org.elasticsearch.xpack.security.authc.esnative.NativeUsersStore; @@ -149,6 +151,10 @@ public void testValidUserWithInternalUsername() { testValidUser(new User(AuthenticationTestHelper.randomInternalUsername())); } + public void testValidUserWithMaxLengthUsername() { + testValidUser(new User(randomUsername(NativeRealmValidationUtil.MAX_NAME_LENGTH))); + } + private void testValidUser(User user) { NativeUsersStore usersStore = mock(NativeUsersStore.class); TransportService transportService = new TransportService( @@ -199,7 +205,21 @@ public void onFailure(Exception e) { verify(usersStore, times(1)).putUser(eq(request), anyActionListener()); } - public void testInvalidUser() { + public void testInvalidUserWithSpecialChars() { + testInvalidUser(new User("fóóbár")); + } + + public void testInvalidUserWithExtraLongUsername() { + testInvalidUser( + new User( + randomUsername( + randomIntBetween(NativeRealmValidationUtil.MAX_NAME_LENGTH + 1, NativeRealmValidationUtil.MAX_NAME_LENGTH * 2) + ) + ) + ); + } + + private void testInvalidUser(User user) { NativeUsersStore usersStore = mock(NativeUsersStore.class); TransportService transportService = new TransportService( Settings.EMPTY, @@ -213,7 +233,7 @@ public void testInvalidUser() { TransportPutUserAction action = new TransportPutUserAction(Settings.EMPTY, mock(ActionFilters.class), usersStore, transportService); final PutUserRequest request = new PutUserRequest(); - request.username("fóóbár"); + request.username(user.principal()); request.roles("bar"); ActionRequestValidationException validation = request.validate(); assertNull(validation); @@ -225,6 +245,17 @@ public void testInvalidUser() { assertThat(validation.validationErrors().size(), is(1)); } + /** + * Generates a random username whose length is exactly as given {@code length}. + */ + private String randomUsername(long length) { + StringBuilder username = new StringBuilder(); + for (int i = 0; i < length; i++) { + username.append(randomFrom(Validation.VALID_NAME_CHARS)); + } + return username.toString(); + } + public void testException() { final Exception e = randomFrom(new ElasticsearchSecurityException(""), new IllegalStateException(), new ValidationException()); final User user = new User("joe");