From 2080f7dbd897d6130542dbf88e37641a41625eb5 Mon Sep 17 00:00:00 2001 From: Alistair Coles Date: Thu, 26 Feb 2015 15:16:22 +0000 Subject: [PATCH] Fix tempauth acl checks when simplejson has no speedups As documented in linked bug report, tempauth unit tests were seen to fail on a system where simplejson was installed but without the speedups extension. This is because the tempauth account acl validation checks that values are type str, but without the speedups extension the json parser is returning unicode objects. Fix is to have the acl validator tolerate those objects being unicode or str. Also change common/bufferedhttp.py to coerce ring device to type str when constructing a path, in order to avoid a UnicodeDecodeError when httplib sends a message that has non-ascii header values. Change-Id: I01524282cbaa25dc4b6dfa09f3f4723516cdba99 Closes-Bug: 1425776 --- swift/common/bufferedhttp.py | 5 ++ swift/common/middleware/tempauth.py | 8 +-- test/unit/common/middleware/test_tempauth.py | 30 +++++++--- test/unit/common/test_bufferedhttp.py | 60 ++++++++++++++------ 4 files changed, 73 insertions(+), 30 deletions(-) diff --git a/swift/common/bufferedhttp.py b/swift/common/bufferedhttp.py index d4a977c21e..2b3ec1609d 100644 --- a/swift/common/bufferedhttp.py +++ b/swift/common/bufferedhttp.py @@ -155,6 +155,11 @@ def http_connect(ipaddr, port, device, partition, method, path, path = path.encode("utf-8") except UnicodeError as e: logging.exception(_('Error encoding to UTF-8: %s'), str(e)) + if isinstance(device, unicode): + try: + device = device.encode("utf-8") + except UnicodeError as e: + logging.exception(_('Error encoding to UTF-8: %s'), str(e)) path = quote('/' + device + '/' + str(partition) + path) return http_connect_raw( ipaddr, port, method, path, headers, query_string, ssl) diff --git a/swift/common/middleware/tempauth.py b/swift/common/middleware/tempauth.py index 93f55ff031..dfde519f42 100644 --- a/swift/common/middleware/tempauth.py +++ b/swift/common/middleware/tempauth.py @@ -447,16 +447,16 @@ class TempAuth(object): # on ACLs, TempAuth is not such an auth system. At this point, # it thinks it is authoritative. if key not in tempauth_acl_keys: - return 'Key %r not recognized' % key + return "Key '%s' not recognized" % key for key in tempauth_acl_keys: if key not in result: continue if not isinstance(result[key], list): - return 'Value for key %r must be a list' % key + return "Value for key '%s' must be a list" % key for grantee in result[key]: - if not isinstance(grantee, str): - return 'Elements of %r list must be strings' % key + if not isinstance(grantee, basestring): + return "Elements of '%s' list must be strings" % key # Everything looks fine, no errors found internal_hdr = get_sys_meta_prefix('account') + 'core-access-control' diff --git a/test/unit/common/middleware/test_tempauth.py b/test/unit/common/middleware/test_tempauth.py index b9be84bb92..e8af310c82 100644 --- a/test/unit/common/middleware/test_tempauth.py +++ b/test/unit/common/middleware/test_tempauth.py @@ -1,3 +1,4 @@ +# -*- coding: utf-8 -*- # Copyright (c) 2011-2015 OpenStack Foundation # # Licensed under the Apache License, Version 2.0 (the "License"); @@ -13,6 +14,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import json import unittest from contextlib import contextmanager, nested from base64 import b64encode @@ -22,7 +24,7 @@ import mock from swift.common.middleware import tempauth as auth from swift.common.middleware.acl import format_acl from swift.common.swob import Request, Response -from swift.common.utils import split_path, get_swift_info +from swift.common.utils import split_path NO_CONTENT_RESP = (('204 No Content', {}, ''),) # mock server response @@ -111,10 +113,6 @@ class TestAuth(unittest.TestCase): def setUp(self): self.test_auth = auth.filter_factory({})(FakeApp()) - def test_swift_info(self): - info = get_swift_info() - self.assertTrue(info['tempauth']['account_acls']) - def _make_request(self, path, **kwargs): req = Request.blank(path, **kwargs) req.environ['swift.cache'] = FakeMemcache() @@ -1200,7 +1198,8 @@ class TestAccountAcls(unittest.TestCase): user_groups = test_auth._get_user_groups('admin', 'admin:user', 'AUTH_admin') good_headers = {'X-Auth-Token': 'AUTH_t'} - good_acl = '{"read-only":["a","b"]}' + good_acl = json.dumps({"read-only": [u"á", "b"]}) + bad_list_types = '{"read-only": ["a", 99]}' bad_acl = 'syntactically invalid acl -- this does not parse as JSON' wrong_acl = '{"other-auth-system":["valid","json","but","wrong"]}' bad_value_acl = '{"read-write":["fine"],"admin":"should be a list"}' @@ -1220,7 +1219,9 @@ class TestAccountAcls(unittest.TestCase): req = self._make_request(target, user_groups=user_groups, headers=dict(good_headers, **update)) resp = req.get_response(test_auth) - self.assertEquals(resp.status_int, 204) + self.assertEquals(resp.status_int, 204, + 'Expected 204, got %s, response body: %s' + % (resp.status_int, resp.body)) # syntactically valid empty acls should go through for acl in empty_acls: @@ -1243,14 +1244,25 @@ class TestAccountAcls(unittest.TestCase): req = self._make_request(target, headers=dict(good_headers, **update)) resp = req.get_response(test_auth) self.assertEquals(resp.status_int, 400) - self.assertEquals(errmsg % "Key '", resp.body[:39]) + self.assertTrue(resp.body.startswith( + errmsg % "Key 'other-auth-system' not recognized"), resp.body) # acls with good keys but bad values also get a 400 update = {'x-account-access-control': bad_value_acl} req = self._make_request(target, headers=dict(good_headers, **update)) resp = req.get_response(test_auth) self.assertEquals(resp.status_int, 400) - self.assertEquals(errmsg % "Value", resp.body[:39]) + self.assertTrue(resp.body.startswith( + errmsg % "Value for key 'admin' must be a list"), resp.body) + + # acls with non-string-types in list also get a 400 + update = {'x-account-access-control': bad_list_types} + req = self._make_request(target, headers=dict(good_headers, **update)) + resp = req.get_response(test_auth) + self.assertEquals(resp.status_int, 400) + self.assertTrue(resp.body.startswith( + errmsg % "Elements of 'read-only' list must be strings"), + resp.body) # acls with wrong json structure also get a 400 update = {'x-account-access-control': not_dict_acl} diff --git a/test/unit/common/test_bufferedhttp.py b/test/unit/common/test_bufferedhttp.py index a663a3d121..6e51973147 100644 --- a/test/unit/common/test_bufferedhttp.py +++ b/test/unit/common/test_bufferedhttp.py @@ -1,4 +1,5 @@ -# Copyright (c) 2010-2012 OpenStack Foundation +# -*- coding: utf-8 -*- +# Copyright (c) 2010-2012 OpenStack Foundation # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -12,6 +13,7 @@ # implied. # See the License for the specific language governing permissions and # limitations under the License. +import mock import unittest @@ -22,6 +24,24 @@ from eventlet import spawn, Timeout, listen from swift.common import bufferedhttp +class MockHTTPSConnection(object): + + def __init__(self, hostport): + pass + + def putrequest(self, method, path, skip_host=0): + self.path = path + pass + + def putheader(self, header, *values): + # Verify that path and values can be safely joined + # Essentially what Python 2.7 does that caused us problems. + '\r\n\t'.join((self.path,) + values) + + def endheaders(self): + pass + + class TestBufferedHTTP(unittest.TestCase): def test_http_connect(self): @@ -76,22 +96,6 @@ class TestBufferedHTTP(unittest.TestCase): raise Exception(err) def test_nonstr_header_values(self): - - class MockHTTPSConnection(object): - - def __init__(self, hostport): - pass - - def putrequest(self, method, path, skip_host=0): - pass - - def putheader(self, header, *values): - # Essentially what Python 2.7 does that caused us problems. - '\r\n\t'.join(values) - - def endheaders(self): - pass - origHTTPSConnection = bufferedhttp.HTTPSConnection bufferedhttp.HTTPSConnection = MockHTTPSConnection try: @@ -106,6 +110,28 @@ class TestBufferedHTTP(unittest.TestCase): finally: bufferedhttp.HTTPSConnection = origHTTPSConnection + def test_unicode_values(self): + # simplejson may decode the ring devices as str or unicode + # depending on whether speedups is installed and/or the values are + # non-ascii. Verify all types are tolerated in combination with + # whatever type path might be and possible encoded non-ascii in + # a header value. + with mock.patch('swift.common.bufferedhttp.HTTPSConnection', + MockHTTPSConnection): + for dev in ('sda', u'sda', u'sdá', u'sdá'.encode('utf-8')): + for path in ( + '/v1/a', u'/v1/a', u'/v1/á', u'/v1/á'.encode('utf-8')): + for header in ('abc', u'abc', u'ábc'.encode('utf-8')): + try: + bufferedhttp.http_connect( + '127.0.0.1', 8080, dev, 1, 'GET', path, + headers={'X-Container-Meta-Whatever': header}, + ssl=True) + except Exception as e: + self.fail( + 'Exception %r for device=%r path=%r header=%r' + % (e, dev, path, header)) + if __name__ == '__main__': unittest.main()