Merge lp:~jimmy-sigint/mailman/restapi_auth into lp:mailman

[Jimmy Bergman]

In my opinion the REST API needs to be authenticated for the following reasons:

1. Even though it is by default exposed only on localhost, this means that all local users can administer mailing-lists instead of only some specific user like root.

2. It makes sense to use the REST API for integrating with external systems. These external systems will often be on other servers, causing the need for exposing the REST API on different interfaces than the loopback interface. For this authentication is a requirement.

The change in my branch solves this by adding a single shared username/password in the webservice section of the config using the parameters admin_user and admin_pass. The API is then changed to require HTTP basic auth using these credentials.

[Barry Warsaw (barry)]

Thanks for the branch Jimmy. I think you make some good points about enabling some form of authentication in the REST server. I've looked at:

http://ish.io/embedded/restish/guard.html

which provides examples of hooking up repoze.who. Did you think about that and if so, why did you choose not to use it (adding a dependency and the extra code complexity is a valid answer :).

Have you thought about using something like OAuth? Are you concerned at all about cleartext passwords?

Finally, while your patch looks basically decent (I'd quibble with some whitespace, but that's unimportant), please consider adding a test and/or some documentation (the latter perhaps as a doctest). And are you willing and able to assign your copyright to the FSF?

[Jimmy Bergman (jimmy-sigint)]

Hi

Well, adding a dependency and the extra code complexity was mainly the reason - yes.

I think the use case for repoze.who is more "I need good way to use an existing authentication store in an WSGI application".

In the mailman REST server case I didn't think that there existed an existing user store to plug into (at least with the same meaning that I was aiming for, i.e. global access to the installation like in the current REST server).

So while I could of course use repoze.who, I don't think it has that much merit for this simple case (one credential for the complete app).

OAuth for this feels overkill as well to me.

If the goal was more granular auth in the REST server (perhaps ability to use authenticate as list owner/subscriber/site admin and become authorized only for the related things), then I would have gone for repoze.who and perhaps OAuth.

But I figured that your choice to not have any authentication at all reflected a wish to KISS, and that wish is something which I very much agree with; hence the single user/pass shared secret.

Regarding clear text passwords, sure - but I think that a simple solution for this is to use SSL for the transport - which I think is supported with the use_https property.

If you're ok with the code (and by all means you can suggest indentation-changes, I like real tabs myself - but tried to use the project style, perhaps unsuccessfully) then I'll go ahead and commit test/doc to the branch.

Regarding the copyright, sure - this is no problem, just point me in the right direction.

[Barry Warsaw (barry)]

I agree with your rationale, and I do think that basic auth is better than nothing. ;) I haven't tried it, but your code looks essentially correct. Please do extend your branch with tests and docs. Contact me at barry at list dot org and I'll get the FSF assignment process started.

Thanks for you contribution to Mailman!

[lp:~jimmy-sigint/mailman/restapi_auth]

6956. By root <root@s97> on 2010-09-29

Add doctest for connecting using invalid credentials and some documentation regarding the basic auth for the REST server.

[Barry Warsaw (barry)]

Thanks for the updates Jimmy, this looks pretty good and I will merge it into trunk, with some modifications. Mostly, there are a few style issues, but nothing major. Things to take note:

* import sorting order as per STYLEGUIDE.txt
* preferred use of single quotes rather than double quotes
* vertical whitespace as per STYLEGUIDE.txt
* lines too long
* in restish errors, I prefer to use b'foo' instead of str('foo') to indicate error messages
* i changed some of the logic in root.py so the key access from request.environ only happens once
* you don't need to use ...None, None... in the test for bad auth; just use keyword arguments
* in dump_json(), instead of defaulting the keywords to strings, use None and grab the defaults from the config
* new style string interpolation instead of %s's
* add new parameters to docstring for call_http() and dump_json()
* b64encode() instead of encodestring()

review: Approve

=== modified file 'src/mailman/config/schema.cfg'
--- src/mailman/config/schema.cfg	2010-03-28 22:20:10 +0000
+++ src/mailman/config/schema.cfg	2010-09-29 08:41:51 +0000
@@ -298,6 +298,11 @@
 # The API version number for the current API.
 api_version: 3.0
 
+# The administrative username.
+admin_user: restadmin
+
+# The administrative password.
+admin_pass: restpass
 
 [language.master]
 # Template for language definitions.  The section name must be [language.xx]
 
=== modified file 'src/mailman/rest/docs/basic.txt'
--- src/mailman/rest/docs/basic.txt	2010-09-15 15:37:54 +0000
+++ src/mailman/rest/docs/basic.txt	2010-09-29 08:41:51 +0000
@@ -2,12 +2,20 @@
 REST server
 ===========
 
-Mailman exposes a REST_ HTTP server for administrative control.
+Mailman exposes a REST HTTP server for administrative control.
 
 The server listens for connections on a configurable host name and port.
+
+It is always protected by HTTP basic authentication using a single global
+username and password. The credentials are set in the webservice section
+of the config using the admin_user and admin_pass properties.
+
 Because the REST server has full administrative access, it should always be
