Skip to content

rework ECH examples and add more - #601

Open
sebastian-carpenter wants to merge 2 commits into
wolfSSL:masterfrom
sebastian-carpenter:tls-ech-examples
Open

rework ECH examples and add more#601
sebastian-carpenter wants to merge 2 commits into
wolfSSL:masterfrom
sebastian-carpenter:tls-ech-examples

Conversation

@sebastian-carpenter

@sebastian-carpenter sebastian-carpenter commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

ECH

  1. The GREASE ECH example was an incorrect usage of ECH, removed it.

  2. Deprecated the server-ech-local example but did keep it since it is nice and simple.

  3. Updated the other examples to be more robust and to use features added recently. Improved comments throughout.

  4. Added examples for multi-tenant ECH: server-ech-multi-sni, server-ech-multi-ctx

  • Added cert/keys for these examples and updated some that were old.

Not-ECH

Added examples for multi-tenant TLS: server-tls-sni-callback, server-tls-ctx-swap

  • Just stripped down the added ECH examples
  • These examples also send the tenant name to the client

Docs

Updated README.md to reflect current state of examples

Dependecies

Depends on: wolfSSL/wolfssl#10913

@sebastian-carpenter sebastian-carpenter self-assigned this Jul 15, 2026
Copilot AI review requested due to automatic review settings July 15, 2026 18:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR modernizes and expands the TLS Encrypted Client Hello (ECH) examples in tls/, replacing the prior GREASE-based flow with clearer “local” and multi-tenant server patterns and updating supporting documentation/cert material.

Changes:

  • Added multi-tenant ECH server examples (server-ech-multi-sni, server-ech-multi-ctx) demonstrating inner-SNI dispatch with either per-connection cert install or mid-handshake CTX swap.
  • Updated local/client ECH examples and tls/README.md to reflect the new recommended workflows and to document ECH rejection/retry-config behavior.
  • Removed the deprecated/incorrect GREASE ECH probe example and refreshed various cert artifacts used by the examples.

Reviewed changes

Copilot reviewed 15 out of 21 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
tls/server-ech-multi-sni.c New multi-tenant ECH server selecting tenant cert/key via SNI callback.
tls/server-ech-multi-ctx.c New multi-tenant ECH server swapping to a tenant WOLFSSL_CTX mid-handshake (OpenSSL extra APIs).
tls/server-ech-local.c Expanded comments and behavior around ECH status and minimal local ECH flow.
tls/README.md Reworked ECH section to document Cloudflare flow, local flow, and multi-tenant servers.
tls/client-ech.c Changed Cloudflare ECH client to take base64 ECH config arg and print retry configs on failure.
tls/client-ech-local.c Updated local client to take base64 config + target SNI, perform domain verification, and print retry configs on failure.
tls/client-ech-grease.c Removed GREASE probe example.
certs/tenant-b-cert.pem Added tenant cert used by multi-tenant ECH servers.
certs/tenant-a-cert.pem Added tenant cert used by multi-tenant ECH servers.
certs/server-revoked-cert.pem Refreshed certificate contents/metadata.
certs/server-ecc-rsa.pem Refreshed certificate contents/metadata.
certs/server-cert.pem Refreshed certificate contents/metadata.
certs/ech-public-key.pem Added key material for the ECH public name/cert pairing.
certs/ech-public-cert.pem Added cert material for the ECH public name.
certs/ca-cert.pem Refreshed CA certificate contents/metadata.

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

Comment thread tls/server-ech-multi-sni.c
Comment thread tls/server-ech-multi-sni.c Outdated
Comment thread tls/server-ech-multi-sni.c Outdated
Comment thread tls/server-ech-multi-ctx.c
Comment thread tls/server-ech-multi-ctx.c
Comment thread tls/client-ech.c
Comment thread tls/client-ech.c Outdated
Comment thread tls/client-ech-local.c
Comment thread tls/client-ech-local.c Outdated
Comment thread certs/ech-public-key.pem

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 21 changed files in this pull request and generated 12 comments.

