openstack/swift
Code Issues Proposed changes

Rip out pickle support in our memcached client

Browse Source 
We said this would be going away back in 1.7.0 -- lets actually remove it.

Change-Id: I9742dd907abea86da9259740d913924bb1ce73e7
Related-Change: Id7d6d547b103b4f23ebf5be98b88f09ec6027ce4
This commit is contained in:
Tim Burke 2020-06-18 10:29:15 -07:00
parent b621a6f932
commit 11b9761cdf
7 changed files with 23 additions and 150 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
doc/manpages/proxy-server.conf.5
View File

@ -415,21 +415,6 @@ read from /etc/swift/memcache.conf (see memcache.conf-sample) or lacking that
file, it will default to 127.0.0.1:11211. You can specify multiple servers
separated with commas, as in: 10.1.2.3:11211,10.1.2.4:11211. (IPv6
addresses must follow rfc3986 section-3.2.2, i.e. [::1]:11211)
.IP \fBmemcache_serialization_support\fR
This sets how memcache values are serialized and deserialized:
.RE
.PD 0
.RS 10
.IP "0 = older, insecure pickle serialization"
.IP "1 = json serialization but pickles can still be read (still insecure)"
.IP "2 = json serialization only (secure and the default)"
.RE
.RS 10
To avoid an instant full cache flush, existing installations should upgrade with 0, then set to 1 and reload, then after some time (24 hours) set to 2 and reload. In the future, the ability to use pickle serialization will be removed.
If not set in the configuration file, the value for memcache_serialization_support will be read from /etc/swift/memcache.conf if it exists (see memcache.conf-sample). Otherwise, the default value as indicated above will be used.
.RE
.PD

10
etc/memcache.conf-sample
View File

@ -5,16 +5,6 @@
# (IPv6 addresses must follow rfc3986 section-3.2.2, i.e. [::1]:11211)
# memcache_servers = 127.0.0.1:11211
#
# Sets how memcache values are serialized and deserialized:
# 0 = older, insecure pickle serialization
# 1 = json serialization but pickles can still be read (still insecure)
# 2 = json serialization only (secure and the default)
# To avoid an instant full cache flush, existing installations should
# upgrade with 0, then set to 1 and reload, then after some time (24 hours)
# set to 2 and reload.
# In the future, the ability to use pickle serialization will be removed.
# memcache_serialization_support = 2
#
# Sets the maximum number of connections to each memcached server per worker
# memcache_max_connections = 2
#

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

@ -740,18 +740,6 @@ use = egg:swift#memcache
# follow rfc3986 section-3.2.2, i.e. [::1]:11211)
# memcache_servers = 127.0.0.1:11211
#
# Sets how memcache values are serialized and deserialized:
# 0 = older, insecure pickle serialization
# 1 = json serialization but pickles can still be read (still insecure)
# 2 = json serialization only (secure and the default)
# If not set here, the value for memcache_serialization_support will be read
# from /etc/swift/memcache.conf (see memcache.conf-sample).
# To avoid an instant full cache flush, existing installations should
# upgrade with 0, then set to 1 and reload, then after some time (24 hours)
# set to 2 and reload.
# In the future, the ability to use pickle serialization will be removed.
# memcache_serialization_support = 2
#
# Sets the maximum number of connections to each memcached server per worker
# memcache_max_connections = 2
#

37
swift/common/memcached.py
View File

@ -45,7 +45,6 @@ http://github.com/memcached/memcached/blob/1.4.2/doc/protocol.txt
"""
import six
import six.moves.cPickle as pickle
import json
import logging
import time
@ -66,7 +65,6 @@ IO_TIMEOUT = 2.0
PICKLE_FLAG = 1
JSON_FLAG = 2
NODE_WEIGHT = 50
PICKLE_PROTOCOL = 2
TRY_COUNT = 3
# if ERROR_LIMIT_COUNT errors occur in ERROR_LIMIT_TIME seconds, the server
@ -176,7 +174,7 @@ class MemcacheRing(object):
def __init__(
self, servers, connect_timeout=CONN_TIMEOUT,
io_timeout=IO_TIMEOUT, pool_timeout=POOL_TIMEOUT,
tries=TRY_COUNT, allow_pickle=False, allow_unpickle=False,
tries=TRY_COUNT,
max_conns=2, tls_context=None, logger=None,
error_limit_count=ERROR_LIMIT_COUNT,
error_limit_time=ERROR_LIMIT_TIME,
@ -200,8 +198,6 @@ class MemcacheRing(object):
self._connect_timeout = connect_timeout
self._io_timeout = io_timeout
self._pool_timeout = pool_timeout
self._allow_pickle = allow_pickle
self._allow_unpickle = allow_unpickle or allow_pickle
if logger is None:
self.logger = logging.getLogger()
else:
@ -294,8 +290,7 @@ class MemcacheRing(object):
:param key: key
:param value: value
:param serialize: if True, value is serialized with JSON before sending
to memcache, or with pickle if configured to use
pickle instead of JSON (to avoid cache poisoning)
to memcache
:param time: the time to live
:param min_compress_len: minimum compress length, this parameter was
added to keep the signature compatible with
@ -305,10 +300,7 @@ class MemcacheRing(object):
key = md5hash(key)
timeout = sanitize_timeout(time)
flags = 0
if serialize and self._allow_pickle:
value = pickle.dumps(value, PICKLE_PROTOCOL)
flags |= PICKLE_FLAG
elif serialize:
if serialize:
if isinstance(value, bytes):
value = value.decode('utf8')
value = json.dumps(value).encode('ascii')
@ -344,8 +336,7 @@ class MemcacheRing(object):
def get(self, key):
"""
Gets the object specified by key. It will also unserialize the object
before returning if it is serialized in memcache with JSON, or if it
is pickled and unpickling is allowed.
before returning if it is serialized in memcache with JSON.
:param key: key
:returns: value of the key in memcache
@ -366,11 +357,8 @@ class MemcacheRing(object):
size = int(line[3])
value = fp.read(size)
if int(line[2]) & PICKLE_FLAG:
if self._allow_unpickle:
value = pickle.loads(value)
else:
value = None
elif int(line[2]) & JSON_FLAG:
value = None
if int(line[2]) & JSON_FLAG:
value = json.loads(value)
fp.readline()
line = fp.readline().strip().split()
@ -479,8 +467,7 @@ class MemcacheRing(object):
:param server_key: key to use in determining which server in the ring
is used
:param serialize: if True, value is serialized with JSON before sending
to memcache, or with pickle if configured to use
pickle instead of JSON (to avoid cache poisoning)
to memcache.
:param time: the time to live
:min_compress_len: minimum compress length, this parameter was added
to keep the signature compatible with
@ -493,10 +480,7 @@ class MemcacheRing(object):
for key, value in mapping.items():
key = md5hash(key)
flags = 0
if serialize and self._allow_pickle:
value = pickle.dumps(value, PICKLE_PROTOCOL)
flags |= PICKLE_FLAG
elif serialize:
if serialize:
if isinstance(value, bytes):
value = value.decode('utf8')
value = json.dumps(value).encode('ascii')
@ -540,10 +524,7 @@ class MemcacheRing(object):
size = int(line[3])
value = fp.read(size)
if int(line[2]) & PICKLE_FLAG:
if self._allow_unpickle:
value = pickle.loads(value)
else:
value = None
value = None
elif int(line[2]) & JSON_FLAG:
value = json.loads(value)
responses[line[1]] = value

15
swift/common/middleware/memcache.py
View File

