Remove ability to set only the certificate hash for a registrar (#891)

core/src/main/java/google/registry/model/OteAccountBuilder.java

@@ -211,11 +211,6 @@ public final class OteAccountBuilder {
     return transformRegistrars(builder -> builder.setPassword(password));
   }
 
-  /** Sets the client certificate hash to all the OT&E Registrars. */
-  public OteAccountBuilder setCertificateHash(String certHash) {
-    return transformRegistrars(builder -> builder.setClientCertificateHash(certHash));
-  }
-
   /** Sets the client certificate to all the OT&E Registrars. */
   public OteAccountBuilder setCertificate(String asciiCert, DateTime now) {
     return transformRegistrars(builder -> builder.setClientCertificate(asciiCert, now));

core/src/main/java/google/registry/model/registrar/Registrar.java

@@ -856,26 +856,6 @@ public class Registrar extends ImmutableObject
       }
     }
 
-    /**
-     * Sets client certificate hash, but not the certificate.
-     *
-     * <p><b>Warning:</b> {@link #setClientCertificate(String, DateTime)} sets the hash for you and
-     * is preferred. Calling this method will nullify the {@code clientCertificate} field.
-     */
-    public Builder setClientCertificateHash(String clientCertificateHash) {
-      if (clientCertificateHash != null) {
-        checkArgument(
-            Pattern.matches("[A-Za-z0-9+/]+", clientCertificateHash),
-            "--cert_hash not a valid base64 (no padding) value");
-        checkArgument(
-            base64().decode(clientCertificateHash).length == 256 / 8,
-            "--cert_hash base64 does not decode to 256 bits");
-      }
-      getInstance().clientCertificate = null;
-      getInstance().clientCertificateHash = clientCertificateHash;
-      return this;
-    }
-
     public Builder setContactsRequireSyncing(boolean contactsRequireSyncing) {
       getInstance().contactsRequireSyncing = contactsRequireSyncing;
       return this;

core/src/main/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java

@@ -138,15 +138,6 @@ abstract class CreateOrUpdateRegistrarCommand extends MutatingCommand {
       validateWith = PathParameter.InputFile.class)
   Path clientCertificateFilename;
 
-  @Nullable
-  @Parameter(
-    names = "--cert_hash",
-    description =
-        "Hash of client certificate (SHA256 base64 no padding). Do not use this unless "
-            + "you want to store ONLY the hash and not the full certificate"
-  )
-  String clientCertificateHash;
-
   @Nullable
   @Parameter(
       names = "--failover_cert_file",
@@ -375,14 +366,6 @@ abstract class CreateOrUpdateRegistrarCommand extends MutatingCommand {
         }
         builder.setFailoverClientCertificate(asciiCert, now);
       }
-      if (!isNullOrEmpty(clientCertificateHash)) {
-        checkArgument(clientCertificateFilename == null,
-            "Can't specify both --cert_hash and --cert_file");
-        if ("null".equals(clientCertificateHash)) {
-          clientCertificateHash = null;
-        }
-        builder.setClientCertificateHash(clientCertificateHash);
-      }
       if (ianaId != null) {
         builder.setIanaIdentifier(ianaId.orElse(null));
       }

core/src/main/java/google/registry/tools/SetupOteCommand.java

@@ -65,13 +65,6 @@ final class SetupOteCommand extends ConfirmingCommand implements CommandWithRemo
       validateWith = PathParameter.InputFile.class)
   private Path certFile;
 
-  @Parameter(
-      names = {"-h", "--certhash"},
-      description =
-          "Hash of client certificate (SHA256 base64 no padding). Do not use this unless "
-              + "you want to store ONLY the hash and not the full certificate.")
-  private String certHash;
-
   @Parameter(
       names = {"--overwrite"},
       description = "Whether to replace existing entities if we encounter any, instead of failing.")
@@ -89,9 +82,7 @@ final class SetupOteCommand extends ConfirmingCommand implements CommandWithRemo
   /** Run any pre-execute command checks */
   @Override
   protected void init() throws Exception {
-    checkArgument(
+    checkArgument(certFile != null, "Must specify exactly one client certificate file.");
-        certFile == null ^ certHash == null,
-        "Must specify exactly one of client certificate file or client certificate hash.");
 
     password = passwordGenerator.createString(PASSWORD_LENGTH);
     oteAccountBuilder =
@@ -101,16 +92,10 @@ final class SetupOteCommand extends ConfirmingCommand implements CommandWithRemo
             .setIpAllowList(ipAllowList)
             .setReplaceExisting(overwrite);
 
-    if (certFile != null) {
       String asciiCert = MoreFiles.asCharSource(certFile, US_ASCII).read();
       // Don't wait for create_registrar to fail if it's a bad certificate file.
       loadCertificate(asciiCert);
       oteAccountBuilder.setCertificate(asciiCert, clock.nowUtc());
-    }
-
-    if (certHash != null) {
-      oteAccountBuilder.setCertificateHash(certHash);
-    }
   }
 
   @Override

core/src/test/java/google/registry/flows/EppLoginTlsTest.java

@@ -44,13 +44,13 @@ class EppLoginTlsTest extends EppTestCase {
     persistResource(
         loadRegistrar("NewRegistrar")
             .asBuilder()
-            .setClientCertificateHash(CertificateSamples.SAMPLE_CERT_HASH)
+            .setClientCertificate(CertificateSamples.SAMPLE_CERT, DateTime.now(UTC))
             .build());
     // Set a cert for the second registrar, or else any cert will be allowed for login.
     persistResource(
         loadRegistrar("TheRegistrar")
             .asBuilder()
-            .setClientCertificateHash(CertificateSamples.SAMPLE_CERT2_HASH)
+            .setClientCertificate(CertificateSamples.SAMPLE_CERT2, DateTime.now(UTC))
             .build());
   }
 

core/src/test/java/google/registry/flows/session/LoginFlowViaTlsTest.java

@@ -15,6 +15,7 @@
 package google.registry.flows.session;
 
 import static google.registry.testing.DatabaseHelper.persistResource;
+import static org.joda.time.DateTimeZone.UTC;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.net.InetAddresses;
@@ -26,6 +27,7 @@ import google.registry.model.registrar.Registrar;
 import google.registry.testing.CertificateSamples;
 import google.registry.util.CidrAddressBlock;
 import java.util.Optional;
+import org.joda.time.DateTime;
 import org.junit.jupiter.api.Test;
 
 /** Unit tests for {@link LoginFlow} when accessed via a TLS transport. */
@@ -41,7 +43,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
   @Override
   protected Registrar.Builder getRegistrarBuilder() {
     return super.getRegistrarBuilder()
-        .setClientCertificateHash(GOOD_CERT)
+        .setClientCertificate(CertificateSamples.SAMPLE_CERT, DateTime.now(UTC))
         .setIpAddressAllowList(
             ImmutableList.of(CidrAddressBlock.create(InetAddresses.forString(GOOD_IP.get()), 32)));
   }

core/src/test/java/google/registry/model/OteAccountBuilderTest.java

@@ -156,16 +156,6 @@ public final class OteAccountBuilderTest {
         .isTrue();
   }
 
-  @Test
-  void testCreateOteEntities_setCertificateHash() {
-    OteAccountBuilder.forClientId("myclientid")
-        .setCertificateHash(SAMPLE_CERT_HASH)
-        .buildAndPersist();
-
-    assertThat(Registrar.loadByClientId("myclientid-3").get().getClientCertificateHash())
-        .isEqualTo(SAMPLE_CERT_HASH);
-  }
-
   @Test
   void testCreateOteEntities_setCertificate() {
     OteAccountBuilder.forClientId("myclientid")

core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java

@@ -21,7 +21,6 @@ import static google.registry.persistence.transaction.TransactionManagerFactory.
 import static google.registry.testing.CertificateSamples.SAMPLE_CERT;
 import static google.registry.testing.CertificateSamples.SAMPLE_CERT3;
 import static google.registry.testing.CertificateSamples.SAMPLE_CERT3_HASH;
-import static google.registry.testing.CertificateSamples.SAMPLE_CERT_HASH;
 import static google.registry.testing.DatabaseHelper.createTlds;
 import static google.registry.testing.DatabaseHelper.persistNewRegistrar;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
@@ -447,29 +446,6 @@ class CreateRegistrarCommandTest extends CommandTestCase<CreateRegistrarCommand>
     assertThat(registrar).isEmpty();
   }
 
