Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Package layering and passwd/group #1843

Open
jlebon opened this issue May 22, 2019 · 1 comment
Open

Package layering and passwd/group #1843

jlebon opened this issue May 22, 2019 · 1 comment

Comments

@jlebon
Copy link
Member

jlebon commented May 22, 2019

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants