[asterisk-dev] [Code Review]: res_sip and res_sip_session design review

[Mark Michelson]

> On Jan. 3, 2013, 7:34 a.m., jcolp wrote:
> > Authenticator comments:
> > 1. Should there be support for multiple authenticators?
> > 2. What is the role of the authenticator in its entirety?
> > 3. How does this impact our usage of the provided pjsip authentication framework? Does our default provided implementation simply become a user/wrapper around it? Is it used for all authenticators?
> > 4. Can you document the get_authentication_credentials callback a bit more? What is an implementor specifically supposed to put in the structure when that function is called?
> > 5. Will the authenticator API calls be expected to be called by implemented modules when they deem it necessary?
> > 
> > Endpoint identifier comments:
> > 1. It's come up before that people want to change the matching order so we should probably remember to make this possible for the default provided ones (shouldn't be TOO hard), or describe how to do it.
> > 2. Should explicitly mention that the returned ast_sip_endpoint is an ao2 object and will be returned with the reference count bumped
> 
> Mark Michelson wrote:
>     Authenticator comments:
>     1. Should there be support for multiple authenticators?
>     
>     My initial thought is not to support multiple authenticators.
>     
>     2. What is the role of the authenticator in its entirety?
>     
>     The authenticator has three roles, each corresponding to a callback in the ast_sip_authenticator structure. The authenticator is queried for three things:
>     
>     * Should an incoming request be authenticated?
>     * Does the incoming message contain proper authentication credentials?
>     * What credentials should be used in order to send an authentication challenge?
>     
>     3. How does this impact our usage of the provided pjsip authentication framework? Does our default provided implementation simply become a user/wrapper around it? Is it used for all authenticators?
>     
>     Let's consider the three callbacks. The first and third will just use endpoint configuration in order to determine if the request should be authenticated and what parameters should be used in the authentication challenge. The second one, authenticate_request(), will use the PJSIP authentication framework to check the credentials. Keep in mind that res_sip will provide a function, independent of any authenticator, that will use digest credentials in order to send a properly-constructed 401 response.
>     
>     So to answer your final question, all authenticators may not necessarily use pjsip's authentication, but they certainly would be able to if they wanted. We may wish to add a wrapper function that calls into PJSIP's authentication routines to ease the burden for any authenticator that wants to just use digest authentication.
>     
>     4. Can you document the get_authentication_credentials callback a bit more? What is an implementor specifically supposed to put in the structure when that function is called?
>     
>     Yes, I can flesh that out some more.
>     
>     5. Will the authenticator API calls be expected to be called by implemented modules when they deem it necessary?
>     
>     As currently defined, yes. However, I suspect that the general structure will be the same everywhere, so it may be worthwhile to wrap it all up in a "check authentication" function in res_sip for convenience.
>     
>     Endpoint identifier comments:
>     1. It's come up before that people want to change the matching order so we should probably remember to make this possible for the default provided ones (shouldn't be TOO hard), or describe how to do it.
>     
>     When you say matching order, do you mean the order in which endpoint identifiers are checked, or the order that endpoints are iterated over within each endpoint identifier? If it's the first, we could set up some sort of priority system so that you can state when an endpoint identifier gets called. Either that, or we could use module load order as a way of ordering.
>     
>     2. Should explicitly mention that the returned ast_sip_endpoint is an ao2 object and will be returned with the reference count bumped
>     
>     Sure, will do.
> 
> jcolp wrote:
>     Endpoint identifier #1: The other in which endpoint identifiers are checked. As long as we document how we choose to do this (be it module load order, or whatever) then I'm happy.
>     
>     I shall await the tweaking of the rest before responding further.

I have updated the page with what you suggested:

* ast_sip_digest_challenge_data has more explanation
* Added an ast_sip_check_authentication() function intended to be a simple one-call method to authenticate incoming requests
* Added notice that ast_sip_identify_endpoint() returns a refcounted object that needs to have its refcount decremented when completed
* Added note about endpoint identifiers being run based on module load order


- Mark


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


On Dec. 20, 2012, 1:17 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2251/
> -----------------------------------------------------------
> 
> (Updated Dec. 20, 2012, 1:17 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and jcolp.
> 
> 
> Summary
> -------
> 
> This is a proposal for a res_sip and res_sip_session API for use in the new SIP channel driver. The pages are located here:
> 
> https://wiki.asterisk.org/wiki/display/AST/res_sip+design
> https://wiki.asterisk.org/wiki/display/AST/res_sip_session+design
> 
> Please let me know what you think of these.
> 
> There are a few things that are not here and that probably should
> * A struct called ast_sip_endpoint is referenced in a few places, but it is not defined. This is because a SIP endpoint is more-or-less defined by the DAL, which is currently under development by Mr. Joshua Colp. Once endpoint configuration and related structures are defined, they can be added in to these pages.
> * There are no functions in res_sip_session for iterating over SDP media streams or attributes, nor are there any functions for aiding in creating SDPs. These likely should exist, but I have not placed them here now since I have difficulty seeing what parameters will be necessary nor what they might return.
> 
> 
> Diffs
> -----
> 
> 
> Diff: https://reviewboard.asterisk.org/r/2251/diff
> 
> 
> Testing
> -------
> 
> The wiki page renders properly.
> 
> 
> Thanks,
> 
> Mark
> 
>

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