-  @Test
-  void testSuccess_clientCertHashFlag() throws Exception {
-    runCommandForced(
-        "--name=blobio",
-        "--password=some_password",
-        "--registrar_type=REAL",
-        "--iana_id=8",
-        "--cert_hash=" + SAMPLE_CERT_HASH,
-        "--passcode=01234",
-        "--icann_referral_email=foo@bar.test",
-        "--street=\"123 Fake St\"",
-        "--city Fakington",
-        "--state MA",
-        "--zip 00351",
-        "--cc US",
-        "clientz");
-
-    Optional<Registrar> registrar = Registrar.loadByClientId("clientz");
-    assertThat(registrar).isPresent();
-    assertThat(registrar.get().getClientCertificate()).isNull();
-    assertThat(registrar.get().getClientCertificateHash()).isEqualTo(SAMPLE_CERT_HASH);
-  }
-
   @Test
   void testSuccess_failoverClientCertFileFlag() throws Exception {
     fakeClock.setTo(DateTime.parse("2020-11-01T00:00:00Z"));
@@ -1182,74 +1158,6 @@ class CreateRegistrarCommandTest extends CommandTestCase<CreateRegistrarCommand>
                 "clientz"));
   }
 
-  @Test
-  void testFailure_certHashAndCertFile() {
-    assertThrows(
-        IllegalArgumentException.class,
-        () ->
-            runCommandForced(
-                "--name=blobio",
-                "--password=some_password",
-                "--registrar_type=REAL",
-                "--iana_id=8",
-                "--cert_file=" + getCertFilename(),
-                "--cert_hash=ABCDEF",
-                "--passcode=01234",
-                "--icann_referral_email=foo@bar.test",
-                "--street=\"123 Fake St\"",
-                "--city Fakington",
-                "--state MA",
-                "--zip 00351",
-                "--cc US",
-                "clientz"));
-  }
-
-  @Test
-  void testFailure_certHashNotBase64() {
-    IllegalArgumentException thrown =
-        assertThrows(
-            IllegalArgumentException.class,
-            () ->
-                runCommandForced(
-                    "--name=blobio",
-                    "--password=some_password",
-                    "--registrar_type=REAL",
-                    "--iana_id=8",
-                    "--cert_hash=!",
-                    "--passcode=01234",
-                    "--icann_referral_email=foo@bar.test",
-                    "--street=\"123 Fake St\"",
-                    "--city Fakington",
-                    "--state MA",
-                    "--zip 00351",
-                    "--cc US",
-                    "clientz"));
-    assertThat(thrown).hasMessageThat().contains("base64");
-  }
-
-  @Test
-  void testFailure_certHashNotA256BitValue() {
-    IllegalArgumentException thrown =
-        assertThrows(
-            IllegalArgumentException.class,
-            () ->
-                runCommandForced(
-                    "--name=blobio",
-                    "--password=some_password",
-                    "--registrar_type=REAL",
-                    "--iana_id=8",
-                    "--cert_hash=abc",
-                    "--passcode=01234",
-                    "--icann_referral_email=foo@bar.test",
-                    "--street=\"123 Fake St\"",
-                    "--city Fakington",
-                    "--state MA",
-                    "--zip 00351",
-                    "--cc US",
-                    "clientz"));
-    assertThat(thrown).hasMessageThat().contains("256");
-  }
-
   @Test
   void testFailure_missingName() {
     IllegalArgumentException thrown =

core/src/test/java/google/registry/tools/SetupOteCommandTest.java

@@ -18,7 +18,6 @@ import static com.google.common.truth.Truth.assertThat;
 import static google.registry.model.registrar.Registrar.State.ACTIVE;
 import static google.registry.model.registry.Registry.TldState.GENERAL_AVAILABILITY;
 import static google.registry.model.registry.Registry.TldState.START_DATE_SUNRISE;
-import static google.registry.testing.CertificateSamples.SAMPLE_CERT;
 import static google.registry.testing.CertificateSamples.SAMPLE_CERT_HASH;
 import static google.registry.testing.DatabaseHelper.createTld;
 import static google.registry.testing.DatabaseHelper.loadRegistrar;
@@ -98,8 +97,7 @@ class SetupOteCommandTest extends CommandTestCase<SetupOteCommand> {
       String registrarName,
       String allowedTld,
       String password,
-      ImmutableList<CidrAddressBlock> ipAllowList,
+      ImmutableList<CidrAddressBlock> ipAllowList) {
-      boolean hashOnly) {
     Registrar registrar = loadRegistrar(registrarName);
     assertThat(registrar).isNotNull();
     assertThat(registrar.getAllowedTlds()).containsExactlyElementsIn(ImmutableSet.of(allowedTld));
@@ -108,18 +106,6 @@ class SetupOteCommandTest extends CommandTestCase<SetupOteCommand> {
     assertThat(registrar.verifyPassword(password)).isTrue();
     assertThat(registrar.getIpAddressAllowList()).isEqualTo(ipAllowList);
     assertThat(registrar.getClientCertificateHash()).isEqualTo(SAMPLE_CERT_HASH);
-    // If certificate hash is provided, there's no certificate file stored with the registrar.
-    if (!hashOnly) {
-      assertThat(registrar.getClientCertificate()).isEqualTo(SAMPLE_CERT);
-    }
-  }
-
-  private void verifyRegistrarCreation(
-      String registrarName,
-      String allowedTld,
-      String password,
-      ImmutableList<CidrAddressBlock> ipAllowList) {
-    verifyRegistrarCreation(registrarName, allowedTld, password, ipAllowList, false);
   }
 
   private void verifyRegistrarContactCreation(String registrarName, String email) {
@@ -184,24 +170,6 @@ class SetupOteCommandTest extends CommandTestCase<SetupOteCommand> {
     verifyRegistrarContactCreation("abc-5", "abc@email.com");
   }
 
-  @Test
-  void testSuccess_certificateHash() throws Exception {
-    runCommandForced(
-        "--ip_allow_list=1.1.1.1",
-        "--registrar=blobio",
-        "--email=contact@email.com",
-        "--certhash=" + SAMPLE_CERT_HASH);
-
-    verifyTldCreation("blobio-eap", "BLOBIOE3", GENERAL_AVAILABILITY, true);
-
-    ImmutableList<CidrAddressBlock> ipAddress =
-        ImmutableList.of(CidrAddressBlock.create("1.1.1.1"));
-
-    verifyRegistrarCreation("blobio-5", "blobio-eap", PASSWORD, ipAddress, true);
-
-    verifyRegistrarContactCreation("blobio-5", "contact@email.com");
-  }
-
   @Test
   void testSuccess_multipleIps() throws Exception {
     runCommandForced(
@@ -256,7 +224,7 @@ class SetupOteCommandTest extends CommandTestCase<SetupOteCommand> {
   }
 
   @Test
-  void testFailure_missingCertificateFileAndCertificateHash() {
+  void testFailure_missingCertificateFile() {
     IllegalArgumentException thrown =
         assertThrows(
             IllegalArgumentException.class,
@@ -265,26 +233,7 @@ class SetupOteCommandTest extends CommandTestCase<SetupOteCommand> {
                     "--ip_allow_list=1.1.1.1", "--email=contact@email.com", "--registrar=blobio"));
     assertThat(thrown)
         .hasMessageThat()
-        .contains(
+        .contains("Must specify exactly one client certificate file.");
-            "Must specify exactly one of client certificate file or client certificate hash.");
-  }
-
-  @Test
-  void testFailure_suppliedCertificateFileAndCertificateHash() {
-    IllegalArgumentException thrown =
-        assertThrows(
-            IllegalArgumentException.class,
-            () ->
-                runCommandForced(
-                    "--ip_allow_list=1.1.1.1",
-                    "--email=contact@email.com",
-                    "--registrar=blobio",
-                    "--certfile=" + getCertFilename(),
-                    "--certhash=" + SAMPLE_CERT_HASH));
-    assertThat(thrown)
-        .hasMessageThat()
-        .contains(
-            "Must specify exactly one of client certificate file or client certificate hash.");
   }
 
   @Test

core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java

@@ -21,7 +21,6 @@ import static google.registry.persistence.transaction.TransactionManagerFactory.
 import static google.registry.testing.CertificateSamples.SAMPLE_CERT;
 import static google.registry.testing.CertificateSamples.SAMPLE_CERT3;
 import static google.registry.testing.CertificateSamples.SAMPLE_CERT3_HASH;
-import static google.registry.testing.CertificateSamples.SAMPLE_CERT_HASH;
 import static google.registry.testing.DatabaseHelper.createTlds;
 import static google.registry.testing.DatabaseHelper.loadRegistrar;
 import static google.registry.testing.DatabaseHelper.persistResource;
@@ -339,14 +338,6 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
     assertThat(registrar.getFailoverClientCertificate()).isEqualTo(SAMPLE_CERT3);
   }
 
-  @Test
-  void testSuccess_certHash() throws Exception {
-    assertThat(loadRegistrar("NewRegistrar").getClientCertificateHash()).isNull();
-    runCommand("--cert_hash=" + SAMPLE_CERT_HASH, "--force", "NewRegistrar");
-    assertThat(loadRegistrar("NewRegistrar").getClientCertificateHash())
-        .isEqualTo(SAMPLE_CERT_HASH);
-  }
-
   @Test
   void testSuccess_clearCert() throws Exception {
     persistResource(
@@ -359,18 +350,6 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
     assertThat(loadRegistrar("NewRegistrar").getClientCertificate()).isNull();
   }
 
-  @Test
-  void testSuccess_clearCertHash() throws Exception {
-    persistResource(
-        loadRegistrar("NewRegistrar")
-            .asBuilder()
-            .setClientCertificateHash(SAMPLE_CERT_HASH)
-            .build());
-    assertThat(isNullOrEmpty(loadRegistrar("NewRegistrar").getClientCertificateHash())).isFalse();
-    runCommand("--cert_hash=null", "--force", "NewRegistrar");
-    assertThat(loadRegistrar("NewRegistrar").getClientCertificateHash()).isNull();
-  }
-
   @Test
   void testSuccess_ianaId() throws Exception {
     assertThat(loadRegistrar("NewRegistrar").getIanaIdentifier()).isEqualTo(8);
@@ -762,18 +741,6 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
         () -> runCommand("--cert_file=" + writeToTmpFile("ABCDEF"), "--force", "NewRegistrar"));
   }
 
-  @Test
-  void testFailure_certHashAndCertFile() {
-    assertThrows(
-        IllegalArgumentException.class,
-        () ->
-            runCommand(
-                "--cert_file=" + getCertFilename(SAMPLE_CERT3),
-                "--cert_hash=ABCDEF",
-                "--force",
-                "NewRegistrar"));
-  }
-
   @Test
   void testFailure_missingClientId() {
     assertThrows(ParameterException.class, () -> runCommand("--force"));

core/src/test/java/google/registry/tools/ValidateLoginCredentialsCommandTest.java

@@ -20,6 +20,7 @@ import static google.registry.testing.DatabaseHelper.createTld;
 import static google.registry.testing.DatabaseHelper.loadRegistrar;
 import static google.registry.testing.DatabaseHelper.persistResource;
 import static google.registry.testing.EppExceptionSubject.assertAboutEppExceptions;
+import static org.joda.time.DateTimeZone.UTC;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import com.beust.jcommander.ParameterException;
@@ -33,6 +34,7 @@ import google.registry.testing.CertificateSamples;
 import google.registry.util.CidrAddressBlock;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import org.joda.time.DateTime;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
@@ -50,7 +52,7 @@ class ValidateLoginCredentialsCommandTest extends CommandTestCase<ValidateLoginC
         loadRegistrar("NewRegistrar")
             .asBuilder()
             .setPassword(PASSWORD)
-            .setClientCertificateHash(CERT_HASH)
+            .setClientCertificate(CertificateSamples.SAMPLE_CERT, DateTime.now(UTC))
             .setIpAddressAllowList(ImmutableList.of(new CidrAddressBlock(CLIENT_IP)))
             .setState(ACTIVE)
             .setAllowedTlds(ImmutableSet.of("tld"))

core/src/test/java/google/registry/webdriver/RegistrarConsoleScreenshotTest.java

@@ -342,26 +342,6 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
     driver.diffPage("edit");
   }
 
-  @RetryingTest(3)
-  void settingsSecurityWithHashOnly() throws Throwable {
-    server.runInAppEngineEnvironment(
-        () -> {
-          persistResource(
-              loadRegistrar("TheRegistrar")
-                  .asBuilder()
-                  .setClientCertificateHash(CertificateSamples.SAMPLE_CERT_HASH)
-                  .build());
-          return null;
-        });
-    driver.manage().window().setSize(new Dimension(1050, 2000));
-    driver.get(server.getUrl("/registrar#security-settings"));
-    driver.waitForDisplayedElement(By.tagName("h1"));
-    driver.diffPage("view");
-    driver.waitForDisplayedElement(By.id("reg-app-btn-edit")).click();
-    driver.waitForDisplayedElement(By.tagName("h1"));
-    driver.diffPage("edit");
-  }
-
   @RetryingTest(3)
   void index_registrarDisabled() throws Throwable {
     server.runInAppEngineEnvironment(