rpmostree-core: Reload user/groups after each %post script

[travier]

As part of the migration to direct sysusers support in RPMs, packages are no longer creating their users/groups in %pre scripts as they rely on RPM to do the work.

In rpm-ostree unified core composes, the users/groups are not created this way as we don't use/call out to those new RPM scriptlets. (is that correct? to be confirmed).

Thus the "new" users (i.e. those not part of the default passwd & group files set in the manifests) are created when the systemd %post scriptlet runs the sysusers command.

As a workaround, we now reload the list of users/groups after each %post to make sure it is up-to-date.

See: https://fedoraproject.org/wiki/Changes/RPMSuportForSystemdSysusers
See: rpm-software-management/rpm@009d139

See: fedora-silverblue/issue-tracker#636
See: https://gitlab.com/fedora/ostree/sig/-/issues/70
See: #5333

[keszybz]

Doesn't look too terrible ;)

[travier]

PR description is my understanding, may not be fully accurate, suggestions welcomed (and then I'll update the commit message).

[AdamWill]

The code is written as it is specifically in order not to do this sort of thing. There's a long comment to that effect:

      /* We're technically deviating from RPM here by running all the %pre's
       * beforehand, rather than each package's %pre & %post in order. Though I
       * highly doubt this should cause any issues. The advantage of doing it
       * this way is that we only need to read the passwd/group files once
       * before applying the overrides, rather than after each %pre.
       */

so it seems like we might want to try and honor that goal somehow. Maybe special-case systemd and do something like this?

1. pre loop
2. read passwd
3. systemd post
4. read passwd again
5. post loop

if we don't want to do that, we should probably at least update the comment...

[AdamWill]

also, doing it this way doesn't feel 100% safe, because it seems like it would rely on systemd coming very early in the %post loop - because it must be done before any other package containing a file owned by a user or group that would be created by systemd's %post - but we don't appear to do anything to guarantee that that's the case?

[travier]

That should work but would make this very specific to the systemd package name. I agree that this is a bit of a brute force logic but maybe this is safer at the cost of re-reading the files a bit often?

[AdamWill]

well, then, how about:

1. pre loop
2. directly cause a run of systemd-sysusers in whatever context it needs to happen
3. read passwd
4. post loop

that seems most similar to what rpm is now doing, i guess?

[travier]

also, doing it this way doesn't feel 100% safe, because it seems like it would rely on systemd coming very early in the %post loop - because it must be done before any other package containing a file owned by a user or group that would be created by systemd's %post - but we don't appear to do anything to guarantee that that's the case?

Agree that this is not a "fix", but more of a workaround. Ideally, we would add the RPM sysusers scriptlet support to rpm-ostree as a specific phase here before running the %post.

[travier]

For this to be "correct" (i.e. matching how RPM does it) we would have to re-implement rpm-software-management/rpm@009d139 as far as I understand.

[travier]

well, then, how about:

1. pre loop
2. directly cause a run of systemd-sysusers in whatever context it needs to happen
3. read passwd
4. post loop

that seems most similar to what rpm is now doing, i guess?

That could be another option.

[AdamWill]

i'm trying to figure out how to subvert the 'run a package script' bits to just run a script we make up on the fly...

[travier]

rpm-ostree runs post-process scripts via https://github.com/coreos/rpm-ostree/blob/main/rust/src/composepost.rs#L515, so we should be able to directly add a call to sysusers using a similar logic.

[AdamWill]

well, Ii'm trying to hook into the various levels of script running function in src/libpriv/rpmostree-scripts.cxx , which does more or less the same. But it's kinda tedious to figure out if there's a neat way to slip in, it's all tied into RPM business logic. but I don't want to repeat all the bwrap stuff...

for now I guess I'll just test this PR as-is, if it at least fixes ostree builds for now that's better than what we have, we can slap it in as a downstream patch till Colin or someone can decide what the best long-term fix is...

[AdamWill]

it does look like this works around the issue for now, so I'll go ahead and backport it downstream officially. I'll try and figure out if there's a significant impact on run time, too, once the test completes.

[travier]

Rebased and updated the commit description

[travier]

From a discussion with Jonathan, this will not work for package layering as the systemd %post won't be called there, so this is not a complete workaround.

[jlebon]

Whipped this up while chatting with @travier:

diff --git a/src/libpriv/rpmostree-core.cxx b/src/libpriv/rpmostree-core.cxx
index 70905996..208696fe 100644
--- a/src/libpriv/rpmostree-core.cxx
+++ b/src/libpriv/rpmostree-core.cxx
@@ -4422,6 +4422,26 @@ rpmostree_context_assemble (RpmOstreeContext *self, GCancellable *cancellable, G
 
   progress->end ("");
 
+  /* if the %__systemd_sysusers macro is defined, that _very likely_
+   * means that we're using an rpm with built-in sysusers support:
+   * https://fedoraproject.org/wiki/Changes/RPMSuportForSystemdSysusers Ideally,
+   * librpm would expose that part as a public API but it's currently just
+   * done during the transaction. So here we just do it ourselves (though a bit
+   * differently; librpm checks for the user/group provides before proceeding to call
+   * sysusers _just_ for that package. Here we do it for all of them... See also:
+   * https://github.com/coreos/rpm-ostree/issues/5333 */
+  g_autofree char *sysusers_cmd = rpmExpand ("%__systemd_sysusers", NULL);
+  if (sysusers_cmd && *sysusers_cmd)
+    {
+      CXX_TRY_VAR (bwrap,
+                   rpmostreecxx::bubblewrap_new_with_mutability (
+                       tmprootfs_dfd, rpmostreecxx::BubblewrapMutability::MutateFreely),
+                   error);
+      bwrap->append_child_arg (sysusers_cmd);
+      if (!CXX (bwrap->run (*cancellable), error))
+        return glnx_prefix_error (error, "Running systemd-sysusers (%s)", sysusers_cmd);
+    }
+
   /* Some packages expect to be able to make temporary files here
    * for obvious reasons, but we otherwise make `/var` read-only.
    */

Not tested...

[cgwalters]

The approach of running systemd-sysusers "wholesale" does seem to match with the current design.

rpmostreecxx::BubblewrapMutability::Immutable

Hmm don't we need mutability? Though I forget if that means just /usr is immutable.

[jlebon]

The approach of running systemd-sysusers "wholesale" does seem to match with the current design.

rpmostreecxx::BubblewrapMutability::Immutable

Hmm don't we need mutability? Though I forget if that means just /usr is immutable.

Ahh I think you're right. I thought the reverse and that it only implied /usr. There isn't a mode currently for "/usr only", but meh.. fine with MutateFreely. Updated patch! I think @travier is going to take it for a spin and update this PR if it works.