Return to using hash for login validation (#1084)

* Return to using hash for login validation

This PR also removes the start date for certificate enforcement.

* Inline verify certificate compliance

core/src/main/java/google/registry/flows/TlsCredentials.java

@@ -16,7 +16,6 @@ package google.registry.flows;
 
 import static com.google.common.base.MoreObjects.toStringHelper;
 import static google.registry.request.RequestParameters.extractOptionalHeader;
-import static google.registry.util.X509Utils.loadCertificate;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
@@ -26,24 +25,17 @@ import com.google.common.net.InetAddresses;
 import dagger.Module;
 import dagger.Provides;
 import google.registry.config.RegistryConfig.Config;
-import google.registry.config.RegistryEnvironment;
 import google.registry.flows.EppException.AuthenticationErrorException;
 import google.registry.flows.certs.CertificateChecker;
 import google.registry.flows.certs.CertificateChecker.InsecureCertificateException;
 import google.registry.model.registrar.Registrar;
 import google.registry.request.Header;
 import google.registry.util.CidrAddressBlock;
-import google.registry.util.Clock;
 import google.registry.util.ProxyHttpHeaders;
-import java.io.ByteArrayInputStream;
 import java.net.InetAddress;
-import java.security.cert.CertificateException;
-import java.security.cert.X509Certificate;
-import java.util.Base64;
 import java.util.Optional;
 import javax.inject.Inject;
 import javax.servlet.http.HttpServletRequest;
-import org.joda.time.DateTime;
 
 /**
  * Container and validation for TLS certificate and IP-allow-listing.
@@ -54,10 +46,6 @@ import org.joda.time.DateTime;
  *   <dt>X-SSL-Certificate
  *   <dd>This field should contain a base64 encoded digest of the client's TLS certificate. It is
  *       used only if the validation of the full certificate fails.
- *   <dt>X-SSL-Full-Certificate
- *   <dd>This field should contain a base64 encoding of the client's TLS certificate. It is
- *       validated during an EPP login command against a known good value that is transmitted out of
- *       band.
  *   <dt>X-Forwarded-For
  *   <dd>This field should contain the host and port of the connecting client. It is validated
  *       during an EPP login command against an IP allow list that is transmitted out of band.
@@ -66,30 +54,22 @@ import org.joda.time.DateTime;
 public class TlsCredentials implements TransportCredentials {
 
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-  private static final DateTime CERT_ENFORCEMENT_START_TIME =
-      DateTime.parse("2021-03-01T16:00:00Z");
 
   private final boolean requireSslCertificates;
   private final Optional<String> clientCertificateHash;
-  private final Optional<String> clientCertificate;
   private final Optional<InetAddress> clientInetAddr;
   private final CertificateChecker certificateChecker;
-  private final Clock clock;
 
   @Inject
   public TlsCredentials(
       @Config("requireSslCertificates") boolean requireSslCertificates,
       @Header(ProxyHttpHeaders.CERTIFICATE_HASH) Optional<String> clientCertificateHash,
-      @Header(ProxyHttpHeaders.FULL_CERTIFICATE) Optional<String> clientCertificate,
       @Header(ProxyHttpHeaders.IP_ADDRESS) Optional<String> clientAddress,
-      CertificateChecker certificateChecker,
+      CertificateChecker certificateChecker) {
-      Clock clock) {
     this.requireSslCertificates = requireSslCertificates;
     this.clientCertificateHash = clientCertificateHash;
-    this.clientCertificate = clientCertificate;
     this.clientInetAddr = clientAddress.map(TlsCredentials::parseInetAddress);
     this.certificateChecker = certificateChecker;
-    this.clock = clock;
   }
 
   static InetAddress parseInetAddress(String asciiAddr) {
@@ -103,7 +83,7 @@ public class TlsCredentials implements TransportCredentials {
   @Override
   public void validate(Registrar registrar, String password) throws AuthenticationErrorException {
     validateIp(registrar);
-    validateCertificate(registrar);
+    validateCertificateHash(registrar);
     validatePassword(registrar, password);
   }
 
@@ -137,89 +117,8 @@ public class TlsCredentials implements TransportCredentials {
     throw new BadRegistrarIpAddressException();
   }
 
-  /**
-   * Verifies client SSL certificate is permitted to issue commands as {@code registrar}.
-   *
-   * @throws MissingRegistrarCertificateException if frontend didn't send certificate header
-   * @throws BadRegistrarCertificateException if registrar requires certificate and it didn't match
-   */
   @VisibleForTesting
-  void validateCertificate(Registrar registrar) throws AuthenticationErrorException {
+  void validateCertificateHash(Registrar registrar) throws AuthenticationErrorException {
-    // Check that certificate is present in registrar object
-    if (!registrar.getClientCertificate().isPresent()
-        && !registrar.getFailoverClientCertificate().isPresent()) {
-      // Log an error and validate using certificate hash instead
-      // TODO(sarahbot): throw a RegistrarCertificateNotConfiguredException once hash is no longer
-      // used as failover
-      logger.atWarning().log(
-          "There is no certificate configured for registrar %s.", registrar.getClientId());
-    } else if (!clientCertificate.isPresent()) {
-      // Check that the request included the full certificate
-      // Log an error and validate using certificate hash instead
-      // TODO(sarahbot): throw a MissingRegistrarCertificateException once hash is no longer used as
-      // failover
-      logger.atWarning().log(
-          "Request from registrar %s did not include X-SSL-Full-Certificate.",
-          registrar.getClientId());
-    } else {
-      X509Certificate passedCert;
-      Optional<X509Certificate> storedCert;
-      Optional<X509Certificate> storedFailoverCert;
-
-      try {
-        storedCert = deserializePemCert(registrar.getClientCertificate());
-        storedFailoverCert = deserializePemCert(registrar.getFailoverClientCertificate());
-        passedCert = decodeCertString(clientCertificate.get());
-      } catch (Exception e) {
-        // TODO(Sarahbot@): remove this catch once we know it's working
-        logger.atWarning().log(
-            "Error converting certificate string to certificate for %s: %s",
-            registrar.getClientId(), e);
-        validateCertificateHash(registrar);
-        return;
-      }
-
-      // Check if the certificate is equal to the one on file for the registrar.
-      if (passedCert.equals(storedCert.orElse(null))
-          || passedCert.equals(storedFailoverCert.orElse(null))) {
-        // Check certificate for any requirement violations
-        // TODO(Sarahbot@): Throw exceptions instead of just logging once requirement enforcement
-        // begins
-        try {
-          certificateChecker.validateCertificate(passedCert);
-        } catch (InsecureCertificateException e) {
-          // TODO(Sarahbot@): Remove this if statement after March 1. After March 1, exception
-          // should be thrown in all environments.
-          // throw exception in unit tests and Sandbox
-          if (RegistryEnvironment.get().equals(RegistryEnvironment.UNITTEST)
-              || RegistryEnvironment.get().equals(RegistryEnvironment.SANDBOX)
-              || clock.nowUtc().isAfter(CERT_ENFORCEMENT_START_TIME)) {
-            throw new CertificateContainsSecurityViolationsException(e);
-          }
-          logger.atWarning().log(
-              "Registrar certificate used for %s does not meet certificate requirements: %s",
-              registrar.getClientId(), e.getMessage());
-        } catch (Exception e) {
-          logger.atWarning().log(
-              "Error validating certificate for %s: %s", registrar.getClientId(), e);
-        }
-        // successfully validated, return here since hash validation is not necessary
-        return;
-      }
-      // Log an error and validate using certificate hash instead
-      // TODO(sarahbot): throw a BadRegistrarCertificateException once hash is no longer used as
-      // failover
-      logger.atWarning().log("Non-matching certificate for registrar %s.", registrar.getClientId());
-    }
-    validateCertificateHash(registrar);
-  }
-
-  private void validateCertificateHash(Registrar registrar) throws AuthenticationErrorException {
-    logger.atWarning().log(
-        "Error validating certificate for %s, attempting to validate using certificate hash.",
-        registrar.getClientId());
-    // Check the certificate hash as a failover
-    // TODO(sarahbot): Remove hash checks once certificate checks are working.
     if (!registrar.getClientCertificateHash().isPresent()
         && !registrar.getFailoverClientCertificateHash().isPresent()) {
       if (requireSslCertificates) {
@@ -247,6 +146,20 @@ public class TlsCredentials implements TransportCredentials {
           registrar.getFailoverClientCertificateHash());
       throw new BadRegistrarCertificateException();
     }
+    if (requireSslCertificates) {
+      String passedCert =
+          clientCertificateHash.equals(registrar.getClientCertificateHash())
+              ? registrar.getClientCertificate().get()
+              : registrar.getFailoverClientCertificate().get();
+      try {
+        certificateChecker.validateCertificate(passedCert);
+      } catch (InsecureCertificateException e) {
+        logger.atWarning().log(
+            "Registrar certificate used for %s does not meet certificate requirements: %s",
+            registrar.getClientId(), e.getMessage());
+        throw new CertificateContainsSecurityViolationsException(e);
+      }
+    }
   }
 
   private void validatePassword(Registrar registrar, String password)
@@ -256,26 +169,9 @@ public class TlsCredentials implements TransportCredentials {
     }
   }
 
-  // Converts a PEM formatted certificate string into an X509Certificate
-  private Optional<X509Certificate> deserializePemCert(Optional<String> certificateString)
-      throws CertificateException {
-    if (certificateString.isPresent()) {
-      return Optional.of(loadCertificate(certificateString.get()));
-    }
-    return Optional.empty();
-  }
-
-  // Decodes the string representation of an encoded certificate back into an X509Certificate
-  private X509Certificate decodeCertString(String encodedCertString) throws CertificateException {
-    byte decodedCert[] = Base64.getDecoder().decode(encodedCertString);
-    ByteArrayInputStream inputStream = new ByteArrayInputStream(decodedCert);
-    return loadCertificate(inputStream);
-  }
-
   @Override
   public String toString() {
     return toStringHelper(getClass())
-        .add("clientCertificate", clientCertificate.orElse(null))
         .add("clientCertificateHash", clientCertificateHash.orElse(null))
         .add("clientAddress", clientInetAddr.orElse(null))
         .toString();
@@ -336,14 +232,6 @@ public class TlsCredentials implements TransportCredentials {
       return extractOptionalHeader(req, ProxyHttpHeaders.CERTIFICATE_HASH);
     }
 
-    @Provides
-    @Header(ProxyHttpHeaders.FULL_CERTIFICATE)
-    static Optional<String> provideClientCertificate(HttpServletRequest req) {
-      // Note: This header is actually required, we just want to handle its absence explicitly
-      // by throwing an EPP exception rather than a generic Bad Request exception.
-      return extractOptionalHeader(req, ProxyHttpHeaders.FULL_CERTIFICATE);
-    }
-
     @Provides
     @Header(ProxyHttpHeaders.IP_ADDRESS)
     static Optional<String> provideIpAddress(HttpServletRequest req) {

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

@@ -18,10 +18,8 @@ import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.base.Strings.isNullOrEmpty;
 import static google.registry.util.PreconditionsUtils.checkArgumentPresent;
-import static google.registry.util.X509Utils.encodeX509CertificateFromPemString;
 import static google.registry.util.X509Utils.getCertificateHash;
 import static google.registry.util.X509Utils.loadCertificate;
-import static java.nio.charset.StandardCharsets.US_ASCII;
 
 import com.beust.jcommander.Parameter;
 import com.beust.jcommander.Parameters;
@@ -30,7 +28,6 @@ import google.registry.flows.certs.CertificateChecker;
 import google.registry.model.registrar.Registrar;
 import google.registry.tools.params.PathParameter;
 import google.registry.util.Clock;
-import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.Optional;
 import javax.annotation.Nullable;
@@ -80,10 +77,7 @@ final class ValidateLoginCredentialsCommand implements CommandWithRemoteApi {
     checkArgument(
         clientCertificatePath == null || isNullOrEmpty(clientCertificateHash),
         "Can't specify both --cert_hash and --cert_file");
-    String encodedCertificate = "";
     if (clientCertificatePath != null) {
-      String certificateString = new String(Files.readAllBytes(clientCertificatePath), US_ASCII);
-      encodedCertificate = encodeX509CertificateFromPemString(certificateString);
       clientCertificateHash = getCertificateHash(loadCertificate(clientCertificatePath));
     }
     Registrar registrar =
@@ -92,10 +86,8 @@ final class ValidateLoginCredentialsCommand implements CommandWithRemoteApi {
     new TlsCredentials(
             true,
             Optional.ofNullable(clientCertificateHash),
-            Optional.ofNullable(encodedCertificate),
             Optional.ofNullable(clientIpAddress),
-            certificateChecker,
+            certificateChecker)
-            clock)
         .validate(registrar, password);
     checkState(
         registrar.isLive(), "Registrar %s has non-live state: %s", clientId, registrar.getState());

core/src/test/java/google/registry/flows/EppLoginTlsTest.java

@@ -14,30 +14,24 @@
 
 package google.registry.flows;
 
+import static google.registry.testing.CertificateSamples.SAMPLE_CERT3_HASH;
 import static google.registry.testing.DatabaseHelper.loadRegistrar;
 import static google.registry.testing.DatabaseHelper.persistResource;
-import static google.registry.testing.LogsSubject.assertAboutLogs;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
-import static google.registry.util.X509Utils.encodeX509Certificate;
+import static google.registry.util.X509Utils.getCertificateHash;
-import static google.registry.util.X509Utils.encodeX509CertificateFromPemString;
 import static org.joda.time.DateTimeZone.UTC;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedMap;
-import com.google.common.testing.TestLogHandler;
-import google.registry.config.RegistryEnvironment;
 import google.registry.flows.certs.CertificateChecker;
 import google.registry.testing.AppEngineExtension;
 import google.registry.testing.CertificateSamples;
 import google.registry.testing.SystemPropertyExtension;
 import google.registry.util.SelfSignedCaCertificate;
 import java.io.StringWriter;
-import java.security.cert.CertificateException;
 import java.security.cert.X509Certificate;
 import java.util.Optional;
-import java.util.logging.Level;
-import java.util.logging.Logger;
 import org.joda.time.DateTime;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Order;
@@ -66,25 +60,17 @@ class EppLoginTlsTest extends EppTestCase {
           ImmutableSet.of("secp256r1", "secp384r1"),
           clock);
 
-  private final Logger loggerToIntercept =
+  void setCredentials(String clientCertificateHash) {
-      Logger.getLogger(TlsCredentials.class.getCanonicalName());
-  private final TestLogHandler handler = new TestLogHandler();
-
-  private String encodedCertString;
-
-  void setCredentials(String clientCertificateHash, String clientCertificate) {
     setTransportCredentials(
         new TlsCredentials(
             true,
             Optional.ofNullable(clientCertificateHash),
-            Optional.ofNullable(clientCertificate),
             Optional.of("192.168.1.100:54321"),
-            certificateChecker,
+            certificateChecker));
-            clock));
   }
 
   @BeforeEach
-  void beforeEach() throws CertificateException {
+  void beforeEach() {
     persistResource(
         loadRegistrar("NewRegistrar")
             .asBuilder()
@@ -96,20 +82,18 @@ class EppLoginTlsTest extends EppTestCase {
             .asBuilder()
             .setClientCertificate(CertificateSamples.SAMPLE_CERT2, DateTime.now(UTC))
             .build());
-    loggerToIntercept.addHandler(handler);
-    encodedCertString = encodeX509CertificateFromPemString(CertificateSamples.SAMPLE_CERT3);
   }
 
   @Test
   void testLoginLogout() throws Exception {
-    setCredentials(null, encodedCertString);
+    setCredentials(SAMPLE_CERT3_HASH);
     assertThatLoginSucceeds("NewRegistrar", "foo-BAR2");
     assertThatLogoutSucceeds();
   }
 
   @Test
   void testLogin_wrongPasswordFails() throws Exception {
-    setCredentials(CertificateSamples.SAMPLE_CERT3_HASH, encodedCertString);
+    setCredentials(SAMPLE_CERT3_HASH);
     // For TLS login, we also check the epp xml password.
     assertThatLogin("NewRegistrar", "incorrect")
         .hasResponse(
@@ -119,7 +103,7 @@ class EppLoginTlsTest extends EppTestCase {
 
   @Test
   void testMultiLogin() throws Exception {
-    setCredentials(CertificateSamples.SAMPLE_CERT3_HASH, encodedCertString);
+    setCredentials(SAMPLE_CERT3_HASH);
     assertThatLoginSucceeds("NewRegistrar", "foo-BAR2");
     assertThatLogoutSucceeds();
     assertThatLoginSucceeds("NewRegistrar", "foo-BAR2");
@@ -133,7 +117,7 @@ class EppLoginTlsTest extends EppTestCase {
 
   @Test
   void testNonAuthedLogin_fails() throws Exception {
-    setCredentials(CertificateSamples.SAMPLE_CERT3_HASH, encodedCertString);
+    setCredentials(SAMPLE_CERT3_HASH);
     assertThatLogin("TheRegistrar", "password2")
         .hasResponse(
             "response_error.xml",
@@ -143,7 +127,7 @@ class EppLoginTlsTest extends EppTestCase {
 
   @Test
   void testBadCertificate_failsBadCertificate2200() throws Exception {
-    setCredentials("laffo", "cert");
+    setCredentials("laffo");
     assertThatLogin("NewRegistrar", "foo-BAR2")
         .hasResponse(
             "response_error.xml",
@@ -153,7 +137,7 @@ class EppLoginTlsTest extends EppTestCase {
 
   @Test
   void testGfeDidntProvideClientCertificate_failsMissingCertificate2200() throws Exception {
-    setCredentials(null, null);
+    setCredentials(null);
     assertThatLogin("NewRegistrar", "foo-BAR2")
         .hasResponse(
             "response_error.xml",
@@ -162,7 +146,7 @@ class EppLoginTlsTest extends EppTestCase {
 
   @Test
   void testGoodPrimaryCertificate() throws Exception {
-    setCredentials(null, encodedCertString);
+    setCredentials(SAMPLE_CERT3_HASH);
     DateTime now = DateTime.now(UTC);
     persistResource(
         loadRegistrar("NewRegistrar")
@@ -175,7 +159,7 @@ class EppLoginTlsTest extends EppTestCase {
 
   @Test
   void testGoodFailoverCertificate() throws Exception {
-    setCredentials(null, encodedCertString);
+    setCredentials(SAMPLE_CERT3_HASH);
     DateTime now = DateTime.now(UTC);
     persistResource(
         loadRegistrar("NewRegistrar")
@@ -188,7 +172,7 @@ class EppLoginTlsTest extends EppTestCase {
 
   @Test
   void testMissingPrimaryCertificateButHasFailover_usesFailover() throws Exception {
-    setCredentials(null, encodedCertString);
+    setCredentials(SAMPLE_CERT3_HASH);
     DateTime now = DateTime.now(UTC);
     persistResource(
         loadRegistrar("NewRegistrar")
@@ -201,7 +185,7 @@ class EppLoginTlsTest extends EppTestCase {
 
   @Test
   void testRegistrarHasNoCertificatesOnFile_fails() throws Exception {
-    setCredentials("laffo", "cert");
+    setCredentials("laffo");
     DateTime now = DateTime.now(UTC);
     persistResource(
         loadRegistrar("NewRegistrar")
@@ -217,9 +201,8 @@ class EppLoginTlsTest extends EppTestCase {
 
   @Test
   void testCertificateDoesNotMeetRequirements_fails() throws Exception {
-    String proxyEncoded = encodeX509CertificateFromPemString(CertificateSamples.SAMPLE_CERT);
     // SAMPLE_CERT has a validity period that is too long
-    setCredentials(CertificateSamples.SAMPLE_CERT_HASH, proxyEncoded);
+    setCredentials(CertificateSamples.SAMPLE_CERT_HASH);
     persistResource(
         loadRegistrar("NewRegistrar")
             .asBuilder()
@@ -251,10 +234,8 @@ class EppLoginTlsTest extends EppTestCase {
       pw.writeObject(generator);
     }
 
-    String proxyEncoded = encodeX509Certificate(certificate);
-
     // SAMPLE_CERT has a validity period that is too long
-    setCredentials(null, proxyEncoded);
+    setCredentials(getCertificateHash(certificate));
     persistResource(
         loadRegistrar("NewRegistrar")
             .asBuilder()
@@ -273,114 +254,4 @@ class EppLoginTlsTest extends EppTestCase {
                     + "Certificate validity period is too long; it must be less than or equal to"
                     + " 398 days."));
   }
-
-  @Test
-  // TODO(sarahbot@): Remove this test once requirements are enforced in production
-  void testCertificateDoesNotMeetRequirementsInProduction_beforeStartDate_succeeds()
-      throws Exception {
-    RegistryEnvironment.PRODUCTION.setup(systemPropertyExtension);
-    // SAMPLE_CERT has a validity period that is too long
-    String proxyEncoded = encodeX509CertificateFromPemString(CertificateSamples.SAMPLE_CERT);
-    setCredentials(null, proxyEncoded);
-    persistResource(
-        loadRegistrar("NewRegistrar")
-            .asBuilder()
-            .setClientCertificate(CertificateSamples.SAMPLE_CERT, clock.nowUtc())
-            .setFailoverClientCertificate(CertificateSamples.SAMPLE_CERT2, clock.nowUtc())
-            .build());
-    // Even though the certificate contains security violations, the login will succeed in
-    // production
-    assertThatLogin("NewRegistrar", "foo-BAR2")
-        .atTime(DateTime.parse("2020-01-01T16:00:00Z"))
-        .hasSuccessfulLogin();
-    assertAboutLogs()
-        .that(handler)
-        .hasLogAtLevelWithMessage(
-            Level.WARNING,
-            "Registrar certificate used for NewRegistrar does not meet certificate requirements:"
-                + " Certificate validity period is too long; it must be less than or equal to 398"
-                + " days.");
-  }
-
-  @Test
-  void testCertificateDoesNotMeetRequirementsInProduction_afterStartDate_fails() throws Exception {
-    RegistryEnvironment.PRODUCTION.setup(systemPropertyExtension);
-    String proxyEncoded = encodeX509CertificateFromPemString(CertificateSamples.SAMPLE_CERT);
-    // SAMPLE_CERT has a validity period that is too long
-    setCredentials(CertificateSamples.SAMPLE_CERT_HASH, proxyEncoded);
-    persistResource(
-        loadRegistrar("NewRegistrar")
-            .asBuilder()
-            .setClientCertificate(CertificateSamples.SAMPLE_CERT, clock.nowUtc())
-            .setFailoverClientCertificate(CertificateSamples.SAMPLE_CERT2, clock.nowUtc())
-            .build());
-    assertThatLogin("NewRegistrar", "foo-BAR2")
-        .atTime(DateTime.parse("2021-04-01T16:00:00Z"))
-        .hasResponse(
-            "response_error.xml",
-            ImmutableMap.of(
-                "CODE",
-                "2200",
-                "MSG",
-                "Registrar certificate contains the following security violations:\n"
-                    + "Certificate validity period is too long; it must be less than or equal to"
-                    + " 398 days."));
-  }
-
-  @Test
-  void testRegistrarCertificateContainsExtraMetadata_succeeds() throws Exception {
-    String certPem =
-        String.format(
-            "Bag Attributes\n"
-                + "    localKeyID: 1F 1C 3A 3A 4C 03 EC C4 BC 7A C3 21 A9 F2 13 66 21 B8 7B 26 \n"
-                + "subject=/C=US/ST=New York/L=New"
-                + " York/O=Test/OU=ABC/CN=tester.test/emailAddress=test-certificate@test.test\n"
-                + "issuer=/C=US/ST=NY/L=NYC/O=ABC/OU=TEST CA/CN=TEST"
-                + " CA/emailAddress=testing@test.test\n"
-                + "%s",
-            CertificateSamples.SAMPLE_CERT3);
-
-    setCredentials(null, encodeX509CertificateFromPemString(certPem));
-    DateTime now = DateTime.now(UTC);
-    persistResource(
-        loadRegistrar("NewRegistrar")
-            .asBuilder()
-            .setClientCertificate(certPem, now)
-            .setFailoverClientCertificate(CertificateSamples.SAMPLE_CERT2, now)
-            .build());
-    assertThatLoginSucceeds("NewRegistrar", "foo-BAR2");
-  }
-
-  @Test
-  void testRegistrarCertificateContainsExtraMetadataAndViolations_fails() throws Exception {
-    String certPem =
-        String.format(
-            "Bag Attributes\n"
-                + "    localKeyID: 1F 1C 3A 3A 4C 03 EC C4 BC 7A C3 21 A9 F2 13 66 21 B8 7B 26 \n"
-                + "subject=/C=US/ST=New York/L=New"
-                + " York/O=Test/OU=ABC/CN=tester.test/emailAddress=test-certificate@test.test\n"
-                + "issuer=/C=US/ST=NY/L=NYC/O=ABC/OU=TEST CA/CN=TEST"
-                + " CA/emailAddress=testing@test.test\n"
-                + "%s",
-            CertificateSamples.SAMPLE_CERT);
-
-    setCredentials(null, encodeX509CertificateFromPemString(certPem));
-    DateTime now = DateTime.now(UTC);
-    persistResource(
-        loadRegistrar("NewRegistrar")
-            .asBuilder()
-            .setClientCertificate(certPem, now)
-            .setFailoverClientCertificate(CertificateSamples.SAMPLE_CERT2, now)
-            .build());
-    assertThatLogin("NewRegistrar", "foo-BAR2")
-        .hasResponse(
-            "response_error.xml",
-            ImmutableMap.of(
-                "CODE",
-                "2200",
-                "MSG",
-                "Registrar certificate contains the following security violations:\n"
-                    + "Certificate validity period is too long; it must be less than or equal to"
-                    + " 398 days."));
-  }
 }

core/src/test/java/google/registry/flows/FlowRunnerTest.java

@@ -153,17 +153,10 @@ class FlowRunnerTest {
   void testRun_loggingStatement_tlsCredentials() throws Exception {
     flowRunner.credentials =
         new TlsCredentials(
-            true,
+            true, Optional.of("abc123def"), Optional.of("127.0.0.1"), certificateChecker);
-            Optional.of("abc123def"),
-            Optional.of("cert046F5A3"),
-            Optional.of("127.0.0.1"),
-            certificateChecker,
-            clock);
     flowRunner.run(eppMetricBuilder);
     assertThat(Splitter.on("\n\t").split(findFirstLogMessageByPrefix(handler, "EPP Command\n\t")))
-        .contains(
+        .contains("TlsCredentials{clientCertificateHash=abc123def," + " clientAddress=/127.0.0.1}");
-            "TlsCredentials{clientCertificate=cert046F5A3, clientCertificateHash=abc123def,"
-                + " clientAddress=/127.0.0.1}");
   }
 
   @Test

core/src/test/java/google/registry/flows/TlsCredentialsTest.java

@@ -67,13 +67,7 @@ final class TlsCredentialsTest {
   @Test
   void testClientCertificateAndHash_missing() {
     TlsCredentials tls =
-        new TlsCredentials(
+        new TlsCredentials(true, Optional.empty(), Optional.of("192.168.1.1"), certificateChecker);
-            true,
-            Optional.empty(),
-            Optional.empty(),
-            Optional.of("192.168.1.1"),
-            certificateChecker,
-            clock);
     persistResource(
         loadRegistrar("TheRegistrar")
             .asBuilder()
@@ -81,19 +75,13 @@ final class TlsCredentialsTest {
             .build());
     assertThrows(
         MissingRegistrarCertificateException.class,
-        () -> tls.validateCertificate(Registrar.loadByClientId("TheRegistrar").get()));
+        () -> tls.validateCertificateHash(Registrar.loadByClientId("TheRegistrar").get()));
   }
 
   @Test
   void test_missingIpAddress_doesntAllowAccess() {
     TlsCredentials tls =
-        new TlsCredentials(
+        new TlsCredentials(false, Optional.of("certHash"), Optional.empty(), certificateChecker);
-            false,
-            Optional.of("certHash"),
-            Optional.empty(),
-            Optional.empty(),
-            certificateChecker,
-            clock);
     persistResource(
         loadRegistrar("TheRegistrar")
             .asBuilder()
@@ -105,60 +93,22 @@ final class TlsCredentialsTest {
         () -> tls.validate(Registrar.loadByClientId("TheRegistrar").get(), "password"));
   }
 
-  @Test
-  void test_validateCertificate_canBeConfiguredToBypassCertHashes() throws Exception {
-    TlsCredentials tls =
-        new TlsCredentials(
-            false,
-            Optional.of("certHash"),
-            Optional.of("cert"),
-            Optional.of("192.168.1.1"),
-            certificateChecker,
-            clock);
-    persistResource(
-        loadRegistrar("TheRegistrar")
-            .asBuilder()
-            .setClientCertificate(null, clock.nowUtc())
-            .setFailoverClientCertificate(null, clock.nowUtc())
-            .build());
-    // This would throw a RegistrarCertificateNotConfiguredException if cert hashes were not
-    // bypassed
-    tls.validateCertificate(Registrar.loadByClientId("TheRegistrar").get());
-  }
-
-  @Test
-  void testProvideClientCertificate() {
-    HttpServletRequest req = mock(HttpServletRequest.class);
-    when(req.getHeader(ProxyHttpHeaders.FULL_CERTIFICATE)).thenReturn("data");
-    assertThat(TlsCredentials.EppTlsModule.provideClientCertificate(req)).hasValue("data");
-  }
-
   @Test
   void testClientCertificate_notConfigured() {
     TlsCredentials tls =
         new TlsCredentials(
-            true,
+            true, Optional.of("hash"), Optional.of("192.168.1.1"), certificateChecker);
-            Optional.of("hash"),
-            Optional.of(SAMPLE_CERT),
-            Optional.of("192.168.1.1"),
-            certificateChecker,
-            clock);
     persistResource(loadRegistrar("TheRegistrar").asBuilder().build());
     assertThrows(
         RegistrarCertificateNotConfiguredException.class,
-        () -> tls.validateCertificate(Registrar.loadByClientId("TheRegistrar").get()));
+        () -> tls.validateCertificateHash(Registrar.loadByClientId("TheRegistrar").get()));
   }
 
   @Test
-  void test_validateCertificate_canBeConfiguredToBypassCerts() throws Exception {
+  void test_validateCertificateHash_canBeConfiguredToBypassCerts() throws Exception {
     TlsCredentials tls =
         new TlsCredentials(
-            false,
+            false, Optional.of("certHash"), Optional.of("192.168.1.1"), certificateChecker);
-            Optional.of("certHash"),
-            Optional.of("cert"),
-            Optional.of("192.168.1.1"),
-            certificateChecker,
-            clock);
     persistResource(
         loadRegistrar("TheRegistrar")
             .asBuilder()
@@ -166,6 +116,6 @@ final class TlsCredentialsTest {
             .setFailoverClientCertificate(null, clock.nowUtc())
             .build());
     // This would throw a RegistrarCertificateNotConfiguredException if cert hashes wren't bypassed.
-    tls.validateCertificate(Registrar.loadByClientId("TheRegistrar").get());
+    tls.validateCertificateHash(Registrar.loadByClientId("TheRegistrar").get());
   }
 }

core/src/test/java/google/registry/flows/session/LoginFlowViaTlsTest.java

@@ -16,7 +16,6 @@ package google.registry.flows.session;
 
 import static google.registry.testing.DatabaseHelper.persistResource;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
-import static google.registry.util.X509Utils.encodeX509CertificateFromPemString;
 import static org.joda.time.DateTimeZone.UTC;
 
 import com.google.common.collect.ImmutableList;
@@ -31,18 +30,9 @@ import google.registry.flows.certs.CertificateChecker;
 import google.registry.model.registrar.Registrar;
 import google.registry.testing.CertificateSamples;
 import google.registry.util.CidrAddressBlock;
-import google.registry.util.SelfSignedCaCertificate;
-import java.io.StringWriter;
-import java.security.cert.CertificateException;
-import java.security.cert.X509Certificate;
-import java.util.Base64;
 import java.util.Optional;
 import org.joda.time.DateTime;
-import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
-import org.testcontainers.shaded.org.bouncycastle.openssl.jcajce.JcaMiscPEMGenerator;
-import org.testcontainers.shaded.org.bouncycastle.util.io.pem.PemObjectGenerator;
-import org.testcontainers.shaded.org.bouncycastle.util.io.pem.PemWriter;
 
 /** Unit tests for {@link LoginFlow} when accessed via a TLS transport. */
 public class LoginFlowViaTlsTest extends LoginFlowTestCase {
@@ -50,7 +40,6 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
   private static final Optional<String> GOOD_CERT = Optional.of(CertificateSamples.SAMPLE_CERT3);
   private static final Optional<String> GOOD_CERT_HASH =
       Optional.of(CertificateSamples.SAMPLE_CERT3_HASH);
-  private static final Optional<String> BAD_CERT = Optional.of(CertificateSamples.SAMPLE_CERT2);
   private static final Optional<String> BAD_CERT_HASH =
       Optional.of(CertificateSamples.SAMPLE_CERT2_HASH);
   private static final Optional<String> GOOD_IP = Optional.of("192.168.1.1");
@@ -64,12 +53,6 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
           2048,
           ImmutableSet.of("secp256r1", "secp384r1"),
           clock);
-  private Optional<String> encodedCertString;
-
-  @BeforeEach
-  void beforeEach() throws CertificateException {
-    encodedCertString = Optional.of(encodeX509CertificateFromPemString(GOOD_CERT.get()));
-  }
 
   @Override
   protected Registrar.Builder getRegistrarBuilder() {
@@ -82,42 +65,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
   @Test
   void testSuccess_withGoodCredentials() throws Exception {
     persistResource(getRegistrarBuilder().build());
-    credentials =
+    credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_IP, certificateChecker);
-        new TlsCredentials(
-            true, GOOD_CERT_HASH, encodedCertString, GOOD_IP, certificateChecker, clock);
-    doSuccessfulTest("login_valid.xml");
-  }
-
-  @Test
-  void testSuccess_withNewlyConstructedCertificate() throws Exception {
-    X509Certificate certificate =
-        SelfSignedCaCertificate.create(
-                "test", clock.nowUtc().minusDays(100), clock.nowUtc().plusDays(150))
-            .cert();
-
-    StringWriter sw = new StringWriter();
-    try (PemWriter pw = new PemWriter(sw)) {
-      PemObjectGenerator generator = new JcaMiscPEMGenerator(certificate);
-      pw.writeObject(generator);
-    }
-
-    persistResource(
-        super.getRegistrarBuilder()
-            .setClientCertificate(sw.toString(), DateTime.now(UTC))
-            .setIpAddressAllowList(
-                ImmutableList.of(
-                    CidrAddressBlock.create(InetAddresses.forString(GOOD_IP.get()), 32)))
-            .build());
-
-    String encodedCertificate = Base64.getEncoder().encodeToString(certificate.getEncoded());
-    credentials =
-        new TlsCredentials(
-            true,
-            Optional.empty(),
-            Optional.of(encodedCertificate),
-            GOOD_IP,
-            certificateChecker,
-            clock);
     doSuccessfulTest("login_valid.xml");
   }
 
@@ -128,9 +76,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
             .setIpAddressAllowList(
                 ImmutableList.of(CidrAddressBlock.create("2001:db8:0:0:0:0:1:1/32")))
             .build());
-    credentials =
+    credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_IPV6, certificateChecker);
-        new TlsCredentials(
-            true, GOOD_CERT_HASH, encodedCertString, GOOD_IPV6, certificateChecker, clock);
     doSuccessfulTest("login_valid.xml");
   }
 
@@ -141,9 +87,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
             .setIpAddressAllowList(
                 ImmutableList.of(CidrAddressBlock.create("2001:db8:0:0:0:0:1:1/32")))
             .build());
-    credentials =
+    credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_IPV6, certificateChecker);
-        new TlsCredentials(
-            true, GOOD_CERT_HASH, encodedCertString, GOOD_IPV6, certificateChecker, clock);
     doSuccessfulTest("login_valid.xml");
   }
 
@@ -153,38 +97,21 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
         getRegistrarBuilder()
             .setIpAddressAllowList(ImmutableList.of(CidrAddressBlock.create("192.168.1.255/24")))
             .build());
-    credentials =
+    credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_IP, certificateChecker);
-        new TlsCredentials(
-            true, GOOD_CERT_HASH, encodedCertString, GOOD_IP, certificateChecker, clock);
     doSuccessfulTest("login_valid.xml");
   }
 
   @Test
   void testFailure_incorrectClientCertificateHash() throws Exception {
     persistResource(getRegistrarBuilder().build());
-    String proxyEncoded = encodeX509CertificateFromPemString(BAD_CERT.get());
+    credentials = new TlsCredentials(true, BAD_CERT_HASH, GOOD_IP, certificateChecker);
-    credentials =
-        new TlsCredentials(
-            true, BAD_CERT_HASH, Optional.of(proxyEncoded), GOOD_IP, certificateChecker, clock);
     doFailingTest("login_valid.xml", BadRegistrarCertificateException.class);
   }
 
-  @Test
-  // TODO(Sarahbot): This should fail once hash fallback is removed
-  void testSuccess_missingClientCertificate() throws Exception {
-    persistResource(getRegistrarBuilder().build());
-    credentials =
-        new TlsCredentials(
-            true, GOOD_CERT_HASH, Optional.empty(), GOOD_IP, certificateChecker, clock);
-    doSuccessfulTest("login_valid.xml");
-  }
-
   @Test
   void testFailure_missingClientCertificateAndHash() {
     persistResource(getRegistrarBuilder().build());
-    credentials =
+    credentials = new TlsCredentials(true, Optional.empty(), GOOD_IP, certificateChecker);
-        new TlsCredentials(
-            true, Optional.empty(), Optional.empty(), GOOD_IP, certificateChecker, clock);
     doFailingTest("login_valid.xml", MissingRegistrarCertificateException.class);
   }
 
@@ -197,9 +124,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 =
+    credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, certificateChecker);
-        new TlsCredentials(
-            true, GOOD_CERT_HASH, GOOD_CERT, Optional.empty(), certificateChecker, clock);
     doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class);
   }
 
@@ -212,8 +137,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 =
+    credentials = new TlsCredentials(true, GOOD_CERT_HASH, BAD_IP, certificateChecker);
-        new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, BAD_IP, certificateChecker, clock);
     doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class);
   }
 
@@ -226,8 +150,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 =
+    credentials = new TlsCredentials(true, GOOD_CERT_HASH, BAD_IPV6, certificateChecker);
-        new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, BAD_IPV6, certificateChecker, clock);
     doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class);
   }
 }