From de3af34b66572b6287c994e0fa38028d6ec4cbcf Mon Sep 17 00:00:00 2001 From: Weimin Yu Date: Wed, 20 Mar 2024 14:52:17 -0400 Subject: [PATCH] Verify unblockables are truly unblockable (#2381) * Verify unblockables are truly unblockable Unblockable domains may become blockable due to deregistration or removal from the reserved list. The BSA refresh job is responsible for removing them from the database. This PR verifies that the refreshes are correct. Note that recent changes since last refresh are not reflected in the result, and inconsistency due to recent deregistrations are ignored. Changes in reserved status or IDN validity are not timestamped, therefore we cannot ignore recent inconsistencies. However, these changes are rare. * Addressing code review * Addressing code review --- .../registry/bsa/BsaValidateAction.java | 85 ++++++++++++++++++- .../registry/bsa/persistence/Queries.java | 10 ++- .../registry/config/RegistryConfig.java | 6 ++ .../config/RegistryConfigSettings.java | 1 + .../registry/config/files/default-config.yaml | 3 + .../registry/bsa/BsaValidateActionTest.java | 69 +++++++++++++++ 6 files changed, 172 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/google/registry/bsa/BsaValidateAction.java b/core/src/main/java/google/registry/bsa/BsaValidateAction.java index 158161cdb..e93f636fd 100644 --- a/core/src/main/java/google/registry/bsa/BsaValidateAction.java +++ b/core/src/main/java/google/registry/bsa/BsaValidateAction.java @@ -16,27 +16,42 @@ package google.registry.bsa; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Throwables.getStackTraceAsString; +import static com.google.common.collect.ImmutableList.toImmutableList; import static google.registry.bsa.BsaTransactions.bsaQuery; +import static google.registry.bsa.ReservedDomainsUtils.isReservedDomain; import static google.registry.bsa.persistence.Queries.batchReadBsaLabelText; +import static google.registry.persistence.transaction.TransactionManagerFactory.replicaTm; import static google.registry.request.Action.Method.GET; import static google.registry.request.Action.Method.POST; import static javax.servlet.http.HttpServletResponse.SC_OK; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Sets; import com.google.common.collect.Sets.SetView; import com.google.common.flogger.FluentLogger; +import com.google.common.net.InternetDomainName; +import google.registry.bsa.api.UnblockableDomain; +import google.registry.bsa.api.UnblockableDomain.Reason; import google.registry.bsa.persistence.DownloadScheduler; +import google.registry.bsa.persistence.Queries; import google.registry.config.RegistryConfig.Config; +import google.registry.model.ForeignKeyUtils; +import google.registry.model.domain.Domain; +import google.registry.persistence.VKey; import google.registry.request.Action; import google.registry.request.Response; import google.registry.request.auth.Auth; +import google.registry.util.Clock; +import java.util.Objects; import java.util.Optional; import java.util.stream.Stream; import javax.inject.Inject; +import org.joda.time.DateTime; +import org.joda.time.Duration; /** Validates the BSA data in the database against the most recent block lists. */ @Action( @@ -53,6 +68,8 @@ public class BsaValidateAction implements Runnable { private final IdnChecker idnChecker; private final BsaEmailSender emailSender; private final int transactionBatchSize; + private final Duration maxStaleness; + private final Clock clock; private final BsaLock bsaLock; private final Response response; @@ -62,12 +79,16 @@ public class BsaValidateAction implements Runnable { IdnChecker idnChecker, BsaEmailSender emailSender, @Config("bsaTxnBatchSize") int transactionBatchSize, + @Config("bsaValidationMaxStaleness") Duration maxStaleness, + Clock clock, BsaLock bsaLock, Response response) { this.gcsClient = gcsClient; this.idnChecker = idnChecker; this.emailSender = emailSender; this.transactionBatchSize = transactionBatchSize; + this.maxStaleness = maxStaleness; + this.clock = clock; this.bsaLock = bsaLock; this.response = response; } @@ -103,6 +124,7 @@ public class BsaValidateAction implements Runnable { ImmutableList.Builder errorsBuilder = new ImmutableList.Builder<>(); errorsBuilder.addAll(checkBsaLabels(downloadJobName.get())); + errorsBuilder.addAll(checkUnblockableDomains()); ImmutableList errors = errorsBuilder.build(); @@ -150,6 +172,66 @@ public class BsaValidateAction implements Runnable { return errors.build(); } + ImmutableList checkUnblockableDomains() { + ImmutableList.Builder errors = new ImmutableList.Builder<>(); + Optional lastRead = Optional.empty(); + ImmutableList batch; + do { + batch = Queries.batchReadUnblockableDomains(lastRead, transactionBatchSize); + ImmutableMap> activeDomains = + ForeignKeyUtils.load( + Domain.class, + batch.stream().map(UnblockableDomain::domainName).collect(toImmutableList()), + clock.nowUtc()); + for (var unblockable : batch) { + verifyDomainStillUnblockableWithReason(unblockable, activeDomains).ifPresent(errors::add); + } + if (!batch.isEmpty()) { + lastRead = Optional.of(Iterables.getLast(batch)); + } + } while (batch.size() == transactionBatchSize); + return errors.build(); + } + + Optional verifyDomainStillUnblockableWithReason( + UnblockableDomain domain, ImmutableMap> activeDomains) { + DateTime now = clock.nowUtc(); + boolean isRegistered = activeDomains.containsKey(domain.domainName()); + boolean isReserved = isReservedDomain(domain.domainName(), now); + InternetDomainName domainName = InternetDomainName.from(domain.domainName()); + boolean isInvalid = idnChecker.getAllValidIdns(domainName.parts().get(0)).isEmpty(); + + Reason expectedReason = + isRegistered + ? Reason.REGISTERED + : (isReserved ? Reason.RESERVED : (isInvalid ? Reason.INVALID : null)); + if (Objects.equals(expectedReason, domain.reason())) { + return Optional.empty(); + } + if (isRegistered || domain.reason().equals(Reason.REGISTERED)) { + if (isStalenessAllowed(isRegistered, activeDomains.get(domain.domainName()))) { + return Optional.empty(); + } + } + return Optional.of( + String.format( + "%s: should be %s, found %s", + domain.domainName(), + expectedReason != null ? expectedReason.name() : "BLOCKABLE", + domain.reason())); + } + + boolean isStalenessAllowed(boolean isNewDomain, VKey domainVKey) { + Domain domain = bsaQuery(() -> replicaTm().loadByKey(domainVKey)); + var now = clock.nowUtc(); + if (isNewDomain) { + return domain.getCreationTime().plus(maxStaleness).isAfter(now); + } else { + return domain.getDeletionTime().isBefore(now) + && domain.getDeletionTime().plus(maxStaleness).isAfter(now); + } + } + /** Returns unique labels across all block lists in the download specified by {@code jobName}. */ ImmutableSet fetchDownloadedLabels(String jobName) { ImmutableSet.Builder labelsBuilder = new ImmutableSet.Builder<>(); @@ -171,10 +253,11 @@ public class BsaValidateAction implements Runnable { Optional lastRead = Optional.empty(); do { batch = batchReadBsaLabelText(lastRead, batchSize); - batch.forEach(labelsBuilder::add); + labelsBuilder.addAll(batch); if (!batch.isEmpty()) { lastRead = Optional.of(Iterables.getLast(batch)); } + } while (batch.size() == batchSize); return labelsBuilder.build(); } diff --git a/core/src/main/java/google/registry/bsa/persistence/Queries.java b/core/src/main/java/google/registry/bsa/persistence/Queries.java index c85585483..8ca109100 100644 --- a/core/src/main/java/google/registry/bsa/persistence/Queries.java +++ b/core/src/main/java/google/registry/bsa/persistence/Queries.java @@ -15,6 +15,7 @@ package google.registry.bsa.persistence; import static com.google.common.base.Verify.verify; +import static com.google.common.collect.ImmutableList.toImmutableList; import static google.registry.bsa.BsaStringUtils.DOMAIN_SPLITTER; import static google.registry.bsa.BsaTransactions.bsaQuery; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; @@ -22,6 +23,7 @@ import static google.registry.persistence.transaction.TransactionManagerFactory. import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import google.registry.bsa.api.UnblockableDomain; import google.registry.model.CreateAutoTimestamp; import java.util.List; import java.util.Optional; @@ -45,7 +47,6 @@ public final class Queries { public static ImmutableList batchReadBsaLabelText( Optional lastRead, int batchSize) { - return ImmutableList.copyOf( bsaQuery( () -> @@ -58,6 +59,13 @@ public final class Queries { .getResultList())); } + public static ImmutableList batchReadUnblockableDomains( + Optional lastRead, int batchSize) { + return batchReadUnblockables(lastRead.map(BsaUnblockableDomain::of), batchSize).stream() + .map(BsaUnblockableDomain::toUnblockableDomain) + .collect(toImmutableList()); + } + static Stream queryBsaUnblockableDomainByLabels( ImmutableCollection labels) { return ((Stream) diff --git a/core/src/main/java/google/registry/config/RegistryConfig.java b/core/src/main/java/google/registry/config/RegistryConfig.java index f2e5f9e2c..fec748db5 100644 --- a/core/src/main/java/google/registry/config/RegistryConfig.java +++ b/core/src/main/java/google/registry/config/RegistryConfig.java @@ -1488,6 +1488,12 @@ public final class RegistryConfig { return Duration.standardSeconds(config.bsa.domainCreateTxnCommitTimeLagSeconds); } + @Provides + @Config("bsaValidationMaxStaleness") + public static Duration provideBsaValidationMaxStaleness(RegistryConfigSettings config) { + return Duration.standardSeconds(config.bsa.bsaValidationMaxStalenessSeconds); + } + @Provides @Config("bsaAuthUrl") public static String provideBsaAuthUrl(RegistryConfigSettings config) { diff --git a/core/src/main/java/google/registry/config/RegistryConfigSettings.java b/core/src/main/java/google/registry/config/RegistryConfigSettings.java index 70600e082..eab156452 100644 --- a/core/src/main/java/google/registry/config/RegistryConfigSettings.java +++ b/core/src/main/java/google/registry/config/RegistryConfigSettings.java @@ -275,6 +275,7 @@ public class RegistryConfigSettings { public int bsaMaxNopIntervalHours; public int bsaTxnBatchSize; public int domainCreateTxnCommitTimeLagSeconds; + public int bsaValidationMaxStalenessSeconds; public String authUrl; public int authTokenExpirySeconds; public Map dataUrls; diff --git a/core/src/main/java/google/registry/config/files/default-config.yaml b/core/src/main/java/google/registry/config/files/default-config.yaml index 764b703e0..7b85a803e 100644 --- a/core/src/main/java/google/registry/config/files/default-config.yaml +++ b/core/src/main/java/google/registry/config/files/default-config.yaml @@ -640,6 +640,9 @@ bsa: # Number of entities (labels and unblockable domains) to process in a single # DB transaction. bsaTxnBatchSize: 1000 + # Used by `BsaValidateAction`: ignore inconsistencies caused by recent events + # in the past. This is roughly equal to two `BsaRefreshAction` runs. + bsaValidationMaxStalenessSeconds: 3600 # Http endpoint for acquiring Auth tokens. authUrl: "https://" diff --git a/core/src/test/java/google/registry/bsa/BsaValidateActionTest.java b/core/src/test/java/google/registry/bsa/BsaValidateActionTest.java index 94511ac10..e5251171f 100644 --- a/core/src/test/java/google/registry/bsa/BsaValidateActionTest.java +++ b/core/src/test/java/google/registry/bsa/BsaValidateActionTest.java @@ -16,7 +16,12 @@ package google.registry.bsa; import static com.google.common.base.Throwables.getStackTraceAsString; import static com.google.common.truth.Truth.assertThat; +import static google.registry.bsa.persistence.BsaTestingUtils.persistBsaLabel; import static google.registry.bsa.persistence.BsaTestingUtils.persistDownloadSchedule; +import static google.registry.bsa.persistence.BsaTestingUtils.persistUnblockableDomain; +import static google.registry.testing.DatabaseHelper.createTld; +import static google.registry.testing.DatabaseHelper.persistActiveDomain; +import static google.registry.testing.DatabaseHelper.persistDeletedDomain; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.startsWith; @@ -31,9 +36,12 @@ import com.google.cloud.storage.contrib.nio.testing.LocalStorageHelper; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import google.registry.bsa.api.UnblockableDomain; +import google.registry.bsa.api.UnblockableDomain.Reason; import google.registry.bsa.persistence.BsaTestingUtils; import google.registry.gcs.GcsUtils; import google.registry.groups.GmailClient; +import google.registry.model.domain.Domain; import google.registry.persistence.transaction.JpaTestExtensions; import google.registry.persistence.transaction.JpaTestExtensions.JpaIntegrationWithCoverageExtension; import google.registry.request.Response; @@ -43,6 +51,7 @@ import google.registry.util.EmailMessage; import java.util.concurrent.Callable; import javax.mail.internet.InternetAddress; import org.joda.time.DateTime; +import org.joda.time.Duration; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -58,6 +67,8 @@ public class BsaValidateActionTest { private static final String DOWNLOAD_JOB_NAME = "job"; + private static final Duration MAX_STALENESS = Duration.standardMinutes(1); + FakeClock fakeClock = new FakeClock(DateTime.parse("2023-11-09T02:08:57.880Z")); @RegisterExtension @@ -90,8 +101,11 @@ public class BsaValidateActionTest { idnChecker, new BsaEmailSender(gmailClient, emailRecipient), /* transactionBatchSize= */ 500, + MAX_STALENESS, + fakeClock, bsaLock, response); + createTld("app"); } static void createBlockList(GcsClient gcsClient, BlockListType blockListType, String content) @@ -214,6 +228,61 @@ public class BsaValidateActionTest { assertThat(allErrors).contains("Found 1 unexpected labels in the DB. Examples: [test3]"); } + @Test + void isStalenessAllowed_newDomain_allowed() { + persistBsaLabel("label"); + Domain domain = persistActiveDomain("label.app", fakeClock.nowUtc()); + fakeClock.advanceBy(MAX_STALENESS.minus(Duration.standardSeconds(1))); + assertThat(action.isStalenessAllowed(true, domain.createVKey())).isTrue(); + } + + @Test + void isStalenessAllowed_newDomain_notAllowed() { + persistBsaLabel("label"); + Domain domain = persistActiveDomain("label.app", fakeClock.nowUtc()); + fakeClock.advanceBy(MAX_STALENESS); + assertThat(action.isStalenessAllowed(true, domain.createVKey())).isFalse(); + } + + @Test + void isStalenessAllowed_deletedDomain_allowed() { + persistBsaLabel("label"); + Domain domain = persistDeletedDomain("label.app", fakeClock.nowUtc()); + fakeClock.advanceBy(MAX_STALENESS.minus(Duration.standardSeconds(1))); + assertThat(action.isStalenessAllowed(false, domain.createVKey())).isTrue(); + } + + @Test + void isStalenessAllowed_deletedDomain_notAllowed() { + persistBsaLabel("label"); + Domain domain = persistDeletedDomain("label.app", fakeClock.nowUtc()); + fakeClock.advanceBy(MAX_STALENESS); + assertThat(action.isStalenessAllowed(false, domain.createVKey())).isFalse(); + } + + @Test + void checkUnblockableDomain_noError() { + createTld("app"); + persistActiveDomain("label.app"); + persistBsaLabel("label"); + persistUnblockableDomain(UnblockableDomain.of("label", "app", Reason.REGISTERED)); + when(idnChecker.getAllValidIdns(anyString())).thenReturn(ImmutableSet.of(IdnTableEnum.JA)); + + assertThat(action.checkUnblockableDomains()).isEmpty(); + } + + @Test + void checkUnblockableDomain_error() { + createTld("app"); + persistActiveDomain("label.app"); + persistBsaLabel("label"); + persistUnblockableDomain(UnblockableDomain.of("label", "app", Reason.RESERVED)); + when(idnChecker.getAllValidIdns(anyString())).thenReturn(ImmutableSet.of(IdnTableEnum.JA)); + + assertThat(action.checkUnblockableDomains()) + .containsExactly("label.app: should be REGISTERED, found RESERVED"); + } + @Test void notificationSent_cannotAcquireLock() { when(bsaLock.executeWithLock(any())).thenReturn(false);