Address review comments:

A secret is defined with a tuple (id, target) which can be aliased as (source, destination) using the following format: --mount=type=secret,id=FOO,target=/path/to/secret Existing secret rules: * if source (id) is empty, it is assumed to equal to path.Base(target) * if target (destination) is empty, it is assumed to equal to "/run/secrets/" + path.Base(id) Add an additional attribute to the mount command to specify that the secret should be mounted as environment variable: --mount=type=secret,id=FOO,env=FOO with the following semantics: * it should allow the old behavior of mounting the secret as path (using the target/destination attribute) * it should be optional (e.g. with no 'env' specified, the secret should be mounted as a file following the existing rules) * if specified, the file mount should be optional (uses empty target/destination as indicator) * if should be possible to mount a secret as both - an environment variable and as a file Signed-off-by: a-palchikov <[email protected]>
moby · Aug 19, 2024 · 0fc5f16 · 0fc5f16
1 parent 4669adf
commit 0fc5f16
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 33 deletions.
diff --git a/client/llb/exec.go b/client/llb/exec.go
@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"net"
 	"sort"
+	"strings"
 
 	"github.com/moby/buildkit/solver/pb"
 	"github.com/moby/buildkit/util/system"
@@ -395,19 +396,21 @@ func (e *ExecOp) Marshal(ctx context.Context, c *Constraints) (digest.Digest, []
 				Optional: s.Optional,
 			})
 		}
