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

manifests/group: fix static GID for "tape" group #1832

Merged
merged 1 commit into from
Jul 11, 2022

Conversation

lucab
Copy link
Contributor

@lucab lucab commented Jul 8, 2022

This moves the "tape" group to GID 33.
Apparently the old GID is a typo/bug coming from F20 times, and it
conflicts with the actual static GID allocated to group "gopher".

Ref: https://pagure.io/setup/blob/289bc53aa97c0758d8cf910b99f3515e21a34a40/f/group#_18
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1179585

@travier
Copy link
Member

travier commented Jul 8, 2022

I have mixed feelings. On one hand, it's very unlikely that anyone uses this thus unlikely to create issues, but on the other hand, it creates a potentially hidden issue for those using it, it would definitely be our fault.

Maybe we should announce it? Or we just wait until we have all the pieces ready and we do the whole cleanup, changes and announcement in one go.

@cgwalters
Copy link
Member

Needs a test change?

[2022-07-08T15:08:00.922Z] Jul 08 15:07:57 qemu0 kola-runext-fcos_groups[1759]: failure on setup_groups entry tape

On one hand, it's very unlikely that anyone uses this thus unlikely to create issues, but on the other hand, it creates a potentially hidden issue for those using it, it would definitely be our fault.

I have difficulty imagining anyone using it...

@jlebon
Copy link
Member

jlebon commented Jul 8, 2022

I agree this is very unlikely to cause issues. But also agree that we can't be 100% sure. :)

I guess it's possible someone has been creating a system group with GID 33 which was previously safe to do because it was unallocated but now no longer is. Though that said, SYS_GID_MIN is set to 201 so they'd have to explicit request gid 33, which... crossing that boundary is kind of asking for trouble in the first place.

This moves the "tape" group to GID 33.
Apparently the old GID is a typo/bug coming from F20 times, and it
conflicts with the actual static GID allocated to group "gopher".

Ref: https://pagure.io/setup/blob/289bc53aa97c0758d8cf910b99f3515e21a34a40/f/group#_18
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1179585
@lucab lucab force-pushed the ups/fix-tape-group branch from b767212 to ae86a7c Compare July 11, 2022 09:20
@lucab
Copy link
Contributor Author

lucab commented Jul 11, 2022

This group is IMHO a special case which should be 99.9% safe to move right now to gradually reduce the amount of brokenness around users/groups and avoid a big bang later on.

The tape group is only used by udev rules to create SCSI tape devices on each boot:

$ grep tape /usr/lib/udev/rules.d/50-udev-default.rules
SUBSYSTEM=="scsi_generic|scsi_tape", SUBSYSTEMS=="scsi", ATTRS{type}=="1|8", GROUP="tape"

As such, there isn't really any filesystem entry persisted on disk with this GID (I double-checked this).
Local users that got added to this group will keep working even after the renumbering, on the next reboot which brings the new GID in place.

As for conflicts with a manually created group with GID 33, to my knowledge Linux allows multiple groupnames sharing the same GID (but I wouldn't recommend this, in the same way that I wouldn't recommend ignoring SYS_GID_MIN to begin with).

@travier
Copy link
Member

travier commented Jul 11, 2022

I was thinking more about someone re-using that group as is to run a daemon or a script as unprivileged or something else, which would be wrong I agree but that would work. And then when we update it breaks. Very unlikely I agree so no-objections from me.

@jlebon jlebon merged commit 8772f97 into coreos:testing-devel Jul 11, 2022
@lucab lucab deleted the ups/fix-tape-group branch July 11, 2022 14:51
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.

4 participants