[hopem,r=]

Ensure ssl certs always synced.
Partially-Closes-Bug: 1520339

hooks/keystone_context.py

@@ -189,7 +189,7 @@ class KeystoneContext(context.OSContextGenerator):
     def __call__(self):
         from keystone_utils import (
             api_port, set_admin_token, endpoint_url, resolve_address,
-            PUBLIC, ADMIN, PKI_CERTS_DIR, SSH_USER, ensure_permissions,
+            PUBLIC, ADMIN, PKI_CERTS_DIR, ensure_pki_cert_paths,
         )
         ctxt = {}
         ctxt['token'] = set_admin_token(config('admin-token'))
@@ -219,32 +219,16 @@ class KeystoneContext(context.OSContextGenerator):
 
         enable_pki = config('enable-pki')
         if enable_pki and bool_from_string(enable_pki):
-            ctxt['signing'] = True
+            log("Enabling PKI", level=DEBUG)
             ctxt['token_provider'] = 'pki'
 
-        if 'token_provider' in ctxt:
+        ensure_pki_cert_paths()
-            log("Configuring PKI token cert paths", level=DEBUG)
+        certs = os.path.join(PKI_CERTS_DIR, 'certs')
-            certs = os.path.join(PKI_CERTS_DIR, 'certs')
+        privates = os.path.join(PKI_CERTS_DIR, 'privates')
-            privates = os.path.join(PKI_CERTS_DIR, 'privates')
+        ctxt.update({'certfile': os.path.join(certs, 'signing_cert.pem'),
-            for path in [PKI_CERTS_DIR, certs, privates]:
+                     'keyfile': os.path.join(privates, 'signing_key.pem'),
-                perms = 0o755
+                     'ca_certs': os.path.join(certs, 'ca.pem'),
-                if not os.path.isdir(path):
+                     'ca_key': os.path.join(certs, 'ca_key.pem')})
-                    mkdir(path=path, owner=SSH_USER, group='keystone',
-                          perms=perms)
-                else:
-                    # Ensure accessible by ssh user and group (for sync).
-                    ensure_permissions(path, user=SSH_USER,
-                                       group='keystone', perms=perms)
-
-            signing_paths = {'certfile': os.path.join(certs,
-                                                      'signing_cert.pem'),
-                             'keyfile': os.path.join(privates,
-                                                     'signing_key.pem'),
-                             'ca_certs': os.path.join(certs, 'ca.pem'),
-                             'ca_key': os.path.join(certs, 'ca_key.pem')}
-
-            for key, val in signing_paths.iteritems():
-                ctxt[key] = val
 
         # Base endpoint URL's which are used in keystone responses
         # to unauthenticated requests to redirect clients to the
@@ -255,6 +239,7 @@ class KeystoneContext(context.OSContextGenerator):
         ctxt['admin_endpoint'] = endpoint_url(
             resolve_address(ADMIN),
             api_port('keystone-admin')).rstrip('v2.0')
+
         return ctxt
 
 

hooks/keystone_hooks.py

