saltstack/salt
1
0
Fork 0
mirror of https://github.com/saltstack/salt.git synced 2025-04-17 10:10:20 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

Include specific UID and GID checks in modules.file.check_perms

Browse source 
If the same username exists in more than one nsswitch database, it's
possible to end up in a situation where users can be associated with
more than one UID. When this happens - files that are owned by the user,
but have the "previous" UID, will not have the UID updated by the
`file.directory` state when `recurse` is set to `["user", "group"]`.
This is due to `file.stats` returning the correct owning username,
causing `file.check_perms`, which only compares owner permissions by
name, to believe no permissions changes are required. If
`file.check_perms` was to check permissions via ID as well, it would
return the ID changes to be made.

The PR updates the `file.check_perms` function to also check whether the
UID and GID values of the file match the function arguments for `user`
and `group` should they be IDs.

Previously `file.check_perms` attempted to retrieve usernames via
`file.uid_to_user`. An empty check was done on the return value which
would signify a username not being present on the system - a hard
failure as `os.chown` takes IDs, but no username means no ID. The PR
changes this so that username checks are left to the `file.chown`
function. The error string returned by `file.chown` is appended to
`ret["comments"]` so it can be returned to the user should either the
username or group name not be present.

The PR also attempts to reduce the number of calls to `get_user` and
`get_group` which result in repeat calls to stat. Instead use a single
"post" `file.stats` call and compare user/group/mode changes using that.

This can be reproduced with `libnss-cache` by defining a user with the
same name, but different UID in `/etc/passwd.cache` and
`/etc/group.cache`.
This commit is contained in:
Nick Rhodes 2022-09-20 11:13:08 +01:00 • committed by Gareth J. Greenaway
parent e012e2c38b
commit be05e799fe
3 changed files with 141 additions and 60 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

121
salt/modules/file.py
View file

