From f4c8b38cea5b198727cab4ecc98031468924e30b Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Tue, 22 Mar 2016 12:23:32 -0700 Subject: [PATCH] Stop using client headers for cross-middleware communication Previously, Swift3 used client-facing HTTP headers to pass the S3 access key, signature, and normalized request through the WSGI pipeline. However, swauth did not validate that Swift3 actually set the headers; as a result, an attacker who has captured a single valid request through the S3 API may impersonate the user that issued the request indefinitely through the Swift API. Now, the S3 authentication information will be taken from a separate, client-inaccessible namespace in the WSGI environment as defined in the related change. UpgradeImpact This addresses a breaking API change in Swift3. No currently deployed version of Swift3 will work with this. When upgrading swauth, operators will need to upgrade Swift3 as well. Related-Change: Ia3fbb4938f0daa8845cba4137a01cc43bc1a713c --- swauth/middleware.py | 22 ++++++++++++---------- test/unit/test_middleware.py | 41 +++++++++++++++++++++++++++++------------ 2 files changed, 41 insertions(+), 22 deletions(-) diff --git a/swauth/middleware.py b/swauth/middleware.py index 7955b8e..0d10864 100644 --- a/swauth/middleware.py +++ b/swauth/middleware.py @@ -231,7 +231,7 @@ class Swauth(object): start_response) elif env.get('PATH_INFO', '').startswith(self.auth_prefix): return self.handle(env, start_response) - s3 = env.get('HTTP_AUTHORIZATION') + s3 = env.get('swift3.auth_details') token = env.get('HTTP_X_AUTH_TOKEN', env.get('HTTP_X_STORAGE_TOKEN')) if token and len(token) > swauth.authtypes.MAX_TOKEN_LENGTH: return HTTPBadRequest(body='Token exceeds maximum length.')(env, @@ -309,7 +309,8 @@ class Swauth(object): if expires < time(): groups = None - if env.get('HTTP_AUTHORIZATION'): + s3_auth_details = env.get('swift3.auth_details') + if s3_auth_details: if self.swauth_remote: # TODO(gholt): Support S3-style authorization with # swauth_remote mode @@ -317,12 +318,14 @@ class Swauth(object): 'with swauth_remote mode.') return None try: - account = env['HTTP_AUTHORIZATION'].split(' ')[1] - account, user, sign = account.split(':') + account_user = s3_auth_details['access_key'] + signature_from_user = s3_auth_details['signature'] + msg = s3_auth_details['string_to_sign'] + account, user = account_user.split(':') except Exception: self.logger.debug( - 'Swauth cannot parse Authorization header value %r' % - env['HTTP_AUTHORIZATION']) + 'Swauth cannot parse swift3.auth_details value %r' % + (s3_auth_details, )) return None path = quote('/v1/%s/%s/%s' % (self.auth_account, account, user)) resp = self.make_pre_authed_request( @@ -346,7 +349,6 @@ class Swauth(object): detail = json.loads(resp.body) password = detail['auth'].split(':')[-1] - msg = base64.urlsafe_b64decode(unquote(token)) # https://bugs.python.org/issue5285 if isinstance(password, unicode): @@ -354,9 +356,9 @@ class Swauth(object): if isinstance(msg, unicode): msg = msg.encode('utf-8') - s = base64.encodestring(hmac.new(password, - msg, sha1).digest()).strip() - if s != sign: + valid_signature = base64.encodestring(hmac.new( + password, msg, sha1).digest()).strip() + if signature_from_user != valid_signature: return None groups = [g['name'] for g in detail['groups']] if '.admin' in groups: diff --git a/test/unit/test_middleware.py b/test/unit/test_middleware.py index 60f40a5..e06532c 100644 --- a/test/unit/test_middleware.py +++ b/test/unit/test_middleware.py @@ -13,6 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import base64 from contextlib import contextmanager import json import mock @@ -3977,14 +3978,6 @@ class TestAuth(unittest.TestCase): self.assertEqual(resp.status_int, 400) self.assertEqual(resp.body, 'Token exceeds maximum length.') - def test_crazy_authorization(self): - req = self._make_request('/v1/AUTH_account', headers={ - 'authorization': 'somebody elses header value'}) - resp = req.get_response(self.test_auth) - self.assertEqual(resp.status_int, 401) - self.assertEqual(resp.environ['swift.authorize'], - self.test_auth.denied_response) - def test_default_storage_policy(self): ath = auth.filter_factory({})(FakeApp()) self.assertEqual(ath.default_storage_policy, None) @@ -3993,7 +3986,7 @@ class TestAuth(unittest.TestCase): auth.filter_factory({'default_storage_policy': 'ssd'})(FakeApp()) self.assertEqual(ath.default_storage_policy, 'ssd') - def test_s3_creds_unicode(self): + def test_s3_creds_unicode_bad(self): self.test_auth.app = FakeApp(iter([ ('200 Ok', {}, json.dumps({"auth": unicode("plaintext:key)"), @@ -4001,12 +3994,36 @@ class TestAuth(unittest.TestCase): {'name': ".admin"}]})), ('204 Ok', {'X-Container-Meta-Account-Id': 'AUTH_act'}, '')])) env = \ - {'HTTP_AUTHORIZATION': 'AWS act:user:3yW7oFFWOn+fhHMu7E47RKotL1Q=', + {'swift3.auth_details': { + 'access_key': 'act:user', + # NOTE: signature uses password of 'key', not 'key)' + 'signature': '3yW7oFFWOn+fhHMu7E47RKotL1Q=', + 'string_to_sign': base64.urlsafe_b64decode( + 'UFVUCgoKRnJpLCAyNiBGZWIgMjAxNiAwNjo0NT' + 'ozNCArMDAwMAovY29udGFpbmVyMw==')}, 'PATH_INFO': '/v1/AUTH_act/c1'} - token = 'UFVUCgoKRnJpLCAyNiBGZWIgMjAxNiAwNjo0NT'\ - 'ozNCArMDAwMAovY29udGFpbmVyMw==' + token = 'not used' self.assertEqual(self.test_auth.get_groups(env, token), None) + def test_s3_creds_unicode_good(self): + self.test_auth.app = FakeApp(iter([ + ('200 Ok', {}, + json.dumps({"auth": unicode("plaintext:key)"), + "groups": [{'name': "act:usr"}, {'name': "act"}, + {'name': ".admin"}]})), + ('204 Ok', {'X-Container-Meta-Account-Id': 'AUTH_act'}, '')])) + env = \ + {'swift3.auth_details': { + 'access_key': 'act:user', + 'signature': 'dElf49mbXP8t7F+P1qXZzaf3a50=', + 'string_to_sign': base64.urlsafe_b64decode( + 'UFVUCgoKRnJpLCAyNiBGZWIgMjAxNiAwNjo0NT' + 'ozNCArMDAwMAovY29udGFpbmVyMw==')}, + 'PATH_INFO': '/v1/AUTH_act/c1'} + token = 'not used' + self.assertEqual(self.test_auth.get_groups(env, token), + 'act:usr,act,AUTH_act') + if __name__ == '__main__': unittest.main() -- 2.7.4