@ -33,7 +33,6 @@ class MemcacheMiddleware(object):
self.app = app
self.logger = get_logger(conf, log_route='memcache')
self.memcache_servers = conf.get('memcache_servers')
serialization_format = conf.get('memcache_serialization_support')
try:
# Originally, while we documented using memcache_max_connections
# we only accepted max_connections
@ -44,7 +43,6 @@ class MemcacheMiddleware(object):
memcache_options = {}
if (not self.memcache_servers
or serialization_format is None
or max_conns <= 0):
path = os.path.join(conf.get('swift_dir', '/etc/swift'),
'memcache.conf')
@ -62,13 +60,6 @@ class MemcacheMiddleware(object):
memcache_conf.get('memcache', 'memcache_servers')
except (NoSectionError, NoOptionError):
pass
if serialization_format is None:
try:
serialization_format = \
memcache_conf.get('memcache',
'memcache_serialization_support')
except (NoSectionError, NoOptionError):
pass
if max_conns <= 0:
try:
new_max_conns = \
@ -111,10 +102,6 @@ class MemcacheMiddleware(object):
self.memcache_servers = '127.0.0.1:11211'
if max_conns <= 0:
max_conns = 2
if serialization_format is None:
serialization_format = 2
else:
serialization_format = int(serialization_format)
self.memcache = MemcacheRing(
[s.strip() for s in self.memcache_servers.split(',') if s.strip()],
@ -122,8 +109,6 @@ class MemcacheMiddleware(object):
pool_timeout=pool_timeout,
tries=tries,
io_timeout=io_timeout,
allow_pickle=(serialization_format == 0),
allow_unpickle=(serialization_format <= 1),
max_conns=max_conns,
tls_context=self.tls_context,
logger=self.logger,

68
test/unit/common/middleware/test_memcache.py
View File

@ -37,7 +37,7 @@ class FakeApp(object):
class ExcConfigParser(object):
def read(self, path):
raise Exception('read called with %r' % path)
raise RuntimeError('read called with %r' % path)
class EmptyConfigParser(object):
@ -47,12 +47,10 @@ class EmptyConfigParser(object):
def get_config_parser(memcache_servers='1.2.3.4:5',
memcache_serialization_support='1',
memcache_max_connections='4',
section='memcache',
item_size_warning_threshold='75'):
_srvs = memcache_servers
_sers = memcache_serialization_support
_maxc = memcache_max_connections
_section = section
_warn_threshold = item_size_warning_threshold
@ -64,8 +62,6 @@ def get_config_parser(memcache_servers='1.2.3.4:5',
raise NoSectionError(section_name)
return {
'memcache_servers': memcache_servers,
'memcache_serialization_support':
memcache_serialization_support,
'memcache_max_connections': memcache_max_connections
}
@ -78,10 +74,6 @@ def get_config_parser(memcache_servers='1.2.3.4:5',
if _srvs == 'error':
raise NoOptionError(option, section)
return _srvs
elif option == 'memcache_serialization_support':
if _sers == 'error':
raise NoOptionError(option, section)
return _sers
elif option in ('memcache_max_connections',
'max_connections'):
if _maxc == 'error':
@ -118,23 +110,14 @@ class TestCacheMiddleware(unittest.TestCase):
with mock.patch.object(memcache, 'ConfigParser', ExcConfigParser):
for d in ({},
{'memcache_servers': '6.7.8.9:10'},
{'memcache_serialization_support': '0'},
{'memcache_max_connections': '30'},
{'item_size_warning_threshold': 75},
{'memcache_servers': '6.7.8.9:10',
'memcache_serialization_support': '0'},
{'memcache_servers': '6.7.8.9:10',
'memcache_max_connections': '30'},
{'memcache_serialization_support': '0',
'memcache_max_connections': '30'},
{'memcache_servers': '6.7.8.9:10',
'item_size_warning_threshold': '75'},
{'memcache_serialization_support': '0',
'item_size_warning_threshold': '75'},
{'item_size_warning_threshold': '75',
'memcache_max_connections': '30'},
):
with self.assertRaises(Exception) as catcher:
with self.assertRaises(RuntimeError) as catcher:
memcache.MemcacheMiddleware(FakeApp(), d)
self.assertEqual(
str(catcher.exception),
@ -142,23 +125,15 @@ class TestCacheMiddleware(unittest.TestCase):
def test_conf_set_no_read(self):
with mock.patch.object(memcache, 'ConfigParser', ExcConfigParser):
exc = None
try:
memcache.MemcacheMiddleware(
FakeApp(), {'memcache_servers': '1.2.3.4:5',
'memcache_serialization_support': '2',
'memcache_max_connections': '30',
'item_size_warning_threshold': '80'})
except Exception as err:
exc = err
self.assertIsNone(exc)
memcache.MemcacheMiddleware(
FakeApp(), {'memcache_servers': '1.2.3.4:5',
'memcache_max_connections': '30',
'item_size_warning_threshold': '80'})
def test_conf_default(self):
with mock.patch.object(memcache, 'ConfigParser', EmptyConfigParser):
app = memcache.MemcacheMiddleware(FakeApp(), {})
self.assertEqual(app.memcache_servers, '127.0.0.1:11211')
self.assertEqual(app.memcache._allow_pickle, False)
self.assertEqual(app.memcache._allow_unpickle, False)
self.assertEqual(
app.memcache._client_cache['127.0.0.1:11211'].max_size, 2)
self.assertEqual(app.memcache.item_size_warning_threshold, -1)
@ -168,12 +143,9 @@ class TestCacheMiddleware(unittest.TestCase):
app = memcache.MemcacheMiddleware(
FakeApp(),
{'memcache_servers': '6.7.8.9:10',
'memcache_serialization_support': '0',
'memcache_max_connections': '5',
'item_size_warning_threshold': '75'})
self.assertEqual(app.memcache_servers, '6.7.8.9:10')
self.assertEqual(app.memcache._allow_pickle, True)
self.assertEqual(app.memcache._allow_unpickle, True)
self.assertEqual(
app.memcache._client_cache['6.7.8.9:10'].max_size, 5)
self.assertEqual(app.memcache.item_size_warning_threshold, 75)
@ -209,20 +181,16 @@ class TestCacheMiddleware(unittest.TestCase):
get_config_parser(section='foobar')):
app = memcache.MemcacheMiddleware(FakeApp(), {})
self.assertEqual(app.memcache_servers, '127.0.0.1:11211')
self.assertEqual(app.memcache._allow_pickle, False)
self.assertEqual(app.memcache._allow_unpickle, False)
self.assertEqual(
app.memcache._client_cache['127.0.0.1:11211'].max_size, 2)
def test_conf_extra_no_option(self):
replacement_parser = get_config_parser(
memcache_servers='error', memcache_serialization_support='error',
memcache_servers='error',
memcache_max_connections='error')
with mock.patch.object(memcache, 'ConfigParser', replacement_parser):
app = memcache.MemcacheMiddleware(FakeApp(), {})
self.assertEqual(app.memcache_servers, '127.0.0.1:11211')
self.assertEqual(app.memcache._allow_pickle, False)
self.assertEqual(app.memcache._allow_unpickle, False)
self.assertEqual(
app.memcache._client_cache['127.0.0.1:11211'].max_size, 2)
@ -231,11 +199,8 @@ class TestCacheMiddleware(unittest.TestCase):
app = memcache.MemcacheMiddleware(
FakeApp(),
{'memcache_servers': '6.7.8.9:10',
'memcache_serialization_support': '0',
'max_connections': '5'})
self.assertEqual(app.memcache_servers, '6.7.8.9:10')
self.assertEqual(app.memcache._allow_pickle, True)
self.assertEqual(app.memcache._allow_unpickle, True)
self.assertEqual(
app.memcache._client_cache['6.7.8.9:10'].max_size, 5)
@ -244,11 +209,8 @@ class TestCacheMiddleware(unittest.TestCase):
app = memcache.MemcacheMiddleware(
FakeApp(),
{'memcache_servers': '6.7.8.9:10',
'memcache_serialization_support': '0',
'max_connections': 'bad42'})
self.assertEqual(app.memcache_servers, '6.7.8.9:10')
self.assertEqual(app.memcache._allow_pickle, True)
self.assertEqual(app.memcache._allow_unpickle, True)
self.assertEqual(
app.memcache._client_cache['6.7.8.9:10'].max_size, 4)
@ -267,8 +229,6 @@ class TestCacheMiddleware(unittest.TestCase):
with mock.patch.object(memcache, 'ConfigParser', get_config_parser()):
app = memcache.MemcacheMiddleware(FakeApp(), {})
self.assertEqual(app.memcache_servers, '1.2.3.4:5')
self.assertEqual(app.memcache._allow_pickle, False)
self.assertEqual(app.memcache._allow_unpickle, True)
self.assertEqual(
app.memcache._client_cache['1.2.3.4:5'].max_size, 4)
@ -277,8 +237,6 @@ class TestCacheMiddleware(unittest.TestCase):
memcache_max_connections='bad42')):
app = memcache.MemcacheMiddleware(FakeApp(), {})
self.assertEqual(app.memcache_servers, '1.2.3.4:5')
self.assertEqual(app.memcache._allow_pickle, False)
self.assertEqual(app.memcache._allow_unpickle, True)
self.assertEqual(
app.memcache._client_cache['1.2.3.4:5'].max_size, 2)
@ -286,11 +244,8 @@ class TestCacheMiddleware(unittest.TestCase):
with mock.patch.object(memcache, 'ConfigParser', get_config_parser()):
app = memcache.MemcacheMiddleware(
FakeApp(),
{'memcache_servers': '6.7.8.9:10',
'memcache_serialization_support': '0'})
{'memcache_servers': '6.7.8.9:10'})
self.assertEqual(app.memcache_servers, '6.7.8.9:10')
self.assertEqual(app.memcache._allow_pickle, True)
self.assertEqual(app.memcache._allow_unpickle, True)
self.assertEqual(
app.memcache._client_cache['6.7.8.9:10'].max_size, 4)
@ -301,20 +256,15 @@ class TestCacheMiddleware(unittest.TestCase):
{'memcache_servers': '6.7.8.9:10',
'memcache_max_connections': '42'})
self.assertEqual(app.memcache_servers, '6.7.8.9:10')
self.assertEqual(app.memcache._allow_pickle, False)
self.assertEqual(app.memcache._allow_unpickle, True)
self.assertEqual(
app.memcache._client_cache['6.7.8.9:10'].max_size, 42)
def test_filter_factory(self):
factory = memcache.filter_factory({'max_connections': '3'},
memcache_servers='10.10.10.10:10',
memcache_serialization_support='1')
memcache_servers='10.10.10.10:10')
thefilter = factory('myapp')
self.assertEqual(thefilter.app, 'myapp')
self.assertEqual(thefilter.memcache_servers, '10.10.10.10:10')
self.assertEqual(thefilter.memcache._allow_pickle, False)
self.assertEqual(thefilter.memcache._allow_unpickle, True)
self.assertEqual(
thefilter.memcache._client_cache['10.10.10.10:10'].max_size, 3)

