Merge "Rip out pickle support in our memcached client"

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
 

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
 #

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
 #

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
+                          to memcache
-                          pickle instead of JSON (to avoid cache poisoning)
         :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:
+        if serialize:
-            value = pickle.dumps(value, PICKLE_PROTOCOL)
-            flags |= PICKLE_FLAG
-        elif 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
+        before returning if it is serialized in memcache with JSON.
-        is pickled and unpickling is allowed.
 
         :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:
+                            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
+                          to memcache.
-                          pickle instead of JSON (to avoid cache poisoning)
         :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:
+            if serialize:
-                value = pickle.dumps(value, PICKLE_PROTOCOL)
-                flags |= PICKLE_FLAG
-            elif serialize:
                 if isinstance(value, bytes):
                     value = value.decode('utf8')
                 value = json.dumps(value).encode('ascii')
@@ -540,9 +524,6 @@ 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 = json.loads(value)

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,

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)
 
     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_servers': '6.7.8.9:10'})
-                 'memcache_serialization_support': '0'})
         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_servers='10.10.10.10:10')
-                                          memcache_serialization_support='1')
         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)
 

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
+        self.assertEqual(len(mock.cache), 1)
-        memcache_client._allow_unpickle = True
+        key = next(iter(mock.cache))
-        self.assertEqual(memcache_client.get('some_key'), [1, 2, 3])
+        self.assertEqual(mock.cache[key][0], b'2')  # JSON_FLAG
-        memcache_client._allow_unpickle = False
+        # 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: