Formal support for "password_hash" in Put User (#35242)

tvernum · web-flow · commit 231f6c1595a2 · 2018-11-14T16:46:31.000+11:00
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 &amp; officially supported.

It also prevents the request from containing both a "password" and a "password_hash".
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
@@ -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
@@ -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<String, Object> 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
@@ -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: {}
