From 55d7293a7b5f3b023bd498ca1bccdadcf9894600 Mon Sep 17 00:00:00 2001 From: Jay Modi Date: Tue, 14 Aug 2018 13:18:07 -0600 Subject: [PATCH] Move validation to server for put user requests (#32471) This change moves the validation for values of usernames and passwords from the request to the transport action. This is done to prevent the need to move more classes into protocol once we add this API to the high level rest client. Additionally, this resolves an issue where validation depends on settings and we always pass empty settings instead of the actual settings. Relates #32332 --- .../security/action/user/PutUserRequest.java | 69 ++++++++++------- .../action/user/TransportPutUserAction.java | 74 +++++++++++++------ .../action/user/PutUserRequestTests.java | 10 --- .../user/TransportPutUserActionTests.java | 24 +++++- .../authc/esnative/NativeRealmIntegTests.java | 9 +-- 5 files changed, 121 insertions(+), 65 deletions(-) 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 1d2f792b29809..e51e6422f53fb 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.cluster.metadata.IndexNameExpressionResolver; @@ -19,11 +20,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; @@ -38,37 +43,62 @@ public TransportPutUserAction(Settings settings, ThreadPool threadPool, ActionFi @Override protected void doExecute(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 cb4192b93d768..a4f6fa9f4d230 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.cluster.metadata.IndexNameExpressionResolver; @@ -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; @@ -196,12 +198,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(ThreadPool.class), mock(ActionFilters.class), + mock(IndexNameExpressionResolver.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(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 5589d328bc11d..8556c3456b478 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; @@ -490,14 +489,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() ); @@ -531,7 +530,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()); @@ -547,7 +546,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());