salt/fix-user.list_groups-omits-remote-groups.patch

From 70509ff67d4eb734c88032913134092257a0d35b Mon Sep 17 00:00:00 2001
From: Flex Liu <fliu@suse.com>
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 <nicholasmhughes@gmail.com>
---
 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