From ae6eb0d159d279331514cb57de7767d390597b3a Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Fri, 18 Nov 2016 12:33:45 +1100 Subject: [PATCH] Enforce username validation on PUT security/user (elastic/elasticsearch#4118) Validation was being applied within UsersTool but not via the REST API This also causes some validation messages in PutUser to change. Closes elastic/elasticsearch#4018 Original commit: elastic/x-pack-elasticsearch@8e02146deead45e9fac28334d0e3b7dfc2a2b14a --- .../security/action/user/PutUserRequest.java | 7 +++ .../action/user/PutUserRequestTests.java | 61 +++++++++++++++++++ .../authc/esnative/NativeRealmIntegTests.java | 13 +++- 3 files changed, 79 insertions(+), 2 deletions(-) create mode 100644 elasticsearch/src/test/java/org/elasticsearch/xpack/security/action/user/PutUserRequestTests.java diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/security/action/user/PutUserRequest.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/security/action/user/PutUserRequest.java index 441fdd394cb..702e742963a 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/security/action/user/PutUserRequest.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/security/action/user/PutUserRequest.java @@ -13,8 +13,10 @@ import org.elasticsearch.common.bytes.BytesArray; 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.security.authc.support.CharArrays; import org.elasticsearch.xpack.security.support.MetadataUtils; +import org.elasticsearch.xpack.security.support.Validation; import java.io.IOException; import java.util.Map; @@ -43,6 +45,11 @@ public class PutUserRequest extends ActionRequest implements UserRequest, WriteR 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); diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/security/action/user/PutUserRequestTests.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/security/action/user/PutUserRequestTests.java new file mode 100644 index 00000000000..81b36004447 --- /dev/null +++ b/elasticsearch/src/test/java/org/elasticsearch/xpack/security/action/user/PutUserRequestTests.java @@ -0,0 +1,61 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.security.action.user; + +import org.elasticsearch.action.ActionRequestValidationException; +import org.elasticsearch.test.ESTestCase; + +import java.util.Collections; +import java.util.Date; + +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; + +public class PutUserRequestTests extends ESTestCase { + + public void testValidateReturnsNullForCorrectData() throws Exception { + final PutUserRequest request = new PutUserRequest(); + request.username("foo"); + request.roles("bar"); + request.metadata(Collections.singletonMap("created", new Date())); + final ActionRequestValidationException validation = request.validate(); + assertThat(validation, is(nullValue())); + } + + public void testValidateRejectsNullUserName() throws Exception { + final PutUserRequest request = new PutUserRequest(); + request.username(null); + request.roles("bar"); + final ActionRequestValidationException validation = request.validate(); + assertThat(validation, is(notNullValue())); + assertThat(validation.validationErrors(), contains(is("user is missing"))); + assertThat(validation.validationErrors().size(), is(1)); + } + + public void testValidateRejectsUserNameThatStartsWithAnInvalidCharacter() throws Exception { + final PutUserRequest request = new PutUserRequest(); + request.username("$foo"); + request.roles("bar"); + final ActionRequestValidationException validation = request.validate(); + assertThat(validation, is(notNullValue())); + assertThat(validation.validationErrors(), contains(containsString("must begin with"))); + assertThat(validation.validationErrors().size(), is(1)); + } + + public void testValidateRejectsMetaDataWithLeadingUnderscore() throws Exception { + final PutUserRequest request = new PutUserRequest(); + request.username("foo"); + request.roles("bar"); + request.metadata(Collections.singletonMap("_created", new Date())); + final ActionRequestValidationException validation = request.validate(); + assertThat(validation, is(notNullValue())); + assertThat(validation.validationErrors(), contains(containsString("metadata keys"))); + assertThat(validation.validationErrors().size(), is(1)); + } +} diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java index fae54b824e5..2a79f7bfb16 100644 --- a/elasticsearch/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java +++ b/elasticsearch/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java @@ -479,6 +479,15 @@ public class NativeRealmIntegTests extends NativeRealmIntegTestCase { } } + public void testCannotCreateUserWithLeadingDigitInUserName() throws Exception { + SecurityClient client = securityClient(); + ValidationException v = expectThrows(ValidationException.class, + () -> client.preparePutUser("99bottles", "on-th3-w@ll".toCharArray(), "admin_role").get() + ); + assertThat(v.getMessage(), containsString("valid username")); + assertThat(v.getMessage(), containsString("must begin with")); + } + public void testUsersAndRolesDoNotInterfereWithIndicesStats() throws Exception { client().prepareIndex("foo", "bar").setSource("ignore", "me").get(); @@ -504,7 +513,7 @@ public class NativeRealmIntegTests extends NativeRealmIntegTestCase { final String username = randomFrom(ElasticUser.NAME, KibanaUser.NAME); IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> securityClient().preparePutUser(username, randomBoolean() ? "changeme".toCharArray() : null, "admin").get()); - assertThat(exception.getMessage(), containsString("user [" + username + "] is reserved")); + assertThat(exception.getMessage(), containsString("Username [" + username + "] is reserved")); exception = expectThrows(IllegalArgumentException.class, () -> securityClient().prepareDeleteUser(username).get()); @@ -520,7 +529,7 @@ public class NativeRealmIntegTests extends NativeRealmIntegTestCase { exception = expectThrows(IllegalArgumentException.class, () -> securityClient().preparePutUser(AnonymousUser.DEFAULT_ANONYMOUS_USERNAME, "foobar".toCharArray()).get()); - assertThat(exception.getMessage(), containsString("user [" + AnonymousUser.DEFAULT_ANONYMOUS_USERNAME + "] is anonymous")); + assertThat(exception.getMessage(), containsString("Username [" + AnonymousUser.DEFAULT_ANONYMOUS_USERNAME + "] is reserved")); exception = expectThrows(IllegalArgumentException.class, () -> securityClient().preparePutUser(SystemUser.NAME, "foobar".toCharArray()).get());