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

[Maxim Dounin]

Hello!

On Thu, Oct 10, 2013 at 04:33:34PM -0700, Piotr Sikora wrote:

> Oops, one line was 81 chars long... Fixed patch below.
> 
> # HG changeset patch
> # User Piotr Sikora <piotr at cloudflare.com>
> # Date 1381447913 25200
> #      Thu Oct 10 16:31:53 2013 -0700
> # Node ID 4617733b2d7130313241253ef22958790d6fc902
> # Parent  5483d9e77b3287b00b1104a07688bda37bc7351e
> SSL: added ability to set keys used for Session Tickets (RFC5077).
> 
> In order to support key rollover, ssl_session_ticket_key can be defined
> multiple times. The first key will be used to issue and resume Session
> Tickets, while the rest will be used only to resume them.
> 
>     ssl_session_ticket_key   session_ticket_current.key;
>     ssl_session_ticket_key   session_ticket_curr-1h.key;
>     ssl_session_ticket_key   session_ticket_curr-2h.key;
> 
> Please note that nginx supports Session Tickets even without explicit
> configuration of the keys and this feature should be only used in setups
> where SSL traffic is distributed across multiple nginx servers.

Looks almost good, see some minor nits below.

[...]

> --- a/src/event/ngx_event_openssl.c Wed Oct 02 15:07:17 2013 +0400
> +++ b/src/event/ngx_event_openssl.c Thu Oct 10 16:31:53 2013 -0700
> @@ -38,6 +38,18 @@ static void ngx_ssl_expire_sessions(ngx_
>  static void ngx_ssl_session_rbtree_insert_value(ngx_rbtree_node_t *temp,
>      ngx_rbtree_node_t *node, ngx_rbtree_node_t *sentinel);
> 
> +#ifdef SSL_CTRL_SET_TLSEXT_TICKET_KEY_CB
> +static int ngx_ssl_session_ticket_key_callback(ngx_ssl_conn_t *ssl_conn,
> +    unsigned char *name, unsigned char *iv, EVP_CIPHER_CTX *ectx,
> +    HMAC_CTX *hctx, int enc);
> +
> +#ifdef OPENSSL_NO_SHA256
> +#define ngx_ssl_session_ticket_md  EVP_sha1
> +#else
> +#define ngx_ssl_session_ticket_md  EVP_sha256
> +#endif
> +#endif
> +

Wouldn't it better to move ngx_ssl_session_ticket_md defines 
to ngx_ssl_session_ticket_key_callback() implementation?

[...]

> @@ -84,6 +96,9 @@ int  ngx_ssl_server_conf_index;
>  int  ngx_ssl_session_cache_index;
>  int  ngx_ssl_certificate_index;
>  int  ngx_ssl_stapling_index;
> +#ifdef SSL_CTRL_SET_TLSEXT_TICKET_KEY_CB
> +int  ngx_ssl_session_ticket_keys_index;
> +#endif
> 
> 
>  ngx_int_t
> @@ -155,6 +170,18 @@ ngx_ssl_init(ngx_log_t *log)
>          return NGX_ERROR;
>      }
> 
> +#ifdef SSL_CTRL_SET_TLSEXT_TICKET_KEY_CB
> +
> +    ngx_ssl_session_ticket_keys_index = SSL_CTX_get_ex_new_index(0, NULL, NULL,
> +                                                                 NULL, NULL);
> +    if (ngx_ssl_session_ticket_keys_index == -1) {
> +        ngx_ssl_error(NGX_LOG_ALERT, log, 0,
> +                      "SSL_CTX_get_ex_new_index() failed");
> +        return NGX_ERROR;
> +    }
> +
> +#endif
> +

Do we really need these #ifdef's?  I don't think saving several 
bytes in ancient OpenSSL versions worth it (and the 
ngx_ssl_stapling_index doesn't have #ifdef's).

[...]

> +ngx_int_t
> +ngx_ssl_session_ticket_keys(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_array_t *paths)
> +{
> +    u_char                         buf[48];
> +    ssize_t                        n;
> +    ngx_str_t                     *path;
> +    ngx_file_t                     file;
> +    ngx_uint_t                     i;
> +    ngx_array_t                   *keys;
> +    ngx_file_info_t                fi;
> +    ngx_ssl_session_ticket_key_t  *key;

Wouldn't ngx_ssl_ticket_key_t be better?  Up to you, but 
"ngx_ssl_session_ticket_key_t" looks a bit too long.

[...]

> +    if (enc == 1) {
> +        /* encrypt session ticket */
> +
> +#if (NGX_DEBUG)
> +        ngx_hex_dump(buf, key[0].name, 16);
> +
> +        ngx_log_debug3(NGX_LOG_DEBUG_HTTP, c->log, 0,
> +                       "ssl session ticket encrypt, key: \"%*s\" (%s session)",
> +                       32, buf,
> +                       SSL_session_reused(ssl_conn) ? "reused" : "new");
> +#endif

What about something like this:

        ngx_log_debug3(NGX_LOG_DEBUG_HTTP, c->log, 0,
                       "ssl session ticket encrypt, key: \"%*s\" (%s session)",
                       ngx_hex_dump(buf, key[0].name, 16) - buf, buf,
                       SSL_session_reused(ssl_conn) ? "reused" : "new");

?

(Well, actually I'm not sure we need key name logged, but it 
probably doesn't matter much.)

> +
> +        RAND_pseudo_bytes(iv, 16);
> +        EVP_EncryptInit_ex(ectx, EVP_aes_128_cbc(), NULL, key[0].aes_key, iv);
> +        HMAC_Init_ex(hctx, key[0].hmac_key, 16, ngx_ssl_session_ticket_md(),
> +                     NULL);

Just in case, using a shorter name "ngx_ssl_ticket_md()" would 
save us from wrapping here and in decrypt case.

> +        memcpy(name, key[0].name, 16);
> +
> +        return 0;
> +
> +    } else {
> +        /* decrypt session ticket */
> +
> +        for (i = 0; i < keys->nelts; i++) {
> +            if (ngx_strncmp(name, key[i].name, 16) == 0) {

Shouldn't it be ngx_memcmp(), as it checks binary data now?

> +                break;
> +            }
> +        }
> +
> +        if (i == keys->nelts) {

Adding "goto found" instead of "break" would allow to avoid this 
additional check.  Up to you though.

> +            ngx_hex_dump(buf, name, 16);
> +
> +            ngx_log_debug2(NGX_LOG_DEBUG_HTTP, c->log, 0,
> +                           "ssl session ticket decrypt, key: \"%*s\" "
> +                           "not found", 32, buf);
> +            return 0;
> +        }
> +
> +#if (NGX_DEBUG)
> +        ngx_hex_dump(buf, key[i].name, 16);
> +
> +        ngx_log_debug3(NGX_LOG_DEBUG_HTTP, c->log, 0,
> +                       "ssl session ticket decrypt, key: \"%*s\"%s",
> +                       32, buf, (i == 0) ? " (default)" : "");
> +#endif
> +
> +        HMAC_Init_ex(hctx, key[i].hmac_key, 16, ngx_ssl_session_ticket_md(),
> +                     NULL);
> +        EVP_DecryptInit_ex(ectx, EVP_aes_128_cbc(), NULL, key[i].aes_key, iv);
> +
> +        return (i == 0) ? 1 : 2 /* renew */;

[...]

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