Multi-key KMIP keymaster

Now that the trivial keymaster supports multiple keys, let's do
something similar for the KMIP keymaster. Additional keys are
configured as:

    key_id_<secret_id> = <KMIP unique identifier>

While it might be tempting to use the unique identifier directly as the
secret_id, the added indirection allows operators to move keys between
different backends, which may cause different identifiers to be issued.

As with the trivial keymaster, the key to use for PUTs and POSTs is
specified with:

    active_root_secret_id = <secret_id>

Change-Id: Ie52508e47d15ec5c4e96902d3c9f5f282d275683

swift/common/middleware/crypto/kmip_keymaster.py

@@ -38,6 +38,8 @@ and add a new filter section::
     [filter:kmip_keymaster]
     use = egg:swift#kmip_keymaster
     key_id = <unique id of secret to be fetched from the KMIP service>
+    key_id_<secret_id> = <unique id of additional secret to be fetched>
+    active_root_secret_id = <secret_id to be used for new encryptions>
     host = <KMIP server host>
     port = <KMIP server port>
     certfile = /path/to/client/cert.pem
@@ -46,12 +48,27 @@ and add a new filter section::
     username = <KMIP username>
     password = <KMIP password>
 
-Apart from ``use`` and ``key_id`` the options are as defined for a PyKMIP
+Apart from ``use``, ``key_id*``, ``active_root_secret_id`` the options are
-client. The authoritative definition of these options can be found at
+as defined for a PyKMIP client. The authoritative definition of these options
-`https://pykmip.readthedocs.io/en/latest/client.html`_
+can be found at `https://pykmip.readthedocs.io/en/latest/client.html`_
 
-The value of the ``key_id`` option should be the unique identifier for a secret
+The value of each ``key_id*`` option should be a unique identifier for a secret
-that will be retrieved from the KMIP service.
+to be retrieved from the KMIP service. Any of these secrets may be used for
+*decryption*.
+
+The value of the ``active_root_secret_id`` option should be the ``secret_id``
+for the secret that should be used for all new *encryption*. If not specified,
+the ``key_id`` secret will be used.
+
+.. note::
+
+    To ensure there is no loss of data availability, deploying a new key to
+    your cluster requires a two-stage config change. First, add the new key
+    to the ``key_id_<secret_id>`` option and restart the proxy-server. Do this
+    for all proxies. Next, set the ``active_root_secret_id`` option to the
+    new secret id and restart the proxy. Again, do this for all proxies. This
+    process ensures that all proxies will have the new key available for
+    *decryption* before any proxy uses it for *encryption*.
 
 The keymaster configuration can alternatively be defined in a separate config
 file by using the ``keymaster_config_path`` option::
@@ -67,6 +84,9 @@ example::
 
     [kmip_keymaster]
     key_id = 1234567890
+    key_id_foo = 2468024680
+    key_id_bar = 1357913579
+    active_root_secret_id = foo
     host = 127.0.0.1
     port = 5696
     certfile = /etc/swift/kmip_client.crt
@@ -81,7 +101,7 @@ class KmipKeyMaster(keymaster.KeyMaster):
     log_route = 'kmip_keymaster'
     keymaster_opts = ('host', 'port', 'certfile', 'keyfile',
                       'ca_certs', 'username', 'password',
-                      'active_root_secret_id', 'key_id')
+                      'active_root_secret_id', 'key_id*')
     keymaster_conf_section = 'kmip_keymaster'
 
     def _get_root_secret(self, conf):
@@ -96,25 +116,36 @@ class KmipKeyMaster(keymaster.KeyMaster):
                 'keymaster_config_path option in the proxy server config to '
                 'specify a config file.')
 
-        key_id = conf.get('key_id')
-        if not key_id:
-            raise ValueError('key_id option is required')
-
         kmip_logger = logging.getLogger('kmip')
         for handler in self.logger.logger.handlers:
             kmip_logger.addHandler(handler)
 
+        multikey_opts = self._load_multikey_opts(conf, 'key_id')
+        if not multikey_opts:
+            raise ValueError('key_id option is required')
+        kmip_to_secret = {}
+        root_secrets = {}
         with ProxyKmipClient(
             config=section,
             config_file=conf['__file__']
         ) as client:
-            secret = client.get(key_id)
+            for opt, secret_id, kmip_id in multikey_opts:
-        if (secret.cryptographic_algorithm.name,
+                if kmip_id in kmip_to_secret:
-                secret.cryptographic_length) != ('AES', 256):
+                    # Save some round trips if there are multiple
-            raise ValueError('Expected an AES-256 key, not %s-%d' % (
+                    # secret_ids for a single kmip_id
-                secret.cryptographic_algorithm.name,
+                    root_secrets[secret_id] = root_secrets[
-                secret.cryptographic_length))
+                        kmip_to_secret[kmip_id]]
-        return secret.value
+                    continue
+                secret = client.get(kmip_id)
+                algo = secret.cryptographic_algorithm.name
+                length = secret.cryptographic_length
+                if (algo, length) != ('AES', 256):
+                    raise ValueError(
+                        'Expected key %s to be an AES-256 key, not %s-%d' % (
+                            kmip_id, algo, length))
+                root_secrets[secret_id] = secret.value
+                kmip_to_secret.setdefault(kmip_id, secret_id)
+        return root_secrets
 
 
 def filter_factory(global_conf, **local_conf):

test/unit/common/middleware/crypto/test_kmip_keymaster.py

@@ -29,13 +29,14 @@ from swift.common.middleware.crypto.kmip_keymaster import KmipKeyMaster
 
 
 class MockProxyKmipClient(object):
-    def __init__(self, secret):
+    def __init__(self, secrets, calls, kwargs):
-        self.secret = secret
+        calls.append(('__init__', kwargs))
-        self.uid = None
+        self.secrets = secrets
+        self.calls = calls
 
     def get(self, uid):
-        self.uid = uid
+        self.calls.append(('get', uid))
-        return self.secret
+        return self.secrets[uid]
 
     def __enter__(self):
         return self
@@ -53,11 +54,11 @@ def create_secret(algorithm_name, length, value):
     return secret
 
 
-def create_mock_client(secret, calls):
+def create_mock_client(secrets, calls):
     def mock_client(*args, **kwargs):
-        client = MockProxyKmipClient(secret)
+        if args:
-        calls.append({'args': args, 'kwargs': kwargs, 'client': client})
+            raise Exception('unexpected args provided: %r' % (args,))
-        return client
+        return MockProxyKmipClient(secrets, calls, kwargs)
     return mock_client
 
 
@@ -73,18 +74,62 @@ class TestKmipKeymaster(unittest.TestCase):
         conf = {'__file__': '/etc/swift/proxy-server.conf',
                 '__name__': 'filter:kmip_keymaster',
                 'key_id': '1234'}
-        secret = create_secret('AES', 256, b'x' * 32)
+        secrets = {'1234': create_secret('AES', 256, b'x' * 32)}
         calls = []
         klass = 'swift.common.middleware.crypto.kmip_keymaster.ProxyKmipClient'
-        with mock.patch(klass, create_mock_client(secret, calls)):
+        with mock.patch(klass, create_mock_client(secrets, calls)):
             km = KmipKeyMaster(None, conf)
 
-        self.assertEqual(secret.value, km.root_secret)
+        self.assertEqual({None: b'x' * 32}, km._root_secrets)
+        self.assertEqual(None, km.active_secret_id)
         self.assertIsNone(km.keymaster_config_path)
