From a7eaa409e804f218aa06fd02d9166b9a5998b48a Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Mon, 2 Jul 2018 10:38:40 +0300 Subject: [PATCH] Fix TransportChangePasswordActionTests testIncorrectPasswordHashingAlgorithm is based on the assumption that the algorithm selected for the change password request is different than the one selected for the NativeUsersStore. pbkdf2_10000 is the same as pbkdf2 since 10000 is the default cost factor for pbkdf2 and thus should not be used as an option for the passwordHashingSettings. Also make sure that the same algorithm is used for settings and change password requests in other tests for consistency, even if we expect to not reach the code where the algorithm is checked for now. Resolves #31696 Reverts 1c4f480794f2465c78e8e29645956f16971eeead --- .../TransportChangePasswordActionTests.java | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportChangePasswordActionTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportChangePasswordActionTests.java index 33ed3fc5d97..e512ad4f23f 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportChangePasswordActionTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportChangePasswordActionTests.java @@ -47,18 +47,21 @@ import static org.mockito.Mockito.verifyZeroInteractions; public class TransportChangePasswordActionTests extends ESTestCase { public void testAnonymousUser() { + final String hashingAlgorithm = randomFrom("pbkdf2", "pbkdf2_1000", "bcrypt", "bcrypt9"); Settings settings = Settings.builder().put(AnonymousUser.ROLES_SETTING.getKey(), "superuser").build(); AnonymousUser anonymousUser = new AnonymousUser(settings); NativeUsersStore usersStore = mock(NativeUsersStore.class); - TransportService transportService = new TransportService(Settings.EMPTY, null, null, TransportService.NOOP_TRANSPORT_INTERCEPTOR, + Settings passwordHashingSettings = Settings.builder(). + put(XPackSettings.PASSWORD_HASHING_ALGORITHM.getKey(), hashingAlgorithm).build(); + TransportService transportService = new TransportService(passwordHashingSettings, null, null, TransportService.NOOP_TRANSPORT_INTERCEPTOR, x -> null, null, Collections.emptySet()); TransportChangePasswordAction action = new TransportChangePasswordAction(settings, transportService, mock(ActionFilters.class), usersStore); ChangePasswordRequest request = new ChangePasswordRequest(); + // Request will fail before the request hashing algorithm is checked, but we use the same algorithm as in settings for consistency request.username(anonymousUser.principal()); - request.passwordHash(Hasher.resolve( - randomFrom("pbkdf2", "pbkdf2_1000", "bcrypt", "bcrypt9")).hash(SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING)); + request.passwordHash(Hasher.resolve(hashingAlgorithm).hash(SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING)); final AtomicReference throwableRef = new AtomicReference<>(); final AtomicReference responseRef = new AtomicReference<>(); @@ -81,16 +84,19 @@ public class TransportChangePasswordActionTests extends ESTestCase { } public void testInternalUsers() { + final String hashingAlgorithm = randomFrom("pbkdf2", "pbkdf2_1000", "bcrypt", "bcrypt9"); NativeUsersStore usersStore = mock(NativeUsersStore.class); - TransportService transportService = new TransportService(Settings.EMPTY, null, null, TransportService.NOOP_TRANSPORT_INTERCEPTOR, + Settings passwordHashingSettings = Settings.builder(). + put(XPackSettings.PASSWORD_HASHING_ALGORITHM.getKey(), hashingAlgorithm).build(); + TransportService transportService = new TransportService(passwordHashingSettings, null, null, TransportService.NOOP_TRANSPORT_INTERCEPTOR, x -> null, null, Collections.emptySet()); TransportChangePasswordAction action = new TransportChangePasswordAction(Settings.EMPTY, transportService, mock(ActionFilters.class), usersStore); ChangePasswordRequest request = new ChangePasswordRequest(); request.username(randomFrom(SystemUser.INSTANCE.principal(), XPackUser.INSTANCE.principal())); - request.passwordHash(Hasher.resolve( - randomFrom("pbkdf2", "pbkdf2_1000", "bcrypt", "bcrypt9")).hash(SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING)); + // Request will fail before the request hashing algorithm is checked, but we use the same algorithm as in settings for consistency + request.passwordHash(Hasher.resolve(hashingAlgorithm).hash(SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING)); final AtomicReference throwableRef = new AtomicReference<>(); final AtomicReference responseRef = new AtomicReference<>(); @@ -153,7 +159,6 @@ public class TransportChangePasswordActionTests extends ESTestCase { verify(usersStore, times(1)).changePassword(eq(request), any(ActionListener.class)); } - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/31696") public void testIncorrectPasswordHashingAlgorithm() { final User user = randomFrom(new ElasticUser(true), new KibanaUser(true), new User("joe")); final Hasher hasher = Hasher.resolve(randomFrom("pbkdf2", "pbkdf2_1000", "bcrypt9", "bcrypt5")); @@ -166,7 +171,7 @@ public class TransportChangePasswordActionTests extends ESTestCase { TransportService transportService = new TransportService(Settings.EMPTY, null, null, TransportService.NOOP_TRANSPORT_INTERCEPTOR, x -> null, null, Collections.emptySet()); Settings passwordHashingSettings = Settings.builder().put(XPackSettings.PASSWORD_HASHING_ALGORITHM.getKey(), - randomFrom("pbkdf2_50000", "pbkdf2_10000", "bcrypt11", "bcrypt8", "bcrypt")).build(); + randomFrom("pbkdf2_50000", "pbkdf2_100000", "bcrypt11", "bcrypt8", "bcrypt")).build(); TransportChangePasswordAction action = new TransportChangePasswordAction(passwordHashingSettings, transportService, mock(ActionFilters.class), usersStore); action.doExecute(mock(Task.class), request, new ActionListener() {