Sign request loads in _package_load, not in _send_req_*

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.

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.

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"]:

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"]
         )

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

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",