Hello!

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

> # HG changeset patch
> # User Piotr Sikora <piotr@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

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