diff --git a/core/src/main/java/google/registry/flows/EppRequestHandler.java b/core/src/main/java/google/registry/flows/EppRequestHandler.java index da5474b58..b727472db 100644 --- a/core/src/main/java/google/registry/flows/EppRequestHandler.java +++ b/core/src/main/java/google/registry/flows/EppRequestHandler.java @@ -75,14 +75,6 @@ public class EppRequestHandler { && eppOutput.getResponse().getResult().getCode() == SUCCESS_AND_CLOSE) { response.setHeader(ProxyHttpHeaders.EPP_SESSION, "close"); } - // If a login request returns a success, a logged-in header is added to the response to inform - // the proxy that it is no longer necessary to send the full client certificate to the backend - // for this connection. - if (eppOutput.isResponse() - && eppOutput.getResponse().isLoginResponse() - && eppOutput.isSuccess()) { - response.setHeader(ProxyHttpHeaders.LOGGED_IN, "true"); - } } catch (Exception e) { logger.atWarning().withCause(e).log("handleEppCommand general exception."); response.setStatus(SC_BAD_REQUEST); diff --git a/core/src/main/java/google/registry/flows/TlsCredentials.java b/core/src/main/java/google/registry/flows/TlsCredentials.java index 56b741717..1b90f417b 100644 --- a/core/src/main/java/google/registry/flows/TlsCredentials.java +++ b/core/src/main/java/google/registry/flows/TlsCredentials.java @@ -26,6 +26,7 @@ import com.google.common.net.InetAddresses; import dagger.Module; import dagger.Provides; import google.registry.config.RegistryConfig.Config; +import google.registry.config.RegistryEnvironment; import google.registry.flows.EppException.AuthenticationErrorException; import google.registry.flows.certs.CertificateChecker; import google.registry.flows.certs.CertificateChecker.InsecureCertificateException; @@ -66,11 +67,11 @@ public class TlsCredentials implements TransportCredentials { public TlsCredentials( @Config("requireSslCertificates") boolean requireSslCertificates, @Header(ProxyHttpHeaders.CERTIFICATE_HASH) Optional clientCertificateHash, - @Header(ProxyHttpHeaders.IP_ADDRESS) Optional clientAddress, + Optional clientInetAddr, CertificateChecker certificateChecker) { this.requireSslCertificates = requireSslCertificates; this.clientCertificateHash = clientCertificateHash; - this.clientInetAddr = clientAddress.map(TlsCredentials::parseInetAddress); + this.clientInetAddr = clientInetAddr; this.certificateChecker = certificateChecker; } @@ -104,18 +105,25 @@ public class TlsCredentials implements TransportCredentials { } // In the rare unexpected case that the client inet address wasn't passed along at all, then // by default deny access. - if (clientInetAddr.isPresent()) { - for (CidrAddressBlock cidrAddressBlock : ipAddressAllowList) { - if (cidrAddressBlock.contains(clientInetAddr.get())) { - // IP address is in allow list; return early. - return; - } + if (!clientInetAddr.isPresent()) { + logger.atWarning().log( + "Authentication error: Missing IP address for registrar %s.", registrar.getRegistrarId()); + throw new BadRegistrarIpAddressException(clientInetAddr); + } + for (CidrAddressBlock cidrAddressBlock : ipAddressAllowList) { + if (cidrAddressBlock.contains(clientInetAddr.get())) { + // IP address is in allow list; return early. + return; } } - logger.atInfo().log( + logger.atWarning().log( "Authentication error: IP address %s is not allow-listed for registrar %s; allow list is:" + " %s", - clientInetAddr, registrar.getRegistrarId(), ipAddressAllowList); + clientInetAddr, + registrar.getRegistrarId(), + RegistryEnvironment.get() == RegistryEnvironment.PRODUCTION + ? "redacted in production" + : ipAddressAllowList); throw new BadRegistrarIpAddressException(clientInetAddr); } @@ -232,7 +240,7 @@ public class TlsCredentials implements TransportCredentials { ? String.format( "Registrar IP address %s is not in stored allow list", clientInetAddr.get().getHostAddress()) - : "Registrar IP address is not in stored allow list"); + : "Registrar IP address is missing"); } } @@ -249,9 +257,14 @@ public class TlsCredentials implements TransportCredentials { } @Provides - @Header(ProxyHttpHeaders.IP_ADDRESS) - static Optional provideIpAddress(HttpServletRequest req) { - return extractOptionalHeader(req, ProxyHttpHeaders.IP_ADDRESS); + static Optional provideIpAddress(HttpServletRequest req) { + Optional clientAddress = extractOptionalHeader(req, ProxyHttpHeaders.IP_ADDRESS); + Optional fallbackClientAddress = + extractOptionalHeader(req, ProxyHttpHeaders.IP_ADDRESS); + Optional clientInetAddr = clientAddress.map(TlsCredentials::parseInetAddress); + return clientInetAddr.isPresent() + ? clientInetAddr + : fallbackClientAddress.map(TlsCredentials::parseInetAddress); } } } diff --git a/core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java b/core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java index 76c9bd1fa..a52351eef 100644 --- a/core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java +++ b/core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java @@ -23,6 +23,7 @@ import static google.registry.util.X509Utils.loadCertificate; import com.beust.jcommander.Parameter; import com.beust.jcommander.Parameters; +import com.google.common.net.InetAddresses; import google.registry.flows.TlsCredentials; import google.registry.flows.certs.CertificateChecker; import google.registry.model.registrar.Registrar; @@ -85,7 +86,7 @@ final class ValidateLoginCredentialsCommand implements Command { new TlsCredentials( true, Optional.ofNullable(clientCertificateHash), - Optional.ofNullable(clientIpAddress), + Optional.ofNullable(InetAddresses.forString(clientIpAddress)), certificateChecker) .validate(registrar, password); checkState( diff --git a/core/src/test/java/google/registry/flows/EppLoginTlsTest.java b/core/src/test/java/google/registry/flows/EppLoginTlsTest.java index c0f70160c..a14d4eaa2 100644 --- a/core/src/test/java/google/registry/flows/EppLoginTlsTest.java +++ b/core/src/test/java/google/registry/flows/EppLoginTlsTest.java @@ -23,6 +23,7 @@ import static google.registry.util.X509Utils.getCertificateHash; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; +import com.google.common.net.InetAddresses; import google.registry.flows.certs.CertificateChecker; import google.registry.persistence.transaction.JpaTestExtensions; import google.registry.persistence.transaction.JpaTestExtensions.JpaIntegrationTestExtension; @@ -66,7 +67,7 @@ class EppLoginTlsTest extends EppTestCase { new TlsCredentials( true, Optional.ofNullable(clientCertificateHash), - Optional.of("192.168.1.100:54321"), + Optional.of(InetAddresses.forString("192.168.1.100")), certificateChecker)); } diff --git a/core/src/test/java/google/registry/flows/EppTestCase.java b/core/src/test/java/google/registry/flows/EppTestCase.java index 44b90dffc..d8d486ade 100644 --- a/core/src/test/java/google/registry/flows/EppTestCase.java +++ b/core/src/test/java/google/registry/flows/EppTestCase.java @@ -43,7 +43,6 @@ import google.registry.persistence.VKey; import google.registry.testing.FakeClock; import google.registry.testing.FakeHttpSession; import google.registry.testing.FakeResponse; -import google.registry.util.ProxyHttpHeaders; import java.util.Map; import java.util.Objects; import java.util.Optional; @@ -126,10 +125,6 @@ public class EppTestCase { setUpSession(); FakeResponse response = executeXmlCommand(input); - // Check that the logged-in header was added to the response - assertThat(response.getHeaders()) - .isEqualTo(ImmutableMap.of(ProxyHttpHeaders.LOGGED_IN, "true")); - verifyAndReturnOutput(response.getPayload(), expectedOutput, inputFilename, outputFilename); } @@ -146,10 +141,6 @@ public class EppTestCase { setUpSession(); FakeResponse response = executeXmlCommand(input); - // Checks that the Logged-In header is not in the response. If testing the login command, use - // assertLoginCommandAndResponse instead of this method. - assertThat(response.getHeaders()).doesNotContainEntry(ProxyHttpHeaders.LOGGED_IN, "true"); - return verifyAndReturnOutput( response.getPayload(), expectedOutput, inputFilename, outputFilename); } diff --git a/core/src/test/java/google/registry/flows/FlowRunnerTest.java b/core/src/test/java/google/registry/flows/FlowRunnerTest.java index 76c8732d4..4b1353c2c 100644 --- a/core/src/test/java/google/registry/flows/FlowRunnerTest.java +++ b/core/src/test/java/google/registry/flows/FlowRunnerTest.java @@ -30,6 +30,7 @@ import com.google.common.base.Joiner; import com.google.common.base.Splitter; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; +import com.google.common.net.InetAddresses; import com.google.common.testing.TestLogHandler; import google.registry.flows.certs.CertificateChecker; import google.registry.model.eppcommon.Trid; @@ -186,7 +187,10 @@ class FlowRunnerTest { void testRun_loggingStatement_tlsCredentials() throws Exception { flowRunner.credentials = new TlsCredentials( - true, Optional.of("abc123def"), Optional.of("127.0.0.1"), certificateChecker); + true, + Optional.of("abc123def"), + Optional.of(InetAddresses.forString("127.0.0.1")), + certificateChecker); flowRunner.run(eppMetricBuilder); assertThat(Splitter.on("\n\t").split(findFirstLogMessageByPrefix(handler, "EPP Command\n\t"))) .contains("TlsCredentials{clientCertificateHash=abc123def," + " clientAddress=/127.0.0.1}"); diff --git a/core/src/test/java/google/registry/flows/TlsCredentialsTest.java b/core/src/test/java/google/registry/flows/TlsCredentialsTest.java index e3049b11d..10d6030ed 100644 --- a/core/src/test/java/google/registry/flows/TlsCredentialsTest.java +++ b/core/src/test/java/google/registry/flows/TlsCredentialsTest.java @@ -27,6 +27,7 @@ import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; +import com.google.common.net.InetAddresses; import google.registry.flows.TlsCredentials.BadRegistrarIpAddressException; import google.registry.flows.TlsCredentials.MissingRegistrarCertificateException; import google.registry.flows.TlsCredentials.RegistrarCertificateNotConfiguredException; @@ -71,7 +72,11 @@ final class TlsCredentialsTest { @Test void testClientCertificateAndHash_missing() { TlsCredentials tls = - new TlsCredentials(true, Optional.empty(), Optional.of("192.168.1.1"), certificateChecker); + new TlsCredentials( + true, + Optional.empty(), + Optional.of(InetAddresses.forString("192.168.1.1")), + certificateChecker); persistResource( loadRegistrar("TheRegistrar") .asBuilder() @@ -83,10 +88,13 @@ final class TlsCredentialsTest { } @Test - void test_missingIpAddress_doesntAllowAccess() { + void test_wrongIpAddress_doesntAllowAccess() { TlsCredentials tls = new TlsCredentials( - false, Optional.of("certHash"), Optional.of("127.0.0.1"), certificateChecker); + false, + Optional.of("certHash"), + Optional.of(InetAddresses.forString("127.0.0.1")), + certificateChecker); persistResource( loadRegistrar("TheRegistrar") .asBuilder() @@ -104,11 +112,33 @@ final class TlsCredentialsTest { .isEqualTo("Registrar IP address 127.0.0.1 is not in stored allow list"); } + @Test + void test_missingIpAddress_doesntAllowAccess() { + TlsCredentials tls = + new TlsCredentials(false, Optional.of("certHash"), Optional.empty(), certificateChecker); + persistResource( + loadRegistrar("TheRegistrar") + .asBuilder() + .setClientCertificate(SAMPLE_CERT, clock.nowUtc()) + .setIpAddressAllowList(ImmutableSet.of(CidrAddressBlock.create("3.5.8.13"))) + .build()); + + BadRegistrarIpAddressException thrown = + assertThrows( + BadRegistrarIpAddressException.class, + () -> tls.validate(Registrar.loadByRegistrarId("TheRegistrar").get(), "password")); + + assertThat(thrown).hasMessageThat().isEqualTo("Registrar IP address is missing"); + } + @Test void testClientCertificate_notConfigured() { TlsCredentials tls = new TlsCredentials( - true, Optional.of("hash"), Optional.of("192.168.1.1"), certificateChecker); + true, + Optional.of("hash"), + Optional.of(InetAddresses.forString("192.168.1.1")), + certificateChecker); persistResource(loadRegistrar("TheRegistrar").asBuilder().build()); assertThrows( RegistrarCertificateNotConfiguredException.class, @@ -119,7 +149,10 @@ final class TlsCredentialsTest { void test_validateCertificateHash_canBeConfiguredToBypassCerts() throws Exception { TlsCredentials tls = new TlsCredentials( - false, Optional.of("certHash"), Optional.of("192.168.1.1"), certificateChecker); + false, + Optional.of("certHash"), + Optional.of(InetAddresses.forString("192.168.1.1")), + certificateChecker); persistResource( loadRegistrar("TheRegistrar") .asBuilder() @@ -134,7 +167,10 @@ final class TlsCredentialsTest { void test_validateCertificateHash_passWithFailOverCerticate() throws Exception { TlsCredentials tls = new TlsCredentials( - false, Optional.of(SAMPLE_CERT_HASH), Optional.of("192.168.1.1"), certificateChecker); + false, + Optional.of(SAMPLE_CERT_HASH), + Optional.of(InetAddresses.forString("192.168.1.1")), + certificateChecker); 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 532f2e5ca..7ab923e19 100644 --- a/core/src/test/java/google/registry/flows/session/LoginFlowViaTlsTest.java +++ b/core/src/test/java/google/registry/flows/session/LoginFlowViaTlsTest.java @@ -30,6 +30,7 @@ import google.registry.flows.certs.CertificateChecker; import google.registry.model.registrar.Registrar; import google.registry.testing.CertificateSamples; import google.registry.util.CidrAddressBlock; +import java.net.InetAddress; import java.util.Optional; import org.joda.time.DateTime; import org.junit.jupiter.api.Test; @@ -42,10 +43,14 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase { Optional.of(CertificateSamples.SAMPLE_CERT3_HASH); 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"); - private static final Optional GOOD_IPV6 = Optional.of("2001:db8::1"); - private static final Optional BAD_IPV6 = Optional.of("2001:db8::2"); + private static final Optional GOOD_IP = + Optional.of(InetAddresses.forString("192.168.1.1")); + private static final Optional BAD_IP = + Optional.of(InetAddresses.forString("1.1.1.1")); + private static final Optional GOOD_IPV6 = + Optional.of(InetAddresses.forString("2001:db8::1")); + private static final Optional BAD_IPV6 = + Optional.of(InetAddresses.forString("2001:db8::2")); private final CertificateChecker certificateChecker = new CertificateChecker( ImmutableSortedMap.of(START_OF_TIME, 825, DateTime.parse("2020-09-01T00:00:00Z"), 398), @@ -59,8 +64,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase { protected Registrar.Builder getRegistrarBuilder() { return super.getRegistrarBuilder() .setClientCertificate(GOOD_CERT.get(), DateTime.now(UTC)) - .setIpAddressAllowList( - ImmutableList.of(CidrAddressBlock.create(InetAddresses.forString(GOOD_IP.get()), 32))); + .setIpAddressAllowList(ImmutableList.of(CidrAddressBlock.create(GOOD_IP.get(), 32))); } @Test @@ -129,7 +133,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase { CidrAddressBlock.create(InetAddresses.forString("192.168.1.1"), 32), CidrAddressBlock.create(InetAddresses.forString("2001:db8::1"), 128))) .build()); - credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, certificateChecker); + credentials = new TlsCredentials(true, GOOD_CERT_HASH, Optional.empty(), certificateChecker); doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class); } diff --git a/proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java b/proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java index fb0233dcf..42946c5a1 100644 --- a/proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java +++ b/proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java @@ -124,6 +124,7 @@ public class EppServiceHandler extends HttpsRelayServiceHandler { .headers() .set(ProxyHttpHeaders.CERTIFICATE_HASH, sslClientCertificateHash) .set(ProxyHttpHeaders.IP_ADDRESS, clientAddress) + .set(ProxyHttpHeaders.FALLBACK_IP_ADDRESS, clientAddress) .set(HttpHeaderNames.CONTENT_TYPE, EPP_CONTENT_TYPE) .set(HttpHeaderNames.ACCEPT, EPP_CONTENT_TYPE); return request; diff --git a/proxy/src/test/java/google/registry/proxy/TestUtils.java b/proxy/src/test/java/google/registry/proxy/TestUtils.java index ba185cc2c..b5677c058 100644 --- a/proxy/src/test/java/google/registry/proxy/TestUtils.java +++ b/proxy/src/test/java/google/registry/proxy/TestUtils.java @@ -100,7 +100,8 @@ public final class TestUtils { .set(HttpHeaderNames.CONTENT_TYPE, "application/epp+xml") .set("accept", "application/epp+xml") .set(ProxyHttpHeaders.CERTIFICATE_HASH, sslClientCertificateHash) - .set(ProxyHttpHeaders.IP_ADDRESS, clientAddress); + .set(ProxyHttpHeaders.IP_ADDRESS, clientAddress) + .set(ProxyHttpHeaders.FALLBACK_IP_ADDRESS, clientAddress); if (cookies.length != 0) { request.headers().set("cookie", ClientCookieEncoder.STRICT.encode(cookies)); } diff --git a/util/src/main/java/google/registry/util/ProxyHttpHeaders.java b/util/src/main/java/google/registry/util/ProxyHttpHeaders.java index 202c10fd8..9d6cbe133 100644 --- a/util/src/main/java/google/registry/util/ProxyHttpHeaders.java +++ b/util/src/main/java/google/registry/util/ProxyHttpHeaders.java @@ -19,30 +19,25 @@ import com.google.common.net.HttpHeaders; /** Utility class of HTTP header names used for HTTP calls between Nomulus and the proxy. */ public final class ProxyHttpHeaders { - /** - * HTTP header name used to pass a full SSL certificate from the proxy to Nomulus. - * - *

