From 8d675a4b8cb8637e7eaf4bb7a5441d1367b1ae7a Mon Sep 17 00:00:00 2001 From: jianglai Date: Fri, 14 Sep 2018 16:01:56 -0700 Subject: [PATCH] Remove checking of SNI headers This is only useful when we used the [] proxy because the GFE requires SNI during handshake in order to request the client certificate. The GCP proxy does not need this (it always requests the client certificate). We do not need to check for its existence. Also removed the checking of internal headers for ssl cert hash used only by the [] proxy. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=213059027 --- docs/flows.md | 1 - java/google/registry/flows/EppTlsAction.java | 5 --- .../google/registry/flows/TlsCredentials.java | 33 +------------------ .../registry/flows/session/LoginFlow.java | 1 - .../ValidateLoginCredentialsCommand.java | 2 +- .../registry/flows/EppLoginTlsTest.java | 4 +-- .../registry/flows/EppTlsActionTest.java | 2 -- .../google/registry/flows/FlowRunnerTest.java | 5 ++- .../registry/flows/TlsCredentialsTest.java | 17 ++-------- .../flows/session/LoginFlowViaTlsTest.java | 26 +++++---------- 10 files changed, 17 insertions(+), 79 deletions(-) diff --git a/docs/flows.md b/docs/flows.md index c83484dc1..79bf91276 100644 --- a/docs/flows.md +++ b/docs/flows.md @@ -1174,7 +1174,6 @@ An EPP flow for login. * Registrar certificate does not match stored certificate. * Registrar IP address is not in stored whitelist. * Registrar certificate not present. - * SNI header is required. * Registrar password is incorrect. * Registrar with this client ID could not be found. * 2201 diff --git a/java/google/registry/flows/EppTlsAction.java b/java/google/registry/flows/EppTlsAction.java index bf7607877..5537d667b 100644 --- a/java/google/registry/flows/EppTlsAction.java +++ b/java/google/registry/flows/EppTlsAction.java @@ -43,11 +43,6 @@ public class EppTlsAction implements Runnable { @Override public void run() { - // Check that SNI header is present. This is a signal that we're receiving traffic proxied by a - // GFE, which is the expectation of this servlet. The value is unused. - if (!tlsCredentials.hasSni()) { - logger.atWarning().log("Request did not include required SNI header."); - } eppRequestHandler.executeEpp( new HttpSessionMetadata(session), tlsCredentials, diff --git a/java/google/registry/flows/TlsCredentials.java b/java/google/registry/flows/TlsCredentials.java index 04566cb77..f4119925c 100644 --- a/java/google/registry/flows/TlsCredentials.java +++ b/java/google/registry/flows/TlsCredentials.java @@ -48,9 +48,6 @@ import javax.servlet.http.HttpServletRequest; *
X-Forwarded-For *
This field should contain the host and port of the connecting client. It is validated * during an EPP login command against an IP whitelist that is transmitted out of band. - *
X-GFE-Requested-Servername-SNI - *
This field should contain the servername that the client requested during the TLS - * handshake. It is unused, but expected to be present in the GFE-proxied configuration. * */ public class TlsCredentials implements TransportCredentials { @@ -58,18 +55,15 @@ public class TlsCredentials implements TransportCredentials { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private final String clientCertificateHash; - private final String sni; private final InetAddress clientInetAddr; @Inject @VisibleForTesting public TlsCredentials( @Header("X-SSL-Certificate") String clientCertificateHash, - @Header("X-Forwarded-For") Optional clientAddress, - @Header("X-Requested-Servername-SNI") String sni) { + @Header("X-Forwarded-For") Optional clientAddress) { this.clientCertificateHash = clientCertificateHash; this.clientInetAddr = clientAddress.isPresent() ? parseInetAddress(clientAddress.get()) : null; - this.sni = sni; } static InetAddress parseInetAddress(String asciiAddr) { @@ -80,11 +74,6 @@ public class TlsCredentials implements TransportCredentials { } } - /** Returns {@code true} if frontend passed us the requested server name. */ - boolean hasSni() { - return !isNullOrEmpty(sni); - } - @Override public void validate(Registrar registrar, String password) throws AuthenticationErrorException { validateIp(registrar); @@ -120,7 +109,6 @@ public class TlsCredentials implements TransportCredentials { /** * Verifies client SSL certificate is permitted to issue commands as {@code registrar}. * - * @throws NoSniException if frontend didn't send host or certificate hash headers * @throws MissingRegistrarCertificateException if frontend didn't send certificate hash header * @throws BadRegistrarCertificateException if registrar requires certificate and it didn't match */ @@ -133,11 +121,6 @@ public class TlsCredentials implements TransportCredentials { return; } if (isNullOrEmpty(clientCertificateHash)) { - // If there's no SNI header that's probably why we don't have a cert, so send a specific - // message. Otherwise, send a missing certificate message. - if (!hasSni()) { - throw new NoSniException(); - } logger.atInfo().log("Request did not include X-SSL-Certificate"); throw new MissingRegistrarCertificateException(); } @@ -165,7 +148,6 @@ public class TlsCredentials implements TransportCredentials { return toStringHelper(getClass()) .add("clientCertificateHash", clientCertificateHash) .add("clientAddress", clientInetAddr) - .add("sni", sni) .toString(); } @@ -183,13 +165,6 @@ public class TlsCredentials implements TransportCredentials { } } - /** SNI header is required. */ - public static class NoSniException extends AuthenticationErrorException { - public NoSniException() { - super("SNI header is required"); - } - } - /** Registrar IP address is not in stored whitelist. */ public static class BadRegistrarIpAddressException extends AuthenticationErrorException { public BadRegistrarIpAddressException() { @@ -211,11 +186,5 @@ public class TlsCredentials implements TransportCredentials { static Optional provideForwardedFor(HttpServletRequest req) { return extractOptionalHeader(req, "X-Forwarded-For"); } - - @Provides - @Header("X-Requested-Servername-SNI") - static String provideRequestedServername(HttpServletRequest req) { - return extractRequiredHeader(req, "X-Requested-Servername-SNI"); - } } } diff --git a/java/google/registry/flows/session/LoginFlow.java b/java/google/registry/flows/session/LoginFlow.java index 08ce41da4..bebd4603c 100644 --- a/java/google/registry/flows/session/LoginFlow.java +++ b/java/google/registry/flows/session/LoginFlow.java @@ -56,7 +56,6 @@ import javax.inject.Inject; * @error {@link google.registry.flows.TlsCredentials.BadRegistrarCertificateException} * @error {@link google.registry.flows.TlsCredentials.BadRegistrarIpAddressException} * @error {@link google.registry.flows.TlsCredentials.MissingRegistrarCertificateException} - * @error {@link google.registry.flows.TlsCredentials.NoSniException} * @error {@link google.registry.flows.TransportCredentials.BadRegistrarPasswordException} * @error {@link LoginFlow.AlreadyLoggedInException} * @error {@link LoginFlow.BadRegistrarClientIdException} diff --git a/java/google/registry/tools/ValidateLoginCredentialsCommand.java b/java/google/registry/tools/ValidateLoginCredentialsCommand.java index af882b78c..a4ac79904 100644 --- a/java/google/registry/tools/ValidateLoginCredentialsCommand.java +++ b/java/google/registry/tools/ValidateLoginCredentialsCommand.java @@ -78,7 +78,7 @@ final class ValidateLoginCredentialsCommand implements CommandWithRemoteApi { Registrar registrar = checkArgumentPresent( Registrar.loadByClientId(clientId), "Registrar %s not found", clientId); - new TlsCredentials(clientCertificateHash, Optional.of(clientIpAddress), null) + new TlsCredentials(clientCertificateHash, Optional.of(clientIpAddress)) .validate(registrar, password); checkState(!registrar.getState().equals(Registrar.State.PENDING), "Account pending"); } diff --git a/javatests/google/registry/flows/EppLoginTlsTest.java b/javatests/google/registry/flows/EppLoginTlsTest.java index 30b9b1685..121285f8f 100644 --- a/javatests/google/registry/flows/EppLoginTlsTest.java +++ b/javatests/google/registry/flows/EppLoginTlsTest.java @@ -40,8 +40,8 @@ public class EppLoginTlsTest extends EppTestCase { void setClientCertificateHash(String clientCertificateHash) { - setTransportCredentials(new TlsCredentials( - clientCertificateHash, Optional.of("192.168.1.100:54321"), "test.example")); + setTransportCredentials( + new TlsCredentials(clientCertificateHash, Optional.of("192.168.1.100:54321"))); } @Before diff --git a/javatests/google/registry/flows/EppTlsActionTest.java b/javatests/google/registry/flows/EppTlsActionTest.java index b911f28a4..a1dabc5aa 100644 --- a/javatests/google/registry/flows/EppTlsActionTest.java +++ b/javatests/google/registry/flows/EppTlsActionTest.java @@ -21,7 +21,6 @@ import static org.mockito.Matchers.eq; import static org.mockito.Matchers.same; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; import google.registry.testing.FakeHttpSession; import google.registry.testing.ShardableTestCase; @@ -41,7 +40,6 @@ public class EppTlsActionTest extends ShardableTestCase { EppTlsAction action = new EppTlsAction(); action.inputXmlBytes = INPUT_XML_BYTES; action.tlsCredentials = mock(TlsCredentials.class); - when(action.tlsCredentials.hasSni()).thenReturn(true); action.session = new FakeHttpSession(); action.session.setAttribute("CLIENT_ID", "ClientIdentifier"); action.eppRequestHandler = mock(EppRequestHandler.class); diff --git a/javatests/google/registry/flows/FlowRunnerTest.java b/javatests/google/registry/flows/FlowRunnerTest.java index 11bba256d..82a3362fa 100644 --- a/javatests/google/registry/flows/FlowRunnerTest.java +++ b/javatests/google/registry/flows/FlowRunnerTest.java @@ -164,11 +164,10 @@ public class FlowRunnerTest extends ShardableTestCase { @Test public void testRun_loggingStatement_tlsCredentials() throws Exception { - flowRunner.credentials = new TlsCredentials("abc123def", Optional.of("127.0.0.1"), "sni"); + flowRunner.credentials = new TlsCredentials("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, sni=sni}"); + .contains("TlsCredentials{clientCertificateHash=abc123def, clientAddress=/127.0.0.1}"); } @Test diff --git a/javatests/google/registry/flows/TlsCredentialsTest.java b/javatests/google/registry/flows/TlsCredentialsTest.java index 065163295..7e5fb1d1a 100644 --- a/javatests/google/registry/flows/TlsCredentialsTest.java +++ b/javatests/google/registry/flows/TlsCredentialsTest.java @@ -46,21 +46,8 @@ public final class TlsCredentialsTest { } @Test - public void testProvideRequestedServername() { - HttpServletRequest req = mock(HttpServletRequest.class); - when(req.getHeader("X-Requested-Servername-SNI")).thenReturn("data"); - assertThat(TlsCredentials.EppTlsModule.provideRequestedServername(req)) - .isEqualTo("data"); - } + public void testNothing1() {} @Test - public void testProvideRequestedServername_missing() { - HttpServletRequest req = mock(HttpServletRequest.class); - BadRequestException thrown = - assertThrows( - BadRequestException.class, - () -> TlsCredentials.EppTlsModule.provideRequestedServername(req)); - assertThat(thrown).hasMessageThat().contains("Missing header: X-Requested-Servername-SNI"); - } - + public void testNothing2() {} } diff --git a/javatests/google/registry/flows/session/LoginFlowViaTlsTest.java b/javatests/google/registry/flows/session/LoginFlowViaTlsTest.java index f6010e359..096b1535f 100644 --- a/javatests/google/registry/flows/session/LoginFlowViaTlsTest.java +++ b/javatests/google/registry/flows/session/LoginFlowViaTlsTest.java @@ -22,7 +22,6 @@ import google.registry.flows.TlsCredentials; import google.registry.flows.TlsCredentials.BadRegistrarCertificateException; import google.registry.flows.TlsCredentials.BadRegistrarIpAddressException; import google.registry.flows.TlsCredentials.MissingRegistrarCertificateException; -import google.registry.flows.TlsCredentials.NoSniException; import google.registry.model.registrar.Registrar; import google.registry.testing.CertificateSamples; import google.registry.util.CidrAddressBlock; @@ -50,7 +49,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase { @Test public void testSuccess_withGoodCredentials() throws Exception { persistResource(getRegistrarBuilder().build()); - credentials = new TlsCredentials(GOOD_CERT, GOOD_IP, "goo.example"); + credentials = new TlsCredentials(GOOD_CERT, GOOD_IP); doSuccessfulTest("login_valid.xml"); } @@ -61,7 +60,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase { .setIpAddressWhitelist(ImmutableList.of( CidrAddressBlock.create("2001:db8:0:0:0:0:1:1/32"))) .build()); - credentials = new TlsCredentials(GOOD_CERT, GOOD_IPV6, "goo.example"); + credentials = new TlsCredentials(GOOD_CERT, GOOD_IPV6); doSuccessfulTest("login_valid.xml"); } @@ -72,7 +71,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase { .setIpAddressWhitelist(ImmutableList.of( CidrAddressBlock.create("2001:db8:0:0:0:0:1:1/32"))) .build()); - credentials = new TlsCredentials(GOOD_CERT, GOOD_IPV6, "goo.example"); + credentials = new TlsCredentials(GOOD_CERT, GOOD_IPV6); doSuccessfulTest("login_valid.xml"); } @@ -83,31 +82,24 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase { .setIpAddressWhitelist(ImmutableList.of( CidrAddressBlock.create("192.168.1.255/24"))) .build()); - credentials = new TlsCredentials(GOOD_CERT, GOOD_IP, "goo.example"); + credentials = new TlsCredentials(GOOD_CERT, GOOD_IP); doSuccessfulTest("login_valid.xml"); } @Test public void testFailure_incorrectClientCertificateHash() { persistResource(getRegistrarBuilder().build()); - credentials = new TlsCredentials(BAD_CERT, GOOD_IP, "goo.example"); + credentials = new TlsCredentials(BAD_CERT, GOOD_IP); doFailingTest("login_valid.xml", BadRegistrarCertificateException.class); } @Test public void testFailure_missingClientCertificateHash() { persistResource(getRegistrarBuilder().build()); - credentials = new TlsCredentials(null, GOOD_IP, "goo.example"); + credentials = new TlsCredentials(null, GOOD_IP); doFailingTest("login_valid.xml", MissingRegistrarCertificateException.class); } - @Test - public void testFailure_noSniAndCertRequired() { - persistResource(getRegistrarBuilder().build()); - credentials = new TlsCredentials(null, GOOD_IP, null); - doFailingTest("login_valid.xml", NoSniException.class); - } - @Test public void testFailure_missingClientIpAddress() { persistResource( @@ -116,7 +108,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(GOOD_CERT, Optional.empty(), "goo.example"); + credentials = new TlsCredentials(GOOD_CERT, Optional.empty()); doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class); } @@ -128,7 +120,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(GOOD_CERT, BAD_IP, "goo.example"); + credentials = new TlsCredentials(GOOD_CERT, BAD_IP); doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class); } @@ -140,7 +132,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(GOOD_CERT, BAD_IPV6, "goo.example"); + credentials = new TlsCredentials(GOOD_CERT, BAD_IPV6); doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class); } }