Fix race condition when setting up client https connection #20228

[markvandertol]

As discussed in #20228

Fixes a race condition when multiple connections are setup simultaneously that could result in an error when setting up a https connection or sending the wrong SNI to the server.

A better fix would have been to not pass around a mutable SSLParameters object between actors, but that cannot be done without breaking the public api.

[akka-ci]

Can one of the repo owners verify this patch?

[ktoso]

OK TO TEST

[ktoso]

LGTM – Thanks for the fix (and finding it in the first place)!

The better fix mentioned is tricky to pull off, I wonder if we really would be able to do it in a nice way.
On one hand, letting users set "the usual things" on SSLContext is "known territory" for many, so it's a nice api to expose; but the mutability is a pain as seen here... We'd have to otherwise expose our own immutable APIs for passing in more config, that we apply to the context in the actual Actor I guess.

[aruediger]

This can be done in one go without cloning the params if hostInfo is None:

private def applySNI(params: NegotiateNewSession): Unit = for {
  sslParams <- params.sslParameters
  (hostname, _) <- hostInfo
} yield {
  //first copy the mutable SLLParameters before modifying to prevent race condition in `setServerNames`
  val clone = new SSLParameters()
  clone.setCipherSuites(sslParams.getCipherSuites)
  clone.setProtocols(sslParams.getProtocols)
  clone.setWantClientAuth(sslParams.getWantClientAuth)
  clone.setNeedClientAuth(sslParams.getNeedClientAuth)
  clone.setEndpointIdentificationAlgorithm(sslParams.getEndpointIdentificationAlgorithm)
  clone.setAlgorithmConstraints(sslParams.getAlgorithmConstraints)
  clone.setSNIMatchers(sslParams.getSNIMatchers)
  clone.setUseCipherSuitesOrder(sslParams.getUseCipherSuitesOrder)

  // apply the changes
  clone.setServerNames(Collections.singletonList(new SNIHostName(hostname)))
  engine.setSSLParameters(clone)
}

[aruediger]

setServerNames is called only once here.

[ktoso]

I'll be touching this area in a PR in the next hour or so, will apply this idea – thanks @2Beaucoup!

[aruediger]

👍

[akka-ci]

Test PASSed.

[ktoso]

Thanks again @markvandertol 👍