[asterisk-dev] [Code Review] 3647: Enable Forward Secrecy (PFS) in TLS

[Alexander Traud]

> On June 19, 2014, 2:13 p.m., Matt Jordan wrote:
> > branches/12/main/tcptls.c, lines 435-447
> > <https://reviewboard.asterisk.org/r/3647/diff/1/?file=59799#file59799line435>
> >
> >     Given how tricky it is to set up TLS support, I'd go ahead and add an off nominal handler for BIO_new_file here. Something like:
> >     
> >     if (bio != NULL) {
> >      ...
> >     } else {
> >         ast_log(LOG_WARNING, "Failed to open  private key file %s: %s(%d)\n", cfg->pvtfile, strerror(errno), errno);
> >     }
> 
> Alexander Traud wrote:
>     The private file was already opened (and used) some lines above. I am just re-using the reference.
>     The idea of this patch is to fail silently, when there are no DH parameters because those affect performance. Or stated differently, DH parameters are optional. Only administrators (who care) should enable it (and see success). Furthermore in Asterisk 12 chan_sip, the private file itself is optional. This is a bit different to pjsip were the TLS private file is mandatory.
> 
> Matt Jordan wrote:
>     Looking at where we use cfg->pvtfile above:
>     
>     		char *tmpprivate = ast_strlen_zero(cfg->pvtfile) ? cfg->certfile : cfg->pvtfile;
>     		if (SSL_CTX_use_certificate_chain_file(cfg->ssl_ctx, cfg->certfile) == 0) {
>     			if (!client) {
>     				/* Clients don't need a certificate, but if its setup we can use it */
>     				ast_verb(0, "SSL error loading cert file. <%s>\n", cfg->certfile);
>     				cfg->enabled = 0;
>     				SSL_CTX_free(cfg->ssl_ctx);
>     				cfg->ssl_ctx = NULL;
>     				return 0;
>     			}
>     		}
>     
>     So yes, if this is a TLS server and if cfg->pvtfile is specified, we will emit an error.
>     
>     However, if this is a client, we won't emit any error if the pvtfile is specified but cannot be opened. Since in your case a client could specify a pvtfile and the open could fail (again) when BIO_new_file is called, a warning message at least is appropriate. The user configured Asterisk to use a private key file and we failed to access it. Telling them that they have a configuration error is a nice thing to do, as opposed to silently moving on.

I am sorry, I do not get your point, yet. This patch is a no-op for TLS clients. Consequently, I see no case of an configuration error which this patch adds for this case. Therefore, I did not add any warning/error/notice.

For the TLS server case, there is no message if cfg->pvtfile (sip.conf tlsprivatekey) is missing because chan_sip falls back to cfg->certfile (sip.conf tlscertfile). It is OK (and expected) that BIO_new_file fails silently because DH cipher suites shall stay optional.


- Alexander


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3647/#review12192
-----------------------------------------------------------


On June 19, 2014, 2:34 p.m., Alexander Traud wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3647/
> -----------------------------------------------------------
> 
> (Updated June 19, 2014, 2:34 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23905
>     https://issues.asterisk.org/jira/browse/ASTERISK-23905
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> see Bugs
> 
> 
> Diffs
> -----
> 
>   trunk/main/tcptls.c 416071 
> 
> Diff: https://reviewboard.asterisk.org/r/3647/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alexander Traud
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140620/7763625c/attachment.html>