From 50c5793952a725b5629c5dcd82f26b92716e628a Mon Sep 17 00:00:00 2001 From: Dan Duvall Date: Fri, 9 Mar 2018 15:46:19 -0800 Subject: 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 --- build/instructions.go | 7 +++++-- build/instructions_test.go | 22 +++++++++++++++++++--- build/macros.go | 7 ++++--- build/macros_test.go | 2 ++ config/artifacts.go | 4 ++-- config/artifacts_test.go | 8 ++++++-- config/variant.go | 25 ++++++++++++++----------- config/variant_test.go | 28 ++++++++++++++++------------ docker/instructions.go | 7 ++++++- docker/instructions_test.go | 22 +++++++++++++++++----- 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) { -- cgit v1.2.1