[Asterisk-code-review] chan sip: tcp/tls honour insecure=port (asterisk[master])
Jaco Kroon
asteriskteam at digium.com
Thu Aug 23 06:15:49 CDT 2018
Jaco Kroon has posted comments on this change. ( https://gerrit.asterisk.org/9858 )
Change subject: chan_sip: tcp/tls honour insecure=port
......................................................................
Patch Set 5:
(24 comments)
I've prepared a next iteration on this but we need consensus on whether or not peer5 should be matched purely by IP or not. I think on everything else we agree.
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG
Commit Message:
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@26
PS6, Line 26: transports).
> s/is/are/
Done
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@33
PS6, Line 33:
> s/the there/there/
Done
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@61
PS6, Line 61: 2 - Incoming UDP request from 1.1.1.1:
> s/TCP/UDP/
Done
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@95
PS6, Line 95:
:
:
> pass 1: […]
Done
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@98
PS6, Line 98:
:
:
:
> - current: as per previous
Done
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@103
PS6, Line 103:
:
:
> pass 1: […]
Done
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@109
PS6, Line 109:
:
:
:
> pass 1: […]
Done
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@114
PS6, Line 114:
:
:
:
:
> - new: as per previous
Done
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@122
PS6, Line 122:
:
> pass 1: […]
Done
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@131
PS6, Line 131:
:
:
> pass 1: […]
Done
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@135
PS6, Line 135:
:
:
:
> pass 1: […]
Done
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@140
PS6, Line 140:
:
> pass 1: […]
As I understand the purpose here we're looking for a peer that can be used to auth against based on IP:port. Not based on name. If we find peer5 here insecure=invite could apply else auth will be required. Even if we don't find anything here it's still possible that peer5 will be found during the name search by name phase?
Finding peer5 here will regress again for https://issues.asterisk.org/jira/browse/ASTERISK-27457 - which is what introduced the patch specifically to eliminate this in the first place.
My personal opinion is that we should not match peer5 here. In my experience with SIP/TCP (majority Yealink and SNOM, but also some ) UACs will open a single TCP connection, issue REGISTER and keep the connection open, eliminating the problem you're referring to. If the endpoint is behind NAT the administrator will already be setting options in asterisk to deal with NAT eliminating your concerns (as far as I understand).
To make matters worse for Alexander, if he also happens to set insecure=invite, the "guest" user will automatically authenticate as the registered from the same IP peer. This wasn't even raised in that issue.
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@145
PS6, Line 145:
:
> pass 1: […]
Done
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@148
PS6, Line 148:
:
:
> pass 1: […]
Done
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@152
PS6, Line 152:
:
> pass 1: […]
See above.
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@157
PS6, Line 157:
:
> pass 1: […]
Done
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@160
PS6, Line 160:
:
> pass 1: […]
Done
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@163
PS6, Line 163:
:
> pass 1: […]
See above.
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@166
PS6, Line 166:
> s/doesn't/don't/
Done
https://gerrit.asterisk.org/#/c/9858/5/channels/chan_sip.c
File channels/chan_sip.c:
https://gerrit.asterisk.org/#/c/9858/5/channels/chan_sip.c@34457
PS5, Line 34457: return peer2->host_dynamic ? 0 : (CMP_MATCH | CMP_STOP);
> The best you can do for the second pass that matches by IP address only is to just return "match" he […]
This will break the use case again for Alexander, and is what the original patch tried to address.
Based on what I've seen (obviously limited experience) TCP based UACs will open a TCP connection to the server, and utilize that single TCP connection for all further SIP communications. For this reason I believe that this change is safe. For TCP communications, especially when behind NAT, the REGISTER address means nothing and we won't be able to establish a new tcp connection to that anyway and we need to re-use the connection on which the REGISTER came in anyway.
What's the risk if we miss a match? I'm guessing we could reject connections that in the current iteration of the code is being rejected anyway. Based on feedback from Alexander (for whom current apparently is working) he's seeing the same thing that I'm seeing. Specifically he mentioned to me in offline email that the current code is working for him, but he doesn't have static hosts configured.
The above code is my best-effort attempt at both re-enabling the previous behaviour for static TCP hosts, as well as enabling guests over TCP for Alexander from hosts where there already exists registrations on.
Also consider the case where there are multiple registrations coming from the same IP address.
https://gerrit.asterisk.org/#/c/9858/5/channels/chan_sip.c@34460
PS5, Line 34460: static int peer_ipcmp_cb(void *obj, void *arg, int flags)
: {
: return peer_ipcmp_cb_full(obj, arg, NULL, flags);
: }
> This is dead code and needs to be removed. […]
Not dead. Used in load_module function to create the peers_by_ip container. This container is used in various places and I don't feel comfortable enough to eliminate those uses.
https://gerrit.asterisk.org/#/c/9858/6/channels/chan_sip.c
File channels/chan_sip.c:
https://gerrit.asterisk.org/#/c/9858/6/channels/chan_sip.c@34446
PS6, Line 34446: * for non-UDP the source port can't typically be controlled, we only want
> "can't" was a better word before where you have "won't" now.
No. Alexander pointed out that it is possible for a TCP connection to be source-port controlled (in fact, I realized FTP relied on that for using source port 21 for return TCP connections).
So I think won't is the better word choice here since technically it can be controlled.
https://gerrit.asterisk.org/#/c/9858/6/channels/chan_sip.c@34458
PS6, Line 34458: }
> For the reasons I stated previously you need to just return CMP_MATCH here. […]
Please refer my comments and explanation in the commit message. The question really is what's the desired behaviour and I suspect the new behaviour is. If IP matching fails then hopefully we'll name match and still request authentication. Else it's down to guest land, and if guest is disabled, it's back to authentication.
https://gerrit.asterisk.org/#/c/9858/6/channels/chan_sip.c@34461
PS6, Line 34461: {
: return peer_ipcmp_cb_full(obj, arg, NULL, flags);
: }
:
> Delete this function for the reasons I stated previously.
It's still in use in load_module() for the peers_by_ip container (which is used in various places). I'm not comfortable making this modification. From what I can tell this is merely a wrapper function that simply provides a default argument, so I also don't get why it's considered confusing (maybe that's the whole point).
--
To view, visit https://gerrit.asterisk.org/9858
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61a9804e4feba9c7224c481f7a10bf7eb7c7f2a2
Gerrit-Change-Number: 9858
Gerrit-PatchSet: 5
Gerrit-Owner: Jaco Kroon <jaco at uls.co.za>
Gerrit-Reviewer: Jaco Kroon <jaco at uls.co.za>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Thu, 23 Aug 2018 11:15:49 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180823/4d3e6756/attachment-0001.html>
More information about the asterisk-code-review
mailing list