Summary:	ASTERISK-15084: [patch] RFC 4474 Implementation of SIP identity
Reporter:	Patrick A Looby (plooby)	Labels:	patch
Date Opened:	2009-11-05 21:36:51.000-0600	Date Closed:
Priority:	Major	Regression?	No
Status:	Open/New	Components:	Channels/chan_sip/NewFeature
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) ics_identity_228661.patch ( 1) ics_identity.patch ( 2) patch_oej_sip-identity-trunk_235773.patch
Description:	The attached patch provides an RFC 4474 implementation for asterisk trunk. Keys and certificates may be obtained from https://reference.inchargesys.com/ /ed
Comments:	By: Olle Johansson (oej) 2009-11-06 02:17:47.000-0600 We need to test this code with Kamailio, that also have an implementation of SIP identity. By: Olle Johansson (oej) 2009-11-06 02:19:19.000-0600 +static char identity_priv_url_prefix[IDENTITY_PRIV_URL_PREFIX]; /*!< private url prefix is stored here used to fetch +  the user private key from http or https server*/ +static char identity_priv_url_suffix[IDENTITY_PRIV_URL_SUFFIX]; /*!< private url suffix is stored here used to fetch +  the user private key from http or https server +  which can be .pem or .cer*/ +static char identity_pub_url_prefix[IDENTITY_PUB_URL_PREFIX];   /*!< public url prefix is stored here used to fetch the +  user public key from http or https server used */ +static char identity_pub_url_suffix[IDENTITY_PUB_URL_SUFFIX];   /*!< public url suffix is stored here used to fetch the +  user public key from http or https server used */ +static char identity_priv_path[AST_CONFIG_MAX_PATH];            /*!< location where private keys are stored */ +static char identity_pub_path[AST_CONFIG_MAX_PATH]; /*!< location where public keys are stored */ +static char identity_cache_path[AST_CONFIG_MAX_PATH];        /*!< location where keys and certs are cached */ + Can you make a structure of this, as we will need a key per domain? By: Olle Johansson (oej) 2009-11-06 02:19:44.000-0600 int rfc4474res; /*!< rfc 4474 result signed or validated etc*/ This one should be an enum, the one you defined above By: Olle Johansson (oej) 2009-11-06 02:20:34.000-0600 static const char* ident_status(enum rfc4474_res code) RFC numbers are not permanent, can we use "sipident" as a prefix instead of "rfc4474" ? By: Olle Johansson (oej) 2009-11-06 02:21:31.000-0600 default: + rv="rfc4474 identity unknown status"; + Since you are using enum, you should not have to have a default. Without it, the compiler will complain if someone adds an ENUM value that you haven't got in the ident_status function. By: Olle Johansson (oej) 2009-11-06 02:22:57.000-0600 In fucntion id_check_cache there's a lot of logging that propably should move to ast_debug. I don't want a NOTICE message when keys are loaded and it works. By: Olle Johansson (oej) 2009-11-06 02:25:50.000-0600 Since Asterisk already uses CURL, wouldn't it be better to use libcurl instead of implementing our own HTTP client? We could make it a compile dependency for chan_sip. I also think about all these certificate read stuff. Maybe that belongs in generic functions outside of chan_sip. Asterisk as a product have a certificate store - shouldn't we use that instead of implementing yet another one in chan_sip? By: Olle Johansson (oej) 2009-11-06 02:27:45.000-0600 The base64 encode functions already exist for voicemail. We don't need to duplicate those. Regardless, they belong in asterisk core, not in the channel By: Olle Johansson (oej) 2009-11-06 02:28:54.000-0600 I do appreciate all the doxygen comments! Thanks! By: Olle Johansson (oej) 2009-11-06 02:30:32.000-0600 Check if you need to run ast_free(sig) in more places in the function id_verify. By: Olle Johansson (oej) 2009-11-06 02:32:42.000-0600 This code needs to be prepared to handle multiple domains. By: Olle Johansson (oej) 2009-11-06 02:33:29.000-0600 bx_builtin_setvar_helper(c,"rfc4474_id_result",res); Channel variablese that asterisk sets are always UPPER CASE By: Olle Johansson (oej) 2009-11-06 02:34:17.000-0600 if (!strcasecmp(v->name, "identity_rfc4474_priv_url_prefix")) { Skip the rfc4474 part here. By: Olle Johansson (oej) 2009-11-06 02:36:17.000-0600 We propable need an attribute in the caller ID structure attached to the ast_chan to make this more multiprotocol-like. A kind of trust attribute that you can check and act on in the dialplan. How we're going to implement that is a discussion for asterisk-dev By: Birger "WIMPy" Harzenetter (wimpy) 2009-11-06 02:45:08.000-0600 I would suggest a CALLERID(screen) to get interoperability with Q931. By: Olle Johansson (oej) 2009-11-06 02:45:22.000-0600 +; URL port override (80=default) +priv_url_port=80 +pub_url_port=80 THis is not a good port, since we have HTTP services on that port ;-) By: Leif Madsen (lmadsen) 2009-11-06 09:18:30.000-0600 Wow, all these comments really should be moved into a reviewboard issue. By: Olle Johansson (oej) 2009-11-06 10:46:55.000-0600 I'll start with a branch where Ed and I can work on this. We have some design issues to sort out first. By: Patrick A Looby (plooby) 2009-11-08 11:17:30.000-0600 ics_identity_228661.patch addresses naming and type issues discussed above.  It preempts the prior patch.  More to come after discussions occur. By: Ed Guy (edguy3) 2009-12-11 14:18:20.000-0600 I've got a Debian repository containing daily builds of trunk and sip_identity available for lenny i386.   See http://debs.VoipEnabler.com/  for details. Please drop me a note if there are any problems with it. /ed guy By: Olle Johansson (oej) 2009-12-13 02:04:08.000-0600 Ed and I discussed this issue on Friday and I need to create a branch for it. Will do soon. By: Olle Johansson (oej) 2009-12-14 04:57:31.000-0600 Created branch sip-identity-trunk and sip-identity-1.4 for testing and further development. Re-read the RFC and it has some pretty strong text around E.164 numbers and identity we need to check. By: Olle Johansson (oej) 2009-12-14 05:01:05.000-0600 Any patches/changes from now on has to be related to the branch. By: Ed Guy (edguy3) 2009-12-20 13:42:07.000-0600 I cant check in directly yet, so the attached file:    patch_oej_sip-identity-trunk_235773.patch makes most of the improvements discussed. The patch is wrt the new branch. (Note: There are a couple non-style-compliant comments that will be removed when the code is checked into svn.) By: Ed Guy (edguy3) 2010-01-07 19:04:46.000-0600 Note: I'm placing daily builds of this branch at http://debs.voipenabler.com/ Patches are here < http://debs.voipenabler.com/patches > as well. By: snuffy (snuffy) 2010-01-07 20:09:24.000-0600 edguy3: Your patches folder shows a problem in diff generation since 21st december. Might want to fix that By: Leif Madsen (lmadsen) 2010-03-12 14:45:40.000-0600 To move this forward, we need to get this on reviewboard so it can be reviewed. All non-trivial changes ideally go through reviewboard for community review. By: Olle Johansson (oej) 2010-03-12 15:29:26.000-0600 I think it's important that we test interoperability too. This code will have to be tested so it's generic enough to be included and just not made for a particular service. By: Leif Madsen (lmadsen) 2010-03-15 13:30:20 Agreed. Whoever is working on this issue is welcome to perform those tests.  My comments were added after speaking with Ed Guy via email a couple of days ago as he was curious how to move this forward for inclusion in trunk. By: Olle Johansson (oej) 2010-03-15 13:40:20 The reason that I'm particularly worried about this large patch is that it's made for a very specific solution promoted by a company and we need to make sure that it's generic before we decide to include it. Testing at SIPit is propably a good thing, unless we can arrange tests before that. By: Leif Madsen (lmadsen) 2010-03-15 14:49:48 I don't disagree.