DTLS 1.3: don't echo legacy_session_id in ServerHello#10007
DTLS 1.3: don't echo legacy_session_id in ServerHello#10007douzzer merged 8 commits intowolfSSL:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds DTLS 1.3 compliance behavior to avoid echoing the ClientHello legacy session ID in ServerHello, along with a regression test to validate the on-wire encoding.
Changes:
- Add a new DTLS 1.3 test that verifies
legacy_session_id_echois empty in ServerHello. - Update TLS 1.3 message construction/parsing paths to ensure DTLS 1.3 does not include/echo a session ID (including HRR transcript reconstruction).
- Add a guard in
DoTls13ServerHelloto enforce an empty session ID for DTLS 1.3.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/api/test_dtls.h | Registers the new DTLS 1.3 regression test in the test declarations list. |
| tests/api/test_dtls.c | Implements a memio-based DTLS 1.3 test that inspects the ServerHello bytes for empty legacy_session_id_echo. |
| src/tls13.c | Ensures DTLS 1.3 does not store/echo legacy session IDs and adjusts HRR handshake-hash reconstruction accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
rizlik
left a comment
rizlik
left a comment
There was a problem hiding this comment.
I do think we should avoid to send the legacy session id in client hello if doing 1.3 resumption.
This also means we should manually set the legacy id in the test.
Add test for flexible 1.3/1.2 client doing 1.2 resumption against a flexible 1.3/1.2 server for both dtls and tls
|
I'm not sure about that. This section encourages populating it in the CH
|
Check the important MUST part:
We probably send a random value due to Middlebox compatibility if resuming with 1.3 PSK method. |
There was a problem hiding this comment.
Pull request overview
Updates wolfSSL DTLS 1.3 behavior to comply with RFC 9147 by ensuring legacy_session_id_echo is empty in ServerHello, and adds regression tests to prevent session-id resumption/version-downgrade issues.
Changes:
- Enforce empty
legacy_session_id_echofor DTLS 1.3 (and QUIC) in TLS 1.3 ServerHello processing/sending. - Adjust DTLS 1.3 handling around session IDs/HRR hashing to avoid echoing the client’s legacy session id.
- Add new TLS/DTLS API tests covering DTLS 1.3 session id echo behavior and a session-id resumption downgrade scenario.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/api/test_tls.h | Registers the new TLS resumption/downgrade regression test. |
| tests/api/test_tls.c | Adds test_tls_session_id_resume_downgrade covering resumption behavior. |
| tests/api/test_dtls.h | Registers the new DTLS 1.3 legacy session id echo regression test. |
| tests/api/test_dtls.c | Adds test_dtls13_no_session_id_echo validating RFC 9147 ServerHello requirements. |
| src/tls13.c | Ensures DTLS 1.3/QUIC don’t echo legacy session id; updates HRR hashing and ClientHello options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
This PR updates wolfSSL’s DTLS 1.3 behavior to comply with RFC 9147 by ensuring legacy_session_id_echo is not echoed in ServerHello (and related paths), even when the client sends a non-empty legacy_session_id.
Changes:
- Enforce empty
legacy_session_id_echofor DTLS 1.3 (and QUIC) inServerHelloparsing/serialization and HRR transcript reconstruction. - Avoid storing/copying the client’s legacy session ID in DTLS 1.3 server-side processing to prevent accidental echoing.
- Add a DTLS 1.3 regression test and adjust memio dump helpers to use
XFILEI/O.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
wolfssl/internal.h |
Makes WOLFSSL_SESSION.version unconditionally present to support broader use. |
src/tls13.c |
Enforces empty legacy session ID echo for DTLS 1.3/QUIC and updates CH/SH/HRR handling accordingly. |
src/dtls.c |
Prevents copying ClientHello legacy session ID into the server session in DTLS 1.3 stateless reply. |
src/tls.c |
Removes HAVE_SESSION_TICKET guard around resumption/session-parameter checks in PSK parsing. |
src/ssl_sess.c |
Makes session version propagation unconditional (matching struct change). |
tests/api/test_dtls.c |
Adds DTLS 1.3 wire-check test for empty legacy_session_id_echo. |
tests/api/test_dtls.h |
Registers the new DTLS 1.3 test. |
tests/utils.c |
Switches memio dump writing from BIO to XFILE APIs. |
tests/api.c |
Switches memio dump writing from BIO to XFILE APIs and aligns with unconditional session version. |
Comments suppressed due to low confidence (1)
src/tls.c:1
- Removing the
HAVE_SESSION_TICKETguard makes this block compile in configurations whereWOLFSSL_SESSIONmay not includecipherSuite0/cipherSuite(e.g.,NO_RESUME_SUITE_CHECK+ noSESSION_CERTS+ no(WOLFSSL_TLS13 && HAVE_SESSION_TICKET)), causing compile-time struct-member errors. Re-introduce a compile-time guard that matches the actual presence of these fields (or make the session fields unconditional if that’s the intended direction).
/* tls.c
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
retest this please no history. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/ssl_sess.c:1
- Session serialization/deserialization compatibility: this decode now unconditionally expects the protocol version bytes. Previously, the presence of these bytes depended on compile-time options, so a session blob produced by an older build/config (that omitted version) will now fail to deserialize with
BUFFER_ERROR. IfwolfSSL_i2d_SSL_SESSION/wolfSSL_d2i_SSL_SESSIONare part of the public API surface, consider makingd2ibackward-compatible (e.g., detect legacy encoding based on remaining length / an explicit format version tag) or documenting this as a breaking change with a clear migration story.
/* ssl_sess.c
wolfcrypt/test/test.c:1
- This RSA test buffer sizing change (and the memio dump I/O refactors elsewhere) appears unrelated to the stated PR goal of DTLS 1.3
legacy_session_id_echobehavior. Consider splitting unrelated changes into separate PRs, or expand the PR description to explain why these adjustments are necessary in the context of ZD21376.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/tls.c:1
- The
HAVE_SESSION_TICKETguard removal makes this block compile in configurations wheressl->sessionmay not include the referenced fields (e.g.,cipherSuite0/ cipherSuiteare conditionally compiled inWOLFSSL_SESSION). This can become a compile-time break in minimal builds. Consider restoring an appropriate feature guard (e.g., around resumption-only-ticket logic) or aligning the block’s#ifconditions with theWOLFSSL_SESSIONmember availability macros that controlcipherSuite0/cipherSuite/haveEMS/version.
/* tls.c
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Failures are present in master too |
douzzer
added
Staged
douzzer
left a comment
douzzer
left a comment
There was a problem hiding this comment.
[all-gcc-c99] [7 of 492] [d6a243faa4]
configure${config_analyzer_note}... real 0m8.800s user 0m5.400s sys 0m4.386s
build...tests/api/test_dtls.c: In function ‘test_dtls13_no_session_id_echo’:
a1d7c8de40 (<juliusz@wolfssl.com> 2026-03-26 15:33:27 +0100 2648) if (sess->sessionIDSz == 0) {
tests/api/test_dtls.c:2648:13: error: potential null pointer dereference [-Werror=null-dereference]
2648 | if (sess->sessionIDSz == 0) {
| ~~~~^~~~~~~~~~~~~
a1d7c8de40 (<juliusz@wolfssl.com> 2026-03-26 15:33:27 +0100 2648) if (sess->sessionIDSz == 0) {
tests/api/test_dtls.c:2648:13: error: potential null pointer dereference [-Werror=null-dereference]
tests/api/test_dtls.c:2648:13: error: potential null pointer dereference [-Werror=null-dereference]
tests/api/test_dtls.c:2648:13: error: potential null pointer dereference [-Werror=null-dereference]
tests/api/test_dtls.c:2648:13: error: potential null pointer dereference [-Werror=null-dereference]
tests/api/test_dtls.c:2648:13: error: potential null pointer dereference [-Werror=null-dereference]
cc1: all warnings being treated as errors
make[2]: *** [Makefile:10612: tests/api/unit_test-test_dtls.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [Makefile:11465: all-recursive] Error 1
make: *** [Makefile:6282: all] Error 2
real 0m9.655s user 1m20.078s sys 0m3.929s
scenario started 2026-03-30T04:14:16.480090Z, real elapsed 0m18.500257s
all-gcc-c99 fail_build
failed config: 'EXTRA_CPPFLAGS=-Werror' '--srcdir' '.' '--disable-jobserver' '--enable-option-checking=fatal' '--enable-all' '--enable-acert' '--enable-dtls13' '--enable-dtls-mtu' '--enable-dtls-frag-ch' '--enable-dtlscid' '--enable-quic' '--with-sys-crypto-policy' '--enable-sp-math-all' 'CC=gcc' 'CFLAGS=-DTEST_ALWAYS_RUN_TO_END' 'CPPFLAGS=-std=c99 -pedantic -Wdeclaration-after-statement -Wnull-dereference -DTEST_LIBWOLFSSL_SOURCES_INCLUSION_SEQUENCE -Wdeclaration-after-statement -DNO_WOLFSSL_CIPHER_SUITE_TEST -DWOLFSSL_OLD_PRIME_CHECK -DWOLFCRYPT_TEST_LINT'
|
@douzzer fixed |
douzzer
left a comment
douzzer
left a comment
There was a problem hiding this comment.
../testing/Jenkins/generic-config-parser/generic-config-parser.sh -s 110 -c 5 ../testing/Jenkins/generic-config-parser/config-flags/PRB-single-flag.txt
[...]
Testing configuration:
--enable-rsapub --enable-cryptonly
71
Testing DEFAULT: --enable-rsapub --enable-cryptonly
[...]
wolfcrypt/test/test.c: In function ‘rsa_sig_test’:
wolfcrypt/test/test.c:22931:12: error: unused variable ‘outSz’ [-Werror=unused-variable]
22931 | word32 outSz = RSA_TEST_BYTES;
| ^~~~~
|
@douzzer fixed |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ZD21376