Use better null-handling around registrar certificates (#922)

* Use better null-handling around registrar certificates

Now with Optional it's always very clear whether they do or do not have values.
isNullOrEmpty() shouldn't be necessary anymore (indeed it wasn't necessary prior
to this either, as the relevant setters in the Registrar builder already coerced
empty strings to null). And also the cert hash is a required HTTP header, so it
will error out in the Dagger component if null or empty long before getting to
any other code.

* Merge branch 'master' into optional-get-certs
This commit is contained in:
Ben McIlwain 2021-01-07 19:30:09 -05:00 committed by GitHub
parent 7a62aa0602
commit 9e03ae453c
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
16 changed files with 139 additions and 100 deletions

View file

@ -36,7 +36,8 @@ class EppLoginTlsTest extends EppTestCase {
void setClientCertificateHash(String clientCertificateHash) {
setTransportCredentials(
new TlsCredentials(true, clientCertificateHash, Optional.of("192.168.1.100:54321")));
new TlsCredentials(
true, Optional.ofNullable(clientCertificateHash), Optional.of("192.168.1.100:54321")));
}
@BeforeEach
@ -107,7 +108,7 @@ class EppLoginTlsTest extends EppTestCase {
@Test
void testGfeDidntProvideClientCertificate_failsMissingCertificate2200() throws Exception {
setClientCertificateHash("");
setClientCertificateHash(null);
assertThatLogin("NewRegistrar", "foo-BAR2")
.hasResponse(
"response_error.xml",

View file

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

View file

@ -14,7 +14,8 @@
package google.registry.flows;
import static com.google.common.truth.Truth.assertThat;
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;
@ -22,9 +23,12 @@ 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 google.registry.flows.TlsCredentials.BadRegistrarIpAddressException;
import google.registry.flows.TlsCredentials.MissingRegistrarCertificateException;
import google.registry.model.registrar.Registrar;
import google.registry.request.HttpException.BadRequestException;
import google.registry.testing.AppEngineExtension;
import google.registry.util.CidrAddressBlock;
import java.util.Optional;
import javax.servlet.http.HttpServletRequest;
import org.joda.time.DateTime;
@ -42,22 +46,40 @@ final class TlsCredentialsTest {
void testProvideClientCertificateHash() {
HttpServletRequest req = mock(HttpServletRequest.class);
when(req.getHeader("X-SSL-Certificate")).thenReturn("data");
assertThat(TlsCredentials.EppTlsModule.provideClientCertificateHash(req)).isEqualTo("data");
assertThat(TlsCredentials.EppTlsModule.provideClientCertificateHash(req)).hasValue("data");
}
@Test
void testProvideClientCertificateHash_missing() {
HttpServletRequest req = mock(HttpServletRequest.class);
BadRequestException thrown =
assertThrows(
BadRequestException.class,
() -> TlsCredentials.EppTlsModule.provideClientCertificateHash(req));
assertThat(thrown).hasMessageThat().contains("Missing header: X-SSL-Certificate");
void testClientCertificateHash_missing() {
TlsCredentials tls = new TlsCredentials(true, Optional.empty(), Optional.of("192.168.1.1"));
persistResource(
loadRegistrar("TheRegistrar")
.asBuilder()
.setClientCertificate(SAMPLE_CERT, DateTime.now(UTC))
.build());
assertThrows(
MissingRegistrarCertificateException.class,
() -> tls.validateCertificate(Registrar.loadByClientId("TheRegistrar").get()));
}
@Test
void test_missingIpAddress_doesntAllowAccess() {
TlsCredentials tls = new TlsCredentials(false, Optional.of("certHash"), Optional.empty());
persistResource(
loadRegistrar("TheRegistrar")
.asBuilder()
.setClientCertificate(SAMPLE_CERT, DateTime.now(UTC))
.setIpAddressAllowList(ImmutableSet.of(CidrAddressBlock.create("3.5.8.13")))
.build());
assertThrows(
BadRegistrarIpAddressException.class,
() -> tls.validate(Registrar.loadByClientId("TheRegistrar").get(), "password"));
}
@Test
void test_validateCertificate_canBeConfiguredToBypassCertHashes() throws Exception {
TlsCredentials tls = new TlsCredentials(false, "certHash", Optional.of("192.168.1.1"));
TlsCredentials tls =
new TlsCredentials(false, Optional.of("certHash"), Optional.of("192.168.1.1"));
persistResource(
loadRegistrar("TheRegistrar")
.asBuilder()

View file

@ -33,8 +33,10 @@ 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 String GOOD_CERT = CertificateSamples.SAMPLE_CERT_HASH;
private static final String BAD_CERT = CertificateSamples.SAMPLE_CERT2_HASH;
private static final Optional<String> GOOD_CERT =
Optional.of(CertificateSamples.SAMPLE_CERT_HASH);
private static final Optional<String> BAD_CERT =
Optional.of(CertificateSamples.SAMPLE_CERT2_HASH);
private static final Optional<String> GOOD_IP = Optional.of("192.168.1.1");
private static final Optional<String> BAD_IP = Optional.of("1.1.1.1");
private static final Optional<String> GOOD_IPV6 = Optional.of("2001:db8::1");
@ -97,7 +99,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
@Test
void testFailure_missingClientCertificateHash() {
persistResource(getRegistrarBuilder().build());
credentials = new TlsCredentials(true, null, GOOD_IP);
credentials = new TlsCredentials(true, Optional.empty(), GOOD_IP);
doFailingTest("login_valid.xml", MissingRegistrarCertificateException.class);
}

View file

@ -163,9 +163,9 @@ public final class OteAccountBuilderTest {
.buildAndPersist();
assertThat(Registrar.loadByClientId("myclientid-3").get().getClientCertificateHash())
.isEqualTo(SAMPLE_CERT_HASH);
.hasValue(SAMPLE_CERT_HASH);
assertThat(Registrar.loadByClientId("myclientid-3").get().getClientCertificate())
.isEqualTo(SAMPLE_CERT);
.hasValue(SAMPLE_CERT);
}
@Test

View file

@ -190,18 +190,18 @@ class RegistrarTest extends EntityTestCase {
fakeClock.advanceOneMilli();
registrar = registrar.asBuilder().setClientCertificate(SAMPLE_CERT, fakeClock.nowUtc()).build();
assertThat(registrar.getLastCertificateUpdateTime()).isEqualTo(fakeClock.nowUtc());
assertThat(registrar.getClientCertificate()).isEqualTo(SAMPLE_CERT);
assertThat(registrar.getClientCertificateHash()).isEqualTo(SAMPLE_CERT_HASH);
assertThat(registrar.getClientCertificate()).hasValue(SAMPLE_CERT);
assertThat(registrar.getClientCertificateHash()).hasValue(SAMPLE_CERT_HASH);
}
@TestOfyAndSql
void testDeleteCertificateHash_alsoDeletesHash() {
assertThat(registrar.getClientCertificateHash()).isNotNull();
assertThat(registrar.getClientCertificateHash()).isPresent();
fakeClock.advanceOneMilli();
registrar = registrar.asBuilder().setClientCertificate(null, fakeClock.nowUtc()).build();
assertThat(registrar.getLastCertificateUpdateTime()).isEqualTo(fakeClock.nowUtc());
assertThat(registrar.getClientCertificate()).isNull();
assertThat(registrar.getClientCertificateHash()).isNull();
assertThat(registrar.getClientCertificate()).isEmpty();
assertThat(registrar.getClientCertificateHash()).isEmpty();
}
@TestOfyAndSql
@ -213,21 +213,21 @@ class RegistrarTest extends EntityTestCase {
.setFailoverClientCertificate(SAMPLE_CERT2, fakeClock.nowUtc())
.build();
assertThat(registrar.getLastCertificateUpdateTime()).isEqualTo(fakeClock.nowUtc());
assertThat(registrar.getFailoverClientCertificate()).isEqualTo(SAMPLE_CERT2);
assertThat(registrar.getFailoverClientCertificateHash()).isEqualTo(SAMPLE_CERT2_HASH);
assertThat(registrar.getFailoverClientCertificate()).hasValue(SAMPLE_CERT2);
assertThat(registrar.getFailoverClientCertificateHash()).hasValue(SAMPLE_CERT2_HASH);
}
@TestOfyAndSql
void testDeleteFailoverCertificateHash_alsoDeletesHash() {
registrar =
registrar.asBuilder().setFailoverClientCertificate(SAMPLE_CERT, fakeClock.nowUtc()).build();
assertThat(registrar.getFailoverClientCertificateHash()).isNotNull();
assertThat(registrar.getFailoverClientCertificateHash()).isPresent();
fakeClock.advanceOneMilli();
registrar =
registrar.asBuilder().setFailoverClientCertificate(null, fakeClock.nowUtc()).build();
assertThat(registrar.getLastCertificateUpdateTime()).isEqualTo(fakeClock.nowUtc());
assertThat(registrar.getFailoverClientCertificate()).isNull();
assertThat(registrar.getFailoverClientCertificateHash()).isNull();
assertThat(registrar.getFailoverClientCertificate()).isEmpty();
assertThat(registrar.getFailoverClientCertificateHash()).isEmpty();
}
@TestOfyAndSql

View file

@ -95,7 +95,7 @@ class CreateRegistrarCommandTest extends CommandTestCase<CreateRegistrarCommand>
assertThat(registrar.getState()).isEqualTo(Registrar.State.ACTIVE);
assertThat(registrar.getAllowedTlds()).isEmpty();
assertThat(registrar.getIpAddressAllowList()).isEmpty();
assertThat(registrar.getClientCertificateHash()).isNull();
assertThat(registrar.getClientCertificateHash()).isEmpty();
assertThat(registrar.getPhonePasscode()).isEqualTo("01234");
assertThat(registrar.getCreationTime()).isIn(Range.closed(before, after));
assertThat(registrar.getLastUpdateTime()).isEqualTo(registrar.getCreationTime());
@ -383,7 +383,7 @@ class CreateRegistrarCommandTest extends CommandTestCase<CreateRegistrarCommand>
Optional<Registrar> registrar = Registrar.loadByClientId("clientz");
assertThat(registrar).isPresent();
assertThat(registrar.get().getClientCertificateHash()).isEqualTo(SAMPLE_CERT3_HASH);
assertThat(registrar.get().getClientCertificateHash()).hasValue(SAMPLE_CERT3_HASH);
}
@Test
@ -467,10 +467,10 @@ class CreateRegistrarCommandTest extends CommandTestCase<CreateRegistrarCommand>
Optional<Registrar> registrarOptional = Registrar.loadByClientId("clientz");
assertThat(registrarOptional).isPresent();
Registrar registrar = registrarOptional.get();
assertThat(registrar.getClientCertificate()).isNull();
assertThat(registrar.getClientCertificateHash()).isNull();
assertThat(registrar.getFailoverClientCertificate()).isEqualTo(SAMPLE_CERT3);
assertThat(registrar.getFailoverClientCertificateHash()).isEqualTo(SAMPLE_CERT3_HASH);
assertThat(registrar.getClientCertificate()).isEmpty();
assertThat(registrar.getClientCertificateHash()).isEmpty();
assertThat(registrar.getFailoverClientCertificate()).hasValue(SAMPLE_CERT3);
assertThat(registrar.getFailoverClientCertificateHash()).hasValue(SAMPLE_CERT3_HASH);
}
@Test

View file

@ -15,6 +15,7 @@
package google.registry.tools;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
import static google.registry.model.registrar.Registrar.State.ACTIVE;
import static google.registry.model.registry.Registry.TldState.GENERAL_AVAILABILITY;
import static google.registry.model.registry.Registry.TldState.START_DATE_SUNRISE;
@ -105,7 +106,7 @@ class SetupOteCommandTest extends CommandTestCase<SetupOteCommand> {
assertThat(registrar.getState()).isEqualTo(ACTIVE);
assertThat(registrar.verifyPassword(password)).isTrue();
assertThat(registrar.getIpAddressAllowList()).isEqualTo(ipAllowList);
assertThat(registrar.getClientCertificateHash()).isEqualTo(SAMPLE_CERT_HASH);
assertThat(registrar.getClientCertificateHash()).hasValue(SAMPLE_CERT_HASH);
}
private void verifyRegistrarContactCreation(String registrarName, String email) {

View file

@ -14,7 +14,6 @@
package google.registry.tools;
import static com.google.common.base.Strings.isNullOrEmpty;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
@ -250,22 +249,22 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
void testSuccess_certFile() throws Exception {
fakeClock.setTo(DateTime.parse("2020-11-01T00:00:00Z"));
Registrar registrar = loadRegistrar("NewRegistrar");
assertThat(registrar.getClientCertificate()).isNull();
assertThat(registrar.getClientCertificateHash()).isNull();
assertThat(registrar.getClientCertificate()).isEmpty();
assertThat(registrar.getClientCertificateHash()).isEmpty();
runCommand("--cert_file=" + getCertFilename(SAMPLE_CERT3), "--force", "NewRegistrar");
registrar = loadRegistrar("NewRegistrar");
// NB: Hash was computed manually using 'openssl x509 -fingerprint -sha256 -in ...' and then
// converting the result from a hex string to non-padded base64 encoded string.
assertThat(registrar.getClientCertificate()).isEqualTo(SAMPLE_CERT3);
assertThat(registrar.getClientCertificateHash()).isEqualTo(SAMPLE_CERT3_HASH);
assertThat(registrar.getClientCertificate()).hasValue(SAMPLE_CERT3);
assertThat(registrar.getClientCertificateHash()).hasValue(SAMPLE_CERT3_HASH);
}
@Test
void testFail_certFileWithViolation() throws Exception {
fakeClock.setTo(DateTime.parse("2020-11-01T00:00:00Z"));
Registrar registrar = loadRegistrar("NewRegistrar");
assertThat(registrar.getClientCertificate()).isNull();
assertThat(registrar.getClientCertificateHash()).isNull();
assertThat(registrar.getClientCertificate()).isEmpty();
assertThat(registrar.getClientCertificateHash()).isEmpty();
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
@ -274,15 +273,15 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
.isEqualTo(
"Certificate validity period is too long; it must be less than or equal to 398"
+ " days.");
assertThat(registrar.getClientCertificate()).isNull();
assertThat(registrar.getClientCertificate()).isEmpty();
}
@Test
void testFail_certFileWithMultipleViolations() throws Exception {
fakeClock.setTo(DateTime.parse("2055-10-01T00:00:00Z"));
Registrar registrar = loadRegistrar("NewRegistrar");
assertThat(registrar.getClientCertificate()).isNull();
assertThat(registrar.getClientCertificateHash()).isNull();
assertThat(registrar.getClientCertificate()).isEmpty();
assertThat(registrar.getClientCertificateHash()).isEmpty();
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
@ -291,14 +290,14 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
.isEqualTo(
"Certificate is expired.\nCertificate validity period is too long; it must be less"
+ " than or equal to 398 days.");
assertThat(registrar.getClientCertificate()).isNull();
assertThat(registrar.getClientCertificate()).isEmpty();
}
@Test
void testFail_failoverCertFileWithViolation() throws Exception {
fakeClock.setTo(DateTime.parse("2020-11-01T00:00:00Z"));
Registrar registrar = loadRegistrar("NewRegistrar");
assertThat(registrar.getFailoverClientCertificate()).isNull();
assertThat(registrar.getFailoverClientCertificate()).isEmpty();
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
@ -308,14 +307,14 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
.isEqualTo(
"Certificate validity period is too long; it must be less than or equal to 398"
+ " days.");
assertThat(registrar.getFailoverClientCertificate()).isNull();
assertThat(registrar.getFailoverClientCertificate()).isEmpty();
}
@Test
void testFail_failoverCertFileWithMultipleViolations() throws Exception {
fakeClock.setTo(DateTime.parse("2055-10-01T00:00:00Z"));
Registrar registrar = loadRegistrar("NewRegistrar");
assertThat(registrar.getFailoverClientCertificate()).isNull();
assertThat(registrar.getFailoverClientCertificate()).isEmpty();
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
@ -325,17 +324,17 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
.isEqualTo(
"Certificate is expired.\nCertificate validity period is too long; it must be less"
+ " than or equal to 398 days.");
assertThat(registrar.getFailoverClientCertificate()).isNull();
assertThat(registrar.getFailoverClientCertificate()).isEmpty();
}
@Test
void testSuccess_failoverCertFile() throws Exception {
fakeClock.setTo(DateTime.parse("2020-11-01T00:00:00Z"));
Registrar registrar = loadRegistrar("NewRegistrar");
assertThat(registrar.getFailoverClientCertificate()).isNull();
assertThat(registrar.getFailoverClientCertificate()).isEmpty();
runCommand("--failover_cert_file=" + getCertFilename(SAMPLE_CERT3), "--force", "NewRegistrar");
registrar = loadRegistrar("NewRegistrar");
assertThat(registrar.getFailoverClientCertificate()).isEqualTo(SAMPLE_CERT3);
assertThat(registrar.getFailoverClientCertificate()).hasValue(SAMPLE_CERT3);
}
@Test
@ -345,9 +344,9 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
.asBuilder()
.setClientCertificate(SAMPLE_CERT, DateTime.now(UTC))
.build());
assertThat(isNullOrEmpty(loadRegistrar("NewRegistrar").getClientCertificate())).isFalse();
assertThat(loadRegistrar("NewRegistrar").getClientCertificate()).isPresent();
runCommand("--cert_file=/dev/null", "--force", "NewRegistrar");
assertThat(loadRegistrar("NewRegistrar").getClientCertificate()).isNull();
assertThat(loadRegistrar("NewRegistrar").getClientCertificate()).isEmpty();
}
@Test

View file

@ -371,7 +371,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
clock.setTo(DateTime.parse("2020-11-02T00:00:00Z"));
doTestUpdate(
Role.OWNER,
Registrar::getClientCertificate,
r -> r.getClientCertificate().orElse(null),
CertificateSamples.SAMPLE_CERT3,
(builder, s) -> builder.setClientCertificate(s, clock.nowUtc()));
}
@ -431,7 +431,7 @@ class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase {
clock.setTo(DateTime.parse("2020-11-02T00:00:00Z"));
doTestUpdate(
Role.OWNER,
Registrar::getFailoverClientCertificate,
r -> r.getFailoverClientCertificate().orElse(null),
CertificateSamples.SAMPLE_CERT3,
(builder, s) -> builder.setFailoverClientCertificate(s, clock.nowUtc()));
}

View file

@ -15,6 +15,7 @@
package google.registry.ui.server.registrar;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.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;
@ -121,10 +122,10 @@ class SecuritySettingsTest extends RegistrarSettingsActionTestCase {
"op", "update", "id", CLIENT_ID, "args", jsonMap));
assertThat(response).containsEntry("status", "SUCCESS");
Registrar registrar = loadRegistrar(CLIENT_ID);
assertThat(registrar.getClientCertificate()).isEqualTo(SAMPLE_CERT3);
assertThat(registrar.getClientCertificateHash()).isEqualTo(SAMPLE_CERT3_HASH);
assertThat(registrar.getFailoverClientCertificate()).isNull();
assertThat(registrar.getFailoverClientCertificateHash()).isNull();
assertThat(registrar.getClientCertificate()).hasValue(SAMPLE_CERT3);
assertThat(registrar.getClientCertificateHash()).hasValue(SAMPLE_CERT3_HASH);
assertThat(registrar.getFailoverClientCertificate()).isEmpty();
assertThat(registrar.getFailoverClientCertificateHash()).isEmpty();
assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
verifyNotificationEmailsSent();
}
@ -138,8 +139,8 @@ class SecuritySettingsTest extends RegistrarSettingsActionTestCase {
"op", "update", "id", CLIENT_ID, "args", jsonMap));
assertThat(response).containsEntry("status", "SUCCESS");
Registrar registrar = loadRegistrar(CLIENT_ID);
assertThat(registrar.getFailoverClientCertificate()).isEqualTo(SAMPLE_CERT3);
assertThat(registrar.getFailoverClientCertificateHash()).isEqualTo(SAMPLE_CERT3_HASH);
assertThat(registrar.getFailoverClientCertificate()).hasValue(SAMPLE_CERT3);
assertThat(registrar.getFailoverClientCertificateHash()).hasValue(SAMPLE_CERT3_HASH);
assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
verifyNotificationEmailsSent();
}
@ -160,10 +161,10 @@ class SecuritySettingsTest extends RegistrarSettingsActionTestCase {
"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.getFailoverClientCertificate()).isEqualTo(SAMPLE_CERT2);
assertThat(registrar.getFailoverClientCertificateHash()).isEqualTo(SAMPLE_CERT2_HASH);
assertThat(registrar.getClientCertificate()).hasValue(SAMPLE_CERT);
assertThat(registrar.getClientCertificateHash()).hasValue(SAMPLE_CERT_HASH);
assertThat(registrar.getFailoverClientCertificate()).hasValue(SAMPLE_CERT2);
assertThat(registrar.getFailoverClientCertificateHash()).hasValue(SAMPLE_CERT2_HASH);
assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
}