[asterisk-dev] [Code Review]: chan_sip: removing some non-compliant code in v10 and minor fixes

[Olle E Johansson]

> On Oct. 20, 2011, 11:13 a.m., David Vossel wrote:
> > I'm not sure we should make the registration change change for rejecting Registrations without a user name in the To header.  We have probably treated the domain as the user name when no user name is present since the beginning of chan_sip.  I certainly hope this logic isn't being depended on somewhere, but since we allow it, it probably is. Below is what I found on the user name in the To header URI.  Unless there is some other verbiage I missed that uses the "MUST" key word in relation to this behavior, it seems to me like this falls in the category of being liberal about what we accept and strict about what we send.  Does it cause any problems the way it is?
> > 
> > 
> > RFC 3261 section 10.2.
> >       To: The To header field contains the address of record whose
> >            registration is to be created, queried, or modified.  The To
> >            header field and the Request-URI field typically differ, as
> >            the former contains a user name.  This address-of-record MUST
> >            be a SIP URI or SIPS URI.
> 
> Olle E Johansson wrote:
>     sip:digium.com is a valid SIP uri. The question here is if it's valid to register to it. I think it is.
>     
>     Section 10.2 says this but is not normative:
>     "To: The To header field contains the address of record whose
>                registration is to be created, queried, or modified.  The To
>                header field and the Request-URI field typically differ, as
>                the former contains a user name.  This address-of-record MUST
>                be a SIP URI or SIPS URI."
>     
>     The definition of address-of-record is:
>     "  Address-of-Record: An address-of-record (AOR) is a SIP or SIPS URI
>              that points to a domain with a location service that can map
>              the URI to another URI where the user might be available.
>              Typically, the location service is populated through
>              registrations.  An AOR is frequently thought of as the "public
>              address" of the user."
>     
>     I e-mailed sip-implementors to see if someone can clear this up.
> 
> Terry Wilson wrote:
>     > Unless there is some other verbiage I missed that uses the "MUST" key word in relation to this behavior, it seems to me like this falls in the category of being liberal about what we accept and strict about what we send.  Does it cause any problems the way it is?
>     
>     Since it says "This address-of-record MUST be a SIP URI or SIPS URI" and a SIP URI is defined as:
>     SIP-URI          =  "sip:" [ userinfo ] hostport uri-parameters [ headers ]
>     
>     which shows that userinfo is optional, but hostport is required, then the RFC does say that we MUST treat the sip:blah as a domain (hostport) URI. As far as causing problems, it breaks the domain= setting in sip.conf for limiting requests by the domain they are addressed to. After staring at things for a while, I think fixing this would be a good idea.
> 
> Terry Wilson wrote:
>     To clarify, though the hostport is required in the SIP URI, I don't think I would say that the username is. Just because the Request-URI and the AOR in the To: header *typically* differ, it does not seem that they are required to differ. I would keep the check on the empty domain name, but not reject an empty username. In fact, I would probably, just to make things consistent set an empty name to the domain and treat a REGISTER with To: <example.com> as a registration for peer [example.com].

I would say that it's up to local policy on how to handle this. In Asterisk a INVITE to sip:terry.edv will look for extension "s".

Looking for a peer named "example.com" would be to implement a whole new level of peer matching, something I resist to do at this point. To be very clear: It would be very bad.

I would simply treat this case as "no match with existing peer" and continue along that code path.


- Olle E


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


On Oct. 19, 2011, 12:30 p.m., wdoekes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1533/
> -----------------------------------------------------------
> 
> (Updated Oct. 19, 2011, 12:30 p.m.)
> 
> 
> Review request for Asterisk Developers and Paul Belanger.
> 
> 
> Summary
> -------
> 
> See the bug report: there were some XXX'es in the code about code that should be removed.
> 
> The patch does this:
> 
> (1) register_verify won't accept a To: without user-part anymore (illegal according to rfc3261, 10.2)
> 
> (2) check_user_full still doesn't require a user-part, but it won't match usernames by domain anymore. (i.e. it doesn't treat sip:domain as sip:domain at domain anymore)
> 
> (3) there was some freaky logic going on in get_msg_text, I had to rewrite it to make it make sense.
> 
> (4) in the reqresp parser there were lots of if (params) inside a big if (params) block. I scrapped the useless if's.
> 
> 
> This addresses bug ASTERISK-18389.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18389
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 341249 
>   /trunk/channels/sip/reqresp_parser.c 341249 
> 
> Diff: https://reviewboard.asterisk.org/r/1533/diff
> 
> 
> Testing
> -------
> 
> It compiles.
> 
> 
> Thanks,
> 
> wdoekes
> 
>

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