[ZeroIntensity]

Based on #135435.

• Issue: Support TLS Encrypted ClientHello (ECH) #89730

[read-the-docs-community]

Documentation build overview

📚 cpython-previews | 🛠️ Build #35366597 | 📁 Comparing c5d9c39 against main (7ee9ddd)

  🔍 Preview build  

24 files changed · ± 24 modified

± Modified

• builtins/functions.html
• c-api/code.html
• deprecations/index.html
• deprecations/pending-removal-in-3.19.html
• deprecations/pending-removal-in-future.html
• library/cmd.html
• library/concurrent.futures.html
• library/datetime.html
• library/dis.html
• library/email.utils.html
• and 14 more...

[sftcd]

Good to see this being progressed. Feel free to reach out if we can help or test something.

[gpshead]

Disclosure: I used Claude Fable to help analyze the PR as I don't spend much time deep in the _ssl module guts these days, or speak sparkly new ECH terms yet. It read OpenSSL's source and the RFC, and it worked to reproduce many of the behaviors described. I've read through the findings myself and edited some for the wording and suggestions - but yes this will still read a lot like a model review. -gps

This was reviewed against RFC 9849 and a local build with OpenSSL 4.0. The comments below are mostly about the docs (the GREASE example contradicts the RFC's 6.2.1, and the rejection/retry semantics need spelling out). One seemingly upstream OpenSSL 4.0 bug (noted below) that affects set_outer_alpn_protocols(), and some smaller API consistency questions. The docs example and the outer-ALPN constraint are the ones to block on.

[gpshead]

This suppress is hiding something users will hit. When the outer ALPN list isn't a subset of the inner one and the server rejects ECH, the client doesn't get SSLECHRejectedError. It gets a plain SSLError (BAD_EXTENSION), with no retry_config, and get_ech_status() reports ECH_STATUS_FAILED. I tried three combinations against OpenSSL 4.0 with a stale config:

inner	outer	client result
h2	unset	SSLECHRejectedError, retry config present
h2, http/1.1	http/1.1	SSLECHRejectedError, retry config present
h2	http/1.1	SSLError: BAD_EXTENSION, no retry config

This breaks the recovery flow in RFC 9849 6.1.6 for anyone using set_outer_alpn_protocols() the way 10.5 contemplates (a different, non-sensitive list in the outer hello).

I think this is an OpenSSL bug - cc: @sftcd . In ssl/statem/extensions_clnt.c (openssl-4.0), tls_parse_stoc_alpn() validates the server's selection against s->ext.alpn (the inner list) unconditionally and raises SSL_R_BAD_EXTENSION. tls_construct_ctos_alpn() sent s->ext.ech.alpn_outer in the outer hello, and after rejection that is the list the server chose from. The SNI parser in the same file already handles this case ("if we tried ECH and failed, the outer is what's expected"); ALPN doesn't. Because the handshake dies there, it never reaches the ech_required abort in statem_clnt.c that sets retry_configs_ok, which is why no retry config is available. OpenSSL's own tests only use disjoint inner/outer ALPN on the success path. Worth raising upstream. [This was Claude Fable's finding by analyzing the behavior, RFC, and code -gps]

What to do? For now, assuming that is an upstream issue, lets document the constraint on set_outer_alpn_protocols(). Should this test assert the actual exception rather than suppress it, so we notice when the behavior changes or is that going to turn into a change-detector based on the version/behavior of OpenSSL linked against?

We could just assert the server-side ALPN as now and add a comment linking the upstream issue, so the suppress is explained perhaps.

[ZeroIntensity]

Yeah, this looks like an OpenSSL bug. I added a comment to the test and a note to the docs. I don't think this should be blocking, though; once OpenSSL fixes things, we can just fix the test and the docs.

[bedevere-app]

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

[ZeroIntensity]

Looks like there are three things that should ideally be fixed upstream in OpenSSL, but I don't think they need to block this PR:

1. OSSL_ECHSTORE_read_echconfiglist rejects the entire entry list when it finds an unusable entry, even when another entry is usable. I don't think this is a violation of the RFC, but it'd be nice if it didn't do that (or if there were an API that didn't do that).
2. Additionally, OSSL_ECHSTORE_read_echconfiglist does not ignore versions that it doesn't recognize, which is a violation of the RFC: "Clients MUST ignore any ECHConfig structure with a version they do not support." It's a real pain for us to try to implement that here, because we'll have to do some parsing on the ECH config and rely on some OpenSSL implementation details (such as the error returned for an unrecognized version).
3. When using SSL_ech_set1_outer_alpn_protos/SSL_CTX_ech_set1_outer_alpn_protos, OpenSSL checks the server's selection against the inner list even when the server used the outer ClientHello. This causes a BAD_EXTENSION error, which we're suppressing in the tests (see this comment).

cc @sftcd

[gpshead]

comments are docs related:

The set_ech_config() docs still say SSLError is raised when the input "contains no suitable value". They should say that, with current OpenSSL, one unusable entry can reject the whole list despite the RFC.

that's the flaw vs RFC 9849 section 4 that says clients MUST ignore both kinds of entry. This is likely to matter in practice in the long term: servers list configs in preference order, so a future ECH version or a new KEM would probably come first, and a CPython client would then fail to load the list at all instead of using the fallback entry.

while ECH is new, i'm doubting people run into that? so not blocking on this other than updating the docs to call it out.

The retry loop calls socket.create_connection((hostname, 443)) again, which re-resolves DNS. the rfc 6.1.6 says it is not safe to retry against addresses from a new DNS query. This is pedantic - but... given docs examples are often used as a starting point for real code some pedantry there can be good.

---

A small correction to item 2 in your upstream OpenSSL list: OSSL_ECHSTORE_read_echconfiglist does skip unknown versions, except when one is the first entry. So that turns into "(a) format detection shouldn't depend on the first entry's version, and (b) unusable entries (unsupported KEM/suites, mandatory extensions, bad public_name) should be skipped, not fail the list."

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit c5d9c39 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F160318%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.