diff --git a/changelog/66931.fixed.md b/changelog/66931.fixed.md new file mode 100644 index 00000000000..264c89e4427 --- /dev/null +++ b/changelog/66931.fixed.md @@ -0,0 +1,4 @@ +transports.tcp: ensure pull path is being used before attempting chmod. +The fix prevents an unnecessary traceback when the TCP transport is +not using unix sockets. No functionaly has changed as the traceback +occurs when an async task was about to exit anyway. diff --git a/salt/transport/tcp.py b/salt/transport/tcp.py index b45deebd965..de758c9eccc 100644 --- a/salt/transport/tcp.py +++ b/salt/transport/tcp.py @@ -10,7 +10,6 @@ import asyncio.exceptions import errno import logging import multiprocessing -import os import queue import select import socket @@ -1150,7 +1149,13 @@ class TCPPuller: """ def __init__( - self, host=None, port=None, path=None, io_loop=None, payload_handler=None + self, + host=None, + port=None, + path=None, + mode=0o600, + io_loop=None, + payload_handler=None, ): """ Create a new Tornado IPC server @@ -1170,6 +1175,7 @@ class TCPPuller: self.host = host self.port = port self.path = path + self.mode = mode self._started = False self.payload_handler = payload_handler @@ -1187,7 +1193,7 @@ class TCPPuller: # Start up the ioloop if self.path: log.trace("IPCServer: binding to socket: %s", self.path) - self.sock = tornado.netutil.bind_unix_socket(self.path) + self.sock = tornado.netutil.bind_unix_socket(self.path, self.mode) else: log.trace("IPCServer: binding to socket: %s:%s", self.host, self.port) self.sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) @@ -1421,8 +1427,9 @@ class PublishServer(salt.transport.base.DaemonizedPublishServer): "Publish server binding pub to %s ssl=%r", self.pub_path, self.ssl ) 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) + sock = tornado.netutil.bind_unix_socket( + self.pub_path, self.pub_path_perms + ) else: log.debug( "Publish server binding pub to %s:%s ssl=%r", @@ -1451,18 +1458,17 @@ class PublishServer(salt.transport.base.DaemonizedPublishServer): pull_host = self.pull_host pull_port = self.pull_port - self.pull_sock = TCPPuller( - host=self.pull_host, - port=self.pull_port, - path=self.pull_path, - io_loop=io_loop, - payload_handler=publish_payload, - ) - - # Securely create socket with salt.utils.files.set_umask(0o177): + self.pull_sock = TCPPuller( + host=self.pull_host, + port=self.pull_port, + path=self.pull_path, + mode=self.pull_path_perms, + io_loop=io_loop, + payload_handler=publish_payload, + ) + # Securely create socket self.pull_sock.start() - os.chmod(self.pull_path, self.pull_path_perms) self.started.set() def pre_fork(self, process_manager): diff --git a/tests/pytests/unit/transport/test_tcp.py b/tests/pytests/unit/transport/test_tcp.py index d064b561c11..4066abbd5a1 100644 --- a/tests/pytests/unit/transport/test_tcp.py +++ b/tests/pytests/unit/transport/test_tcp.py @@ -685,3 +685,62 @@ async def test_pub_server_publish_payload_closed_stream(master_opts, io_loop): server.clients = {client} await server.publish_payload(package, topic_list) assert server.clients == set() + + +async def test_pub_server_pull_path_no_perms(master_opts, io_loop): + def publish_payload(payload): + return payload + + pubserv = salt.transport.tcp.PublishServer( + master_opts, + pub_host="127.0.0.1", + pub_port=5151, + pull_host="127.0.0.1", + pull_port=5152, + ) + with patch("os.chmod") as p: + await pubserv.publisher(publish_payload) + assert p.call_count == 0 + + +async def test_pub_server_publisher_pull_path_perms(master_opts, io_loop, tmp_path): + def publish_payload(payload): + return payload + + pull_path = tmp_path / "pull.ipc" + pull_path_perms = 0o664 + pubserv = salt.transport.tcp.PublishServer( + master_opts, + pub_host="127.0.0.1", + pub_port=5151, + pull_host=None, + pull_port=None, + pull_path=str(pull_path), + pull_path_perms=pull_path_perms, + ) + with patch("os.chmod") as p: + await pubserv.publisher(publish_payload) + assert p.call_count == 1 + assert p.call_args.args == (pubserv.pull_path, pubserv.pull_path_perms) + + +async def test_pub_server_publisher_pub_path_perms(master_opts, io_loop, tmp_path): + def publish_payload(payload): + return payload + + pub_path = tmp_path / "pub.ipc" + pub_path_perms = 0o664 + pubserv = salt.transport.tcp.PublishServer( + master_opts, + pub_host=None, + pub_port=None, + pub_path=str(pub_path), + pub_path_perms=pub_path_perms, + pull_host="127.0.0.1", + pull_port=5151, + pull_path=None, + ) + with patch("os.chmod") as p: + await pubserv.publisher(publish_payload) + assert p.call_count == 1 + assert p.call_args.args == (pubserv.pub_path, pubserv.pub_path_perms)