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