[GitHub] [openwhisk] rabbah commented on a change in pull request #5065: Upgrade to Akka 2.6.11

[GitBox <gi...@apache.org>]

rabbah commented on a change in pull request #5065:
URL: https://github.com/apache/openwhisk/pull/5065#discussion_r586079248



##########
File path: common/scala/src/main/scala/org/apache/openwhisk/common/Https.scala
##########
@@ -35,8 +33,16 @@ object Https {
     cs
   }
 
-  def connectionContext(httpsConfig: HttpsConfig, sslConfig: Option[AkkaSSLConfig] = None) = {
+  def httpsInsecureClient(context: SSLContext): HttpsConnectionContext =
+    ConnectionContext.httpsClient((host, port) => {
+      val engine = context.createSSLEngine(host, port)
+      engine.setUseClientMode(true)
+      // WARNING: this creates an SSL Engine without enabling endpoint identification/verification procedures
+      // Disabling host name verification is a very bad idea, please don't unless you have a very good reason to.

Review comment:
       can you add a comment why it is ok here, given the warning in the comment? ie when is `withDisableHostnameVerification` set and ok?

##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/containerpool/logging/SplunkLogStore.scala
##########
@@ -98,12 +97,22 @@ class SplunkLogStore(
 
   val maxPendingRequests = 500
 
+  def createInsecureSslEngine(host: String, port: Int): SSLEngine = {

Review comment:
       does it make sense to refactor this method into a singleton (it's used in more than one place)?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org