Merge pull request saltstack#301 from garethgreenaway/3004_1_syndic_f…

…ixes

[3004.2] Syndic Fixes

changelog/61868.fixed

@@ -0,0 +1 @@
+Make sure the correct key is being used when verifying or validating communication, eg. when a Salt syndic is involved use syndic_master.pub and when a Salt minion is involved use minion_master.pub.

salt/transport/mixins/auth.py

@@ -43,7 +43,7 @@ def _verify_master_signature(self, payload):
                 )
 
             # Verify that the signature is valid
-            master_pubkey_path = os.path.join(self.opts["pki_dir"], "minion_master.pub")
+            master_pubkey_path = os.path.join(self.opts["pki_dir"], self.auth.mpub)
             if not salt.crypt.verify_signature(
                 master_pubkey_path, payload["load"], payload.get("sig")
             ):

salt/transport/tcp.py

@@ -295,7 +295,7 @@ def crypted_transfer_decode_dictentry(
         signed_msg = pcrypt.loads(ret[dictkey])
 
         # Validate the master's signature.
-        master_pubkey_path = os.path.join(self.opts["pki_dir"], "minion_master.pub")
+        master_pubkey_path = os.path.join(self.opts["pki_dir"], self.auth.mpub)
         if not salt.crypt.verify_signature(
             master_pubkey_path, signed_msg["data"], signed_msg["sig"]
         ):

salt/transport/zeromq.py

@@ -256,7 +256,7 @@ def crypted_transfer_decode_dictentry(
         signed_msg = pcrypt.loads(ret[dictkey])
 
         # Validate the master's signature.
-        master_pubkey_path = os.path.join(self.opts["pki_dir"], "minion_master.pub")
+        master_pubkey_path = os.path.join(self.opts["pki_dir"], self.auth.mpub)
         if not salt.crypt.verify_signature(
             master_pubkey_path, signed_msg["data"], signed_msg["sig"]
         ):

tests/pytests/unit/transport/test_tcp.py

@@ -1,13 +1,53 @@
 import contextlib
+import os
 import socket
 
 import attr
 import pytest
 import salt.exceptions
+import salt.transport.mixins.auth
 import salt.transport.tcp
 from salt.ext.tornado import concurrent, gen, ioloop
 from saltfactories.utils.ports import get_unused_localhost_port
-from tests.support.mock import MagicMock, patch
+from tests.support.mock import MagicMock, PropertyMock, create_autospec, patch
+
+
+@pytest.fixture
+def fake_keys():
+    with patch("salt.crypt.AsyncAuth.get_keys", autospec=True):
+        yield
+
+
+@pytest.fixture
+def fake_crypto():
+    with patch("salt.transport.tcp.PKCS1_OAEP", create=True) as fake_crypto:
+        yield fake_crypto
+
+
+@pytest.fixture
+def fake_authd():
+    @salt.ext.tornado.gen.coroutine
+    def return_nothing():
+        raise salt.ext.tornado.gen.Return()
+
+    with patch(
+        "salt.crypt.AsyncAuth.authenticated", new_callable=PropertyMock
+    ) as mock_authed, patch(
+        "salt.crypt.AsyncAuth.authenticate",
+        autospec=True,
+        return_value=return_nothing(),
+    ), patch(
+        "salt.crypt.AsyncAuth.gen_token", autospec=True, return_value=42
+    ):
+        mock_authed.return_value = False
+        yield
+
+
+@pytest.fixture
+def fake_crypticle():
+    with patch("salt.crypt.Crypticle") as fake_crypticle:
+        fake_crypticle.generate_key_string.return_value = "fakey fake"
+        yield fake_crypticle
 
 
 @pytest.fixture
@@ -405,3 +445,110 @@ def _sleep(t):
             client.io_loop.run_sync(client._connect)
     finally:
         client.close()
+
+
+async def test_when_async_req_channel_with_syndic_role_should_use_syndic_master_pub_file_to_verify_master_sig(
+    fake_keys, fake_crypto, fake_crypticle
+):
+    # Syndics use the minion pki dir, but they also create a syndic_master.pub
+    # file for comms with the Salt master
+    expected_pubkey_path = os.path.join("/etc/salt/pki/minion", "syndic_master.pub")
+    fake_crypto.new.return_value.decrypt.return_value = "decrypted_return_value"
+    mockloop = MagicMock()
+    opts = {
+        "master_uri": "tcp://127.0.0.1:4506",
+        "interface": "127.0.0.1",
+        "ret_port": 4506,
+        "ipv6": False,
+        "sock_dir": ".",
+        "pki_dir": "/etc/salt/pki/minion",
+        "id": "syndic",
+        "__role": "syndic",
+        "keysize": 4096,
+    }
+    client = salt.transport.tcp.AsyncTCPReqChannel(opts, io_loop=mockloop)
+
+    dictkey = "pillar"
+    target = "minion"
+
+    # Mock auth and message client.
+    client.auth._authenticate_future = MagicMock()
+    client.auth._authenticate_future.done.return_value = True
+    client.auth._authenticate_future.exception.return_value = None
+    client.auth._crypticle = MagicMock()
+    client.message_client = create_autospec(client.message_client)
+
+    @salt.ext.tornado.gen.coroutine
+    def mocksend(msg, timeout=60, tries=3):
+        raise salt.ext.tornado.gen.Return({"pillar": "data", "key": "value"})
+
+    client.message_client.send = mocksend
+
+    # Note the 'ver' value in 'load' does not represent the the 'version' sent
+    # in the top level of the transport's message.
+    load = {
+        "id": target,
+        "grains": {},
+        "saltenv": "base",
+        "pillarenv": "base",
+        "pillar_override": True,
+        "extra_minion_data": {},
+        "ver": "2",
+        "cmd": "_pillar",
+    }
+    fake_nonce = 42
+    with patch(
+        "salt.crypt.verify_signature", autospec=True, return_value=True
+    ) as fake_verify, patch(
+        "salt.payload.loads",
+        autospec=True,
+        return_value={"key": "value", "nonce": fake_nonce, "pillar": "data"},
+    ), patch(
+        "uuid.uuid4", autospec=True
+    ) as fake_uuid:
+        fake_uuid.return_value.hex = fake_nonce
+        ret = await client.crypted_transfer_decode_dictentry(
+            load,
+            dictkey="pillar",
+        )
+
+        assert fake_verify.mock_calls[0].args[0] == expected_pubkey_path
+
+
+async def test_mixin_should_use_correct_path_when_syndic(
+    fake_keys, fake_authd, fake_crypticle
+):
+    mockloop = MagicMock()
+    expected_pubkey_path = os.path.join("/etc/salt/pki/minion", "syndic_master.pub")
+    opts = {
+        "master_uri": "tcp://127.0.0.1:4506",
+        "interface": "127.0.0.1",
+        "ret_port": 4506,
+        "ipv6": False,
+        "sock_dir": ".",
+        "pki_dir": "/etc/salt/pki/minion",
+        "id": "syndic",
+        "__role": "syndic",
+        "keysize": 4096,
+        "sign_pub_messages": True,
+    }
+
+    with patch(
+        "salt.crypt.verify_signature", autospec=True, return_value=True
+    ) as fake_verify, patch(
+        "salt.utils.msgpack.loads",
+        autospec=True,
+        return_value={"enc": "aes", "load": "", "sig": "fake_signature"},
+    ):
+        client = salt.transport.tcp.AsyncTCPPubChannel(opts, io_loop=mockloop)
+        client.message_client = MagicMock()
+        client.message_client.on_recv.side_effect = lambda x: x(b"some_data")
+        await client.connect()
+        client.auth._crypticle = fake_crypticle
+
+        @client.on_recv
+        def test_recv_function(*args, **kwargs):
+            ...
+
+        await test_recv_function
+        assert fake_verify.mock_calls[0].args[0] == expected_pubkey_path

tests/pytests/unit/transport/test_zeromq.py

@@ -23,7 +23,7 @@
 import salt.utils.stringutils
 from salt.master import SMaster
 from salt.transport.zeromq import AsyncReqMessageClientPool
-from tests.support.mock import MagicMock, patch
+from tests.support.mock import MagicMock, create_autospec, patch
 
 try:
     from M2Crypto import RSA
@@ -608,6 +608,7 @@ async def test_req_chan_decode_data_dict_entry_v2(pki_dir):
     auth = client.auth
     auth._crypticle = salt.crypt.Crypticle(opts, AES_KEY)
     client.auth = MagicMock()
+    client.auth.mpub = auth.mpub
     client.auth.authenticated = True
     client.auth.get_keys = auth.get_keys
     client.auth.crypticle.dumps = auth.crypticle.dumps
@@ -672,6 +673,7 @@ async def test_req_chan_decode_data_dict_entry_v2_bad_nonce(pki_dir):
     auth = client.auth
     auth._crypticle = salt.crypt.Crypticle(opts, AES_KEY)
     client.auth = MagicMock()
+    client.auth.mpub = auth.mpub
     client.auth.authenticated = True
     client.auth.get_keys = auth.get_keys
     client.auth.crypticle.dumps = auth.crypticle.dumps
@@ -735,6 +737,7 @@ async def test_req_chan_decode_data_dict_entry_v2_bad_signature(pki_dir):
     auth = client.auth
     auth._crypticle = salt.crypt.Crypticle(opts, AES_KEY)
     client.auth = MagicMock()
+    client.auth.mpub = auth.mpub
     client.auth.authenticated = True
     client.auth.get_keys = auth.get_keys
     client.auth.crypticle.dumps = auth.crypticle.dumps
@@ -814,6 +817,7 @@ async def test_req_chan_decode_data_dict_entry_v2_bad_key(pki_dir):
     auth = client.auth
     auth._crypticle = salt.crypt.Crypticle(opts, AES_KEY)
     client.auth = MagicMock()
+    client.auth.mpub = auth.mpub
     client.auth.authenticated = True
     client.auth.get_keys = auth.get_keys
     client.auth.crypticle.dumps = auth.crypticle.dumps
@@ -1273,3 +1277,70 @@ async def test_req_chan_auth_v2_new_minion_without_master_pub(pki_dir, io_loop):
     assert "sig" in ret
     ret = client.auth.handle_signin_response(signin_payload, ret)
     assert ret == "retry"
+
+
+async def test_when_async_req_channel_with_syndic_role_should_use_syndic_master_pub_file_to_verify_master_sig(
+    pki_dir,
+):
+    # Syndics use the minion pki dir, but they also create a syndic_master.pub
+    # file for comms with the Salt master
+    expected_pubkey_path = str(pki_dir.join("minion").join("syndic_master.pub"))
+    mockloop = MagicMock()
+    opts = {
+        "master_uri": "tcp://127.0.0.1:4506",
+        "interface": "127.0.0.1",
+        "ret_port": 4506,
+        "ipv6": False,
+        "sock_dir": ".",
+        "pki_dir": str(pki_dir.join("minion")),
+        "id": "syndic",
+        "__role": "syndic",
+        "keysize": 4096,
+    }
+    master_opts = dict(opts, pki_dir=str(pki_dir.join("master")))
+    server = salt.transport.zeromq.ZeroMQReqServerChannel(master_opts)
+    client = salt.transport.zeromq.AsyncZeroMQReqChannel(opts, io_loop=mockloop)
+
+    dictkey = "pillar"
+    target = "minion"
+    pillar_data = {"pillar1": "data1"}
+
+    # Mock auth and message client.
+    client.auth._authenticate_future = MagicMock()
+    client.auth._authenticate_future.done.return_value = True
+    client.auth._authenticate_future.exception.return_value = None
+    client.auth._crypticle = salt.crypt.Crypticle(opts, AES_KEY)
+    client.message_client = create_autospec(client.message_client)
+
+    @salt.ext.tornado.gen.coroutine
+    def mocksend(msg, timeout=60, tries=3):
+        client.message_client.msg = msg
+        load = client.auth.crypticle.loads(msg["load"])
+        ret = server._encrypt_private(
+            pillar_data, dictkey, target, nonce=load["nonce"], sign_messages=True
+        )
+        raise salt.ext.tornado.gen.Return(ret)
+
+    client.message_client.send = mocksend
+
+    # Note the 'ver' value in 'load' does not represent the the 'version' sent
+    # in the top level of the transport's message.
+    load = {
+        "id": target,
+        "grains": {},
+        "saltenv": "base",
+        "pillarenv": "base",
+        "pillar_override": True,
+        "extra_minion_data": {},
+        "ver": "2",
+        "cmd": "_pillar",
+    }
+    with patch(
+        "salt.crypt.verify_signature", autospec=True, return_value=True
+    ) as fake_verify:
+        ret = await client.crypted_transfer_decode_dictentry(
+            load,
+            dictkey="pillar",
+        )
+
+        assert fake_verify.mock_calls[0].args[0] == expected_pubkey_path