PSARC/2024/105 pkg(7) user action uid selection must not use reserved…

… range

37049008 pkg should not allocate uid/gid in the system reseved range
35721851 pkg fails to to preserve gid of existing group when manifest doesn't define a gid
37049046 test_minugid monumentally broken - will always pass
37049103 uid/gid allocation should not be a class default_value

src/man/pkg.7

@@ -1,8 +1,8 @@
 '\" te
-.\" Copyright (c) 2009, 2020, Oracle and/or its affiliates. All rights reserved.
+.\" Copyright (c) 2009, 2024, Oracle and/or its affiliates. All rights reserved.
 .\" Copyright (c) 2012, OmniTI Computer Consulting, Inc. All rights reserved.
-.\" Copyright 2022 OmniOS Community Edition (OmniOSce) Association.
-.Dd November 16, 2022
+.\" Copyright 2024 OmniOS Community Edition (OmniOSce) Association.
+.Dd November 14, 2024
 .Dt PKG 7
 .Os
 .Sh NAME
@@ -1178,7 +1178,7 @@ attribute is installed on the system, so that the package can be used to
 satisfy dependencies.
 .Pp
 The following attributes, named in accordance with the parameters on
-.Xr pkginfo 7 ,
+.Xr pkginfo 5 ,
 are recognised:
 .Bl -tag -width 6n
 .It Sy category
@@ -1308,7 +1308,7 @@ A short, one-line description of the package.
 The
 .Sy group
 action defines a UNIX group as defined in
-.Xr group 7 .
+.Xr group 5 .
 No support is present for group passwords.
 Groups defined with this action initially have no user list.
 Users can be added with the
@@ -1320,7 +1320,8 @@ The following attributes are recognised:
 The value for the name of the group.
 .It Sy gid
 The group's unique numerical id.
-The default value is the first free group under 100.
+If not specified the first free value in the dynamic allocation range 100-499
+in the target image is used.
 .El
 .\"
 .Ss "User Actions"
@@ -1353,7 +1354,7 @@ The encrypted password of the user.
 Default value is
 .Sy \&*LK* .
 See
-.Xr shadow 7 .
+.Xr shadow 5 .
 The special value
 .Sy UP
 can be used to indicate that the
@@ -1370,7 +1371,8 @@ report an unexpected change and
 will not change the value back to that of the manifest.
 .It Sy uid
 The unique uid of the user.
-Default value is first free value under 100.
+If not specified the first free value in the dynamic allocation range 100-499
+in the target image is used.
 .It Sy group
 The name of the user's primary group.
 Must be found in
@@ -1395,7 +1397,7 @@ Default value is empty.
 .It Sy group-list
 Secondary groups to which the user belongs.
 See
-.Xr group 7 .
+.Xr group 5 .
 .It Sy ftpuser
 Can be set to
 .Sy true
@@ -1405,48 +1407,48 @@ The default value of
 .Sy true
 indicates that the user is permitted to login via FTP.
 See
-.Xr ftpusers 7 .
+.Xr ftpusers 5 .
 .It Sy lastchg
 The number of days between January 1, 1970, and the date that the password was
 last modified.
 Default value is empty.
 See
-.Xr shadow 7 .
+.Xr shadow 5 .
 .It Sy min
 The minimum number of days required between password changes.
 This field must be set to 0 or above to enable password aging.
 Default value is empty.
 See
-.Xr shadow 7 .
+.Xr shadow 5 .
 .It Sy max
 The maximum number of days the password is valid.
 Default value is empty.
 See
-.Xr shadow 7 .
+.Xr shadow 5 .
 .It Sy warn
 The number of days before password expires that the user is warned.
 See
-.Xr shadow 7 .
+.Xr shadow 5 .
 .It Sy inactive
 The number of days of inactivity allowed for that user.
 This is counted on a per-machine basis.
 The information about the last login is taken from the machine's
 .Sy lastlog
 file.
 See
-.Xr shadow 7 .
+.Xr shadow 5 .
 .It Sy expire
 An absolute date expressed as the number of days since the UNIX Epoch (January
 1, 1970).
 When this number is reached, the login can no longer be used.
 For example, an expire value of 13514 specifies a login expiration of January
 1, 2007.
 See
-.Xr shadow 7 .
+.Xr shadow 5 .
 .It Sy flag
 Set to empty.
 See
-.Xr shadow 7 .
+.Xr shadow 5 .
 .El
 .\"
 .Sh ACTUATORS
@@ -1671,12 +1673,6 @@ dependencies.
 Because of restrictions that the global zone imposes on non-global zones, the
 non-global zones must have access to the packages of the global zone and must
 have a similar publisher configuration.
-Both of these objectives are achieved by using a
-.Sy system repository
-.Po see the
-.Xr pkg.sysrepo 8
-man page
-.Pc .
 The system repository provides access to the publishers configured in the
 global zone and information about how those publishers are configured.
 To prevent non-global zones from choosing different packages during
@@ -2010,12 +2006,11 @@ directory hierarchy are Private, and are subject to change.
 .Xr pkg 1 ,
 .Xr pkgsend 1 ,
 .Xr svcs 1 ,
-.Xr ftpusers 7 ,
-.Xr group 7 ,
-.Xr pkginfo 7 ,
-.Xr shadow 7 ,
+.Xr ftpusers 5 ,
+.Xr group 5 ,
+.Xr pkginfo 5 ,
+.Xr shadow 5 ,
 .Xr add_drv 8 ,
 .Xr devlinks 8 ,
 .Xr pkg.depotd 8 ,
-.Xr pkg.sysrepo 8 ,
 .Xr svcadm 8

src/modules/actions/group.py

@@ -87,8 +87,10 @@ def install(self, pkgplan, orig, retry=False):
         #        (XXX this doesn't chown any files on-disk)
         # else, nothing to do
         if cur_attrs:
+            # if the action doesn't specify the gid keep the one
+            # already allocated.
             if "gid" not in self.attrs:
-                self.attrs["gid"] = cur_attrs["gid"]
+                template["gid"] = cur_attrs["gid"]
             elif self.attrs["gid"] != cur_attrs["gid"]:
                 cur_gid = cur_attrs["gid"]
                 template = cur_attrs
@@ -131,6 +133,9 @@ def install(self, pkgplan, orig, retry=False):
             if cur_attrs["user-list"]:
                 template["user-list"] = cur_attrs["user-list"]
 
+        if "gid" not in template:
+            template["gid"] = gr.getnextgid()
+
         gr.setvalue(template)
         try:
             gr.writefile()

src/modules/actions/user.py

@@ -181,6 +181,12 @@ def install(self, pkgplan, orig, retry=False):
             self.attrs["group-list"] = self.attrlist("group-list")
             final_attrs = self.merge(orig_attrs, cur_attrs)
 
+            # When the user already exists uid will be in cur_attrs,
+            # if this is a new user and a uid is not specified in self.attrs
+            # we need to allocate one.
+            if "uid" not in final_attrs:
+                final_attrs["uid"] = pw.getnextuid()
+
             pw.setvalue(final_attrs)
 
             if "group-list" in final_attrs:

src/modules/cfgfiles.py

@@ -76,6 +76,33 @@ def __str__(self):
             self.filename, self.keys, self.column_names, self.index
         )
 
+    # Historically this class allocated from the 0-99 range, but
+    # UID/GID 0-99 are reserved for the OS vendor and Solaris always
+    # registers the id/name mappings.
+    #
+    # Following other platforms behaviour:
+    #   https://en.wikipedia.org/wiki/User_identifier
+    #
+    # 100-499 will be used for actions where no uid/gid provided.
+    MIN_DYNAMIC_ID = 100
+    MAX_DYNAMIC_ID = 499
+
+    def getnextid(self, values, idtype):
+        """returns a free id (uid or gid) from the dynamic space 100-499"""
+        ids = set()
+        for t in values:
+            if t[1]:
+                nid = int(t[1][idtype])
+                if nid >= self.MIN_DYNAMIC_ID and nid <= self.MAX_DYNAMIC_ID:
+                    ids.add(nid)
+        for i in range(self.MIN_DYNAMIC_ID, self.MAX_DYNAMIC_ID + 1):
+            if i not in ids:
+                return i
+        raise RuntimeError(
+            f"No free {idtype} in dynamic allocation range: "
+            + f"{self.MIN_DYNAMIC_ID}-{self.MAX_DYNAMIC_ID}."
+        )
+
     def getcolumnnames(self):
         return self.column_names
 
@@ -260,7 +287,6 @@ def __init__(self, path_prefix, lock=False):
         if lock:
             self.lock()
         self.readfile()
-        self.password_file.default_values["uid"] = self.getnextuid()
 
     def __str__(self):
         return "PasswordFile: [{0} {1}]".format(
@@ -293,15 +319,7 @@ def removevalue(self, template):
         self.shadow_file.removevalue(template)
 
     def getnextuid(self):
-        """returns next free system (<=99) uid"""
-        uids = []
-        for t in self.password_file.index.values():
-            if t[1]:
-                uids.append(t[1]["uid"])
-        for i in range(100):
-            if str(i) not in uids:
-                return i
-        raise RuntimeError("No free system uids")
+        return self.getnextid(self.password_file.index.values(), "uid")
 
     def getcolumnnames(self):
         names = self.password_file.column_names.copy()
@@ -351,18 +369,9 @@ def __init__(self, image):
         )
 
         self.readfile()
-        self.default_values["gid"] = self.getnextgid()
 
     def getnextgid(self):
-        """returns next free system (<=99) gid"""
-        gids = []
-        for t in self.index.values():
-            if t[1]:
-                gids.append(t[1]["gid"])
-        for i in range(100):
-            if str(i) not in gids:
-                return i
-        raise RuntimeError("No free system gids")
+        return self.getnextid(self.index.values(), "gid")
 
     def adduser(self, groupname, username):
         """ "add named user to group; does not check if user exists"""

src/tests/cli/t_pkg_install.py

@@ -3816,10 +3816,10 @@ def test_upgrade3(self):
         self._assertEditables()
 
         # make sure Kermie is still installed and still has our local
-        # changes
+        # changes. The uid should be the first in the dynamic range 100-499.
         self.file_contains(
             "etc/passwd",
-            "Kermit:x:5:4:Kermit loves Miss Piggy:/export/home/Kermit:",
+            "Kermit:x:100:4:Kermit loves Miss Piggy:/export/home/Kermit:",
         )
 
         # also make sure that /etc/silly hasn't been removed and added
@@ -6184,13 +6184,21 @@ def test_preexisting_group_install(self):
         # properly install group w/o a gid
         self.pkg("install simplegroup@0")
         self.pkg("verify simplegroup")
+        # check the uid did not change
+        with open(gpath) as f:
+            gdata = f.readlines()
+        self.assertTrue(
+            len([a for a in gdata if a.find("muppets::1010") == 0]) == 1
+        )
         # install w/ different gid
         self.pkg("install simplegroup@1")
         self.pkg("verify simplegroup")
-        # check # lines beginning w/ 'muppets' in group file
+        # check lines beginning with 'muppets' in group file
         with open(gpath) as f:
             gdata = f.readlines()
-        self.assertTrue(len([a for a in gdata if a.find("muppets") == 0]) == 1)
+        self.assertTrue(
+            len([a for a in gdata if a.find("muppets::70") == 0]) == 1
+        )
 
         # make sure we can add new version of same package
         self.pkg("update simplegroup")
@@ -6502,15 +6510,21 @@ def minugid_helper(self, install_cmd):
             self.pkg("install ugidtest")
         else:
             self.pkg("exact-install basics ugidtest")
-        passwd_file = open(os.path.join(self.get_img_path(), "/etc/passwd"))
+        passwd_file = open(os.path.join(self.get_img_path(), "etc/passwd"))
+        userfound = False
         for line in passwd_file:
             if line.startswith("dummy"):
-                self.assertTrue(line.startswith("dummy:x:5:"))
+                self.assertTrue(line.startswith("dummy:x:100:"))
+                userfound = True
         passwd_file.close()
-        group_file = open(os.path.join(self.get_img_path(), "/etc/group"))
+        self.assertTrue(userfound)
+        group_file = open(os.path.join(self.get_img_path(), "etc/group"))
+        groupfound = False
         for line in group_file:
             if line.startswith("dummy"):
-                self.assertTrue(line.startswith("dummy::5:"))
+                self.assertTrue(line.startswith("dummy::100:"))
+                groupfound = True
+        self.assertTrue(groupfound)
         group_file.close()
 
     def test_upgrade_with_user(self):