diff --git a/changelog/68181.fixed.md b/changelog/68181.fixed.md new file mode 100644 index 000000000000..7995e8feb55b --- /dev/null +++ b/changelog/68181.fixed.md @@ -0,0 +1 @@ +Fixed event signature verification failing under ``minion_sign_messages``. The minion was signing the return load before ``salt.channel.client.AsyncReqChannel._package_load`` attached transport metadata (``nonce``, ``ts``, ``tok``, ``id``), so the bytes the master re-serialized to verify did not match what was signed and every signed return was dropped. Signing is now performed inside ``_package_load`` after the metadata is attached, against the same bytes the master verifies. diff --git a/salt/channel/client.py b/salt/channel/client.py index 1c13ad8d10b5..bb59cd6da106 100644 --- a/salt/channel/client.py +++ b/salt/channel/client.py @@ -14,6 +14,7 @@ import salt.ext.tornado.gen import salt.ext.tornado.ioloop import salt.payload +import salt.serializers.msgpack import salt.transport.frame import salt.utils.event import salt.utils.files @@ -168,6 +169,20 @@ def _package_load(self, load, nonce=None): load["ts"] = int(time.time()) load["tok"] = self.auth.gen_token(b"salt") load["id"] = self.opts["id"] + if self.opts.get("minion_sign_messages"): + # ReqServerChannel strips ``nonce`` and ``tok`` from the + # load before it reaches AESFuncs._return for verification + # (see salt/channel/server.py). Sign a view that excludes + # those transport-only fields so the bytes the master + # verifies match what we sign here. + to_sign = { + k: v for k, v in load.items() if k not in ("nonce", "tok") + } + load["sig"] = salt.crypt.sign_message( + os.path.join(self.opts["pki_dir"], "minion.pem"), + salt.serializers.msgpack.serialize(to_sign), + algorithm=self.opts["signing_algorithm"], + ) except TypeError: # Backwards compatability for non dict loads, let the load get # sent and fail to authenticate. diff --git a/salt/master.py b/salt/master.py index e10704ad5e4c..c29de0c75417 100644 --- a/salt/master.py +++ b/salt/master.py @@ -1663,7 +1663,10 @@ def _return(self, load): ) serialized_load = salt.serializers.msgpack.serialize(load) if not salt.crypt.verify_signature( - this_minion_pubkey, serialized_load, sig + this_minion_pubkey, + serialized_load, + sig, + algorithm=self.opts["signing_algorithm"], ): log.info("Failed to verify event signature from minion %s.", load["id"]) if self.opts["drop_messages_signature_fail"]: diff --git a/salt/minion.py b/salt/minion.py index 903692501669..3ae5d2d9576b 100644 --- a/salt/minion.py +++ b/salt/minion.py @@ -1724,14 +1724,6 @@ def _load_modules( return functions, returners, errors, executors def _send_req_sync(self, load, timeout): - # XXX: Signing should happen in RequestChannel to be fixed in 3008 - if self.opts["minion_sign_messages"]: - log.trace("Signing event to be published onto the bus.") - minion_privkey_path = os.path.join(self.opts["pki_dir"], "minion.pem") - sig = salt.crypt.sign_message( - minion_privkey_path, salt.serializers.msgpack.serialize(load) - ) - load["sig"] = sig with salt.utils.event.get_event("minion", opts=self.opts, listen=True) as event: request_id = str(uuid.uuid4()) log.trace("Send request to main id=%s", request_id) @@ -1751,15 +1743,7 @@ def _send_req_sync(self, load, timeout): @salt.ext.tornado.gen.coroutine def _send_req_async(self, load, timeout): - # XXX: Signing should happen in RequestChannel to be fixed in 3008 # XXX: This is only used by syndic - if self.opts["minion_sign_messages"]: - log.trace("Signing event to be published onto the bus.") - minion_privkey_path = os.path.join(self.opts["pki_dir"], "minion.pem") - sig = salt.crypt.sign_message( - minion_privkey_path, salt.serializers.msgpack.serialize(load) - ) - load["sig"] = sig with salt.utils.event.get_event("minion", opts=self.opts, listen=True) as event: request_id = str(uuid.uuid4()) log.trace("Send request to main id=%s", request_id) @@ -1788,13 +1772,6 @@ def _send_req_async_main(self, load, timeout): top level process in the main thread only. Worker threads and processess should call _send_req_sync or _send_req_async as nessecery. """ - if self.opts["minion_sign_messages"]: - log.trace("Signing event to be published onto the bus.") - minion_privkey_path = os.path.join(self.opts["pki_dir"], "minion.pem") - sig = salt.crypt.sign_message( - minion_privkey_path, salt.serializers.msgpack.serialize(load) - ) - load["sig"] = sig ret = yield self.req_channel.send( load, timeout=timeout, tries=self.opts["return_retry_tries"] ) @@ -4340,26 +4317,12 @@ def timeout_handler(*args): ) def _send_req_sync(self, load, timeout): - if self.opts["minion_sign_messages"]: - log.trace("Signing event to be published onto the bus.") - minion_privkey_path = os.path.join(self.opts["pki_dir"], "minion.pem") - sig = salt.crypt.sign_message( - minion_privkey_path, salt.serializers.msgpack.serialize(load) - ) - load["sig"] = sig return self.req_channel.send( load, timeout=timeout, tries=self.opts["return_retry_tries"] ) @salt.ext.tornado.gen.coroutine def _send_req_async(self, load, timeout): - if self.opts["minion_sign_messages"]: - log.trace("Signing event to be published onto the bus.") - minion_privkey_path = os.path.join(self.opts["pki_dir"], "minion.pem") - sig = salt.crypt.sign_message( - minion_privkey_path, salt.serializers.msgpack.serialize(load) - ) - load["sig"] = sig ret = yield self.async_req_channel.send( load, timeout=timeout, tries=self.opts["return_retry_tries"] ) diff --git a/tests/pytests/integration/master/test_signed_returns.py b/tests/pytests/integration/master/test_signed_returns.py new file mode 100644 index 000000000000..9ab8e3319720 --- /dev/null +++ b/tests/pytests/integration/master/test_signed_returns.py @@ -0,0 +1,52 @@ +""" +Regression test for https://github.com/saltstack/salt/issues/68181. + +With ``minion_sign_messages`` enabled the master must verify the minion's +return signature successfully. Previously the minion signed the return load +before ``AsyncReqChannel._package_load`` attached transport metadata +(``nonce``, ``ts``, ``tok``, ``id``), so the bytes the master re-serialized +to verify never matched what was signed and every signed return was dropped +under ``drop_messages_signature_fail``. +""" + +from tests.conftest import FIPS_TESTRUN + + +def test_signed_minion_return_is_verified(salt_master_factory): + """ + A signed ``test.ping`` return must be accepted by the master when + ``drop_messages_signature_fail`` is set. If signature verification fails + the master drops the return and the CLI never gets a result. + """ + master = salt_master_factory.salt_master_daemon( + "test-68181-master", + overrides={ + "log_level": "info", + "drop_messages_signature_fail": True, + "require_minion_sign_messages": True, + "fips_mode": FIPS_TESTRUN, + "signing_algorithm": ( + "PKCS1v15-SHA224" if FIPS_TESTRUN else "PKCS1v15-SHA1" + ), + "publish_signing_algorithm": ( + "PKCS1v15-SHA224" if FIPS_TESTRUN else "PKCS1v15-SHA1" + ), + }, + ) + minion = master.salt_minion_daemon( + "test-68181-minion", + overrides={ + "log_level": "info", + "minion_sign_messages": True, + "fips_mode": FIPS_TESTRUN, + "encryption_algorithm": "OAEP-SHA224" if FIPS_TESTRUN else "OAEP-SHA1", + "signing_algorithm": ( + "PKCS1v15-SHA224" if FIPS_TESTRUN else "PKCS1v15-SHA1" + ), + }, + ) + cli = master.salt_cli(timeout=60) + with master.started(), minion.started(): + ret = cli.run("--timeout=30", "test.ping", minion_tgt=minion.id) + assert ret.returncode == 0, ret + assert ret.data is True diff --git a/tests/pytests/unit/test_master.py b/tests/pytests/unit/test_master.py index 5b16519e5083..d9947ef83d99 100644 --- a/tests/pytests/unit/test_master.py +++ b/tests/pytests/unit/test_master.py @@ -4,12 +4,15 @@ import pytest +import salt.channel.client import salt.config import salt.crypt +import salt.exceptions import salt.master +import salt.serializers.msgpack import salt.utils.files import salt.utils.platform -from tests.support.mock import patch +from tests.support.mock import MagicMock, patch from tests.support.runtests import RUNTIME_VARS try: @@ -237,6 +240,97 @@ def test_pub_ret_traversal(encrypted_requests, tmp_path): ) +def test_return_signature_verifies_after_channel_packaging(tmp_path, caplog): + """ + Regression test for #68181. + + With ``minion_sign_messages`` enabled, the minion previously signed the + return load before ``AsyncReqChannel._package_load`` attached transport + metadata (``nonce``, ``ts``, ``tok``, ``id``). The bytes the master + re-serialized to verify therefore did not match what was signed, and + every signed return was silently dropped under + ``drop_messages_signature_fail``. Signing is now done inside + ``_package_load`` after the metadata is attached. + """ + salt.crypt.gen_keys(str(tmp_path), "minion", 2048) + pki_dir = tmp_path / "pki" + pki_dir.mkdir() + accepted = pki_dir / "minions" + accepted.mkdir() + with salt.utils.files.fopen(accepted / "minion", "wb") as wfp: + with salt.utils.files.fopen(tmp_path / "minion.pub", "rb") as rfp: + wfp.write(rfp.read()) + + aes_funcs = salt.master.AESFuncs( + opts={ + "pki_dir": str(pki_dir), + "cachedir": str(tmp_path / "cache"), + "sock_dir": str(tmp_path / "sock_drawer"), + "conf_file": str(tmp_path / "config.conf"), + "fileserver_backend": ["local"], + "master_job_cache": False, + "require_minion_sign_messages": True, + "drop_messages_signature_fail": True, + # SHA224 so the test works on FIPS-enabled platforms too. + "signing_algorithm": salt.crypt.PKCS1v15_SHA224, + } + ) + + # Load as Minion._prepare_return_pub would build it for a test.ping return. + load = { + "cmd": "_return", + "id": "minion", + "success": True, + "fun_args": [], + "jid": "20260527000000000000", + "return": True, + "retcode": 0, + "fun": "test.ping", + "out": "nested", + } + + # Build an AsyncReqChannel just complete enough to exercise _package_load. + # We bypass __init__ to avoid spinning up a real transport / auth handshake. + channel = salt.channel.client.AsyncReqChannel.__new__( + salt.channel.client.AsyncReqChannel + ) + channel.opts = { + "id": "minion", + "pki_dir": str(tmp_path), + "minion_sign_messages": True, + "encryption_algorithm": salt.crypt.OAEP_SHA224, + "signing_algorithm": salt.crypt.PKCS1v15_SHA224, + } + channel.auth = MagicMock() + channel.auth.gen_token.return_value = b"\x00" * 256 + # Bypass session encryption so we can read the load the master would see. + channel.auth.session_crypticle = MagicMock() + channel.auth.session_crypticle.dumps = lambda payload: payload + + packaged = channel._package_load(load) + inner_load = packaged["load"] + + # ReqServerChannel pops these transport-only fields before the load reaches + # AESFuncs._return. Mirror that here. + inner_load.pop("nonce", None) + inner_load.pop("tok", None) + + assert "sig" in inner_load, ( + "Channel did not attach a signature to the outbound load even though " + "minion_sign_messages is enabled (#68181)." + ) + + with patch("salt.utils.job.store_job") as store_job, caplog.at_level("INFO"): + ret = aes_funcs._return(inner_load) + + assert "Failed to verify event signature" not in caplog.text, ( + "Master rejected a valid signed return because the channel signed " + "the load before attaching transport metadata (#68181)." + ) + assert ret is not False + assert store_job.called + + def _git_pillar_base_config(tmp_path): return { "__role": "master",