WIP: add sysusers to compose #1376

peterbaouoft · 2018-05-29T17:57:35Z

Hi, this PR includes the early version of implementation for #49. There are still many parts can be added/improved, but I would like to put this PR out to have some feedback/discussion to make sure I am on the right track :p.

Things did so far in this PR, also see discussion

Convert passwd entries into sysuser struct
Convert group entries into sysuser struct
Collect sysuser struct information and put them into hashtable
Added initial logic for conversion into compose and rpmostree_check_passwd_groups

Things still left undone:

Test the integration and usability of sysusers with compose
Refactor duplicated code, corner cases, and integration test
Add logic of removing /usr/lib/passwd and nss-altfiles?, when sysusers option is specified.

PS: sorry for this PR to be taking so long... I had really bad time management for past few months, and I was kinda hesitant to have a discussion back then :(. But hopefully, I will do better at those in the future =).

peterbaouoft · 2018-05-29T18:00:20Z

src/libpriv/rpmostree-passwd-util.c

@@ -250,7 +412,7 @@ rpmostree_check_passwd_groups (gboolean passwd,
 return TRUE; /* Note early return */
 else if (g_str_equal (chk_type, "previous"))
 ; /* Handled below */
- else if (g_str_equal (chk_type, "file"))
+ else if (g_str_equal (chk_type, "file") || g_str_equal (chk_type, "sysusers"))


For sysusers option, I think it might be good if we also check passwd/group as b4, which is why I added those lines. But let me know what you think about this. =)

cgwalters · 2018-05-30T19:06:58Z

I only scanned this at a very high level. Having unit tests is great. Have you played around much with whether or not the results will work? I am a bit concerned that we'll run into some blockers...I heard that an attempt to do this for all RPMs tripped up on e.g.:

/usr/sbin/useradd -M -N -g postgres -o -r -d /var/lib/pgsql -s /bin/bash \
	-c "PostgreSQL Server" -u 26 postgres >/dev/null 2>&1 || :

which actually wants to allocate a home directory and interactive shell. But eh...maybe we punt and translate those to systemd unit files which just run that on boot?

I bet this would be fairly easy to play around with without changing rpm-ostree if you did a scripted/manual conversion of /usr/lib/passwd,group to sysusers.d snippets, then just removed nss-altfiles from /etc/nsswitch.conf and rebooted and checked if things still worked.

cgwalters · 2018-05-30T19:08:42Z

src/libpriv/rpmostree-passwd-util.c

@@ -213,6 +228,152 @@ compare_group_ents (gconstpointer a, gconstpointer b)
 return strcmp ((*sa)->name, (*sb)->name);
 }

+gboolean
+rpmostree_passwd_ents2sysusers (gboolean is_passwd,


The wrapper function here feels unnecessary, let's just inline it into the one caller?

cgwalters

And in this PR we aren't writing anything, just adding some utility functions for converting?

cgwalters · 2018-05-30T19:09:22Z

src/libpriv/rpmostree-passwd-util.c

+ else
+ sysent->id = g_strdup_printf ("%u", convent->uid);
+
+ sysent->type = g_strdup ("u");


Minor, we can make this member a const char* and avoid the strdup/free.

cgwalters · 2018-05-30T19:11:10Z

src/libpriv/rpmostree-passwd-util.c

+ {
+ char *stored_gid = colon + 1;
+ g_print ("current_gid is %s\n", current_gid);
+ g_print ("stored_gid is %s\n", stored_gid);


Let's remove the debug bits if you don't need them anymore.

cgwalters · 2018-05-30T19:12:01Z

src/libpriv/rpmostree-passwd-util.c

+ g_print ("stored_gid is %s\n", stored_gid);
+ /* We skip the insertion if we found gid matches */
+ if (g_str_equal (stored_gid, current_gid))
+ {


Indentation looks off.

cgwalters · 2018-05-30T19:12:43Z

src/libpriv/rpmostree-passwd-util.c

+ else
+ {
+ *error = g_error_new (G_IO_ERROR, G_IO_ERROR_FAILED, "mismatch between /lib/passwd and /lib/group");
+ g_print("Catastrophic failure\n");


return glnx_throw (error, "mismatch between passwd/group: %s %s", stored_gid, current_gid); or so.

cgwalters · 2018-05-30T19:13:03Z

src/libpriv/rpmostree-passwd-util.c

+ sysent->name = g_strdup (convent->name);
+ /* Unset the gecos & dir for g entries */
+ sysent->gecos = g_strdup ("-");
+ sysent->dir = g_strdup ("-");


Maybe use NULL to mean "default" here to avoid a strdup of a constant.

cgwalters · 2018-05-30T19:14:03Z

src/libpriv/rpmostree-passwd-util.c

+ sysuser_ent->name, sysuser_ent->id,
+ sysuser_ent->gecos, sysuser_ent->dir,
+ NULL);
+ g_string_append_printf(sysuser_content, "%s\n", line_content);


BTW our code style has space between identifier and paren, so g_string_append_printf (sysuser_content and elsewhere.

peterbaouoft · 2018-05-30T19:30:21Z

And in this PR we aren't writing anything, just adding some utility functions for converting?

Yup, I wanted to have this WIP out, to have some discussions first and to make sure I am still on the right track =). I felt I did a bad job communicating/discussing ideas earlier, so I wanted to do more of it(discussion) :p.

The next step I am planning to do will be adding this conversion to compose and test out if sysusers will work (and solve/test the blocker you mentioned above).

peterbaouoft · 2018-05-30T19:31:28Z

Have you played around much with whether or not the results will work?

Not yet... I will try the postgres cmd you suggested earlier and report back the status =).

I bet this would be fairly easy to play around with without changing rpm-ostree if you did a scripted/manual conversion of /usr/lib/passwd,group to sysusers.d snippets, then just removed nss-altfiles from /etc/nsswitch.conf and rebooted and checked if things still worked

Sure, lemme try those on AH, and see if things still work ok or not :).

peterbaouoft · 2018-06-01T15:28:54Z

STATUS: the testing blockers might need a bit more time. I was trying to integrate the current conversion code into rpm-ostree and tested conversion of /etc/passwd to sysusers.d files, but it seems like there is a case on Atomic Host with the following:

error: mismatch between passwd/group: 100 20
[root@localhost ~]# cat /usr/lib/group | grep 20
games:x:20:
[root@localhost ~]# cat /usr/lib/passwd | grep 100
games:x:12:100:games:/usr/games:/sbin/nologin
[root@localhost ~]# cat /usr/lib/group | grep 100 
users:x:100:
[root@localhost ~]# groups games
games : users
[root@localhost ~]# id games 
uid=12(games) gid=100(users) groups=100(users)

It is possible that... even though games name exist in both passwd and group files, system user games is still not in group games... leading to conversion problem in the current code =(.

So, I will try manual conversion for now, and hopefully will have results soon. Thanks, and sorry for the long response time.

peterbaouoft · 2018-06-01T19:33:21Z

STATUS UPDATE Second: I tried out the systemd-sysusers with the current implementation of conversion, there were few problems for the execution:
1: Trailing garbage for sysusers content generated

The content for sysusers generation is like the following, it is weird that I see those trailing garbage...

2: systemd-sysusers status has condition failed label

systemctl status systemd-sysusers
 ● systemd-sysusers.service - Create System Users
   Loaded: loaded (/usr/lib/systemd/system/systemd-sysusers.service; static; vendor preset: disabled)
   Active: inactive (dead)
Condition: start condition failed at Thu 2018-05-10 18:13:56 EDT; 3 weeks 0 days ago
           └─ ConditionNeedsUpdate=/etc was not met
     Docs: man:sysusers.d(5)
           man:systemd-sysusers.service(8)

3: With the current approach of conversion, sysusers genreation is failing. I researched a bit, and it turns out for id, it describes the following:

The syntax "uid:gid" is also supported to allow creating user and group pairs with different numeric UID and GID values. The group with the indicated GID must get created explicitly before or it must already exist.

So, the original approach for uid:gid does not work :(. I should of read more carefully when implementing it =(.

Will address/try out implementation to try to resolve above bugs shortly. Will also try out the manual translation of postgres entries into sysusers... (Sorry, I originally thought I need to convert the entire /usr/lib/passwd manually, but I realized it should be enough to just copy the sysuser content into /etc/passwd and only generate postgres sysusers ....)

peterbaouoft · 2018-06-01T21:06:47Z

I am a bit concerned that we'll run into some blockers...I heard that an attempt to do this for all RPMs tripped up on e.g.:

So, I have tested the postgres blocker, and it seems like when there are 6 fields, trailing garbage will occur and the 6th field will be ignored..

E.g:

[root@localhost ~]# cat /etc/sysusers.d/ah.conf 
u postgres 26 "Postgresql Server" /var/lib/pgsql /bin/bash
[root@localhost ~]# systemd-sysusers
[/etc/sysusers.d/ah.conf:1] Trailing garbage.
[root@localhost ~]# cat /etc/sysusers.d/ah.conf 
u postgres 26 "Postgresql Server" /var/lib/pgsql
[root@localhost ~]# systemd-sysusers
Creating group postgres with gid 26.
Creating user postgres (Postgresql Server) with uid 26 and gid 26.

But it is weird tho.... on the documentation side, it said shell can be included. And for the code base, https://github.com/systemd/systemd/blob/master/src/sysusers/sysusers.c#L1390, it also seems shell get recognized..

EDIT: Was running systemd-sysusers on systemd-234-9.fc27.x86_64

peterbaouoft · 2018-06-01T21:08:22Z

UPDATE: Actually, I looked into systemd commit histories, and it looks like login shell is recently introduced to systemd(Feb 5 ish). I will double check on a newer systemd version and see if the result still works. =)

peterbaouoft · 2018-06-04T15:03:57Z

Hi, @cgwalters, I tested the blocker on f28 with systemd.v238 and here is the output:

[root@localhost ~]# mount -o remount,rw /usr ( need to do that because systemd-sysusers depends an update on /usr)
[root@localhost ~]# touch /usr/sysusers.tmp 
[root@localhost ~]# cat /etc/sysusers.d/new_ah.conf 
u postgres 26 "Postgresql Server" /var/lib/pgsql /bin/bash
[root@localhost ~]# rpm-ostree status 
State: idle; auto updates disabled
Deployments:
* ostree://fedora-atomic:fedora/28/x86_64/atomic-host
                   Version: 28.20180527.0 (2018-05-27 19:05:29)
                BaseCommit: 291ea90da29bc5abe757b5a50813b3de1396b08412939a89b3b671aba9856093
              GPGSignature: Valid signature by 128CF232A9371991C8A65695E08E7E629DB62FB1
       RemovedBasePackages: nss-altfiles-2.18.1-10.fc27.x86_64

  ostree://fedora-atomic:fedora/28/x86_64/atomic-host
                   Version: 28.20180527.0 (2018-05-27 19:05:29)
                    Commit: 291ea90da29bc5abe757b5a50813b3de1396b08412939a89b3b671aba9856093
              GPGSignature: Valid signature by 128CF232A9371991C8A65695E08E7E629DB62FB1
[root@localhost ~]# id postgres
id: 'postgres': no such user
[root@localhost ~]# cat /etc/group | grep postgres

After reboot, looks like postgres is generated correctly =).

[root@localhost ~]# systemctl reboot 
[root@localhost ~]# Connection to 192.168.121.122 closed by remote host.
Connection to 192.168.121.122 closed.
[rbao@unused-10-15-17-147 sysusers_test]$ vagrant ssh
Last login: Mon Jun  4 14:52:40 2018 from 192.168.121.1
[vagrant@localhost ~]$ id postgres
uid=26(postgres) gid=26(postgres) groups=26(postgres)
[vagrant@localhost ~]$ cat /etc/passwd | grep postgres
postgres:x:26:26:Postgresql Server:/var/lib/pgsql:/bin/bash

Let me know if my testing process is correct or not, thanks =).

peterbaouoft · 2018-06-04T15:04:38Z

For now, I think I will look into rkt's implementation of sysusers and try redesigning the logic of current implementation. They seem to adopt systemd-sysusers 2-years ago. Will also report back which path I decided to go for converting sysusers =).

peterbaouoft · 2018-06-05T14:07:32Z

Hi, I have thought about the implementation yesterday, I am thinking we can avoid detecting "collisions" for sysusers, because collision case will likely only happen if there is an exact duplicate entries in /etc/passwd or /etc/group.

That way, we do not need to have a hashtable to track entries anymore.(which is complicated IMO :( ). Instead, we will have the following conversion logic:

1: We use a pointer array to track all of the sysuser entries (still a struct)
2: We want to have the pointer array ordered such that --> type 'g' sysuser entries is always placed before 'u' entries, and both of them will be placed before 'm' entries. See sysuser types

Possible drawbacks:

The sorting speed might be slow
Flexibility might be limited

Benefit:

Easier to implement, do not need to worry about collision logic that much
Had much easier method to sort the entries, which hashtable itself does not provide

I will be trying this method out for now, but let me know your thoughts on this, and I am willing to change accordingly :p. Thanks, @cgwalters.

cgwalters · 2018-06-06T12:48:26Z

The sorting speed might be slow

In general don't worry about performance at first unless your algorithm is worse than quadratic, and g_ptr_array_sort() is a merge sort which is O(n log n). Also the size of data here is tiny right?

because collision case will likely only happen if there is an exact duplicate entries in /etc/passwd or /etc/group.

Yeah, that shouldn't happen - I'd say we fatally error out if we find out it did.

peterbaouoft · 2018-06-14T15:16:23Z

STATUS UPDATE:
To have generated sysusers entries work, there needed to be few extra workarounds that need to be applied.

1: systemd-sysusers will error out if there already exists a group entry in /etc/gshadow
workaround: will be removing the existing system user entries from gshadow ( as they will be regernerated ).

2: systemd itself ships /usr/lib/sysusers.d/xx.conf file by default, and they will interact during the users generation

/usr/lib/sysusers.d
[root@localhost sysusers.d]# ls
basic.conf  dbus.conf  dnsmasq.conf  systemd.conf

workaround: create symlinks with exact same name, but pointing to /dev/null so the entries in /usr/lib/sysusers.d got override/ignored

3: Need to remove altfiles from nsswitch.conf
workaround 1: can add a condition to skip replace_nsswitch
workaround 2: go through the nsswitch.conf file and remove altfiles if there happens to be one

4: Passwd/group entries regeneration during rpm-ostree compose
workaround: Let sysuser logic behave same as the existing user generation in _data_from_json.

NOTE: for item 2 and 3, it might take me some extra time to work with... As I am still quite new to file handling in GLib, but I will try the best to remove WIP label before next Monday. =) Thank you for your patience.

rh-atomic-bot · 2018-06-15T13:19:45Z

☔ The latest upstream changes (presumably ec0af35) made this pull request unmergeable. Please resolve the merge conflicts.

cgwalters · 2018-06-20T18:36:50Z

For the shadow files, see ostreedev/ostree#1562

Ideally it would work to delete them from the compose (ostree) and have a tmpfiles.d to create this on boot (before sysusers runs).

peterbaouoft · 2018-06-21T17:45:23Z

Status Update:
Hi, I have chatted with steve a bit today, and it seems like the team has higher priority tasks to achieve. So, part of my effort will be made towards learning/catching up with the OTA content/work. But I will still spend 20-30% of the day working on this change.

I think the remaining bits needing to do are likely to be the following:

Split up the conversion bits into separate PR to reduce review effort
Handle correctly for the interactions between deploying actions (upgrade, rebase, and package layering) and user passwd data (tho that might be in a different PR).
Systemd-sysusers still have some corner cases. E.g: we have games:x:12:100:games:/usr/games:/sbin/nologin and games:x:20:. In that case games user has another group as its primary group, and systemd-sysuser will not handle this corner case correctly.
Add more detailed integration tests to test sysuser functionalities.

peterbaouoft · 2018-06-25T15:21:19Z

Status update:
Sorry, got sidetracked a bit, and forgot to track this. I had a discussion with colin last Wednesday about some of the concerns/workarounds above, and below is part of the discussion:

1: We can create a unified conf for systemd-sysuser placed in /usr/lib/sysusers.d that contains all the converted entries originally from /usr/lib/passwd. (/etc/sysusers.d/ will be difficult because of 3 way merge problem)
2: To avoid duplicated sysuser entries from existing packages, one way we could do is to delete old /usr/lib/sysusers.d folder, or another way is to redesign the data structure a bit to track existing sysuser entries
3: For existing /etc/gshadow and /etc/shadow problem for sysusers, we can delete them during compose time, and regenerate the content possibly using systemd-tmpfiles or systemd-sysusers default.

4: The existing 3 way merge logic for /etc/ should be as following:

There are 3 "parts" are getting merged for configuration, the current deployment's /etc, the default configuration stored in /usr/etc and the new deployment's /etc
The difference between current deployment and default configuration is computed, and for files are added, it will be added to the new deployment
for files are deleted, it will be gone for the new deployment
for files are modified, it will be replacing the file in the new deployment

Note: Due to 3 way merge logic above, sysuser will have some difficulties as /etc/{passwd, group} will always remain modified compared to default configuration, because group/passwd entries will be later added by systemd-sysusers.

peterbaouoft · 2018-06-26T21:10:51Z

STATUS UPDATE:
1: 3 way merge problem with /etc. As mentioned earlier, due to the nature of systmed-sysusers(running during boot time), /etc/{passwd, group} will always remain modified, causing problems during 3 way merge. (And really, that is why we used nss-altfiles all of this past time I think)

A way to fix the problem can be one of the following methods:

Have extra files that keeps track of the non-system user
Whenever a deploy operation is happening. Reedit the /etc/{passwd, group} from the current deployment we are merging such that there will be no system user in there. We can do so by using data structures that compares entries that we have in /usr/lib/sysusers.d/rpm-ostree.conf, and remove whatever entries that is duplicated in the existing conf file.

Then, theoretically, package layering and upgrade problem should be solved. Because all we need to do is now maintain the sysuser entries in /usr/lib/sysusers.d.

Implementation:

During package layering, when adding users in rpmostree_context_assemble, we collect those entries and add to /usr/lib/sysusers.d/rpm-ostree.conf instead. (The name for conf can surely change).
When we remove a package during package layering, it will be similar to upgrade and rebase, we clean up /etc/{passwd,group} files and let systemd-sysuser regenerate them.

But may be.. I am understanding the usage of systemd-sysuser wrong... @cgwalters can you sanity check whether my suggestion implementation above is reasonable? Thanks =).

