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

Fix #586: Use user name instead of userid #589

Merged
merged 1 commit into from
Oct 6, 2021

Conversation

tobiasge
Copy link
Member

Related Issue: #586

New Behavior

  • No error when container is started as root

Contrast to Current Behavior

  • [alert] 7#7 getpwnam("101") failed, user "101" not found

Discussion: Benefits and Drawbacks

  • The container should error out when started as root, even when it is not recommended to do that.

Changes to the Wiki

  • Better explanation for Openshift / Kubernetes that the container can run as any userid as long as the groupid is set to 0. (This is the default for Openshift)

Proposed Release Note Entry

  • Using names instead of IDs to set the user

Double Check

  • I have read the comments and followed the PR template.
  • I have explained my PR according to the information in the comments.
  • My PR targets the develop branch.

Nginx unit needs the user and group parameter as names.
Copy link
Collaborator

@cimnine cimnine left a comment

Choose a reason for hiding this comment

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

When you put it like this, the solution looks obvious. Thanks.

@Spiritus44
Copy link

Hello,

Can someone approve this PR?
I myself rebuilt a netbox image and it's solved the issue.

Thanks a lot!

@kkthxbye-code
Copy link

Using names instead of IDs to set the user

I feel like this is misleading. The uid of the user unit is 100, not 101.

unit:x:100:101:NGINX Unit:/var/lib/unit:/sbin/nologin

So you are changing the running user from uid 101 (nonexistent user) to uid 100 (the unit user).

According to this comment, using a nonexistent userid was on purpose.

I think I get what you are trying to do, but the description does not match the change.

Also as a sidenote, if you are running docker rootless or with userns, there is nothing wrong with running the container as root.

@cimnine
Copy link
Collaborator

cimnine commented Oct 5, 2021

So you are changing the running user from uid 101 (nonexistent user) to uid 100 (the unit user).

Does it matter? (This is a serious question, I don't mean to sound snarky.)

According to this comment, using a nonexistent userid was on purpose.

I disagree. IIRC, it was never our intention to use a userid with no corresponding entry in /etc/passwd. We just happened to use a userid with not corresponding entry, and we figured this was fine to us as the entry was not technically necessary.

@cimnine cimnine added the bug This issue describes a confirmed bug. label Oct 5, 2021
@kkthxbye-code
Copy link

Does it matter? (This is a serious question, I don't mean to sound snarky.)

Does it matter in a practical sense, maybe. Files touched by unit in mounted folders (media only?) will now have a new UID. Could cause issues depending on permission setup on the host.

Using a low UID is also a little iffy, if the goal is to isolate the container better from the host. UID 100 is _apt and 101 is systemd-timesync on debian for example, most distros start application users from uid 100. Running the container as the root group also doesn't seem like the right choice when trying to isolate the host.

My other gripe was with creating changes that are not what the commit message states. The commit should mention that the user was changed from 101 (nonexistent) to 100 (unit) and the PR should at least mention why, reading the PR it doesn't seem that @tobiasge was aware of this, which is why I pointed it out.

@tobiasge
Copy link
Member Author

tobiasge commented Oct 5, 2021

Does it matter? (This is a serious question, I don't mean to sound snarky.)

Does it matter in a practical sense, maybe. Files touched by unit in mounted folders (media only?) will now have a new UID. Could cause issues depending on permission setup on the host.

Using a low UID is also a little iffy, if the goal is to isolate the container better from the host. UID 100 is _apt and 101 is systemd-timesync on debian for example, most distros start application users from uid 100. Running the container as the root group also doesn't seem like the right choice when trying to isolate the host.

We're now using the account that is created when installing the nginx-unit package for Alpine. As for the root group: Openshift (and Kubernetes) use this group (and a random userid) for pods that are run in those plattforms. So I think this is OK.

My other gripe was with creating changes that are not what the commit message states. The commit should mention that the user was changed from 101 (nonexistent) to 100 (unit) and the PR should at least mention why, reading the PR it doesn't seem that @tobiasge was aware of this, which is why I pointed it out.

I'm aware of the change. Will mention it in the release notes.

@cimnine cimnine merged commit daaea77 into netbox-community:develop Oct 6, 2021
@tobiasge tobiasge deleted the user-unit branch October 14, 2021 06:35
matofeder added a commit to osism/ansible-collection-services that referenced this pull request Aug 14, 2024
Nginx Unit user UID has changed to 999 which breaks git data source sync
in Netbox.

Netbox 3.7.x container fixed this and use user name and group `unit:root`
instead od user ID.

This fix sets the default netbox user in the same way as `unit:root`.

Refer to related issue and netbox-docker PR:
- netbox-community/netbox-docker#1200
- netbox-community/netbox-docker#589

Signed-off-by: Matej Feder <matej.feder@dnation.cloud>
berendt pushed a commit to osism/ansible-collection-services that referenced this pull request Aug 14, 2024
Nginx Unit user UID has changed to 999 which breaks git data source sync
in Netbox.

Netbox 3.7.x container fixed this and use user name and group `unit:root`
instead od user ID.

This fix sets the default netbox user in the same way as `unit:root`.

Refer to related issue and netbox-docker PR:
- netbox-community/netbox-docker#1200
- netbox-community/netbox-docker#589

Signed-off-by: Matej Feder <matej.feder@dnation.cloud>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue describes a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants