File backport-syndic-auth-fixes.patch of Package salt
From 54ab69e74beb83710d0bf6049039d13e260d5517 Mon Sep 17 00:00:00 2001
From: Alexander Graul <agraul@suse.com>
Date: Tue, 13 Sep 2022 11:26:21 +0200
Subject: [PATCH] Backport Syndic auth fixes
[3004.2] Syndic Fixes
(cherry picked from commit 643bd4b572ca97466e085ecd1d84da45b1684332)
Co-authored-by: Megan Wilhite <megan.wilhite@gmail.com>
---
changelog/61868.fixed | 1 +
salt/transport/mixins/auth.py | 2 +-
salt/transport/tcp.py | 2 +-
salt/transport/zeromq.py | 2 +-
tests/pytests/unit/transport/test_tcp.py | 149 +++++++++++++++++++-
tests/pytests/unit/transport/test_zeromq.py | 73 +++++++++-
6 files changed, 224 insertions(+), 5 deletions(-)
create mode 100644 changelog/61868.fixed
diff --git a/changelog/61868.fixed b/changelog/61868.fixed
new file mode 100644
index 0000000000..0169c48e99
--- /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.
diff --git a/salt/transport/mixins/auth.py b/salt/transport/mixins/auth.py
index 1e2e8e6b7b..e5c6a5345f 100644
--- a/salt/transport/mixins/auth.py
+++ b/salt/transport/mixins/auth.py
@@ -43,7 +43,7 @@ class AESPubClientMixin:
)
# 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 f00b3c40eb..2821be82c7 100644
--- a/salt/transport/tcp.py
+++ b/salt/transport/tcp.py
@@ -295,7 +295,7 @@ class AsyncTCPReqChannel(salt.transport.client.ReqChannel):
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 aa06298ee1..8199378239 100644
--- a/salt/transport/zeromq.py
+++ b/salt/transport/zeromq.py
@@ -255,7 +255,7 @@ class AsyncZeroMQReqChannel(salt.transport.client.ReqChannel):
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 3b6e175472..e41edcc37e 100644
--- a/tests/pytests/unit/transport/test_tcp.py
+++ b/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 test_client_reconnect_backoff(client_socket):
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
diff --git a/tests/pytests/unit/transport/test_zeromq.py b/tests/pytests/unit/transport/test_zeromq.py
index 1f0515c91a..c3093f4b19 100644
--- a/tests/pytests/unit/transport/test_zeromq.py
+++ b/tests/pytests/unit/transport/test_zeromq.py
@@ -23,7 +23,7 @@ import salt.utils.process
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
--
2.37.3
