Allow cert hash and fix array out of bound problem in OT&E command

Allow specifying certificate hash other than certificate file. This makes things easier when only setting up EAP registrars. The certificate hash can be easily pulled from existing registrars (SUNRISE, GA, etc) with automation.

Also fixes a bug where we always expect the registrar name + phase string to be at least 7-character long.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=188511561

java/google/registry/tools/CreateOrUpdateRegistrarCommand.java

@@ -142,10 +142,12 @@ abstract class CreateOrUpdateRegistrarCommand extends MutatingCommand {
 
   @Nullable
   @Parameter(
-      names = "--cert_hash",
+    names = "--cert_hash",
-      description = "Hash of client certificate (SHA256 base64 no padding). Do not use this unless "
+    description =
-          + "you want to store ONLY the hash and not the full certificate")
+        "Hash of client certificate (SHA256 base64 no padding). Do not use this unless "
-  private String clientCertificateHash;
+            + "you want to store ONLY the hash and not the full certificate"
+  )
+  String clientCertificateHash;
 
   @Nullable
   @Parameter(

java/google/registry/tools/SetupOteCommand.java

@@ -42,9 +42,7 @@ import org.joda.time.DateTime;
 import org.joda.time.Duration;
 
 /** Composite command to set up OT&E TLDs and accounts. */
-@Parameters(
+@Parameters(separators = " =", commandDescription = "Set up OT&E TLDs and registrars")
-    separators = " =",
-    commandDescription = "Set up OT&E TLDs and registrars")
 final class SetupOteCommand extends ConfirmingCommand implements RemoteApiCommand {
 
   // Regex: 3-14 alphanumeric characters or hyphens, the first of which must be a letter.
@@ -72,37 +70,50 @@ final class SetupOteCommand extends ConfirmingCommand implements RemoteApiComman
   Set<String> validDnsWriterNames;
 
   @Parameter(
-      names = {"-r", "--registrar"},
+    names = {"-r", "--registrar"},
-      description = "must 1) consist of only lowercase letters, numbers, or hyphens, "
+    description =
-          + "2) start with a letter, and 3) be between 3 and 14 characters (inclusive). "
+        "must 1) consist of only lowercase letters, numbers, or hyphens, "
-          + "We require 1 and 2 since the registrar name will be used to create TLDs,"
+            + "2) start with a letter, and 3) be between 3 and 14 characters (inclusive). "
-          + "and we require 3 since we append \"-[1234]\" to the name to create client"
+            + "We require 1 and 2 since the registrar name will be used to create TLDs,"
-          + "IDs which are required by the EPP XML schema to be between 3-16 chars.",
+            + "and we require 3 since we append \"-[1234]\" to the name to create client"
-      required = true)
+            + "IDs which are required by the EPP XML schema to be between 3-16 chars.",
+    required = true
+  )
   private String registrar;
 
   @Parameter(
-      names = {"-w", "--ip_whitelist"},
+    names = {"-w", "--ip_whitelist"},
-      description = "comma separated list of IP addreses or CIDR ranges",
+    description = "comma separated list of IP addreses or CIDR ranges",
-      required = true)
+    required = true
+  )
   private List<String> ipWhitelist = new ArrayList<>();
 
   @Parameter(
-      names = {"-c", "--certfile"},
+    names = {"-c", "--certfile"},
-      description = "full path to cert file in PEM format (best if on local storage)",
+    description = "full path to cert file in PEM format (best if on local storage)",
-      required = true,
+    validateWith = PathParameter.InputFile.class
-      validateWith = PathParameter.InputFile.class)
+  )
   private Path certFile;
 
   @Parameter(
-      names = {"--dns_writers"},
+    names = {"-h", "--certhash"},
-      description = "comma separated list of DNS writers to use on all TLDs",
+    description =
-      required = true)
+        "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 = {"--dns_writers"},
+    description = "comma separated list of DNS writers to use on all TLDs",
+    required = true
+  )
   private List<String> dnsWriters;
 
   @Parameter(
-      names = {"--premium_list"},
+    names = {"--premium_list"},
-      description = "premium list to apply to all TLDs")
+    description = "premium list to apply to all TLDs"
+  )
   private String premiumList = DEFAULT_PREMIUM_LIST;
 
   // TODO: (b/74079782) remove this flag once OT&E for .app is complete.
@@ -112,8 +123,7 @@ final class SetupOteCommand extends ConfirmingCommand implements RemoteApiComman
   )
   private boolean eapOnly = false;
 
