From 70509ff67d4eb734c88032913134092257a0d35b Mon Sep 17 00:00:00 2001 From: Flex Liu Date: Tue, 2 Jul 2024 15:25:30 +0800 Subject: [PATCH] Fix user.list_groups omits remote groups * fixes saltstack/salt#64953 user.list_groups omits remote groups * fixes saltstack/salt#65029 support for pysss can be removed * add changlog entries * add tests for _getgrall and local vs remote group handling * add negative tests for _getgrall * root can still read the file and tests run as root * remove permission check as its probably an unreachable edge case --------- Co-authored-by: nicholasmhughes --- changelog/64888.fixed.md | 1 + changelog/64953.fixed.md | 1 + changelog/65029.removed.md | 1 + salt/auth/pam.py | 9 --- salt/utils/user.py | 73 ++++++++++++------- .../functional/utils/user/test__getgrall.py | 44 +++++++++++ tests/pytests/unit/utils/test_user.py | 29 ++++++++ 7 files changed, 122 insertions(+), 36 deletions(-) create mode 100644 changelog/64888.fixed.md create mode 100644 changelog/64953.fixed.md create mode 100644 changelog/65029.removed.md create mode 100644 tests/pytests/functional/utils/user/test__getgrall.py create mode 100644 tests/pytests/unit/utils/test_user.py diff --git a/changelog/64888.fixed.md b/changelog/64888.fixed.md new file mode 100644 index 0000000000..08b2efd042 --- /dev/null +++ b/changelog/64888.fixed.md @@ -0,0 +1 @@ +Fixed grp.getgrall() in utils/user.py causing performance issues diff --git a/changelog/64953.fixed.md b/changelog/64953.fixed.md new file mode 100644 index 0000000000..f0b1ed46f1 --- /dev/null +++ b/changelog/64953.fixed.md @@ -0,0 +1 @@ +Fix user.list_groups omits remote groups via sssd, etc. diff --git a/changelog/65029.removed.md b/changelog/65029.removed.md new file mode 100644 index 0000000000..d09f10b4ba --- /dev/null +++ b/changelog/65029.removed.md @@ -0,0 +1 @@ +Tech Debt - support for pysss removed due to functionality addition in Python 3.3 diff --git a/salt/auth/pam.py b/salt/auth/pam.py index f0397c1062..12af29bbdb 100644 --- a/salt/auth/pam.py +++ b/salt/auth/pam.py @@ -24,15 +24,6 @@ authenticated against. This defaults to `login` The Python interface to PAM does not support authenticating as ``root``. -.. note:: Using PAM groups with SSSD groups on python2. - - To use sssd with the PAM eauth module and groups the `pysss` module is - needed. On RedHat/CentOS this is `python-sss`. - - This should not be needed with python >= 3.3, because the `os` modules has the - `getgrouplist` function. - - .. note:: This module executes itself in a subprocess in order to user the system python and pam libraries. We do this to avoid openssl version conflicts when running under a salt onedir build. diff --git a/salt/utils/user.py b/salt/utils/user.py index 2f1ca65cf9..3588b3804a 100644 --- a/salt/utils/user.py +++ b/salt/utils/user.py @@ -31,13 +31,6 @@ try: except ImportError: HAS_GRP = False -try: - import pysss - - HAS_PYSSS = True -except ImportError: - HAS_PYSSS = False - try: import salt.utils.win_functions @@ -289,30 +282,35 @@ def get_group_list(user, include_default=True): return [] group_names = None ugroups = set() - if hasattr(os, "getgrouplist"): - # Try os.getgrouplist, available in python >= 3.3 - log.trace("Trying os.getgrouplist for '%s'", user) - try: - user_group_list = os.getgrouplist(user, pwd.getpwnam(user).pw_gid) - group_names = [ - _group.gr_name - for _group in grp.getgrall() - if _group.gr_gid in user_group_list - ] - except Exception: # pylint: disable=broad-except - pass - elif HAS_PYSSS: - # Try pysss.getgrouplist - log.trace("Trying pysss.getgrouplist for '%s'", user) - try: - group_names = list(pysss.getgrouplist(user)) - except Exception: # pylint: disable=broad-except - pass + # Try os.getgrouplist, available in python >= 3.3 + log.trace("Trying os.getgrouplist for '%s'", user) + try: + user_group_list = sorted(os.getgrouplist(user, pwd.getpwnam(user).pw_gid)) + local_grall = _getgrall() + local_gids = sorted(lgrp.gr_gid for lgrp in local_grall) + max_idx = -1 + local_max = local_gids[max_idx] + while local_max >= 65000: + max_idx -= 1 + local_max = local_gids[max_idx] + user_group_list_local = [lgrp for lgrp in user_group_list if lgrp <= local_max] + user_group_list_remote = [rgrp for rgrp in user_group_list if rgrp > local_max] + local_group_names = [ + _group.gr_name + for _group in local_grall + if _group.gr_gid in user_group_list_local + ] + remote_group_names = [ + grp.getgrgid(group_id).gr_name for group_id in user_group_list_remote + ] + group_names = local_group_names + remote_group_names + except Exception: # pylint: disable=broad-except + pass if group_names is None: # Fall back to generic code # Include the user's default group to match behavior of - # os.getgrouplist() and pysss.getgrouplist() + # os.getgrouplist() log.trace("Trying generic group list for '%s'", user) group_names = [g.gr_name for g in grp.getgrall() if user in g.gr_mem] try: @@ -389,3 +387,24 @@ def get_gid(group=None): return grp.getgrnam(group).gr_gid except KeyError: return None + + +def _getgrall(root=None): + """ + Alternative implemetantion for getgrall, that uses only /etc/group + """ + ret = [] + root = "/" if not root else root + etc_group = os.path.join(root, "etc/group") + with salt.utils.files.fopen(etc_group) as fp_: + for line in fp_: + line = salt.utils.stringutils.to_unicode(line) + comps = line.strip().split(":") + # Generate a getgrall compatible output + comps[2] = int(comps[2]) + if comps[3]: + comps[3] = [mem.strip() for mem in comps[3].split(",")] + else: + comps[3] = [] + ret.append(grp.struct_group(comps)) + return ret diff --git a/tests/pytests/functional/utils/user/test__getgrall.py b/tests/pytests/functional/utils/user/test__getgrall.py new file mode 100644 index 0000000000..db994019e6 --- /dev/null +++ b/tests/pytests/functional/utils/user/test__getgrall.py @@ -0,0 +1,44 @@ +from textwrap import dedent + +import pytest + +pytest.importorskip("grp") + +import grp + +import salt.utils.user + + +@pytest.fixture(scope="function") +def etc_group(tmp_path): + etcgrp = tmp_path / "etc" / "group" + etcgrp.parent.mkdir() + etcgrp.write_text( + dedent( + """games:x:50: + docker:x:959:debian,salt + salt:x:1000:""" + ) + ) + return etcgrp + + +def test__getgrall(etc_group): + group_lines = [ + ["games", "x", 50, []], + ["docker", "x", 959, ["debian", "salt"]], + ["salt", "x", 1000, []], + ] + expected_grall = [grp.struct_group(comps) for comps in group_lines] + + grall = salt.utils.user._getgrall(root=str(etc_group.parent.parent)) + + assert grall == expected_grall + + +def test__getgrall_bad_format(etc_group): + with etc_group.open("a") as _fp: + _fp.write("\n# some comment here\n") + + with pytest.raises(IndexError): + salt.utils.user._getgrall(root=str(etc_group.parent.parent)) diff --git a/tests/pytests/unit/utils/test_user.py b/tests/pytests/unit/utils/test_user.py new file mode 100644 index 0000000000..17c6b1551f --- /dev/null +++ b/tests/pytests/unit/utils/test_user.py @@ -0,0 +1,29 @@ +from types import SimpleNamespace + +import pytest + +from tests.support.mock import MagicMock, patch + +pytest.importorskip("grp") + +import grp + +import salt.utils.user + + +def test_get_group_list(): + getpwname = SimpleNamespace(pw_gid=1000) + getgrgid = MagicMock(side_effect=[SimpleNamespace(gr_name="remote")]) + group_lines = [ + ["games", "x", 50, []], + ["salt", "x", 1000, []], + ] + getgrall = [grp.struct_group(comps) for comps in group_lines] + with patch("os.getgrouplist", MagicMock(return_value=[50, 1000, 12000])), patch( + "pwd.getpwnam", MagicMock(return_value=getpwname) + ), patch("salt.utils.user._getgrall", MagicMock(return_value=getgrall)), patch( + "grp.getgrgid", getgrgid + ): + group_list = salt.utils.user.get_group_list("salt") + assert group_list == ["games", "remote", "salt"] + getgrgid.assert_called_once() -- 2.35.3