@@ -75,7 +75,6 @@ from keystone_utils import (
     clear_ssl_synced_units,
     is_db_initialised,
     update_certs_if_available,
-    is_pki_enabled,
     ensure_ssl_dir,
     ensure_pki_dir_permissions,
     ensure_permissions,
@@ -84,6 +83,7 @@ from keystone_utils import (
     ensure_ssl_dirs,
     REQUIRED_INTERFACES,
     check_optional_relations,
+    ensure_pki_cert_paths,
 )
 
 from charmhelpers.contrib.hahelpers.cluster import (
@@ -177,8 +177,7 @@ def config_changed_postupgrade():
     update_nrpe_config()
     CONFIGS.write_all()
 
-    if is_pki_enabled():
+    initialise_pki()
-        initialise_pki()
 
     update_all_identity_relation_units()
 
@@ -194,11 +193,14 @@ def config_changed_postupgrade():
 
 @synchronize_ca_if_changed(fatal=True)
 def initialise_pki():
-    """Create certs and keys required for PKI token signing.
+    """Create certs and keys required for token signing.
+
+    Used for PKI and signing token revocation list.
 
     NOTE: keystone.conf [signing] section must be up-to-date prior to
           executing this.
     """
+    ensure_pki_cert_paths()
     if not peer_units() or is_ssl_cert_master():
         log("Ensuring PKI token certs created", level=DEBUG)
         cmd = ['keystone-manage', 'pki_setup', '--keystone-user', 'keystone',
@@ -373,44 +375,36 @@ def send_ssl_sync_request():
     Note the we do nothing if the setting is already applied.
     """
     unit = local_unit().replace('/', '-')
-    count = 0
+    # Start with core config (e.g. used for signing revoked token list)
+    ssl_config = 0b1
 
     use_https = config('use-https')
     if use_https and bool_from_string(use_https):
-        count += 1
+        ssl_config ^= 0b10
 
     https_service_endpoints = config('https-service-endpoints')
     if (https_service_endpoints and
             bool_from_string(https_service_endpoints)):
-        count += 2
+        ssl_config ^= 0b100
 
     enable_pki = config('enable-pki')
     if enable_pki and bool_from_string(enable_pki):
-        count += 3
+        ssl_config ^= 0b1000
 
     key = 'ssl-sync-required-%s' % (unit)
-    settings = {key: count}
+    settings = {key: ssl_config}
 
-    # If all ssl is disabled ensure this is set to 0 so that cluster hook runs
+    prev = 0b0
-    # and endpoints are updated.
-    if not count:
-        log("Setting %s=%s" % (key, count), level=DEBUG)
-        for rid in relation_ids('cluster'):
-            relation_set(relation_id=rid, relation_settings=settings)
-
-        return
-
-    prev = 0
     rid = None
     for rid in relation_ids('cluster'):
         for unit in related_units(rid):
-            _prev = relation_get(rid=rid, unit=unit, attribute=key) or 0
+            _prev = relation_get(rid=rid, unit=unit, attribute=key) or 0b0
             if _prev and _prev > prev:
-                prev = _prev
+                prev = bin(_prev)
 
-    if rid and prev < count:
+    if rid and prev ^ ssl_config:
         clear_ssl_synced_units()
-        log("Setting %s=%s" % (key, count), level=DEBUG)
+        log("Setting %s=%s" % (key, bin(ssl_config)), level=DEBUG)
         relation_set(relation_id=rid, relation_settings=settings)
 
 
@@ -455,8 +449,7 @@ def cluster_changed():
 
     check_peer_actions()
 
-    if is_pki_enabled():
+    initialise_pki()
-        initialise_pki()
 
     # Figure out if we need to mandate a sync
     units = get_ssl_sync_request_units()

hooks/keystone_utils.py

@@ -963,20 +963,6 @@ def is_ssl_cert_master(votes=None):
     return False
 
 
-def is_ssl_enabled():
-    use_https = config('use-https')
-    https_service_endpoints = config('https-service-endpoints')
-    if ((use_https and bool_from_string(use_https)) or
-            (https_service_endpoints and
-                bool_from_string(https_service_endpoints)) or
-            is_pki_enabled()):
-        log("SSL/HTTPS is enabled", level=DEBUG)
-        return True
-
-    log("SSL/HTTPS is NOT enabled", level=DEBUG)
-    return False
-
-
 def get_ssl_cert_master_votes():
     """Returns a list of unique votes."""
     votes = []
@@ -997,10 +983,6 @@ def ensure_ssl_cert_master():
     Normally the cluster leader will take control but we allow for this to be
     ignored since this could be called before the cluster is ready.
     """
-    # Don't do anything if we are not in ssl/https mode
-    if not is_ssl_enabled():
-        return False
-
     master_override = False
     elect = is_elected_leader(CLUSTER_RES)
 
@@ -1060,6 +1042,23 @@ def is_pki_enabled():
     return False
 
 
+def ensure_pki_cert_paths():
+    certs = os.path.join(PKI_CERTS_DIR, 'certs')
+    privates = os.path.join(PKI_CERTS_DIR, 'privates')
+    not_exists = [p for p in [PKI_CERTS_DIR, certs, privates]
+                  if not os.path.exists(p)]
+    if not_exists:
+        log("Configuring token signing cert paths", level=DEBUG)
+        perms = 0o755
+        for path in not_exists:
+            if not os.path.isdir(path):
+                mkdir(path=path, owner=SSH_USER, group='keystone', perms=perms)
+            else:
+                # Ensure accessible by ssh user and group (for sync).
+                ensure_permissions(path, user=SSH_USER, group='keystone',
+                                   perms=perms)
+
+
 def ensure_pki_dir_permissions():
     # Ensure accessible by unison user and group (for sync).
     ensure_permissions(PKI_CERTS_DIR, user=SSH_USER, group='keystone',
@@ -1131,10 +1130,10 @@ def synchronize_ca(fatal=False):
         peer_service_actions['restart'].append('apache2')
         peer_actions.append('update-ca-certificates')
 
-    if is_pki_enabled():
+    # NOTE: certs needed for token signing e.g. pki and revocation list query.
-        log("Syncing token certs", level=DEBUG)
+    log("Syncing token certs", level=DEBUG)
-        paths_to_sync.append(PKI_CERTS_DIR)
+    paths_to_sync.append(PKI_CERTS_DIR)
-        peer_actions.append('ensure-pki-permissions')
+    peer_actions.append('ensure-pki-permissions')
 
     if not paths_to_sync:
         log("Nothing to sync - skipping", level=DEBUG)

templates/kilo/keystone.conf

@@ -70,8 +70,6 @@ driver = keystone.assignment.backends.{{ assignment_backend }}.Assignment
 
 [oauth1]
 
-[signing]
-
 [auth]
 methods = external,password,token,oauth1
 password = keystone.auth.plugins.password.Password

unit_tests/test_keystone_contexts.py

@@ -26,11 +26,9 @@ class TestKeystoneContexts(CharmTestCase):
     @patch('keystone_utils.ensure_permissions')
     @patch('keystone_utils.determine_ports')
     @patch('keystone_utils.is_ssl_cert_master')
-    @patch('keystone_utils.is_ssl_enabled')
     @patch.object(context, 'log')
     def test_apache_ssl_context_ssl_not_master(self,
                                                mock_log,
-                                               mock_is_ssl_enabled,
                                                mock_is_ssl_cert_master,
                                                mock_determine_ports,
                                                mock_ensure_permissions,
@@ -38,7 +36,6 @@ class TestKeystoneContexts(CharmTestCase):
                                                mock_mkdir,
                                                mock_cert_provided_in_config):
         mock_cert_provided_in_config.return_value = False
-        mock_is_ssl_enabled.return_value = True
         mock_is_ssl_cert_master.return_value = False
 
         context.ApacheSSLContext().configure_cert('foo')
@@ -49,7 +46,6 @@ class TestKeystoneContexts(CharmTestCase):
 
     @patch('keystone_utils.determine_ports')
     @patch('keystone_utils.is_ssl_cert_master')
-    @patch('keystone_utils.is_ssl_enabled')
     @patch('charmhelpers.contrib.openstack.context.config')
     @patch('charmhelpers.contrib.openstack.context.is_clustered')
     @patch('charmhelpers.contrib.openstack.context.determine_apache_port')
@@ -62,10 +58,8 @@ class TestKeystoneContexts(CharmTestCase):
                                                 mock_determine_apache_port,
                                                 mock_is_clustered,
                                                 mock_config,
-                                                mock_is_ssl_enabled,
                                                 mock_is_ssl_cert_master,
                                                 mock_determine_ports):
-        mock_is_ssl_enabled.return_value = True
         mock_is_ssl_cert_master.return_value = True
         mock_https.return_value = True
         mock_unit_get.return_value = '1.2.3.4'

unit_tests/test_keystone_hooks.py

@@ -312,9 +312,9 @@ class KeystoneRelationTests(CharmTestCase):
     @patch('keystone_utils.ensure_ssl_cert_master')
     @patch('keystone_utils.ensure_ssl_dirs')
     @patch.object(hooks, 'ensure_permissions')
+    @patch.object(hooks, 'ensure_pki_cert_paths')
     @patch.object(hooks, 'ensure_pki_dir_permissions')
     @patch.object(hooks, 'ensure_ssl_dir')
-    @patch.object(hooks, 'is_pki_enabled')
     @patch.object(hooks, 'is_ssl_cert_master')
     @patch.object(hooks, 'send_ssl_sync_request')
     @patch.object(hooks, 'peer_units')
@@ -334,15 +334,14 @@ class KeystoneRelationTests(CharmTestCase):
                                               mock_peer_units,
                                               mock_send_ssl_sync_request,
                                               mock_is_ssl_cert_master,
-                                              mock_is_pki_enabled,
                                               mock_ensure_ssl_dir,
+                                              mock_ensure_pki_cert_paths,
                                               mock_ensure_permissions,
                                               mock_ensure_pki_dir_permissions,
                                               mock_ensure_ssl_dirs,
                                               mock_ensure_ssl_cert_master,
                                               mock_log, git_requested):
         git_requested.return_value = False
-        mock_is_pki_enabled.return_value = True
         mock_is_ssl_cert_master.return_value = True
         self.is_db_initialised.return_value = True
         self.is_db_ready.return_value = True
@@ -376,9 +375,9 @@ class KeystoneRelationTests(CharmTestCase):
     @patch('keystone_utils.ensure_ssl_dirs')
     @patch.object(hooks, 'update_all_identity_relation_units')
     @patch.object(hooks, 'ensure_permissions')
+    @patch.object(hooks, 'ensure_pki_cert_paths')
     @patch.object(hooks, 'ensure_pki_dir_permissions')
     @patch.object(hooks, 'ensure_ssl_dir')
-    @patch.object(hooks, 'is_pki_enabled')
     @patch.object(hooks, 'peer_units')
     @patch.object(hooks, 'is_ssl_cert_master')
     @patch.object(hooks, 'cluster_joined')
@@ -393,16 +392,15 @@ class KeystoneRelationTests(CharmTestCase):
                                                   ensure_user, cluster_joined,
                                                   mock_is_ssl_cert_master,
                                                   mock_peer_units,
-                                                  mock_is_pki_enabled,
                                                   mock_ensure_ssl_dir,
                                                   mock_ensure_permissions,
+                                                  mock_ensure_pki_cert_paths,
                                                   mock_ensure_pki_permissions,
                                                   mock_update_all_id_rel_units,
                                                   ensure_ssl_dirs,
                                                   mock_ensure_ssl_cert_master,
                                                   mock_log, git_requested):
         git_requested.return_value = False
-        mock_is_pki_enabled.return_value = True
         mock_is_ssl_cert_master.return_value = True
         mock_peer_units.return_value = []
         self.openstack_upgrade_available.return_value = False
@@ -426,9 +424,9 @@ class KeystoneRelationTests(CharmTestCase):
     @patch('keystone_utils.ensure_ssl_cert_master')
     @patch('keystone_utils.ensure_ssl_dirs')
     @patch.object(hooks, 'ensure_permissions')
+    @patch.object(hooks, 'ensure_pki_cert_paths')
     @patch.object(hooks, 'ensure_pki_dir_permissions')
     @patch.object(hooks, 'ensure_ssl_dir')
-    @patch.object(hooks, 'is_pki_enabled')
     @patch.object(hooks, 'is_ssl_cert_master')
     @patch.object(hooks, 'send_ssl_sync_request')
     @patch.object(hooks, 'peer_units')
@@ -447,15 +445,14 @@ class KeystoneRelationTests(CharmTestCase):
                                                    mock_peer_units,
                                                    mock_send_ssl_sync_request,
                                                    mock_is_ssl_cert_master,
-                                                   mock_is_pki_enabled,
                                                    mock_ensure_ssl_dir,
                                                    mock_ensure_permissions,
+                                                   mock_ensure_pki_cert_paths,
                                                    mock_ensure_pki_permissions,
                                                    mock_ensure_ssl_dirs,
                                                    mock_ensure_ssl_cert_master,
                                                    mock_log, git_requested):
         git_requested.return_value = False
-        mock_is_pki_enabled.return_value = True
         mock_is_ssl_cert_master.return_value = True
         self.is_db_ready.return_value = True
         self.is_db_initialised.return_value = True
@@ -485,12 +482,12 @@ class KeystoneRelationTests(CharmTestCase):
             remote_unit='unit/0')
         admin_relation_changed.assert_called_with('identity-service:0')
 
+    @patch.object(hooks, 'initialise_pki')
     @patch.object(hooks, 'git_install_requested')
     @patch.object(hooks, 'config_value_changed')
     @patch('keystone_utils.log')
     @patch('keystone_utils.ensure_ssl_cert_master')
     @patch.object(hooks, 'ensure_ssl_dir')
-    @patch.object(hooks, 'is_pki_enabled')
     @patch.object(hooks, 'send_ssl_sync_request')
     @patch.object(hooks, 'is_db_initialised')
     @patch.object(hooks, 'is_db_ready')
@@ -510,14 +507,13 @@ class KeystoneRelationTests(CharmTestCase):
                                         mock_is_db_ready,
                                         mock_is_db_initialised,
                                         mock_send_ssl_sync_request,
-                                        mock_is_pki_enabled,
                                         mock_ensure_ssl_dir,
                                         mock_ensure_ssl_cert_master,
                                         mock_log, config_val_changed,
-                                        git_requested):
+                                        git_requested,
+                                        mock_initialise_pki):
         git_requested.return_value = True
         mock_ensure_ssl_cert_master.return_value = False
-        mock_is_pki_enabled.return_value = False
         self.openstack_upgrade_available.return_value = False
         self.is_elected_leader.return_value = True
         mock_peer_units.return_value = []
@@ -544,11 +540,11 @@ class KeystoneRelationTests(CharmTestCase):
         self.assertFalse(self.openstack_upgrade_available.called)
         self.assertFalse(self.do_openstack_upgrade_reexec.called)
 
+    @patch.object(hooks, 'initialise_pki')
     @patch.object(hooks, 'git_install_requested')
     @patch.object(hooks, 'config_value_changed')
     @patch.object(hooks, 'ensure_ssl_dir')
     @patch.object(hooks, 'configure_https')
-    @patch.object(hooks, 'is_pki_enabled')
     @patch.object(hooks, 'is_ssl_cert_master')
     @patch.object(hooks, 'peer_units')
     @patch.object(unison, 'get_homedir')
@@ -559,12 +555,12 @@ class KeystoneRelationTests(CharmTestCase):
                                                           ensure_user,
                                                           get_home,
                                                           peer_units, is_ssl,
-                                                          is_pki, config_https,
+                                                          config_https,
                                                           ensure_ssl_dir,
                                                           config_value_changed,
-                                                          git_requested):
+                                                          git_requested,
+                                                          mock_initialise_pki):
         ensure_ssl_cert.return_value = False
-        is_pki.return_value = False
         peer_units.return_value = []
 
         git_requested.return_value = False
@@ -619,6 +615,7 @@ class KeystoneRelationTests(CharmTestCase):
             user=self.ssh_user, group='juju_keystone',
             peer_interface='cluster', ensure_local_user=True)
 
+    @patch.object(hooks, 'initialise_pki')
     @patch.object(hooks, 'update_all_identity_relation_units')
     @patch.object(hooks, 'get_ssl_sync_request_units')
     @patch.object(hooks, 'is_ssl_cert_master')
@@ -638,7 +635,8 @@ class KeystoneRelationTests(CharmTestCase):
                              mock_peer_units,
                              mock_is_ssl_cert_master,
                              mock_get_ssl_sync_request_units,
-                             mock_update_all_identity_relation_units):
+                             mock_update_all_identity_relation_units,
+                             mock_initialise_pki):
 
         relation_settings = {'foo_passwd': '123',
                              'identity-service:16_foo': 'bar'}

unit_tests/test_keystone_utils.py

@@ -479,22 +479,11 @@ class TestKeystoneUtils(CharmTestCase):
         self.assertTrue(utils.is_db_ready())
 
     @patch.object(utils, 'peer_units')
-    @patch.object(utils, 'is_ssl_enabled')
+    def test_ensure_ssl_cert_master_ssl_no_peers(self, mock_peer_units):
-    def test_ensure_ssl_cert_master_no_ssl(self, mock_is_ssl_enabled,
-                                           mock_peer_units):
-        mock_is_ssl_enabled.return_value = False
-        self.assertFalse(utils.ensure_ssl_cert_master())
-        self.assertFalse(self.relation_set.called)
-
-    @patch.object(utils, 'peer_units')
-    @patch.object(utils, 'is_ssl_enabled')
-    def test_ensure_ssl_cert_master_ssl_no_peers(self, mock_is_ssl_enabled,
-                                                 mock_peer_units):
         def mock_rel_get(unit=None, **kwargs):
             return None
 
         self.relation_get.side_effect = mock_rel_get
-        mock_is_ssl_enabled.return_value = True
         self.relation_ids.return_value = ['cluster:0']
         self.local_unit.return_value = 'unit/0'
         self.related_units.return_value = []
@@ -508,9 +497,7 @@ class TestKeystoneUtils(CharmTestCase):
                                              relation_settings=settings)
 
     @patch.object(utils, 'peer_units')
-    @patch.object(utils, 'is_ssl_enabled')
     def test_ensure_ssl_cert_master_ssl_master_no_peers(self,
-                                                        mock_is_ssl_enabled,
                                                         mock_peer_units):
         def mock_rel_get(unit=None, **kwargs):
             if unit == 'unit/0':
@@ -519,7 +506,6 @@ class TestKeystoneUtils(CharmTestCase):
             return None
 
         self.relation_get.side_effect = mock_rel_get
-        mock_is_ssl_enabled.return_value = True
         self.relation_ids.return_value = ['cluster:0']
         self.local_unit.return_value = 'unit/0'
         self.related_units.return_value = []
@@ -533,10 +519,7 @@ class TestKeystoneUtils(CharmTestCase):
                                              relation_settings=settings)
 
     @patch.object(utils, 'peer_units')
-    @patch.object(utils, 'is_ssl_enabled')
+    def test_ensure_ssl_cert_master_ssl_not_leader(self, mock_peer_units):
-    def test_ensure_ssl_cert_master_ssl_not_leader(self, mock_is_ssl_enabled,
-                                                   mock_peer_units):
-        mock_is_ssl_enabled.return_value = True
         self.relation_ids.return_value = ['cluster:0']
         self.local_unit.return_value = 'unit/0'
         mock_peer_units.return_value = ['unit/1']
@@ -546,9 +529,7 @@ class TestKeystoneUtils(CharmTestCase):
         self.assertFalse(self.relation_set.called)
 
     @patch.object(utils, 'peer_units')
-    @patch.object(utils, 'is_ssl_enabled')
     def test_ensure_ssl_cert_master_is_leader_new_peer(self,
-                                                       mock_is_ssl_enabled,
                                                        mock_peer_units):
         def mock_rel_get(unit=None, **kwargs):
             if unit == 'unit/0':
@@ -557,7 +538,6 @@ class TestKeystoneUtils(CharmTestCase):
             return 'unknown'
 
         self.relation_get.side_effect = mock_rel_get
-        mock_is_ssl_enabled.return_value = True
         self.relation_ids.return_value = ['cluster:0']
         self.local_unit.return_value = 'unit/0'
         mock_peer_units.return_value = ['unit/1']
@@ -570,9 +550,7 @@ class TestKeystoneUtils(CharmTestCase):
                                              relation_settings=settings)
 
     @patch.object(utils, 'peer_units')
-    @patch.object(utils, 'is_ssl_enabled')
     def test_ensure_ssl_cert_master_is_leader_no_new_peer(self,
-                                                          mock_is_ssl_enabled,
                                                           mock_peer_units):
         def mock_rel_get(unit=None, **kwargs):
             if unit == 'unit/0':
@@ -581,7 +559,6 @@ class TestKeystoneUtils(CharmTestCase):
             return 'unit/0'
 
         self.relation_get.side_effect = mock_rel_get
-        mock_is_ssl_enabled.return_value = True
         self.relation_ids.return_value = ['cluster:0']
         self.local_unit.return_value = 'unit/0'
         mock_peer_units.return_value = ['unit/1']
@@ -621,9 +598,7 @@ class TestKeystoneUtils(CharmTestCase):
         )
 
     @patch.object(utils, 'peer_units')
-    @patch.object(utils, 'is_ssl_enabled')
     def test_ensure_ssl_cert_master_is_leader_bad_votes(self,
-                                                        mock_is_ssl_enabled,
                                                         mock_peer_units):
         counter = {0: 0}
 
@@ -637,7 +612,6 @@ class TestKeystoneUtils(CharmTestCase):
             return ret
 
         self.relation_get.side_effect = mock_rel_get
-        mock_is_ssl_enabled.return_value = True
         self.relation_ids.return_value = ['cluster:0']
         self.local_unit.return_value = 'unit/0'
         mock_peer_units.return_value = ['unit/1']