[Home]

Summary:ASTERISK-24575: [patch]Make capath work for res_pjsip
Reporter:cloos (cloos)Labels:
Date Opened:2014-12-01 15:03:58.000-0600Date Closed:2015-01-16 11:47:28.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:pjproject/pjsip Resources/res_pjsip
Versions:SVN 12.7.1 13.0.1 Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) pj-ca-path-12.diff
( 1) pj-ca-path-trunk.diff
( 2) pjproject-ca-path.diff
Description:Every component which does tls supports capaths in addition to cafiles, except pjsip.

Comments:By: cloos (cloos) 2014-12-01 15:05:11.155-0600

Obviously the affected versions are everything with chan_pjsip, 12 thru trunk.

But the jira UI is too broken readily to specify that.
By: cloos (cloos) 2014-12-01 15:06:42.934-0600

Update asterisk to support ca_path settings for res_pjsip.

Depends on forthcoming pjproject patch to compile.

By: cloos (cloos) 2014-12-01 15:08:29.458-0600

Update pjproject to accept ca_path settings.

The openssl api already used accepts both a path to a file (for ca_file) and a path to a directory (for ca_path).

Openssl searches the ca_file (if specified) before searching the ca_path directory.

By: cloos (cloos) 2014-12-01 15:09:42.150-0600

The attached patches are initial untested versions.

I do not have a chan_pjsip config to test with, yet; still have to write one before I can test these two patches.

By: cloos (cloos) 2014-12-01 15:11:28.376-0600

Oh, and the asterisk patch applies cleanly to branches/13, too.

But not cleanly to branches/12.

By: cloos (cloos) 2014-12-03 14:39:34.684-0600

Updated patch against pjproject.

Now compile tested.

By: cloos (cloos) 2014-12-03 14:40:55.616-0600

This version cleanly applies to branch 12.
By: Rusty Newton (rnewton) 2014-12-04 09:44:10.117-0600

Go ahead and put the res_pjsip patches on Reviewboard. See the [Patch Contribution Process|https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process].

Add a link on this issue to your review.

Remember that new features are generally accepted only for trunk, so your review should be submitted against trunk.
By: cloos (cloos) 2014-12-04 09:50:07.955-0600

The reviewboard link is:  https://reviewboard.asterisk.org/r/4230/

By: Mark Michelson (mmichelson) 2014-12-04 16:25:52.977-0600

I had a look at the Asterisk code review on reviewboard. Aside from what opticron mentioned about alembic scripts, it looks good to me.

The PJSIP code has potential problems though:

{code}
-    PJ_ASSERT_RETURN(pool && CA_file && cert_file && privkey_file, PJ_EINVAL);
+    PJ_ASSERT_RETURN(pool && CA_file && CA_path && cert_file && privkey_file, PJ_EINVAL);
{code}

From what I understand about CA_file and CA_path, only one needs to be provided. This code will assert if one of CA_file or CA_path is NULL. From Asterisk, this won't ever happen, since AST_STRING_FIELDs can never be NULL. But if this is being added and to be used by other projects, this would need to be altered to allow for one of CA_file or CA_path to be NULL. Similarly, the subsequent {{pj_strdup_with_null()}} calls for CA_file and CA_path in this function would need to be made NULL-tolerant.

Also, I don't know how OpenSSL handles calls to SSL_CTX_load_verify_locations() when it is given empty strings for the CAfile or CApath parameters. https://www.openssl.org/docs/ssl/SSL_CTX_load_verify_locations.html mentions what happens when NULL parameters are passed in, but it doesn't mention what happens when an empty string is passed in. If behavior is the same as for a NULL pointer, that's fine, but I just want to make sure we're not setting up a situation where we cause breakage because we end up passing an empty string instead of a NULL pointer into the function.
By: cloos (cloos) 2014-12-04 16:35:28.393-0600

Mark,

Thanks.  That assert change is indeed wrong.

And file does have to be NULL or a valid pem file for path to be considered.
(cf X509_STORE_load_locations() in crypto/x509/x509_d2.c)

I’ll fix the patch to ensure that empty strings are not sent.

Thanks again.

By: Mark Michelson (mmichelson) 2015-01-13 10:40:05.427-0600

Hey there, I was looking to get this issue moved forward. Since it appears that the PJProject patch has not been updated since the original problems were pointed out, I was planning on taking over that patch's development (with credit being given to you, the original author, of course). Is this okay, or do you have a second version of the patch that has not been uploaded yet?
By: cloos (cloos) 2015-01-14 10:54:34.038-0600

Mark,

I got behind on too many things and didn’t get back to this.

Based on one of the comments I intended the new patch to add two APIs, one to specify just a path and one to specify both, alongside the current API.

I still have a pile of things to get done, so please feel free to take that part over and get it pushed.

Thanks for asking.

By: Mark Michelson (mmichelson) 2015-01-14 13:41:02.642-0600

No problem! I'll get this done as soon as possible.
By: Mark Michelson (mmichelson) 2015-01-15 08:44:01.535-0600

Teluu were pretty quick to merge in the CA path support patch. I'm going to post a new review board request that has the Asterisk-side changes I made.
By: Mark Michelson (mmichelson) 2015-01-15 09:43:54.210-0600

The new version of the Asterisk review is at https://reviewboard.asterisk.org/r/4344/