Merge pull request moby#5336 from tonistiigi/dockerfile-secret-env-mask

dockerfile: mask usage of secret env in command name

Author: tonistiigi

frontend/dockerfile/dockerfile2llb/convert.go

@@ -1263,7 +1263,7 @@ func dispatchRun(d *dispatchState, c *instructions.RunCommand, proxy *llb.ProxyE
 		return err
 	}
 	env := getEnv(d.state)
-	opt = append(opt, llb.WithCustomName(prefixCommand(d, uppercaseCmd(processCmdEnv(&shlex, customname, env)), d.prefixPlatform, pl, env)))
+	opt = append(opt, llb.WithCustomName(prefixCommand(d, uppercaseCmd(processCmdEnv(&shlex, customname, withSecretEnvMask(c, env))), d.prefixPlatform, pl, env)))
 	for _, h := range dopt.extraHosts {
 		opt = append(opt, llb.AddExtraHost(h.Host, h.IP))
 	}

frontend/dockerfile/dockerfile2llb/convert_secrets.go

@@ -6,6 +6,7 @@ import (
 	"github.com/moby/buildkit/client/llb"
 	"github.com/moby/buildkit/frontend/dockerfile/instructions"
 	"github.com/moby/buildkit/frontend/dockerfile/parser"
+	"github.com/moby/buildkit/frontend/dockerfile/shell"
 	"github.com/pkg/errors"
 )
 
@@ -69,3 +70,52 @@ func dispatchSecret(d *dispatchState, m *instructions.Mount, loc []parser.Range)
 
 	return llb.AddSecretWithDest(id, target, opts...), nil
 }
+
+// withSecretEnvMask returns an EnvGetter that masks secret values in the environment.
+// This is not needed to hide actual secret values but to make it clear that the value is loaded from a secret.
+func withSecretEnvMask(c *instructions.RunCommand, env shell.EnvGetter) shell.EnvGetter {
+	ev := &llb.EnvList{}
+	set := false
+	mounts := instructions.GetMounts(c)
+	for _, mount := range mounts {
+		if mount.Type == instructions.MountTypeSecret {
+			if mount.Env != nil {
+				ev = ev.AddOrReplace(*mount.Env, "****")
+				set = true
+			}
+		}
+	}
+	if !set {
+		return env
+	}
+	return &secretEnv{
+		base: env,
+		env:  ev,
+	}
+}
+
+type secretEnv struct {
+	base shell.EnvGetter
+	env  *llb.EnvList
+}
+
+func (s *secretEnv) Get(key string) (string, bool) {
+	v, ok := s.env.Get(key)
+	if ok {
+		return v, true
+	}
+	return s.base.Get(key)
+}
+
+func (s *secretEnv) Keys() []string {
+	bkeys := s.base.Keys()
+	skeys := s.env.Keys()
+	keys := make([]string, 0, len(bkeys)+len(skeys))
+	for _, k := range bkeys {
+		if _, ok := s.env.Get(k); !ok {
+			keys = append(keys, k)
+		}
+	}
+	keys = append(keys, skeys...)
+	return keys
+}

frontend/dockerfile/dockerfile_secrets_test.go

@@ -1,14 +1,17 @@
 package dockerfile
 
 import (
+	"strings"
 	"testing"
+	"time"
 
 	"github.com/containerd/continuity/fs/fstest"
 	"github.com/moby/buildkit/client"
 	"github.com/moby/buildkit/frontend/dockerui"
 	"github.com/moby/buildkit/session"
 	"github.com/moby/buildkit/session/secrets/secretsprovider"
 	"github.com/moby/buildkit/util/testutil/integration"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/tonistiigi/fsutil"
 )
@@ -89,6 +92,7 @@ func testSecretAsEnviron(t *testing.T, sb integration.Sandbox) {
 
 	dockerfile := []byte(`
 FROM busybox
+ENV SECRET_ENV=foo
 RUN --mount=type=secret,id=mysecret,env=SECRET_ENV [ "$SECRET_ENV" == "pw" ] && [ ! -f /run/secrets/mysecret ] || false
 `)
 
@@ -101,6 +105,22 @@ RUN --mount=type=secret,id=mysecret,env=SECRET_ENV [ "$SECRET_ENV" == "pw" ] &&
 	require.NoError(t, err)
 	defer c.Close()
 
+	done := make(chan struct{})
+	status := make(chan *client.SolveStatus)
+	hasStatus := false
+
+	go func() {
+		for st := range status {
+			for _, v := range st.Vertexes {
+				if strings.Contains(v.Name, "/run/secrets/mysecret") {
+					hasStatus = true
+					assert.Contains(t, v.Name, `[ "****" == "pw" ] && `)
+				}
+			}
+		}
+		close(done)
+	}()
+
 	_, err = f.Solve(sb.Context(), c, client.SolveOpt{
 		LocalMounts: map[string]fsutil.FS{
 			dockerui.DefaultLocalNameDockerfile: dir,
@@ -109,8 +129,16 @@ RUN --mount=type=secret,id=mysecret,env=SECRET_ENV [ "$SECRET_ENV" == "pw" ] &&
 		Session: []session.Attachable{secretsprovider.FromMap(map[string][]byte{
 			"mysecret": []byte("pw"),
 		})},
-	}, nil)
+	}, status)
 	require.NoError(t, err)
+
+	select {
+	case <-done:
+	case <-time.After(10 * time.Second):
+		require.Fail(t, "timed out waiting for status")
+	}
+
+	require.True(t, hasStatus)
 }
 
 func testSecretAsEnvironWithFileMount(t *testing.T, sb integration.Sandbox) {