Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unintentional(?) breaking change to type of sslconfig #1104

Closed
cmcaine opened this issue Sep 6, 2023 · 1 comment · Fixed by #1106
Closed

Unintentional(?) breaking change to type of sslconfig #1104

cmcaine opened this issue Sep 6, 2023 · 1 comment · Fixed by #1106

Comments

@cmcaine
Copy link
Contributor

cmcaine commented Sep 6, 2023

const SOCKET_TYPE_TLS = Ref{Any}(OpenSSL.SSLStream)

This is a breaking change because it changes the accepted type for the sslconfig keyword argument that many functions in the public API of HTTP.jl accept.

Did you intend to change this? It seems unrelated to your other changes in b64beb7#r126540636

@cmcaine
Copy link
Contributor Author

cmcaine commented Sep 6, 2023

Because other users may now depend on sslconfig accepting an OpenSSL.SSLContext, we could choose the TLS library based on the type of the provided sslconfig (if provided), which seems like an improvement to me.

This is the only function that needs to be modified:

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...)

The change of default TLS socket type should still be reverted or documented, imo.

cmcaine added a commit to cmcaine/HTTP.jl that referenced this issue Sep 7, 2023
cmcaine added a commit to cmcaine/HTTP.jl that referenced this issue Sep 7, 2023
Fixes JuliaWeb#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.
cmcaine added a commit to cmcaine/HTTP.jl that referenced this issue Sep 7, 2023
cmcaine added a commit to cmcaine/HTTP.jl that referenced this issue Sep 7, 2023
Fixes JuliaWeb#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.
quinnj pushed a commit that referenced this issue Apr 6, 2024
…#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant