Permissions issue when querying builder on Windows

[colinhemmings]

When I set up Buildkit on Windows using the approach in the docs, I was unable to query the builder in Buildx.

Steps:

• Setup Buildkit, Containerd, and Buildx builder as per the docs (using Admin shell)
• Open a new Powershell terminal (non-admin)
• Try to query the builder docker buildx du and the builder is classed as inactive.
• Try to run a build and it will error

This worked is using an admin shell, however I believe this is based on how the npipe is configured. As I can query the engine with in the non-admin shell. When I inspect the permissions for the engine vs the buildkit pipe, they differ:

• https://github.com/moby/moby/blob/master/daemon/listeners/listeners_windows.go#L25
• https://github.com/moby/buildkit/blob/master/cmd/buildkitd/main_windows.go#L23-L29

[colinhemmings]

cc @profnandaa

[thaJeztah]

@colinhemmings was the Windows Engine in your case installed through Docker Desktop?

Afaik, the default permissions for the Docker Engine's named pipe are (by design) restricted to admin users, as the Engine runs as a privileged service, so being able to access the API (similar to on Linux) means any user that can access the API is able to escalate to a privileged user.

On Docker Desktop (again, if memory serves me well), the named pipe also gets a docker-users group added, which allows non-privileged users that are in that group to get access. That logic is not part of the engine itself though, but set up as part of Docker Desktop.

[colinhemmings]

Yes, it's installed as part of DD. Yes, I see the docker-users group. I assumed the config was part of the setting up the npipe. Perhaps this is just an update to the docs then to add the group to the buildkit pipe.

[colinhemmings]

Looking at that code for the Engine, it would appear it does add the groups to the pipe based on what is passed in. We will need to add the same functionality to Buildkit to support controlling access to the pipe.

[profnandaa]

Since currently we are yet to have a low-priv ("rootless") access to buildkitd, being able to do some things from low-priv (like querying) and then some of them only on admin will be a confusing experience for users. Though again, I don't see any harm on allowing read-like operations from a low privileged client.

Can point me to the code on Docker engine that does this, would like to compare.

[thaJeztah]

Looking at that code for the Engine, it would appear it does add the groups to the pipe based on what is passed in. We will need to add the same functionality to Buildkit to support controlling access to the pipe.

Oh! Maybe I misremembered, and it's adding the group to the pipe (and Docker Desktop handled creating the group - that may be the part I remembered ☺️ )

[thaJeztah]

Hmm.. trying to find where a group is set though; I see that (unlike the Unix counterpart), there's no default group set on Windows; https://github.com/moby/moby/blob/faf84d7f0a1f2e6badff6f720a3e1e559c356fff/cmd/dockerd/config_windows.go#L18

Unix/Linux code has a default (docker) group set there;
https://github.com/moby/moby/blob/faf84d7f0a1f2e6badff6f720a3e1e559c356fff/cmd/dockerd/config_unix.go#L26

But depending on how the daemon is run may also set this through systemd; https://github.com/moby/moby/blob/faf84d7f0a1f2e6badff6f720a3e1e559c356fff/contrib/init/systemd/docker.socket#L4-L10

And I don't see a default being set here for Windows; https://github.com/moby/moby/blob/faf84d7f0a1f2e6badff6f720a3e1e559c356fff/daemon/listeners/listeners_windows.go#L25-L47

I'm not on Windows myself though, so not sure if there's possibly something else setting it.

[colinhemmings]

I have the engine running on Windows (via Docker Desktop), and the permissions on the pipe are set correctly; my user and group are added with RW access. My assumption was that these groups were passed to Moby by Docker Desktop.
If we can use the same approach for buildkit, then I can set the group configuration before starting Buildkit.

@profnandaa I think it already had Read access by default, but RW is needed. I'm not saying that we want to allow RW by default, but supporting it via config.

[tonistiigi]

Tentatively adding to v0.13.2 milestone, but we would need a patch for tomorrow for it to make it.

[tonistiigi]

If the solution is that user needs to pass group on starting buildkitd, then adding new option like --socket-group sgtm (we can also put it to toml). For the patch that we can pick into v0.13.2 this flag should be windows-only. If it also makes sense for unix then we can have additional patch for linux that we can ship with v0.14

[profnandaa]

Tentatively adding to v0.13.2 milestone, but we would need a patch for tomorrow for it to make it.

Ok, let me give it a shot tomorrow.

[colinhemmings]

Happy to give it a test when it's ready!

[profnandaa]

@colinhemmings @tonistiigi -- turns out that this path wasn't being called when creating the npipe https://github.com/moby/buildkit/blob/master/cmd/buildkitd/main_windows.go#L23-L29, see fix at #4872
Do we still pursue the --socket-group config idea?

[tonistiigi]

Looks like we already have --group defined as a flag that gets resolved to gid and then passed into this function. But if you look at windows implementation of sys.GetLocalListener then it just drops it. So (eventually) we should definitely fix the --group so it works on both windows and linux. What should be the default if no --group set and if https://github.com/moby/buildkit/blob/master/cmd/buildkitd/main_windows.go#L23-L29 is intended and secure behavior is not clear to me. If you say it is the correct default SecurityDescriptor then I'll take your word for it.

[tonistiigi]

fixed by #4875