Require SSL certificate hash on login by default

Note that it's possible to set a config option to disable this functionality
on a per-environment basis (we're disabling it for sandbox), but in general
SSL certificate hashes should be required for increased security.

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

java/google/registry/config/RegistryConfig.java

@@ -573,6 +573,17 @@ public final class RegistryConfig {
       return beamBucketUrl + "/templates/spec11";
     }
 
+    /**
+     * Returns whether an SSL certificate hash is required to log in via EPP and run flows.
+     *
+     * @see google.registry.flows.TlsCredentials
+     */
+    @Provides
+    @Config("requireSslCertificates")
+    public static boolean provideRequireSslCertificates(RegistryConfigSettings config) {
+      return config.registryPolicy.requireSslCertificates;
+    }
+
     /**
      * Returns the default job zone to run Apache Beam (Cloud Dataflow) jobs in.
      *

java/google/registry/config/RegistryConfigSettings.java

@@ -92,6 +92,7 @@ public class RegistryConfigSettings {
     public String rdapTos;
     public String rdapTosStaticUrl;
     public String spec11EmailBodyTemplate;
+    public boolean requireSslCertificates;
   }
 
   /** Configuration for Cloud Datastore. */

java/google/registry/config/files/default-config.yaml

@@ -183,6 +183,11 @@ registryPolicy:
     If you have any questions regarding this notice, please contact
     {REPLY_TO_EMAIL}.
 
+  # Whether to require an SSL certificate hash in order to be able to log in
+  # via EPP and run commands. This can be false for testing environments but
+  # should generally be true for production environments, for added security.
+  requireSslCertificates: true
+
 datastore:
   # Number of commit log buckets in Datastore. Lowering this after initial
   # install risks losing up to a days' worth of differential backups.

java/google/registry/flows/TlsCredentials.java

@@ -26,6 +26,7 @@ import com.google.common.net.HostAndPort;
 import com.google.common.net.InetAddresses;
 import dagger.Module;
 import dagger.Provides;
+import google.registry.config.RegistryConfig.Config;
 import google.registry.flows.EppException.AuthenticationErrorException;
 import google.registry.model.registrar.Registrar;
 import google.registry.request.Header;
@@ -54,14 +55,17 @@ public class TlsCredentials implements TransportCredentials {
 
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
+  private final boolean requireSslCertificates;
   private final String clientCertificateHash;
   private final InetAddress clientInetAddr;
 
   @Inject
   @VisibleForTesting
   public TlsCredentials(
+      @Config("requireSslCertificates") boolean requireSslCertificates,
       @Header("X-SSL-Certificate") String clientCertificateHash,
       @Header("X-Forwarded-For") Optional<String> clientAddress) {
+    this.requireSslCertificates = requireSslCertificates;
     this.clientCertificateHash = clientCertificateHash;
     this.clientInetAddr = clientAddress.isPresent() ? parseInetAddress(clientAddress.get()) : null;
   }
@@ -112,14 +116,18 @@ public class TlsCredentials implements TransportCredentials {
    * @throws MissingRegistrarCertificateException if frontend didn't send certificate hash header
    * @throws BadRegistrarCertificateException if registrar requires certificate and it didn't match
    */
-  private void validateCertificate(Registrar registrar) throws AuthenticationErrorException {
+  @VisibleForTesting
+  void validateCertificate(Registrar registrar) throws AuthenticationErrorException {
     if (isNullOrEmpty(registrar.getClientCertificateHash())
         && isNullOrEmpty(registrar.getFailoverClientCertificateHash())) {
-      logger.atInfo().log(
+      if (requireSslCertificates) {
-          "Skipping SSL certificate check because %s doesn't have any certificate hashes on file",
+        throw new RegistrarCertificateNotConfiguredException();
-          registrar.getClientId());
+      } else {
+        // If the environment is configured to allow missing SSL certificate hashes and this hash is
+        // missing, then bypass the certificate hash checks.
         return;
       }
+    }
     if (isNullOrEmpty(clientCertificateHash)) {
       logger.atInfo().log("Request did not include X-SSL-Certificate");
       throw new MissingRegistrarCertificateException();
@@ -165,6 +173,14 @@ public class TlsCredentials implements TransportCredentials {
     }
   }
 
+  /** Registrar certificate is not configured. */
+  public static class RegistrarCertificateNotConfiguredException
+      extends AuthenticationErrorException {
+    public RegistrarCertificateNotConfiguredException() {
+      super("Registrar certificate is not configured");
+    }
+  }
+
   /** Registrar IP address is not in stored whitelist. */
   public static class BadRegistrarIpAddressException extends AuthenticationErrorException {
     public BadRegistrarIpAddressException() {

java/google/registry/tools/ValidateLoginCredentialsCommand.java

@@ -78,7 +78,7 @@ final class ValidateLoginCredentialsCommand implements CommandWithRemoteApi {
     Registrar registrar =
         checkArgumentPresent(
             Registrar.loadByClientId(clientId), "Registrar %s not found", clientId);
-    new TlsCredentials(clientCertificateHash, Optional.of(clientIpAddress))
+    new TlsCredentials(true, clientCertificateHash, Optional.of(clientIpAddress))
         .validate(registrar, password);
     checkState(!registrar.getState().equals(Registrar.State.PENDING), "Account pending");
   }

javatests/google/registry/flows/EppLoginTlsTest.java

@@ -33,15 +33,11 @@ import org.junit.runners.JUnit4;
 @RunWith(JUnit4.class)
 public class EppLoginTlsTest extends EppTestCase {
 
-  @Rule
+  @Rule public final AppEngineRule appEngine = AppEngineRule.builder().withDatastore().build();
-  public final AppEngineRule appEngine = AppEngineRule.builder()
-      .withDatastore()
-      .build();
-
 
   void setClientCertificateHash(String clientCertificateHash) {
     setTransportCredentials(
-        new TlsCredentials(clientCertificateHash, Optional.of("192.168.1.100:54321")));
+        new TlsCredentials(true, clientCertificateHash, Optional.of("192.168.1.100:54321")));
   }
 
   @Before
@@ -160,7 +156,7 @@ public class EppLoginTlsTest extends EppTestCase {
   }
 
   @Test
-  public void testRegistrarHasNoCertificatesOnFile_disablesCertChecking() throws Exception {
+  public void testRegistrarHasNoCertificatesOnFile_fails() throws Exception {
     setClientCertificateHash("laffo");
     DateTime now = DateTime.now(UTC);
     persistResource(
@@ -169,6 +165,9 @@ public class EppLoginTlsTest extends EppTestCase {
             .setClientCertificate(null, now)
             .setFailoverClientCertificate(null, now)
             .build());
-    assertThatLoginSucceeds("NewRegistrar", "foo-BAR2");
+    assertThatLogin("NewRegistrar", "foo-BAR2")
+        .hasResponse(
+            "response_error.xml",
+            ImmutableMap.of("CODE", "2200", "MSG", "Registrar certificate is not configured"));
   }
 }

javatests/google/registry/flows/FlowRunnerTest.java

@@ -152,7 +152,7 @@ public class FlowRunnerTest extends ShardableTestCase {
 
   @Test
   public void testRun_loggingStatement_tlsCredentials() throws Exception {
-    flowRunner.credentials = new TlsCredentials("abc123def", Optional.of("127.0.0.1"));
+    flowRunner.credentials = new TlsCredentials(true, "abc123def", Optional.of("127.0.0.1"));
     flowRunner.run(eppMetricBuilder);
     assertThat(Splitter.on("\n\t").split(findFirstLogMessageByPrefix(handler, "EPP Command\n\t")))
         .contains("TlsCredentials{clientCertificateHash=abc123def, clientAddress=/127.0.0.1}");

javatests/google/registry/flows/TlsCredentialsTest.java

@@ -15,19 +15,31 @@
 package google.registry.flows;
 
 import static com.google.common.truth.Truth.assertThat;
+import static google.registry.testing.DatastoreHelper.loadRegistrar;
+import static google.registry.testing.DatastoreHelper.persistResource;
 import static google.registry.testing.JUnitBackports.assertThrows;
+import static org.joda.time.DateTimeZone.UTC;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
+import google.registry.model.registrar.Registrar;
 import google.registry.request.HttpException.BadRequestException;
+import google.registry.testing.AppEngineRule;
+import google.registry.testing.ShardableTestCase;
+import java.util.Optional;
 import javax.servlet.http.HttpServletRequest;
+import org.joda.time.DateTime;
+import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
 /** Unit tests for {@link TlsCredentials}. */
 @RunWith(JUnit4.class)
-public final class TlsCredentialsTest {
+public final class TlsCredentialsTest extends ShardableTestCase {
+
+  @Rule public final AppEngineRule appEngine = AppEngineRule.builder().withDatastore().build();
+
   @Test
   public void testProvideClientCertificateHash() {
     HttpServletRequest req = mock(HttpServletRequest.class);
@@ -46,8 +58,15 @@ public final class TlsCredentialsTest {
   }
 
   @Test
-  public void testNothing1() {}
+  public void test_validateCertificate_canBeConfiguredToBypassCertHashes() throws Exception {
-
+    TlsCredentials tls = new TlsCredentials(false, "certHash", Optional.of("192.168.1.1"));
-  @Test
+    persistResource(
-  public void testNothing2() {}
+        loadRegistrar("TheRegistrar")
+            .asBuilder()
+            .setClientCertificate(null, DateTime.now(UTC))
+            .setFailoverClientCertificate(null, DateTime.now(UTC))
+            .build());
+    // This would throw a RegistrarCertificateNotConfiguredException if cert hashes wren't bypassed.
+    tls.validateCertificate(Registrar.loadByClientId("TheRegistrar").get());
+  }
 }

javatests/google/registry/flows/session/LoginFlowViaTlsTest.java

@@ -49,7 +49,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
   @Test
   public void testSuccess_withGoodCredentials() throws Exception {
     persistResource(getRegistrarBuilder().build());
-    credentials = new TlsCredentials(GOOD_CERT, GOOD_IP);
+    credentials = new TlsCredentials(true, GOOD_CERT, GOOD_IP);
     doSuccessfulTest("login_valid.xml");
   }
 
@@ -60,7 +60,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
             .setIpAddressWhitelist(ImmutableList.of(
                 CidrAddressBlock.create("2001:db8:0:0:0:0:1:1/32")))
             .build());
-    credentials = new TlsCredentials(GOOD_CERT, GOOD_IPV6);
+    credentials = new TlsCredentials(true, GOOD_CERT, GOOD_IPV6);
     doSuccessfulTest("login_valid.xml");
   }
 
@@ -71,7 +71,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
             .setIpAddressWhitelist(ImmutableList.of(
                 CidrAddressBlock.create("2001:db8:0:0:0:0:1:1/32")))
             .build());
-    credentials = new TlsCredentials(GOOD_CERT, GOOD_IPV6);
+    credentials = new TlsCredentials(true, GOOD_CERT, GOOD_IPV6);
     doSuccessfulTest("login_valid.xml");
   }
 
@@ -82,21 +82,21 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
             .setIpAddressWhitelist(ImmutableList.of(
                 CidrAddressBlock.create("192.168.1.255/24")))
             .build());
-    credentials = new TlsCredentials(GOOD_CERT, GOOD_IP);
+    credentials = new TlsCredentials(true, GOOD_CERT, GOOD_IP);
     doSuccessfulTest("login_valid.xml");
   }
 
   @Test
   public void testFailure_incorrectClientCertificateHash() {
     persistResource(getRegistrarBuilder().build());
-    credentials = new TlsCredentials(BAD_CERT, GOOD_IP);
+    credentials = new TlsCredentials(true, BAD_CERT, GOOD_IP);
     doFailingTest("login_valid.xml", BadRegistrarCertificateException.class);
   }
 
   @Test
   public void testFailure_missingClientCertificateHash() {
     persistResource(getRegistrarBuilder().build());
-    credentials = new TlsCredentials(null, GOOD_IP);
+    credentials = new TlsCredentials(true, null, GOOD_IP);
     doFailingTest("login_valid.xml", MissingRegistrarCertificateException.class);
   }
 
@@ -108,7 +108,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
                 CidrAddressBlock.create(InetAddresses.forString("192.168.1.1"), 32),
                 CidrAddressBlock.create(InetAddresses.forString("2001:db8::1"), 128)))
             .build());
-    credentials = new TlsCredentials(GOOD_CERT, Optional.empty());
+    credentials = new TlsCredentials(true, GOOD_CERT, Optional.empty());
     doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class);
   }
 
@@ -120,7 +120,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
                 CidrAddressBlock.create(InetAddresses.forString("192.168.1.1"), 32),
                 CidrAddressBlock.create(InetAddresses.forString("2001:db8::1"), 128)))
             .build());
-    credentials = new TlsCredentials(GOOD_CERT, BAD_IP);
+    credentials = new TlsCredentials(true, GOOD_CERT, BAD_IP);
     doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class);
   }
 
@@ -132,7 +132,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
                 CidrAddressBlock.create(InetAddresses.forString("192.168.1.1"), 32),
                 CidrAddressBlock.create(InetAddresses.forString("2001:db8::1"), 128)))
             .build());
-    credentials = new TlsCredentials(GOOD_CERT, BAD_IPV6);
+    credentials = new TlsCredentials(true, GOOD_CERT, BAD_IPV6);
     doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class);
   }
 }