Enhancement: Public extension manager for ECH by sebastian-carpenter · Pull Request #10568 · wolfSSL/wolfssl

sebastian-carpenter · 2026-06-01T20:48:43Z

Description

Reworks ECH public-vs-private extension handling. The old path swapped a single SNI string in place on the live extension list - this was fragile and SNI-only. This PR generalizes it to a public-extension list on the ECH struct that's swapped by extension type, so the outer ClientHello can carry arbitrary public extensions.

ZD#21931

Public extension manager

Add TLSX* extensions to WOLFSSL_ECH holding the public extensions (e.g. the public-name SNI), populated in TLSX_ECH_Use from echConfig->publicName.
Replace TLSX_EchChangeSNI/TLSX_EchRestoreSNI with:
- TLSX_EchShouldHideInner - true when ECH is the outer type.
- TLSX_EchSwapExtensions - swaps matching extension types between ssl->extensions and ech->extensions; unmatched public exts are prepended; popCount reverses leading nodes to preserve OuterExtensions ordering.
- TLSX_EchReplaceExtensions - accept: free the public list; reject: swap public exts into ssl->extensions.
TLSX_GetSizeWithEch/TLSX_WriteWithEch install the public exts before size/write and swap back after.
ForceZero the hpke; free ech->extensions in TLSX_ECH_Free.

SNI changes

Remove the old SNI-state machinery: drop the EchStateSNI enum and the sniState/privateName fields, plus the sniState transitions in DoTls13HandShakeMsgType.
TLSX_SNI_Parse: drop the sniState-based private-name save/restore. On outer-CH parse, match against any echConfig->publicName and write a matched public SNI to ech->extensions, not ssl->extensions; simplify the matched/cacheOnly and response-flag logic.
Inner SNI is now optional - removed the client-side "inner SNI must be present" requirement enforced by TLSX_EchChangeSNI.
SendTls13ClientHello: read padding length via TLSX_SNI_GetRequest instead of ech->privateName.
Call TLSX_EchReplaceExtensions at the accept/reject points (EchCheckAcceptance, DoTls13ServerHello reject fallback, DoTls13ClientHello). This installs the correct SNI for use later in the handshake.

Padding

If the client ssl had no SNI then it would take the generic no SNI path. Updated padding calculation to fallback to ctx's SNI if it is available.

Testing

Add test_wolfSSL_Tls13_ECH_wire_sni: drive the handshake manually and assert the public name is present and the private name absent in raw CH1/CH2 bytes, across accept/reject.
Update test_wolfSSL_Tls13_ECH_no_private_name: no-inner-SNI now succeeds against a permissive server (ACCEPTED); rejected only with ABORT_ON_ABSENCE.
Add b64MandatoryFirst config case; test_ech_server_sni_callback rejects a wrong public name; add a double-public-SNI scenario to bad_configs_ex.
Remove obsolete test_wolfSSL_Tls13_ECH_long_SNI (its overflow target - the deleted swap helpers - no longer exist) and relocate the over-long-SNI BAD_LENGTH_E check into test_wolfSSL_UseSNI_params.
Testing for the new TLSX_EchSwapExtensions function: test_TLSX_EchSwapExtensions. This emulates the swap between ssl->extensions and ech->extensions with various configs: 'match', 'no match', 'match and non-match', 'match and no-match + out of order'. This required qualifying the function with WOLFSSL_TEST_VIS.

Added testing from #10542:

More efficient *_wire_sni test
Interop testing for when ECH is rejected

Follow-ups

Lays groundwork for managing multiple public extensions and multiple SNI's in the outer ClientHello.

Checklist

added tests
updated/added doxygen
updated appropriate READMEs
Updated manual and documentation

Copilot

Pull request overview

This PR refactors TLS 1.3 ECH public-vs-private extension handling by introducing a dedicated “public extensions” list on the WOLFSSL_ECH object and swapping extensions by type, replacing the previous SNI-only in-place string swap approach. It also updates SNI/ECH parsing and handshake flow to install the correct extensions on accept/reject paths, and adds/updates tests to validate wire visibility of public vs private SNI.

Changes:

Add WOLFSSL_ECH::extensions to hold “public” extensions and implement swapping/replacement helpers used during ClientHello size/write and accept/reject handling.
Rework SNI parsing and ECH handshake transitions to remove the old sniState/privateName machinery and support public-name matching for outer ClientHello.
Update API tests to cover new behaviors (wire-SNI visibility, no-inner-SNI behavior, new bad config cases) and relocate long-SNI length validation coverage.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
`wolfssl/internal.h`	Removes old SNI-state fields and adds `WOLFSSL_ECH::extensions` plus `TLSX_EchReplaceExtensions()` prototype.
`src/tls.c`	Implements public-extension swapping/replacement, updates SNI parsing, and updates ECH lifecycle management.
`src/tls13.c`	Adjusts ClientHello padding derivation and installs correct extension sets at ECH accept/reject points (incl. HRR).
`tests/api.c`	Adds/updates ECH/SNI tests (wire visibility, no-private-name behavior, new config cases) and relocates length checks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

github-actions · 2026-06-08T18:34:07Z

retest this please

github-actions · 2026-06-08T22:43:37Z

MemBrowse Memory Report

gcc-arm-cortex-m4-openssl-compat

FLASH: .text +64 B (+0.0%, 767,540 B / 1,048,576 B, total: 73% used)

gcc-arm-cortex-m7-tls13

FLASH: .text +64 B (+0.0%, 234,754 B / 262,144 B, total: 90% used)

linuxkm-standard

Data: __patchable_function_entries +16 B (+0.0%, 46,000 B)
No memory changes detected for:
gcc-arm-cortex-m0plus
gcc-arm-cortex-m3
gcc-arm-cortex-m4
gcc-arm-cortex-m4-baremetal
gcc-arm-cortex-m4-crypto-only
gcc-arm-cortex-m4-dtls13
gcc-arm-cortex-m4-min-ecc
gcc-arm-cortex-m4-pkcs7
gcc-arm-cortex-m4-pq
gcc-arm-cortex-m4-rsa-only
gcc-arm-cortex-m4-sp-math
gcc-arm-cortex-m4-tls12
gcc-arm-cortex-m4-tls13
gcc-arm-cortex-m7
gcc-arm-cortex-m7-pq
linuxkm-pie
stm32-sim-stm32h753

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

sebastian-carpenter · 2026-06-09T18:56:35Z

Jenkins retest this please.

dgarske

Skoll Code Review

Scan type: reviewOverall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)

Posted findings

[Medium] Multi-public-extension swap/reversal path is untested — src/tls.c:16682-16734
[Low] ECH padding length only considers ssl-extensions, not ctx-level inner SNI — src/tls13.c:4844-4855
[Info] Pointer declaration style inconsistent with repo convention — src/tls.c:2378
[Info] TLSX_EchShouldHideInner can be a single boolean expression — src/tls.c:16665-16671

Review generated by Skoll

dgarske

Skoll Code Review

Scan type: reviewOverall recommendation: COMMENT
Findings: 2 total — 2 posted, 0 skipped
2 finding(s) posted as inline comments (see file-level comments below)

Posted findings

[Low] TLSX_FreeAll echList tracks only a single ECH extension per list — src/tls.c:15190-15216
[Low] BAD_STATE_E on restore-swap leaves ssl-extensions inconsistent without repair — src/tls.c:16895-16901, 17111-17117

Review generated by Skoll

dgarske

Skoll Code Review

Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 6 total — 6 posted, 0 skipped
5 finding(s) posted as inline comments (see file-level comments below)

Posted findings

