Skip to content

Commit

Permalink
finagle-core: Remove SslClientEngineFactory.getHostname
Browse files Browse the repository at this point in the history
Problem

The use of `SslClientEngineFactory.getHostname` is highly discouraged
because it uses the `getHostName` method of the passed in `InetSocketAddress`
if the hostname within the `SslClientConfiguration` is None. The `getHostName`
method of `InetSocketAddress` performs a reverse DNS lookup when provided with
an IP address, which is undesriable.

Solution

Remove `SslClientEngineFactory.getHostname` and encourage folks to use
`SslClientEngineFactory.getHostString` instead.

JIRA Issues: CSL-8416

Differential Revision: https://phabricator.twitter.biz/D334087
  • Loading branch information
ryanoneill authored and jenkins committed Jun 27, 2019
1 parent 54d4acf commit 6e44cc3
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 17 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.rst
Expand Up @@ -27,6 +27,10 @@ Breaking API Changes
been removed. Users should migrate to using the `build` method which takes a
`ServiceFactory[Req, Rep]` as a parameter. ``PHAB_ID=D331011``

* finagle-core: The `c.t.f.ssl.client.SslClientEngineFactory#getHostname` method has been removed.
All uses should be changed to use the `getHostString` method of `SslClientEngineFactory` instead.
``PHAB_ID=DD334087``

* finagle-http: The `setOriginAndCredentials`, `setMaxAge`, `setMethod`, and `setHeaders` methods
of `c.t.f.http.Cors.HttpFilter` are no longer overridable. ``PHAB_ID=D332765``

Expand Down
Expand Up @@ -77,19 +77,6 @@ object SslClientEngineFactory {
new Engine(sslEngine)
}

/**
* Return the hostname from the [[SslClientConfiguration configuration]] if set,
* or fall back to the hostname of the `java.net.InetSocketAddress`.
*
* @note If the config does not contain a hostname, and the address was created
* with a literal IP address, this method will perform a reverse DNS lookup.
*/
def getHostname(isa: InetSocketAddress, config: SslClientConfiguration): String =
config.hostname match {
case Some(host) => host
case None => isa.getHostName
}

/**
* Return the hostname from the [[SslClientConfiguration configuration]] if set,
* or fall back to the host string of the `java.net.InetSocketAddress`.
Expand Down
Expand Up @@ -21,14 +21,14 @@ class SslClientEngineFactoryTest extends FunSuite {
new Engine(sslContext.createSSLEngine())
}

test("getHostname with config.hostname set") {
test("getHostString with config.hostname set") {
val config = SslClientConfiguration(hostname = Some("localhost.twitter.com"))
assert("localhost.twitter.com" == SslClientEngineFactory.getHostname(isa, config))
assert("localhost.twitter.com" == SslClientEngineFactory.getHostString(isa, config))
}

test("getHostname without config.hostname set") {
test("getHostString without config.hostname set") {
val config = SslClientConfiguration()
assert("localhost" == SslClientEngineFactory.getHostname(isa, config))
assert("localhost" == SslClientEngineFactory.getHostString(isa, config))
}

test("configureEngine sets client mode, protocols, and cipher suites") {
Expand Down

0 comments on commit 6e44cc3

Please sign in to comment.