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