Skip to content

Conversation

@yedayak
Copy link
Contributor

@yedayak yedayak commented Dec 2, 2025

Only the TLS 1.2 structure for now since it's simpler, and only has a single label type.
This has the bonus of also testing libressl that only supports logging keys in TLS 1.2

Fix fallout in GnuTLS.

@testclutch
Copy link

Analysis of PR #19816 at fa623505:

Test 2090 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 48 different CI jobs (the link just goes to one of them).

Generated by Testclutch

@yedayak yedayak force-pushed the sslkeylog-test branch 2 times, most recently from 0bc103d to d289417 Compare December 3, 2025 11:05
@yedayak
Copy link
Contributor Author

yedayak commented Dec 3, 2025

I managed to reproduce the wolfssl failure, it seems that it needs to be built with CFLAGS=-DHAVE_EX_DATA=1 or else wolfSSL_get_app_data and wolfSSL_set_app_data don't work. Not sure why this doesn't fail other tests though
Edit: wolfssl has a configure option to enable it, added it to the GHA task for building opensslextra, but I guess the cache will need to be deleted?

@yedayak yedayak marked this pull request as ready for review December 3, 2025 11:38
@github-actions github-actions bot added the CI Continuous Integration label Dec 4, 2025
@bagder
Copy link
Member

bagder commented Dec 4, 2025

@vszakats any idea on the wolfSSL thing @yedayak mentions above?

@vszakats
Copy link
Member

vszakats commented Dec 5, 2025

Hm, it's curl: ../../lib/vtls/wolfssl.c:486: wssl_vtls_new_session_cb: Assertion cf != ((void *)0)' failed.
It's curious how this is triggered by setting SSLKEYLOGFILE. Session cache depends on this wolfSSL feature, in some cases? I'll take a look tomorrow.

This patch builds wolfssl-opensslextra for this PR, without using the cache:

--- a/.github/workflows/linux.yml
+++ b/.github/workflows/linux.yml
@@ -481,18 +481,8 @@ jobs:
             --disable-benchmark --disable-crypttests --disable-examples --prefix=/home/runner/wolfssl-all
           make install
 
-      - name: 'cache wolfssl (opensslextra)'  # does support `OPENSSL_COEXIST`
-        if: ${{ contains(matrix.build.install_steps, 'wolfssl-opensslextra') }}
-        uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.3.0
-        id: cache-wolfssl-opensslextra
-        env:
-          cache-name: cache-wolfssl-opensslextra
-        with:
-          path: ~/wolfssl-opensslextra
-          key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.WOLFSSL_VERSION }}
-
       - name: 'build wolfssl (opensslextra)'
-        if: ${{ contains(matrix.build.install_steps, 'wolfssl-opensslextra') && steps.cache-wolfssl-opensslextra.outputs.cache-hit != 'true' }}
+        if: ${{ contains(matrix.build.install_steps, 'wolfssl-opensslextra') }}
         run: |
           curl --disable --fail --silent --show-error --connect-timeout 15 --max-time 120 --retry 6 --retry-connrefused \
             --location "https://github.com/wolfSSL/wolfssl/archive/v${WOLFSSL_VERSION}-stable.tar.gz" | tar -xz

@vszakats
Copy link
Member

vszakats commented Dec 5, 2025

This wolfSSL option is automatically enabled for wolfSSL QUIC builds,
which makes it easier to go unnoticed.

The public macro telling the feature is built-in is HAVE_EX_DATA.
curl should probably gracefully exclude features that depend on this
feature.

Did not yet dig why it's triggering, but opened #19852 trying to avoid
a leak for !HAVE_EX_DATA wolfSSLs.

@vszakats
Copy link
Member

vszakats commented Dec 5, 2025

Confirmed that patch #19852 fixed the wolfssl-opensslextra crash.

vszakats added a commit that referenced this pull request Dec 7, 2025
Without this option `wolfSSL_get_app_data()` always returns NULL.
Disable codepaths using it (and its `set` pair) when curl is built
against a wolfSSL library with this option missing.

Fixing:
```
curl: ../../lib/vtls/wolfssl.c:486: wssl_vtls_new_session_cb: Assertion `cf != ((void *)0)' failed.
```

wolfSSL can be built with the `--enable-context-extra-user-data` or
`-DWOLFSSL_EX_DATA` option to enable this feature. Some higher-level
features also enable it automatically like QUIC, ASIO.

Reported-by: Yedaya Katsman
Bug: #19816 (comment)
Ref: https://github.com/curl/curl/actions/runs/19871780796/job/56949160740

Closes #19852
Only the TLS 1.2 structure for now since it's simpler, and only has a
single label type. This has the bonus of also testing libressl that only
supports logging keys in TLS 1.2
@bagder bagder closed this in e76080f Dec 9, 2025
@bagder
Copy link
Member

bagder commented Dec 9, 2025

Thanks!

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

Labels

CI Continuous Integration tests

Development

Successfully merging this pull request may close these issues.

4 participants