Fix malformed shadow entries

changelog/58423.fixed

@@ -0,0 +1,2 @@
+linux_shadow: Fix cases where malformed shadow entries cause `user.present`
+states to fail.

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):

tests/unit/modules/test_linux_shadow.py

@@ -1,13 +1,11 @@
 """
     :codeauthor: Erik Johnson <[email protected]>
 """
-
 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):
         """