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

Proposed by Jimmy Bergman
Status: Merged
Approved by: Barry Warsaw
Approved revision: 6956
Merged at revision: 6957
Proposed branch: lp:~jimmy-sigint/mailman/restapi_auth
Merge into: lp:mailman
Diff against target: 178 lines (+52/-10)
5 files modified
src/mailman/config/schema.cfg (+5/-0)
src/mailman/rest/docs/basic.txt (+22/-3)
src/mailman/rest/root.py (+14/-2)
src/mailman/testing/layers.py (+6/-2)
src/mailman/tests/test_documentation.py (+5/-3)
To merge this branch: bzr merge lp:~jimmy-sigint/mailman/restapi_auth
Reviewer Review Type Date Requested Status
Barry Warsaw Approve
Review via email: mp+36833@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Barry Warsaw (barry) wrote :

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?

Revision history for this message
Jimmy Bergman (jimmy-sigint) wrote :

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.

Revision history for this message
Barry Warsaw (barry) wrote :

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!

6956. By root <root@s97>

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

Revision history for this message
Barry Warsaw (barry) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/mailman/config/schema.cfg'
2--- src/mailman/config/schema.cfg 2010-03-28 22:20:10 +0000
3+++ src/mailman/config/schema.cfg 2010-09-29 08:41:51 +0000
4@@ -298,6 +298,11 @@
5 # The API version number for the current API.
6 api_version: 3.0
7
8+# The administrative username.
9+admin_user: restadmin
10+
11+# The administrative password.
12+admin_pass: restpass
13
14 [language.master]
15 # Template for language definitions. The section name must be [language.xx]
16
17=== modified file 'src/mailman/rest/docs/basic.txt'
18--- src/mailman/rest/docs/basic.txt 2010-09-15 15:37:54 +0000
19+++ src/mailman/rest/docs/basic.txt 2010-09-29 08:41:51 +0000
20@@ -2,12 +2,20 @@
21 REST server
22 ===========
23
24-Mailman exposes a REST_ HTTP server for administrative control.
25+Mailman exposes a REST HTTP server for administrative control.
26
27 The server listens for connections on a configurable host name and port.
28+
29+It is always protected by HTTP basic authentication using a single global
30+username and password. The credentials are set in the webservice section
31+of the config using the admin_user and admin_pass properties.
32+
33 Because the REST server has full administrative access, it should always be
34-run only on localhost, unless you really know what you're doing. The Mailman
35-major and minor version numbers are in the URL.
36+run only on localhost, unless you really know what you're doing. In addition
37+you should set the username and password to secure values and distribute
38+them to any REST clients with reasonable precautions.
39+
40+The Mailman major and minor version numbers are in the URL.
41
42 System information can be retrieved from the server. By default JSON is
43 returned.
44@@ -30,5 +38,16 @@
45 ...
46 HTTPError: HTTP Error 404: 404 Not Found
47
48+Invalid credentials
49+===================
50+
51+When you try to access the REST server using invalid credentials you will get
52+an appropriate HTTP 401 Unauthorized error.
53+
54+ >>> dump_json('http://localhost:8001/3.0/system', None, None, 'baduser', 'badpass')
55+ Traceback (most recent call last):
56+ ...
57+ HTTPError: HTTP Error 401: 401 Unauthorized
58+ ...
59
60 .. _REST: http://en.wikipedia.org/wiki/REST
61
62=== modified file 'src/mailman/rest/root.py'
63--- src/mailman/rest/root.py 2010-02-26 01:38:25 +0000
64+++ src/mailman/rest/root.py 2010-09-29 08:41:51 +0000
65@@ -25,7 +25,8 @@
66 ]
67
68
69-from restish import http, resource
70+from restish import http, resource, guard
71+from base64 import b64decode
72
73 from mailman.config import config
74 from mailman.core.system import system
75@@ -36,6 +37,16 @@
76
77
78
79
80+def webservice_auth_checker(request, obj):
81+ if "HTTP_AUTHORIZATION" in request.environ and request.environ["HTTP_AUTHORIZATION"].startswith("Basic "):
82+ credentials = b64decode(request.environ["HTTP_AUTHORIZATION"][6:])
83+ username, password = credentials.split(":", 1)
84+
85+ if username != config.webservice.admin_user or password != config.webservice.admin_pass:
86+ raise guard.GuardError(str("User is not authorized for the REST api."))
87+ else:
88+ raise guard.GuardError(str("The REST api requires authentication."))
89+
90 class Root(resource.Resource):
91 """The RESTful root resource.
92
93@@ -44,11 +55,12 @@
94 and we start at 3.0 to match the Mailman version number. That may not
95 always be the case though.
96 """
97+
98 @resource.child(config.webservice.api_version)
99+ @guard.guard(webservice_auth_checker)
100 def api_version(self, request, segments):
101 return TopLevel()
102
103-
104 class TopLevel(resource.Resource):
105 """Top level collections and entries."""
106
107
108=== modified file 'src/mailman/testing/layers.py'
109--- src/mailman/testing/layers.py 2010-08-30 15:03:00 +0000
110+++ src/mailman/testing/layers.py 2010-09-29 08:41:51 +0000
111@@ -37,7 +37,8 @@
112
113 from pkg_resources import resource_string
114 from textwrap import dedent
115-from urllib2 import urlopen, URLError
116+from urllib2 import urlopen, URLError, Request
117+from base64 import encodestring
118 from zope.component import getUtility
119
120 from mailman.config import config
121@@ -273,7 +274,10 @@
122 until = datetime.datetime.now() + TEST_TIMEOUT
123 while datetime.datetime.now() < until:
124 try:
125- fp = urlopen('http://localhost:8001/3.0/system')
126+ request = Request('http://localhost:8001/3.0/system')
127+ base64string = encodestring('%s:%s' % ("restadmin", "restpass")).replace('\n', '')
128+ request.add_header("Authorization", "Basic %s" % base64string)
129+ fp = urlopen(request)
130 except URLError:
131 pass
132 else:
133
134=== modified file 'src/mailman/tests/test_documentation.py'
135--- src/mailman/tests/test_documentation.py 2010-08-24 01:37:20 +0000
136+++ src/mailman/tests/test_documentation.py 2010-09-29 08:41:51 +0000
137@@ -39,6 +39,7 @@
138 from httplib2 import Http
139 from urllib import urlencode
140 from urllib2 import HTTPError
141+from base64 import encodestring
142
143 import mailman
144
145@@ -109,7 +110,7 @@
146 print '{0:{2}}: {1}'.format(key, msgdata[key], longest)
147
148
149-def call_http(url, data=None, method=None):
150+def call_http(url, data=None, method=None, username="restadmin", password="restpass"):
151 """'Call' a URL with a given HTTP method and return the resulting object.
152
153 The object will have been JSON decoded.
154@@ -131,6 +132,7 @@
155 else:
156 method = 'POST'
157 method = method.upper()
158+ headers['Authorization'] = 'Basic %s' % encodestring('%s:%s' % (username, password)).replace('\n', '')
159 response, content = Http().request(url, method, data, headers)
160 # If we did not get a 2xx status code, make this look like a urllib2
161 # exception, for backward compatibility with existing doctests.
162@@ -143,7 +145,7 @@
163 return json.loads(content)
164
165
166-def dump_json(url, data=None, method=None):
167+def dump_json(url, data=None, method=None, username="restadmin", password="restpass"):
168 """Print the JSON dictionary read from a URL.
169
170 :param url: The url to open, read, and print.
171@@ -153,7 +155,7 @@
172 :param method: Alternative HTTP method to use.
173 :type method: str
174 """
175- data = call_http(url, data, method)
176+ data = call_http(url, data, method, username, password)
177 if data is None:
178 return
179 for key in sorted(data):