From b95ad9c0230ca58281449465ce783374b84e2095 Mon Sep 17 00:00:00 2001 From: Jorge Niedbalski Date: Thu, 22 Jun 2017 12:52:57 -0400 Subject: [PATCH] Restrict get_image_location policy to role:admin This patch will cause /etc/glance/policy.json to be updated so that *_image_location rules are set to role:admin so that only admins can see that info. On first update the original values are stored on the local kvstore in case they need to be retrieved for later restoring or comparison. Closes-Bug: #1699565 Change-Id: Id6198d534af95013af47c0c1292ac65c79470af4 --- charmhelpers/contrib/openstack/utils.py | 15 +++++++-- hooks/glance_relations.py | 6 ++-- hooks/glance_utils.py | 39 +++++++++++++++++++++++ unit_tests/test_glance_relations.py | 39 +++++++++++++++++++---- unit_tests/test_glance_utils.py | 42 +++++++++++++++++++++++-- 5 files changed, 129 insertions(+), 12 deletions(-) diff --git a/charmhelpers/contrib/openstack/utils.py b/charmhelpers/contrib/openstack/utils.py index 9e5af342..e1d852db 100644 --- a/charmhelpers/contrib/openstack/utils.py +++ b/charmhelpers/contrib/openstack/utils.py @@ -2045,14 +2045,25 @@ def token_cache_pkgs(source=None, release=None): def update_json_file(filename, items): """Updates the json `filename` with a given dict. - :param filename: json filename (i.e.: /etc/glance/policy.json) + :param filename: path to json file (e.g. /etc/glance/policy.json) :param items: dict of items to update """ + if not items: + return + with open(filename) as fd: policy = json.load(fd) + + # Compare before and after and if nothing has changed don't write the file + # since that could cause unnecessary service restarts. + before = json.dumps(policy, indent=4, sort_keys=True) policy.update(items) + after = json.dumps(policy, indent=4, sort_keys=True) + if before == after: + return + with open(filename, "w") as fd: - fd.write(json.dumps(policy, indent=4)) + fd.write(after) @cached diff --git a/hooks/glance_relations.py b/hooks/glance_relations.py index a770b4c7..b74afa50 100755 --- a/hooks/glance_relations.py +++ b/hooks/glance_relations.py @@ -41,6 +41,7 @@ from glance_utils import ( assess_status, reinstall_paste_ini, is_api_ready, + update_image_location_policy, ) from charmhelpers.core.hookenv import ( config, @@ -118,7 +119,6 @@ from charmhelpers.contrib.openstack.context import ( from charmhelpers.contrib.charmsupport import nrpe from charmhelpers.contrib.hardening.harden import harden - hooks = Hooks() CONFIGS = register_configs() @@ -290,7 +290,7 @@ def object_store_joined(): return [image_service_joined(rid) for rid in relation_ids('image-service')] - + update_image_location_policy() CONFIGS.write(GLANCE_API_CONF) @@ -421,6 +421,7 @@ def config_changed(): # NOTE(jamespage): trigger any configuration related changes # for cephx permissions restrictions ceph_changed() + update_image_location_policy() @hooks.hook('cluster-relation-joined') @@ -456,6 +457,7 @@ def upgrade_charm(): reinstall_paste_ini() configure_https() update_nrpe_config() + update_image_location_policy() CONFIGS.write_all() diff --git a/hooks/glance_utils.py b/hooks/glance_utils.py index 7506c4ce..837f2b41 100644 --- a/hooks/glance_utils.py +++ b/hooks/glance_utils.py @@ -14,6 +14,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import json import os import shutil import subprocess @@ -37,6 +38,7 @@ from charmhelpers.core.hookenv import ( charm_dir, config, log, + INFO, relation_ids, service_name, ) @@ -87,6 +89,7 @@ from charmhelpers.contrib.openstack.utils import ( pause_unit, resume_unit, token_cache_pkgs, + update_json_file, ) from charmhelpers.core.templating import render @@ -142,6 +145,7 @@ GLANCE_REGISTRY_PASTE = os.path.join(GLANCE_CONF_DIR, 'glance-registry-paste.ini') GLANCE_API_PASTE = os.path.join(GLANCE_CONF_DIR, 'glance-api-paste.ini') +GLANCE_POLICY_FILE = os.path.join(GLANCE_CONF_DIR, "policy.json") CEPH_CONF = "/etc/ceph/ceph.conf" CHARM_CEPH_CONF = '/var/lib/charm/{}/ceph.conf' @@ -345,6 +349,8 @@ def restart_map(): if enable_memcache(source=config('openstack-origin')): _map.append((MEMCACHED_CONF, ['memcached'])) + _map.append((GLANCE_POLICY_FILE, ['glance-api', 'glance-registry'])) + return OrderedDict(_map) @@ -684,3 +690,36 @@ def reinstall_paste_ini(): def is_api_ready(configs): return (not incomplete_relation_data(configs, REQUIRED_INTERFACES)) + + +def update_image_location_policy(): + """Update *_image_location policy to restrict to admin role. + + We do this unconditonally and keep a record of the original as installed by + the package. + """ + if CompareOpenStackReleases(os_release('glance-common')) < 'kilo': + # NOTE(hopem): at the time of writing we are unable to do this for + # earlier than Kilo due to LP: #1502136 + return + + db = kv() + policies = ["get_image_location", "set_image_location", + "delete_image_location"] + for policy_key in policies: + # Save original value at time of first install in case we ever need to + # revert. + db_key = "policy_{}".format(policy_key) + if db.get(db_key) is None: + p = json.loads(open(GLANCE_POLICY_FILE).read()) + if policy_key in p: + db.set(db_key, p[policy_key]) + db.flush() + else: + log("key '{}' not found in policy file".format(policy_key), + level=INFO) + + policy_value = 'role:admin' + log("Updating Glance policy file setting policy " + "'{}':'{}'".format(policy_key, policy_value), level=INFO) + update_json_file(GLANCE_POLICY_FILE, {policy_key: policy_value}) diff --git a/unit_tests/test_glance_relations.py b/unit_tests/test_glance_relations.py index 81952f58..1527f74f 100644 --- a/unit_tests/test_glance_relations.py +++ b/unit_tests/test_glance_relations.py @@ -397,9 +397,10 @@ class GlanceRelationTests(CharmTestCase): 'swift relation incomplete' ) + @patch.object(relations, 'update_image_location_policy') @patch.object(relations, 'CONFIGS') def test_object_store_joined_with_identity_service_with_object_store( - self, configs): + self, configs, mock_update_image_location_policy): configs.complete_contexts = MagicMock() configs.complete_contexts.return_value = ['identity-service', 'object-store'] @@ -407,6 +408,18 @@ class GlanceRelationTests(CharmTestCase): relations.object_store_joined() self.assertEqual([call('/etc/glance/glance-api.conf')], configs.write.call_args_list) + self.assertTrue(mock_update_image_location_policy.called) + + @patch.object(relations, 'update_image_location_policy') + @patch.object(relations, 'CONFIGS') + def test_object_store_joined_with_expose_image_locations_false( + self, configs, mock_update_image_location_policy): + configs.complete_contexts = MagicMock() + configs.complete_contexts.return_value = ['identity-service', + 'object-store'] + configs.write = MagicMock() + relations.object_store_joined() + self.assertTrue(mock_update_image_location_policy.called) def test_ceph_joined(self): relations.ceph_joined() @@ -616,22 +629,27 @@ class GlanceRelationTests(CharmTestCase): relations.keystone_changed() [self.assertIn(call(r), imgsj.call_args_list) for r in rids] + @patch.object(relations, 'update_image_location_policy') @patch.object(relations, 'configure_https') @patch.object(relations, 'git_install_requested') def test_config_changed_no_openstack_upgrade(self, git_requested, - configure_https): + configure_https, + mock_update_policy): git_requested.return_value = False self.openstack_upgrade_available.return_value = False relations.config_changed() self.open_port.assert_called_with(9292) self.assertTrue(configure_https.called) + self.assertTrue(mock_update_policy.called) + @patch.object(relations, 'update_image_location_policy') @patch.object(relations, 'status_set') @patch.object(relations, 'configure_https') @patch.object(relations, 'git_install_requested') def test_config_changed_with_openstack_upgrade(self, git_requested, configure_https, - status): + status, + mock_update_policy): git_requested.return_value = False self.openstack_upgrade_available.return_value = True relations.config_changed() @@ -641,21 +659,27 @@ class GlanceRelationTests(CharmTestCase): ) self.assertTrue(self.do_openstack_upgrade.called) self.assertTrue(configure_https.called) + self.assertTrue(mock_update_policy.called) + @patch.object(relations, 'update_image_location_policy') @patch.object(relations, 'git_install_requested') - def test_config_changed_with_openstack_upgrade_action(self, git_requested): + def test_config_changed_with_openstack_upgrade_action(self, git_requested, + mock_update_policy): git_requested.return_value = False self.openstack_upgrade_available.return_value = True self.test_config.set('action-managed-upgrade', True) relations.config_changed() self.assertFalse(self.do_openstack_upgrade.called) + self.assertTrue(mock_update_policy.called) + @patch.object(relations, 'update_image_location_policy') @patch.object(relations, 'configure_https') @patch.object(relations, 'git_install_requested') @patch.object(relations, 'config_value_changed') def test_config_changed_git_updated(self, config_val_changed, - git_requested, configure_https): + git_requested, configure_https, + mock_update_image_location_policy): git_requested.return_value = True repo = 'cloud:trusty-juno' openstack_origin_git = { @@ -676,6 +700,7 @@ class GlanceRelationTests(CharmTestCase): self.git_install.assert_called_with(projects_yaml) self.assertFalse(self.do_openstack_upgrade.called) self.assertTrue(configure_https.called) + self.assertTrue(mock_update_image_location_policy.called) @patch.object(relations, 'CONFIGS') def test_cluster_changed(self, configs): @@ -704,18 +729,20 @@ class GlanceRelationTests(CharmTestCase): call('/etc/haproxy/haproxy.cfg')], configs.write.call_args_list) + @patch.object(relations, 'update_image_location_policy') @patch.object(utils, 'config') @patch.object(utils, 'token_cache_pkgs') @patch.object(relations, 'CONFIGS') @patch.object(utils, 'git_install_requested') def test_upgrade_charm(self, git_requested, configs, token_cache_pkgs, - util_config): + util_config, mock_update_image_location_policy): git_requested.return_value = False self.filter_installed_packages.return_value = ['test'] relations.upgrade_charm() self.apt_install.assert_called_with(['test'], fatal=True) self.assertTrue(configs.write_all.called) self.assertTrue(self.reinstall_paste_ini.called) + self.assertTrue(mock_update_image_location_policy.called) def test_ha_relation_joined(self): self.get_hacluster_config.return_value = { diff --git a/unit_tests/test_glance_utils.py b/unit_tests/test_glance_utils.py index df6665e3..2640e773 100644 --- a/unit_tests/test_glance_utils.py +++ b/unit_tests/test_glance_utils.py @@ -15,7 +15,7 @@ import os from collections import OrderedDict -from mock import patch, call, MagicMock +from mock import patch, call, MagicMock, mock_open os.environ['JUJU_UNIT_NAME'] = 'glance' import hooks.glance_utils as utils @@ -145,7 +145,8 @@ class TestGlanceUtils(CharmTestCase): (utils.HAPROXY_CONF, ['haproxy']), (utils.HTTPS_APACHE_CONF, ['apache2']), (utils.HTTPS_APACHE_24_CONF, ['apache2']), - (utils.MEMCACHED_CONF, ['memcached']) + (utils.MEMCACHED_CONF, ['memcached']), + (utils.GLANCE_POLICY_FILE, ['glance-api', 'glance-registry']) ]) self.assertEqual(ex_map, utils.restart_map()) self.enable_memcache.return_value = False @@ -442,3 +443,40 @@ class TestGlanceUtils(CharmTestCase): def test_is_api_ready_false(self): self._test_is_api_ready(False) + + @patch.object(utils, 'json') + @patch.object(utils, 'update_json_file') + @patch.object(utils, 'kv') + @patch.object(utils, 'os_release') + def test_update_image_location_policy(self, mock_os_release, mock_kv, + mock_update_json_file, mock_json): + db_vals = {} + + def fake_db_get(key): + return db_vals.get(key) + + db_obj = mock_kv.return_value + db_obj.get = MagicMock() + db_obj.get.side_effect = fake_db_get + db_obj.set = MagicMock() + + fake_open = mock_open() + with patch.object(utils, 'open', fake_open, create=True): + mock_json.loads.return_value = {'get_image_location': '', + 'set_image_location': '', + 'delete_image_location': ''} + + mock_os_release.return_value = 'icehouse' + utils.update_image_location_policy() + self.assertFalse(mock_kv.called) + + mock_os_release.return_value = 'kilo' + utils.update_image_location_policy() + self.assertTrue(mock_kv.called) + db_obj.get.assert_has_calls([call('policy_get_image_location'), + call('policy_set_image_location'), + call('policy_delete_image_location')]) + db_obj.set.assert_has_calls([call('policy_get_image_location', ''), + call('policy_set_image_location', ''), + call('policy_delete_image_location', + '')])