From 2b87aed26fc371e59138cef92d11488c76ff511b Mon Sep 17 00:00:00 2001 From: Sujith Vadakkepat Date: Tue, 31 Mar 2020 15:12:12 -0700 Subject: [PATCH 1/2] Fix updating password hash without replacing the entry (#333) (cherry-picked from commit a09fcc47f1b0679d3fe0ec6fdb9adc8be5a073c2) --- .../dlic/rest/api/AccountApiAction.java | 34 +++++------ .../dlic/rest/api/AccountApiTest.java | 59 +++++++++++++++++++ 2 files changed, 75 insertions(+), 18 deletions(-) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/AccountApiAction.java b/src/main/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/AccountApiAction.java index f36ef5618..9c511c132 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/AccountApiAction.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/AccountApiAction.java @@ -32,6 +32,7 @@ import org.elasticsearch.action.index.IndexResponse; import org.elasticsearch.client.Client; import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.settings.Settings; @@ -200,32 +201,29 @@ protected void handlePut(RestChannel channel, final RestRequest request, final C return; } - final Settings settings = indexBaseConfigurationRepository.getConfiguration(ConfigConstants.CONFIGNAME_INTERNAL_USERS); final String currentPassword = additionalSettingsBuilder.get("current_password"); - final String hash = settings.get(username + ".hash"); - if (hash == null || !OpenBSDBCrypt.checkPassword(hash, currentPassword.toCharArray())) { + @SuppressWarnings("unchecked") + Map internalUserEntry = (Map) config.get(username); + final String currentHash = internalUserEntry.get("hash"); + if (currentHash == null || !OpenBSDBCrypt.checkPassword(currentHash, currentPassword.toCharArray())) { badRequestResponse(channel, "Could not validate your current password."); return; } - additionalSettingsBuilder.remove("current_password"); - // if password is set, it takes precedence over hash - final String plainTextPassword = additionalSettingsBuilder.get("password"); - final String origHash = additionalSettingsBuilder.get("hash"); - if (plainTextPassword != null && plainTextPassword.length() > 0) { - additionalSettingsBuilder.remove("password"); - additionalSettingsBuilder.put("hash", hash(plainTextPassword.toCharArray())); - } else if (origHash != null && origHash.length() > 0) { - additionalSettingsBuilder.remove("password"); - } else if (plainTextPassword != null && plainTextPassword.isEmpty() && origHash == null) { - additionalSettingsBuilder.remove("password"); + final String password = additionalSettingsBuilder.get("password"); + final String hash; + if (Strings.isNullOrEmpty(password)) { + hash = additionalSettingsBuilder.get("hash"); + } else { + hash = hash(password.toCharArray()); + } + if (Strings.isNullOrEmpty(hash)) { + badRequestResponse(channel, "Both provided password and hash cannot be null/empty."); + return; } - config.remove(username); - - // checks complete, create or update the user - config.put(username, Utils.convertJsonToxToStructuredMap(additionalSettingsBuilder.build())); + internalUserEntry.put("hash", hash); saveAnUpdateConfigs(client, request, ConfigConstants.CONFIGNAME_INTERNAL_USERS, Utils.convertStructuredMapToBytes(config), new OnSucessActionListener(channel) { @Override diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/AccountApiTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/AccountApiTest.java index 036b422e2..7a865a4d9 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/AccountApiTest.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/AccountApiTest.java @@ -94,6 +94,26 @@ public void testPutAccount() throws Exception { response = rh.executePutRequest(ENDPOINT, payload, encodeBasicHeader(testUser, testPass)); assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + // test - bad request as hash/password is missing + payload = "{\"current_password\":\"" + testPass + "\"}"; + response = rh.executePutRequest(ENDPOINT, payload, encodeBasicHeader(testUser, testPass)); + assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + + // test - bad request as password is empty + payload = "{\"password\":\"" + "" + "\", \"current_password\":\"" + testPass + "\"}"; + response = rh.executePutRequest(ENDPOINT, payload, encodeBasicHeader(testUser, testPass)); + assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + + // test - bad request as hash is empty + payload = "{\"hash\":\"" + "" + "\", \"current_password\":\"" + testPass + "\"}"; + response = rh.executePutRequest(ENDPOINT, payload, encodeBasicHeader(testUser, testPass)); + assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + + // test - bad request as hash and password are empty + payload = "{\"hash\": \"\", \"password\": \"\", \"current_password\":\"" + testPass + "\"}"; + response = rh.executePutRequest(ENDPOINT, payload, encodeBasicHeader(testUser, testPass)); + assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + // test - bad request as invalid parameters are present payload = "{\"password\":\"new-pass\", \"current_password\":\"" + testPass + "\", \"backend_roles\": []}"; response = rh.executePutRequest(ENDPOINT, payload, encodeBasicHeader(testUser, testPass)); @@ -152,4 +172,43 @@ public void testPutAccount() throws Exception { response = rh.executePutRequest(ENDPOINT, payload, encodeBasicHeader("admin", "admin")); assertEquals(HttpStatus.SC_NOT_FOUND, response.getStatusCode()); } + + @Test + public void testPutAccountRetainsAccountInformation() throws Exception { + // arrange + setup(); + final String testUsername = "test"; + final String testPassword = "test-password"; + final String newPassword = "new-password"; + final String createInternalUserPayload = "{\n" + + " \"password\": \"" + testPassword + "\",\n" + + " \"roles\": [\"test-backend-role-1\"],\n" + + " \"attributes\": {\n" + + " \"attribute1\": \"value1\"\n" + + " }\n" + + "}"; + final String changePasswordPayload = "{\"password\":\"" + newPassword + "\", \"current_password\":\"" + testPassword + "\"}"; + final String internalUserEndpoint = "/_opendistro/_security/api/internalusers/" + testUsername; + + // create user + rh.sendAdminCertificate = true; + HttpResponse response = rh.executePutRequest(internalUserEndpoint, createInternalUserPayload); + assertEquals(HttpStatus.SC_OK, response.getStatusCode()); + rh.sendAdminCertificate = false; + + // change password to new-password + response = rh.executePutRequest(ENDPOINT, changePasswordPayload, encodeBasicHeader(testUsername, testPassword)); + assertEquals(HttpStatus.SC_OK, response.getStatusCode()); + + // assert account information has not changed + rh.sendAdminCertificate = true; + response = rh.executeGetRequest(internalUserEndpoint); + assertEquals(HttpStatus.SC_OK, response.getStatusCode()); + Settings responseBody = Settings.builder() + .loadFromSource(response.getBody(), XContentType.JSON) + .build() + .getAsSettings(testUsername); + assertTrue(responseBody.getAsList("roles").contains("test-backend-role-1")); + assertEquals(responseBody.getAsSettings("attributes").get("attribute1"), "value1"); + } } From 0a4ee27806e10f8f032a18351c642a30009ae5df Mon Sep 17 00:00:00 2001 From: Sujith Vadakkepat Date: Tue, 31 Mar 2020 15:18:43 -0700 Subject: [PATCH 2/2] Version bump 0.10.1.2 --- build.gradle | 2 +- opendistro-elasticsearch-security.release-notes | 8 +++++++- plugin-descriptor.properties | 2 +- pom.xml | 4 ++-- 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/build.gradle b/build.gradle index ab167ca57..513537a62 100644 --- a/build.gradle +++ b/build.gradle @@ -14,7 +14,7 @@ ext { group = "com.amazon.opendistroforelasticsearch" // Increment the final digit when there's a new plugin versions for the same opendistro version // Reset the final digit to 0 when upgrading to a new opendistro version -version = "${opendistroVersion}.1" + (isSnapshot ? "-SNAPSHOT" : "") +version = "${opendistroVersion}.2" + (isSnapshot ? "-SNAPSHOT" : "") if (!project.hasProperty("archivePath")) { diff --git a/opendistro-elasticsearch-security.release-notes b/opendistro-elasticsearch-security.release-notes index 730b16c85..22ea7cd9e 100644 --- a/opendistro-elasticsearch-security.release-notes +++ b/opendistro-elasticsearch-security.release-notes @@ -1,4 +1,10 @@ -## 2020-03-05, Version 0.10.1.1 (Current) +## 2020-04-02, Version 0.10.1.2 (Current) + +- Fixed bug which caused user to lose roles on account password update +- Performance improvement by memorizing results of resolveIndexPatterns for Bulk requests +- Performance improvement by implementing faster version of implies type perm + +## 2020-03-05, Version 0.10.1.1 - Adding capability to hot reload ssl certificates - Added changes for SuperAdmin to update/add/delete reserved configs diff --git a/plugin-descriptor.properties b/plugin-descriptor.properties index 3b9d75279..5c42b15fd 100644 --- a/plugin-descriptor.properties +++ b/plugin-descriptor.properties @@ -3,7 +3,7 @@ description=Provide access control related features for Elasticsearch 6 # # 'version': plugin's version -version=0.10.1.1 +version=0.10.1.2 # # 'name': the plugin name name=opendistro_security diff --git a/pom.xml b/pom.xml index 46aef2ac0..9631a1851 100644 --- a/pom.xml +++ b/pom.xml @@ -35,7 +35,7 @@ com.amazon.opendistroforelasticsearch opendistro_security jar - 0.10.1.1 + 0.10.1.2 Open Distro Security for Elasticsearch Open Distro For Elasticsearch Security https://github.com/opendistro-for-elasticsearch/security @@ -52,7 +52,7 @@ https://github.com/opendistro-for-elasticsearch/security scm:git:git@github.com:opendistro-for-elasticsearch/security.git scm:git:git@github.com:opendistro-for-elasticsearch/security.git - 0.10.1.1 + 0.10.1.2