[asterisk-dev] [Code Review]: Add IPv6 Address Support To Security Events Framework

[elguero]

> On Feb. 29, 2012, 10:08 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, lines 2154-2161
> > <https://reviewboard.asterisk.org/r/1777/diff/1/?file=25107#file25107line2154>
> >
> >     This looks wrong. Why does a function named mansession_ast_sockaddr_remote() not take a struct mansession as parameter? Is it just a wrapper for ast_sockaddr_from_sin()? Is it still necessary?

Yes, this probably could be removed altogether.  I made this change to try and match what was previously being done and didn't take another look at it.  It really is just a wrapper and we could call ast_sockaddr_from_sin() directly.


> On Feb. 29, 2012, 10:08 a.m., Simon Perreault wrote:
> > /trunk/main/security_events.c, line 513
> > <https://reviewboard.asterisk.org/r/1777/diff/1/?file=25108#file25108line513>
> >
> >     Please add parens around the or-condition to ensure correct operator precedence.

Okay... done.


> On Feb. 29, 2012, 10:08 a.m., Simon Perreault wrote:
> > /trunk/main/security_events.c, lines 527-528
> > <https://reviewboard.asterisk.org/r/1777/diff/1/?file=25108#file25108line527>
> >
> >     You could just make two calls to ast_str_append() to remove the need for temporary variables.

Okay... done.


> On Feb. 29, 2012, 10:08 a.m., Simon Perreault wrote:
> > /trunk/main/manager.c, line 2157
> > <https://reviewboard.asterisk.org/r/1777/diff/1/?file=25107#file25107line2157>
> >
> >     Would it be possible to remove this call to ast_sockaddr_from_sin()? I would expect that as modules become IPv6-ready such calls would disappear...

I am not sure... I thought I did try to check if manager.c had been changed to support IPv6 yet.  Unless I am missing something, this module hasn't been changed yet.  I see ast_sockaddr_from_sin() and ast_sockaddr_to_sin() throughout the code.  So, I think this call still needs to stay for the time being since I am just touching the security events side of things.


> On Feb. 29, 2012, 10:08 a.m., Simon Perreault wrote:
> > /trunk/channels/chan_sip.c, line 3445
> > <https://reviewboard.asterisk.org/r/1777/diff/1/?file=25104#file25104line3445>
> >
> >     Why is this new check necessary?

Without this check, a warning message is generated saying to remove localaddr and/or externaddr settings.  But, we can't do that if we are supporting both IPv4 and IPv6.  We still need NAT functionality on IPv4 addresses, unless I am doing something wrong.  Therefore, if I am doing udpbindaddr=::, this message is a bit of a nuisance.

Unless there is a better way?


- elguero


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


On Feb. 29, 2012, 9:43 a.m., elguero wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1777/
> -----------------------------------------------------------
> 
> (Updated Feb. 29, 2012, 9:43 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> The current API supports only IPv4 for security events. 
> 
> *  This patch changes the API to support IPv6 and updates the components that use this API.
> 
> *  It quiets a warning that is being displayed if binding to IPv6 and IPv4 ( udpbindaddr=:: ). 
> 
> *  It also eliminates an error that was being generated since the current implementation was treating an IPv6 socket address as if it was IPv4.
> 
> *  Some copyright dates were updated as well.
> 
> 
> This addresses bug ASTERISK-19447.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19447
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 357541 
>   /trunk/channels/chan_sip.c 357541 
>   /trunk/channels/sip/security_events.c 357541 
>   /trunk/include/asterisk/security_events_defs.h 357541 
>   /trunk/main/manager.c 357541 
>   /trunk/main/security_events.c 357541 
> 
> Diff: https://reviewboard.asterisk.org/r/1777/diff
> 
> 
> Testing
> -------
> 
> Tested SIP implementation on CentOS 5.7, connected with IPv4 clients and IPv6 clients.  Also tested SIP on Fedora 16 vm with IPv4.  Security log now shows IPv4 or IPv6 addresses when a security event is generated.
> 
> 
> Thanks,
> 
> elguero
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120229/1ea12788/attachment-0001.htm>