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

[Maxim Dounin]

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