diff --git a/etc/keymaster.conf-sample b/etc/keymaster.conf-sample index 620740e0d2..c413df7792 100644 --- a/etc/keymaster.conf-sample +++ b/etc/keymaster.conf-sample @@ -1,4 +1,13 @@ [keymaster] +# Over time, the format of crypto metadata on disk may change slightly to resolve +# ambiguities. In general, you want to be writing the newest version, but to +# ensure that all writes can still be read during rolling upgrades, there's the +# option to write older formats as well. +# Before upgrading from Swift 2.20.0 or earlier, ensure this is set to 1 +# Before upgrading from Swift 2.25.0 or earlier, ensure this is set to at most 2 +# After upgrading all proxy servers, set this to 3 (currently the highest version) +# meta_version_to_write = 3 + # Sets the root secret from which encryption keys are derived. This must be set # before first use to a value that is a base64 encoding of at least 32 bytes. # The security of all encrypted data critically depends on this key, therefore @@ -16,6 +25,15 @@ # backends that use Keystone for authentication. Currently, the only # implemented backend is for Barbican. +# Over time, the format of crypto metadata on disk may change slightly to resolve +# ambiguities. In general, you want to be writing the newest version, but to +# ensure that all writes can still be read during rolling upgrades, there's the +# option to write older formats as well. +# Before upgrading from Swift 2.20.0 or earlier, ensure this is set to 1 +# Before upgrading from Swift 2.25.0 or earlier, ensure this is set to at most 2 +# After upgrading all proxy servers, set this to 3 (currently the highest version) +# meta_version_to_write = 3 + # The api_class tells Castellan which key manager to use to access the external # key management system. The default value that accesses Barbican is # castellan.key_manager.barbican_key_manager.BarbicanKeyManager. @@ -79,6 +97,15 @@ # The kmip_keymaster section is used to configure a keymaster that fetches an # encryption root secret from a KMIP service. +# Over time, the format of crypto metadata on disk may change slightly to resolve +# ambiguities. In general, you want to be writing the newest version, but to +# ensure that all writes can still be read during rolling upgrades, there's the +# option to write older formats as well. +# Before upgrading from Swift 2.20.0 or earlier, ensure this is set to 1 +# Before upgrading from Swift 2.25.0 or earlier, ensure this is set to at most 2 +# After upgrading all proxy servers, set this to 3 (currently the highest version) +# meta_version_to_write = 3 + # The value of the ``key_id`` option should be the unique identifier for a # secret that will be retrieved from the KMIP service. The secret should be an # AES-256 symmetric key. diff --git a/etc/proxy-server.conf-sample b/etc/proxy-server.conf-sample index 4a1b643b85..a8829bd0db 100644 --- a/etc/proxy-server.conf-sample +++ b/etc/proxy-server.conf-sample @@ -1052,6 +1052,18 @@ use = egg:swift#copy [filter:keymaster] use = egg:swift#keymaster +# Over time, the format of crypto metadata on disk may change slightly to resolve +# ambiguities. In general, you want to be writing the newest version, but to +# ensure that all writes can still be read during rolling upgrades, there's the +# option to write older formats as well. +# Before upgrading from Swift 2.20.0 or Swift 2.19.1 or earlier, ensure this is set to 1 +# Before upgrading from Swift 2.25.0 or earlier, ensure this is set to at most 2 +# After upgrading all proxy servers, set this to 3 (currently the highest version) +# +# The default is currently 2 to support upgrades with no configuration changes, +# but may change to 3 in the future. +meta_version_to_write = 2 + # Sets the root secret from which encryption keys are derived. This must be set # before first use to a value that is a base64 encoding of at least 32 bytes. # The security of all encrypted data critically depends on this key, therefore diff --git a/swift/common/middleware/crypto/keymaster.py b/swift/common/middleware/crypto/keymaster.py index da337d37e6..87353769ec 100644 --- a/swift/common/middleware/crypto/keymaster.py +++ b/swift/common/middleware/crypto/keymaster.py @@ -14,10 +14,11 @@ # limitations under the License. import hashlib import hmac +import six from swift.common.exceptions import UnknownSecretIdError from swift.common.middleware.crypto.crypto_utils import CRYPTO_KEY_CALLBACK -from swift.common.swob import Request, HTTPException, wsgi_to_bytes +from swift.common.swob import Request, HTTPException, wsgi_to_str, str_to_wsgi from swift.common.utils import readconf, strict_b64decode, get_logger, \ split_path from swift.common.wsgi import WSGIContext @@ -33,7 +34,8 @@ class KeyMasterContext(WSGIContext): = HMAC_SHA256(, ) """ - def __init__(self, keymaster, account, container, obj): + def __init__(self, keymaster, account, container, obj, + meta_version_to_write='2'): """ :param keymaster: a Keymaster instance :param account: account name @@ -47,8 +49,11 @@ class KeyMasterContext(WSGIContext): self.obj = obj self._keys = {} self.alternate_fetch_keys = None + self.meta_version_to_write = meta_version_to_write def _make_key_id(self, path, secret_id, version): + if version in ('1', '2'): + path = str_to_wsgi(path) key_id = {'v': version, 'path': path} if secret_id: # stash secret_id so that decrypter can pass it back to get the @@ -76,8 +81,9 @@ class KeyMasterContext(WSGIContext): if key_id: secret_id = key_id.get('secret_id') version = key_id['v'] - if version not in ('1', '2'): + if version not in ('1', '2', '3'): raise ValueError('Unknown key_id version: %s' % version) + if version == '1' and not key_id['path'].startswith( '/' + self.account + '/'): # Well shoot. This was the bug that made us notice we needed @@ -90,7 +96,32 @@ class KeyMasterContext(WSGIContext): check_path = ( self.account, self.container or key_cont, self.obj or key_obj) + if version in ('1', '2') and ( + key_acct, key_cont, key_obj) != check_path: + # Older py3 proxies may have written down crypto meta as WSGI + # strings; we still need to be able to read that + try: + if six.PY2: + alt_path = tuple( + part.decode('utf-8').encode('latin1') + for part in (key_acct, key_cont, key_obj)) + else: + alt_path = tuple( + part.encode('latin1').decode('utf-8') + for part in (key_acct, key_cont, key_obj)) + except UnicodeError: + # Well, it was worth a shot + pass + else: + if check_path == alt_path or ( + check_path[:2] == alt_path[:2] and not self.obj): + # This object is affected by bug #1888037 + key_acct, key_cont, key_obj = alt_path + if (key_acct, key_cont, key_obj) != check_path: + # Pipeline may have been misconfigured, with copy right of + # encryption. In that case, path in meta may not be the + # request path. self.keymaster.logger.info( "Path stored in meta (%r) does not match path from " "request (%r)! Using path from meta.", @@ -100,9 +131,11 @@ class KeyMasterContext(WSGIContext): else: secret_id = self.keymaster.active_secret_id # v1 had a bug where we would claim the path was just the object - # name if the object started with a slash. Bump versions to - # establish that we can trust the path. - version = '2' + # name if the object started with a slash. + # v1 and v2 had a bug on py3 where we'd write the path in meta as + # a WSGI string (ie, as Latin-1 chars decoded from UTF-8 bytes). + # Bump versions to establish that we can trust the path. + version = self.meta_version_to_write key_acct, key_cont, key_obj = ( self.account, self.container, self.obj) @@ -215,6 +248,11 @@ class BaseKeyMaster(object): raise ValueError('No secret loaded for active_root_secret_id %s' % self.active_secret_id) + self.meta_version_to_write = conf.get('meta_version_to_write') or '2' + if self.meta_version_to_write not in ('1', '2', '3'): + raise ValueError('Unknown/unsupported metadata version: %r' % + self.meta_version_to_write) + @property def root_secret(self): # Returns the default root secret; this is here for historical reasons @@ -261,13 +299,15 @@ class BaseKeyMaster(object): req = Request(env) try: - parts = req.split_path(2, 4, True) + parts = [wsgi_to_str(part) for part in req.split_path(2, 4, True)] except ValueError: return self.app(env, start_response) if req.method in ('PUT', 'POST', 'GET', 'HEAD'): # handle only those request methods that may require keys - km_context = KeyMasterContext(self, *parts[1:]) + km_context = KeyMasterContext( + self, *parts[1:], + meta_version_to_write=self.meta_version_to_write) try: return km_context.handle_request(req, start_response) except HTTPException as err_resp: @@ -292,8 +332,9 @@ class BaseKeyMaster(object): self.logger.warning('Unrecognised secret id: %s' % secret_id) raise UnknownSecretIdError(secret_id) else: - return hmac.new(key, wsgi_to_bytes(path), - digestmod=hashlib.sha256).digest() + if not six.PY2: + path = path.encode('utf-8') + return hmac.new(key, path, digestmod=hashlib.sha256).digest() class KeyMaster(BaseKeyMaster): diff --git a/test/unit/common/middleware/crypto/test_keymaster.py b/test/unit/common/middleware/crypto/test_keymaster.py index 7d2c9b856f..63a74b6b6f 100644 --- a/test/unit/common/middleware/crypto/test_keymaster.py +++ b/test/unit/common/middleware/crypto/test_keymaster.py @@ -20,6 +20,7 @@ import hmac import os import mock +import six import unittest from getpass import getuser @@ -49,17 +50,45 @@ class TestKeymaster(unittest.TestCase): self.app = keymaster.KeyMaster(self.swift, TEST_KEYMASTER_CONF) def test_object_path(self): - self.verify_keys_for_path( + self.verify_v3_keys_for_path( '/a/c/o', expected_keys=('object', 'container')) + self.verify_v3_keys_for_path( + '/a/c//o', expected_keys=('object', 'container')) self.verify_keys_for_path( '/a/c//o', expected_keys=('object', 'container')) + self.verify_v1_keys_for_path( + '/a/c//o', expected_keys=('object', 'container')) def test_container_path(self): - self.verify_keys_for_path( + self.verify_v3_keys_for_path( '/a/c', expected_keys=('container',)) - def verify_keys_for_path(self, path, expected_keys, key_id=None): + def test_unicode_object_path(self): + # NB: path is WSGI + self.verify_v3_keys_for_path( + '/\xe2\x98\x83/\xf0\x9f\x8c\xb4/\xf0\x9f\x8c\x8a', + expected_keys=('object', 'container')) + self.verify_keys_for_path( + '/\xe2\x98\x83/\xf0\x9f\x8c\xb4/\xf0\x9f\x8c\x8a', + expected_keys=('object', 'container')) + self.verify_v1_keys_for_path( + '/\xe2\x98\x83/\xf0\x9f\x8c\xb4/\xf0\x9f\x8c\x8a', + expected_keys=('object', 'container')) + + # Double-whammy: *also* hit the os.path.join issue + self.verify_v3_keys_for_path( + '/\xe2\x98\x83/\xf0\x9f\x8c\xb4//\xf0\x9f\x8c\x8a', + expected_keys=('object', 'container')) + self.verify_keys_for_path( + '/\xe2\x98\x83/\xf0\x9f\x8c\xb4//\xf0\x9f\x8c\x8a', + expected_keys=('object', 'container')) + self.verify_v1_keys_for_path( + '/\xe2\x98\x83/\xf0\x9f\x8c\xb4//\xf0\x9f\x8c\x8a', + expected_keys=('object', 'container')) + + def verify_v3_keys_for_path(self, wsgi_path, expected_keys, key_id=None): put_keys = None + self.app.meta_version_to_write = '3' for method, resp_class, status in ( ('PUT', swob.HTTPCreated, '201'), ('POST', swob.HTTPAccepted, '202'), @@ -67,9 +96,9 @@ class TestKeymaster(unittest.TestCase): ('HEAD', swob.HTTPNoContent, '204')): resp_headers = {} self.swift.register( - method, '/v1' + path, resp_class, resp_headers, b'') + method, '/v1' + wsgi_path, resp_class, resp_headers, b'') req = Request.blank( - '/v1' + path, environ={'REQUEST_METHOD': method}) + '/v1' + wsgi_path, environ={'REQUEST_METHOD': method}) start_response, calls = capture_start_response() self.app(req.environ, start_response) self.assertEqual(1, len(calls)) @@ -80,7 +109,48 @@ class TestKeymaster(unittest.TestCase): keys = req.environ.get(CRYPTO_KEY_CALLBACK)(key_id=key_id) self.assertIn('id', keys) id = keys.pop('id') + path = swob.wsgi_to_str(wsgi_path) self.assertEqual(path, id['path']) + self.assertEqual('3', id['v']) + keys.pop('all_ids') + self.assertListEqual(sorted(expected_keys), sorted(keys.keys()), + '%s %s got keys %r, but expected %r' + % (method, path, keys.keys(), expected_keys)) + if put_keys is not None: + # check all key sets were consistent for this path + self.assertDictEqual(put_keys, keys) + else: + put_keys = keys + self.app.meta_version_to_write = '2' # Clean up after ourselves + return put_keys + + def verify_keys_for_path(self, wsgi_path, expected_keys, key_id=None): + put_keys = None + for method, resp_class, status in ( + ('PUT', swob.HTTPCreated, '201'), + ('POST', swob.HTTPAccepted, '202'), + ('GET', swob.HTTPOk, '200'), + ('HEAD', swob.HTTPNoContent, '204')): + resp_headers = {} + self.swift.register( + method, '/v1' + wsgi_path, resp_class, resp_headers, b'') + req = Request.blank( + '/v1' + wsgi_path, environ={'REQUEST_METHOD': method}) + start_response, calls = capture_start_response() + self.app(req.environ, start_response) + self.assertEqual(1, len(calls)) + self.assertTrue(calls[0][0].startswith(status)) + self.assertNotIn('swift.crypto.override', req.environ) + self.assertIn(CRYPTO_KEY_CALLBACK, req.environ, + '%s not set in env' % CRYPTO_KEY_CALLBACK) + keys = req.environ.get(CRYPTO_KEY_CALLBACK)(key_id=key_id) + self.assertIn('id', keys) + id = keys.pop('id') + path = swob.wsgi_to_str(wsgi_path) + if six.PY2: + self.assertEqual(path, id['path']) + else: + self.assertEqual(swob.str_to_wsgi(path), id['path']) self.assertEqual('2', id['v']) keys.pop('all_ids') self.assertListEqual(sorted(expected_keys), sorted(keys.keys()), @@ -93,6 +163,49 @@ class TestKeymaster(unittest.TestCase): put_keys = keys return put_keys + def verify_v1_keys_for_path(self, wsgi_path, expected_keys, key_id=None): + put_keys = None + self.app.meta_version_to_write = '1' + for method, resp_class, status in ( + ('PUT', swob.HTTPCreated, '201'), + ('POST', swob.HTTPAccepted, '202'), + ('GET', swob.HTTPOk, '200'), + ('HEAD', swob.HTTPNoContent, '204')): + resp_headers = {} + self.swift.register( + method, '/v1' + wsgi_path, resp_class, resp_headers, b'') + req = Request.blank( + '/v1' + wsgi_path, environ={'REQUEST_METHOD': method}) + start_response, calls = capture_start_response() + self.app(req.environ, start_response) + self.assertEqual(1, len(calls)) + self.assertTrue(calls[0][0].startswith(status)) + self.assertNotIn('swift.crypto.override', req.environ) + self.assertIn(CRYPTO_KEY_CALLBACK, req.environ, + '%s not set in env' % CRYPTO_KEY_CALLBACK) + keys = req.environ.get(CRYPTO_KEY_CALLBACK)(key_id=key_id) + self.assertIn('id', keys) + id = keys.pop('id') + path = swob.wsgi_to_str(wsgi_path) + if '//' in path: + path = path[path.index('//') + 1:] + if six.PY2: + self.assertEqual(path, id['path']) + else: + self.assertEqual(swob.str_to_wsgi(path), id['path']) + self.assertEqual('1', id['v']) + keys.pop('all_ids') + self.assertListEqual(sorted(expected_keys), sorted(keys.keys()), + '%s %s got keys %r, but expected %r' + % (method, path, keys.keys(), expected_keys)) + if put_keys is not None: + # check all key sets were consistent for this path + self.assertDictEqual(put_keys, keys) + else: + put_keys = keys + self.app.meta_version_to_write = '2' # Clean up after ourselves + return put_keys + def test_key_uniqueness(self): # a rudimentary check that different keys are made for different paths ref_path_parts = ('a1', 'c1', 'o1') @@ -432,7 +545,7 @@ class TestKeymaster(unittest.TestCase): return orig_create_key(path, secret_id) context = keymaster.KeyMasterContext(self.app, 'a', 'c', 'o') - for version in ('1', '2'): + for version in ('1', '2', '3'): with mock.patch.object(self.app, 'create_key', mock_create_key): keys = context.fetch_crypto_keys(key_id={ 'v': version, 'path': '/a/c/o'}) @@ -502,7 +615,7 @@ class TestKeymaster(unittest.TestCase): # request path doesn't match stored path -- this could happen if you # misconfigured your proxy to have copy right of encryption context = keymaster.KeyMasterContext(self.app, 'a', 'not-c', 'not-o') - for version in ('1', '2'): + for version in ('1', '2', '3'): with mock.patch.object(self.app, 'create_key', mock_create_key): keys = context.fetch_crypto_keys(key_id={ 'v': version, 'path': '/a/c/o'}) @@ -554,6 +667,106 @@ class TestKeymaster(unittest.TestCase): self.assertEqual(expected_keys, keys) self.assertEqual([('/a/c', None), ('/a/c//o', None)], calls) + def test_v2_keys(self): + secrets = {None: os.urandom(32), + '22': os.urandom(33)} + conf = {} + for secret_id, secret in secrets.items(): + opt = ('encryption_root_secret%s' % + (('_%s' % secret_id) if secret_id else '')) + conf[opt] = base64.b64encode(secret) + conf['active_root_secret_id'] = '22' + self.app = keymaster.KeyMaster(self.swift, conf) + orig_create_key = self.app.create_key + calls = [] + + def mock_create_key(path, secret_id=None): + calls.append((path, secret_id)) + return orig_create_key(path, secret_id) + + container = u'\N{SNOWMAN}' + obj = u'\N{SNOWFLAKE}' + if six.PY2: + container = container.encode('utf-8') + obj = obj.encode('utf-8') + good_con_path = '/a/%s' % container + good_path = '/a/%s/%s' % (container, obj) + + if six.PY2: + mangled_con_path = ('/a/%s' % container).decode( + 'latin-1').encode('utf-8') + mangled_path = ('/a/%s/%s' % ( + container, obj)).decode('latin-1').encode('utf-8') + else: + mangled_con_path = ('/a/%s' % container).encode( + 'utf-8').decode('latin-1') + mangled_path = ('/a/%s/%s' % ( + container, obj)).encode('utf-8').decode('latin-1') + + context = keymaster.KeyMasterContext(self.app, 'a', container, obj) + for version in ('1', '2', '3'): + with mock.patch.object(self.app, 'create_key', mock_create_key): + keys = context.fetch_crypto_keys(key_id={ + 'v': version, 'path': good_path}) + key_id_path = (good_path if version == '3' or six.PY2 + else mangled_path) + expected_keys = { + 'container': hmac.new(secrets[None], b'/a/\xe2\x98\x83', + digestmod=hashlib.sha256).digest(), + 'object': hmac.new( + secrets[None], b'/a/\xe2\x98\x83/\xe2\x9d\x84', + digestmod=hashlib.sha256).digest(), + 'id': {'path': key_id_path, 'v': version}, + 'all_ids': [ + {'path': key_id_path, 'v': version}, + {'path': key_id_path, 'secret_id': '22', 'v': version}]} + self.assertEqual(expected_keys, keys) + self.assertEqual([(good_con_path, None), (good_path, None)], calls) + del calls[:] + + context = keymaster.KeyMasterContext(self.app, 'a', container, obj) + for version in ('1', '2'): + with mock.patch.object(self.app, 'create_key', mock_create_key): + keys = context.fetch_crypto_keys(key_id={ + 'v': version, 'path': mangled_path}) + key_id_path = (good_path if six.PY2 else mangled_path) + expected_keys = { + 'container': hmac.new(secrets[None], b'/a/\xe2\x98\x83', + digestmod=hashlib.sha256).digest(), + 'object': hmac.new( + secrets[None], b'/a/\xe2\x98\x83/\xe2\x9d\x84', + digestmod=hashlib.sha256).digest(), + 'id': {'path': key_id_path, 'v': version}, + 'all_ids': [ + {'path': key_id_path, 'v': version}, + {'path': key_id_path, 'secret_id': '22', 'v': version}]} + self.assertEqual(expected_keys, keys) + self.assertEqual([(good_con_path, None), (good_path, None)], calls) + del calls[:] + + # If v3, we know to trust the meta -- presumably, data was PUT with + # the mojibake path then COPYed to the right path (but with bad + # pipeline placement for copy) + with mock.patch.object(self.app, 'create_key', mock_create_key): + keys = context.fetch_crypto_keys(key_id={ + 'v': '3', 'path': mangled_path}) + expected_keys = { + 'container': hmac.new( + secrets[None], b'/a/\xc3\xa2\xc2\x98\xc2\x83', + digestmod=hashlib.sha256).digest(), + 'object': hmac.new( + secrets[None], + b'/a/\xc3\xa2\xc2\x98\xc2\x83/\xc3\xa2\xc2\x9d\xc2\x84', + digestmod=hashlib.sha256).digest(), + 'id': {'path': mangled_path, 'v': '3'}, + 'all_ids': [ + {'path': mangled_path, 'v': '3'}, + {'path': mangled_path, 'secret_id': '22', 'v': '3'}]} + self.assertEqual(expected_keys, keys) + self.assertEqual([(mangled_con_path, None), (mangled_path, None)], + calls) + del calls[:] + @mock.patch('swift.common.middleware.crypto.keymaster.readconf') def test_keymaster_config_path(self, mock_readconf): for secret in (os.urandom(32), os.urandom(33), os.urandom(50)):