Http4s: BlazeClientBuilder: SslContext.getDefault can throw outside the monad

[nigredo-tori]

This is another appearance of the problem mentioned in #1956 . On some systems SslContext.getDefault can throw, which is not handled correctly. BlazeClientBuilder uses SslContext.getDefault as a default argument, which leaves capturing the exception to the user.

[kevinmeredith]

Is the motivation to use:

sslContext: Option[SSLContext] = Some(SSLContext.getDefault)): BlazeClientBuilder[F] =

such that cacerts will be used for the Truststore as a default?

Note - I don't know that that's the behavior of SSLContext.getDefault, but am asking.

[nigredo-tori]

@kevinmeredith, would you care to rephrase your question? English is not my first language, and I have trouble parsing your comment after a day of work.

Anyway, this issue has nothing to do with the inner workings of SSLContext. We have an old monolith system that (for legal reasons) has to use non-standard cryptography implementation, which breaks the default SSL context. We're attempting to gradually switch to microservice architecture using http4s for communication. However, since we're just starting, the services use bare HTTP for communication - so we didn't bother with passing sslContext, and got an exception. We have obviously worked around this, but the fact that the default argument value can throw outside the monad is still a bug.

[kevinmeredith]

Hi @nigredo-tori - I'm sorry that I was not clear. My above question was meant for the http4s maintainers.

[rossabaker]

I'm not generally a fan of passing effects as arguments. Option and None have different meanings here, so we can't pass None and getOrElse it. Some(null) would be awful. Eval is not for effect capture. A sum type to work around the fact that it fails for a few people would be awkward.

Maybe F[Option[SslContext]] is the least bad choice?

[nigredo-tori]

@rossabaker, I don't think we need to pass an effect. As far as I can see, None just leads to an SSLContext.getDefault call down the road, but that one is lazy, and is evaluated inside a Future. So switching from Some(SSLContext.getDefault) to None should suffice here.

[rossabaker]

Oh, you're right. I thought None was how we disabled SSL. But there's not a compelling reason to disable SSL: just don't call https URLs. Or, create an SSLContext that rejects everything.

Yes, None seems like a good approach here.

[rossabaker]

Maybe in the long run, a sum type rather than abusing Option is a better way to express this. But #2604 fixes this in a binary compatible way in 0.20.

[kevinmeredith]

Maybe in the long run, a sum type rather than abusing Option is a better way to express this.

I firmly agree that using a sum type would make this code easier to understand, @rossabaker. My teammate and I just ran into an HTTPS error, following the 0.20.x -> 0.21.4 upgrade, in code using BlazeClientBuilder#apply with a None value for sslContext. We figured out what we did wrong wrong when we saw https://github.com/http4s/http4s/blob/v0.20.23/blaze-client/src/main/scala/org/http4s/client/blaze/BlazeClientBuilder.scala#L135-L139.

[rossabaker]

Too late for 0.21, but we could modify that in 1.0. Want to give it a shot?