Conversation
|
Excellent work. ECH would be very useful in nginx. My biggest concern with proposed approach is that this integration makes it virtually impossible to use nginx ECH with other TLS libraries such as BoringSSL or AWS-LC. They have no notion of ECHCONFIG PEM format. I would strongly recommend to have a simple configuration similar to |
|
Thanks for taking a look!
That's not correct. Lighttpd supports both OpenSSL and boring with this format. That's s simple enough shim to handle the file format at the application layer. See here for the code. I'd be happy to do similarly for nginx should boring support be considered a requirement for this PR. (Which'd not be unreasonable.)
I really don't think we want an ECH Lastly, the apache2 maintainers seemed fine with this aspect in the comments they've made on the almost equivalent PR to this one. |
Thanks for the correction! Indeed, not hardcoding |
|
push above includes the obvious things from @yaroslavros comments today |
|
As an FYI, the corresponding apache httpd functionality has been committed there: apache/httpd@0c9cd09 |
Indeed we do need BoringSSL support. Also, reading a PEM file in nginx is only a half of the problem. The other half is how to create those files with BoringSSL? |
|
Regarding the code, @sftcd could you please follow the nginx code style, see https://github.com/nginx/nginx/blob/master/CONTRIBUTING.md for details. |
src/event/ngx_event_openssl.c
Outdated
| 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, "worked but bad name"); | ||
| break; | ||
| case SSL_ECH_STATUS_SUCCESS: | ||
| snprintf(buf, PATH_MAX, "success"); | ||
| break; | ||
| case SSL_ECH_STATUS_GREASE: | ||
| snprintf(buf, PATH_MAX, "GREASEd ECH"); | ||
| break; | ||
| case SSL_ECH_STATUS_BACKEND: | ||
| snprintf(buf, PATH_MAX, "Backend/inner ECH"); | ||
| break; | ||
| default: | ||
| snprintf(buf, PATH_MAX, "error getting ECH status"); | ||
| break; |
There was a problem hiding this comment.
I'm not sure PATH_MAX is related to these values. Moreover, I suggest short upper-case status strings (NOT_TRIED, FAILED etc), see ngx_ssl_get_client_verify() for example.
There was a problem hiding this comment.
The values do not fully match, eg SSL_ECH_STATUS_BAD_NAME -> WORKED_BAD_NAME, SSL_ECH_STATUS_GREASE -> GREASED.
Also, could you please explain what the SSL_ECH_STATUS_BACKEND status means. I looked into the code and it looks like the flag only makes sense in split mode.
|
@arut: happy to make changes along the above lines (maybe tomorrow before I get time to start that), but I've a question first: over the last week or so, the freenginx project has added ECH support taking a slightly different approach to what I did with this PR, the main difference is that they chose to configure file names for the ECH PEM files rather than a directory name (from which those ECH PEM files are read) as is done here. I don't know how or if nginx and freenginx try to maintain config file or other compatibility, but probably good if you tell me that you prefer following the file name or directory name approach before we go much further. Some details can be seen here |
Totally reasonable. If it's ok I'd suggest trying to agree how to use the OpenSSL ECH APIs first, then add a shim to make the same thing work with boring (or AWS-LC). That'd be simpler for me at least:-) Anyway, I'd be happy to follow such a plan.
That's easy enough. I knocked up a small bash script that shows how. |
Had tried to, and now have tried again (seeing that I'd missed some). Might take a few goes to be quite right, but happy to get there. Next push will include that. |
|
push above tries to address @arut comments above, modulo when to do what about boring etc. (and of course my mistakes in trying to do the right thing:-) |
arut
left a comment
There was a problem hiding this comment.
Also don't forget to do all this for the Stream module as well.
src/event/ngx_event_openssl.h
Outdated
| /* check defines from <openssl/ssl.h> for ECH support */ | ||
| #if !defined(SSL_OP_ECH_GREASE) | ||
| #define OPENSSL_NO_ECH | ||
| #endif | ||
| #ifndef OPENSSL_NO_ECH | ||
| #include <openssl/ech.h> | ||
| #endif |
There was a problem hiding this comment.
Why do we need this block? As I see, openssl/ech.h is included automatically by openssl/ssl.h.
Regarding checking OPENSSL_NO_ECH in nginx code for conditional compilation. Since ECH may not be available at all, relying solely on this macro is not a good idea. Modifying this macro is not a good idea either. We already have one feature that's compiled conditionally and may not be available at all, which is OCSP stapling. Here's how this problem is solved for OCSP. In the case of ECH, the solution would look like this:
#if (!defined OPENSSL_NO_ECH && defined SSL_OP_ECH_GREASE)
...
#endif
Now this brings up another problem. In your change there are multiple conditional blocks with #ifndef OPENSSL_NO_ECH and using the condition above would significantly complicate the code, especially if we add BoringSSL ECH at some point. The solution is to minimize the number of those #if's. And again, stapling is a good example. What we need to do is to try to move all conditional compilation to ngx_event_openssl.c. I will comment the code below for more details.
There was a problem hiding this comment.
Ok, aiming for that now.
There was a problem hiding this comment.
As I mentioned before, I don't think it's a good idea to define/redefine OpenSSL macros like OPENSSL_NO_ECH in nginx code. I suggest using the condition #if (!defined OPENSSL_NO_ECH && defined SSL_OP_ECH_GREASE) instead and regrouping the code in a way that reduces the number of such ifs. I think 2 ifs should be enough, one for the functions and one for the variables.
There was a problem hiding this comment.
Ok, tried to do that better now - should we aim for setting an NGX_ECH from auto/lib/openssl/conf or something though? Not sure if that'd be better. See what you think when I push the next rev.
| { ngx_string("ssl_ech_inner_sni"), NULL, ngx_http_ssl_variable, | ||
| (uintptr_t) ngx_ssl_get_ech_inner_sni, NGX_HTTP_VAR_CHANGEABLE, 0 }, | ||
| { ngx_string("ssl_ech_outer_sni"), NULL, ngx_http_ssl_variable, | ||
| (uintptr_t) ngx_ssl_get_ech_outer_sni, NGX_HTTP_VAR_CHANGEABLE, 0 }, |
There was a problem hiding this comment.
I"m not sure we need both of them. The outer SNI does make sense. But I'm not sure we need the inner one. The outer one can be called ssl_ech_server_name, similar to ssl_server_name we already have, which is the SNI value (and is the inner SNI for ECH).
There was a problem hiding this comment.
fair enough that we don't really need ssl_ech_inner_sni but I'd argue to keep the name for the outer - ssl_ech_server_name would easily confuse as to whether it means inner or outer.
There was a problem hiding this comment.
In any case, we don't use the term sni in variables and directives. It's always $ssl_server_name, $ssl_preread_server_name, proxy_ssl_server_name. Let's try ssl_ech_outer_server_name at least.
There was a problem hiding this comment.
ack ssl_ech_outer_server_name it is. I didn't change the function name but can if you prefer. (For now the function is still ngx_ssl_get_ech_outer_sni().
|
|
||
| #ifndef OPENSSL_NO_ECH | ||
| { ngx_string("ssl_echkeydir"), | ||
| NGX_HTTP_MAIN_CONF|NGX_CONF_TAKE1, |
There was a problem hiding this comment.
I think this directive should be allowed at the server level as well. Please add NGX_HTTP_SRV_CONF:
NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_FLAG,
There was a problem hiding this comment.
I'll argue (a bit) against that - it seems simpler and better if ECH is inherently set up/available for all virtual servers rather than for some, and there's no real cost to doing so I think. I don't see why you'd want ECH to be available only for some virtual servers and not others?
There was a problem hiding this comment.
I don't think we need to force it for all servers. And anyway, you are already using the server configuration (ngx_stream_ssl_srv_conf_t) which means it's already per-server. And there's no cost in enabling this directive at the server level as well. I'm not advocating for removing the main scope btw, let's just add the server scope in addition to it.
There was a problem hiding this comment.
sure - did that, hopefully correctly:-)
|
Those all look sensible, thanks. Will get at 'em shortly but travelling today. |
Also please rebase the code to the recent master. We had some SNI-related changes there recently. |
|
✅ All required contributors have signed the F5 CLA for this PR. Thank you! |
|
I have hereby read the F5 CLA and agree to its terms |
|
recheck |
|
The push above fixes the things requested this week except:
I'll take a look at 1 above and we can discuss 2/3/4. First though, I'm gonna do a bit of playing with boringssl to see if the work the lighttpd maintainer did translates over as easily as I think it should. |
|
The push above is a version that seems to work (with limited localhost tests) with BoringSSL. Likely would need more work, but passes those localhost tests ok (with real or GREASEd ECH and an HRR case). |
|
The push above tries to add ECH to the stream module. Not that sure I've done that correctly, but it works in local testing. |
|
@arut just checking where we're at (I have to do some project-internal reporting for month end:-) - do you need me to do anything to progress things? |
I'm getting back to this now. I've been busy with the release. @sftcd, any news about split mode? Feels like split mode is a bigger feature than shared mode. |
Interesting! A lot of people have said to me that they don't find split-mode so interesting due to how they terminate TLS, but OTOH, we did a small trial with the Wikimedia foundation and they'd have loved to use split-mode if they had someone to be the client-facing server for them. (Finding such a someone turns out not that easy.) Anyway, yes, I have a split-mode implementation for nginx, (and also for haproxy) using the stream module, but that needs an API that hasn't yet been agreed with the OpenSSL maintainers and that isn't in the OpenSSL feature branch. You'd have to build against a personal (or DEfO-project-specific) fork of OpenSSL rather than the OpenSSL project's ECH feature branch. The split-mode API required is an oddball thing from the POV of the OpenSSL library - you need to use an You can see the DEfO-project fork's split-mode API here and the core of how we use that in nginx here. I'd not be surprised if the latter needed changes btw:-) IIUC split-mode is also not yet supported by other TLS libraries. So I think the way forward would be to handle ECH shared-mode first, then later to add split-mode in a separate PR once we've agreed a split-mode API with OpenSSL maintainers. (Once I've gotten that API agreed with OpenSSL maintainers I do plan to raise corresponding PRs with TLS server projects.) |
|
Push above tries to handle all today's @arut comments. |
|
Below is my patch on top of the last version. It mostly style. I hope you don't mind.
Also, I'm not quite sure how to actually observe the The patch: diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c
index 0a22c99fe..3043bf7c3 100644
--- a/src/event/ngx_event_openssl.c
+++ b/src/event/ngx_event_openssl.c
@@ -1659,7 +1659,7 @@ ngx_ssl_ech_files(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_array_t *filenames)
#ifdef SSL_OP_ECH_GREASE
int numkeys;
BIO *in;
- ngx_int_t rv;
+ ngx_int_t rc;
ngx_str_t *filename;
ngx_uint_t i;
OSSL_ECHSTORE *es;
@@ -1670,40 +1670,49 @@ ngx_ssl_ech_files(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_array_t *filenames)
es = OSSL_ECHSTORE_new(NULL, NULL);
if (es == NULL) {
- ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
- "OSSL_ECHSTORE_new() failed" );
+ ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0, "OSSL_ECHSTORE_new() failed");
return NGX_ERROR;
}
- /* process each ssl_ech_file directive file name */
- rv = NGX_ERROR;
- numkeys = 0;
+ rc = NGX_ERROR;
filename = filenames->elts;
+
for (i = 0; i < filenames->nelts; i++) {
if (ngx_conf_full_name(cf->cycle, &filename[i], 1) != NGX_OK) {
goto cleanup;
}
+ in = BIO_new_file((char *) filename[i].data, "r");
+ if (in == NULL) {
+ ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
+ "BIO_new_file(\"%s\") failed", filename[i].data);
+ goto cleanup;
+ }
+
/*
* We only set the ECHConfigList from the first file read to use
- * in ECH retry-configs (via the "i == 0" below).
+ * in ECH retry-configs.
+ *
* That allows many sensible key rotation schemes so that the
* values sent in ECH retry-configs are smaller and current.
* For example, if the first file name has the current ECH
* private key, and a second one has the previously used key
* that some clients may still use due to DNS caching.
*/
- if ((in = BIO_new_file((char *)filename[i].data, "r")) == NULL
- || OSSL_ECHSTORE_read_pem(es, in, (i == 0)) != 1) {
- ngx_ssl_error(NGX_LOG_ALERT, ssl->log, 0,
- "OSSL_ECHSTORE_read_pem() failed for: %s",
- filename[i].data);
- goto cleanup;
- }
- BIO_free_all(in);
- in = NULL;
+ if (OSSL_ECHSTORE_read_pem(es, in, i ? OSSL_ECH_NO_RETRY
+ : OSSL_ECH_FOR_RETRY)
+ != 1)
+ {
+ ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
+ "OSSL_ECHSTORE_read_pem(%s) failed",
+ filename[i].data);
+ BIO_free(in);
+ goto cleanup;
+ }
+
+ BIO_free(in);
}
/*
@@ -1711,26 +1720,31 @@ ngx_ssl_ech_files(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_array_t *filenames)
* private key in there (the PEM file spec allows zero or one
* private key per file)
*/
- if (OSSL_ECHSTORE_num_keys(es, &numkeys) != 1
- || numkeys <= 0
- || SSL_CTX_set1_echstore(ssl->ctx, es) != 1) {
+
+ if (OSSL_ECHSTORE_num_keys(es, &numkeys) != 1) {
+ ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
+ "OSSL_ECHSTORE_num_keys(%s) failed");
+ goto cleanup;
+ }
+
+ if (numkeys > 0 && SSL_CTX_set1_echstore(ssl->ctx, es) != 1) {
ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
"SSL_CTX_set1_echstore() failed");
goto cleanup;
}
- rv = NGX_OK;
+ rc = NGX_OK;
cleanup:
OSSL_ECHSTORE_free(es);
- BIO_free_all(in);
- return rv;
+ return rc;
#else
- ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
- "ngx_ssl_ech_files: ECH configured but not supported");
- return NGX_ERROR;
+ ngx_log_error(NGX_LOG_WARN, ssl->log, 0,
+ "\"ssl_ech_file\" is not supported on this platform, "
+ "ignored");
+ return NGX_OK;
#endif
}
@@ -5786,30 +5800,27 @@ ngx_ssl_get_ech_status(ngx_connection_t *c, ngx_pool_t *pool, ngx_str_t *s)
inner_sni = NULL;
outer_sni = NULL;
+
echrv = SSL_ech_get1_status(c->ssl->connection, &inner_sni, &outer_sni);
+
switch (echrv) {
case SSL_ECH_STATUS_NOT_TRIED:
ngx_str_set(s, "NOT_TRIED");
break;
- case SSL_ECH_STATUS_FAILED:
- ngx_str_set(s, "FAILED");
- break;
- case SSL_ECH_STATUS_BAD_NAME:
- ngx_str_set(s, "WORKED_BAD_NAME");
- break;
case SSL_ECH_STATUS_SUCCESS:
ngx_str_set(s, "SUCCESS");
break;
case SSL_ECH_STATUS_GREASE:
- ngx_str_set(s, "GREASED");
+ ngx_str_set(s, "GREASE");
break;
case SSL_ECH_STATUS_BACKEND:
- ngx_str_set(s, "INNER");
+ ngx_str_set(s, "BACKEND");
break;
default:
- ngx_str_set(s, "STATUS_ERROR");
+ ngx_str_set(s, "FAILED");
break;
}
+
OPENSSL_free(inner_sni);
OPENSSL_free(outer_sni);
#endif
@@ -5817,7 +5828,8 @@ ngx_ssl_get_ech_status(ngx_connection_t *c, ngx_pool_t *pool, ngx_str_t *s)
}
ngx_int_t
-ngx_ssl_get_ech_outer_sni(ngx_connection_t *c, ngx_pool_t *pool, ngx_str_t *s)
+ngx_ssl_get_ech_outer_server_name(ngx_connection_t *c, ngx_pool_t *pool,
+ ngx_str_t *s)
{
#if defined(SSL_OP_ECH_GREASE)
int echrv;
@@ -5825,16 +5837,23 @@ ngx_ssl_get_ech_outer_sni(ngx_connection_t *c, ngx_pool_t *pool, ngx_str_t *s)
inner_sni = NULL;
outer_sni = NULL;
- s->len = 0;
+
echrv = SSL_ech_get1_status(c->ssl->connection, &inner_sni, &outer_sni);
+
if (echrv == SSL_ECH_STATUS_SUCCESS && outer_sni) {
s->len = ngx_strlen(outer_sni);
+
s->data = ngx_pnalloc(pool, s->len);
if (s->data == NULL) {
return NGX_ERROR;
}
+
ngx_memcpy(s->data, outer_sni, s->len);
+
+ } else {
+ s->len = 0;
}
+
OPENSSL_free(inner_sni);
OPENSSL_free(outer_sni);
#else
diff --git a/src/event/ngx_event_openssl.h b/src/event/ngx_event_openssl.h
index 1fc775694..4f9f9e714 100644
--- a/src/event/ngx_event_openssl.h
+++ b/src/event/ngx_event_openssl.h
@@ -342,8 +342,8 @@ ngx_int_t ngx_ssl_get_server_name(ngx_connection_t *c, ngx_pool_t *pool,
ngx_str_t *s);
ngx_int_t ngx_ssl_get_ech_status(ngx_connection_t *c, ngx_pool_t *pool,
ngx_str_t *s);
-ngx_int_t ngx_ssl_get_ech_outer_sni(ngx_connection_t *c, ngx_pool_t *pool,
- ngx_str_t *s);
+ngx_int_t ngx_ssl_get_ech_outer_server_name(ngx_connection_t *c,
+ ngx_pool_t *pool, ngx_str_t *s);
ngx_int_t ngx_ssl_get_alpn_protocol(ngx_connection_t *c, ngx_pool_t *pool,
ngx_str_t *s);
ngx_int_t ngx_ssl_get_raw_certificate(ngx_connection_t *c, ngx_pool_t *pool,
diff --git a/src/http/modules/ngx_http_ssl_module.c b/src/http/modules/ngx_http_ssl_module.c
index d9adc0b76..0fe838020 100644
--- a/src/http/modules/ngx_http_ssl_module.c
+++ b/src/http/modules/ngx_http_ssl_module.c
@@ -117,6 +117,13 @@ static ngx_command_t ngx_http_ssl_commands[] = {
0,
NULL },
+ { ngx_string("ssl_ech_file"),
+ NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE1,
+ ngx_conf_set_str_array_slot,
+ NGX_HTTP_SRV_CONF_OFFSET,
+ offsetof(ngx_http_ssl_srv_conf_t, ech_files),
+ NULL },
+
{ ngx_string("ssl_password_file"),
NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE1,
ngx_http_ssl_password_file,
@@ -215,13 +222,6 @@ static ngx_command_t ngx_http_ssl_commands[] = {
offsetof(ngx_http_ssl_srv_conf_t, session_tickets),
NULL },
- { ngx_string("ssl_ech_file"),
- NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE1,
- ngx_conf_set_str_array_slot,
- NGX_HTTP_SRV_CONF_OFFSET,
- offsetof(ngx_http_ssl_srv_conf_t, ech_files),
- NULL },
-
{ ngx_string("ssl_session_ticket_key"),
NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE1,
ngx_conf_set_str_array_slot,
@@ -388,7 +388,8 @@ static ngx_http_variable_t ngx_http_ssl_vars[] = {
(uintptr_t) ngx_ssl_get_ech_status, NGX_HTTP_VAR_CHANGEABLE, 0 },
{ ngx_string("ssl_ech_outer_server_name"), NULL, ngx_http_ssl_variable,
- (uintptr_t) ngx_ssl_get_ech_outer_sni, NGX_HTTP_VAR_CHANGEABLE, 0 },
+ (uintptr_t) ngx_ssl_get_ech_outer_server_name,
+ NGX_HTTP_VAR_CHANGEABLE, 0 },
{ ngx_string("ssl_client_cert"), NULL, ngx_http_ssl_variable,
(uintptr_t) ngx_ssl_get_certificate, NGX_HTTP_VAR_CHANGEABLE, 0 },
@@ -656,6 +657,7 @@ ngx_http_ssl_create_srv_conf(ngx_conf_t *cf)
sscf->certificates = NGX_CONF_UNSET_PTR;
sscf->certificate_keys = NGX_CONF_UNSET_PTR;
sscf->certificate_cache = NGX_CONF_UNSET_PTR;
+ sscf->ech_files = NGX_CONF_UNSET_PTR;
sscf->passwords = NGX_CONF_UNSET_PTR;
sscf->conf_commands = NGX_CONF_UNSET_PTR;
sscf->builtin_session_cache = NGX_CONF_UNSET;
@@ -666,7 +668,6 @@ ngx_http_ssl_create_srv_conf(ngx_conf_t *cf)
sscf->ocsp_cache_zone = NGX_CONF_UNSET_PTR;
sscf->stapling = NGX_CONF_UNSET;
sscf->stapling_verify = NGX_CONF_UNSET;
- sscf->ech_files = NGX_CONF_UNSET_PTR;
return sscf;
}
@@ -708,6 +709,8 @@ ngx_http_ssl_merge_srv_conf(ngx_conf_t *cf, void *parent, void *child)
ngx_conf_merge_ptr_value(conf->certificate_cache, prev->certificate_cache,
NULL);
+ ngx_conf_merge_ptr_value(conf->ech_files, prev->ech_files, NULL);
+
ngx_conf_merge_ptr_value(conf->passwords, prev->passwords, NULL);
ngx_conf_merge_str_value(conf->dhparam, prev->dhparam, "");
@@ -892,8 +895,6 @@ ngx_http_ssl_merge_srv_conf(ngx_conf_t *cf, void *parent, void *child)
return NGX_CONF_ERROR;
}
- ngx_conf_merge_ptr_value(conf->ech_files, prev->ech_files, NULL);
-
if (ngx_ssl_ech_files(cf, &conf->ssl, conf->ech_files) != NGX_OK) {
return NGX_CONF_ERROR;
}
diff --git a/src/http/modules/ngx_http_ssl_module.h b/src/http/modules/ngx_http_ssl_module.h
index 07e45744c..a078d44f8 100644
--- a/src/http/modules/ngx_http_ssl_module.h
+++ b/src/http/modules/ngx_http_ssl_module.h
@@ -42,7 +42,6 @@ typedef struct {
ngx_ssl_cache_t *certificate_cache;
ngx_str_t dhparam;
- ngx_array_t *ech_files;
ngx_str_t ecdh_curve;
ngx_str_t client_certificate;
ngx_str_t trusted_certificate;
@@ -50,6 +49,7 @@ typedef struct {
ngx_str_t ciphers;
+ ngx_array_t *ech_files;
ngx_array_t *passwords;
ngx_array_t *conf_commands;
diff --git a/src/stream/ngx_stream_ssl_module.c b/src/stream/ngx_stream_ssl_module.c
index aaf2c6010..4b64fd6b0 100644
--- a/src/stream/ngx_stream_ssl_module.c
+++ b/src/stream/ngx_stream_ssl_module.c
@@ -126,6 +126,13 @@ static ngx_command_t ngx_stream_ssl_commands[] = {
0,
NULL },
+ { ngx_string("ssl_ech_file"),
+ NGX_STREAM_MAIN_CONF|NGX_STREAM_SRV_CONF|NGX_CONF_TAKE1,
+ ngx_conf_set_str_array_slot,
+ NGX_STREAM_SRV_CONF_OFFSET,
+ offsetof(ngx_stream_ssl_srv_conf_t, ech_files),
+ NULL },
+
{ ngx_string("ssl_password_file"),
NGX_STREAM_MAIN_CONF|NGX_STREAM_SRV_CONF|NGX_CONF_TAKE1,
ngx_stream_ssl_password_file,
@@ -147,13 +154,6 @@ static ngx_command_t ngx_stream_ssl_commands[] = {
offsetof(ngx_stream_ssl_srv_conf_t, dhparam),
NULL },
- { ngx_string("ssl_ech_file"),
- NGX_STREAM_MAIN_CONF|NGX_STREAM_SRV_CONF|NGX_CONF_TAKE1,
- ngx_conf_set_str_array_slot,
- NGX_STREAM_SRV_CONF_OFFSET,
- offsetof(ngx_stream_ssl_srv_conf_t, ech_files),
- NULL },
-
{ ngx_string("ssl_ecdh_curve"),
NGX_STREAM_MAIN_CONF|NGX_STREAM_SRV_CONF|NGX_CONF_TAKE1,
ngx_conf_set_str_slot,
@@ -383,7 +383,8 @@ static ngx_stream_variable_t ngx_stream_ssl_vars[] = {
(uintptr_t) ngx_ssl_get_ech_status, NGX_STREAM_VAR_CHANGEABLE, 0 },
{ ngx_string("ssl_ech_outer_server_name"), NULL, ngx_stream_ssl_variable,
- (uintptr_t) ngx_ssl_get_ech_outer_sni, NGX_STREAM_VAR_CHANGEABLE, 0 },
+ (uintptr_t) ngx_ssl_get_ech_outer_server_name,
+ NGX_STREAM_VAR_CHANGEABLE, 0 },
{ ngx_string("ssl_client_cert"), NULL, ngx_stream_ssl_variable,
(uintptr_t) ngx_ssl_get_certificate, NGX_STREAM_VAR_CHANGEABLE, 0 },
@@ -901,6 +902,7 @@ ngx_stream_ssl_create_srv_conf(ngx_conf_t *cf)
sscf->certificates = NGX_CONF_UNSET_PTR;
sscf->certificate_keys = NGX_CONF_UNSET_PTR;
sscf->certificate_cache = NGX_CONF_UNSET_PTR;
+ sscf->ech_files = NGX_CONF_UNSET_PTR;
sscf->passwords = NGX_CONF_UNSET_PTR;
sscf->conf_commands = NGX_CONF_UNSET_PTR;
sscf->prefer_server_ciphers = NGX_CONF_UNSET;
@@ -916,7 +918,6 @@ ngx_stream_ssl_create_srv_conf(ngx_conf_t *cf)
sscf->ocsp_cache_zone = NGX_CONF_UNSET_PTR;
sscf->stapling = NGX_CONF_UNSET;
sscf->stapling_verify = NGX_CONF_UNSET;
- sscf->ech_files = NGX_CONF_UNSET_PTR;
return sscf;
}
@@ -957,6 +958,8 @@ ngx_stream_ssl_merge_srv_conf(ngx_conf_t *cf, void *parent, void *child)
ngx_conf_merge_ptr_value(conf->certificate_cache, prev->certificate_cache,
NULL);
+ ngx_conf_merge_ptr_value(conf->ech_files, prev->ech_files, NULL);
+
ngx_conf_merge_ptr_value(conf->passwords, prev->passwords, NULL);
ngx_conf_merge_str_value(conf->dhparam, prev->dhparam, "");
@@ -1136,8 +1139,6 @@ ngx_stream_ssl_merge_srv_conf(ngx_conf_t *cf, void *parent, void *child)
return NGX_CONF_ERROR;
}
- ngx_conf_merge_ptr_value(conf->ech_files, prev->ech_files, NULL);
-
if (ngx_ssl_ech_files(cf, &conf->ssl, conf->ech_files) != NGX_OK) {
return NGX_CONF_ERROR;
}
diff --git a/src/stream/ngx_stream_ssl_module.h b/src/stream/ngx_stream_ssl_module.h
index de86c5a2f..6fdd8f88c 100644
--- a/src/stream/ngx_stream_ssl_module.h
+++ b/src/stream/ngx_stream_ssl_module.h
@@ -41,7 +41,6 @@ typedef struct {
ngx_ssl_cache_t *certificate_cache;
ngx_str_t dhparam;
- ngx_array_t *ech_files;
ngx_str_t ecdh_curve;
ngx_str_t client_certificate;
ngx_str_t trusted_certificate;
@@ -50,6 +49,7 @@ typedef struct {
ngx_str_t ciphers;
+ ngx_array_t *ech_files;
ngx_array_t *passwords;
ngx_array_t *conf_commands;
|
I don't mind at all! In fact I'm glad to see that as it'll likely save some iterations where I get it wrong:-) |
|
Push above has the patch from @arut (thanks again!) and passes my local tests fine. |
|
|
||
| OPENSSL_free(inner_sni); | ||
| OPENSSL_free(outer_sni); | ||
| #endif |
There was a problem hiding this comment.
Missed the #else ... s->len part here
There was a problem hiding this comment.
yep, added there thanks
There was a problem hiding this comment.
oops, forgot to git add that 1st time, so it's in the 2nd push below
|
Also, let's remove the |
Sure. Is there somewhere that you would like some documentation or is that better left for another day? (That file is still in #973 anyway so not lost.) |
Sure. Let's make it a blog post at https://blog.nginx.org. We are currently working on a process for external contributors. We would appreciate your contribution there. I will come back with more details later. For the record, last version of the doc: https://github.com/sftcd/nginx/blob/cf5179918b7a1557d5e864f0874c9032340c58a6/ECH-build.md |
Great. Let me know in the meantime if there's anything else to do for the PR. |
Please add a period at the end. |
| #endif | ||
| return NGX_OK; | ||
| } | ||
|
|
There was a problem hiding this comment.
+1 empty line here please (need 2)
|
Otherwise looks ok, please squash. |
Done! And btw, if you or a colleague wanted to just write bloggy text, fire ahead - I'm not the most bloggy type of person, so probably better if someone who likes doing that makes a 1st cut and I can suggest edits. But whatever works is fine. |
We really appreciate your contribution. And we are open to both code and blog post contributions. But we can collaborate, review the text and suggest improvements. |
|
And likewise thanks for being willing to process the PR, esp. with me getting the nginx code style stuff wrong so often:-) |
|
@sftcd following up on the blog post discussion, here's our community blog repo: https://github.com/nginx/community-blog Please create a blog post PR with the Also, last time I read it, it needed some cleanup due to old syntax used. |
|
Hi, is there an up-to-date tutorial available for reference? It appears this patch has undergone significant changes from the initial version. |
|
I'm updating things here at the moment - not finished but I think the main sections there are up to date. If not, or you see other errors, be great to know. |
Thanks for your effort! Worked. Found some typos: https://github.com/defo-project/nginx/blob/master/ECH-build.md?plain=1#L83 shoule be (parameters updated): $OSSL ech -public_name example.com -out example.com.pem.echhttps://github.com/defo-project/nginx/blob/master/ECH-build.md?plain=1#L133 and ssl variable should be Also, it seems that HTTPS records support wildcard domains, but after configuring a wildcard domain, ECH cannot take effect. You must set the specific domain name; otherwise the browser cannot match it correctly. Please advise how to elegantly configure multiple sites on the same Nginx server. |
Great, and thanks for the typo-fixes.
Sorry I'm not sure I quite get the question - for me 'wildcard domain' is a DNS concept and I don't think of operating those via Nginx. Configuring ECH for all servers in a configuration should just need putting the |
For example, if I want to enable ECH for three of my sites — foo1.example.com, foo2.example.com, and foo3.example.com — I have to create three separate I actually have more than ten sites running on the same nginx server, and I want to enable ECH for all of them. Is there a more elegant way to manage the HTTPS DNS records, such as using a wildcard domain like *.example.com? I tried using a wildcard HTTPS record, and DNS itself accepts it, but ECH doesn’t work unless the domain is explicitly specified. |
That depends on your authoritative DNS service or software. I'd guess, but am not sure, that it is supported in various forms by DNS service providers and authoritative DNS server software. For example, cloudflare have some instructions at https://developers.cloudflare.com/dns/manage-dns-records/reference/wildcard-dns-records/ but I've not used that so can't really help more sorry. |
Proposed changes
This PR adds Encrypted Client Hello (ECH) functionality to NGINX, when using OpenSSL for TLS.
This addresses #266
Notes:
Lastly, for open-ness, our work on this has been funded by the Open Technology Fund (OTF) in the DEfO project.