diff --git a/changelog/66228.fixed.md b/changelog/66228.fixed.md new file mode 100644 index 000000000000..620b2c1e8b0b --- /dev/null +++ b/changelog/66228.fixed.md @@ -0,0 +1 @@ +Make sure the master_event_pub.ipc file has correct reed/write permissions for salt group. diff --git a/salt/transport/base.py b/salt/transport/base.py index 4a491d87ce54..66deaa15bdd3 100644 --- a/salt/transport/base.py +++ b/salt/transport/base.py @@ -211,6 +211,14 @@ def ipc_publish_client(node, opts, io_loop): def ipc_publish_server(node, opts): + """ + Create an IPC publish server. + + With the exception of a master's pull_path, all ipc path permission have + user read/write permissions. On a master the ipc publish server's pull_path + permissions are also group read/write. This is done to facilitate non root + users running the salt cli to execute jobs on a master. + """ # Default to TCP for now kwargs = {"transport": "tcp", "ssl": None} if opts["ipc_mode"] == "tcp": @@ -233,6 +241,7 @@ def ipc_publish_server(node, opts): kwargs.update( pub_path=os.path.join(opts["sock_dir"], "master_event_pub.ipc"), pull_path=os.path.join(opts["sock_dir"], "master_event_pull.ipc"), + pub_path_perms=0o660, ) else: id_hash = _minion_hash( diff --git a/salt/transport/tcp.py b/salt/transport/tcp.py index 3b8bba58195f..6632da0f8c4e 100644 --- a/salt/transport/tcp.py +++ b/salt/transport/tcp.py @@ -10,6 +10,7 @@ import errno import logging import multiprocessing +import os import queue import select import socket @@ -1327,6 +1328,8 @@ def __init__( pull_host=None, pull_port=None, pull_path=None, + pull_path_perms=0o600, + pub_path_perms=0o600, ssl=None, ): self.opts = opts @@ -1337,6 +1340,8 @@ def __init__( self.pull_host = pull_host self.pull_port = pull_port self.pull_path = pull_path + self.pull_path_perms = pull_path_perms + self.pub_path_perms = pub_path_perms self.ssl = ssl @property @@ -1355,6 +1360,8 @@ def __getstate__(self): "pull_host": self.pull_host, "pull_port": self.pull_port, "pull_path": self.pull_path, + "pub_path_perms": self.pub_path_perms, + "pull_path_perms": self.pull_path_perms, } def publish_daemon( @@ -1406,7 +1413,9 @@ async def publisher( log.debug( "Publish server binding pub to %s ssl=%r", self.pub_path, self.ssl ) - sock = tornado.netutil.bind_unix_socket(self.pub_path) + with salt.utils.files.set_umask(0o177): + sock = tornado.netutil.bind_unix_socket(self.pub_path) + os.chmod(self.pub_path, self.pub_path_perms) else: log.debug( "Publish server binding pub to %s:%s ssl=%r", @@ -1446,6 +1455,7 @@ async def publisher( # Securely create socket with salt.utils.files.set_umask(0o177): self.pull_sock.start() + os.chmod(self.pull_path, self.pull_path_perms) def pre_fork(self, process_manager): """ diff --git a/salt/transport/ws.py b/salt/transport/ws.py index 8a842e18d296..20e55efe36b1 100644 --- a/salt/transport/ws.py +++ b/salt/transport/ws.py @@ -1,6 +1,7 @@ import asyncio import logging import multiprocessing +import os import socket import time import warnings @@ -259,6 +260,8 @@ def __init__( pull_host=None, pull_port=None, pull_path=None, + pull_path_perms=0o600, + pub_path_perms=0o600, ssl=None, ): self.opts = opts @@ -268,6 +271,8 @@ def __init__( self.pull_host = pull_host self.pull_port = pull_port self.pull_path = pull_path + self.pull_path_perms = pull_path_perms + self.pub_path_perms = pub_path_perms self.ssl = ssl self.clients = set() self._run = None @@ -291,6 +296,8 @@ def __getstate__(self): "pull_host": self.pull_host, "pull_port": self.pull_port, "pull_path": self.pull_path, + "pull_path_perms": self.pull_path_perms, + "pub_path_perms": self.pub_path_perms, } def publish_daemon( @@ -338,8 +345,11 @@ async def publisher( server = aiohttp.web.Server(self.handle_request) runner = aiohttp.web.ServerRunner(server) await runner.setup() - site = aiohttp.web.UnixSite(runner, self.pub_path, ssl_context=ctx) - log.info("Publisher binding to socket %s", self.pub_path) + with salt.utils.files.set_umask(0o177): + log.info("Publisher binding to socket %s", self.pub_path) + site = aiohttp.web.UnixSite(runner, self.pub_path, ssl_context=ctx) + await site.start() + os.chmod(self.pub_path, self.pub_path_perms) else: sock = _get_socket(self.opts) sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) @@ -352,7 +362,7 @@ async def publisher( await runner.setup() site = aiohttp.web.SockSite(runner, sock, ssl_context=ctx) log.info("Publisher binding to socket %s:%s", self.pub_host, self.pub_port) - await site.start() + await site.start() self._pub_payload = publish_payload if self.pull_path: @@ -360,6 +370,7 @@ async def publisher( self.puller = await asyncio.start_unix_server( self.pull_handler, self.pull_path ) + os.chmod(self.pull_path, self.pull_path_perms) else: self.puller = await asyncio.start_server( self.pull_handler, self.pull_host, self.pull_port diff --git a/salt/transport/zeromq.py b/salt/transport/zeromq.py index 478057232fad..fe61cb8808f5 100644 --- a/salt/transport/zeromq.py +++ b/salt/transport/zeromq.py @@ -852,6 +852,8 @@ def __init__( pull_host=None, pull_port=None, pull_path=None, + pull_path_perms=0o600, + pub_path_perms=0o600, ): self.opts = opts self.pub_host = pub_host @@ -864,6 +866,8 @@ def __init__( self.pull_host = pull_host self.pull_port = pull_port self.pull_path = pull_path + self.pub_path_perms = pub_path_perms + self.pull_path_perms = pull_path_perms if pull_path: self.pull_uri = f"ipc://{pull_path}" else: @@ -930,14 +934,14 @@ def _get_sockets(self, context, ioloop): if self.pub_path: os.chmod( # nosec self.pub_path, - 0o600, + self.pub_path_perms, ) log.info("Starting the Salt Puller on %s", self.pull_uri) pull_sock.bind(self.pull_uri) if self.pull_path: os.chmod( # nosec self.pull_path, - 0o600, + self.pull_path_perms, ) return pull_sock, pub_sock, monitor diff --git a/tests/pytests/functional/states/test_pkg.py b/tests/pytests/functional/states/test_pkg.py index a178d2f2cc6b..2b60663bb8e6 100644 --- a/tests/pytests/functional/states/test_pkg.py +++ b/tests/pytests/functional/states/test_pkg.py @@ -231,10 +231,13 @@ def test_pkg_002_installed_with_version(PKG_TARGETS, states, latest_version): @pytest.mark.requires_salt_states("pkg.installed", "pkg.removed") @pytest.mark.slow_test -def test_pkg_003_installed_multipkg(caplog, PKG_TARGETS, modules, states): +def test_pkg_003_installed_multipkg(caplog, PKG_TARGETS, modules, states, grains): """ This is a destructive test as it installs and then removes two packages """ + if grains["os_family"] == "Arch": + pytest.skip("Arch needs refresh_db logic added to golden image") + version = modules.pkg.version(*PKG_TARGETS) # If this assert fails, we need to find new targets, this test needs to @@ -256,10 +259,14 @@ def test_pkg_003_installed_multipkg(caplog, PKG_TARGETS, modules, states): @pytest.mark.usefixtures("VERSION_SPEC_SUPPORTED") @pytest.mark.requires_salt_states("pkg.installed", "pkg.removed") @pytest.mark.slow_test -def test_pkg_004_installed_multipkg_with_version(PKG_TARGETS, latest_version, states): +def test_pkg_004_installed_multipkg_with_version( + PKG_TARGETS, latest_version, states, grains +): """ This is a destructive test as it installs and then removes two packages """ + if grains["os_family"] == "Arch": + pytest.skip("Arch needs refresh_db logic added to golden image") version = latest_version(PKG_TARGETS[0]) # If this assert fails, we need to find new targets, this test needs to @@ -868,6 +875,7 @@ def test_pkg_cap_003_installed_multipkg_with_version( latest_version, modules, states, + grains, ): """ This is a destructive test as it installs and then removes two packages diff --git a/tests/pytests/functional/transport/base.py b/tests/pytests/functional/transport/base.py new file mode 100644 index 000000000000..de573f05fdef --- /dev/null +++ b/tests/pytests/functional/transport/base.py @@ -0,0 +1,194 @@ +import multiprocessing +import stat +import time + +import pytest + +import salt.transport.base +import salt.transport.tcp +import salt.transport.ws +import salt.transport.zeromq + + +@pytest.mark.parametrize("kind", salt.transport.base.TRANSPORTS) +def test_master_ipc_socket_perms(kind, tmp_path): + opts = { + "ipc_mode": "ipc", + "hash_type": "md5", + "hash_id": "master", + "id": "master", + "sock_dir": str(tmp_path), + } + server = salt.transport.base.ipc_publish_server("master", opts) + + # IPC Server always uses tcp transport, this could change in the future. + assert isinstance(server, salt.transport.tcp.PublishServer) + + proc = multiprocessing.Process(target=server.publish_daemon, args=(lambda x: x,)) + proc.start() + time.sleep(1) + try: + pub_path = tmp_path / "master_event_pub.ipc" + assert pub_path.exists() + status = pub_path.stat() + + assert status.st_mode & stat.S_IRUSR + assert status.st_mode & stat.S_IWUSR + assert not status.st_mode & stat.S_IXUSR + + assert status.st_mode & stat.S_IRGRP + assert status.st_mode & stat.S_IWGRP + assert not status.st_mode & stat.S_IXGRP + + assert not status.st_mode & stat.S_IROTH + assert not status.st_mode & stat.S_IWOTH + assert not status.st_mode & stat.S_IXOTH + + pull_path = tmp_path / "master_event_pull.ipc" + status = pull_path.stat() + + assert status.st_mode & stat.S_IRUSR + assert status.st_mode & stat.S_IWUSR + assert not status.st_mode & stat.S_IXUSR + + assert not status.st_mode & stat.S_IRGRP + assert not status.st_mode & stat.S_IWGRP + assert not status.st_mode & stat.S_IXGRP + + assert not status.st_mode & stat.S_IROTH + assert not status.st_mode & stat.S_IWOTH + assert not status.st_mode & stat.S_IXOTH + finally: + proc.terminate() + proc.join() + proc.close() + + +@pytest.mark.parametrize("kind", salt.transport.base.TRANSPORTS) +def test_minion_ipc_socket_perms(kind, tmp_path): + opts = { + "ipc_mode": "ipc", + "hash_type": "md5", + "hash_id": "minion", + "id": "minion", + "sock_dir": str(tmp_path), + } + server = salt.transport.base.ipc_publish_server("minion", opts) + + # IPC Server always uses tcp transport, this could change in the future. + assert isinstance(server, salt.transport.tcp.PublishServer) + + proc = multiprocessing.Process(target=server.publish_daemon, args=(lambda x: x,)) + proc.start() + time.sleep(1) + try: + id_hash = salt.transport.base._minion_hash( + hash_type=opts["hash_type"], + minion_id=opts.get("hash_id", opts["id"]), + ) + pub_path = tmp_path / f"minion_event_{id_hash}_pub.ipc" + assert pub_path.exists() + status = pub_path.stat() + + assert status.st_mode & stat.S_IRUSR + assert status.st_mode & stat.S_IWUSR + assert not status.st_mode & stat.S_IXUSR + + assert not status.st_mode & stat.S_IRGRP + assert not status.st_mode & stat.S_IWGRP + assert not status.st_mode & stat.S_IXGRP + + assert not status.st_mode & stat.S_IROTH + assert not status.st_mode & stat.S_IWOTH + assert not status.st_mode & stat.S_IXOTH + + pull_path = tmp_path / f"minion_event_{id_hash}_pull.ipc" + status = pull_path.stat() + + assert status.st_mode & stat.S_IRUSR + assert status.st_mode & stat.S_IWUSR + assert not status.st_mode & stat.S_IXUSR + + assert not status.st_mode & stat.S_IRGRP + assert not status.st_mode & stat.S_IWGRP + assert not status.st_mode & stat.S_IXGRP + + assert not status.st_mode & stat.S_IROTH + assert not status.st_mode & stat.S_IWOTH + assert not status.st_mode & stat.S_IXOTH + finally: + proc.terminate() + proc.join() + proc.close() + + +TRANSPORT_MAP = { + "zeromq": salt.transport.zeromq.PublishServer, + "tcp": salt.transport.tcp.PublishServer, + "ws": salt.transport.ws.PublishServer, +} + + +def test_check_all_transports(): + """ + Ensure we are testing all existing transports. If adding a transport it + should be tested by 'test_transport_socket_perms_conform'. + """ + assert sorted(TRANSPORT_MAP.keys()) == sorted(salt.transport.base.TRANSPORTS) + + +@pytest.mark.parametrize("kind", salt.transport.base.TRANSPORTS) +def test_transport_socket_perms_conform(kind, tmp_path): + opts = { + "ipc_mode": "ipc", + "hash_type": "md5", + "hash_id": "master", + "id": "master", + "ipv6": False, + "sock_dir": str(tmp_path), + } + kwargs = { + "pub_path": str(tmp_path / "pub.ipc"), + "pull_path": str(tmp_path / "pull.ipc"), + "pub_path_perms": 0o660, + } + server = TRANSPORT_MAP[kind](opts, **kwargs) + + proc = multiprocessing.Process(target=server.publish_daemon, args=(lambda x: x,)) + proc.start() + time.sleep(1) + try: + pub_path = tmp_path / "pub.ipc" + assert pub_path.exists() + status = pub_path.stat() + + assert status.st_mode & stat.S_IRUSR + assert status.st_mode & stat.S_IWUSR + assert not status.st_mode & stat.S_IXUSR + + assert status.st_mode & stat.S_IRGRP + assert status.st_mode & stat.S_IWGRP + assert not status.st_mode & stat.S_IXGRP + + assert not status.st_mode & stat.S_IROTH + assert not status.st_mode & stat.S_IWOTH + assert not status.st_mode & stat.S_IXOTH + + pull_path = tmp_path / "pull.ipc" + status = pull_path.stat() + + assert status.st_mode & stat.S_IRUSR + assert status.st_mode & stat.S_IWUSR + assert not status.st_mode & stat.S_IXUSR + + assert not status.st_mode & stat.S_IRGRP + assert not status.st_mode & stat.S_IWGRP + assert not status.st_mode & stat.S_IXGRP + + assert not status.st_mode & stat.S_IROTH + assert not status.st_mode & stat.S_IWOTH + assert not status.st_mode & stat.S_IXOTH + finally: + proc.terminate() + proc.join() + proc.close() diff --git a/tests/pytests/integration/master/test_ipc_perms.py b/tests/pytests/integration/master/test_ipc_perms.py new file mode 100644 index 000000000000..85f70be893df --- /dev/null +++ b/tests/pytests/integration/master/test_ipc_perms.py @@ -0,0 +1,12 @@ +import pathlib +import stat + + +def test_master_event_pub_ipc_perms(salt_master): + pub_path = pathlib.Path(salt_master.config["sock_dir"]) / "master_event_pub.ipc" + assert pub_path.exists() + status = pub_path.stat() + assert status.st_mode & stat.S_IRUSR + assert status.st_mode & stat.S_IWUSR + assert status.st_mode & stat.S_IRGRP + assert status.st_mode & stat.S_IWGRP diff --git a/tests/unit/test_module_names.py b/tests/unit/test_module_names.py index 15d06e0ed66f..147945f02b3f 100644 --- a/tests/unit/test_module_names.py +++ b/tests/unit/test_module_names.py @@ -44,6 +44,7 @@ os.path.join("tests", "wheeltest.py"), os.path.join("tests", "zypp_plugin.py"), os.path.join("tests", "pytests", "functional", "cache", "helpers.py"), + os.path.join("tests", "pytests", "functional", "transport", "base.py"), os.path.join("tests", "pytests", "unit", "states", "virt", "helpers.py"), ]