From d2fc2614575b04fd9cab5ae589880b92eee9b186 Mon Sep 17 00:00:00 2001 From: Matthew Oliver Date: Fri, 19 Aug 2016 16:17:31 +1000 Subject: [PATCH] Authorise versioned write PUTs before copy Currently a versioned write PUT uses a pre-authed request to move it into the versioned container before checking whether the user is authorised. This can lead to some interesting behaviour whereby a user can select a versioned object path that it does not have access to, request a put on that versioned object, and this request will execute the copy part of the request before it fails due to lack of permissions. This patch changes the behaviour to be the same as versioned DELETE where the request is authorised before anything is moved. Change-Id: Ia8b92251718d10b1eb44a456f28d3d2569a30003 Closes-Bug: #1562175 --- swift/common/middleware/versioned_writes.py | 10 +++ test/functional/tests.py | 5 ++ .../middleware/test_versioned_writes.py | 62 ++++++++++++++++--- 3 files changed, 68 insertions(+), 9 deletions(-) diff --git a/swift/common/middleware/versioned_writes.py b/swift/common/middleware/versioned_writes.py index 3ad8bd2eb1..c8c48b761d 100644 --- a/swift/common/middleware/versioned_writes.py +++ b/swift/common/middleware/versioned_writes.py @@ -407,6 +407,16 @@ class VersionedWritesContext(WSGIContext): def _copy_current(self, req, versions_cont, api_version, account_name, object_name): + # validate the write access to the versioned container before + # making any backend requests + if 'swift.authorize' in req.environ: + container_info = get_container_info( + req.environ, self.app) + req.acl = container_info.get('write_acl') + aresp = req.environ['swift.authorize'](req) + if aresp: + raise aresp + get_resp = self._get_source_object(req, req.path_info) if 'X-Object-Manifest' in get_resp.headers: diff --git a/test/functional/tests.py b/test/functional/tests.py index ff75272e54..092c3800d9 100644 --- a/test/functional/tests.py +++ b/test/functional/tests.py @@ -4123,11 +4123,16 @@ class TestObjectVersioning(Base): cfg={'use_token': self.env.storage_token3}) # user3 cannot write or delete from source container either + number_of_versions = versions_container.info()['object_count'] self.assertRaises(ResponseError, versioned_obj.write, "some random user trying to write data", cfg={'use_token': self.env.storage_token3}) + self.assertEqual(number_of_versions, + versions_container.info()['object_count']) self.assertRaises(ResponseError, versioned_obj.delete, cfg={'use_token': self.env.storage_token3}) + self.assertEqual(number_of_versions, + versions_container.info()['object_count']) # user2 can't read or delete from versions-location self.assertRaises(ResponseError, backup_file.read, diff --git a/test/unit/common/middleware/test_versioned_writes.py b/test/unit/common/middleware/test_versioned_writes.py index 4d7d0552b3..1c3176911e 100644 --- a/test/unit/common/middleware/test_versioned_writes.py +++ b/test/unit/common/middleware/test_versioned_writes.py @@ -402,8 +402,12 @@ class VersionedWritesTestCase(VersionedWritesBaseTestCase): 'swift.trans_id': 'fake_trans_id'}) status, headers, body = self.call_vw(req) self.assertEqual(status, '200 OK') - self.assertEqual(len(self.authorized), 1) + self.assertEqual(len(self.authorized), 2) + # Versioned writes middleware now calls auth on the incoming request + # before we try the GET and then at the proxy, so there are 2 + # atuhorized for the same request. self.assertRequestEqual(req, self.authorized[0]) + self.assertRequestEqual(req, self.authorized[1]) self.assertEqual(2, self.app.call_count) self.assertEqual(['VW', None], self.app.swift_sources) self.assertEqual({'fake_trans_id'}, set(self.app.txn_ids)) @@ -456,8 +460,12 @@ class VersionedWritesTestCase(VersionedWritesBaseTestCase): 'CONTENT_LENGTH': '100'}) status, headers, body = self.call_vw(req) self.assertEqual(status, '201 Created') - self.assertEqual(len(self.authorized), 1) + # The middleware now auths the request before the inital GET, the + # same GET that gets the X-Object-Manifest back. So a second auth is + # now done. + self.assertEqual(len(self.authorized), 2) self.assertRequestEqual(req, self.authorized[0]) + self.assertRequestEqual(req, self.authorized[1]) self.assertEqual(2, self.app.call_count) def test_delete_object_no_versioning_with_container_config_true(self): @@ -497,7 +505,9 @@ class VersionedWritesTestCase(VersionedWritesBaseTestCase): 'swift.trans_id': 'fake_trans_id'}) status, headers, body = self.call_vw(req) self.assertEqual(status, '201 Created') - self.assertEqual(len(self.authorized), 1) + # authorized twice now because versioned_writes now makes a check on + # PUT + self.assertEqual(len(self.authorized), 2) self.assertRequestEqual(req, self.authorized[0]) self.assertEqual(['VW', 'VW', None], self.app.swift_sources) self.assertEqual({'fake_trans_id'}, set(self.app.txn_ids)) @@ -577,7 +587,9 @@ class VersionedWritesTestCase(VersionedWritesBaseTestCase): 'CONTENT_LENGTH': '100'}) status, headers, body = self.call_vw(req) self.assertEqual(status, '200 OK') - self.assertEqual(len(self.authorized), 1) + # authorized twice now because versioned_writes now makes a check on + # PUT + self.assertEqual(len(self.authorized), 2) self.assertRequestEqual(req, self.authorized[0]) # check that sysmeta header was used @@ -869,7 +881,7 @@ class VersionedWritesTestCase(VersionedWritesBaseTestCase): 'CONTENT_LENGTH': '0'}) status, headers, body = self.call_vw(req) self.assertEqual(status, '404 Not Found') - self.assertEqual(len(self.authorized), 1) + self.assertEqual(len(self.authorized), 2) req.environ['REQUEST_METHOD'] = 'PUT' self.assertRequestEqual(req, self.authorized[0]) @@ -903,7 +915,7 @@ class VersionedWritesTestCase(VersionedWritesBaseTestCase): status, headers, body = self.call_vw(req) self.assertEqual(status, '204 No Content') self.assertEqual('', body) - self.assertEqual(len(self.authorized), 1) + self.assertEqual(len(self.authorized), 2) req.environ['REQUEST_METHOD'] = 'PUT' self.assertRequestEqual(req, self.authorized[0]) @@ -1044,6 +1056,32 @@ class VersionedWritesTestCase(VersionedWritesBaseTestCase): ('GET', prefix_listing_prefix + 'marker=&reverse=on'), ]) + def test_denied_PUT_of_versioned_object(self): + authorize_call = [] + self.app.register( + 'GET', '/v1/a/c/o', swob.HTTPOk, + {'last-modified': 'Thu, 1 Jan 1970 00:00:01 GMT'}, 'passed') + + def fake_authorize(req): + # we should deny the object PUT + authorize_call.append(req) + return swob.HTTPForbidden() + + cache = FakeCache({'sysmeta': {'versions-location': 'ver_cont'}}) + req = Request.blank( + '/v1/a/c/o', + environ={'REQUEST_METHOD': 'PUT', 'swift.cache': cache, + 'swift.authorize': fake_authorize, + 'CONTENT_LENGTH': '0'}) + # Save off a copy, as the middleware may modify the original + expected_req = Request(req.environ.copy()) + status, headers, body = self.call_vw(req) + self.assertEqual(status, '403 Forbidden') + self.assertEqual(len(authorize_call), 1) + self.assertRequestEqual(expected_req, authorize_call[0]) + + self.assertEqual(self.app.calls, []) + class VersionedWritesOldContainersTestCase(VersionedWritesBaseTestCase): def test_delete_latest_version_success(self): @@ -1379,11 +1417,17 @@ class VersionedWritesCopyingTestCase(VersionedWritesBaseTestCase): headers={'Destination': 'tgt_cont/tgt_obj'}) status, headers, body = self.call_filter(req) self.assertEqual(status, '201 Created') - self.assertEqual(len(self.authorized), 2) + self.assertEqual(len(self.authorized), 3) self.assertEqual('GET', self.authorized[0].method) self.assertEqual('/v1/a/src_cont/src_obj', self.authorized[0].path) + # At the moment we are calling authorize on the incoming request in + # the middleware before we do the PUT (and the source GET) and again + # on the incoming request when it gets to the proxy. So the 2nd and + # 3rd auths look the same. self.assertEqual('PUT', self.authorized[1].method) self.assertEqual('/v1/a/tgt_cont/tgt_obj', self.authorized[1].path) + self.assertEqual('PUT', self.authorized[2].method) + self.assertEqual('/v1/a/tgt_cont/tgt_obj', self.authorized[2].path) # note the GET on tgt_cont/tgt_obj is pre-authed self.assertEqual(3, self.app.call_count, self.app.calls) @@ -1407,7 +1451,7 @@ class VersionedWritesCopyingTestCase(VersionedWritesBaseTestCase): headers={'Destination': 'tgt_cont/tgt_obj'}) status, headers, body = self.call_filter(req) self.assertEqual(status, '201 Created') - self.assertEqual(len(self.authorized), 2) + self.assertEqual(len(self.authorized), 3) self.assertEqual('GET', self.authorized[0].method) self.assertEqual('/v1/a/src_cont/src_obj', self.authorized[0].path) self.assertEqual('PUT', self.authorized[1].method) @@ -1435,7 +1479,7 @@ class VersionedWritesCopyingTestCase(VersionedWritesBaseTestCase): 'Destination-Account': 'tgt_a'}) status, headers, body = self.call_filter(req) self.assertEqual(status, '201 Created') - self.assertEqual(len(self.authorized), 2) + self.assertEqual(len(self.authorized), 3) self.assertEqual('GET', self.authorized[0].method) self.assertEqual('/v1/src_a/src_cont/src_obj', self.authorized[0].path) self.assertEqual('PUT', self.authorized[1].method)