Skip to content

Commit

Permalink
finagle-http: add withSniHostname api
Browse files Browse the repository at this point in the history
Problem:
SNI HostName verification fails in netty
when a colon or similar special character is present

Solution:
Pipe SNI hostnames through finagle
and validate them via bytes rather than direct text

JIRA Issues: CSL-11177

Differential Revision: https://phabricator.twitter.biz/D712652
  • Loading branch information
joybestourous authored and jenkins committed Aug 4, 2021
1 parent a200b01 commit a8ec457
Show file tree
Hide file tree
Showing 10 changed files with 80 additions and 2 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ New Features
need for extra boilerplate to convert the destination String to a `c.t.finagle.Name` when
specifying both `dest` and `label` in String form. ``PHAB_ID=D706140``

* finagle-http, finagle-thriftmux: introduce client.withSni() API. Use this api to specify an
SNI hostname for TLS clients. ``PHAB_ID=D712652``

Runtime Behavior Changes
~~~~~~~~~~~~~~~~~~~~~~~~

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,18 @@ class ClientTransportParams[A <: Stack.Parameterized[A]](self: Stack.Parameteriz
)
}

/**
* Configures SNI hostname for SSL
*
* @see [[https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SNIHostName.html Java's
* SNIHostName]] for more details.
*/

def sni(hostname: String): A = {
self
.configured(Transport.ClientSsl(Some(SslClientConfiguration(sniHostName = Some(hostname)))))
}