2: shadow file entries.. Hmm, this can be tricky to do so.. As systemd-sysusers itself also writes entries into them... I am still kinda unsure what will be the benefit of using systemd-tempfiles vs systemd-sysusers. I still look more into systemd-tmpfiles documentation to figure that out.

cgwalters · 2018-06-26T21:21:12Z

During package layering, when adding users in rpmostree_context_assemble, we collect those entries and add to /usr/lib/sysusers.d/rpm-ostree.conf instead. (The name for conf can surely change).

Yeah, this one is messy, because need to handle both the base tree and layers, so I think we need two files. Let's say /usr/lib/sysusers.d/rpm-ostree-base.conf and /usr/lib/sysusers.d/rpm-ostree-layer.conf

Though in theory we could append to an existing /usr/lib/sysusers.d/rpm-ostree.conf when doing layering...but we'd have to be careful to break the hardlink, and eh...it seems cleaner to have them be separate anyways.

When we remove a package during package layering, it will be similar to upgrade and rebase, we clean up /etc/{passwd,group} files and let systemd-sysuser regenerate them.

In general, that's a bit dangerous to do because the uid/gid could be reused, and if there's old data lying around in e.g. /var a daemon may be able to access it. On classic systems, users/groups don't get deleted on package uninstallation for this reason among others. And we'll have the same semantic with sysusers, and I think that's OK. Admins can always go in and prune things manually.

