openstack/swift
Code Issues Proposed changes

Authorise versioned write PUTs before copy

Browse Source 
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
This commit is contained in:
Matthew Oliver 2016-08-19 16:17:31 +10:00 committed by Thiago da Silva
parent 9d08d17b4f
commit d2fc261457
3 changed files with 68 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
swift/common/middleware/versioned_writes.py
View File

@ -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:

5
test/functional/tests.py
View File

@ -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,

62
test/unit/common/middleware/test_versioned_writes.py
View File

@ -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)