From f9aa92c7ce835a4e0790251469b1f817e17c0730 Mon Sep 17 00:00:00 2001 From: Aurelien Lourot Date: Tue, 4 Aug 2020 12:40:47 +0200 Subject: [PATCH] Do not leak credentials on leader-set failure This will also give us more insights into the leader-set failure happening in the linked bug. Also updated project files from latest release-tools templates. Also blacklisted libjuju 2.8.3 which causes spurious JujuAPIError's. Change-Id: I51b890098df6d918c1d84adba272559ef45411bb Partial-Bug: #1890256 --- hooks/keystone_utils.py | 29 ++++++++++++++++++++++------- requirements.txt | 1 + test-requirements.txt | 4 +++- tox.ini | 2 +- unit_tests/test_keystone_utils.py | 23 ++++++++++++++++++----- 5 files changed, 45 insertions(+), 14 deletions(-) diff --git a/hooks/keystone_utils.py b/hooks/keystone_utils.py index 7fce0cec..349eaab1 100644 --- a/hooks/keystone_utils.py +++ b/hooks/keystone_utils.py @@ -1462,7 +1462,7 @@ def set_admin_passwd(passwd, user=None): if user is None: user = config('admin-user') - leader_set({'{}_passwd'.format(user): passwd}) + _leader_set_secret({'{}_passwd'.format(user): passwd}) def get_api_version(): @@ -1620,7 +1620,7 @@ def _migrate_admin_password(): if is_leader() and os.path.exists(STORED_PASSWD): log('Migrating on-disk stored passwords to leader storage') with open(STORED_PASSWD) as fd: - leader_set({"admin_passwd": fd.readline().strip('\n')}) + _leader_set_secret({"admin_passwd": fd.readline().strip('\n')}) os.unlink(STORED_PASSWD) @@ -1630,8 +1630,8 @@ def _migrate_service_passwords(): if is_leader() and os.path.exists(SERVICE_PASSWD_PATH): log('Migrating on-disk stored passwords to leader storage') creds = load_stored_passwords() - for k, v in creds.items(): - leader_set({"{}_passwd".format(k): v}) + _leader_set_secret({"{}_passwd".format(k): v + for k, v in creds.items()}) os.unlink(SERVICE_PASSWD_PATH) @@ -1645,7 +1645,7 @@ def get_service_password(service_username): def set_service_password(passwd, user): - leader_set({"{}_passwd".format(user): passwd}) + _leader_set_secret({"{}_passwd".format(user): passwd}) def is_password_changed(username, passwd): @@ -2535,7 +2535,7 @@ def key_leader_set(): `CREDENTIAL_KEY_REPOSITORY` directories. Note that this function will fail if it is called on the unit that is not the leader. - :raises: :class:`subprocess.CalledProcessError` if the leader_set fails. + :raises: :class:`RuntimeError` if the leader_set fails. """ disk_keys = {} for key_repository in [FERNET_KEY_REPOSITORY, CREDENTIAL_KEY_REPOSITORY]: @@ -2544,7 +2544,7 @@ def key_leader_set(): with open(os.path.join(key_repository, key_number), 'r') as f: disk_keys[key_repository][key_number] = f.read() - leader_set({'key_repository': json.dumps(disk_keys)}) + _leader_set_secret({'key_repository': json.dumps(disk_keys)}) def key_write(): @@ -2702,3 +2702,18 @@ def endpoints_dict(settings): } return endpoints + + +def _leader_set_secret(secret_dict): + """Wrapper around leader_set doing its best to prevent leaking secrets.""" + if not is_leader(): + raise RuntimeError("This unit isn't the leader and therefore can't " + "call leader_set() with the given secrets") + try: + leader_set(secret_dict) + except subprocess.CalledProcessError as e: + # Do not log 'e' or 'e.cmd' as this would leak secrets. Raise a + # different exception to make sure that no one above will leak the + # secrets by accident. + raise RuntimeError('leader-set failed with {}: {}'.format(e.returncode, + e.output)) diff --git a/requirements.txt b/requirements.txt index 2316401b..8ba19415 100644 --- a/requirements.txt +++ b/requirements.txt @@ -7,6 +7,7 @@ # requirements. They are intertwined. Also, Zaza itself should specify # all of its own requirements and if it doesn't, fix it there. # +setuptools<50.0.0 # https://github.com/pypa/setuptools/commit/04e3df22df840c6bb244e9b27bc56750c44b7c85 pbr>=1.8.0,<1.9.0 simplejson>=2.2.0 netifaces>=0.10.4 diff --git a/test-requirements.txt b/test-requirements.txt index 7d9c2587..98fa7bd3 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -7,12 +7,14 @@ # requirements. They are intertwined. Also, Zaza itself should specify # all of its own requirements and if it doesn't, fix it there. # +setuptools<50.0.0 # https://github.com/pypa/setuptools/commit/04e3df22df840c6bb244e9b27bc56750c44b7c85 charm-tools>=2.4.4 requests>=2.18.4 mock>=1.2 -flake8>=2.2.4,<=2.4.1 +flake8>=2.2.4 stestr>=2.2.0 coverage>=4.5.2 pyudev # for ceph-* charm unit tests (need to fix the ceph-* charm unit tests/mocking) +juju!=2.8.3 # this version causes spurious JujuAPIError's git+https://github.com/openstack-charmers/zaza.git#egg=zaza;python_version>='3.0' git+https://github.com/openstack-charmers/zaza-openstack-tests.git#egg=zaza.openstack diff --git a/tox.ini b/tox.ini index b835733a..e2d58f59 100644 --- a/tox.ini +++ b/tox.ini @@ -116,5 +116,5 @@ commands = functest-run-suite --keep-model --bundle {posargs} [flake8] -ignore = E402,E226 +ignore = E402,E226,W503,W504 exclude = */charmhelpers diff --git a/unit_tests/test_keystone_utils.py b/unit_tests/test_keystone_utils.py index 7f111dd0..def26a47 100644 --- a/unit_tests/test_keystone_utils.py +++ b/unit_tests/test_keystone_utils.py @@ -367,6 +367,7 @@ class TestKeystoneUtils(CharmTestCase): **relation_data) @patch.object(utils, 'get_real_role_names') + @patch.object(utils, 'is_leader') @patch.object(utils, 'leader_set') @patch.object(utils, 'leader_get') @patch.object(utils, 'get_api_version') @@ -378,10 +379,11 @@ class TestKeystoneUtils(CharmTestCase): def test_add_service_to_keystone_no_clustered_no_https_complete_values( self, KeystoneManager, add_endpoint, ensure_valid_service, _resolve_address, create_user, get_api_version, leader_get, - leader_set, _get_real_role_names, test_api_version=2): + leader_set, is_leader, _get_real_role_names, test_api_version=2): _get_real_role_names.return_value = ['Member', 'SpecialRole'] get_api_version.return_value = test_api_version leader_get.return_value = None + is_leader.return_value = True relation_id = 'identity-service:0' remote_unit = 'unit/0' self.get_service_password.return_value = 'password' @@ -1099,19 +1101,25 @@ class TestKeystoneUtils(CharmTestCase): self.assertEqual(utils.get_admin_passwd(), 'admin') leader_get.assert_called_with('test_passwd') + @patch.object(utils, 'is_leader') @patch.object(utils, 'leader_set') - def test_set_admin_password(self, leader_set): + def test_set_admin_password(self, leader_set, is_leader): + is_leader.return_value = True utils.set_admin_passwd('secret') leader_set.assert_called_once_with({'admin_passwd': 'secret'}) + @patch.object(utils, 'is_leader') @patch.object(utils, 'leader_set') - def test_set_admin_password_config_username(self, leader_set): + def test_set_admin_password_config_username(self, leader_set, is_leader): + is_leader.return_value = True self.test_config.set('admin-user', 'username') utils.set_admin_passwd('secret') leader_set.assert_called_once_with({'username_passwd': 'secret'}) + @patch.object(utils, 'is_leader') @patch.object(utils, 'leader_set') - def test_set_admin_password_username(self, leader_set): + def test_set_admin_password_username(self, leader_set, is_leader): + is_leader.return_value = True utils.set_admin_passwd('secret', user='username') leader_set.assert_called_once_with({'username_passwd': 'secret'}) @@ -1688,10 +1696,12 @@ class TestKeystoneUtils(CharmTestCase): utils.fernet_rotate() self.subprocess.check_output.called_with(cmd) + @patch.object(utils, 'is_leader') @patch.object(utils, 'leader_set') @patch('os.listdir') - def test_key_leader_set(self, listdir, leader_set): + def test_key_leader_set(self, listdir, leader_set, is_leader): listdir.return_value = ['0', '1'] + is_leader.return_value = True self.time.time.return_value = "the-time" with patch.object(builtins, 'open', mock_open( read_data="some_data")): @@ -1881,6 +1891,7 @@ class TestKeystoneUtils(CharmTestCase): mock_leader_get.assert_called_with('_charm-keystone-admin_passwd') @patch.object(utils, 'get_manager') + @patch.object(utils, 'is_leader') @patch.object(utils, 'leader_set') @patch.object(utils, 'resolve_address') @patch.object(utils, 'endpoint_url') @@ -1895,6 +1906,7 @@ class TestKeystoneUtils(CharmTestCase): mock_endpoint_url, mock_resolve_address, mock_leader_set, + mock_is_leader, mock_get_manager): configs = MagicMock() mock_get_api_suffix.return_value = 'suffix' @@ -1902,6 +1914,7 @@ class TestKeystoneUtils(CharmTestCase): mock_endpoint_url.side_effect = ( lambda x, y, z: 'http://{}:{}/{}'.format(x, y, z)) mock_leader_get.return_value = 'fakepassword' + mock_is_leader.return_value = True mock_get_manager().resolve_user_id.return_value = 'fakeid' self.os_release.return_value = 'queens' utils.bootstrap_keystone(configs=configs)