From 5487323edaf9c1723209271214c0c4286811e992 Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Mon, 7 Dec 2015 15:04:38 +0100 Subject: [PATCH] [hopem,r=] Ensure ssl certs always synced. Partially-Closes-Bug: 1520339 --- hooks/keystone_context.py | 35 +++++++--------------- hooks/keystone_hooks.py | 43 ++++++++++++---------------- hooks/keystone_utils.py | 43 ++++++++++++++-------------- templates/kilo/keystone.conf | 2 -- unit_tests/test_keystone_contexts.py | 6 ---- unit_tests/test_keystone_hooks.py | 34 +++++++++++----------- unit_tests/test_keystone_utils.py | 30 ++----------------- 7 files changed, 67 insertions(+), 126 deletions(-) diff --git a/hooks/keystone_context.py b/hooks/keystone_context.py index 0c023688..5a5f5bbb 100644 --- a/hooks/keystone_context.py +++ b/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: - log("Configuring PKI token cert paths", level=DEBUG) - certs = os.path.join(PKI_CERTS_DIR, 'certs') - privates = os.path.join(PKI_CERTS_DIR, 'privates') - for path in [PKI_CERTS_DIR, certs, privates]: - perms = 0o755 - 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) - - 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 + ensure_pki_cert_paths() + certs = os.path.join(PKI_CERTS_DIR, 'certs') + privates = os.path.join(PKI_CERTS_DIR, 'privates') + ctxt.update({'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')}) # 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 diff --git a/hooks/keystone_hooks.py b/hooks/keystone_hooks.py index 6975ae03..2158c2db 100755 --- a/hooks/keystone_hooks.py +++ b/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 - # 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 + prev = 0b0 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() diff --git a/hooks/keystone_utils.py b/hooks/keystone_utils.py index 87e71b76..d6a0d032 100644 --- a/hooks/keystone_utils.py +++ b/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(): - log("Syncing token certs", level=DEBUG) - paths_to_sync.append(PKI_CERTS_DIR) - peer_actions.append('ensure-pki-permissions') + # NOTE: certs needed for token signing e.g. pki and revocation list query. + log("Syncing token certs", level=DEBUG) + paths_to_sync.append(PKI_CERTS_DIR) + peer_actions.append('ensure-pki-permissions') if not paths_to_sync: log("Nothing to sync - skipping", level=DEBUG) diff --git a/templates/kilo/keystone.conf b/templates/kilo/keystone.conf index 28454a30..4ab52ff0 100644 --- a/templates/kilo/keystone.conf +++ b/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 diff --git a/unit_tests/test_keystone_contexts.py b/unit_tests/test_keystone_contexts.py index ef732648..a9cc606f 100644 --- a/unit_tests/test_keystone_contexts.py +++ b/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' diff --git a/unit_tests/test_keystone_hooks.py b/unit_tests/test_keystone_hooks.py index b9427549..70b95fdc 100644 --- a/unit_tests/test_keystone_hooks.py +++ b/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'} diff --git a/unit_tests/test_keystone_utils.py b/unit_tests/test_keystone_utils.py index 03626237..1da2644b 100644 --- a/unit_tests/test_keystone_utils.py +++ b/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_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 test_ensure_ssl_cert_master_ssl_no_peers(self, 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_is_ssl_enabled, - mock_peer_units): - mock_is_ssl_enabled.return_value = True + def test_ensure_ssl_cert_master_ssl_not_leader(self, mock_peer_units): 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']