[asterisk-dev] [Code Review] 3679: WebRTC: Add SHA-256 support, change DTLS-SRTP negotiation, add finer grain control of things.

[Joshua Colp]

> On June 28, 2014, 8:35 p.m., Matt Jordan wrote:
> > /branches/11/res/res_rtp_asterisk.c, lines 1061-1079
> > <https://reviewboard.asterisk.org/r/3679/diff/1/?file=61060#file61060line1061>
> >
> >     Does this leak read_BIO and write_BIO on the rtp/rtcp structs?


> On June 28, 2014, 8:35 p.m., Matt Jordan wrote:
> > /branches/11/res/res_rtp_asterisk.c, lines 868-882
> > <https://reviewboard.asterisk.org/r/3679/diff/1/?file=61060#file61060line868>
> >
> >     I'd create a shared routine that correctly disposes of these three objects on either the rtp or rtcp struct.

The only time these three need to be individually destroyed is during setup. Once associated with the structure the SSL_free function will also free the read_bio and write_bio.

>From the OpenSSL documentation:

SSL_free() also calls the free()ing procedures for indirectly affected items, if applicable: the buffering BIO, the read and write BIOs, cipher lists specially created for this ssl, the SSL_SESSION. Do not explicitly free these indirectly freed up items before or after calling SSL_free(), as trying to free things twice may lead to program failure.


> On June 28, 2014, 8:35 p.m., Matt Jordan wrote:
> > /branches/11/res/res_rtp_asterisk.c, lines 2266-2272
> > <https://reviewboard.asterisk.org/r/3679/diff/1/?file=61060#file61060line2266>
> >
> >     And call a shared destruction routine here

Since the underlying destruction routine would only call SSL_free I'm not sure there would be much value, but after making a common structure it may happen anyway.


- Joshua


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


On June 26, 2014, 3:49 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3679/
> -----------------------------------------------------------
> 
> (Updated June 26, 2014, 3:49 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-22961 and ASTERISK-23026
>     https://issues.asterisk.org/jira/browse/ASTERISK-22961
>     https://issues.asterisk.org/jira/browse/ASTERISK-23026
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This change does the following:
> 
> 1. Adds SHA-256 support for DTLS-SRTP. This is done in an extensible way so if we need to add other hashes it should be relatively easy to.
> 2. Adds the ability to force "AVP" for DTLS streams for greater interoperability.
> 3. Sets the ICE role to controlled or controlling depending on offer/answer.
> 4. Provides the ability to verify only fingerprint, certificate, or both.
> 5. Adds DTLS negotiation to RTCP.
> 6. Changes DTLS negotiation to occur after ICE negotiation completes.
> 7. Adds handling of DTLS traffic before ICE negotiation has formally completed.
> 
> 
> Diffs
> -----
> 
>   /branches/11/res/res_rtp_asterisk.c 417252 
>   /branches/11/main/rtp_engine.c 417252 
>   /branches/11/include/asterisk/rtp_engine.h 417252 
>   /branches/11/configs/sip.conf.sample 417252 
>   /branches/11/channels/sip/include/sip.h 417252 
>   /branches/11/channels/chan_sip.c 417252 
> 
> Diff: https://reviewboard.asterisk.org/r/3679/diff/
> 
> 
> Testing
> -------
> 
> Tested inbound and outbound calls against:
> 
> Chrome
> Yandex Browser
> Opera
> Maxthon
> Firefox
> 
> Note that hold/unhold only currently works against Chrome based browsers.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

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