[Asterisk-code-review] res_rtp_asterisk: bad audio (static) due to incomplete dtls/srtp setup (asterisk[16])

[Kevin Harwell]

Hello Joshua Colp, Friendly Automation, 

I'd like you to reexamine a change. Please visit

    https://gerrit.asterisk.org/c/asterisk/+/13805

to look at the new patch set (#2).

Change subject: res_rtp_asterisk: bad audio (static) due to incomplete dtls/srtp setup
......................................................................

res_rtp_asterisk: bad audio (static) due to incomplete dtls/srtp setup

There was a race condition between client initiated DTLS setup, and handling
of server side ice completion that caused the underlying SSL object to get
cleared during DTLS initialization. If this happened Asterisk would be left
in a partial DTLS setup state. RTP packets were sent and received, but were
not being encrypted and decrypted. This resulted in no audio, or static.

Specifically, this occurred when '__rtp_recvfrom' was processing the handshake
sequence from the client to the server, and then 'ast_rtp_on_ice_complete'
gets called from another thread and clears the SSL object when calling the
'dtls_perform_setup' function. The timing had to be just right in the sense
that from the external SSL library perspective SSL initialization completed
(rtp recv), Asterisk clears/resets the SSL object (ice done), and then checks
to see if SSL is intialized (rtp recv). Since it was cleared, Asterisk thinks
it is not finished, thus not completing 'dtls_srtp_setup'.

This patch removes calls to 'dtls_perform_setup', which clears the SSL object,
in 'ast_rtp_on_ice_complete'. When ice completes, there is no reason to clear
the underlying SSL object. If an ice candidate changes a full protocol level
renegotiation occurs. Also, in the case of bundled ICE candidates are reused
when a stream is added. So no real reason to have to clear, and reset in this
instance.

Also, this patch adds a bit of extra logging to aid in diagnosis of any future
problems.

ASTERISK-28742 #close

Change-Id: I34c9e6bad5a39b087164646e2836e3e48fe6892f
---
M res/res_rtp_asterisk.c
1 file changed, 28 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/05/13805/2
-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/13805
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I34c9e6bad5a39b087164646e2836e3e48fe6892f
Gerrit-Change-Number: 13805
Gerrit-PatchSet: 2
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-CC: Richard Mudgett <rmudgett at digium.com>
Gerrit-MessageType: newpatchset
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200214/10caf2e2/attachment.html>