diff --git a/core/src/main/java/google/registry/flows/session/LoginFlow.java b/core/src/main/java/google/registry/flows/session/LoginFlow.java index 369960b28..1d2d04e88 100644 --- a/core/src/main/java/google/registry/flows/session/LoginFlow.java +++ b/core/src/main/java/google/registry/flows/session/LoginFlow.java @@ -47,7 +47,6 @@ import google.registry.model.eppinput.EppInput.Options; import google.registry.model.eppinput.EppInput.Services; import google.registry.model.eppoutput.EppResponse; import google.registry.model.registrar.Registrar; -import google.registry.util.PasswordUtils.HashAlgorithm; import java.util.Optional; import java.util.Set; import javax.inject.Inject; @@ -142,17 +141,8 @@ public class LoginFlow implements MutatingFlow { throw new RegistrarAccountNotActiveException(); } - if (login.getNewPassword().isPresent() - || registrar.get().getCurrentHashAlgorithm(login.getPassword()).orElse(null) - != HashAlgorithm.SCRYPT) { - String newPassword = - login - .getNewPassword() - .orElseGet( - () -> { - logger.atInfo().log("Rehashing existing registrar password with Scrypt"); - return login.getPassword(); - }); + if (login.getNewPassword().isPresent()) { + String newPassword = login.getNewPassword().get(); // Load fresh from database (bypassing the cache) to ensure we don't save stale data. Optional freshRegistrar = Registrar.loadByRegistrarId(login.getClientId()); if (freshRegistrar.isEmpty()) { diff --git a/core/src/main/java/google/registry/model/console/User.java b/core/src/main/java/google/registry/model/console/User.java index 5c5fce604..efa72f176 100644 --- a/core/src/main/java/google/registry/model/console/User.java +++ b/core/src/main/java/google/registry/model/console/User.java @@ -86,8 +86,7 @@ public class User extends UpdateAutoTimestampEntity implements Buildable { return false; } return PasswordUtils.verifyPassword( - registryLockPassword, registryLockPasswordHash, registryLockPasswordSalt) - .isPresent(); + registryLockPassword, registryLockPasswordHash, registryLockPasswordSalt); } /** diff --git a/core/src/main/java/google/registry/model/registrar/Registrar.java b/core/src/main/java/google/registry/model/registrar/Registrar.java index 84ca2c17b..972e2bac4 100644 --- a/core/src/main/java/google/registry/model/registrar/Registrar.java +++ b/core/src/main/java/google/registry/model/registrar/Registrar.java @@ -62,7 +62,6 @@ import google.registry.model.tld.Tld.TldType; import google.registry.persistence.VKey; import google.registry.util.CidrAddressBlock; import google.registry.util.PasswordUtils; -import google.registry.util.PasswordUtils.HashAlgorithm; import java.security.cert.CertificateParsingException; import java.util.Comparator; import java.util.List; @@ -642,10 +641,6 @@ public class Registrar extends UpdateAutoTimestampEntity implements Buildable, J } public boolean verifyPassword(String password) { - return getCurrentHashAlgorithm(password).isPresent(); - } - - public Optional getCurrentHashAlgorithm(String password) { return PasswordUtils.verifyPassword(password, passwordHash, salt); } diff --git a/core/src/main/java/google/registry/model/registrar/RegistrarPoc.java b/core/src/main/java/google/registry/model/registrar/RegistrarPoc.java index f008d86b0..f67377283 100644 --- a/core/src/main/java/google/registry/model/registrar/RegistrarPoc.java +++ b/core/src/main/java/google/registry/model/registrar/RegistrarPoc.java @@ -38,7 +38,6 @@ import google.registry.model.UnsafeSerializable; import google.registry.model.registrar.RegistrarPoc.RegistrarPocId; import google.registry.persistence.VKey; import google.registry.util.PasswordUtils; -import google.registry.util.PasswordUtils.HashAlgorithm; import java.io.Serializable; import java.util.Map; import java.util.Optional; @@ -242,10 +241,6 @@ public class RegistrarPoc extends ImmutableObject implements Jsonifiable, Unsafe || isNullOrEmpty(registryLockPasswordHash)) { return false; } - return getCurrentHashAlgorithm(registryLockPassword).isPresent(); - } - - public Optional getCurrentHashAlgorithm(String registryLockPassword) { return PasswordUtils.verifyPassword( registryLockPassword, registryLockPasswordHash, registryLockPasswordSalt); } diff --git a/core/src/main/java/google/registry/ui/server/registrar/RegistryLockPostAction.java b/core/src/main/java/google/registry/ui/server/registrar/RegistryLockPostAction.java index beb4f6361..4012d056f 100644 --- a/core/src/main/java/google/registry/ui/server/registrar/RegistryLockPostAction.java +++ b/core/src/main/java/google/registry/ui/server/registrar/RegistryLockPostAction.java @@ -47,7 +47,6 @@ import google.registry.request.auth.UserAuthInfo; import google.registry.security.JsonResponseHelper; import google.registry.tools.DomainLockUtils; import google.registry.util.EmailMessage; -import google.registry.util.PasswordUtils.HashAlgorithm; import java.net.URISyntaxException; import java.util.Map; import java.util.Optional; @@ -223,19 +222,6 @@ public class RegistryLockPostAction implements Runnable, JsonActionRunner.JsonAc checkArgument( registrarPoc.verifyRegistryLockPassword(postInput.password), "Incorrect registry lock password for contact"); - if (registrarPoc.getCurrentHashAlgorithm(postInput.password).orElse(null) - != HashAlgorithm.SCRYPT) { - logger.atInfo().log("Rehashing existing registry lock password with Scrypt."); - tm().transact( - () -> { - tm().update( - tm().loadByEntity(registrarPoc) - .asBuilder() - .setAllowedToSetRegistryLockPassword(true) - .setRegistryLockPassword(postInput.password) - .build()); - }); - } return registrarPoc .getRegistryLockEmailAddress() .orElseThrow( diff --git a/core/src/test/java/google/registry/flows/session/LoginFlowTestCase.java b/core/src/test/java/google/registry/flows/session/LoginFlowTestCase.java index 2cb5c3df6..fbb338bcc 100644 --- a/core/src/test/java/google/registry/flows/session/LoginFlowTestCase.java +++ b/core/src/test/java/google/registry/flows/session/LoginFlowTestCase.java @@ -14,15 +14,11 @@ package google.registry.flows.session; -import static com.google.common.io.BaseEncoding.base64; import static com.google.common.truth.Truth.assertThat; -import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.testing.DatabaseHelper.deleteResource; import static google.registry.testing.DatabaseHelper.loadRegistrar; import static google.registry.testing.DatabaseHelper.persistResource; import static google.registry.testing.EppExceptionSubject.assertAboutEppExceptions; -import static google.registry.util.PasswordUtils.HashAlgorithm.SCRYPT; -import static google.registry.util.PasswordUtils.HashAlgorithm.SHA256; import static org.junit.jupiter.api.Assertions.assertThrows; import com.google.common.collect.ImmutableMap; @@ -42,7 +38,6 @@ import google.registry.model.eppoutput.EppOutput; import google.registry.model.registrar.Registrar; import google.registry.model.registrar.Registrar.State; import google.registry.testing.DatabaseHelper; -import google.registry.util.PasswordUtils; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -191,33 +186,6 @@ public abstract class LoginFlowTestCase extends FlowTestCase { doFailingTest("login_valid.xml", RegistrarAccountNotActiveException.class); } - @Test - void testSuccess_sha256Password() throws Exception { - String password = "foo-BAR2"; - tm().transact( - () -> { - // The salt is not exposed by Registrar (nor should it be), so we query it - // directly. - String encodedSalt = - tm().query("SELECT salt FROM Registrar WHERE registrarId = :id", String.class) - .setParameter("id", registrar.getRegistrarId()) - .getSingleResult(); - byte[] salt = base64().decode(encodedSalt); - String newHash = PasswordUtils.hashPassword(password, salt, SHA256); - // Set password directly, as the Java method would have used Scrypt. - tm().query("UPDATE Registrar SET passwordHash = :hash WHERE registrarId = :id") - .setParameter("id", registrar.getRegistrarId()) - .setParameter("hash", newHash) - .executeUpdate(); - }); - assertThat(loadRegistrar("NewRegistrar").getCurrentHashAlgorithm(password).get()) - .isEqualTo(SHA256); - doSuccessfulTest("login_valid.xml"); - // Verifies that after successfully login, the password is re-hased with Scrypt. - assertThat(loadRegistrar("NewRegistrar").getCurrentHashAlgorithm(password).get()) - .isEqualTo(SCRYPT); - } - @Test void testFailure_incorrectPassword() { persistResource(getRegistrarBuilder().setPassword("diff password").build()); diff --git a/core/src/test/java/google/registry/ui/server/registrar/RegistryLockPostActionTest.java b/core/src/test/java/google/registry/ui/server/registrar/RegistryLockPostActionTest.java index 429b798b6..65411647c 100644 --- a/core/src/test/java/google/registry/ui/server/registrar/RegistryLockPostActionTest.java +++ b/core/src/test/java/google/registry/ui/server/registrar/RegistryLockPostActionTest.java @@ -15,10 +15,8 @@ package google.registry.ui.server.registrar; import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap; -import static com.google.common.io.BaseEncoding.base64; import static com.google.common.truth.Truth.assertThat; import static google.registry.model.EppResourceUtils.loadByForeignKey; -import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.testing.DatabaseHelper.createTld; import static google.registry.testing.DatabaseHelper.loadRegistrar; import static google.registry.testing.DatabaseHelper.persistResource; @@ -27,8 +25,6 @@ import static google.registry.testing.SqlHelper.getRegistryLockByVerificationCod import static google.registry.testing.SqlHelper.saveRegistryLock; import static google.registry.tools.LockOrUnlockDomainCommand.REGISTRY_LOCK_STATUSES; import static google.registry.ui.server.registrar.RegistryLockGetActionTest.userFromRegistrarPoc; -import static google.registry.util.PasswordUtils.HashAlgorithm.SCRYPT; -import static google.registry.util.PasswordUtils.HashAlgorithm.SHA256; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; @@ -42,9 +38,6 @@ import google.registry.model.console.RegistrarRole; import google.registry.model.console.UserRoles; import google.registry.model.domain.Domain; import google.registry.model.domain.RegistryLock; -import google.registry.model.registrar.RegistrarPoc; -import google.registry.model.registrar.RegistrarPoc.RegistrarPocId; -import google.registry.persistence.VKey; import google.registry.persistence.transaction.JpaTestExtensions; import google.registry.persistence.transaction.JpaTestExtensions.JpaIntegrationTestExtension; import google.registry.persistence.transaction.JpaTransactionManagerExtension; @@ -61,7 +54,6 @@ import google.registry.testing.DeterministicStringGenerator; import google.registry.testing.FakeClock; import google.registry.tools.DomainLockUtils; import google.registry.util.EmailMessage; -import google.registry.util.PasswordUtils; import google.registry.util.StringGenerator.Alphabets; import java.util.Map; import java.util.Optional; @@ -131,48 +123,6 @@ final class RegistryLockPostActionTest { assertSuccess(response, "lock", "Marla.Singer.RegistryLock@crr.com"); } - @Test - void testSuccess_lock_sha256Password() throws Exception { - tm().transact( - () -> { - // The salt is not exposed by RegistrarPoc (nor should it be), so we query - // it directly. - String encodedSalt = - tm().query( - "SELECT registryLockPasswordSalt FROM RegistrarPoc " - + "WHERE emailAddress = :email " - + "AND registrarId = :registrarId", - String.class) - .setParameter("email", "Marla.Singer@crr.com") - .setParameter("registrarId", "TheRegistrar") - .getSingleResult(); - byte[] salt = base64().decode(encodedSalt); - String newHash = PasswordUtils.hashPassword("hi", salt, SHA256); - // Set password directly, as the Java method would have used Scrypt. - tm().query("UPDATE RegistrarPoc SET registryLockPasswordHash = :hash") - .setParameter("hash", newHash) - .executeUpdate(); - }); - RegistrarPoc registrarPoc = - tm().transact( - () -> - tm().loadByKey( - VKey.create( - RegistrarPoc.class, - new RegistrarPocId("Marla.Singer@crr.com", "TheRegistrar")))); - assertThat(registrarPoc.getCurrentHashAlgorithm("hi").get()).isEqualTo(SHA256); - Map response = action.handleJsonRequest(lockRequest()); - RegistrarPoc updatedRegistrarPoc = - tm().transact( - () -> - tm().loadByKey( - VKey.create( - RegistrarPoc.class, - new RegistrarPocId("Marla.Singer@crr.com", "TheRegistrar")))); - assertThat(updatedRegistrarPoc.getCurrentHashAlgorithm("hi").get()).isEqualTo(SCRYPT); - assertSuccess(response, "lock", "Marla.Singer.RegistryLock@crr.com"); - } - @Test void testSuccess_unlock() throws Exception { saveRegistryLock(createLock().asBuilder().setLockCompletionTime(clock.nowUtc()).build()); diff --git a/util/src/main/java/google/registry/util/PasswordUtils.java b/util/src/main/java/google/registry/util/PasswordUtils.java index 365aec9a5..af590a901 100644 --- a/util/src/main/java/google/registry/util/PasswordUtils.java +++ b/util/src/main/java/google/registry/util/PasswordUtils.java @@ -15,82 +15,29 @@ package google.registry.util; import static com.google.common.io.BaseEncoding.base64; -import static google.registry.util.PasswordUtils.HashAlgorithm.SCRYPT; -import static google.registry.util.PasswordUtils.HashAlgorithm.SHA256; import static java.nio.charset.StandardCharsets.US_ASCII; import com.google.common.base.Supplier; -import com.google.common.base.Suppliers; -import com.google.common.flogger.FluentLogger; import com.google.common.primitives.Bytes; -import java.security.MessageDigest; -import java.security.NoSuchAlgorithmException; import java.security.SecureRandom; import java.util.Arrays; -import java.util.Optional; import org.bouncycastle.crypto.generators.SCrypt; -/** Common utility class to handle password hashing and salting */ +/** + * Common utility class to handle password hashing and salting /* + * + *

We use a memory-hard hashing algorithm (Scrypt) to prevent brute-force attacks on passwords. + * + *

Note that in tests, we simply concatenate the password and salt which is much faster and + * reduces the overall test run time by a half. Our tests are not verifying that SCRYPT is + * implemented correctly anyway. + * + * @see Scrypt + */ public final class PasswordUtils { - private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - private static final Supplier SHA256_DIGEST_SUPPLIER = - Suppliers.memoize( - () -> { - try { - return MessageDigest.getInstance("SHA-256"); - } catch (NoSuchAlgorithmException e) { - // All implementations of MessageDigest are required to support SHA-256. - throw new RuntimeException( - "All MessageDigest implementations are required to support SHA-256 but this one" - + " didn't", - e); - } - }); - private PasswordUtils() {} - /** - * Password hashing algorithm that takes a password and a salt (both as {@code byte[]}) and - * returns a hash. - */ - public enum HashAlgorithm { - /** - * SHA-2 that returns a 256-bit digest. - * - * @see SHA-2 - */ - @Deprecated - SHA256 { - @Override - byte[] hash(byte[] password, byte[] salt) { - return SHA256_DIGEST_SUPPLIER - .get() - .digest((new String(password, US_ASCII) + base64().encode(salt)).getBytes(US_ASCII)); - } - }, - - /** - * Memory-hard hashing algorithm, preferred over SHA-256. - * - *

Note that in tests, we simply concatenate the password and salt which is much faster and - * reduces the overall test run time by a half. Our tests are not verifying that SCRYPT is - * implemented correctly anyway. - * - * @see Scrypt - */ - SCRYPT { - @Override - byte[] hash(byte[] password, byte[] salt) { - return RegistryEnvironment.get() == RegistryEnvironment.UNITTEST - ? Bytes.concat(password, salt) - : SCrypt.generate(password, salt, 32768, 8, 1, 256); - } - }; - - abstract byte[] hash(byte[] password, byte[] salt); - } - public static final Supplier SALT_SUPPLIER = () -> { // The generated hashes are 256 bits, and the salt should generally be of the same size. @@ -99,38 +46,25 @@ public final class PasswordUtils { return salt; }; - public static String hashPassword(String password, byte[] salt) { - return hashPassword(password, salt, SCRYPT); + private static byte[] hashPassword(byte[] password, byte[] salt) { + return RegistryEnvironment.get() == RegistryEnvironment.UNITTEST + ? Bytes.concat(password, salt) + : SCrypt.generate(password, salt, 32768, 8, 1, 256); } - /** Returns the hash of the password using the provided salt and {@link HashAlgorithm}. */ - public static String hashPassword(String password, byte[] salt, HashAlgorithm algorithm) { - return base64().encode(algorithm.hash(password.getBytes(US_ASCII), salt)); + /** Returns the hash of the password using the provided salt. */ + public static String hashPassword(String password, byte[] salt) { + return base64().encode(hashPassword(password.getBytes(US_ASCII), salt)); } /** * Verifies a password by regenerating the hash with the provided salt and comparing it to the * provided hash. - * - *

This method will first try to use {@link HashAlgorithm#SCRYPT} to verify the password, and - * falls back to {@link HashAlgorithm#SHA256} if the former fails. - * - * @return the {@link HashAlgorithm} used to successfully verify the password, or {@link - * Optional#empty()} if neither works. */ - public static Optional verifyPassword(String password, String hash, String salt) { + public static boolean verifyPassword(String password, String hash, String salt) { byte[] decodedHash = base64().decode(hash); byte[] decodedSalt = base64().decode(salt); - byte[] calculatedHash = SCRYPT.hash(password.getBytes(US_ASCII), decodedSalt); - if (Arrays.equals(decodedHash, calculatedHash)) { - logger.atInfo().log("Scrypt hash verified."); - return Optional.of(SCRYPT); - } - calculatedHash = SHA256.hash(password.getBytes(US_ASCII), decodedSalt); - if (Arrays.equals(decodedHash, calculatedHash)) { - logger.atInfo().log("SHA256 hash verified."); - return Optional.of(SHA256); - } - return Optional.empty(); + byte[] calculatedHash = hashPassword(password.getBytes(US_ASCII), decodedSalt); + return Arrays.equals(decodedHash, calculatedHash); } } diff --git a/util/src/test/java/google/registry/util/PasswordUtilsTest.java b/util/src/test/java/google/registry/util/PasswordUtilsTest.java index 80cad0f42..bc59db284 100644 --- a/util/src/test/java/google/registry/util/PasswordUtilsTest.java +++ b/util/src/test/java/google/registry/util/PasswordUtilsTest.java @@ -16,8 +16,6 @@ package google.registry.util; import static com.google.common.io.BaseEncoding.base64; import static com.google.common.truth.Truth.assertThat; -import static google.registry.util.PasswordUtils.HashAlgorithm.SCRYPT; -import static google.registry.util.PasswordUtils.HashAlgorithm.SHA256; import static google.registry.util.PasswordUtils.SALT_SUPPLIER; import static google.registry.util.PasswordUtils.hashPassword; import static google.registry.util.PasswordUtils.verifyPassword; @@ -53,18 +51,8 @@ final class PasswordUtilsTest { byte[] salt = SALT_SUPPLIER.get(); String password = "mySuperSecurePassword"; String hashedPassword = hashPassword(password, salt); - assertThat(hashedPassword).isEqualTo(hashPassword(password, salt, SCRYPT)); - assertThat(verifyPassword(password, hashedPassword, base64().encode(salt)).get()) - .isEqualTo(SCRYPT); - } - - @Test - void testVerify_sha256() { - byte[] salt = SALT_SUPPLIER.get(); - String password = "mySuperSecurePassword"; - String hashedPassword = hashPassword(password, salt, SHA256); - assertThat(verifyPassword(password, hashedPassword, base64().encode(salt)).get()) - .isEqualTo(SHA256); + assertThat(hashedPassword).isEqualTo(hashPassword(password, salt)); + assertThat(verifyPassword(password, hashedPassword, base64().encode(salt))).isTrue(); } @Test @@ -72,7 +60,6 @@ final class PasswordUtilsTest { byte[] salt = SALT_SUPPLIER.get(); String password = "mySuperSecurePassword"; String hashedPassword = hashPassword(password, salt); - assertThat(verifyPassword(password + "a", hashedPassword, base64().encode(salt)).isPresent()) - .isFalse(); + assertThat(verifyPassword(password + "a", hashedPassword, base64().encode(salt))).isFalse(); } }