From 64986442bc4c0cbcab54e8eaf15ee311a6726af7 Mon Sep 17 00:00:00 2001 From: jianglai Date: Fri, 9 Mar 2018 11:23:29 -0800 Subject: [PATCH] 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 --- .../tools/CreateOrUpdateRegistrarCommand.java | 10 +- .../registry/tools/SetupOteCommand.java | 80 +++++++----- .../registry/tools/SetupOteCommandTest.java | 118 +++++++++++++++++- 3 files changed, 169 insertions(+), 39 deletions(-) diff --git a/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java b/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java index 074074470..0b741c14b 100644 --- a/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java +++ b/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java @@ -142,10 +142,12 @@ abstract class CreateOrUpdateRegistrarCommand extends MutatingCommand { @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") - private String clientCertificateHash; + 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( diff --git a/java/google/registry/tools/SetupOteCommand.java b/java/google/registry/tools/SetupOteCommand.java index bfb19e846..57ed2f8dc 100644 --- a/java/google/registry/tools/SetupOteCommand.java +++ b/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( - separators = " =", - commandDescription = "Set up OT&E TLDs and registrars") +@Parameters(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 validDnsWriterNames; @Parameter( - names = {"-r", "--registrar"}, - description = "must 1) consist of only lowercase letters, numbers, or hyphens, " - + "2) start with a letter, and 3) be between 3 and 14 characters (inclusive). " - + "We require 1 and 2 since the registrar name will be used to create TLDs," - + "and we require 3 since we append \"-[1234]\" to the name to create client" - + "IDs which are required by the EPP XML schema to be between 3-16 chars.", - required = true) + names = {"-r", "--registrar"}, + description = + "must 1) consist of only lowercase letters, numbers, or hyphens, " + + "2) start with a letter, and 3) be between 3 and 14 characters (inclusive). " + + "We require 1 and 2 since the registrar name will be used to create TLDs," + + "and we require 3 since we append \"-[1234]\" to the name to create client" + + "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"}, - description = "comma separated list of IP addreses or CIDR ranges", - required = true) + names = {"-w", "--ip_whitelist"}, + description = "comma separated list of IP addreses or CIDR ranges", + required = true + ) private List ipWhitelist = new ArrayList<>(); @Parameter( - names = {"-c", "--certfile"}, - description = "full path to cert file in PEM format (best if on local storage)", - required = true, - validateWith = PathParameter.InputFile.class) + names = {"-c", "--certfile"}, + description = "full path to cert file in PEM format (best if on local storage)", + validateWith = PathParameter.InputFile.class + ) private Path certFile; @Parameter( - names = {"--dns_writers"}, - description = "comma separated list of DNS writers to use on all TLDs", - required = true) + 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 = {"--dns_writers"}, + description = "comma separated list of DNS writers to use on all TLDs", + required = true + ) private List dnsWriters; @Parameter( - names = {"--premium_list"}, - description = "premium list to apply to all TLDs") + names = {"--premium_list"}, + 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 - StringGenerator passwordGenerator; + @Inject 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( - "%S%X", tldName.replaceAll("[^a-z0-9]", "").substring(0, 7), roidSuffixCounter++); + String tldNameAlphaNumerical = tldName.replaceAll("[^a-z0-9]", ""); + 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 diff --git a/javatests/google/registry/tools/SetupOteCommandTest.java b/javatests/google/registry/tools/SetupOteCommandTest.java index ec760abac..c7a3312e8 100644 --- a/javatests/google/registry/tools/SetupOteCommandTest.java +++ b/javatests/google/registry/tools/SetupOteCommandTest.java @@ -120,7 +120,8 @@ public class SetupOteCommandTest extends CommandTestCase { String registrarName, String allowedTld, String password, - ImmutableList ipWhitelist) { + ImmutableList 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 { 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 ipWhitelist) { + verifyRegistrarCreation(registrarName, allowedTld, password, ipWhitelist, false); } @Test @@ -179,6 +191,79 @@ public class SetupOteCommandTest extends CommandTestCase { 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 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 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 { } @Test - public void testFailure_missingCertificateFile() throws Exception { - ParameterException thrown = + public void testFailure_missingCertificateFileAndCertificateHash() throws Exception { + 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