Fix ownership on artifact copies

Summary:
The implementation of D984 did not include enforcing ownership for
`build.CopyFrom` instruction and so artifacts copied from one image to
another via `copies:` were problematically owned as root.

In order to fix this behavior:
 1. `config.ArtifactConfig` `build.CopyFrom` instructions are now
    injected duration `build.PhaseInstall`
 2. `config.VariantConfig` calls `build.ApplyUser` for these artifact
    instructions as well using the `runs.as` user
 3. `build.CopyAs` was refactored to wrap any `build.Instruction` which
    should only really be used with `build.Copy` or `build.CopyFrom`.

Test Plan:
Run `go test ./...`. Run `blubber` against configuration with a variant that
uses `copies` and verify that the `COPY --from` instructions also include a
`--chown` flag.

Reviewers: thcipriani, mmodell, hashar, #release-engineering-team, demon

Reviewed By: thcipriani, #release-engineering-team

Tags: #release-engineering-team

Differential Revision: https://phabricator.wikimedia.org/D1002

10 files changed, 91 insertions, 41 deletions

diff --git a/build/instructions.go b/build/instructions.go
index 295221e..0167a8a 100644
--- a/build/instructions.go
+++ b/build/instructions.go
@@ -81,17 +81,20 @@ func (copy Copy) Compile() []string {
 // CopyAs is a concrete build instruction for copying source
 // files/directories and setting their ownership to the given UID/GID.
 //
+// While it can technically wrap any build.Instruction, it is meant to be used
+// with build.Copy and build.CopyFrom to enforce file/directory ownership.
+//
 type CopyAs struct {
 	UID uint // owner UID
 	GID uint // owner GID
-	Copy
+	Instruction
 }
 
 // Compile returns the variant name unquoted and all quoted CopyAs instruction
 // fields.
 //
 func (ca CopyAs) Compile() []string {
-	return append([]string{fmt.Sprintf("%d:%d", ca.UID, ca.GID)}, ca.Copy.Compile()...)
+	return append([]string{fmt.Sprintf("%d:%d", ca.UID, ca.GID)}, ca.Instruction.Compile()...)
 }
 
 // CopyFrom is a concrete build instruction for copying source
diff --git a/build/instructions_test.go b/build/instructions_test.go
index 29508e2..77938dd 100644
--- a/build/instructions_test.go
+++ b/build/instructions_test.go
@@ -37,9 +37,25 @@ func TestCopy(t *testing.T) {
 }
 
 func TestCopyAs(t *testing.T) {
-	i := build.CopyAs{123, 124, build.Copy{[]string{"source1", "source2"}, "dest"}}
-
-	assert.Equal(t, []string{"123:124", `"source1"`, `"source2"`, `"dest"`}, i.Compile())
+	t.Run("wrapping Copy", func(t *testing.T) {
+		i := build.CopyAs{
+			123,
+			124,
+			build.Copy{[]string{"source1", "source2"}, "dest"},
+		}
+
+		assert.Equal(t, []string{"123:124", `"source1"`, `"source2"`, `"dest"`}, i.Compile())
+	})
+
+	t.Run("wrapping CopyFrom", func(t *testing.T) {
+		i := build.CopyAs{
+			123,
+			124,
+			build.CopyFrom{"foo", build.Copy{[]string{"source1", "source2"}, "dest"}},
+		}
+
+		assert.Equal(t, []string{"123:124", "foo", `"source1"`, `"source2"`, `"dest"`}, i.Compile())
+	})
 }
 
 func TestCopyFrom(t *testing.T) {
diff --git a/build/macros.go b/build/macros.go
index 5d3422e..08556d1 100644
--- a/build/macros.go
+++ b/build/macros.go
@@ -11,9 +11,10 @@ func ApplyUser(uid uint, gid uint, instructions []Instruction) []Instruction {
 	applied := make([]Instruction, len(instructions))
 
 	for i, instruction := range instructions {
-		if copy, iscopy := instruction.(Copy); iscopy {
-			applied[i] = CopyAs{uid, gid, copy}
-		} else {
+		switch instruction.(type) {
+		case Copy, CopyFrom:
+			applied[i] = CopyAs{uid, gid, instruction}
+		default:
 			applied[i] = instruction
 		}
 	}
diff --git a/build/macros_test.go b/build/macros_test.go
index e47cf8d..c5066a6 100644
--- a/build/macros_test.go
+++ b/build/macros_test.go
@@ -12,12 +12,14 @@ func TestApplyUser(t *testing.T) {
 	instructions := []build.Instruction{
 		build.Copy{[]string{"foo"}, "bar"},
 		build.Copy{[]string{"baz"}, "qux"},
+		build.CopyFrom{"foo", build.Copy{[]string{"a"}, "b"}},
 	}
 
 	assert.Equal(t,
 		[]build.Instruction{
 			build.CopyAs{123, 223, build.Copy{[]string{"foo"}, "bar"}},
 			build.CopyAs{123, 223, build.Copy{[]string{"baz"}, "qux"}},
+			build.CopyAs{123, 223, build.CopyFrom{"foo", build.Copy{[]string{"a"}, "b"}}},
 		},
 		build.ApplyUser(123, 223, instructions),
 	)
diff --git a/config/artifacts.go b/config/artifacts.go
index 74efcab..9f23e42 100644
--- a/config/artifacts.go
+++ b/config/artifacts.go
@@ -23,14 +23,14 @@ type ArtifactsConfig struct {
 // InstructionsForPhase injects instructions into the given build phase that
 // copy configured artifacts.
 //
-// PhasePostInstall
+// PhaseInstall
 //
 // Injects build.CopyFrom instructions for the configured source and
 // destination paths.
 //
 func (ac ArtifactsConfig) InstructionsForPhase(phase build.Phase) []build.Instruction {
 	switch phase {
-	case build.PhasePostInstall:
+	case build.PhaseInstall:
 		return []build.Instruction{
 			build.CopyFrom{ac.From, build.Copy{[]string{ac.Source}, ac.Destination}},
 		}
diff --git a/config/artifacts_test.go b/config/artifacts_test.go
index b7ba07f..85bad02 100644
--- a/config/artifacts_test.go
+++ b/config/artifacts_test.go
@@ -60,15 +60,19 @@ func TestArtifactsConfigInstructions(t *testing.T) {
 		assert.Empty(t, cfg.InstructionsForPhase(build.PhasePreInstall))
 	})
 
-	t.Run("PhasePostInstall", func(t *testing.T) {
+	t.Run("PhaseInstall", func(t *testing.T) {
 		assert.Equal(t,
 			[]build.Instruction{build.CopyFrom{
 				"foo",
 				build.Copy{[]string{"/source/path"}, "/destination/path"},
 			}},
-			cfg.InstructionsForPhase(build.PhasePostInstall),
+			cfg.InstructionsForPhase(build.PhaseInstall),
 		)
 	})
+
+	t.Run("PhasePostInstall", func(t *testing.T) {
+		assert.Empty(t, cfg.InstructionsForPhase(build.PhasePostInstall))
+	})
 }
 
 func TestArtifactsConfigValidation(t *testing.T) {
diff --git a/config/variant.go b/config/variant.go
index e3562bf..85bb1a0 100644
--- a/config/variant.go
+++ b/config/variant.go
@@ -34,14 +34,9 @@ func (vc *VariantConfig) Merge(vc2 VariantConfig) {
 //
 func (vc *VariantConfig) InstructionsForPhase(phase build.Phase) []build.Instruction {
 	instructions := vc.CommonConfig.InstructionsForPhase(phase)
-	ainstructions := []build.Instruction{}
 
-	for _, artifact := range vc.allArtifacts() {
-		ainstructions = append(ainstructions, artifact.InstructionsForPhase(phase)...)
-	}
-
-	instructions = append(ainstructions, instructions...)
 	var switchUser string
+	var uid, gid uint
 
 	switch phase {
 	case build.PhasePrivileged:
@@ -49,12 +44,14 @@ func (vc *VariantConfig) InstructionsForPhase(phase build.Phase) []build.Instruc
 
 	case build.PhasePrivilegeDropped:
 		switchUser = vc.Lives.As
-		instructions = build.ApplyUser(vc.Lives.UID, vc.Lives.GID, instructions)
+		uid, gid = vc.Lives.UID, vc.Lives.GID
 
 	case build.PhasePreInstall:
-		instructions = build.ApplyUser(vc.Lives.UID, vc.Lives.GID, instructions)
+		uid, gid = vc.Lives.UID, vc.Lives.GID
 
 	case build.PhaseInstall:
+		uid, gid = vc.Lives.UID, vc.Lives.GID
+
 		if vc.Copies == "" {
 			if vc.SharedVolume.True {
 				instructions = append(instructions, build.Volume{vc.Lives.In})
@@ -63,17 +60,19 @@ func (vc *VariantConfig) InstructionsForPhase(phase build.Phase) []build.Instruc
 			}
 		}
 
-		instructions = build.ApplyUser(vc.Lives.UID, vc.Lives.GID, instructions)
-
 	case build.PhasePostInstall:
 		switchUser = vc.Runs.As
-		instructions = build.ApplyUser(vc.Runs.UID, vc.Runs.GID, instructions)
+		uid, gid = vc.Runs.UID, vc.Runs.GID
 
 		if len(vc.EntryPoint) > 0 {
 			instructions = append(instructions, build.EntryPoint{vc.EntryPoint})
 		}
 	}
 
+	for _, artifact := range vc.allArtifacts() {
+		instructions = append(instructions, artifact.InstructionsForPhase(phase)...)
+	}
+
 	if switchUser != "" {
 		instructions = append(
 			[]build.Instruction{
@@ -84,6 +83,10 @@ func (vc *VariantConfig) InstructionsForPhase(phase build.Phase) []build.Instruc
 		)
 	}
 
+	if uid != 0 {
+		instructions = build.ApplyUser(uid, gid, instructions)
+	}
+
 	return instructions
 }
 
diff --git a/config/variant_test.go b/config/variant_test.go
index f906e67..6a1f615 100644
--- a/config/variant_test.go
+++ b/config/variant_test.go
@@ -67,12 +67,6 @@ func TestVariantLoops(t *testing.T) {
 
 func TestVariantConfigInstructions(t *testing.T) {
 	t.Run("PhaseInstall", func(t *testing.T) {
-		t.Run("copies", func(t *testing.T) {
-			cfg := config.VariantConfig{Copies: "foo"}
-
-			assert.Empty(t, cfg.InstructionsForPhase(build.PhaseInstall))
-		})
-
 		t.Run("shared volume", func(t *testing.T) {
 			cfg := config.VariantConfig{}
 			cfg.Lives.In = "/srv/service"
@@ -98,9 +92,7 @@ func TestVariantConfigInstructions(t *testing.T) {
 				cfg.InstructionsForPhase(build.PhaseInstall),
 			)
 		})
-	})
 
-	t.Run("PhasePostInstall", func(t *testing.T) {
 		t.Run("for copies and artifacts", func(t *testing.T) {
 			cfg := config.VariantConfig{
 				Copies: "foo",
@@ -116,7 +108,7 @@ func TestVariantConfigInstructions(t *testing.T) {
 					build.CopyFrom{"foo", build.Copy{[]string{config.LocalLibPrefix}, config.LocalLibPrefix}},
 					build.CopyFrom{"build", build.Copy{[]string{"/foo/src"}, "/foo/dst"}},
 				},
-				cfg.InstructionsForPhase(build.PhasePostInstall),
+				cfg.InstructionsForPhase(build.PhaseInstall),
 			)
 		})
 
@@ -125,17 +117,29 @@ func TestVariantConfigInstructions(t *testing.T) {
 				Artifacts: []config.ArtifactsConfig{
 					{From: "build", Source: "/foo/src", Destination: "/foo/dst"},
 				},
-				CommonConfig: config.CommonConfig{Lives: config.LivesConfig{In: "/srv/service"}},
+				CommonConfig: config.CommonConfig{
+					Lives: config.LivesConfig{
+						In: "/srv/service",
+						UserConfig: config.UserConfig{
+							UID: 123,
+							GID: 223,
+						},
+					},
+				},
 			}
 
 			assert.Equal(t,
 				[]build.Instruction{
-					build.CopyFrom{"build", build.Copy{[]string{"/foo/src"}, "/foo/dst"}},
+					build.CopyAs{123, 223, build.Copy{[]string{"."}, "."}},
+					build.CopyAs{123, 223, build.CopyFrom{"build", build.Copy{[]string{"/foo/src"}, "/foo/dst"}}},
 				},
-				cfg.InstructionsForPhase(build.PhasePostInstall),
+				cfg.InstructionsForPhase(build.PhaseInstall),
 			)
 		})
 
+	})
+
+	t.Run("PhasePostInstall", func(t *testing.T) {
 		t.Run("with entrypoint", func(t *testing.T) {
 			cfg := config.VariantConfig{
 				CommonConfig: config.CommonConfig{
diff --git a/docker/instructions.go b/docker/instructions.go
index 8463b0e..56c04b9 100644
--- a/docker/instructions.go
+++ b/docker/instructions.go
@@ -26,7 +26,12 @@ func NewInstruction(bi build.Instruction) (Instruction, error) {
 
 		switch bi.(type) {
 		case build.CopyAs:
-			i.flags = []string{"chown"}
+			switch bi.(build.CopyAs).Instruction.(type) {
+			case build.Copy:
+				i.flags = []string{"chown"}
+			case build.CopyFrom:
+				i.flags = []string{"chown", "from"}
+			}
 		case build.CopyFrom:
 			i.flags = []string{"from"}
 		}
diff --git a/docker/instructions_test.go b/docker/instructions_test.go
index 6215841..07b71c7 100644
--- a/docker/instructions_test.go
+++ b/docker/instructions_test.go
@@ -42,13 +42,25 @@ func TestCopy(t *testing.T) {
 }
 
 func TestCopyAs(t *testing.T) {
-	i := build.CopyAs{123, 124, build.Copy{[]string{"foo1", "foo2"}, "bar"}}
+	t.Run("with Copy", func(t *testing.T) {
+		i := build.CopyAs{123, 124, build.Copy{[]string{"foo1", "foo2"}, "bar"}}
 
-	di, err := docker.NewInstruction(i)
+		di, err := docker.NewInstruction(i)
 
-	if assert.NoError(t, err) {
-		assert.Equal(t, "COPY --chown=123:124 [\"foo1\", \"foo2\", \"bar\"]\n", di.Compile())
-	}
+		if assert.NoError(t, err) {
+			assert.Equal(t, "COPY --chown=123:124 [\"foo1\", \"foo2\", \"bar\"]\n", di.Compile())
+		}
+	})
+
+	t.Run("with CopyFrom", func(t *testing.T) {
+		i := build.CopyAs{123, 124, build.CopyFrom{"foo", build.Copy{[]string{"foo1", "foo2"}, "bar"}}}
+
+		di, err := docker.NewInstruction(i)
+
+		if assert.NoError(t, err) {
+			assert.Equal(t, "COPY --chown=123:124 --from=foo [\"foo1\", \"foo2\", \"bar\"]\n", di.Compile())
+		}
+	})
 }
 
 func TestCopyFrom(t *testing.T) {