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

buildkitd: allow --group for windows #4875

Merged
merged 1 commit into from
Apr 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 5 additions & 4 deletions cmd/buildkitd/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,11 @@ type LogConfig struct {
}

type GRPCConfig struct {
Address []string `toml:"address"`
DebugAddress string `toml:"debugAddress"`
UID *int `toml:"uid"`
GID *int `toml:"gid"`
Address []string `toml:"address"`
DebugAddress string `toml:"debugAddress"`
UID *int `toml:"uid"`
GID *int `toml:"gid"`
SecurityDescriptor string `toml:"securityDescriptor"`
Copy link
Collaborator

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.

Copy link
Member Author

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.


TLS TLSConfig `toml:"tls"`
// MaxRecvMsgSize int `toml:"max_recv_message_size"`
Expand Down
35 changes: 28 additions & 7 deletions cmd/buildkitd/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"os"
"os/user"
"path/filepath"
"runtime"
"sort"
"strconv"
"strings"
Expand Down Expand Up @@ -395,9 +396,18 @@ func newGRPCListeners(cfg config.GRPCConfig) ([]net.Listener, error) {
if err != nil {
return nil, err
}

sd := cfg.SecurityDescriptor
if sd == "" {
sd, err = groupToSecurityDescriptor("")
if err != nil {
return nil, err
}
}

listeners := make([]net.Listener, 0, len(addrs))
for _, addr := range addrs {
l, err := getListener(addr, *cfg.UID, *cfg.GID, tlsConfig)
l, err := getListener(addr, *cfg.UID, *cfg.GID, sd, tlsConfig)
if err != nil {
for _, l := range listeners {
l.Close()
Expand Down Expand Up @@ -567,11 +577,19 @@ func applyMainFlags(c *cli.Context, cfg *config.Config) error {
}

if group := c.String("group"); group != "" {
gid, err := groupToGid(group)
if err != nil {
return err
if runtime.GOOS == "windows" {
secDescriptor, err := groupToSecurityDescriptor(group)
if err != nil {
return err
}
cfg.GRPC.SecurityDescriptor = secDescriptor
} else {
gid, err := groupToGid(group)
if err != nil {
return err
}
cfg.GRPC.GID = &gid
}
cfg.GRPC.GID = &gid
}

if tlscert := c.String("tlscert"); tlscert != "" {
Expand Down Expand Up @@ -626,7 +644,7 @@ func groupToGid(group string) (int, error) {
return id, nil
}

func getListener(addr string, uid, gid int, tlsConfig *tls.Config) (net.Listener, error) {
func getListener(addr string, uid, gid int, secDescriptor string, tlsConfig *tls.Config) (net.Listener, error) {
addrSlice := strings.SplitN(addr, "://", 2)
if len(addrSlice) < 2 {
return nil, errors.Errorf("address %s does not contain proto, you meant unix://%s ?",
Expand All @@ -639,6 +657,9 @@ func getListener(addr string, uid, gid int, tlsConfig *tls.Config) (net.Listener
if tlsConfig != nil {
bklog.L.Warnf("TLS is disabled for %s", addr)
}
if proto == "npipe" {
return getLocalListener(listenAddr, secDescriptor)
}
return sys.GetLocalListener(listenAddr, uid, gid)
case "fd":
return listenFD(listenAddr, tlsConfig)
Expand Down Expand Up @@ -907,7 +928,7 @@ func parseBoolOrAuto(s string) (*bool, error) {
func runTraceController(p string, exp sdktrace.SpanExporter) error {
server := grpc.NewServer()
tracev1.RegisterTraceServiceServer(server, &traceCollector{exporter: exp})
l, err := getLocalListener(p)
l, err := getLocalListener(p, "")
if err != nil {
return errors.Wrap(err, "creating trace controller listener")
}
Expand Down
6 changes: 5 additions & 1 deletion cmd/buildkitd/main_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func listenFD(addr string, tlsConfig *tls.Config) (net.Listener, error) {
return nil, errors.New("not supported yet")
}

func getLocalListener(listenerPath string) (net.Listener, error) {
func getLocalListener(listenerPath, _ string) (net.Listener, error) {
uid := os.Getuid()
l, err := sys.GetLocalListener(listenerPath, uid, uid)
if err != nil {
Expand All @@ -60,3 +60,7 @@ func getLocalListener(listenerPath string) (net.Listener, error) {
}
return l, nil
}

func groupToSecurityDescriptor(_ string) (string, error) {
return "", nil
}
26 changes: 23 additions & 3 deletions cmd/buildkitd/main_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ package main

import (
"crypto/tls"
"fmt"
"net"
"strings"

"github.com/Microsoft/go-winio"
_ "github.com/moby/buildkit/solver/llbsolver/ops"
Expand All @@ -19,14 +21,18 @@ func listenFD(_ string, _ *tls.Config) (net.Listener, error) {
return nil, errors.New("listening server on fd not supported on windows")
}

func getLocalListener(listenerPath string) (net.Listener, error) {
pc := &winio.PipeConfig{
func getLocalListener(listenerPath, secDescriptor string) (net.Listener, error) {
if secDescriptor == "" {
// 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)"
Copy link
Collaborator

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.

Copy link
Member Author

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).

Copy link
Collaborator

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.

}

pc := &winio.PipeConfig{
SecurityDescriptor: secDescriptor,
}

listener, err := winio.ListenPipe(listenerPath, pc)
Expand All @@ -35,3 +41,17 @@ func getLocalListener(listenerPath string) (net.Listener, error) {
}
return listener, nil
}

func groupToSecurityDescriptor(group string) (string, error) {
sddl := "D:P(A;;GA;;;BA)(A;;GA;;;SY)"
if group != "" {
for _, g := range strings.Split(group, ",") {
sid, err := winio.LookupSidByName(g)
if err != nil {
return "", errors.Wrapf(err, "failed to lookup sid for group %s", g)
}
sddl += fmt.Sprintf("(A;;GRGW;;;%s)", sid)
}
}
return sddl, nil
}