diff --git a/core/src/main/java/google/registry/module/frontend/FrontendComponent.java b/core/src/main/java/google/registry/module/frontend/FrontendComponent.java index 56e867886..98ff720ca 100644 --- a/core/src/main/java/google/registry/module/frontend/FrontendComponent.java +++ b/core/src/main/java/google/registry/module/frontend/FrontendComponent.java @@ -17,6 +17,7 @@ package google.registry.module.frontend; import com.google.monitoring.metrics.MetricReporter; import dagger.Component; import dagger.Lazy; +import google.registry.config.CertificateCheckerModule; import google.registry.config.CredentialModule; import google.registry.config.RegistryConfig.ConfigModule; import google.registry.flows.ServerTridProviderModule; @@ -44,6 +45,7 @@ import javax.inject.Singleton; @Component( modules = { AuthModule.class, + CertificateCheckerModule.class, ConfigModule.class, ConsoleConfigModule.class, CredentialModule.class, diff --git a/core/src/main/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java b/core/src/main/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java index 2103c3867..b55384ddf 100644 --- a/core/src/main/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java +++ b/core/src/main/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java @@ -22,7 +22,6 @@ import static google.registry.persistence.transaction.TransactionManagerFactory. import static google.registry.util.DomainNameUtils.canonicalizeDomainName; import static google.registry.util.RegistrarUtils.normalizeRegistrarName; import static java.nio.charset.StandardCharsets.US_ASCII; -import static java.nio.charset.StandardCharsets.UTF_8; import static org.joda.time.DateTimeZone.UTC; import com.beust.jcommander.Parameter; @@ -40,14 +39,9 @@ import google.registry.tools.params.OptionalPhoneNumberParameter; import google.registry.tools.params.OptionalStringParameter; import google.registry.tools.params.PathParameter; import google.registry.util.CertificateChecker; -import google.registry.util.CertificateChecker.CertificateViolation; import google.registry.util.CidrAddressBlock; -import java.io.ByteArrayInputStream; import java.nio.file.Files; import java.nio.file.Path; -import java.security.cert.CertificateException; -import java.security.cert.CertificateFactory; -import java.security.cert.X509Certificate; import java.util.ArrayList; import java.util.Arrays; import java.util.LinkedHashMap; @@ -55,7 +49,6 @@ import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; -import java.util.stream.Collectors; import javax.annotation.Nullable; import javax.inject.Inject; import org.joda.money.CurrencyUnit; @@ -369,7 +362,7 @@ abstract class CreateOrUpdateRegistrarCommand extends MutatingCommand { // existing certificate without providing a replacement. An uploaded empty certificate file // will prevent the registrar from being able to establish EPP connections. if (!asciiCert.equals("")) { - verifyCertificate(asciiCert); + certificateChecker.validateCertificate(asciiCert); } builder.setClientCertificate(asciiCert, now); } @@ -378,7 +371,7 @@ abstract class CreateOrUpdateRegistrarCommand extends MutatingCommand { String asciiCert = new String(Files.readAllBytes(failoverClientCertificateFilename), US_ASCII); if (!asciiCert.equals("")) { - verifyCertificate(asciiCert); + certificateChecker.validateCertificate(asciiCert); } builder.setFailoverClientCertificate(asciiCert, now); } @@ -482,22 +475,6 @@ abstract class CreateOrUpdateRegistrarCommand extends MutatingCommand { } } - private void verifyCertificate(String certificateString) throws CertificateException { - X509Certificate certificate = - (X509Certificate) - CertificateFactory.getInstance("X509") - .generateCertificate(new ByteArrayInputStream(certificateString.getBytes(UTF_8))); - ImmutableSet violations = - certificateChecker.checkCertificate(certificate); - if (!violations.isEmpty()) { - String displayMessages = - violations.stream() - .map(violation -> violation.getDisplayMessage(certificateChecker)) - .collect(Collectors.joining("\n")); - throw new CertificateException(displayMessages); - } - } - @Override protected String execute() throws Exception { // Save registrar to Datastore and output its response 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 6f8a32fb3..2d49938f2 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 @@ -56,6 +56,7 @@ import google.registry.ui.forms.FormFieldException; import google.registry.ui.server.RegistrarFormFields; import google.registry.ui.server.SendEmailUtils; import google.registry.util.AppEngineServiceUtils; +import google.registry.util.CertificateChecker; import google.registry.util.CollectionUtils; import google.registry.util.DiffUtils; import java.util.HashSet; @@ -92,6 +93,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA @Inject SendEmailUtils sendEmailUtils; @Inject AuthenticatedRegistrarAccessor registrarAccessor; @Inject AuthResult authResult; + @Inject CertificateChecker certificateChecker; @Inject RegistrarSettingsAction() {} @@ -306,19 +308,43 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA RegistrarFormFields.IP_ADDRESS_ALLOW_LIST_FIELD .extractUntyped(args) .orElse(ImmutableList.of())); - RegistrarFormFields.CLIENT_CERTIFICATE_FIELD - .extractUntyped(args) - .ifPresent( - certificate -> builder.setClientCertificate(certificate, tm().getTransactionTime())); - RegistrarFormFields.FAILOVER_CLIENT_CERTIFICATE_FIELD - .extractUntyped(args) - .ifPresent( - certificate -> - builder.setFailoverClientCertificate(certificate, tm().getTransactionTime())); + + Optional certificateString = + RegistrarFormFields.CLIENT_CERTIFICATE_FIELD.extractUntyped(args); + if (certificateString.isPresent()) { + if (validateCertificate(initialRegistrar.getClientCertificate(), certificateString.get())) { + builder.setClientCertificate(certificateString.get(), tm().getTransactionTime()); + } + } + + Optional failoverCertificateString = + RegistrarFormFields.FAILOVER_CLIENT_CERTIFICATE_FIELD.extractUntyped(args); + if (failoverCertificateString.isPresent()) { + if (validateCertificate( + initialRegistrar.getFailoverClientCertificate(), failoverCertificateString.get())) { + builder.setFailoverClientCertificate( + failoverCertificateString.get(), tm().getTransactionTime()); + } + } return checkNotChangedUnlessAllowed(builder, initialRegistrar, Role.OWNER); } + /** + * Returns true if the registrar should accept the new certificate. Returns false if the + * certificate is already the one stored for the registrar. + */ + private boolean validateCertificate(String existingCertificate, String certificateString) { + if ((existingCertificate == null) || !existingCertificate.equals(certificateString)) { + // TODO(sarhabot): remove this check after November 1, 2020 + if (tm().getTransactionTime().isAfter(DateTime.parse("2020-11-01T00:00:00Z"))) { + certificateChecker.validateCertificate(certificateString); + } + return true; + } + return false; + } + /** * Updates a registrar with the ADMIN-controlled args from the http request. * diff --git a/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java b/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java index a151953b1..7166a0823 100644 --- a/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java +++ b/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java @@ -40,7 +40,6 @@ import com.google.common.net.MediaType; import google.registry.model.registrar.Registrar; import google.registry.util.CertificateChecker; import java.io.IOException; -import java.security.cert.CertificateException; import java.util.Optional; import org.joda.money.CurrencyUnit; import org.joda.time.DateTime; @@ -389,9 +388,9 @@ class CreateRegistrarCommandTest extends CommandTestCase @Test void testFail_clientCertFileFlagWithViolation() throws Exception { fakeClock.setTo(DateTime.parse("2020-10-01T00:00:00Z")); - CertificateException thrown = + IllegalArgumentException thrown = assertThrows( - CertificateException.class, + IllegalArgumentException.class, () -> runCommandForced( "--name=blobio", @@ -419,9 +418,9 @@ class CreateRegistrarCommandTest extends CommandTestCase @Test void testFail_clientCertFileFlagWithMultipleViolations() throws Exception { fakeClock.setTo(DateTime.parse("2055-10-01T00:00:00Z")); - CertificateException thrown = + IllegalArgumentException thrown = assertThrows( - CertificateException.class, + IllegalArgumentException.class, () -> runCommandForced( "--name=blobio", @@ -499,9 +498,9 @@ class CreateRegistrarCommandTest extends CommandTestCase @Test void testFail_failoverClientCertFileFlagWithViolations() throws Exception { fakeClock.setTo(DateTime.parse("2020-11-01T00:00:00Z")); - CertificateException thrown = + IllegalArgumentException thrown = assertThrows( - CertificateException.class, + IllegalArgumentException.class, () -> runCommandForced( "--name=blobio", @@ -529,9 +528,9 @@ class CreateRegistrarCommandTest extends CommandTestCase @Test void testFail_failoverClientCertFileFlagWithMultipleViolations() throws Exception { fakeClock.setTo(DateTime.parse("2055-11-01T00:00:00Z")); - CertificateException thrown = + IllegalArgumentException thrown = assertThrows( - CertificateException.class, + IllegalArgumentException.class, () -> runCommandForced( "--name=blobio", @@ -1181,52 +1180,10 @@ class CreateRegistrarCommandTest extends CommandTestCase "clientz")); } - @Test - void testFailure_invalidCertFileContents() { - assertThrows( - CertificateException.class, - () -> - runCommandForced( - "--name=blobio", - "--password=some_password", - "--registrar_type=REAL", - "--iana_id=8", - "--cert_file=" + writeToTmpFile("ABCDEF"), - "--passcode=01234", - "--icann_referral_email=foo@bar.test", - "--street=\"123 Fake St\"", - "--city Fakington", - "--state MA", - "--zip 00351", - "--cc US", - "clientz")); - } - - @Test - void testFailure_invalidFailoverCertFileContents() { - assertThrows( - CertificateException.class, - () -> - runCommandForced( - "--name=blobio", - "--password=some_password", - "--registrar_type=REAL", - "--iana_id=8", - "--failover_cert_file=" + writeToTmpFile("ABCDEF"), - "--passcode=01234", - "--icann_referral_email=foo@bar.test", - "--street=\"123 Fake St\"", - "--city Fakington", - "--state MA", - "--zip 00351", - "--cc US", - "clientz")); - } - @Test void testFailure_certHashAndCertFile() { assertThrows( - CertificateException.class, + IllegalArgumentException.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 992a33f73..ef25389c2 100644 --- a/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java +++ b/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java @@ -41,7 +41,6 @@ import google.registry.persistence.VKey; import google.registry.testing.AppEngineExtension; import google.registry.util.CertificateChecker; import google.registry.util.CidrAddressBlock; -import java.security.cert.CertificateException; import java.util.Optional; import org.joda.money.CurrencyUnit; import org.joda.time.DateTime; @@ -267,9 +266,9 @@ class UpdateRegistrarCommandTest extends CommandTestCase Registrar registrar = loadRegistrar("NewRegistrar"); assertThat(registrar.getClientCertificate()).isNull(); assertThat(registrar.getClientCertificateHash()).isNull(); - CertificateException thrown = + IllegalArgumentException thrown = assertThrows( - CertificateException.class, + IllegalArgumentException.class, () -> runCommand("--cert_file=" + getCertFilename(), "--force", "NewRegistrar")); assertThat(thrown.getMessage()) .isEqualTo( @@ -284,9 +283,9 @@ class UpdateRegistrarCommandTest extends CommandTestCase Registrar registrar = loadRegistrar("NewRegistrar"); assertThat(registrar.getClientCertificate()).isNull(); assertThat(registrar.getClientCertificateHash()).isNull(); - CertificateException thrown = + IllegalArgumentException thrown = assertThrows( - CertificateException.class, + IllegalArgumentException.class, () -> runCommand("--cert_file=" + getCertFilename(), "--force", "NewRegistrar")); assertThat(thrown.getMessage()) .isEqualTo( @@ -300,9 +299,9 @@ class UpdateRegistrarCommandTest extends CommandTestCase fakeClock.setTo(DateTime.parse("2020-11-01T00:00:00Z")); Registrar registrar = loadRegistrar("NewRegistrar"); assertThat(registrar.getFailoverClientCertificate()).isNull(); - CertificateException thrown = + IllegalArgumentException thrown = assertThrows( - CertificateException.class, + IllegalArgumentException.class, () -> runCommand("--failover_cert_file=" + getCertFilename(), "--force", "NewRegistrar")); assertThat(thrown.getMessage()) @@ -317,9 +316,9 @@ class UpdateRegistrarCommandTest extends CommandTestCase fakeClock.setTo(DateTime.parse("2055-10-01T00:00:00Z")); Registrar registrar = loadRegistrar("NewRegistrar"); assertThat(registrar.getFailoverClientCertificate()).isNull(); - CertificateException thrown = + IllegalArgumentException thrown = assertThrows( - CertificateException.class, + IllegalArgumentException.class, () -> runCommand("--failover_cert_file=" + getCertFilename(), "--force", "NewRegistrar")); assertThat(thrown.getMessage()) diff --git a/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java b/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java index 5b0ddb55c..2c9b1dd70 100644 --- a/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java +++ b/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java @@ -43,6 +43,7 @@ import google.registry.util.EmailMessage; import java.util.Map; import java.util.function.BiFunction; import java.util.function.Function; +import org.joda.time.DateTime; import org.json.simple.JSONValue; import org.json.simple.parser.ParseException; import org.junit.jupiter.api.Test; @@ -367,6 +368,18 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase { @Test void testUpdate_clientCertificate() { + clock.setTo(DateTime.parse("2020-11-02T00:00:00Z")); + doTestUpdate( + Role.OWNER, + Registrar::getClientCertificate, + CertificateSamples.SAMPLE_CERT3, + (builder, s) -> builder.setClientCertificate(s, clock.nowUtc())); + } + + @Test + void testUpdate_clientCertificateWithViolationsBeforeNovemberSucceeds() { + // TODO(sarahbot): remove this test after November 1, 2020. + clock.setTo(DateTime.parse("2018-07-02T00:00:00Z")); doTestUpdate( Role.OWNER, Registrar::getClientCertificate, @@ -374,15 +387,216 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase { (builder, s) -> builder.setClientCertificate(s, clock.nowUtc())); } + @Test + void testUpdate_otherFieldsWhenClientCertificateWithViolationsAlreadyExistedSucceeds() { + // TODO(sarahbot): remove this test after November 1, 2020. + + // The frontend will always send the entire registrar entity back for an update, so the checks + // on the certificate should only run if a new certificate is being uploaded. All other updates + // after November 1st should still succeed even if a bad certificate is stored. + + // Set a bad certificate before checks on uploads are enforced + clock.setTo(DateTime.parse("2018-07-02T00:00:00Z")); + Registrar existingRegistrar = loadRegistrar(CLIENT_ID); + existingRegistrar = + existingRegistrar + .asBuilder() + .setClientCertificate(CertificateSamples.SAMPLE_CERT, clock.nowUtc()) + .build(); + persistResource(existingRegistrar); + + // Update the other registrar fields after enforcement begins should succeed + clock.setTo(DateTime.parse("2020-11-02T00:00:00Z")); + Map args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap()); + args.put("url", "test.url"); + args.put("phoneNumber", "+1.1234567890"); + Map response = + action.handleJsonRequest( + ImmutableMap.of( + "op", "update", + "id", CLIENT_ID, + "args", args)); + + assertThat(response).containsEntry("status", "SUCCESS"); + assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS"); + } + + @Test + void testUpdate_clientCertificateWithViolationsAlreadyExistedSucceeds() { + // TODO(sarahbot): remove this test after November 1, 2020. + + // The frontend will always send the entire registrar entity back for an update, so the checks + // on the certificate should only run if it is a new certificate + + // Set a bad certificate before checks on uploads are enforced + clock.setTo(DateTime.parse("2018-07-02T00:00:00Z")); + Registrar existingRegistrar = loadRegistrar(CLIENT_ID); + existingRegistrar = + existingRegistrar + .asBuilder() + .setClientCertificate(CertificateSamples.SAMPLE_CERT, clock.nowUtc()) + .build(); + persistResource(existingRegistrar); + + // Update with the same certificate after enforcement starts + clock.setTo(DateTime.parse("2020-11-02T00:00:00Z")); + Map args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap()); + args.put("clientCertificate", CertificateSamples.SAMPLE_CERT); + Map response = + action.handleJsonRequest( + ImmutableMap.of( + "op", "update", + "id", CLIENT_ID, + "args", args)); + + assertThat(response).containsEntry("status", "SUCCESS"); + assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS"); + assertNoTasksEnqueued("sheet"); + } + + @Test + void testUpdate_clientCertificateWithViolationsFails() { + clock.setTo(DateTime.parse("2020-11-02T00:00:00Z")); + Map args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap()); + args.put("clientCertificate", CertificateSamples.SAMPLE_CERT); + Map response = + action.handleJsonRequest( + ImmutableMap.of( + "op", "update", + "id", CLIENT_ID, + "args", args)); + + assertThat(response) + .containsExactly( + "status", + "ERROR", + "results", + ImmutableList.of(), + "message", + "Certificate validity period is too long; it must be less than or equal to 398" + + " days."); + assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: IllegalArgumentException"); + assertNoTasksEnqueued("sheet"); + } + + @Test + void testUpdate_clientCertificateWithMultipleViolationsFails() { + clock.setTo(DateTime.parse("2055-11-01T00:00:00Z")); + Map args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap()); + args.put("clientCertificate", CertificateSamples.SAMPLE_CERT); + Map response = + action.handleJsonRequest( + ImmutableMap.of( + "op", "update", + "id", CLIENT_ID, + "args", args)); + + assertThat(response) + .containsExactly( + "status", + "ERROR", + "results", + ImmutableList.of(), + "message", + "Certificate is expired.\nCertificate validity period is too long; it must be less" + + " than or equal to 398 days."); + assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: IllegalArgumentException"); + assertNoTasksEnqueued("sheet"); + } + @Test void testUpdate_failoverClientCertificate() { + clock.setTo(DateTime.parse("2020-11-02T00:00:00Z")); doTestUpdate( Role.OWNER, Registrar::getFailoverClientCertificate, - CertificateSamples.SAMPLE_CERT, + CertificateSamples.SAMPLE_CERT3, (builder, s) -> builder.setFailoverClientCertificate(s, clock.nowUtc())); } + @Test + void testUpdate_failoverClientCertificateWithViolationsAlreadyExistedSucceeds() { + // TODO(sarahbot): remove this test after November 1, 2020. + + // The frontend will always send the entire registrar entity back for an update, so the checks + // on the certificate should only run if it is a new certificate + + // Set a bad certificate before checks on uploads are enforced + clock.setTo(DateTime.parse("2018-07-02T00:00:00Z")); + Registrar existingRegistrar = loadRegistrar(CLIENT_ID); + existingRegistrar = + existingRegistrar + .asBuilder() + .setFailoverClientCertificate(CertificateSamples.SAMPLE_CERT, clock.nowUtc()) + .build(); + persistResource(existingRegistrar); + + // Update with the same certificate after enforcement starts + clock.setTo(DateTime.parse("2020-11-02T00:00:00Z")); + Map args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap()); + args.put("failoverClientCertificate", CertificateSamples.SAMPLE_CERT); + Map response = + action.handleJsonRequest( + ImmutableMap.of( + "op", "update", + "id", CLIENT_ID, + "args", args)); + + assertThat(response).containsEntry("status", "SUCCESS"); + assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS"); + assertNoTasksEnqueued("sheet"); + } + + @Test + void testUpdate_failoverClientCertificateWithViolationsFails() { + clock.setTo(DateTime.parse("2020-11-02T00:00:00Z")); + Map args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap()); + args.put("failoverClientCertificate", CertificateSamples.SAMPLE_CERT); + Map response = + action.handleJsonRequest( + ImmutableMap.of( + "op", "update", + "id", CLIENT_ID, + "args", args)); + + assertThat(response) + .containsExactly( + "status", + "ERROR", + "results", + ImmutableList.of(), + "message", + "Certificate validity period is too long; it must be less than or equal to 398" + + " days."); + assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: IllegalArgumentException"); + assertNoTasksEnqueued("sheet"); + } + + @Test + void testUpdate_failoverClientCertificateWithMultipleViolationsFails() { + clock.setTo(DateTime.parse("2055-11-01T00:00:00Z")); + Map args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap()); + args.put("failoverClientCertificate", CertificateSamples.SAMPLE_CERT); + Map response = + action.handleJsonRequest( + ImmutableMap.of( + "op", "update", + "id", CLIENT_ID, + "args", args)); + + assertThat(response) + .containsExactly( + "status", + "ERROR", + "results", + ImmutableList.of(), + "message", + "Certificate is expired.\nCertificate validity period is too long; it must be less" + + " than or equal to 398 days."); + assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: IllegalArgumentException"); + assertNoTasksEnqueued("sheet"); + } + @Test void testUpdate_allowedTlds() { doTestUpdate( diff --git a/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java b/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java index a1a783294..0416337a9 100644 --- a/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java +++ b/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java @@ -24,6 +24,7 @@ import static google.registry.security.JsonHttpTestUtils.createJsonPayload; import static google.registry.testing.DatastoreHelper.createTlds; import static google.registry.testing.DatastoreHelper.disallowRegistrarAccess; import static google.registry.testing.DatastoreHelper.loadRegistrar; +import static google.registry.util.DateTimeUtils.START_OF_TIME; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -31,6 +32,7 @@ import com.google.appengine.api.users.User; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSetMultimap; +import com.google.common.collect.ImmutableSortedMap; import com.google.common.truth.Truth; import google.registry.model.ofy.Ofy; import google.registry.model.registrar.RegistrarContact; @@ -46,6 +48,7 @@ import google.registry.testing.FakeClock; import google.registry.testing.InjectExtension; import google.registry.ui.server.SendEmailUtils; import google.registry.util.AppEngineServiceUtils; +import google.registry.util.CertificateChecker; import google.registry.util.EmailMessage; import google.registry.util.SendEmailService; import java.io.PrintWriter; @@ -115,6 +118,12 @@ public abstract class RegistrarSettingsActionTestCase { AuthResult.create( AuthLevel.USER, UserAuthInfo.create(new User("user@email.com", "email.com", "12345"), false)); + action.certificateChecker = + new CertificateChecker( + ImmutableSortedMap.of(START_OF_TIME, 825, DateTime.parse("2020-09-01T00:00:00Z"), 398), + 30, + 2048, + clock); inject.setStaticField(Ofy.class, "clock", clock); when(req.getMethod()).thenReturn("POST"); when(rsp.getWriter()).thenReturn(new PrintWriter(writer)); diff --git a/core/src/test/java/google/registry/ui/server/registrar/SecuritySettingsTest.java b/core/src/test/java/google/registry/ui/server/registrar/SecuritySettingsTest.java index 380f1c379..efa6b83a7 100644 --- a/core/src/test/java/google/registry/ui/server/registrar/SecuritySettingsTest.java +++ b/core/src/test/java/google/registry/ui/server/registrar/SecuritySettingsTest.java @@ -18,6 +18,8 @@ import static com.google.common.truth.Truth.assertThat; import static google.registry.testing.CertificateSamples.SAMPLE_CERT; import static google.registry.testing.CertificateSamples.SAMPLE_CERT2; import static google.registry.testing.CertificateSamples.SAMPLE_CERT2_HASH; +import static google.registry.testing.CertificateSamples.SAMPLE_CERT3; +import static google.registry.testing.CertificateSamples.SAMPLE_CERT3_HASH; import static google.registry.testing.CertificateSamples.SAMPLE_CERT_HASH; import static google.registry.testing.DatastoreHelper.loadRegistrar; import static google.registry.testing.DatastoreHelper.persistResource; @@ -27,6 +29,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import google.registry.model.registrar.Registrar; import java.util.Map; +import org.joda.time.DateTime; import org.junit.jupiter.api.Test; /** @@ -38,10 +41,11 @@ class SecuritySettingsTest extends RegistrarSettingsActionTestCase { @Test void testPost_updateCert_success() throws Exception { + clock.setTo(DateTime.parse("2020-11-01T00:00:00Z")); Registrar modified = loadRegistrar(CLIENT_ID) .asBuilder() - .setClientCertificate(SAMPLE_CERT, clock.nowUtc()) + .setClientCertificate(SAMPLE_CERT3, clock.nowUtc()) .build(); Map response = action.handleJsonRequest(ImmutableMap.of( "op", "update", @@ -67,17 +71,58 @@ class SecuritySettingsTest extends RegistrarSettingsActionTestCase { assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: FormFieldException"); } + @Test + void testPost_updateCertWithViolations_failure() { + clock.setTo(DateTime.parse("2055-11-01T00:00:00Z")); + Map reqJson = loadRegistrar(CLIENT_ID).toJsonMap(); + reqJson.put("clientCertificate", SAMPLE_CERT); + Map response = + action.handleJsonRequest( + ImmutableMap.of( + "op", "update", + "id", CLIENT_ID, + "args", reqJson)); + assertThat(response).containsEntry("status", "ERROR"); + assertThat(response) + .containsEntry( + "message", + "Certificate is expired.\nCertificate validity period is too long; it must be less" + + " than or equal to 398 days."); + assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: IllegalArgumentException"); + } + + @Test + void testPost_updateFailoverCertWithViolations_failure() { + clock.setTo(DateTime.parse("2055-11-01T00:00:00Z")); + Map reqJson = loadRegistrar(CLIENT_ID).toJsonMap(); + reqJson.put("failoverClientCertificate", SAMPLE_CERT2); + Map response = + action.handleJsonRequest( + ImmutableMap.of( + "op", "update", + "id", CLIENT_ID, + "args", reqJson)); + assertThat(response).containsEntry("status", "ERROR"); + assertThat(response) + .containsEntry( + "message", + "Certificate is expired.\nCertificate validity period is too long; it must be less" + + " than or equal to 398 days."); + assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: IllegalArgumentException"); + } + @Test void testChangeCertificates() throws Exception { + clock.setTo(DateTime.parse("2020-11-01T00:00:00Z")); Map jsonMap = loadRegistrar(CLIENT_ID).toJsonMap(); - jsonMap.put("clientCertificate", SAMPLE_CERT); + jsonMap.put("clientCertificate", SAMPLE_CERT3); jsonMap.put("failoverClientCertificate", null); Map response = action.handleJsonRequest(ImmutableMap.of( "op", "update", "id", CLIENT_ID, "args", jsonMap)); assertThat(response).containsEntry("status", "SUCCESS"); Registrar registrar = loadRegistrar(CLIENT_ID); - assertThat(registrar.getClientCertificate()).isEqualTo(SAMPLE_CERT); - assertThat(registrar.getClientCertificateHash()).isEqualTo(SAMPLE_CERT_HASH); + assertThat(registrar.getClientCertificate()).isEqualTo(SAMPLE_CERT3); + assertThat(registrar.getClientCertificateHash()).isEqualTo(SAMPLE_CERT3_HASH); assertThat(registrar.getFailoverClientCertificate()).isNull(); assertThat(registrar.getFailoverClientCertificateHash()).isNull(); assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS"); @@ -86,14 +131,15 @@ class SecuritySettingsTest extends RegistrarSettingsActionTestCase { @Test void testChangeFailoverCertificate() throws Exception { + clock.setTo(DateTime.parse("2020-11-01T00:00:00Z")); Map jsonMap = loadRegistrar(CLIENT_ID).toJsonMap(); - jsonMap.put("failoverClientCertificate", SAMPLE_CERT2); + jsonMap.put("failoverClientCertificate", SAMPLE_CERT3); Map response = action.handleJsonRequest(ImmutableMap.of( "op", "update", "id", CLIENT_ID, "args", jsonMap)); assertThat(response).containsEntry("status", "SUCCESS"); Registrar registrar = loadRegistrar(CLIENT_ID); - assertThat(registrar.getFailoverClientCertificate()).isEqualTo(SAMPLE_CERT2); - assertThat(registrar.getFailoverClientCertificateHash()).isEqualTo(SAMPLE_CERT2_HASH); + assertThat(registrar.getFailoverClientCertificate()).isEqualTo(SAMPLE_CERT3); + assertThat(registrar.getFailoverClientCertificateHash()).isEqualTo(SAMPLE_CERT3_HASH); assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS"); verifyNotificationEmailsSent(); } diff --git a/util/src/main/java/google/registry/util/CertificateChecker.java b/util/src/main/java/google/registry/util/CertificateChecker.java index 6f177f558..06b591d96 100644 --- a/util/src/main/java/google/registry/util/CertificateChecker.java +++ b/util/src/main/java/google/registry/util/CertificateChecker.java @@ -16,13 +16,18 @@ package google.registry.util; import static com.google.common.base.Preconditions.checkArgument; import static google.registry.util.DateTimeUtils.START_OF_TIME; +import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; +import java.io.ByteArrayInputStream; import java.security.PublicKey; +import java.security.cert.CertificateException; +import java.security.cert.CertificateFactory; import java.security.cert.X509Certificate; import java.security.interfaces.RSAPublicKey; import java.util.Date; +import java.util.stream.Collectors; import org.joda.time.DateTime; import org.joda.time.Days; @@ -68,8 +73,23 @@ public class CertificateChecker { } /** - * Checks a certificate for violations and returns a list of all the violations the certificate - * has. + * Checks the given certificate string for violations and throws an exception if any violations + * exist. + */ + public void validateCertificate(String certificateString) { + ImmutableSet violations = checkCertificate(certificateString); + if (!violations.isEmpty()) { + String displayMessages = + violations.stream() + .map(violation -> getViolationDisplayMessage(violation)) + .collect(Collectors.joining("\n")); + throw new IllegalArgumentException(displayMessages); + } + } + + /** + * Checks a given certificate for violations and returns a list of all the violations the + * certificate has. */ public ImmutableSet checkCertificate(X509Certificate certificate) { ImmutableSet.Builder violations = new ImmutableSet.Builder<>(); @@ -104,6 +124,25 @@ public class CertificateChecker { return violations.build(); } + /** + * Converts a given string to a certificate and checks it for violations, returning a list of all + * the violations the certificate has. + */ + public ImmutableSet checkCertificate(String certificateString) { + X509Certificate certificate; + + try { + certificate = + (X509Certificate) + CertificateFactory.getInstance("X509") + .generateCertificate(new ByteArrayInputStream(certificateString.getBytes(UTF_8))); + } catch (CertificateException e) { + throw new IllegalArgumentException("Unable to read given certificate."); + } + + return checkCertificate(certificate); + } + /** * Returns whether the certificate is nearing expiration. * diff --git a/util/src/test/java/google/registry/util/CertificateCheckerTest.java b/util/src/test/java/google/registry/util/CertificateCheckerTest.java index 4704e7bb2..15511a5ee 100644 --- a/util/src/test/java/google/registry/util/CertificateCheckerTest.java +++ b/util/src/test/java/google/registry/util/CertificateCheckerTest.java @@ -21,6 +21,7 @@ import static google.registry.util.CertificateChecker.CertificateViolation.NOT_Y import static google.registry.util.CertificateChecker.CertificateViolation.RSA_KEY_LENGTH_TOO_SHORT; import static google.registry.util.CertificateChecker.CertificateViolation.VALIDITY_LENGTH_TOO_LONG; import static google.registry.util.DateTimeUtils.START_OF_TIME; +import static org.junit.jupiter.api.Assertions.assertThrows; import com.google.common.collect.ImmutableSortedMap; import google.registry.testing.FakeClock; @@ -35,6 +36,54 @@ import org.junit.jupiter.api.Test; class CertificateCheckerTest { private static final String SSL_HOST = "www.example.tld"; + private static final String GOOD_CERTIFICATE = + "-----BEGIN CERTIFICATE-----\n" + + "MIIDyzCCArOgAwIBAgIUJnhiVrxAxgwkLJzHPm1w/lBoNs4wDQYJKoZIhvcNAQEL\n" + + "BQAwdTELMAkGA1UEBhMCVVMxETAPBgNVBAgMCE5ldyBZb3JrMREwDwYDVQQHDAhO\n" + + "ZXcgWW9yazEPMA0GA1UECgwGR29vZ2xlMR0wGwYDVQQLDBRkb21haW4tcmVnaXN0\n" + + "cnktdGVzdDEQMA4GA1UEAwwHY2xpZW50MTAeFw0yMDEwMTIxNzU5NDFaFw0yMTA0\n" + + "MzAxNzU5NDFaMHUxCzAJBgNVBAYTAlVTMREwDwYDVQQIDAhOZXcgWW9yazERMA8G\n" + + "A1UEBwwITmV3IFlvcmsxDzANBgNVBAoMBkdvb2dsZTEdMBsGA1UECwwUZG9tYWlu\n" + + "LXJlZ2lzdHJ5LXRlc3QxEDAOBgNVBAMMB2NsaWVudDEwggEiMA0GCSqGSIb3DQEB\n" + + "AQUAA4IBDwAwggEKAoIBAQC0msirO7kXyGEC93stsNYGc02Z77Q2qfHFwaGYkUG8\n" + + "QvOF5SWN+jwTo5Td6Jj26A26a8MLCtK45TCBuMRNcUsHhajhT19ocphO20iY3zhi\n" + + "ycwV1id0iwME4kPd1m57BELRE9tUPOxF81/JQXdR1fwT5KRVHYRDWZhaZ5aBmlZY\n" + + "3t/H9Ly0RBYyApkMaGs3nlb94OOug6SouUfRt02S59ja3wsE2SVF/Eui647OXP7O\n" + + "QdYXofxuqLoNkE8EnAdl43/enGLiCIVd0G2lABibFF+gbxTtfgbg7YtfUZJdL+Mb\n" + + "RAcAtuLXEamNQ9H63JgVF16PlQVCDz2XyI3uCfPpDDiBAgMBAAGjUzBRMB0GA1Ud\n" + + "DgQWBBQ26bWk8qfEBjXs/xZ4m8JZyalnITAfBgNVHSMEGDAWgBQ26bWk8qfEBjXs\n" + + "/xZ4m8JZyalnITAPBgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3DQEBCwUAA4IBAQAZ\n" + + "VcsgslBKanKOieJ5ik2d9qzOMXKfBuWPRFWbkC3t9i5awhHqnGAaj6nICnnMZIyt\n" + + "rdx5lZW5aaQyf0EP/90JAA8Xmty4A6MXmEjQAMiCOpP3A7eeS6Xglgi8IOZl4/bg\n" + + "LonW62TUkilo5IiFt/QklFTeHIjXB+OvA8+2Quqyd+zp7v6KnhXjvaomim78DhwE\n" + + "0PIUnjmiRpGpHfTVioTdfhPHZ2Y93Y8K7juL93sQog9aBu5m9XRJCY6wGyWPE83i\n" + + "kmLfGzjcnaJ6kqCd9xQRFZ0JwHmGlkAQvFoeengbNUqSyjyVgsOoNkEsrWwe/JFO\n" + + "iqBvjEhJlvRoefvkdR98\n" + + "-----END CERTIFICATE-----\n"; + private static final String BAD_CERTIFICATE = + "-----BEGIN CERTIFICATE-----\n" + + "MIIDvTCCAqWgAwIBAgIJANoEy6mYwalPMA0GCSqGSIb3DQEBCwUAMHUxCzAJBgNV\n" + + "BAYTAlVTMREwDwYDVQQIDAhOZXcgWW9yazERMA8GA1UEBwwITmV3IFlvcmsxDzAN\n" + + "BgNVBAoMBkdvb2dsZTEdMBsGA1UECwwUZG9tYWluLXJlZ2lzdHJ5LXRlc3QxEDAO\n" + + "BgNVBAMMB2NsaWVudDIwHhcNMTUwODI2MTkyODU3WhcNNDMwMTExMTkyODU3WjB1\n" + + "MQswCQYDVQQGEwJVUzERMA8GA1UECAwITmV3IFlvcmsxETAPBgNVBAcMCE5ldyBZ\n" + + "b3JrMQ8wDQYDVQQKDAZHb29nbGUxHTAbBgNVBAsMFGRvbWFpbi1yZWdpc3RyeS10\n" + + "ZXN0MRAwDgYDVQQDDAdjbGllbnQyMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIB\n" + + "CgKCAQEAw2FtuDyoR+rUJHp6k7KwaoHGHPV1xnC8IpG9O0SZubOXrFrnBHggBsbu\n" + + "+DsknbHXjmoihSFFem0KQqJg5y34aDAHXQV3iqa7nDfb1x4oc5voVz9gqjdmGKNm\n" + + "WF4MTIPNMu8KY52M852mMCxODK+6MZYp7wCmVa63KdCm0bW/XsLgoA/+FVGwKLhf\n" + + "UqFzt10Cf+87zl4VHrSaJqcHBYM6yAO5lvkr5VC6g8rRQ+dJ+pBT2D99YpSF1aFc\n" + + "rWbBreIypixZAnXm/Xoogu6RnohS29VCJp2dXFAJmKXGwyKNQFXfEKxZBaBi8uKH\n" + + "XF459795eyF9xHgSckEgu7jZlxOk6wIDAQABo1AwTjAdBgNVHQ4EFgQUv26AsQyc\n" + + "kLOjkhqcFLOuueB33l4wHwYDVR0jBBgwFoAUv26AsQyckLOjkhqcFLOuueB33l4w\n" + + "DAYDVR0TBAUwAwEB/zANBgkqhkiG9w0BAQsFAAOCAQEANBuV+QDISSnGAEHKbR40\n" + + "zUYdOjdZ399zcFNqTSPHwmE0Qu8pbmXhofpBfjzrcv0tkVbhSLYnT22qhx7aDmhb\n" + + "bOS8CeVYCwl5eiDTkJly3pRZLzJpy+UT5z8SPxO3MrTqn+wuj0lBpWRTBCWYAUpr\n" + + "IFRmgVB3IwVb60UIuxhmuk8TVss2SzNrdhdt36eAIPJ0RWEb0KHYHi35Y6lt4f+t\n" + + "iVk+ZR0cCbHUs7Q1RqREXHd/ICuMRLY/MsadVQ9WDqVOridh198X/OIqdx/p9kvJ\n" + + "1R80jDcVGNhYVXLmHu4ho4xrOaliSYvUJSCmaaSEGVZ/xE5PI7S6A8RMdj0iXLSt\n" + + "Bg==\n" + + "-----END CERTIFICATE-----\n"; private FakeClock fakeClock = new FakeClock(); private CertificateChecker certificateChecker = @@ -189,6 +238,24 @@ class CertificateCheckerTest { .containsExactly(ALGORITHM_CONSTRAINED); } + @Test + void test_checkCertificate_validCertificateString() throws Exception { + fakeClock.setTo(DateTime.parse("2020-11-01T00:00:00Z")); + assertThat(certificateChecker.checkCertificate(GOOD_CERTIFICATE)).isEmpty(); + assertThat(certificateChecker.checkCertificate(BAD_CERTIFICATE)) + .containsExactly(VALIDITY_LENGTH_TOO_LONG); + } + + @Test + void test_checkCertificate_invalidCertificateString() throws Exception { + fakeClock.setTo(DateTime.parse("2020-11-01T00:00:00Z")); + IllegalArgumentException thrown = + assertThrows( + IllegalArgumentException.class, + () -> certificateChecker.checkCertificate("bad certificate string")); + assertThat(thrown).hasMessageThat().isEqualTo("Unable to read given certificate."); + } + @Test void test_isNearingExpiration_yesItIs() throws Exception { fakeClock.setTo(DateTime.parse("2021-09-20T00:00:00Z"));