From 9b79f5af2c13cac9976cefe2787617a58d769a97 Mon Sep 17 00:00:00 2001 From: Lai Jiang Date: Tue, 28 Nov 2023 13:20:01 -0500 Subject: [PATCH] Add a dedicated IP header to accommodate Java 17 on GAE (#2224) For reasons unclear at this point, Java 17's servlet implementation on GAE injects IP addresses (including unroutable private IPs) into the standard X-Forwarded-For header, which we currently use to embed registrar IP addresses to check against the allow list. This results in the server not properly parsing the header and rejecting legitimate connections. This PR sets a custom header that should not be interfered with by any JVM implementation to store the IP address, while maintaining the old header as a fallback. The proxy will set both headers to allow the server to gracefully migrate from Java 8 and Java 17 (and potentially rollback). Also removed some headers and logic that are not used. --- .../registry/flows/EppRequestHandler.java | 8 ---- .../google/registry/flows/TlsCredentials.java | 41 ++++++++++------ .../ValidateLoginCredentialsCommand.java | 3 +- .../registry/flows/EppLoginTlsTest.java | 3 +- .../google/registry/flows/EppTestCase.java | 9 ---- .../google/registry/flows/FlowRunnerTest.java | 6 ++- .../registry/flows/TlsCredentialsTest.java | 48 ++++++++++++++++--- .../flows/session/LoginFlowViaTlsTest.java | 18 ++++--- .../proxy/handler/EppServiceHandler.java | 1 + .../java/google/registry/proxy/TestUtils.java | 3 +- .../registry/util/ProxyHttpHeaders.java | 25 ++++------ 11 files changed, 102 insertions(+), 63 deletions(-) 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() {} }