diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/PutUserRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/PutUserRequest.java index c018ad5f73eda..f37072b9cf0fc 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/PutUserRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/PutUserRequest.java @@ -13,10 +13,7 @@ import org.elasticsearch.common.bytes.BytesReference; 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.authc.support.CharArrays; -import org.elasticsearch.xpack.core.security.support.MetadataUtils; -import org.elasticsearch.xpack.core.security.support.Validation; import java.io.IOException; import java.util.Map; @@ -34,6 +31,7 @@ public class PutUserRequest extends ActionRequest implements UserRequest, WriteR private String email; private Map metadata; private char[] passwordHash; + private char[] password; private boolean enabled = true; private RefreshPolicy refreshPolicy = RefreshPolicy.IMMEDIATE; @@ -45,18 +43,15 @@ public ActionRequestValidationException validate() { ActionRequestValidationException validationException = null; if (username == null) { validationException = addValidationError("user is missing", validationException); - } else { - Validation.Error error = Validation.Users.validateUsername(username, false, Settings.EMPTY); - if (error != null) { - validationException = addValidationError(error.toString(), validationException); - } } if (roles == null) { validationException = addValidationError("roles are missing", validationException); } - if (metadata != null && MetadataUtils.containsReservedMetadata(metadata)) { - validationException = addValidationError("metadata keys may not start with [" + MetadataUtils.RESERVED_PREFIX + "]", - validationException); + if (metadata != null && metadata.keySet().stream().anyMatch(s -> s.startsWith("_"))) { + validationException = addValidationError("metadata keys may not start with [_]", validationException); + } + if (password != null && passwordHash != null) { + validationException = addValidationError("only one of [password, passwordHash] can be provided", validationException); } // we do not check for a password hash here since it is possible that the user exists and we don't want to update the password return validationException; @@ -86,8 +81,12 @@ public void passwordHash(@Nullable char[] passwordHash) { this.passwordHash = passwordHash; } - public boolean enabled() { - return enabled; + public void enabled(boolean enabled) { + this.enabled = enabled; + } + + public void password(@Nullable char[] password) { + this.password = password; } /** @@ -130,8 +129,8 @@ public char[] passwordHash() { return passwordHash; } - public void enabled(boolean enabled) { - this.enabled = enabled; + public boolean enabled() { + return enabled; } @Override @@ -139,16 +138,16 @@ public String[] usernames() { return new String[] { username }; } + @Nullable + public char[] password() { + return password; + } + @Override public void readFrom(StreamInput in) throws IOException { super.readFrom(in); username = in.readString(); - BytesReference passwordHashRef = in.readBytesReference(); - if (passwordHashRef == BytesArray.EMPTY) { - passwordHash = null; - } else { - passwordHash = CharArrays.utf8BytesToChars(BytesReference.toBytes(passwordHashRef)); - } + passwordHash = readCharArrayFromStream(in); roles = in.readStringArray(); fullName = in.readOptionalString(); email = in.readOptionalString(); @@ -161,13 +160,10 @@ public void readFrom(StreamInput in) throws IOException { public void writeTo(StreamOutput out) throws IOException { super.writeTo(out); out.writeString(username); - BytesReference passwordHashRef; - if (passwordHash == null) { - passwordHashRef = null; - } else { - passwordHashRef = new BytesArray(CharArrays.toUtf8Bytes(passwordHash)); + writeCharArrayToStream(out, passwordHash); + if (password != null) { + throw new IllegalStateException("password cannot be serialized. it is only used for HL rest"); } - out.writeBytesReference(passwordHashRef); out.writeStringArray(roles); out.writeOptionalString(fullName); out.writeOptionalString(email); @@ -180,4 +176,23 @@ public void writeTo(StreamOutput out) throws IOException { refreshPolicy.writeTo(out); out.writeBoolean(enabled); } + + private static char[] readCharArrayFromStream(StreamInput in) throws IOException { + BytesReference charBytesRef = in.readBytesReference(); + if (charBytesRef == BytesArray.EMPTY) { + return null; + } else { + return CharArrays.utf8BytesToChars(BytesReference.toBytes(charBytesRef)); + } + } + + private static void writeCharArrayToStream(StreamOutput out, char[] chars) throws IOException { + final BytesReference charBytesRef; + if (chars == null) { + charBytesRef = null; + } else { + charBytesRef = new BytesArray(CharArrays.toUtf8Bytes(chars)); + } + out.writeBytesReference(charBytesRef); + } } 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 ebc1612afca1b..a2715896da683 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 @@ -8,6 +8,7 @@ import org.apache.logging.log4j.message.ParameterizedMessage; import org.apache.logging.log4j.util.Supplier; 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; @@ -18,11 +19,15 @@ 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.Validation; import org.elasticsearch.xpack.core.security.user.AnonymousUser; import org.elasticsearch.xpack.core.security.user.SystemUser; +import org.elasticsearch.xpack.core.security.user.XPackSecurityUser; import org.elasticsearch.xpack.core.security.user.XPackUser; import org.elasticsearch.xpack.security.authc.esnative.NativeUsersStore; +import static org.elasticsearch.action.ValidateActions.addValidationError; + public class TransportPutUserAction extends HandledTransportAction { private final NativeUsersStore usersStore; @@ -36,37 +41,62 @@ public TransportPutUserAction(Settings settings, ActionFilters actionFilters, @Override protected void doExecute(Task task, final PutUserRequest request, final ActionListener listener) { + final ActionRequestValidationException validationException = validateRequest(request); + if (validationException != null) { + listener.onFailure(validationException); + } else { + usersStore.putUser(request, new ActionListener() { + @Override + public void onResponse(Boolean created) { + if (created) { + logger.info("added user [{}]", request.username()); + } else { + logger.info("updated user [{}]", request.username()); + } + listener.onResponse(new PutUserResponse(created)); + } + + @Override + public void onFailure(Exception e) { + logger.error((Supplier) () -> new ParameterizedMessage("failed to put user [{}]", request.username()), e); + listener.onFailure(e); + } + }); + } + } + + private ActionRequestValidationException validateRequest(PutUserRequest request) { + ActionRequestValidationException validationException = null; final String username = request.username(); if (ClientReservedRealm.isReserved(username, settings)) { if (AnonymousUser.isAnonymousUsername(username, settings)) { - listener.onFailure(new IllegalArgumentException("user [" + username + "] is anonymous and cannot be modified via the API")); - return; + validationException = + addValidationError("user [" + username + "] is anonymous and cannot be modified via the API", validationException); } else { - listener.onFailure(new IllegalArgumentException("user [" + username + "] is reserved and only the " + - "password can be changed")); - return; + validationException = addValidationError("user [" + username + "] is reserved and only the " + + "password can be changed", validationException); + } + } else if (SystemUser.NAME.equals(username) || XPackUser.NAME.equals(username) || XPackSecurityUser.NAME.equals(username)) { + validationException = addValidationError("user [" + username + "] is internal", validationException); + } else { + Validation.Error usernameError = Validation.Users.validateUsername(username, true, settings); + if (usernameError != null) { + validationException = addValidationError(usernameError.toString(), validationException); } - } else if (SystemUser.NAME.equals(username) || XPackUser.NAME.equals(username)) { - listener.onFailure(new IllegalArgumentException("user [" + username + "] is internal")); - return; } - usersStore.putUser(request, new ActionListener() { - @Override - public void onResponse(Boolean created) { - if (created) { - logger.info("added user [{}]", request.username()); - } else { - logger.info("updated user [{}]", request.username()); + if (request.roles() != null) { + for (String role : request.roles()) { + Validation.Error roleNameError = Validation.Roles.validateRoleName(role, true); + if (roleNameError != null) { + validationException = addValidationError(roleNameError.toString(), validationException); } - listener.onResponse(new PutUserResponse(created)); } + } - @Override - public void onFailure(Exception e) { - logger.error((Supplier) () -> new ParameterizedMessage("failed to put user [{}]", request.username()), e); - listener.onFailure(e); - } - }); + if (request.password() != null) { + validationException = addValidationError("password should never be passed to the transport action", validationException); + } + return validationException; } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/PutUserRequestTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/PutUserRequestTests.java index af3a89c77b6f0..952448db4869d 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/PutUserRequestTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/PutUserRequestTests.java @@ -39,16 +39,6 @@ public void testValidateRejectsNullUserName() throws Exception { assertThat(validation.validationErrors().size(), is(1)); } - public void testValidateRejectsUserNameThatHasInvalidCharacters() throws Exception { - final PutUserRequest request = new PutUserRequest(); - request.username("fóóbár"); - request.roles("bar"); - final ActionRequestValidationException validation = request.validate(); - assertThat(validation, is(notNullValue())); - assertThat(validation.validationErrors(), contains(containsString("must be"))); - assertThat(validation.validationErrors().size(), is(1)); - } - public void testValidateRejectsMetaDataWithLeadingUnderscore() throws Exception { final PutUserRequest request = new PutUserRequest(); request.username("foo"); 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 b7eeb78fad317..36ba3f46b5e9e 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 @@ -7,6 +7,7 @@ import org.elasticsearch.ElasticsearchSecurityException; import org.elasticsearch.action.ActionListener; +import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.PlainActionFuture; import org.elasticsearch.common.ValidationException; @@ -37,6 +38,7 @@ import java.util.Collections; import java.util.concurrent.atomic.AtomicReference; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; @@ -194,12 +196,32 @@ public void onFailure(Exception e) { } }); + assertThat(throwableRef.get(), is(nullValue())); assertThat(responseRef.get(), is(notNullValue())); assertThat(responseRef.get().created(), is(created)); - assertThat(throwableRef.get(), is(nullValue())); verify(usersStore, times(1)).putUser(eq(request), any(ActionListener.class)); } + public void testInvalidUser() { + NativeUsersStore usersStore = mock(NativeUsersStore.class); + TransportService transportService = new TransportService(Settings.EMPTY, mock(Transport.class), null, + TransportService.NOOP_TRANSPORT_INTERCEPTOR, x -> null, null, Collections.emptySet()); + TransportPutUserAction action = new TransportPutUserAction(Settings.EMPTY, mock(ActionFilters.class), + usersStore, transportService); + + final PutUserRequest request = new PutUserRequest(); + request.username("fóóbár"); + request.roles("bar"); + ActionRequestValidationException validation = request.validate(); + assertNull(validation); + + PlainActionFuture responsePlainActionFuture = new PlainActionFuture<>(); + action.doExecute(mock(Task.class), request, responsePlainActionFuture); + validation = expectThrows(ActionRequestValidationException.class, responsePlainActionFuture::actionGet); + assertThat(validation.validationErrors(), contains(containsString("must be"))); + assertThat(validation.validationErrors().size(), is(1)); + } + public void testException() { final Exception e = randomFrom(new ElasticsearchSecurityException(""), new IllegalStateException(), new ValidationException()); final User user = new User("joe"); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java index 7f9e25e77e6dd..6ebf6dca2cf1e 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java @@ -13,7 +13,6 @@ import org.elasticsearch.action.support.PlainActionFuture; import org.elasticsearch.client.Client; import org.elasticsearch.common.Strings; -import org.elasticsearch.common.ValidationException; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.collect.MapBuilder; import org.elasticsearch.common.settings.SecureString; @@ -492,14 +491,14 @@ public void testCannotCreateUserWithShortPassword() throws Exception { client.preparePutUser("joe", randomAlphaOfLengthBetween(0, 5).toCharArray(), hasher, "admin_role").get(); fail("cannot create a user without a password < 6 characters"); - } catch (ValidationException v) { + } catch (IllegalArgumentException v) { assertThat(v.getMessage().contains("password"), is(true)); } } public void testCannotCreateUserWithInvalidCharactersInName() throws Exception { SecurityClient client = securityClient(); - ValidationException v = expectThrows(ValidationException.class, + IllegalArgumentException v = expectThrows(IllegalArgumentException.class, () -> client.preparePutUser("fóóbár", "my-am@zing-password".toCharArray(), hasher, "admin_role").get() ); @@ -533,7 +532,7 @@ public void testOperationsOnReservedUsers() throws Exception { IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> securityClient().preparePutUser(username, randomBoolean() ? SecuritySettingsSourceField.TEST_PASSWORD.toCharArray() : null, hasher, "admin").get()); - assertThat(exception.getMessage(), containsString("Username [" + username + "] is reserved")); + assertThat(exception.getMessage(), containsString("user [" + username + "] is reserved")); exception = expectThrows(IllegalArgumentException.class, () -> securityClient().prepareDeleteUser(username).get()); @@ -551,7 +550,7 @@ public void testOperationsOnReservedUsers() throws Exception { exception = expectThrows(IllegalArgumentException.class, () -> securityClient().preparePutUser(AnonymousUser.DEFAULT_ANONYMOUS_USERNAME, "foobar".toCharArray(), hasher).get()); - assertThat(exception.getMessage(), containsString("Username [" + AnonymousUser.DEFAULT_ANONYMOUS_USERNAME + "] is reserved")); + assertThat(exception.getMessage(), containsString("user [" + AnonymousUser.DEFAULT_ANONYMOUS_USERNAME + "] is anonymous")); exception = expectThrows(IllegalArgumentException.class, () -> securityClient().preparePutUser(SystemUser.NAME, "foobar".toCharArray(), hasher).get());