And in the end what we really want is 1) services to be containerized 2) to use the new dynamic users where possible - a lot of the OS could do this.

peterbaouoft · 2018-06-26T21:52:14Z

Thanks for the reply \o/!

Let's say /usr/lib/sysusers.d/rpm-ostree-base.conf and /usr/lib/sysusers.d/rpm-ostree-layer.conf

Yup, that makes sense to me.

In general, that's a bit dangerous to do because the uid/gid could be reused, and if there's old data lying around in e.g. /var a daemon may be able to access it. On classic systems, users/groups don't get deleted on package uninstallation for this reason among others

Right, I recall I have seen that in Dynamic user documentation. Makes sense. I think we don't need to do the user removal then.

In that case, we will only need to cleanup for /etc/{passwd,group} during rebase/upgrade operations I think. I will start doing so :).

cgwalters · 2018-08-23T12:54:14Z

I think this one is probably now the biggest blocker for making it easier for people to do custom composes. Having uids change on you is an evil trap.

I need to dive into these patches again in depth, but it looks like we could at least merge some of the preparatory patches?

peterbaouoft · 2018-08-23T13:10:41Z

Hi, thanks for the update :).

biggest blocker for making it easier for people to do custom composes. Having uids change on you is an evil trap

This sounds exciting, good to see this patch to be needed soon =D. Out of curiosity, do you have more info related to that?

