From d5db6c16bcd219a280351e0c44215181051ccc3c Mon Sep 17 00:00:00 2001 From: sarahcaseybot Date: Tue, 1 Feb 2022 15:25:00 -0500 Subject: [PATCH] Add DS validation to match Cloud DNS (#1487) * Add DS validation to match Cloud DNS * Add checks to flows * Add some flow tests * Add tests for DomainCreateFlow * Add tests for UpdateDomainCommand * Fix docs test * Small fixes * Remove builder from tests --- .../flows/domain/DomainCreateFlow.java | 1 + .../flows/domain/DomainFlowUtils.java | 50 +++++++++++- .../flows/domain/DomainUpdateFlow.java | 1 + .../google/registry/tools/DigestType.java | 58 +++++++++++++ .../java/google/registry/tools/DsRecord.java | 11 +++ .../flows/domain/DomainCreateFlowTest.java | 17 ++++ .../flows/domain/DomainUpdateFlowTest.java | 81 ++++++++++++++++--- .../tools/CreateDomainCommandTest.java | 36 ++++++++- .../tools/UpdateDomainCommandTest.java | 55 +++++++++---- .../domain_create_dsdata_bad_algorithms.xml | 77 ++++++++++++++++++ .../domain_create_dsdata_bad_digest_types.xml | 77 ++++++++++++++++++ .../tools/server/domain_create_complete.xml | 4 +- .../tools/server/domain_update_add.xml | 4 +- .../tools/server/domain_update_complete.xml | 6 +- .../server/domain_update_complete_abc.xml | 6 +- .../tools/server/domain_update_remove.xml | 2 +- .../server/domain_update_set_ds_records.xml | 4 +- docs/flows.md | 2 + 18 files changed, 445 insertions(+), 47 deletions(-) create mode 100644 core/src/main/java/google/registry/tools/DigestType.java create mode 100644 core/src/test/resources/google/registry/flows/domain/domain_create_dsdata_bad_algorithms.xml create mode 100644 core/src/test/resources/google/registry/flows/domain/domain_create_dsdata_bad_digest_types.xml diff --git a/core/src/main/java/google/registry/flows/domain/DomainCreateFlow.java b/core/src/main/java/google/registry/flows/domain/DomainCreateFlow.java index f3bee2ef2..77605a4bf 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainCreateFlow.java +++ b/core/src/main/java/google/registry/flows/domain/DomainCreateFlow.java @@ -169,6 +169,7 @@ import org.joda.time.Duration; * @error {@link DomainFlowUtils.FeesMismatchException} * @error {@link DomainFlowUtils.FeesRequiredDuringEarlyAccessProgramException} * @error {@link DomainFlowUtils.FeesRequiredForPremiumNameException} + * @error {@link DomainFlowUtils.InvalidDsRecordException} * @error {@link DomainFlowUtils.InvalidIdnDomainLabelException} * @error {@link DomainFlowUtils.InvalidPunycodeException} * @error {@link DomainFlowUtils.InvalidTcnIdChecksumException} diff --git a/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java b/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java index eda6c79a1..4a7042bda 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java +++ b/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java @@ -129,6 +129,7 @@ import google.registry.model.tld.label.ReservedList; import google.registry.model.tmch.ClaimsListDao; import google.registry.persistence.VKey; import google.registry.tldconfig.idn.IdnLabelValidator; +import google.registry.tools.DigestType; import google.registry.util.Idn; import java.math.BigDecimal; import java.util.Collection; @@ -144,6 +145,7 @@ import org.joda.money.CurrencyUnit; import org.joda.money.Money; import org.joda.time.DateTime; import org.joda.time.Duration; +import org.xbill.DNS.DNSSEC.Algorithm; /** Static utility functions for domain flows. */ public class DomainFlowUtils { @@ -293,13 +295,46 @@ public class DomainFlowUtils { /** Check that the DS data that will be set on a domain is valid. */ static void validateDsData(Set dsData) throws EppException { - if (dsData != null && dsData.size() > MAX_DS_RECORDS_PER_DOMAIN) { - throw new TooManyDsRecordsException( - String.format( - "A maximum of %s DS records are allowed per domain.", MAX_DS_RECORDS_PER_DOMAIN)); + if (dsData != null) { + if (dsData.size() > MAX_DS_RECORDS_PER_DOMAIN) { + throw new TooManyDsRecordsException( + String.format( + "A maximum of %s DS records are allowed per domain.", MAX_DS_RECORDS_PER_DOMAIN)); + } + // TODO(sarahbot@): Add signature length verification + ImmutableList invalidAlgorithms = + dsData.stream() + .filter(ds -> !validateAlgorithm(ds.getAlgorithm())) + .collect(toImmutableList()); + if (!invalidAlgorithms.isEmpty()) { + throw new InvalidDsRecordException( + String.format( + "Domain contains DS record(s) with an invalid algorithm wire value: %s", + invalidAlgorithms)); + } + ImmutableList invalidDigestTypes = + dsData.stream() + .filter(ds -> !DigestType.fromWireValue(ds.getDigestType()).isPresent()) + .collect(toImmutableList()); + if (!invalidDigestTypes.isEmpty()) { + throw new InvalidDsRecordException( + String.format( + "Domain contains DS record(s) with an invalid digest type: %s", + invalidDigestTypes)); + } } } + public static boolean validateAlgorithm(int alg) { + if (alg > 255 || alg < 0) { + return false; + } + // Algorithms that are reserved or unassigned will just return a string representation of their + // integer wire value. + String algorithm = Algorithm.string(alg); + return !algorithm.equals(Integer.toString(alg)); + } + /** We only allow specifying years in a period. */ static Period verifyUnitIsYears(Period period) throws EppException { if (!checkNotNull(period).getUnit().equals(Period.Unit.YEARS)) { @@ -1217,6 +1252,13 @@ public class DomainFlowUtils { } } + /** Domain has an invalid DS record. */ + static class InvalidDsRecordException extends ParameterValuePolicyErrorException { + public InvalidDsRecordException(String message) { + super(message); + } + } + /** Domain name is under tld which doesn't exist. */ static class TldDoesNotExistException extends ParameterValueRangeErrorException { public TldDoesNotExistException(String tld) { diff --git a/core/src/main/java/google/registry/flows/domain/DomainUpdateFlow.java b/core/src/main/java/google/registry/flows/domain/DomainUpdateFlow.java index b257b8d9a..a124de9cc 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainUpdateFlow.java +++ b/core/src/main/java/google/registry/flows/domain/DomainUpdateFlow.java @@ -114,6 +114,7 @@ import org.joda.time.DateTime; * @error {@link DomainFlowUtils.EmptySecDnsUpdateException} * @error {@link DomainFlowUtils.FeesMismatchException} * @error {@link DomainFlowUtils.FeesRequiredForNonFreeOperationException} + * @error {@link DomainFlowUtils.InvalidDsRecordException} * @error {@link DomainFlowUtils.LinkedResourcesDoNotExistException} * @error {@link DomainFlowUtils.LinkedResourceInPendingDeleteProhibitsOperationException} * @error {@link DomainFlowUtils.MaxSigLifeChangeNotSupportedException} diff --git a/core/src/main/java/google/registry/tools/DigestType.java b/core/src/main/java/google/registry/tools/DigestType.java new file mode 100644 index 000000000..251399461 --- /dev/null +++ b/core/src/main/java/google/registry/tools/DigestType.java @@ -0,0 +1,58 @@ +// Copyright 2022 The Nomulus Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package google.registry.tools; + +import java.util.Optional; + +/** + * Enumerates the DNSSEC digest types for use with Delegation Signer records. + * + *

This also enforces the set of types that are valid for use with Cloud DNS. Customers cannot + * create DS records containing any other digest type. + * + *

The complete list can be found here: + * https://www.iana.org/assignments/ds-rr-types/ds-rr-types.xhtml + */ +public enum DigestType { + SHA1(1), + SHA256(2), + // Algorithm number 3 is GOST R 34.11-94 and is deliberately NOT SUPPORTED. + // This algorithm was reviewed by ise-crypto and deemed academically broken (b/207029800). + // In addition, RFC 8624 specifies that this algorithm MUST NOT be used for DNSSEC delegations. + // TODO(sarhabot@): Add note in Cloud DNS code to notify the Registry of any new changes to + // supported digest types. + SHA384(4); + + private final int wireValue; + + DigestType(int wireValue) { + this.wireValue = wireValue; + } + + /** Fetches a DigestType enumeration constant by its IANA assigned value. */ + public static Optional fromWireValue(int wireValue) { + for (DigestType alg : DigestType.values()) { + if (alg.getWireValue() == wireValue) { + return Optional.of(alg); + } + } + return Optional.empty(); + } + + /** Fetches a value in the range [0, 255] that encodes this DS digest type on the wire. */ + public int getWireValue() { + return wireValue; + } +} diff --git a/core/src/main/java/google/registry/tools/DsRecord.java b/core/src/main/java/google/registry/tools/DsRecord.java index ebbfdfc97..fb619d51f 100644 --- a/core/src/main/java/google/registry/tools/DsRecord.java +++ b/core/src/main/java/google/registry/tools/DsRecord.java @@ -16,6 +16,7 @@ package google.registry.tools; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.collect.ImmutableList.toImmutableList; +import static google.registry.util.PreconditionsUtils.checkArgumentPresent; import com.beust.jcommander.IStringConverter; import com.google.auto.value.AutoValue; @@ -25,6 +26,7 @@ import com.google.common.base.Splitter; import com.google.common.io.BaseEncoding; import com.google.template.soy.data.SoyListData; import com.google.template.soy.data.SoyMapData; +import google.registry.flows.domain.DomainFlowUtils; import java.util.List; @AutoValue @@ -46,6 +48,15 @@ abstract class DsRecord { "digest should be even-lengthed hex, but is %s (length %s)", digest, digest.length()); + checkArgumentPresent( + DigestType.fromWireValue(digestType), + String.format("DS record uses an unrecognized digest type: %d", digestType)); + + if (!DomainFlowUtils.validateAlgorithm(alg)) { + throw new IllegalArgumentException( + String.format("DS record uses an unrecognized algorithm: %d", alg)); + } + return new AutoValue_DsRecord(keyTag, alg, digestType, digest); } diff --git a/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java b/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java index 4c99eced4..c0ffb21ee 100644 --- a/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java +++ b/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java @@ -106,6 +106,7 @@ import google.registry.flows.domain.DomainFlowUtils.FeeDescriptionParseException import google.registry.flows.domain.DomainFlowUtils.FeesMismatchException; import google.registry.flows.domain.DomainFlowUtils.FeesRequiredDuringEarlyAccessProgramException; import google.registry.flows.domain.DomainFlowUtils.FeesRequiredForPremiumNameException; +import google.registry.flows.domain.DomainFlowUtils.InvalidDsRecordException; import google.registry.flows.domain.DomainFlowUtils.InvalidIdnDomainLabelException; import google.registry.flows.domain.DomainFlowUtils.InvalidPunycodeException; import google.registry.flows.domain.DomainFlowUtils.InvalidTcnIdChecksumException; @@ -1002,6 +1003,22 @@ class DomainCreateFlowTest extends ResourceFlowTestCase { private static final DelegationSignerData SOME_DSDATA = - DelegationSignerData.create(1, 2, 3, base16().decode("0123")); + DelegationSignerData.create(1, 2, 2, base16().decode("0123")); private static final ImmutableMap OTHER_DSDATA_TEMPLATE_MAP = ImmutableMap.of( "KEY_TAG", "12346", @@ -536,7 +537,7 @@ class DomainUpdateFlowTest extends ResourceFlowTestCase builder = new ImmutableSet.Builder<>(); for (int i = 0; i < 7; ++i) { - builder.add(DelegationSignerData.create(i, 2, 3, new byte[] {0, 1, 2})); + builder.add(DelegationSignerData.create(i, 2, 2, new byte[] {0, 1, 2})); } ImmutableSet commonDsData = builder.build(); @@ -643,7 +644,7 @@ class DomainUpdateFlowTest extends ResourceFlowTestCase builder = new ImmutableSet.Builder<>(); for (int i = 0; i < 7; ++i) { - builder.add(DelegationSignerData.create(i, 2, 3, new byte[] {0, 1, 2})); + builder.add(DelegationSignerData.create(i, 2, 2, new byte[] {0, 1, 2})); } ImmutableSet commonDsData = builder.build(); @@ -813,11 +814,69 @@ class DomainUpdateFlowTest extends ResourceFlowTestCase builder = new ImmutableSet.Builder<>(); for (int i = 0; i < 8; ++i) { - builder.add(DelegationSignerData.create(i, 2, 3, new byte[] {0, 1, 2})); + builder.add(DelegationSignerData.create(i, 2, 2, new byte[] {0, 1, 2})); } setEppInput("domain_update_dsdata_add.xml", OTHER_DSDATA_TEMPLATE_MAP); diff --git a/core/src/test/java/google/registry/tools/CreateDomainCommandTest.java b/core/src/test/java/google/registry/tools/CreateDomainCommandTest.java index 3cc1006b2..1784b417f 100644 --- a/core/src/test/java/google/registry/tools/CreateDomainCommandTest.java +++ b/core/src/test/java/google/registry/tools/CreateDomainCommandTest.java @@ -50,7 +50,7 @@ class CreateDomainCommandTest extends EppToolCommandTestCase + runCommandForced( + "--client=NewRegistrar", + "--registrant=crr-admin", + "--admins=crr-admin", + "--techs=crr-tech", + "--ds_records=1 2 3 abcd", + "example.tld")); + assertThat(thrown).hasMessageThat().isEqualTo("DS record uses an unrecognized digest type: 3"); + } + + @Test + void testFailure_invalidAlgorithm() { + IllegalArgumentException thrown = + assertThrows( + IllegalArgumentException.class, + () -> + runCommandForced( + "--client=NewRegistrar", + "--registrant=crr-admin", + "--admins=crr-admin", + "--techs=crr-tech", + "--ds_records=1 999 4 abcd", + "example.tld")); + assertThat(thrown).hasMessageThat().isEqualTo("DS record uses an unrecognized algorithm: 999"); + } + @Test void testFailure_dsRecordsNot4Parts() { IllegalArgumentException thrown = diff --git a/core/src/test/java/google/registry/tools/UpdateDomainCommandTest.java b/core/src/test/java/google/registry/tools/UpdateDomainCommandTest.java index ce1138416..840a800b4 100644 --- a/core/src/test/java/google/registry/tools/UpdateDomainCommandTest.java +++ b/core/src/test/java/google/registry/tools/UpdateDomainCommandTest.java @@ -86,12 +86,12 @@ class UpdateDomainCommandTest extends EppToolCommandTestCase + runCommandForced( + "--client=NewRegistrar", "--add_ds_records=1 299 2 abcd", "example.tld")); + assertThat(thrown).hasMessageThat().isEqualTo("DS record uses an unrecognized algorithm: 299"); + } + + @TestOfyAndSql + void testFailure_invalidDsRecordDigestType() { + IllegalArgumentException thrown = + assertThrows( + IllegalArgumentException.class, + () -> + runCommandForced( + "--client=NewRegistrar", "--add_ds_records=1 2 3 abcd", "example.tld")); + assertThat(thrown).hasMessageThat().isEqualTo("DS record uses an unrecognized digest type: 3"); + } + @TestOfyAndSql void testFailure_provideDsRecordsAndAddDsRecords() { IllegalArgumentException thrown = @@ -638,8 +659,8 @@ class UpdateDomainCommandTest extends EppToolCommandTestCase runCommandForced( "--client=NewRegistrar", - "--add_ds_records=1 2 3 abcd", - "--ds_records=4 5 6 EF01", + "--add_ds_records=1 2 2 abcd", + "--ds_records=4 5 1 EF01", "example.tld")); assertThat(thrown) .hasMessageThat() @@ -656,8 +677,8 @@ class UpdateDomainCommandTest extends EppToolCommandTestCase runCommandForced( "--client=NewRegistrar", - "--remove_ds_records=7 8 9 12ab", - "--ds_records=4 5 6 EF01", + "--remove_ds_records=7 8 1 12ab", + "--ds_records=4 5 1 EF01", "example.tld")); assertThat(thrown) .hasMessageThat() @@ -674,7 +695,7 @@ class UpdateDomainCommandTest extends EppToolCommandTestCase runCommandForced( "--client=NewRegistrar", - "--add_ds_records=1 2 3 abcd", + "--add_ds_records=1 2 2 abcd", "--clear_ds_records", "example.tld")); assertThat(thrown) @@ -692,7 +713,7 @@ class UpdateDomainCommandTest extends EppToolCommandTestCase runCommandForced( "--client=NewRegistrar", - "--remove_ds_records=7 8 9 12ab", + "--remove_ds_records=7 8 1 12ab", "--clear_ds_records", "example.tld")); assertThat(thrown) diff --git a/core/src/test/resources/google/registry/flows/domain/domain_create_dsdata_bad_algorithms.xml b/core/src/test/resources/google/registry/flows/domain/domain_create_dsdata_bad_algorithms.xml new file mode 100644 index 000000000..4938f6f5a --- /dev/null +++ b/core/src/test/resources/google/registry/flows/domain/domain_create_dsdata_bad_algorithms.xml @@ -0,0 +1,77 @@ + + + + + + example.tld + 2 + + ns1.example.net + ns2.example.net + + jd1234 + sh8013 + sh8013 + + 2fooBAR + + + + + + + 12345 + 99 + 1 + 49FD46E6C4B45C55D4AC + + + 12346 + 3 + 1 + 49FD46E6C4B45C55D4AC + + + 12347 + 3 + 1 + 49FD46E6C4B45C55D4AC + + + 12348 + 3 + 1 + 49FD46E6C4B45C55D4AC + + + 12349 + 98 + 1 + 49FD46E6C4B45C55D4AC + + + 12350 + 3 + 1 + 49FD46E6C4B45C55D4AC + + + 12351 + 3 + 1 + 49FD46E6C4B45C55D4AC + + + 12352 + 3 + 1 + 49FD46E6C4B45C55D4AC + + + + ABC-12345 + + diff --git a/core/src/test/resources/google/registry/flows/domain/domain_create_dsdata_bad_digest_types.xml b/core/src/test/resources/google/registry/flows/domain/domain_create_dsdata_bad_digest_types.xml new file mode 100644 index 000000000..3f235257f --- /dev/null +++ b/core/src/test/resources/google/registry/flows/domain/domain_create_dsdata_bad_digest_types.xml @@ -0,0 +1,77 @@ + + + + + + example.tld + 2 + + ns1.example.net + ns2.example.net + + jd1234 + sh8013 + sh8013 + + 2fooBAR + + + + + + + 12345 + 3 + 100 + 49FD46E6C4B45C55D4AC + + + 12346 + 3 + 3 + 49FD46E6C4B45C55D4AC + + + 12347 + 3 + 1 + 49FD46E6C4B45C55D4AC + + + 12348 + 3 + 1 + 49FD46E6C4B45C55D4AC + + + 12349 + 3 + 1 + 49FD46E6C4B45C55D4AC + + + 12350 + 3 + 1 + 49FD46E6C4B45C55D4AC + + + 12351 + 3 + 1 + 49FD46E6C4B45C55D4AC + + + 12352 + 3 + 1 + 49FD46E6C4B45C55D4AC + + + + ABC-12345 + + diff --git a/core/src/test/resources/google/registry/tools/server/domain_create_complete.xml b/core/src/test/resources/google/registry/tools/server/domain_create_complete.xml index d09d33a3c..bcbb2ac39 100644 --- a/core/src/test/resources/google/registry/tools/server/domain_create_complete.xml +++ b/core/src/test/resources/google/registry/tools/server/domain_create_complete.xml @@ -25,13 +25,13 @@ 1 2 - 3 + 2 ABCD 4 5 - 6 + 1 EF01 diff --git a/core/src/test/resources/google/registry/tools/server/domain_update_add.xml b/core/src/test/resources/google/registry/tools/server/domain_update_add.xml index dd7c09501..aaab20984 100644 --- a/core/src/test/resources/google/registry/tools/server/domain_update_add.xml +++ b/core/src/test/resources/google/registry/tools/server/domain_update_add.xml @@ -22,13 +22,13 @@ 1 2 - 3 + 2 ABCD 4 5 - 6 + 1 EF01 diff --git a/core/src/test/resources/google/registry/tools/server/domain_update_complete.xml b/core/src/test/resources/google/registry/tools/server/domain_update_complete.xml index cf0e79774..ae3aa2d0f 100644 --- a/core/src/test/resources/google/registry/tools/server/domain_update_complete.xml +++ b/core/src/test/resources/google/registry/tools/server/domain_update_complete.xml @@ -37,7 +37,7 @@ 7 8 - 9 + 1 12AB @@ -51,13 +51,13 @@ 1 2 - 3 + 2 ABCD 4 5 - 6 + 1 EF01 diff --git a/core/src/test/resources/google/registry/tools/server/domain_update_complete_abc.xml b/core/src/test/resources/google/registry/tools/server/domain_update_complete_abc.xml index 4fb44d05e..9b0966b5e 100644 --- a/core/src/test/resources/google/registry/tools/server/domain_update_complete_abc.xml +++ b/core/src/test/resources/google/registry/tools/server/domain_update_complete_abc.xml @@ -37,7 +37,7 @@ 7 8 - 9 + 1 12AB @@ -51,13 +51,13 @@ 1 2 - 3 + 2 ABCD 4 5 - 6 + 1 EF01 diff --git a/core/src/test/resources/google/registry/tools/server/domain_update_remove.xml b/core/src/test/resources/google/registry/tools/server/domain_update_remove.xml index f5f77fbcf..1a99325c4 100644 --- a/core/src/test/resources/google/registry/tools/server/domain_update_remove.xml +++ b/core/src/test/resources/google/registry/tools/server/domain_update_remove.xml @@ -21,7 +21,7 @@ 7 8 - 9 + 1 12AB diff --git a/core/src/test/resources/google/registry/tools/server/domain_update_set_ds_records.xml b/core/src/test/resources/google/registry/tools/server/domain_update_set_ds_records.xml index 8cc00ceb6..04f301350 100644 --- a/core/src/test/resources/google/registry/tools/server/domain_update_set_ds_records.xml +++ b/core/src/test/resources/google/registry/tools/server/domain_update_set_ds_records.xml @@ -16,13 +16,13 @@ 1 2 - 3 + 2 ABCD 4 5 - 6 + 1 EF01 diff --git a/docs/flows.md b/docs/flows.md index 241783596..2d8491f8d 100644 --- a/docs/flows.md +++ b/docs/flows.md @@ -398,6 +398,7 @@ An EPP flow that creates a new domain resource. * The fee description passed in the transform command matches multiple fee types. * The fee description passed in the transform command cannot be parsed. + * Domain has an invalid DS record. * Domain name starts with xn-- but is not a valid IDN. * The specified trademark validator is not supported. * Domain labels cannot begin with a dash. @@ -810,6 +811,7 @@ statuses are updated at once. * 2306 * Cannot add and remove the same value. * More than one contact for a given role is not allowed. + * Domain has an invalid DS record. * Missing type attribute for contact. * The secDNS:all element must have value 'true' if present. * Too many DS records set on a domain.