diff --git a/gradle/changelog/always_encrypt_passwords.yaml b/gradle/changelog/always_encrypt_passwords.yaml new file mode 100644 index 0000000000..578ef3e437 --- /dev/null +++ b/gradle/changelog/always_encrypt_passwords.yaml @@ -0,0 +1,2 @@ +- type: fixed + description: Make sure, that passwords are always stored encrypted ([#2085](https://github.com/scm-manager/scm-manager/pull/2085)) diff --git a/scm-test/src/main/java/sonia/scm/user/UserManagerTestBase.java b/scm-test/src/main/java/sonia/scm/user/UserManagerTestBase.java index 1afc9cc443..29bbb18afa 100644 --- a/scm-test/src/main/java/sonia/scm/user/UserManagerTestBase.java +++ b/scm-test/src/main/java/sonia/scm/user/UserManagerTestBase.java @@ -166,7 +166,7 @@ public abstract class UserManagerTestBase extends ManagerTestBase { } assertNotNull(reference); - assertEquals(reference.getDisplayName(), "Tricia McMillan"); + assertEquals("Tricia McMillan", reference.getDisplayName()); } @Test @@ -181,7 +181,7 @@ public abstract class UserManagerTestBase extends ManagerTestBase { User otherUser = manager.get("zaphod"); assertNotNull(otherUser); - assertEquals(otherUser.getDisplayName(), "Tricia McMillan"); + assertEquals("Tricia McMillan", otherUser.getDisplayName()); } @Test(expected = NotFoundException.class) @@ -234,7 +234,7 @@ public abstract class UserManagerTestBase extends ManagerTestBase { assertNotNull(manager.get("zaphod")); zaphod.setDisplayName("Tricia McMillan"); manager.refresh(zaphod); - assertEquals(zaphod.getDisplayName(), "Zaphod Beeblebrox"); + assertEquals("Zaphod Beeblebrox", zaphod.getDisplayName()); } @Test(expected = NotFoundException.class) diff --git a/scm-webapp/src/main/java/sonia/scm/lifecycle/modules/ScmSecurityModule.java b/scm-webapp/src/main/java/sonia/scm/lifecycle/modules/ScmSecurityModule.java index e683460246..c0fa215a4a 100644 --- a/scm-webapp/src/main/java/sonia/scm/lifecycle/modules/ScmSecurityModule.java +++ b/scm-webapp/src/main/java/sonia/scm/lifecycle/modules/ScmSecurityModule.java @@ -148,9 +148,8 @@ public class ScmSecurityModule extends ShiroWebModule */ private PasswordService createPasswordService() { - DefaultPasswordService passwordService = new DefaultPasswordService(); + DefaultPasswordService passwordService = new IdempotentPasswordService(); DefaultHashService hashService = new DefaultHashService(); - hashService.setHashIterations(ITERATIONS); passwordService.setHashService(hashService); @@ -161,4 +160,19 @@ public class ScmSecurityModule extends ShiroWebModule /** Field description */ private final ExtensionProcessor extensionProcessor; + + static class IdempotentPasswordService extends DefaultPasswordService { + + private boolean isEncrypted(Object password) { + return password instanceof String && ((String) password).startsWith("$shiro1$SHA-512$"); + } + + @Override + public String encryptPassword(Object plaintext) { + if (isEncrypted(plaintext)) { + return plaintext.toString(); + } + return super.encryptPassword(plaintext); + } + } } diff --git a/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java b/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java index 8f21293fbe..a126beadf7 100644 --- a/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java +++ b/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java @@ -28,6 +28,7 @@ import com.github.sdorra.ssp.PermissionActionCheck; import com.google.inject.Inject; import com.google.inject.Singleton; import org.apache.shiro.SecurityUtils; +import org.apache.shiro.authc.credential.PasswordService; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import sonia.scm.ContextEntry; @@ -36,11 +37,9 @@ import sonia.scm.HandlerEventType; import sonia.scm.ManagerDaoAdapter; import sonia.scm.NotFoundException; import sonia.scm.SCMContextProvider; -import sonia.scm.TransformFilter; import sonia.scm.search.SearchRequest; import sonia.scm.search.SearchUtil; import sonia.scm.security.Authentications; -import sonia.scm.util.CollectionAppender; import sonia.scm.util.Util; import java.io.IOException; @@ -68,14 +67,18 @@ public class DefaultUserManager extends AbstractUserManager //~--- constructors --------------------------------------------------------- + private final PasswordService passwordService; + /** * Constructs ... * + * @param passwordService * @param userDAO */ @Inject - public DefaultUserManager(UserDAO userDAO) + public DefaultUserManager(PasswordService passwordService, UserDAO userDAO) { + this.passwordService = passwordService; this.userDAO = userDAO; this.managerDaoAdapter = new ManagerDaoAdapter<>(userDAO); } @@ -125,6 +128,7 @@ public class DefaultUserManager extends AbstractUserManager } logger.info("create user {} of type {}", user.getName(), user.getType()); + ensurePasswordEncrypted(user); return managerDaoAdapter.create( user, @@ -167,6 +171,7 @@ public class DefaultUserManager extends AbstractUserManager @Override public void modify(User user) { logger.info("modify user {} of type {}", user.getName(), user.getType()); + ensurePasswordEncrypted(user); managerDaoAdapter.modify( user, UserPermissions::modify, @@ -174,6 +179,10 @@ public class DefaultUserManager extends AbstractUserManager notModified -> fireEvent(HandlerEventType.MODIFY, user, notModified)); } + private void ensurePasswordEncrypted(User user) { + user.setPassword(passwordService.encryptPassword(user.getPassword())); + } + /** * Method description * @@ -364,7 +373,7 @@ public class DefaultUserManager extends AbstractUserManager throw new InvalidPasswordException(ContextEntry.ContextBuilder.entity("PasswordChange", "-").in(User.class, user.getName())); } - user.setPassword(newPassword); + user.setPassword(passwordService.encryptPassword(newPassword)); managerDaoAdapter.modify( user, diff --git a/scm-webapp/src/test/java/sonia/scm/lifecycle/modules/IdempotentPasswordServiceTest.java b/scm-webapp/src/test/java/sonia/scm/lifecycle/modules/IdempotentPasswordServiceTest.java new file mode 100644 index 0000000000..4c54ddc768 --- /dev/null +++ b/scm-webapp/src/test/java/sonia/scm/lifecycle/modules/IdempotentPasswordServiceTest.java @@ -0,0 +1,54 @@ +/* + * MIT License + * + * Copyright (c) 2020-present Cloudogu GmbH and Contributors + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +package sonia.scm.lifecycle.modules; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; +import sonia.scm.lifecycle.modules.ScmSecurityModule.IdempotentPasswordService; + +import static org.assertj.core.api.Assertions.assertThat; + +@ExtendWith(MockitoExtension.class) +class IdempotentPasswordServiceTest { + + @Test + void shouldEncryptCleartextPassword() { + IdempotentPasswordService passwordService = new IdempotentPasswordService(); + + String encryptedPassword = passwordService.encryptPassword("some simple password"); + + assertThat(encryptedPassword).startsWith("$shiro1$"); + } + + @Test + void shouldKeepAlreadyEncryptedPassword() { + IdempotentPasswordService passwordService = new IdempotentPasswordService(); + + String encryptedPassword = passwordService.encryptPassword("$shiro1$SHA-512$8192$XHkPE4rU53P/TsZNrAYdSw==$k5OehxvFr4C8rNk6pLYwtX9g5qbKKcsjFOwd0l29X3s="); + + assertThat(encryptedPassword).isEqualTo("$shiro1$SHA-512$8192$XHkPE4rU53P/TsZNrAYdSw==$k5OehxvFr4C8rNk6pLYwtX9g5qbKKcsjFOwd0l29X3s="); + } +} diff --git a/scm-webapp/src/test/java/sonia/scm/user/DefaultUserManagerTest.java b/scm-webapp/src/test/java/sonia/scm/user/DefaultUserManagerTest.java index 66c4f78f6d..535d004c2b 100644 --- a/scm-webapp/src/test/java/sonia/scm/user/DefaultUserManagerTest.java +++ b/scm-webapp/src/test/java/sonia/scm/user/DefaultUserManagerTest.java @@ -26,6 +26,7 @@ package sonia.scm.user; import com.github.sdorra.shiro.ShiroRule; import com.github.sdorra.shiro.SubjectAware; +import org.apache.shiro.authc.credential.PasswordService; import org.assertj.core.api.Assertions; import org.junit.Before; import org.junit.Rule; @@ -35,6 +36,9 @@ import sonia.scm.NotFoundException; import sonia.scm.store.JAXBConfigurationStoreFactory; import sonia.scm.user.xml.XmlUserDAO; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -52,52 +56,54 @@ public class DefaultUserManagerTest extends UserManagerTestBase { @Rule public ShiroRule shiro = new ShiroRule(); - private UserDAO userDAO; + private final UserDAO userDAO = mock(UserDAO.class); + private final ArgumentCaptor userCaptor = ArgumentCaptor.forClass(User.class); + private final PasswordService passwordService = mock(PasswordService.class); + + private UserManager userManager; - /** - * Method description - * - * @return - */ @Override public UserManager createManager() { - return new DefaultUserManager(createXmlUserDAO()); + return new DefaultUserManager(passwordService, createXmlUserDAO()); } @Before - public void initDao() { + public void initMocks() { User trillian = UserTestData.createTrillian(); trillian.setPassword("oldEncrypted"); - userDAO = mock(UserDAO.class); when(userDAO.getType()).thenReturn("xml"); when(userDAO.get("trillian")).thenReturn(trillian); + doNothing().when(userDAO).modify(userCaptor.capture()); + + when(passwordService.encryptPassword(anyString())).thenAnswer(invocation -> invocation.getArgument(0)); + + userManager = new DefaultUserManager(passwordService, userDAO); } @Test(expected = InvalidPasswordException.class) public void shouldFailChangePasswordForWrongOldPassword() { - UserManager userManager = new DefaultUserManager(userDAO); - userManager.changePasswordForLoggedInUser("wrongPassword", "$shiro1$secret"); } @Test public void shouldSucceedChangePassword() { - ArgumentCaptor userCaptor = ArgumentCaptor.forClass(User.class); - - doNothing().when(userDAO).modify(userCaptor.capture()); - - UserManager userManager = new DefaultUserManager(userDAO); - userManager.changePasswordForLoggedInUser("oldEncrypted", "newEncrypted"); Assertions.assertThat(userCaptor.getValue().getPassword()).isEqualTo("newEncrypted"); } + @Test + public void shouldEncryptChangedPassword() { + when(passwordService.encryptPassword("newPassword")).thenReturn("newEncrypted"); + + userManager.changePasswordForLoggedInUser("oldEncrypted", "newPassword"); + + Assertions.assertThat(userCaptor.getValue().getPassword()).isEqualTo("newEncrypted"); + } + @Test(expected = NotFoundException.class) public void shouldFailOverwritePasswordForMissingUser() { - UserManager userManager = new DefaultUserManager(userDAO); - userManager.overwritePassword("notExisting", "---"); } @@ -106,25 +112,53 @@ public class DefaultUserManagerTest extends UserManagerTestBase { User trillian = new User("trillian"); trillian.setExternal(true); when(userDAO.get("trillian")).thenReturn(trillian); - UserManager userManager = new DefaultUserManager(userDAO); userManager.overwritePassword("trillian", "---"); } @Test public void shouldSucceedOverwritePassword() { - ArgumentCaptor userCaptor = ArgumentCaptor.forClass(User.class); - - doNothing().when(userDAO).modify(userCaptor.capture()); - - UserManager userManager = new DefaultUserManager(userDAO); - userManager.overwritePassword("trillian", "newEncrypted"); Assertions.assertThat(userCaptor.getValue().getPassword()).isEqualTo("newEncrypted"); } - //~--- methods -------------------------------------------------------------- + @Test + public void shouldEncryptOverwrittenPassword() { + when(passwordService.encryptPassword("newPassword")).thenReturn("newEncrypted"); + + userManager.overwritePassword("trillian", "newPassword"); + + Assertions.assertThat(userCaptor.getValue().getPassword()).isEqualTo("newEncrypted"); + } + + @Test + public void shouldEncryptPasswordOnModify() { + User zaphod = UserTestData.createZaphod(); + when(passwordService.encryptPassword("password")).thenReturn("encrypted"); + + manager.create(zaphod); + zaphod.setPassword("password"); + manager.modify(zaphod); + + User otherUser = manager.get("zaphod"); + + assertNotNull(otherUser); + assertEquals("encrypted" , otherUser.getPassword()); + } + + @Test + public void shouldEncryptPasswordOnCreate() { + User zaphod = UserTestData.createZaphod(); + zaphod.setPassword("password"); + when(passwordService.encryptPassword("password")).thenReturn("encrypted"); + + manager.create(zaphod); + + User otherUser = manager.get("zaphod"); + + assertEquals("encrypted", otherUser.getPassword()); + } private XmlUserDAO createXmlUserDAO() { return new XmlUserDAO(new JAXBConfigurationStoreFactory(contextProvider, locationResolver, null));