buildkitd: allow --group for windows

[tonistiigi]

No description provided.

[gabriel-samfira]

Just one change as far as I can see, and it has to do with the default SDDL in case none is provided by the user.

[gabriel-samfira]

The default can have just the system user allowed, and we can remove Authenticated users. This way we have a "secure default".

So this can be replaced with:

secDescriptor = "D:P(A;;GRGW;;;SY)"

Or better yet, the SDDL you have in groupToSecurityDescriptor is also OK. It allows builtin administrators and system user Generic All.

[tonistiigi]

This will only be used in traceController listener. The buildkitd socket securitydescriptor will always be initialized to groupToSecurityDescriptor("") if not set.

Happy to change that but I don't really understand why this default is different than the one in groupToSecurityDescriptor (that I copied from moby/moby).

[gabriel-samfira]

The bit:

(A;;GRGW;;;AU)

means allow generic read and write for Authenticated users. That means all users except guest. The one in groupToSecurityDescriptor allows generic all to the builtin\administrators group and the LocalSystem user, both of which are essentially administrators of the system, which is desirable in both cases (trace controller listener as well).

But if the trace controller is not always started, I guess it's fine to leave it as it is.

[gabriel-samfira]

This will be an interesting config option for users to set 😄, but it's in line with the type of value linux users need to set, and it is correct compared to using group names here.

[tonistiigi]

I think we will mostly expect users to use --group flag.

[profnandaa]

Thanks! testing and getting back asap.

// this fixes #4864 and #4873 -- I'll do a follow up with some documentation on doc/windows.md and also update docs/buildkitd.toml.md.

[profnandaa]

Testing with an existing docker-users group that the non-admin user belongs to, works well!

buildkitd --group="<deduct>\docker-users"

whoami /groups
...
....
<deduct>\docker-users                                        Alias            S-1-5-21-xxx-xxx-3751290628-1002

> [Breakpoint 1] main.newGRPCListeners() C:/dev/container-core/buildkit/cmd/buildkitd/main.go:400 (hits goroutine(1):1 total:1) (PC: 0x1fdf030)
   395:         tlsConfig, err := serverCredentials(cfg.TLS)
   396:         if err != nil {
   397:                 return nil, err
   398:         }
   399:
=> 400:         sd := cfg.SecurityDescriptor
   401:         if sd == "" {
   402:                 sd, err = groupToSecurityDescriptor("")
   403:                 if err != nil {
   404:                         return nil, err
   405:                 }
(dlv) p cfg.SecurityDescriptor
"D:P(A;;GA;;;BA)(A;;GA;;;SY)(A;;GRGW;;;S-1-5-21-xxx-xxx-3751290628-1002)"

And now run buildctl from a non-admin terminal.

nit: this will need a minor update to include "or named pipe":

buildkit/cmd/buildkitd/main.go

Line 184 in 5152118

Usage: "group (name or gid) which will own all Unix socket listening addresses",

I can make that change as part of the documentation update.

[colinhemmings]

@gabriel-samfira @profnandaa It works for me will the --group flag as well. Including multiple groups.

[tonistiigi]

Although it doesn't allow me to specify multiple groups, not a blocker

Comma separated works for this case