fix(secrets): isolate lazy secret loading (#539)

executor/linux/build.go

@@ -13,6 +13,8 @@ import (
 	"golang.org/x/sync/errgroup"
 
 	"github.com/go-vela/types/constants"
+	"github.com/go-vela/types/library"
+	"github.com/go-vela/types/pipeline"
 	"github.com/go-vela/worker/internal/build"
 	context2 "github.com/go-vela/worker/internal/context"
 	"github.com/go-vela/worker/internal/image"
@@ -195,6 +197,9 @@ func (c *client) PlanBuild(ctx context.Context) error {
 		c.Secrets[secret.Name] = s
 	}
 
+	// escape newlines in secrets loaded on build_start
+	escapeNewlineSecrets(c.Secrets)
+
 	return nil
 }
 
@@ -538,6 +543,59 @@ func (c *client) ExecBuild(ctx context.Context) error {
 
 		_log.SetData([]byte(""))
 
+		// -------------- Lazy Loading Secrets [start] --------------
+		//
+		// this requires a small preface and brief description on
+		// how normal secrets make it into a container:
+		//
+		// 1. pull secrets
+		// 2. add them to the internal secrets map @ c.Secrets
+		// 3. call escapeNewlineSecrets() on c.Secrets
+		// 4. inject them into the container via injectSecrets()
+		// 5. call container.Substitute()
+		//
+		// 1-3 happens in PlanBuild. 4 and 5 happens in
+		// CreateStep and CreateService and for secrets added
+		// via plugin.
+		//
+		// it's important to call out that container.Substitute()
+		// can inadvertently(?) tweak the value of secrets,
+		// particularly multiline secrets and/or secrets with
+		// escaped newlines (for example). even worse, calling it
+		// multiple times on the same container can tweak
+		// them further. this is due to the json marshal/unmarshal
+		// dance that happens during the substitution process.
+		//
+		// we can't move .Substitute() here because other aspects
+		// of the build process depend on variables being
+		// substituted earlier.
+		//
+		// so, to ensure lazy loaded secrets get the same
+		// (mis)treatment and value (!) as regular secrets,
+		// we will do the following here:
+		//
+		//  1. create a temporary map for lazy loaded secrets
+		//  2. pull the lazy loaded secrets
+		//  3. add them to temporary map
+		//  4. call escapeNewlineSecrets() on temp map
+		//  5. IF there are no lazy secrets, we stop here
+		//  6. create a temporary copy of the step/container
+		//  7. remove all existing environment variables except
+		//     those needed for secret injection from the temp
+		//     copy of the step/container
+		//  8. inject the lazy loaded secrets into the
+		//     temp step/container
+		//  9. call .Substitute on the temp step/container
+		// 10. move the lazy loaded secrets over to the
+		//     actual step/container
+		//
+		// this will ensure the lazy loaded secrets return
+		// the same value as they would as regular secrets
+		// and also keep this process isolated to lazy secrets
+
+		// create a temporary map akin to c.Secrets
+		lazySecrets := make(map[string]*library.Secret)
+
 		// iterate through step secrets
 		for _, s := range _step.Secrets {
 			// iterate through each secret provided in the pipeline
@@ -585,19 +643,56 @@ func (c *client) ExecBuild(ctx context.Context) error {
 					return err
 				}
 
-				// add secret to the map
-				c.Secrets[secret.Name] = s
+				// add secret to the temp map
+				lazySecrets[secret.Name] = s
 			}
 		}
 
-		c.Logger.Debug("escaping newlines in secrets")
-		escapeNewlineSecrets(c.Secrets)
+		// if we had lazy secrets, get them into the container
+		if len(lazySecrets) > 0 {
+			// create a copy of the current step/container
+			tmpStep := new(pipeline.Container)
+			*tmpStep = *_step
 
-		// inject secrets for container
-		err = injectSecrets(_step, c.Secrets)
-		if err != nil {
-			return err
+			c.Logger.Debug("clearing environment in temp step/container")
+			// empty the environment
+			tmpStep.Environment = map[string]string{}
+			// but keep VELA_BUILD_EVENT as it's used in injectSecrets
+			if _, ok := _step.Environment["VELA_BUILD_EVENT"]; ok {
+				tmpStep.Environment["VELA_BUILD_EVENT"] = _step.Environment["VELA_BUILD_EVENT"]
+			}
+
+			c.Logger.Debug("escaping newlines in lazy loaded secrets")
+			// escape newlines for secrets loaded on step_start
+			escapeNewlineSecrets(lazySecrets)
+
+			c.Logger.Debug("injecting lazy loaded secrets")
+			// inject secrets for container
+			err = injectSecrets(tmpStep, lazySecrets)
+			if err != nil {
+				return err
+			}
+
+			c.Logger.Debug("substituting container configuration after lazy loaded secret injection")
+			// substitute container configuration
+			//
+			// https://pkg.go.dev/github.com/go-vela/types/pipeline#Container.Substitute
+			err = tmpStep.Substitute()
+			if err != nil {
+				return err
+			}
+
+			c.Logger.Debug("merge lazy loaded secrets into container")
+			// merge lazy load secrets into original container
+			err = _step.MergeEnv(tmpStep.Environment)
+			if err != nil {
+				return fmt.Errorf("failed to merge environment")
+			}
+
+			// clear out temporary var
+			tmpStep = nil
 		}
+		// -------------- Lazy Loading Secrets [end] --------------
 
 		c.Logger.Infof("planning %s step", _step.Name)
 		// plan the step

executor/linux/build_test.go

@@ -1139,6 +1139,20 @@ func TestLinux_PlanBuild(t *testing.T) {
 			runtime:  constants.DriverKubernetes,
 			pipeline: "testdata/build/stages/basic.yml",
 		},
+		{
+			name:     "docker-basic steps with lazy and regular secrets pipeline",
+			failure:  false,
+			logError: false,
+			runtime:  constants.DriverDocker,
+			pipeline: "testdata/build/steps/basic_secrets.yml",
+		},
+		{
+			name:     "kubernetes-basic steps with lazy and regular secrets pipeline",
+			failure:  false,
+			logError: false,
+			runtime:  constants.DriverKubernetes,
+			pipeline: "testdata/build/steps/basic_secrets.yml",
+		},
 	}
 
 	// run test
@@ -1621,6 +1635,20 @@ func TestLinux_ExecBuild(t *testing.T) {
 			runtime:  constants.DriverDocker,
 			pipeline: "testdata/build/steps/img_notfound.yml",
 		},
+		{
+			name:     "docker-basic steps with lazy and regular secrets pipeline",
+			failure:  false,
+			logError: false,
+			runtime:  constants.DriverDocker,
+			pipeline: "testdata/build/steps/basic_secrets.yml",
+		},
+		{
+			name:     "kubernetes-basic steps with lazy and regular secrets pipeline",
+			failure:  false,
+			logError: false,
+			runtime:  constants.DriverKubernetes,
+			pipeline: "testdata/build/steps/basic_secrets.yml",
+		},
 		//{
 		//	name:     "kubernetes-steps pipeline with image not found",
 		//	failure:  true, // FIXME: make Kubernetes mock simulate failure similar to Docker mock

executor/linux/service.go

@@ -39,9 +39,6 @@ func (c *client) CreateService(ctx context.Context, ctn *pipeline.Container) err
 		return err
 	}
 
-	logger.Debug("escaping newlines in secrets")
-	escapeNewlineSecrets(c.Secrets)
-
 	logger.Debug("injecting secrets")
 	// inject secrets for container
 	err = injectSecrets(ctn, c.Secrets)

executor/linux/step.go

@@ -49,9 +49,6 @@ func (c *client) CreateStep(ctx context.Context, ctn *pipeline.Container) error
 		return err
 	}
 
-	logger.Debug("escaping newlines in secrets")
-	escapeNewlineSecrets(c.Secrets)
-
 	logger.Debug("injecting secrets")
 	// inject secrets for container
 	err = injectSecrets(ctn, c.Secrets)

executor/linux/testdata/build/steps/basic_secrets.yml

@@ -0,0 +1,22 @@
+---
+version: "1"
+steps:
+  - name: test
+    commands:
+      - echo ${FOO}
+    environment:
+      FOO: bar
+    image: alpine:latest
+    pull: true
+    secrets: [ lazy, regular ]
+
+secrets:
+  - name: lazy
+    key: github/octocat/lazy
+    engine: native
+    type: repo
+    pull: step_start
+  - name: regular
+    key: github/octocat/regular
+    engine: native
+    type: repo