[asterisk-dev] [Code Review] SIP TCP/TLS: ensure that the contact header properly supports TLS/improved support for PAT/port redirection

[David Vossel]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/392/#review1147
-----------------------------------------------------------



/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/392/#comment2636>

    It seems to me that the expected behavior would be that the externtlsport and externtcpport options would only be used when they are explicitly set in the config.  This statement also overrides the port specified by the externip in every case...  A solution for this might be to set externtcpport and extertlsport to 0 by default and only override us->sin_port here if the type and extern(type)port is set...
    
    example.
    *us = externip;
    if ((p->socket.type == SIP_TRANSPORT_TCP) && externtcpport) {
        us->sip_port = htons(externtcpport);
    } else if ((p->socket.type == SIP_TRANSPORT_TLS) && externtlsport) {
        us->sip_port = htons(externtlsport);
    } else {
      /* no special port rules apply, use default externip port */
    }
    
    
    ...then in reload, maybe something like this...
    /* zero by default */
    externtlsport=0;
    externtcpport=0;
    
    if (value is externtlsport) {
        externtlsport=value;
    }
    if (value is extrntcpport) {
        externtcpport=value;
    }
    
    
    I'm not completely familiar with this part of the code as I have never used the externip option, but functionally these are some of the things that caught my eye. 



/trunk/include/asterisk/tcptls.h
<https://reviewboard.asterisk.org/r/392/#comment2637>

    These values seem to be limited to chan_sip, or at least I am unaware of why they should be required in the generic tcptls code.  A better option may be to create a chan_sip global value to store the extertlsport and externtcpport right under the struct sockaddr_in externip; value.  Since they are only representing ports, they could be set as a uint16_t.


- David


On 2009-10-06 11:13:29, ebroad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/392/
> -----------------------------------------------------------
> 
> (Updated 2009-10-06 11:13:29)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> https://issues.asterisk.org/view.php?id=15880
> 
> This patch adds support for TCP/TLS in the Contact header when using NAT, specifically externip or externhost. The original issue was that Asterisk sent 5060 as the port in the contact header whether TLS was used or not. Additionally, this patch adds 2 config options to sip.conf, specifically externtcpport and externtlsport. This allows a user to specify different external ports for TCP and TLS other than those used internally, this is especially useful in in a PAT/port redirection setup. 
> 
> 
> This addresses bug 15880.
>     https://issues.asterisk.org/view.php?id=15880
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 222222 
>   /trunk/configs/sip.conf.sample 222222 
>   /trunk/include/asterisk/tcptls.h 222222 
> 
> Diff: https://reviewboard.asterisk.org/r/392/diff
> 
> 
> Testing
> -------
> 
> Tested both inbound and outbound calls using Counterpath's Bria softphone, with Asterisk behind a NAT firewall(Cisco ASA), and with the appropriate ports redirected. The client was behind a NAT firewall(Cisco PIX), using STUN. 
> 
> 
> Thanks,
> 
> ebroad
> 
>