diff --git a/core/src/main/java/google/registry/flows/TlsCredentials.java b/core/src/main/java/google/registry/flows/TlsCredentials.java index 6d77d95e0..91e573f22 100644 --- a/core/src/main/java/google/registry/flows/TlsCredentials.java +++ b/core/src/main/java/google/registry/flows/TlsCredentials.java @@ -25,7 +25,10 @@ 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; @@ -60,17 +63,20 @@ public class TlsCredentials implements TransportCredentials { private final Optional clientCertificateHash; private final Optional clientCertificate; private final Optional clientInetAddr; + private final CertificateChecker certificateChecker; @Inject public TlsCredentials( @Config("requireSslCertificates") boolean requireSslCertificates, @Header("X-SSL-Certificate") Optional clientCertificateHash, @Header("X-SSL-Full-Certificate") Optional clientCertificate, - @Header("X-Forwarded-For") Optional clientAddress) { + @Header("X-Forwarded-For") Optional clientAddress, + CertificateChecker certificateChecker) { this.requireSslCertificates = requireSslCertificates; this.clientCertificateHash = clientCertificateHash; this.clientCertificate = clientCertificate; this.clientInetAddr = clientAddress.map(TlsCredentials::parseInetAddress); + this.certificateChecker = certificateChecker; } static InetAddress parseInetAddress(String asciiAddr) { @@ -146,6 +152,24 @@ public class TlsCredentials implements TransportCredentials { // Check if the certificate is equal to the one on file for the registrar. if (clientCertificate.equals(registrar.getClientCertificate()) || clientCertificate.equals(registrar.getFailoverClientCertificate())) { + // Check certificate for any requirement violations + // TODO(Sarahbot@): Throw exceptions instead of just logging once requirement enforcement + // begins + try { + certificateChecker.validateCertificate(clientCertificate.get()); + } catch (InsecureCertificateException e) { + // throw exception in unit tests and Sandbox + if (RegistryEnvironment.get().equals(RegistryEnvironment.UNITTEST) + || RegistryEnvironment.get().equals(RegistryEnvironment.SANDBOX)) { + 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; } @@ -211,6 +235,20 @@ public class TlsCredentials implements TransportCredentials { } } + /** Registrar certificate contains the following security violations: ... */ + public static class CertificateContainsSecurityViolationsException + extends AuthenticationErrorException { + InsecureCertificateException exception; + + CertificateContainsSecurityViolationsException(InsecureCertificateException exception) { + super( + String.format( + "Registrar certificate contains the following security violations:\n%s", + exception.getMessage())); + this.exception = exception; + } + } + /** Registrar certificate not present. */ public static class MissingRegistrarCertificateException extends AuthenticationErrorException { MissingRegistrarCertificateException() { diff --git a/core/src/main/java/google/registry/flows/certs/CertificateChecker.java b/core/src/main/java/google/registry/flows/certs/CertificateChecker.java index e90145243..aeef1356b 100644 --- a/core/src/main/java/google/registry/flows/certs/CertificateChecker.java +++ b/core/src/main/java/google/registry/flows/certs/CertificateChecker.java @@ -89,14 +89,14 @@ public class CertificateChecker { * Checks the given certificate string for violations and throws an exception if any violations * exist. */ - public void validateCertificate(String certificateString) { + public void validateCertificate(String certificateString) throws InsecureCertificateException { ImmutableSet violations = checkCertificate(certificateString); if (!violations.isEmpty()) { String displayMessages = violations.stream() .map(violation -> getViolationDisplayMessage(violation)) .collect(Collectors.joining("\n")); - throw new IllegalArgumentException(displayMessages); + throw new InsecureCertificateException(violations, displayMessages); } } @@ -258,4 +258,14 @@ public class CertificateChecker { return certificateChecker.getViolationDisplayMessage(this); } } + + /** Exception to throw when a certificate has security violations. */ + public static class InsecureCertificateException extends Exception { + ImmutableSet violations; + + InsecureCertificateException(ImmutableSet violations, String message) { + super(message); + this.violations = violations; + } + } } diff --git a/core/src/main/java/google/registry/tools/RegistryToolComponent.java b/core/src/main/java/google/registry/tools/RegistryToolComponent.java index 2f420bd3f..cb68eb7be 100644 --- a/core/src/main/java/google/registry/tools/RegistryToolComponent.java +++ b/core/src/main/java/google/registry/tools/RegistryToolComponent.java @@ -168,6 +168,8 @@ interface RegistryToolComponent { void inject(ValidateEscrowDepositCommand command); + void inject(ValidateLoginCredentialsCommand command); + void inject(WhoisQueryCommand command); AppEngineConnection appEngineConnection(); diff --git a/core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java b/core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java index c62d12630..07aa9868a 100644 --- a/core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java +++ b/core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java @@ -25,12 +25,14 @@ import static java.nio.charset.StandardCharsets.US_ASCII; import com.beust.jcommander.Parameter; import com.beust.jcommander.Parameters; import google.registry.flows.TlsCredentials; +import google.registry.flows.certs.CertificateChecker; import google.registry.model.registrar.Registrar; import google.registry.tools.params.PathParameter; import java.nio.file.Files; import java.nio.file.Path; import java.util.Optional; import javax.annotation.Nullable; +import javax.inject.Inject; /** A command to test registrar login credentials. */ @Parameters(separators = " =", commandDescription = "Test registrar login credentials") @@ -68,6 +70,8 @@ final class ValidateLoginCredentialsCommand implements CommandWithRemoteApi { description = "Client ip address to pretend to use") private String clientIpAddress = "10.0.0.1"; + @Inject CertificateChecker certificateChecker; + @Override public void run() throws Exception { checkArgument( @@ -85,7 +89,8 @@ final class ValidateLoginCredentialsCommand implements CommandWithRemoteApi { true, Optional.ofNullable(clientCertificateHash), Optional.ofNullable(clientCertificate), - Optional.ofNullable(clientIpAddress)) + Optional.ofNullable(clientIpAddress), + certificateChecker) .validate(registrar, password); checkState( registrar.isLive(), "Registrar %s has non-live state: %s", clientId, registrar.getState()); diff --git a/core/src/main/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java b/core/src/main/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java index f5fe3c2f0..464f9672e 100644 --- a/core/src/main/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java +++ b/core/src/main/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java @@ -39,6 +39,7 @@ import com.google.common.collect.Streams; import com.google.common.flogger.FluentLogger; import google.registry.config.RegistryEnvironment; import google.registry.flows.certs.CertificateChecker; +import google.registry.flows.certs.CertificateChecker.InsecureCertificateException; import google.registry.model.registrar.Registrar; import google.registry.model.registrar.RegistrarContact; import google.registry.model.registrar.RegistrarContact.Type; @@ -337,7 +338,11 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA private boolean validateCertificate( Optional existingCertificate, String certificateString) { if (!existingCertificate.isPresent() || !existingCertificate.get().equals(certificateString)) { + try { certificateChecker.validateCertificate(certificateString); + } catch (InsecureCertificateException e) { + throw new IllegalArgumentException(e.getMessage()); + } return true; } return false; diff --git a/core/src/test/java/google/registry/flows/EppLoginTlsTest.java b/core/src/test/java/google/registry/flows/EppLoginTlsTest.java index 64e2dbdd7..40bb60764 100644 --- a/core/src/test/java/google/registry/flows/EppLoginTlsTest.java +++ b/core/src/test/java/google/registry/flows/EppLoginTlsTest.java @@ -16,16 +16,33 @@ package google.registry.flows; 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 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.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; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; +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; /** Test logging in with TLS credentials. */ class EppLoginTlsTest extends EppTestCase { @@ -34,13 +51,30 @@ class EppLoginTlsTest extends EppTestCase { final AppEngineExtension appEngine = AppEngineExtension.builder().withDatastoreAndCloudSql().build(); + @RegisterExtension + @Order(value = Integer.MAX_VALUE) + final SystemPropertyExtension systemPropertyExtension = new SystemPropertyExtension(); + + private final CertificateChecker certificateChecker = + new CertificateChecker( + ImmutableSortedMap.of(START_OF_TIME, 825, DateTime.parse("2020-09-01T00:00:00Z"), 398), + 30, + 2048, + ImmutableSet.of("secp256r1", "secp384r1"), + clock); + + private final Logger loggerToIntercept = + Logger.getLogger(TlsCredentials.class.getCanonicalName()); + private final TestLogHandler handler = new TestLogHandler(); + void setCredentials(String clientCertificateHash, String clientCertificate) { setTransportCredentials( new TlsCredentials( true, Optional.ofNullable(clientCertificateHash), Optional.ofNullable(clientCertificate), - Optional.of("192.168.1.100:54321"))); + Optional.of("192.168.1.100:54321"), + certificateChecker)); } @BeforeEach @@ -48,7 +82,7 @@ class EppLoginTlsTest extends EppTestCase { persistResource( loadRegistrar("NewRegistrar") .asBuilder() - .setClientCertificate(CertificateSamples.SAMPLE_CERT, DateTime.now(UTC)) + .setClientCertificate(CertificateSamples.SAMPLE_CERT3, DateTime.now(UTC)) .build()); // Set a cert for the second registrar, or else any cert will be allowed for login. persistResource( @@ -56,18 +90,19 @@ class EppLoginTlsTest extends EppTestCase { .asBuilder() .setClientCertificate(CertificateSamples.SAMPLE_CERT2, DateTime.now(UTC)) .build()); + loggerToIntercept.addHandler(handler); } @Test void testLoginLogout() throws Exception { - setCredentials(CertificateSamples.SAMPLE_CERT_HASH, CertificateSamples.SAMPLE_CERT); + setCredentials(CertificateSamples.SAMPLE_CERT3_HASH, CertificateSamples.SAMPLE_CERT3); assertThatLoginSucceeds("NewRegistrar", "foo-BAR2"); assertThatLogoutSucceeds(); } @Test void testLogin_wrongPasswordFails() throws Exception { - setCredentials(CertificateSamples.SAMPLE_CERT_HASH, CertificateSamples.SAMPLE_CERT); + setCredentials(CertificateSamples.SAMPLE_CERT3_HASH, CertificateSamples.SAMPLE_CERT3); // For TLS login, we also check the epp xml password. assertThatLogin("NewRegistrar", "incorrect") .hasResponse( @@ -77,7 +112,7 @@ class EppLoginTlsTest extends EppTestCase { @Test void testMultiLogin() throws Exception { - setCredentials(CertificateSamples.SAMPLE_CERT_HASH, CertificateSamples.SAMPLE_CERT); + setCredentials(CertificateSamples.SAMPLE_CERT3_HASH, CertificateSamples.SAMPLE_CERT3); assertThatLoginSucceeds("NewRegistrar", "foo-BAR2"); assertThatLogoutSucceeds(); assertThatLoginSucceeds("NewRegistrar", "foo-BAR2"); @@ -120,12 +155,12 @@ class EppLoginTlsTest extends EppTestCase { @Test void testGoodPrimaryCertificate() throws Exception { - setCredentials(CertificateSamples.SAMPLE_CERT_HASH, CertificateSamples.SAMPLE_CERT); + setCredentials(CertificateSamples.SAMPLE_CERT3_HASH, CertificateSamples.SAMPLE_CERT3); DateTime now = DateTime.now(UTC); persistResource( loadRegistrar("NewRegistrar") .asBuilder() - .setClientCertificate(CertificateSamples.SAMPLE_CERT, now) + .setClientCertificate(CertificateSamples.SAMPLE_CERT3, now) .setFailoverClientCertificate(CertificateSamples.SAMPLE_CERT2, now) .build()); assertThatLoginSucceeds("NewRegistrar", "foo-BAR2"); @@ -133,26 +168,26 @@ class EppLoginTlsTest extends EppTestCase { @Test void testGoodFailoverCertificate() throws Exception { - setCredentials(CertificateSamples.SAMPLE_CERT2_HASH, CertificateSamples.SAMPLE_CERT2); + setCredentials(CertificateSamples.SAMPLE_CERT3_HASH, CertificateSamples.SAMPLE_CERT3); DateTime now = DateTime.now(UTC); persistResource( loadRegistrar("NewRegistrar") .asBuilder() .setClientCertificate(CertificateSamples.SAMPLE_CERT, now) - .setFailoverClientCertificate(CertificateSamples.SAMPLE_CERT2, now) + .setFailoverClientCertificate(CertificateSamples.SAMPLE_CERT3, now) .build()); assertThatLoginSucceeds("NewRegistrar", "foo-BAR2"); } @Test void testMissingPrimaryCertificateButHasFailover_usesFailover() throws Exception { - setCredentials(CertificateSamples.SAMPLE_CERT2_HASH, CertificateSamples.SAMPLE_CERT2); + setCredentials(CertificateSamples.SAMPLE_CERT3_HASH, CertificateSamples.SAMPLE_CERT3); DateTime now = DateTime.now(UTC); persistResource( loadRegistrar("NewRegistrar") .asBuilder() .setClientCertificate(null, now) - .setFailoverClientCertificate(CertificateSamples.SAMPLE_CERT2, now) + .setFailoverClientCertificate(CertificateSamples.SAMPLE_CERT3, now) .build()); assertThatLoginSucceeds("NewRegistrar", "foo-BAR2"); } @@ -172,4 +207,85 @@ class EppLoginTlsTest extends EppTestCase { "response_error.xml", ImmutableMap.of("CODE", "2200", "MSG", "Registrar certificate is not configured")); } + + @Test + void testCertificateDoesNotMeetRequirements_fails() throws Exception { + // SAMPLE_CERT has a validity period that is too long + setCredentials(CertificateSamples.SAMPLE_CERT_HASH, CertificateSamples.SAMPLE_CERT); + persistResource( + loadRegistrar("NewRegistrar") + .asBuilder() + .setClientCertificate(CertificateSamples.SAMPLE_CERT, clock.nowUtc()) + .setFailoverClientCertificate(CertificateSamples.SAMPLE_CERT2, clock.nowUtc()) + .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.")); + } + + @Test + void testCertificateDoesNotMeetMultipleRequirements_fails() throws Exception { + + X509Certificate certificate = + SelfSignedCaCertificate.create( + "test", clock.nowUtc().plusDays(100), clock.nowUtc().plusDays(5000)) + .cert(); + + StringWriter sw = new StringWriter(); + try (PemWriter pw = new PemWriter(sw)) { + PemObjectGenerator generator = new JcaMiscPEMGenerator(certificate); + pw.writeObject(generator); + } + + // SAMPLE_CERT has a validity period that is too long + setCredentials(CertificateSamples.SAMPLE_CERT_HASH, sw.toString()); + persistResource( + loadRegistrar("NewRegistrar") + .asBuilder() + .setClientCertificate(sw.toString(), clock.nowUtc()) + .setFailoverClientCertificate(CertificateSamples.SAMPLE_CERT2, clock.nowUtc()) + .build()); + assertThatLogin("NewRegistrar", "foo-BAR2") + .hasResponse( + "response_error.xml", + ImmutableMap.of( + "CODE", + "2200", + "MSG", + "Registrar certificate contains the following security violations:\n" + + "Certificate is expired.\n" + + "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_succeeds() throws Exception { + RegistryEnvironment.PRODUCTION.setup(systemPropertyExtension); + // SAMPLE_CERT has a validity period that is too long + setCredentials(CertificateSamples.SAMPLE_CERT_HASH, CertificateSamples.SAMPLE_CERT); + 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 + assertThatLoginSucceeds("NewRegistrar", "foo-BAR2"); + 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."); + } } diff --git a/core/src/test/java/google/registry/flows/FlowRunnerTest.java b/core/src/test/java/google/registry/flows/FlowRunnerTest.java index b3ddbd73c..5188b1f1c 100644 --- a/core/src/test/java/google/registry/flows/FlowRunnerTest.java +++ b/core/src/test/java/google/registry/flows/FlowRunnerTest.java @@ -19,6 +19,7 @@ import static com.google.common.truth.Truth.assertWithMessage; import static com.google.common.truth.Truth8.assertThat; import static google.registry.testing.TestDataHelper.loadFile; import static google.registry.testing.TestLogHandlerUtils.findFirstLogMessageByPrefix; +import static google.registry.util.DateTimeUtils.START_OF_TIME; import static java.nio.charset.StandardCharsets.UTF_8; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -27,8 +28,10 @@ import static org.mockito.Mockito.verify; import com.google.common.base.Joiner; import com.google.common.base.Splitter; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableSortedMap; import com.google.common.flogger.LoggerConfig; import com.google.common.testing.TestLogHandler; +import google.registry.flows.certs.CertificateChecker; import google.registry.model.eppcommon.Trid; import google.registry.model.eppoutput.EppOutput.ResponseOrGreeting; import google.registry.model.eppoutput.EppResponse; @@ -38,6 +41,7 @@ import google.registry.testing.FakeClock; import google.registry.testing.FakeHttpSession; import java.util.List; import java.util.Optional; +import org.joda.time.DateTime; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -54,6 +58,16 @@ class FlowRunnerTest { private final TestLogHandler handler = new TestLogHandler(); + protected final FakeClock clock = new FakeClock(); + + private final CertificateChecker certificateChecker = + new CertificateChecker( + ImmutableSortedMap.of(START_OF_TIME, 825, DateTime.parse("2020-09-01T00:00:00Z"), 398), + 30, + 2048, + ImmutableSet.of("secp256r1", "secp384r1"), + clock); + static class TestCommandFlow implements Flow { @Override public ResponseOrGreeting run() { @@ -139,7 +153,11 @@ class FlowRunnerTest { void testRun_loggingStatement_tlsCredentials() throws Exception { flowRunner.credentials = new TlsCredentials( - true, Optional.of("abc123def"), Optional.of("cert"), Optional.of("127.0.0.1")); + true, + Optional.of("abc123def"), + Optional.of("cert"), + Optional.of("127.0.0.1"), + certificateChecker); flowRunner.run(eppMetricBuilder); assertThat(Splitter.on("\n\t").split(findFirstLogMessageByPrefix(handler, "EPP Command\n\t"))) .contains("TlsCredentials{clientCertificateHash=abc123def, clientAddress=/127.0.0.1}"); diff --git a/core/src/test/java/google/registry/flows/TlsCredentialsTest.java b/core/src/test/java/google/registry/flows/TlsCredentialsTest.java index 8745844b0..d3e77838a 100644 --- a/core/src/test/java/google/registry/flows/TlsCredentialsTest.java +++ b/core/src/test/java/google/registry/flows/TlsCredentialsTest.java @@ -18,17 +18,20 @@ import static com.google.common.truth.Truth8.assertThat; import static google.registry.testing.CertificateSamples.SAMPLE_CERT; import static google.registry.testing.DatabaseHelper.loadRegistrar; import static google.registry.testing.DatabaseHelper.persistResource; -import static org.joda.time.DateTimeZone.UTC; +import static google.registry.util.DateTimeUtils.START_OF_TIME; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableSortedMap; import google.registry.flows.TlsCredentials.BadRegistrarIpAddressException; import google.registry.flows.TlsCredentials.MissingRegistrarCertificateException; import google.registry.flows.TlsCredentials.RegistrarCertificateNotConfiguredException; +import google.registry.flows.certs.CertificateChecker; import google.registry.model.registrar.Registrar; import google.registry.testing.AppEngineExtension; +import google.registry.testing.FakeClock; import google.registry.util.CidrAddressBlock; import java.util.Optional; import javax.servlet.http.HttpServletRequest; @@ -43,6 +46,16 @@ final class TlsCredentialsTest { final AppEngineExtension appEngine = AppEngineExtension.builder().withDatastoreAndCloudSql().build(); + protected final FakeClock clock = new FakeClock(); + + private final CertificateChecker certificateChecker = + new CertificateChecker( + ImmutableSortedMap.of(START_OF_TIME, 825, DateTime.parse("2020-09-01T00:00:00Z"), 398), + 30, + 2048, + ImmutableSet.of("secp256r1", "secp384r1"), + clock); + @Test void testProvideClientCertificateHash() { HttpServletRequest req = mock(HttpServletRequest.class); @@ -53,11 +66,16 @@ final class TlsCredentialsTest { @Test void testClientCertificateAndHash_missing() { TlsCredentials tls = - new TlsCredentials(true, Optional.empty(), Optional.empty(), Optional.of("192.168.1.1")); + new TlsCredentials( + true, + Optional.empty(), + Optional.empty(), + Optional.of("192.168.1.1"), + certificateChecker); persistResource( loadRegistrar("TheRegistrar") .asBuilder() - .setClientCertificate(SAMPLE_CERT, DateTime.now(UTC)) + .setClientCertificate(SAMPLE_CERT, clock.nowUtc()) .build()); assertThrows( MissingRegistrarCertificateException.class, @@ -67,11 +85,12 @@ final class TlsCredentialsTest { @Test void test_missingIpAddress_doesntAllowAccess() { TlsCredentials tls = - new TlsCredentials(false, Optional.of("certHash"), Optional.empty(), Optional.empty()); + new TlsCredentials( + false, Optional.of("certHash"), Optional.empty(), Optional.empty(), certificateChecker); persistResource( loadRegistrar("TheRegistrar") .asBuilder() - .setClientCertificate(SAMPLE_CERT, DateTime.now(UTC)) + .setClientCertificate(SAMPLE_CERT, clock.nowUtc()) .setIpAddressAllowList(ImmutableSet.of(CidrAddressBlock.create("3.5.8.13"))) .build()); assertThrows( @@ -83,12 +102,16 @@ final class TlsCredentialsTest { void test_validateCertificate_canBeConfiguredToBypassCertHashes() throws Exception { TlsCredentials tls = new TlsCredentials( - false, Optional.of("certHash"), Optional.of("cert"), Optional.of("192.168.1.1")); + false, + Optional.of("certHash"), + Optional.of("cert"), + Optional.of("192.168.1.1"), + certificateChecker); persistResource( loadRegistrar("TheRegistrar") .asBuilder() - .setClientCertificate(null, DateTime.now(UTC)) - .setFailoverClientCertificate(null, DateTime.now(UTC)) + .setClientCertificate(null, clock.nowUtc()) + .setFailoverClientCertificate(null, clock.nowUtc()) .build()); // This would throw a RegistrarCertificateNotConfiguredException if cert hashes were not // bypassed @@ -105,7 +128,11 @@ final class TlsCredentialsTest { void testClientCertificate_notConfigured() { TlsCredentials tls = new TlsCredentials( - true, Optional.of("hash"), Optional.of(SAMPLE_CERT), Optional.of("192.168.1.1")); + true, + Optional.of("hash"), + Optional.of(SAMPLE_CERT), + Optional.of("192.168.1.1"), + certificateChecker); persistResource(loadRegistrar("TheRegistrar").asBuilder().build()); assertThrows( RegistrarCertificateNotConfiguredException.class, @@ -116,12 +143,16 @@ final class TlsCredentialsTest { void test_validateCertificate_canBeConfiguredToBypassCerts() throws Exception { TlsCredentials tls = new TlsCredentials( - false, Optional.of("certHash"), Optional.of("cert"), Optional.of("192.168.1.1")); + false, + Optional.of("certHash"), + Optional.of("cert"), + Optional.of("192.168.1.1"), + certificateChecker); persistResource( loadRegistrar("TheRegistrar") .asBuilder() - .setClientCertificate(null, DateTime.now(UTC)) - .setFailoverClientCertificate(null, DateTime.now(UTC)) + .setClientCertificate(null, clock.nowUtc()) + .setFailoverClientCertificate(null, clock.nowUtc()) .build()); // This would throw a RegistrarCertificateNotConfiguredException if cert hashes wren't bypassed. tls.validateCertificate(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 9a91e3de5..2b0f6b4cd 100644 --- a/core/src/test/java/google/registry/flows/session/LoginFlowViaTlsTest.java +++ b/core/src/test/java/google/registry/flows/session/LoginFlowViaTlsTest.java @@ -15,14 +15,18 @@ package google.registry.flows.session; import static google.registry.testing.DatabaseHelper.persistResource; +import static google.registry.util.DateTimeUtils.START_OF_TIME; import static org.joda.time.DateTimeZone.UTC; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableSortedMap; import com.google.common.net.InetAddresses; import google.registry.flows.TlsCredentials; import google.registry.flows.TlsCredentials.BadRegistrarCertificateException; import google.registry.flows.TlsCredentials.BadRegistrarIpAddressException; import google.registry.flows.TlsCredentials.MissingRegistrarCertificateException; +import google.registry.flows.certs.CertificateChecker; import google.registry.model.registrar.Registrar; import google.registry.testing.CertificateSamples; import google.registry.util.CidrAddressBlock; @@ -33,9 +37,9 @@ import org.junit.jupiter.api.Test; /** Unit tests for {@link LoginFlow} when accessed via a TLS transport. */ public class LoginFlowViaTlsTest extends LoginFlowTestCase { - private static final Optional GOOD_CERT = Optional.of(CertificateSamples.SAMPLE_CERT); + private static final Optional GOOD_CERT = Optional.of(CertificateSamples.SAMPLE_CERT3); private static final Optional GOOD_CERT_HASH = - Optional.of(CertificateSamples.SAMPLE_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); @@ -43,11 +47,18 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase { private static final Optional BAD_IP = Optional.of("1.1.1.1"); private static final Optional GOOD_IPV6 = Optional.of("2001:db8::1"); private static final Optional BAD_IPV6 = Optional.of("2001:db8::2"); + private final CertificateChecker certificateChecker = + new CertificateChecker( + ImmutableSortedMap.of(START_OF_TIME, 825, DateTime.parse("2020-09-01T00:00:00Z"), 398), + 30, + 2048, + ImmutableSet.of("secp256r1", "secp384r1"), + clock); @Override protected Registrar.Builder getRegistrarBuilder() { return super.getRegistrarBuilder() - .setClientCertificate(CertificateSamples.SAMPLE_CERT, DateTime.now(UTC)) + .setClientCertificate(GOOD_CERT.get(), DateTime.now(UTC)) .setIpAddressAllowList( ImmutableList.of(CidrAddressBlock.create(InetAddresses.forString(GOOD_IP.get()), 32))); } @@ -55,7 +66,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase { @Test void testSuccess_withGoodCredentials() throws Exception { persistResource(getRegistrarBuilder().build()); - credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, GOOD_IP); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, GOOD_IP, certificateChecker); doSuccessfulTest("login_valid.xml"); } @@ -66,7 +77,8 @@ 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, GOOD_CERT, GOOD_IPV6); + credentials = + new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, GOOD_IPV6, certificateChecker); doSuccessfulTest("login_valid.xml"); } @@ -77,7 +89,8 @@ 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, GOOD_CERT, GOOD_IPV6); + credentials = + new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, GOOD_IPV6, certificateChecker); doSuccessfulTest("login_valid.xml"); } @@ -87,14 +100,14 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase { getRegistrarBuilder() .setIpAddressAllowList(ImmutableList.of(CidrAddressBlock.create("192.168.1.255/24"))) .build()); - credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, GOOD_IP); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, GOOD_IP, certificateChecker); doSuccessfulTest("login_valid.xml"); } @Test void testFailure_incorrectClientCertificateHash() { persistResource(getRegistrarBuilder().build()); - credentials = new TlsCredentials(true, BAD_CERT_HASH, BAD_CERT, GOOD_IP); + credentials = new TlsCredentials(true, BAD_CERT_HASH, BAD_CERT, GOOD_IP, certificateChecker); doFailingTest("login_valid.xml", BadRegistrarCertificateException.class); } @@ -102,14 +115,16 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase { // 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); + credentials = + new TlsCredentials(true, GOOD_CERT_HASH, Optional.empty(), GOOD_IP, certificateChecker); doSuccessfulTest("login_valid.xml"); } @Test void testFailure_missingClientCertificateAndHash() { persistResource(getRegistrarBuilder().build()); - credentials = new TlsCredentials(true, Optional.empty(), Optional.empty(), GOOD_IP); + credentials = + new TlsCredentials(true, Optional.empty(), Optional.empty(), GOOD_IP, certificateChecker); doFailingTest("login_valid.xml", MissingRegistrarCertificateException.class); } @@ -122,7 +137,8 @@ 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()); + credentials = + new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, Optional.empty(), certificateChecker); doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class); } @@ -135,7 +151,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); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, BAD_IP, certificateChecker); doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class); } @@ -148,7 +164,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); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, BAD_IPV6, certificateChecker); doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class); } } diff --git a/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java b/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java index afd095333..29d5defd5 100644 --- a/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java +++ b/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java @@ -38,6 +38,7 @@ import com.google.common.collect.ImmutableSortedMap; import com.google.common.collect.Range; import com.google.common.net.MediaType; import google.registry.flows.certs.CertificateChecker; +import google.registry.flows.certs.CertificateChecker.InsecureCertificateException; import google.registry.model.registrar.Registrar; import java.io.IOException; import java.util.Optional; @@ -389,9 +390,9 @@ class CreateRegistrarCommandTest extends CommandTestCase @Test void testFail_clientCertFileFlagWithViolation() throws Exception { fakeClock.setTo(DateTime.parse("2020-10-01T00:00:00Z")); - IllegalArgumentException thrown = + InsecureCertificateException thrown = assertThrows( - IllegalArgumentException.class, + InsecureCertificateException.class, () -> runCommandForced( "--name=blobio", @@ -419,9 +420,9 @@ class CreateRegistrarCommandTest extends CommandTestCase @Test void testFail_clientCertFileFlagWithMultipleViolations() throws Exception { fakeClock.setTo(DateTime.parse("2055-10-01T00:00:00Z")); - IllegalArgumentException thrown = + InsecureCertificateException thrown = assertThrows( - IllegalArgumentException.class, + InsecureCertificateException.class, () -> runCommandForced( "--name=blobio", @@ -476,9 +477,9 @@ class CreateRegistrarCommandTest extends CommandTestCase @Test void testFail_failoverClientCertFileFlagWithViolations() throws Exception { fakeClock.setTo(DateTime.parse("2020-11-01T00:00:00Z")); - IllegalArgumentException thrown = + InsecureCertificateException thrown = assertThrows( - IllegalArgumentException.class, + InsecureCertificateException.class, () -> runCommandForced( "--name=blobio", @@ -506,9 +507,9 @@ class CreateRegistrarCommandTest extends CommandTestCase @Test void testFail_failoverClientCertFileFlagWithMultipleViolations() throws Exception { fakeClock.setTo(DateTime.parse("2055-11-01T00:00:00Z")); - IllegalArgumentException thrown = + InsecureCertificateException thrown = assertThrows( - IllegalArgumentException.class, + InsecureCertificateException.class, () -> runCommandForced( "--name=blobio", diff --git a/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java b/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java index 43fee5cef..06ec102fe 100644 --- a/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java +++ b/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java @@ -33,6 +33,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; import google.registry.flows.certs.CertificateChecker; +import google.registry.flows.certs.CertificateChecker.InsecureCertificateException; import google.registry.model.registrar.Registrar; import google.registry.model.registrar.Registrar.State; import google.registry.model.registrar.Registrar.Type; @@ -265,9 +266,9 @@ class UpdateRegistrarCommandTest extends CommandTestCase Registrar registrar = loadRegistrar("NewRegistrar"); assertThat(registrar.getClientCertificate()).isEmpty(); assertThat(registrar.getClientCertificateHash()).isEmpty(); - IllegalArgumentException thrown = + InsecureCertificateException thrown = assertThrows( - IllegalArgumentException.class, + InsecureCertificateException.class, () -> runCommand("--cert_file=" + getCertFilename(), "--force", "NewRegistrar")); assertThat(thrown.getMessage()) .isEqualTo( @@ -282,9 +283,9 @@ class UpdateRegistrarCommandTest extends CommandTestCase Registrar registrar = loadRegistrar("NewRegistrar"); assertThat(registrar.getClientCertificate()).isEmpty(); assertThat(registrar.getClientCertificateHash()).isEmpty(); - IllegalArgumentException thrown = + InsecureCertificateException thrown = assertThrows( - IllegalArgumentException.class, + InsecureCertificateException.class, () -> runCommand("--cert_file=" + getCertFilename(), "--force", "NewRegistrar")); assertThat(thrown.getMessage()) .isEqualTo( @@ -298,9 +299,9 @@ class UpdateRegistrarCommandTest extends CommandTestCase fakeClock.setTo(DateTime.parse("2020-11-01T00:00:00Z")); Registrar registrar = loadRegistrar("NewRegistrar"); assertThat(registrar.getFailoverClientCertificate()).isEmpty(); - IllegalArgumentException thrown = + InsecureCertificateException thrown = assertThrows( - IllegalArgumentException.class, + InsecureCertificateException.class, () -> runCommand("--failover_cert_file=" + getCertFilename(), "--force", "NewRegistrar")); assertThat(thrown.getMessage()) @@ -315,9 +316,9 @@ class UpdateRegistrarCommandTest extends CommandTestCase fakeClock.setTo(DateTime.parse("2055-10-01T00:00:00Z")); Registrar registrar = loadRegistrar("NewRegistrar"); assertThat(registrar.getFailoverClientCertificate()).isEmpty(); - IllegalArgumentException thrown = + InsecureCertificateException thrown = assertThrows( - IllegalArgumentException.class, + InsecureCertificateException.class, () -> runCommand("--failover_cert_file=" + getCertFilename(), "--force", "NewRegistrar")); assertThat(thrown.getMessage()) diff --git a/core/src/test/java/google/registry/tools/ValidateLoginCredentialsCommandTest.java b/core/src/test/java/google/registry/tools/ValidateLoginCredentialsCommandTest.java index 1e41edc10..aca5ce70f 100644 --- a/core/src/test/java/google/registry/tools/ValidateLoginCredentialsCommandTest.java +++ b/core/src/test/java/google/registry/tools/ValidateLoginCredentialsCommandTest.java @@ -20,14 +20,17 @@ import static google.registry.testing.DatabaseHelper.createTld; import static google.registry.testing.DatabaseHelper.loadRegistrar; import static google.registry.testing.DatabaseHelper.persistResource; import static google.registry.testing.EppExceptionSubject.assertAboutEppExceptions; +import static google.registry.util.DateTimeUtils.START_OF_TIME; import static org.joda.time.DateTimeZone.UTC; import static org.junit.jupiter.api.Assertions.assertThrows; import com.beust.jcommander.ParameterException; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableSortedMap; import google.registry.flows.EppException; import google.registry.flows.TransportCredentials.BadRegistrarPasswordException; +import google.registry.flows.certs.CertificateChecker; import google.registry.model.registrar.Registrar; import google.registry.model.registrar.Registrar.State; import google.registry.testing.CertificateSamples; @@ -42,7 +45,7 @@ import org.junit.jupiter.api.Test; class ValidateLoginCredentialsCommandTest extends CommandTestCase { private static final String PASSWORD = "foo-BAR2"; - private static final String CERT_HASH = CertificateSamples.SAMPLE_CERT_HASH; + private static final String CERT_HASH = CertificateSamples.SAMPLE_CERT3_HASH; private static final String CLIENT_IP = "1.2.3.4"; @BeforeEach @@ -52,11 +55,18 @@ class ValidateLoginCredentialsCommandTest extends CommandTestCase