From 17ace935f34c2900d8718a55d262fce31b8d1a51 Mon Sep 17 00:00:00 2001
From: Aditya R <arajan@redhat.com>
Date: Tue, 15 Feb 2022 14:42:12 +0530
Subject: [PATCH] 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: https://github.com/containers/buildah/issues/2937

Signed-off-by: Aditya R <arajan@redhat.com>
---
 imagebuildah/stage_executor.go | 63 +++++++++++++++++++++++++---------
 tests/history.bats             | 34 ++++++++++++++++++
 2 files changed, 81 insertions(+), 16 deletions(-)

diff --git a/imagebuildah/stage_executor.go b/imagebuildah/stage_executor.go
index ac1068fbf49..be777a05c09 100644
--- 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 behviour.
+				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
index ee28895e8f7..99e17d4d652 100644
--- 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'
+}