diff --git a/changelog/58423.fixed b/changelog/58423.fixed new file mode 100644 index 000000000000..ea678fb69856 --- /dev/null +++ b/changelog/58423.fixed @@ -0,0 +1,2 @@ +linux_shadow: Fix cases where malformed shadow entries cause `user.present` +states to fail. diff --git a/salt/modules/linux_shadow.py b/salt/modules/linux_shadow.py index def2811bb79f..aa18789a3786 100644 --- a/salt/modules/linux_shadow.py +++ b/salt/modules/linux_shadow.py @@ -15,7 +15,6 @@ import salt.utils.data import salt.utils.files -import salt.utils.stringutils from salt.exceptions import CommandExecutionError try: @@ -390,11 +389,15 @@ def set_password(name, password, use_usermod=False, root=None): lines = [] user_found = False lstchg = str((datetime.datetime.today() - datetime.datetime(1970, 1, 1)).days) - with salt.utils.files.fopen(s_file, "rb") as fp_: + with salt.utils.files.fopen(s_file, "r") as fp_: for line in fp_: - line = salt.utils.stringutils.to_unicode(line) - comps = line.strip().split(":") + # Fix malformed entry by first ignoring extra fields, then + # adding missing fields. + comps = line.strip().split(":")[:9] if comps[0] == name: + num_missing = 9 - len(comps) + if num_missing: + comps.extend([""] * num_missing) user_found = True comps[1] = password comps[2] = lstchg @@ -410,7 +413,6 @@ def set_password(name, password, use_usermod=False, root=None): ) else: with salt.utils.files.fopen(s_file, "w+") as fp_: - lines = [salt.utils.stringutils.to_str(_l) for _l in lines] fp_.writelines(lines) uinfo = info(name, root=root) return uinfo["passwd"] == password @@ -531,7 +533,6 @@ def _getspnam(name, root=None): passwd = os.path.join(root, "etc/shadow") with salt.utils.files.fopen(passwd) as fp_: for line in fp_: - line = salt.utils.stringutils.to_unicode(line) comps = line.strip().split(":") if comps[0] == name: # Generate a getspnam compatible output @@ -549,7 +550,6 @@ def _getspall(root=None): passwd = os.path.join(root, "etc/shadow") with salt.utils.files.fopen(passwd) as fp_: for line in fp_: - line = salt.utils.stringutils.to_unicode(line) comps = line.strip().split(":") # Generate a getspall compatible output for i in range(2, 9): diff --git a/tests/unit/modules/test_linux_shadow.py b/tests/unit/modules/test_linux_shadow.py index 844b4c56e802..576069ba8e8b 100644 --- a/tests/unit/modules/test_linux_shadow.py +++ b/tests/unit/modules/test_linux_shadow.py @@ -1,13 +1,11 @@ """ :codeauthor: Erik Johnson """ - import textwrap import pytest import salt.utils.platform -import salt.utils.stringutils from tests.support.mixins import LoaderModuleMockMixin from tests.support.mock import DEFAULT, MagicMock, mock_open, patch from tests.support.unit import TestCase, skipIf @@ -64,15 +62,16 @@ def test_set_password(self): Test the corner case in which shadow.set_password is called for a user that has an entry in /etc/passwd but not /etc/shadow. """ + original_file = textwrap.dedent( + """\ + foo:orighash:17955:::::: + bar:somehash:17955:::::: + """ + ) + original_lines = original_file.splitlines(True) + data = { - "/etc/shadow": salt.utils.stringutils.to_bytes( - textwrap.dedent( - """\ - foo:orighash:17955:::::: - bar:somehash:17955:::::: - """ - ) - ), + "/etc/shadow": original_file, "*": Exception("Attempted to open something other than /etc/shadow"), } isfile_mock = MagicMock( @@ -114,7 +113,7 @@ def test_set_password(self): # Should only have the same two users in the file assert len(lines) == 2 # The first line should be unchanged - assert lines[0] == "foo:orighash:17955::::::\n" + assert lines[0] == original_lines[0] # The second line should have the new password hash assert lines[1].split(":")[:2] == [user, password] @@ -199,6 +198,113 @@ def test_info(self): expected_result, sorted(result.items(), key=lambda x: x[0]) ) + @pytest.mark.skip_if_not_root + def test_set_password_malformed_shadow_entry(self): + """ + Test that Salt will repair a malformed shadow entry (that is, one that + doesn't have the correct number of fields). + """ + original_file = textwrap.dedent( + """\ + valid:s00persekr1thash:17955:::::: + tooshort:orighash:17955::::: + toolong:orighash:17955::::::: + """ + ) + original_lines = original_file.splitlines(True) + + data = { + "/etc/shadow": original_file, + "*": Exception("Attempted to open something other than /etc/shadow"), + } + isfile_mock = MagicMock( + side_effect=lambda x: True if x == "/etc/shadow" else DEFAULT + ) + password = "newhash" + shadow_info_mock = MagicMock(return_value={"passwd": password}) + + # + # CASE 1: Fix an entry with too few fields + # + user = "tooshort" + user_exists_mock = MagicMock( + side_effect=lambda x, **y: 0 if x == ["id", user] else DEFAULT + ) + with patch( + "salt.utils.files.fopen", mock_open(read_data=data) + ) as shadow_mock, patch("os.path.isfile", isfile_mock), patch.object( + shadow, "info", shadow_info_mock + ), patch.dict( + shadow.__salt__, {"cmd.retcode": user_exists_mock} + ), patch.dict( + shadow.__grains__, {"os": "CentOS"} + ): + result = shadow.set_password(user, password, use_usermod=False) + + assert result + filehandles = shadow_mock.filehandles["/etc/shadow"] + # We should only have opened twice, once to read the contents and once + # to write. + assert len(filehandles) == 2 + # We're rewriting the entire file + assert filehandles[1].mode == "w+" + # We should be calling writelines instead of write, to rewrite the + # entire file. + assert len(filehandles[1].writelines_calls) == 1 + # Make sure we wrote the correct info + lines = filehandles[1].writelines_calls[0] + # Should only have the same three users in the file + assert len(lines) == 3 + # The first and third line should be unchanged + assert lines[0] == original_lines[0] + assert lines[2] == original_lines[2] + # The second line should have the new password hash, and it should have + # gotten "fixed" by adding another colon. + fixed = lines[1].split(":") + assert fixed[:2] == [user, password] + assert len(fixed) == 9 + + # + # CASE 2: Fix an entry with too many fields + # + user = "toolong" + user_exists_mock = MagicMock( + side_effect=lambda x, **y: 0 if x == ["id", user] else DEFAULT + ) + with patch( + "salt.utils.files.fopen", mock_open(read_data=data) + ) as shadow_mock, patch("os.path.isfile", isfile_mock), patch.object( + shadow, "info", shadow_info_mock + ), patch.dict( + shadow.__salt__, {"cmd.retcode": user_exists_mock} + ), patch.dict( + shadow.__grains__, {"os": "CentOS"} + ): + result = shadow.set_password(user, password, use_usermod=False) + + assert result + filehandles = shadow_mock.filehandles["/etc/shadow"] + # We should only have opened twice, once to read the contents and once + # to write. + assert len(filehandles) == 2 + # We're rewriting the entire file + assert filehandles[1].mode == "w+" + # We should be calling writelines instead of write, to rewrite the + # entire file. + assert len(filehandles[1].writelines_calls) == 1 + # Make sure we wrote the correct info + lines = filehandles[1].writelines_calls[0] + # Should only have the same three users in the file + assert len(lines) == 3 + # The first and second line should be unchanged + assert lines[0] == original_lines[0] + assert lines[1] == original_lines[1] + # The third line should have the new password hash, and it should have + # gotten "fixed" by reducing it to 9 fields instead of 10. + fixed = lines[2].split(":") + assert fixed[:2] == [user, password] + assert len(fixed) == 9 + @pytest.mark.skip_if_not_root def test_list_users(self): """