[PATCH] SSL: added support for TLS Session Tickets (RFC5077).

[Maxim Dounin]

Hello!

On Sat, Sep 28, 2013 at 02:55:36AM -0700, Piotr Sikora wrote:

> # HG changeset patch
> # User Piotr Sikora <piotr at cloudflare.com>
> # Date 1380361691 25200
> #      Sat Sep 28 02:48:11 2013 -0700
> # Node ID 6d3710969a18e2d0d817e297c2e17f941a58cd40
> # Parent  a720f0b0e08345ebb01353250f4031bb6e141385
> SSL: added support for TLS Session Tickets (RFC5077).

As previously noted, the patch description is wrong.  It also 
make sense to add some description of the directive added.

See below for some code comments.

[...]

> +#ifdef SSL_CTRL_SET_TLSEXT_TICKET_KEY_CB
> +
> +typedef struct {
> +    size_t                         name_len;
> +    u_char                         name[16];
> +
> +    u_char                         aes_key[16];
> +    u_char                         hmac_key[16];
> +} ngx_ssl_session_ticket_key_t;
> +
> +
> +typedef struct {
> +    ngx_ssl_session_ticket_key_t  *default_key;
> +    ngx_array_t                    keys;
> +} ngx_ssl_session_ticket_keys_t;
> +
> +#endif

This looks needlessly complicated, see below.

[...]

> @@ -153,6 +158,17 @@ static ngx_command_t  ngx_http_ssl_comma
>        0,
>        NULL },
> 
> +#ifdef SSL_CTRL_SET_TLSEXT_TICKET_KEY_CB
> +
> +    { ngx_string("ssl_session_ticket_key"),
> +      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE23,
> +      ngx_http_ssl_session_ticket_key,
> +      NGX_HTTP_SRV_CONF_OFFSET,
> +      0,
> +      NULL },
> +
> +#endif
> +

This makes the directive unavailable without any meaningfull 
diagnostics if nginx was build with old OpenSSL, which isn't very 
user-friendly.

[...]

> @@ -769,6 +810,146 @@ invalid:
>      return NGX_CONF_ERROR;
>  }
> 
> +#ifdef SSL_CTRL_SET_TLSEXT_TICKET_KEY_CB

Style, there should be 2 blank lines before #ifdef.

[...]

> +    if (value[1].len > 16) {
> +        ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> +                           "\"ssl_session_ticket_key\" name \"%V\" too long, "
> +                           "it cannot exceed 16 characters", &value[1]);
> +        return NGX_CONF_ERROR;
> +    }
> +
> +    if (cf->args->nelts == 4) {
> +
> +        if (ngx_strcmp(value[3].data, "default")) {

Style: as ngx_strcmp() doesn't return a boolean value, I would 
recommend using "!= 0" test instead.

But actually I doubt we at all need an explicit mark for default 
key.  Just using first one for encryption would probably be good 
enough.

I also think it would be better to don't rely on an explicitly 
written name, which will make automatic key rotation a pain - as 
one will have to update both name in a configuration file and a 
file with keys.   E.g. Apache uses a binary file with 48 bytes of 
random data, which is much easier to generate and rotate if 
needed.

[...]

> +    if (ngx_conf_full_name(cf->cycle, &value[2], 1) != NGX_OK) {
> +        return NGX_CONF_ERROR;
> +    }
> +
> +    ngx_memzero(&file, sizeof(ngx_file_t));
> +    file.name = value[2];
> +    file.log = cf->log;
> +
> +    file.fd = ngx_open_file(file.name.data, NGX_FILE_RDONLY, 0, 0);
> +    if (file.fd == NGX_INVALID_FILE) {
> +        ngx_conf_log_error(NGX_LOG_EMERG, cf, ngx_errno,
> +                           ngx_open_file_n " \"%V\" failed", &file.name);
> +
> +        return NGX_CONF_ERROR;
> +    }

Not sure if this code should be here.  Other file operations are 
handled in the ngx_event_openssl.c, and doing the same for session 
tickets might be a good idea as well.  Especially if you'll 
consider adding relevant directives to the mail module.

-- 
Maxim Dounin
http://nginx.org/en/donation.html