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

passwd: sync etc/{,g}shadow according to etc/{passwd,group} #4503

Merged
merged 3 commits into from
Jul 19, 2023

Conversation

HuijingHei
Copy link
Member

@HuijingHei HuijingHei commented Jul 17, 2023

passwd: Rename func data_from_json to write_data_from_treefile


passwd: sync etc/{,g}shadow according to etc/{passwd,group}

Refer to #49 (comment), do testing:

  1. Remove bin line in group and passwd
  2. Build FCOS, see logs:
systemd.post: Creating group 'bin' with GID 1.
systemd.post: Creating user 'bin' (bin) with UID 1 and GID 1.
systemd.post: /etc/gshadow: Group "bin" already exists.

According to @cgwalters 's pointer:

The above log will lead systemd-sysusers (during systemd.post)
exit early before saving the updated /etc/{passwd,group} refer
to code, and bin user/group will not be saved finally.

The root cause is that gshadow is not consistent with group,
gshadow is from setup, and we override group according to https://github.com/coreos/fedora-coreos-config/blob/testing-devel/manifests/group.

The shadow is also from setup, and is not consistent with
passwd, we should also sync it.

Fix coreos/fedora-coreos-tracker#1525


passwd: add enum PasswdKind

Refer to d8dc960

rust/src/passwd.rs Outdated Show resolved Hide resolved
})
.with_context(|| format!("Writing {target_etc_shadow}"))?;
}
_ => {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, this should never be reached and should fail.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, unreachable! is probably OK here.

(Or with more work, we could actually try to pass around an enum PasswdKind { User, Group } instead?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, updated, thanks!

rust/src/passwd.rs Outdated Show resolved Hide resolved
@travier
Copy link
Member

travier commented Jul 17, 2023

Could you add more context / explanation for the change?

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sane to me, just nits to fix generally

rust/src/passwd.rs Outdated Show resolved Hide resolved
@HuijingHei HuijingHei force-pushed the sync-gshadow branch 2 times, most recently from 697043e to 196db3f Compare July 18, 2023 02:01
Refer to coreos#49 (comment),
do testing:
1. Remove bin line in group and passwd
2. Build FCOS, see logs:
```
systemd.post: Creating group 'bin' with GID 1.
systemd.post: Creating user 'bin' (bin) with UID 1 and GID 1.
systemd.post: /etc/gshadow: Group "bin" already exists.
```

According to @cgwalters 's pointer:

The above log will lead systemd-sysusers (during systemd.post)
exit early before saving the updated `/etc/{passwd,group}` refer
to [code](https://github.com/systemd/systemd/blob/main/src/sysusers/sysusers.c#L820),
and bin user/group will not be saved finally.

The root cause is that `gshadow` is not consistent with group,
`gshadow` is from setup, and we override group according to https://github.com/coreos/fedora-coreos-config/blob/testing-devel/manifests/group.

The `shadow` is also from setup, and is not consistent with
passwd, we should also sync it.

Fix coreos/fedora-coreos-tracker#1525
HuijingHei added a commit to HuijingHei/rpm-ostree that referenced this pull request Jul 18, 2023
@HuijingHei
Copy link
Member Author

Could you add more context / explanation for the change?

Sure, add more context and can also see in the commit.

HuijingHei added a commit to HuijingHei/rpm-ostree that referenced this pull request Jul 18, 2023
HuijingHei added a commit to HuijingHei/rpm-ostree that referenced this pull request Jul 18, 2023
HuijingHei added a commit to HuijingHei/rpm-ostree that referenced this pull request Jul 18, 2023
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other cleanup we can do, but not blocking.

rust/src/passwd.rs Outdated Show resolved Hide resolved
HuijingHei added a commit to HuijingHei/rpm-ostree that referenced this pull request Jul 19, 2023
HuijingHei added a commit to HuijingHei/rpm-ostree that referenced this pull request Jul 19, 2023
@HuijingHei
Copy link
Member Author

Sorry to add hold label, drop the label now.

rootfs
.atomic_replace_with(&target_shadow_path, |target_shadow| -> Result<()> {
for user in entries {
writeln!(target_shadow, "{}:*::0:99999:7:::", user.name)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to actually look some of this up... https://www.cyberciti.biz/faq/understanding-etcshadow-file/ is a useful reference. I think for now, it's probably fine to just hardcode these things.

What may be a little bit more sustainable though (albeit very hacky) would be to run useradd --system demouser at build time (on a throwaway rootfs) and then scrape out the defaults it injects.

That way, the constants from libuser are a source-of-truth and we don't have a duplicate copy effectively.

But what we're doing now is a lot better than not injecting shadow entries at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the link, that is helpful.
The above format refers to https://pagure.io/setup/blob/master/f/shadowconvert.sh, the comment still makes sense to me.

@cgwalters cgwalters merged commit d498bac into coreos:main Jul 19, 2023
17 checks passed
@HuijingHei HuijingHei deleted the sync-gshadow branch July 19, 2023 13:57
lukewarmtemp pushed a commit to lukewarmtemp/rpm-ostree that referenced this pull request Aug 8, 2023
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

Successfully merging this pull request may close these issues.

sysusers: remove bin entries and get systemd.post: /etc/gshadow: Group "bin" already exists.
3 participants