[Asterisk-code-review] res_rtp_asterisk: iterate all local addresses looking to populate ICE. (asterisk[13])

[Jaco Kroon]

Jaco Kroon has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/13362 )

Change subject: res_rtp_asterisk: iterate all local addresses looking to populate ICE.
......................................................................


Patch Set 6: Code-Review-1

(1 comment)

I'm going to -1 my own code here, but would like a decision from yourselves as to which route to follow before submitting Patchset 7.

https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c 
File res/res_rtp_asterisk.c:

https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3157 
PS5, Line 3157: 				ast_sockaddr_to_pj_sockaddr(addr, &base);
> If it's fixing a bug then I say go that route. […]
Was originally intended as an optimization (ie, if we have an address available in addr, use it) but after re-reading the RFCs I realized it might actually be fixing a bug as well.  I really don't mind here ... happy to follow recommendation here.

The question then becomes - is this a bug or not?

Essentially there are a few things I see here:

1.  If addr is not ANY - does it make sense to include any candidates other than addr?

2.  Whatever we select for raddr (base) as I understand MUST be a candidate (even though I don't understand WHY), and addr may be ice blacklisted (as used in my code now).  The kernel would drop traffic to those addresses anyway, or worse, deliver it to a different process.

3.  raddr (base) MUST be IPv4 (since STUN only makes sense in IPv4 world, logically, but IPv6 too has NAT implementations which is outside the scope of this discussion).

I'm however not going to submit as a separate patch simply because I don't use STUN so if it is a bug, it doesn't actually affect me (directly, currently).  If this is intended to address a bug fix then I agree, we fork this out onto a separate patch and follow option (3) above.  Of course this may require us to include addr as a candidate (even if it is blacklisted).

On that front, if addr is not ANY - does it make sense to add any other discovered addresses as ICE candidates since if they're not bound the kernel will simply drop the traffic?

So yes, perhaps this should split out into a separate patch.  Since PJSIP always binds the RTP address to ANY this doesn't affect my use-case but since I'm tampering with the code, I may just as well ...

Your call here.  There are two routes here:

(1) Add a check for addr being IPv4 to this snippet (3) above.
(2) Remove the snippet completely and fork into separate patch.

I'm inclined to head to (2) pending discussion on the ML.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/13362
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: I109eaffc3e2b432f00bf958e3caa0f38cacb4edb
Gerrit-Change-Number: 13362
Gerrit-PatchSet: 6
Gerrit-Owner: Jaco Kroon <jaco at uls.co.za>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Jaco Kroon <jaco at uls.co.za>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Wed, 01 Apr 2020 06:37:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Comment-In-Reply-To: Jaco Kroon <jaco at uls.co.za>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200401/bef99852/attachment-0001.html>