Summary: | ASTERISK-24575: [patch]Make capath work for res_pjsip | ||
Reporter: | cloos (cloos) | Labels: | |
Date Opened: | 2014-12-01 15:03:58.000-0600 | Date Closed: | 2015-01-16 11:47:28.000-0600 |
Priority: | Major | Regression? | No |
Status: | Closed/Complete | Components: | 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/ |