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

configure: error out on multissl + http3 #12807

Closed
wants to merge 2 commits into from
Closed

Conversation

bagder
Copy link
Member

@bagder bagder commented Jan 26, 2024

Since the QUIC/h3 code has not knowledge or handling of multissl it might bring unintended consequences if we allow it.

Ref: #12806

@bagder bagder added build TLS HTTP/3 h3 or quic related labels Jan 26, 2024
@bagder bagder mentioned this pull request Jan 26, 2024
@@ -4691,6 +4691,9 @@ fi

if test "x$USE_NGTCP2_H3" = "x1" -o "x$USE_QUICHE" = "x1" \
-o "x$USE_OPENSSL_H3" = "x1" -o "x$USE_MSH3" = "x1"; then
if test "x$CURL_WITH_MULTI_SSL" = "x1"; then
AC_MSG_ERROR([MultiSSL cannot be enabled with HTTP/3 and vice versa])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTTP3 + Schannel/SecureTransport are working fine and the above disallows them too. It'd be useful to let these combos through. (curl-for-win uses them in default builds.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are working fine

In what combinations have you tested this? We don't test anything of this in CI and the h3 code has no multissl awareness. It feels brittle and risky.

Copy link
Member

@vszakats vszakats Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not doing extra testing with these builds, but they were built like this (OpenSSL + Schannel) since MultiSSL became available.

Since the introduction of HTTP/3 (around 2022-05-23), I haven't re-evaluated that, but quictls/LibreSSL + Schannel continued to build and appeared to work fine in basic tests and without any obvious issues or bug reports. They also have fairly distinct codepaths, except some crypto backend stuff unrelated to HTTP/3.

It'd be a big shock if this combo was broken all along.

...testing with fresh debug builds under WINE (with script #12806, but with curl.se, --disable and just -v; not -v4), I see no leaks, but schannel doesn't use H2, only H1 (also true with pure schannel build) and it does actually touch the QUIC codepath (with QUIC connect to), but works. LibreSSL also works with no leaks, in H3 mode. (-v4 resets H3 to H2. Is it a valid curl option?)

[ If this is truly broken, or unsupported, curl-for-win will need a reshuffle. Back then without Schannel, it was not possible to download cacert.pem (to bootstrap OpenSSL) via HTTPS because of the lack of native CA support. Maybe --ca-native/CURLSSLOPT_NATIVE_CA options fix that now? Schannel is also useful to do cross-checks for bug reports, and also to keep the Schannel codepath build-tested with curl-for-win options. I'd also expect at least some users relying on this in some form or another. ]

edit: -4 is for IPv4 of course.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't there also be something like this:

#if defined(CURL_WITH_MULTI_SSL) && defined(USE_NGHTTP3)
#error "Multi-SSL with Nghttp3 is unsupported"
#endif

Ref. the snippet in curl_setup.h:

#if defined(USE_LIBIDN2) && defined(USE_WIN32_IDN)
#error "Both libidn2 and WinIDN are enabled, choose one."
#endif

Copy link
Member Author

@bagder bagder Jan 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The QUIC code has no awareness of multissl. Nobody has worked on that yet. The QUIC backends can't change TLS at run-time. It is not about nghttp3 specifically. It goes for all QUIC related code in libcurl.

If we in the multssl code initialize the selected TLS, it might be a separate TLS library used there than is used in QUIC which then is not initialized properly before use.

I have not actually tried this myself, but I can see how things like this can happen. I rather be cautious and say that QUIC/h3 is not tested or confirmed to work with multissl.

I like to believe that multissl is less needed these days since the introduction of --ca-native, but I don't know exactly for which other purposes people previously have selected to use the Schannel backend.

Copy link
Member

@vszakats vszakats Jan 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess in that case, the best is to shut down MultiSSL. (* in curl-for-win)

Just confirmed on a real Windows machine with network, that --ca-native works, so Schannel is no longer necessary for using HTTPS (without having a hand-rolled CA bundle that is.) curl-for-win also ships with a CA bundle.

As for macOS, --ca-native doesn't work, but there is a CA bundle at /etc/ssl/cert.pem (and also one provided for example by Homebrew.) I'm guessing these are still not native (and thus missing custom entries added via Keychain Access), but good enough to boot.

vszakats added a commit to curl/curl-for-win that referenced this pull request Jan 27, 2024
vszakats added a commit to curl/curl-for-win that referenced this pull request Jan 27, 2024
This will not actively detect and prevent enabling multiple TLS backends
(aka MultiSSL) enabled with HTTP/3, e.g. OpenSSL[-fork] +
mbedTLS/wolfSSL, but those cases might be rejected by the curl build
itself.

In effect MultiSSL and HTTP/3 are mutually exclusive options in curl.

Ref: curl/curl#12807
vszakats added a commit to curl/curl-for-win that referenced this pull request Jan 27, 2024
This will not actively detect and prevent enabling multiple TLS backends
(aka MultiSSL) enabled with HTTP/3, e.g. OpenSSL[-fork] +
mbedTLS/wolfSSL, but those cases might be rejected by the curl build
itself.

In effect MultiSSL and HTTP/3 are mutually exclusive options in curl.

after:
```
curl 8.5.0 (x86_64-w64-mingw32) libcurl/8.5.0 LibreSSL/3.8.2 zlib/1.3.1 brotli/1.1.0 zstd/1.5.5 WinIDN libpsl/0.21.5 libssh2/1.11.0 nghttp2/1.59.0 ngtcp2/1.2.0 nghttp3/1.1.0
Release-Date: 2023-12-06
Protocols: dict file ftp ftps gopher gophers http https imap imaps ldap ldaps mqtt pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp ws wss
Features: alt-svc AsynchDNS brotli HSTS HTTP2 HTTP3 HTTPS-proxy IDN IPv6 Largefile libz NTLM PSL SSL threadsafe UnixSockets zstd
```

before:
```
curl 8.5.0 (x86_64-w64-mingw32) libcurl/8.5.0 LibreSSL/3.8.2 (Schannel) zlib/1.3.1 brotli/1.1.0 zstd/1.5.5 WinIDN libpsl/0.21.5 libssh2/1.11.0 nghttp2/1.59.0 ngtcp2/1.2.0 nghttp3/1.1.0
Release-Date: 2023-12-06
Protocols: dict file ftp ftps gopher gophers http https imap imaps ldap ldaps mqtt pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp ws wss
Features: alt-svc AsynchDNS brotli HSTS HTTP2 HTTP3 HTTPS-proxy IDN IPv6 Kerberos Largefile libz MultiSSL NTLM PSL SPNEGO SSL SSPI threadsafe UnixSockets zstd
```

Ref: curl/curl#12807
@vszakats
Copy link
Member

vszakats commented Jan 27, 2024

Same for CMake:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -720,6 +720,10 @@ if(USE_MSH3)
   list(APPEND CURL_LIBS ${MSH3_LIBRARIES})
 endif()
 
+if(CURL_WITH_MULTI_SSL AND (USE_NGTCP2 OR USE_QUICHE OR USE_MSH3))
+  message(FATAL_ERROR "MultiSSL cannot be enabled with HTTP/3 and vice versa.")
+endif()
+
 if(NOT CURL_DISABLE_SRP AND (HAVE_GNUTLS_SRP OR HAVE_OPENSSL_SRP))
   set(USE_TLS_SRP 1)
 endif()

Since the QUIC/h3 code has no knowledge or handling of multissl it might
bring unintended consequences if we allow it.

Ref: #12806
@bagder bagder closed this in 011325f Jan 29, 2024
@bagder bagder deleted the bagder/multissl-h3 branch January 29, 2024 15:37
@bagder
Copy link
Member Author

bagder commented Jan 29, 2024

thanks @vszakats and @gvanem for your assistance!

junhochoi added a commit to junhochoi/homebrew-cloudflare that referenced this pull request Feb 1, 2024
--with-secure-transport is commented (I don't think it's actually used)
(ref: curl/curl#12807)
junhochoi added a commit to junhochoi/homebrew-cloudflare that referenced this pull request Feb 1, 2024
--with-secure-transport is removed (I don't think it's actually used)
(ref: curl/curl#12807)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build HTTP/3 h3 or quic related TLS
Development

Successfully merging this pull request may close these issues.

None yet

3 participants