[PATCH] SNI: better server name handling.

[Valentin V. Bartenev]

On Wednesday 22 May 2013 03:11:36 Piotr Sikora wrote:
> # HG changeset patch
> # User Piotr Sikora <piotr at cloudflare.com>
> # Date 1369177319 25200
> # Node ID 4b277448dfd56751c7c88477e78b2ba3cf6ae472
> # Parent  1d68b502088c9d6e6603e9699354e36d03d77f9c
> SNI: better server name handling.
> 
> Acknowledge acceptance of SNI server name to the OpenSSL library,
> which in turn lets the client know that it was accepted (by sending
> "server_name" TLS extension in the "ServerHello" handshake message,
> as suggested by RFC4366).
> 
> Previously, this would happen only in case when requested server name
> was on the "server_name" list and either: there were multiple virtual
> servers defined for the same listening port or there was at least one
> regular expression with captures in the "server_name" directive.
> 

Nice catch, but I'm not happy with the solution.  With your patch, client
will be acknowledged of acceptance even if the server name is not found.
I believe such behavior isn't consistent with RFC 4366, and it prevents the
client to know that specified virtual host doesn't exist on the server, which
effectively makes it useless.

Let me propose a better (from my point of view) patch at the end of my message.


> As a consequence, this change also:
> 1. Preserves requested SNI server name for future use.
>
> 2. Avoids unnecessary setting of SSL options if the virtual server
>    didn't change.
>

While I have nothing against such micro-optimizations, I would like to see them
as separate patches.


> 3. Avoids unnecessary lookup of virtual server later on if requested
>    HTTP server name is the same as requested SNI server name.
> 

It also changes behavior a bit when "ssl_verify_client" is used, but I don't
think that we care much about that case, especially since the change is needed
for $ssl_servername.

Probably you should combine it with your patch that adds the $ssl_servername
variable (and also rename $ssl_servername to $ssl_server_name).


[...]
> diff -r 1d68b502088c -r 4b277448dfd5 src/http/ngx_http_request.c
> --- a/src/http/ngx_http_request.c       Tue May 21 21:47:50 2013 +0400
> +++ b/src/http/ngx_http_request.c       Tue May 21 16:01:59 2013 -0700
> @@ -773,6 +773,7 @@
>      ngx_http_ssl_srv_conf_t   *sscf;
>      ngx_http_core_loc_conf_t  *clcf;
>      ngx_http_core_srv_conf_t  *cscf;
> +    ngx_int_t                  rc;
> 
[...]

There's a style problem. Please, sort variable declarations in ascending order
of the length of their type name (or alphabetically when they are equal).

  wbr, Valentin V. Bartenev


# HG changeset patch
# User Valentin Bartenev <vbart at nginx.com>
# Date 1369673389 -14400
# Node ID 3252ce05c7aedd0ad22c9e9db115dbbb5be22807
# Parent  2139768ee404a2e8b1e6164dbae0dc911fe14f1f
SNI: always add server names if SSL is enabled for the port.

It allows to acknowledge acceptance of SNI server name to the OpenSSL
library, which in turn lets the client know that it was accepted (by
sending "server_name" TLS extension in the "ServerHello" handshake
message, as suggested by RFC4366).

Previously, this would happen only in case when requested server name
was on the "server_name" list and either: there were multiple virtual
servers defined for the same listening port or there was at least one
regular expression with captures in the "server_name" directive.

diff -r 2139768ee404 -r 3252ce05c7ae src/http/ngx_http.c
--- a/src/http/ngx_http.c       Fri May 24 22:28:09 2013 +0400
+++ b/src/http/ngx_http.c       Mon May 27 20:49:49 2013 +0400
@@ -1444,6 +1444,9 @@ ngx_http_optimize_servers(ngx_conf_t *cf
 #if (NGX_PCRE)
                 || addr[a].default_server->captures
 #endif
+#if (NGX_HTTP_SSL && defined SSL_CTRL_SET_TLSEXT_HOSTNAME)
+                || addr[a].opt.ssl
+#endif
                )
             {
                 if (ngx_http_server_names(cf, cmcf, &addr[a]) != NGX_OK) {