[Asterisk-code-review] chan sip: tcp/tls honour insecure=port (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/9858 )

Change subject: chan_sip: tcp/tls honour insecure=port
......................................................................


Patch Set 5: Code-Review-1

(13 comments)

https://gerrit.asterisk.org/#/c/9858/5//COMMIT_MSG
Commit Message:

https://gerrit.asterisk.org/#/c/9858/5//COMMIT_MSG@27
PS5, Line 27: irrespecive
spell


https://gerrit.asterisk.org/#/c/9858/5//COMMIT_MSG@31
PS5, Line 31: For everything else, match only if we have a host= that is
            : not dynamic set for that peer.
This sentence needs updating because of the comment about the dynamic host.


https://gerrit.asterisk.org/#/c/9858/5//COMMIT_MSG@54
PS5, Line 54: Use cases:
You need to reorganize the use cases you are explaining.  You jump around a lot where the reader can get confused about what you are talking about.


https://gerrit.asterisk.org/#/c/9858/5//COMMIT_MSG@63
PS5, Line 63:  - will match peer4 if there registered port matches; else
s/there/the/


https://gerrit.asterisk.org/#/c/9858/5//COMMIT_MSG@64
PS5, Line 64:  - will match peer2 if if insecure=port is set for peer2.
s/if if/if/


https://gerrit.asterisk.org/#/c/9858/5//COMMIT_MSG@66
PS5, Line 66: For 1 and 2, would always match the first defined peer, possibly
            : resulting in peer not allowed to use transport during a later check.
I think you are missing "previously" as the first word in the sentence.  Otherwise, this paragraph makes no sense for the new behavior.  It makes more sense when describing previous behavior.


https://gerrit.asterisk.org/#/c/9858/5//COMMIT_MSG@73
PS5, Line 73: Previously WS and WSS peers would need to be dynamic to match correctly,
            : or only have one registration from a given source IP.
This is confusing as you have not dealt much with WS/WSS and you don't have a use case defined for those transports.


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@34437
PS5, Line 34437: 		/* Now only return a match if the port matches, as well. */
s/Now/On the first pass/


https://gerrit.asterisk.org/#/c/9858/5/channels/chan_sip.c@34442
PS5, Line 34442: 	/* We can only reach here is peer2 is for SIP_INSECURE_PORT, in
s/here is/here if/


https://gerrit.asterisk.org/#/c/9858/5/channels/chan_sip.c@34447
PS5, Line 34447: 	 * to check the source IP, but only if the host isn't dynamic.  This isn't
This comment paragraph needs updating.  See my comment below when checking the host_dynamic flags.


https://gerrit.asterisk.org/#/c/9858/5/channels/chan_sip.c@34456
PS5, Line 34456: 	} else
The current code does not need the else keyword here because the if clause returns.


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" here for non-udp protocols.

The port of a static peer is going to be the default port or an explicit port to contact the peer.  It is not likely to be the source port of the transport.  For dynamic peers that require registration, the port won't always be the transport's source port but what the REGISTER's contact header says.  (NAT not enabled or rport not set in the REGISTER message)  Again, the received contact port is not likely to be the transport's source port.

* The indention is not needed here because the else keyword above can be removed.
* FYI: peer2->host_dynamic is not set by the caller.  I think you meant peer->host_dynamic.  However, you should not use that flag for the reasons already stated.


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.  It can cause misinterpretation of how peer_ipcmp_cb_full() actually works because this function can never get called.



-- 
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: Tue, 21 Aug 2018 00:15:39 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180820/4b564f77/attachment.html>