Summary:ASTERISK-06629: [branch] SSL encryption for Asterisk Manager Interface (AMI)
Reporter:John Todd (jtodd)Labels:
Date Opened:2006-03-27 21:24:25.000-0600Date Closed:2006-12-08 09:01:26.000-0600
Versions:Frequency of
Environment:Attachments:( 0) ami-ssl-patch2
( 1) ssl.txt
( 2) ssl-patch.txt
( 3) ttb.diff
( 4) ttb2.diff
Description:It is our humble opinion that everything going over an IP network should be encrypted from end to end, or at least from an end to an intermediate trusted point.  The Manager Interface certainly fails that motto currently, so we've developed a solution.

Several files are enclosed which should work on SVN TRUNK as of today (2006-03-27) for enabling the encryption of AMI socket connections via OpenSSL.  This is completely backwards-compatible with existing AMI clients, since the system will just wait on the socket for a configurable number of milliseconds before failing over to non-encrypted versions (if desired.)

The new Makefile will automatically generate a cert, since it is expected that nobody will ever actually do this without it happening as a matter of course.  ;-)  Of course, normal certs can be utilized.

Requires OpenSSL, but that is not a big deal since we already require OpenSSL for other tasks.  Can easily be commented out (one line) during compile.

These patches are being contributed by Tello Inc.  I have a disclaimer on file, and I will submit a disclaimer for Tello shortly, as I assume that is required.  

Concept/some docs: John Todd (jtodd@tello.com)
Programmers: Mahesh Karoshi (mkaroshi@tello.com) and Remco Treffkorn (rtreffkorn@tello.com)
Comments:By: Olle Johansson (oej) 2006-03-27 22:00:07.000-0600

Created branch "team/oej/ami_ssl"

By: Olle Johansson (oej) 2006-03-29 10:40:01.000-0600

Updated branch with some code formatting and coding guidelines fixes. Please review.

By: John Todd (jtodd) 2006-03-29 11:55:20.000-0600

Looks fine!  Thanks for the updates.  The changes all look appropriate.  The folks that developed this are "first-time" Asterisk developers, so some of the WARNING versus ERROR stuff is learned with time.

For the rest of the modifications, I would make one change:

In my original patch, the Makefile had -DAMI_WITH_SSL commented out, which was accidental.  I'd actually suggest that your new version using ASTCFLAGS be set to compile SSL AMI support by default. (the notes indicates that it will be "disabled in svn")

We already use OpenSSL, so it's not like this requires a "new" dependency.  Additionally, I fear that most people never change the default settings of their Makefile, and most third-party applications will want to use SSL, so we have a mismatch in expectations.  If Asterisk compiles the SSL support in by default, there is a much higher chance that the most widespread mode of use will be... the default: SSL.

By: davetroy (davetroy) 2006-03-30 11:18:47.000-0600

I downloaded Olle's ami_ssl branch and it worked great first try.  I was able to connect via SSL without a problem and the implementation seems well thought out and logical.

I am going to proceed with trying to integrate the same codebase into AstManProxy (as well as add in the Challenge action and a few more features contributed by users).  Of course in AstManProxy we'll have to add both client and server functionality, which ought to be interesting...

Good job getting this done and out there!  I've been meaning to delve into this for a while...

By: Olle Johansson (oej) 2006-04-04 09:40:42

Updated branch to trunk, resolved conflict caused by recent changes of manager.c

By: davetroy (davetroy) 2006-04-04 17:37:04

For what it's worth, I have integrated the same SSL code into astmanproxy:

With this addition and the use of the astmanproxy http driver, it now supports https directly, so you can do things like this:

And get output back as XML, plaintext, or csv.

So, with SSL in proxy and in manager.c, astmanproxy enables client->proxy->server connections to be ssl (or not) on a per-connection basis.

Will have a major release on this shortly.

By: John Todd (jtodd) 2006-04-13 15:51:17

Olle -
 Please make this minor patch in the branch that you're maintaining.  This is a fix for the client test code that we include for sample use.

[mkaroshi@karoshi wm]$ diff -Naur ../client_ssl.c client_ssl.c
--- ../client_ssl.c     2006-04-13 13:37:47.000000000 -0700
+++ client_ssl.c        2006-04-13 13:40:01.000000000 -0700
@@ -181,7 +181,7 @@

      //printf("closing the socket: %d\n", socket);
-       ret= close(socket);
+       ret= close(get_real_fd(socket));

      if (ssl)
              SSL_free (ssl);

