File fix-user.list_groups-omits-remote-groups.patch of Package venv-salt-minion
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
