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-0600Date Closed:
Versions:Frequency of
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

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

+ 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


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)

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:
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.