| Summary: | ASTERISK-21894: [patch] Initial support for SIP/TLS tlsverifyclient | ||||
| Reporter: | Serhij Stasyuk (stas) | Labels: | patch | ||
| Date Opened: | 2013-06-10 11:12:51 | Date Closed: | |||
| Priority: | Major | Regression? | |||
| Status: | Open/New | Components: | Channels/chan_sip/TCP-TLS | ||
| Versions: | SVN 11.4.0 13.18.4 | Frequency of Occurrence | |||
| Related Issues: |
| ||||
| Environment: | Attachments: | ( 0) asterisk-trunk-siptls.patch | |||
| Description: | Here is initial support for tlsverifyclient for sip channels.
Now it "works" only for peers. RFC 5922 http://tools.ietf.org/html/rfc5922 requires server to compare domain name with SIP headers. This is not done yet. The very first thing that is verified during mutual TLS verification on server side is certificate exchange. OpenSSL handles all certificate-related tasks but it does not verify CN and subjectAltName against desired one. Desired name (SIP) is not exactly available at the moment of SSL session establishment, so the only name we can use is host peer field from config. This comparison is done by this patch. I'm not sure what is the reason of disabling wildcard certificate matching required by Section 7.2 of RFC 5922 <http://tools.ietf.org/html/rfc5922#section-7.2> Wildcard certificate is very convenient mechanism for some deployment schemes and is adopted by customers and service providers and I see no reason to restrict their usage here. If it is required, configuration option can be introduced, like tlsallowwildcards. Patch can be easily adopted to it instead of constant. | ||||
| Comments: | By: Serhij Stasyuk (stas) 2013-06-10 11:21:59.835-0500 Sorry, will add patch later, after signing submission license agreement By: Michael L. Young (elguero) 2013-06-11 09:51:03.197-0500 I noticed that you patch is against Asterisk 11.3.0. In order for new features to be accepted, we will need a patch against trunk. By: Serhij Stasyuk (stas) 2013-06-12 08:30:07.839-0500 Ok, thanks, will prepare patch against trunk By: Michael L. Young (elguero) 2013-06-12 11:17:03.532-0500 Serhij, Thanks for your contribution! It would appear that this is your first time contributing. I would like to suggest that you look at the following information: - [Coding Guidelines|https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines] - [Patch And Code Submissions|https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines#AsteriskIssueGuidelines-PatchandCodesubmission] - [Reviewboard Usage|https://wiki.asterisk.org/wiki/display/AST/Reviewboard+Usage] In looking briefly at your patch, there are some code format and guideline issues that need to be addressed. Thanks again for your contribution to the project. By: Serhij Stasyuk (stas) 2013-06-13 06:00:45.967-0500 Thanks, Michael. > In looking briefly at your patch, there are some code format and guideline issues that need to be addressed. I'll look deeper into it. By: Serhij Stasyuk (stas) 2013-06-13 07:31:52.974-0500 Sorry, I've just re-checked my patch and I do not see format issues there :( Please ignore formatting of tcptls-wildcard.h and tcptls-wildcard.c -- I tried to keep them in the closest form with the original source (as it is mentioned in their headers) -- OpenSSL 1.0.2 (crypto/x509v3/v3_util.c and crypto/x509v3/x509v3.h). Maybe better to name them tcptls-x509v3.[ch] Large block in tcptls.c with space-only change can't be avoided from my POV -- I need to split "else if" line and enclose "if" into "else" block. Also "2.2. File structure and header inclusion" from "Coding Guidelines" is not related for new files in this patch (tcptls-wildcard.[ch]) -- they do not use anything from asterisk and I really do not know what to write in their "copyright" section - they are just extract of new OpenSSL features needed by normal certificate names verification. Also I am not sure what is the best place to put definition of sip_tcptls_pressl, so I put it right before first usage (static struct ast_tcptls_session_args sip_tls_desc). I moved to a new line \brief before sip_tcptls_pressl implementation according to guidelines. I requested access to Review Board from Matt Jordan, as it is mentioned in "Reviewboard Usage". I am planning to implement configurable wildcard certificate matching and certificate verification depth by separate patches to minimize their sizes. Please point me to issues in my patch so I fix them. Sorry for inconvenience and thanks a lot for your help. By: Michael L. Young (elguero) 2013-06-13 08:20:34.830-0500 Nothing major but here were the items that I saw. You are using C++ style commenting. Also, not sure that we need this commented out bit in there. {noformat} ast_debug(3, "Remote address is '%s'\n", ast_sockaddr_stringify_host_remote(&tcptls_session->remote_address)); // (const char *peer, struct ast_sockaddr *addr, char *callbackexten, int realtime, int which_objects, int devstate_only, int transport) // Does not work: peer = sip_find_peer_full(NULL, &tcptls_session->remote_address, NULL, FALSE, FINDALLDEVICES, FALSE, 0); ao2_lock(peers); {noformat} There should be a space after the "for" and before the "(". "for (k=0..." {noformat} ao2_iterator_destroy(it_peers); for(k = 0; k < total_peers; k++) { peer = peerarray[k]; {noformat} In regards to the other files you are bringing in, I think someone else will need to chime in on those and the best way to handle bringing that in. I know that we have to make sure about licensing and stuff. Thanks for your contributions... I am sure the community will appreciate them. By: Serhij Stasyuk (stas) 2013-06-13 08:41:29.737-0500 Ouch, sorry. Really forget about "sip_find_peer_full" lines - I was trying to make my patch working with API function but my tries were not successful :( Maybe someone more experienced will check it later. Wrapped with TODO and enclosed into C-style comment. "for(k=" was copied from _sip_show_peers without proper attention to indentation, sorry. Fixed. Also changed "struct ao2_iterator* it_peers;" to "struct ao2_iterator *it_peers;" to make look of variable definitions more unified. "ast_verb" lines merged into one. Its length doesn't exceed limit. Again, thanks a lot for your help and time. By: Matt Jordan (mjordan) 2013-06-13 12:08:10.245-0500 I'm curious: why are we including large swathes from OpenSSL? Reproducing chunks of code from other libraries is generally a bad idea. If this code requires a particular version of OpenSSL to compile, then it should make that determination based on the results of {{configure}} and optionally enable those features in Asterisk. By: Matt Jordan (mjordan) 2013-06-13 12:09:27.816-0500 Additionally, code submitted to Asterisk should use the standard Asterisk preamble. By: Serhij Stasyuk (stas) 2013-06-13 12:16:15.274-0500 > why are we including large swathes from OpenSSL? OpenSSL version 1.0.2 is not released yet, it is available only from SVN (version number is already defined in it). I'm not sure what version it will be exactly and when it will be released. > Additionally, code submitted to Asterisk should use the standard Asterisk preamble. I've already commented it if you are talking about "2.2. File structure and header inclusion". If it is about something else please point me to the guide or note. Thanks for your comments and in advance for more help from you :) By: Matt Jordan (mjordan) 2015-03-14 10:17:40.537-0500 Thanks for the contribution! If you'd like your contribution to be included faster, you should submit your patch for code review by the Asterisk Developer Community. To do so, please follow the Code Review [1] instructions on the wiki. Be sure to: * Verify that your patch conforms to the Coding Guidelines [2] * Review the Code Review Checklist [3] for common items reviewers will look for * If necessary, provide tests for the Asterisk Test Suite that verify the correctness of your patch [4] When ready, submit your patch and any tests to Review Board [5] for code review. Thanks! [1] https://wiki.asterisk.org/wiki/display/AST/Code+Review [2] https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines [3] https://wiki.asterisk.org/wiki/display/AST/Code+Review+Checklist [4] https://wiki.asterisk.org/wiki/display/AST/Asterisk+Test+Suite+Documentation [5] https://wiki.asterisk.org/wiki/display/AST/Review+Board+Usage | ||||