From 6dea667d20cb8a8ae58e3e1220780ec4d55a13cb Mon Sep 17 00:00:00 2001 From: Daniel J Walsh Date: Thu, 28 May 2020 06:28:36 -0400 Subject: [PATCH] Do not validate paths on remote platforms Modify validate functions to work on a remote clients. Any of the path checks will not work on remote machines or make sense on remote clients. Therefore they should not be checked. Signed-off-by: Daniel J Walsh --- Makefile | 18 ++++----- pkg/config/config.go | 61 ++++------------------------ pkg/config/config_local.go | 77 ++++++++++++++++++++++++++++++++++++ pkg/config/config_remote.go | 21 ++++++++++ pkg/config/config_unix.go | 15 ------- pkg/config/config_windows.go | 10 ----- pkg/config/default.go | 16 +++++--- 7 files changed, 126 insertions(+), 92 deletions(-) create mode 100644 pkg/config/config_local.go create mode 100644 pkg/config/config_remote.go delete mode 100644 pkg/config/config_unix.go delete mode 100644 pkg/config/config_windows.go diff --git a/Makefile b/Makefile index 46e85bba8..d89f0a90e 100644 --- a/Makefile +++ b/Makefile @@ -6,7 +6,7 @@ GO_BUILD=$(GO) build ifeq ($(shell go help mod >/dev/null 2>&1 && echo true), true) GO_BUILD=GO111MODULE=on $(GO) build -mod=vendor endif -BUILDTAGS := "" +BUILDTAGS := DESTDIR ?= PREFIX := /usr/local CONFIGDIR := ${PREFIX}/share/containers @@ -32,18 +32,18 @@ define go-get endef define go-build - GOOS=$(1) GOARCH=$(2) $(GO) build -tags $(BUILDTAGS) ./... + GOOS=$(1) GOARCH=$(2) $(GO) build -tags "$(3)" ./... endef .PHONY: build-cross: - $(call go-build,linux,386) - $(call go-build,linux,arm) - $(call go-build,linux,arm64) - $(call go-build,linux,ppc64le) - $(call go-build,linux,s390x) - $(call go-build,windows,amd64) - $(call go-build,windows,386) + $(call go-build,linux,386,${BUILDTAGS}) + $(call go-build,linux,arm,${BUILDTAGS}) + $(call go-build,linux,arm64,${BUILDTAGS}) + $(call go-build,linux,ppc64le,${BUILDTAGS}) + $(call go-build,linux,s390x,${BUILDTAGS}) + $(call go-build,windows,amd64,remote ${BUILDTAGS}) + $(call go-build,windows,386,remote ${BUILDTAGS}) .PHONY: all all: build-amd64 build-386 diff --git a/pkg/config/config.go b/pkg/config/config.go index e63aa9407..7e60de6b6 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -7,7 +7,6 @@ import ( "path/filepath" "strings" "sync" - "syscall" "github.com/BurntSushi/toml" "github.com/containers/common/pkg/capabilities" @@ -540,17 +539,8 @@ func (c *Config) Validate() error { // It returns an `error` on validation failure, otherwise // `nil`. func (c *EngineConfig) Validate() error { - // Relative paths can cause nasty bugs, because core paths we use could - // shift between runs (or even parts of the program - the OCI runtime - // uses a different working directory than we do, for example. - if c.StaticDir != "" && !filepath.IsAbs(c.StaticDir) { - return fmt.Errorf("static directory must be an absolute path - instead got %q", c.StaticDir) - } - if c.TmpDir != "" && !filepath.IsAbs(c.TmpDir) { - return fmt.Errorf("temporary directory must be an absolute path - instead got %q", c.TmpDir) - } - if c.VolumePath != "" && !filepath.IsAbs(c.VolumePath) { - return fmt.Errorf("volume path must be an absolute path - instead got %q", c.VolumePath) + if err := c.validatePaths(); err != nil { + return err } // Check if the pullPolicy from containers.conf is valid @@ -566,22 +556,13 @@ func (c *EngineConfig) Validate() error { // It returns an `error` on validation failure, otherwise // `nil`. func (c *ContainersConfig) Validate() error { - for _, u := range c.DefaultUlimits { - ul, err := units.ParseUlimit(u) - if err != nil { - return fmt.Errorf("unrecognized ulimit %s: %v", u, err) - } - _, err = ul.GetRlimit() - if err != nil { - return err - } + + if err := c.validateUlimits(); err != nil { + return err } - for _, d := range c.Devices { - _, _, _, err := Device(d) - if err != nil { - return err - } + if err := c.validateDevices(); err != nil { + return err } if c.LogSizeMax >= 0 && c.LogSizeMax < OCIBufSize { @@ -600,8 +581,7 @@ func (c *ContainersConfig) Validate() error { // execution checks. It returns an `error` on validation failure, otherwise // `nil`. func (c *NetworkConfig) Validate() error { - - if c.NetworkConfigDir != cniConfigDir { + if c.NetworkConfigDir != _cniConfigDir { err := isDirectory(c.NetworkConfigDir) if err != nil { return errors.Wrapf(err, "invalid network_config_dir: %s", c.NetworkConfigDir) @@ -803,31 +783,6 @@ func resolveHomeDir(path string) (string, error) { return strings.Replace(path, "~", home, 1), nil } -// isDirectory tests whether the given path exists and is a directory. It -// follows symlinks. -func isDirectory(path string) error { - path, err := resolveHomeDir(path) - if err != nil { - return err - } - - info, err := os.Stat(path) - if err != nil { - return err - } - - if !info.Mode().IsDir() { - // Return a PathError to be consistent with os.Stat(). - return &os.PathError{ - Op: "stat", - Path: path, - Err: syscall.ENOTDIR, - } - } - - return nil -} - func rootlessConfigPath() (string, error) { if configHome := os.Getenv("XDG_CONFIG_HOME"); configHome != "" { return filepath.Join(configHome, _configPath), nil diff --git a/pkg/config/config_local.go b/pkg/config/config_local.go new file mode 100644 index 000000000..db68cc347 --- /dev/null +++ b/pkg/config/config_local.go @@ -0,0 +1,77 @@ +// +build !remote + +package config + +import ( + "fmt" + "os" + "path/filepath" + "syscall" + + units "github.com/docker/go-units" +) + +// isDirectory tests whether the given path exists and is a directory. It +// follows symlinks. +func isDirectory(path string) error { + path, err := resolveHomeDir(path) + if err != nil { + return err + } + + info, err := os.Stat(path) + if err != nil { + return err + } + + if !info.Mode().IsDir() { + // Return a PathError to be consistent with os.Stat(). + return &os.PathError{ + Op: "stat", + Path: path, + Err: syscall.ENOTDIR, + } + } + + return nil +} + +func (c *EngineConfig) validatePaths() error { + // Relative paths can cause nasty bugs, because core paths we use could + // shift between runs or even parts of the program. - The OCI runtime + // uses a different working directory than we do, for example. + if c.StaticDir != "" && !filepath.IsAbs(c.StaticDir) { + return fmt.Errorf("static directory must be an absolute path - instead got %q", c.StaticDir) + } + if c.TmpDir != "" && !filepath.IsAbs(c.TmpDir) { + return fmt.Errorf("temporary directory must be an absolute path - instead got %q", c.TmpDir) + } + if c.VolumePath != "" && !filepath.IsAbs(c.VolumePath) { + return fmt.Errorf("volume path must be an absolute path - instead got %q", c.VolumePath) + } + return nil +} + +func (c *ContainersConfig) validateDevices() error { + for _, d := range c.Devices { + _, _, _, err := Device(d) + if err != nil { + return err + } + } + return nil +} + +func (c *ContainersConfig) validateUlimits() error { + for _, u := range c.DefaultUlimits { + ul, err := units.ParseUlimit(u) + if err != nil { + return fmt.Errorf("unrecognized ulimit %s: %v", u, err) + } + _, err = ul.GetRlimit() + if err != nil { + return err + } + } + return nil +} diff --git a/pkg/config/config_remote.go b/pkg/config/config_remote.go new file mode 100644 index 000000000..2dc938f68 --- /dev/null +++ b/pkg/config/config_remote.go @@ -0,0 +1,21 @@ +// +build remote + +package config + +// isDirectory tests whether the given path exists and is a directory. It +// follows symlinks. +func isDirectory(path string) error { + return nil +} + +func (c *EngineConfig) validatePaths() error { + return nil +} + +func (c *ContainersConfig) validateDevices() error { + return nil +} + +func (c *ContainersConfig) validateUlimits() error { + return nil +} diff --git a/pkg/config/config_unix.go b/pkg/config/config_unix.go deleted file mode 100644 index f270f2e95..000000000 --- a/pkg/config/config_unix.go +++ /dev/null @@ -1,15 +0,0 @@ -// +build !windows - -package config - -// Defaults for linux/unix if none are specified -const ( - cniConfigDir = "/etc/cni/net.d/" -) - -var cniBinDir = []string{ - "/usr/libexec/cni", - "/usr/lib/cni", - "/usr/local/lib/cni", - "/opt/cni/bin", -} diff --git a/pkg/config/config_windows.go b/pkg/config/config_windows.go deleted file mode 100644 index f6a6512a1..000000000 --- a/pkg/config/config_windows.go +++ /dev/null @@ -1,10 +0,0 @@ -// +build windows - -package config - -// Defaults for linux/unix if none are specified -const ( - cniConfigDir = "C:\\cni\\etc\\net.d\\" -) - -var cniBinDir = []string{"C:\\cni\\bin\\"} diff --git a/pkg/config/default.go b/pkg/config/default.go index 185ce8cee..055b135e2 100644 --- a/pkg/config/default.go +++ b/pkg/config/default.go @@ -53,9 +53,6 @@ var ( // DefaultDetachKeys is the default keys sequence for detaching a // container DefaultDetachKeys = "ctrl-p,ctrl-q" -) - -var ( // ErrConmonOutdated indicates the version of conmon found (whether via the configuration or $PATH) // is out of date for the current podman version ErrConmonOutdated = errors.New("outdated conmon version") @@ -80,15 +77,24 @@ var ( "CAP_SETUID", "CAP_SYS_CHROOT", } + + cniBinDir = []string{ + "/usr/libexec/cni", + "/usr/lib/cni", + "/usr/local/lib/cni", + "/opt/cni/bin", + } ) const ( - // EtcDir is the sysconfdir where podman should look for system config files. + // _etcDir is the sysconfdir where podman should look for system config files. // It can be overridden at build time. _etcDir = "/etc" // InstallPrefix is the prefix where podman will be installed. // It can be overridden at build time. _installPrefix = "/usr" + // _cniConfigDir is the directory where cni plugins are found + _cniConfigDir = "/etc/cni/net.d/" // CgroupfsCgroupsManager represents cgroupfs native cgroup manager CgroupfsCgroupsManager = "cgroupfs" // DefaultApparmorProfile specifies the default apparmor profile for the container. @@ -191,7 +197,7 @@ func DefaultConfig() (*Config, error) { }, Network: NetworkConfig{ DefaultNetwork: "podman", - NetworkConfigDir: cniConfigDir, + NetworkConfigDir: _cniConfigDir, CNIPluginDirs: cniBinDir, }, Engine: *defaultEngineConfig,