[Home]

Summary:ASTERISK-10512: [patch] Add HTTP Basic & Digest Auth (rfc2617) for manager web interface.
Reporter:Yuri (ys)Labels:
Date Opened:2007-10-12 06:48:58Date Closed:2010-05-17 10:16:56
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/NewFeature
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) digest_auth_r108895_v3.diff
( 1) digest_auth_r118255_v4.diff
( 2) digest_auth_r148468_v5.diff
( 3) digest_auth_r99859_v2.diff
( 4) trunk_chan_sip.diff
( 5) trunk_managerhttp_digest_auth.v1.diff
( 6) trunk_utils.diff
Description:I found, that manager web interface used "Cookie" Header for authenticate the user. This require two http request, one for authenticate and next for commands.
This patch add only Basic authentication scheme implementation, as defined in rfc2617.
If used this scheme, httptimeout are unused, but we don't need to keep a http session (and mansession) alive, after HTTP Request is processed.








****** ADDITIONAL INFORMATION ******

This patch is touched next files:

main/http.c
main/manager.c
include/asterisk/http.h
configs/manager.conf.sample

You can choise method by "webauth" parameters in manager.conf:
webauth = http_auth ; use rfc2617 authentication
webauth = cookie  ; use Cookies to store user ident.
Comments:By: Brandon Kruse (bkruse) 2007-10-26 09:03:56

This is very interesting.

I am a GUI developer, and use the http interface and manager over http EXTENSIVELY.


This is very cool, glad to see new things being added in terms of that.

-bk
By: Brandon Kruse (bkruse) 2007-12-06 09:35:43.000-0600

If we could add this, it would be awesome.

I think overall, (even in manager.c it says /* XXX we should probably not read the config file on every authentication attempt */) this would be great for any system with intensive manager activity.

-bk
By: Marcos Jose Setim (msetim) 2007-12-06 10:21:22.000-0600

It's a nice feature.

I had work with HTTP interface and I always have to send a GET to check the connection state :'(

I was reading your perform implementation and I have a question. When the manager will re-read the users.conf and manager.conf to get a new user put into memory.

Congratulations ys

Thanks


By: Olle Johansson (oej) 2007-12-10 07:42:06.000-0600

Basic HTTP auth is *very* insecure. I don't really like supporting it. There's a reason why we don't support it in the SIP standards any more.

We already have code for digest HTTP auth in chan_sip, maybe we need to move that to a common library and use it for both modules.
By: Marcos Jose Setim (msetim) 2007-12-10 09:17:45.000-0600

WoW! IMHO, To pass the user and password using get parameters from URL is more insecure anyway.

I see two problems using the actual method:

1. Any network monitoring can detect the URL ( In basic HTTP too, so it's not to easy to see )
2. Anyone can connect to manager only typing a URL with user and pass ( In a call center context it's is very insecure )

Well, I'm not a network specialist however it's my two cents.


By: Yuri (ys) 2007-12-10 09:37:04.000-0600

oej.

I'am working on digest auth for http session. If digest HTTP auth in chan_sip moved to public (shared) asterisk lib - it's great!

About cookie auth - this method, except " *very* insecure", also liable to DoS attack, but all http request create new mansession. Basic auth method don't allocate any (session) data until the auth is complete.
By: Yuri (ys) 2007-12-17 07:30:43.000-0600

I upload new patch that implement HTTP Digest access authentication - RFC 2617 (and RFC2069 for backward compatibility)

In this patch I add 3 callback for following uri:

<prefix>/arawman - Raw HTTP manager interface w/Digest authentication
<prefix>/amanager - HTML manager interface w/Digest authentication
<prefix>/amxml - XML manager interface w/Digest authentication

Now it have some limitation:

Only GET method are supported (hardcoded).
Only one realm (global_realm) supported.
Only "auth" qop-value used (hardcoded).
Timer for nonce expiration are hardcoded (2 min).


By: Olle Johansson (oej) 2007-12-18 02:24:42.000-0600

Good start.

Now, we don't want duplicate code. We should move Digest auth header parsing to a common file from chan_sip and manager and only have one function for it.

In chan_sip, if there's no realm configuration I stick with the system name in asterisk.conf. If that's not found, the host name of the system.
By: Yuri (ys) 2007-12-20 08:57:08.000-0600

As start to share digest code, I upload trunk_utils.diff file, where I add digest parser function.
This function used for parsing incomming Digest request and responce header and also makes some checks.

Also,  in trunk_chan_sip.diff file I add some changes to chan_sip concerning digest authentication handing:

1. I change randdata  type in sip_pvt struct  to unsigned long. This field used only Asterisk is UAS, and we expect, that nonce value in Digest answer can be converted to unsigned long, otherwise this nonce is wrong.
2. "qop" now int flag, but now supported only "auth" qop value (or no qop).
3. If Asterisk (as UAS) sent Digest auth request (as defined in rfc3261 p22.4) it MUST be in format, defined in RFC2617, but responce from client
can be processed  in RFC-2617 format and (if client not support it) in RFC2069 format for backwards compatibility.
4. check_auth():  Now we can check auth both in RFC-2617 and RFC-2069 format; added nonce_count check for pedantic mode; "uri" MUST always exist in digest auth responce.
5. build_reply_digest function: remove "opaque" key/value field if Digest auth responce sent in  RFC2069 format.
By: Yuri (ys) 2008-01-23 08:07:08.000-0600

I upload new digest_auth_r99859_v2.diff file, where:

I sync version with current revision.
Share digest parse function (utils.c)
Share static function ftype2mtype() as ast_ftype2mtype(), and remove duplicate code (main/http.c and res/res_phoneprov.c)
Create SIP RFC-2617/RFC-2069 Digest authentication request and responce processing.
...


File affected:

channels/chan_sip.c
include/asterisk/utils.h
include/asterisk/http.h
main/utils.c
main/manager.c
main/http.c
res/res_phoneprov.c
By: Yuri (ys) 2008-03-15 12:27:18

I upload new digest_auth_r108895_v3.diff patch where:

Sync with rev. 108895.

Remove "Basic authentication scheme", now utilize only "digest authentication scheme".

Revert and hold changes for chan_sip.c

Added new generic ast_http_send() function, that send http responce to client.
Features:
1. Generate basic http responce headers.
2. All additional http responce header passed to this function in separate argument.
3. Can send content of responce from preallocated ast_str* string and/or from openned file.
4. Calculate value for "Content-Length" responce header and generate this header. (both for ast_str* string and data from opened file)
5. Prevent dublicate code, used for http header generation in some callback.
6. Added code for support "HEAD" http request method.

New ast_http_auth() and ast_http_error() helper function use ast_http_send() for sending authorize or error responce (4XX code).

Added code for processing "If-None-Match" request header and generation for the "Etag" responce header in static callback, and "304 Not Modified" responce, if file is unchanged.


Defination for http callback function are changed:
1. all http callback send data or error/auth responce to client without http helper assistance.
2. static_content in ast_http_uri struct in this case are unneded, callback can set this flag for ast_http_send() function itself.
By: Yuri (ys) 2008-05-26 07:49:15

digest_auth_r118255_v4.diff:

Sync with actual revision 118255.

Make xml_translate() more logical. (and prevent memory leak).
Move mansession to astobj2 container.

Decision about suproted method are moved to http callback, that can make it more adaptable in future.
Removed http "GET" varible processing limitation (from uri string) only for GET http method, but this is not violate the standart. Such as in PHP language, this variables comes to $_GET[] global array.
By: Terry Wilson (twilson) 2008-10-06 15:41:30

ys: I'm interested in moving this forward.  Is there a chance I could get you to update the patch for trunk again--then I will put it into a branch and turn on automerge to try to help us keep it up to date while we work on getting it in?  (Thanks for putting so much work into it!)
By: Yuri (ys) 2008-10-13 08:12:52

I upload new patch: digest_auth_r148468_v5.diff

Sync with actual revision 148468.
Fix some trivial bugs.

TODO: implementation of nonce cache.
By: Terry Wilson (twilson) 2008-10-13 12:43:56

I have added a branch at http://svn.digium.com/svn/asterisk/team/group/manager_http_auth and set up automerge, so it should be easier to keep it up to date now, anyway.  I'll try to take a more in-depth look at everything real soon.  Thanks for all of the work!
By: Terry Wilson (twilson) 2008-10-13 14:08:43

I went ahead and fixed a couple of things that were causing it to fail to compile when ./configure --enable-dev-mode is used (dev mode is quite handy btw for finding hard to debug issues like using variables w/o initialization, etc.).
By: Terry Wilson (twilson) 2008-10-13 15:36:19

What are your thoughts on not having a separate uri for http auth/cookie requests and doing the parsing based on either a setting (like you mention webauth=http_auth above which doesn't seem to be implemented), or maybe just taking auth header details and using them if they exist, otherwise falling back to cookie based?

There just seems to be a lot of code duplication between the auth_http_callback and generic_http_callback.  I haven't looked enough to see how big a task it would be, just curious as to your opinion, since you wrote it and are more familiar with the design.  :-)
By: Terry Wilson (twilson) 2008-12-09 10:14:20.000-0600

Ok, I have brought the branch back up to trunk and merged your latest patch into the branch.  If you could make any future patches against the http://svn.digium.com/svn/asterisk/team/group/manager_http_auth branch, it should make keeping this up to date easier.

In response to my last comment, I guess I can see how one might want to use both methods of auth at the same time, so I can see the usefulness of having separate uris.  I'm going to go ahead and go through the patch and see if I can come up with anything that it would need before merging, do some testing, etc.
By: Terry Wilson (twilson) 2008-12-09 10:35:08.000-0600

Also, for future reference, when making a patch for a new feature, please don't make a lot of changes to things like whitespace, adding braces, etc. to the change--it makes it much harder to go through the patch and see what changes the feature introduces.  All formatting changes should go in a separate patch.
By: Terry Wilson (twilson) 2008-12-09 16:13:54.000-0600

Also, I see no reason to remove Basic auth as we support HTTPS and Digest authentication provides no benefit to Basic auth over HTTPS.  If the code was already written, I don't see any reason not to include it.
By: Terry Wilson (twilson) 2008-12-10 10:12:19.000-0600

ys: just checking that you are still around and working on this
By: Yuri (ys) 2008-12-14 06:51:46.000-0600

otherwiseguy

Excuse for long absence and thank you for the branch.

But, I the second time see that automerge canceled, because of conflicts
with patches from a trunk.

As you can see, I remove "webauth=" parameters and create separate URI
for the HTTP Digest authentication for backward compatibility with existing
asterisk GUI (and GUI from foreign developers). Also, that so gives the chance
to use authentication failover based on URI path.

About Basic auth:
I remove Basic HTTP auth, so-as oej, in due time, has specified it as unsecured.
(At that point in time there was no support for https.)
Also, Basic HTTP auth, don't give me ability exactly compare web client and
mansession data, if several sessions with one account are used.
Digest HTTP auth provide "nonce" (and "opaque") value, that can be used as key
for sessions searching. But, when used IE, "opaque" can't be used
for these purposes, in connection with strange implementation of
Digest HTTP auth in IE.
By: Terry Wilson (twilson) 2008-12-15 12:54:59.000-0600

I fixed the automerge problem.  Sorry about that.  The automerge-email that I received accidentally matched a mail filter I had for something else and I didn't see it.
By: Digium Subversion (svnbot) 2009-04-23 15:36:36

Repository: asterisk
Revision: 190349

U   trunk/include/asterisk/http.h
U   trunk/include/asterisk/utils.h
U   trunk/main/astobj2.c
U   trunk/main/http.c
U   trunk/main/manager.c
U   trunk/main/utils.c
U   trunk/res/res_http_post.c
U   trunk/res/res_phoneprov.c

------------------------------------------------------------------------
r190349 | tilghman | 2009-04-23 15:36:36 -0500 (Thu, 23 Apr 2009) | 10 lines

Support HTTP digest authentication for the http manager interface.
(closes issue ASTERISK-10512)
Reported by: ys
Patches:
      digest_auth_r148468_v5.diff uploaded by ys (license 281)
      SVN branch http://svn.digium.com/svn/asterisk/team/group/manager_http_auth
Tested by: ys, twilson, tilghman
Review: http://reviewboard.digium.com/r/223/
Reviewed by: tilghman,russellb,mmichelson

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=190349
By: Digium Subversion (svnbot) 2009-04-23 15:37:17

Repository: asterisk
Revision: 190350

_U  branches/1.6.2/

------------------------------------------------------------------------
r190350 | tilghman | 2009-04-23 15:37:16 -0500 (Thu, 23 Apr 2009) | 16 lines

Blocked revisions 190349 via svnmerge

........
 r190349 | tilghman | 2009-04-23 15:36:35 -0500 (Thu, 23 Apr 2009) | 10 lines
 
 Support HTTP digest authentication for the http manager interface.
 (closes issue ASTERISK-10512)
  Reported by: ys
  Patches:
        digest_auth_r148468_v5.diff uploaded by ys (license 281)
        SVN branch http://svn.digium.com/svn/asterisk/team/group/manager_http_auth
  Tested by: ys, twilson, tilghman
  Review: http://reviewboard.digium.com/r/223/
  Reviewed by: tilghman,russellb,mmichelson
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=190350
By: Digium Subversion (svnbot) 2010-05-17 10:16:56

Repository: asterisk
Revision: 263460

U   trunk/main/manager.c

------------------------------------------------------------------------
r263460 | lmadsen | 2010-05-17 10:14:22 -0500 (Mon, 17 May 2010) | 7 lines

Missing newlines added to Set-Cookie line in manager.c

Sean Bright pointed out that we lost a set of newline characters in commit
190349 on a line I had recently changed. Yay for code review on commits.

(issue ASTERISK-16001, ASTERISK-10512)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=263460