From 6e44cc3937c0693cb265b7402b86f1ce0390aea2 Mon Sep 17 00:00:00 2001
From: Ryan O'Neill <ryano@twitter.com>
Date: Thu, 27 Jun 2019 19:06:38 +0000
Subject: [PATCH] finagle-core: Remove SslClientEngineFactory.getHostname

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
---
 CHANGELOG.rst                                       |  4 ++++
 .../finagle/ssl/client/SslClientEngineFactory.scala | 13 -------------
 .../ssl/client/SslClientEngineFactoryTest.scala     |  8 ++++----
 3 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 8fbe64314d..4c33c9305c 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -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``
 
diff --git a/finagle-core/src/main/scala/com/twitter/finagle/ssl/client/SslClientEngineFactory.scala b/finagle-core/src/main/scala/com/twitter/finagle/ssl/client/SslClientEngineFactory.scala
index f2fe38893b..5c511a68c2 100644
--- a/finagle-core/src/main/scala/com/twitter/finagle/ssl/client/SslClientEngineFactory.scala
+++ b/finagle-core/src/main/scala/com/twitter/finagle/ssl/client/SslClientEngineFactory.scala
@@ -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`.
diff --git a/finagle-core/src/test/scala/com/twitter/finagle/ssl/client/SslClientEngineFactoryTest.scala b/finagle-core/src/test/scala/com/twitter/finagle/ssl/client/SslClientEngineFactoryTest.scala
index f413f2564b..6cc4ae34d2 100644
--- a/finagle-core/src/test/scala/com/twitter/finagle/ssl/client/SslClientEngineFactoryTest.scala
+++ b/finagle-core/src/test/scala/com/twitter/finagle/ssl/client/SslClientEngineFactoryTest.scala
@@ -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") {