[asterisk-dev] [Code Review] SIP authentication support

[jcolp]

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



/team/group/pimp_my_sip/include/asterisk/res_sip.h
<https://reviewboard.asterisk.org/r/2310/#comment14864>

    We should start specifying what the id of sorcery objects are, per a comment from mjordan in another review.



/team/group/pimp_my_sip/include/asterisk/res_sip.h
<https://reviewboard.asterisk.org/r/2310/#comment14858>

    Storing pointers to the auth objects like this implies a specific load/reload order and also means you are caching information. This is bad. If this information is backed by a database then updating the database won't be reflected here.



/team/group/pimp_my_sip/res/res_sip_authenticator_digest.c
<https://reviewboard.asterisk.org/r/2310/#comment14859>

    static



/team/group/pimp_my_sip/res/res_sip_authenticator_digest.c
<https://reviewboard.asterisk.org/r/2310/#comment14865>

    I think we'll end up sticking with thread local storage, it does indeed feel cleaner.



/team/group/pimp_my_sip/res/res_sip_authenticator_digest.c
<https://reviewboard.asterisk.org/r/2310/#comment14860>

    You can const more of this.



/team/group/pimp_my_sip/res/res_sip_authenticator_digest.c
<https://reviewboard.asterisk.org/r/2310/#comment14862>

    I'd say go for it. Might as well go the full distance.



/team/group/pimp_my_sip/res/res_sip_authenticator_digest.c
<https://reviewboard.asterisk.org/r/2310/#comment14863>

    If the stale code gets added I think we can safely make this occur on reload as well. Any outstanding authentication would just get a stale response and should then retry.


- jcolp


On Feb. 8, 2013, 11 a.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2310/
> -----------------------------------------------------------
> 
> (Updated Feb. 8, 2013, 11 a.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and jcolp.
> 
> 
> Summary
> -------
> 
> This introduces inbound authentication support for the new SIP work.
> 
> I reworked the initial concept of authentication drastically. I realized that the original API I came up with was making it awkward to use any other authentication scheme than digest authentication. The newer authenticator callbacks are now simpler: there is one to find out whether an endpoint requires authentication and a second one to do whatever authentication is necessary, returning the results of such attempts.
> 
> This adds an authenticator, res_sip_authenticator_digest, that uses digest authentication in order to authenticate. Initially, I thought I was going to need some features from PJSIP trunk, but I realized after backporting them that they really didn't help the situation any. Thus I had to resort to using some thread-local storage in order to be able to access certain data in one of the PJSIP authentication callbacks. Due to the threading model in use, this is safe.
> 
> Authentication is configured via a type=auth section in res_sip.conf. Then an endpoint can specify auth=blah in order to tie authentication credentials to the endpoint. Endpoints can specify multiple auth sections if they have credentials for multiple realms. Similarly, multiple enpdoints may specify the same auth section if for whatever reason endpoints share credentials. This means that Asterisk may send multiple WWW-Authenticate headers out in an authentication challenge and can cope with multiple Authorization headers in requests.
> 
> I made a change to the sorcery API. The sorcery apply handler now returns an int instead of void. This way, if there is an error when applying an objectset, it can be detected. I did this because once an auth object has been constructed, I wasn't sure of another way to do some verification that the configured auth was sane. If a different change should be made, or if I was missing a simpler way to verify a constructed auth object, please let me know. IMO, this change makes good sense, though, especially as I was changing the transport apply handler to be able to return an error condition when it encounters problems.
> 
> 
> This addresses bug ASTERISK-20953.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20953
> 
> 
> Diffs
> -----
> 
>   /team/group/pimp_my_sip/include/asterisk/res_sip.h 381065 
>   /team/group/pimp_my_sip/include/asterisk/sorcery.h 381065 
>   /team/group/pimp_my_sip/main/astobj2.c 381065 
>   /team/group/pimp_my_sip/main/sorcery.c 381065 
>   /team/group/pimp_my_sip/res/res_sip.c 381065 
>   /team/group/pimp_my_sip/res/res_sip/config_auth.c PRE-CREATION 
>   /team/group/pimp_my_sip/res/res_sip/config_transport.c 381065 
>   /team/group/pimp_my_sip/res/res_sip/sip_configuration.c 381065 
>   /team/group/pimp_my_sip/res/res_sip_authenticator_digest.c PRE-CREATION 
>   /team/group/pimp_my_sip/res/res_sip_session.c 381065 
>   /team/group/pimp_my_sip/tests/test_sorcery.c 381065 
> 
> Diff: https://reviewboard.asterisk.org/r/2310/diff
> 
> 
> Testing
> -------
> 
> Inbound calls have been made when authentication is configured and when it is not. I have ensured that endpoints with no configured authentication do not get challenged and that endpoints with configured authentication do get challenged.
> 
> I also tested both the password and md5_cred versions of storing authentication and verified that authentication worked properly.
> 
> 
> Thanks,
> 
> Mark
> 
>

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