From 40097064295747103939321a9014c1f37da30666 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Wed, 30 Nov 2022 21:40:08 +0100 Subject: [PATCH 01/19] Add tests for state module and issue 63144 --- tests/pytests/unit/modules/test_gpg.py | 36 ++++- tests/pytests/unit/states/test_gpg.py | 206 +++++++++++++++++++++++++ 2 files changed, 241 insertions(+), 1 deletion(-) create mode 100644 tests/pytests/unit/states/test_gpg.py diff --git a/tests/pytests/unit/modules/test_gpg.py b/tests/pytests/unit/modules/test_gpg.py index 697f53c65418..96b1b086069e 100644 --- a/tests/pytests/unit/modules/test_gpg.py +++ b/tests/pytests/unit/modules/test_gpg.py @@ -15,7 +15,7 @@ import pytest import salt.modules.gpg as gpg -from tests.support.mock import MagicMock, call, patch +from tests.support.mock import MagicMock, Mock, call, patch pytest.importorskip("gnupg") @@ -1039,3 +1039,37 @@ def test_gpg_decrypt_message_with_gpg_passphrase_in_pillar(gpghome): gnupghome=str(gpghome.path), ) assert ret["res"] is True + + +def test_gpg_receive_keys_no_user_id(): + with patch("salt.modules.gpg._create_gpg") as create: + with patch.dict( + gpg.__salt__, {"user.info": MagicMock(), "config.option": Mock()} + ): + import_result = MagicMock() + import_result.__bool__.return_value = False + for var, val in { + "gpg": Mock(), + "imported": 0, + "results": [], + "fingerprints": [], + "count": 1, + "no_user_id": 0, + "imported_rsa": 0, + "unchanged": 0, + "n_uids": 0, + "n_subk": 0, + "n_sigs": 0, + "n_revoc": 0, + "sec_read": 0, + "sec_imported": 0, + "sec_dups": 0, + "not_imported": 0, + "stderr": "gpg: key ABCDEF0123456789: no user ID\ngpg: Total number processed: 1\n[GNUPG:] IMPORT_RES 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0\n", + "data": b"", + }.items(): + setattr(import_result, var, val) + create.return_value.recv_keys.return_value = import_result + res = gpg.receive_keys(keys="abc", user="abc") + assert res["res"] is False + assert "no user ID" in res["message"] diff --git a/tests/pytests/unit/states/test_gpg.py b/tests/pytests/unit/states/test_gpg.py new file mode 100644 index 000000000000..9c3959a80fd2 --- /dev/null +++ b/tests/pytests/unit/states/test_gpg.py @@ -0,0 +1,206 @@ +import pytest + +import salt.states.gpg as gpg +from tests.support.mock import Mock, patch + + +@pytest.fixture +def configure_loader_modules(): + return {gpg: {"__opts__": {"test": False}}} + + +@pytest.fixture +def keys_list(): + return [ + { + "keyid": "A", + "fingerprint": "A", + "uids": ["Key A"], + "created": "2010-04-01", + "keyLength": "4096", + "ownerTrust": "Ultimately Trusted", + "trust": "Ultimately Trusted", + }, + { + "keyid": "B", + "fingerprint": "B", + "uids": ["Key B"], + "created": "2017-03-06", + "keyLength": "4096", + "ownerTrust": "Unknown", + "trust": "Fully Trusted", + }, + { + "keyid": "C", + "fingerprint": "C", + "uids": ["Key C"], + "expires": "2022-06-24", + "created": "2018-06-24", + "keyLength": "4096", + "ownerTrust": "Unknown", + "trust": "Expired", + }, + { + "keyid": "D", + "fingerprint": "D", + "uids": ["Key D"], + "created": "2018-01-18", + "keyLength": "3072", + "ownerTrust": "Unknown", + "trust": "Unknown", + }, + { + "keyid": "E", + "fingerprint": "E", + "uids": ["Key E"], + "expires": "2222-11-18", + "created": "2019-11-20", + "keyLength": "4096", + "ownerTrust": "Unknown", + "trust": "Unknown", + }, + ] + + +@pytest.fixture +def gpg_list_keys(request, keys_list): + list_ = Mock(spec="salt.modules.gpg.list_keys") + list_.return_value = getattr(request, "param", keys_list) + with patch.dict(gpg.__salt__, {"gpg.list_keys": list_}): + yield list_ + + +@pytest.fixture +def gpg_trust(request): + trust = Mock(spec="salt.modules.gpg.trust_key") + trust.return_value = getattr(request, "param", {}) + with patch.dict(gpg.__salt__, {"gpg.trust_key": trust}): + yield trust + + +@pytest.fixture() +def gpg_receive(request): + recv = Mock(spec="salt.modules.gpg.receive_keys") + recv.return_value = getattr(request, "param", {}) + with patch.dict(gpg.__salt__, {"gpg.receive_keys": recv}): + yield recv + + +@pytest.fixture() +def gpg_delete(request): + delete = Mock(spec="salt.modules.gpg.delete_key") + delete.return_value = getattr( + request, "param", {"res": True, "message": ["Public key for A deleted"]} + ) + with patch.dict(gpg.__salt__, {"gpg.delete_key": delete}): + yield delete + + +@pytest.mark.usefixtures("gpg_list_keys") +def test_gpg_present_no_changes(gpg_receive, gpg_trust): + ret = gpg.present("A") + assert ret["result"] + assert not ret["changes"] + gpg_receive.assert_not_called() + gpg_trust.assert_not_called() + + +@pytest.mark.usefixtures("gpg_list_keys") +@pytest.mark.parametrize( + "gpg_trust,expected", + [ + ({"res": True, "message": ["Setting ownership trust to Marginally"]}, True), + ({"res": False, "message": ["KeyID A not in GPG keychain"]}, False), + ], + indirect=["gpg_trust"], +) +def test_gpg_present_trust_change(gpg_receive, gpg_trust, expected): + ret = gpg.present("A", trust="marginally") + assert ret["result"] == expected + assert bool(ret["changes"]) == expected + gpg_trust.assert_called_once() + gpg_receive.assert_not_called() + + +@pytest.mark.usefixtures("gpg_list_keys") +@pytest.mark.parametrize( + "gpg_receive,expected", + [ + ({"res": True, "message": ["Key new added to keychain"]}, True), + ( + { + "res": False, + "message": [ + "Something went wrong during gpg call: gpg: key new: no user ID" + ], + }, + False, + ), + ], + indirect=["gpg_receive"], +) +def test_gpg_present_new_key(gpg_receive, gpg_trust, expected): + ret = gpg.present("new") + assert ret["result"] == expected + assert bool(ret["changes"]) == expected + gpg_receive.assert_called_once() + gpg_trust.assert_not_called() + + +@pytest.mark.usefixtures("gpg_list_keys") +@pytest.mark.parametrize( + "gpg_receive", + [ + {"res": True, "message": ["Key new added to keychain"]}, + ], + indirect=True, +) +@pytest.mark.parametrize( + "gpg_trust,expected", + [ + ({"res": True, "message": ["Setting ownership trust to Marginally"]}, True), + ({"res": False, "message": ["KeyID A not in GPG keychain"]}, False), + ], + indirect=["gpg_trust"], +) +@pytest.mark.usefixtures("gpg_list_keys") +def test_gpg_present_new_key_and_trust(gpg_receive, gpg_trust, expected): + ret = gpg.present("new", trust="marginally") + assert ret["result"] == expected + # the key is always marked as added + assert bool(ret["changes"]) + gpg_receive.assert_called_once() + gpg_trust.assert_called_once() + + +@pytest.mark.usefixtures("gpg_list_keys") +def test_gpg_absent_no_changes(gpg_delete): + ret = gpg.absent("nonexistent") + assert ret["result"] + assert not ret["changes"] + gpg_delete.assert_not_called() + + +@pytest.mark.usefixtures("gpg_list_keys") +@pytest.mark.parametrize( + "gpg_delete,expected", + [ + ({"res": True, "message": ["Public key for A deleted"]}, True), + ( + { + "res": False, + "message": [ + "Secret key exists, delete first or pass delete_secret=True." + ], + }, + False, + ), + ], + indirect=["gpg_delete"], +) +@pytest.mark.usefixtures("gpg_list_keys") +def test_gpg_absent_delete_key(gpg_delete, expected): + ret = gpg.absent("A") + assert ret["result"] == expected + assert bool(ret["changes"]) == expected + gpg_delete.assert_called_once() From f643152b6eefd67dd7e8b15eb20efe757184a727 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Wed, 30 Nov 2022 21:49:11 +0100 Subject: [PATCH 02/19] Make gpg state module report correct result and changes --- changelog/63153.fixed | 1 + salt/states/gpg.py | 55 +++++++++++++++++++++++-------------------- 2 files changed, 31 insertions(+), 25 deletions(-) create mode 100644 changelog/63153.fixed diff --git a/changelog/63153.fixed b/changelog/63153.fixed new file mode 100644 index 000000000000..f1c617f8cd60 --- /dev/null +++ b/changelog/63153.fixed @@ -0,0 +1 @@ +Fixed GPG state module always reports success without changes diff --git a/salt/states/gpg.py b/salt/states/gpg.py index 4329e6685a63..1c0697d45ca5 100644 --- a/salt/states/gpg.py +++ b/salt/states/gpg.py @@ -8,6 +8,8 @@ import logging +import salt.utils.dictupdate + log = logging.getLogger(__name__) _VALID_TRUST_VALUES = [ @@ -86,23 +88,24 @@ def present( trust_level=trust, user=user, ) - if "result" in result and not result["result"]: - ret["result"] = result["result"] - ret["comment"].append(result["comment"]) + if result["res"] is False: + ret["result"] = result["res"] + ret["comment"].extend(result["message"]) else: + salt.utils.dictupdate.set_dict_key_value( + ret, f"changes:{key}:trust", trust + ) ret["comment"].append( - "Set trust level for {} to {}".format(key, trust) + f"Set trust level for {key} to {trust}" ) else: ret["comment"].append( - "GPG Public Key {} already in correct trust state".format( - key - ) + f"GPG Public Key {key} already in correct trust state" ) else: - ret["comment"].append("Invalid trust level {}".format(trust)) + ret["comment"].append(f"Invalid trust level {trust}") - ret["comment"].append("GPG Public Key {} already in keychain ".format(key)) + ret["comment"].append(f"GPG Public Key {key} already in keychain") else: result = __salt__["gpg.receive_keys"]( @@ -111,11 +114,14 @@ def present( user, gnupghome, ) - if "result" in result and not result["result"]: - ret["result"] = result["result"] - ret["comment"].append(result["comment"]) + if result["res"] is False: + ret["result"] = result["res"] + ret["comment"].extend(result["message"]) else: - ret["comment"].append("Adding {} to GPG keychain".format(name)) + ret["comment"].append(f"Added {key} to GPG keychain") + salt.utils.dictupdate.set_dict_key_value( + ret, f"changes:{key}:added", True + ) if trust: if trust in _VALID_TRUST_VALUES: @@ -124,15 +130,13 @@ def present( trust_level=trust, user=user, ) - if "result" in result and not result["result"]: - ret["result"] = result["result"] - ret["comment"].append(result["comment"]) + if result["res"] is False: + ret["result"] = result["res"] + ret["comment"].extend(result["message"]) else: - ret["comment"].append( - "Set trust level for {} to {}".format(key, trust) - ) + ret["comment"].append(f"Set trust level for {key} to {trust}") else: - ret["comment"].append("Invalid trust level {}".format(trust)) + ret["comment"].append(f"Invalid trust level {trust}") ret["comment"] = "\n".join(ret["comment"]) return ret @@ -177,12 +181,13 @@ def absent(name, keys=None, user=None, gnupghome=None, **kwargs): user, gnupghome, ) - if "result" in result and not result["result"]: - ret["result"] = result["result"] - ret["comment"].append(result["comment"]) + if result["res"] is False: + ret["result"] = result["res"] + ret["comment"].extend(result["message"]) else: - ret["comment"].append("Deleting {} from GPG keychain".format(name)) + ret["comment"].append(f"Deleted {key} from GPG keychain") + salt.utils.dictupdate.append_dict_key_value(ret, "changes:deleted", key) else: - ret["comment"].append("{} not found in GPG keychain".format(name)) + ret["comment"].append(f"{key} not found in GPG keychain") ret["comment"] = "\n".join(ret["comment"]) return ret From a925f271db80124efd639b18d76667d199a06da6 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Wed, 30 Nov 2022 21:53:17 +0100 Subject: [PATCH 03/19] Add tests for issue 63156 --- tests/pytests/unit/states/test_gpg.py | 37 ++++++++++++++++++++------- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/tests/pytests/unit/states/test_gpg.py b/tests/pytests/unit/states/test_gpg.py index 9c3959a80fd2..4d352d68e895 100644 --- a/tests/pytests/unit/states/test_gpg.py +++ b/tests/pytests/unit/states/test_gpg.py @@ -73,7 +73,11 @@ def gpg_list_keys(request, keys_list): @pytest.fixture def gpg_trust(request): trust = Mock(spec="salt.modules.gpg.trust_key") - trust.return_value = getattr(request, "param", {}) + trust.return_value = getattr( + request, + "param", + {"res": True, "message": ["Setting ownership trust to Marginally"]}, + ) with patch.dict(gpg.__salt__, {"gpg.trust_key": trust}): yield trust @@ -81,7 +85,9 @@ def gpg_trust(request): @pytest.fixture() def gpg_receive(request): recv = Mock(spec="salt.modules.gpg.receive_keys") - recv.return_value = getattr(request, "param", {}) + recv.return_value = getattr( + request, "param", {"res": True, "message": ["Key new added to keychain"]} + ) with patch.dict(gpg.__salt__, {"gpg.receive_keys": recv}): yield recv @@ -148,13 +154,6 @@ def test_gpg_present_new_key(gpg_receive, gpg_trust, expected): @pytest.mark.usefixtures("gpg_list_keys") -@pytest.mark.parametrize( - "gpg_receive", - [ - {"res": True, "message": ["Key new added to keychain"]}, - ], - indirect=True, -) @pytest.mark.parametrize( "gpg_trust,expected", [ @@ -173,6 +172,17 @@ def test_gpg_present_new_key_and_trust(gpg_receive, gpg_trust, expected): gpg_trust.assert_called_once() +@pytest.mark.usefixtures("gpg_list_keys") +@pytest.mark.parametrize("key,trust", [("new", None), ("A", "marginally")]) +def test_gpg_present_test_mode_no_changes(gpg_receive, gpg_trust, key, trust): + with patch.dict(gpg.__opts__, {"test": True}): + ret = gpg.present(key, trust=trust) + gpg_receive.assert_not_called() + gpg_trust.assert_not_called() + assert ret["result"] is None + assert bool(ret["changes"]) + + @pytest.mark.usefixtures("gpg_list_keys") def test_gpg_absent_no_changes(gpg_delete): ret = gpg.absent("nonexistent") @@ -204,3 +214,12 @@ def test_gpg_absent_delete_key(gpg_delete, expected): assert ret["result"] == expected assert bool(ret["changes"]) == expected gpg_delete.assert_called_once() + + +@pytest.mark.usefixtures("gpg_list_keys") +def test_gpg_absent_test_mode_no_changes(gpg_delete): + with patch.dict(gpg.__opts__, {"test": True}): + ret = gpg.absent("A") + gpg_delete.assert_not_called() + assert ret["result"] is None + assert bool(ret["changes"]) From f59c5453bce7866cce3755d8bc66626a820a4ad5 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Wed, 30 Nov 2022 21:55:40 +0100 Subject: [PATCH 04/19] Make gpg state module respect test mode --- changelog/63156.fixed | 1 + salt/states/gpg.py | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+) create mode 100644 changelog/63156.fixed diff --git a/changelog/63156.fixed b/changelog/63156.fixed new file mode 100644 index 000000000000..85efe22e2262 --- /dev/null +++ b/changelog/63156.fixed @@ -0,0 +1 @@ +Fixed GPG state module does not respect test mode diff --git a/salt/states/gpg.py b/salt/states/gpg.py index 1c0697d45ca5..bbe1183d79b2 100644 --- a/salt/states/gpg.py +++ b/salt/states/gpg.py @@ -82,6 +82,15 @@ def present( if trust: if trust in _VALID_TRUST_VALUES: if current_keys[key]["trust"] != TRUST_MAP[trust]: + if __opts__["test"]: + ret["result"] = None + ret["comment"].append( + f"Would have set trust level for {key} to {trust}" + ) + salt.utils.dictupdate.set_dict_key_value( + ret, f"changes:{key}:trust", trust + ) + continue # update trust level result = __salt__["gpg.trust_key"]( keyid=key, @@ -108,6 +117,13 @@ def present( ret["comment"].append(f"GPG Public Key {key} already in keychain") else: + if __opts__["test"]: + ret["result"] = None + ret["comment"].append(f"Would have added {key} to GPG keychain") + salt.utils.dictupdate.set_dict_key_value( + ret, f"changes:{key}:added", True + ) + continue result = __salt__["gpg.receive_keys"]( keyserver, key, @@ -176,6 +192,11 @@ def absent(name, keys=None, user=None, gnupghome=None, **kwargs): for key in keys: if key in current_keys: + if __opts__["test"]: + ret["result"] = None + ret["comment"].append(f"Would have deleted {key} from GPG keychain") + salt.utils.dictupdate.append_dict_key_value(ret, "changes:deleted", key) + continue result = __salt__["gpg.delete_key"]( key, user, From 483d80727a2a75299bff51996c4aa180a6fe1630 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Wed, 30 Nov 2022 22:07:25 +0100 Subject: [PATCH 05/19] Fix gpg.receive_keys returns success on failed import --- changelog/63144.fixed | 1 + salt/modules/gpg.py | 39 +++++++++++++++++--------- tests/pytests/unit/modules/test_gpg.py | 2 +- 3 files changed, 28 insertions(+), 14 deletions(-) create mode 100644 changelog/63144.fixed diff --git a/changelog/63144.fixed b/changelog/63144.fixed new file mode 100644 index 000000000000..cc100dcd21f7 --- /dev/null +++ b/changelog/63144.fixed @@ -0,0 +1 @@ +Fixed gpg.receive_keys returns success on failed import diff --git a/salt/modules/gpg.py b/salt/modules/gpg.py index 60ffa8ef092d..af9f10cdb240 100644 --- a/salt/modules/gpg.py +++ b/salt/modules/gpg.py @@ -909,7 +909,7 @@ def receive_keys(keyserver=None, keys=None, user=None, gnupghome=None): salt '*' gpg.receive_keys keys=3FAD9F1E user=username """ - ret = {"res": True, "changes": {}, "message": []} + ret = {"res": True, "message": []} gpg = _create_gpg(user, gnupghome) @@ -920,18 +920,31 @@ def receive_keys(keyserver=None, keys=None, user=None, gnupghome=None): keys = keys.split(",") recv_data = gpg.recv_keys(keyserver, *keys) - for result in recv_data.results: - if "ok" in result: - if result["ok"] == "1": - ret["message"].append( - "Key {} added to keychain".format(result["fingerprint"]) - ) - elif result["ok"] == "0": - ret["message"].append( - "Key {} already exists in keychain".format(result["fingerprint"]) - ) - elif "problem" in result: - ret["message"].append("Unable to add key to keychain") + try: + if recv_data.results: + for result in recv_data.results: + if "ok" in result: + if result["ok"] == "1": + ret["message"].append( + "Key {} added to keychain".format(result["fingerprint"]) + ) + elif result["ok"] == "0": + ret["message"].append( + "Key {} already exists in keychain".format( + result["fingerprint"] + ) + ) + elif "problem" in result: + ret["message"].append("Unable to add key to keychain") + elif not bool(recv_data): + ret["res"] = False + ret["message"] = [ + f"Something went wrong during gpg call: {recv_data.stderr}" + ] + except AttributeError: + ret["res"] = False + ret["message"] = ["Invalid return from python-gpg"] + return ret diff --git a/tests/pytests/unit/modules/test_gpg.py b/tests/pytests/unit/modules/test_gpg.py index 96b1b086069e..a5d18639ddef 100644 --- a/tests/pytests/unit/modules/test_gpg.py +++ b/tests/pytests/unit/modules/test_gpg.py @@ -1072,4 +1072,4 @@ def test_gpg_receive_keys_no_user_id(): create.return_value.recv_keys.return_value = import_result res = gpg.receive_keys(keys="abc", user="abc") assert res["res"] is False - assert "no user ID" in res["message"] + assert "no user ID" in res["message"][0] From 6a85acb34de2253b7b8b3c587ed078608211c72a Mon Sep 17 00:00:00 2001 From: jeanluc Date: Wed, 30 Nov 2022 22:53:42 +0100 Subject: [PATCH 06/19] Add tests for issue 63159 --- tests/pytests/unit/modules/test_gpg.py | 12 ++++++++++++ tests/pytests/unit/states/test_gpg.py | 16 ++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/tests/pytests/unit/modules/test_gpg.py b/tests/pytests/unit/modules/test_gpg.py index a5d18639ddef..0a2595081614 100644 --- a/tests/pytests/unit/modules/test_gpg.py +++ b/tests/pytests/unit/modules/test_gpg.py @@ -1073,3 +1073,15 @@ def test_gpg_receive_keys_no_user_id(): res = gpg.receive_keys(keys="abc", user="abc") assert res["res"] is False assert "no user ID" in res["message"][0] + + +def test_gpg_delete_key_honors_gnupghome(): + with patch("salt.modules.gpg._create_gpg") as create: + with patch("salt.modules.gpg.get_key") as get_key: + gnupghome = "/pls_respect_me" + get_key.return_value = None + gpg.delete_key("foo", gnupghome=gnupghome) + create.assert_called_with(None, gnupghome) + get_key.assert_called_with( + keyid="foo", fingerprint=None, user=None, gnupghome=gnupghome + ) diff --git a/tests/pytests/unit/states/test_gpg.py b/tests/pytests/unit/states/test_gpg.py index 4d352d68e895..2dca40eb09f5 100644 --- a/tests/pytests/unit/states/test_gpg.py +++ b/tests/pytests/unit/states/test_gpg.py @@ -223,3 +223,19 @@ def test_gpg_absent_test_mode_no_changes(gpg_delete): gpg_delete.assert_not_called() assert ret["result"] is None assert bool(ret["changes"]) + + +def test_gpg_absent_list_keys_with_gnupghome_and_user(gpg_list_keys): + gnupghome = "/pls_respect_me" + user = "imthereaswell" + gpg.absent("nonexistent", gnupghome=gnupghome, user=user) + gpg_list_keys.assert_called_with(gnupghome=gnupghome, user=user) + + +@pytest.mark.usefixtures("gpg_list_keys") +def test_gpg_absent_delete_key_called_with_correct_kwargs(gpg_delete): + key = "A" + user = "hellothere" + gnupghome = "/pls_sir" + gpg.absent(key, user=user, gnupghome=gnupghome) + gpg_delete.assert_called_with(keyid=key, gnupghome=gnupghome, user=user) From 1be95d4a8d74c1adc347822bb7f5a15d11c2e432 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Wed, 30 Nov 2022 22:56:31 +0100 Subject: [PATCH 07/19] Make deleting keys with user/gnupghome possible --- changelog/63159.fixed | 1 + salt/modules/gpg.py | 2 +- salt/states/gpg.py | 8 ++++---- 3 files changed, 6 insertions(+), 5 deletions(-) create mode 100644 changelog/63159.fixed diff --git a/changelog/63159.fixed b/changelog/63159.fixed new file mode 100644 index 000000000000..914bb703b285 --- /dev/null +++ b/changelog/63159.fixed @@ -0,0 +1 @@ +Fixed gpg.absent with gnupghome/user, fixed gpg.delete_key with gnupghome diff --git a/salt/modules/gpg.py b/salt/modules/gpg.py index af9f10cdb240..4efc349ef4c4 100644 --- a/salt/modules/gpg.py +++ b/salt/modules/gpg.py @@ -556,7 +556,7 @@ def delete_key( return ret gpg = _create_gpg(user, gnupghome) - key = get_key(keyid, fingerprint, user) + key = get_key(keyid=keyid, fingerprint=fingerprint, user=user, gnupghome=gnupghome) def __delete_key(fingerprint, secret, use_passphrase): if use_passphrase: diff --git a/salt/states/gpg.py b/salt/states/gpg.py index bbe1183d79b2..4b465bb8c4ce 100644 --- a/salt/states/gpg.py +++ b/salt/states/gpg.py @@ -178,7 +178,7 @@ def absent(name, keys=None, user=None, gnupghome=None, **kwargs): ret = {"name": name, "result": True, "changes": {}, "comment": []} - _current_keys = __salt__["gpg.list_keys"]() + _current_keys = __salt__["gpg.list_keys"](user=user, gnupghome=gnupghome) current_keys = [] for key in _current_keys: @@ -198,9 +198,9 @@ def absent(name, keys=None, user=None, gnupghome=None, **kwargs): salt.utils.dictupdate.append_dict_key_value(ret, "changes:deleted", key) continue result = __salt__["gpg.delete_key"]( - key, - user, - gnupghome, + keyid=key, + user=user, + gnupghome=gnupghome, ) if result["res"] is False: ret["result"] = result["res"] From 0a31ef1d13587138479933fc71610b1e913d6eb2 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Wed, 30 Nov 2022 22:59:25 +0100 Subject: [PATCH 08/19] Fix gpg state module docs --- salt/states/gpg.py | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/salt/states/gpg.py b/salt/states/gpg.py index 4b465bb8c4ce..3a9503aaf2e1 100644 --- a/salt/states/gpg.py +++ b/salt/states/gpg.py @@ -35,30 +35,28 @@ def present( name, keys=None, user=None, keyserver=None, gnupghome=None, trust=None, **kwargs ): """ - Ensure GPG public key is present in keychain + Ensure a GPG public key is present in the GPG keychain. name - The unique name or keyid for the GPG public key. + The key ID of the GPG public key. keys - The keyId or keyIds to add to the GPG keychain. + The key ID or key IDs to add to the GPG keychain. user - Add GPG keys to the specified user's keychain + Add GPG keys to the specified user's keychain. keyserver The keyserver to retrieve the keys from. gnupghome - Override GNUPG Home directory + Override GnuPG home directory. trust Trust level for the key in the keychain, - ignored by default. Valid trust levels: + ignored by default. Valid trust levels: expired, unknown, not_trusted, marginally, fully, ultimately - - """ ret = {"name": name, "result": True, "changes": {}, "comment": []} @@ -160,20 +158,19 @@ def present( def absent(name, keys=None, user=None, gnupghome=None, **kwargs): """ - Ensure GPG public key is absent in keychain + Ensure a GPG public key is absent from the keychain. name - The unique name or keyid for the GPG public key. + The key ID of the GPG public key. keys - The keyId or keyIds to add to the GPG keychain. + The key ID or key IDs to remove from the GPG keychain. user - Remove GPG keys from the specified user's keychain + Remove GPG keys from the specified user's keychain. gnupghome - Override GNUPG Home directory - + Override GnuPG home directory. """ ret = {"name": name, "result": True, "changes": {}, "comment": []} From af4ab3fe6a570e85e6ca2b4cc31ef76d9e4ecb91 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Sun, 11 Dec 2022 14:05:31 +0100 Subject: [PATCH 09/19] Backport message fixes --- salt/states/gpg.py | 6 +++--- tests/pytests/unit/states/test_gpg.py | 18 ++++++++---------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/salt/states/gpg.py b/salt/states/gpg.py index 3a9503aaf2e1..14ad37d98f00 100644 --- a/salt/states/gpg.py +++ b/salt/states/gpg.py @@ -97,7 +97,7 @@ def present( ) if result["res"] is False: ret["result"] = result["res"] - ret["comment"].extend(result["message"]) + ret["comment"].append(result["message"]) else: salt.utils.dictupdate.set_dict_key_value( ret, f"changes:{key}:trust", trust @@ -146,7 +146,7 @@ def present( ) if result["res"] is False: ret["result"] = result["res"] - ret["comment"].extend(result["message"]) + ret["comment"].append(result["message"]) else: ret["comment"].append(f"Set trust level for {key} to {trust}") else: @@ -201,7 +201,7 @@ def absent(name, keys=None, user=None, gnupghome=None, **kwargs): ) if result["res"] is False: ret["result"] = result["res"] - ret["comment"].extend(result["message"]) + ret["comment"].append(result["message"]) else: ret["comment"].append(f"Deleted {key} from GPG keychain") salt.utils.dictupdate.append_dict_key_value(ret, "changes:deleted", key) diff --git a/tests/pytests/unit/states/test_gpg.py b/tests/pytests/unit/states/test_gpg.py index 2dca40eb09f5..2df53fbee17e 100644 --- a/tests/pytests/unit/states/test_gpg.py +++ b/tests/pytests/unit/states/test_gpg.py @@ -76,7 +76,7 @@ def gpg_trust(request): trust.return_value = getattr( request, "param", - {"res": True, "message": ["Setting ownership trust to Marginally"]}, + {"res": True, "message": "Setting ownership trust to Marginally"}, ) with patch.dict(gpg.__salt__, {"gpg.trust_key": trust}): yield trust @@ -96,7 +96,7 @@ def gpg_receive(request): def gpg_delete(request): delete = Mock(spec="salt.modules.gpg.delete_key") delete.return_value = getattr( - request, "param", {"res": True, "message": ["Public key for A deleted"]} + request, "param", {"res": True, "message": "Public key for A deleted"} ) with patch.dict(gpg.__salt__, {"gpg.delete_key": delete}): yield delete @@ -115,8 +115,8 @@ def test_gpg_present_no_changes(gpg_receive, gpg_trust): @pytest.mark.parametrize( "gpg_trust,expected", [ - ({"res": True, "message": ["Setting ownership trust to Marginally"]}, True), - ({"res": False, "message": ["KeyID A not in GPG keychain"]}, False), + ({"res": True, "message": "Setting ownership trust to Marginally"}, True), + ({"res": False, "message": "KeyID A not in GPG keychain"}, False), ], indirect=["gpg_trust"], ) @@ -157,8 +157,8 @@ def test_gpg_present_new_key(gpg_receive, gpg_trust, expected): @pytest.mark.parametrize( "gpg_trust,expected", [ - ({"res": True, "message": ["Setting ownership trust to Marginally"]}, True), - ({"res": False, "message": ["KeyID A not in GPG keychain"]}, False), + ({"res": True, "message": "Setting ownership trust to Marginally"}, True), + ({"res": False, "message": "KeyID A not in GPG keychain"}, False), ], indirect=["gpg_trust"], ) @@ -195,13 +195,11 @@ def test_gpg_absent_no_changes(gpg_delete): @pytest.mark.parametrize( "gpg_delete,expected", [ - ({"res": True, "message": ["Public key for A deleted"]}, True), + ({"res": True, "message": "Public key for A deleted"}, True), ( { "res": False, - "message": [ - "Secret key exists, delete first or pass delete_secret=True." - ], + "message": "Secret key exists, delete first or pass delete_secret=True.", }, False, ), From e2b6dc47f628beb3aa7efb876c6d19364183411b Mon Sep 17 00:00:00 2001 From: jeanluc Date: Sun, 11 Dec 2022 14:19:07 +0100 Subject: [PATCH 10/19] Backport more meaningful tests --- tests/pytests/functional/states/test_gpg.py | 150 ++++++++++++++++++++ tests/pytests/unit/modules/test_gpg.py | 12 -- tests/pytests/unit/states/test_gpg.py | 89 +----------- 3 files changed, 157 insertions(+), 94 deletions(-) create mode 100644 tests/pytests/functional/states/test_gpg.py diff --git a/tests/pytests/functional/states/test_gpg.py b/tests/pytests/functional/states/test_gpg.py new file mode 100644 index 000000000000..4809f7906522 --- /dev/null +++ b/tests/pytests/functional/states/test_gpg.py @@ -0,0 +1,150 @@ +import shutil +import subprocess + +import psutil +import pytest + +try: + import gnupg as gnupglib + + HAS_GNUPG = True +except ImportError: + HAS_GNUPG = False + + +pytestmark = [ + pytest.mark.skipif(HAS_GNUPG is False, reason="Needs python-gnupg library"), + pytest.mark.skip_if_binaries_missing("gpg", reason="Needs gpg binary"), +] + + +@pytest.fixture +def gpghome(tmp_path): + root = tmp_path / "gpghome" + root.mkdir(mode=0o0700) + try: + yield root + finally: + # Make sure we don't leave any gpg-agents running behind + gpg_connect_agent = shutil.which("gpg-connect-agent") + if gpg_connect_agent: + gnupghome = root / ".gnupg" + if not gnupghome.is_dir(): + gnupghome = root + try: + subprocess.run( + [gpg_connect_agent, "killagent", "/bye"], + env={"GNUPGHOME": str(gnupghome)}, + shell=False, + check=True, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + ) + except subprocess.CalledProcessError: + # This is likely CentOS 7 or Amazon Linux 2 + pass + + # If the above errored or was not enough, as a last resort, let's check + # the running processes. + for proc in psutil.process_iter(): + try: + if "gpg-agent" in proc.name(): + for arg in proc.cmdline(): + if str(root) in arg: + proc.terminate() + except Exception: # pylint: disable=broad-except + pass + + +@pytest.fixture +def gpg(loaders, states, gpghome): + try: + yield states.gpg + finally: + pass + + +@pytest.fixture +def key_a_fp(): + return "EF03765F59EE904930C8A781553A82A058C0C795" + + +@pytest.fixture +def key_a_pub(): + return """\ +-----BEGIN PGP PUBLIC KEY BLOCK----- + +mI0EY4fxHQEEAJvXEaaw+o/yZCwMOJbt5FQHbVMMDX/0YI8UdzsE5YCC4iKnoC3x +FwFdkevKj3qp+45iBGLLnalfXIcVGXJGACB+tPHgsfHaXSDQPSfmX6jbZ6pHosSm +v1tTixY+NTJzGL7hDLz2sAXTbYmTbXeE9ifWWk6NcIwZivUbhNRBM+KxABEBAAG0 +LUtleSBBIChHZW5lcmF0ZWQgYnkgU2FsdFN0YWNrKSA8a2V5YUBleGFtcGxlPojR +BBMBCAA7FiEE7wN2X1nukEkwyKeBVTqCoFjAx5UFAmOH8R0CGy8FCwkIBwICIgIG +FQoJCAsCBBYCAwECHgcCF4AACgkQVTqCoFjAx5XURAQAguOwI+49lG0Kby+Bsyv3 +of3GgxvhS1Qa7+ysj088az5GVt0pqVe3SbRVvn/jyC6yZvWuv94KdL3R7hCeEz2/ +JakCRJ4wxEsdeASE8t9H/oTqD0I5asMa9EMvn5ICEGeLsTeQb7OYYihTQj7HJLG6 +pDEmK8EhJDvV/9o0lnhm/9w= +=Wc0O +-----END PGP PUBLIC KEY BLOCK-----""" + + +@pytest.fixture +def gnupg(gpghome): + return gnupglib.GPG(gnupghome=str(gpghome)) + + +@pytest.fixture +def gnupg_keyring(gpghome, keyring): + return gnupglib.GPG(gnupghome=str(gpghome), keyring=keyring) + + +@pytest.fixture(params=["a"]) +def pubkeys_present(gnupg, request): + pubkeys = [request.getfixturevalue(f"key_{x}_pub") for x in request.param] + fingerprints = [request.getfixturevalue(f"key_{x}_fp") for x in request.param] + gnupg.import_keys("\n".join(pubkeys)) + present_keys = gnupg.list_keys() + for fp in fingerprints: + assert any(x["fingerprint"] == fp for x in present_keys) + yield + # cleanup is taken care of by gpghome and tmp_path + + +@pytest.mark.usefixtures("pubkeys_present") +def test_gpg_present_no_changes(gpghome, gpg, gnupg, key_a_fp): + assert gnupg.list_keys(keys=key_a_fp) + ret = gpg.present( + key_a_fp[-16:], trust="unknown", gnupghome=str(gpghome), keyserver="nonexistent" + ) + assert ret.result + assert not ret.changes + + +@pytest.mark.skip_unless_on_linux( + reason="Complains about deleting private keys first when they are absent" +) +@pytest.mark.usefixtures("pubkeys_present") +def test_gpg_absent(gpghome, gpg, gnupg, key_a_fp): + assert gnupg.list_keys(keys=key_a_fp) + ret = gpg.absent(key_a_fp[-16:], gnupghome=str(gpghome)) + assert ret.result + assert ret.changes + assert "deleted" in ret.changes + assert ret.changes["deleted"] + + +def test_gpg_absent_no_changes(gpghome, gpg, gnupg, key_a_fp): + assert not gnupg.list_keys(keys=key_a_fp) + ret = gpg.absent(key_a_fp[-16:], gnupghome=str(gpghome)) + assert ret.result + assert not ret.changes + + +@pytest.mark.usefixtures("pubkeys_present") +def test_gpg_absent_test_mode_no_changes(gpghome, gpg, gnupg, key_a_fp): + assert gnupg.list_keys(keys=key_a_fp) + ret = gpg.absent(key_a_fp[-16:], gnupghome=str(gpghome), test=True) + assert ret.result is None + assert ret.changes + assert "deleted" in ret.changes + assert ret.changes["deleted"] + assert gnupg.list_keys(keys=key_a_fp) diff --git a/tests/pytests/unit/modules/test_gpg.py b/tests/pytests/unit/modules/test_gpg.py index 0a2595081614..a5d18639ddef 100644 --- a/tests/pytests/unit/modules/test_gpg.py +++ b/tests/pytests/unit/modules/test_gpg.py @@ -1073,15 +1073,3 @@ def test_gpg_receive_keys_no_user_id(): res = gpg.receive_keys(keys="abc", user="abc") assert res["res"] is False assert "no user ID" in res["message"][0] - - -def test_gpg_delete_key_honors_gnupghome(): - with patch("salt.modules.gpg._create_gpg") as create: - with patch("salt.modules.gpg.get_key") as get_key: - gnupghome = "/pls_respect_me" - get_key.return_value = None - gpg.delete_key("foo", gnupghome=gnupghome) - create.assert_called_with(None, gnupghome) - get_key.assert_called_with( - keyid="foo", fingerprint=None, user=None, gnupghome=gnupghome - ) diff --git a/tests/pytests/unit/states/test_gpg.py b/tests/pytests/unit/states/test_gpg.py index 2df53fbee17e..13ba97cd493c 100644 --- a/tests/pytests/unit/states/test_gpg.py +++ b/tests/pytests/unit/states/test_gpg.py @@ -92,25 +92,6 @@ def gpg_receive(request): yield recv -@pytest.fixture() -def gpg_delete(request): - delete = Mock(spec="salt.modules.gpg.delete_key") - delete.return_value = getattr( - request, "param", {"res": True, "message": "Public key for A deleted"} - ) - with patch.dict(gpg.__salt__, {"gpg.delete_key": delete}): - yield delete - - -@pytest.mark.usefixtures("gpg_list_keys") -def test_gpg_present_no_changes(gpg_receive, gpg_trust): - ret = gpg.present("A") - assert ret["result"] - assert not ret["changes"] - gpg_receive.assert_not_called() - gpg_trust.assert_not_called() - - @pytest.mark.usefixtures("gpg_list_keys") @pytest.mark.parametrize( "gpg_trust,expected", @@ -122,8 +103,8 @@ def test_gpg_present_no_changes(gpg_receive, gpg_trust): ) def test_gpg_present_trust_change(gpg_receive, gpg_trust, expected): ret = gpg.present("A", trust="marginally") - assert ret["result"] == expected - assert bool(ret["changes"]) == expected + assert ret["result"] is expected + assert bool(ret["changes"]) is expected gpg_trust.assert_called_once() gpg_receive.assert_not_called() @@ -147,8 +128,8 @@ def test_gpg_present_trust_change(gpg_receive, gpg_trust, expected): ) def test_gpg_present_new_key(gpg_receive, gpg_trust, expected): ret = gpg.present("new") - assert ret["result"] == expected - assert bool(ret["changes"]) == expected + assert ret["result"] is expected + assert bool(ret["changes"]) is expected gpg_receive.assert_called_once() gpg_trust.assert_not_called() @@ -165,9 +146,9 @@ def test_gpg_present_new_key(gpg_receive, gpg_trust, expected): @pytest.mark.usefixtures("gpg_list_keys") def test_gpg_present_new_key_and_trust(gpg_receive, gpg_trust, expected): ret = gpg.present("new", trust="marginally") - assert ret["result"] == expected + assert ret["result"] is expected # the key is always marked as added - assert bool(ret["changes"]) + assert ret["changes"] gpg_receive.assert_called_once() gpg_trust.assert_called_once() @@ -180,60 +161,4 @@ def test_gpg_present_test_mode_no_changes(gpg_receive, gpg_trust, key, trust): gpg_receive.assert_not_called() gpg_trust.assert_not_called() assert ret["result"] is None - assert bool(ret["changes"]) - - -@pytest.mark.usefixtures("gpg_list_keys") -def test_gpg_absent_no_changes(gpg_delete): - ret = gpg.absent("nonexistent") - assert ret["result"] - assert not ret["changes"] - gpg_delete.assert_not_called() - - -@pytest.mark.usefixtures("gpg_list_keys") -@pytest.mark.parametrize( - "gpg_delete,expected", - [ - ({"res": True, "message": "Public key for A deleted"}, True), - ( - { - "res": False, - "message": "Secret key exists, delete first or pass delete_secret=True.", - }, - False, - ), - ], - indirect=["gpg_delete"], -) -@pytest.mark.usefixtures("gpg_list_keys") -def test_gpg_absent_delete_key(gpg_delete, expected): - ret = gpg.absent("A") - assert ret["result"] == expected - assert bool(ret["changes"]) == expected - gpg_delete.assert_called_once() - - -@pytest.mark.usefixtures("gpg_list_keys") -def test_gpg_absent_test_mode_no_changes(gpg_delete): - with patch.dict(gpg.__opts__, {"test": True}): - ret = gpg.absent("A") - gpg_delete.assert_not_called() - assert ret["result"] is None - assert bool(ret["changes"]) - - -def test_gpg_absent_list_keys_with_gnupghome_and_user(gpg_list_keys): - gnupghome = "/pls_respect_me" - user = "imthereaswell" - gpg.absent("nonexistent", gnupghome=gnupghome, user=user) - gpg_list_keys.assert_called_with(gnupghome=gnupghome, user=user) - - -@pytest.mark.usefixtures("gpg_list_keys") -def test_gpg_absent_delete_key_called_with_correct_kwargs(gpg_delete): - key = "A" - user = "hellothere" - gnupghome = "/pls_sir" - gpg.absent(key, user=user, gnupghome=gnupghome) - gpg_delete.assert_called_with(keyid=key, gnupghome=gnupghome, user=user) + assert ret["changes"] From 81d87fe931fa6a072f06aa4136c4003b4496ead8 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Sun, 11 Dec 2022 18:40:06 +0100 Subject: [PATCH 11/19] Backport fix for delete_key --- salt/modules/gpg.py | 32 ++++++++++++++++---------- tests/pytests/unit/modules/test_gpg.py | 11 ++++++--- 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/salt/modules/gpg.py b/salt/modules/gpg.py index 4efc349ef4c4..805e7251a6ee 100644 --- a/salt/modules/gpg.py +++ b/salt/modules/gpg.py @@ -559,12 +559,10 @@ def delete_key( key = get_key(keyid=keyid, fingerprint=fingerprint, user=user, gnupghome=gnupghome) def __delete_key(fingerprint, secret, use_passphrase): - if use_passphrase: + if secret and use_passphrase: gpg_passphrase = __salt__["pillar.get"]("gpg_passphrase") if not gpg_passphrase: - ret["res"] = False - ret["message"] = "gpg_passphrase not available in pillar." - return ret + return "gpg_passphrase not available in pillar." else: out = gpg.delete_keys(fingerprint, secret, passphrase=gpg_passphrase) else: @@ -573,7 +571,7 @@ def __delete_key(fingerprint, secret, use_passphrase): if key: fingerprint = key["fingerprint"] - skey = get_secret_key(keyid, fingerprint, user) + skey = get_secret_key(keyid, fingerprint, user, gnupghome=gnupghome) if skey: if not delete_secret: ret["res"] = False @@ -582,19 +580,29 @@ def __delete_key(fingerprint, secret, use_passphrase): ] = "Secret key exists, delete first or pass delete_secret=True." return ret else: - if str(__delete_key(fingerprint, True, use_passphrase)) == "ok": + out = __delete_key(fingerprint, True, use_passphrase) + if str(out) == "ok": # Delete the secret key - ret["message"] = "Secret key for {} deleted\n".format(fingerprint) + ret["message"] = f"Secret key for {fingerprint} deleted\n" + else: + ret["res"] = False + ret[ + "message" + ] = f"Failed to delete secret key for {fingerprint}: {out}" + return ret # Delete the public key - if str(__delete_key(fingerprint, False, use_passphrase)) == "ok": - ret["message"] += "Public key for {} deleted".format(fingerprint) - ret["res"] = True - return ret + out = __delete_key(fingerprint, False, use_passphrase) + if str(out) == "ok": + ret["res"] = True + ret["message"] += f"Public key for {fingerprint} deleted" + else: + ret["res"] = False + ret["message"] += f"Failed to delete public key for {fingerprint}: {out}" else: ret["res"] = False ret["message"] = "Key not available in keychain." - return ret + return ret def get_key(keyid=None, fingerprint=None, user=None, gnupghome=None): diff --git a/tests/pytests/unit/modules/test_gpg.py b/tests/pytests/unit/modules/test_gpg.py index a5d18639ddef..139cfa7cf369 100644 --- a/tests/pytests/unit/modules/test_gpg.py +++ b/tests/pytests/unit/modules/test_gpg.py @@ -466,8 +466,8 @@ def test_delete_key_with_passphrase_without_gpg_passphrase_in_pillar(gpghome): ] _expected_result = { - "res": True, - "message": "gpg_passphrase not available in pillar.", + "res": False, + "message": "Failed to delete secret key for xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx: gpg_passphrase not available in pillar.", } mock_opt = MagicMock(return_value="root") @@ -546,10 +546,15 @@ def test_delete_key_with_passphrase_with_gpg_passphrase_in_pillar(gpghome): ) as gnupg_delete_keys: ret = gpg.delete_key("xxxxxxxxxxxxxxxx", delete_secret=True) assert ret == _expected_result + gnupg_delete_keys.assert_any_call( + "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", + True, + passphrase=GPG_TEST_KEY_PASSPHRASE, + ) gnupg_delete_keys.assert_called_with( "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", False, - passphrase=GPG_TEST_KEY_PASSPHRASE, + expect_passphrase=False, ) From c093ba9233c8c39d784340221609d9f502dc070f Mon Sep 17 00:00:00 2001 From: jeanluc Date: Tue, 18 Apr 2023 21:58:00 +0200 Subject: [PATCH 12/19] Rename changelog files to .md --- changelog/{63144.fixed => 63144.fixed.md} | 0 changelog/{63153.fixed => 63153.fixed.md} | 0 changelog/{63156.fixed => 63156.fixed.md} | 0 changelog/{63159.fixed => 63159.fixed.md} | 0 4 files changed, 0 insertions(+), 0 deletions(-) rename changelog/{63144.fixed => 63144.fixed.md} (100%) rename changelog/{63153.fixed => 63153.fixed.md} (100%) rename changelog/{63156.fixed => 63156.fixed.md} (100%) rename changelog/{63159.fixed => 63159.fixed.md} (100%) diff --git a/changelog/63144.fixed b/changelog/63144.fixed.md similarity index 100% rename from changelog/63144.fixed rename to changelog/63144.fixed.md diff --git a/changelog/63153.fixed b/changelog/63153.fixed.md similarity index 100% rename from changelog/63153.fixed rename to changelog/63153.fixed.md diff --git a/changelog/63156.fixed b/changelog/63156.fixed.md similarity index 100% rename from changelog/63156.fixed rename to changelog/63156.fixed.md diff --git a/changelog/63159.fixed b/changelog/63159.fixed.md similarity index 100% rename from changelog/63159.fixed rename to changelog/63159.fixed.md From 1e8c30364456d3143a9cf7553a72039ce770fad5 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Wed, 5 Jul 2023 15:20:40 +0200 Subject: [PATCH 13/19] Run pyupgrade for Python >=3.8 --- salt/modules/gpg.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/salt/modules/gpg.py b/salt/modules/gpg.py index 805e7251a6ee..5e8c78a22f38 100644 --- a/salt/modules/gpg.py +++ b/salt/modules/gpg.py @@ -122,7 +122,7 @@ def _get_user_info(user=None): # if it doesn't exist then fall back to user Salt running as userinfo = _get_user_info() else: - raise SaltInvocationError("User {} does not exist".format(user)) + raise SaltInvocationError(f"User {user} does not exist") return userinfo @@ -1007,12 +1007,12 @@ def trust_key(keyid=None, fingerprint=None, trust_level=None, user=None): if key: if "fingerprint" not in key: ret["res"] = False - ret["message"] = "Fingerprint not found for keyid {}".format(keyid) + ret["message"] = f"Fingerprint not found for keyid {keyid}" return ret fingerprint = key["fingerprint"] else: ret["res"] = False - ret["message"] = "KeyID {} not in GPG keychain".format(keyid) + ret["message"] = f"KeyID {keyid} not in GPG keychain" return ret else: ret["res"] = False @@ -1022,7 +1022,7 @@ def trust_key(keyid=None, fingerprint=None, trust_level=None, user=None): if trust_level not in _VALID_TRUST_LEVELS: return "ERROR: Valid trust levels - {}".format(",".join(_VALID_TRUST_LEVELS)) - stdin = "{}:{}\n".format(fingerprint, NUM_TRUST_DICT[trust_level]) + stdin = f"{fingerprint}:{NUM_TRUST_DICT[trust_level]}\n" cmd = [_gpg(), "--import-ownertrust"] _user = user @@ -1418,7 +1418,7 @@ def encrypt( if result.ok: if not bare: if output: - ret["comment"] = "Encrypted data has been written to {}".format(output) + ret["comment"] = f"Encrypted data has been written to {output}" else: ret["comment"] = result.data else: @@ -1506,7 +1506,7 @@ def decrypt( if result.ok: if not bare: if output: - ret["comment"] = "Decrypted data has been written to {}".format(output) + ret["comment"] = f"Decrypted data has been written to {output}" else: ret["comment"] = result.data else: From 82436e9433487bf7bd8e40c8d047fb2c5c6cf21f Mon Sep 17 00:00:00 2001 From: jeanluc Date: Tue, 12 Sep 2023 00:04:09 +0200 Subject: [PATCH 14/19] Add test for issue 65169 gpg.present succeeds when the keyserver is unreachable --- tests/pytests/unit/modules/test_gpg.py | 37 ++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/pytests/unit/modules/test_gpg.py b/tests/pytests/unit/modules/test_gpg.py index 139cfa7cf369..afa148ea707c 100644 --- a/tests/pytests/unit/modules/test_gpg.py +++ b/tests/pytests/unit/modules/test_gpg.py @@ -1078,3 +1078,40 @@ def test_gpg_receive_keys_no_user_id(): res = gpg.receive_keys(keys="abc", user="abc") assert res["res"] is False assert "no user ID" in res["message"][0] + + +def test_gpg_receive_keys_keyserver_unavailable(): + with patch("salt.modules.gpg._create_gpg") as create: + with patch.dict( + gpg.__salt__, {"user.info": MagicMock(), "config.option": Mock()} + ): + import_result = MagicMock() + import_result.__bool__.return_value = False + for var, val in { + "gpg": Mock(), + "results": [ + {"fingerprint": None, "problem": "0", "text": "Other failure"} + ], + "fingerprints": [], + "count": 0, + "no_user_id": 0, + "imported": 0, + "imported_rsa": 0, + "unchanged": 0, + "n_uids": 0, + "n_subk": 0, + "n_sigs": 0, + "n_revoc": 0, + "sec_read": 0, + "sec_imported": 0, + "sec_dups": 0, + "not_imported": 0, + "data": b"", + "stderr": "[GNUPG:] FAILURE recv-keys 167772346\ngpg: keyserver receive failed: No keyserver available\n", + "returncode": 2, + }.items(): + setattr(import_result, var, val) + create.return_value.recv_keys.return_value = import_result + res = gpg.receive_keys(keys="abc", user="abc") + assert res["res"] is False + assert any("No keyserver available" in x for x in res["message"]) From faef43535541998f0f1bf805ad95ce3bcb2fb9c8 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Tue, 12 Sep 2023 00:10:53 +0200 Subject: [PATCH 15/19] Make `gpg.receive_keys` report failure when GPG does This makes `gpg.present` report the correct result when a keyserver is unreachable and thus fixes issue 65169 in concert with several related fixes in this branch. --- changelog/65169.fixed.md | 1 + salt/modules/gpg.py | 17 ++++++++--------- 2 files changed, 9 insertions(+), 9 deletions(-) create mode 100644 changelog/65169.fixed.md diff --git a/changelog/65169.fixed.md b/changelog/65169.fixed.md new file mode 100644 index 000000000000..8210d1b62d79 --- /dev/null +++ b/changelog/65169.fixed.md @@ -0,0 +1 @@ +Fixed `gpg.present` succeeds when the keyserver is unreachable diff --git a/salt/modules/gpg.py b/salt/modules/gpg.py index 5e8c78a22f38..3694c433467b 100644 --- a/salt/modules/gpg.py +++ b/salt/modules/gpg.py @@ -934,21 +934,20 @@ def receive_keys(keyserver=None, keys=None, user=None, gnupghome=None): if "ok" in result: if result["ok"] == "1": ret["message"].append( - "Key {} added to keychain".format(result["fingerprint"]) + f"Key {result['fingerprint']} added to keychain" ) elif result["ok"] == "0": ret["message"].append( - "Key {} already exists in keychain".format( - result["fingerprint"] - ) + f"Key {result['fingerprint']} already exists in keychain" ) elif "problem" in result: - ret["message"].append("Unable to add key to keychain") - elif not bool(recv_data): + ret["message"].append( + f"Unable to add key to keychain: {result.get('text', 'No further description')}" + ) + + if not bool(recv_data): ret["res"] = False - ret["message"] = [ - f"Something went wrong during gpg call: {recv_data.stderr}" - ] + ret["message"].append(f"GPG reported failure: {recv_data.stderr}") except AttributeError: ret["res"] = False ret["message"] = ["Invalid return from python-gpg"] From 5c6e3ae549f6af74d39f7b8f8b59990ff95568af Mon Sep 17 00:00:00 2001 From: jeanluc Date: Tue, 12 Sep 2023 03:19:36 +0200 Subject: [PATCH 16/19] Clean up new module unit tests --- tests/pytests/unit/modules/test_gpg.py | 110 +++++++++++++------------ 1 file changed, 56 insertions(+), 54 deletions(-) diff --git a/tests/pytests/unit/modules/test_gpg.py b/tests/pytests/unit/modules/test_gpg.py index afa148ea707c..165564edc5ad 100644 --- a/tests/pytests/unit/modules/test_gpg.py +++ b/tests/pytests/unit/modules/test_gpg.py @@ -1046,72 +1046,74 @@ def test_gpg_decrypt_message_with_gpg_passphrase_in_pillar(gpghome): assert ret["res"] is True -def test_gpg_receive_keys_no_user_id(): +@pytest.fixture(params={}) +def import_result_mock(request): + defaults = { + "gpg": Mock(), + "imported": 0, + "results": [], + "fingerprints": [], + "count": 0, + "no_user_id": 0, + "imported_rsa": 0, + "unchanged": 0, + "n_uids": 0, + "n_subk": 0, + "n_sigs": 0, + "n_revoc": 0, + "sec_read": 0, + "sec_imported": 0, + "sec_dups": 0, + "not_imported": 0, + "stderr": "", + "data": b"", + } + defaults.update(request.param) + import_result = MagicMock() + import_result.__bool__.return_value = False + for var, val in defaults.items(): + setattr(import_result, var, val) + return import_result + + +@pytest.mark.parametrize( + "import_result_mock", + ( + { + "count": 1, + "stderr": "gpg: key ABCDEF0123456789: no user ID\ngpg: Total number processed: 1\n[GNUPG:] IMPORT_RES 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0\n", + }, + ), + indirect=True, +) +def test_gpg_receive_keys_no_user_id(import_result_mock): with patch("salt.modules.gpg._create_gpg") as create: with patch.dict( gpg.__salt__, {"user.info": MagicMock(), "config.option": Mock()} ): - import_result = MagicMock() - import_result.__bool__.return_value = False - for var, val in { - "gpg": Mock(), - "imported": 0, - "results": [], - "fingerprints": [], - "count": 1, - "no_user_id": 0, - "imported_rsa": 0, - "unchanged": 0, - "n_uids": 0, - "n_subk": 0, - "n_sigs": 0, - "n_revoc": 0, - "sec_read": 0, - "sec_imported": 0, - "sec_dups": 0, - "not_imported": 0, - "stderr": "gpg: key ABCDEF0123456789: no user ID\ngpg: Total number processed: 1\n[GNUPG:] IMPORT_RES 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0\n", - "data": b"", - }.items(): - setattr(import_result, var, val) - create.return_value.recv_keys.return_value = import_result + create.return_value.recv_keys.return_value = import_result_mock res = gpg.receive_keys(keys="abc", user="abc") assert res["res"] is False - assert "no user ID" in res["message"][0] + assert any("no user ID" in x for x in res["message"]) -def test_gpg_receive_keys_keyserver_unavailable(): +@pytest.mark.parametrize( + "import_result_mock", + ( + { + "results": [{"fingerprint": None, "problem": "0", "text": "Other failure"}], + "stderr": "[GNUPG:] FAILURE recv-keys 167772346\ngpg: keyserver receive failed: No keyserver available\n", + "returncode": 2, + }, + ), + indirect=True, +) +def test_gpg_receive_keys_keyserver_unavailable(import_result_mock): with patch("salt.modules.gpg._create_gpg") as create: with patch.dict( gpg.__salt__, {"user.info": MagicMock(), "config.option": Mock()} ): - import_result = MagicMock() - import_result.__bool__.return_value = False - for var, val in { - "gpg": Mock(), - "results": [ - {"fingerprint": None, "problem": "0", "text": "Other failure"} - ], - "fingerprints": [], - "count": 0, - "no_user_id": 0, - "imported": 0, - "imported_rsa": 0, - "unchanged": 0, - "n_uids": 0, - "n_subk": 0, - "n_sigs": 0, - "n_revoc": 0, - "sec_read": 0, - "sec_imported": 0, - "sec_dups": 0, - "not_imported": 0, - "data": b"", - "stderr": "[GNUPG:] FAILURE recv-keys 167772346\ngpg: keyserver receive failed: No keyserver available\n", - "returncode": 2, - }.items(): - setattr(import_result, var, val) - create.return_value.recv_keys.return_value = import_result + create.return_value.recv_keys.return_value = import_result_mock res = gpg.receive_keys(keys="abc", user="abc") assert res["res"] is False assert any("No keyserver available" in x for x in res["message"]) From 863cf7763a492233603689d879504dcbafa38d49 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Wed, 13 Sep 2023 12:29:25 +0200 Subject: [PATCH 17/19] Commit review remarks --- salt/modules/gpg.py | 2 +- tests/pytests/functional/states/test_gpg.py | 21 ++++++--------------- 2 files changed, 7 insertions(+), 16 deletions(-) diff --git a/salt/modules/gpg.py b/salt/modules/gpg.py index 3694c433467b..7f9a4712e37a 100644 --- a/salt/modules/gpg.py +++ b/salt/modules/gpg.py @@ -945,7 +945,7 @@ def receive_keys(keyserver=None, keys=None, user=None, gnupghome=None): f"Unable to add key to keychain: {result.get('text', 'No further description')}" ) - if not bool(recv_data): + if not recv_data: ret["res"] = False ret["message"].append(f"GPG reported failure: {recv_data.stderr}") except AttributeError: diff --git a/tests/pytests/functional/states/test_gpg.py b/tests/pytests/functional/states/test_gpg.py index 4809f7906522..684c7f215a43 100644 --- a/tests/pytests/functional/states/test_gpg.py +++ b/tests/pytests/functional/states/test_gpg.py @@ -4,16 +4,9 @@ import psutil import pytest -try: - import gnupg as gnupglib - - HAS_GNUPG = True -except ImportError: - HAS_GNUPG = False - +gnupglib = pytest.importorskip("gnupg", reason="Needs python-gnupg library") pytestmark = [ - pytest.mark.skipif(HAS_GNUPG is False, reason="Needs python-gnupg library"), pytest.mark.skip_if_binaries_missing("gpg", reason="Needs gpg binary"), ] @@ -98,7 +91,7 @@ def gnupg_keyring(gpghome, keyring): @pytest.fixture(params=["a"]) -def pubkeys_present(gnupg, request): +def _pubkeys_present(gnupg, request): pubkeys = [request.getfixturevalue(f"key_{x}_pub") for x in request.param] fingerprints = [request.getfixturevalue(f"key_{x}_fp") for x in request.param] gnupg.import_keys("\n".join(pubkeys)) @@ -109,7 +102,7 @@ def pubkeys_present(gnupg, request): # cleanup is taken care of by gpghome and tmp_path -@pytest.mark.usefixtures("pubkeys_present") +@pytest.mark.usefixtures("_pubkeys_present") def test_gpg_present_no_changes(gpghome, gpg, gnupg, key_a_fp): assert gnupg.list_keys(keys=key_a_fp) ret = gpg.present( @@ -119,12 +112,10 @@ def test_gpg_present_no_changes(gpghome, gpg, gnupg, key_a_fp): assert not ret.changes -@pytest.mark.skip_unless_on_linux( - reason="Complains about deleting private keys first when they are absent" -) -@pytest.mark.usefixtures("pubkeys_present") +@pytest.mark.usefixtures("_pubkeys_present") def test_gpg_absent(gpghome, gpg, gnupg, key_a_fp): assert gnupg.list_keys(keys=key_a_fp) + assert not gnupg.list_keys(keys=key_a_fp, secret=True) ret = gpg.absent(key_a_fp[-16:], gnupghome=str(gpghome)) assert ret.result assert ret.changes @@ -139,7 +130,7 @@ def test_gpg_absent_no_changes(gpghome, gpg, gnupg, key_a_fp): assert not ret.changes -@pytest.mark.usefixtures("pubkeys_present") +@pytest.mark.usefixtures("_pubkeys_present") def test_gpg_absent_test_mode_no_changes(gpghome, gpg, gnupg, key_a_fp): assert gnupg.list_keys(keys=key_a_fp) ret = gpg.absent(key_a_fp[-16:], gnupghome=str(gpghome), test=True) From 72ba9bcfbbb43c484fd3accf18a94965ce245906 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Wed, 13 Sep 2023 18:01:57 +0200 Subject: [PATCH 18/19] Enable new func tests for GPG state module on Win --- tests/pytests/functional/states/test_gpg.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/pytests/functional/states/test_gpg.py b/tests/pytests/functional/states/test_gpg.py index 684c7f215a43..80e86aa3a7b2 100644 --- a/tests/pytests/functional/states/test_gpg.py +++ b/tests/pytests/functional/states/test_gpg.py @@ -8,6 +8,7 @@ pytestmark = [ pytest.mark.skip_if_binaries_missing("gpg", reason="Needs gpg binary"), + pytest.mark.windows_whitelisted, ] From 2245d666d0d7147f5466dc48ac3948ebfadc31bf Mon Sep 17 00:00:00 2001 From: jeanluc Date: Wed, 13 Sep 2023 18:55:03 +0200 Subject: [PATCH 19/19] Review comments 2 --- tests/pytests/unit/modules/test_gpg.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/pytests/unit/modules/test_gpg.py b/tests/pytests/unit/modules/test_gpg.py index 165564edc5ad..de37c26dfb7e 100644 --- a/tests/pytests/unit/modules/test_gpg.py +++ b/tests/pytests/unit/modules/test_gpg.py @@ -1047,7 +1047,7 @@ def test_gpg_decrypt_message_with_gpg_passphrase_in_pillar(gpghome): @pytest.fixture(params={}) -def import_result_mock(request): +def _import_result_mock(request): defaults = { "gpg": Mock(), "imported": 0, @@ -1077,7 +1077,7 @@ def import_result_mock(request): @pytest.mark.parametrize( - "import_result_mock", + "_import_result_mock", ( { "count": 1, @@ -1086,19 +1086,19 @@ def import_result_mock(request): ), indirect=True, ) -def test_gpg_receive_keys_no_user_id(import_result_mock): +def test_gpg_receive_keys_no_user_id(_import_result_mock): with patch("salt.modules.gpg._create_gpg") as create: with patch.dict( gpg.__salt__, {"user.info": MagicMock(), "config.option": Mock()} ): - create.return_value.recv_keys.return_value = import_result_mock + create.return_value.recv_keys.return_value = _import_result_mock res = gpg.receive_keys(keys="abc", user="abc") assert res["res"] is False assert any("no user ID" in x for x in res["message"]) @pytest.mark.parametrize( - "import_result_mock", + "_import_result_mock", ( { "results": [{"fingerprint": None, "problem": "0", "text": "Other failure"}], @@ -1108,12 +1108,12 @@ def test_gpg_receive_keys_no_user_id(import_result_mock): ), indirect=True, ) -def test_gpg_receive_keys_keyserver_unavailable(import_result_mock): +def test_gpg_receive_keys_keyserver_unavailable(_import_result_mock): with patch("salt.modules.gpg._create_gpg") as create: with patch.dict( gpg.__salt__, {"user.info": MagicMock(), "config.option": Mock()} ): - create.return_value.recv_keys.return_value = import_result_mock + create.return_value.recv_keys.return_value = _import_result_mock res = gpg.receive_keys(keys="abc", user="abc") assert res["res"] is False assert any("No keyserver available" in x for x in res["message"])