From 058730d0f632f87ae1145201878c197f8873ca32 Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Tue, 11 Jun 2024 11:13:00 +0100 Subject: [PATCH] fix: ensure nested frontend builds get secret translation (#7595) * fix: ensure nested frontend builds get secret translation When doing a nested frontend build - i.e. when dockerfile.v0 calls gateway.v0 (triggered by a syntax directive), we need to ensure that the second frontend gets the same secret translation as each prior layer. To do this, we heavily refactor the filtering gateway and essentially have it pass itself into the frontend Solve - this propagates the filtering all the way down, so secrets can be correctly accessed. Signed-off-by: Justin Chadwell * tests: add test for remote frontend Signed-off-by: Justin Chadwell * chore: add changelog Signed-off-by: Justin Chadwell --------- Signed-off-by: Justin Chadwell --- .../unreleased/Fixed-20240610-162518.yaml | 6 + core/integration/container_test.go | 36 ++-- engine/buildkit/client.go | 169 +++++++++--------- 3 files changed, 120 insertions(+), 91 deletions(-) create mode 100644 .changes/unreleased/Fixed-20240610-162518.yaml diff --git a/.changes/unreleased/Fixed-20240610-162518.yaml b/.changes/unreleased/Fixed-20240610-162518.yaml new file mode 100644 index 000000000..2d12c6533 --- /dev/null +++ b/.changes/unreleased/Fixed-20240610-162518.yaml @@ -0,0 +1,6 @@ +kind: Fixed +body: 'core: handle secrets in dockerfile builds with syntax directives' +time: 2024-06-10T16:25:18.328274656+01:00 +custom: + Author: jedevc + PR: "7595" diff --git a/core/integration/container_test.go b/core/integration/container_test.go index ce2ee50c1..cf091a8c4 100644 --- a/core/integration/container_test.go +++ b/core/integration/container_test.go @@ -240,23 +240,37 @@ CMD echo "stage2" t.Run("with build secrets", func(t *testing.T) { sec := c.SetSecret("my-secret", "barbar") - src := contextDir. - WithNewFile("Dockerfile", - `FROM golang:1.18.2-alpine + dockerfile := `FROM golang:1.18.2-alpine WORKDIR /src RUN --mount=type=secret,id=my-secret,required=true test "$(cat /run/secrets/my-secret)" = "barbar" RUN --mount=type=secret,id=my-secret,required=true cp /run/secrets/my-secret /secret -CMD cat /secret -`) +CMD cat /secret && (cat /secret | tr "[a-z]" "[A-Z]") +` - stdout, err := c.Container().Build(src, dagger.ContainerBuildOpts{ - Secrets: []*dagger.Secret{sec}, - }).Stdout(ctx) - require.NoError(t, err) - require.Contains(t, stdout, "***") + t.Run("builtin frontend", func(t *testing.T) { + src := contextDir.WithNewFile("Dockerfile", dockerfile) + + stdout, err := c.Container().Build(src, dagger.ContainerBuildOpts{ + Secrets: []*dagger.Secret{sec}, + }).Stdout(ctx) + require.NoError(t, err) + require.Contains(t, stdout, "***") + require.Contains(t, stdout, "BARBAR") + }) + + t.Run("remote frontend", func(t *testing.T) { + src := contextDir.WithNewFile("Dockerfile", "#syntax=docker/dockerfile:1\n"+dockerfile) + + stdout, err := c.Container().Build(src, dagger.ContainerBuildOpts{ + Secrets: []*dagger.Secret{sec}, + }).Stdout(ctx) + require.NoError(t, err) + require.Contains(t, stdout, "***") + require.Contains(t, stdout, "BARBAR") + }) }) - t.Run("with input build secrets", func(t *testing.T) { + t.Run("prevent duplicate secret transform", func(t *testing.T) { sec := c.SetSecret("my-secret", "barbar") // src is a directory that has a secret dependency in it's build graph diff --git a/engine/buildkit/client.go b/engine/buildkit/client.go index 6a184aecf..b82d0e932 100644 --- a/engine/buildkit/client.go +++ b/engine/buildkit/client.go @@ -311,52 +311,14 @@ func (c *Client) Solve(ctx context.Context, req bkgw.SolveRequest) (_ *Result, r // include upstream cache imports, if any req.CacheImports = c.UpstreamCacheImports - // include exec metadata that isn't included in the cache key - var llbRes *bkfrontend.Result - switch { - case req.Definition != nil && req.Definition.Def != nil: - llbRes, err = c.llbBridge.Solve(ctx, req, c.ID()) - if err != nil { - return nil, wrapError(ctx, err, c.ID()) - } - case req.Frontend != "": - // HACK: don't force evaluation like this, we can write custom frontend - // wrappers (for dockerfile.v0 and gateway.v0) that read from ctx to - // replace the llbBridge it knows about. - // This current implementation may be limited when it comes to - // implement provenance/etc. - - f, ok := c.Frontends[req.Frontend] - if !ok { - return nil, fmt.Errorf("invalid frontend: %s", req.Frontend) - } - - gw := newFilterGateway(c.llbBridge, req) - gw.secretTranslator = ctx.Value("secret-translator").(func(string) (string, error)) - - llbRes, err = f.Solve( - ctx, - gw, - c.worker, // also implements Executor - req.FrontendOpt, - req.FrontendInputs, - c.ID(), - c.SessionManager, - ) - if err != nil { - return nil, err - } - if req.Evaluate { - err = llbRes.EachRef(func(ref bksolver.ResultProxy) error { - _, err := ref.Result(ctx) - return err - }) - if err != nil { - return nil, err - } - } - default: - llbRes = &bkfrontend.Result{} + // handle secret translation + gw := newFilterGateway(c, req) + if v := ctx.Value("secret-translator"); v != nil { + gw.secretTranslator = v.(func(string) (string, error)) + } + llbRes, err := gw.Solve(ctx, req, c.ID()) + if err != nil { + return nil, wrapError(ctx, err, c.ID()) } res, err := solverresult.ConvertResult(llbRes, func(rp bksolver.ResultProxy) (*ref, error) { @@ -791,12 +753,15 @@ type filteringGateway struct { // in the secret store. secretTranslator func(string) (string, error) + // client is the top-most client that is owning the filtering process + client *Client + // skipInputs specifies op digests that were part of the request inputs and // so shouldn't be processed. skipInputs map[digest.Digest]struct{} } -func newFilterGateway(bridge bkfrontend.FrontendLLBBridge, req bkgw.SolveRequest) *filteringGateway { +func newFilterGateway(client *Client, req bkgw.SolveRequest) *filteringGateway { inputs := map[digest.Digest]struct{}{} for _, inp := range req.FrontendInputs { for _, def := range inp.Def { @@ -805,54 +770,98 @@ func newFilterGateway(bridge bkfrontend.FrontendLLBBridge, req bkgw.SolveRequest } return &filteringGateway{ - FrontendLLBBridge: bridge, - skipInputs: inputs, + client: client, + FrontendLLBBridge: client.llbBridge, + + skipInputs: inputs, } } func (gw *filteringGateway) Solve(ctx context.Context, req bkfrontend.SolveRequest, sid string) (*bkfrontend.Result, error) { - if req.Definition != nil && req.Definition.Def != nil { - dag, err := DefToDAG(req.Definition) - if err != nil { - return nil, err - } - if err := dag.Walk(func(dag *OpDAG) error { - if _, ok := gw.skipInputs[*dag.OpDigest]; ok { - return SkipInputs + switch { + case req.Definition != nil && req.Definition.Def != nil: + if gw.secretTranslator != nil { + dag, err := DefToDAG(req.Definition) + if err != nil { + return nil, err } - execOp, ok := dag.AsExec() - if !ok { - return nil - } + if err := dag.Walk(func(dag *OpDAG) error { + if _, ok := gw.skipInputs[*dag.OpDigest]; ok { + return SkipInputs + } - for _, secret := range execOp.ExecOp.GetSecretenv() { - secret.ID, err = gw.secretTranslator(secret.ID) - if err != nil { - return err + execOp, ok := dag.AsExec() + if !ok { + return nil } - } - for _, mount := range execOp.ExecOp.GetMounts() { - if mount.MountType != bksolverpb.MountType_SECRET { - continue + + for _, secret := range execOp.ExecOp.GetSecretenv() { + secret.ID, err = gw.secretTranslator(secret.ID) + if err != nil { + return err + } } - secret := mount.SecretOpt - secret.ID, err = gw.secretTranslator(secret.ID) - if err != nil { - return err + for _, mount := range execOp.ExecOp.GetMounts() { + if mount.MountType != bksolverpb.MountType_SECRET { + continue + } + secret := mount.SecretOpt + secret.ID, err = gw.secretTranslator(secret.ID) + if err != nil { + return err + } } + return nil + }); err != nil { + return nil, err } - return nil - }); err != nil { - return nil, err + + newDef, err := dag.Marshal() + if err != nil { + return nil, err + } + req.Definition = newDef + } + + return gw.FrontendLLBBridge.Solve(ctx, req, sid) + + case req.Frontend != "": + // HACK: don't force evaluation like this, we can write custom frontend + // wrappers (for dockerfile.v0 and gateway.v0) that read from ctx to + // replace the llbBridge it knows about. + // This current implementation may be limited when it comes to + // implement provenance/etc. + + f, ok := gw.client.Frontends[req.Frontend] + if !ok { + return nil, fmt.Errorf("invalid frontend: %s", req.Frontend) } - newDef, err := dag.Marshal() + llbRes, err := f.Solve( + ctx, + gw, + gw.client.worker, // also implements Executor + req.FrontendOpt, + req.FrontendInputs, + sid, + gw.client.SessionManager, + ) if err != nil { return nil, err } - req.Definition = newDef - } + if req.Evaluate { + err = llbRes.EachRef(func(ref bksolver.ResultProxy) error { + _, err := ref.Result(ctx) + return err + }) + if err != nil { + return nil, err + } + } + return llbRes, nil - return gw.FrontendLLBBridge.Solve(ctx, req, sid) + default: + return &bkfrontend.Result{}, nil + } }