curl ECH+QUIC fix by sftcd · Pull Request #30727 · openssl/openssl

sftcd · 2026-04-08T10:46:59Z

This PR tweaks how custom extensions are handled by ECH and forces the session id length in the client hello to zero in order to enable use of ECH with QUIC for a curl build.

This PR is a fix for #30697

mattcaswell · 2026-04-08T11:18:36Z

CI failures look relevant.

Is it possible to have a test for this? I assume our own QUIC stack is similarly affected by this, so perhaps something in quicapitest.c which sets up a QUIC connection with ECH and verifies that a connection is established.

sftcd · 2026-04-08T11:22:55Z

CI failures look relevant.

Looking at that now.

Is it possible to have a test for this? I assume our own QUIC stack is similarly affected by this, so perhaps something in quicapitest.c which sets up a QUIC connection with ECH and verifies that a connection is established.

Will take a look at that. (In an hour-ish.) It should be possible given that this seems to work for openssl s_client -quic as well, but not sure if we'll see something new with a QUIC+ECH server setup. (Worth finding out though.)

mattcaswell · 2026-04-08T12:27:28Z

This LGTM so far.

t8m · 2026-04-08T15:49:11Z

-        sess_id_len = sizeof(s->tmp_session_id);
+        if (SSL_IS_QUIC_HANDSHAKE(s))
+            sess_id_len = 0;
+        else
+            sess_id_len = sizeof(s->tmp_session_id);


I am curious why this is needed. What is different between QUIC and regular TLS connection that needs this change?

That was more or less done by observation;-) I didn't find where the QUIC code does it, but the session id in the ClientHello is 'legacy' for QUIC, which I guess uses QUIC connection ids instead. For TLS/TCP cases we want a standard 32 octet sized session id.

Hmm. I actually think is more to do with whether the SSL_OP_ENABLE_MIDDLEBOX_COMPAT option is set. This is set by default, but if it's NOT set (which it isn't for QUIC), and we're TLSv1.3 only, then the sess_id_len is 0.

Edited my note above to actually make sense and not say the opposite of what I actually meant!!

so if compat mode is set, then use a 32 octet session id, but otherwise zero length? (that matches 8446 all right) - that seems to work ok with curl etc.

just pushed a version that I think does that

sftcd · 2026-04-08T20:34:51Z

The fixup above adds a test, which, surprisingly, just worked when I figured out the test code. There are a couple of TODO(ECH) things in the test code though where I could do with some guidance or hints.

sftcd · 2026-04-09T01:01:16Z

After much faffing about to find the right way to make the CI happy, tests now pass - but there's still the couple of TODO(ECH) things in the test code:

the first might be ok as-is, not sure - it loads the default provider as without that we get a fail reading in the ECH PEM data (I don't really grok what setup_tests() is doing with loading providers, nor why tbh)
the second is more worrisome - according to valgrind, we're leaking 277 bytes when running the new test, mostly by not freeing up some BIO * things inside the qtserv structure (I think). My worry is that might be related to the transcript buffer being handled differently, which might be tricky to figure out (there are a couple of BIO_free calls in ssl/ech/ech_internal.c that could be related, but I'm absolutely not even close to sure in this case;-)

Other than that, if it makes is easier, happy to squash the loads of fixups here, just tell me.

But on the whole, it's a nice surprise that ECH seems to just work (almost if not quite correctly) for QUIC:-)

sftcd · 2026-04-09T10:27:05Z

fixup above have good changes suggested by @mattcaswell

mattcaswell · 2026-04-09T15:36:30Z

+        if ((s->options & SSL_OP_ENABLE_MIDDLEBOX_COMPAT) != 0)
+            sess_id_len = sizeof(s->tmp_session_id);
+        else
+            sess_id_len = 0;


Is it possible to end up here and end up not negotiating TLSv1.3? The conditions at the moment for sending a zero length sess id are minimum protocol is TLSv1.3 AND middlebox mode is off.

ECH is only defined for TSLv1.3 though, would have to look to check if there's a code path here before that's enforced

could add a version check there too if you'd prefer

adding a check for 1.3 if we're doing ECH (and bailing if not) just before here works fine - want me to push that up?

it looks like this:

$ git diff diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 819555f24a..36c698c163 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -1489,6 +1489,10 @@ __owur CON_FUNC_RETURN tls_construct_client_hello(SSL_CONNECTION *s, WPACKET *pk #ifndef OPENSSL_NO_ECH /* same session ID is used for inner/outer when doing ECH */ if (s->ext.ech.es != NULL) { + if (s->version != TLS1_3_VERSION) { + SSLfatal(s, SSL_AD_PROTOCOL_VERSION, SSL_R_UNSUPPORTED_SSL_VERSION); + return CON_FUNC_ERROR; + } if ((s->options & SSL_OP_ENABLE_MIDDLEBOX_COMPAT) != 0) sess_id_len = sizeof(s->tmp_session_id); else

As an FYI, with the code above our TLSv12 test case fails there, so I'd say it's good to add. I'll push that (and apologies for forcing a re-review)

openssl-machine · 2026-04-11T08:00:52Z

This pull request is ready to merge

Reviewed-by: Tomas Mraz <tomas@openssl.foundation> Reviewed-by: Matt Caswell <matt@openssl.foundation> MergeDate: Sat Apr 11 18:29:37 2026 (Merged from #30727)

npajkovsky · 2026-04-11T18:38:42Z

Thank you. Merged to master and openssl-4.0.

Reviewed-by: Tomas Mraz <tomas@openssl.foundation> Reviewed-by: Matt Caswell <matt@openssl.foundation> MergeDate: Sat Apr 11 18:34:40 2026 (Merged from #30727)

curl ECH+QUIC fix

27d2069

fixup! curl ECH+QUIC fix

5751d04

openssl-machine added the approval: review pending This pull request needs review by a committer label Apr 8, 2026

mattcaswell added branch: master Applies to master branch triaged: bug The issue/pr is/fixes a bug labels Apr 8, 2026

t8m added the branch: 4.0 Applies to openssl-4.0 label Apr 8, 2026

t8m reviewed Apr 8, 2026

View reviewed changes

fixup! fixup! curl ECH+QUIC fix

5f9635d

sftcd added 7 commits April 8, 2026 21:35

fixup! fixup! curl ECH+QUIC fix

8465710

fixup! fixup! fixup! curl ECH+QUIC fix

127fc02

fixup! fixup! fixup! curl ECH+QUIC fix

0e53d68

fixup! fixup! fixup! curl ECH+QUIC fix

20e3e83

fixup! fixup! curl ECH+QUIC fix

5ae78b9

fixup! fixup! curl ECH+QUIC fix

a3133f5

fixup! fixup! curl ECH+QUIC fix

27e3e5b

mattcaswell modified the milestones: 4.0.0, 4.0.0 Final Release Apr 9, 2026

mattcaswell reviewed Apr 9, 2026

View reviewed changes

Comment thread test/quicapitest.c Outdated

mattcaswell reviewed Apr 9, 2026

View reviewed changes

Comment thread test/quicapitest.c Outdated

mattcaswell reviewed Apr 9, 2026

View reviewed changes

Comment thread test/quicapitest.c Outdated

fixup! fixup! curl ECH+QUIC fix

83b4bd0

mattcaswell previously approved these changes Apr 9, 2026

View reviewed changes

mattcaswell requested a review from t8m April 9, 2026 10:39

fixup! fixup! curl ECH+QUIC fix

f18e9b2

sftcd dismissed mattcaswell’s stale review via f18e9b2 April 9, 2026 14:29

t8m previously approved these changes Apr 9, 2026

View reviewed changes

t8m requested a review from mattcaswell April 9, 2026 15:33

t8m added the tests: present The PR has suitable tests present label Apr 9, 2026

mattcaswell reviewed Apr 9, 2026

View reviewed changes

fixup! fixup! curl ECH+QUIC fix

23707db

sftcd dismissed t8m’s stale review via 23707db April 9, 2026 16:55

t8m requested review from mattcaswell and t8m April 9, 2026 18:39

t8m approved these changes Apr 9, 2026

View reviewed changes

mattcaswell approved these changes Apr 10, 2026

View reviewed changes

openssl-machine pushed a commit that referenced this pull request Apr 11, 2026

curl ECH+QUIC fix

96c76dc

Reviewed-by: Tomas Mraz <tomas@openssl.foundation> Reviewed-by: Matt Caswell <matt@openssl.foundation> MergeDate: Sat Apr 11 18:29:37 2026 (Merged from #30727)

npajkovsky closed this Apr 11, 2026

openssl-machine pushed a commit that referenced this pull request Apr 11, 2026

curl ECH+QUIC fix

41192a1

Reviewed-by: Tomas Mraz <tomas@openssl.foundation> Reviewed-by: Matt Caswell <matt@openssl.foundation> MergeDate: Sat Apr 11 18:34:40 2026 (Merged from #30727)

This was referenced Apr 13, 2026

[master, 4.0] NEWS and CHANGES update #30798

Closed

release: update CHANGES.md and NEWS.md #30804

Closed

release: update CHANGES.md and NEWS.md #30805

Closed

Uh oh!

Conversation

sftcd commented Apr 8, 2026

Uh oh!

mattcaswell commented Apr 8, 2026

Uh oh!

sftcd commented Apr 8, 2026

Uh oh!

mattcaswell commented Apr 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcaswell Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sftcd Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sftcd commented Apr 8, 2026

Uh oh!

sftcd commented Apr 9, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sftcd commented Apr 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openssl-machine commented Apr 11, 2026

Uh oh!

npajkovsky commented Apr 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mattcaswell Apr 9, 2026 •

edited

Loading

sftcd Apr 9, 2026 •

edited

Loading