Would it be possible to support SSL_CTX_set_select_certificate_cb in BoringSSL, as this PR currently uses SSL_CTX_set_client_hello_cb for OpenSSL?

A history of the topic: https://trac.nginx.org/nginx/ticket/844

In a nutshell, a previous attempt was made by Piotr Sikora 7 years ago. That effort was rejected by Maxim Dounin. Apart from the style/tradition issue and the implementation issue, the biggest concern was that ESNI may not be compatible with this approach. Now that ESNI is no longer around, we need to make sure this approach is compatible with ECH. Seems like there should be less issues here. I would expect a separate call for the inner ClientHello, but this needs to be verified.

Apart from the session resume issue, one of the major reasons for this change is enabling per-server ssl_protocols, as follows from the tickets. It should at least be specified in the commit log. Also, as mentioned by @nandsky, a similar change should be made for BoringSSL to allow the same functionality there, despite there's no session resume issue in BoringSSL.

SSL_CTX_set_select_certificate_cb and per-server ssl_protocols is out of scope of this change.

This change aims to solve the specific deficiency of the servername callback in OpenSSL which is called too late wrt SSL session resumption.

So far, I don't think we need to touch BoringSSL API bits, as it doesn't suffer from this problem.

Considering ESNI and now ECH, OpenSSL doesn't support it, so I don't see anything to discuss so far.
Once it will (ever) support, we may reconsider this.

ECH is not going to be an issue.
Both BoringSSL and the OpenSSL WIP branch process inner ClientHello before the earliest available callback. The name we get in SSL_CTX_set_select_certificate_cb/SSL_CTX_set_client_hello_cb matches the value available from SSL_CTX_set_tlsext_servername_callback.

Now, I'm not quite happy with how this forces us to apply ECH configs to a listener scope instead of a server scope, because the default server must be able to unwrap the ECH extension before we are able switch to another SSL_CTX. Let's see if that changes during the OpenSSL server-side part review.

Going back to the current patch:

• The general logic looks sound. There are differences in behavior compared to 46b9f5d, but all of those make sense.

• We still ignore RFC6066 Section 3 requirement to decline the session resumption for TLSv1.2 connections with mismatched server names, but RFC8446 Section 4.2.11 text allows this for TLSv1.3. This should be fine, as we're preserving the existing behavior.

• The CVE is already patched, and there's no immediate issue requiring this change. Well, 46b9f5d blocks some ambiguous requests, but we can live with that.
  Might as well take a look at the BoringSSL changes to ensure consistency of the behavior as much as we can (with obvious exception of LibreSSL).

• There are now 3 places in the code implementing TLSEXT_TYPE_server_name parser. Is it possible to deduplicate at least the new 2?

• There's a lot of ifdefs and odd constructions (success/error) in ngx_http_ssl_servername. Would it be reasonable to split the entry points for SSL callbacks (ngx_http_ssl_client_hello/ngx_http_ssl_servername) and make these invoke a common handler, something like ngx_http_ssl_set_servername? That was partially implemented in the patches by Piotr and looked more maintainable across 3 different SSL callbacks.

SSL_CTX_set_select_certificate_cb and per-server ssl_protocols is out of scope of this change.

This change aims to solve the specific deficiency of the servername callback in OpenSSL which is called too late wrt SSL session resumption.
So far, I don't think we need to touch BoringSSL API bits, as it doesn't suffer from this problem.

I agree it is not the goal of this change. But since the callback is now called before OpenSSL negotiates the protocol version, the change does this as well. My point is, the behavior needs to be consistent across libraries.

Considering ESNI and now ECH, OpenSSL doesn't support it, so I don't see anything to discuss so far. Once it will (ever) support, we may reconsider this.

It may be too late to reconsider.

ECH is not going to be an issue. Both BoringSSL and the OpenSSL WIP branch process inner ClientHello before the earliest available callback. The name we get in SSL_CTX_set_select_certificate_cb/SSL_CTX_set_client_hello_cb matches the value available from SSL_CTX_set_tlsext_servername_callback.

Note it's just WiP. Considering how much is happening in openssl/openssl@86da814 while merging OpenSSL client-side support into a feature-branch (not even mainstream), I wouldn't rely on that.

The use of SSL_CTX_set_client_hello_cb will break ssl_client_hello_by_lua_block, which to be sure, is a separate project but many people use it.
https://github.com/openresty/lua-resty-core/blob/master/lib/ngx/ssl/clienthello.md#set_protocols
And it can solve the problem of choosing TLS version per server block.

[ .. strip unrelated .. ]

Going back to the current patch:

* The general logic looks sound. There are differences in behavior compared to [46b9f5d](https://github.com/nginx/nginx/commit/46b9f5d389447b3b822ea71f5ac86ebc316c2975), but all of those make sense.

Note that 46b9f5d is the result of an (awkward) processing order in OpenSSL.

* We still ignore RFC6066 Section 3 requirement to decline the session resumption for TLSv1.2 connections with mismatched server names, but RFC8446 Section 4.2.11 text allows this for TLSv1.3. This should be fine, as we're preserving the existing behavior.

This follows the OpenSSL behaviour as documented in the SSL_get_servername manual page.
The change preserves and mimics OpenSSL behaviour as possible.

* The CVE is already patched, and there's no immediate issue requiring this change.  Well, [46b9f5d](https://github.com/nginx/nginx/commit/46b9f5d389447b3b822ea71f5ac86ebc316c2975) blocks some ambiguous requests, but we can live with that.
  Might as well take a look at the BoringSSL changes to ensure consistency of the behavior as much as we can (with obvious exception of LibreSSL).

46b9f5d is largely a kludge, this will make me happy when it goes away.

* There are now 3 places in the code implementing `TLSEXT_TYPE_server_name` parser.  Is it possible to deduplicate at least the new 2?

* There's a lot of ifdefs and odd constructions (`success`/`error`) in `ngx_http_ssl_servername`.  Would it be reasonable to split the entry points for SSL callbacks (`ngx_http_ssl_client_hello`/`ngx_http_ssl_servername`) and make these invoke a common handler, something like `ngx_http_ssl_set_servername`? That was partially implemented in the patches by Piotr and looked more maintainable across 3 different SSL callbacks.

These two are now addressed in the updated change, specifically c393026.

While perusing this pull-request I stumbled upon this change in
ngx_stream_ssl_merge_srv_conf()

+#ifdef SSL_CLIENT_HELLO_SUCCESS
+    {
+    ngx_ssl_client_hello_arg  *arg;
+
+    arg = ngx_pcalloc(cf->pool, sizeof(ngx_ssl_client_hello_arg));
+    arg->servername = ngx_stream_ssl_servername;
+
+    SSL_CTX_set_client_hello_cb(conf->ssl.ctx, ngx_ssl_client_hello_callback,
+                                arg);
+    }
+#endif
+

I like the use of a new block (always a fan of reducing variable scope),
but shouldn't its contents be indented (just like we do for most other
types of blocks)?

While perusing this pull-request I stumbled upon this change in ngx_stream_ssl_merge_srv_conf()

+#ifdef SSL_CLIENT_HELLO_SUCCESS
+    {
+    ngx_ssl_client_hello_arg  *arg;
+
+    arg = ngx_pcalloc(cf->pool, sizeof(ngx_ssl_client_hello_arg));
+    arg->servername = ngx_stream_ssl_servername;
+
+    SSL_CTX_set_client_hello_cb(conf->ssl.ctx, ngx_ssl_client_hello_callback,
+                                arg);
+    }
+#endif
+

I like the use of a new block (always a fan of reducing variable scope), but shouldn't its contents be indented (just like we do for most other types of blocks)?

The referenced code is used to pass the function pointer in the callback
argument. To avoid mixing function/data pointers, it is passed embedded
in a helper structure.

Such declaration is generally used in nginx to avoid messing with ifdefs
in a local variable declaration block in the beginning of a function, and
also to improve code change locality; see declaring variables under ifndef
SSL_OP_NO_COMPRESSION for such a good example.
Curly braces here are used to appease pre-C99 compilers. In this case,
per established code style, indentation is not used deliberately, such as
if curly braces were not actually added.

On a related note, I pondered how to make this code snippet look less awkward.

Here, cycle pool allocation is not actually needed and can be removed.
The decision to initialize the ClientHello callback argument is build time,
it can be made statically. Moving the visibility to global scope, akin to
ngx_stream_ssl_sess_id_ctx, allows setting the callback in a clean and
compact way, similar to the nearby SSL_CTX_set_tlsext_servername_callback().

changes:

• sni_accepted is a bit-field
• static ngx_ssl_client_hello_arg

The commit message mentions AWS-LC; SSL_CTX_set_client_hello_cb compatibility was added last month (aws/aws-lc@f10416a) and is handled a few lines above the select_certificate_cb. It's not a major concern though, as any past versions of AWS-LC are unsupported. More importantly, both OpenSSL and AWS-LC now differ in behavior from BoringSSL.

I'm aware of 4 modules that register SSL_CTX_set_client_hello_cb:

• lua, as already mentioned in SNI: using ClientHello callback. #562 (comment)
• nginx-ssl-ja3
• ja4-nginx-module
• nginx-acme requires early callback to intercept the tls-alpn-01 challenge, ACME: tls-alpn-01 challenge implementation nginx-acme#27

I don't want to block the current PR, but I'd like to discuss if we can provide an API for using SSL callbacks from a third-party module before the next release.

I'm aware of 4 modules that register SSL_CTX_set_client_hello_cb:

* lua, as already mentioned in [SNI: using ClientHello callback. #562 (comment)](https://github.com/nginx/nginx/pull/562#issuecomment-2831171146)

* [nginx-ssl-ja3](https://github.com/fooinha/nginx-ssl-ja3)

* [ja4-nginx-module](https://github.com/FoxIO-LLC/ja4-nginx-module)

* nginx-acme requires early callback to intercept the tls-alpn-01 challenge, [ACME: tls-alpn-01 challenge implementation nginx-acme#27](https://github.com/nginx/nginx-acme/pull/27)

I don't want to block the current PR, but I'd like to discuss if we can provide an API for using SSL callbacks from a third-party module before the next release.

It's hard to say how this callback is used in these modules without deeply
looking into the sources. If it is used for some immediate actions like
switching virtual server configurations, then some interface should be
provided to keep them working. E.g., that could be registering in callback
waiters, to be called from ngx_ssl_client_hello_callback() later, similar
to cb->servername(). I'll think about this.

(GH interface doesn't allow to properly reference or respond to this comment, ref'ing it manually)

I don't think passing ngx_str_t is a good idea though, that would break API compatibility with server name callback, I'd like to preserve to keep code changes (and maintenance costs) as minimal.

@bavshin-f5 wrote:

Can you elaborate? We never used void *arg of the server name callback before, and nothing implies that the user data pointer must be a null-terminated string.

Err, not API, just compatibility. Passing it as ngx_str_t is more solid, but this would require even more changes in ngx_http_ssl_servername() I'd like to avoid to keep it readable.

Changes to evaluate:

• converted to passing "ngx_str_t *", as requested by @bavshin-f5

Changes:

• restored logging of SNI not sent
• added BoringSSL support, as mentioned in SNI: using ClientHello callback. #562 (comment)

Changes:

• exdata index rename
• SSL_CTX_set_client_hello_cb arg passed through exdata
• build fix with OpenSSL < 0.9.8f
• style & comment changes
• simplified setting of callbacks in ssl modules, added separately for the sake of review

I want to proxy TLS connections with an ALPN of acme-tls/1 to an "upstream" ALPN terminator like ualpn/uacme.

The aim is to share ACME challenges to a single IP address between Nginx which controls port 443, and other services on that IP. Linux at least doesn't have a way to do this in the kernel since ClientHello can span multiple TCP segments.

Considering nginx-acme module, it should likely be configured per-SNI-virtual-host, so this internal ACME responder can take some ACME challenges, while letting others (perhaps any that don't match) go to the "upstream".

At present I'm peeling off where http module peeks at the ALPN and detects "h2". It works but it seems like there is soon to be an SNI callback way of doing this (maybe h2 detection would use it also?) and I don't want to get in the way.

If someone could point to where things are likely to land considering this PR and nginx-acme, I would appreciate it.

ualpn will want to receive and handle a pristine TLS handshake. It is no longer possible to achieve that once we started feeding the data to the OpenSSL routines, so the changes here are irrelevant for your goal.
It's also not possible to make the decision to forward unhandled alpn challenges to the "upstream" in nginx-acme for the same reason.

A possible configuration with an external tls-alpn-01 responder could be based on ngx_stream_ssl_preread_module and ngx_stream_pass_module (untested):

http {
    server {
        listen 8443 ssl;
        ...
    }
}

stream {
    map $ssl_preread_alpn_protocols $next_server {
        ~\bacme-tls/1\b  127.0.0.1:8444; # ualpn proxy
        default          127.0.0.1:8443; # https listener
    }

    server {
        listen 443;
        ssl_preread on;
        # preread_buffer_size ...; # adjust as necessary
        pass $next_server;
    }

    server {
        listen 127.0.0.1:8444;
        proxy_pass ualpn;
    }
}

And if you want to mix internal and external tls-alpn-01 responders, you will have to separate them in the same place, stream preread.

I see 3 options:

1. stream_pass_module + preread + virtual host handling?

A possible configuration with an external tls-alpn-01 responder could be based on [ngx_stream_ssl_preread_module]
And if you want to mix internal and external tls-alpn-01 responders, you will have to separate them in the same place, stream preread.

2. http_module: RFC proxy for http TLS ALPN acme-tls/1 #907 here the ACME ALPN is diverted in http_module where h2 ALPN and proxy header are tested with MSG_PEEK

3. expose the above point to modules so nginx-acme can participate

Changes: merged simplified setting of callbacks; function sorting & minor cleanup.

As for me, the code has become noticeably cleaner.

Thanks for testing.