snapstate/check_snap: add snap_docker to shared system-usernames #9396

anonymouse64 · 2020-09-22T17:30:50Z

After discussion with Gustavo and Samuele, we decided that it is best for the Canonical maintained docker snap to start listening on it's socket with group ownership of a specific snap_docker group that is shared with the host system on Ubuntu Core specifically (and eventually perhaps also on classic Ubuntu, but that requires additional design/thought). This is because it allows us to eventually add mechanisms for users to be added to this group on Ubuntu Core and facilitate non-root user access to the docker socket via i.e. docker commands.

Marking as blocked because while this PR is simple, I think we also want associated changes in the review-tools to block any snaps from being uploaded that use the snap_docker group, except the docker snap. AFAICT, other, non-docker snaps in practice cannot abuse membership of the snap_docker group alone to gain access to the docker socket due to our security-in-depth approach because they would still need to be provided access to the socket via AppArmor using the docker interface which is considered super-privileged and already blocked in the store, but perhaps this understanding is too naive on my part. Additionally, dockerd in the docker snap will not drop privileges to this snap_docker user, it will still run as root due to the operations it needs to take to start containers, so this new system-usernames group will really only be used by the docker snap for specifically it's socket group ownership and nothing else.

This PR is also the second ever system-username we are supporting in snapd, so we may want to explore other additional tests, etc. before we start adding additional system-usernames.

I have manually tested this PR with a version of the docker snap which supports listening on its socket using the snap_docker group, which I will make available for testing on this PR somehow.

Stacked on top of #9395 for the test helpers improvement so that we can actually test this out in snapstate properly.

jdstrand · 2020-09-22T18:08:58Z

Thanks for the PR. I've not had a chance to look at this extensively, but wanted to comment on the approach (and as it pertains to the use cases from https://forum.snapcraft.io/t/multiple-users-and-groups-in-snaps/1461/3).

I will say that the current implementation of snap_daemon uses "shared" scoping and because you are using *snap_*docker, this PR aligns with the concept shared scoping. (I suspect that the microk8s team would be interested in a 'snap_microk8s' user as well).

As for snapd being updated at a future date to add non-root users to the group, I would encourage you to consider use case 2, 'Device access' whenever you start to design that feature (even if you don't implement use case 2) since there will implementation and possibly UX overlap. Not sure if adding systemd directives for socket owner/group/perms is also on the horizon, but that might help docker if it has that type of systemd integration....

As for the review-tools, today, only the snap_daemon system-username is supported and use of any other username will result in a manual review. The long term goal is to have the store say what the snap is allowed to specify in some manner, but we can create new tests in the review-tools that utilize the overrides mechanism for now. This is not difficult to add; please feel free to create a bug against the review-tools and we can do this for you.

mvo5

Code looks good, one suggestion about the test.

overlord/snapstate/check_snap_test.go

anonymouse64 · 2020-09-23T10:29:42Z

I will say that the current implementation of snap_daemon uses "shared" scoping and because you are using *snap_*docker, this PR aligns with the concept shared scoping. (I suspect that the microk8s team would be interested in a 'snap_microk8s' user as well).

Right, the plan for now is that the docker snap will use snap_docker specifically because it is shared with the host system, and the group definition will be put into the extrausers db with a gid/uid greater than 500. This is desirable because as we discovered in snapcore/core20#72, nss-extrausers explicitly ignores groups/users in extrausers db with gid/uid's less than 500 so to add users to the group currently the group needs to be in extrausers, thus why we are not using the docker group anymore.

As for snapd being updated at a future date to add non-root users to the group, I would encourage you to consider use case 2, 'Device access' whenever you start to design that feature (even if you don't implement use case 2) since there will implementation and possibly UX overlap.

Agreed, there will likely be some UX overlap, I think the plan for this is probably just to expose an additional parameter for the /v2/users POST API, so that brand store apps with snapd-control creating users can just specify additional groups to place the user in when creating the user, but this hasn't been designed yet, so we will see.

Not sure if adding systemd directives for socket owner/group/perms is also on the horizon, but that might help docker if it has that type of systemd integration....

Well it's on a queue somewhere so probably safe to say is is on some horizon :-)

As for the review-tools, today, only the snap_daemon system-username is supported and use of any other username will result in a manual review. The long term goal is to have the store say what the snap is allowed to specify in some manner, but we can create new tests in the review-tools that utilize the overrides mechanism for now. This is not difficult to add; please feel free to create a bug against the review-tools and we can do this for you.

Sure, I will file a bug there about adding an override for the "docker" snap to allow it to use snap_docker as a shared system-username.

anonymouse64 · 2020-09-23T15:13:08Z

This PR may be more blocked than I realized... After talking with @pedronis again we need to discuss the classic case for docker before moving forward with this even on Core, so I will leave this PR open in the hope that we can resolve that case soonish, but I may close it if the meeting doesn't happen quickly so as to keep our PR count low and reopen after that discussion has happened with the final result

zyga

Did a pass over the change. I'd love to know what the classic result will be like. Left a small comment on one of the test logic.

zyga · 2020-09-28T09:41:47Z

osutil/group.go

@@ -29,13 +29,13 @@ import (

 // FindUid returns the identifier of the given UNIX user name. It will
 // automatically fallback to use "getent" if needed.
-func FindUid(username string) (uint64, error) {
+var FindUid = func(username string) (uint64, error) {


Isn't this just?

var FindUid = findUid

zyga · 2020-09-28T09:41:57Z

osutil/group.go

 	return findUid(username)
 }

 // FindGid returns the identifier of the given UNIX group name. It will
 // automatically fallback to use "getent" if needed.
-func FindGid(groupname string) (uint64, error) {
+var FindGid = func(groupname string) (uint64, error) {


Ditto here.

zyga · 2020-09-28T09:53:45Z

overlord/snapstate/check_snap.go

@@ -174,6 +174,7 @@ var featureSet = map[string]bool{
 // https://github.com/lxc/lxd/blob/master/doc/userns-idmap.md
 var supportedSystemUsernames = map[string]uint32{
 	"snap_daemon": 584788,
+	"snap_docker": 584789,


Man, reading the comment above this was exhausting.

…roup This will be used by the docker snap on Ubuntu Core specifically for now, so that users can be added to this group via the snapd /v2/users API eventually, but allowing snaps to create/use this group is a prerequisite. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

@mvo5

Thanks to @mvo5 for the patch/suggestion. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

anonymouse64 · 2021-05-07T17:03:17Z

This needs more work unfortunately but we don't have time for it right now

anonymouse64 added ⛔ Blocked Needs security review Can only be merged once security gave a :+1: labels Sep 22, 2020

anonymouse64 requested a review from jdstrand September 22, 2020 17:30

mvo5 approved these changes Sep 23, 2020

View reviewed changes

overlord/snapstate/check_snap_test.go Outdated Show resolved Hide resolved

zyga reviewed Sep 28, 2020

View reviewed changes

anonymouse64 added 3 commits September 28, 2020 11:28

o/snapstate/check_snap_test.go: add tests for snap_docker too

8557682

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

o/snapstate/check_snap_test.go: refactor tests to be less duplicative

6f86bbd

Thanks to @mvo5 for the patch/suggestion. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

anonymouse64 force-pushed the feature/snap-docker-shared-system-usernames branch from 8f67935 to 6f86bbd Compare September 28, 2020 16:28

anonymouse64 closed this May 7, 2021

anonymouse64 mentioned this pull request Jun 10, 2021

snapstate/check_snap: add snap_microk8s to shared system-usernames #10375

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

snapstate/check_snap: add snap_docker to shared system-usernames #9396

snapstate/check_snap: add snap_docker to shared system-usernames #9396

anonymouse64 commented Sep 22, 2020

jdstrand commented Sep 22, 2020

mvo5 left a comment

anonymouse64 commented Sep 23, 2020

anonymouse64 commented Sep 23, 2020

zyga left a comment

zyga Sep 28, 2020

zyga Sep 28, 2020

zyga Sep 28, 2020

anonymouse64 commented May 7, 2021

snapstate/check_snap: add snap_docker to shared system-usernames #9396

snapstate/check_snap: add snap_docker to shared system-usernames #9396

Conversation

anonymouse64 commented Sep 22, 2020

jdstrand commented Sep 22, 2020

mvo5 left a comment

Choose a reason for hiding this comment

anonymouse64 commented Sep 23, 2020

anonymouse64 commented Sep 23, 2020

zyga left a comment

Choose a reason for hiding this comment

zyga Sep 28, 2020

Choose a reason for hiding this comment

zyga Sep 28, 2020

Choose a reason for hiding this comment

zyga Sep 28, 2020

Choose a reason for hiding this comment

anonymouse64 commented May 7, 2021