[High] Servers with WOLFSSL_SNI_ABORT_ON_ABSENCE now abort every ECH handshake at the outer ClientHello, even when a valid inner SNI is present — src/tls.c:2520-2557
[Medium] test_wolfSSL_Tls13_ECH_no_private_name ABORT_ON_ABSENCE case encodes the outer-parse abort; missing test for ABORT_ON_ABSENCE with inner SNI present — tests/api.c:14625-14639
[Low] BAD_STATE_E restore-failure paths log with WOLFSSL_MSG only, no WOLFSSL_ERROR_VERBOSE — src/tls.c:16900-16907, src/tls.c:17117-17124
[Low] Inline reimplementation of TLSX_SetResponse in TLSX_SNI_Parse — src/tls.c:2550-2557
[Low] EchCheckAcceptance calls TLSX_EchReplaceExtensions even when EchCalcAcceptance failed — src/tls13.c:5310-5314
[Info] PUB_NAME changed to example.com for all interop scenarios, not just the new reject tests — .github/scripts/openssl-ech.sh:97-99

Review generated by Skoll

dgarske · 2026-06-11T19:04:06Z

+    if (!matched)
+        matched = cacheOnly;
+
    if (matched ||


🔴 [High] Servers with WOLFSSL_SNI_ABORT_ON_ABSENCE now abort every ECH handshake at the outer ClientHello, even when a valid…

The PR redirects a publicName-matched outer SNI into ech->extensions (writeList = &ech->extensions when workingConfig != NULL), and TLSX_SNI_SetStatus(*writeList, ...) records WOLFSSL_SNI_REAL_MATCH only on that private copy. The configured SNI node in ssl->extensions/ctx->extensions is left at WOLFSSL_SNI_NO_MATCH. TLSX_SNI_VerifyParse (called unconditionally at the end of every TLSX_Parse, src/tls.c:18685) only consults ssl->extensions/ctx->extensions, so when the server has WOLFSSL_SNI_ABORT_ON_ABSENCE set it returns SNI_ABSENT_ERROR while parsing the OUTER ClientHello -- before the inner hello (which may carry a perfectly valid private SNI) is ever processed. In the old code the publicName match was written into ssl->extensions via TLSX_UseSNI(&ssl->extensions, ...) with REAL_MATCH, so the outer VerifyParse passed and the inner SNI was then evaluated. EMPIRICALLY VERIFIED on this branch (--enable-ech build): adding wolfSSL_UseSNI(c_ssl, WOLFSSL_SNI_HOST_NAME, echCbTestPrivateName, ...) to the ABORT_ON_ABSENCE case of test_wolfSSL_Tls13_ECH_no_private_name still fails the handshake with error = -402, No Server Name Indication extension Error, i.e. the abort fires on CH1's outer parse regardless of the inner SNI. This affects both the accept path (server can never accept ECH when ABORT_ON_ABSENCE is set) and the reject path (server aborts instead of continuing the outer handshake with retry configs).

Fix: Make the outer-CH ABORT_ON_ABSENCE check aware of the public-SNI match now stored in ech->extensions (or defer SNI_VERIFY_PARSE enforcement to the inner CH when ECH decryption succeeded, or mirror the match status onto the ssl-level SNI for the outer pass). Add a regression test: server with ABORT_ON_ABSENCE + ECH, client with private inner SNI -> handshake must succeed with ECH ACCEPTED.

Addressed. Moved the block that replaces the ech extensions to happen before the SNI_VERIFY.

Added WOLFSSL_SNI_ABORT_ON_ABSENCE to the wire_sni test which is expected to pass, thus testing the desired accept case. Correct connections should no longer fail.

dgarske · 2026-06-11T19:04:06Z


    ExpectIntEQ(test_ssl_memio_setup(&test_ctx), TEST_SUCCESS);

+    /* SNI is permissive by default, force a failure when SNI is absent */


🟠 [Medium] test_wolfSSL_Tls13_ECH_no_private_name ABORT_ON_ABSENCE case encodes the outer-parse abort; missing test for…

The comment says 'SNI is permissive by default, force a failure when SNI is absent', implying the failure is caused by the missing inner SNI. In reality the SNI_ABSENT_ERROR triggers during the OUTER ClientHello parse (the inner hello is never consulted), so the same failure occurs even when an inner SNI is present and valid (verified empirically). The test as written passes for the wrong reason and locks in the regressed behavior described in the BLOCK finding. There is no test covering the combination ABORT_ON_ABSENCE + inner SNI present, which is exactly the case that regressed.

Fix: After fixing the BLOCK finding, add the ABORT_ON_ABSENCE + inner-SNI-present scenario asserting handshake success and ECH ACCEPTED on both sides, and fix the misleading comment on the absent-SNI case.

added sni option to wire_sni test.

dgarske · 2026-06-11T19:04:06Z

-        TLSX_SNI_SetStatus(ssl->extensions, type, (byte)matchStat);
+        TLSX_SNI_SetStatus(*writeList, type, (byte)matchStat);
+
+        if (!cacheOnly) {


🔵 [Low] Inline reimplementation of TLSX_SetResponse in TLSX_SNI_Parse

The code comments 'This is just a reimplementation of TLSX_SetResponse to accept a TLSX* instead' and then open-codes the find-and-set. A small list-based helper (e.g. TLSX_SetResponseInList(TLSX* list, TLSX_Type type)) that TLSX_SetResponse also calls would remove the duplication and keep response-flag semantics in one place for future public-extension types (the PR's stated follow-up direction).

Fix: Extract a shared helper; not blocking.

wrote the given helper function, removed inline code and replaced.

dgarske · 2026-06-11T19:04:06Z

        ssl->hsHashes = tmpHashes;
    }

+    /* Skip only when the HRR signals ECH acceptance


🔵 [Low] EchCheckAcceptance calls TLSX_EchReplaceExtensions even when EchCalcAcceptance failed

The new replace call at the end of EchCheckAcceptance runs even when EchCalcAcceptance returned an error (the if (ret == 0) block above was skipped, echAccepted may be stale). It is harmless because the handshake aborts on the non-zero return, but swapping the public extensions into ssl->extensions on a hard-error path is unnecessary state mutation and makes the function's failure behavior harder to reason about.

Fix: Gate the replace on ret == 0 for clarity; behavior is unchanged either way since the connection aborts on error.

added the ret == 0 guard

dgarske · 2026-06-11T19:04:06Z

 TMP_LOG="$WORKSPACE/tmp_file.log"
 PRIV_NAME="ech-private-name.com"
-PUB_NAME="ech-public-name.com"
+PUB_NAME="example.com"


⚪ [Info] PUB_NAME changed to example.com for all interop scenarios, not just the new reject tests

PUB_NAME was changed from "ech-public-name.com" to "example.com" so it matches the public name embedded in the hard-coded REJECT_ECH_CONFIG. This silently changes the public name used by every pre-existing interop scenario (suite tests, HRR, PQC), slightly reducing name diversity in the non-reject coverage. If intentional, a brief comment tying PUB_NAME to REJECT_ECH_CONFIG would help; otherwise the reject path could use its own name variable. The set -e at the top of the script does make the chained grep -q assertions effective, so the new reject verification logic is sound.

Fix: Add a comment documenting the coupling between PUB_NAME and REJECT_ECH_CONFIG, or scope the example.com name to the reject paths.

documented. Also explained why PRIV_NAME is what it is

dgarske

See these skoll review items -> #10568 (review)

- *_wire_sni test is now more efficient - openssl-ech workflow now does interop with ECH rejection extra improvements: - tested TLSX_EchSwapExtensions - added ctx level SNI to padding calculation - Improvement of SNI handling for ECH

sebastian-carpenter self-assigned this Jun 1, 2026

Copilot AI review requested due to automatic review settings June 1, 2026 20:48

Copilot started reviewing on behalf of sebastian-carpenter June 1, 2026 20:48 View session

Copilot AI reviewed Jun 1, 2026

View reviewed changes

Comment thread src/tls.c

Comment thread src/tls.c

Comment thread tests/api.c Outdated

sebastian-carpenter mentioned this pull request Jun 8, 2026

CI Addition (ECH): Check only the public SNI is visible #10542

Closed

4 tasks

sebastian-carpenter force-pushed the tls-ech-ext-public branch from 2e125d1 to aa4831f Compare June 8, 2026 16:44

sebastian-carpenter marked this pull request as ready for review June 8, 2026 18:30

sebastian-carpenter force-pushed the tls-ech-ext-public branch 2 times, most recently from 8c2fa10 to 61e1638 Compare June 9, 2026 16:25

dgarske assigned wolfSSL-Bot Jun 9, 2026

dgarske requested a review from Copilot June 9, 2026 17:48

Copilot started reviewing on behalf of dgarske June 9, 2026 17:48 View session

Copilot AI reviewed Jun 9, 2026

View reviewed changes

Comment thread src/tls.c

Comment thread src/tls13.c Outdated

dgarske reviewed Jun 9, 2026

View reviewed changes

Comment thread src/tls.c Outdated

Comment thread src/tls13.c

Comment thread src/tls.c Outdated

Comment thread src/tls.c

sebastian-carpenter added 4 commits June 9, 2026 22:43

ech public extension management

bc2fb23

add public sni handling to public ext

96cb4d1

minor fixes

bc7768c

move long_sni regression test

31ddc5b

sebastian-carpenter force-pushed the tls-ech-ext-public branch 2 times, most recently from 4eef82d to b4058e9 Compare June 10, 2026 16:52

dgarske reviewed Jun 10, 2026

View reviewed changes

Comment thread src/tls.c

Comment thread src/tls.c

sebastian-carpenter force-pushed the tls-ech-ext-public branch from b4058e9 to a965758 Compare June 10, 2026 18:31

sebastian-carpenter removed their assignment Jun 11, 2026

sebastian-carpenter added the For This Release Release version 5.9.2 label Jun 11, 2026

sebastian-carpenter self-assigned this Jun 11, 2026

dgarske reviewed Jun 11, 2026

View reviewed changes

dgarske unassigned sebastian-carpenter Jun 11, 2026

dgarske assigned sebastian-carpenter Jun 11, 2026

dgarske requested changes Jun 11, 2026

View reviewed changes

testing improvements (from wolfSSL#10542):

ee10978

- *_wire_sni test is now more efficient - openssl-ech workflow now does interop with ECH rejection extra improvements: - tested TLSX_EchSwapExtensions - added ctx level SNI to padding calculation - Improvement of SNI handling for ECH

sebastian-carpenter force-pushed the tls-ech-ext-public branch from a965758 to ee10978 Compare June 12, 2026 05:01

sebastian-carpenter removed their assignment Jun 12, 2026


		ExpectIntEQ(test_ssl_memio_setup(&test_ctx), TEST_SUCCESS);

		/* SNI is permissive by default, force a failure when SNI is absent */

Conversation

sebastian-carpenter commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Public extension manager

SNI changes

Padding

Testing

Follow-ups

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jun 8, 2026

Uh oh!

github-actions Bot commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

MemBrowse Memory Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

sebastian-carpenter commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dgarske left a comment

Choose a reason for hiding this comment

Skoll Code Review

Posted findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dgarske left a comment

Choose a reason for hiding this comment

Skoll Code Review

Posted findings

Uh oh!

Uh oh!

Uh oh!

dgarske left a comment

Choose a reason for hiding this comment

Skoll Code Review

Posted findings

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgarske left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

sebastian-carpenter commented Jun 1, 2026 •

edited

Loading

github-actions Bot commented Jun 8, 2026 •

edited

Loading

sebastian-carpenter commented Jun 9, 2026 •

edited

Loading