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@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

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel