coreos / rpm-ostree

⚛📦 Hybrid image/package system with atomic upgrades and package layering
https://coreos.github.io/rpm-ostree
Other
874 stars 196 forks source link

Package layering and passwd/group #1843

Open jlebon opened 5 years ago

jlebon commented 5 years ago

TL;DR: More issues from nss-altfiles which should be fixed by switching to systemd-sysusers (#1679). Just wanted to spell them out here for visibility and something to link to, as well as to make sure they do get correctly fixed by systemd-sysusers.


So, there are actually two issues today with layering packages that create uids/gids:

  1. Because rpm-ostree client-side layering starts "from scratch" each time, ISTM it's possible for system users/groups to change ids on different runs. This is of course an issue if e.g. /var already owns files from the previous uid.
  2. During assembly, we do:
/* We actually want RPM to inject to /usr/lib/passwd - we
 * accomplish this by temporarily renaming /usr/lib/passwd -> /usr/etc/passwd
 * (Which appears as /etc/passwd via our compatibility symlink in the bubblewrap
 *  script runner). We also copy the merge deployment's /etc/passwd to
 *  /usr/lib/passwd, so that %pre scripts are aware of newly added system users
 *  not in the tree's /usr/lib/passwd (through nss-altfiles in the container).
 */
gboolean
rpmostree_passwd_prepare_rpm_layering (int                rootfs_dfd,
                                       const char        *merge_passwd_dir,
                                       gboolean          *out_have_passwd,
                                       GCancellable      *cancellable,
                                       GError           **error)
{

We don't really have a choice; we need to make sure that groups/users we create don't collide with pre-existing local system users/groups. One issue though is that if there's a system group in /etc/group (which is a hack we recommend to work around usermod -a -G not working), then the %pre getent will see it and won't actually create an entry in /usr/lib/group. This has an odd hysteresis effect: the first run of layering a package like libvirt, there will be a libvirt entry in /usr/lib/group, but after the user adds the libvirt group to /etc/group as well, subsequent layered commits won't actually have libvirt in /usr/lib/group.

This issue is exacerbated if the package has files owned by this group. If that's the case, then layering will simply fail since the core only reads in /usr/lib/group for system groups. This is the case for wireshark-cli: https://pagure.io/teamsilverblue/issue/74.

The following patch works:

diff --git a/src/libpriv/rpmostree-core.c b/src/libpriv/rpmostree-core.c
index 841b30f4..b92b52a5 100644
--- a/src/libpriv/rpmostree-core.c
+++ b/src/libpriv/rpmostree-core.c
@@ -4106,6 +4106,24 @@ rpmostree_context_assemble (RpmOstreeContext      *self,
             }
         }

+      /* Also add usr/lib/group, which is the merge deployment's /etc/group, so that
+       * sysgroups added there can be used by rpmfi overrides. */
+      if (faccessat (tmprootfs_dfd, "usr/lib/group", F_OK, 0) == 0)
+        {
+          g_autofree char *contents =
+            glnx_file_get_contents_utf8_at (tmprootfs_dfd, "usr/lib/group",
+                                            NULL, cancellable, error);
+          if (!contents)
+            return FALSE;
+
+          groupents_ptr = rpmostree_passwd_data2groupents (contents);
+          for (guint i = 0; i < groupents_ptr->len; i++)
+            {
+              struct conv_group_ent *ent = groupents_ptr->pdata[i];
+              g_hash_table_insert (groupents, ent->name, ent);
+            }
+        }
+
       {
       g_auto(RpmOstreeProgress) task = { 0, };
       rpmostree_output_task_begin (&task, "Running post scripts");

though it's really another hack on top of the pile of hacks. I also haven't fully thought through how it'd affect migration to sysusers.

sgallagher commented 1 year ago

Just to add another datapoint here (and reignite this discussion):

I'm using CoreOS with the tang server package overlaid atop it. This package has the following scriptlet:

%pre
%sysusers_create_compat %{SOURCE1}
exit 0

%post
%systemd_post %{name}d.socket

# Let's make sure any existing keys are readable only
# by the owner/group.
if [ -d /var/db/tang ]; then
    for k in /var/db/tang/*.jwk; do
        test -e "${k}" || continue
        chmod 0440 -- "${k}"
    done
    for k in /var/db/tang/.*.jwk; do
        test -e "${k}" || continue
        chmod 0440 -- "${k}"
    done
    chown tang:tang -R /var/db/tang
fi

If I'm reading this correctly, this should be ensuring that if the syusers-created UID/GID changes, it chown()s them to the updated user, but that isn't happening. The resulting effect is that upgrades have a tendency to change the UID/GID and make the keys unreadable, thereby preventing my systems relying on network-bound disk encryption from decrypting their drives.