history: only add proxy vars to history if specified

`Buildkit/Docker` adds variables in pre-allowlist for e.g `proxy variables` to OCI/Docker history only if user explicitly specifies them in Dockerfile using `ARG`. By default variables in pre-allowlist e.g `proxy variables` will be used normally but will not be leaked into `docker/OCI` history of images. A test for following behviour is added with this commit and similar test can be verified against `Docker/Buildkit` Closes: containers#2937 Signed-off-by: Aditya R <[email protected]>
flouthoc · Feb 15, 2022 · 5bf42d5 · 5bf42d5
1 parent 5d93f6a
commit 5bf42d5
Show file tree

Hide file tree

Showing 2 changed files with 81 additions and 16 deletions.
diff --git a/imagebuildah/stage_executor.go b/imagebuildah/stage_executor.go
@@ -19,6 +19,7 @@ import (
 	"github.com/containers/buildah/pkg/parse"
 	"github.com/containers/buildah/pkg/rusage"
 	"github.com/containers/buildah/util"
+	config "github.com/containers/common/pkg/config"
 	cp "github.com/containers/image/v5/copy"
 	"github.com/containers/image/v5/docker/reference"
 	"github.com/containers/image/v5/manifest"
@@ -49,21 +50,22 @@ import (
 // If we're naming the result of the build, only the last stage will apply that
 // name to the image that it produces.
 type StageExecutor struct {
-	ctx             context.Context
-	executor        *Executor
-	log             func(format string, args ...interface{})
-	index           int
-	stages          imagebuilder.Stages
-	name            string
-	builder         *buildah.Builder
-	preserved       int
-	volumes         imagebuilder.VolumeSet
-	volumeCache     map[string]string
-	volumeCacheInfo map[string]os.FileInfo
-	mountPoint      string
-	output          string
-	containerIDs    []string
-	stage           *imagebuilder.Stage
+	ctx                   context.Context
+	executor              *Executor
+	log                   func(format string, args ...interface{})
+	index                 int
+	stages                imagebuilder.Stages
+	name                  string
+	builder               *buildah.Builder
+	preserved             int
+	volumes               imagebuilder.VolumeSet
+	volumeCache           map[string]string
+	volumeCacheInfo       map[string]os.FileInfo
+	mountPoint            string
+	output                string
+	containerIDs          []string
+	stage                 *imagebuilder.Stage
+	argsFromContainerfile []string
 }
 
 // Preserve informs the stage executor that from this point on, it needs to
@@ -1228,6 +1230,11 @@ func (s *StageExecutor) getCreatedBy(node *parser.Node, addedContentSummary stri
 	}
 	switch strings.ToUpper(node.Value) {
 	case "ARG":
+		for _, variable := range strings.Fields(node.Original) {
+			if variable != "ARG" {
+				s.argsFromContainerfile = append(s.argsFromContainerfile, variable)
+			}
+		}
 		buildArgs := s.getBuildArgsKey()
 		return "/bin/sh -c #(nop) ARG " + buildArgs
 	case "RUN":
@@ -1271,7 +1278,31 @@ func (s *StageExecutor) getBuildArgsResolvedForRun() string {
 			if inImage {
 				envs = append(envs, fmt.Sprintf("%s=%s", key, configuredEnvs[key]))
 			} else {
-				envs = append(envs, fmt.Sprintf("%s=%s", key, value))
+				// By default everything must be added to history.
+				// Following variable is configured to false only for special cases.
+				addToHistory := true
+
+				// Following value is being assigned from build-args,
+				// check if this key belongs to any of the predefined allowlist args e.g Proxy Variables
+				// and if that arg is not manually set in Containerfile/Dockerfile
+				// then don't write its value to history.
+				// Following behaviour ensures parity with docker/buildkit.
+				for _, variable := range config.ProxyEnv {
+					if key == variable {
+						// found in predefined args
+						// so don't add to history
+						// unless user did explicit `ARG <some-predefined-proxy-variable>`
+						addToHistory = false
+						for _, processedArg := range s.argsFromContainerfile {
+							if key == processedArg {
+								addToHistory = true
+							}
+						}
+					}
+				}
+				if addToHistory {
+					envs = append(envs, fmt.Sprintf("%s=%s", key, value))
+				}
 			}
 		}
 	}

diff --git a/tests/history.bats b/tests/history.bats
@@ -108,3 +108,37 @@ function testconfighistory() {
   run_buildah inspect --format '{{range .Docker.History}}{{println .CreatedBy}}{{end}}' runimg
   expect_output --substring "/bin/sh -c uname -a"
 }
+
+@test "history should not contain whitelisted vars unless set in ARG" {
+  _prefetch busybox
+  ctxdir=${TESTDIR}/bud
+  mkdir -p $ctxdir
+  cat >$ctxdir/Dockerfile <<EOF
+FROM busybox
+RUN echo \$HTTP_PROXY
+EOF
+
+  run_buildah build --signature-policy ${TESTSDIR}/policy.json -t test --build-arg HTTP_PROXY="helloworld" ${ctxdir}
+  expect_output --substring 'helloworld'
+  run_buildah inspect --format '{{range .Docker.History}}{{println .CreatedBy}}{{end}}' test
+  # history should not contain value for HTTP_PROXY since it was not in Containerfile
+  assert "$output" !~ 'HTTP_PROXY=helloworld'
+  assert "$output" !~ 'helloworld'
+}
+
+@test "history should contain whitelisted vars when set in ARG" {
+  _prefetch busybox
+  ctxdir=${TESTDIR}/bud
+  mkdir -p $ctxdir
+  cat >$ctxdir/Dockerfile <<EOF
+FROM busybox
+ARG HTTP_PROXY
+RUN echo \$HTTP_PROXY
+EOF
+
+  run_buildah build --signature-policy ${TESTSDIR}/policy.json -t test --build-arg HTTP_PROXY="helloworld" ${ctxdir}
+  expect_output --substring 'helloworld'
+  run_buildah inspect --format '{{range .Docker.History}}{{println .CreatedBy}}{{end}}' test
+  # history should not contain value for HTTP_PROXY since it was not in Containerfile
+  expect_output --substring 'HTTP_PROXY=helloworld'
+}