[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/#review15180
-----------------------------------------------------------



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

    These don't need an initialization value since they are assigned a value the first time they are used.
    
    Also you could put the declaration of the variables at the start of any block that encloses the lifetime of the variable instead of the beginning of the function.
    
    if () {
      int x;
    
      func();
    } else {
      int y;
    
      func();
    }
    
    
    The guidelines say you cannot declare variables in the middle of code like this:
    
    if () {
      int x; /* x is ok since it is at the beginning of a code block */
      x = 5;
      func();
      int y; /* y is not ok since it is declared in the middle of code */
      y = x + 4;
    }



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

    These two tests can be combined to reduce indentation:
    
    if (certfile_len >= 8
        && !strncmp(...)) {
    }



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

    This is effectively the same code duplicated for ecc and dsa.  Make a subroutine that takes the variants as parameters:
    
    sub(cfg->certfile, certfile_len, "ecc", "ECC")
    
    Also the code is always attempting to use both cert files even if ecc works.


- rmudgett


On March 30, 2015, 3:34 a.m., Alexander Traud wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4441/
> -----------------------------------------------------------
> 
> (Updated March 30, 2015, 3:34 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 
>   trunk/configs/samples/sip.conf.sample 428526 
> 
> 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/20150410/bfd7cc56/attachment-0001.html>