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

build: do not publish HAVE_BORINGSSL, HAVE_AWSLC macros #12065

Closed
wants to merge 1 commit into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 8, 2023

Syncing this up with CMake.

Source code uses the built-in OPENSSL_IS_AWSLC and
OPENSSL_IS_BORINSSL macros to detect BoringSSL and AWS-LC. No help is
necessary from the build tools.

The one use of HAVE_BORINGSSL in the source turned out to be no longer
necessary for warning-free BoringSSL + Schannel builds. Ref: #1610 #2634

autotools detects this anyway for display purposes.
CMake detects this to decide whether to use the BoringSSL-specific
crypto lib with ngtcp2. It detects AWS-LC, but doesn't use the detection
result just yet (planned in #12066).

Ref: #11964

Closes #12065

Syncing this up with CMake.

Source code uses the built-in `OPENSSL_IS_AWSLC` and
`OPENSSL_IS_BORINSSL` macros to detect BoringSSL and AWS-LC. No help
is necessary from the build tools.

autotools detects this anyway for display purposes.
CMake detects this to decide whether to use the BoringSSL-specific
crypto lib with ngtcp2. It detects AWS-LC, but doesn't use the
detection results just yet.

Ref: curl#11964

Closes #xxxxx
@vszakats
Copy link
Member Author

vszakats commented Oct 8, 2023

When the single use of HAVE_BORINGSSL was introduced in cd34ffa, it was used in lib/ldap.c, later moved to schannel.h in 274940d. It was used to avoid wincrypt.h redefinitions with BoringSSL + Schannel MultiSSL builds. One side effect of using HAVE_BORINGSSL (compared to OPENSSL_IS_BORINGSSL) is that it's present before including openssl.h.

Making tests now to see if this trick is still necessary. [→ Confirmed not necessary #12065 (comment).]

Issue: BoringSSL no longer compiles with curl_ngtcp2.c, after introducing SSL_CTX_set_ciphersuites into it unconditionally in aa9a6a1. BoringSSL misses this function. Disabling H3 and retesting.

@bagder
Copy link
Member

bagder commented Oct 8, 2023

BoringSSL no longer compiles with curl_ngtcp2

I tried to copy the logic from openssl.c when I did that, but then clearly I failed...

@jay
Copy link
Member

jay commented Oct 8, 2023

curl/lib/vtls/openssl.c

Lines 203 to 217 in 6fa1d81

/* Whether SSL_CTX_set_ciphersuites is available.
* OpenSSL: supported since 1.1.1 (commit a53b5be6a05)
* BoringSSL: no
* LibreSSL: supported since 3.4.1 (released 2021-10-14)
*/
#if ((OPENSSL_VERSION_NUMBER >= 0x10101000L && \
!defined(LIBRESSL_VERSION_NUMBER)) || \
(defined(LIBRESSL_VERSION_NUMBER) && \
LIBRESSL_VERSION_NUMBER >= 0x3040100fL)) && \
!defined(OPENSSL_IS_BORINGSSL)
#define HAVE_SSL_CTX_SET_CIPHERSUITES
#if !defined(OPENSSL_IS_AWSLC)
#define HAVE_SSL_CTX_SET_POST_HANDSHAKE_AUTH
#endif
#endif

and then #ifdef HAVE_SSL_CTX_SET_CIPHERSUITES

@jay
Copy link
Member

jay commented Oct 8, 2023

though for ngtcp2 maybe we don't need as much legacy check

diff --git a/lib/vquic/curl_ngtcp2.c b/lib/vquic/curl_ngtcp2.c
index 27711ef..f01f90c 100644
--- a/lib/vquic/curl_ngtcp2.c
+++ b/lib/vquic/curl_ngtcp2.c
@@ -430,6 +430,7 @@ static CURLcode quic_ssl_ctx(SSL_CTX **pssl_ctx,
     }
   }
 
+#ifndef OPENSSL_IS_BORINGSSL
   {
     const char *ciphers13 = conn->ssl_config.cipher_list13 ?
       conn->ssl_config.cipher_list13 : QUIC_CIPHERS;
@@ -439,6 +440,7 @@ static CURLcode quic_ssl_ctx(SSL_CTX **pssl_ctx,
     }
     infof(data, "QUIC cipher selection: %s", ciphers13);
   }
+#endif
 
   /* Open the file if a TLS or QUIC backend has not done this before. */
   Curl_tls_keylog_open();

@vszakats
Copy link
Member Author

vszakats commented Oct 8, 2023

@jay: Agreed, H3 assumes LibreSSL 3.7 or quictls 3.0 (or AWC-LC, not sure when it added this function but 1.15.0 has it).

@vszakats
Copy link
Member Author

vszakats commented Oct 8, 2023

The HAVE_BORINGSSL trick is no longer needed, confirmed with a warning-free BoringSSL + Schannel build:

curl 8.4.0-DEV (x86_64-w64-mingw32) libcurl/8.4.0-DEV BoringSSL/b98ce18c (Schannel) zlib/1.2.13 brotli/1.1.0 zstd/1.5.4 WinIDN nghttp2/1.52.0
Release-Date: [unreleased]
Protocols: dict file ftp ftps gopher gophers http https imap imaps ldap ldaps mqtt pop3 pop3s rtsp smb smbs smtp smtps telnet tftp ws wss
Features: alt-svc AsynchDNS brotli HSTS HTTP2 HTTPS-proxy IDN IPv6 Kerberos Largefile libz MultiSSL NTLM SPNEGO SSL SSPI threadsafe Unicode UnixSockets zstd

@vszakats vszakats closed this in 58a95b6 Oct 8, 2023
@vszakats vszakats deleted the curl-drop-unused branch October 8, 2023 22:36
vszakats added a commit to vszakats/curl that referenced this pull request Oct 8, 2023
Add guard around `SSL_CTX_set_ciphersuites()` use.

Bug: curl#12065 (comment)

Follow-up to aa9a6a1

Co-authored-by: Jay Satiro
Closes #xxxxx
vszakats added a commit that referenced this pull request Oct 9, 2023
Add guard around `SSL_CTX_set_ciphersuites()` use.

Bug: #12065 (comment)

Follow-up to aa9a6a1

Co-authored-by: Jay Satiro
Reviewed-by: Daniel Stenberg
Closes #12067
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants