Merge lp:~blamar/nova/lp732866 into lp:~hudson-openstack/nova/trunk

[Brian Lamar]

Fixed lp732866 by catching relevant `exception.NotFound` exception. Tests did not uncover this vulnerability due to "incorrect" FakeAuthManager. I say "incorrect" because potentially different implementations (LDAP or Database driven) of AuthManager might return different errors from `get_user_from_access_key`.

Also, removed all references to 'bacon', 'ham', 'herp', and 'derp' and replaced them with hopefully more helpful terms.

Long story short it addresses the immediate issue while throughly ignoring the larger issue, which is correctly testing all implementations of Auth. I find this acceptable as currently the future of auth is in flux.

[lp:~blamar/nova/lp732866]

792. By Brian Lamar on 2011-03-11

Removed EOL whitespace in accordance with PEP-8.

[Brian Waldon (bcwaldon)]

This seems like a good fix to the issue at hand. It's probably a great idea to at least *seem* professional with regards to test strings, too.

review: Approve

[justinsb (justin-fathomdb)]

LGTM.

I also tried cleaning up some of the FakeAuthManager issues, and currently I have a test that is neutered until this goes through. Happy to fix up both once this goes in - I'd prefer to see this go in first actually.

review: Approve

[Jay Pipes (jaypipes)]

Excellent work, Brian! :)

review: Approve

[Rick Harris (rconradharris)]

Great job, Brian.

review: Approve

=== modified file 'nova/api/openstack/auth.py'
--- nova/api/openstack/auth.py	2011-03-11 19:49:32 +0000
+++ nova/api/openstack/auth.py	2011-03-11 22:44:32 +0000
@@ -135,7 +135,11 @@
         req - wsgi.Request object
         """
         ctxt = context.get_admin_context()
-        user = self.auth.get_user_from_access_key(key)
+
+        try:
+            user = self.auth.get_user_from_access_key(key)
+        except exception.NotFound:
+            user = None
 
         if user and user.name == username:
             token_hash = hashlib.sha1('%s%s%f' % (username, key,

=== modified file 'nova/tests/api/openstack/fakes.py'
--- nova/tests/api/openstack/fakes.py	2011-03-11 19:49:32 +0000
+++ nova/tests/api/openstack/fakes.py	2011-03-11 22:44:32 +0000
@@ -321,7 +321,10 @@
                        (user.id == p.project_manager_id)]
 
     def get_user_from_access_key(self, key):
-        return FakeAuthManager.auth_data.get(key, None)
+        try:
+            return FakeAuthManager.auth_data[key]
+        except KeyError:
+            raise exc.NotFound
 
 
 class FakeRateLimiter(object):

=== modified file 'nova/tests/api/openstack/test_auth.py'
--- nova/tests/api/openstack/test_auth.py	2011-03-10 22:33:45 +0000
+++ nova/tests/api/openstack/test_auth.py	2011-03-11 22:44:32 +0000
@@ -51,11 +51,12 @@
 
     def test_authorize_user(self):
         f = fakes.FakeAuthManager()
-        f.add_user('derp', nova.auth.manager.User(1, 'herp', None, None, None))
+        f.add_user('user1_key',
+            nova.auth.manager.User(1, 'user1', None, None, None))
 
         req = webob.Request.blank('/v1.0/')
-        req.headers['X-Auth-User'] = 'herp'
-        req.headers['X-Auth-Key'] = 'derp'
+        req.headers['X-Auth-User'] = 'user1'
+        req.headers['X-Auth-Key'] = 'user1_key'
         result = req.get_response(fakes.wsgi_app())
         self.assertEqual(result.status, '204 No Content')
         self.assertEqual(len(result.headers['X-Auth-Token']), 40)
@@ -65,13 +66,13 @@
 
     def test_authorize_token(self):
         f = fakes.FakeAuthManager()
-        u = nova.auth.manager.User(1, 'herp', None, None, None)
-        f.add_user('derp', u)
-        f.create_project('test', u)
+        u = nova.auth.manager.User(1, 'user1', None, None, None)
+        f.add_user('user1_key', u)
+        f.create_project('user1_project', u)
 
         req = webob.Request.blank('/v1.0/', {'HTTP_HOST': 'foo'})
-        req.headers['X-Auth-User'] = 'herp'
-        req.headers['X-Auth-Key'] = 'derp'
+        req.headers['X-Auth-User'] = 'user1'
+        req.headers['X-Auth-Key'] = 'user1_key'
         result = req.get_response(fakes.wsgi_app())
         self.assertEqual(result.status, '204 No Content')
         self.assertEqual(len(result.headers['X-Auth-Token']), 40)
@@ -92,7 +93,7 @@
 
     def test_token_expiry(self):
         self.destroy_called = False
-        token_hash = 'bacon'
+        token_hash = 'token_hash'
 
         def destroy_token_mock(meh, context, token):
             self.destroy_called = True
@@ -109,15 +110,26 @@
             bad_token)
 
         req = webob.Request.blank('/v1.0/')
-        req.headers['X-Auth-Token'] = 'bacon'
+        req.headers['X-Auth-Token'] = 'token_hash'
         result = req.get_response(fakes.wsgi_app())
         self.assertEqual(result.status, '401 Unauthorized')
         self.assertEqual(self.destroy_called, True)
 
-    def test_bad_user(self):
-        req = webob.Request.blank('/v1.0/')
-        req.headers['X-Auth-User'] = 'herp'
-        req.headers['X-Auth-Key'] = 'derp'
+    def test_bad_user_bad_key(self):
+        req = webob.Request.blank('/v1.0/')
+        req.headers['X-Auth-User'] = 'unknown_user'
+        req.headers['X-Auth-Key'] = 'unknown_user_key'
+        result = req.get_response(fakes.wsgi_app())
+        self.assertEqual(result.status, '401 Unauthorized')
+
+    def test_bad_user_good_key(self):
+        f = fakes.FakeAuthManager()
+        u = nova.auth.manager.User(1, 'user1', None, None, None)
+        f.add_user('user1_key', u)
+
+        req = webob.Request.blank('/v1.0/')
+        req.headers['X-Auth-User'] = 'unknown_user'
+        req.headers['X-Auth-Key'] = 'user1_key'
         result = req.get_response(fakes.wsgi_app())
         self.assertEqual(result.status, '401 Unauthorized')
 
@@ -128,7 +140,7 @@
 
     def test_bad_token(self):
         req = webob.Request.blank('/v1.0/')
-        req.headers['X-Auth-Token'] = 'baconbaconbacon'
+        req.headers['X-Auth-Token'] = 'unknown_token'
         result = req.get_response(fakes.wsgi_app())
         self.assertEqual(result.status, '401 Unauthorized')
 
@@ -137,11 +149,11 @@
     def test_token_expiry(self):
         ctx = context.get_admin_context()
         tok = db.auth_token_create(ctx, dict(
-                token_hash='bacon',
+                token_hash='test_token_hash',
                 cdn_management_url='',
                 server_management_url='',
                 storage_url='',
-                user_id='ham',
+                user_id='user1',
                 ))
 
         db.auth_token_update(ctx, tok.token_hash, dict(
@@ -149,13 +161,13 @@
                 ))
 
         req = webob.Request.blank('/v1.0/')
-        req.headers['X-Auth-Token'] = 'bacon'
+        req.headers['X-Auth-Token'] = 'test_token_hash'
         result = req.get_response(fakes.wsgi_app())
         self.assertEqual(result.status, '401 Unauthorized')
 
     def test_token_doesnotexist(self):
         req = webob.Request.blank('/v1.0/')
-        req.headers['X-Auth-Token'] = 'ham'
+        req.headers['X-Auth-Token'] = 'nonexistant_token_hash'
         result = req.get_response(fakes.wsgi_app())
         self.assertEqual(result.status, '401 Unauthorized')
 
@@ -178,13 +190,13 @@
 
     def test_authorize_token(self):
         f = fakes.FakeAuthManager()
-        u = nova.auth.manager.User(1, 'herp', None, None, None)
-        f.add_user('derp', u)
+        u = nova.auth.manager.User(1, 'user1', None, None, None)
+        f.add_user('user1_key', u)
         f.create_project('test', u)
 
         req = webob.Request.blank('/v1.0/')
-        req.headers['X-Auth-User'] = 'herp'
-        req.headers['X-Auth-Key'] = 'derp'
+        req.headers['X-Auth-User'] = 'user1'
+        req.headers['X-Auth-Key'] = 'user1_key'
         result = req.get_response(fakes.wsgi_app())
         self.assertEqual(len(result.headers['X-Auth-Token']), 40)