-  @Inject
+  @Inject StringGenerator passwordGenerator;
-  StringGenerator passwordGenerator;
 
   /**
    * Long registrar names are truncated and then have an incrementing digit appended at the end so
@@ -139,8 +149,12 @@ final class SetupOteCommand extends ConfirmingCommand implements RemoteApiComman
     command.mainParameters = ImmutableList.of(tldName);
     command.pendingDeleteLength = pendingDeleteLength;
     command.premiumListName = Optional.of(premiumList);
-    command.roidSuffix = String.format(
+    String tldNameAlphaNumerical = tldName.replaceAll("[^a-z0-9]", "");
-        "%S%X", tldName.replaceAll("[^a-z0-9]", "").substring(0, 7), roidSuffixCounter++);
+    command.roidSuffix =
+        String.format(
+            "%S%X",
+            tldNameAlphaNumerical.substring(0, Math.min(tldNameAlphaNumerical.length(), 7)),
+            roidSuffixCounter++);
     command.redemptionGracePeriod = redemptionGracePeriod;
     if (isEarlyAccess) {
       command.eapFeeSchedule = EAP_FEE_SCHEDULE;
@@ -158,6 +172,7 @@ final class SetupOteCommand extends ConfirmingCommand implements RemoteApiComman
     command.registrarType = Registrar.Type.OTE;
     command.password = password;
     command.clientCertificateFilename = certFile;
+    command.clientCertificateHash = certHash;
     command.ipWhitelist = ipWhitelist;
     command.street = ImmutableList.of("e-street");
     command.city = "Neverland";
@@ -175,7 +190,8 @@ final class SetupOteCommand extends ConfirmingCommand implements RemoteApiComman
   /** Run any pre-execute command checks */
   @Override
   protected boolean checkExecutionState() throws Exception {
-    checkArgument(REGISTRAR_PATTERN.matcher(registrar).matches(),
+    checkArgument(
+        REGISTRAR_PATTERN.matcher(registrar).matches(),
         "Registrar name is invalid (see usage text for requirements).");
 
     boolean warned = false;
@@ -193,8 +209,14 @@ final class SetupOteCommand extends ConfirmingCommand implements RemoteApiComman
       return false;
     }
 
+    checkArgument(
+        certFile == null ^ certHash == null,
+        "Must specify exactly one of client certificate file or client certificate hash.");
+
     // Don't wait for create_registrar to fail if it's a bad certificate file.
-    loadCertificate(certFile.toAbsolutePath());
+    if (certFile != null) {
+      loadCertificate(certFile.toAbsolutePath());
+    }
     return true;
   }
 
@@ -219,7 +241,7 @@ final class SetupOteCommand extends ConfirmingCommand implements RemoteApiComman
           + "    " + registrar + "-3 (access to TLD " + registrar + "-ga)\n"
           + "    " + registrar + "-4 (access to TLD " + registrar + "-ga)\n"
           + "    " + registrar + "-5 (access to TLD " + registrar + "-eap)";
-   }
+    }
   }
 
   @Override

javatests/google/registry/tools/SetupOteCommandTest.java