-        self.assertEqual({'config_file': '/etc/swift/proxy-server.conf',
+        self.assertEqual(calls, [
-                          'config': 'filter:kmip_keymaster'},
+            ('__init__', {'config_file': '/etc/swift/proxy-server.conf',
-                         calls[0]['kwargs'])
+                          'config': 'filter:kmip_keymaster'}),
-        self.assertEqual('1234', calls[0]['client'].uid)
+            ('get', '1234'),
+        ])
+
+    def test_multikey_config_in_filter_section(self):
+        conf = {'__file__': '/etc/swift/proxy-server.conf',
+                '__name__': 'filter:kmip_keymaster',
+                'key_id': '1234',
+                'key_id_xyzzy': 'foobar',
+                'key_id_alt_secret_id': 'foobar',
+                'active_root_secret_id': 'xyzzy'}
+        secrets = {'1234': create_secret('AES', 256, b'x' * 32),
+                   'foobar': create_secret('AES', 256, b'y' * 32)}
+        calls = []
+        klass = 'swift.common.middleware.crypto.kmip_keymaster.ProxyKmipClient'
+        with mock.patch(klass, create_mock_client(secrets, calls)):
+            km = KmipKeyMaster(None, conf)
+
+        self.assertEqual({None: b'x' * 32, 'xyzzy': b'y' * 32,
+                          'alt_secret_id': b'y' * 32},
+                         km._root_secrets)
+        self.assertEqual('xyzzy', km.active_secret_id)
+        self.assertIsNone(km.keymaster_config_path)
+        self.assertEqual(calls, [
+            ('__init__', {'config_file': '/etc/swift/proxy-server.conf',
+                          'config': 'filter:kmip_keymaster'}),
+            ('get', '1234'),
+            ('get', 'foobar'),
+        ])
+
+    def test_bad_active_key(self):
+        conf = {'__file__': '/etc/swift/proxy-server.conf',
+                '__name__': 'filter:kmip_keymaster',
+                'key_id': '1234',
+                'key_id_xyzzy': 'foobar',
+                'active_root_secret_id': 'unknown'}
+        secrets = {'1234': create_secret('AES', 256, b'x' * 32),
+                   'foobar': create_secret('AES', 256, b'y' * 32)}
+        calls = []
+        klass = 'swift.common.middleware.crypto.kmip_keymaster.ProxyKmipClient'
+        with mock.patch(klass, create_mock_client(secrets, calls)), \
+                self.assertRaises(ValueError) as raised:
+            KmipKeyMaster(None, conf)
+        self.assertEqual('No secret loaded for active_root_secret_id unknown',
+                         str(raised.exception))
 
     def test_config_in_separate_file(self):
         km_conf = """
@@ -98,17 +143,48 @@ class TestKmipKeymaster(unittest.TestCase):
         conf = {'__file__': '/etc/swift/proxy-server.conf',
                 '__name__': 'filter:kmip_keymaster',
                 'keymaster_config_path': km_config_file}
-        secret = create_secret('AES', 256, b'x' * 32)
+        secrets = {'4321': create_secret('AES', 256, b'x' * 32)}
         calls = []
         klass = 'swift.common.middleware.crypto.kmip_keymaster.ProxyKmipClient'
-        with mock.patch(klass, create_mock_client(secret, calls)):
+        with mock.patch(klass, create_mock_client(secrets, calls)):
             km = KmipKeyMaster(None, conf)
-        self.assertEqual(secret.value, km.root_secret)
+        self.assertEqual({None: b'x' * 32}, km._root_secrets)
+        self.assertEqual(None, km.active_secret_id)
         self.assertEqual(km_config_file, km.keymaster_config_path)
-        self.assertEqual({'config_file': km_config_file,
+        self.assertEqual(calls, [
-                          'config': 'kmip_keymaster'},
+            ('__init__', {'config_file': km_config_file,
-                         calls[0]['kwargs'])
+                          'config': 'kmip_keymaster'}),
-        self.assertEqual('4321', calls[0]['client'].uid)
+            ('get', '4321')])
+
+    def test_multikey_config_in_separate_file(self):
+        km_conf = """
+        [kmip_keymaster]
+        key_id = 4321
+        key_id_secret_id = another id
+        active_root_secret_id = secret_id
+        """
+        km_config_file = os.path.join(self.tempdir, 'km.conf')
+        with open(km_config_file, 'wb') as fd:
+            fd.write(dedent(km_conf))
+
+        conf = {'__file__': '/etc/swift/proxy-server.conf',
+                '__name__': 'filter:kmip_keymaster',
+                'keymaster_config_path': km_config_file}
+        secrets = {'4321': create_secret('AES', 256, b'x' * 32),
+                   'another id': create_secret('AES', 256, b'y' * 32)}
+        calls = []
+        klass = 'swift.common.middleware.crypto.kmip_keymaster.ProxyKmipClient'
+        with mock.patch(klass, create_mock_client(secrets, calls)):
+            km = KmipKeyMaster(None, conf)
+        self.assertEqual({None: b'x' * 32, 'secret_id': b'y' * 32},
+                         km._root_secrets)
+        self.assertEqual('secret_id', km.active_secret_id)
+        self.assertEqual(km_config_file, km.keymaster_config_path)
+        self.assertEqual(calls, [
+            ('__init__', {'config_file': km_config_file,
+                          'config': 'kmip_keymaster'}),
+            ('get', '4321'),
+            ('get', 'another id')])
 
     def test_proxy_server_conf_dir(self):
         proxy_server_conf_dir = os.path.join(self.tempdir, 'proxy_server.d')
@@ -139,49 +215,52 @@ class TestKmipKeymaster(unittest.TestCase):
         conf = {'__file__': proxy_server_conf_dir,
                 '__name__': 'filter:kmip_keymaster',
                 'keymaster_config_path': km_config_file}
-        secret = create_secret('AES', 256, b'x' * 32)
+        secrets = {'789': create_secret('AES', 256, b'x' * 32)}
         calls = []
         klass = 'swift.common.middleware.crypto.kmip_keymaster.ProxyKmipClient'
-        with mock.patch(klass, create_mock_client(secret, calls)):
+        with mock.patch(klass, create_mock_client(secrets, calls)):
             km = KmipKeyMaster(None, conf)
-        self.assertEqual(secret.value, km.root_secret)
+        self.assertEqual({None: b'x' * 32}, km._root_secrets)
+        self.assertEqual(None, km.active_secret_id)
         self.assertEqual(km_config_file, km.keymaster_config_path)
-        self.assertEqual({'config_file': km_config_file,
+        self.assertEqual(calls, [
-                          'config': 'kmip_keymaster'},
+            ('__init__', {'config_file': km_config_file,
-                         calls[0]['kwargs'])
+                          'config': 'kmip_keymaster'}),
-        self.assertEqual('789', calls[0]['client'].uid)
+            ('get', '789')])
 
     def test_bad_key_length(self):
         conf = {'__file__': '/etc/swift/proxy-server.conf',
                 '__name__': 'filter:kmip_keymaster',
                 'key_id': '1234'}
-        secret = create_secret('AES', 128, b'x' * 16)
+        secrets = {'1234': create_secret('AES', 128, b'x' * 16)}
         calls = []
         klass = 'swift.common.middleware.crypto.kmip_keymaster.ProxyKmipClient'
-        with mock.patch(klass, create_mock_client(secret, calls)):
+        with mock.patch(klass, create_mock_client(secrets, calls)):
             with self.assertRaises(ValueError) as cm:
                 KmipKeyMaster(None, conf)
-        self.assertIn('Expected an AES-256 key', str(cm.exception))
+        self.assertIn('Expected key 1234 to be an AES-256 key',
-        self.assertEqual({'config_file': '/etc/swift/proxy-server.conf',
+                      str(cm.exception))
-                          'config': 'filter:kmip_keymaster'},
+        self.assertEqual(calls, [
-                         calls[0]['kwargs'])
+            ('__init__', {'config_file': '/etc/swift/proxy-server.conf',
-        self.assertEqual('1234', calls[0]['client'].uid)
+                          'config': 'filter:kmip_keymaster'}),
+            ('get', '1234')])
 
     def test_bad_key_algorithm(self):
         conf = {'__file__': '/etc/swift/proxy-server.conf',
                 '__name__': 'filter:kmip_keymaster',
                 'key_id': '1234'}
-        secret = create_secret('notAES', 256, b'x' * 32)
+        secrets = {'1234': create_secret('notAES', 256, b'x' * 32)}
         calls = []
         klass = 'swift.common.middleware.crypto.kmip_keymaster.ProxyKmipClient'
-        with mock.patch(klass, create_mock_client(secret, calls)):
+        with mock.patch(klass, create_mock_client(secrets, calls)):
             with self.assertRaises(ValueError) as cm:
                 KmipKeyMaster(None, conf)
-        self.assertIn('Expected an AES-256 key', str(cm.exception))
+        self.assertIn('Expected key 1234 to be an AES-256 key',
-        self.assertEqual({'config_file': '/etc/swift/proxy-server.conf',
+                      str(cm.exception))
-                          'config': 'filter:kmip_keymaster'},
+        self.assertEqual(calls, [
-                         calls[0]['kwargs'])
+            ('__init__', {'config_file': '/etc/swift/proxy-server.conf',
-        self.assertEqual('1234', calls[0]['client'].uid)
+                          'config': 'filter:kmip_keymaster'}),
+            ('get', '1234')])
 
     def test_missing_key_id(self):
         conf = {'__file__': '/etc/swift/proxy-server.conf',