[asterisk-dev] [Code Review]: Add a test for verifying when username information can be leaked via differing nat settings

Terry Wilson reviewboard at asterisk.org
Mon Nov 21 10:37:29 CST 2011 


> On Nov. 21, 2011, 2:37 a.m., wdoekes wrote:
> > /asterisk/trunk/tests/channels/SIP/nat_supertest/sipp/register.xml, lines 26-27
> > <https://reviewboard.asterisk.org/r/1590/diff/1/?file=21804#file21804line26>
> >
> >     Perhaps note that checking for this 100 should be removed at some point.

It is optional, so it will be ignored. I leave it in because 1.4 still sends a 100 Trying on a REGISTER. I thought we were going to take that out, but it is still there.


> On Nov. 21, 2011, 2:37 a.m., wdoekes wrote:
> > /asterisk/trunk/tests/channels/SIP/nat_supertest/run-test, line 134
> > <https://reviewboard.asterisk.org/r/1590/diff/1/?file=21803#file21803line134>
> >
> >     Same tuple problem as the inavlid [sic] log warnings.
> >     
> >     (And another one below at log.error.)

Fixed.


> On Nov. 21, 2011, 2:37 a.m., wdoekes wrote:
> > /asterisk/trunk/tests/channels/SIP/nat_supertest/run-test, lines 91-98
> > <https://reviewboard.asterisk.org/r/1590/diff/1/?file=21803#file21803line91>
> >
> >     str = '%d;%s\n' % ((5062, 5061)[port_matches_via], ('ignored', 'rport')[rport_specified])
> >     
> >     String concatenation is always a non-pattern.

Fixed.


> On Nov. 21, 2011, 2:37 a.m., wdoekes wrote:
> > /asterisk/trunk/tests/channels/SIP/nat_supertest/run-test, line 88
> > <https://reviewboard.asterisk.org/r/1590/diff/1/?file=21803#file21803line88>
> >
> >     Touching this global here is not real nice. But apart from that, 'stop' is probably not a property object:
> >     
> >     reactor.stop()

Fixed to reactor.stop(). If we fail to update the config file, badness happens. I suppose we could return False and retry if it turns out that Asterisk is inconsistent somehow responding to our UpdateConfig. What would you like to see here?


> On Nov. 21, 2011, 2:37 a.m., wdoekes wrote:
> > /asterisk/trunk/tests/channels/SIP/nat_supertest/run-test, line 54
> > <https://reviewboard.asterisk.org/r/1590/diff/1/?file=21803#file21803line54>
> >
> >     Is TestCase an old-style class? Otherwise use:
> >     
> >     super(TestCase, self).__init__()
> >     
> >     (Same for TestCase.run() below.)

Wouldn't this be super(SIPNatTest, self).__init__() or am I misunderstanding the purpose of super()? If so, then fixed.


> On Nov. 21, 2011, 2:37 a.m., wdoekes wrote:
> > /asterisk/trunk/lib/python/asterisk/syncami.py, line 39
> > <https://reviewboard.asterisk.org/r/1590/diff/1/?file=21799#file21799line39>
> >
> >     Don't do this:
> >     
> >     '%s' % (err)
> >     
> >     Do this:
> >     
> >     '%s' % (err,)
> >     
> >     Or if you're really really sure that err can safely be converted to a real basestring:
> >     
> >     '%s' % err
> >     
> >     But even then, I still prefer the tuple.

Fixed.


> On Nov. 21, 2011, 2:37 a.m., wdoekes wrote:
> > /asterisk/trunk/lib/python/asterisk/syncami.py, lines 17-18
> > <https://reviewboard.asterisk.org/r/1590/diff/1/?file=21799#file21799line17>
> >
> >     I like this.
> >     
> >     Although I would prefer new-style classes, since: "A "New Class" is the recommended way to create a class in modern Python."
> >     
> >     I.e. class SyncAMI(object)

Fixed.


- Terry


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


On Nov. 17, 2011, 1:20 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1590/
> -----------------------------------------------------------
> 
> (Updated Nov. 17, 2011, 1:20 p.m.)
> 
> 
> Review request for Asterisk Developers, Paul Belanger and mjordan.
> 
> 
> Summary
> -------
> 
> This test loops through all combinations of nat=route/no (force_rport/no in 1.8+) for [general] and [peer] in sip.conf, whether or not the incoming request specified ;rport in the Via, and whether request was sent from the same port as the one listed in the Via, for both a valid and an invalid user. The test records when the behavior differs and matches against when we expect that behavior to differ for each version of Asterisk (1.4, 1.6.2, 1.8, 10, trunk). It also uses a Quine-McClusky minimization solver to reduce the list of failures to a function describing the failures. For example:
> On 1.4:
>     (((NOT port_matches_via) AND peer_nat AND (NOT general_nat)) OR ((NOT port_matches_via) AND (NOT peer_nat) AND general_nat))
> and 1.8:
>     (((NOT port_matches_via) AND (NOT rport_specified) AND (NOT peer_nat) AND general_nat) OR ((NOT port_matches_via) AND (NOT rport_specified) AND peer_nat AND (NOT general_nat)))
> 
> As you see, there is an extra term in 1.8 because in 1.8 we honor the ;rport in the Via no matter what, so differing behavior only occurs when a request comes in *not* specifying ;rport. There is no equivalent for nat=route or nat=never in 1.8+ (these values were originally added to work around a bug in old Uniden phone firmware).
> 
> This patch also adds a very small SyncAMI class for synchronously sending AMI requests (over HTTP because it is much simpler to do via HTTP's built-in request/response architecture) and getting back a response. It also modifies the default manager.conf and http.conf files to support the new API. SyncAMI makes it much easier to do things like update a config and reload it over AMI as you don't have to split up what you are doing into a lot of callback functions and you really do want to block execution until they are complete. Normally, I would have done a separate review for SyncAMI, but as this patch requires its use I've left them together for the review.
> 
> 
> Diffs
> -----
> 
>   /asterisk/trunk/configs/branch-1.4/manager.conf 2750 
>   /asterisk/trunk/configs/http.conf PRE-CREATION 
>   /asterisk/trunk/configs/manager.conf 2750 
>   /asterisk/trunk/lib/python/asterisk/syncami.py PRE-CREATION 
>   /asterisk/trunk/lib/python/qm.py PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/nat_supertest/configs/ast1/extensions.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/nat_supertest/configs/ast1/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/nat_supertest/run-test PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/nat_supertest/sipp/register.xml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/nat_supertest/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/tests.yaml 2750 
> 
> Diff: https://reviewboard.asterisk.org/r/1590/diff
> 
> 
> Testing
> -------
> 
> Tested against 1.4 and 1.8 and have gotten the values I expect back. The nat options for 1.4 and 1.6.2; and 1.8, 10, and trunk are the same, so I expect similar behavior there.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20111121/6af2541f/attachment-0001.htm>

More information about the asterisk-dev mailing list