/**
* Enables TCP tunneling via `HTTP CONNECT` through an HTTP proxy [1] on this client
* (default: disabled).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import com.twitter.finagle.ssl.{
*/
case class SslClientConfiguration(
hostname: Option[String] = None,
sniHostName: Option[String] = None,
keyCredentials: KeyCredentials = KeyCredentials.Unspecified,
trustCredentials: TrustCredentials = TrustCredentials.Unspecified,
cipherSuites: CipherSuites = CipherSuites.Unspecified,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ package com.twitter.finagle.ssl.client
import com.twitter.finagle.{Address, Stack}
import com.twitter.finagle.ssl._
import java.net.InetSocketAddress
import javax.net.ssl.SSLContext
import java.nio.charset.StandardCharsets
import java.util.Collections
import javax.net.ssl.{SNIHostName, SSLContext, SSLParameters}

/**
* Instances of this class provide a method to create Finagle
Expand Down Expand Up @@ -55,6 +57,19 @@ object SslClientEngineFactory {
// This matters for handshaking.
sslEngine.setUseClientMode(true)

config.sniHostName match {
case Some(sni) =>
//SSL Parameters to set SNI TLS Extension
val sslParameters = new SSLParameters()
sslParameters.setServerNames(
Collections.singletonList(
new SNIHostName(sni.getBytes(StandardCharsets.UTF_8))
)
)
sslEngine.setSSLParameters(sslParameters)
case _ => // no-op
}

SslConfigurations.configureProtocols(sslEngine, config.protocols)
SslConfigurations.configureCipherSuites(sslEngine, config.cipherSuites)
SslConfigurations.configureHostnameVerification(sslEngine, config.hostname)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ public void testConfiguration() {
Option<String> hostname = Scala.asOption(Optional.empty());

SslClientConfiguration config = new SslClientConfiguration(
hostname,
hostname,
KeyCredentialsConfig.UNSPECIFIED,
TrustCredentialsConfig.UNSPECIFIED,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package com.twitter.finagle.ssl.client
import com.twitter.finagle.Address
import com.twitter.finagle.ssl.{CipherSuites, Engine, Protocols}
import java.net.InetSocketAddress
import javax.net.ssl.SSLContext
import javax.net.ssl.{SNIHostName, SSLContext}
import org.scalatest.funsuite.AnyFunSuite

class SslClientEngineFactoryTest extends AnyFunSuite {
Expand Down Expand Up @@ -87,4 +87,13 @@ class SslClientEngineFactoryTest extends AnyFunSuite {
assert(sslEngine.getPeerPort() == -1)
}

test("If sniHostNames are included, MtlsClientParams should use them") {
val sniTestName = "somehost:1234"
val sniHostName = new SNIHostName(sniTestName.getBytes)
val config = SslClientConfiguration(None, Some(sniTestName))
val testengine = createTestEngine()
SslClientEngineFactory.configureEngine(testengine, config)

assert(testengine.self.getSSLParameters.getServerNames.get(0).equals(sniHostName))
}
}
8 changes: 8 additions & 0 deletions finagle-http/src/main/scala/com/twitter/finagle/Http.scala
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,14 @@ object Http extends Client[Request, Response] with HttpRichClient with Server[Re

def withTlsWithoutValidation: Client = withTransport.tlsWithoutValidation

/**
* Configures the sni hostname for SSL.
*
* @see [[https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SNIHostName.html Java's
* SNIHostName]] for more details.
*/
def withSni(hostname: String): Client = withTransport.sni(hostname)

/**
* For HTTP1*, configures the max size of headers
* For HTTP2, sets the MAX_HEADER_LIST_SIZE setting which is the maximum
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ public void onFailure(Throwable cause) {}
.withAdmissionControl().maxPendingRequests(100)
.withSessionQualifier().noFailFast()
.withTls("foo.com")
.withSni("somehost:1234")
.configured(new Label("test").mk())
.withDecompression(true)
.withHttp2();
Expand Down
15 changes: 15 additions & 0 deletions finagle-http/src/test/scala/com/twitter/finagle/HttpTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package com.twitter.finagle
import com.twitter.finagle.filter.NackAdmissionFilter
import com.twitter.finagle.http.{ClientEndpointer, Request, Response, serverErrorsAsFailures}
import com.twitter.finagle.service.{ReqRep, ResponseClass, ResponseClassifier}
import com.twitter.finagle.ssl.client.SslClientConfiguration
import com.twitter.finagle.transport.Transport.ClientSsl
import com.twitter.finagle.stats.InMemoryStatsReceiver
import com.twitter.util.{Await, Duration, Future, Return}
import java.net.InetSocketAddress
Expand Down Expand Up @@ -35,6 +37,7 @@ class HttpTest extends AnyFunSuite with Eventually {
import com.twitter.finagle.http.{Status => HStatus}

def rep(code: HStatus): Response = Response(code)

def reqRep(rep: Response): ReqRep = ReqRep(Request("/index.cgi"), Return(rep))

val rc = Http.responseClassifierParam.responseClassifier
Expand All @@ -58,6 +61,7 @@ class HttpTest extends AnyFunSuite with Eventually {
import com.twitter.finagle.http.{Status => HStatus}

def rep(code: HStatus): Response = Response(code)

def reqRep(rep: Response): ReqRep = ReqRep(Request("/index.cgi"), Return(rep))

val rc = Http.responseClassifierParam.responseClassifier
Expand Down Expand Up @@ -137,4 +141,15 @@ class HttpTest extends AnyFunSuite with Eventually {
assert(transporter == ClientEndpointer.HttpEndpointer)
assert(listener(Stack.Params.empty).toString == "Netty4Listener")
}

test("If sniHostNames are included, MtlsClientParams should use them") {
val hostname = "somehost:1234"
val client = Http.client.withSni(hostname)
val ssl = client.params[ClientSsl].sslClientConfiguration
ssl match {
case Some(s: SslClientConfiguration) =>
assert(s.sniHostName.get == hostname)
case _ => // no-op
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import com.twitter.finagle.dispatch.PipeliningDispatcher
import com.twitter.finagle.liveness.FailureAccrualFactory
import com.twitter.finagle.param.{Label, Stats, Tracer => PTracer}
import com.twitter.finagle.service._
import com.twitter.finagle.ssl.client.SslClientConfiguration
import com.twitter.finagle.stats._
import com.twitter.finagle.thrift.{
ClientId,
Expand All @@ -22,6 +23,7 @@ import com.twitter.finagle.thriftmux.service.ThriftMuxResponseClassifier
import com.twitter.finagle.thriftmux.thriftscala._
import com.twitter.finagle.tracing.Annotation.{ClientSend, ServerRecv}
import com.twitter.finagle.tracing._
import com.twitter.finagle.transport.Transport.ClientSsl
import com.twitter.finagle.transport.{Transport, TransportContext}
import com.twitter.finagle.util.DefaultTimer
import com.twitter.io.Buf
Expand Down Expand Up @@ -2252,4 +2254,15 @@ class EndToEndTest
}
}
}

test("with sni hostname") {
val hostname = "somehost:1234"
val client = ThriftMux.client.withTransport.sni(hostname)
val ssl = client.params[ClientSsl].sslClientConfiguration
ssl match {
case Some(s: SslClientConfiguration) =>
assert(s.sniHostName.get == hostname)
case _ => // no-op
}
}
}

0 comments on commit a8ec457

Please sign in to comment.