<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1741/">https://reviewboard.asterisk.org/r/1741/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'm with the idea in spirit, but API and ABI changes like these should not happen in the midst of a release series. My feelings on this are the same as I had on an issue of Terry's recently. If you can make the change by adding to the API without having to change existing functions, that would be preferred. I also feel like you should withhold from adding to the ast_srtp_res if at all possible in the release branches.</pre>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/1741/diff/2/?file=24291#file24291line1840" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/include/asterisk/rtp_engine.h</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">struct ast_channel *ast_rtp_instance_get_chan(struct ast_rtp_instance *instance);</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1840</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * \param remote_policy The policy for the remote endpoint</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1841</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * \param local_policy Our policy for the remote endpoint</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Typo on line 1841. Should be "local" instead of "remote"</pre>
</div>
<br />
<p>- Mark</p>
<br />
<p>On February 16th, 2012, 11:39 a.m., Matt Jordan wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers, Joshua Colp, Mark Michelson, and otherwiseguy.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated Feb. 16, 2012, 11:39 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Currently, when using res_srtp, once the SRTP policy has been added to the current session the policy is locked into place. Any attempt to replace an existing policy, which would be needed if the remote endpoint negotiated a new cryptographic key, is instead rejected in res_srtp. We thus need to have a mechanism to replace an existing policy associated with either a local or remote endpoint.
If we needed to change the key for a local policy (which we don't do, but its a hypothetical situation), changing the crytographic key is easy. The old stream associated with the local SSRC is removed, and a new stream is added. The libsrtp library has straightforward calls for this.
However, for the remote policy, it isn't as easy. Because our remote policy uses a wildcard type to match on any inbound SSRC value, it can't be replaced. The libsrtp library explicitly disallows changing wildcard policies (as the policies are applied to all streams matching the respective inbound/outbound direction, so replacing a wildcard policy would entail replacing all streams associated with that policy). As such, the only thing that can be done is to deallocate the old session and create a new session.
The replaces one problem with another - while we can replace the old session with a new session and a new remote policy, we would effectively destroy our local policy / stream when we do that. Currently, there is not an imposed order on when the local/remote policies are added (and in fact, we add the local one before the remote one currently). Thus, when replacing an SRTP session with a new one, the order in which things are done has to be:
1) Destroy the old SRTP session
2) Create a new SRTP session and add one of the two policies
3) Add the other policy
If we don't add both policies at the same time, we could end up in a situation where we set the local policy, and then set the remote policy, blowing out the previously added local policy. Good times.
This patch does the following:
1) It combines the adding of remote/local policies onto an SRTP session. Although this changes the rtp_engine API, in effect its minor, as the two were always added at the same time anyway by users of the API. This allows us to control the order in which things are added in res_srtp, and users of the API don't have to worry.
2) It adds a new virtual method to the res_srtp API, replace. This combines the destroy/create methods.
3) We now check for the type of stream we are adding. If we are adding a policy for a specific SSRC, we replace the existing stream. If we are adding a policy for a wildcard, we bail out if we already have a policy existing for that wildcard; otherwise we add it.
This patch does some other cleanup in unprotect and unload module, including toning down the log statements and shutting down the libsrtp library on unload.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Made sure that the initial patch didn't break the SRTP test in the TestSuite. Made sure that the module could be loaded and unloaded.
More testing is needed to make sure that the whole thing actually works.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/1.8/include/asterisk/res_srtp.h <span style="color: grey">(354547)</span></li>
<li>/branches/1.8/include/asterisk/rtp_engine.h <span style="color: grey">(354547)</span></li>
<li>/branches/1.8/main/rtp_engine.c <span style="color: grey">(354547)</span></li>
<li>/branches/1.8/res/res_srtp.c <span style="color: grey">(354547)</span></li>
<li>/branches/1.8/channels/sip/sdp_crypto.c <span style="color: grey">(354547)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1741/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>