By: davetroy (davetroy) 2006-04-24 21:34:33

What is the status on whether this will go into trunk?

By: John Todd (jtodd) 2006-05-08 18:05:50

To make the current ami_ssl tree work, after "./configure" you'll need to use "make CPPFLAGS=-I/usr/kerberos/include" to find the appropriate Kerberos libraries.  This probably should be automagically found by the "configure" script, but we're not autoconf wizards...

By: Mark Spencer (markster) 2006-05-11 03:40:56

I have a few concerns about this "inverted file descriptor" method.  Why aren't we just using structures?  It should be the case now, by the way, that manager is isolated from direct access to its file descriptor so it may be much easier to add as a "flavor" of manager just like the HTTP manager stuff.

By: John Todd (jtodd) 2006-05-11 15:44:50

The structure-based way to do this I don't think existed at the time the patch was written, back in March (but I could be wrong.)  The patch is being re-written with the structure method, and should be available shortly.

By: Olle Johansson (oej) 2006-05-16 10:14:17

Added to test-this-branch

By: John Todd (jtodd) 2006-05-16 12:56:24

Patch for structure support should be available by Friday of this week.

By: Olle Johansson (oej) 2006-05-16 14:51:05

Moved the branch to a group branch so other coders can assist in maintenance.

By: John Todd (jtodd) 2006-05-16 18:33:33

There apparently were significant missing pieces of code from the existing SVN tree (ami-ssl) that are addressed by the patch I just attached (ami-ssl-patch2) which should be applied to the "live" code in the ami-ssl branch.

By: Olle Johansson (oej) 2006-05-17 01:08:16

Thanks, John. Added your patch.

By: John Todd (jtodd) 2006-05-17 19:37:18

Small change for compile on CentOS which apparently works 100%:

Change res/res_agi.c line 120 from:
 ast_carefulwrite(fd, stuff, strlen(stuff), 100);


 ast_carefulwrite(fd, stuff, strlen(stuff), 100, NULL);

...but then for some inexplicable reason, I'm unable to get it to compile on RH9.0 because of krb5.h failures, despite my setting CPPFLAG=-I/usr/kerberos/include before ./configure.  It seems to include the directory OK right up to chan_zap, where the -I flag vanishes from the compilation:

