openstack/swift
Code Issues Proposed changes

py3: Work with proper native string paths in crypto meta

Browse Source 
Previously, we would work with these paths as WSGI strings -- this would
work fine when all data were read and written on the same major version
of Python, but fail pretty badly during and after upgrading Python.

In particular, if a py3 proxy-server tried to read existing data that
was written down by a py2 proxy-server, it would hit an error and
respond 500. Worse, if an un-upgraded py2 proxy tried to read data that
was freshly-written by a py3 proxy, it would serve corrupt data back to
the client (including a corrupt/invalid ETag and Content-Type).

Now, ensure that both py2 and py3 write down paths as native strings.
Make an effort to still work with WSGI-string metadata, though it can be
ambiguous as to whether a string is a WSGI string or not. The heuristic
used is if

 * the path from metadata does not match the (native-string) request
   path and
 * the path from metadata (when interpreted as a WSGI string) can be
   "un-wsgi-fied" without any encode/decode errors and
 * the native-string path from metadata *does* match the native-string
   request path

then trust the path from the request. By contrast, we usually prefer the
path from metadata in case there was a pipeline misconfiguration (see
related bug).

Add the ability to read and write a new, unambiguous version of metadata
that always has the path as a native string. To support rolling
upgrades, a new config option is added: meta_version_to_write. This
defaults to 2 to support rolling upgrades without configuration changes,
but the default may change to 3 in a future release.

UpgradeImpact
=============
When upgrading from Swift 2.20.0 or Swift 2.19.1 or earlier, set

    meta_version_to_write = 1

in your keymaster's configuration. Regardless of prior Swift version, set

    meta_version_to_write = 3

after upgrading all proxy servers.

When switching from Python 2 to Python 3, first upgrade Swift while on
Python 2, then upgrade to Python 3.

Change-Id: I00c6693c42c1a0220b64d8016d380d5985339658
Closes-Bug: #1888037
Related-Bug: #1813725
This commit is contained in:
Tim Burke
2020-07-20 14:18:33 -07:00
parent d6399b32e7
commit 7d429318dd
4 changed files with 310 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
etc/keymaster.conf-sample
View File

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

12
etc/proxy-server.conf-sample
View File

@@ -1113,6 +1113,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

61
swift/common/middleware/crypto/keymaster.py
View File

@@ -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):
<path_key> = HMAC_SHA256(<root_secret>, <path>)
"""
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)
@@ -219,6 +252,11 @@ class BaseKeyMaster(object):
raise ValueError('Secret with id %s is %s, not bytes' % (
secret_id, type(secret)))
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
@@ -265,13 +303,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:
@@ -296,8 +336,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):

227
test/unit/common/middleware/crypto/test_keymaster.py
View File

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