diff --git a/core/src/main/java/google/registry/flows/TlsCredentials.java b/core/src/main/java/google/registry/flows/TlsCredentials.java index 59c6ee181..1364285a9 100644 --- a/core/src/main/java/google/registry/flows/TlsCredentials.java +++ b/core/src/main/java/google/registry/flows/TlsCredentials.java @@ -42,6 +42,9 @@ import javax.servlet.http.HttpServletRequest; *
*
X-SSL-Certificate *
This field should contain a base64 encoded digest of the client's TLS certificate. It is + * used only if the validation of the full certificate fails. + *
X-SSL-Full-Certificate + *
This field should contain a base64 encoding of the client's TLS certificate. It is * validated during an EPP login command against a known good value that is transmitted out of * band. *
X-Forwarded-For @@ -55,15 +58,18 @@ public class TlsCredentials implements TransportCredentials { private final boolean requireSslCertificates; private final Optional clientCertificateHash; + private final Optional clientCertificate; private final Optional clientInetAddr; @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) { this.requireSslCertificates = requireSslCertificates; this.clientCertificateHash = clientCertificateHash; + this.clientCertificate = clientCertificate; this.clientInetAddr = clientAddress.map(TlsCredentials::parseInetAddress); } @@ -91,7 +97,7 @@ public class TlsCredentials implements TransportCredentials { ImmutableList ipAddressAllowList = registrar.getIpAddressAllowList(); if (ipAddressAllowList.isEmpty()) { logger.atInfo().log( - "Skipping IP allow list check because %s doesn't have an IP allow list", + "Skipping IP allow list check because %s doesn't have an IP allow list.", registrar.getClientId()); return; } @@ -115,11 +121,46 @@ public class TlsCredentials implements TransportCredentials { /** * Verifies client SSL certificate is permitted to issue commands as {@code registrar}. * - * @throws MissingRegistrarCertificateException if frontend didn't send certificate hash header + * @throws MissingRegistrarCertificateException if frontend didn't send certificate header * @throws BadRegistrarCertificateException if registrar requires certificate and it didn't match */ @VisibleForTesting void validateCertificate(Registrar registrar) throws AuthenticationErrorException { + // Check that certificate is present in registrar object + if (!registrar.getClientCertificate().isPresent() + && !registrar.getFailoverClientCertificate().isPresent()) { + // Log an error and validate using certificate hash instead + // TODO(sarahbot): throw a RegistrarCertificateNotConfiguredException once hash is no longer + // used as failover + logger.atWarning().log( + "There is no certificate configured for registrar %s.", registrar.getClientId()); + } + // Check that the request included the full certificate + else if (!clientCertificate.isPresent()) { + // Log an error and validate using certificate hash instead + // TODO(sarahbot): throw a MissingRegistrarCertificateException once hash is no longer used as + // failover + logger.atWarning().log( + "Request from registrar %s did not include X-SSL-Full-Certificate.", + registrar.getClientId()); + } else { + // Check if the certificate is equal to the one on file for the registrar. + if (clientCertificate.equals(registrar.getClientCertificate()) + || clientCertificate.equals(registrar.getFailoverClientCertificate())) { + // successfully validated, return here since hash validation is not necessary + return; + } + // Log an error and validate using certificate hash instead + // TODO(sarahbot): throw a BadRegistrarCertificateException once hash is no longer used as + // failover + logger.atWarning().log("Non-matching certificate for registrar %s.", registrar.getClientId()); + } + validateCertificateHash(registrar); + } + + private void validateCertificateHash(Registrar registrar) throws AuthenticationErrorException { + // Check the certificate hash as a failover + // TODO(sarahbot): Remove hash checks once certificate checks are working. if (!registrar.getClientCertificateHash().isPresent() && !registrar.getFailoverClientCertificateHash().isPresent()) { if (requireSslCertificates) { @@ -130,14 +171,17 @@ public class TlsCredentials implements TransportCredentials { return; } } + // Check that the request included the certificate hash if (!clientCertificateHash.isPresent()) { - logger.atInfo().log("Request did not include X-SSL-Certificate"); + logger.atInfo().log( + "Request from registrar %s did not include X-SSL-Certificate.", registrar.getClientId()); throw new MissingRegistrarCertificateException(); } + // Check if the certificate hash is equal to the one on file for the registrar. if (!clientCertificateHash.equals(registrar.getClientCertificateHash()) && !clientCertificateHash.equals(registrar.getFailoverClientCertificateHash())) { logger.atWarning().log( - "bad certificate hash (%s) for %s, wanted either %s or %s", + "Non-matching certificate hash (%s) for %s, wanted either %s or %s.", clientCertificateHash, registrar.getClientId(), registrar.getClientCertificateHash(), @@ -202,6 +246,14 @@ public class TlsCredentials implements TransportCredentials { return extractOptionalHeader(req, "X-SSL-Certificate"); } + @Provides + @Header("X-SSL-Full-Certificate") + static Optional provideClientCertificate(HttpServletRequest req) { + // Note: This header is actually required, we just want to handle its absence explicitly + // by throwing an EPP exception rather than a generic Bad Request exception. + return extractOptionalHeader(req, "X-SSL-Full_Certificate"); + } + @Provides @Header("X-Forwarded-For") static Optional provideForwardedFor(HttpServletRequest req) { diff --git a/core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java b/core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java index 1f12adf2c..c62d12630 100644 --- a/core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java +++ b/core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java @@ -55,6 +55,7 @@ final class ValidateLoginCredentialsCommand implements CommandWithRemoteApi { validateWith = PathParameter.InputFile.class) private Path clientCertificatePath; + // TODO(sarahbot@): Remove this after hash fallback is removed @Nullable @Parameter( names = {"-h", "--cert_hash"}, @@ -72,15 +73,19 @@ final class ValidateLoginCredentialsCommand implements CommandWithRemoteApi { checkArgument( clientCertificatePath == null || isNullOrEmpty(clientCertificateHash), "Can't specify both --cert_hash and --cert_file"); + String clientCertificate = ""; if (clientCertificatePath != null) { - clientCertificateHash = getCertificateHash( - loadCertificate(new String(Files.readAllBytes(clientCertificatePath), US_ASCII))); + clientCertificate = new String(Files.readAllBytes(clientCertificatePath), US_ASCII); + clientCertificateHash = getCertificateHash(loadCertificate(clientCertificate)); } Registrar registrar = checkArgumentPresent( Registrar.loadByClientId(clientId), "Registrar %s not found", clientId); new TlsCredentials( - true, Optional.ofNullable(clientCertificateHash), Optional.ofNullable(clientIpAddress)) + true, + Optional.ofNullable(clientCertificateHash), + Optional.ofNullable(clientCertificate), + Optional.ofNullable(clientIpAddress)) .validate(registrar, password); checkState( registrar.isLive(), "Registrar %s has non-live state: %s", clientId, registrar.getState()); diff --git a/core/src/test/java/google/registry/flows/EppLoginTlsTest.java b/core/src/test/java/google/registry/flows/EppLoginTlsTest.java index 8112852bb..64e2dbdd7 100644 --- a/core/src/test/java/google/registry/flows/EppLoginTlsTest.java +++ b/core/src/test/java/google/registry/flows/EppLoginTlsTest.java @@ -34,10 +34,13 @@ class EppLoginTlsTest extends EppTestCase { final AppEngineExtension appEngine = AppEngineExtension.builder().withDatastoreAndCloudSql().build(); - void setClientCertificateHash(String clientCertificateHash) { + void setCredentials(String clientCertificateHash, String clientCertificate) { setTransportCredentials( new TlsCredentials( - true, Optional.ofNullable(clientCertificateHash), Optional.of("192.168.1.100:54321"))); + true, + Optional.ofNullable(clientCertificateHash), + Optional.ofNullable(clientCertificate), + Optional.of("192.168.1.100:54321"))); } @BeforeEach @@ -57,14 +60,14 @@ class EppLoginTlsTest extends EppTestCase { @Test void testLoginLogout() throws Exception { - setClientCertificateHash(CertificateSamples.SAMPLE_CERT_HASH); + setCredentials(CertificateSamples.SAMPLE_CERT_HASH, CertificateSamples.SAMPLE_CERT); assertThatLoginSucceeds("NewRegistrar", "foo-BAR2"); assertThatLogoutSucceeds(); } @Test void testLogin_wrongPasswordFails() throws Exception { - setClientCertificateHash(CertificateSamples.SAMPLE_CERT_HASH); + setCredentials(CertificateSamples.SAMPLE_CERT_HASH, CertificateSamples.SAMPLE_CERT); // For TLS login, we also check the epp xml password. assertThatLogin("NewRegistrar", "incorrect") .hasResponse( @@ -74,7 +77,7 @@ class EppLoginTlsTest extends EppTestCase { @Test void testMultiLogin() throws Exception { - setClientCertificateHash(CertificateSamples.SAMPLE_CERT_HASH); + setCredentials(CertificateSamples.SAMPLE_CERT_HASH, CertificateSamples.SAMPLE_CERT); assertThatLoginSucceeds("NewRegistrar", "foo-BAR2"); assertThatLogoutSucceeds(); assertThatLoginSucceeds("NewRegistrar", "foo-BAR2"); @@ -88,7 +91,7 @@ class EppLoginTlsTest extends EppTestCase { @Test void testNonAuthedLogin_fails() throws Exception { - setClientCertificateHash(CertificateSamples.SAMPLE_CERT_HASH); + setCredentials(CertificateSamples.SAMPLE_CERT_HASH, CertificateSamples.SAMPLE_CERT); assertThatLogin("TheRegistrar", "password2") .hasResponse( "response_error.xml", @@ -98,7 +101,7 @@ class EppLoginTlsTest extends EppTestCase { @Test void testBadCertificate_failsBadCertificate2200() throws Exception { - setClientCertificateHash("laffo"); + setCredentials("laffo", "cert"); assertThatLogin("NewRegistrar", "foo-BAR2") .hasResponse( "response_error.xml", @@ -108,7 +111,7 @@ class EppLoginTlsTest extends EppTestCase { @Test void testGfeDidntProvideClientCertificate_failsMissingCertificate2200() throws Exception { - setClientCertificateHash(null); + setCredentials(null, null); assertThatLogin("NewRegistrar", "foo-BAR2") .hasResponse( "response_error.xml", @@ -117,7 +120,7 @@ class EppLoginTlsTest extends EppTestCase { @Test void testGoodPrimaryCertificate() throws Exception { - setClientCertificateHash(CertificateSamples.SAMPLE_CERT_HASH); + setCredentials(CertificateSamples.SAMPLE_CERT_HASH, CertificateSamples.SAMPLE_CERT); DateTime now = DateTime.now(UTC); persistResource( loadRegistrar("NewRegistrar") @@ -130,7 +133,7 @@ class EppLoginTlsTest extends EppTestCase { @Test void testGoodFailoverCertificate() throws Exception { - setClientCertificateHash(CertificateSamples.SAMPLE_CERT2_HASH); + setCredentials(CertificateSamples.SAMPLE_CERT2_HASH, CertificateSamples.SAMPLE_CERT2); DateTime now = DateTime.now(UTC); persistResource( loadRegistrar("NewRegistrar") @@ -143,7 +146,7 @@ class EppLoginTlsTest extends EppTestCase { @Test void testMissingPrimaryCertificateButHasFailover_usesFailover() throws Exception { - setClientCertificateHash(CertificateSamples.SAMPLE_CERT2_HASH); + setCredentials(CertificateSamples.SAMPLE_CERT2_HASH, CertificateSamples.SAMPLE_CERT2); DateTime now = DateTime.now(UTC); persistResource( loadRegistrar("NewRegistrar") @@ -156,7 +159,7 @@ class EppLoginTlsTest extends EppTestCase { @Test void testRegistrarHasNoCertificatesOnFile_fails() throws Exception { - setClientCertificateHash("laffo"); + setCredentials("laffo", "cert"); DateTime now = DateTime.now(UTC); persistResource( loadRegistrar("NewRegistrar") diff --git a/core/src/test/java/google/registry/flows/FlowRunnerTest.java b/core/src/test/java/google/registry/flows/FlowRunnerTest.java index e1f298a4b..b3ddbd73c 100644 --- a/core/src/test/java/google/registry/flows/FlowRunnerTest.java +++ b/core/src/test/java/google/registry/flows/FlowRunnerTest.java @@ -138,7 +138,8 @@ class FlowRunnerTest { @Test void testRun_loggingStatement_tlsCredentials() throws Exception { flowRunner.credentials = - new TlsCredentials(true, Optional.of("abc123def"), Optional.of("127.0.0.1")); + new TlsCredentials( + true, Optional.of("abc123def"), Optional.of("cert"), 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}"); diff --git a/core/src/test/java/google/registry/flows/TlsCredentialsTest.java b/core/src/test/java/google/registry/flows/TlsCredentialsTest.java index 093e9dd09..8745844b0 100644 --- a/core/src/test/java/google/registry/flows/TlsCredentialsTest.java +++ b/core/src/test/java/google/registry/flows/TlsCredentialsTest.java @@ -26,6 +26,7 @@ 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.flows.TlsCredentials.RegistrarCertificateNotConfiguredException; import google.registry.model.registrar.Registrar; import google.registry.testing.AppEngineExtension; import google.registry.util.CidrAddressBlock; @@ -50,8 +51,9 @@ final class TlsCredentialsTest { } @Test - void testClientCertificateHash_missing() { - TlsCredentials tls = new TlsCredentials(true, Optional.empty(), Optional.of("192.168.1.1")); + void testClientCertificateAndHash_missing() { + TlsCredentials tls = + new TlsCredentials(true, Optional.empty(), Optional.empty(), Optional.of("192.168.1.1")); persistResource( loadRegistrar("TheRegistrar") .asBuilder() @@ -64,7 +66,8 @@ final class TlsCredentialsTest { @Test void test_missingIpAddress_doesntAllowAccess() { - TlsCredentials tls = new TlsCredentials(false, Optional.of("certHash"), Optional.empty()); + TlsCredentials tls = + new TlsCredentials(false, Optional.of("certHash"), Optional.empty(), Optional.empty()); persistResource( loadRegistrar("TheRegistrar") .asBuilder() @@ -79,7 +82,41 @@ final class TlsCredentialsTest { @Test void test_validateCertificate_canBeConfiguredToBypassCertHashes() throws Exception { TlsCredentials tls = - new TlsCredentials(false, Optional.of("certHash"), Optional.of("192.168.1.1")); + new TlsCredentials( + false, Optional.of("certHash"), Optional.of("cert"), Optional.of("192.168.1.1")); + persistResource( + loadRegistrar("TheRegistrar") + .asBuilder() + .setClientCertificate(null, DateTime.now(UTC)) + .setFailoverClientCertificate(null, DateTime.now(UTC)) + .build()); + // This would throw a RegistrarCertificateNotConfiguredException if cert hashes were not + // bypassed + tls.validateCertificate(Registrar.loadByClientId("TheRegistrar").get()); + } + + void testProvideClientCertificate() { + HttpServletRequest req = mock(HttpServletRequest.class); + when(req.getHeader("X-SSL-Full-Certificate")).thenReturn("data"); + assertThat(TlsCredentials.EppTlsModule.provideClientCertificate(req)).isEqualTo("data"); + } + + @Test + void testClientCertificate_notConfigured() { + TlsCredentials tls = + new TlsCredentials( + true, Optional.of("hash"), Optional.of(SAMPLE_CERT), Optional.of("192.168.1.1")); + persistResource(loadRegistrar("TheRegistrar").asBuilder().build()); + assertThrows( + RegistrarCertificateNotConfiguredException.class, + () -> tls.validateCertificate(Registrar.loadByClientId("TheRegistrar").get())); + } + + @Test + void test_validateCertificate_canBeConfiguredToBypassCerts() throws Exception { + TlsCredentials tls = + new TlsCredentials( + false, Optional.of("certHash"), Optional.of("cert"), Optional.of("192.168.1.1")); persistResource( loadRegistrar("TheRegistrar") .asBuilder() 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 a4406d199..9a91e3de5 100644 --- a/core/src/test/java/google/registry/flows/session/LoginFlowViaTlsTest.java +++ b/core/src/test/java/google/registry/flows/session/LoginFlowViaTlsTest.java @@ -33,9 +33,11 @@ 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 = + private static final Optional GOOD_CERT = Optional.of(CertificateSamples.SAMPLE_CERT); + private static final Optional GOOD_CERT_HASH = Optional.of(CertificateSamples.SAMPLE_CERT_HASH); - private static final Optional BAD_CERT = + private static final Optional BAD_CERT = Optional.of(CertificateSamples.SAMPLE_CERT2); + private static final Optional BAD_CERT_HASH = Optional.of(CertificateSamples.SAMPLE_CERT2_HASH); private static final Optional GOOD_IP = Optional.of("192.168.1.1"); private static final Optional BAD_IP = Optional.of("1.1.1.1"); @@ -53,7 +55,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase { @Test void testSuccess_withGoodCredentials() throws Exception { persistResource(getRegistrarBuilder().build()); - credentials = new TlsCredentials(true, GOOD_CERT, GOOD_IP); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, GOOD_IP); doSuccessfulTest("login_valid.xml"); } @@ -64,7 +66,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase { .setIpAddressAllowList( ImmutableList.of(CidrAddressBlock.create("2001:db8:0:0:0:0:1:1/32"))) .build()); - credentials = new TlsCredentials(true, GOOD_CERT, GOOD_IPV6); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, GOOD_IPV6); doSuccessfulTest("login_valid.xml"); } @@ -75,7 +77,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase { .setIpAddressAllowList( ImmutableList.of(CidrAddressBlock.create("2001:db8:0:0:0:0:1:1/32"))) .build()); - credentials = new TlsCredentials(true, GOOD_CERT, GOOD_IPV6); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, GOOD_IPV6); doSuccessfulTest("login_valid.xml"); } @@ -85,21 +87,29 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase { getRegistrarBuilder() .setIpAddressAllowList(ImmutableList.of(CidrAddressBlock.create("192.168.1.255/24"))) .build()); - credentials = new TlsCredentials(true, GOOD_CERT, GOOD_IP); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, GOOD_IP); doSuccessfulTest("login_valid.xml"); } @Test void testFailure_incorrectClientCertificateHash() { persistResource(getRegistrarBuilder().build()); - credentials = new TlsCredentials(true, BAD_CERT, GOOD_IP); + credentials = new TlsCredentials(true, BAD_CERT_HASH, BAD_CERT, GOOD_IP); doFailingTest("login_valid.xml", BadRegistrarCertificateException.class); } @Test - void testFailure_missingClientCertificateHash() { + // TODO(Sarahbot): This should fail once hash fallback is removed + void testSuccess_missingClientCertificate() throws Exception { persistResource(getRegistrarBuilder().build()); - credentials = new TlsCredentials(true, Optional.empty(), GOOD_IP); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, Optional.empty(), GOOD_IP); + doSuccessfulTest("login_valid.xml"); + } + + @Test + void testFailure_missingClientCertificateAndHash() { + persistResource(getRegistrarBuilder().build()); + credentials = new TlsCredentials(true, Optional.empty(), Optional.empty(), GOOD_IP); doFailingTest("login_valid.xml", MissingRegistrarCertificateException.class); } @@ -112,7 +122,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, Optional.empty()); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, Optional.empty()); doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class); } @@ -125,7 +135,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, BAD_IP); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, BAD_IP); doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class); } @@ -138,7 +148,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, BAD_IPV6); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, BAD_IPV6); doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class); } } diff --git a/core/src/test/java/google/registry/tools/ValidateLoginCredentialsCommandTest.java b/core/src/test/java/google/registry/tools/ValidateLoginCredentialsCommandTest.java index 6a2e7e039..1e41edc10 100644 --- a/core/src/test/java/google/registry/tools/ValidateLoginCredentialsCommandTest.java +++ b/core/src/test/java/google/registry/tools/ValidateLoginCredentialsCommandTest.java @@ -64,7 +64,7 @@ class ValidateLoginCredentialsCommandTest extends CommandTestCase