From 11b9761cdf0fb7a00b1add913a0be31d8e8b93da Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Thu, 18 Jun 2020 10:29:15 -0700 Subject: [PATCH] Rip out pickle support in our memcached client We said this would be going away back in 1.7.0 -- lets actually remove it. Change-Id: I9742dd907abea86da9259740d913924bb1ce73e7 Related-Change: Id7d6d547b103b4f23ebf5be98b88f09ec6027ce4 --- doc/manpages/proxy-server.conf.5 | 15 ----- etc/memcache.conf-sample | 10 --- etc/proxy-server.conf-sample | 12 ---- swift/common/memcached.py | 37 +++-------- swift/common/middleware/memcache.py | 15 ----- test/unit/common/middleware/test_memcache.py | 68 +++----------------- test/unit/common/test_memcached.py | 16 ++--- 7 files changed, 23 insertions(+), 150 deletions(-) diff --git a/doc/manpages/proxy-server.conf.5 b/doc/manpages/proxy-server.conf.5 index 5ebeef43af..1c03197ea8 100644 --- a/doc/manpages/proxy-server.conf.5 +++ b/doc/manpages/proxy-server.conf.5 @@ -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 diff --git a/etc/memcache.conf-sample b/etc/memcache.conf-sample index b375eb4029..f85e49edc6 100644 --- a/etc/memcache.conf-sample +++ b/etc/memcache.conf-sample @@ -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 # diff --git a/etc/proxy-server.conf-sample b/etc/proxy-server.conf-sample index ef49c430f7..44a456219e 100644 --- a/etc/proxy-server.conf-sample +++ b/etc/proxy-server.conf-sample @@ -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 # diff --git a/swift/common/memcached.py b/swift/common/memcached.py index 1a371e58b6..d9cde54093 100644 --- a/swift/common/memcached.py +++ b/swift/common/memcached.py @@ -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 diff --git a/swift/common/middleware/memcache.py b/swift/common/middleware/memcache.py index 4fa2b551ca..562b0b9d88 100644 --- a/swift/common/middleware/memcache.py +++ b/swift/common/middleware/memcache.py @@ -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, diff --git a/test/unit/common/middleware/test_memcache.py b/test/unit/common/middleware/test_memcache.py index a5fe65fe63..8ba0c7e150 100644 --- a/test/unit/common/middleware/test_memcache.py +++ b/test/unit/common/middleware/test_memcache.py @@ -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) diff --git a/test/unit/common/test_memcached.py b/test/unit/common/test_memcached.py index 06e567d868..69371fab6f 100644 --- a/test/unit/common/test_memcached.py +++ b/test/unit/common/test_memcached.py @@ -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: