[asterisk-dev] [Code Review] SRTP support for Asterisk

[Terry Wilson]

> On 2010-05-26 11:38:33, Mark Michelson wrote:
> > /trunk/channels/chan_iax2.c, lines 13640-13641
> > <https://reviewboard.asterisk.org/r/191/diff/3/?file=9802#file9802line13640>
> >
> >     It looks like you missed this item that Russell commented on. You have to use buflen instead of sizeof(buf).
> >     
> >     Edit: On further inspection, this diff does not appear to have any of the corrections Russell suggested in his last review.

Yeah, in the code this is fixed. Somehow it looks like I didn't update the review properly. :-/


> On 2010-05-26 11:38:33, Mark Michelson wrote:
> > /trunk/channels/sip/include/srtp.h, lines 34-36
> > <https://reviewboard.asterisk.org/r/191/diff/3/?file=9807#file9807line34>
> >
> >     Spaces around shift operators.

Fixed


> On 2010-05-26 11:38:33, Mark Michelson wrote:
> > /trunk/channels/sip/include/sdp_crypto.h, lines 33-40
> > <https://reviewboard.asterisk.org/r/191/diff/3/?file=9805#file9805line33>
> >
> >     Please add some doxygen so it is more clear what these functions are intended to do.

Done


> On 2010-05-26 11:38:33, Mark Michelson wrote:
> > /trunk/channels/sip/sdp_crypto.c, lines 93-99
> > <https://reviewboard.asterisk.org/r/191/diff/3/?file=9808#file9808line93>
> >
> >     I'm not an authority on things, but are these conditions severe enough that they should warrant freeing p and returning NULL?

I wish I was an authority... But, that certainly sounds reasonable to me. Done.


> On 2010-05-26 11:38:33, Mark Michelson wrote:
> > /trunk/funcs/func_channel.c, lines 357-364
> > <https://reviewboard.asterisk.org/r/191/diff/3/?file=9812#file9812line357>
> >
> >     I think you should add an else block here with at least a warning message stating that the data could not be retrieved.

I'm not sure I agree. The default is that things definitely aren't "secure". This includes channels that don't support any kind of secure signaling/media. There is nothing wrong with the datastore not being there, it is the default. All we care about is if something is secure or not. Not why it isn't.


> On 2010-05-26 11:38:33, Mark Michelson wrote:
> > /trunk/channels/sip/srtp.c, lines 39-41
> > <https://reviewboard.asterisk.org/r/191/diff/3/?file=9809#file9809line39>
> >
> >     As with Russell's earlier comment, there is no need to print an error message since ast_calloc will do this too.

Done.


- Terry


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


On 2010-05-04 20:07:15, Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/191/
> -----------------------------------------------------------
> 
> (Updated 2010-05-04 20:07:15)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> SRTP support for Asterisk using Sdescriptions. This has been sitting around for a while, so I figured that it should at least get some review.  Full description of setup at http://lists.digium.com/pipermail/asterisk-dev/2009-January/036029.html
> 
> 
> This addresses bug 5413.
>     https://issues.asterisk.org/view.php?id=5413
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 259665 
>   /trunk/build_tools/menuselect-deps.in 259665 
>   /trunk/channels/chan_iax2.c 259665 
>   /trunk/channels/chan_sip.c 259665 
>   /trunk/channels/sip/dialplan_functions.c 259665 
>   /trunk/channels/sip/include/sdp_crypto.h PRE-CREATION 
>   /trunk/channels/sip/include/sip.h 259665 
>   /trunk/channels/sip/include/srtp.h PRE-CREATION 
>   /trunk/channels/sip/sdp_crypto.c PRE-CREATION 
>   /trunk/channels/sip/srtp.c PRE-CREATION 
>   /trunk/configure UNKNOWN 
>   /trunk/configure.ac 259665 
>   /trunk/funcs/func_channel.c 259665 
>   /trunk/include/asterisk/autoconfig.h.in 259665 
>   /trunk/include/asterisk/frame.h 259665 
>   /trunk/include/asterisk/global_datastores.h 259665 
>   /trunk/include/asterisk/res_srtp.h PRE-CREATION 
>   /trunk/include/asterisk/rtp_engine.h 259665 
>   /trunk/main/asterisk.exports.in 259665 
>   /trunk/main/channel.c 259665 
>   /trunk/main/global_datastores.c 259665 
>   /trunk/main/rtp_engine.c 259665 
>   /trunk/makeopts.in 259665 
>   /trunk/res/res_rtp_asterisk.c 259665 
>   /trunk/res/res_srtp.c PRE-CREATION 
>   /trunk/res/res_srtp.exports.in PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/191/diff
> 
> 
> Testing
> -------
> 
> 4 external tests written covering:
> Running with res_srtp noloaded to emulate a user not having libsrtp installed (to make sure we don't accidentally rely on SRTP support)
> Making a call with a user with encrypted=yes when libsrtp support is not enabled fails
> Making a call with encrypted=yes when libsrtp present results in an encrypted call (which also tests the CHANNEL(secure_media) function
> Using CHANNEL(secure_bridge_media) results in the outgoing call attempting to use encryption
> 
> In addition, I have tested a Polycom VVX-1500 to ensure that video + audio SRTP works.
> 
> 
> Thanks,
> 
> Terry
> 
>