From a53b12a62c0da2a654d13a26f75c7dc214c13e6a Mon Sep 17 00:00:00 2001 From: Alistair Coles Date: Wed, 8 Jun 2016 15:33:22 +0100 Subject: [PATCH] Don't encrypt update override etags for empty object Fix an anomaly where object metadata for an empty object has no encrypted etag, but if the encrypter received a container update override etag in footers or headers then it would encrypt that, so we'd have encrypted metadata in the container listing but not in the object metadata. (Empty object etags are not encrypted because the object content is revealed by its size anyway). This patch changes the override handling to not encrypt override etags that correspond to an empty object, with one exception: if for some reason the received override etag value is that of an empty string but there *was* an object body, then we'll encrypt the override etag, because it's value is not obvious from the object size. Change-Id: I8d7da34d6d98f351f59174883bc4d5ed0416c101 --- swift/common/middleware/crypto/encrypter.py | 12 ++- swift/common/utils.py | 2 + swift/obj/diskfile.py | 3 +- .../middleware/crypto/test_encrypter.py | 79 +++++++++++++++++-- 4 files changed, 83 insertions(+), 13 deletions(-) diff --git a/swift/common/middleware/crypto/encrypter.py b/swift/common/middleware/crypto/encrypter.py index 2719d47700..b6c651fb84 100644 --- a/swift/common/middleware/crypto/encrypter.py +++ b/swift/common/middleware/crypto/encrypter.py @@ -25,7 +25,8 @@ from swift.common.request_helpers import get_object_transient_sysmeta, \ strip_user_meta_prefix, is_user_meta, update_etag_is_at_header from swift.common.swob import Request, Match, HTTPException, \ HTTPUnprocessableEntity -from swift.common.utils import get_logger, config_true_value +from swift.common.utils import get_logger, config_true_value, \ + MD5_OF_EMPTY_STRING def encrypt_header_val(crypto, value, key): @@ -149,13 +150,18 @@ class EncInputWrapper(object): 'X-Object-Sysmeta-Container-Update-Override-Etag', container_listing_etag_header) or plaintext_etag - if container_listing_etag is not None: + if (container_listing_etag is not None and + (container_listing_etag != MD5_OF_EMPTY_STRING or + plaintext_etag)): # Encrypt the container-listing etag using the container key # and a random IV, and use it to override the container update # value, with the crypto parameters appended. We use the # container key here so that only that key is required to # decrypt all etag values in a container listing when handling - # a container GET request. + # a container GET request. Don't encrypt an EMPTY_ETAG + # unless there actually was some body content, in which case + # the container-listing etag is possibly conveying some + # non-obvious information. val, crypto_meta = encrypt_header_val( self.crypto, container_listing_etag, self.keys['container']) diff --git a/swift/common/utils.py b/swift/common/utils.py index d3ef5a7dcb..69a4727166 100644 --- a/swift/common/utils.py +++ b/swift/common/utils.py @@ -118,6 +118,8 @@ F_SETPIPE_SZ = getattr(fcntl, 'F_SETPIPE_SZ', 1031) # Used by the parse_socket_string() function to validate IPv6 addresses IPV6_RE = re.compile("^\[(?P
.*)\](:(?P[0-9]+))?$") +MD5_OF_EMPTY_STRING = 'd41d8cd98f00b204e9800998ecf8427e' + class InvalidHashPathConfigError(ValueError): diff --git a/swift/obj/diskfile.py b/swift/obj/diskfile.py index 7f9135f229..9e69e954f2 100644 --- a/swift/obj/diskfile.py +++ b/swift/obj/diskfile.py @@ -58,7 +58,7 @@ from swift.common.utils import mkdirs, Timestamp, \ fsync_dir, drop_buffer_cache, lock_path, write_pickle, \ config_true_value, listdir, split_path, ismount, remove_file, \ get_md5_socket, F_SETPIPE_SZ, decode_timestamps, encode_timestamps, \ - tpool_reraise + tpool_reraise, MD5_OF_EMPTY_STRING from swift.common.splice import splice, tee from swift.common.exceptions import DiskFileQuarantined, DiskFileNotExist, \ DiskFileCollision, DiskFileNoSpace, DiskFileDeviceUnavailable, \ @@ -86,7 +86,6 @@ TMP_BASE = 'tmp' get_data_dir = partial(get_policy_string, DATADIR_BASE) get_async_dir = partial(get_policy_string, ASYNCDIR_BASE) get_tmp_dir = partial(get_policy_string, TMP_BASE) -MD5_OF_EMPTY_STRING = 'd41d8cd98f00b204e9800998ecf8427e' def _get_filename(fd): diff --git a/test/unit/common/middleware/crypto/test_encrypter.py b/test/unit/common/middleware/crypto/test_encrypter.py index 0f9553cad7..1fa765c66b 100644 --- a/test/unit/common/middleware/crypto/test_encrypter.py +++ b/test/unit/common/middleware/crypto/test_encrypter.py @@ -225,7 +225,7 @@ class TestEncrypter(unittest.TestCase): self.assertEqual('', resp.body) self.assertEqual(EMPTY_ETAG, resp.headers['Etag']) - def test_PUT_with_other_footers(self): + def _test_PUT_with_other_footers(self, override_etag): # verify handling of another middleware's footer callback cont_key = fetch_crypto_keys()['container'] body_key = os.urandom(32) @@ -240,7 +240,7 @@ class TestEncrypter(unittest.TestCase): 'X-Object-Sysmeta-Container-Update-Override-Size': 'other override', 'X-Object-Sysmeta-Container-Update-Override-Etag': - 'final etag'} + override_etag} env = {'REQUEST_METHOD': 'PUT', CRYPTO_KEY_CALLBACK: fetch_crypto_keys, @@ -304,7 +304,7 @@ class TestEncrypter(unittest.TestCase): cont_key = fetch_crypto_keys()['container'] cont_etag_iv = base64.b64decode(actual_meta['iv']) self.assertEqual(FAKE_IV, cont_etag_iv) - self.assertEqual(encrypt('final etag', cont_key, cont_etag_iv), + self.assertEqual(encrypt(override_etag, cont_key, cont_etag_iv), base64.b64decode(parts[0])) # verify body crypto meta @@ -321,7 +321,15 @@ class TestEncrypter(unittest.TestCase): base64.b64decode(actual['body_key']['iv'])) self.assertEqual(fetch_crypto_keys()['id'], actual['key_id']) - def test_PUT_with_etag_override_in_headers(self): + def test_PUT_with_other_footers(self): + self._test_PUT_with_other_footers('override etag') + + def test_PUT_with_other_footers_and_empty_etag(self): + # verify that an override etag value of EMPTY_ETAG will be encrypted + # when there was a non-zero body length + self._test_PUT_with_other_footers(EMPTY_ETAG) + + def _test_PUT_with_etag_override_in_headers(self, override_etag): # verify handling of another middleware's # container-update-override-etag in headers plaintext = 'FAKE APP' @@ -333,7 +341,7 @@ class TestEncrypter(unittest.TestCase): 'content-length': str(len(plaintext)), 'Etag': plaintext_etag, 'X-Object-Sysmeta-Container-Update-Override-Etag': - 'final etag'} + override_etag} req = Request.blank( '/v1/a/c/o', environ=env, body=plaintext, headers=hdrs) self.app.register('PUT', '/v1/a/c/o', HTTPCreated, {}) @@ -366,9 +374,17 @@ class TestEncrypter(unittest.TestCase): cont_etag_iv = base64.b64decode(actual_meta['iv']) self.assertEqual(FAKE_IV, cont_etag_iv) - self.assertEqual(encrypt('final etag', cont_key, cont_etag_iv), + self.assertEqual(encrypt(override_etag, cont_key, cont_etag_iv), base64.b64decode(parts[0])) + def test_PUT_with_etag_override_in_headers(self): + self._test_PUT_with_etag_override_in_headers('override_etag') + + def test_PUT_with_etag_override_in_headers_and_empty_etag(self): + # verify that an override etag value of EMPTY_ETAG will be encrypted + # when there was a non-zero body length + self._test_PUT_with_etag_override_in_headers(EMPTY_ETAG) + def test_PUT_with_bad_etag_in_other_footers(self): # verify that etag supplied in footers from other middleware overrides # header etag when validating inbound plaintext etags @@ -448,9 +464,10 @@ class TestEncrypter(unittest.TestCase): # check that an upstream footer callback gets called other_footers = { - 'Etag': 'other etag', + 'Etag': EMPTY_ETAG, 'X-Object-Sysmeta-Other': 'other sysmeta', - 'X-Backend-Container-Update-Override-Etag': 'other override'} + 'X-Object-Sysmeta-Container-Update-Override-Etag': + 'other override'} env.update({'swift.callback.update_footers': lambda footers: footers.update(other_footers)}) req = Request.blank('/v1/a/c/o', environ=env, body='', headers=hdrs) @@ -461,6 +478,52 @@ class TestEncrypter(unittest.TestCase): self.assertEqual('201 Created', resp.status) self.assertEqual('response etag', resp.headers['Etag']) self.assertEqual(1, len(call_headers)) + + # verify encrypted override etag for container update. + self.assertIn( + 'X-Object-Sysmeta-Container-Update-Override-Etag', call_headers[0]) + parts = call_headers[0][ + 'X-Object-Sysmeta-Container-Update-Override-Etag'].rsplit(';', 1) + self.assertEqual(2, len(parts)) + cont_key = fetch_crypto_keys()['container'] + + param = parts[1].strip() + crypto_meta_tag = 'swift_meta=' + self.assertTrue(param.startswith(crypto_meta_tag), param) + actual_meta = json.loads( + urllib.unquote_plus(param[len(crypto_meta_tag):])) + self.assertEqual(Crypto().cipher, actual_meta['cipher']) + self.assertEqual(fetch_crypto_keys()['id'], actual_meta['key_id']) + + cont_etag_iv = base64.b64decode(actual_meta['iv']) + self.assertEqual(FAKE_IV, cont_etag_iv) + self.assertEqual(encrypt('other override', cont_key, cont_etag_iv), + base64.b64decode(parts[0])) + + # verify that other middleware's footers made it to app + other_footers.pop('X-Object-Sysmeta-Container-Update-Override-Etag') + for k, v in other_footers.items(): + self.assertEqual(v, call_headers[0][k]) + # verify no encryption footers + for k in call_headers[0]: + self.assertFalse(k.lower().startswith('x-object-sysmeta-crypto-')) + + # if upstream footer override etag is for an empty body then check that + # it is not encrypted + other_footers = { + 'Etag': EMPTY_ETAG, + 'X-Object-Sysmeta-Container-Update-Override-Etag': EMPTY_ETAG} + env.update({'swift.callback.update_footers': + lambda footers: footers.update(other_footers)}) + req = Request.blank('/v1/a/c/o', environ=env, body='', headers=hdrs) + + call_headers = [] + resp = req.get_response(encrypter.Encrypter(NonReadingApp(), {})) + + self.assertEqual('201 Created', resp.status) + self.assertEqual('response etag', resp.headers['Etag']) + self.assertEqual(1, len(call_headers)) + # verify that other middleware's footers made it to app for k, v in other_footers.items(): self.assertEqual(v, call_headers[0][k])