From 2020dcb50ffcd99c7e24cae39e2ab7436d341848 Mon Sep 17 00:00:00 2001 From: jianglai Date: Fri, 19 Oct 2018 13:27:42 -0700 Subject: [PATCH] Refactor StringGenerator bindings Make every dependency request explicit on what encoding is used. Also get rid of InjectRule in XjcToDomainResourceConverterTest. Random number generator providers are separated to secure and insecure ones. The insecure ones must be explicitly requested (usually for use cases where security is not of concern, for better speed). ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=217921422 --- .../registry/config/RegistryConfig.java | 41 ++++++++++-- .../rde/imports/RdeDomainImportAction.java | 14 ++-- .../imports/XjcToDomainResourceConverter.java | 20 +----- .../registry/tmch/NordnUploadAction.java | 2 +- .../tools/CreateAnchorTenantCommand.java | 2 + .../registry/tools/CreateContactCommand.java | 2 + .../registry/tools/CreateDomainCommand.java | 2 + .../GenerateAllocationTokensCommand.java | 7 +- .../registry/tools/RegistryToolComponent.java | 1 - .../registry/tools/RegistryToolModule.java | 67 ------------------- .../registry/tools/SetupOteCommand.java | 5 +- .../registry/util/RandomStringGenerator.java | 5 +- .../google/registry/util/StringGenerator.java | 3 +- .../imports/RdeDomainImportActionTest.java | 17 +++-- .../XjcToDomainResourceConverterTest.java | 12 ++-- 15 files changed, 84 insertions(+), 116 deletions(-) delete mode 100644 java/google/registry/tools/RegistryToolModule.java diff --git a/java/google/registry/config/RegistryConfig.java b/java/google/registry/config/RegistryConfig.java index 5a3e9f01e..726949248 100644 --- a/java/google/registry/config/RegistryConfig.java +++ b/java/google/registry/config/RegistryConfig.java @@ -28,11 +28,15 @@ import com.google.common.net.HostAndPort; import dagger.Module; import dagger.Provides; import google.registry.config.RegistryConfigSettings.AppEngine.ToolsServiceUrl; +import google.registry.util.RandomStringGenerator; +import google.registry.util.StringGenerator; import google.registry.util.TaskQueueUtils; import java.lang.annotation.Documented; import java.lang.annotation.Retention; import java.net.URI; import java.net.URL; +import java.security.NoSuchAlgorithmException; +import java.security.ProviderException; import java.security.SecureRandom; import java.util.Optional; import java.util.Random; @@ -1261,14 +1265,43 @@ public final class RegistryConfig { } /** - * Returns a singleton random number generator. + * Returns a singleton insecure random number generator that is fast. * - * @see google.registry.util.UrlFetchUtils + *

This binding is intentionally qualified so that any requester must explicitly acknowledge + * that using an insecure random number generator is fine for its use case. */ @Singleton @Provides - public static Random provideRandom() { - return new SecureRandom(); + @Config("insecureRandom") + public static Random provideInsecureRandom() { + return new Random(); + }; + + /** Returns a singleton secure random number generator this is slow. */ + @Singleton + @Provides + public static SecureRandom provideSecureRandom() { + try { + return SecureRandom.getInstance("NativePRNG"); + } catch (NoSuchAlgorithmException e) { + throw new ProviderException(e); + } + } + + /** Returns a singleton random string generator using Base58 encoding. */ + @Singleton + @Provides + @Config("base58StringGenerator") + public static StringGenerator provideBase58StringGenerator(SecureRandom secureRandom) { + return new RandomStringGenerator(StringGenerator.Alphabets.BASE_58, secureRandom); + } + + /** Returns a singleton random string generator using Base58 encoding. */ + @Singleton + @Provides + @Config("base64StringGenerator") + public static StringGenerator provideBase64StringGenerator(SecureRandom secureRandom) { + return new RandomStringGenerator(StringGenerator.Alphabets.BASE_64, secureRandom); } } diff --git a/java/google/registry/rde/imports/RdeDomainImportAction.java b/java/google/registry/rde/imports/RdeDomainImportAction.java index 638f65dc9..97043d9d8 100644 --- a/java/google/registry/rde/imports/RdeDomainImportAction.java +++ b/java/google/registry/rde/imports/RdeDomainImportAction.java @@ -54,6 +54,7 @@ import google.registry.request.Action; import google.registry.request.Parameter; import google.registry.request.Response; import google.registry.request.auth.Auth; +import google.registry.util.StringGenerator; import google.registry.util.SystemClock; import google.registry.xjc.JaxbFragment; import google.registry.xjc.rdedomain.XjcRdeDomain; @@ -83,6 +84,7 @@ public class RdeDomainImportAction implements Runnable { protected final String importBucketName; protected final String importFileName; protected final Optional mapShards; + protected final StringGenerator stringGenerator; @Inject public RdeDomainImportAction( @@ -90,12 +92,14 @@ public class RdeDomainImportAction implements Runnable { Response response, @Config("rdeImportBucket") String importBucketName, @Parameter(PATH) String importFileName, - @Parameter(PARAM_MAP_SHARDS) Optional mapShards) { + @Parameter(PARAM_MAP_SHARDS) Optional mapShards, + @Config("base64StringGenerator") StringGenerator stringGenerator) { this.mrRunner = mrRunner; this.response = response; this.importBucketName = importBucketName; this.importFileName = importFileName; this.mapShards = mapShards; + this.stringGenerator = stringGenerator; } @Override @@ -122,7 +126,7 @@ public class RdeDomainImportAction implements Runnable { * Creates a new {@link RdeDomainImportMapper} */ private RdeDomainImportMapper createMapper() { - return new RdeDomainImportMapper(importBucketName); + return new RdeDomainImportMapper(importBucketName, stringGenerator); } /** Mapper to import domains from an escrow file. */ @@ -132,11 +136,13 @@ public class RdeDomainImportAction implements Runnable { private static final long serialVersionUID = -7645091075256589374L; private final String importBucketName; + private final StringGenerator stringGenerator; private transient RdeImportUtils importUtils; private transient DnsQueue dnsQueue; - public RdeDomainImportMapper(String importBucketName) { + public RdeDomainImportMapper(String importBucketName, StringGenerator stringGenerator) { this.importBucketName = importBucketName; + this.stringGenerator = stringGenerator; } private RdeImportUtils getImportUtils() { @@ -196,7 +202,7 @@ public class RdeDomainImportAction implements Runnable { createAutoRenewPollMessageForDomainImport(xjcDomain, historyEntry); DomainResource domain = XjcToDomainResourceConverter.convertDomain( - xjcDomain, autorenewBillingEvent, autorenewPollMessage); + xjcDomain, autorenewBillingEvent, autorenewPollMessage, stringGenerator); getDnsQueue().addDomainRefreshTask(domain.getFullyQualifiedDomainName()); // Keep a list of "extra objects" that need to be saved along with the domain // and add to it if necessary. diff --git a/java/google/registry/rde/imports/XjcToDomainResourceConverter.java b/java/google/registry/rde/imports/XjcToDomainResourceConverter.java index 563dbddae..e8dd07969 100644 --- a/java/google/registry/rde/imports/XjcToDomainResourceConverter.java +++ b/java/google/registry/rde/imports/XjcToDomainResourceConverter.java @@ -42,8 +42,6 @@ import google.registry.model.registry.Registries; import google.registry.model.registry.Registry; import google.registry.model.transfer.TransferData; import google.registry.model.transfer.TransferStatus; -import google.registry.util.NonFinalForTesting; -import google.registry.util.RandomStringGenerator; import google.registry.util.StringGenerator; import google.registry.util.XmlToEnumMapper; import google.registry.xjc.domain.XjcDomainContactType; @@ -54,27 +52,12 @@ import google.registry.xjc.rdedomain.XjcRdeDomainElement; import google.registry.xjc.rdedomain.XjcRdeDomainTransferDataType; import google.registry.xjc.rgp.XjcRgpStatusType; import google.registry.xjc.secdns.XjcSecdnsDsDataType; -import java.security.NoSuchAlgorithmException; -import java.security.ProviderException; -import java.security.SecureRandom; import java.util.function.Function; import org.joda.time.DateTime; /** Utility class that converts an {@link XjcRdeDomainElement} into a {@link DomainResource}. */ final class XjcToDomainResourceConverter extends XjcToEppResourceConverter { - @NonFinalForTesting - static StringGenerator stringGenerator = - new RandomStringGenerator(StringGenerator.Alphabets.BASE_64, getRandom()); - - static SecureRandom getRandom() { - try { - return SecureRandom.getInstance("NativePRNG"); - } catch (NoSuchAlgorithmException e) { - throw new ProviderException(e); - } - } - private static final XmlToEnumMapper TRANSFER_STATUS_MAPPER = XmlToEnumMapper.create(TransferStatus.values()); @@ -152,7 +135,8 @@ final class XjcToDomainResourceConverter extends XjcToEppResourceConverter { static DomainResource convertDomain( XjcRdeDomain domain, BillingEvent.Recurring autoRenewBillingEvent, - PollMessage.Autorenew autoRenewPollMessage) { + PollMessage.Autorenew autoRenewPollMessage, + StringGenerator stringGenerator) { GracePeriodConverter gracePeriodConverter = new GracePeriodConverter(domain, Key.create(autoRenewBillingEvent)); DomainResource.Builder builder = diff --git a/java/google/registry/tmch/NordnUploadAction.java b/java/google/registry/tmch/NordnUploadAction.java index 2fc69c0c2..4632c20bb 100644 --- a/java/google/registry/tmch/NordnUploadAction.java +++ b/java/google/registry/tmch/NordnUploadAction.java @@ -89,7 +89,7 @@ public final class NordnUploadAction implements Runnable { @Inject Clock clock; @Inject Retrier retrier; - @Inject Random random; + @Inject @Config("insecureRandom") Random random; @Inject LordnRequestInitializer lordnRequestInitializer; @Inject URLFetchService fetchService; @Inject @Config("tmchMarksdbUrl") String tmchMarksdbUrl; diff --git a/java/google/registry/tools/CreateAnchorTenantCommand.java b/java/google/registry/tools/CreateAnchorTenantCommand.java index 776a6cbcb..d7ceea2a6 100644 --- a/java/google/registry/tools/CreateAnchorTenantCommand.java +++ b/java/google/registry/tools/CreateAnchorTenantCommand.java @@ -25,6 +25,7 @@ import com.beust.jcommander.Parameter; import com.beust.jcommander.Parameters; import com.google.common.net.InternetDomainName; import com.google.template.soy.data.SoyMapData; +import google.registry.config.RegistryConfig.Config; import google.registry.tools.soy.CreateAnchorTenantSoyInfo; import google.registry.util.StringGenerator; import javax.inject.Inject; @@ -72,6 +73,7 @@ final class CreateAnchorTenantCommand extends MutatingEppToolCommand { private boolean fee; @Inject + @Config("base64StringGenerator") StringGenerator passwordGenerator; @Override diff --git a/java/google/registry/tools/CreateContactCommand.java b/java/google/registry/tools/CreateContactCommand.java index b73f697a0..fbd0f597d 100644 --- a/java/google/registry/tools/CreateContactCommand.java +++ b/java/google/registry/tools/CreateContactCommand.java @@ -20,6 +20,7 @@ import static com.google.common.base.Strings.isNullOrEmpty; import com.beust.jcommander.Parameter; import com.beust.jcommander.Parameters; import com.google.template.soy.data.SoyMapData; +import google.registry.config.RegistryConfig.Config; import google.registry.tools.params.PhoneNumberParameter; import google.registry.tools.soy.ContactCreateSoyInfo; import google.registry.util.StringGenerator; @@ -103,6 +104,7 @@ final class CreateContactCommand extends MutatingEppToolCommand { private String password; @Inject + @Config("base64StringGenerator") StringGenerator passwordGenerator; private static final int PASSWORD_LENGTH = 16; diff --git a/java/google/registry/tools/CreateDomainCommand.java b/java/google/registry/tools/CreateDomainCommand.java index a183cf195..a41e81b4d 100644 --- a/java/google/registry/tools/CreateDomainCommand.java +++ b/java/google/registry/tools/CreateDomainCommand.java @@ -23,6 +23,7 @@ import static org.joda.time.DateTimeZone.UTC; import com.beust.jcommander.Parameter; import com.beust.jcommander.Parameters; import com.google.template.soy.data.SoyMapData; +import google.registry.config.RegistryConfig.Config; import google.registry.model.pricing.PremiumPricingEngine.DomainPrices; import google.registry.tools.soy.DomainCreateSoyInfo; import google.registry.util.StringGenerator; @@ -46,6 +47,7 @@ final class CreateDomainCommand extends CreateOrUpdateDomainCommand private boolean forcePremiums; @Inject + @Config("base64StringGenerator") StringGenerator passwordGenerator; private static final int PASSWORD_LENGTH = 16; diff --git a/java/google/registry/tools/GenerateAllocationTokensCommand.java b/java/google/registry/tools/GenerateAllocationTokensCommand.java index f0cc11927..7274f57fa 100644 --- a/java/google/registry/tools/GenerateAllocationTokensCommand.java +++ b/java/google/registry/tools/GenerateAllocationTokensCommand.java @@ -31,6 +31,7 @@ import com.google.common.base.Splitter; import com.google.common.collect.ImmutableSet; import com.google.common.io.Files; import com.googlecode.objectify.Key; +import google.registry.config.RegistryConfig.Config; import google.registry.model.domain.token.AllocationToken; import google.registry.util.NonFinalForTesting; import google.registry.util.Retrier; @@ -40,7 +41,6 @@ import java.io.IOException; import java.util.Collection; import java.util.Deque; import javax.inject.Inject; -import javax.inject.Named; /** Command to generate and persist {@link AllocationToken}s. */ @Parameters( @@ -80,7 +80,10 @@ class GenerateAllocationTokensCommand implements CommandWithRemoteApi { description = "Do not actually persist the tokens; defaults to false") boolean dryRun; - @Inject @Named("base58StringGenerator") StringGenerator stringGenerator; + @Inject + @Config("base58StringGenerator") + StringGenerator stringGenerator; + @Inject Retrier retrier; private static final int BATCH_SIZE = 20; diff --git a/java/google/registry/tools/RegistryToolComponent.java b/java/google/registry/tools/RegistryToolComponent.java index 42d65f523..56a7c14c8 100644 --- a/java/google/registry/tools/RegistryToolComponent.java +++ b/java/google/registry/tools/RegistryToolComponent.java @@ -64,7 +64,6 @@ import javax.inject.Singleton; KeyringModule.class, KmsModule.class, RdeModule.class, - RegistryToolModule.class, SystemClockModule.class, SystemSleeperModule.class, URLFetchServiceModule.class, diff --git a/java/google/registry/tools/RegistryToolModule.java b/java/google/registry/tools/RegistryToolModule.java deleted file mode 100644 index eb7cefac8..000000000 --- a/java/google/registry/tools/RegistryToolModule.java +++ /dev/null @@ -1,67 +0,0 @@ -// Copyright 2017 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 dagger.Binds; -import dagger.Module; -import dagger.Provides; -import google.registry.util.RandomStringGenerator; -import google.registry.util.StringGenerator; -import google.registry.util.StringGenerator.Alphabets; -import java.security.NoSuchAlgorithmException; -import java.security.ProviderException; -import java.security.SecureRandom; -import javax.inject.Named; - -/** Dagger module for Registry Tool. */ -@Module -abstract class RegistryToolModule { - - @Provides - static RegistryToolEnvironment provideRegistryToolEnvironment() { - return RegistryToolEnvironment.get(); - } - - @Binds - abstract StringGenerator provideStringGenerator(RandomStringGenerator stringGenerator); - - @Provides - static SecureRandom provideSecureRandom() { - try { - return SecureRandom.getInstance("NativePRNG"); - } catch (NoSuchAlgorithmException e) { - throw new ProviderException(e); - } - } - - @Provides - @Named("alphabetBase64") - static String provideAlphabetBase64() { - return Alphabets.BASE_64; - } - - @Provides - @Named("alphabetBase58") - static String provideAlphabetBase58() { - return Alphabets.BASE_58; - } - - @Provides - @Named("base58StringGenerator") - static StringGenerator provideBase58StringGenerator( - @Named("alphabetBase58") String alphabet, SecureRandom random) { - return new RandomStringGenerator(alphabet, random); - } -} diff --git a/java/google/registry/tools/SetupOteCommand.java b/java/google/registry/tools/SetupOteCommand.java index af0d34045..404ef1e88 100644 --- a/java/google/registry/tools/SetupOteCommand.java +++ b/java/google/registry/tools/SetupOteCommand.java @@ -27,6 +27,7 @@ import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSortedMap; import com.google.re2j.Pattern; +import google.registry.config.RegistryConfig.Config; import google.registry.config.RegistryEnvironment; import google.registry.model.common.GaeUserIdConverter; import google.registry.model.registrar.Registrar; @@ -139,7 +140,9 @@ final class SetupOteCommand extends ConfirmingCommand implements CommandWithRemo ) private boolean eapOnly = false; - @Inject StringGenerator passwordGenerator; + @Inject + @Config("base64StringGenerator") + StringGenerator passwordGenerator; /** * Long registrar names are truncated and then have an incrementing digit appended at the end so diff --git a/java/google/registry/util/RandomStringGenerator.java b/java/google/registry/util/RandomStringGenerator.java index 625448d4e..db75519ea 100644 --- a/java/google/registry/util/RandomStringGenerator.java +++ b/java/google/registry/util/RandomStringGenerator.java @@ -17,16 +17,13 @@ package google.registry.util; import static com.google.common.base.Preconditions.checkArgument; import java.security.SecureRandom; -import javax.inject.Inject; -import javax.inject.Named; /** Random string generator. */ public class RandomStringGenerator extends StringGenerator { private final SecureRandom random; - @Inject - public RandomStringGenerator(@Named("alphabetBase64") String alphabet, SecureRandom random) { + public RandomStringGenerator(String alphabet, SecureRandom random) { super(alphabet); this.random = random; } diff --git a/java/google/registry/util/StringGenerator.java b/java/google/registry/util/StringGenerator.java index 9c2aa25bd..bb3be99e8 100644 --- a/java/google/registry/util/StringGenerator.java +++ b/java/google/registry/util/StringGenerator.java @@ -18,10 +18,11 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Strings.isNullOrEmpty; import com.google.common.collect.ImmutableList; +import java.io.Serializable; import java.util.Collection; /** String generator. */ -public abstract class StringGenerator { +public abstract class StringGenerator implements Serializable { public static final int DEFAULT_PASSWORD_LENGTH = 16; diff --git a/javatests/google/registry/rde/imports/RdeDomainImportActionTest.java b/javatests/google/registry/rde/imports/RdeDomainImportActionTest.java index b4ebca5cb..acdfa80e8 100644 --- a/javatests/google/registry/rde/imports/RdeDomainImportActionTest.java +++ b/javatests/google/registry/rde/imports/RdeDomainImportActionTest.java @@ -50,9 +50,12 @@ import google.registry.model.transfer.TransferStatus; import google.registry.request.Response; import google.registry.testing.FakeResponse; import google.registry.testing.mapreduce.MapreduceTestCase; +import google.registry.util.RandomStringGenerator; +import google.registry.util.StringGenerator; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.security.SecureRandom; import java.util.List; import java.util.Optional; import javax.annotation.Nullable; @@ -91,12 +94,14 @@ public class RdeDomainImportActionTest extends MapreduceTestCase { @@ -426,7 +424,7 @@ public class XjcToDomainResourceConverterTest { createAutoRenewPollMessageForDomainImport(xjcDomain, historyEntry); ofy().save().entities(historyEntry, autorenewBillingEvent, autorenewPollMessage); return XjcToDomainResourceConverter.convertDomain( - xjcDomain, autorenewBillingEvent, autorenewPollMessage); + xjcDomain, autorenewBillingEvent, autorenewPollMessage, stringGenerator); }); }