-		pm := &pb.Mount{
-			Input:     pb.Empty,
-			Dest:      s.Target,
-			MountType: pb.MountType_SECRET,
-			SecretOpt: &pb.SecretOpt{
-				ID:       s.ID,
-				Uid:      uint32(s.UID),
-				Gid:      uint32(s.GID),
-				Optional: s.Optional,
-				Mode:     uint32(s.Mode),
-			},
+		if s.Target != nil {
+			pm := &pb.Mount{
+				Input:     pb.Empty,
+				Dest:      *s.Target,
+				MountType: pb.MountType_SECRET,
+				SecretOpt: &pb.SecretOpt{
+					ID:       s.ID,
+					Uid:      uint32(s.UID),
+					Gid:      uint32(s.GID),
+					Optional: s.Optional,
+					Mode:     uint32(s.Mode),
+				},
+			}
+			peo.Mounts = append(peo.Mounts, pm)
 		}
-		peo.Mounts = append(peo.Mounts, pm)
 	}
 
 	for _, s := range e.ssh {
@@ -679,7 +682,19 @@ type SSHInfo struct {
 // AddSecret is a RunOption that adds a secret to the exec.
 func AddSecret(dest string, opts ...SecretOption) RunOption {
 	return runOptionFunc(func(ei *ExecInfo) {
-		s := &SecretInfo{ID: dest, Target: dest, Mode: 0400}
+		s := &SecretInfo{ID: dest, Target: &dest, Mode: 0400}
+		for _, opt := range opts {
+			opt.SetSecretOption(s)
+		}
+		ei.Secrets = append(ei.Secrets, *s)
+	})
+}
+
+// AddSecretWithDest is a RunOption that adds a secret to the exec
+// with an optional destination.
+func AddSecretWithDest(src string, dest *string, opts ...SecretOption) RunOption {
+	return runOptionFunc(func(ei *ExecInfo) {
+		s := &SecretInfo{ID: src, Target: dest, Mode: 0400}
 		for _, opt := range opts {
 			opt.SetSecretOption(s)
 		}
@@ -698,8 +713,9 @@ func (fn secretOptionFunc) SetSecretOption(si *SecretInfo) {
 }
 
 type SecretInfo struct {
-	ID     string
-	Target string
+	ID string
+	// Target optionally specifies the target for the secret mount
+	Target *string
 	// Env optionally names the environment variable for the secret
 	Env      *string
 	Mode     int
@@ -725,7 +741,12 @@ func SecretAsEnv(v bool) SecretOption {
 			si.Env = nil
 			return
 		}
-		si.Env = &si.Target
+		if si.Target == nil {
+			return
+		}
+		target := strings.Clone(*si.Target)
+		si.Env = &target
+		si.Target = nil
 	})
 }
 

diff --git a/frontend/dockerfile/dockerfile2llb/convert_secrets.go b/frontend/dockerfile/dockerfile2llb/convert_secrets.go
@@ -22,9 +22,17 @@ func dispatchSecret(d *dispatchState, m *instructions.Mount, loc []parser.Range)
 		id = path.Base(m.Target)
 	}
 
-	target := m.Target
-	if target == "" {
-		target = "/run/secrets/" + path.Base(id)
+	var target *string
+	if m.Target != "" {
+		target = &m.Target
+	}
+
+	if m.Env == nil {
+		dest := m.Target
+		if dest == "" {
+			dest = "/run/secrets/" + path.Base(id)
+		}
+		target = &dest
 	}
 
 	if _, ok := d.outline.secrets[id]; !ok {
@@ -40,11 +48,7 @@ func dispatchSecret(d *dispatchState, m *instructions.Mount, loc []parser.Range)
 		opts = append(opts, llb.SecretOptional)
 	}
 	if m.Env != nil {
-		env := *m.Env
-		if env == "" {
-			env = path.Base(id)
-		}
-		opts = append(opts, llb.SecretAsEnvName(env))
+		opts = append(opts, llb.SecretAsEnvName(*m.Env))
 	}
 
 	if m.UID != nil || m.GID != nil || m.Mode != nil {
@@ -63,5 +67,5 @@ func dispatchSecret(d *dispatchState, m *instructions.Mount, loc []parser.Range)
 		opts = append(opts, llb.SecretFileOpt(uid, gid, mode))
 	}
 
-	return llb.AddSecret(target, opts...), nil
+	return llb.AddSecretWithDest(id, target, opts...), nil
 }
diff --git a/frontend/dockerfile/dockerfile_secrets_test.go b/frontend/dockerfile/dockerfile_secrets_test.go
@@ -17,7 +17,7 @@ var secretsTests = integration.TestFuncs(
 	testSecretFileParams,
 	testSecretRequiredWithoutValue,
 	testSecretAsEnviron,
-	testSecretAsEnvironWithOverride,
+	testSecretAsEnvironWithFileMount,
 )
 
 func init() {
@@ -89,7 +89,7 @@ func testSecretAsEnviron(t *testing.T, sb integration.Sandbox) {
 
 	dockerfile := []byte(`
 FROM busybox
-RUN --mount=type=secret,id=mysecret,env [ "$mysecret" == "pw" ] && [ -f /run/secrets/mysecret ] || false
+RUN --mount=type=secret,id=mysecret,env=SECRET_ENV [ "$SECRET_ENV" == "pw" ] && [ ! -f /run/secrets/mysecret ] || false
 `)
 
 	dir := integration.Tmpdir(
@@ -113,13 +113,13 @@ RUN --mount=type=secret,id=mysecret,env [ "$mysecret" == "pw" ] && [ -f /run/sec
 	require.NoError(t, err)
 }
 
-func testSecretAsEnvironWithOverride(t *testing.T, sb integration.Sandbox) {
+func testSecretAsEnvironWithFileMount(t *testing.T, sb integration.Sandbox) {
 	integration.SkipOnPlatform(t, "windows")
 	f := getFrontend(t, sb)
 
 	dockerfile := []byte(`
 FROM busybox
-RUN --mount=type=secret,id=mysecret,env=MY_SECRET [ "$MY_SECRET" == "pw" ] || false
+RUN --mount=type=secret,id=mysecret,target=/run/secrets/secret,env=SECRET_ENV [ "$SECRET_ENV" == "pw" ] && [ -f /run/secrets/secret ] || false
 `)
 
 	dir := integration.Tmpdir(

diff --git a/frontend/dockerfile/instructions/commands_runmount.go b/frontend/dockerfile/instructions/commands_runmount.go
@@ -139,7 +139,6 @@ func parseMount(val string, expander SingleWordExpander) (*Mount, error) {
 	m := &Mount{Type: MountTypeBind}
 
 	roAuto := true
-	envDefault := ""
 
 	for _, field := range fields {
 		key, value, ok := strings.Cut(field, "=")
@@ -158,9 +157,6 @@ func parseMount(val string, expander SingleWordExpander) (*Mount, error) {
 				m.ReadOnly = false
 				roAuto = false
 				continue
-			case "env":
-				m.Env = &envDefault
-				continue
 			case "required":
 				if m.Type == MountTypeSecret || m.Type == MountTypeSSH {
 					m.Required = true