From 2f5d28b8345737b200818ff3ee1d83f04d2807fa Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Fri, 2 Nov 2018 17:38:29 +1100 Subject: [PATCH 1/2] Formal support for "password_user" in Put User For some time, the PutUser REST API has supported storing a pre-hashed password for a user. The change adds validation and tests around that feature so that it can be documented & officially supported. --- .../action/user/PutUserRequestBuilder.java | 10 ++-- .../user/PutUserRequestBuilderTests.java | 49 +++++++++++++++++++ .../rest-api-spec/test/users/10_basic.yml | 48 ++++++++++++++++++ 3 files changed, 104 insertions(+), 3 deletions(-) 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..20fccc4b14d2e 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 @@ -80,7 +80,12 @@ 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 + "]"); + } request.passwordHash(passwordHash); return this; } @@ -120,7 +125,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); @@ -176,5 +181,4 @@ public PutUserRequestBuilder source(String username, BytesReference source, XCon return this; } } - } 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..3e80feeb47c84 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 @@ -8,6 +8,7 @@ import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.client.Client; import org.elasticsearch.common.bytes.BytesArray; +import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.core.security.action.user.PutUserRequest; @@ -19,6 +20,7 @@ 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 +137,51 @@ 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())); + } } 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: {} From 3f91fdc459332a7e54ebc992bd292c482f4d3514 Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Mon, 12 Nov 2018 13:15:47 +1100 Subject: [PATCH 2/2] Prevent setting password and password_hash --- .../action/user/PutUserRequestBuilder.java | 16 ++++++++++--- .../user/PutUserRequestBuilderTests.java | 23 +++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) 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 20fccc4b14d2e..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 { @@ -86,6 +87,9 @@ public PutUserRequestBuilder passwordHash(char[] passwordHash, Hasher configured 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; } @@ -181,4 +185,10 @@ public PutUserRequestBuilder source(String username, BytesReference source, XCon return this; } } + + 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 3e80feeb47c84..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,8 +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; @@ -17,6 +20,8 @@ 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; @@ -184,4 +189,22 @@ public void testWithPasswordHashThatsNotReallyAHash() throws IOException { 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")); + } }