16
test/unit/common/test_memcached.py
View File

@ -767,24 +767,18 @@ class TestMemcached(unittest.TestCase):
def test_serialization(self):
memcache_client = memcached.MemcacheRing(['1.2.3.4:11211'],
allow_pickle=True,
logger=self.logger)
mock = MockMemcached()
memcache_client._client_cache['1.2.3.4:11211'] = MockedMemcachePool(
[(mock, mock)] * 2)
memcache_client.set('some_key', [1, 2, 3])
self.assertEqual(memcache_client.get('some_key'), [1, 2, 3])
memcache_client._allow_pickle = False
memcache_client._allow_unpickle = True
self.assertEqual(memcache_client.get('some_key'), [1, 2, 3])
memcache_client._allow_unpickle = False
self.assertEqual(len(mock.cache), 1)
key = next(iter(mock.cache))
self.assertEqual(mock.cache[key][0], b'2') # JSON_FLAG
# Pretend we've got some really old pickle data in there
mock.cache[key] = (b'1',) + mock.cache[key][1:]
self.assertIsNone(memcache_client.get('some_key'))
memcache_client.set('some_key', [1, 2, 3])
self.assertEqual(memcache_client.get('some_key'), [1, 2, 3])
memcache_client._allow_unpickle = True
self.assertEqual(memcache_client.get('some_key'), [1, 2, 3])
memcache_client._allow_pickle = True
self.assertEqual(memcache_client.get('some_key'), [1, 2, 3])
def test_connection_pooling(self):
with patch('swift.common.memcached.socket') as mock_module: