diff --git a/core/src/main/java/google/registry/flows/TlsCredentials.java b/core/src/main/java/google/registry/flows/TlsCredentials.java index dcd573faa..c4234a0fe 100644 --- a/core/src/main/java/google/registry/flows/TlsCredentials.java +++ b/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; *
X-SSL-Certificate *
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. - *
X-SSL-Full-Certificate - *
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. *
X-Forwarded-For *
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 clientCertificateHash; - private final Optional clientCertificate; private final Optional clientInetAddr; private final CertificateChecker certificateChecker; - private final Clock clock; @Inject public TlsCredentials( @Config("requireSslCertificates") boolean requireSslCertificates, @Header(ProxyHttpHeaders.CERTIFICATE_HASH) Optional clientCertificateHash, - @Header(ProxyHttpHeaders.FULL_CERTIFICATE) Optional clientCertificate, @Header(ProxyHttpHeaders.IP_ADDRESS) Optional clientAddress, - CertificateChecker certificateChecker, - Clock clock) { + CertificateChecker certificateChecker) { 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 { - // 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 storedCert; - Optional 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. + void validateCertificateHash(Registrar registrar) throws AuthenticationErrorException { 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 deserializePemCert(Optional 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 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 provideIpAddress(HttpServletRequest req) { diff --git a/core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java b/core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java index f00ff3d8c..32453b0ef 100644 --- a/core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java +++ b/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, - clock) + certificateChecker) .validate(registrar, password); checkState( registrar.isLive(), "Registrar %s has non-live state: %s", clientId, registrar.getState()); diff --git a/core/src/test/java/google/registry/flows/EppLoginTlsTest.java b/core/src/test/java/google/registry/flows/EppLoginTlsTest.java index a60ed3075..143d29a51 100644 --- a/core/src/test/java/google/registry/flows/EppLoginTlsTest.java +++ b/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.encodeX509CertificateFromPemString; +import static google.registry.util.X509Utils.getCertificateHash; 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 = - Logger.getLogger(TlsCredentials.class.getCanonicalName()); - private final TestLogHandler handler = new TestLogHandler(); - - private String encodedCertString; - - void setCredentials(String clientCertificateHash, String clientCertificate) { + void setCredentials(String clientCertificateHash) { setTransportCredentials( new TlsCredentials( true, Optional.ofNullable(clientCertificateHash), - Optional.ofNullable(clientCertificate), Optional.of("192.168.1.100:54321"), - certificateChecker, - clock)); + certificateChecker)); } @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.")); - } } diff --git a/core/src/test/java/google/registry/flows/FlowRunnerTest.java b/core/src/test/java/google/registry/flows/FlowRunnerTest.java index dedec3507..1d6bd9e1b 100644 --- a/core/src/test/java/google/registry/flows/FlowRunnerTest.java +++ b/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, - Optional.of("abc123def"), - Optional.of("cert046F5A3"), - Optional.of("127.0.0.1"), - certificateChecker, - clock); + true, Optional.of("abc123def"), Optional.of("127.0.0.1"), certificateChecker); flowRunner.run(eppMetricBuilder); assertThat(Splitter.on("\n\t").split(findFirstLogMessageByPrefix(handler, "EPP Command\n\t"))) - .contains( - "TlsCredentials{clientCertificate=cert046F5A3, clientCertificateHash=abc123def," - + " clientAddress=/127.0.0.1}"); + .contains("TlsCredentials{clientCertificateHash=abc123def," + " clientAddress=/127.0.0.1}"); } @Test diff --git a/core/src/test/java/google/registry/flows/TlsCredentialsTest.java b/core/src/test/java/google/registry/flows/TlsCredentialsTest.java index d69eeece4..80689f500 100644 --- a/core/src/test/java/google/registry/flows/TlsCredentialsTest.java +++ b/core/src/test/java/google/registry/flows/TlsCredentialsTest.java @@ -67,13 +67,7 @@ final class TlsCredentialsTest { @Test void testClientCertificateAndHash_missing() { TlsCredentials tls = - new TlsCredentials( - true, - Optional.empty(), - Optional.empty(), - Optional.of("192.168.1.1"), - certificateChecker, - clock); + new TlsCredentials(true, Optional.empty(), Optional.of("192.168.1.1"), certificateChecker); 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( - false, - Optional.of("certHash"), - Optional.empty(), - Optional.empty(), - certificateChecker, - clock); + new TlsCredentials(false, Optional.of("certHash"), Optional.empty(), certificateChecker); 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, - Optional.of("hash"), - Optional.of(SAMPLE_CERT), - Optional.of("192.168.1.1"), - certificateChecker, - clock); + true, Optional.of("hash"), Optional.of("192.168.1.1"), certificateChecker); 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, - Optional.of("certHash"), - Optional.of("cert"), - Optional.of("192.168.1.1"), - certificateChecker, - clock); + false, Optional.of("certHash"), Optional.of("192.168.1.1"), certificateChecker); 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()); } } diff --git a/core/src/test/java/google/registry/flows/session/LoginFlowViaTlsTest.java b/core/src/test/java/google/registry/flows/session/LoginFlowViaTlsTest.java index 8c5c15fce..9b5fe1ef3 100644 --- a/core/src/test/java/google/registry/flows/session/LoginFlowViaTlsTest.java +++ b/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 GOOD_CERT = Optional.of(CertificateSamples.SAMPLE_CERT3); private static final Optional GOOD_CERT_HASH = Optional.of(CertificateSamples.SAMPLE_CERT3_HASH); - private static final Optional BAD_CERT = Optional.of(CertificateSamples.SAMPLE_CERT2); private static final Optional BAD_CERT_HASH = Optional.of(CertificateSamples.SAMPLE_CERT2_HASH); private static final Optional 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 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 = - 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); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_IP, certificateChecker); 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 = - new TlsCredentials( - true, GOOD_CERT_HASH, encodedCertString, GOOD_IPV6, certificateChecker, clock); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_IPV6, certificateChecker); 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 = - new TlsCredentials( - true, GOOD_CERT_HASH, encodedCertString, GOOD_IPV6, certificateChecker, clock); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_IPV6, certificateChecker); 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 = - new TlsCredentials( - true, GOOD_CERT_HASH, encodedCertString, GOOD_IP, certificateChecker, clock); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_IP, certificateChecker); 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, Optional.of(proxyEncoded), GOOD_IP, certificateChecker, clock); + credentials = new TlsCredentials(true, BAD_CERT_HASH, GOOD_IP, certificateChecker); 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 = - new TlsCredentials( - true, Optional.empty(), Optional.empty(), GOOD_IP, certificateChecker, clock); + credentials = new TlsCredentials(true, Optional.empty(), GOOD_IP, certificateChecker); 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 = - new TlsCredentials( - true, GOOD_CERT_HASH, GOOD_CERT, Optional.empty(), certificateChecker, clock); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, certificateChecker); 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 = - new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, BAD_IP, certificateChecker, clock); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, BAD_IP, certificateChecker); 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 = - new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, BAD_IPV6, certificateChecker, clock); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, BAD_IPV6, certificateChecker); doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class); } }