Review Board 1.7.16


res_sip and res_sip_session design review

Review Request #2251 - Created Dec. 20, 2012 and submitted

Mark Michelson
Reviewers
asterisk-dev
jcolp, mjordan
Asterisk
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.
The wiki page renders properly.
Posted (Jan. 3, 2013, 1:34 a.m.)
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
  1. 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.
  2. 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.
  3. 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
Posted (Jan. 7, 2013, 4:06 a.m.)
res_sip_session --

Structures:

1. How does a session extension persist data on a session? mod_data in pjsip_inv_session?
2. How does a session extension know the life time of the session? Does it know based on the messages? (Should we have common logic which then calls into callbacks in the extension)?
3. I'm not a huge fan of using "extension" in the way you have, I understand why but it has other meanings in our system.
4. The SDP stuff isn't just for INVITEs - it's for 200 OKs, 180s, 183s, etc.
5. If the SDP stuff was extended more we could have the complete logic for each type of media (audio, video, T.38 udptl) separated out into different modules... would make organization cleaner. 

Common SIP methods:

1. Your ast_sip_session_get_identity is missing the rdata
2. How does something calling ast_sip_session_send_reinvite know the result?
  1. Structures:
    
    1. I'm not sure that mod_data in the pjsip_inv_session would be appropriate because it's likely that res_sip_session will be storing the ast_sip_session in the pjsip_inv_session mod_data. I can think of two possibilities for this:
        a) We copy the "session cookies" model from Asterisk SCF. In other words, given a SIP session, a session extension can add a cookie or retrieve a cookie as desired. The ast_sip_session structure would be where the data is stored.
        b) We add session_begin() and session_end() callbacks to the session extension callbacks so that session extensions can have more clear points to create and destroy session-specific data. The session extensions could then store the data locally where they please.
    
    Personally, I'm a fan of a) over b).
    
    2. Currently, a session extension would have to infer this information based on the content of the messages. Given that this could be error-prone, I think providing callbacks for when a session is begun and ended would be a safer option.
    
    3. Yeah, agreed. I'm certainly open to suggestions for a better name since things can really get confusing if someone mentions a "SIP extension".  Maybe "adjunct", "addendum", or "supplement" (Yay thesauruses!) ?
    
    4. Yep, I'll change language so that words like "offer" and "answer" are used instead of specific SIP methods.
    
    5. Indeed. SDP handling is something that needs a bit more effort put in here. Right now it's encompassed in a single handler but really, it should be separated by stream types. "Audio" "Video" "Image" "Text" and "Other" perhaps? Maybe specifically state T.38 instead of image?
    
    Common SIP methods:
    
    1. Will fix
    2. I assume by "result" you mean the response to the reinvite? My assumption is that the majority of entities that would call for a SIP reinvite to be sent would be SIP session extensions. When the response comes in, they'd have their incoming_response() callback called. If something other than a SIP session extension were to send a reinvite, they would not currently be able to see the response to the reinvite. Got any suggestions of how to go about it? Maybe provide an optional callback function in ast_sip_session_send_reinvite() when the response arrives?
  2. 1. Agree with 'a'
    2. Agreed
    3. Hum hum hum, supplement for now?
    4. Yay!
    5. I think we should make it generic and have a handler specify a string which is the type carried on the media stream. That way in the future if people want to add even more (such as application) then no code changes needed. As well - being able to support multiple handlers of the same type with order loaded taking preference would be handy... first one that handles it stops processing. Cumulative implementation of types!
    
    2. I think an optional callback would work nicely. Otherwise code duplication can occur for it.
Posted (Jan. 8, 2013, 9:13 a.m.)
I've updated the wiki pages to have the recommended changes from Joshua and from Saul on the dev list

* There is a function to append body data and a function to add multipart bodies.
* SDP handling has been changed to use handlers of individual stream types instead of handlers for entire SDPs.
* ast_sip_session_send_reinvite() now has an optional callback to be called when the response is received.
* ast_sip_session now supports session cookies.
* "session extensions" are now "session supplements" in all cases.
* Errors regarding missing parameters in functions have been cleared up.
  1. Also added session_begin() and session_end() callbacks to session supplements.
Posted (Jan. 22, 2013, 9:45 a.m.)
I'm going to close this review. At this point, no one is commenting anymore and we've already begun writing code. You'll have another chance to comment on things once the first set of code gets up for review.

https://reviewboard.asterisk.org/ runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to asteriskteam@digium.com.