From 9b1bbda2658572162ca44e9b10fa05549fe14d84 Mon Sep 17 00:00:00 2001 From: Harshal Sawant Date: Tue, 18 Jul 2023 11:53:24 +0530 Subject: [PATCH 1/3] Add missing saltenv tag which was causing file not found error in the module State was only looking into the base saltenv for certificate file when add_store or del_store was called. --- salt/states/win_certutil.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/salt/states/win_certutil.py b/salt/states/win_certutil.py index fa3c78e4af31..2b3e3215e946 100644 --- a/salt/states/win_certutil.py +++ b/salt/states/win_certutil.py @@ -65,7 +65,7 @@ def add_store(name, store, saltenv="base"): ret["result"] = False return ret - cert_serial = __salt__["certutil.get_cert_serial"](name) + cert_serial = __salt__["certutil.get_cert_serial"](name, saltenv) if cert_serial is None: ret["comment"] = f"Invalid certificate file: {name}" ret["result"] = False @@ -81,7 +81,7 @@ def add_store(name, store, saltenv="base"): ret["result"] = None return ret - retcode = __salt__["certutil.add_store"](name, store, retcode=True) + retcode = __salt__["certutil.add_store"](name, store, saltenv, retcode=True) if retcode != 0: ret["comment"] = f"Error adding certificate: {name}" ret["result"] = False @@ -151,7 +151,7 @@ def del_store(name, store, saltenv="base"): ret["result"] = None return ret - retcode = __salt__["certutil.del_store"](name, store, retcode=True) + retcode = __salt__["certutil.del_store"](name, store, saltenv, retcode=True) if retcode != 0: ret["comment"] = f"Error removing certificate: {name}" ret["result"] = False From 02c0e56b11a18810112645c2400e31e440185959 Mon Sep 17 00:00:00 2001 From: Harshal Sawant Date: Tue, 18 Jul 2023 11:58:49 +0530 Subject: [PATCH 2/3] Fixed module not loading when called from its respective state FIX: SyntaxError positional argument follows keyword argument --- salt/modules/win_certutil.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/salt/modules/win_certutil.py b/salt/modules/win_certutil.py index 891a3242bd16..c5205a619246 100644 --- a/salt/modules/win_certutil.py +++ b/salt/modules/win_certutil.py @@ -90,7 +90,7 @@ def get_stored_cert_serials(store): return matches -def add_store(source, store, retcode=False, saltenv="base"): +def add_store(source, store, saltenv="base", retcode=False): """ Add the cert to the given Certificate Store @@ -132,7 +132,7 @@ def add_store(source, store, retcode=False, saltenv="base"): return __salt__["cmd.run"](cmd) -def del_store(source, store, retcode=False, saltenv="base"): +def del_store(source, store, saltenv="base", retcode=False): """ Delete the cert from the given Certificate Store From 11660b011f05396ddf32776a1fd7b2f16729a618 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Mon, 15 Jun 2026 18:38:17 -0700 Subject: [PATCH 3/3] Address review feedback: use saltenv kwarg, fix del_store, add tests - Revert the parameter-order swap in win_certutil module (keep retcode before saltenv to avoid breaking callers using positional arguments) per twangboy/Ch3LL review feedback - Pass saltenv=saltenv as a keyword argument in all four state-to-module calls so non-base saltenvs are honoured during cert resolution - Fix the previously-missed get_cert_serial call in del_store (it was still hardcoded to saltenv="base") - Add regression tests verifying saltenv propagation in both add_store and del_store state functions - Add changelog fragment (64659.fixed.md) --- changelog/64659.fixed.md | 1 + salt/modules/win_certutil.py | 4 +- salt/states/win_certutil.py | 8 +-- .../pytests/unit/states/test_win_certutil.py | 54 +++++++++++++++++++ 4 files changed, 61 insertions(+), 6 deletions(-) create mode 100644 changelog/64659.fixed.md diff --git a/changelog/64659.fixed.md b/changelog/64659.fixed.md new file mode 100644 index 000000000000..f3577aeb6d9b --- /dev/null +++ b/changelog/64659.fixed.md @@ -0,0 +1 @@ +Fix ``certutil.add_store`` and ``certutil.del_store`` states ignoring ``saltenv`` when resolving the certificate serial, causing "Invalid certificate file" errors when the certificate lives in a non-``base`` saltenv. diff --git a/salt/modules/win_certutil.py b/salt/modules/win_certutil.py index c5205a619246..891a3242bd16 100644 --- a/salt/modules/win_certutil.py +++ b/salt/modules/win_certutil.py @@ -90,7 +90,7 @@ def get_stored_cert_serials(store): return matches -def add_store(source, store, saltenv="base", retcode=False): +def add_store(source, store, retcode=False, saltenv="base"): """ Add the cert to the given Certificate Store @@ -132,7 +132,7 @@ def add_store(source, store, saltenv="base", retcode=False): return __salt__["cmd.run"](cmd) -def del_store(source, store, saltenv="base", retcode=False): +def del_store(source, store, retcode=False, saltenv="base"): """ Delete the cert from the given Certificate Store diff --git a/salt/states/win_certutil.py b/salt/states/win_certutil.py index 2b3e3215e946..5445ea57758e 100644 --- a/salt/states/win_certutil.py +++ b/salt/states/win_certutil.py @@ -65,7 +65,7 @@ def add_store(name, store, saltenv="base"): ret["result"] = False return ret - cert_serial = __salt__["certutil.get_cert_serial"](name, saltenv) + cert_serial = __salt__["certutil.get_cert_serial"](name, saltenv=saltenv) if cert_serial is None: ret["comment"] = f"Invalid certificate file: {name}" ret["result"] = False @@ -81,7 +81,7 @@ def add_store(name, store, saltenv="base"): ret["result"] = None return ret - retcode = __salt__["certutil.add_store"](name, store, saltenv, retcode=True) + retcode = __salt__["certutil.add_store"](name, store, saltenv=saltenv, retcode=True) if retcode != 0: ret["comment"] = f"Error adding certificate: {name}" ret["result"] = False @@ -135,7 +135,7 @@ def del_store(name, store, saltenv="base"): ret["result"] = False return ret - cert_serial = __salt__["certutil.get_cert_serial"](name) + cert_serial = __salt__["certutil.get_cert_serial"](name, saltenv=saltenv) if cert_serial is None: ret["comment"] = f"Invalid certificate file: {name}" ret["result"] = False @@ -151,7 +151,7 @@ def del_store(name, store, saltenv="base"): ret["result"] = None return ret - retcode = __salt__["certutil.del_store"](name, store, saltenv, retcode=True) + retcode = __salt__["certutil.del_store"](name, store, saltenv=saltenv, retcode=True) if retcode != 0: ret["comment"] = f"Error removing certificate: {name}" ret["result"] = False diff --git a/tests/pytests/unit/states/test_win_certutil.py b/tests/pytests/unit/states/test_win_certutil.py index 5e8a0a987dff..953687e501a7 100644 --- a/tests/pytests/unit/states/test_win_certutil.py +++ b/tests/pytests/unit/states/test_win_certutil.py @@ -119,3 +119,57 @@ def test_del_store_fail_check(): ): out = certutil.del_store("/path/to/cert.cer", "TrustedPublisher") assert expected == out + + +def test_add_store_saltenv_passed_to_get_cert_serial(): + """ + Test that saltenv is forwarded to certutil.get_cert_serial in add_store + so that certificates in non-base saltenvs can be found. + """ + cache_mock = MagicMock(return_value="/tmp/cert.cer") + get_cert_serial_mock = MagicMock(return_value="ABCDEF") + get_store_serials_mock = MagicMock(return_value=["123456"]) + add_mock = MagicMock(return_value=0) + with patch.dict( + certutil.__salt__, + { + "cp.cache_file": cache_mock, + "certutil.get_cert_serial": get_cert_serial_mock, + "certutil.get_stored_cert_serials": get_store_serials_mock, + "certutil.add_store": add_mock, + }, + ): + certutil.add_store("/path/to/cert.cer", "TrustedPublisher", saltenv="test") + get_cert_serial_mock.assert_called_once_with( + "/path/to/cert.cer", saltenv="test" + ) + add_mock.assert_called_once_with( + "/path/to/cert.cer", "TrustedPublisher", saltenv="test", retcode=True + ) + + +def test_del_store_saltenv_passed_to_get_cert_serial(): + """ + Test that saltenv is forwarded to certutil.get_cert_serial in del_store + so that certificates in non-base saltenvs can be found. + """ + cache_mock = MagicMock(return_value="/tmp/cert.cer") + get_cert_serial_mock = MagicMock(return_value="ABCDEF") + get_store_serials_mock = MagicMock(return_value=["123456", "ABCDEF"]) + del_mock = MagicMock(return_value=0) + with patch.dict( + certutil.__salt__, + { + "cp.cache_file": cache_mock, + "certutil.get_cert_serial": get_cert_serial_mock, + "certutil.get_stored_cert_serials": get_store_serials_mock, + "certutil.del_store": del_mock, + }, + ): + certutil.del_store("/path/to/cert.cer", "TrustedPublisher", saltenv="test") + get_cert_serial_mock.assert_called_once_with( + "/path/to/cert.cer", saltenv="test" + ) + del_mock.assert_called_once_with( + "/path/to/cert.cer", "TrustedPublisher", saltenv="test", retcode=True + )