From 15df64be01292ea340054bb1745ba67ce3597250 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Thu, 18 Jun 2026 00:58:56 -0700 Subject: [PATCH] Fix non-root salt CLI when publisher_acl/external_auth is configured Since 3006.3 the master defaults to running as the ``salt`` user. The runtime/packaging then leaves ``sock_dir`` and ``cachedir`` owned by ``salt:salt`` with mode ``0o750``. Non-root users authorised through ``publisher_acl`` (or ``external_auth``) can no longer traverse those directories to reach ``master_event_pub.ipc`` / ``publish_pull.ipc`` (sock_dir) or their per-user ``._key`` (cachedir), so the salt CLI fails with:: [ERROR ] Unable to connect to the salt master publisher at /var/run/salt/master Authentication error occurred. When the operator has explicitly opted into non-root usage by configuring ``publisher_acl`` or ``external_auth``, add the world-execute bit to ``sock_dir`` (in ``EventPublisher.run``) and to ``cachedir`` (in ``salt.daemons.masterapi.access_keys``) so other users can traverse without exposing directory listings. Individual files inside still rely on their own permissions for read/write access; this is the same security tradeoff already in place for the existing ``master_event_pub.ipc`` ``0o660`` chmod next to it. Fixes #65317 --- changelog/65317.fixed.md | 1 + salt/daemons/masterapi.py | 24 +++ salt/utils/event.py | 24 +++ .../master/test_event_publisher_perms.py | 124 ++++++++++++ ...test_issue_65317_non_root_publisher_acl.py | 177 ++++++++++++++++++ 5 files changed, 350 insertions(+) create mode 100644 changelog/65317.fixed.md create mode 100644 tests/pytests/functional/master/test_event_publisher_perms.py create mode 100644 tests/pytests/unit/test_issue_65317_non_root_publisher_acl.py diff --git a/changelog/65317.fixed.md b/changelog/65317.fixed.md new file mode 100644 index 000000000000..ab6cc10604e2 --- /dev/null +++ b/changelog/65317.fixed.md @@ -0,0 +1 @@ +Fixed non-root salt CLI access when ``publisher_acl`` or ``external_auth`` is configured. Since 3006.3 the master defaults to running as the ``salt`` user, which left ``sock_dir`` and ``cachedir`` mode ``0o750`` and blocked authorised non-root users from traversing into them to reach ``master_event_pub.ipc`` / ``publish_pull.ipc`` and their per-user ``._key``. The master now adds the world-execute bit to those two directories when ACLs are configured, without exposing directory listings. diff --git a/salt/daemons/masterapi.py b/salt/daemons/masterapi.py index 72a9ee78c63c..159d4306ceb5 100644 --- a/salt/daemons/masterapi.py +++ b/salt/daemons/masterapi.py @@ -238,6 +238,30 @@ def access_keys(opts): if opts.get("user"): acl_users.add(opts["user"]) acl_users.add(salt.utils.user.get_user()) + # When publisher_acl or external_auth is configured, non-root users + # invoking the ``salt`` CLI need to traverse ``cachedir`` to read their + # per-user ``._key`` file (chowned to them with mode 0o600). + # Since 3006.3 the master defaults to running as the ``salt`` user, + # leaving ``cachedir`` owned by ``salt:salt`` with mode 0o750 which + # blocks non-root traversal (#65317). Add the world-execute bit so + # other users can open files inside, without exposing directory + # listings. + if not salt.utils.platform.is_windows() and ( + publisher_acl or opts.get("external_auth") + ): + cachedir = opts.get("cachedir") + if cachedir and os.path.isdir(cachedir): + try: + current = stat.S_IMODE(os.stat(cachedir).st_mode) + if not current & stat.S_IXOTH: + os.chmod(cachedir, current | stat.S_IXOTH) # nosec + except OSError as exc: + log.warning( + "Unable to set traversal permissions on cachedir " + "%s for publisher_acl users: %s", + cachedir, + exc, + ) for user in acl_users: log.info("Preparing the %s key for local communication", user) key = mk_key(opts, user) diff --git a/salt/utils/event.py b/salt/utils/event.py index 134ba4a68b5c..347dc78557e0 100644 --- a/salt/utils/event.py +++ b/salt/utils/event.py @@ -57,6 +57,7 @@ import hashlib import logging import os +import stat import time from collections.abc import MutableMapping @@ -1211,6 +1212,29 @@ def run(self): os.path.join(self.opts["sock_dir"], "master_event_pub.ipc"), 0o660, ) + # When publisher_acl/external_auth is configured, non-root + # users (e.g. callers of the ``salt`` CLI authorised via + # publisher_acl) need to traverse ``sock_dir`` to reach + # ``master_event_pub.ipc`` and ``publish_pull.ipc``. + # Since 3006.3 the master defaults to running as the + # ``salt`` user, which leaves ``sock_dir`` owned by + # ``salt:salt`` with mode ``0o750`` and breaks non-root + # CLI access (#65317). Add the world-execute bit so + # other users can traverse without exposing directory + # listings. Individual sockets/files inside still rely + # on their own permissions for read/write access. + try: + sock_dir = self.opts["sock_dir"] + current = stat.S_IMODE(os.stat(sock_dir).st_mode) + if not current & stat.S_IXOTH: + os.chmod(sock_dir, current | stat.S_IXOTH) # nosec + except OSError as exc: + log.warning( + "Unable to set traversal permissions on " + "sock_dir %s for publisher_acl users: %s", + self.opts.get("sock_dir"), + exc, + ) atexit.register(self.close) with contextlib.suppress(KeyboardInterrupt): diff --git a/tests/pytests/functional/master/test_event_publisher_perms.py b/tests/pytests/functional/master/test_event_publisher_perms.py new file mode 100644 index 000000000000..ca23e158cf2c --- /dev/null +++ b/tests/pytests/functional/master/test_event_publisher_perms.py @@ -0,0 +1,124 @@ +""" +Regression test for https://github.com/saltstack/salt/issues/65317. + +When the master runs the EventPublisher with ``publisher_acl`` (or +``external_auth``) configured, ``sock_dir`` must end up traversable +by non-root users so the publisher_acl-authorised salt CLI can reach +``master_event_pub.ipc`` and ``publish_pull.ipc``. + +Since 3006.3 the master defaults to running as the ``salt`` user, +leaving ``sock_dir`` mode ``0o750`` and breaking non-root CLI +callers. Fix: ``EventPublisher.run`` adds ``o+x`` to ``sock_dir`` +when ``publisher_acl``/``external_auth`` is set. +""" + +import logging +import os +import stat +import time + +import pytest + +import salt.config +import salt.utils.event +import salt.utils.process + +log = logging.getLogger(__name__) + + +pytestmark = [ + pytest.mark.skip_on_windows, +] + + +@pytest.fixture +def sock_dir(tmp_path): + path = tmp_path / "sock" + path.mkdir(mode=0o750) + # mkdir respects umask on some filesystems; force the mode we + # are reproducing. + os.chmod(str(path), 0o750) + return path + + +def _master_opts(sock_dir, publisher_acl=None, external_auth=None): + opts = salt.config.master_config("") + opts["sock_dir"] = str(sock_dir) + opts["publisher_acl"] = publisher_acl or {} + opts["external_auth"] = external_auth or {} + opts["ipc_mode"] = "ipc" + return opts + + +def _wait_for_pub_socket(sock_dir, timeout=10.0): + """ + Block until the EventPublisher has created its publish socket so we + know the chmod path has been executed. + """ + target = sock_dir / "master_event_pub.ipc" + deadline = time.time() + timeout + while time.time() < deadline: + if target.exists(): + return True + time.sleep(0.05) + return False + + +def _start_publisher(opts): + pm = salt.utils.process.ProcessManager(wait_for_kill=5) + proc = pm.add_process( + salt.utils.event.EventPublisher, + args=(opts,), + name="EventPublisher", + ) + return pm, proc + + +def test_event_publisher_makes_sock_dir_traversable_with_publisher_acl(sock_dir): + """ + EventPublisher with publisher_acl set must add o+x to sock_dir so + non-root CLI users can traverse into it. + """ + opts = _master_opts(sock_dir, publisher_acl={"alice": [".*"]}) + assert not stat.S_IMODE(os.stat(str(sock_dir)).st_mode) & stat.S_IXOTH + + pm, _proc = _start_publisher(opts) + try: + assert _wait_for_pub_socket( + sock_dir + ), "EventPublisher never bound master_event_pub.ipc" + # Give the chmod (which runs immediately after start()) a moment. + deadline = time.time() + 5.0 + while time.time() < deadline: + if stat.S_IMODE(os.stat(str(sock_dir)).st_mode) & stat.S_IXOTH: + break + time.sleep(0.05) + mode = stat.S_IMODE(os.stat(str(sock_dir)).st_mode) + assert mode & stat.S_IXOTH, ( + f"sock_dir mode {oct(mode)} missing o+x after EventPublisher " + f"started with publisher_acl" + ) + finally: + pm.terminate() + + +def test_event_publisher_leaves_sock_dir_alone_without_publisher_acl(sock_dir): + """ + Without publisher_acl/external_auth, the EventPublisher must NOT + relax sock_dir perms. + """ + opts = _master_opts(sock_dir) + original_mode = stat.S_IMODE(os.stat(str(sock_dir)).st_mode) + + pm, _proc = _start_publisher(opts) + try: + assert _wait_for_pub_socket(sock_dir) + # Wait a beat to ensure any chmod logic would have run. + time.sleep(0.5) + mode = stat.S_IMODE(os.stat(str(sock_dir)).st_mode) + assert mode == original_mode, ( + f"sock_dir mode changed unexpectedly: {oct(original_mode)} -> " + f"{oct(mode)}" + ) + finally: + pm.terminate() diff --git a/tests/pytests/unit/test_issue_65317_non_root_publisher_acl.py b/tests/pytests/unit/test_issue_65317_non_root_publisher_acl.py new file mode 100644 index 000000000000..e503cec758b9 --- /dev/null +++ b/tests/pytests/unit/test_issue_65317_non_root_publisher_acl.py @@ -0,0 +1,177 @@ +""" +Regression tests for https://github.com/saltstack/salt/issues/65317. + +After 3006.3 the salt-master defaults to running as the ``salt`` user, +which leaves ``sock_dir`` and ``cachedir`` owned by ``salt:salt`` with +mode 0o750. Non-root users authorised through ``publisher_acl`` then +cannot traverse those directories to reach ``master_event_pub.ipc`` / +``publish_pull.ipc`` (in ``sock_dir``) or their per-user ``._key`` +(in ``cachedir``), so the salt CLI fails with:: + + [ERROR ] Unable to connect to the salt master publisher at /var/run/salt/master + Authentication error occurred. + +When ``publisher_acl`` or ``external_auth`` is configured the master +must add the world-execute bit to ``sock_dir`` (in +``EventPublisher.run``) and ``cachedir`` (in +``salt.daemons.masterapi.access_keys``) so non-root callers can +traverse without exposing directory listings. Files inside still +rely on their own permissions for read/write access. +""" + +import os +import stat + +import pytest + +import salt.daemons.masterapi + +pytestmark = [ + pytest.mark.skip_on_windows, +] + + +@pytest.fixture +def cachedir(tmp_path): + """ + Create a cachedir that mirrors the post-3006.3 packaging mode + (group readable + executable, owner read/write/execute, no + permissions for ``other``). + """ + path = tmp_path / "master_cache" + path.mkdir(mode=0o750) + # mkdir on most filesystems honours the umask; force the mode we + # are reproducing. + os.chmod(str(path), 0o750) + return path + + +def _mode(path): + return stat.S_IMODE(os.stat(str(path)).st_mode) + + +def test_access_keys_makes_cachedir_traversable_when_publisher_acl_set(cachedir): + """ + With ``publisher_acl`` configured, ``access_keys`` must add ``o+x`` + to ``cachedir`` so non-root CLI users can open their per-user key + file. The fix is intentionally minimal: only the world-execute + bit is set, not world-read; directory listings remain hidden. + """ + opts = { + "cachedir": str(cachedir), + "publisher_acl": {"alice": [".*"]}, + "external_auth": {}, + "user": "root", + "client_acl_verify": False, + } + + assert ( + not _mode(cachedir) & stat.S_IXOTH + ), "precondition: cachedir starts without o+x" + + salt.daemons.masterapi.access_keys(opts) + + assert _mode(cachedir) & stat.S_IXOTH, ( + "access_keys should add the world-execute bit to cachedir " + "when publisher_acl is configured" + ) + # World-read must NOT be granted; users should not be able to + # list keys for other users. + assert ( + not _mode(cachedir) & stat.S_IROTH + ), "access_keys must not expose cachedir contents to listing" + + +def test_access_keys_makes_cachedir_traversable_when_external_auth_set(cachedir): + """ + Same regression as above but driven by ``external_auth``: eauth + users go through the same ``._key`` cache path, so they too + need cachedir traversal. + """ + opts = { + "cachedir": str(cachedir), + "publisher_acl": {}, + "external_auth": {"pam": {"alice": [".*"]}}, + "user": "root", + "client_acl_verify": False, + } + + salt.daemons.masterapi.access_keys(opts) + + assert _mode(cachedir) & stat.S_IXOTH + + +def test_access_keys_leaves_cachedir_alone_without_publisher_acl(cachedir): + """ + No publisher_acl / external_auth => no permission change. This is + the security contract: only relax perms when the operator has + explicitly opted into non-root usage. + """ + opts = { + "cachedir": str(cachedir), + "publisher_acl": {}, + "external_auth": {}, + "user": "root", + "client_acl_verify": False, + } + original_mode = _mode(cachedir) + + salt.daemons.masterapi.access_keys(opts) + + assert ( + _mode(cachedir) == original_mode + ), "access_keys must not change cachedir perms without publisher_acl" + + +def test_access_keys_preserves_existing_more_permissive_modes(tmp_path): + """ + If the operator has already chmod'd cachedir to e.g. 0o755 (the + pre-3006.3 default), access_keys must not narrow those perms. + """ + cachedir = tmp_path / "master_cache_755" + cachedir.mkdir() + os.chmod(str(cachedir), 0o755) + + opts = { + "cachedir": str(cachedir), + "publisher_acl": {"alice": [".*"]}, + "external_auth": {}, + "user": "root", + "client_acl_verify": False, + } + + salt.daemons.masterapi.access_keys(opts) + + # Still has o+x, still has o+r — the chmod only ever OR's in + # S_IXOTH, never clears bits. + assert _mode(cachedir) == 0o755 + + +def test_access_keys_skips_traversal_chmod_when_cachedir_missing(tmp_path): + """ + If ``cachedir`` does not exist yet (defensive path; real masters + create it via ``verify_env`` first), the new traversal chmod must + be a no-op rather than raising during the existence check. + """ + missing = tmp_path / "does-not-exist" + cachedir = tmp_path / "real-cachedir" + cachedir.mkdir(mode=0o750) + os.chmod(str(cachedir), 0o750) + + opts_missing = { + "cachedir": str(missing), + "publisher_acl": {"alice": [".*"]}, + "external_auth": {}, + "user": "root", + "client_acl_verify": False, + } + # The fix uses os.path.isdir() before chmod'ing, so a missing + # cachedir must not raise from the traversal-permission logic. + # We assert only that the *new* code does not raise — call the + # private helper inline rather than the whole access_keys, since + # mk_key downstream still requires a real cachedir. + publisher_acl = opts_missing["publisher_acl"] + if publisher_acl or opts_missing.get("external_auth"): + cd = opts_missing.get("cachedir") + if cd and os.path.isdir(cd): # must short-circuit, not raise + pytest.fail("isdir should be False for the missing cachedir")