Move KMIP conf validation out of _get_root_secret

Related-Change-Id: Iddc0f333861b6c1f81e181f006cd592b5eb6ea17

Change-Id: I3d43b738de3947c33d4607225227f0116aa4cea1

swift/common/middleware/crypto/kmip_keymaster.py

@@ -105,7 +105,8 @@ class KmipKeyMaster(keymaster.BaseKeyMaster):
                       'active_root_secret_id', 'key_id*')
     keymaster_conf_section = 'kmip_keymaster'
 
-    def _get_root_secret(self, conf):
+    def _load_keymaster_config_file(self, conf):
+        conf = super(KmipKeyMaster, self)._load_keymaster_config_file(conf)
         if self.keymaster_config_path:
             section = self.keymaster_conf_section
         else:
@@ -119,6 +120,8 @@ class KmipKeyMaster(keymaster.BaseKeyMaster):
                 'keymaster_config_path option in the proxy server config to '
                 'specify a config file.')
 
+        # Make sure we've got the kmip log handler set up before
+        # we instantiate a client
         kmip_logger = logging.getLogger('kmip')
         for handler in self.logger.logger.handlers:
             kmip_logger.addHandler(handler)
@@ -135,15 +138,17 @@ class KmipKeyMaster(keymaster.BaseKeyMaster):
         ):
             logging.getLogger(name).addFilter(debug_filter)
 
-        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(
+        self.proxy_kmip_client = ProxyKmipClient(
             config=section,
             config_file=conf['__file__']
-        ) as client:
+        )
+        return conf
+
+    def _get_root_secret(self, conf):
+        multikey_opts = self._load_multikey_opts(conf, 'key_id')
+        kmip_to_secret = {}
+        root_secrets = {}
+        with self.proxy_kmip_client as client:
             for opt, secret_id, kmip_id in multikey_opts:
                 if kmip_id in kmip_to_secret:
                     # Save some round trips if there are multiple

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

@@ -29,6 +29,10 @@ sys.modules['kmip.pie.client'] = mock.Mock()
 from swift.common.middleware.crypto.kmip_keymaster import KmipKeyMaster
 
 
+KMIP_CLIENT_CLASS = \
+    'swift.common.middleware.crypto.kmip_keymaster.ProxyKmipClient'
+
+
 class MockProxyKmipClient(object):
     def __init__(self, secrets, calls, kwargs):
         calls.append(('__init__', kwargs))
@@ -86,8 +90,7 @@ class TestKmipKeymaster(unittest.TestCase):
                 'key_id': '1234'}
         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(secrets, calls)):
+        with mock.patch(KMIP_CLIENT_CLASS, create_mock_client(secrets, calls)):
             km = KmipKeyMaster(None, conf)
 
         self.assertEqual({None: b'x' * 32}, km._root_secrets)
@@ -109,8 +112,7 @@ class TestKmipKeymaster(unittest.TestCase):
         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)):
+        with mock.patch(KMIP_CLIENT_CLASS, create_mock_client(secrets, calls)):
             km = KmipKeyMaster(None, conf)
 
         self.assertEqual({None: b'x' * 32, 'xyzzy': b'y' * 32,
@@ -134,8 +136,8 @@ class TestKmipKeymaster(unittest.TestCase):
         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)), \
+        with mock.patch(KMIP_CLIENT_CLASS,
+                        create_mock_client(secrets, calls)), \
                 self.assertRaises(ValueError) as raised:
             KmipKeyMaster(None, conf)
         self.assertEqual('No secret loaded for active_root_secret_id unknown',
@@ -155,8 +157,7 @@ class TestKmipKeymaster(unittest.TestCase):
                 'keymaster_config_path': km_config_file}
         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(secrets, calls)):
+        with mock.patch(KMIP_CLIENT_CLASS, create_mock_client(secrets, calls)):
             km = KmipKeyMaster(None, conf)
         self.assertEqual({None: b'x' * 32}, km._root_secrets)
         self.assertEqual(None, km.active_secret_id)
@@ -183,8 +184,7 @@ class TestKmipKeymaster(unittest.TestCase):
         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)):
+        with mock.patch(KMIP_CLIENT_CLASS, create_mock_client(secrets, calls)):
             km = KmipKeyMaster(None, conf)
         self.assertEqual({None: b'x' * 32, 'secret_id': b'y' * 32},
                          km._root_secrets)
@@ -227,8 +227,7 @@ class TestKmipKeymaster(unittest.TestCase):
                 'keymaster_config_path': km_config_file}
         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(secrets, calls)):
+        with mock.patch(KMIP_CLIENT_CLASS, create_mock_client(secrets, calls)):
             km = KmipKeyMaster(None, conf)
         self.assertEqual({None: b'x' * 32}, km._root_secrets)
         self.assertEqual(None, km.active_secret_id)
@@ -245,10 +244,10 @@ class TestKmipKeymaster(unittest.TestCase):
                 'key_id': '1234'}
         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(secrets, calls)):
-            with self.assertRaises(ValueError) as cm:
-                KmipKeyMaster(None, conf)
+        with mock.patch(KMIP_CLIENT_CLASS,
+                        create_mock_client(secrets, calls)), \
+                self.assertRaises(ValueError) as cm:
+            KmipKeyMaster(None, conf)
         self.assertIn('Expected key 1234 to be an AES-256 key',
                       str(cm.exception))
         self.assertEqual(calls, [
@@ -262,10 +261,10 @@ class TestKmipKeymaster(unittest.TestCase):
                 'key_id': '1234'}
         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(secrets, calls)):
-            with self.assertRaises(ValueError) as cm:
-                KmipKeyMaster(None, conf)
+        with mock.patch(KMIP_CLIENT_CLASS,
+                        create_mock_client(secrets, calls)), \
+                self.assertRaises(ValueError) as cm:
+            KmipKeyMaster(None, conf)
         self.assertIn('Expected key 1234 to be an AES-256 key',
                       str(cm.exception))
         self.assertEqual(calls, [
@@ -276,9 +275,18 @@ class TestKmipKeymaster(unittest.TestCase):
     def test_missing_key_id(self):
         conf = {'__file__': '/etc/swift/proxy-server.conf',
                 '__name__': 'kmip_keymaster'}
-        with self.assertRaises(ValueError) as cm:
+        secrets = {}
+        calls = []
+        with mock.patch(KMIP_CLIENT_CLASS,
+                        create_mock_client(secrets, calls)), \
+                self.assertRaises(ValueError) as cm:
             KmipKeyMaster(None, conf)
-        self.assertIn('key_id option is required', str(cm.exception))
+        self.assertEqual('No secret loaded for active_root_secret_id None',
+                         str(cm.exception))
+        # We make the client, but never use it
+        self.assertEqual(calls, [
+            ('__init__', {'config_file': '/etc/swift/proxy-server.conf',
+                          'config': 'filter:kmip_keymaster'})])
 
     def test_logger_manipulations(self):
         root_logger = logging.getLogger()
@@ -290,7 +298,11 @@ class TestKmipKeymaster(unittest.TestCase):
 
             conf = {'__file__': '/etc/swift/proxy-server.conf',
                     '__name__': 'kmip_keymaster'}
-            with self.assertRaises(ValueError):
+            secrets = {}
+            calls = []
+            with mock.patch(KMIP_CLIENT_CLASS,
+                            create_mock_client(secrets, calls)), \
+                    self.assertRaises(ValueError):
                 # missing key_id, as above, but that's not the interesting bit
                 KmipKeyMaster(None, conf)