ECH-PR #22938
ECH-PR #22938
Conversation
github-actions
bot
added
the
severity: fips change
t8m
added
branch: master
github-actions
bot
added
the
severity: ABI change
Ah! So |
|
Please do that. I want to verify that the ABIDIFF check will detect the changes. |
Did that, looks like it passed the ABIDIFF one? |
|
It shows that there is an ABI change and that is reflected by the label. |
Just checking: that's the expected outcome given new APIs are being added isn't it? (If not, I'm confused:-) |
Yes. I am just saying that the label is just indication of an ABI change. If that ABI change is OK or not has to be carefully reviewed. Also that the ABIDIFF output in the CI job might give some hints. |
|
Thanks for your work on this. I'd like to see this. However, realistically, this is too big to land as one PR. I think the way forward here is for you to create a subset of this PR implementing only the minimum viable functionality for ECH on the client side only. If my reading of your design document is correct, this means providing only the following APIs (at most, preferably less): int SSL_ech_set1_echconfig(SSL *s, const unsigned char *val, size_t len);
int SSL_CTX_ech_set1_echconfig(SSL_CTX *ctx, const unsigned char *val,
size_t len);
int SSL_CTX_ech_set_outer_alpn_protos(SSL *s, const unsigned char *protos,
unsigned int protos_len);
int SSL_ech_set_outer_server_name(SSL *s, const char *outer_name, int no_outer);
int SSL_ech_get_retry_config(SSL *s, unsigned char **ec, size_t *eclen);
int SSL_ech_get_status(SSL *s, char **inner_sni, char **outer_sni);
void SSL_ech_set_callback(SSL *s, SSL_ech_cb_func f);
void SSL_CTX_ech_set_callback(SSL_CTX *ctx, SSL_ech_cb_func f);
#define SSL_OP_ECH_IGNORE_CID SSL_OP_BIT(36)Command line tool updates, and convenience functionality (e.g. to allow clients to calculate ECH configs) can also be omitted in this initial PR. The idea is a MVP PR which makes client-side usage feasible, not necessarily easy. |
|
Hiya,
On 18/12/2023 09:34, Hugo Landau wrote:
Realistically, this is too big to land as one PR.
Yep, it's gigantic;-( If there's a way to split it into multiple
PRs that makes sense and will help get things done, I'm happy to
try take such a route. I'm not sure there is really though, but
let's explore the one you've suggested...
I think the way forward here is for you to create a subset of this PR implementing only the minimum viable functionality for ECH on the client side only.
So how'd one test that? (I'm assuming that a PR that can't be
shown to work is a non-starter, even if it's part of a series
that overall ends up demonstrably working.)
I think by the time you had a working client with test code
for the server you'd be back at a nearly-as-big PR. Or were you
thinking of something else? I'm assuming that merging client
code that can only be tested via non-merged application layer
code against the CF test server or one of my test servers wouldn't
be ok to merge. But maybe that's not a good assumption?
(BTW, if splitting into functionally different PRs is the way
to go, I think I'd argue to start with the server, that's a
bit simpler in terms of the number of existing files touched,
could at least be tested from a released browser, and having
those APIs more settled would help with trying to upstream
changes to e.g. web servers, which I think is more pressing,
time-wise, than stability for the equivalent client-side APIs.)
If my reading of your design document is correct, this means providing only the following APIs (at most, preferably less):
```c
int SSL_ech_set1_echconfig(SSL *s, const unsigned char *val, size_t len);
int SSL_CTX_ech_set1_echconfig(SSL_CTX *ctx, const unsigned char *val,
size_t len);
int SSL_CTX_ech_set_outer_alpn_protos(SSL *s, const unsigned char *protos,
unsigned int protos_len);
int SSL_ech_set_outer_server_name(SSL *s, const char *outer_name, int no_outer);
int SSL_ech_get_retry_config(SSL *s, unsigned char **ec, size_t *eclen);
int SSL_ech_get_status(SSL *s, char **inner_sni, char **outer_sni);
void SSL_ech_set_callback(SSL *s, SSL_ech_cb_func f);
void SSL_CTX_ech_set_callback(SSL_CTX *ctx, SSL_ech_cb_func f);
#define SSL_OP_ECH_IGNORE_CID SSL_OP_BIT(36)
```
Command line tool updates, and convenience functionality (e.g. to allow clients to calculate ECH configs) can also be omitted in this initial PR. The idea is a MVP PR which makes client-side usage feasible, not necessarily easy.
There are some client-side things that could be removed (e.g.
``ossl_ech_find_echconfigs()``) but taking all that and the
command line stuff out, doesn't really reduce the size to
something that much more manageable. (Not saying we shouldn't,
but just not sure how much it'll help by itself.)
Cheers,
S.
|
I don't see the issue here. IMO an existing ECH server is an acceptable test target. See 95-test_external_cf_quiche.t for an example of how we did this with QUIC. |
Hmm. Interesting. Happy to take a look again at what it'd be like splitting into multiple PRs. That said, and while I don't doubt your advice, I'd be a bit happier putting in that chunk of effort if other maintainers also figured this was a good path forward... |
Absolutely — it would be good to have a second opinion here. |
|
@sftcd given you've already a working implementation and that an interop test would be if not mandatory then very much welcome, I'd say doing what Hugo suggests would definitely make sense to me. |
|
OK, I'll take a look at splitting into separate PRs with external tooling for tests and get back here. Given the time of year that'll likely take a while (I'm also looking at fuzzing as discussed earlier). Meanwhile, I'd still appreciate any comments if someone wants to delve into the code changes - those won't be much affected by a split into >1 PR. |
|
So I've taken a look at splitting the PR into 3: server, client and extras (in that order, each building on the earlier) and while I fear it'll feel a bit like treading a treacle torus (in that I'll end up where I started after loadsa trudging;-) I can see that'll make review/landing more tractable, and refactoring generally does improve things, so I'm up for giving it a shot. Two asks first though:
Sound reasonable? |
Yeah, I've no idea about that one tbh, so hints welcome! this links to a build log for that, the meaty part of which says: |
So (after holidays:-), I've started on this. So far I've got a basic external test using boringssl that runs it's client vs the openssl server and vice versa. That's here and seems to work - comments are welcome. Next step is to do some kind of split into ECH server, client and extras that could each be a separate PR. That'll take a bit of time maybe as it'll call for some possibly extensive refactoring of code I'm sure. (I've not updated this PR yet with that, will think about whether it'll be better to close this and open new ones or to keep this PR number for the first of those.) |
| That padding will apply to the Certificate, CertificateVerify and | ||
| EncryptedExtensions handshake messages emitted by the server and | ||
| will ensure those are at least a minimum length, (currently 1808, | ||
| 480 and 32 octet minima respectively). The value will also be |
There was a problem hiding this comment.
Actually I think the "32 octet" is correct there, as the "minima" is the plural. But maybe my grammar's wrong too;-)
There was a problem hiding this comment.
I remember seeing written "octets" few lines below, no ?
There was a problem hiding this comment.
yep, but I think the grammar in both cases is correct tbh
|
I made the above changes to a rebased dev branch that I've pushed to github - will push to this branch in a bit. (Late here now.) |
|
Huh, I seem to have accidentally closed the PR when pushing to it.... will fix and reopen when I figure out how:-) |
|
I think I undid that bad push so hopefully the PR is now up to date with those changes from yesterday and rebased.. |
ssl/statem/extensions_srvr.c
Outdated
| return 0; | ||
| } | ||
| OSSL_TRACE_BEGIN(TLS) { | ||
| BIO_printf(trc_out,"set 8 zeros for ECH acccpt confirm in HRR\n"); |
ssl/statem/extensions_clnt.c
Outdated
| /* | ||
| * if we called this for inner and did send then | ||
| * the following two things were set just before | ||
| * returning (i.e. at the bottom of this fucntion) |
include/internal/packet.h
Outdated
| * (because pkt may be a subpacket so we don't here have a pointer to | ||
| * the allocated buffer) | ||
| */ | ||
| __owur static ossl_inline int PACKET_replace(PACKET *pkt, PACKET *newpkt) |
ssl/ech.c
Outdated
| * | ||
| * If an extension constructor has side-effects then it is (in general) | ||
| * unsafe to call twice. For others, we need to be able to call twice, | ||
| * if we do want possibly different values in inner and outer, of if |
ssl/ech.c
Outdated
| * @brief decode the DNS name in a binary RRData | ||
| * @param buf points to the buffer (in/out) | ||
| * @param remaining points to the remaining buffer length (in/out) | ||
| * @param dnsname returns the string form name on success |
There was a problem hiding this comment.
form is right, but string-form is better so did that (same in a few other places)
include/openssl/ssl.h.in
Outdated
| @@ -428,6 +431,28 @@ typedef int (*SSL_async_callback_fn)(SSL *s, void *arg); | |||
|
|
|||
| #define SSL_OP_PREFER_NO_DHE_KEX SSL_OP_BIT(35) | |||
|
|
|||
| #ifndef OPENSSL_NO_ECH | |||
| /* Set this to tell client to emit greased ECH values */ | |||
There was a problem hiding this comment.
sure (though indentation in that file seems all over the place)
util/libssl.num
Outdated
| SSL_CTX_ech_server_flush_keys ? 3_4_0 EXIST::FUNCTION:ECH | ||
| SSL_CTX_ech_raw_decrypt ? 3_4_0 EXIST::FUNCTION:ECH | ||
| SSL_CTX_ech_set_callback ? 3_4_0 EXIST::FUNCTION:ECH | ||
| ossl_ech_make_echconfig ? 3_4_0 EXIST::FUNCTION:ECH |
There was a problem hiding this comment.
ossl_... is the prefix for internals methods, not for public ones.
There was a problem hiding this comment.
yep, I made that change in response to Viktor's review (here)
test/sslapitest.c
Outdated
| if (ecl_string == NULL) | ||
| goto out; | ||
| memcpy(ecl_string, lnbuf, readbytes); | ||
| /* zap the '\n' if present */ |
test/sslapitest.c
Outdated
|
|
||
| #if 0 | ||
| #ifndef OPENSSL_NO_USABLE_ECH | ||
| if (!is_fips && hpke_setlibctx(libctx)!=1) |
There was a problem hiding this comment.
That's all under a #if 0 so I just took it out. (Probably a remnant from the HPKE PR which originated in this code too.)
apps/lib/s_cb.c
Outdated
| {"encrypted ClientHello (draft-13)", TLSEXT_TYPE_ech}, | ||
| {"outer exts", TLSEXT_TYPE_outer_extensions}, | ||
| /* not an ECH change, but seems to be missing */ | ||
| {"early_data", TLSEXT_TYPE_early_data}, |
There was a problem hiding this comment.
is early_data dependent of ECH ?
Else the #endif should move up, no ?
There was a problem hiding this comment.
Not dependent, no. I just added that as it made reading logs easier when I was testing and kept it within the ifdef , but sure, moving it out is better so did that.
apps/s_client.c
Outdated
| @@ -2060,11 +2376,37 @@ int s_client_main(int argc, char **argv) | |||
| */ | |||
| SSL_CTX_set_session_cache_mode(ctx, SSL_SESS_CACHE_CLIENT | |||
| | SSL_SESS_CACHE_NO_INTERNAL_STORE); | |||
| BIO_printf(bio_err,"Setting new_session_cb\n"); | |||
There was a problem hiding this comment.
nit: space after comma.
doc/man1/openssl-s_client.pod.in
Outdated
|
|
||
| When not really doing Encrypted Client Hello (ECH), one can emit a so-called | ||
| GREASE value, which is essentially a random value in order to try ensure that | ||
| server code doesn't ossify. |
There was a problem hiding this comment.
I remember some documentation writing guidelines stating that :
- do not write using short forms like `don't' .
There was a problem hiding this comment.
Sure - reworded that. (The curl build actually has a linter that objects to that and other usage they don't like;-)
doc/man1/openssl-s_server.pod.in
Outdated
| =item B<-ech_dir> I<dir> | ||
|
|
||
| Load all new/modified Encrypted Client Hello (ECH) PEM files from the relevant directory. | ||
| A file is considered modified if it's modification time is one second or more younger |
There was a problem hiding this comment.
nit : its modification time.
doc/designs/ech-api.md
Outdated
| outer CH messages. | ||
|
|
||
| ```c | ||
| int ossl_ech_make_echconfig(unsigned char *echconfig, size_t *echconfiglen, |
There was a problem hiding this comment.
General remark: you are almost using only POD data type as method arguments types.
If I was you, i will replace all the pairs of (buffer, length) arguments
with a dedicated simple structure like this :
typedef struct binbuf_t {
size_t len;
unsigned char* data;
} BINBUF;
This will factorize and reduce the amount of "basic shores" code, imho.
Unless the project already have this kind of datatype, i will not use it publicly.
There was a problem hiding this comment.
Hmm - defining a BINBUF struct like the above seems like either a big change that ought be used in loads of places, or else creating an oddity for ECH. Given many other OpenSSL APIs use pairs of unsigned char * and a size_t to represent buffers, I'd say leaving this as-is is better. (Unless of course I'm misinterpreting your comment:-)
doc/man1/openssl-ech.pod.in
Outdated
| various forms and their components displayed. | ||
|
|
||
| The "ECHConfig PEM file" format mentioned below is specified in | ||
| L<https://datatracker.ietf.org/doc/draft-farrell-tls-pemesni/> and conists of |
ssl/statem/extensions_srvr.c
Outdated
|
|
||
| /* | ||
| * @brief answer a draft-13 ECH, as needed | ||
| * @param s is the SSL session |
There was a problem hiding this comment.
doc : better check each session mention as it seems that a rename/refactoring has occured with QUIC support
There was a problem hiding this comment.
yeah, hopefully I got all those now in the various files
| @@ -653,6 +680,13 @@ int tls_collect_extensions(SSL_CONNECTION *s, PACKET *packet, | |||
| SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, SSL_R_BAD_EXTENSION); | |||
| goto err; | |||
| } | |||
| #ifndef OPENSSL_NO_ECH | |||
There was a problem hiding this comment.
nit : useless #ifndef ... as there is no code inside.
There was a problem hiding this comment.
I think I convinced myself at some stage there may be a crash there and added the comment to remind me to check with some unsupported extension. For the moment, I've added a TODO to make such a test - could be I was wrong in the first place, or something's been fixed since.
ssl/ssl_lib.c
Outdated
| @@ -4098,6 +4185,13 @@ SSL_CTX *SSL_CTX_new_ex(OSSL_LIB_CTX *libctx, const char *propq, | |||
|
|
|||
| ssl_ctx_system_config(ret); | |||
|
|
|||
| #ifndef OPENSSL_NO_ECH | |||
| ret->ext.nechs = 0; | |||
There was a problem hiding this comment.
ah, some tab chars, fixed
ssl/ech_local.h
Outdated
| # define OSSL_ECH_SAME_EXT_CONTINUE 2 /* generate a new value for outer CH */ | ||
|
|
||
| /* | ||
| * During extension construction (in extensions_clnt.c |
There was a problem hiding this comment.
nit: you may format this block on 80 cols instead of 54 ?
Or there is a reason to keep it narrow ?
There was a problem hiding this comment.
no reason, re-formatted
| unsigned char *pub; | ||
| unsigned int nsuites; | ||
| ech_ciphersuite_t *ciphersuites; | ||
| unsigned int maximum_name_length; |
There was a problem hiding this comment.
'uint8' ( or uint8_t ?) is already used in current codebase.
There was a problem hiding this comment.
I think I had that at one stage but changed back for some reason (maybe one of the ports run in the workflows) but I don't recall. I've added a TODO to look again at the types used in ECHConfig and ECHConfigList ('cause as you say other structs do use uint16_t etc now).
There was a problem hiding this comment.
So I checked this out and I think those types are best left as-is. As a test I changed the version field to be a uint16_t but then that generates compile warnings such as the one below due to the PACKET APis dealing with unsigned ints. I'd argue that making such changes would therefore just add lots of casts that could be more error-prone and no real benefit, other than slightly smaller storage for structs which are not expected to be big in real use-cases anyway.
ssl/ech.c: In function 'ECHConfigList_from_binary':
ssl/ech.c:801:40: warning: passing argument 2 of 'PACKET_get_net_2' from incompatible pointer type [-Wincompatible-pointer-types]
801 | || !PACKET_get_net_2(&pkt, &ec->version)
| ^~~~~~~~~~~~
| |
| uint16_t * {aka short unsigned int *}
In file included from ssl/ssl_local.h:32,
from ssl/ech.c:11:
include/internal/packet.h:150:75: note: expected 'unsigned int *' but argument is of type 'uint16_t *' {aka 'short unsigned int *'}
150 | __owur static ossl_inline int PACKET_get_net_2(PACKET *pkt, unsigned int *data)
There was a problem hiding this comment.
I replaced the TODO I'd put in for this with a comment reflecting the above. Will push that next time.
doc/man3/SSL_ech_set1_echconfig.pod
Outdated
| SVCB and HTTPS resource records are defined in | ||
| L<https://datatracker.ietf.org/doc/draft-ietf-dnsop-svcb-https/>. | ||
|
|
||
| If an ECH public value is associated with an I<SSL> connecction, and if TLS version |
ssl/ech.c
Outdated
| * Strings used in ECH crypto derivations (odd format for EBCDIC goodness) | ||
| */ | ||
| /* "ech accept confirmation" */ | ||
| static char OSSL_ECH_ACCEPT_CONFIRM_STRING[] = "\x65\x63\x68\x20\x61\x63\x63\x65\x70\x74\x20\x63\x6f\x6e\x66\x69\x72\x6d\x61\x74\x69\x6f\x6e"; |
There was a problem hiding this comment.
yep probably should be so did that
| WPACKET_get_total_written(&hpkt, toklen); | ||
| *hrrtok = OPENSSL_malloc(*toklen); | ||
| if (*hrrtok == NULL) | ||
| return 0; |
There was a problem hiding this comment.
This return will create a leak of hpkt and hpkt_mem .
ssl/ech.c
Outdated
| OPENSSL_free(tbf->exts[i]); | ||
| OPENSSL_free(tbf->exts); | ||
| OPENSSL_free(tbf->encoding_start); | ||
| memset(tbf, 0, sizeof(ECHConfig)); |
There was a problem hiding this comment.
Do you really need to clear this content ?
And why this free() function don't free the passed object ?
There was a problem hiding this comment.
The memset() isn't really needed no, but did help debugging earlier.
As to why not free the passed object, it's because there can be an array of 'em, so the higher level code does that.
ssl/ech.c
Outdated
| for (i = 0; i != tbf->nrecs; i++) | ||
| ECHConfig_free(&tbf->recs[i]); | ||
| OPENSSL_free(tbf->recs); | ||
| memset(tbf, 0, sizeof(ECHConfigList)); |
There was a problem hiding this comment.
Your free() functions should really free his argument.
Else you are on way to a lot of confusion.
There was a problem hiding this comment.
Same reason as above - there can be an array, so higher level code frees that.
There was a problem hiding this comment.
In the current codebase, arrays are managed using the STACK_OF() api.
The STACK_OF(...)_pop_free() method will do the array memory release.
|
|
||
| # define OSSL_ECH_MAX_LINELEN 1000 /* for a sanity check */ | ||
|
|
||
| static char *echconfiglist_from_PEM(const char *echkeyfile) |
There was a problem hiding this comment.
You may add a second parameter to return the readbytes value.
There was a problem hiding this comment.
Could do, but as it returns a string, using strlen() on that seems ok, esp. for test code.
There was a problem hiding this comment.
I prefer to avoid calling strlen() when you already know/compute the result.
Many thanks for all those - given this code's been under development for quite a while, it's great to get more eyeballs on it to spot things like you've done above.
I'll look at that later if that's ok and push up the above changes first (shortly), then go over the various TODOs added in response to comments above. |
|
Commits above include all the obvious fixes from above and the TODOs mentioned. That does seem to have some kind of build problem with curl though so looking at that before the TODOs. |
| */ | ||
| int ech_2bcompressed(int ind) | ||
| { | ||
| int nexts = OSSL_NELEM(ech_ext_handling); |
| return OSSL_ECH_SAME_EXT_ERR; | ||
| type = ech_ext_handling[tind].type; | ||
| /* If this index'd extension won't be compressed, we're done */ | ||
| if (tind >= nexts) |
There was a problem hiding this comment.
If this condition is true, you already have an OOB read on previous line.
There was a problem hiding this comment.
well spotted! actually though that check (on 3259) is redundant as it was checked already on 3255 (just before the snippet quoted) so there's not OOB issue and I've removed the redundant check now.
|
|
||
| /* | ||
| * @brief Try figure out ECHConfig encodng by looking for telltales | ||
| * @param eklen is the length of rrval |
There was a problem hiding this comment.
it is a JavaDoc (or Doxygen ?) syntax to highlight any parameter or variable name.
|
|
||
| # define OSSL_ECH_MAX_LINELEN 1000 /* for a sanity check */ | ||
|
|
||
| static char *echconfiglist_from_PEM(const char *echkeyfile) |
There was a problem hiding this comment.
I prefer to avoid calling strlen() when you already know/compute the result.
| # include <internal/ech_helpers.h> | ||
| #endif | ||
|
|
||
| /* unused, to avoid warning. */ |
There was a problem hiding this comment.
a static variable who is unused should be removed, no ?
There was a problem hiding this comment.
That's a pattern in many of the fuzz/*.c files I just copied, e.g. here
ssl/ech.c
Outdated
| for (i = 0; i != tbf->nrecs; i++) | ||
| ECHConfig_free(&tbf->recs[i]); | ||
| OPENSSL_free(tbf->recs); | ||
| memset(tbf, 0, sizeof(ECHConfigList)); |
There was a problem hiding this comment.
In the current codebase, arrays are managed using the STACK_OF() api.
The STACK_OF(...)_pop_free() method will do the array memory release.
|
|
||
| /** | ||
| * @brief Free an OSSL_ECH_ENCCH | ||
| * @param tbf is a ptr to an SSL_ECH structure |
There was a problem hiding this comment.
did s/ev/tbf/ in the function prototype to fix that
Sorta, but maybe not;-) What I'm current doing is making changes in my "main" local branch (ECH-draft-13c) and then I have a script to update this PR from that. We also have a CI thing (here) that does a nightly merge with upstream and build for openssl and each of the web servers for which we've done PoC ECH integrations that is also fed from the ECH-draft-13c branch (manually, when interesting changes have happened or we need to fix something to handle merge issues with upstream). So it's easier for me, for now, to follow that workflow. When/if we get this PR closer to something that may be merged, then I guess it'll be better to switch to treating this branch as the "main" one, but I guess we're not there yet. |
|
@FdaSilvaYY said (in connection with how >1 ECHConfig is handled in structs...)
That's a fair point - I've not looked at using |
|
Question about GREASE : Is there any common parts with PR #19646 : |
Ah - didn't know about that one. GREASEing ECH is a little different (*) but there likely should be some commonality in APIs, especially as GREASEing, being a good idea to counter ossification, is being adopted as a mechanism by other protocols, (e.g. QUIC, MLS) so one could envisage general controls that'd work for more than just TLS or ECH. I'd be happy to collaborate on figuring that out if/when either PR is being handled. One possibility might be to move the ECH GREASEing API code to #19646 perhaps. (Search for GREASE in https://github.com/sftcd/openssl/blob/ECH-PR/doc/designs/ech-api.md for details.) (*) GREASEing ECH is a little different, as GREASE'd ECH, as well as countering ossification, could be considered to be a partial/potential mitigation against the risk that e.g. nation-state censors would block all ECH - once browsers commonly emit GREASE'd ECH extension values (which they do now), then that should somewhat dis-incent some censors from just blocking all ECH. That difference does justify a more complex API so that non-browser applications have the ability to mimic then-current ECH GREASEing behaviour of e.g. a browser in order to be less likely to have their use of ECH blocked. Note though that in all this we're dealing with risks that are hard-to-impossible to quantify at coding time. |
|
This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago |
|
This PR is in a state where it requires action by @openssl/otc but the last update was 61 days ago |
|
This PR is in a state where it requires action by @openssl/otc but the last update was 92 days ago |
This PR contains an implementation of Encrypted ClientHello (ECH) as specified in the IETF's TLS working group
Internet-draft. As noted in the minutes of a recent IETF meeting, the spec is on the cusp of working group last call, and should become an RFC in the new year. This PR responds to this issue.
The OpenSSL project's policy of course is to not merge such PRs until the RFC issues, which is entirely reasonable. In this case, unfortunately, the PR is (ahem;-) pretty BIG, so as review will likely take some time, I'm opening the PR now in the hope that the stars roughly align and the PR will be sufficiently reviewed and in a suitable state to be merged at about the time the RFC issues.
Development of this PR has been funded by the OTF in the DEfO project which is currently funded for another ~18 months, so we should be able to respond to reviews and continue development of this code until it's part of an OpenSSL release assuming all goes well. (Even if all doesn't go well and it takes longer, we'll still be about:-)
ECH is currently shipping in browsers and is no longer behind a flag. This code successfully interoperates with current browser releases. As part of the DEfO project, we have developed proof-of-concept integrations for ECH with curl, haproxy, apache2, nginx and lighttpd all of which seem to interoperate. This PR also supports the so-called "split-mode" for ECH and the nginx and haproxy integrations can make use of that. More details can be found on the DEfO web site. As the TLS WG have decided to register the ECH extension code-points that are currently in use in browsers and this code, we don't expect to see protocol changes that would affect interop between now and when the RFC issues.
Given that this is a pretty huge PR, I've tried to make it a little easier to review by grouping the changes into a smallish number of related commits, those are:
As posted, this PR passes almost all the CI build tests (there's one cross-compile error I don't understand, and some of the newer ABI diff stuff I'm not sure how to fix). I've not so far done code-coverage testing, as I expect that'll be better done when we're closer to being in a position to merge.
Apologies again that this is so big - we tried and failed to find ways to break it up into meaningful smaller PRs. It's probably just in the nature of the kind of change ECH represents that that's not really doable. I look forward to
responding to any and all review comments as this PR is reviewed.
Checklist