From b1266c95e8d9f8206415d2821929d4161869b699 Mon Sep 17 00:00:00 2001 From: gbrodman Date: Mon, 17 Nov 2025 15:11:22 -0500 Subject: [PATCH] Add and default to Argon2 hashing (#2877) We've previously been using Scrypt since PR #2191 which, while being a memory-hard slow function, isn't the optimal solution according to the OWASP recommendations. While we could get away with increasing the parallelization parameter to 3, it's better to just switch to the most-recommended solution if we're switching things up anyway. For the transition, we do something similar to PR #2191 where if the previous-algorithm's hash is successful, we re-hash with Argo2id and store that version. By doing this, we should not need any intervention for registrars who log in at any point during the transition period. Much of this PR, especially the parts where we re-hash the passwords in Argon2 instead of Scrypt upon login, is based on the code that was eventually removed in #2310. --- .../registry/flows/session/LoginFlow.java | 16 ++- .../google/registry/model/console/User.java | 5 + .../registry/model/registrar/Registrar.java | 5 + .../console/ConsoleRegistryLockAction.java | 26 +++- .../flows/session/LoginFlowTestCase.java | 30 +++++ .../ConsoleRegistryLockActionTest.java | 59 +++++++-- .../google/registry/util/PasswordUtils.java | 112 ++++++++++++++++-- .../registry/util/PasswordUtilsTest.java | 20 +++- 8 files changed, 241 insertions(+), 32 deletions(-) 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 406c3d2b1..11d58c4a1 100644 --- a/core/src/main/java/google/registry/flows/session/LoginFlow.java +++ b/core/src/main/java/google/registry/flows/session/LoginFlow.java @@ -47,6 +47,7 @@ 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; import google.registry.util.StopwatchLogger; import jakarta.inject.Inject; import java.util.Optional; @@ -150,8 +151,19 @@ public class LoginFlow implements MutatingFlow { throw new RegistrarAccountNotActiveException(); } - if (login.getNewPassword().isPresent()) { - String newPassword = login.getNewPassword().get(); + // TODO(b/458423787): Remove this circa March 2026 after enough time has passed for the logins + // to have transitioned to Argon2 hashing. + if (login.getNewPassword().isPresent() + || registrar.get().getCurrentHashAlgorithm(login.getPassword()).orElse(null) + != PasswordUtils.HashAlgorithm.ARGON_2_ID) { + String newPassword = + login + .getNewPassword() + .orElseGet( + () -> { + logger.atInfo().log("Rehashing existing registrar password with ARGON_2_ID"); + return login.getPassword(); + }); // Load fresh from database (bypassing the cache) to ensure we don't save stale data. Optional freshRegistrar = Registrar.loadByRegistrarId(login.getClientId()); stopwatch.tick("LoginFlow reload freshRegistrar"); 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 67467a421..c85b29607 100644 --- a/core/src/main/java/google/registry/model/console/User.java +++ b/core/src/main/java/google/registry/model/console/User.java @@ -37,6 +37,7 @@ import google.registry.tools.IamClient; import google.registry.tools.ServiceConnection; import google.registry.tools.server.UpdateUserGroupAction; import google.registry.util.PasswordUtils; +import google.registry.util.PasswordUtils.HashAlgorithm; import google.registry.util.RegistryEnvironment; import jakarta.persistence.Column; import jakarta.persistence.Embeddable; @@ -229,6 +230,10 @@ public class User extends UpdateAutoTimestampEntity implements Buildable { || 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/model/registrar/Registrar.java b/core/src/main/java/google/registry/model/registrar/Registrar.java index ecfccef1a..59790fd35 100644 --- a/core/src/main/java/google/registry/model/registrar/Registrar.java +++ b/core/src/main/java/google/registry/model/registrar/Registrar.java @@ -67,6 +67,7 @@ import google.registry.persistence.converter.CurrencyToStringMapUserType; import google.registry.persistence.transaction.TransactionManager; import google.registry.util.CidrAddressBlock; import google.registry.util.PasswordUtils; +import google.registry.util.PasswordUtils.HashAlgorithm; import jakarta.mail.internet.AddressException; import jakarta.mail.internet.InternetAddress; import jakarta.persistence.AttributeOverride; @@ -672,6 +673,10 @@ 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/ui/server/console/ConsoleRegistryLockAction.java b/core/src/main/java/google/registry/ui/server/console/ConsoleRegistryLockAction.java index c2d59af6d..5c658f8ce 100644 --- a/core/src/main/java/google/registry/ui/server/console/ConsoleRegistryLockAction.java +++ b/core/src/main/java/google/registry/ui/server/console/ConsoleRegistryLockAction.java @@ -23,6 +23,7 @@ import static jakarta.servlet.http.HttpServletResponse.SC_OK; import static jakarta.servlet.http.HttpServletResponse.SC_UNAUTHORIZED; import com.google.common.collect.ImmutableList; +import com.google.common.flogger.FluentLogger; import com.google.gson.annotations.Expose; import google.registry.flows.EppException; import google.registry.flows.domain.DomainFlowUtils; @@ -40,6 +41,7 @@ import google.registry.request.Response; import google.registry.request.auth.Auth; import google.registry.tools.DomainLockUtils; import google.registry.util.EmailMessage; +import google.registry.util.PasswordUtils.HashAlgorithm; import jakarta.inject.Inject; import jakarta.mail.internet.AddressException; import jakarta.mail.internet.InternetAddress; @@ -61,13 +63,16 @@ import org.joda.time.Duration; auth = Auth.AUTH_PUBLIC_LOGGED_IN) public class ConsoleRegistryLockAction extends ConsoleApiAction { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + static final String PATH = "/console-api/registry-lock"; static final String VERIFICATION_EMAIL_TEMPLATE = """ Please click the link below to perform the lock / unlock action on domain %s. Note: this\ code will expire in one hour. - %s"""; + %s\ + """; private final DomainLockUtils domainLockUtils; private final GmailClient gmailClient; @@ -114,7 +119,6 @@ public class ConsoleRegistryLockAction extends ConsoleApiAction { optionalPostInput.orElseThrow(() -> new IllegalArgumentException("No POST input provided")); String domainName = postInput.domainName(); boolean isLock = postInput.isLock(); - Optional maybePassword = Optional.ofNullable(postInput.password()); Optional relockDurationMillis = Optional.ofNullable(postInput.relockDurationMillis()); try { @@ -126,11 +130,25 @@ public class ConsoleRegistryLockAction extends ConsoleApiAction { // Passwords aren't required for admin users, otherwise we need to validate it boolean isAdmin = user.getUserRoles().isAdmin(); if (!isAdmin) { - checkArgument(maybePassword.isPresent(), "No password provided"); - if (!user.verifyRegistryLockPassword(maybePassword.get())) { + checkArgument(postInput.password != null, "No password provided"); + Optional hashAlgorithm = user.getCurrentHashAlgorithm(postInput.password); + if (hashAlgorithm.isEmpty()) { setFailedResponse("Incorrect registry lock password", SC_UNAUTHORIZED); return; } + // TODO(b/458423787): Remove this circa March 2026 after enough time has passed for the logins + // to have transitioned to Argon2 hashing. + if (hashAlgorithm.get() != HashAlgorithm.ARGON_2_ID) { + logger.atInfo().log("Rehashing existing registry lock password with ARGON_2_ID."); + tm().transact( + () -> + tm().update( + tm().loadByEntity(user) + .asBuilder() + .removeRegistryLockPassword() + .setRegistryLockPassword(postInput.password) + .build())); + } } String registryLockEmail = 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 fbb338bcc..772b43bff 100644 --- a/core/src/test/java/google/registry/flows/session/LoginFlowTestCase.java +++ b/core/src/test/java/google/registry/flows/session/LoginFlowTestCase.java @@ -14,7 +14,9 @@ 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; @@ -38,6 +40,8 @@ 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 google.registry.util.PasswordUtils.HashAlgorithm; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -186,6 +190,32 @@ public abstract class LoginFlowTestCase extends FlowTestCase { doFailingTest("login_valid.xml", RegistrarAccountNotActiveException.class); } + @Test + void testSuccess_scryptPasswordToArgon2() 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, HashAlgorithm.SCRYPT_P_1); + // Set password directly, as the Java method would have used Argon2. + 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(HashAlgorithm.SCRYPT_P_1); + doSuccessfulTest("login_valid.xml"); + // Verifies that after successfully login, the password is re-hased with Scrypt. + assertThat(loadRegistrar("NewRegistrar").getCurrentHashAlgorithm(password).get()) + .isEqualTo(HashAlgorithm.ARGON_2_ID); + } + @Test void testFailure_incorrectPassword() { persistResource(getRegistrarBuilder().setPassword("diff password").build()); diff --git a/core/src/test/java/google/registry/ui/server/console/ConsoleRegistryLockActionTest.java b/core/src/test/java/google/registry/ui/server/console/ConsoleRegistryLockActionTest.java index f6a75b746..b9fd58eae 100644 --- a/core/src/test/java/google/registry/ui/server/console/ConsoleRegistryLockActionTest.java +++ b/core/src/test/java/google/registry/ui/server/console/ConsoleRegistryLockActionTest.java @@ -14,7 +14,9 @@ package google.registry.ui.server.console; +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.createTld; import static google.registry.testing.DatabaseHelper.loadByEntity; import static google.registry.testing.DatabaseHelper.loadRegistrar; @@ -49,6 +51,8 @@ import google.registry.testing.DeterministicStringGenerator; import google.registry.testing.FakeResponse; import google.registry.tools.DomainLockUtils; import google.registry.util.EmailMessage; +import google.registry.util.PasswordUtils; +import google.registry.util.PasswordUtils.HashAlgorithm; import google.registry.util.StringGenerator; import jakarta.mail.internet.InternetAddress; import java.io.IOException; @@ -88,16 +92,17 @@ public class ConsoleRegistryLockActionTest extends ConsoleActionBaseTestCase { createTld("test"); defaultDomain = persistActiveDomain("example.test"); user = - new User.Builder() - .setEmailAddress("user@theregistrar.com") - .setRegistryLockEmailAddress("registrylock@theregistrar.com") - .setUserRoles( - new UserRoles.Builder() - .setRegistrarRoles( - ImmutableMap.of("TheRegistrar", RegistrarRole.PRIMARY_CONTACT)) - .build()) - .setRegistryLockPassword("registryLockPassword") - .build(); + persistResource( + new User.Builder() + .setEmailAddress("user@theregistrar.com") + .setRegistryLockEmailAddress("registrylock@theregistrar.com") + .setUserRoles( + new UserRoles.Builder() + .setRegistrarRoles( + ImmutableMap.of("TheRegistrar", RegistrarRole.PRIMARY_CONTACT)) + .build()) + .setRegistryLockPassword("registryLockPassword") + .build()); action = createGetAction(); } @@ -277,6 +282,40 @@ public class ConsoleRegistryLockActionTest extends ConsoleActionBaseTestCase { assertThat(loadByEntity(defaultDomain).getStatusValues()).containsExactly(StatusValue.INACTIVE); } + @Test + void testPost_lock_scryptPasswordToArgon2() throws Exception { + tm().transact( + () -> { + // The salt is not exposed by User (nor should it be), so we query it directly. + String encodedSalt = + tm().query( + "SELECT registryLockPasswordSalt FROM User WHERE emailAddress =" + + " :emailAddress", + String.class) + .setParameter("emailAddress", user.getEmailAddress()) + .getSingleResult(); + byte[] salt = base64().decode(encodedSalt); + String newHash = + PasswordUtils.hashPassword( + "registryLockPassword", salt, HashAlgorithm.SCRYPT_P_1); + // Set password directly, as the Java method would have used Argon2. + tm().query("UPDATE User SET registryLockPasswordHash = :hash") + .setParameter("hash", newHash) + .executeUpdate(); + }); + user = loadByEntity(user); + assertThat(user.getCurrentHashAlgorithm("registryLockPassword").get()) + .isEqualTo(HashAlgorithm.SCRYPT_P_1); + action = createDefaultPostAction(true); + action.run(); + verifyEmail(); + assertThat(response.getStatus()).isEqualTo(SC_OK); + assertThat(getMostRecentRegistryLockByRepoId(defaultDomain.getRepoId())).isPresent(); + user = loadByEntity(user); + assertThat(user.getCurrentHashAlgorithm("registryLockPassword").get()) + .isEqualTo(HashAlgorithm.ARGON_2_ID); + } + @Test void testPost_unlock() throws Exception { saveRegistryLock(createDefaultLockBuilder().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 af590a901..57cf06162 100644 --- a/util/src/main/java/google/registry/util/PasswordUtils.java +++ b/util/src/main/java/google/registry/util/PasswordUtils.java @@ -15,28 +15,40 @@ package google.registry.util; import static com.google.common.io.BaseEncoding.base64; +import static google.registry.util.PasswordUtils.HashAlgorithm.ARGON_2_ID; +import static google.registry.util.PasswordUtils.HashAlgorithm.SCRYPT_P_1; import static java.nio.charset.StandardCharsets.US_ASCII; import com.google.common.base.Supplier; +import com.google.common.flogger.FluentLogger; import com.google.common.primitives.Bytes; import java.security.SecureRandom; import java.util.Arrays; +import java.util.Optional; +import org.bouncycastle.crypto.generators.Argon2BytesGenerator; import org.bouncycastle.crypto.generators.SCrypt; +import org.bouncycastle.crypto.params.Argon2Parameters; /** - * 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. + *

We use a memory-hard hashing algorithm (Argon2id) 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 + * reduces the overall test run time by a half. Our tests are not verifying that Argon2 is * implemented correctly anyway. * * @see Scrypt + * @see Argon2 */ public final class PasswordUtils { - private PasswordUtils() {} + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + // Argon2 requires an "init" call on the generator each time the salt is changed. As a result, + // each thread needs its own generator, then we can init it with the new salt every time a new + // hash is required. + private static final ThreadLocal argon2BytesGenerator = + ThreadLocal.withInitial(Argon2BytesGenerator::new); public static final Supplier SALT_SUPPLIER = () -> { @@ -46,25 +58,101 @@ public final class PasswordUtils { return salt; }; - 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); + public enum HashAlgorithm { + /** + * SCrypt algorithm using a parallelization factor of 1. + * + *

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. + * + *

TODO(b/458423787): Remove this circa March 2026 after enough time has passed for the + * logins to have transitioned to Argon2 hashing. + * + * @see Scrypt + */ + @Deprecated + SCRYPT_P_1 { + @Override + byte[] hash(byte[] password, byte[] salt) { + return RegistryEnvironment.get() == RegistryEnvironment.UNITTEST + ? Bytes.concat(password, salt) + : SCrypt.generate(password, salt, 32768, 8, 1, 256); + } + }, + + /** + * Argon2id algorithm using 3 iterations and 12 MiB. + * + *

Note that in tests, we simply concatenate the salt and password (the inverse order from + * Scrypt) which is much faster and reduces the overall test run time by a half. Our tests are + * not verifying that Argon2 is implemented correctly anyway. + * + * @see Argon2 + * @see OWASP + * cheat sheet + */ + ARGON_2_ID { + @Override + byte[] hash(byte[] password, byte[] salt) { + if (RegistryEnvironment.get() == RegistryEnvironment.UNITTEST) { + return Bytes.concat(salt, password); + } + Argon2BytesGenerator generator = argon2BytesGenerator.get(); + // For Argon2, the salt is part of the parameters so we must reinitialize each time + generator.init( + new Argon2Parameters.Builder(Argon2Parameters.ARGON2_id) + .withVersion(Argon2Parameters.ARGON2_VERSION_13) + .withIterations(3) + .withMemoryAsKB(12288) + .withParallelism(1) + .withSalt(salt) + .build()); + byte[] result = new byte[256]; + generator.generateBytes(password, result); + return result; + } + }; + + abstract byte[] hash(byte[] password, byte[] 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)); + return hashPassword(password, salt, ARGON_2_ID); + } + + /** 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)); } /** * 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#ARGON_2_ID} to verify the password, + * and falls back to {@link HashAlgorithm#SCRYPT_P_1} if the former fails. + * + * @return the {@link HashAlgorithm} used to successfully verify the password, or {@link + * Optional#empty()} if neither works. */ - public static boolean verifyPassword(String password, String hash, String salt) { + public static Optional verifyPassword(String password, String hash, String salt) { byte[] decodedHash = base64().decode(hash); byte[] decodedSalt = base64().decode(salt); - byte[] calculatedHash = hashPassword(password.getBytes(US_ASCII), decodedSalt); - return Arrays.equals(decodedHash, calculatedHash); + byte[] decodedPassword = password.getBytes(US_ASCII); + + if (Arrays.equals(decodedHash, ARGON_2_ID.hash(decodedPassword, decodedSalt))) { + logger.atInfo().log("ARGON_2_ID hash verified."); + return Optional.of(ARGON_2_ID); + } + if (Arrays.equals(decodedHash, SCRYPT_P_1.hash(decodedPassword, decodedSalt))) { + logger.atInfo().log("SCRYPT_P_1 hash verified."); + return Optional.of(SCRYPT_P_1); + } + return Optional.empty(); } + + private PasswordUtils() {} } diff --git a/util/src/test/java/google/registry/util/PasswordUtilsTest.java b/util/src/test/java/google/registry/util/PasswordUtilsTest.java index bc59db284..d885db0e1 100644 --- a/util/src/test/java/google/registry/util/PasswordUtilsTest.java +++ b/util/src/test/java/google/registry/util/PasswordUtilsTest.java @@ -16,6 +16,8 @@ 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.ARGON_2_ID; +import static google.registry.util.PasswordUtils.HashAlgorithm.SCRYPT_P_1; import static google.registry.util.PasswordUtils.SALT_SUPPLIER; import static google.registry.util.PasswordUtils.hashPassword; import static google.registry.util.PasswordUtils.verifyPassword; @@ -47,12 +49,22 @@ final class PasswordUtilsTest { } @Test - void testVerify_scrypt_default() { + void testVerify_argon2_default() { byte[] salt = SALT_SUPPLIER.get(); String password = "mySuperSecurePassword"; String hashedPassword = hashPassword(password, salt); - assertThat(hashedPassword).isEqualTo(hashPassword(password, salt)); - assertThat(verifyPassword(password, hashedPassword, base64().encode(salt))).isTrue(); + assertThat(hashedPassword).isEqualTo(hashPassword(password, salt, ARGON_2_ID)); + assertThat(verifyPassword(password, hashedPassword, base64().encode(salt))) + .hasValue(ARGON_2_ID); + } + + @Test + void testVerify_scrypt() { + byte[] salt = SALT_SUPPLIER.get(); + String password = "mySuperSecurePassword"; + String hashedPassword = hashPassword(password, salt, SCRYPT_P_1); + assertThat(verifyPassword(password, hashedPassword, base64().encode(salt))) + .hasValue(SCRYPT_P_1); } @Test @@ -60,6 +72,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))).isFalse(); + assertThat(verifyPassword(password + "a", hashedPassword, base64().encode(salt))).isEmpty(); } }