Comment thread tls/server-ech-multi-sni.c
Comment thread tls/server-ech-multi-ctx.c
Comment thread tls/server-ech-local.c
Comment thread tls/server-ech-multi-sni.c
Comment thread tls/server-ech-multi-ctx.c
Comment thread tls/client-ech-local.c
Comment thread certs/ech-public-key.pem
Comment thread tls/client-ech.c
Comment thread tls/server-ech-local.c Outdated
Comment thread tls/client-ech-local.c Outdated

@dgarske dgarske left a comment

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.

Skoll Code Review

Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 12 total — 12 posted, 0 skipped
11 finding(s) posted as inline comments (see file-level comments below)
1 finding(s) not tied to a diff line (full detail below)

Posted findings

  • [Medium] Regenerated CA orphans the shipped CRLs (issuer DN no longer matches)certs/ca-cert.pem:1-10
  • [Medium] certs/ech-public-key.pem is a PRIVATE key with a name that reads as "public key"certs/ech-public-key.pem:1
  • [Medium] Both "tenants" share one RSA key pair, weakening the multi-tenant demonstrationtls/server-ech-multi-sni.c:62-67
  • [Medium] SNI callback performs blocking disk I/O on every handshaketls/server-ech-multi-sni.c:139-142
  • [Medium] #define HAVE_SIGNAL immediately before #ifdef HAVE_SIGNAL defeats its own guardtls/server-ech-multi-sni.c:42-45
  • [Medium] client-ech.c hardcodes a Cloudflare anycast IP that this PR already had to updatetls/client-ech.c:98
  • [Medium] No automated coverage for the four new example programstls/server-ech-multi-sni.c:1
  • [Low] ctx-swap examples omit the documented wolfSSL_set_SSL_CTX caveat about pinned SSL-level settingstls/server-tls-ctx-swap.c:22-33
  • [Low] selectedTenant is a mutable file-scope global rather than per-connection statetls/server-tls-sni-callback.c:97
  • [Low] Dropped newline in "Client: %s" makes server output run togethertls/server-ech-local.c:251
  • [Low] ECH clients report "ECH failed" for handshakes where ECH was never in playtls/client-ech-local.c:236-245

Findings not tied to a diff line

server-ech-multi-ctx: CTX swap drops ECH state, silently breaking ECH acceptance

File: tls/server-ech-multi-ctx.c:146,253-277
Function: sni_select_tenant / main
Severity: High

The per-tenant WOLFSSL_CTX objects built in main() only get a cert and key -- they never get an ECH config. The SNI callback then swaps the live connection onto one of them with wolfSSL_set_SSL_CTX(ssl, tenants->ctx).

In wolfSSL 5.9.2 (verified across three independent 5.9.2 source trees, all identical here) DoTls13ClientHello() in src/tls13.c runs SNI_Callback(ssl) at line 7886 and then gates the remaining ECH work on ssl->ctx->echConfigs:

  • line 7958: if (echX != NULL && ssl->ctx->echConfigs != NULL && ... innerClientHello != NULL) -> EchHashHelloInner() (hashes ClientHelloInner into hsHashesEch) and innerCount = 1
  • line 8440 (SendTls13ServerHello): if (ssl->ctx->echConfigs != NULL && !ssl->options.disableECH) -> EchWriteAcceptance(), which writes the ECH accept confirmation into the last 8 bytes of the ServerHello random

Because the callback has already replaced ssl->ctx with a tenant CTX whose echConfigs == NULL, both blocks are skipped. The server decrypts the inner ClientHello and serves the correct tenant certificate, but never signals ECH acceptance, so the client's wolfSSL_GetEchStatus() reports rejection/failure (the example client then prints Client: ECH failed), and the inner-hello transcript hash is not maintained. EchWriteAcceptance()/EchHashHelloInner() themselves only touch ssl-local state, so the failure is purely from the ssl->ctx->echConfigs gate.

Note server-ech-multi-sni.c is unaffected -- it installs cert/key on the WOLFSSL object and never swaps the CTX.

Recommendation: Attach an ECH config to each tenant CTX (or otherwise keep ssl->ctx->echConfigs non-NULL after the swap) and re-run the example end to end against the dependent wolfSSL PR, confirming the client reports Client: ECH successful and the server reports Client connected with ECH. If ECH acceptance cannot be preserved across a CTX swap, document that limitation prominently in the file header and README rather than presenting it as an equivalent alternative to server-ech-multi-sni.

