From 3b0d7256cbafd31f8229f83dc94c5526814d14cf Mon Sep 17 00:00:00 2001 From: "Gareth J. Greenaway" Date: Thu, 31 Mar 2022 13:41:40 -0700 Subject: [PATCH 1/9] 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 | 2 +- salt/transport/tcp.py | 2 +- salt/transport/zeromq.py | 2 +- tests/pytests/unit/transport/test_tcp.py | 147 +++++++++++++++++++- tests/pytests/unit/transport/test_zeromq.py | 73 +++++++++- 5 files changed, 221 insertions(+), 5 deletions(-) diff --git a/salt/transport/mixins/auth.py b/salt/transport/mixins/auth.py index 1e2e8e6b7bfa..e5c6a5345f74 100644 --- a/salt/transport/mixins/auth.py +++ b/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") ): diff --git a/salt/transport/tcp.py b/salt/transport/tcp.py index f00b3c40eb6a..2821be82c71e 100644 --- a/salt/transport/tcp.py +++ b/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"] ): diff --git a/salt/transport/zeromq.py b/salt/transport/zeromq.py index 9e61b2325545..dcbfca607135 100644 --- a/salt/transport/zeromq.py +++ b/salt/transport/zeromq.py @@ -255,7 +255,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"] ): diff --git a/tests/pytests/unit/transport/test_tcp.py b/tests/pytests/unit/transport/test_tcp.py index 3b6e17547256..61f409935211 100644 --- a/tests/pytests/unit/transport/test_tcp.py +++ b/tests/pytests/unit/transport/test_tcp.py @@ -4,12 +4,50 @@ 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", autospec=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 def message_client_pool(): sock_pool_size = 5 @@ -405,3 +443,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 = "/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 = "/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 diff --git a/tests/pytests/unit/transport/test_zeromq.py b/tests/pytests/unit/transport/test_zeromq.py index 1f0515c91a9a..fd5ce3241296 100644 --- a/tests/pytests/unit/transport/test_zeromq.py +++ b/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 = os.path.join(pki_dir, "minion", "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 From 7e86d89a60466254aa1797e23f73e52a91479728 Mon Sep 17 00:00:00 2001 From: "Gareth J. Greenaway" Date: Fri, 8 Apr 2022 08:21:02 -0700 Subject: [PATCH 2/9] Adding changelog. --- changelog/61868.fixed | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/61868.fixed diff --git a/changelog/61868.fixed b/changelog/61868.fixed new file mode 100644 index 000000000000..0169c48e99d2 --- /dev/null +++ b/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. From f180258263abe33992fd1f04f9aef7328d3da80c Mon Sep 17 00:00:00 2001 From: "Gareth J. Greenaway" Date: Fri, 8 Apr 2022 08:54:07 -0700 Subject: [PATCH 3/9] running pre-commit manually. --- tests/pytests/unit/transport/test_tcp.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/pytests/unit/transport/test_tcp.py b/tests/pytests/unit/transport/test_tcp.py index 61f409935211..3459d83ff24c 100644 --- a/tests/pytests/unit/transport/test_tcp.py +++ b/tests/pytests/unit/transport/test_tcp.py @@ -48,6 +48,7 @@ def fake_crypticle(): fake_crypticle.generate_key_string.return_value = "fakey fake" yield fake_crypticle + @pytest.fixture def message_client_pool(): sock_pool_size = 5 From 23c137e07fdcb6c9c42f974c785dc48bbf5fd629 Mon Sep 17 00:00:00 2001 From: "Gareth J. Greenaway" Date: Mon, 11 Apr 2022 15:15:49 -0700 Subject: [PATCH 4/9] Fixing failing tests on older Python versions. --- tests/pytests/unit/transport/test_zeromq.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/pytests/unit/transport/test_zeromq.py b/tests/pytests/unit/transport/test_zeromq.py index fd5ce3241296..c3093f4b19f3 100644 --- a/tests/pytests/unit/transport/test_zeromq.py +++ b/tests/pytests/unit/transport/test_zeromq.py @@ -1284,7 +1284,7 @@ async def test_when_async_req_channel_with_syndic_role_should_use_syndic_master_ ): # 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(pki_dir, "minion", "syndic_master.pub") + expected_pubkey_path = str(pki_dir.join("minion").join("syndic_master.pub")) mockloop = MagicMock() opts = { "master_uri": "tcp://127.0.0.1:4506", From d54bec473637fbc34ebd657c6632f8b9f496799f Mon Sep 17 00:00:00 2001 From: "Gareth J. Greenaway" Date: Tue, 12 Apr 2022 11:02:12 -0700 Subject: [PATCH 5/9] Account for salt.transport.tcp.PKCS1_OAEP not being available and just create it, to ensure tests pass. --- tests/pytests/unit/transport/test_tcp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/pytests/unit/transport/test_tcp.py b/tests/pytests/unit/transport/test_tcp.py index 3459d83ff24c..8d33d83a4b15 100644 --- a/tests/pytests/unit/transport/test_tcp.py +++ b/tests/pytests/unit/transport/test_tcp.py @@ -19,7 +19,7 @@ def fake_keys(): @pytest.fixture def fake_crypto(): - with patch("salt.transport.tcp.PKCS1_OAEP", autospec=True) as fake_crypto: + with patch("salt.transport.tcp.PKCS1_OAEP", autospec=True, create=True) as fake_crypto: yield fake_crypto From 458fa8be85454bb78ad7326a04c797c385e5eb4e Mon Sep 17 00:00:00 2001 From: "Gareth J. Greenaway" Date: Tue, 12 Apr 2022 11:23:43 -0700 Subject: [PATCH 6/9] Running pre-commit manually. --- tests/pytests/unit/transport/test_tcp.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/pytests/unit/transport/test_tcp.py b/tests/pytests/unit/transport/test_tcp.py index 8d33d83a4b15..620edf20db13 100644 --- a/tests/pytests/unit/transport/test_tcp.py +++ b/tests/pytests/unit/transport/test_tcp.py @@ -19,7 +19,9 @@ def fake_keys(): @pytest.fixture def fake_crypto(): - with patch("salt.transport.tcp.PKCS1_OAEP", autospec=True, create=True) as fake_crypto: + with patch( + "salt.transport.tcp.PKCS1_OAEP", autospec=True, create=True + ) as fake_crypto: yield fake_crypto From e8f1ffcdac24e740d7c2467620b55fff826935f8 Mon Sep 17 00:00:00 2001 From: "Gareth J. Greenaway" Date: Tue, 12 Apr 2022 13:36:07 -0700 Subject: [PATCH 7/9] Cannot use autospec and create at the same time. Removing autospec. --- tests/pytests/unit/transport/test_tcp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/pytests/unit/transport/test_tcp.py b/tests/pytests/unit/transport/test_tcp.py index 620edf20db13..f3e304125db4 100644 --- a/tests/pytests/unit/transport/test_tcp.py +++ b/tests/pytests/unit/transport/test_tcp.py @@ -20,7 +20,7 @@ def fake_keys(): @pytest.fixture def fake_crypto(): with patch( - "salt.transport.tcp.PKCS1_OAEP", autospec=True, create=True + "salt.transport.tcp.PKCS1_OAEP", create=True ) as fake_crypto: yield fake_crypto From 8f7819c551a6ba069288d4fd71f9536def4f24a8 Mon Sep 17 00:00:00 2001 From: "Gareth J. Greenaway" Date: Tue, 12 Apr 2022 15:05:43 -0700 Subject: [PATCH 8/9] Running pre-commit manually. --- tests/pytests/unit/transport/test_tcp.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/pytests/unit/transport/test_tcp.py b/tests/pytests/unit/transport/test_tcp.py index f3e304125db4..d42d1fe97059 100644 --- a/tests/pytests/unit/transport/test_tcp.py +++ b/tests/pytests/unit/transport/test_tcp.py @@ -19,9 +19,7 @@ def fake_keys(): @pytest.fixture def fake_crypto(): - with patch( - "salt.transport.tcp.PKCS1_OAEP", create=True - ) as fake_crypto: + with patch("salt.transport.tcp.PKCS1_OAEP", create=True) as fake_crypto: yield fake_crypto From 1b07dc960ac939cb1c8116a3f54a9d289f9fdd44 Mon Sep 17 00:00:00 2001 From: "Gareth J. Greenaway" Date: Tue, 10 May 2022 09:52:06 -0700 Subject: [PATCH 9/9] fixing failing tests, ensuring that the correct path is used. --- tests/pytests/unit/transport/test_tcp.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/pytests/unit/transport/test_tcp.py b/tests/pytests/unit/transport/test_tcp.py index d42d1fe97059..e41edcc37e86 100644 --- a/tests/pytests/unit/transport/test_tcp.py +++ b/tests/pytests/unit/transport/test_tcp.py @@ -1,4 +1,5 @@ import contextlib +import os import socket import attr @@ -451,7 +452,7 @@ async def test_when_async_req_channel_with_syndic_role_should_use_syndic_master_ ): # Syndics use the minion pki dir, but they also create a syndic_master.pub # file for comms with the Salt master - expected_pubkey_path = "/etc/salt/pki/minion/syndic_master.pub" + 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 = { @@ -518,7 +519,7 @@ async def test_mixin_should_use_correct_path_when_syndic( fake_keys, fake_authd, fake_crypticle ): mockloop = MagicMock() - expected_pubkey_path = "/etc/salt/pki/minion/syndic_master.pub" + 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",