Add certificate checks to RegistrarSettingsAction (#843)

* Add certificate checks to RegistrarSettingsAction

* Add some comments

* Add more functionality to CertificateChecker and update call sites

* Small code cleanups

* Small format fix

core/src/main/java/google/registry/module/frontend/FrontendComponent.java

@@ -17,6 +17,7 @@ package google.registry.module.frontend;
 import com.google.monitoring.metrics.MetricReporter;
 import dagger.Component;
 import dagger.Lazy;
+import google.registry.config.CertificateCheckerModule;
 import google.registry.config.CredentialModule;
 import google.registry.config.RegistryConfig.ConfigModule;
 import google.registry.flows.ServerTridProviderModule;
@@ -44,6 +45,7 @@ import javax.inject.Singleton;
 @Component(
     modules = {
       AuthModule.class,
+      CertificateCheckerModule.class,
       ConfigModule.class,
       ConsoleConfigModule.class,
       CredentialModule.class,

core/src/main/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java

@@ -22,7 +22,6 @@ import static google.registry.persistence.transaction.TransactionManagerFactory.
 import static google.registry.util.DomainNameUtils.canonicalizeDomainName;
 import static google.registry.util.RegistrarUtils.normalizeRegistrarName;
 import static java.nio.charset.StandardCharsets.US_ASCII;
-import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.joda.time.DateTimeZone.UTC;
 
 import com.beust.jcommander.Parameter;
@@ -40,14 +39,9 @@ import google.registry.tools.params.OptionalPhoneNumberParameter;
 import google.registry.tools.params.OptionalStringParameter;
 import google.registry.tools.params.PathParameter;
 import google.registry.util.CertificateChecker;
-import google.registry.util.CertificateChecker.CertificateViolation;
 import google.registry.util.CidrAddressBlock;
-import java.io.ByteArrayInputStream;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.security.cert.CertificateException;
-import java.security.cert.CertificateFactory;
-import java.security.cert.X509Certificate;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.LinkedHashMap;
@@ -55,7 +49,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
-import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 import javax.inject.Inject;
 import org.joda.money.CurrencyUnit;
@@ -369,7 +362,7 @@ abstract class CreateOrUpdateRegistrarCommand extends MutatingCommand {
         // existing certificate without providing a replacement. An uploaded empty certificate file
         // will prevent the registrar from being able to establish EPP connections.
         if (!asciiCert.equals("")) {
-          verifyCertificate(asciiCert);
+          certificateChecker.validateCertificate(asciiCert);
         }
         builder.setClientCertificate(asciiCert, now);
       }
@@ -378,7 +371,7 @@ abstract class CreateOrUpdateRegistrarCommand extends MutatingCommand {
         String asciiCert =
             new String(Files.readAllBytes(failoverClientCertificateFilename), US_ASCII);
         if (!asciiCert.equals("")) {
-          verifyCertificate(asciiCert);
+          certificateChecker.validateCertificate(asciiCert);
         }
         builder.setFailoverClientCertificate(asciiCert, now);
       }
@@ -482,22 +475,6 @@ abstract class CreateOrUpdateRegistrarCommand extends MutatingCommand {
     }
   }
 
-  private void verifyCertificate(String certificateString) throws CertificateException {
-    X509Certificate certificate =
-        (X509Certificate)
-            CertificateFactory.getInstance("X509")
-                .generateCertificate(new ByteArrayInputStream(certificateString.getBytes(UTF_8)));
-    ImmutableSet<CertificateViolation> violations =
-        certificateChecker.checkCertificate(certificate);
-    if (!violations.isEmpty()) {
-      String displayMessages =
-          violations.stream()
-              .map(violation -> violation.getDisplayMessage(certificateChecker))
-              .collect(Collectors.joining("\n"));
-      throw new CertificateException(displayMessages);
-    }
-  }
-
   @Override
   protected String execute() throws Exception {
     // Save registrar to Datastore and output its response

core/src/main/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java

@@ -56,6 +56,7 @@ import google.registry.ui.forms.FormFieldException;
 import google.registry.ui.server.RegistrarFormFields;
 import google.registry.ui.server.SendEmailUtils;
 import google.registry.util.AppEngineServiceUtils;
+import google.registry.util.CertificateChecker;
 import google.registry.util.CollectionUtils;
 import google.registry.util.DiffUtils;
 import java.util.HashSet;
@@ -92,6 +93,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
   @Inject SendEmailUtils sendEmailUtils;
   @Inject AuthenticatedRegistrarAccessor registrarAccessor;
   @Inject AuthResult authResult;
+  @Inject CertificateChecker certificateChecker;
 
   @Inject RegistrarSettingsAction() {}
 
@@ -306,19 +308,43 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
         RegistrarFormFields.IP_ADDRESS_ALLOW_LIST_FIELD
             .extractUntyped(args)
             .orElse(ImmutableList.of()));
-    RegistrarFormFields.CLIENT_CERTIFICATE_FIELD
-        .extractUntyped(args)
-        .ifPresent(
-            certificate -> builder.setClientCertificate(certificate, tm().getTransactionTime()));
-    RegistrarFormFields.FAILOVER_CLIENT_CERTIFICATE_FIELD
-        .extractUntyped(args)
-        .ifPresent(
-            certificate ->
-                builder.setFailoverClientCertificate(certificate, tm().getTransactionTime()));
+
+    Optional<String> certificateString =
+        RegistrarFormFields.CLIENT_CERTIFICATE_FIELD.extractUntyped(args);
+    if (certificateString.isPresent()) {
+      if (validateCertificate(initialRegistrar.getClientCertificate(), certificateString.get())) {
+        builder.setClientCertificate(certificateString.get(), tm().getTransactionTime());
+      }
+    }
+
+    Optional<String> failoverCertificateString =
+        RegistrarFormFields.FAILOVER_CLIENT_CERTIFICATE_FIELD.extractUntyped(args);
+    if (failoverCertificateString.isPresent()) {
+      if (validateCertificate(
+          initialRegistrar.getFailoverClientCertificate(), failoverCertificateString.get())) {
+        builder.setFailoverClientCertificate(
+            failoverCertificateString.get(), tm().getTransactionTime());
+      }
+    }
 
     return checkNotChangedUnlessAllowed(builder, initialRegistrar, Role.OWNER);
   }
 
+  /**
+   * Returns true if the registrar should accept the new certificate. Returns false if the
+   * certificate is already the one stored for the registrar.
+   */
+  private boolean validateCertificate(String existingCertificate, String certificateString) {
+    if ((existingCertificate == null) || !existingCertificate.equals(certificateString)) {
+      // TODO(sarhabot): remove this check after November 1, 2020
+      if (tm().getTransactionTime().isAfter(DateTime.parse("2020-11-01T00:00:00Z"))) {
+        certificateChecker.validateCertificate(certificateString);
+      }
+      return true;
+    }
+    return false;
+  }
+
   /**
    * Updates a registrar with the ADMIN-controlled args from the http request.
    *