Referenced code: tls/server-ech-multi-ctx.c:146,253-277 (10 lines)


Review generated by Skoll

Comment thread certs/ca-cert.pem
Not After : Sep 14 21:25:29 2027 GMT
Subject: C = US, ST = Montana, L = Bozeman, O = Sawtooth, OU = Consulting, CN = www.wolfssl.com, emailAddress = info@wolfssl.com
Not Before: Jun 11 21:44:28 2026 GMT
Not After : Mar 7 21:44:28 2029 GMT

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] Regenerated CA orphans the shipped CRLs (issuer DN no longer matches)

The CA was regenerated with a changed subject DN (emailAddress went from info@wolfssl.com to facts@wolfssl.com) and a new serial. certs/crl.pem and certs/crl/crl.pem were not regenerated and still carry the old issuer DN:

$ openssl crl -in certs/crl/crl.pem -noout -issuer
issuer=... CN = www.wolfssl.com, emailAddress = info@wolfssl.com
$ openssl x509 -in certs/ca-cert.pem -noout -subject
subject=... CN = www.wolfssl.com, emailAddress = facts@wolfssl.com

certmanager/certverify.c loads certs/ca-cert.pem as the CA and then certs/crl/crl.pem via wolfSSL_CertManagerLoadCRLBuffer(). Signer lookup is by issuer-DN hash, so with the DNs diverged the CRL can no longer resolve a signer (ASN_CRL_NO_SIGNER_E) when built with --enable-crl. (I could not run this locally -- the installed wolfSSL lacks HAVE_CRL -- but the DN mismatch is deterministic. Note the CRL is also already past nextUpdate=Sep 11 2025, so this compounds pre-existing staleness rather than being the sole cause.)

I verified the rest of the regeneration is self-consistent: ca-cert.der/ca-cert.pem share serial 6719D2A8..., server-cert.pem/.der/server-key.pem still pair, and server-cert.pem, tenant-a/b-cert.pem, and ech-public-cert.pem all openssl verify OK against the new CA.

Fix: Either regenerate the CRLs alongside the CA, or revert the CA/server cert regeneration if it is not actually required by this PR -- the ECH work only needs the new ech-public-* and tenant-* certs, which could have been signed by the existing CA. Also double-check utasker/wolfSSLServerTask.h and embedded/certs.h, which embed byte-array copies of the old server-cert.der/ca-cert.der.

Comment thread certs/ech-public-key.pem
@@ -0,0 +1,28 @@
-----BEGIN PRIVATE KEY-----

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] certs/ech-public-key.pem is a PRIVATE key with a name that reads as "public key"

The new file certs/ech-public-key.pem begins with -----BEGIN PRIVATE KEY----- and is loaded via wolfSSL_CTX_use_PrivateKey_file(). The name is meant as "the key for the ECH public name", but this repo already uses the *keyPub.pem / *-keyPub.der suffix for genuine public keys (client-keyPub.pem, ecc-keyPub.pem), so ech-public-key.pem reads as a public key. A file named *-public-key.pem is exactly the kind of artifact someone copies into a public location or a repo without a second thought.

Fix: Rename to something unambiguous such as ech-public-name-key.pem (and .der), pairing with ech-public-name-cert.pem, and update the two ECH_KEY_FILE/ECH_CERT_FILE defines in server-ech-multi-sni.c and server-ech-multi-ctx.c plus the README table.

#define ECH_CERT_FILE "../certs/ech-public-cert.pem"
#define ECH_KEY_FILE "../certs/ech-public-key.pem"

#define TENANT_A_NAME "tenant-a.example"

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] Both "tenants" share one RSA key pair, weakening the multi-tenant demonstration

All four new examples define TENANT_A_KEY_FILE and TENANT_B_KEY_FILE as the same file, ../certs/server-key.pem. I confirmed the certs share a modulus:

tenant-a modulus md5: d4ae592234a92018d55da1d5782f97ec
tenant-b modulus md5: d4ae592234a92018d55da1d5782f97ec
server-key  modulus md5: d4ae592234a92018d55da1d5782f97ec

They also share the same Subject Key Identifier B3:11:32:C9:.... The entire point of these examples is showing per-tenant credential isolation; having every tenant present the same key pair (and the same key as the default host) makes the isolation illusory and is a pattern readers will copy. New certs were already generated for this PR, so generating a distinct key per tenant costs nothing.

Fix: Generate certs/tenant-a-key.pem and certs/tenant-b-key.pem (distinct keys), re-issue the tenant certs against them, and update the four examples plus the README tables. Applies identically to server-ech-multi-ctx.c:67-72, server-tls-sni-callback.c:57-62, and server-tls-ctx-swap.c:63-68.

for (; tenants->name != NULL; tenants++) {
if (strlen((const char*)tenants->name) == nameLen &&
strncmp((const char*)name, tenants->name, nameLen) == 0) {
if (wolfSSL_use_certificate_file(ssl, tenants->certFile,

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] SNI callback performs blocking disk I/O on every handshake

sni_select_tenant() calls wolfSSL_use_certificate_file() and wolfSSL_use_PrivateKey_file() from inside the handshake callback, so every single connection re-opens, reads, and PEM-decodes the tenant cert and key from disk mid-handshake. In a real multi-tenant front end this is a per-connection filesystem hit and PEM parse on the hot path, and any client can trigger it at will just by sending an SNI.

The server-ech-multi-ctx.c / server-tls-ctx-swap.c variants correctly pre-load into per-tenant CTXs up front; the SNI-callback variants do not, and nothing in the file header or README notes the difference. Since these are teaching examples, readers will copy the callback verbatim.

Fix: Either pre-load each tenant's cert/key into memory at startup and use the _buffer() variants in the callback, or add an explicit comment stating that the file-based load is for brevity and should be replaced with a cached buffer in production. Same issue in server-tls-sni-callback.c:133-136.

#include <netinet/in.h>
#include <unistd.h>

#define HAVE_SIGNAL

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] #define HAVE_SIGNAL immediately before #ifdef HAVE_SIGNAL defeats its own guard

Four files introduce this pattern:

#define HAVE_SIGNAL
#ifdef HAVE_SIGNAL
#include <signal.h>
#endif

The unconditional #define makes the #ifdef a no-op -- the guard can never be turned off, so the code is not actually portable to a platform without \<signal.h>, which is the only reason to have the guard. It also silently redefines a macro name that the wolfSSL build system and other examples use as a build-configured feature flag, so this example's local definition can disagree with the rest of the tree (and will emit a redefinition warning if the toolchain already defines it).

Fix: Drop the unconditional #define HAVE_SIGNAL and let the build define it (or just include \<signal.h> unconditionally, since these examples already require POSIX sockets). Applies to server-ech-local.c:42-45, server-ech-multi-ctx.c:47-50, server-tls-sni-callback.c and server-tls-ctx-swap.c at the same location.

@@ -0,0 +1,369 @@
/* server-ech-multi-sni.c

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] No automated coverage for the four new example programs

This PR adds four new example programs (server-ech-multi-sni, server-ech-multi-ctx, server-tls-sni-callback, server-tls-ctx-swap) totalling ~1500 lines, plus non-trivial rework of three existing ECH examples. The repo's only CI is .github/workflows/arduino*.yml, so none of these are built or exercised anywhere.

I compiled all seven changed/new files locally with gcc -Wall -Wextra -Os against wolfSSL 5.9.2 -- all clean -- and ran server-tls-sni-callback end to end against examples/client/client, confirming the tenant-a (TLS 1.3), tenant-b (TLS 1.2), no-SNI, and unknown-SNI paths all behave as the README describes. But the ECH variants could not be exercised (no HAVE_ECH build available), which is exactly where the BLOCK finding above lives. A scripted smoke test would have caught it.

Fix: Add a smoke-test script exercising each server against its documented client (both the ECH-accepted and ECH-rejected paths), and wire the tls/ Makefile build into CI so at minimum the examples are compile-checked under --enable-ech, --enable-sni, and --enable-opensslextra.

Comment thread tls/server-tls-ctx-swap.c
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA
*/

/* Multi-CTX TLS server: a default WOLFSSL_CTX fronts multiple tenants, with a

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] ctx-swap examples omit the documented wolfSSL_set_SSL_CTX caveat about pinned SSL-level settings

wolfSSL's own implementation of wolfSSL_set_SSL_CTX() (src/ssl.c) carries an explicit warning that the examples do not relay:

 * SECURITY: swapping ssl->ctx switches cm-resolved settings (CA store,
 * CRL, OCSP) to the new CTX but leaves ssl-cached ones (verify mode and
 * callback, minDowngrade, key-size minimums, suites, version bounds)
 * pinned to the original. SNI callbacks must re-apply those ssl-level
 * settings explicitly; CRL/OCSP isolation requires an SSL-local store.

The examples work because every tenant CTX here is configured identically, but a reader adapting this pattern to tenants with genuinely different verify modes or version bounds will silently get the default CTX's settings. Relatedly, wolfSSL_CTX_SetMinVersion(ctx, WOLFSSL_TLSV1_2) is applied only to the default CTX and not to the per-tenant CTXs -- harmless today for the same reason, but inconsistent and misleading.

Fix: Add a short comment in the callback (or the file header) relaying the pinned-settings caveat, and apply wolfSSL_CTX_SetMinVersion() to the tenant CTXs too so the example is internally consistent. Same in server-ech-multi-ctx.c:146.

const char* keyFile;
} tls_tenant;

/* This is used to print which tenant was selected */

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] selectedTenant is a mutable file-scope global rather than per-connection state

static const tls_tenant* selectedTenant is written by the SNI callback and read by main() to build the reply. It works here because the server is strictly single-connection-at-a-time and main() resets it to NULL before each wolfSSL_accept(), but it is not thread-safe and does not generalize -- the first thing anyone does with a multi-tenant server example is make it concurrent. wolfSSL provides per-connection storage for exactly this.

Fix: Store the selection per-connection with wolfSSL_set_ex_data()/wolfSSL_get_ex_data(), or add a comment stating the global is only safe because this example handles one connection at a time. Same pattern in server-tls-ctx-swap.c:112.

Comment thread tls/server-ech-local.c
ret = wolfSSL_read(ssl, buff, sizeof(buff)-1);
if (ret > 0) {
/* Print to stdout any data the client sends */
printf("Client: %s", buff);

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] Dropped newline in "Client: %s" makes server output run together

The diff changes printf("Client: %s\n", buff) to printf("Client: %s", buff), which assumes the client's message carries its own trailing newline. That holds for client-ech-local (it uses fgets()), but not for other clients. Running server-tls-sni-callback (same new pattern) against wolfSSL's stock examples/client/client produced:

Client: hello wolfssl!Shutdown complete

The README for the multi-tenant SNI section explicitly tells the reader to drive these servers with ./examples/client/client -S tenant-a.example, so this is the documented path.

Fix: Restore the trailing newline (a stray blank line for newline-terminated messages is far less confusing than concatenated output), or strip a trailing newline from buff before printing. Same change in server-ech-multi-sni.c:314, server-ech-multi-ctx.c:347, server-tls-sni-callback.c, and server-tls-ctx-swap.c:321.

Comment thread tls/client-ech-local.c
/* Cleanup and return */
cleanup:
/* Cleanup and return */
if (wolfSSL_GetEchStatus(ssl) == WOLFSSL_ECH_STATUS_ACCEPTED) {

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] ECH clients report "ECH failed" for handshakes where ECH was never in play

The cleanup: block collapses every non-ACCEPTED status into Client: ECH failed. wolfSSL_GetEchStatus() distinguishes four states (NOT_OFFERED, GREASE, REJECTED, ACCEPTED), and the difference matters for the workflow these examples teach: a stale config that produced retry configs (REJECTED) is a very different diagnosis from a config that was never applied (NOT_OFFERED) or a GREASE probe. Since this PR deliberately removed the standalone GREASE example and folded GREASE behaviour into a comment in client-ech.c, the client is now the only place a reader can observe that distinction.

Fix: Replace the two-way branch with a switch that names each status. Same block in client-ech.c:246-255.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants