[Asterisk-code-review] chan sip: tcp/tls honour insecure=port (asterisk[master])
Richard Mudgett
asteriskteam at digium.com
Tue Aug 21 15:14:17 CDT 2018
Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/9858 )
Change subject: chan_sip: tcp/tls honour insecure=port
......................................................................
Patch Set 6: Code-Review-1
(22 comments)
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG
Commit Message:
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@26
PS6, Line 26: It should be noted that when looking for a peer, two passes is made, the
s/is/are/
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@33
PS6, Line 33: In previous behaviour the there was special handling for
s/the there/there/
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@61
PS6, Line 61: 4. If doing TCP, match if matched against peer also has
s/TCP/UDP/
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@95
PS6, Line 95: * peer5 if registered from 1.1.1.1 and source port matches;
: * peer1 or peer2 if from port 5060 (ordering); else
: * peer3.
pass 1:
* peer1 or peer2 if from port 5060
* peer3 if from port 5061
* peer5 if registered as 1.1.1.1 and source port matches
pass 2:
* peer3
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@98
PS6, Line 98: - current:
: * peer5 if registered from 1.1.1.1 and source port matches;
: * peer1 or peer2 if from port 5060 (ordering); else
: * peer 3.
- current: as per previous
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@103
PS6, Line 103: * peer5 if registered from 1.1.1.1 and source port matches;
: * peer2 if from port 5060; else
: * peer 3.
pass 1:
* peer2 if port 5060
* peer3 if port 5061
* peer5 if registered as 1.1.1.1 and source port matches
pass 2:
* peer3
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@109
PS6, Line 109: * peer5 if registered from 1.1.1.2 and port matches (irrespective of
: transport protocol);
: * peer4 if source port is 5060; else
: * no match (guest).
pass 1:
* peer 4 if from port 5060
* peer 5 if registered as 1.1.1.2 and source port matches
pass 2:
* no match (guest)
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@114
PS6, Line 114: - new:
: * peer5 if registered from 1.1.1.2 and port matches (and udp
: transport is allowed by the matched peer);
: * peer4 if source port is 5060; else
: * no match (guest).
- new: as per previous
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@122
PS6, Line 122: * peer5 if registered from that address (and port if not insecure=port).
: * no match (guest)
pass 1:
* peer 5 if registered as that address and source port matches
pass 2:
* no match (guest)
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@131
PS6, Line 131: * peer1 or peer5 if peer5 registered from 1.1.1.1;
: * peer1 (unless the source port happens to be 5060 then there is a
: possibility based on ordering of matching peer2); else
pass 1:
* peer1
* peer2 if from 5060
* peer3 if from 5061
* peer5 if registered as 1.1.1.1
pass 2:
* cannot happen as we would always find peer1 in the first pass
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@135
PS6, Line 135: * peer5 if registered from 1.1.1.1 and source port matches;
: * peer1 or peer2 if and only if source port happens to be 5060
: (ordering);
: * no match (guest)
pass 1:
* peer1 or peer2 if from 5060
* peer3 if from 5061
* peer5 if registered as 1.1.1.1 and source port matches
pass 2:
* no match (guest)
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@140
PS6, Line 140: * peer5 if registered from 1.1.1.1 and source port matches;
: * peer1.
pass 1:
* peer1 if from 5060
* peer5 if registered as 1.1.1.1 and source port matches
pass2:
* peer1
* peer5 if registered as 1.1.1.1 <- We cannot eliminate dynamic peers because the registered CONTACT port is not guaranteed to be the source port of the message
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@145
PS6, Line 145: * peer4 or peer5 if peer5 registered from 1.1.1.2
: * peer4.
pass 1:
* peer4
* peer5 if registered as 1.1.1.2
pass 2:
* cannot happen as we would always find peer4 in the first pass
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@148
PS6, Line 148: * peer5 if peer5 registered from 1.1.1.2 and source port matches;
: * peer4 if source port happens to be port 5060; else
: * no match (guest).
pass 1:
* peer4 if from 5060
* peer5 if registered as 1.1.1.2 and source port matches
pass2:
* no match (guest)
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@152
PS6, Line 152: * peer5 if peer5 registered from 1.1.1.2 and source port matches;
: * peer4.
pass 1:
* peer4 if from 5060
* peer5 if registered as 1.1.1.2 and source port matches
pass2:
* peer4
* peer5 if registered as 1.1.1.2 <- We cannot eliminate dynamic peers because the registered CONTACT port is not guaranteed to be the source port of the message
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@157
PS6, Line 157: * peer5 if registered from that address; else
: * no match (guest)
pass 1:
* peer 5 if registered as that address
pass 2:
* no match (guest)
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@160
PS6, Line 160: * peer5 if registered from that address + port; else
: * no match (guest)
pass 1:
* peer 5 if registered as that address and port
pass 2:
* no match (guest)
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@163
PS6, Line 163: * peer5 if registered from that address + port; else
: * no match (guest)
pass 1:
* peer 5 if registered as that address and port
pass 2:
* peer 5 if registered as that address <- We cannot elimitate dynamic peers because the registered CONTACT port is not guaranteed to be the source port of the message
https://gerrit.asterisk.org/#/c/9858/6//COMMIT_MSG@166
PS6, Line 166: It should be noted the test cases doesn't make explicit mention of TLS,
s/doesn't/don't/
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 won't typically be controlled, we only want
"can't" was a better word before where you have "won't" now.
https://gerrit.asterisk.org/#/c/9858/6/channels/chan_sip.c@34458
PS6, Line 34458: return peer->host_dynamic ? 0 : (CMP_MATCH | CMP_STOP);
For the reasons I stated previously you need to just return CMP_MATCH here. You also need to update the comments and commit message to not consider dynamic vs static peers.
The peer->addr is the CONTACT address and is not necessarily the source address of the incoming message (especially the port). This is true for static peers and can be true for dynamic peers.
https://gerrit.asterisk.org/#/c/9858/6/channels/chan_sip.c@34461
PS6, Line 34461: static int peer_ipcmp_cb(void *obj, void *arg, int flags)
: {
: return peer_ipcmp_cb_full(obj, arg, NULL, flags);
: }
Delete this function for the reasons I stated previously.
--
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: 6
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: Tue, 21 Aug 2018 20:14:17 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180821/55affeba/attachment-0001.html>
More information about the asterisk-code-review
mailing list