Conversation
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate DTLS handling issues and critical test build-guard failures block approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds configurable DTLS cookie handling so servers can process fragmented initial DTLS 1.3 ClientHellos when cookies are disabled.
Changes:
- Adds cookie policy APIs and shared secret lifecycle handling.
- Updates DTLS accept paths and callback behavior.
- Adds DTLS 1.2/1.3 tests and documentation.
File summaries
| File | Change | Final findings |
|---|---|---|
wolfssl/ssl.h |
Public cookie APIs and callback documentation | None |
wolfssl/internal.h |
Internal state and helper declarations | None |
tests/api/test_tls13.c |
TLS 1.3 API coverage | None |
tests/api/test_dtls13.h |
DTLS 1.3 test registration | None |
tests/api/test_dtls13.c |
DTLS 1.3 handoff and fragmentation tests | None |
tests/api/test_dtls.h |
Shared DTLS test declarations | None |
tests/api/test_dtls.c |
DTLS cookie, fragmentation, and policy tests | Critical (1 vote): Missing server-build guards cause compilation/link failures. Two tests also require both client and server support. |
src/tls13.c |
TLS 1.3 cookie and accept handling | Moderate (1 vote): Stateful processing must account for disabled HRR-cookie support. |
src/ssl.c |
Callback state reset | None |
src/ssl_api_hs.c |
Generic accept state handling | Moderate (1 vote): No-HRR-cookie builds can reject valid no-cookie DTLS 1.3 ClientHellos and must retain DTLS 1.2 HVR downgrade behavior. |
src/ssl_api_dtls.c |
Cookie policy and callback implementation | None |
src/internal.c |
DTLS cookie initialization | Moderate (1 vote): Reset chGoodCbDone when initializing a reused server connection. |
doc/dox_comments/header_files/ssl.h |
Public API documentation | None |
Review details
Suppressed comments (3)
src/internal.c:2440
- This initializer is also used by
wolfSSL_set_accept_state(), so a reused server object can enter a new handshake withchGoodCbDonestill set from its previous no-cookie connection.DtlsNoCookieChGood()will then suppress the callback for the new ClientHello. Reset this per-connection bit alongside the cookie policy when initializing the server side.
src/ssl_api_hs.c:931 - In a build with
WOLFSSL_SEND_HRR_COOKIEdisabled, DTLS 1.3 has no HRR-cookie path, butInitSSL_DtlsServer()still setssendCookieto 1. This leaves the generic accept path stateless before parsing the version, soDoClientHelloStateless()rejects a valid DTLS 1.3 no-cookie ClientHello instead of transitioning to stateful processing; generic methods also need to retain the DTLS 1.2 HVR downgrade behavior.
if (!ssl->options.sendCookie)
ssl->options.dtlsStateful = 1;
src/tls13.c:17781
- This test assumes
sendCookiemeans that a DTLS 1.3 cookie mechanism is available. WithWOLFSSL_SEND_HRR_COOKIEdisabled,InitSSL_DtlsServer()still initializes this bit to 1 for every DTLS server, so this path leavesdtlsStatefulfalse and sends the first no-cookie ClientHello throughDoClientHelloStateless(), which rejects it (and cannot reassemble a first fragment). Please make the stateful decision account for the compile-time absence of HRR-cookie support; the genericwolfSSL_accept()path needs the same correction.
if (!ssl->options.sendCookie)
ssl->options.dtlsStateful = 1;
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
rizlik
force-pushed
the
dtls13_cookie_disabled
branch
2 times, most recently
from
September 17, 2026 08:58
2356c7b to
1a0dbc1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
First PR in a series to implement DTLS server that can handle bounded fragmented Client Hello.
This first PR allow to switch return routability check completely by disabling cookies.
A server with cookies disabled is compatible with other libraries that fragments ClientHellos to accomodate big PQC keyshares.