diff --git a/actions.yaml b/actions.yaml index f17f953f..26ebc8af 100644 --- a/actions.yaml +++ b/actions.yaml @@ -79,3 +79,20 @@ run-deferred-hooks: show-deferred-events: descrpition: | Show the outstanding restarts +rotate-service-user-password: + description: | + Rotate the specified rabbitmq-server user's password. The current password + is replaced with a randomly generated password. The password is changed on + the relation to the user's units. This may result in a control plane outage + for the duration of the password changing process. + params: + service-user: + type: string + description: | + The username of the rabbitmq-server service as specified in + list-service-usernames. +list-service-usernames: + description: | + List the usernames of the passwords that have been provided on the + amqp relations. The service username passed to + 'rotate-service-user-password' needs to be on this list. diff --git a/actions/actions.py b/actions/actions.py index 2af985b8..d4975963 100755 --- a/actions/actions.py +++ b/actions/actions.py @@ -19,6 +19,7 @@ import re from collections import OrderedDict from subprocess import check_output, CalledProcessError, PIPE import sys +import traceback _path = os.path.dirname(os.path.realpath(__file__)) @@ -71,6 +72,9 @@ from hooks.rabbit_utils import ( list_vhosts, vhost_queue_info, rabbitmq_version_newer_or_equal, + get_usernames_for_passwords, + NotLeaderError, + InvalidServiceUserError, ) @@ -299,6 +303,39 @@ def show_deferred_events(args): os_utils.show_deferred_events_action_helper() +def list_service_usernames(args): + """List the service usernames known in this model that can be rotated.""" + usernames = get_usernames_for_passwords() + action_set({'usernames': usernames or []}) + + +def rotate_service_user_password(args): + """Rotate the service user's password. + + The parameter must be passed in the service-user parameter. + """ + service_user = action_get("service-user") + if service_user is None: + action_fail( + "The 'service-user' parameter was not passed and is required.") + return + try: + rabbitmq_server_relations.rotate_service_user_password(service_user) + except NotLeaderError: + action_fail( + "This unit either isn't the leader or is not ready to do " + "leader actions. The rotate-service-user-password action. " + "can't be run at the moment. Please verify that the unit is the " + "leader and that the cluster is ready.") + except InvalidServiceUserError: + action_fail( + "Service username {} is not valid for password rotation. Please " + "check the action 'list-service-users' for the correct username." + .format(service_user)) + except Exception: + raise + + # A dictionary of all the defined actions to callables (which take # parsed arguments). ACTIONS = { @@ -313,6 +350,8 @@ ACTIONS = { "restart-services": restart, "run-deferred-hooks": run_deferred_hooks, "show-deferred-events": show_deferred_events, + "list-service-usernames": list_service_usernames, + "rotate-service-user-password": rotate_service_user_password, } @@ -332,7 +371,10 @@ def main(args): try: action(args) except Exception as e: + log("Action {} failed: {}\nTrackback:\n{}" + .format(action_name, str(e), traceback.format_exc()), ERROR) action_fail("Action {} failed: {}".format(action_name, str(e))) + _run_atexit() diff --git a/actions/list-service-usernames b/actions/list-service-usernames new file mode 120000 index 00000000..405a394e --- /dev/null +++ b/actions/list-service-usernames @@ -0,0 +1 @@ +actions.py \ No newline at end of file diff --git a/actions/rotate-service-user-password b/actions/rotate-service-user-password new file mode 120000 index 00000000..405a394e --- /dev/null +++ b/actions/rotate-service-user-password @@ -0,0 +1 @@ +actions.py \ No newline at end of file diff --git a/hooks/rabbit_utils.py b/hooks/rabbit_utils.py index ad42930e..59e2a71e 100644 --- a/hooks/rabbit_utils.py +++ b/hooks/rabbit_utils.py @@ -94,7 +94,7 @@ from charmhelpers.core.host import ( from charmhelpers.contrib.peerstorage import ( peer_store, - peer_retrieve + peer_retrieve, ) from charmhelpers.fetch import ( @@ -136,6 +136,7 @@ COORD_KEYS = [COORD_KEY_RESTART, COORD_KEY_PKG_UPGRADE, COORD_KEY_CLUSTER] _named_passwd = '/var/lib/charm/{}/{}.passwd' _local_named_passwd = '/var/lib/charm/{}/{}.local_passwd' +_service_password_glob = '/var/lib/charm/{}/*.passwd' # hook_contexts are used as a convenient mechanism to render templates @@ -179,6 +180,16 @@ def CONFIG_FILES(): return _cfiles +class NotLeaderError(Exception): + """Exception raised if not the leader.""" + pass + + +class InvalidServiceUserError(Exception): + """Exception raised if an invalid Service User is detected.""" + pass + + class ConfigRenderer(object): """ This class is a generic configuration renderer for @@ -590,6 +601,24 @@ def create_user(user, password, tags=[]): apply_tags(user, tags) +def change_user_password(user, new_password): + """Change the password of the rabbitmq user. + + :param user: the user to change; must exist in the rabbitmq instance. + :type user: str + :param new_password: the password to change to. + :type new_password: str + :raises KeyError: if the user doesn't exist. + """ + exists = user_exists(user) + if not exists: + msg = "change_user_password: user '{}' doesn't exist.".format(user) + log(msg, ERROR) + raise KeyError(msg) + rabbitmqctl('change_password', user, new_password) + log("Changed password on rabbitmq for user: {}".format(user), INFO) + + def grant_permissions(user, vhost): """Grant all permissions on a vhost to a user. @@ -1178,7 +1207,7 @@ def get_rabbit_password_on_disk(username, password=None, local=False): def migrate_passwords_to_peer_relation(): '''Migrate any passwords storage on disk to cluster peer relation''' - for f in glob.glob('/var/lib/charm/{}/*.passwd'.format(service_name())): + for f in glob.glob(_service_password_glob.format(service_name())): _key = os.path.basename(f) with open(f, 'r') as passwd: _value = passwd.read().strip() @@ -1190,6 +1219,48 @@ def migrate_passwords_to_peer_relation(): pass +def get_usernames_for_passwords_on_disk(): + """Return a list of usernames that have passwords on the disk. + + Note this is only for non local passwords (i.e. that end in .passwd) + + :returns: the list of usernames with passwords on the disk. + :rtype: List[str] + """ + return [ + os.path.splitext(os.path.basename(f))[0] + for f in glob.glob(_service_password_glob.format(service_name()))] + + +def get_usernames_for_passwords(): + """Return a list of usernames that have passwords. + + This checks BOTH the peer relationship (leader-storage, or the fallback to + the 'cluster' relation) and on disk. If the peer storage has usernames, + ignore the ones on disk (as they have already been migrated), otherwise + return the ones on disk. + + The keys that have passwords in peer storage end with .passwd. + + :returns: the list of usernames that have had passwords set. + :rtype: List[str] + """ + # first get from leader settings/peer relation, if available + peer_keys = None + try: + peer_keys = peer_retrieve(None) + except ValueError: + pass + if peer_keys is None: + peer_keys = {} + usernames = set(u[:-7] for u in peer_keys.keys() if u.endswith(".passwd")) + # if usernames were found in peer storage, return them. + if usernames: + return sorted(usernames) + # otherwise, return the ones on disk, if any + return sorted(get_usernames_for_passwords_on_disk()) + + def get_rabbit_password(username, password=None, local=False): ''' Retrieve, generate or store a rabbit password for the provided username using peer relation cluster''' diff --git a/hooks/rabbitmq_server_relations.py b/hooks/rabbitmq_server_relations.py index 90d90e97..e7218c80 100755 --- a/hooks/rabbitmq_server_relations.py +++ b/hooks/rabbitmq_server_relations.py @@ -100,6 +100,7 @@ from charmhelpers.core.hookenv import ( ) from charmhelpers.core.host import ( cmp_pkgrevno, + pwgen, service_stop, service_restart, ) @@ -315,6 +316,77 @@ def configure_amqp(username, vhost, relation_id, admin=False, return password +def rotate_service_user_password(service_username): + """Rotate the service username and update the relation. + + This only works on the leader unit due to how peer storage is overlayed on + leader storage. + + :param service_username: the username to rotate the password for. + :type service_username: str + :raises NotLeaderError: if the unit is not the leader. + :raises InvalidServiceUserError: if the service_username doesn't exist. + """ + if not rabbit.leader_node_is_ready(): + raise rabbit.NotLeaderError( + "This unit can't perform leadership actions and so the password " + "cannot be rotated.") + if service_username not in rabbit.get_usernames_for_passwords(): + raise rabbit.InvalidServiceUserError( + "Username {} is not valid for password rotation." + .format(service_username)) + + # pick a new password. + new_passwd = pwgen(length=64) + + # Update the password in rabbitmq + rabbit.change_user_password(service_username, new_passwd) + + # Update the setting either locally on disk, leader settings or peer + # storage. + try: + peer_store("{}.passwd".format(service_username), new_passwd) + except ValueError: + # if there was no cluster, just push to leader settings. + leader_set({"{}.passwd".format(service_username): new_passwd}) + + # Note that the password is not stored in the local cache, so the kv() + # won't need updating for this. The related unit with the username does + # need finding, though. Note that the username may have a '_' in it if + # multiple prefixes are used, but that was specified as service_username + pattern = re.compile(r"(\S+)_username") + for rid in relation_ids('amqp'): + key = None + for unit in related_units(rid): + current = relation_get(rid=rid, unit=unit) or {} + # the username is either as 'username' or '{previx}_username' + if 'username' in current: + key = 'password' + break + for key in current.keys(): + match = pattern.match(key) + if match: + key = '_'.join((match[1], 'password')) + break + else: + continue + break + if key is not None: + log("Updating password on key {} on relation_id: {}" + .format(key, rid), + INFO) + relation_set(relation_id=rid, + relation_settings={key: new_passwd}) + # set the password for the peer as well for update_client to work + # on the non-leader units + peer_key = "{}_{}".format(rid, key) + try: + peer_store(peer_key, new_passwd) + except ValueError: + # if there was no cluster, just push to leader settings. + leader_set({peer_key: new_passwd}) + + def update_clients(check_deferred_restarts=True): """Update amqp client relation hooks @@ -326,7 +398,7 @@ def update_clients(check_deferred_restarts=True): :type check_deferred_events: bool """ if check_deferred_restarts and get_deferred_restarts(): - log("Not sendinfg client update as a restart is pending.", INFO) + log("Not sending client update as a restart is pending.", INFO) return _leader_node_is_ready = rabbit.leader_node_is_ready() _client_node_is_ready = rabbit.client_node_is_ready() diff --git a/tests/tests.yaml b/tests/tests.yaml index 523e85b2..08d5b685 100644 --- a/tests/tests.yaml +++ b/tests/tests.yaml @@ -12,6 +12,7 @@ dev_bundles: tests: - zaza.openstack.charm_tests.rabbitmq_server.tests.RabbitMQDeferredRestartTest - zaza.openstack.charm_tests.rabbitmq_server.tests.RmqTests + - zaza.openstack.charm_tests.rabbitmq_server.tests.RmqRotateServiceUserPasswordTests tests_options: force_deploy: diff --git a/unit_tests/test_actions.py b/unit_tests/test_actions.py index 330e2d89..ed1f060d 100644 --- a/unit_tests/test_actions.py +++ b/unit_tests/test_actions.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import re from unittest import mock from functools import wraps @@ -307,3 +308,104 @@ class MainTestCase(RabbitActionTestCase): with mock.patch.dict(actions.ACTIONS, {"foo": dummy_action}): actions.main(["foo"]) self.assertEqual(dummy_calls, ["Action foo failed: uh oh"]) + + +class ListServiceUsersTestCase(CharmTestCase): + + def setUp(self): + super().setUp(actions, ['action_set', 'get_usernames_for_passwords']) + + def test_list_service_usernames(self): + self.get_usernames_for_passwords.return_value = ['a', 'b'] + actions.list_service_usernames([]) + self.get_usernames_for_passwords.assert_called_once_with() + self.action_set.assert_called_once_with( + {'usernames': ['a', 'b']}) + + +class RotateServiceUserPasswordTestCase(CharmTestCase): + + def setUp(self): + super().setUp(actions, ['action_get', 'action_fail']) + + def _assert_regex_in(self, regex, mock_item): + pattern = re.compile(regex) + calls = mock_item.call_args_list + for call in calls: + args = call[0] + msg = args[0] + print("message: {}".format(msg)) + print("regex: {}".format(regex)) + if pattern.match(msg): + print("pattern matched.") + return + self.fail("regex {} not found in any log.".format(regex)) + + @mock.patch.object(actions.rabbitmq_server_relations, + 'rotate_service_user_password') + def test_rotate_service_user_password( + self, mock_rotate_service_user_password): + self.action_get.return_value = 'keystone' + + actions.rotate_service_user_password([]) + + self.action_get.assert_called_once_with('service-user') + mock_rotate_service_user_password.assert_called_once_with('keystone') + + @mock.patch.object(actions.rabbitmq_server_relations, + 'rotate_service_user_password') + def test_rotate_service_user_password__service_user_none( + self, mock_rotate_service_user_password): + self.action_get.return_value = None + + actions.rotate_service_user_password([]) + + self.action_fail.assert_called_once_with( + "The 'service-user' parameter was not passed and is required.") + mock_rotate_service_user_password.assert_not_called() + + @mock.patch.object(actions.rabbitmq_server_relations, + 'rotate_service_user_password') + def test_rotate_service_user_password__not_leader( + self, mock_rotate_service_user_password): + self.action_get.return_value = 'keystone' + + def _error(*args, **kwargs): + raise actions.NotLeaderError('bang') + + mock_rotate_service_user_password.side_effect = _error + + actions.rotate_service_user_password([]) + + self._assert_regex_in(r"^This unit .* isn't the leader", + self.action_fail) + + @mock.patch.object(actions.rabbitmq_server_relations, + 'rotate_service_user_password') + def test_rotate_service_user_password__invalid_service_user( + self, mock_rotate_service_user_password): + self.action_get.return_value = 'keystone' + + def _error(*args, **kwargs): + raise actions.InvalidServiceUserError('bang') + + mock_rotate_service_user_password.side_effect = _error + + actions.rotate_service_user_password([]) + + self._assert_regex_in(r"^Service username .* not valid", + self.action_fail) + + @mock.patch.object(actions.rabbitmq_server_relations, + 'rotate_service_user_password') + def test_rotate_service_user_password__other_exception( + self, mock_rotate_service_user_password): + self.action_get.return_value = 'keystone' + + def _error(*args, **kwargs): + raise actions.Exception('bang') + + mock_rotate_service_user_password.side_effect = _error + + with self.assertRaises(Exception): + actions.rotate_service_user_password([]) diff --git a/unit_tests/test_rabbit_utils.py b/unit_tests/test_rabbit_utils.py index 5f61f5ba..16faff3f 100644 --- a/unit_tests/test_rabbit_utils.py +++ b/unit_tests/test_rabbit_utils.py @@ -1630,6 +1630,89 @@ class UtilsTests(CharmTestCase): mock_rabbitmqctl.assert_called_once_with( 'set_user_tags', 'user1', 'monitor') + @mock.patch.object(rabbit_utils, 'rabbitmqctl') + @mock.patch.object(rabbit_utils, 'user_exists') + @mock.patch("rabbit_utils.log") + def test_change_user_password__user_doesnt_exist( + self, + mock_log, + mock_user_exists, + mock_rabbitmqctl + ): + mock_user_exists.return_value = False + with self.assertRaises(KeyError): + rabbit_utils.change_user_password('a-user', 'new-password') + mock_rabbitmqctl.assert_not_called() + + @mock.patch.object(rabbit_utils, 'rabbitmqctl') + @mock.patch.object(rabbit_utils, 'user_exists') + @mock.patch("rabbit_utils.log") + def test_change_user_password( + self, + mock_log, + mock_user_exists, + mock_rabbitmqctl + ): + mock_user_exists.return_value = True + rabbit_utils.change_user_password('a-user', 'new-password') + mock_rabbitmqctl.assert_called_once_with( + 'change_password', 'a-user', 'new-password') + + @mock.patch.object(rabbit_utils, 'service_name') + @mock.patch('glob.glob') + def test_get_usernames_for_passwords_on_disk( + self, mock_glob, mock_service_name): + mock_glob.return_value = ['a.passwd', 'b.passwd'] + mock_service_name.return_value = 'the-service' + self.assertEqual(rabbit_utils.get_usernames_for_passwords_on_disk(), + ['a', 'b']) + mock_glob.assert_called_once_with( + rabbit_utils._service_password_glob.format('the-service')) + + @mock.patch.object(rabbit_utils, 'peer_retrieve') + @mock.patch.object(rabbit_utils, 'get_usernames_for_passwords_on_disk') + def test_get_usernames_for_passwords__peer_retrieve_is_none( + self, + mock_get_usernames_for_passwords_on_disk, + mock_peer_retrieve, + ): + mock_peer_retrieve.return_value = None + mock_get_usernames_for_passwords_on_disk.return_value = ['a'] + self.assertEqual(rabbit_utils.get_usernames_for_passwords(), ['a']) + mock_peer_retrieve.assert_called_once_with(None) + + @mock.patch.object(rabbit_utils, 'peer_retrieve') + @mock.patch.object(rabbit_utils, 'get_usernames_for_passwords_on_disk') + def test_get_usernames_for_passwords__peer_retrieve_raises_valueerror( + self, + mock_get_usernames_for_passwords_on_disk, + mock_peer_retrieve, + ): + def _error(*args, **kwargs): + raise ValueError('bang') + + mock_peer_retrieve.return_value = ['b.passwd'] + mock_peer_retrieve.side_effect = _error + mock_get_usernames_for_passwords_on_disk.return_value = ['a'] + self.assertEqual(rabbit_utils.get_usernames_for_passwords(), ['a']) + mock_peer_retrieve.assert_called_once_with(None) + + @mock.patch.object(rabbit_utils, 'peer_retrieve') + @mock.patch.object(rabbit_utils, 'get_usernames_for_passwords_on_disk') + def test_get_usernames_for_passwords__peer_retrieve_ok( + self, + mock_get_usernames_for_passwords_on_disk, + mock_peer_retrieve, + ): + mock_peer_retrieve.return_value = { + 'c.passwd': 'c-pass', + 'b.passwd': 'b-pass', + 'z.thing': 'thing' + } + mock_get_usernames_for_passwords_on_disk.return_value = ['a'] + self.assertEqual(rabbit_utils.get_usernames_for_passwords(), + ['b', 'c']) + @mock.patch.object(rabbit_utils, 'rabbitmqctl') @mock.patch.object(rabbit_utils, 'list_user_vhost_permissions') def test_grant_permissions(self, mock_list_user_vhost_permissions, diff --git a/unit_tests/test_rabbitmq_server_relations.py b/unit_tests/test_rabbitmq_server_relations.py index d86ffbe0..1f9d8478 100644 --- a/unit_tests/test_rabbitmq_server_relations.py +++ b/unit_tests/test_rabbitmq_server_relations.py @@ -328,6 +328,119 @@ class RelationUtil(CharmTestCase): if os.path.exists(tmpdir): shutil.rmtree(tmpdir) + @patch.object(rabbitmq_server_relations.rabbit, 'leader_node_is_ready') + def test_rotate_service_user_password__not_leader( + self, + mock_leader_node_is_ready, + ): + mock_leader_node_is_ready.return_value = False + with self.assertRaises( + rabbitmq_server_relations.rabbit.NotLeaderError): + rabbitmq_server_relations.rotate_service_user_password('glance') + + @patch.object(rabbitmq_server_relations.rabbit, + 'get_usernames_for_passwords') + @patch.object(rabbitmq_server_relations.rabbit, 'leader_node_is_ready') + def test_rotate_service_user_password__invalid_username( + self, + mock_leader_node_is_ready, + mock_get_usernames_for_passwords, + ): + mock_leader_node_is_ready.return_value = True + mock_get_usernames_for_passwords.return_value = ['cinder', 'glance'] + with self.assertRaises( + rabbitmq_server_relations.rabbit.InvalidServiceUserError): + rabbitmq_server_relations.rotate_service_user_password('other') + + @patch('rabbitmq_server_relations.relation_get') + @patch('rabbitmq_server_relations.relation_set') + @patch.object(rabbitmq_server_relations, 'leader_set') + @patch.object(rabbitmq_server_relations, 'peer_store') + @patch.object(rabbitmq_server_relations.rabbit, 'change_user_password') + @patch.object(rabbitmq_server_relations, 'pwgen') + @patch.object(rabbitmq_server_relations.rabbit, + 'get_usernames_for_passwords') + @patch.object(rabbitmq_server_relations.rabbit, 'leader_node_is_ready') + def test_rotate_service_user_password__peer_store_valueerror( + self, + mock_leader_node_is_ready, + mock_get_usernames_for_passwords, + mock_pwgen, + mock_change_user_password, + mock_peer_store, + mock_leader_set, + mock_relation_set, + mock_relation_get + ): + mock_leader_node_is_ready.return_value = True + mock_get_usernames_for_passwords.return_value = ['cinder', 'glance'] + self.relation_ids.return_value = ['amqp:0'] + self.related_units.return_value = ['glance/0'] + mock_relation_get.return_value = { + 'username': 'glance', + } + mock_pwgen.return_value = "new-password" + + def _error(*args, **kwargs): + raise ValueError('bang') + + mock_peer_store.side_effect = _error + + rabbitmq_server_relations.rotate_service_user_password('glance') + + mock_change_user_password.assert_called_once_with( + 'glance', 'new-password') + mock_peer_store.assert_has_calls([ + call('glance.passwd', 'new-password'), + call('amqp:0_password', 'new-password')]) + mock_leader_set.assert_has_calls([ + call({'glance.passwd': 'new-password'}), + call({'amqp:0_password': 'new-password'})]) + mock_relation_set.assert_called_once_with( + relation_id='amqp:0', + relation_settings={'password': 'new-password'}) + + @patch('rabbitmq_server_relations.relation_get') + @patch('rabbitmq_server_relations.relation_set') + @patch.object(rabbitmq_server_relations, 'leader_set') + @patch.object(rabbitmq_server_relations, 'peer_store') + @patch.object(rabbitmq_server_relations.rabbit, 'change_user_password') + @patch.object(rabbitmq_server_relations, 'pwgen') + @patch.object(rabbitmq_server_relations.rabbit, + 'get_usernames_for_passwords') + @patch.object(rabbitmq_server_relations.rabbit, 'leader_node_is_ready') + def test_rotate_service_user_password__peer_store_ok( + self, + mock_leader_node_is_ready, + mock_get_usernames_for_passwords, + mock_pwgen, + mock_change_user_password, + mock_peer_store, + mock_leader_set, + mock_relation_set, + mock_relation_get + ): + mock_leader_node_is_ready.return_value = True + mock_get_usernames_for_passwords.return_value = ['cinder', 'glance'] + self.relation_ids.return_value = ['amqp:0'] + self.related_units.return_value = ['glance/0'] + mock_relation_get.return_value = { + 'some_username': 'glance', + } + mock_pwgen.return_value = "new-password" + + rabbitmq_server_relations.rotate_service_user_password('glance') + + mock_change_user_password.assert_called_once_with( + 'glance', 'new-password') + mock_peer_store.assert_has_calls([ + call('glance.passwd', 'new-password'), + call('amqp:0_some_password', 'new-password')]) + mock_leader_set.assert_not_called() + mock_relation_set.assert_called_once_with( + relation_id='amqp:0', + relation_settings={'some_password': 'new-password'}) + @patch('rabbit_utils.lsb_release') @patch.object(rabbitmq_server_relations.rabbit, 'grant_permissions') @patch('rabbit_utils.create_user')