Skip to content

build: do not publish HAVE_BORINGSSL, HAVE_AWSLC macros#12065

Closed
vszakats wants to merge 1 commit intocurl:masterfrom
vszakats:curl-drop-unused
Closed

build: do not publish HAVE_BORINGSSL, HAVE_AWSLC macros#12065
vszakats wants to merge 1 commit intocurl:masterfrom
vszakats:curl-drop-unused

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.

3 participants