From 51b885b3b5797b5d7bfc4a1b0005b4257729d059 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Mon, 25 Jun 2018 15:40:30 -0700 Subject: [PATCH] s3_acl: Require swift_owner authz to create buckets Otherwise, users can create buckets in accounts they don't own. Change-Id: I13d557c32b12529ef1087c52f7af302a33d33acb --- swift/common/middleware/s3api/acl_handlers.py | 5 ++++- swift/common/middleware/s3api/s3request.py | 3 +++ test/functional/s3api/test_bucket.py | 6 +++++- test/unit/common/middleware/s3api/__init__.py | 7 ++++--- test/unit/common/middleware/s3api/helpers.py | 12 +++++++++--- test/unit/common/middleware/s3api/test_bucket.py | 6 ++++++ 6 files changed, 31 insertions(+), 8 deletions(-) diff --git a/swift/common/middleware/s3api/acl_handlers.py b/swift/common/middleware/s3api/acl_handlers.py index ad563dad7e..d623d0906b 100644 --- a/swift/common/middleware/s3api/acl_handlers.py +++ b/swift/common/middleware/s3api/acl_handlers.py @@ -53,7 +53,7 @@ import sys from swift.common.middleware.s3api.subresource import ACL, Owner, encode_acl from swift.common.middleware.s3api.s3response import MissingSecurityHeader, \ - MalformedACLError, UnexpectedContent + MalformedACLError, UnexpectedContent, AccessDenied from swift.common.middleware.s3api.etree import fromstring, XMLSyntaxError, \ DocumentInvalid from swift.common.middleware.s3api.utils import MULTIUPLOAD_SUFFIX, \ @@ -209,6 +209,9 @@ class BucketAclHandler(BaseAclHandler): req_acl = ACL.from_headers(self.req.headers, Owner(self.user_id, self.user_id)) + if not self.req.environ.get('swift_owner'): + raise AccessDenied() + # To avoid overwriting the existing bucket's ACL, we send PUT # request first before setting the ACL to make sure that the target # container does not exist. diff --git a/swift/common/middleware/s3api/s3request.py b/swift/common/middleware/s3api/s3request.py index 5833921f89..bb94061b1c 100644 --- a/swift/common/middleware/s3api/s3request.py +++ b/swift/common/middleware/s3api/s3request.py @@ -1341,6 +1341,9 @@ class S3AclRequest(S3Request): # tempauth self.user_id = self.access_key + sw_req.environ.get('swift.authorize', lambda req: None)(sw_req) + self.environ['swift_owner'] = sw_req.environ.get('swift_owner', False) + # Need to skip S3 authorization on subsequent requests to prevent # overwriting the account in PATH_INFO del self.headers['Authorization'] diff --git a/test/functional/s3api/test_bucket.py b/test/functional/s3api/test_bucket.py index ce508b5c34..451dfbd47c 100644 --- a/test/functional/s3api/test_bucket.py +++ b/test/functional/s3api/test_bucket.py @@ -151,7 +151,11 @@ class TestS3ApiBucket(S3ApiBase): self.conn.make_request('PUT', 'bucket') status, headers, body = self.conn.make_request('PUT', 'bucket') - self.assertEqual(get_error_code(body), 'BucketAlreadyExists') + # If the user can't create buckets, they shouldn't even know + # whether the bucket exists. For some reason, though, when s3_acl + # is disabled, we translate 403 -> BucketAlreadyExists?? + self.assertIn(get_error_code(body), + ('AccessDenied', 'BucketAlreadyExists')) def test_put_bucket_with_LocationConstraint(self): bucket = 'bucket' diff --git a/test/unit/common/middleware/s3api/__init__.py b/test/unit/common/middleware/s3api/__init__.py index d2d8ce2591..7963e615c3 100644 --- a/test/unit/common/middleware/s3api/__init__.py +++ b/test/unit/common/middleware/s3api/__init__.py @@ -106,7 +106,8 @@ class S3ApiTestCase(unittest.TestCase): elem = fromstring(body, 'Error') return elem.find('./Message').text - def _test_method_error(self, method, path, response_class, headers={}): + def _test_method_error(self, method, path, response_class, headers={}, + env={}): if not path.startswith('/'): path = '/' + path # add a missing slash before the path @@ -117,8 +118,8 @@ class S3ApiTestCase(unittest.TestCase): self.swift.register(method, uri, response_class, headers, None) headers.update({'Authorization': 'AWS test:tester:hmac', 'Date': self.get_date_header()}) - req = swob.Request.blank(path, environ={'REQUEST_METHOD': method}, - headers=headers) + env.update({'REQUEST_METHOD': method}) + req = swob.Request.blank(path, environ=env, headers=headers) status, headers, body = self.call_s3api(req) return self._get_error_code(body) diff --git a/test/unit/common/middleware/s3api/helpers.py b/test/unit/common/middleware/s3api/helpers.py index 71d31f6238..63a4839800 100644 --- a/test/unit/common/middleware/s3api/helpers.py +++ b/test/unit/common/middleware/s3api/helpers.py @@ -53,9 +53,15 @@ class FakeSwift(object): env['REMOTE_USER'] = 'authorized' if env['REQUEST_METHOD'] == 'TEST': - # AccessDenied by default at s3acl authenticate - env['swift.authorize'] = \ - lambda req: swob.HTTPForbidden(request=req) + + def authorize_cb(req): + # Assume swift owner, if not yet set + req.environ.setdefault('swift_owner', True) + # But then default to blocking authz, to ensure we've replaced + # the default auth system + return swob.HTTPForbidden(request=req) + + env['swift.authorize'] = authorize_cb else: env['swift.authorize'] = lambda req: None diff --git a/test/unit/common/middleware/s3api/test_bucket.py b/test/unit/common/middleware/s3api/test_bucket.py index e98e0e03ac..8346fcc847 100644 --- a/test/unit/common/middleware/s3api/test_bucket.py +++ b/test/unit/common/middleware/s3api/test_bucket.py @@ -498,6 +498,12 @@ class TestS3ApiBucket(S3ApiTestCase): swob.HTTPCreated) self.assertEqual(code, 'InvalidBucketName') + @s3acl(s3acl_only=True) + def test_bucket_PUT_error_non_owner(self): + code = self._test_method_error('PUT', '/bucket', swob.HTTPAccepted, + env={'swift_owner': False}) + self.assertEqual(code, 'AccessDenied') + @s3acl def test_bucket_PUT(self): req = Request.blank('/bucket',