Skip to content

Commit

Permalink
Add certificate checks to RegistrarSettingsAction (#843)
Browse files Browse the repository at this point in the history 
* Add certificate checks to RegistrarSettingsAction

* Add some comments

* Add more functionality to CertificateChecker and update call sites

* Small code cleanups

* Small format fix
  • Loading branch information
@sarahcaseybot
sarahcaseybot committed Oct 23, 2020
1 parent f52e887 commit 576c05f
Show file tree
Hide file tree
Showing 10 changed files with 441 additions and 105 deletions.
2 changes: 2 additions & 0 deletions core/src/main/java/google/registry/module/frontend/FrontendComponent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
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;
Expand Down Expand Up @@ -44,6 +45,7 @@
@Component(
modules = {
AuthModule.class,
CertificateCheckerModule.class,
ConfigModule.class,
ConsoleConfigModule.class,
CredentialModule.class,
Expand Down
27 changes: 2 additions & 25 deletions core/src/main/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
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;
Expand All @@ -40,22 +39,16 @@
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;
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;
Expand Down Expand Up @@ -369,7 +362,7 @@ protected final void init() throws Exception {
// 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);
}
Expand All @@ -378,7 +371,7 @@ protected final void init() throws Exception {
String asciiCert =
new String(Files.readAllBytes(failoverClientCertificateFilename), US_ASCII);
if (!asciiCert.equals("")) {
verifyCertificate(asciiCert);
certificateChecker.validateCertificate(asciiCert);
}
builder.setFailoverClientCertificate(asciiCert, now);
}
Expand Down Expand Up @@ -482,22 +475,6 @@ protected final void init() throws Exception {
}
}

private void verifyCertificate(String certificateString) throws CertificateException {
X509Certificate certificate =
(X509Certificate)
CertificateFactory.getInstance("X509")
.generateCertificate(new ByteArrayInputStream(certificateString.getBytes(UTF_8)));
ImmutableSet<CertificateViolation> violations =
certificateChecker.checkCertificate(certificate);
if (!violations.isEmpty()) {
String displayMessages =
violations.stream()
.map(violation -> violation.getDisplayMessage(certificateChecker))
.collect(Collectors.joining("\n"));
throw new CertificateException(displayMessages);
}
}

@Override
protected String execute() throws Exception {
// Save registrar to Datastore and output its response
Expand Down
44 changes: 35 additions & 9 deletions core/src/main/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
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;
Expand Down Expand Up @@ -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() {}

Expand Down Expand Up @@ -306,19 +308,43 @@ private Registrar checkAndUpdateOwnerControlledFields(
RegistrarFormFields.IP_ADDRESS_ALLOW_LIST_FIELD
.extractUntyped(args)
.orElse(ImmutableList.of()));
RegistrarFormFields.CLIENT_CERTIFICATE_FIELD
.extractUntyped(args)
.ifPresent(
certificate -> builder.setClientCertificate(certificate, tm().getTransactionTime()));
RegistrarFormFields.FAILOVER_CLIENT_CERTIFICATE_FIELD
.extractUntyped(args)
.ifPresent(
certificate ->
builder.setFailoverClientCertificate(certificate, tm().getTransactionTime()));

Optional<String> certificateString =
RegistrarFormFields.CLIENT_CERTIFICATE_FIELD.extractUntyped(args);
if (certificateString.isPresent()) {
if (validateCertificate(initialRegistrar.getClientCertificate(), certificateString.get())) {
builder.setClientCertificate(certificateString.get(), tm().getTransactionTime());
}
}

Optional<String> failoverCertificateString =
RegistrarFormFields.FAILOVER_CLIENT_CERTIFICATE_FIELD.extractUntyped(args);
if (failoverCertificateString.isPresent()) {
if (validateCertificate(
initialRegistrar.getFailoverClientCertificate(), failoverCertificateString.get())) {
builder.setFailoverClientCertificate(
failoverCertificateString.get(), tm().getTransactionTime());
}
}

return checkNotChangedUnlessAllowed(builder, initialRegistrar, Role.OWNER);
}

/**
* Returns true if the registrar should accept the new certificate. Returns false if the
* certificate is already the one stored for the registrar.
*/
private boolean validateCertificate(String existingCertificate, String certificateString) {
if ((existingCertificate == null) || !existingCertificate.equals(certificateString)) {
// TODO(sarhabot): remove this check after November 1, 2020
if (tm().getTransactionTime().isAfter(DateTime.parse("2020-11-01T00:00:00Z"))) {
certificateChecker.validateCertificate(certificateString);
}
return true;
}
return false;
}

/**
* Updates a registrar with the ADMIN-controlled args from the http request.
*
Expand Down
61 changes: 9 additions & 52 deletions core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
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;
Expand Down Expand Up @@ -389,9 +388,9 @@ void testSuccess_clientCertFileFlag() throws Exception {
@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",
Expand Down Expand Up @@ -419,9 +418,9 @@ void testFail_clientCertFileFlagWithViolation() throws Exception {
@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",
Expand Down Expand Up @@ -499,9 +498,9 @@ void testSuccess_failoverClientCertFileFlag() throws Exception {
@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",
Expand Down Expand Up @@ -529,9 +528,9 @@ void testFail_failoverClientCertFileFlagWithViolations() throws Exception {
@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",
Expand Down Expand Up @@ -1181,52 +1180,10 @@ void testSuccess_ipAllowListFlagWithNull() {
"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",
Expand Down
17 changes: 8 additions & 9 deletions core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
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;
Expand Down Expand Up @@ -267,9 +266,9 @@ void testFail_certFileWithViolation() throws Exception {
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(
Expand All @@ -284,9 +283,9 @@ void testFail_certFileWithMultipleViolations() throws Exception {
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(
Expand All @@ -300,9 +299,9 @@ void testFail_failoverCertFileWithViolation() throws Exception {
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())
Expand All @@ -317,9 +316,9 @@ void testFail_failoverCertFileWithMultipleViolations() throws Exception {
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())
Expand Down
Loading

0 comments on commit 576c05f

Please sign in to comment.