@@ -120,7 +120,8 @@ public class SetupOteCommandTest extends CommandTestCase<SetupOteCommand> {
       String registrarName,
       String allowedTld,
       String password,
-      ImmutableList<CidrAddressBlock> ipWhitelist) {
+      ImmutableList<CidrAddressBlock> ipWhitelist,
+      boolean hashOnly) {
     Registrar registrar = loadRegistrar(registrarName);
     assertThat(registrar).isNotNull();
     assertThat(registrar.getAllowedTlds()).containsExactlyElementsIn(ImmutableSet.of(allowedTld));
@@ -128,8 +129,19 @@ public class SetupOteCommandTest extends CommandTestCase<SetupOteCommand> {
     assertThat(registrar.getState()).isEqualTo(ACTIVE);
     assertThat(registrar.testPassword(password)).isTrue();
     assertThat(registrar.getIpAddressWhitelist()).isEqualTo(ipWhitelist);
-    assertThat(registrar.getClientCertificate()).isEqualTo(SAMPLE_CERT);
     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> ipWhitelist) {
+    verifyRegistrarCreation(registrarName, allowedTld, password, ipWhitelist, false);
   }
 
   @Test
@@ -179,6 +191,79 @@ public class SetupOteCommandTest extends CommandTestCase<SetupOteCommand> {
     verifyRegistrarCreation("blobio-5", "blobio-eap", passwords.get(4), ipAddress);
   }
 
+  @Test
+  public void testSuccess_shortRegistrarName() throws Exception {
+    runCommandForced(
+        "--ip_whitelist=1.1.1.1",
+        "--registrar=abc",
+        "--dns_writers=VoidDnsWriter",
+        "--certfile=" + getCertFilename());
+
+    verifyTldCreation(
+        "abc-sunrise",
+        "ABCSUNR0",
+        TldState.START_DATE_SUNRISE,
+        "VoidDnsWriter",
+        "default_sandbox_list");
+    verifyTldCreation(
+        "abc-landrush", "ABCLAND1", TldState.LANDRUSH, "VoidDnsWriter", "default_sandbox_list");
+    verifyTldCreation(
+        "abc-ga",
+        "ABCGA2",
+        TldState.GENERAL_AVAILABILITY,
+        "VoidDnsWriter",
+        "default_sandbox_list",
+        Duration.standardMinutes(60),
+        Duration.standardMinutes(10),
+        Duration.standardMinutes(5),
+        false);
+    verifyTldCreation(
+        "abc-eap",
+        "ABCEAP3",
+        TldState.GENERAL_AVAILABILITY,
+        "VoidDnsWriter",
+        "default_sandbox_list",
+        Duration.standardMinutes(60),
+        Duration.standardMinutes(10),
+        Duration.standardMinutes(5),
+        true);
+
+    ImmutableList<CidrAddressBlock> ipAddress =
+        ImmutableList.of(CidrAddressBlock.create("1.1.1.1"));
+
+    verifyRegistrarCreation("abc-1", "abc-sunrise", passwords.get(0), ipAddress);
+    verifyRegistrarCreation("abc-2", "abc-landrush", passwords.get(1), ipAddress);
+    verifyRegistrarCreation("abc-3", "abc-ga", passwords.get(2), ipAddress);
+    verifyRegistrarCreation("abc-4", "abc-ga", passwords.get(3), ipAddress);
+    verifyRegistrarCreation("abc-5", "abc-eap", passwords.get(4), ipAddress);
+  }
+
+  @Test
+  public void testSuccess_certificateHash() throws Exception {
+    runCommandForced(
+        "--eap_only",
+        "--ip_whitelist=1.1.1.1",
+        "--registrar=blobio",
+        "--dns_writers=VoidDnsWriter",
+        "--certhash=" + SAMPLE_CERT_HASH);
+
+    verifyTldCreation(
+        "blobio-eap",
+        "BLOBIOE3",
+        TldState.GENERAL_AVAILABILITY,
+        "VoidDnsWriter",
+        "default_sandbox_list",
+        Duration.standardMinutes(60),
+        Duration.standardMinutes(10),
+        Duration.standardMinutes(5),
+        true);
+
+    ImmutableList<CidrAddressBlock> ipAddress =
+        ImmutableList.of(CidrAddressBlock.create("1.1.1.1"));
+
+    verifyRegistrarCreation("blobio-5", "blobio-eap", passwords.get(0), ipAddress, true);
+  }
+
   @Test
   public void testSuccess_eapOnly() throws Exception {
     runCommandForced(
@@ -328,14 +413,35 @@ public class SetupOteCommandTest extends CommandTestCase<SetupOteCommand> {
   }
 
   @Test
-  public void testFailure_missingCertificateFile() throws Exception {
+  public void testFailure_missingCertificateFileAndCertificateHash() throws Exception {
-    ParameterException thrown =
+    IllegalArgumentException thrown =
         assertThrows(
-            ParameterException.class,
+            IllegalArgumentException.class,
             () ->
                 runCommandForced(
                     "--ip_whitelist=1.1.1.1", "--dns_writers=VoidDnsWriter", "--registrar=blobio"));
-    assertThat(thrown).hasMessageThat().contains("option is required: -c, --certfile");
+    assertThat(thrown)
+        .hasMessageThat()
+        .contains(
+            "Must specify exactly one of client certificate file or client certificate hash.");
+  }
+
+  @Test
+  public void testFailure_suppliedCertificateFileAndCertificateHash() throws Exception {
+    IllegalArgumentException thrown =
+        assertThrows(
+            IllegalArgumentException.class,
+            () ->
+                runCommandForced(
+                    "--ip_whitelist=1.1.1.1",
+                    "--dns_writers=VoidDnsWriter",
+                    "--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