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

update ECH APIs to those agreed with OpenSSL maintainers #15945

Closed
wants to merge 4 commits into from

Conversation

sftcd
Copy link
Contributor

@sftcd sftcd commented Jan 8, 2025

We're in the process of getting our ECH code upstreamed to the OpenSSL project. As part of that, we agreed some API changes with the OpenSSL maintainers. This PR has the minor changes to bring the curl ECH code up to date with those.

@github-actions github-actions bot added the TLS label Jan 8, 2025
lib/vtls/openssl.c Outdated Show resolved Hide resolved
@vszakats
Copy link
Member

vszakats commented Jan 9, 2025

Does it mean openssl/ech.h header is dropped from OpenSSL/patch,
or is required to have SSL_set1_ech_config_list declared?

(In BoringSSL/AWS-LC it's declared in openssl/ssl.h)

@sftcd
Copy link
Contributor Author

sftcd commented Jan 9, 2025

So far we still have openssl/ech.h in the feature branch.

@vszakats
Copy link
Member

vszakats commented Jan 9, 2025

From a feature-detection angle it'd help us (possibly other projects)
to sync it with BoringSSL.

This patch drops SSL_ech_set1_echconfig references and
syncs build systems with this PR:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 1ede5306f..5a1542ccd 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -961,11 +961,10 @@ if(USE_ECH)
     if(HAVE_BORINGSSL OR HAVE_AWSLC)
       curl_openssl_check_symbol_exists("SSL_set1_ech_config_list" "openssl/ssl.h" HAVE_SSL_SET1_ECH_CONFIG_LIST)
     elseif(HAVE_OPENSSL)
-      curl_openssl_check_symbol_exists("SSL_ech_set1_echconfig" "openssl/ech.h" HAVE_SSL_ECH_SET1_ECHCONFIG)
+      curl_openssl_check_symbol_exists("SSL_set1_ech_config_list" "openssl/ech.h" HAVE_SSL_SET1_ECH_CONFIG_LIST)
     endif()
     if(HAVE_WOLFSSL_CTX_GENERATEECHCONFIG OR
-       HAVE_SSL_SET1_ECH_CONFIG_LIST OR
-       HAVE_SSL_ECH_SET1_ECHCONFIG)
+       HAVE_SSL_SET1_ECH_CONFIG_LIST)
       set(HAVE_ECH 1)
     endif()
     if(NOT HAVE_ECH)
diff --git a/configure.ac b/configure.ac
index ecb58a924..ea7bd6f3f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -4877,13 +4877,7 @@ if test "x$want_ech" != "xno"; then
   ECH_ENABLED=0
   ECH_SUPPORT=''
 
-  dnl check for OpenSSL
-  if test "x$OPENSSL_ENABLED" = "x1"; then
-    AC_CHECK_FUNCS(SSL_ech_set1_echconfig,
-      ECH_SUPPORT="ECH support available via OpenSSL with SSL_ech_set1_echconfig"
-      ECH_ENABLED=1)
-  fi
-  dnl check for BoringSSL equivalent
+  dnl check for OpenSSL equivalent
   if test "x$OPENSSL_ENABLED" = "x1"; then
     AC_CHECK_FUNCS(SSL_set1_ech_config_list,
       ECH_SUPPORT="ECH support available via BoringSSL with SSL_set1_ech_config_list"
diff --git a/lib/curl_config.h.cmake b/lib/curl_config.h.cmake
index f21c74f49..778e07337 100644
--- a/lib/curl_config.h.cmake
+++ b/lib/curl_config.h.cmake
@@ -850,6 +850,3 @@ ${SIZEOF_TIME_T_CODE}
 
 /* Define to 1 if you have the SSL_set1_ech_config_list function. */
 #cmakedefine HAVE_SSL_SET1_ECH_CONFIG_LIST
-
-/* Define to 1 if you have the SSL_ech_set1_echconfig function. */
-#cmakedefine HAVE_SSL_ECH_SET1_ECHCONFIG
diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c
index 40c3d956f..aebbf1e54 100644
--- a/lib/vtls/openssl.c
+++ b/lib/vtls/openssl.c
@@ -83,8 +83,7 @@
 #include <openssl/tls1.h>
 #include <openssl/evp.h>
 
-#if defined(HAVE_SSL_SET1_ECH_CONFIG_LIST) || \
-    defined(HAVE_SSL_ECH_SET1_ECHCONFIG)
+#if defined(HAVE_SSL_SET1_ECH_CONFIG_LIST)
 #define USE_ECH_OPENSSL
 #endif

@sftcd
Copy link
Contributor Author

sftcd commented Jan 9, 2025

ah, thanks for that patch, applied and will push

@bagder bagder closed this in 1ae47b9 Jan 10, 2025
@bagder
Copy link
Member

bagder commented Jan 10, 2025

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants