HTTP.request(): infer socket_type_tls from sslconfig, if provided (#1106)

* Add NetworkOptions to test deps

* Failing test for #1104

* infer socket_type_tls from sslconfig, if provided

Fixes #1104.

This PR only affects client requests, because those were the only ones
broken. For now OpenSSL is not supported in HTTP.jl servers (and,
honestly, maybe that's a good thing, OpenSSL is kinda crap).

I didn't just revert the change to SOCKET_TYPE_TLS[] because that change
has been released for months and users may now be relying on `sslconfig`
accepting an `OpenSSL.SSLContext` value, just as I was relying on it
accepting an `MbedTLS.SSLConfig` value.

* Delete test/Manifest.toml

Author: cmcaine

Project.toml

@@ -39,6 +39,7 @@ InteractiveUtils = "b77e0a4c-d291-57a0-90e8-8db25a27a240"
 JSON = "682c06a0-de6a-54ab-a142-c8b1cf79cde6"
 Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
 Unitful = "1986cc42-f94f-5a68-af5c-568840ba703d"
+NetworkOptions = "ca575930-c2e3-43a9-ace4-1e988b2c1908"
 
 [targets]
-test = ["BufferedStreams", "Deno_jll", "Distributed", "InteractiveUtils", "JSON", "Test", "Unitful"]
+test = ["BufferedStreams", "Deno_jll", "Distributed", "InteractiveUtils", "JSON", "Test", "Unitful", "NetworkOptions"]

src/clientlayers/ConnectionRequest.jl

@@ -1,8 +1,8 @@
 module ConnectionRequest
 
 using URIs, Sockets, Base64, LoggingExtras, ConcurrentUtilities, ExceptionUnwrapping
-using MbedTLS: SSLContext, SSLConfig
-using OpenSSL: SSLStream
+import MbedTLS
+import OpenSSL
 using ..Messages, ..IOExtras, ..Connections, ..Streams, ..Exceptions
 import ..SOCKET_TYPE_TLS
 
@@ -55,7 +55,7 @@ Close the connection if the request throws an exception.
 Otherwise leave it open so that it can be reused.
 """
 function connectionlayer(handler)
-    return function connections(req; proxy=getproxy(req.url.scheme, req.url.host), socket_type::Type=TCPSocket, socket_type_tls::Type=SOCKET_TYPE_TLS[], readtimeout::Int=0, connect_timeout::Int=30, logerrors::Bool=false, logtag=nothing, kw...)
+    return function connections(req; proxy=getproxy(req.url.scheme, req.url.host), socket_type::Type=TCPSocket, socket_type_tls::Union{Nothing, Type}=nothing, readtimeout::Int=0, connect_timeout::Int=30, logerrors::Bool=false, logtag=nothing, kw...)
         local io, stream
         if proxy !== nothing
             target_url = req.url
@@ -74,7 +74,7 @@ function connectionlayer(handler)
         end
 
         connect_timeout = connect_timeout == 0 && readtimeout > 0 ? readtimeout : connect_timeout
-        IOType = sockettype(url, socket_type, socket_type_tls)
+        IOType = sockettype(url, socket_type, socket_type_tls, get(kw, :sslconfig, nothing))
         start_time = time()
         try
             io = newconnection(IOType, url.host, url.port; readtimeout=readtimeout, connect_timeout=connect_timeout, kw...)
@@ -148,7 +148,62 @@ function connectionlayer(handler)
     end
 end
 
-sockettype(url::URI, tcp, tls) = url.scheme in ("wss", "https") ? tls : tcp
+function sockettype(url::URI, socket_type_tcp, socket_type_tls, sslconfig)
+    if url.scheme in ("wss", "https")
+        tls_socket_type(socket_type_tls, sslconfig)
+    else
+        socket_type_tcp
+    end
+end
+
+"""
+    tls_socket_type(socket_type_tls, sslconfig)::Type
+
+Find the best TLS socket type, given the values of these keyword arguments.
+
+If both are `nothing` then we use the global default: `HTTP.SOCKET_TYPE_TLS[]`.
+If both are not `nothing` then they must agree:
+`sslconfig` must be of the right type to configure `socket_type_tls` or we throw an `ArgumentError`.
+"""
+function tls_socket_type(socket_type_tls::Union{Nothing, Type},
+                         sslconfig::Union{Nothing, MbedTLS.SSLConfig, OpenSSL.SSLContext}
+                         )::Type
+
+    socket_type_matching_sslconfig =
+        if sslconfig isa MbedTLS.SSLConfig
+            MbedTLS.SSLContext
+        elseif sslconfig isa OpenSSL.SSLContext
+            OpenSSL.SSLStream
+        else
+            nothing
+        end
+
+    if socket_type_tls === socket_type_matching_sslconfig
+        # Use the global default TLS socket if they're both nothing, or use
+        # what they both specify if they're not nothing.
+        isnothing(socket_type_tls) ? SOCKET_TYPE_TLS[] : socket_type_tls
+    # If either is nothing, use the other one.
+    elseif isnothing(socket_type_tls)
+        socket_type_matching_sslconfig
+    elseif isnothing(socket_type_matching_sslconfig)
+        socket_type_tls
+    else
+        # If they specify contradictory types, throw an error.
+        # Error thrown in noinline closure to avoid speed penalty in common case
+        @noinline function err(socket_type_tls, sslconfig)
+            msg = """
+                Incompatible values for keyword args `socket_type_tls` and `sslconfig`:
+                    socket_type_tls=$socket_type_tls
+                    typeof(sslconfig)=$(typeof(sslconfig))
+
+                Make them match or provide only one of them.
+                - the socket type MbedTLS.SSLContext is configured by MbedTLS.SSLConfig
+                - the socket type OpenSSL.SSLStream is configured by OpenSSL.SSLContext"""
+            throw(ArgumentError(msg))
+        end
+        err(socket_type_tls, sslconfig)
+    end
+end
 
 function connect_tunnel(io, target_url, req)
     target = "$(URIs.hoststring(target_url.host)):$(target_url.port)"

test/client.jl

@@ -24,6 +24,15 @@ for x in (10, 12)
     @test getfield(HTTP.Connections.OPENSSL_POOL[], max_or_limit) == x
 end
 
+@testset "sslconfig without explicit socket_type_tls #1104" begin
+    # this was supported before 8f35185
+    @test isok(HTTP.get("https://$httpbin/ip", sslconfig=MbedTLS.SSLConfig(false)))
+    # The OpenSSL package doesn't have enough docs, but this is a valid way to initialise an SSLContext.
+    @test isok(HTTP.get("https://$httpbin/ip", sslconfig=OpenSSL.SSLContext(OpenSSL.TLSClientMethod())))
+    # Incompatible socket_type_tls and sslconfig should throw an error.
+    @test_throws ArgumentError HTTP.get("https://$httpbin/ip", sslconfig=MbedTLS.SSLConfig(false), socket_type_tls=OpenSSL.SSLStream)
+end
+
 @testset "@client macro" begin
     @eval module MyClient
         using HTTP