mirrors/nomulus
1
0
Fork 0
mirror of https://github.com/google/nomulus synced 2026-04-16 14:37:30 +00:00
Code Issues Releases Wiki Activity

Add a dedicated IP header to accommodate Java 17 on GAE (#2224)

Browse Source 
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.
This commit is contained in:
Lai Jiang
2023-11-28 13:20:01 -05:00
committed by GitHub
parent 4195871541
commit 9b79f5af2c
11 changed files with 102 additions and 63 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
core/src/main/java/google/registry/flows/EppRequestHandler.java
View File

@@ -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);

41
core/src/main/java/google/registry/flows/TlsCredentials.java
View File

@@ -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<String> clientCertificateHash,
@Header(ProxyHttpHeaders.IP_ADDRESS) Optional<String> clientAddress,
Optional<InetAddress> 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<String> provideIpAddress(HttpServletRequest req) {
return extractOptionalHeader(req, ProxyHttpHeaders.IP_ADDRESS);
static Optional<InetAddress> provideIpAddress(HttpServletRequest req) {
Optional<String> clientAddress = extractOptionalHeader(req, ProxyHttpHeaders.IP_ADDRESS);
Optional<String> fallbackClientAddress =
extractOptionalHeader(req, ProxyHttpHeaders.IP_ADDRESS);
Optional<InetAddress> clientInetAddr = clientAddress.map(TlsCredentials::parseInetAddress);
return clientInetAddr.isPresent()
? clientInetAddr
: fallbackClientAddress.map(TlsCredentials::parseInetAddress);
}
}
}

3
core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java
View File

@@ -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(

3
core/src/test/java/google/registry/flows/EppLoginTlsTest.java
View File

@@ -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));
}

9
core/src/test/java/google/registry/flows/EppTestCase.java
View File

@@ -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);
}

6
core/src/test/java/google/registry/flows/FlowRunnerTest.java
View File

@@ -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}");

48
core/src/test/java/google/registry/flows/TlsCredentialsTest.java
View File

@@ -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()

18
core/src/test/java/google/registry/flows/session/LoginFlowViaTlsTest.java
View File

@@ -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<String> BAD_CERT_HASH =
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");
private static final Optional<String> BAD_IPV6 = Optional.of("2001:db8::2");
private static final Optional<InetAddress> GOOD_IP =
Optional.of(InetAddresses.forString("192.168.1.1"));
private static final Optional<InetAddress> BAD_IP =
Optional.of(InetAddresses.forString("1.1.1.1"));
private static final Optional<InetAddress> GOOD_IPV6 =
Optional.of(InetAddresses.forString("2001:db8::1"));
private static final Optional<InetAddress> 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);
}

1
proxy/src/main/java/google/registry/proxy/handler/EppServiceHandler.java
View File

@@ -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;

3
proxy/src/test/java/google/registry/proxy/TestUtils.java
View File

@@ -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));
}

25
util/src/main/java/google/registry/util/ProxyHttpHeaders.java
View File

@@ -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.
*
* <p>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.
*
* <p>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() {}
}