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
buildkitd: allow --group for windows #4875
Conversation
9bd01b3
to
833cc08
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
// Allow generic read and generic write access to authenticated users | ||
// and system users. On Linux, this pipe seems to be given rw access to | ||
// user, group and others (666). | ||
// TODO(gabriel-samfira): should we restrict access to this pipe to just | ||
// authenticated users? Or Administrators group? | ||
SecurityDescriptor: "D:P(A;;GRGW;;;AU)(A;;GRGW;;;SY)", | ||
secDescriptor = "D:P(A;;GRGW;;;AU)(A;;GRGW;;;SY)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
DebugAddress string `toml:"debugAddress"` | ||
UID *int `toml:"uid"` | ||
GID *int `toml:"gid"` | ||
SecurityDescriptor string `toml:"securityDescriptor"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will mostly expect users to use --group
flag.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
833cc08
to
68671bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@gabriel-samfira @profnandaa It works for me will the |
Comma separated works for this case |
This commit is a follow up of moby#4875, documenting the user of `--group` flag and `grpc.SecurityDescriptor` toml config. Also does a minor fix on the `help` for `--group` to specify Windows named pipe alongside Unix socket. Signed-off-by: Anthony Nandaa <profnandaa@gmail.com>
This commit is a follow up of moby#4875, documenting the user of `--group` flag and `grpc.SecurityDescriptor` toml config. Also does a minor fix on the `help` for `--group` to specify Windows named pipe alongside Unix socket. Signed-off-by: Anthony Nandaa <profnandaa@gmail.com>
This commit is a follow up of moby#4875, documenting the user of `--group` flag and `grpc.SecurityDescriptor` toml config. Also does a minor fix on the `help` for `--group` to specify Windows named pipe alongside Unix socket. Signed-off-by: Anthony Nandaa <profnandaa@gmail.com>
No description provided.