From ba72021274e1c555d444c0d866ae6d9548fc7129 Mon Sep 17 00:00:00 2001 From: localghost Date: Mon, 2 Oct 2017 22:03:42 +0200 Subject: [PATCH 1/3] Fix owner of files uploaded to docker container run as non-root. --- builder/docker/communicator.go | 88 ++++++++++++++++++++-- builder/docker/communicator_test.go | 113 ++++++++++++++++++++++++++++ 2 files changed, 195 insertions(+), 6 deletions(-) diff --git a/builder/docker/communicator.go b/builder/docker/communicator.go index 13684b2f8..bbfdb551f 100644 --- a/builder/docker/communicator.go +++ b/builder/docker/communicator.go @@ -18,12 +18,13 @@ import ( ) type Communicator struct { - ContainerID string - HostDir string - ContainerDir string - Version *version.Version - Config *Config - lock sync.Mutex + ContainerID string + HostDir string + ContainerDir string + Version *version.Version + Config *Config + containerUser *string + lock sync.Mutex } func (c *Communicator) Start(remote *packer.RemoteCmd) error { @@ -154,6 +155,10 @@ func (c *Communicator) uploadFile(dst string, src io.Reader, fi *os.FileInfo) er return fmt.Errorf("Failed to upload to '%s' in container: %s. %s.", dst, stderrOut, err) } + if err := c.fixDestinationOwner(dst); err != nil { + return err + } + return nil } @@ -207,6 +212,10 @@ func (c *Communicator) UploadDir(dst string, src string, exclude []string) error return fmt.Errorf("Failed to upload to '%s' in container: %s. %s.", dst, stderrOut, err) } + if err := c.fixDestinationOwner(dst); err != nil { + return err + } + return nil } @@ -310,3 +319,70 @@ func (c *Communicator) run(cmd *exec.Cmd, remote *packer.RemoteCmd, stdin io.Wri // Set the exit status which triggers waiters remote.SetExited(exitStatus) } + +// TODO Workaround for #5307. Remove once #5409 is fixed. +func (c *Communicator) fixDestinationOwner(destination string) error { + if c.containerUser == nil { + containerUser, err := c.discoverContainerUser() + if err != nil { + return err + } + c.containerUser = &containerUser + } + + if *c.containerUser != "" { + chownArgs := []string{ + "docker", "exec", "--user", "root", c.ContainerID, "/bin/sh", "-c", + fmt.Sprintf("chown -R %s %s", *c.containerUser, destination), + } + if _, err := c.runLocalCommand(chownArgs[0], chownArgs[1:]...); err != nil { + return fmt.Errorf("Failed to set owner of the uploaded file: %s", err) + } + } + + return nil +} + +func (c *Communicator) discoverContainerUser() (string, error) { + var err error + var stdout []byte + inspectArgs := []string{"docker", "inspect", "--format", "{{.Config.User}}", c.ContainerID} + if stdout, err = c.runLocalCommand(inspectArgs[0], inspectArgs[1:]...); err != nil { + return "", fmt.Errorf("Failed to inspect the container: %s", err) + } + return strings.TrimSpace(string(stdout)), nil +} + +func (c *Communicator) runLocalCommand(name string, arg ...string) (stdout []byte, err error) { + localCmd := exec.Command(name, arg...) + + stdoutP, err := localCmd.StdoutPipe() + if err != nil { + return nil, fmt.Errorf("failed to open stdout pipe, %s", err) + } + + stderrP, err := localCmd.StderrPipe() + if err != nil { + return nil, fmt.Errorf("failed to open stderr pipe, %s", err) + } + + if err = localCmd.Start(); err != nil { + return nil, fmt.Errorf("failed to start command, %s", err) + } + + stdout, err = ioutil.ReadAll(stdoutP) + if err != nil { + return nil, fmt.Errorf("failed to read from stdout pipe, %s", err) + } + + stderr, err := ioutil.ReadAll(stderrP) + if err != nil { + return nil, fmt.Errorf("failed to read from stderr pipe, %s", err) + } + + if err := localCmd.Wait(); err != nil { + return nil, fmt.Errorf("%s, %s", stderr, err) + } + + return stdout, nil +} diff --git a/builder/docker/communicator_test.go b/builder/docker/communicator_test.go index 0c98dbbb2..b5b5b1583 100644 --- a/builder/docker/communicator_test.go +++ b/builder/docker/communicator_test.go @@ -209,6 +209,82 @@ func TestLargeDownload(t *testing.T) { } +// TestUploadOwner verifies that owner of uploaded files is the user the container is running as. +func TestUploadOwner(t *testing.T) { + ui := packer.TestUi(t) + cache := &packer.FileCache{CacheDir: os.TempDir()} + + tpl, err := template.Parse(strings.NewReader(testUploadOwnerTemplate)) + if err != nil { + t.Fatalf("Unable to parse config: %s", err) + } + + if os.Getenv("PACKER_ACC") == "" { + t.Skip("This test is only run with PACKER_ACC=1") + } + cmd := exec.Command("docker", "-v") + cmd.Run() + if !cmd.ProcessState.Success() { + t.Error("docker command not found; please make sure docker is installed") + } + + // Setup the builder + builder := &Builder{} + warnings, err := builder.Prepare(tpl.Builders["docker"].Config) + if err != nil { + t.Fatalf("Error preparing configuration %s", err) + } + if len(warnings) > 0 { + t.Fatal("Encountered configuration warnings; aborting") + } + + // Setup the provisioners + fileProvisioner := &file.Provisioner{} + err = fileProvisioner.Prepare(tpl.Provisioners[0].Config) + if err != nil { + t.Fatalf("Error preparing single file upload provisioner: %s", err) + } + + dirProvisioner := &file.Provisioner{} + err = dirProvisioner.Prepare(tpl.Provisioners[1].Config) + if err != nil { + t.Fatalf("Error preparing directory upload provisioner: %s", err) + } + + shellProvisioner := &shell.Provisioner{} + err = shellProvisioner.Prepare(tpl.Provisioners[2].Config) + if err != nil { + t.Fatalf("Error preparing shell provisioner: %s", err) + } + + verifyProvisioner := &shell.Provisioner{} + err = verifyProvisioner.Prepare(tpl.Provisioners[3].Config) + if err != nil { + t.Fatalf("Error preparing verification provisioner: %s", err) + } + + // Add hooks so the provisioners run during the build + hooks := map[string][]packer.Hook{} + hooks[packer.HookProvision] = []packer.Hook{ + &packer.ProvisionHook{ + Provisioners: []packer.Provisioner{ + fileProvisioner, + dirProvisioner, + shellProvisioner, + verifyProvisioner, + }, + ProvisionerTypes: []string{"", "", "", ""}, + }, + } + hook := &packer.DispatchHook{Mapping: hooks} + + artifact, err := builder.Run(ui, hook, cache) + if err != nil { + t.Fatalf("Error running build %s", err) + } + defer artifact.Destroy() +} + const dockerBuilderConfig = ` { "builders": [ @@ -269,3 +345,40 @@ const dockerLargeBuilderConfig = ` ] } ` + +const testUploadOwnerTemplate = ` +{ + "builders": [ + { + "type": "docker", + "image": "ubuntu", + "discard": true, + "run_command": ["-d", "-i", "-t", "-u", "42", "{{.Image}}", "/bin/sh"] + } + ], + "provisioners": [ + { + "type": "file", + "source": "test-fixtures/onecakes/strawberry", + "destination": "/tmp/strawberry-cake" + }, + { + "type": "file", + "source": "test-fixtures/manycakes", + "destination": "/tmp/" + }, + { + "type": "shell", + "inline": "touch /tmp/testUploadOwner" + }, + { + "type": "shell", + "inline": [ + "[ $(stat -c %u /tmp/strawberry-cake) -eq 42 ] || (echo 'Invalid owner of /tmp/strawberry-cake' && exit 1)", + "[ $(stat -c %u /tmp/testUploadOwner) -eq 42 ] || (echo 'Invalid owner of /tmp/testUploadOwner' && exit 1)", + "find /tmp/manycakes | xargs -n1 -IFILE /bin/sh -c '[ $(stat -c %u FILE) -eq 42 ] || (echo \"Invalid owner of FILE\" && exit 1)'" + ] + } + ] +} +` From 5866d4ea24ba3c38f7ceb9b8d42fef92a7152ddc Mon Sep 17 00:00:00 2001 From: localghost Date: Tue, 10 Oct 2017 22:45:47 +0200 Subject: [PATCH 2/3] Move container user inspect to StepConnectDocker. --- builder/docker/communicator.go | 73 +++++---------------------- builder/docker/communicator_test.go | 8 +-- builder/docker/config.go | 31 +++++++----- builder/docker/step_connect_docker.go | 33 ++++++++++-- 4 files changed, 63 insertions(+), 82 deletions(-) diff --git a/builder/docker/communicator.go b/builder/docker/communicator.go index bbfdb551f..d15143172 100644 --- a/builder/docker/communicator.go +++ b/builder/docker/communicator.go @@ -23,7 +23,7 @@ type Communicator struct { ContainerDir string Version *version.Version Config *Config - containerUser *string + ContainerUser string lock sync.Mutex } @@ -322,67 +322,22 @@ func (c *Communicator) run(cmd *exec.Cmd, remote *packer.RemoteCmd, stdin io.Wri // TODO Workaround for #5307. Remove once #5409 is fixed. func (c *Communicator) fixDestinationOwner(destination string) error { - if c.containerUser == nil { - containerUser, err := c.discoverContainerUser() - if err != nil { - return err - } - c.containerUser = &containerUser + if !c.Config.FixUploadOwner { + return nil } - if *c.containerUser != "" { - chownArgs := []string{ - "docker", "exec", "--user", "root", c.ContainerID, "/bin/sh", "-c", - fmt.Sprintf("chown -R %s %s", *c.containerUser, destination), - } - if _, err := c.runLocalCommand(chownArgs[0], chownArgs[1:]...); err != nil { - return fmt.Errorf("Failed to set owner of the uploaded file: %s", err) - } + owner := c.ContainerUser + if owner == "" { + owner = "root" + } + + chownArgs := []string{ + "docker", "exec", "--user", "root", c.ContainerID, "/bin/sh", "-c", + fmt.Sprintf("chown -R %s %s", owner, destination), + } + if output, err := exec.Command(chownArgs[0], chownArgs[1:]...).CombinedOutput(); err != nil { + return fmt.Errorf("Failed to set owner of the uploaded file: %s, %s", err, output) } return nil } - -func (c *Communicator) discoverContainerUser() (string, error) { - var err error - var stdout []byte - inspectArgs := []string{"docker", "inspect", "--format", "{{.Config.User}}", c.ContainerID} - if stdout, err = c.runLocalCommand(inspectArgs[0], inspectArgs[1:]...); err != nil { - return "", fmt.Errorf("Failed to inspect the container: %s", err) - } - return strings.TrimSpace(string(stdout)), nil -} - -func (c *Communicator) runLocalCommand(name string, arg ...string) (stdout []byte, err error) { - localCmd := exec.Command(name, arg...) - - stdoutP, err := localCmd.StdoutPipe() - if err != nil { - return nil, fmt.Errorf("failed to open stdout pipe, %s", err) - } - - stderrP, err := localCmd.StderrPipe() - if err != nil { - return nil, fmt.Errorf("failed to open stderr pipe, %s", err) - } - - if err = localCmd.Start(); err != nil { - return nil, fmt.Errorf("failed to start command, %s", err) - } - - stdout, err = ioutil.ReadAll(stdoutP) - if err != nil { - return nil, fmt.Errorf("failed to read from stdout pipe, %s", err) - } - - stderr, err := ioutil.ReadAll(stderrP) - if err != nil { - return nil, fmt.Errorf("failed to read from stderr pipe, %s", err) - } - - if err := localCmd.Wait(); err != nil { - return nil, fmt.Errorf("%s, %s", stderr, err) - } - - return stdout, nil -} diff --git a/builder/docker/communicator_test.go b/builder/docker/communicator_test.go index b5b5b1583..bdfaef996 100644 --- a/builder/docker/communicator_test.go +++ b/builder/docker/communicator_test.go @@ -209,12 +209,12 @@ func TestLargeDownload(t *testing.T) { } -// TestUploadOwner verifies that owner of uploaded files is the user the container is running as. -func TestUploadOwner(t *testing.T) { +// TestFixUploadOwner verifies that owner of uploaded files is the user the container is running as. +func TestFixUploadOwner(t *testing.T) { ui := packer.TestUi(t) cache := &packer.FileCache{CacheDir: os.TempDir()} - tpl, err := template.Parse(strings.NewReader(testUploadOwnerTemplate)) + tpl, err := template.Parse(strings.NewReader(testFixUploadOwnerTemplate)) if err != nil { t.Fatalf("Unable to parse config: %s", err) } @@ -346,7 +346,7 @@ const dockerLargeBuilderConfig = ` } ` -const testUploadOwnerTemplate = ` +const testFixUploadOwnerTemplate = ` { "builders": [ { diff --git a/builder/docker/config.go b/builder/docker/config.go index 89d28c290..fee08929d 100644 --- a/builder/docker/config.go +++ b/builder/docker/config.go @@ -23,20 +23,21 @@ type Config struct { common.PackerConfig `mapstructure:",squash"` Comm communicator.Config `mapstructure:",squash"` - Author string - Changes []string - Commit bool - ContainerDir string `mapstructure:"container_dir"` - Discard bool - ExecUser string `mapstructure:"exec_user"` - ExportPath string `mapstructure:"export_path"` - Image string - Message string - Privileged bool `mapstructure:"privileged"` - Pty bool - Pull bool - RunCommand []string `mapstructure:"run_command"` - Volumes map[string]string + Author string + Changes []string + Commit bool + ContainerDir string `mapstructure:"container_dir"` + Discard bool + ExecUser string `mapstructure:"exec_user"` + ExportPath string `mapstructure:"export_path"` + Image string + Message string + Privileged bool `mapstructure:"privileged"` + Pty bool + Pull bool + RunCommand []string `mapstructure:"run_command"` + Volumes map[string]string + FixUploadOwner bool `mapstructure:"fix_upload_owner"` // This is used to login to dockerhub to pull a private base container. For // pushing to dockerhub, see the docker post-processors @@ -54,6 +55,8 @@ type Config struct { func NewConfig(raws ...interface{}) (*Config, []string, error) { c := new(Config) + c.FixUploadOwner = true + var md mapstructure.Metadata err := config.Decode(c, &config.DecodeOpts{ Metadata: &md, diff --git a/builder/docker/step_connect_docker.go b/builder/docker/step_connect_docker.go index 7a947d500..45cf2d25d 100644 --- a/builder/docker/step_connect_docker.go +++ b/builder/docker/step_connect_docker.go @@ -1,7 +1,10 @@ package docker import ( + "fmt" "github.com/mitchellh/multistep" + "os/exec" + "strings" ) type StepConnectDocker struct{} @@ -19,14 +22,21 @@ func (s *StepConnectDocker) Run(state multistep.StateBag) multistep.StepAction { return multistep.ActionHalt } + containerUser, err := getContainerUser(containerId) + if err != nil { + state.Put("error", err) + return multistep.ActionHalt + } + // Create the communicator that talks to Docker via various // os/exec tricks. comm := &Communicator{ - ContainerID: containerId, - HostDir: tempDir, - ContainerDir: config.ContainerDir, - Version: version, - Config: config, + ContainerID: containerId, + HostDir: tempDir, + ContainerDir: config.ContainerDir, + Version: version, + Config: config, + ContainerUser: containerUser, } state.Put("communicator", comm) @@ -34,3 +44,16 @@ func (s *StepConnectDocker) Run(state multistep.StateBag) multistep.StepAction { } func (s *StepConnectDocker) Cleanup(state multistep.StateBag) {} + +func getContainerUser(containerId string) (string, error) { + inspectArgs := []string{"docker", "inspect", "--format", "{{.Config.User}}", containerId} + stdout, err := exec.Command(inspectArgs[0], inspectArgs[1:]...).Output() + if err != nil { + errStr := fmt.Sprintf("Failed to inspect the container: %s", err) + if ee, ok := err.(*exec.ExitError); ok { + errStr = fmt.Sprintf("%s, %s", errStr, ee.Stderr) + } + return "", fmt.Errorf(errStr) + } + return strings.TrimSpace(string(stdout)), nil +} From fef5162b01f3afe427430fa00b7af85573701fd0 Mon Sep 17 00:00:00 2001 From: localghost Date: Thu, 12 Oct 2017 21:26:18 +0200 Subject: [PATCH 3/3] Add description of `fix_upload_owner` to documentation. --- website/source/docs/builders/docker.html.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/website/source/docs/builders/docker.html.md b/website/source/docs/builders/docker.html.md index a36ff9cb7..b64a5b959 100644 --- a/website/source/docs/builders/docker.html.md +++ b/website/source/docs/builders/docker.html.md @@ -211,6 +211,10 @@ You must specify (only) one of `commit`, `discard`, or `export_path`. - `container_dir` (string) - The directory inside container to mount temp directory from host server for work [file provisioner](/docs/provisioners/file.html). By default this is set to `/packer-files`. + +- `fix_upload_owner` (boolean) - If true, files uploaded to the container will + be owned by the user the container is running as. If false, the owner will depend + on the version of docker installed in the system. Defaults to true. ## Using the Artifact: Export