[asterisk-dev] [Code Review] 4441: Enable TLS Dual-Certificates (ECC+RSA)

[rmudgett]

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


See coding guidelines: https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines


trunk/main/tcptls.c
<https://reviewboard.asterisk.org/r/4441/#comment25080>

    Please put a blank line between variable declarations and code.
    
    ast_strdup() allocates memory from the heap and that allocation can fail.  There is no NULL pointer check for allocation failure.  If you use ast_strdupa() instead you will allocate from the stack and not have to worry about allocation failure or having to free the memory.
    
    CamelCase is not used per Asterisk coding guidelines.  The names should be ecc_file and ecc_file_len.



trunk/main/tcptls.c
<https://reviewboard.asterisk.org/r/4441/#comment25085>

    It doesn't look like there is a standard cert file naming convention to name the files example_rsa.pem, example_ecc.pem, and example_dsa.pem.  The patch assumes this naming convention.  The patch should verify that cfg->certfile name is in this format before trying.  i.e. Check that cfg->certfile ends with "_rsa.pem".
    
    This should be documented in the sample config files (pjsip.conf.sample and sip.conf.sample at least).  For pjsip the online documentation should be updated in res_pjsip.c.



trunk/main/tcptls.c
<https://reviewboard.asterisk.org/r/4441/#comment25084>

    Probably should check if the file exists first before attempting to load it as a cert file.  That way if it doesn't exist the error loading the cert file message won't happen unless it actually exists.
    
    The same should be done for the dsa cert file.



trunk/main/tcptls.c
<https://reviewboard.asterisk.org/r/4441/#comment25082>

    This should be:
    ast_log(LOG_ERROR, "TLS/SSL error...
    



trunk/main/tcptls.c
<https://reviewboard.asterisk.org/r/4441/#comment25081>

    Guidelines: Declaring variables in the middle of a block is not allowed.
    
    Same here for ast_strdup() allocation failure check.
    
    It seems like a waste to allocate the eccFile then free it then allocate the dsaFile then free it.  Why not allocate the duplicate file once and modify it for each cert file attempt.



trunk/main/tcptls.c
<https://reviewboard.asterisk.org/r/4441/#comment25083>

    This should be:
    ast_log(LOG_ERROR, "TLS/SSL error...
    


- rmudgett


On Feb. 23, 2015, 10:10 a.m., Alexander Traud wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4441/
> -----------------------------------------------------------
> 
> (Updated Feb. 23, 2015, 10:10 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24815
>     https://issues.asterisk.org/jira/browse/ASTERISK-24815
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Already works for Asterisk as the client. Enables dual- (or triple-) certificates for Asterisk as the TLS server. When a client connects via SSL/TLS, the server uses a RSA key-pair usually. However, more such algorithms exist like DSA and ECDSA. If you go for one of those, you would loose compatibility to RSA-only clients. This patch allows you to provide up-to one RSA, ECDSA and DSA key each (= one key or two keys or three keys). Copied over from the Apache HTTP server project, added in version 2.4.8.
> 
> Usage:
> tlscertfile=/etc/asterisk/example_rsa.pem
> Then, the code of this patch picks that path, filename, and searches for files called example_ecc.pem and example_dsa.pem automatically.
> 
> 
> Diffs
> -----
> 
>   trunk/main/tcptls.c 431938 
> 
> Diff: https://reviewboard.asterisk.org/r/4441/diff/
> 
> 
> Testing
> -------
> 
> by developer, manually
> 
> This patch was tested in Ubuntu 14.04 LTS with a certificate from Comodo (ECC; chains-up to AddTrust and UTN) and RapidSSL (RSA; chains-up to GeoTrust and Equifax). TLS clients were CounterPath Bria (BlackBerry) and CSipSimple (Android). The test was done with OpenSSL 1.0.1 and OpenSSL 1.0.2. Both versions work as expected. However, if you use well-known (commercial) certificates, you might use different certificate chains. For this, you need at least OpenSSL 1.0.2. If you use your own certificate authority without a certificate chain, OpenSSL 1.0.1 is sufficient.
> 
> Because no new symbol of OpenSSL was used, I do not see a reason why this patch should not be compatible with older OpenSSL releases. Therefore, no if/def/version is introduced in this patch.
> 
> 
> Thanks,
> 
> Alexander Traud
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150224/091af171/attachment-0001.html>