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

[RFC] hooks: mv docker user/group definition to extrausers #82

Merged
merged 1 commit into from
Sep 10, 2020

Conversation

anonymouse64
Copy link
Contributor

@anonymouse64 anonymouse64 commented Aug 24, 2020

This will allow users to add themselves to the docker group. This in combination
with an upgraded getent which reads from extrausers will allow the docker snap
to be used with non-root users.

Fixes: #72

This will allow users to add themselves to the docker group. This in combination
with an upgraded getent which reads from extrausers will allow the docker snap
to be used with non-root users.

Fixes: canonical#72
Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
@xnox
Copy link
Contributor

xnox commented Sep 8, 2020

Ideally we'd have system-usernames but we do not have that.

The group id / user-id must be preserved, and it is.

The only check is to ensure that it doesn't conflict with consoleconf, when lxd started to do similar things it tripped up first-boot experience into thinking that presence of extrausers at first-boot means that device is already managed, when it was not.

Another thing to check, is what happens upon core20 snap refresh of existing beta instance with this inplace. Cause docker group will be dropped, but will it sync/get added to extra-users on writable parition, or not?

Obviously this is inert, unless docker snap is installed. And adding users to docker group must be done with care, as it opens up more privileges and is closer to root/sudo, then something confined.

@anonymouse64
Copy link
Contributor Author

we discussed most of this offline on mattermost, but just to be clear for the record here I will respond,

The only check is to ensure that it doesn't conflict with consoleconf, when lxd started to do similar things it tripped up first-boot experience into thinking that presence of extrausers at first-boot means that device is already managed, when it was not.

we don't have such a test, but I'm not sure how to write such a test, since we would need to essentially use something like expect over a serial connection to the uc20 VM (granted virtual serial since it's a VM, but still) which I am afraid of for practical reasons. I did this test manually when building this snap and built a fresh image out of a core20 image from this PR and console-conf worked fine for me, so I think it's good, but we don't have proof of that beyond manual testing.

Another thing to check, is what happens upon core20 snap refresh of existing beta instance with this inplace. Cause docker group will be dropped, but will it sync/get added to extra-users on writable parition, or not?

Existing beta users of uc20 will lose all definition of the docker group, however this is not a problem for folks using the docker snap because the status quo is that the docker group is defined in read-only /etc/group, so no user can ever be added to the docker group anyways and all actual users of docker on uc20 need to be sudo anyways. So if the docker group disappears, dockerd will log a message or something and happily fallback to listening on it's unix socket with the root group, which also effectively requires root/sudo to talk to docker, which is the same as the status quo, so no regression.

And adding users to docker group must be done with care, as it opens up more privileges and is closer to root/sudo, then something confined.

yes of course, we do not add any users to the docker group automatically, and this will need to be done manually, even if the docker snap is preinstalled on the image, there will not be any users in the docker group by default (except I guess root by definition?)

@anonymouse64 anonymouse64 marked this pull request as ready for review September 8, 2020 16:40
@xnox xnox merged commit c9c61f0 into canonical:master Sep 10, 2020
@anonymouse64 anonymouse64 deleted the feature/mv-docker-to-extrausers branch February 14, 2022 23:57
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.

move docker user/group to extrausers
2 participants