> 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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