-run only on localhost, unless you really know what you're doing.  The Mailman
-major and minor version numbers are in the URL.
+run only on localhost, unless you really know what you're doing. In addition
+you should set the username and password to secure values and distribute
+them to any REST clients with reasonable precautions.
+
+The Mailman major and minor version numbers are in the URL.
 
 System information can be retrieved from the server.  By default JSON is
 returned.
@@ -30,5 +38,16 @@
     ...
     HTTPError: HTTP Error 404: 404 Not Found
 
+Invalid credentials
+===================
+
+When you try to access the REST server using invalid credentials you will get
+an appropriate HTTP 401 Unauthorized error.
+
+    >>> dump_json('http://localhost:8001/3.0/system', None, None, 'baduser', 'badpass')
+    Traceback (most recent call last):
+    ...
+    HTTPError: HTTP Error 401: 401 Unauthorized
+    ...
 
 .. _REST: http://en.wikipedia.org/wiki/REST
 
=== modified file 'src/mailman/rest/root.py'
--- src/mailman/rest/root.py	2010-02-26 01:38:25 +0000
+++ src/mailman/rest/root.py	2010-09-29 08:41:51 +0000
@@ -25,7 +25,8 @@
     ]
 
 
-from restish import http, resource
+from restish import http, resource, guard
+from base64 import b64decode
 
 from mailman.config import config
 from mailman.core.system import system
@@ -36,6 +37,16 @@
 
 
 
 
+def webservice_auth_checker(request, obj):
+    if "HTTP_AUTHORIZATION" in request.environ and request.environ["HTTP_AUTHORIZATION"].startswith("Basic "):
+        credentials = b64decode(request.environ["HTTP_AUTHORIZATION"][6:])
+        username, password = credentials.split(":", 1)
+
+        if username != config.webservice.admin_user or password != config.webservice.admin_pass:
+            raise guard.GuardError(str("User is not authorized for the REST api."))
+    else:
+        raise guard.GuardError(str("The REST api requires authentication."))
+
 class Root(resource.Resource):
     """The RESTful root resource.
 
@@ -44,11 +55,12 @@
     and we start at 3.0 to match the Mailman version number.  That may not
     always be the case though.
     """
+
     @resource.child(config.webservice.api_version)
+    @guard.guard(webservice_auth_checker)
     def api_version(self, request, segments):
         return TopLevel()
 
-
 class TopLevel(resource.Resource):
     """Top level collections and entries."""
 
 
=== modified file 'src/mailman/testing/layers.py'
--- src/mailman/testing/layers.py	2010-08-30 15:03:00 +0000
+++ src/mailman/testing/layers.py	2010-09-29 08:41:51 +0000
@@ -37,7 +37,8 @@
 
 from pkg_resources import resource_string
 from textwrap import dedent
-from urllib2 import urlopen, URLError
+from urllib2 import urlopen, URLError, Request
+from base64 import encodestring
 from zope.component import getUtility
 
 from mailman.config import config
@@ -273,7 +274,10 @@
         until = datetime.datetime.now() + TEST_TIMEOUT
         while datetime.datetime.now() < until:
             try:
-                fp = urlopen('http://localhost:8001/3.0/system')
+                request = Request('http://localhost:8001/3.0/system')
+                base64string = encodestring('%s:%s' % ("restadmin", "restpass")).replace('\n', '')
+                request.add_header("Authorization", "Basic %s" % base64string)   
+                fp = urlopen(request)
             except URLError:
                 pass
             else:
 
=== modified file 'src/mailman/tests/test_documentation.py'
--- src/mailman/tests/test_documentation.py	2010-08-24 01:37:20 +0000
+++ src/mailman/tests/test_documentation.py	2010-09-29 08:41:51 +0000
@@ -39,6 +39,7 @@
 from httplib2 import Http
 from urllib import urlencode
 from urllib2 import HTTPError
+from base64 import encodestring
 
 import mailman
 
@@ -109,7 +110,7 @@
         print '{0:{2}}: {1}'.format(key, msgdata[key], longest)
 
 
-def call_http(url, data=None, method=None):
+def call_http(url, data=None, method=None, username="restadmin", password="restpass"):
     """'Call' a URL with a given HTTP method and return the resulting object.
 
     The object will have been JSON decoded.
@@ -131,6 +132,7 @@
         else:
             method = 'POST'
     method = method.upper()
+    headers['Authorization'] = 'Basic %s' % encodestring('%s:%s' % (username, password)).replace('\n', '')
     response, content = Http().request(url, method, data, headers)
     # If we did not get a 2xx status code, make this look like a urllib2
     # exception, for backward compatibility with existing doctests.
@@ -143,7 +145,7 @@
     return json.loads(content)
 
 
-def dump_json(url, data=None, method=None):
+def dump_json(url, data=None, method=None, username="restadmin", password="restpass"):
     """Print the JSON dictionary read from a URL.
 
     :param url: The url to open, read, and print.
@@ -153,7 +155,7 @@
     :param method: Alternative HTTP method to use.
     :type method: str
     """
-    data = call_http(url, data, method)
+    data = call_http(url, data, method, username, password)
     if data is None:
         return
     for key in sorted(data):