@ -5101,6 +5101,7 @@ def check_perms(
``follow_symlinks`` option added
"""
name = os.path.expanduser(name)
mode = salt.utils.files.normalize_mode(mode)
if not ret:
ret = {"name": name, "changes": {}, "comment": [], "result": True}
@ -5109,121 +5110,127 @@ def check_perms(
orig_comment = ret["comment"]
ret["comment"] = []
# Check permissions
perms = {}
# Check current permissions
cur = stats(name, follow_symlinks=follow_symlinks)
perms["luser"] = cur["user"]
perms["lgroup"] = cur["group"]
perms["lmode"] = salt.utils.files.normalize_mode(cur["mode"])
# Record initial stat for return later. Check whether we're receiving IDs
# or names so luser == cuser comparison makes sense.
perms = {}
perms["luser"] = cur["uid"] if isinstance(user, int) else cur["user"]
perms["lgroup"] = cur["gid"] if isinstance(group, int) else cur["group"]
perms["lmode"] = cur["mode"]
is_dir = os.path.isdir(name)
is_link = os.path.islink(name)
# user/group changes if needed, then check if it worked
# Check and make user/group/mode changes, then verify they were successful
if user:
if isinstance(user, int):
user = uid_to_user(user)
if (
salt.utils.platform.is_windows()
and user_to_uid(user) != user_to_uid(perms["luser"])
) or (not salt.utils.platform.is_windows() and user != perms["luser"]):
salt.utils.platform.is_windows() and not user_to_uid(user) == cur["uid"]
) or (
not salt.utils.platform.is_windows()
and not user == cur["user"]
and not user == cur["uid"]
):
perms["cuser"] = user
if group:
if isinstance(group, int):
group = gid_to_group(group)
if (
salt.utils.platform.is_windows()
and group_to_gid(group) != group_to_gid(perms["lgroup"])
) or (not salt.utils.platform.is_windows() and group != perms["lgroup"]):
salt.utils.platform.is_windows() and not group_to_gid(group) == cur["gid"]
) or (
not salt.utils.platform.is_windows()
and not group == cur["group"]
and not group == cur["gid"]
):
perms["cgroup"] = group
if "cuser" in perms or "cgroup" in perms:
if not __opts__["test"]:
if os.path.islink(name) and not follow_symlinks:
if is_link and not follow_symlinks:
chown_func = lchown
else:
chown_func = chown
if user is None:
user = perms["luser"]
user = cur["user"]
if group is None:
group = perms["lgroup"]
group = cur["group"]
try:
chown_func(name, user, group)
# Python os.chown() does reset the suid and sgid,
# that's why setting the right mode again is needed here.
set_mode(name, mode)
err = chown_func(name, user, group)
if err:
ret["result"] = False
ret["comment"].append(err)
else:
# Python os.chown() resets the suid and sgid, hence we
# setting the previous mode again. Pending mode changes
# will be applied later.
set_mode(name, cur["mode"])
except OSError:
ret["result"] = False
# Mode changes if needed
if mode is not None:
# File is a symlink, ignore the mode setting
# if follow_symlinks is False
if is_link and not follow_symlinks:
pass
if __opts__["test"] is True:
ret["changes"]["mode"] = mode
else:
if not mode == cur["mode"]:
set_mode(name, mode)
# verify user/group/mode changes
post = stats(name, follow_symlinks=follow_symlinks)
if user:
if isinstance(user, int):
user = uid_to_user(user)
if (
salt.utils.platform.is_windows()
and user_to_uid(user)
!= user_to_uid(get_user(name, follow_symlinks=follow_symlinks))
and user != ""
salt.utils.platform.is_windows() and not user_to_uid(user) == post["uid"]
) or (
not salt.utils.platform.is_windows()
and user != get_user(name, follow_symlinks=follow_symlinks)
and user != ""
and not user == post["user"]
and not user == post["uid"]
):
if __opts__["test"] is True:
ret["changes"]["user"] = user
else:
ret["result"] = False
ret["comment"].append("Failed to change user to {}".format(user))
elif "cuser" in perms and user != "":
elif "cuser" in perms:
ret["changes"]["user"] = user
if group:
if isinstance(group, int):
group = gid_to_group(group)
if (
salt.utils.platform.is_windows()
and group_to_gid(group)
!= group_to_gid(get_group(name, follow_symlinks=follow_symlinks))
and user != ""
salt.utils.platform.is_windows() and not group_to_gid(group) == post["gid"]
) or (
not salt.utils.platform.is_windows()
and group != get_group(name, follow_symlinks=follow_symlinks)
and user != ""
and not group == post["group"]
and not group == post["gid"]
):
if __opts__["test"] is True:
ret["changes"]["group"] = group
else:
ret["result"] = False
ret["comment"].append("Failed to change group to {}".format(group))
elif "cgroup" in perms and user != "":
elif "cgroup" in perms:
ret["changes"]["group"] = group
# Mode changes if needed
if mode is not None:
# File is a symlink, ignore the mode setting
# if follow_symlinks is False
if os.path.islink(name) and not follow_symlinks:
if is_link and not follow_symlinks:
pass
if not mode == post["mode"]:
ret["result"] = False
ret["comment"].append("Failed to change mode to {}".format(mode))
else:
mode = salt.utils.files.normalize_mode(mode)
if mode != perms["lmode"]:
if __opts__["test"] is True:
ret["changes"]["mode"] = mode
else:
set_mode(name, mode)
if mode != salt.utils.files.normalize_mode(get_mode(name)):
ret["result"] = False
ret["comment"].append(
"Failed to change mode to {}".format(mode)
)
else:
ret["changes"]["mode"] = mode
ret["changes"]["mode"] = mode
# Modify attributes of file if needed
if attrs is not None and not is_dir:
# File is a symlink, ignore the mode setting
# if follow_symlinks is False
if os.path.islink(name) and not follow_symlinks:
if is_link and not follow_symlinks:
pass
else:
diff_attrs = _cmp_attrs(name, attrs)

12
salt/states/file.py
View file

@ -746,9 +746,17 @@ def _check_directory(
fchange = {}
path = os.path.join(root, fname)
stats = __salt__["file.stats"](path, None, follow_symlinks)
if user is not None and user != stats.get("user"):
if (
user is not None
and not user == stats.get("user")
and not user == stats.get("uid")
):
fchange["user"] = user
if group is not None and group != stats.get("group"):
if (
group is not None
and not group == stats.get("group")
and not user == stats.get("gid")
):
fchange["group"] = group
smode = salt.utils.files.normalize_mode(stats.get("mode"))
file_mode = salt.utils.files.normalize_mode(file_mode)

68
tests/pytests/unit/modules/file/test_file_check.py
View file

@ -7,13 +7,19 @@ import pytest
import salt.modules.file as filemod
import salt.utils.files
import salt.utils.platform
from tests.support.mock import Mock, patch
log = logging.getLogger(__name__)
@pytest.fixture
def configure_loader_modules():
return {filemod: {"__context__": {}}}
return {
filemod: {
"__context__": {},
"__opts__": {"test": False},
}
}
@pytest.fixture
@ -143,3 +149,63 @@ def test_check_managed_changes_follow_symlinks(a_link, tfile):
follow_symlinks=True,
)
assert ret == {}
@pytest.mark.skip_on_windows(reason="os.symlink is not available on Windows")
@patch("os.path.exists", Mock(return_value=True))
def test_check_perms_user_group_name_and_id():
filename = "/path/to/fnord"
tests = [
# user/group changes needed by name
{
"input": {"user": "cuser", "group": "cgroup"},
"expected": {"user": "cuser", "group": "cgroup"},
},
# no changes needed by name
{"input": {"user": "luser", "group": "lgroup"}, "expected": {}},
# user/group changes needed by id
{
"input": {"user": 1001, "group": 2001},
"expected": {"user": 1001, "group": 2001},
},
# no user/group changes needed by id
{"input": {"user": 3001, "group": 4001}, "expected": {}},
]
for test in tests:
# Consistent initial file stats
stat_out = {
"user": "luser",
"group": "lgroup",
"uid": 3001,
"gid": 4001,
"mode": "123",
}
patch_stats = patch(
"salt.modules.file.stats",
Mock(return_value=stat_out),
)
# "chown" the file to the permissions we want in test["input"]
# pylint: disable=W0640
def fake_chown(cmd, *args, **kwargs):
for k, v in test["input"].items():
stat_out.update({k: v})
patch_chown = patch(
"salt.modules.file.chown",
Mock(side_effect=fake_chown),
)
with patch_stats, patch_chown:
ret, pre_post = filemod.check_perms(
name=filename,
ret={},
user=test["input"]["user"],
group=test["input"]["group"],
mode="123",
follow_symlinks=False,
)
assert ret["changes"] == test["expected"]