OpenSSL ECH integration #551

sftcd · 2025-08-18T17:10:03Z

This PR adds Encrypted Client Hello (ECH) functionality to apache2, when using OpenSSL for TLS.

Notes:

ECH is not yet part of an OpenSSL release. We'd hope ECH will be part of OpenSSL 4.0 in April 2026. However, we have been working with OpenSSL maintainers on the so-called "ECH feature branch" and that branch (subject to the same OpenSSL maintainer approval process as the OpenSSL master branch) now includes sufficient ECH code for web servers like apache2. So there's plenty of time for this PR to be discussed, but starting now may be timely.
This PR includes documentation in a markdown document in the repo's top directory, which is certainly the wrong place, but may be useful short-term. That describes how to do the build, configuration and logging changes, and the code changes for ECH. (So should be a good place for reviewers to start.)
While OpenSSL releases do not yet include ECH support, some other TLS libraries do, in particular boringssl. If useful, we could extend this PR to also support boringssl, or that could be a follow-up. (It'd be good if the server configuration were the same regardless of the TLS library.)
ECH support using these OpenSSL ECH APIs was included in the ligthttpd web server (in January 2025) so some code and patterns are common with that. We also plan to submit similar PRs to apache2 and haproxy, and ideally all would share some commonality.
We have proposed a very similar PR for the NGINX project (OpenSSL ECH integration nginx/nginx#840).
All that said, we're not fixated at all on things being done this way, and would be happy to make whatever changes are desired for apache2 and there are some notes on potential changes in the documentation.

Lastly, for open-ness, our work on this has been funded by the Open Technology Fund (OTF) in the DEfO project.

notroj · 2025-08-19T08:49:32Z

Thanks for the PR, have not reviewed but wanted to try hooking it up in CI: https://github.com/notroj/httpd/actions/runs/17064056660/job/48376952186

rpluem · 2025-08-19T09:06:32Z

First short style nitpick: Please take care to put spaces around operands like =, == and != when doing tests or assignments.

sftcd · 2025-08-19T09:07:36Z

thanks for the CI setup! and that of course found a thing that's hopefully fixed in the above push - if there're more will similarly fix as they come

sftcd · 2025-08-19T09:08:06Z

First short style nitpick: Please take care to put spaces around operands like =, == and != when doing tests or assignments.

will go over those in a bit, thanks

rpluem · 2025-08-19T09:08:29Z

modules/ssl/ssl_engine_init.c

+                "load_echkeys: no directory name - exiting");
+        return -1;
+    }
+    size_t elen=strlen(echdir);


We still try to keep some C89 compatibility for older compilers on more exotic platforms. Hence please move the declaration of elen at the top of the function.

sorry, doing that kind of tidying up now generally

rpluem · 2025-08-19T09:11:23Z

modules/ssl/ssl_engine_config.c

    sc->clienthello_vars       = UNSET;
    sc->session_tickets        = UNSET;
+#ifdef HAVE_OPENSSL_ECH
+    sc->echkeydir             = NULL;


Whitespace Nit:

Suggested change

sc->echkeydir = NULL;

sc->echkeydir = NULL;

rpluem · 2025-08-19T09:23:30Z

Can you please also add some documentation for the new directive SSLECHKeyDir to docs/manual/mod/mod_ssl.xml?
Can you please also add a new txt file to the `changes-entries directory that describes the change?

rpluem · 2025-08-19T09:29:10Z

modules/ssl/ssl_engine_config.c

+    SSLSrvConfigRec *sc = mySrvConfig(cmd->server);
+
+    sc->echkeydir=arg;
+    ap_log_error(APLOG_MARK, APLOG_TRACE4, 0, cmd->server, APLOGNO(10519)


For APLOG_TRACE? logging we don't use APLOGNOs.

Suggested change

ap_log_error(APLOG_MARK, APLOG_TRACE4, 0, cmd->server, APLOGNO(10519)

ap_log_error(APLOG_MARK, APLOG_TRACE4, 0, cmd->server,

ack, doing that generally

rpluem · 2025-08-19T09:41:30Z

Please also document the SSL_ECH_ environment variables in docs/manual/mod/mod_ssl.xml.

sftcd · 2025-08-19T15:32:45Z

push above should tidy the code/address nits; documentation requested still TBD but coming soon:-)

rpluem · 2025-08-19T19:10:42Z

push above should tidy the code/address nits; documentation requested still TBD but coming soon:-)

Thanks. This just leaves the last two comments to modules/ssl/ssl_engine_kernel.c open (move code to ssl_engine_vars.c and loglevel discussion).

sftcd · 2025-08-20T00:45:49Z

In the push above, I took a stab at adding documentation in the right place (docs/manual/mod/mod_ssl.xml) and building the resulting HTML which sort-of seems ok, but likely needs change. I've left the ECH-build.md file in the PR for now, as that still contains information that's not elsewhere at the moment. (We can ditch that once there's nothing left of value in the markdown file.)

sftcd · 2025-08-20T00:46:48Z

push above should tidy the code/address nits; documentation requested still TBD but coming soon:-)

Thanks. This just leaves the last two comments to modules/ssl/ssl_engine_kernel.c open (move code to ssl_engine_vars.c and loglevel discussion).

Apologies for being slow (I often am:-) but I'm not seeing where those "last two comments" are - can you point me at them when you have a chance? Thanks.

rpluem · 2025-08-20T06:35:02Z

In the push above, I took a stab at adding documentation in the right place (docs/manual/mod/mod_ssl.xml) and building the resulting HTML which sort-of seems ok, but likely needs change. I've left the ECH-build.md file in the PR for now, as that still contains information that's not elsewhere at the moment. (We can ditch that once there's nothing left of value in the markdown file.)

Thanks. You can remove the HTML changes from PR as these will be created in a step separately from the commit. Just the changes to the xml file are required.

rpluem · 2025-08-20T06:39:43Z

push above should tidy the code/address nits; documentation requested still TBD but coming soon:-)

Thanks. This just leaves the last two comments to modules/ssl/ssl_engine_kernel.c open (move code to ssl_engine_vars.c and loglevel discussion).

Apologies for being slow (I often am:-) but I'm not seeing where those "last two comments" are - can you point me at them when you have a chance? Thanks.

I just readded the comments to the latest version of the code as I was not sure how I should point you to them.

sftcd · 2025-08-20T16:46:26Z

If I've done it right the (force) push above just removes the documentation files that I shouldn't have included yesterday and make no other change

sftcd · 2025-08-20T16:51:35Z

I just readded the comments to the latest version of the code as I was not sure how I should point you to them.

I've no idea why, but I'm not seeing any comments at all on ssl_engine_kernel.c sorry

rpluem · 2025-08-21T06:29:49Z

I just readded the comments to the latest version of the code as I was not sure how I should point you to them.

I've no idea why, but I'm not seeing any comments at all on ssl_engine_kernel.c sorry

Very strange. I thought that my comments might get removed after another commit or on a force push, but I still can see them on the files tab. There are currently 4 open.

rpluem · 2025-08-19T09:30:59Z

modules/ssl/ssl_engine_init.c

+            const int is_retry_config = OSSL_ECH_FOR_RETRY;
+            if (in != NULL
+                && 1 == OSSL_ECHSTORE_read_pem(es, in, is_retry_config)) {
+                ap_log_error(APLOG_MARK, APLOG_TRACE4, 0, s, APLOGNO(10524)


For APLOG_TRACE? logging we don't use APLOGNOs.

Suggested change

ap_log_error(APLOG_MARK, APLOG_TRACE4, 0, s, APLOGNO(10524)

ap_log_error(APLOG_MARK, APLOG_TRACE4, 0, s,

yep fixed that

rpluem · 2025-08-19T09:31:47Z

modules/ssl/ssl_engine_init.c

+    if (sc!=NULL && sc->echkeydir!=NULL) {
+        SSL_CTX_ech_set_callback(mctx->ssl_ctx, ssl_callback_ECH);
+    } else {
+        ap_log_error(APLOG_MARK, APLOG_TRACE4, 0, s, APLOGNO(10530)


For APLOG_TRACE? logging we don't use APLOGNOs.

Suggested change

ap_log_error(APLOG_MARK, APLOG_TRACE4, 0, s, APLOGNO(10530)

ap_log_error(APLOG_MARK, APLOG_TRACE4, 0, s,

also fixed that

rpluem · 2025-08-19T09:32:27Z

modules/ssl/ssl_engine_init.c

+    int keystried=0;
+    int keysworked=0;
+
+    apr_dir_t *dir;
+    apr_finfo_t direntry;
+    apr_int32_t finfo_flags = APR_FINFO_TYPE|APR_FINFO_NAME;


Same as above regarding C89.

should be fixed now too

rpluem · 2025-08-19T09:35:15Z

modules/ssl/ssl_engine_init.c

+    }
+    apr_dir_close(dir);
+
+    int keysloaded=0;


Same as above regarding C89.

rpluem · 2025-08-19T09:39:46Z

modules/ssl/ssl_engine_kernel.c

+    char *outer_sni=NULL;
+    char buf[PATH_MAX];
+    memset(buf,0,PATH_MAX);
+    int echrv=SSL_ech_get1_status((SSL*)ssl,&inner_sni,&outer_sni);


Same as above regarding C89.

rpluem · 2025-08-19T09:55:27Z

modules/ssl/ssl_engine_kernel.c

+        ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, c, APLOGNO(10534)
+            "ECH not attempted");
+        break;
+    case SSL_ECH_STATUS_FAILED:
+        ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, c, APLOGNO(10535)
+            "ECH tried but failed");
+        break;
+    case SSL_ECH_STATUS_BAD_NAME:
+        ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, c, APLOGNO(10536)
+            "ECH worked but bad name");
+        break;
+    case SSL_ECH_STATUS_SUCCESS:
+        ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, c, APLOGNO(10537)
+            "ECH success outer_sni: %s inner_sni: %s",
+            (outer_sni?outer_sni:"NONE"),(inner_sni?inner_sni:"NONE"));
+        break;
+    default:
+        ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, c, APLOGNO(10538)


I think these should be DEBUG and not INFO messages.

sure, happy to take guidance on what level's best, so changed them to APLOG_DEBUG

rpluem · 2025-08-20T06:36:50Z

modules/ssl/ssl_engine_kernel.c

+#ifdef HAVE_OPENSSL_ECH
+    /* Add the ECH information to the environment */
+    memset(buf, 0, PATH_MAX);
+    echrv = SSL_ech_get1_status((SSL*)ssl, &inner_sni, &outer_sni);
+    switch (echrv) {
+    case SSL_ECH_STATUS_NOT_TRIED:
+        snprintf(buf, PATH_MAX, "not attempted");
+        break;
+    case SSL_ECH_STATUS_FAILED:
+        snprintf(buf, PATH_MAX, "tried but failed");
+        break;
+    case SSL_ECH_STATUS_BAD_NAME:
+        snprintf(buf, PATH_MAX, "ECH worked but bad name");
+        break;
+    case SSL_ECH_STATUS_SUCCESS:
+        snprintf(buf, PATH_MAX, "success");
+        break;
+    default:
+        snprintf(buf, PATH_MAX, "error getting ECH status");
+    }
+    apr_table_set(env, "SSL_ECH_INNER_SNI", (inner_sni ? inner_sni : "NONE"));
+    apr_table_set(env, "SSL_ECH_OUTER_SNI", (outer_sni ? outer_sni : "NONE"));
+    apr_table_set(env, "SSL_ECH_STATUS", buf);
+#endif
+


This whole stuff belongs to ssl_engine_vars.c. You should only amend ssl_hook_Fixup_vars here.

so I've tried to do that, but not sure I'm understanding the reference to ssl_hook_Fixup_vars so might've done it wrong, but it should be closer at least. I also need to run this version in a test setup with PHP, which I've not done yet, (will later today, when on a different n/w where that's easier for me), so I might have more to change when I do that.

Currently it is not the way I would like to see it, but the way I told you it should look like is probably a bit wrong as well :-). The core thing I would like to get to is that the 3 new variables can be queried through the central ssl_var_lookup function from ssl_engine_vars.c to enable other possible consumers of these variables to get its values. Once this is possible you can add them to the environment in ssl_engine_kernel.c::ssl_hook_Fixup via

#ifdef HAVE_OPENSSL_ECH extract_to_env(r, env, "SSL_ECH_INNER_SNI"); extract_to_env(r, env, "SSL_ECH_OUTER_SNI"); extract_to_env(r, env, "SSL_ECH_STATUS_SNI"); #endif

You would implement your extraction in ssl_engine_vars.c::ssl_var_lookup_ssl and would add cases for ECH_INNER_SNI, ECH_OUTER_SNI and ECH_STATUS. The SSL_ prefix gets already striped by ssl_engine_vars.c::ssl_var_lookup when it calls ssl_engine_vars.c::ssl_var_lookup_ssl. Keep in mind that you have to apr_pstrdup() the strings in this case here before returning them. For example have a look how SSL_SESSION_RESUMED or better SESSION_RESUMED is currently implemented (

httpd/modules/ssl/ssl_engine_vars.c

Line 468 in f09577a

else if(ssl != NULL && strcEQ(var, "SESSION_RESUMED")) {

).

rpluem · 2025-08-20T06:38:23Z

modules/ssl/ssl_engine_kernel.c

+    case SSL_ECH_STATUS_NOT_TRIED:
+        ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, c, APLOGNO(10534)
+            "ECH not attempted");
+        break;
+    case SSL_ECH_STATUS_FAILED:
+        ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, c, APLOGNO(10535)
+            "ECH tried but failed");
+        break;
+    case SSL_ECH_STATUS_BAD_NAME:
+        ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, c, APLOGNO(10536)
+            "ECH worked but bad name");
+        break;
+    case SSL_ECH_STATUS_SUCCESS:
+        ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, c, APLOGNO(10537)
+            "ECH success outer_sni: %s inner_sni: %s",
+            (outer_sni ? outer_sni : "NONE"),
+            (inner_sni ? inner_sni : "NONE"));
+        break;
+    default:
+        ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, c, APLOGNO(10538)
+            "Error getting ECH status");


I think these should be DEBUG and not INFO messages.

yep, changed to that

rpluem · 2025-08-20T06:42:36Z

changes-entries/ech.txt

@@ -0,0 +1,3 @@
+
+  *) mod_ssl: Add support for Encrypted Client Hello (ECH)
+     Github #551 [Stephen Farrell]


People who are not committers to this project are typically credited with name and email address (unless you don't want to give yours), hence a pattern like [John Doe [email protected]].

no problem doing that so I added an email

rpluem · 2025-08-20T06:46:36Z

docs/manual/mod/mod_ssl.xml

+<description>Load the set of Encrypted Client Hello (ECH) PEM files in the named directory</description>
+<syntax>SSLECHKeyDir <em>dirname</em></syntax>
+<contextlist><context>server config</context></contextlist>
+<compatibility>Available in httpd 2.X.0 and later</compatibility>


Suggested change

<compatibility>Available in httpd 2.X.0 and later</compatibility>

<compatibility>Available in Apache HTTP Server 2.5.1 and later</compatibility>

sure, also tweaked the example using dig

rpluem · 2025-08-21T06:31:28Z

I think I know what I did wrong: I forgot to submit my review and hence it was only visible to me.

notroj · 2025-08-21T08:00:47Z

Updated CI run https://github.com/notroj/httpd/actions/runs/17120295279/job/48559395252#step:11:2461

rpluem · 2025-08-21T09:22:54Z

modules/ssl/ssl_engine_init.c

+    while ((apr_dir_read(&direntry, finfo_flags, dir)) == APR_SUCCESS) {
+        const char *fname;
+        size_t pnlen = 0;
+        struct stat thestat;


Suggested change

struct stat thestat;

sftcd · 2025-08-21T11:10:35Z

I think I know what I did wrong: I forgot to submit my review and hence it was only visible to me.

Thanks for those, I'll process 'em shortly. (And am relieved I wasn't doing something wrong:-)

sftcd · 2025-08-21T13:16:43Z

The push above tries to address today's @rpluem comments.

notroj · 2025-08-26T16:22:31Z

I still got permission denied trying to push to sftcd:ECH-shared via ssh so maybe there is some other restriction here, oh well.

Latest run: https://github.com/notroj/httpd/actions/runs/17236024872

notroj · 2025-08-26T16:31:31Z

modules/ssl/ssl_engine_init.c

+    apr_finfo_t direntry;
+    apr_int32_t finfo_flags = APR_FINFO_TYPE|APR_FINFO_NAME;
+
+    if (echdir == NULL) {


Checking for echdir!=NULL is redundant since the calling function already checks that, so you can document it as a constraint for the interface.

happy to do that if that's the local preference

notroj · 2025-08-26T16:33:01Z

modules/ssl/ssl_engine_init.c

+                "load_echkeys: directory name too long: %s - exiting", echdir);
+        return -1;
+    }
+    if (!echdir || (apr_dir_open(&dir, echdir, ptemp) != APR_SUCCESS)) {


If apr_dir_open fails catch the returned apr_status_t and pass it to the ap_log_error so the actual error is logged

notroj · 2025-08-26T16:41:46Z

modules/ssl/ssl_engine_init.c

+#else
+    if (sc->echkeydir) {
+        ap_log_error(APLOG_MARK, APLOG_WARN, 0, s, APLOGNO(10533)
+                 "ECHKeyDir configured but no TLSv1.3 so ECH will be ignored.");


IMO if TLSv1.3 is not supported by the TLS toolkit, using SSLECHKeyDir should be a config-time error, so merely warning for this is not sufficient. So in the definition of HAVE_OPENSSL_ECH in ssl_private.h check that SSL_HAVE_PROTOCOL_TLSV1_3 definition too, and drop this case?

Alternative: in ssl_cmd_SSLECHKeyDir check that SSL_HAVE_PROTOCOL_TLSV1_3 is defined and if not always return an error string - this would give better feedback than simply getting an error that the directive isn't supported.

Hmm. I agree having some cases handled as warnings (as above) and others as fatal is a bit off and all should be handled at the same error level. (For the moment I'll do s/WARN/EMERG/ just to be consistent and we can fix better based on the conclusion of this point.)

I guess one could take either of two positions though:

ECH is an optional privacy improvement, already vulnerable to some active attacks (on the DNS), and (some) clients/browsers might in any case fallback to non-ECH (in a 2nd connection) so logging but continuing in the face of misconfiguration is better, or,

if ECH is configured on and the server can't actually do ECH, then that should be treated as fatal so the server admin gets to know about and fix things (which could be the server config, or the server s/w etc.)

I could be convinced either way;-)

Is there any guidance from how other misconfigurations are handled?

Having thought about that, I figure the better thing to do is number 2 above - treat all these errors as fatal and don't start the server (so use APLOG_EMERG) - what convinced me was that if you start an old server that doesn't have any ECH support, and that sees an SSLECHKeyDir directive then that old server will fail to start, so doing likewise in these cases seems correct. So I think the code now in the PR does the right thing.

@notroj - is there a reason to do this in ssl_cmd_SSLECHKeyDir or is it ok as-is?

Not sure it's written down as guidance but yes your (2) is generally preferred, if the configuration cannot be applied, fail at config time. It's better to do it in ssl_cmd_SSLECHKeyDir so that httpd -t will fail for the configuration.

ah right, will make that change so

Just pushed a change for that, let me know if more needed - the commit is here

ylavic · 2025-08-26T16:53:20Z

modules/ssl/ssl_engine_init.c

+    es = OSSL_ECHSTORE_new(NULL, NULL);
+    if (es == NULL) {
+        ap_log_error(APLOG_MARK, APLOG_EMERG, 0, s, APLOGNO(10523)
+                "load_echkeys: can't alloc store");


apr_dir_close(dir);?

ylavic · 2025-08-26T16:53:26Z

modules/ssl/ssl_engine_init.c

+
+    if (!OSSL_ECHSTORE_num_keys(es, &keysloaded)) {
+        ap_log_error(APLOG_MARK, APLOG_EMERG, 0, s, APLOGNO(10526)
+            "OSSL_ECHSTORE_num_keys failed - exiting");


OSSL_ECHSTORE_free(es);?

ylavic · 2025-08-26T16:53:35Z

modules/ssl/ssl_engine_init.c

+    } else {
+        ap_log_error(APLOG_MARK, APLOG_TRACE4, 0, s,
+            "ECHKeyDir not set - using ClientHello callback for SNI");
+        SSL_CTX_set_client_hello_cb(mctx->ssl_ctx, ssl_callback_ClientHello, NULL);


This } else { looks unnecessary if you leave the SSL_CTX_set_client_hello_cb() below out of #else /* HAVE_OPENSSL_ECH */?

But shouldn't we set only one, SSL_CTX_ech_set_callback or SSL_CTX_set_client_hello_cbbut not both at the same time?

Answering myself: Scratch the above. You answered it to me in a later comment and why we do SSL_CTX_set_client_hello_cb since 076e283 if possible. Hence we should always call SSL_CTX_set_client_hello_cb independent of HAVE_OPENSSL_ECH.

So I guess neither ssl_callback_ECH() nor ssl_callback_ClientHello() are that well named - something like ssl_callback_set_vhost_from_[ClientHello|ECH]() might be better. But that said, I'm not sure if the #else is better left in - will get back when I've re-checked which callback fires when in which cases.

So I added a new ECH test to my current PR for the OpenSSL feature branch that basically mimics what ssl_callback_ClientHello() does.

The result is: the client_hello callback fires first, and has the inner CH, after ECH decryption has worked, so all should be well regardless of which callback is used. The ECH callback doesn't fire until the ServerHello message is being prepared (IIRC it has to be that way in case of HRR, as the ECH callback shouldn't fire until HRR and ECH have both succeeded.)

So I guess we could get rid of ssl_callback_ECH() entirely, but I wonder what's the correct thing for the server to do when HRR happens?

I did a test build getting rid of ssl_callback_ECH() and just using ssl_callback_ClientHello() and it seems to work fine, even though the callback is hit twice when we hit HRR, but I didn't seen any bad side-effects of that, and it's what happens already so is presumably ok. I guess that's less change so unless there's a reason to keep ssl_callback_ECH() I'll make that change in a while.

Oops - we still want ssl_callback_ECH() to log the ECH outcome, but the vhost stuff can now be left to the CH callback.

push below (50e1f81) makes these changes

it seems to work fine, even though the callback is hit twice when we hit HRR, but I didn't seen any bad side-effects of that, and it's what happens already so is presumably ok.

If ssl_callback_ClientHello() can be called multiple times with HRR, maybe we should not let its init_vhost() be a noop for the second time (should the SNI be chang{able,ed} in between..)? Maybe for another PR/commit though since it's not really related to ECH.

Oops - we still want ssl_callback_ECH() to log the ECH outcome, but the vhost stuff can now be left to the CH callback.

Agreed.

If ssl_callback_ClientHello() can be called multiple times with HRR, maybe we should not let its init_vhost() be a noop for the second time (should the SNI be chang{able,ed} in between..)? Maybe for another PR/commit though since it's not really related to ECH.

Agree that that'd be for a separate PR. I didn't notice any leak in the HRR case via valgrind fwiw. The TLSv1.3 spec says how the 2nd CH can differ from the first (at https://datatracker.ietf.org/doc/html/rfc8446#section-4.1.2) and there's some text too about that in the ECH spec (as the 2nd CH uses the same HPKE context). Those rules do not allow for changing the SNI between the 1st and 2nd CH's, but of course a bad client might cheat. I'm not sure how or if different TLS libraries make those checks, nor whether there'd be information on that available to applications. When the OpenSSL client hello callback fires, I don't believe you'd know if HRR will happen or not though as the CH hasn't even been parsed, so the library won't have noticed that the key shares are such as to cause HRR. If there's some setup where the init_vhost() can cause side-effects then I guess this would be worth trying to better handle, but I don't know if that's the case.

Sounds like creating an issue for httpd for this is probably right? I can do that but it might be better done by one of you who understand the server code better. (let me know)

sftcd · 2025-08-26T21:11:32Z

I still got permission denied trying to push to sftcd:ECH-shared via ssh so maybe there is some other restriction here, oh well.

That's odd. Maybe if one of you close then re-open the PR that'd do something? Or perhaps you don't care:-) Either's fine by me.

sftcd · 2025-08-26T23:30:29Z

push above has changes from various comments today, with two remaining things to for me to check/to-discuss - how best to handle misconfiguration and the ordering of callbacks (I'll do the checking Wed sometime and get back)

modules/ssl/ssl_engine_init.c

correct tag Co-authored-by: Ruediger Pluem <[email protected]>

better message Co-authored-by: Ruediger Pluem <[email protected]>

modules/ssl/ssl_engine_init.c

space policed Co-authored-by: Ruediger Pluem <[email protected]>

sftcd · 2025-08-27T20:05:19Z

push above simplifies the use of callbacks as per discussion above

notroj · 2025-08-28T07:15:38Z

https://github.com/notroj/httpd/actions/runs/17288258105/job/49069552136#step:11:2463

ssl_engine_kernel.c: In function 'ssl_callback_ECH':
ssl_engine_kernel.c:2396:18: error: unused variable 'ivstatus' [-Werror=unused-variable]
 2396 |     apr_status_t ivstatus;
      |                  ^~~~~~~~
cc1: all warnings being treated as errors

sftcd · 2025-08-28T08:27:41Z

push above removes ivstatus (thanks for re-doing the CI run)

ylavic · 2025-08-28T10:09:23Z

modules/ssl/ssl_engine_kernel.c

+        ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, c, APLOGNO(10541)
+                      "there is NO ECH extension");
+    }
+#endif


Not sure this is needed since ssl_callback_ECH() will log the status anyway?
And I don't think we want this to be logged at APLOG_INFO for every connection, APLOG_DEBUG/TRACEn would be enough IMHO.

yeah, that's fair - I guess I added that while figuring stuff out;-)

it's removed in the push below

rpluem · 2025-09-05T07:29:48Z

@notroj : Can you start another probably final CI build? I think all questions have been addressed.

notroj · 2025-09-08T09:29:44Z

Sorry I did already forgot to mention - all green! https://github.com/notroj/httpd/actions/runs/17299086256

notroj · 2025-09-12T08:06:43Z

Possibly the most reviewed PR ever... I merged this with minor syntax/style changes only. I didn't include the new doc file but included the relevant parts of that text in the commit message and I think most of the rest is covered in the docs.

Many thanks @sftcd !

sftcd · 2025-09-12T11:24:18Z

That's great thanks! It was a pleasure getting/handling those comments. And do ping if/as ECH issues arise, happy to help out as I can.

ECH collected changes

e1cd750

fixup! ECH collected changes

965ffb9

rpluem reviewed Aug 19, 2025

View reviewed changes

fixup! ECH collected changes

74a6e66

fixup! ECH collected changes

329f641

sftcd force-pushed the ECH-shared branch from c8a903d to 329f641 Compare August 20, 2025 16:45

rpluem reviewed Aug 21, 2025

View reviewed changes

rpluem suggested changes Aug 21, 2025

View reviewed changes

fixup! ECH collected changes

43e8b70

notroj reviewed Aug 26, 2025

View reviewed changes

ylavic reviewed Aug 26, 2025

View reviewed changes

fixup! ECH collected changes

6e55d05

rpluem suggested changes Aug 27, 2025

View reviewed changes

modules/ssl/ssl_engine_init.c Outdated Show resolved Hide resolved

modules/ssl/ssl_engine_init.c Outdated Show resolved Hide resolved

sftcd and others added 2 commits August 27, 2025 14:34

Update modules/ssl/ssl_engine_init.c

240e69c

correct tag Co-authored-by: Ruediger Pluem <[email protected]>

Update modules/ssl/ssl_engine_init.c

47d7015

better message Co-authored-by: Ruediger Pluem <[email protected]>

rpluem suggested changes Aug 27, 2025

View reviewed changes

modules/ssl/ssl_engine_init.c Outdated Show resolved Hide resolved

sftcd mentioned this pull request Aug 27, 2025

s_client and s_server command line options for ECH (plus some wndows CI fixes) openssl/openssl#28270

Closed

2 tasks

sftcd and others added 3 commits August 27, 2025 15:31

Update modules/ssl/ssl_engine_init.c

7ccc493

space policed Co-authored-by: Ruediger Pluem <[email protected]>

fixup! ECH collected changes

5a4db6c

fixup! ECH collected changes

50e1f81

sftcd mentioned this pull request Aug 27, 2025

ECH (Encrypted client hello) support haproxy/haproxy#1924

Closed

fixup! ECH collected changes

01b86ba

ylavic reviewed Aug 28, 2025

View reviewed changes

fixup! ECH collected changes

338e6eb

asf-gitbox-commits closed this in 0c9cd09 Sep 12, 2025

vincejv mentioned this pull request Nov 16, 2025

Enable TLS 1.3 sfackler/rust-native-tls#235

Open

	ap_log_error(APLOG_MARK, APLOG_TRACE4, 0, cmd->server, APLOGNO(10519)
	ap_log_error(APLOG_MARK, APLOG_TRACE4, 0, cmd->server,

		@@ -0,0 +1,3 @@

		*) mod_ssl: Add support for Encrypted Client Hello (ECH)
		Github #551 [Stephen Farrell]

	<compatibility>Available in httpd 2.X.0 and later</compatibility>
	<compatibility>Available in Apache HTTP Server 2.5.1 and later</compatibility>

OpenSSL ECH integration #551

OpenSSL ECH integration #551

Uh oh!

Conversation

sftcd commented Aug 18, 2025

Uh oh!

notroj commented Aug 19, 2025

Uh oh!

rpluem commented Aug 19, 2025

Uh oh!

sftcd commented Aug 19, 2025

Uh oh!

sftcd commented Aug 19, 2025

Uh oh!

rpluem Aug 19, 2025 • 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!

rpluem Aug 19, 2025 • 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!

rpluem commented Aug 19, 2025

Uh oh!

rpluem Aug 19, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

rpluem commented Aug 19, 2025

Uh oh!

sftcd commented Aug 19, 2025

Uh oh!

rpluem commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sftcd commented Aug 20, 2025

Uh oh!

sftcd commented Aug 20, 2025

Uh oh!

rpluem commented Aug 20, 2025

Uh oh!

rpluem commented Aug 20, 2025

Uh oh!

sftcd commented Aug 20, 2025

Uh oh!

sftcd commented Aug 20, 2025

Uh oh!

rpluem commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

rpluem Aug 19, 2025 •

edited

Loading

rpluem Aug 19, 2025 •

edited

Loading

rpluem Aug 19, 2025 •

edited

Loading

rpluem commented Aug 19, 2025 •

edited

Loading

rpluem commented Aug 21, 2025 •

edited

Loading