<div dir="ltr">Do we really should fail configuration check if we were not able to read ticket key file?<br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Oct 14, 2013 at 5:29 PM, Maxim Dounin <span dir="ltr"><<a href="mailto:mdounin@mdounin.ru" target="_blank">mdounin@mdounin.ru</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello!<br>
<div class="im"><br>
On Fri, Oct 11, 2013 at 04:22:07PM -0700, Piotr Sikora wrote:<br>
<br>
> Hey Maxim,<br>
><br>
> > Wouldn't it better to move ngx_ssl_session_ticket_md defines<br>
> > to ngx_ssl_session_ticket_key_callback() implementation?<br>
><br>
> You mean inside the function or just above it? I moved them just above it.<br>
<br>
</div>I'm fine with either variant.<br>
<div class="im"><br>
> > Do we really need these #ifdef's? I don't think saving several<br>
> > bytes in ancient OpenSSL versions worth it (and the<br>
> > ngx_ssl_stapling_index doesn't have #ifdef's).<br>
><br>
> Probably not, removed.<br>
><br>
> While there, I moved the definitions and initialization right after<br>
> ngx_ssl_session_cache_index, so that the two session resumption<br>
> indexes are together, hopefully that's alright.<br>
<br>
</div>Looks good.<br>
<div class="im"><br>
> > Wouldn't ngx_ssl_ticket_key_t be better? Up to you, but<br>
> > "ngx_ssl_session_ticket_key_t" looks a bit too long.<br>
><br>
> It's a bit too long, yes, but I prefer to keep everything with the<br>
> same prefix, so either rename everything to "ngx_ssl_ticket_" (which I<br>
> don't like that much) or keep long names.<br>
<br>
</div>I think it would be fine to name functions ngx_ssl_session_ticket_keys()<br>
and ngx_ssl_session_ticket_key_callback(), but use shorter names for<br>
the rest of the identifiers. Especially keeping in mind OpenSSL<br>
calls relevant call SSL_CTX_set_tlsext_ticket_key_cb() anyway.<br>
But I don't insist, up to you.<br>
<div class="im"><br>
> > What about something like this:<br>
> ><br>
> > ngx_log_debug3(NGX_LOG_DEBUG_HTTP, c->log, 0,<br>
> > "ssl session ticket encrypt, key: \"%*s\" (%s session)",<br>
> > ngx_hex_dump(buf, key[0].name, 16) - buf, buf,<br>
> > SSL_session_reused(ssl_conn) ? "reused" : "new");<br>
> ><br>
> > ?<br>
> ><br>
> > (Well, actually I'm not sure we need key name logged, but it<br>
> > probably doesn't matter much.)<br>
><br>
> That's much better, thanks.<br>
><br>
> It's used only for the debugging, so I think it's fine to print it.<br>
<br>
</div>Sometimes too much debugging is bad, too. But let's keep it as is<br>
for now.<br>
<div class="im"><br>
> > Just in case, using a shorter name "ngx_ssl_ticket_md()" would<br>
> > save us from wrapping here and in decrypt case.<br>
><br>
> I know, I had something shorter here originally, but I've decided to<br>
> rename it, so that it would have the same prefix... But if you really<br>
> want to get rid of the wrap, maybe we could use "ngx_ssl_md()" here?<br>
> What do you think?<br>
<br>
</div>The "ngx_ssl_md()" looks wrong for me, as it's a message digest<br>
used for session tickets, not something global for our ssl<br>
implementation.<br>
<div class="im"><br>
> > Shouldn't it be ngx_memcmp(), as it checks binary data now?<br>
><br>
> Good catch, thanks! That was leftover from my previous implementation.<br>
><br>
> > Adding "goto found" instead of "break" would allow to avoid this<br>
> > additional check. Up to you though.<br>
><br>
> Personally, I prefer the explicit "end-of-the-loop" check, but I don't<br>
> mind either way. Changed to "goto found".<br>
<br>
</div>Minor nit: labels should be indented to match blocks, not<br>
unindented. E.g.,<br>
<br>
if (foo) {<br>
if (bar) {<br>
goto found;<br>
}<br>
<br>
found:<br>
<br>
...<br>
<div class="im"> }<br>
<br>
> Updated patch attached, with slightly changed commit message (better<br>
> file names in the example).<br>
><br>
> Please note, that it doesn't have any session timeout logic, so it<br>
> should be committed after the timeout patch :)<br>
<br>
</div>Sure.<br>
<div class="im"><br>
> # HG changeset patch<br>
> # User Piotr Sikora <<a href="mailto:piotr@cloudflare.com">piotr@cloudflare.com</a>><br>
> # Date 1381532724 25200<br>
> # Fri Oct 11 16:05:24 2013 -0700<br>
> # Node ID 296805806a43f4b222c4cf34dd6489b37394e315<br>
> # Parent 5483d9e77b3287b00b1104a07688bda37bc7351e<br>
> SSL: added ability to set keys used for Session Tickets (RFC5077).<br>
><br>
> In order to support key rollover, ssl_session_ticket_key can be defined<br>
> multiple times. The first key will be used to issue and resume Session<br>
> Tickets, while the rest will be used only to resume them.<br>
><br>
> ssl_session_ticket_key session_tickets/current.key;<br>
> ssl_session_ticket_key session_tickets/prev-1h.key;<br>
> ssl_session_ticket_key session_tickets/prev-2h.key;<br>
><br>
> Please note that nginx supports Session Tickets even without explicit<br>
> configuration of the keys and this feature should be only used in setups<br>
> where SSL traffic is distributed across multiple nginx servers.<br>
><br>
> Signed-off-by: Piotr Sikora <<a href="mailto:piotr@cloudflare.com">piotr@cloudflare.com</a>><br>
<br>
</div>[...]<br>
<br>
Committed with the followin minor tweaks:<br>
<br>
--- a/src/event/ngx_event_openssl.c<br>
+++ b/src/event/ngx_event_openssl.c<br>
@@ -2415,8 +2415,8 @@ ngx_ssl_session_ticket_key_callback(ngx_<br>
<br>
RAND_pseudo_bytes(iv, 16);<br>
<div class="im"> EVP_EncryptInit_ex(ectx, EVP_aes_128_cbc(), NULL, key[0].aes_key, iv);<br>
- HMAC_Init_ex(hctx, key[0].hmac_key, 16, ngx_ssl_session_ticket_md(),<br>
</div>- NULL);<br>
<div class="im">+ HMAC_Init_ex(hctx, key[0].hmac_key, 16,<br>
</div>+ ngx_ssl_session_ticket_md(), NULL);<br>
<div class="im"> memcpy(name, key[0].name, 16);<br>
<br>
</div> return 0;<br>
@@ -2436,15 +2436,15 @@ ngx_ssl_session_ticket_key_callback(ngx_<br>
<br>
return 0;<br>
<br>
-found:<br>
+ found:<br>
<br>
ngx_log_debug3(NGX_LOG_DEBUG_HTTP, c->log, 0,<br>
<div class="im"> "ssl session ticket decrypt, key: \"%*s\"%s",<br>
</div><div class="im"> ngx_hex_dump(buf, key[i].name, 16) - buf, buf,<br>
</div><div class="im"> (i == 0) ? " (default)" : "");<br>
<br>
</div><div class="im">- HMAC_Init_ex(hctx, key[i].hmac_key, 16, ngx_ssl_session_ticket_md(),<br>
</div>- NULL);<br>
<div class="im">+ HMAC_Init_ex(hctx, key[i].hmac_key, 16,<br>
</div>+ ngx_ssl_session_ticket_md(), NULL);<br>
<div class="im"> EVP_DecryptInit_ex(ectx, EVP_aes_128_cbc(), NULL, key[i].aes_key, iv);<br>
<br>
</div><div class="im"> return (i == 0) ? 1 : 2 /* renew */;<br>
<br>
</div>Thanks!<br>
<div class="im HOEnZb"><br>
--<br>
Maxim Dounin<br>
<a href="http://nginx.org/en/donation.html" target="_blank">http://nginx.org/en/donation.html</a><br>
<br>
</div><div class="HOEnZb"><div class="h5">_______________________________________________<br>
nginx-devel mailing list<br>
<a href="mailto:nginx-devel@nginx.org">nginx-devel@nginx.org</a><br>
<a href="http://mailman.nginx.org/mailman/listinfo/nginx-devel" target="_blank">http://mailman.nginx.org/mailman/listinfo/nginx-devel</a><br>
</div></div></blockquote></div><br></div>