diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/PutUserRequestBuilder.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/PutUserRequestBuilder.java index eea804d81fe9f..f667bff513b78 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/PutUserRequestBuilder.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/PutUserRequestBuilder.java @@ -54,9 +54,10 @@ public PutUserRequestBuilder password(char[] password, Hasher hasher) { if (password != null) { Validation.Error error = Validation.Users.validatePassword(password); if (error != null) { - ValidationException validationException = new ValidationException(); - validationException.addValidationError(error.toString()); - throw validationException; + throw validationException(error.toString()); + } + if (request.passwordHash() != null) { + throw validationException("password_hash has already been set"); } request.passwordHash(hasher.hash(new SecureString(password))); } else { @@ -80,7 +81,15 @@ public PutUserRequestBuilder email(String email) { return this; } - public PutUserRequestBuilder passwordHash(char[] passwordHash) { + public PutUserRequestBuilder passwordHash(char[] passwordHash, Hasher configuredHasher) { + final Hasher resolvedHasher = Hasher.resolveFromHash(passwordHash); + if (resolvedHasher.equals(configuredHasher) == false) { + throw new IllegalArgumentException("Provided password hash uses [" + resolvedHasher + + "] but the configured hashing algorithm is [" + configuredHasher + "]"); + } + if (request.passwordHash() != null) { + throw validationException("password_hash has already been set"); + } request.passwordHash(passwordHash); return this; } @@ -120,7 +129,7 @@ public PutUserRequestBuilder source(String username, BytesReference source, XCon } else if (User.Fields.PASSWORD_HASH.match(currentFieldName, parser.getDeprecationHandler())) { if (token == XContentParser.Token.VALUE_STRING) { char[] passwordChars = parser.text().toCharArray(); - passwordHash(passwordChars); + passwordHash(passwordChars, hasher); } else { throw new ElasticsearchParseException( "expected field [{}] to be of type string, but found [{}] instead", currentFieldName, token); @@ -177,4 +186,9 @@ public PutUserRequestBuilder source(String username, BytesReference source, XCon } } + private ValidationException validationException(String abc) { + ValidationException validationException = new ValidationException(); + validationException.addValidationError(abc); + return validationException; + } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/PutUserRequestBuilderTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/PutUserRequestBuilderTests.java index a7bad498bdc33..f6b25565b4c35 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/PutUserRequestBuilderTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/PutUserRequestBuilderTests.java @@ -7,7 +7,11 @@ import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.client.Client; +import org.elasticsearch.common.ValidationException; import org.elasticsearch.common.bytes.BytesArray; +import org.elasticsearch.common.bytes.BytesReference; +import org.elasticsearch.common.settings.SecureString; +import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.core.security.action.user.PutUserRequest; @@ -16,9 +20,12 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.util.Collections; +import java.util.LinkedHashMap; import static org.hamcrest.Matchers.arrayContaining; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; import static org.mockito.Mockito.mock; @@ -135,4 +142,69 @@ public void testWithEnabled() throws IOException { builder.source("kibana4", new BytesArray(json.getBytes(StandardCharsets.UTF_8)), XContentType.JSON, Hasher.BCRYPT).request(); assertFalse(request.enabled()); } + + public void testWithValidPasswordHash() throws IOException { + final Hasher hasher = Hasher.BCRYPT4; // this is the fastest hasher we officially support + final char[] hash = hasher.hash(new SecureString("secret".toCharArray())); + final String json = "{\n" + + " \"password_hash\": \"" + new String(hash) + "\"," + + " \"roles\": []\n" + + "}"; + + PutUserRequestBuilder requestBuilder = new PutUserRequestBuilder(mock(Client.class)); + PutUserRequest request = requestBuilder.source("hash_user", + new BytesArray(json.getBytes(StandardCharsets.UTF_8)), XContentType.JSON, hasher).request(); + assertThat(request.passwordHash(), equalTo(hash)); + assertThat(request.username(), equalTo("hash_user")); + } + + public void testWithMismatchedPasswordHash() throws IOException { + final Hasher systemHasher = Hasher.BCRYPT8; + final Hasher userHasher = Hasher.BCRYPT4; // this is the fastest hasher we officially support + final char[] hash = userHasher.hash(new SecureString("secret".toCharArray())); + final String json = "{\n" + + " \"password_hash\": \"" + new String(hash) + "\"," + + " \"roles\": []\n" + + "}"; + + PutUserRequestBuilder builder = new PutUserRequestBuilder(mock(Client.class)); + final IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> { + builder.source("hash_user", new BytesArray(json.getBytes(StandardCharsets.UTF_8)), XContentType.JSON, systemHasher).request(); + }); + assertThat(ex.getMessage(), containsString(userHasher.name())); + assertThat(ex.getMessage(), containsString(systemHasher.name())); + } + + public void testWithPasswordHashThatsNotReallyAHash() throws IOException { + final Hasher systemHasher = Hasher.PBKDF2; + final String json = "{\n" + + " \"password_hash\": \"not-a-hash\"," + + " \"roles\": []\n" + + "}"; + + PutUserRequestBuilder builder = new PutUserRequestBuilder(mock(Client.class)); + final IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> { + builder.source("hash_user", new BytesArray(json.getBytes(StandardCharsets.UTF_8)), XContentType.JSON, systemHasher).request(); + }); + assertThat(ex.getMessage(), containsString(Hasher.NOOP.name())); + assertThat(ex.getMessage(), containsString(systemHasher.name())); + } + + public void testWithBothPasswordAndHash() throws IOException { + final Hasher hasher = randomFrom(Hasher.BCRYPT4, Hasher.PBKDF2_1000); + final String password = randomAlphaOfLength(12); + final char[] hash = hasher.hash(new SecureString(password.toCharArray())); + final LinkedHashMap fields = new LinkedHashMap<>(); + fields.put("password", password); + fields.put("password_hash", new String(hash)); + fields.put("roles", Collections.emptyList()); + BytesReference json = BytesReference.bytes(XContentBuilder.builder(XContentType.JSON.xContent()) + .map(shuffleMap(fields, Collections.emptySet()))); + + PutUserRequestBuilder builder = new PutUserRequestBuilder(mock(Client.class)); + final IllegalArgumentException ex = expectThrows(ValidationException.class, () -> { + builder.source("hash_user", json, XContentType.JSON, hasher).request(); + }); + assertThat(ex.getMessage(), containsString("password_hash has already been set")); + } } diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/users/10_basic.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/users/10_basic.yml index ea152bd677cde..fd41df11ac4f8 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/users/10_basic.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/users/10_basic.yml @@ -13,6 +13,10 @@ teardown: xpack.security.delete_user: username: "joe" ignore: 404 + - do: + xpack.security.delete_user: + username: "bob" + ignore: 404 --- "Test put user api": @@ -101,3 +105,47 @@ teardown: "key2" : "val2" } } +--- +"Test put user with password hash": + + # Mostly this chain of put_user , search index, set value is to work around the fact that the + # rest tests treat anything with a leading "$" as a stashed value, and bcrypt passwords start with "$" + # But it has the nice side effect of automatically adjusting to any changes in the default hasher for + # the ES cluster + - do: + xpack.security.put_user: + username: "bob" + body: > + { + "password" : "correct horse battery staple", + "roles" : [ ] + } + + - do: + get: + index: .security + type: doc + id: user-bob + - set: { _source.password: "hash" } + + - do: + xpack.security.put_user: + username: "joe" + body: > + { + "password_hash" : "$hash", + "roles" : [ "superuser" ] + } + + # base64("joe:correct horse battery staple") = "am9lOmNvcnJlY3QgaG9yc2UgYmF0dGVyeSBzdGFwbGU=" + - do: + headers: + Authorization: "Basic am9lOmNvcnJlY3QgaG9yc2UgYmF0dGVyeSBzdGFwbGU=" + xpack.security.authenticate: {} + - match: { username: "joe" } + + - do: + catch: unauthorized + headers: + Authorization: "Basic am9lOnMza3JpdA==" + xpack.security.authenticate: {}