This header contains the SSL certificate encoded to a string. It is used to pass the client - * certificate used for login to Nomulus for validation. - */ - public static final String FULL_CERTIFICATE = "X-SSL-Full-Certificate"; - /** HTTP header name used to pass the certificate hash from the proxy to Nomulus. */ public static final String CERTIFICATE_HASH = "X-SSL-Certificate"; - /** - * HTTP header name passed from Nomulus to proxy to indicate that a client has successfully logged - * in. - */ - public static final String LOGGED_IN = "Logged-In"; - /** * HTTP header name passed from Nomulus to proxy to indicate that an EPP session should be closed. */ public static final String EPP_SESSION = "Epp-Session"; /** HTTP header name used to pass the client IP address from the proxy to Nomulus. */ - public static final String IP_ADDRESS = HttpHeaders.X_FORWARDED_FOR; + public static final String IP_ADDRESS = "Nomulus-Client-Address"; + + /** + * Fallback HTTP header name used to pass the client IP address from the proxy to Nomulus. + * + *

Note that Java 17's servlet implementation (at least on App Engine) injects some seemingly + * unrelated addresses into this header. We only use this as a fallback so the proxy can + * transition to use the above header that should not be interfered with. + */ + public static final String FALLBACK_IP_ADDRESS = HttpHeaders.X_FORWARDED_FOR; private ProxyHttpHeaders() {} }