make[1]: Entering directory `/usr/src/ami_ssl/channels'
gcc -c -o chan_zap.o -include ../include/autoconfig.h -I../include -I.. -fPIC -DAMI_WITH_SSL  -pipe -Wall -Wstrict-prototypes -Wmissing-prototypes -Wmissing-declarations -g3 -O6 -march=i686  -fomit-frame-pointer  chan_zap.c
In file included from /usr/include/openssl/ssl.h:179,
                from ../include/asterisk/ssl_addon.h:19,
                from ../include/asterisk/manager.h:31,
                from chan_zap.c:100:
/usr/include/openssl/kssl.h:72:18: krb5.h: No such file or directory

By: Serge Vecher (serge-v) 2006-05-18 16:12:02

jtodd: oej has fixed the agi issue in r28393 (reverted in r28395)

By: John Todd (jtodd) 2006-05-19 01:09:07

OK, I can't speak to the AGI issue, but the kerberos includes are still failing on  RH9.0 in the same way I described on 2006-05-17.

By: Olle Johansson (oej) 2006-05-19 01:33:23

Well, that fix was bad, seems like svn is out of synch somewhere. I'll try to look at it today.

By: paul erkkila (pee) 2006-05-19 17:31:53

the tiny diff I just attached fixes the error for me. It's quick and doesn't fix the #ifdef logic like it should. I just want to get to testing ;)

By: paul erkkila (pee) 2006-05-19 17:40:30

the cert: and certificate: targets in the makefile ignore --prefix and other path info from configure.

the cert: and certificate: targets use explicite calls to sudo, which is a bit weird since it kind of assumes you need to be root to run asterisk ;).

By: Serge Vecher (serge-v) 2006-05-20 21:03:33

pee: what's this change all about?

- AST_LIST_HEAD_INIT(&parkinglots);
+ AST_LIST_HEAD_INIT_NOLOCK(&parkinglots);

By: paul erkkila (pee) 2006-05-21 09:47:35

That's just another small fix in the "test-this-branch" branch. Just drop it from the patch if you're not in that branch.

By: Serge Vecher (serge-v) 2006-06-06 16:39:41

Is this 1.4 material?

By: Olle Johansson (oej) 2006-06-14 09:08:15

I've updated the /team/group/ami_ssl branch to current trunk. Seems like we get a compilation error in test-this-branch - something about pthreads. Can the developer please check into this and see if we need another fix. If there are any other updates to this code, please upload now.

Thanks for your cooperation.

By: Remco Treffkorn (rtreffkorn) 2006-06-15 15:10:18

Ok. I fixed the compile problem. Asterisk.h was not the first file included, and this created a problem. We will re-test this afternoon.

By: Serge Vecher (serge-v) 2006-06-15 15:24:18

rtreffkorn: could you please submit a patch for the following branch http://svn.digium.com/svn/asterisk/team/oej/test-this-branch/ ? That's the one that will not compile. Thanks

By: Remco Treffkorn (rtreffkorn) 2006-06-15 16:33:51

I uploaded ttb2.diff. That fixes the two problems I do understand.
I looked at channels/chan_sip.c, and it looks to me this file is corrupt.
I don't think this is related to ssl changes.

By: Serge Vecher (serge-v) 2006-06-15 16:39:04

patches for test-this-branch as requested.

By: Anthony LaMantia (alamantia) 2006-09-05 00:36:37

do you have a diff off the the current subversion trunk?

By: jmls (jmls) 2006-11-01 10:33:21.000-0600

work is being done on this by rizzo. stay tuned ..

By: John Todd (jtodd) 2006-11-01 11:45:03.000-0600

I am actually unclear on the status of this patch.  It was created long ago, and patched against TRUNK.  It worked well.  In fact, it was tested enough that even astmanproxy supports it.  However, it was not evaluated for inclusion into trunk for reasons opaque to me.  Now, there is an HTTP-based manager interface, which can have SSL layered onto it.  (Why a duplicate effort?  Unclear to me.)  That is now in TRUNK as far as I know.  So what of this patch?  Should it be marked as closed or rejected?

By: Luigi Rizzo (rizzo) 2006-11-06 02:41:07.000-0600

jtodd: first-sorry for not coordinating with you when i worked on ssl support.
i was not aware of the patch when i started, and when i found it the
work was already done.
From a purely technical point of view:
1. obviously i am perfectly fine in centralizing in one file the routines
  to handle the ssl initialization. The reason why i put them in http.c
  was that in trunk there was no other user yet and we needed to start
  somewhere, but moving it is trivial. So, in this respect, there is no
  real difference between this code and what is in trunk.

2. The above (as probably discussed by others) should definitely be taken
  a step further, by centralizing also the basic socket/bind/listen/accept
  calls to avoid replicating it. This is something that neither this patch
  nor the code in trunk does, and requires a bit of restructuring of the
  client code (http.c, manager.c etc.) which does its own "polling" of the
  socket within a loop.

3. what i don't like in this specific patch is the way
  "encrypted descriptors" are handled, because
  it is not transparent to applications, which need to be aware of that
  by calling the s_ version of read() and write().

  It's not just that "i don't like the approach", the problem is that it
  makes debugging the code a nightmare, because you have to be sure,
  when you pass a descriptor around, that all interested pieces of code
  do the proper changes. This is easy when you have a small number of
  such places, it is a lot harder when you want to bring SSL support to
  e.g. the CLI or other third party code where you don't have a lot of
  control on how descriptors are used.

4. This said, the code in trunk (http.c) handles a slightly different
  case, because http.c uses FILE * for I/O, and for those there is a
  completely transparent solution (the one that is in trunk) that uses
  fuopen() or fopencookie() to provide the client with a FILE * which
  can handle ssl in a totally transparent way.

5. Unfortunately the solution above doesn't work with low-level file
  descriptors. Here we need some good idea e.g. possibly playing with
  the linker to replace the standard IO library calls with our own,
  ssl-aware, in a transparent way, or some smart trick to hide
  the ssl hooks underneath.
  At the moment i don't have good ideas here (other than using a separate
  thread to provide the encryption between the ssl descriptor and
  the standard one).

By: Luigi Rizzo (rizzo) 2006-12-08 05:43:24.000-0600

Basically, the feature has been imported in trunk, although
not with the patch provided.
Yesterday's commits provides AMI-over-TLS support using the same
mechanism (and code) used for HTTPS.

By: Serge Vecher (serge-v) 2006-12-08 09:01:26.000-0600

scroll keeper adds "as of r.48351." Thanks everyone for good work ...