but it looks like we could at least merge some of the preparatory patches?

I will likely have some spare cycles after today. I can spend some time separating the preparatory commits into a PR tmr. Do you think it(separating patch) is reasonable / fitting for the timeline?

rh-atomic-bot · 2018-08-30T18:20:25Z

☔ The latest upstream changes (presumably 9920094) made this pull request unmergeable. Please resolve the merge conflicts.

peterbaouoft · 2018-08-31T14:42:53Z

Status Update: Rebased , and pushed out what I have so far. Unfortunately, I did not get very far yesterday, so the leftover bits are still pretty similar to the comment I made here.

The implementation bits are explained in the commit messages, if they are not clear, please let me know, and I can hopefully clarify it :).

I will also try to keep an eye on this PR, and really looking forward about its future progress :D. Again, thanks for the help in this PR.

The logic for writing sysusers is integrated into both compose logic and rpmostree_check_passwd_groups. The plan is that whenever "sysuser" is specified in check-passwd section, The compose will use the sysuser logic. The passwd entries instead will be converted to sysuser entries. And eventually at the end of compose a file will be written to '/usr/lib/sysusers.d'.

As tilte. This adds a 'temp-convert' command to rpm-ostree. The main goal of this command is to convert the AH's /usr/lib/passwd and /usr/lib/group for testing purposes. This can be useful to find whether your output for conversion is correct when testing on real-world entries

