Do we really should fail configuration check if we were not able to read
ticket key file?


On Mon, Oct 14, 2013 at 5:29 PM, Maxim Dounin <mdounin@mdounin.ru> wrote:

> Hello!
>
> On Fri, Oct 11, 2013 at 04:22:07PM -0700, Piotr Sikora wrote:
>
> > Hey Maxim,
> >
> > > Wouldn't it better to move ngx_ssl_session_ticket_md defines
> > > to ngx_ssl_session_ticket_key_callback() implementation?
> >
> > You mean inside the function or just above it? I moved them just above
> it.
>
> I'm fine with either variant.
>
> > > 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).
> >
> > Probably not, removed.
> >
> > While there, I moved the definitions and initialization right after
> > ngx_ssl_session_cache_index, so that the two session resumption
> > indexes are together, hopefully that's alright.
>
> Looks good.
>
> > > Wouldn't ngx_ssl_ticket_key_t be better? Up to you, but
> > > "ngx_ssl_session_ticket_key_t" looks a bit too long.
> >
> > It's a bit too long, yes, but I prefer to keep everything with the
> > same prefix, so either rename everything to "ngx_ssl_ticket_" (which I
> > don't like that much) or keep long names.
>
> I think it would be fine to name functions ngx_ssl_session_ticket_keys()
> and ngx_ssl_session_ticket_key_callback(), but use shorter names for
> the rest of the identifiers. Especially keeping in mind OpenSSL
> calls relevant call SSL_CTX_set_tlsext_ticket_key_cb() anyway.
> But I don't insist, up to you.
>
> > > 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.)
> >
> > That's much better, thanks.
> >
> > It's used only for the debugging, so I think it's fine to print it.
>
> Sometimes too much debugging is bad, too. But let's keep it as is
> for now.
>
> > > Just in case, using a shorter name "ngx_ssl_ticket_md()" would
> > > save us from wrapping here and in decrypt case.
> >
> > I know, I had something shorter here originally, but I've decided to
> > rename it, so that it would have the same prefix... But if you really
> > want to get rid of the wrap, maybe we could use "ngx_ssl_md()" here?
> > What do you think?
>
> The "ngx_ssl_md()" looks wrong for me, as it's a message digest
> used for session tickets, not something global for our ssl
> implementation.
>
> > > Shouldn't it be ngx_memcmp(), as it checks binary data now?
> >
> > Good catch, thanks! That was leftover from my previous implementation.
> >
> > > Adding "goto found" instead of "break" would allow to avoid this
> > > additional check. Up to you though.
> >
> > Personally, I prefer the explicit "end-of-the-loop" check, but I don't
> > mind either way. Changed to "goto found".
>
> Minor nit: labels should be indented to match blocks, not
> unindented. E.g.,
>
> if (foo) {
> if (bar) {
> goto found;
> }
>
> found:
>
> ...
> }
>
> > Updated patch attached, with slightly changed commit message (better
> > file names in the example).
> >
> > Please note, that it doesn't have any session timeout logic, so it
> > should be committed after the timeout patch :)
>
> Sure.
>
> > # HG changeset patch
> > # User Piotr Sikora <piotr@cloudflare.com>
> > # Date 1381532724 25200
> > # Fri Oct 11 16:05:24 2013 -0700
> > # Node ID 296805806a43f4b222c4cf34dd6489b37394e315
> > # 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_tickets/current.key;
> > ssl_session_ticket_key session_tickets/prev-1h.key;
> > ssl_session_ticket_key session_tickets/prev-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.
> >
> > Signed-off-by: Piotr Sikora <piotr@cloudflare.com>
>
> [...]
>
> Committed with the followin minor tweaks:
>
> --- a/src/event/ngx_event_openssl.c
> +++ b/src/event/ngx_event_openssl.c
> @@ -2415,8 +2415,8 @@ ngx_ssl_session_ticket_key_callback(ngx_
>
> 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);
> + HMAC_Init_ex(hctx, key[0].hmac_key, 16,
> + ngx_ssl_session_ticket_md(), NULL);
> memcpy(name, key[0].name, 16);
>
> return 0;
> @@ -2436,15 +2436,15 @@ ngx_ssl_session_ticket_key_callback(ngx_
>
> return 0;
>
> -found:
> + found:
>
> ngx_log_debug3(NGX_LOG_DEBUG_HTTP, c->log, 0,
> "ssl session ticket decrypt, key: \"%*s\"%s",
> ngx_hex_dump(buf, key[i].name, 16) - buf, buf,
> (i == 0) ? " (default)" : "");
>
> - HMAC_Init_ex(hctx, key[i].hmac_key, 16,
> ngx_ssl_session_ticket_md(),
> - NULL);
> + 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 */;
>
> Thanks!
>
> --
> Maxim Dounin
> http://nginx.org/en/donation.html
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel