[asterisk-dev] [Code Review] rfc 4474 patch

[Russell Bryant]

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



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/596/#comment3894>

    I'd rather not make chan_sip depend on libcurl.  It's fine for it to be an optional dependency, though.  To do that, instead of <depend>, put <use>.  Then, the code that uses it will have to be wrapped with #ifdef HAVE_CURL.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/596/#comment3895>

    This code assumes that libssl is available, which may not be true.  There are a number of other places that have the same issue.



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/596/#comment3896>

    There are some coding guidelines issues here (and throughout the patch).  I would recommend reviewing doc/CODING-GUIDELINES for formatting recommendations.
    
    "const char* foo" should be "const char *foo"
    
    There should be a space after the switch keyword.
    
    case statements should be aligned with the switch statement.
    
    There should be spaces around '='



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/596/#comment3897>

    no parens around "rv" here



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/596/#comment3898>

    Use ast_copy_string() instead of strncpy()



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/596/#comment3900>

    How do we know that we can trust the URL that we were given for downloading the public key?  Should there be some configuration to allow some control over security policy with this?



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/596/#comment3899>

    Use ast_verb(), though this looks like debug messages



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/596/#comment3901>

    The ast_str() API should be used instead of this manual string building stuff



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/596/#comment3903>

    Try compiling this with ./configure --enable-dev-mode.  This code will produce a warning that should be fixed.



/trunk/configs/sip.conf.sample
<https://reviewboard.asterisk.org/r/596/#comment3902>

    fix up the whitespace issues pointed out by reviewboard in the sample config


- Russell


On 2010-03-30 14:25:43, Ed Guy wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/596/
> -----------------------------------------------------------
> 
> (Updated 2010-03-30 14:25:43)
> 
> 
> Review request for Asterisk Developers, Russell Bryant and Olle E Johansson.
> 
> 
> Summary
> -------
> 
> 
> This is a patch to trunk which implements the subset of RFC 4474 suitable for 
> asterisk calling number authentication.   It can sign SIP INVITEs and verify the 
> signatures of incoming INVITES.  The current implementation implements much 
> of the RFC. The result of the signing and validation is placed in a channel 
> variable ( IDENTITY_RESULT )
> for dialplan interpretation. This allows the dialplan to authenticate the 
> CallerID and prevent SPIT.
> 
> The private keys for signing are taken from 
> a local directory, a cache, or an http server. The private key is used 
> to sign a digest string. The certificate is retrieved from the location 
> specified in the Identity-Info header, or from the cache, if present. 
> SSL is not currently supported. 
> 
> 
> This addresses bug 0016187.
>     https://issues.asterisk.org/view.php?id=0016187
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 255321 
>   /trunk/channels/sip/include/sip.h 255321 
>   /trunk/configs/extensions.conf.sample 255321 
>   /trunk/configs/sip.conf.sample 255321 
> 
> Diff: https://reviewboard.asterisk.org/r/596/diff
> 
> 
> Testing
> -------
> 
> This patch has been tested between multiple asterisk instances and 
> with in-charges reference system which runs openser. 
> 
> 
> Thanks,
> 
> Ed
> 
>