[Asterisk-code-review] chan sip: Allow Preferred sRTP. (asterisk[master])

[Walter Doekes]

Walter Doekes has posted comments on this change.

Change subject: chan_sip: Allow Preferred sRTP.
......................................................................


Patch Set 3: Code-Review-1

(1 comment)

(Downvoting because this patch is broken, not because I'm against suppoorting this optional srtp per se.)

Resuming the conversation from r3830 here:

Alexander wrote:
> Walter, please clarify why you went for this approach and essentially killed [this review].
> ...
> This means, Preferred sRTP is ignored although it could be leveraged.

In ASTERISK-20234, on 6 sept. 2012, Matt Jordan wrote:
> I'm more on the side of thought that says if you want SRTP, you should negotiate for it properly.

Alexander wrote:
> This is not yet supported by Asterisk, but this is the rationale behind automatic/optional on some user-agents; those clients always use the best encryption offered by the remote proxy. If Asterisk is the fall-back outbound proxy for such a client and if Asterisk requires encryption, direct URI calling breaks.

I don't mind optional encryption, of course not. But it appears to me -- I didn't check the RFCs just now -- that optional SRTP should be negotiated by two m= lines, one with SAVP and one without.

Neither of these patches address that SDP scenario.

Like Joshua said: the behaviour change that occurred when loading the module was unacceptable to us. Beta-testing SRTP support on the platform would suddenly involve all SNOM phone owners if this original patch were applied on 11.

https://gerrit.asterisk.org/#/c/3234/3/channels/chan_sip.c
File channels/chan_sip.c:

PS3, Line 10616: 						if (secure_audio == FALSE) {
               : 							ast_log(AST_LOG_NOTICE, "Processed audio crypto attribute without SAVP specified; accepting anyway\n");
               : 							secure_audio = TRUE;
               : 						}
This won't work anymore now that secure_audio has been passed to process_crypto. If it were false, we won't be here.

(Same below with secure_video.)


-- 
To view, visit https://gerrit.asterisk.org/3234
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I42cb779df3a9c7b3dd03a629fb3a296aa4ceb0fd
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Walter Doekes <walter+asterisk at wjd.nu>
Gerrit-HasComments: Yes