diff --git a/CHANGES b/CHANGES index 3b437fb20e..9d1cbea0b5 100644 --- a/CHANGES +++ b/CHANGES @@ -7,6 +7,13 @@ Note that ``PHAB_ID=#`` and ``RB_ID=#`` correspond to associated messages in com Unreleased ---------- +Breaking API Changes +~~~~~~~~~~~~~~~~~~~~ + + * finagle-core: `c.t.f.ssl.client.HostnameVerifier` has been removed since it was using + `sun.security.util.HostnameChecker` which is no longer accessible in JDK 9. + ``PHAB_ID=D144149`` + Runtime Behavior Changes ~~~~~~~~~~~~~~~~~~~~~~~~ @@ -14,6 +21,13 @@ Runtime Behavior Changes the client's request rate is too low to accurately update the EMA value or drop requests. ``PHAB_ID=D143996`` + * finagle-core: SSL/TLS client hostname verification is no longer performed by + `c.t.f.ssl.client.HostnameVerifier`. The same underlying library + `sun.security.util.HostnameChecker` is used to perform the hostname verification. + However it now occurs before the SSL/TLS handshake has been completed, and the + exception on failure has changes from a `c.t.f.SslHostVerificationException` to a + `javax.net.ssl.CertificateException`. ``PHAB_ID=D144149`` + 18.3.0 ------- diff --git a/finagle-core/src/main/scala/com/twitter/finagle/ssl/SslConfigurations.scala b/finagle-core/src/main/scala/com/twitter/finagle/ssl/SslConfigurations.scala index fa22307f7f..11ec3484b0 100644 --- a/finagle-core/src/main/scala/com/twitter/finagle/ssl/SslConfigurations.scala +++ b/finagle-core/src/main/scala/com/twitter/finagle/ssl/SslConfigurations.scala @@ -115,6 +115,25 @@ private[ssl] object SslConfigurations { sslEngine.setEnabledProtocols(protocols.toArray) } + /** + * If a non-empty hostname is supplied, the engine is set to use + * "HTTPS"-style hostname verification. + * + * See: `javax.net.ssl.SSLParameters#setEndpointIdentificationAlgorithm` and + * https://docs.oracle.com/javase/7/docs/technotes/guides/security/StandardNames.html + * for more details. + */ + def configureHostnameVerification( + sslEngine: SSLEngine, + hostname: Option[String] + ): Unit = hostname match { + case Some(_) => + val sslParameters = sslEngine.getSSLParameters() + sslParameters.setEndpointIdentificationAlgorithm("HTTPS") + sslEngine.setSSLParameters(sslParameters) + case None => // do nothing + } + /** * Guard method for failing fast inside of a factory's apply method when * [[KeyCredentials]] are not supported. diff --git a/finagle-core/src/main/scala/com/twitter/finagle/ssl/client/HostnameVerifier.scala b/finagle-core/src/main/scala/com/twitter/finagle/ssl/client/HostnameVerifier.scala deleted file mode 100644 index 526a347065..0000000000 --- a/finagle-core/src/main/scala/com/twitter/finagle/ssl/client/HostnameVerifier.scala +++ /dev/null @@ -1,38 +0,0 @@ -package com.twitter.finagle.ssl.client - -import com.twitter.finagle.{Address, SslHostVerificationException} -import com.twitter.util.Try -import java.security.cert.X509Certificate -import javax.net.ssl.SSLSession -import sun.security.util.HostnameChecker - -private[finagle] object HostnameVerifier extends SslClientSessionVerifier { - private[this] val checker = HostnameChecker.getInstance(HostnameChecker.TYPE_TLS) - - /** - * Run hostname verification on the session. This will fail with a - * [[com.twitter.finagle.SslHostVerificationException]] if the certificate is - * invalid for the given session. - * - * This uses [[sun.security.util.HostnameChecker]]. Any bugs are theirs. - */ - def apply( - address: Address, - config: SslClientConfiguration, - session: SSLSession - ): Boolean = { - config.hostname match { - case Some(host) => - // We take the first certificate from the given `getPeerCertificates` array since the expected - // array structure is peer's own certificate first followed by any certificate authorities. - val isValid = session.getPeerCertificates.headOption.exists { - case x509: X509Certificate => Try(checker.`match`(host, x509)).isReturn - case _ => false - } - - if (isValid) true - else throw new SslHostVerificationException(session.getPeerPrincipal.getName) - case None => SslClientSessionVerifier.AlwaysValid(address, config, session) - } - } -} 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 2ae321dfd7..f6589f0539 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 @@ -57,6 +57,7 @@ object SslClientEngineFactory { SslConfigurations.configureProtocols(sslEngine, config.protocols) SslConfigurations.configureCipherSuites(sslEngine, config.cipherSuites) + SslConfigurations.configureHostnameVerification(sslEngine, config.hostname) } /** diff --git a/finagle-core/src/main/scala/com/twitter/finagle/ssl/client/SslClientSessionVerifier.scala b/finagle-core/src/main/scala/com/twitter/finagle/ssl/client/SslClientSessionVerifier.scala index c4996b5e85..4a8d5ed333 100644 --- a/finagle-core/src/main/scala/com/twitter/finagle/ssl/client/SslClientSessionVerifier.scala +++ b/finagle-core/src/main/scala/com/twitter/finagle/ssl/client/SslClientSessionVerifier.scala @@ -33,7 +33,7 @@ object SslClientSessionVerifier { * whether the SSL/TLS connection should be used. It is an additional level * of verification beyond the standard certificate chain verification checking. * - * @note By default a `HostnameVerifier` will be used if this + * @note By default sessions will be seen as `AlwaysValid` if this * param is not configured. */ case class Param(verifier: SslClientSessionVerifier) { @@ -41,7 +41,7 @@ object SslClientSessionVerifier { (this, Param.param) } object Param { - implicit val param = Stack.Param(Param(HostnameVerifier)) + implicit val param = Stack.Param(Param(AlwaysValid)) } /** diff --git a/finagle-core/src/test/scala/com/twitter/finagle/ssl/SslConfigurationsTest.scala b/finagle-core/src/test/scala/com/twitter/finagle/ssl/SslConfigurationsTest.scala index abd6d1353b..7bc9cdec4e 100644 --- a/finagle-core/src/test/scala/com/twitter/finagle/ssl/SslConfigurationsTest.scala +++ b/finagle-core/src/test/scala/com/twitter/finagle/ssl/SslConfigurationsTest.scala @@ -1,11 +1,8 @@ package com.twitter.finagle.ssl import javax.net.ssl.{SSLContext, SSLEngine} -import org.junit.runner.RunWith import org.scalatest.FunSuite -import org.scalatest.junit.JUnitRunner -@RunWith(classOf[JUnitRunner]) class SslConfigurationsTest extends FunSuite { private[this] def createTestEngine(): SSLEngine = { @@ -61,6 +58,26 @@ class SslConfigurationsTest extends FunSuite { } } + test("configureHostnameVerifier turns on endpoint identification when hostname is set") { + val sslEngine = createTestEngine() + val sslParametersBefore = sslEngine.getSSLParameters + assert(sslParametersBefore.getEndpointIdentificationAlgorithm == null) + + SslConfigurations.configureHostnameVerification(sslEngine, Some("twitter.com")) + val sslParametersAfter = sslEngine.getSSLParameters + assert(sslParametersAfter.getEndpointIdentificationAlgorithm == "HTTPS") + } + + test("configureHostnameVerifier leaves endpoint identification off when hostname is empty") { + val sslEngine = createTestEngine() + val sslParametersBefore = sslEngine.getSSLParameters + assert(sslParametersBefore.getEndpointIdentificationAlgorithm == null) + + SslConfigurations.configureHostnameVerification(sslEngine, None) + val sslParametersAfter = sslEngine.getSSLParameters + assert(sslParametersAfter.getEndpointIdentificationAlgorithm == null) + } + test("checkKeyCredentialsNotSupported does nothing for Unspecified") { val keyCredentials = KeyCredentials.Unspecified SslConfigurations.checkKeyCredentialsNotSupported("TestFactory", keyCredentials)