From 3681b58d358b2ce992806c4215bb2e98d5d66be2 Mon Sep 17 00:00:00 2001 From: Akihiro Suda Date: Sat, 12 Oct 2019 23:05:01 +0900 Subject: [PATCH 1/6] buildkitd: disable TLS for UNIX sockets Fix #1199 Signed-off-by: Akihiro Suda (cherry picked from commit c239629fd9eadc2fbd8c9f8dbbdeccc7f11a0120) --- cmd/buildkitd/main.go | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/cmd/buildkitd/main.go b/cmd/buildkitd/main.go index 078d8467ece6..6ff3de66a98a 100644 --- a/cmd/buildkitd/main.go +++ b/cmd/buildkitd/main.go @@ -53,7 +53,6 @@ import ( "github.com/urfave/cli" "golang.org/x/sync/errgroup" "google.golang.org/grpc" - "google.golang.org/grpc/credentials" ) func init() { @@ -200,13 +199,6 @@ func main() { } } opts := []grpc.ServerOption{unaryInterceptor(ctx), grpc.StreamInterceptor(otgrpc.OpenTracingStreamServerInterceptor(tracer))} - creds, err := serverCredentials(cfg.GRPC.TLS) - if err != nil { - return err - } - if creds != nil { - opts = append(opts, creds) - } server := grpc.NewServer(opts...) // relative path does not work with nightlyone/lockfile @@ -298,10 +290,14 @@ func serveGRPC(cfg config.GRPCConfig, server *grpc.Server, errCh chan error) err if len(addrs) == 0 { return errors.New("--addr cannot be empty") } + tlsConfig, err := serverCredentials(cfg.TLS) + if err != nil { + return err + } eg, _ := errgroup.WithContext(context.Background()) listeners := make([]net.Listener, 0, len(addrs)) for _, addr := range addrs { - l, err := getListener(cfg, addr) + l, err := getListener(addr, cfg.UID, cfg.GID, tlsConfig) if err != nil { for _, l := range listeners { l.Close() @@ -490,7 +486,7 @@ func groupToGid(group string) (int, error) { return id, nil } -func getListener(cfg config.GRPCConfig, addr string) (net.Listener, error) { +func getListener(addr string, uid, gid int, 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 ?", @@ -499,11 +495,18 @@ func getListener(cfg config.GRPCConfig, addr string) (net.Listener, error) { proto := addrSlice[0] listenAddr := addrSlice[1] switch proto { - case "unix", "npipe": - return sys.GetLocalListener(listenAddr, cfg.UID, cfg.GID) + case "unix": + if tlsConfig != nil { + logrus.Warnf("TLS is disabled for %s", addr) + } + return sys.GetLocalListener(listenAddr, uid, gid) case "tcp": - return sockets.NewTCPSocket(listenAddr, nil) + if tlsConfig == nil { + logrus.Warnf("TLS is not enabled for %s. enabling mutual TLS authentication is highly recommended", addr) + } + return sockets.NewTCPSocket(listenAddr, tlsConfig) default: + // TODO: support npipe (with TLS?) return nil, errors.Errorf("addr %s not supported", addr) } } @@ -531,7 +534,7 @@ func unaryInterceptor(globalCtx context.Context) grpc.ServerOption { }) } -func serverCredentials(cfg config.TLSConfig) (grpc.ServerOption, error) { +func serverCredentials(cfg config.TLSConfig) (*tls.Config, error) { certFile := cfg.Cert keyFile := cfg.Key caFile := cfg.CA @@ -565,8 +568,7 @@ func serverCredentials(cfg config.TLSConfig) (grpc.ServerOption, error) { tlsConf.ClientAuth = tls.RequireAndVerifyClientCert tlsConf.ClientCAs = certPool } - creds := grpc.Creds(credentials.NewTLS(tlsConf)) - return creds, nil + return tlsConf, nil } func newController(c *cli.Context, cfg *config.Config) (*control.Controller, error) { From 95552ec4cab1f015a6cb62ae992be6e6dbb5cbf1 Mon Sep 17 00:00:00 2001 From: Troels Liebe Bentsen Date: Wed, 20 Nov 2019 00:09:37 +0100 Subject: [PATCH 2/6] Inherit extended agent so we get modern sign hashes Signed-off-by: Troels Liebe Bentsen (cherry picked from commit 4e7904f37ea8275d894a245d35319dc8cbb2549f) --- session/sshforward/sshprovider/agentprovider.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/session/sshforward/sshprovider/agentprovider.go b/session/sshforward/sshprovider/agentprovider.go index 7aa3e3dfc255..72e55c674475 100644 --- a/session/sshforward/sshprovider/agentprovider.go +++ b/session/sshforward/sshprovider/agentprovider.go @@ -178,7 +178,7 @@ type sock struct { } type readOnlyAgent struct { - agent.Agent + agent.ExtendedAgent } func (a *readOnlyAgent) Add(_ agent.AddedKey) error { From 991002d6db4155d477382735e6dda3d1ec972e1c Mon Sep 17 00:00:00 2001 From: Troels Liebe Bentsen Date: Thu, 21 Nov 2019 21:52:36 +0100 Subject: [PATCH 3/6] Disallow Extensions for now Signed-off-by: Troels Liebe Bentsen (cherry picked from commit 9c70fb99270ffc9faf9e833f74b0ec4a5fee01e0) --- session/sshforward/sshprovider/agentprovider.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/session/sshforward/sshprovider/agentprovider.go b/session/sshforward/sshprovider/agentprovider.go index 72e55c674475..4f569799f7b9 100644 --- a/session/sshforward/sshprovider/agentprovider.go +++ b/session/sshforward/sshprovider/agentprovider.go @@ -196,3 +196,7 @@ func (a *readOnlyAgent) RemoveAll() error { func (a *readOnlyAgent) Lock(_ []byte) error { return errors.Errorf("locking agent not allowed by buildkit") } + +func (a *readOnlyAgent) Extension(_ string, _ []byte) ([]byte, error) { + return nil, errors.Errorf("extensions not allowed by buildkit") +} From c9bb45ad96b016203904c35893a3bf24b7b1fcb3 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Tue, 14 Jan 2020 11:18:59 -0800 Subject: [PATCH 4/6] solver: avoid panic on combined cache load Signed-off-by: Tonis Tiigi (cherry picked from commit 7fc7f6dbf69a9811550f9b9003e8ef686ef77443) --- solver/combinedcache.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/solver/combinedcache.go b/solver/combinedcache.go index 07c494d1cb15..89361bcc04d9 100644 --- a/solver/combinedcache.go +++ b/solver/combinedcache.go @@ -87,6 +87,9 @@ func (cm *combinedCacheManager) Load(ctx context.Context, rec *CacheRecord) (res } } } + if len(results) == 0 { // TODO: handle gracefully + return nil, errors.Errorf("failed to load deleted cache") + } return results[0].Result, nil } From b8ec1faf5609326d878ce5b3ae97234c64b0e66a Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Mon, 13 Jan 2020 17:09:28 -0800 Subject: [PATCH 5/6] dockerfile: clear onbuild rules after triggering Signed-off-by: Tonis Tiigi (cherry picked from commit 393f388ed33445e20cd9817d209e911013785ca0) --- frontend/dockerfile/dockerfile2llb/convert.go | 5 +- frontend/dockerfile/dockerfile_test.go | 108 ++++++++++++++++++ 2 files changed, 111 insertions(+), 2 deletions(-) diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index bb21476c936f..cb6b2828f046 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -345,9 +345,10 @@ func Dockerfile2LLB(ctx context.Context, dt []byte, opt ConvertOpt) (*llb.State, opt.copyImage = DefaultCopyImage } - if err = dispatchOnBuild(d, d.image.Config.OnBuild, opt); err != nil { + if err = dispatchOnBuildTriggers(d, d.image.Config.OnBuild, opt); err != nil { return nil, nil, err } + d.image.Config.OnBuild = nil for _, cmd := range d.commands { if err := dispatch(d, cmd, opt); err != nil { @@ -586,7 +587,7 @@ type command struct { sources []*dispatchState } -func dispatchOnBuild(d *dispatchState, triggers []string, opt dispatchOpt) error { +func dispatchOnBuildTriggers(d *dispatchState, triggers []string, opt dispatchOpt) error { for _, trigger := range triggers { ast, err := parser.Parse(strings.NewReader(trigger)) if err != nil { diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go index e5b711229f81..ccff83d40e99 100644 --- a/frontend/dockerfile/dockerfile_test.go +++ b/frontend/dockerfile/dockerfile_test.go @@ -92,6 +92,7 @@ var allTests = []integration.Test{ testDefaultEnvWithArgs, testEnvEmptyFormatting, testCacheMultiPlatformImportExport, + testOnBuildCleared, } var fileOpTests = []integration.Test{ @@ -3462,6 +3463,113 @@ LABEL foo=bar require.Equal(t, "baz", v) } +func testOnBuildCleared(t *testing.T, sb integration.Sandbox) { + f := getFrontend(t, sb) + + registry, err := sb.NewRegistry() + if errors.Cause(err) == integration.ErrorRequirements { + t.Skip(err.Error()) + } + require.NoError(t, err) + + dockerfile := []byte(` +FROM busybox +ONBUILD RUN mkdir -p /out && echo -n 11 >> /out/foo +`) + + dir, err := tmpdir( + fstest.CreateFile("Dockerfile", dockerfile, 0600), + ) + require.NoError(t, err) + defer os.RemoveAll(dir) + + c, err := client.New(context.TODO(), sb.Address()) + require.NoError(t, err) + defer c.Close() + + target := registry + "/buildkit/testonbuild:base" + + _, err = f.Solve(context.TODO(), c, client.SolveOpt{ + Exports: []client.ExportEntry{ + { + Type: client.ExporterImage, + Attrs: map[string]string{ + "push": "true", + "name": target, + }, + }, + }, + LocalDirs: map[string]string{ + builder.DefaultLocalNameDockerfile: dir, + builder.DefaultLocalNameContext: dir, + }, + }, nil) + require.NoError(t, err) + + dockerfile = []byte(fmt.Sprintf(` + FROM %s + `, target)) + + dir, err = tmpdir( + fstest.CreateFile("Dockerfile", dockerfile, 0600), + ) + require.NoError(t, err) + defer os.RemoveAll(dir) + + target2 := registry + "/buildkit/testonbuild:child" + + _, err = f.Solve(context.TODO(), c, client.SolveOpt{ + Exports: []client.ExportEntry{ + { + Type: client.ExporterImage, + Attrs: map[string]string{ + "push": "true", + "name": target2, + }, + }, + }, + LocalDirs: map[string]string{ + builder.DefaultLocalNameDockerfile: dir, + builder.DefaultLocalNameContext: dir, + }, + }, nil) + require.NoError(t, err) + + dockerfile = []byte(fmt.Sprintf(` + FROM %s AS base + FROM scratch + COPY --from=base /out / + `, target2)) + + dir, err = tmpdir( + fstest.CreateFile("Dockerfile", dockerfile, 0600), + ) + require.NoError(t, err) + defer os.RemoveAll(dir) + + destDir, err := ioutil.TempDir("", "buildkit") + require.NoError(t, err) + defer os.RemoveAll(destDir) + + _, err = f.Solve(context.TODO(), c, client.SolveOpt{ + Exports: []client.ExportEntry{ + { + Type: client.ExporterLocal, + OutputDir: destDir, + }, + }, + LocalDirs: map[string]string{ + builder.DefaultLocalNameDockerfile: dir, + builder.DefaultLocalNameContext: dir, + }, + }, nil) + require.NoError(t, err) + + dt, err := ioutil.ReadFile(filepath.Join(destDir, "foo")) + require.NoError(t, err) + require.Equal(t, "11", string(dt)) +} + func testCacheMultiPlatformImportExport(t *testing.T, sb integration.Sandbox) { f := getFrontend(t, sb) From db97a47fcf2a5fbed122c28e4ffa13ab0ded8f1d Mon Sep 17 00:00:00 2001 From: Cory Bennett Date: Tue, 7 Jan 2020 14:35:17 -0800 Subject: [PATCH 6/6] ensure context is cancelled to prevent goroutine leaks from grpc.newClientStream Signed-off-by: Cory Bennett (cherry picked from commit 463fc8d1b8f41864baa9928b11c02fff36810e42) --- util/flightcontrol/flightcontrol.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/util/flightcontrol/flightcontrol.go b/util/flightcontrol/flightcontrol.go index 120be2f12555..f06d4e895499 100644 --- a/util/flightcontrol/flightcontrol.go +++ b/util/flightcontrol/flightcontrol.go @@ -116,7 +116,9 @@ func newCall(fn func(ctx context.Context) (interface{}, error)) *call { func (c *call) run() { defer c.closeProgressWriter() - v, err := c.fn(c.ctx) + ctx, cancel := context.WithCancel(c.ctx) + defer cancel() + v, err := c.fn(ctx) c.mu.Lock() c.result = v c.err = err