Skip to content

Update hitch support. #9897

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.

Sign up for GitHub

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

Open
kareem-wolfssl wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Update hitch support. #9897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
126f29d
Update hitch configure
kareem-wolfssl Feb 16, 2026
17a3f6f
Update and re-enable hitch CI/CD test.
kareem-wolfssl Mar 5, 2026
9dc0594
ci: use matrix.ref in hitch.yml
MarkAtwood May 11, 2026
bc8c93e
Code review feedback. If WC_USE_STATIC_ASSERT is not defined, then d…
kareem-wolfssl Jun 4, 2026
c7a84a4
Don't define WC_NO_STATIC_ASSERT for tests.
kareem-wolfssl Jun 4, 2026
2015159
Add WC_USE_STATIC_ASSERT to known macros.
kareem-wolfssl Jun 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions .github/workflows/disabled/hitch.yml → .github/workflows/hitch.yml
View file
Open in desktop
Comment thread
kareem-wolfssl marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ jobs:
matrix:
# List of releases to test
include:
- ref: 1.7.3
- ref: 1.8.0

@dgarske dgarske Jun 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟠 [Medium] Two additional hitch tests ignored without explanation

The ignore list grows from 3 to 5 tests: test12-cfg.sh and test41-client-cert-pb.sh are newly skipped for hitch 1.8.0, in addition to the previously skipped test13-r82.sh, test15-proxy-v2-npn.sh, and test39-client-cert-proxy.sh. There is no comment in the workflow (and nothing in the PR description -- the Testing section is empty) explaining why each test is incompatible with the wolfSSL build, whether the failures are expected limitations (e.g. missing NPN, proxy-v2 differences) or temporary gaps that should be tracked. test41-client-cert-pb.sh in particular is a client-cert path; together with the already-skipped test39-client-cert-proxy.sh this means client-certificate behavior through hitch gets thinner CI coverage, and future maintainers have no way to know if these can be re-enabled.

Fix: Add a short comment per newly ignored test stating why it is skipped (known incompatibility vs. tracked gap), or reference the osp issue that documents them, so the list does not silently grow over releases.

ignore-tests: >-
test13-r82.sh test15-proxy-v2-npn.sh test39-client-cert-proxy.sh
test12-cfg.sh test13-r82.sh test15-proxy-v2-npn.sh test39-client-cert-proxy.sh test41-client-cert-pb.sh
name: ${{ matrix.ref }}
if: github.repository_owner == 'wolfssl'
runs-on: ubuntu-latest
Expand All @@ -72,13 +72,13 @@ jobs:
run: |
export DEBIAN_FRONTEND=noninteractive
sudo apt-get update
sudo apt-get install -y libev-dev libssl-dev automake python3-docutils flex bison pkg-config make
sudo apt-get install -y libev-dev automake python3-docutils flex bison pkg-config make

- name: Checkout hitch
uses: actions/checkout@v4
with:
repository: varnish/hitch
ref: 1.7.3
ref: ${{ matrix.ref }}
path: hitch

# Do this before configuring so that it only detects the updated list of
Expand All @@ -92,9 +92,10 @@ jobs:
- name: Configure and build hitch
run: |
cd $GITHUB_WORKSPACE/hitch/
patch -p1 < $GITHUB_WORKSPACE/osp/hitch/hitch_1.7.3.patch
autoreconf -ivf
SSL_CFLAGS="-I$GITHUB_WORKSPACE/build-dir/include/ -I$GITHUB_WORKSPACE/build-dir/include/wolfssl" SSL_LIBS="-L$GITHUB_WORKSPACE/build-dir/lib -lwolfssl" ./configure --with-wolfssl=$GITHUB_WORKSPACE/build-dir/ --enable-silent-rules --enable-documentation --enable-warnings --with-lex --with-yacc --prefix=$GITHUB_WORKSPACE/build-dir
patch -p1 < $GITHUB_WORKSPACE/osp/hitch/hitch_${{ matrix.ref }}.patch
export SSL_CFLAGS="-I$GITHUB_WORKSPACE/build-dir/include/ -I$GITHUB_WORKSPACE/build-dir/include/wolfssl"
export SSL_LIBS="-L$GITHUB_WORKSPACE/build-dir/lib -lwolfssl"
./bootstrap --with-wolfssl=$GITHUB_WORKSPACE/build-dir/ --prefix=$GITHUB_WORKSPACE/build-dir
make -j$(nproc)

- name: Confirm hitch built with wolfSSL
Expand Down
1 change: 1 addition & 0 deletions .wolfssl_known_macro_extras
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -683,6 +683,7 @@ WC_SLHDSA_VERBOSE_DEBUG
WC_SSIZE_TYPE
WC_STRICT_SIG
WC_USE_PIE_FENCEPOSTS_FOR_FIPS
WC_USE_STATIC_ASSERT
WC_WANT_FLAG_DONT_USE_VECTOR_OPS
WIFIESPAT
WIFI_101
Expand Down
7 changes: 7 additions & 0 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9117,6 +9117,13 @@ then
ENABLED_OCSP="yes"
fi

# Requires CSR for wolfSSL_set_tlsext_status_ocsp_resp

@dgarske dgarske Jun 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔵 [Low] Ambiguous 'CSR' abbreviation in configure.ac comment

The comment # Requires CSR for wolfSSL_set_tlsext_status_ocsp_resp uses 'CSR' to mean Certificate Status Request, but in TLS/PKI code 'CSR' overwhelmingly reads as Certificate Signing Request. The change itself is correct and was verified: wolfSSL_set_tlsext_status_ocsp_resp is compile-gated behind HAVE_CERTIFICATE_STATUS_REQUEST/_V2 (src/ssl_api_crl_ocsp.c:524), the option processing at configure.ac:8047 runs before the hitch block so the manual AM_CFLAGS additions are required (exactly mirroring the established pattern at configure.ac:9473-9477), and OCSP is already force-enabled just above with its define consumed later at line 11708. Only the comment wording is worth tightening.

Fix: Spell out 'certificate status request (OCSP stapling)' in the comment to avoid confusion with Certificate Signing Request.

if test "x$ENABLED_CERTIFICATE_STATUS_REQUEST" = "xno"
then
ENABLED_CERTIFICATE_STATUS_REQUEST="yes"
AM_CFLAGS="$AM_CFLAGS -DHAVE_TLS_EXTENSIONS -DHAVE_CERTIFICATE_STATUS_REQUEST"
fi

# Requires ALPN
if test "x$ENABLED_ALPN" = "xno"
then
Expand Down
4 changes: 4 additions & 0 deletions wolfssl/wolfcrypt/types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2186,6 +2186,10 @@ WOLFSSL_API word32 CheckRunTimeSettings(void);

#define WC_CPP_CAT4_(a, b, c, d) a ## b ## c ## d
#define WC_CPP_CAT4(a, b, c, d) WC_CPP_CAT4_(a, b, c, d)
#if !defined(BUILDING_WOLFSSL) && !defined(WOLFSSL_VIS_FOR_TESTS) && \

@dgarske dgarske Jun 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟠 [Medium] wc_static_assert silently becomes a no-op for all external applications, not just hitch

The new gate defines WC_NO_STATIC_ASSERT by default whenever BUILDING_WOLFSSL and WOLFSSL_VIS_FOR_TESTS are absent, which is every downstream application build. This is a deliberate change (commit bc8c93e: avoid pulling \<assert.h> into apps like hitch) and I verified the mechanics are sound: BUILDING_WOLFSSL is confined to the library target flags (src/include.am:47-48) so it does not leak into the generated options.h; in-tree test apps define WOLFSSL_VIS_FOR_TESTS before any wolfSSL include (tests/unit.h:26, wolfcrypt/test/test.c:31), so the dedicated compile-time assert tests in tests/api/test_mldsa_legacy.c stay live; and the no-op expansion struct wc_static_assert_dummy_struct; is warning-clean even when repeated consecutively at file scope as in error-crypt.h:344-346 (verified empirically with gcc/g++ -Wall -Wextra -Wredundant-decls -Wpedantic -std=c89). However, two consequences are worth surfacing: (1) any downstream application that deliberately uses the public wc_static_assert/wc_static_assert2 macros for its own compile-time checks (the documented pattern in doc/dilithium-to-mldsa-migration.md and tests/api/test_mldsa_legacy.c) now silently loses enforcement unless it defines WC_USE_STATIC_ASSERT -- the asserts compile but check nothing, which is the worst failure mode for an assert; (2) the header-level configuration sanity asserts (wolfssl/error-ssl.h:252, wolfssl/wolfcrypt/error-crypt.h:344-346, wolfssl/wolfcrypt/sp_int.h:987-988) no longer fire in application translation units, so a user_settings.h mismatch is only caught when the…

Fix: Add an explanatory comment above the new block (the change is non-obvious and the rationale currently lives only in a commit message), and document WC_USE_STATIC_ASSERT / WC_NO_STATIC_ASSERT semantics wherever build macros are documented, so downstream users who rely on wc_static_assert know the default flipped.

@dgarske dgarske Jun 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔵 [Low] testsuite translation unit silently loses the test.h static assert

testsuite/testsuite.c includes wolfcrypt/test/test.h (which contains wc_static_assert(-(long)MIN_CODE_E < 0x7ffL); at test.h:65) but defines neither BUILDING_WOLFSSL nor WOLFSSL_VIS_FOR_TESTS, so with this PR that assert expands to the no-op dummy struct in the testsuite translation unit. This is benign -- the identical assert is still evaluated when wolfcrypt/test/test.c (which defines WOLFSSL_VIS_FOR_TESTS at line 31) is compiled in the same build -- but it illustrates that the gate also reaches in-tree consumers beyond external applications. Noting for completeness; no action strictly required.

Fix: Optional: define WOLFSSL_VIS_FOR_TESTS in testsuite.c for consistency with the other in-tree test drivers.

!defined(WC_USE_STATIC_ASSERT) && !defined(WC_NO_STATIC_ASSERT)
#define WC_NO_STATIC_ASSERT
#endif
#if defined(WC_NO_STATIC_ASSERT)
#define wc_static_assert(expr) struct wc_static_assert_dummy_struct
#define wc_static_assert2(expr, msg) wc_static_assert(expr)
Expand Down
Loading