fix three client-side ECH bugs (grease 0-RTT, HRR/SH, NULL configs)#596
Merged
fix three client-side ECH bugs (grease 0-RTT, HRR/SH, NULL configs)#596
Conversation
Co-authored-by: OpenAI Codex <codex@openai.com>
Co-authored-by: OpenAI Codex <codex@openai.com>
kazuho
commented
Apr 17, 2026
Instead of building a separate grease ECH extension, reuse the real ECH machinery (inner/outer CH split) then discard the inner state and adopt the outer. This removes the duplicate `build_grease_ech_extension` function and ensures the PSK binder is correctly computed over the outer CH that the server actually sees. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The transcript hash update and PSK binder computation pattern was duplicated between the inner CH path and the grease ECH path. Extract it into a shared helper function. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Revert block-scoped random_secret back to function scope - Remove early break in ECH EE handler; use if/else instead so the case follows the standard src = end pattern - Deduplicate client_decode_ech_config_list call between grease and real ECH paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reuse test_resumption(0, 0) instead of repeating the three subtest calls. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Instead of a global variable to signal grease mode, use a distinct ciphers pointer that get_test_ech_mode recognizes. This encodes the mode into ctx->ech.client.ciphers which is already being manipulated by the test harness. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Expose the ECH configs iovec as a test-wide global (analogous to ctx / ctx_peer) and use the library's own convention: base == NULL means no ECH, len == 0 means grease, len > 0 means real ECH. This eliminates the enum, sentinel cipher list, and translation function. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
@huitema WDYT? |
Collaborator
|
I need to build picoquic against the PR and redo my ECH tests to validate this. Give me a bit of time, please... |
huitema
reviewed
Apr 18, 2026
kazuho
changed the title
huitema
reviewed
Apr 19, 2026
| @@ -2375,6 +2397,7 @@ static int send_client_hello(ptls_t *tls, ptls_message_emitter_t *emitter, ptls_ | |||
Collaborator
There was a problem hiding this comment.
suggestion
}
else if (properties->client.ech.configs.base != NULL) {
This would allow three states:
- if ech.configs is set with a non zero string, do ECH
- if ech.configs is set explicitly with a zero length string but a valid base, do grease,
- if ech.configs is left non configured, with NULL base, do not grease.
An alternative would be to add a "do grease" lag in the properties structure.
The three bit fields encoded only four reachable states. Replace them with a single state enum (NONE/OFFERED/ACCEPTED/GREASE); GREASE is client-only. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Covers the RFC 9849 §6.1.5 case: client offers ECH, HRR confirms acceptance, but the ServerHello's ECH confirmation is corrupted. The client must abort with illegal_parameter rather than fall back to the outer ClientHello. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Per RFC 9849 Section 6.1.5, if HelloRetryRequest confirms ECH acceptance, the ServerHello MUST also confirm it; otherwise the client terminates with an illegal_parameter alert. Previously, the client silently demoted state from ACCEPTED back to OFFERED and fell back to the outer ClientHello. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
kazuho
force-pushed
the
topic/fix-grease-ech-0rtt
branch
from
344b589 to
61fd8f1
Compare
huitema
reviewed
Apr 19, 2026
Collaborator
huitema
left a comment
huitema
left a comment
There was a problem hiding this comment.
I am not sure of what is happening. The comment in line 844 of t/picotls.c says that "base == NULL means no ECH, len == 0 means grease, len > 0 means real ECH", but the code in "sendClientHello" does not test for "base == NULL" on line 2403 of picotls.c. Did we miss someting, or am I reading it wrong?
Observes the ClientHello extensions seen by the server. With
handshake_properties passed and client.ech.configs left zero-initialized
({NULL, 0}), the client must not send the ECH extension, matching the
behavior documented in picotls.h.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The header documented that {NULL, 0} disables ECH, but the client-side
implementation took the grease branch whenever configs.len was zero --
including the zero-initialized {NULL, 0} default. That forced grease on
any handshake that supplied handshake_properties for unrelated reasons
(e.g., QUIC additional extensions), even though the context's ECH setup
wasn't meant to apply. Gate the grease branch on configs.base != NULL to
match the documented API, and expand the comment in picotls.h to cover
all three states explicitly.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
kazuho
commented
Apr 19, 2026
kazuho
changed the title
huitema
approved these changes
Apr 20, 2026
Collaborator
huitema
left a comment
huitema
left a comment
There was a problem hiding this comment.
Looks good now! Thanks for fixing this bug.
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
2 participants
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.
Summary
Three client-side ECH bugs, each with a regression test:
ServerHello that does not confirm must abort with
illegal_parameter;picotls silently fell back to the outer ClientHello.
handshake_properties.client.ech.configs = {NULL, 0}should disable ECH(as
picotls.hdocuments), but the client was still sending GREASE.Details
PSK path, randomizing the outer PSK identity/binder and breaking resumption.
Refactored to send a normal resumption-capable ClientHello with a dummy but
syntactically valid ECH extension, matching RFC 9849 §6.2.1.
client_ech_select_hellodemoted the post-HRRACCEPTEDstate back toOFFEREDon an SH confirmation mismatch. It nowaborts with
PTLS_ALERT_ILLEGAL_PARAMETER.configs.len == 0regardless of
.base. Added the missingconfigs.base != NULLgate so thedefault zero-initialized
handshake_propertiesdoes not force GREASE.Plus internal cleanup: the three ECH bit fields collapsed into a single state
enum, test-harness config handling simplified, and binder computation
deduplicated.
Validation
build/default/test-openssl.t— all 17 subtests pass. Each fix has adedicated regression test that fails without its corresponding code change.
Closes #576.