Enhancement: Public extension manager for ECH#10568
Enhancement: Public extension manager for ECH#10568sebastian-carpenter wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
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::extensionsto 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/privateNamemachinery 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.
sebastian-carpenter
force-pushed
the
tls-ech-ext-public
branch
from
2e125d1 to
aa4831f
Compare
|
retest this please |
|
sebastian-carpenter
force-pushed
the
tls-ech-ext-public
branch
2 times, most recently
from
8c2fa10 to
61e1638
Compare
|
Jenkins retest this please. |
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
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
sebastian-carpenter
force-pushed
the
tls-ech-ext-public
branch
2 times, most recently
from
4eef82d to
b4058e9
Compare
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
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
sebastian-carpenter
force-pushed
the
tls-ech-ext-public
branch
from
b4058e9 to
a965758
Compare
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
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
left a comment
dgarske
left a comment
There was a problem hiding this comment.
See these skoll review items -> #10568 (review)
sebastian-carpenter
force-pushed
the
tls-ech-ext-public
branch
from
a965758 to
ee10978
Compare
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
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
- [Medium] ECH reconciliation block in TLSX_Parse omits the TLS1.3 version guard used by sibling SNI logic —
src/tls.c:18715-18717 - [Low] Duplicated client/server invocation block in openssl-ech.sh reject branches —
.github/scripts/openssl-ech.sh:175-197,275-307
Review generated by Skoll
| #if defined(WOLFSSL_TLS13) && defined(HAVE_ECH) | ||
| /* Reconcile ECH inner/outer extensions before verifying SNI so the verify | ||
| * pass sees the authoritative list */ | ||
| if (ret == 0 && msgType == client_hello && isRequest && |
There was a problem hiding this comment.
🟠 [Medium] ECH reconciliation block in TLSX_Parse omits the TLS1.3 version guard used by sibling SNI logic
The new ECH accept/reject reconciliation block runs whenever msgType == client_hello && isRequest && !echProcessingInner && ssl->ctx->echConfigs != NULL && !ssl->options.disableECH. Unlike the parallel logic this PR added in TLSX_SNI_Parse (line ~2517, which explicitly checks IsAtLeastTLSv1_3(ssl->version) with the comment "A TLS 1.2 hello never gets an inner pass..."), this block has no version guard. It is currently benign only because the server's TLSX_ECH is created exclusively under IsAtLeastTLSv1_3 in TLSX_PopulateExtensions (so ech resolves to NULL for a downgraded TLS 1.2 ClientHello and the block no-ops). Relying on that implicit invariant is more fragile than the explicit check used a few thousand lines earlier in the same file. Adding the guard would make the two ECH code paths consistent and self-documenting.
Fix: Add IsAtLeastTLSv1_3(ssl->version) to the guard so the reconciliation block matches the defensive version check the PR added to TLSX_SNI_Parse.
There was a problem hiding this comment.
In retrospect this guard never made sense to add anyways. Removed the two instances I previously added.
|
|
||
| grep -q "ech_success=1" "$TMP_LOG" | ||
| # test with wolfssl client | ||
| if [ "$REJECT" -ne 0 ]; then |
There was a problem hiding this comment.
🔵 [Low] Duplicated client/server invocation block in openssl-ech.sh reject branches
Both openssl_server() and openssl_client() now wrap the test invocation in an if [ "$REJECT" -ne 0 ]; then ... else ... fi where the only differences between the two branches are || true and the expected grep string. The full multi-line $WOLFSSL_CLIENT / $OPENSSL s_client command is copy-pasted in each branch, so a future flag change must be made in two places. This is CI-only scripting (not src/*.c), so it is purely a maintainability nit.
Fix: Optionally de-duplicate the invocation to a single command and branch only on the success assertion. Not blocking.
There was a problem hiding this comment.
deduplicated
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
#10568 (review)
Make sure you rebase to latest master to pull in CI optimizations.
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-securityOverall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
3 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] [review] Server now hard-rejects an accepted ECH handshake when the outer ClientHello SNI did not match a publicName —
src/tls.c:18724-18741 - [Medium] [review] New UNKNOWN_SNI_HOST_NAME_E error path is not exercised by tests —
src/tls.c:18727-18734 - [Low] [review] Duplicated install/restore swap boilerplate in TLSX_GetSizeWithEch and TLSX_WriteWithEch —
src/tls.c:16911-16935, 17076-17152 - [Info] [review-security] CTX-level private SNI suppression in ECH outer hello relies on implicit semaphore ordering and is untested —
src/tls.c:16896-16937 (TLSX_GetSizeWithEch), src/tls.c:17062-17153 (TLSX_WriteWithEch)
Review generated by Skoll
| ech = (WOLFSSL_ECH*)echX->data; | ||
|
|
||
| if (ech != NULL) { | ||
| if (ech->state == ECH_WRITE_NONE && ech->innerClientHello != NULL) { |
There was a problem hiding this comment.
🟠 [Medium] Server now hard-rejects an accepted ECH handshake when the outer ClientHello SNI did not match a publicName · Behavior Change / Interop
The reconciliation block introduces a new, stricter requirement: when the server successfully decrypts the inner ClientHello on the first flight (state == ECH_WRITE_NONE && innerClientHello != NULL and serverState < SERVER_HELLO_RETRY_REQUEST_COMPLETE), it returns UNKNOWN_SNI_HOST_NAME_E unless ech->extensions carries a TLSX_SERVER_NAME (i.e. the outer SNI matched a configured echConfig->publicName). The old EchStateSNI machinery did not gate ECH acceptance on the outer SNI matching the public name. A peer that omits the outer SNI entirely, or sends an outer SNI that does not match any publicName, will now be rejected even though the inner decrypted successfully and ECH could be accepted. wolfSSL's own client always sends the public name (set in TLSX_ECH_Use), so wolfSSL<->wolfSSL and the tests pass, but this is stricter than the ECH spec requires (the public_name is a SHOULD on the client) and may affect interop with third-party clients.
Fix: Confirm this hard-fail-on-missing-public-name is the intended policy. If interop with clients that send a different/absent outer SNI must be preserved, consider downgrading to a permissive path (accept ECH and route on the inner SNI) rather than returning UNKNOWN_SNI_HOST_NAME_E.
There was a problem hiding this comment.
This is stricter but probably for the better. A client that sends a different public name would stick-out (bad! I don't want to encourage that). The option of omitting the SNI is an interesting idea but is another potential method of fingerprinting..
If someone needs it to be more lenient in the future then an API could be added at that time.
| if (ech->state == ECH_WRITE_NONE && ech->innerClientHello != NULL) { | ||
| /* The outer ClientHello must have carried the echConfig | ||
| * publicName as its SNI */ | ||
| if (ssl->options.serverState < |
There was a problem hiding this comment.
🟠 [Medium] New UNKNOWN_SNI_HOST_NAME_E error path is not exercised by tests · Missing Tests
The PR adds a new server-side rejection path that returns UNKNOWN_SNI_HOST_NAME_E when the outer ClientHello does not carry a public-name SNI on the first flight, yet none of the added/updated tests drive a ClientHello whose outer SNI is missing or does not match a publicName while the inner still decrypts. All new ECH tests (test_wolfSSL_Tls13_ECH_wire_sni, the bad_configs_ex scenarios) rely on the wolfSSL client which always emits the public-name SNI, so this branch is never taken. Error paths introduced by a PR should be covered.
Fix: Add a test that produces an outer ClientHello with no public-name SNI (or a non-matching outer SNI) while the inner decrypts, and assert the handshake fails with UNKNOWN_SNI_HOST_NAME_E. This both documents and locks in the intended behavior of finding #1.
There was a problem hiding this comment.
UNKNOWN_SNI_HOST_NAME was actually the wrong thing to report in this case, changed it to INVALID_PARAMETER so it sends illegal_parameter (as the RFC desires: section 7.1).
Added a test: test_wolfSSL_Tls13_ECH_outer_sni_mismatch
| WC_FREE_VAR_EX(serverName, ssl->heap, DYNAMIC_TYPE_TMP_BUFFER); | ||
| if (ret == 0 && r != 0) | ||
| ret = r; | ||
| if (installed) { |
There was a problem hiding this comment.
🔵 [Low] Duplicated install/restore swap boilerplate in TLSX_GetSizeWithEch and TLSX_WriteWithEch · Code Duplication / Maintainability
Both functions now repeat the same pattern: find ECH, if (TLSX_EchShouldHideInner(ech)) { prepended = TLSX_EchSwapExtensions(...); installed = 1; } before the work, then if (installed) { prepended = TLSX_EchSwapExtensions(..., prepended); if (ret == 0 && prepended != 0) ret = BAD_STATE_E; } after. This install/restore-with-BAD_STATE_E-check boilerplate is identical in both call sites and is easy to get subtly out of sync when one is edited.
Fix: Consider small helpers (e.g. TLSX_EchInstallPublic/TLSX_EchRestorePublic) to share the install/restore-and-verify logic between the size and write paths. Optional cleanup; behavior is correct as written.
There was a problem hiding this comment.
added helpers TLSX_EchConcealExtensions, TLSX_EchExposeExtensions
- *_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 - Changed EchSwapExtensions to append instead of prepend
sebastian-carpenter
force-pushed
the
tls-ech-ext-public
branch
from
ee10978 to
b996def
Compare
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-securityOverall recommendation: COMMENT
Findings: 3 total — 3 posted, 0 skipped
3 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] [review+review-security] ECH outer ClientHello reconciliation may falsely reject valid handshakes (new public-name SNI gate) —
src/tls.c:18766-18784 - [Low] [review] Redundant TLSX_Find(TLSX_ECH) lookups in TLSX_SNI_Parse —
src/tls.c:2456-2458, 2513-2519 - [Info] [review] ForceZero used to zero-initialize a non-secret Hpke struct —
src/tls.c:13890
Review generated by Skoll
| if (echX != NULL) | ||
| ech = (WOLFSSL_ECH*)echX->data; | ||
|
|
||
| if (ech != NULL) { |
There was a problem hiding this comment.
🟠 [Medium] ECH outer ClientHello reconciliation may falsely reject valid handshakes (new public-name SNI gate) · Logic
Both the review and review-security modes flagged the new outer-CH reconciliation gate at the end of TLSX_Parse, which returns INVALID_PARAMETER when ECH is accepted (ech->state == ECH_WRITE_NONE && ech->innerClientHello != NULL, serverState < SERVER_HELLO_RETRY_REQUEST_COMPLETE) but no TLSX_SERVER_NAME was recorded on ech->extensions. The review mode identifies a concrete bug (Medium/SUGGEST): when the server itself configures wolfSSL_UseSNI() with a host name equal to the ECH public name, TLSX_SNI_Parse sets matched=1 first and skips the publicName block (workingConfig stays NULL), so the SNI is routed to ssl->extensions instead of ech->extensions; the reconciliation then finds no SNI on ech->extensions and aborts an otherwise valid ECH-accepted handshake. This combination is plausible because the public name is typically a real fronting host with a matching cert. The review-security mode rates the same gate Info (an intentional, tested tightening per RFC 9849 via test_wolfSSL_Tls13_ECH_outer_sni_mismatch) and notes there is no security/DoS impact, since only the offending client's own handshake fails and it is not attacker-amplifiable; it flags that spec-compliant peers omitting the outer SNI or using an empty public_name will now be rejected. The two severity views differ (Medium bug vs Info awareness); the stricter Medium is retained.
Fix: Relax the reconciliation check to also accept a public-name SNI recorded on ssl->extensions, or make TLSX_SNI_Parse always route a publicName-matching SNI to ech->extensions even when a server-configured SNI already matched (run the publicName check even when matched is true). Confirm the stricter outer-SNI requirement is the intended policy for all supported ECH peers (including those that omit the outer SNI or use an empty public_name); if broader interop leniency is later desired, skip the hard abort when no server-side SNI policy is configured. Add a regression test for a server that configures wolfSSL_UseSNI() with a host name equal to the ECH public name and accepts ECH.
| /* No server SNI configured: when ECH is active the outer SNI still | ||
| * needs to be parsed so it can be matched against the echConfig | ||
| * publicName and recorded on ech->extensions. */ | ||
| if (!ssl->options.disableECH && !ssl->options.echProcessingInner && |
There was a problem hiding this comment.
🔵 [Low] Redundant TLSX_Find(TLSX_ECH) lookups in TLSX_SNI_Parse · Style
The ECH extension is looked up twice on the same ssl->extensions list within one call: once to set checkPublic (TLSX_Find(ssl->extensions, TLSX_ECH) != NULL) and again later to populate echX/ech for the publicName comparison. The earlier lookup discards the result. Caching the echX/ech pointer once would avoid the duplicate list walk and make the two ECH-dependent branches share state.
Fix: Resolve echX/ech once near the top of the server block and reuse it for both the checkPublic decision and the publicName comparison. Optional cleanup; not a correctness issue.
| XFREE(ech, heap, DYNAMIC_TYPE_TMP_BUFFER); | ||
| return MEMORY_E; | ||
| } | ||
| ForceZero(ech->hpke, sizeof(Hpke)); |
There was a problem hiding this comment.
⚪ [Info] ForceZero used to zero-initialize a non-secret Hpke struct · Convention
ForceZero(ech->hpke, sizeof(Hpke)) is added to zero the freshly-XMALLOC'd Hpke struct before wc_HpkeInit. ForceZero is intended for wiping secrets so the compiler cannot elide it; using it for plain initialization is unusual (XMEMSET is the conventional init). This is harmless and consistent with the surrounding ForceZero(ech, sizeof(WOLFSSL_ECH)), so it is cosmetic. The unconditional wc_HpkeFreeKey() call (NULL ephemeralKey) introduced alongside it is safe because wc_ecc_key_free/wc_curve25519_free both null-check.
Fix: Use XMEMSET(ech->hpke, 0, sizeof(Hpke)) for initialization, reserving ForceZero for secret wiping. Take it or leave it; no functional impact.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
tests/api.c:1
- This helper will underflow
storage--when called withcount == 0, causing an out-of-bounds write tostorage->next. Even if current call sites always passcount > 0, it’s brittle for future edits. Add an early return whencount <= 0.
| void TLSX_EchReplaceExtensions(WOLFSSL* ssl, byte accepted) | ||
| { | ||
| int ret = 0; | ||
| TLSX* echX; | ||
| WOLFSSL_ECH* ech; |
| void TLSX_SetResponse(WOLFSSL* ssl, TLSX_Type type); | ||
| /** Mark an extension to be sent back to the client. */ | ||
| void TLSX_SetResponse(WOLFSSL* ssl, TLSX_Type type) | ||
| void TLSX_SetResponseInList(TLSX* list, TLSX_Type type); |
| void TLSX_SetResponseInList(TLSX* list, TLSX_Type type) | ||
| { | ||
| TLSX *extension = TLSX_Find(ssl->extensions, type); | ||
| TLSX *extension = TLSX_Find(list, type); |
| if (ech->hpke != NULL) { | ||
| if (ech->ephemeralKey != NULL) | ||
| wc_HpkeFreeKey(ech->hpke, ech->hpke->kem, ech->ephemeralKey, | ||
| ech->hpke->heap); | ||
| wc_HpkeFreeKey(ech->hpke, ech->hpke->kem, ech->ephemeralKey, | ||
| ech->hpke->heap); |
|
This PR has many changes too late in release process. There are a few minor good things that @sebastian-carpenter is going to pull out into a smaller PR to see if it can go into the release. |
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
TLSX* extensionstoWOLFSSL_ECHholding the public extensions (e.g. the public-name SNI), populated inTLSX_ECH_UsefromechConfig->publicName.TLSX_EchChangeSNI/TLSX_EchRestoreSNIwith:TLSX_EchShouldHideInner- true when ECH is the outer type.TLSX_EchSwapExtensions- swaps matching extension types betweenssl->extensionsandech->extensions; unmatched public exts are prepended;popCountreverses leading nodes to preserve OuterExtensions ordering.TLSX_EchReplaceExtensions- accept: free the public list; reject: swap public exts intossl->extensions.TLSX_GetSizeWithEch/TLSX_WriteWithEchinstall the public exts before size/write and swap back after.ForceZerothe hpke; freeech->extensionsinTLSX_ECH_Free.SNI changes
EchStateSNIenum and thesniState/privateNamefields, plus the sniState transitions inDoTls13HandShakeMsgType.TLSX_SNI_Parse: drop the sniState-based private-name save/restore. On outer-CH parse, match against anyechConfig->publicNameand write a matched public SNI toech->extensions, notssl->extensions; simplify thematched/cacheOnlyand response-flag logic.TLSX_EchChangeSNI.SendTls13ClientHello: read padding length viaTLSX_SNI_GetRequestinstead ofech->privateName.TLSX_EchReplaceExtensionsat the accept/reject points (EchCheckAcceptance,DoTls13ServerHelloreject fallback,DoTls13ClientHello). This installs the correct SNI for use later in the handshake.Padding
Testing
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.test_wolfSSL_Tls13_ECH_no_private_name: no-inner-SNI now succeeds against a permissive server (ACCEPTED); rejected only withABORT_ON_ABSENCE.b64MandatoryFirstconfig case;test_ech_server_sni_callbackrejects a wrong public name; add a double-public-SNI scenario tobad_configs_ex.test_wolfSSL_Tls13_ECH_long_SNI(its overflow target - the deleted swap helpers - no longer exist) and relocate the over-long-SNIBAD_LENGTH_Echeck intotest_wolfSSL_UseSNI_params.TLSX_EchSwapExtensionsfunction: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 withWOLFSSL_TEST_VIS.Added testing from #10542:
Follow-ups
Checklist