cgwalters · 2018-09-06T14:55:20Z

To make this work, we're probably going to need to re-implement sysusers for CentOS7 based systems right?

peterbaouoft · 2018-09-06T18:30:24Z

To make this work, we're probably going to need to re-implement sysusers for CentOS7 based systems right

Hi, I seem to be missing a bit of context here. Can you elaborate a bit more on that? Is it because CentOS7 is a community based version of RHEL? (and if we succeed on c7, we does for rhel also)

And to clarify a bit, by "re-implementing sysusers", do you mean by modifying some existing systemd-sysuser logic or will the logic still be under rpm-ostree compose ?

cgwalters · 2018-09-29T13:57:59Z

The problem with supporting check-passwd: sysusers today is that the systemd-sysusers implementation in systemd doesn't exist in RHEL7. But the logic isn't very complicated - we could basically just reimplement it.

cgwalters · 2018-09-29T14:00:01Z

(Alternatively, we could ignore RHEL7)

cgwalters · 2018-09-29T14:17:12Z

src/app/rpmostree-compose-builtin-tree.c

 if (!rpmostree_check_passwd (self->repo, self->rootfs_dfd, treefile_dirpath, self->treefile,
- self->previous_checksum,
+ self->previous_checksum, &sysuser_entries,


It seems to me that if we're using sysusers - there's really no reason to go into any of the rpmostree_check_passwd() code. With sysusers, the uids are assigned on the client side, unlike every other approach here. The code here was mostly concerned with comparing from the "previous" values, but conceptually we don't have any previous values.

It seems the main thing we're lifting there is the current values of the passwd/group database.

One thing it's tempting to do here too is add a new systemd-sysusers: true boolean, and then that overrides all of the other legacy stuff. Then here in compose-builtin-tree.c we'd do:

if (self->treefile) { if (sysusers) { if (!rpmostree_passwd_sysusers_convert (self->rootfs_fd, cancellable, error)) return FALSE; } else { if (!rpmostree_check_passwd (self->repo, self->rootfs_dfd, ...) return FALSE; } }

or so?

It seems to me that if we're using sysusers - there's really no reason to go into any of the rpmostree_check_passwd() code. With sysusers, the uids are assigned on the client side, unlike every other approach here.

Ha, I was thinking to propose this approach, but then I figured some packages seem to own their specific uid/gid (Can't seem to find a source, so may be I was wrong). One example i believe is daemon sometimes own uid 2? So, I thought check_passwd and check_group was mainly used for that case (the exact mapping of package assignment).

But if not, then I agree, we don't necessarily need to check sysusers for "previous" values.

One thing it's tempting to do here too is add a new systemd-sysusers: true boolean, and then that overrides all of the other legacy stuff. Then here in compose-builtin-tree.c we'd do:

Yea, this implementation makes sense, and if we are skipping that, may be we can also skip the sections here too?
https://github.com/projectatomic/rpm-ostree/blob/master/src/libpriv/rpmostree-postprocess.c#L971-L991.

I was thinking earlier if we could work around the sysuser logic a bit so that the migration of passwd needs not to happen (or skipped) in postprocess.

Ha, I was thinking to propose this approach, but then I figured some packages seem to own their specific uid/gid (Can't seem to find a source, so may be I was wrong). One example i believe is daemon sometimes own uid 2?

A bunch of uid/gid values are fixed; see https://fedoraproject.org/wiki/Packaging:UsersAndGroups and note https://pagure.io/setup/blob/master/f/uidgid

Is that what you're thinking of?

But that...oh wait, argh, I think this calls into question our entire approach here 😦 - I'll comment below.

A bunch of uid/gid values are fixed; see https://fedoraproject.org/wiki/Packaging:UsersAndGroups and note https://pagure.io/setup/blob/master/f/uidgid
Is that what you're thinking of?

Yup. :D

peterbaouoft · 2018-10-01T00:53:15Z

But the logic isn't very complicated - we could basically just reimplement it.

May be a dumb question. If we are doing that, can we reuse some of the existing logic from systemd-sysusers? :P

cgwalters · 2018-10-01T13:23:34Z

May be a dumb question. If we are doing that, can we reuse some of the existing logic from systemd-sysusers? :P

The problem is that systemd has a vast array of "internal" APIs, from hash tables to filesystem utilities, etc. A lot of C projects have this to some degree, though rpm-ostree uses GLib which helps a lot. If you look at that code almost every single function call we'd need to port.

I'm thinking a bit about writing a basic implementation of sysusers in Rust. But trying a line-by-line port of the systemd code could work fine too.

Or, I am also OK declaring this a post-RHEL7 feature too.

cgwalters · 2018-10-01T13:48:46Z

Following up from #1376 (comment) -

The problem with our design that I just realized is that we're not correctly distinguishing between "fixed" and dynamic uid/gid values.

Let's take the two cases of openssh, which has a fixed uid/gid, and chrony which has a dynamic one.

Today server-side flow with this current PR is: useradd → /etc/passwd → sysusers. But with that approach, we've lost the information about whether the invocation of useradd contained the -u flag or not - in other words, whether or not the user has a fixed or dynamic uid.

And this is important for the chrony case, because with the current logic we're effectively assigning a random fixed uid at compose time, right? Whereas the whole goal of sysusers is to assign it client side.

The only way I can think of to fix this is to intercept calls to useradd at compose time and handle the -u argument.

Sorry about not realizing this earlier...this whole problem is very complex!

peterbaouoft · 2018-10-04T11:47:14Z

Hi, sorry about the late reply. Been busy with school work lately...

Today server-side flow with this current PR is: useradd → /etc/passwd → sysusers. But with that approach, we've lost the information about whether the invocation of useradd contained the -u flag or not - in other words, whether or not the user has a fixed or dynamic uid.

I agree with the server side flow, but am confused about why we can't tell between fixed or dynamic id with the flow. I thought useradd will add the user to database no matter if -u flag is specified or not?

And this is important for the chrony case, because with the current logic we're effectively assigning a random fixed uid at compose time, right? Whereas the whole goal of sysusers is to assign it client side.

Yup, I believe so, if useradd assigns a random value at compose time. Ahhhhh.. I see what you mean, we want chrony always to be dynamic? But, isn't assigning a random fixed uid at compose time representing a dynamic uid anyways? (I might be wrong on the concept tho).

peterbaouoft · 2018-10-04T11:50:15Z

The only way I can think of to fix this is to intercept calls to useradd at compose time and handle the -u argument.

If we are going that route, systemd-sysusers seem to be able to handle automatic assignment of uids/gids:

Specify "-" for automatic UID/GID allocation for the user or group (this is strongly recommended unless it is strictly necessary to use a specific UID or GID)

maybe we can use that?

rh-atomic-bot · 2018-10-11T16:50:03Z

☔ The latest upstream changes (presumably 096f8de) made this pull request unmergeable. Please resolve the merge conflicts.

rh-atomic-bot · 2019-09-13T16:09:50Z

Can one of the admins verify this patch?
I understand the following commands:

bot, add author to whitelist
bot, test pull request
bot, test pull request once

openshift-ci-robot · 2021-04-11T10:22:38Z

@peterbaouoft: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

peterbaouoft commented May 29, 2018

View reviewed changes

jlebon added the WIP label May 29, 2018

cgwalters reviewed May 30, 2018

View reviewed changes

peterbaouoft force-pushed the pr/add_sysusers_to_compose branch 2 times, most recently from c6bf9c2 to a9ef91e Compare June 11, 2018 16:11

peterbaouoft mentioned this pull request Jun 15, 2018

Question: modification time for /usr and /etc #1415

Closed

peterbaouoft force-pushed the pr/add_sysusers_to_compose branch 2 times, most recently from ec396b6 to c4472fe Compare June 29, 2018 20:46

peterbaouoft mentioned this pull request Jun 29, 2018

Drop requirement on nss-altfiles, use systemd sysusers #49

Open

peterbaouoft mentioned this pull request Aug 27, 2018

passwd-util/test: add conversion to sysusers entry #1519

Closed

peterbaouoft force-pushed the pr/add_sysusers_to_compose branch from c4472fe to 6677e6b Compare August 31, 2018 14:35

peterbaouoft added 2 commits August 31, 2018 14:43

peterbaouoft force-pushed the pr/add_sysusers_to_compose branch from 6677e6b to 4bb8db1 Compare August 31, 2018 14:44

cgwalters reviewed Sep 29, 2018

View reviewed changes

openshift-ci-robot added the needs-rebase label Apr 11, 2021

lucab added the area/sd-sysusers label Jun 13, 2022

WIP: add sysusers to compose #1376

Are you sure you want to change the base?

WIP: add sysusers to compose #1376

Conversation

peterbaouoft commented May 29, 2018

Choose a reason for hiding this comment

cgwalters commented May 30, 2018

Choose a reason for hiding this comment

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbaouoft commented May 30, 2018

peterbaouoft commented May 30, 2018

peterbaouoft commented Jun 1, 2018 • edited Loading

peterbaouoft commented Jun 1, 2018

peterbaouoft commented Jun 1, 2018 • edited Loading

peterbaouoft commented Jun 1, 2018 • edited Loading

peterbaouoft commented Jun 4, 2018

peterbaouoft commented Jun 4, 2018 • edited Loading

peterbaouoft commented Jun 5, 2018

cgwalters commented Jun 6, 2018

peterbaouoft commented Jun 14, 2018

rh-atomic-bot commented Jun 15, 2018

cgwalters commented Jun 20, 2018

peterbaouoft commented Jun 21, 2018

peterbaouoft commented Jun 25, 2018 • edited Loading

peterbaouoft commented Jun 26, 2018 • edited Loading

cgwalters commented Jun 26, 2018

peterbaouoft commented Jun 26, 2018

cgwalters commented Aug 23, 2018

peterbaouoft commented Aug 23, 2018 • edited Loading

rh-atomic-bot commented Aug 30, 2018

peterbaouoft commented Aug 31, 2018

cgwalters commented Sep 6, 2018

peterbaouoft commented Sep 6, 2018

cgwalters commented Sep 29, 2018

cgwalters commented Sep 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbaouoft commented Oct 1, 2018 • edited Loading

cgwalters commented Oct 1, 2018

cgwalters commented Oct 1, 2018

peterbaouoft commented Oct 4, 2018

peterbaouoft commented Oct 4, 2018

rh-atomic-bot commented Oct 11, 2018

rh-atomic-bot commented Sep 13, 2019

openshift-ci-robot commented Apr 11, 2021

peterbaouoft commented Jun 1, 2018 •

edited

Loading

peterbaouoft commented Jun 1, 2018 •

edited

Loading

peterbaouoft commented Jun 1, 2018 •

edited

Loading

peterbaouoft commented Jun 4, 2018 •

edited

Loading

peterbaouoft commented Jun 25, 2018 •

edited

Loading

peterbaouoft commented Jun 26, 2018 •

edited

Loading

peterbaouoft commented Aug 23, 2018 •

edited

Loading

peterbaouoft commented Oct 1, 2018 •

edited

Loading