From 28be150d1a4ccbccfe3e02ad2af13dea4c7a8284 Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Mon, 28 Aug 2023 13:53:23 -0400 Subject: [PATCH 1/7] fixes saltstack/salt#64953 user.list_groups omits remote groups --- salt/utils/user.py | 48 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 43 insertions(+), 5 deletions(-) diff --git a/salt/utils/user.py b/salt/utils/user.py index 97636674430..7dc991bf8d8 100644 --- a/salt/utils/user.py +++ b/salt/utils/user.py @@ -293,12 +293,29 @@ def get_group_list(user, include_default=True): # 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 + 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 elif HAS_PYSSS: @@ -385,3 +402,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 From 34bf45ee2796dc635493e3148949ff79dd87a9f5 Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Mon, 28 Aug 2023 14:32:36 -0400 Subject: [PATCH 2/7] fixes saltstack/salt#65029 support for pysss can be removed --- salt/auth/pam.py | 9 ------- salt/utils/user.py | 67 +++++++++++++++++----------------------------- 2 files changed, 24 insertions(+), 52 deletions(-) diff --git a/salt/auth/pam.py b/salt/auth/pam.py index f0397c1062f..12af29bbdb8 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 7dc991bf8d8..c9c12f5ca64 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,47 +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 = 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 + # 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] - 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 - 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 + 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: From 0be8c475d9fc0da213e1b0a6e361a5d21f007420 Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Mon, 28 Aug 2023 14:37:30 -0400 Subject: [PATCH 3/7] add changlog entries --- changelog/64953.fixed.md | 1 + changelog/65029.removed.md | 1 + 2 files changed, 2 insertions(+) create mode 100644 changelog/64953.fixed.md create mode 100644 changelog/65029.removed.md diff --git a/changelog/64953.fixed.md b/changelog/64953.fixed.md new file mode 100644 index 00000000000..f0b1ed46f19 --- /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 00000000000..d09f10b4ba3 --- /dev/null +++ b/changelog/65029.removed.md @@ -0,0 +1 @@ +Tech Debt - support for pysss removed due to functionality addition in Python 3.3 From 0af4490c0f164a1f0e94395f916d4953bafb330e Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Mon, 28 Aug 2023 20:17:39 -0400 Subject: [PATCH 4/7] add tests for _getgrall and local vs remote group handling --- .../functional/utils/user/test__getgrall.py | 36 +++++++++++++++++++ tests/pytests/unit/utils/test_user.py | 29 +++++++++++++++ 2 files changed, 65 insertions(+) create mode 100644 tests/pytests/functional/utils/user/test__getgrall.py create mode 100644 tests/pytests/unit/utils/test_user.py 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 00000000000..d13bd2075b8 --- /dev/null +++ b/tests/pytests/functional/utils/user/test__getgrall.py @@ -0,0 +1,36 @@ +from textwrap import dedent + +import pytest + +pytest.importorskip("grp") + +import grp + +import salt.utils.user + + +@pytest.fixture() +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 diff --git a/tests/pytests/unit/utils/test_user.py b/tests/pytests/unit/utils/test_user.py new file mode 100644 index 00000000000..17c6b1551f5 --- /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() From 165621c166bee5bf71dd129bb9030d9738f40097 Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Wed, 30 Aug 2023 19:44:50 -0400 Subject: [PATCH 5/7] add negative tests for _getgrall --- changelog/64888.fixed.md | 1 + .../functional/utils/user/test__getgrall.py | 17 ++++++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 changelog/64888.fixed.md diff --git a/changelog/64888.fixed.md b/changelog/64888.fixed.md new file mode 100644 index 00000000000..08b2efd0424 --- /dev/null +++ b/changelog/64888.fixed.md @@ -0,0 +1 @@ +Fixed grp.getgrall() in utils/user.py causing performance issues diff --git a/tests/pytests/functional/utils/user/test__getgrall.py b/tests/pytests/functional/utils/user/test__getgrall.py index d13bd2075b8..bed8420cd8b 100644 --- a/tests/pytests/functional/utils/user/test__getgrall.py +++ b/tests/pytests/functional/utils/user/test__getgrall.py @@ -9,7 +9,7 @@ import grp import salt.utils.user -@pytest.fixture() +@pytest.fixture(scope="function") def etc_group(tmp_path): etcgrp = tmp_path / "etc" / "group" etcgrp.parent.mkdir() @@ -34,3 +34,18 @@ def test__getgrall(etc_group): grall = salt.utils.user._getgrall(root=str(etc_group.parent.parent)) assert grall == expected_grall + + +def test__getgrall_permission_denied(etc_group): + etc_group.chmod(0o000) + + with pytest.raises(PermissionError): + salt.utils.user._getgrall(root=str(etc_group.parent.parent)) + + +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)) From b19682a297e411e4c2f1a1eaa01beea00ce33875 Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Fri, 1 Sep 2023 17:13:03 -0400 Subject: [PATCH 6/7] root can still read the file and tests run as root --- tests/pytests/functional/utils/user/test__getgrall.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/pytests/functional/utils/user/test__getgrall.py b/tests/pytests/functional/utils/user/test__getgrall.py index bed8420cd8b..824cf9b2295 100644 --- a/tests/pytests/functional/utils/user/test__getgrall.py +++ b/tests/pytests/functional/utils/user/test__getgrall.py @@ -36,6 +36,7 @@ def test__getgrall(etc_group): assert grall == expected_grall +@pytest.mark.skip_if_root() def test__getgrall_permission_denied(etc_group): etc_group.chmod(0o000) From 8fbb5d3a26496e71d859fb9a5447dae652c0af7e Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Fri, 15 Sep 2023 10:32:12 -0400 Subject: [PATCH 7/7] remove permission check as its probably an unreachable edge case --- tests/pytests/functional/utils/user/test__getgrall.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/tests/pytests/functional/utils/user/test__getgrall.py b/tests/pytests/functional/utils/user/test__getgrall.py index 824cf9b2295..db994019e60 100644 --- a/tests/pytests/functional/utils/user/test__getgrall.py +++ b/tests/pytests/functional/utils/user/test__getgrall.py @@ -36,14 +36,6 @@ def test__getgrall(etc_group): assert grall == expected_grall -@pytest.mark.skip_if_root() -def test__getgrall_permission_denied(etc_group): - etc_group.chmod(0o000) - - with pytest.raises(PermissionError): - salt.utils.user._getgrall(root=str(etc_group.parent.parent)) - - def test__getgrall_bad_format(etc_group): with etc_group.open("a") as _fp: _fp.write("\n# some comment here\n")