From cf6492383548b0e31bd7b45202ab73c7857ebea3 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Thu, 17 Aug 2017 11:06:49 -0700 Subject: [PATCH] filepath: Add a stand-alone package for explicit-OS path logic Go's path/filepath has lots of useful path operations, but there is a compile-time decision to select only the logic that applies to your $GOOS. That makes it hard to validate a Windows config from a Linux host (or vice versa) because Go's builtin tools won't tell you whether a path is absolute on the target platform (just whether the path is absolute on *your* platform). This commit adds a new package to do the same sorts of things but with an explicit OS argument. In some cases, there's also an explicit workding directory argument. For example, Go's Abs has [1]: If the path is not absolute it will be joined with the current working directory to turn it into an absolute path. but that doesn't make sense for a cross-platform Abs call because the real current working directory will be for the wrong platform. Instead, cross-platform calls to Abs and similar should fake a working directory as if they were being called from the other platform. The Windows implementation is not very complete, with IsAbs definitely missing a lot of stuff; Abs, Clean, and IsAncestor probably missing stuff; and a lack of Windows-path tests. But the current tools are broken for validating Windows configs anyway, so I've left completing Windows support to future work. The regular expression I've used is similar to what we used to use in pathValid, but has been adjusted based on [2]: The absolute path has the following format: LocalDrive:\Path\FileName I've assumed that c:\a is absolute as well, even though it doesn't quite match the above pattern. And I no longer test for colons, question marks, and other reserved characters [3], although it would be easy to add checks for that as well if we wanted. Besides adding the new package, I updated the config validation to use the new package where appropriate. For example checks for absolute hook paths (based on [4]) now appropriately account for the target platform (although Abs has limited Windows support at the moment, as mentioned above). There are still a number of config validation checks that use Go's stock filepath, because they're based around actual filesystem access (e.g. reading config.json off the disk, asserting that root.path exists on the disk, etc.). Some of those will need logic to convert between path platforms (which I'm leaving to future work). For example, if root.path is formed for another platform, then: * If root.path is absolute (on the target platform), there's no way to check whether root.path exists inside the bundle. * If root.path is relative, we should be converting it from the target platform to the host platform before joining it to our bundle path. For example, with a Windows bundle in "/foo" on a Linux host where root.path is "bar\baz", then runtime-tools should be checking for the root directory in /foo/bar/baz, not in /foo/bar\baz. The root.path example is somewhat guarded by the bundle requirement for siblinghood [5], but I'd rather enforce siblinghood independently from existence [6], since the spec has separate requirements for both. I'm not clear on why we were using pathValid for mount sources on Windows. We've been doing that since 4dcd4840 (validate: add mounts whether nested check, 2016-10-25, #256, [7]), but I see no absolute-path requirement in the spec [8], which only forbids UNC paths and mapped drives for source (which we don't yet have tooling to detect). Perhaps the idea was just to check for paths which contained reserved characters? In any case, I think source handling on Windows should not involve IsAbs and should be addressed more thoroughly in future work. [1]: https://golang.org/pkg/path/filepath/#Abs [2]: https://technet.microsoft.com/en-us/library/ee692607.aspx [3]: https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247%28v=vs.85%29.aspx#naming_conventions [4]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0/config.md#L375 [5]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0/bundle.md#L17-L19 [6]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0/config.md#L43 [7]: https://github.com/opencontainers/runtime-tools/pull/256/files#diff-8351286f57eb81e245c9c99c07f3b34fR413 [8]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#mounts Signed-off-by: W. Trevor King --- Makefile | 2 +- filepath/abs.go | 52 ++++++++++++ filepath/abs_test.go | 171 ++++++++++++++++++++++++++++++++++++++ filepath/ancestor.go | 32 +++++++ filepath/ancestor_test.go | 113 +++++++++++++++++++++++++ filepath/clean.go | 56 +++++++++++++ filepath/clean_test.go | 70 ++++++++++++++++ filepath/doc.go | 6 ++ filepath/join.go | 9 ++ filepath/separator.go | 9 ++ validate/validate.go | 98 ++++------------------ 11 files changed, 534 insertions(+), 84 deletions(-) create mode 100644 filepath/abs.go create mode 100644 filepath/abs_test.go create mode 100644 filepath/ancestor.go create mode 100644 filepath/ancestor_test.go create mode 100644 filepath/clean.go create mode 100644 filepath/clean_test.go create mode 100644 filepath/doc.go create mode 100644 filepath/join.go create mode 100644 filepath/separator.go diff --git a/Makefile b/Makefile index 77626d22c..02077b7d1 100644 --- a/Makefile +++ b/Makefile @@ -53,6 +53,6 @@ test: .gofmt .govet .golint .gotest .golint: golint -set_exit_status $(PACKAGES) -UTDIRS = ./validate/... +UTDIRS = ./filepath/... ./validate/... .gotest: go test $(UTDIRS) diff --git a/filepath/abs.go b/filepath/abs.go new file mode 100644 index 000000000..c19bba26a --- /dev/null +++ b/filepath/abs.go @@ -0,0 +1,52 @@ +package filepath + +import ( + "errors" + "regexp" + "strings" +) + +var windowsAbs = regexp.MustCompile(`^[a-zA-Z]:\\.*$`) + +// Abs is a version of path/filepath's Abs with an explicit operating +// system and current working directory. +func Abs(os, path, cwd string) (_ string, err error) { + if os == "windows" { + return "", errors.New("Abs() does not support windows yet") + } + if IsAbs(os, path) { + return Clean(os, path), nil + } + return Clean(os, Join(os, cwd, path)), nil +} + +// IsAbs is a version of path/filepath's IsAbs with an explicit +// operating system. +func IsAbs(os, path string) bool { + if os == "windows" { + // FIXME: copy hideous logic from Go's + // src/path/filepath/path_windows.go into somewhere where we can + // put 3-clause BSD licensed code. + return windowsAbs.MatchString(path) + } + sep := Separator(os) + + // POSIX has [1]: + // + // > If a pathname begins with two successive characters, + // > the first component following the leading characters + // > may be interpreted in an implementation-defined manner, + // > although more than two leading characters shall be + // > treated as a single character. + // + // And Boost treats // as non-absolute [2], but Linux [3,4], Python + // [5] and Go [6] all treat // as absolute. + // + // [1]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_13 + // [2]: https://github.com/boostorg/filesystem/blob/boost-1.64.0/test/path_test.cpp#L861 + // [3]: http://man7.org/linux/man-pages/man7/path_resolution.7.html + // [4]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/filesystems/path-lookup.md?h=v4.12#n41 + // [5]: https://github.com/python/cpython/blob/v3.6.1/Lib/posixpath.py#L64-L66 + // [6]: https://go.googlesource.com/go/+/go1.8.3/src/path/path.go#199 + return strings.HasPrefix(path, string(sep)) +} diff --git a/filepath/abs_test.go b/filepath/abs_test.go new file mode 100644 index 000000000..7e48c5fa2 --- /dev/null +++ b/filepath/abs_test.go @@ -0,0 +1,171 @@ +package filepath + +import ( + "fmt" + "testing" +) + +func TestAbs(t *testing.T) { + for _, test := range []struct { + os string + path string + cwd string + expected string + }{ + { + os: "linux", + path: "/", + cwd: "/cwd", + expected: "/", + }, + { + os: "linux", + path: "/a", + cwd: "/cwd", + expected: "/a", + }, + { + os: "linux", + path: "/a/", + cwd: "/cwd", + expected: "/a", + }, + { + os: "linux", + path: "//a", + cwd: "/cwd", + expected: "/a", + }, + { + os: "linux", + path: ".", + cwd: "/cwd", + expected: "/cwd", + }, + { + os: "linux", + path: "./c", + cwd: "/a/b", + expected: "/a/b/c", + }, + { + os: "linux", + path: ".//c", + cwd: "/a/b", + expected: "/a/b/c", + }, + { + os: "linux", + path: "../a", + cwd: "/cwd", + expected: "/a", + }, + { + os: "linux", + path: "../../b", + cwd: "/cwd", + expected: "/b", + }, + } { + t.Run( + fmt.Sprintf("Abs(%q,%q,%q)", test.os, test.path, test.cwd), + func(t *testing.T) { + abs, err := Abs(test.os, test.path, test.cwd) + if err != nil { + t.Error(err) + } else if abs != test.expected { + t.Errorf("unexpected result: %q (expected %q)\n", abs, test.expected) + } + }, + ) + } +} + +func TestIsAbs(t *testing.T) { + for _, test := range []struct { + os string + path string + expected bool + }{ + { + os: "linux", + path: "/", + expected: true, + }, + { + os: "linux", + path: "/a", + expected: true, + }, + { + os: "linux", + path: "//", + expected: true, + }, + { + os: "linux", + path: "//a", + expected: true, + }, + { + os: "linux", + path: ".", + expected: false, + }, + { + os: "linux", + path: "./a", + expected: false, + }, + { + os: "linux", + path: ".//a", + expected: false, + }, + { + os: "linux", + path: "../a", + expected: false, + }, + { + os: "linux", + path: "../../a", + expected: false, + }, + { + os: "windows", + path: "c:\\", + expected: true, + }, + { + os: "windows", + path: "c:\\a", + expected: true, + }, + { + os: "windows", + path: ".", + expected: false, + }, + { + os: "windows", + path: ".\\a", + expected: false, + }, + { + os: "windows", + path: "..\\a", + expected: false, + }, + } { + t.Run( + fmt.Sprintf("IsAbs(%q,%q)", test.os, test.path), + func(t *testing.T) { + abs := IsAbs(test.os, test.path) + if abs != test.expected { + t.Errorf("unexpected result: %t (expected %t)\n", abs, test.expected) + } + }, + ) + } +} diff --git a/filepath/ancestor.go b/filepath/ancestor.go new file mode 100644 index 000000000..896cd8206 --- /dev/null +++ b/filepath/ancestor.go @@ -0,0 +1,32 @@ +package filepath + +import ( + "fmt" + "strings" +) + +// IsAncestor returns true when pathB is an strict ancestor of pathA, +// and false where the paths are equal or pathB is outside of pathA. +// Paths that are not absolute will be made absolute with Abs. +func IsAncestor(os, pathA, pathB, cwd string) (_ bool, err error) { + if pathA == pathB { + return false, nil + } + + pathA, err = Abs(os, pathA, cwd) + if err != nil { + return false, err + } + pathB, err = Abs(os, pathB, cwd) + if err != nil { + return false, err + } + sep := Separator(os) + if !strings.HasSuffix(pathA, string(sep)) { + pathA = fmt.Sprintf("%s%c", pathA, sep) + } + if pathA == pathB { + return false, nil + } + return strings.HasPrefix(pathB, pathA), nil +} diff --git a/filepath/ancestor_test.go b/filepath/ancestor_test.go new file mode 100644 index 000000000..bcbe8345a --- /dev/null +++ b/filepath/ancestor_test.go @@ -0,0 +1,113 @@ +package filepath + +import ( + "fmt" + "testing" +) + +func TestIsAncestor(t *testing.T) { + for _, test := range []struct { + os string + pathA string + pathB string + cwd string + expected bool + }{ + { + os: "linux", + pathA: "/", + pathB: "/a", + cwd: "/cwd", + expected: true, + }, + { + os: "linux", + pathA: "/a", + pathB: "/a", + cwd: "/cwd", + expected: false, + }, + { + os: "linux", + pathA: "/a", + pathB: "/", + cwd: "/cwd", + expected: false, + }, + { + os: "linux", + pathA: "/a", + pathB: "/ab", + cwd: "/cwd", + expected: false, + }, + { + os: "linux", + pathA: "/a/", + pathB: "/a", + cwd: "/cwd", + expected: false, + }, + { + os: "linux", + pathA: "//a", + pathB: "/a", + cwd: "/cwd", + expected: false, + }, + { + os: "linux", + pathA: "//a", + pathB: "/a/b", + cwd: "/cwd", + expected: true, + }, + { + os: "linux", + pathA: "/a", + pathB: "/a/", + cwd: "/cwd", + expected: false, + }, + { + os: "linux", + pathA: "/a", + pathB: ".", + cwd: "/cwd", + expected: false, + }, + { + os: "linux", + pathA: "/a", + pathB: "b", + cwd: "/a", + expected: true, + }, + { + os: "linux", + pathA: "/a", + pathB: "../a", + cwd: "/cwd", + expected: false, + }, + { + os: "linux", + pathA: "/a", + pathB: "../a/b", + cwd: "/cwd", + expected: true, + }, + } { + t.Run( + fmt.Sprintf("IsAncestor(%q,%q,%q,%q)", test.os, test.pathA, test.pathB, test.cwd), + func(t *testing.T) { + ancestor, err := IsAncestor(test.os, test.pathA, test.pathB, test.cwd) + if err != nil { + t.Error(err) + } else if ancestor != test.expected { + t.Errorf("unexpected result: %t", ancestor) + } + }, + ) + } +} diff --git a/filepath/clean.go b/filepath/clean.go new file mode 100644 index 000000000..b70c575f2 --- /dev/null +++ b/filepath/clean.go @@ -0,0 +1,56 @@ +package filepath + +import ( + "fmt" + "strings" +) + +// Clean is an explicit-OS version of path/filepath's Clean. +func Clean(os, path string) string { + abs := IsAbs(os, path) + sep := Separator(os) + elements := strings.Split(path, string(sep)) + + // Replace multiple Separator elements with a single one. + for i := 0; i < len(elements); i++ { + if len(elements[i]) == 0 { + elements = append(elements[:i], elements[i+1:]...) + i-- + } + } + + // Eliminate each . path name element (the current directory). + for i := 0; i < len(elements); i++ { + if elements[i] == "." && len(elements) > 1 { + elements = append(elements[:i], elements[i+1:]...) + i-- + } + } + + // Eliminate each inner .. path name element (the parent directory) + // along with the non-.. element that precedes it. + for i := 1; i < len(elements); i++ { + if i > 0 && elements[i] == ".." { + elements = append(elements[:i-1], elements[i+1:]...) + i -= 2 + } + } + + // Eliminate .. elements that begin a rooted path: + // that is, replace "/.." by "/" at the beginning of a path, + // assuming Separator is '/'. + if abs && len(elements) > 0 { + for elements[0] == ".." { + elements = elements[1:] + } + } + + cleaned := strings.Join(elements, string(sep)) + if abs { + cleaned = fmt.Sprintf("%c%s", sep, cleaned) + } + if cleaned == path { + return path + } + return Clean(os, cleaned) +} diff --git a/filepath/clean_test.go b/filepath/clean_test.go new file mode 100644 index 000000000..b65e7c096 --- /dev/null +++ b/filepath/clean_test.go @@ -0,0 +1,70 @@ +package filepath + +import ( + "fmt" + "testing" +) + +func TestClean(t *testing.T) { + for _, test := range []struct { + os string + path string + expected string + }{ + { + os: "linux", + path: "/", + expected: "/", + }, + { + os: "linux", + path: "//", + expected: "/", + }, + { + os: "linux", + path: "/a", + expected: "/a", + }, + { + os: "linux", + path: "/a/", + expected: "/a", + }, + { + os: "linux", + path: "//a", + expected: "/a", + }, + { + os: "linux", + path: ".", + expected: ".", + }, + { + os: "linux", + path: "./c", + expected: "c", + }, + { + os: "linux", + path: ".././a", + expected: "../a", + }, + { + os: "linux", + path: "a/../b", + expected: "b", + }, + } { + t.Run( + fmt.Sprintf("Clean(%q,%q)", test.os, test.path), + func(t *testing.T) { + clean := Clean(test.os, test.path) + if clean != test.expected { + t.Errorf("unexpected result: %q (expected %q)\n", clean, test.expected) + } + }, + ) + } +} diff --git a/filepath/doc.go b/filepath/doc.go new file mode 100644 index 000000000..7ee085bf4 --- /dev/null +++ b/filepath/doc.go @@ -0,0 +1,6 @@ +// Package filepath implements Go's filepath package with explicit +// operating systems (and for some functions and explicit working +// directory). This allows tools built for one OS to operate on paths +// targeting another OS. For example, a Linux build can determine +// whether a path is absolute on Linux or on Windows. +package filepath diff --git a/filepath/join.go b/filepath/join.go new file mode 100644 index 000000000..b865d237c --- /dev/null +++ b/filepath/join.go @@ -0,0 +1,9 @@ +package filepath + +import "strings" + +// Join is an explicit-OS version of path/filepath's Join. +func Join(os string, elem ...string) string { + sep := Separator(os) + return Clean(os, strings.Join(elem, string(sep))) +} diff --git a/filepath/separator.go b/filepath/separator.go new file mode 100644 index 000000000..2c5e8905a --- /dev/null +++ b/filepath/separator.go @@ -0,0 +1,9 @@ +package filepath + +// Separator is an explicit-OS version of path/filepath's Separator. +func Separator(os string) rune { + if os == "windows" { + return '\\' + } + return '/' +} diff --git a/validate/validate.go b/validate/validate.go index bbad95a8b..1c8eacb68 100644 --- a/validate/validate.go +++ b/validate/validate.go @@ -10,7 +10,6 @@ import ( "os" "path/filepath" "reflect" - "regexp" "runtime" "strings" "syscall" @@ -20,6 +19,7 @@ import ( "github.com/blang/semver" "github.com/hashicorp/go-multierror" rspec "github.com/opencontainers/runtime-spec/specs-go" + osFilepath "github.com/opencontainers/runtime-tools/filepath" "github.com/sirupsen/logrus" "github.com/syndtr/gocapability/capability" @@ -202,18 +202,18 @@ func (v *Validator) CheckHooks() (errs error) { logrus.Debugf("check hooks") if v.spec.Hooks != nil { - errs = multierror.Append(errs, checkEventHooks("pre-start", v.spec.Hooks.Prestart, v.HostSpecific)) - errs = multierror.Append(errs, checkEventHooks("post-start", v.spec.Hooks.Poststart, v.HostSpecific)) - errs = multierror.Append(errs, checkEventHooks("post-stop", v.spec.Hooks.Poststop, v.HostSpecific)) + errs = multierror.Append(errs, v.checkEventHooks("prestart", v.spec.Hooks.Prestart, v.HostSpecific)) + errs = multierror.Append(errs, v.checkEventHooks("poststart", v.spec.Hooks.Poststart, v.HostSpecific)) + errs = multierror.Append(errs, v.checkEventHooks("poststop", v.spec.Hooks.Poststop, v.HostSpecific)) } return } -func checkEventHooks(hookType string, hooks []rspec.Hook, hostSpecific bool) (errs error) { - for _, hook := range hooks { - if !filepath.IsAbs(hook.Path) { - errs = multierror.Append(errs, fmt.Errorf("the %s hook %v: is not absolute path", hookType, hook.Path)) +func (v *Validator) checkEventHooks(hookType string, hooks []rspec.Hook, hostSpecific bool) (errs error) { + for i, hook := range hooks { + if !osFilepath.IsAbs(v.platform, hook.Path) { + errs = multierror.Append(errs, fmt.Errorf("hooks.%s[%d].path %v: is not absolute path", hookType, i, hook.Path)) } if hostSpecific { @@ -245,7 +245,7 @@ func (v *Validator) CheckProcess() (errs error) { } process := v.spec.Process - if !filepath.IsAbs(process.Cwd) { + if !osFilepath.IsAbs(v.platform, process.Cwd) { errs = multierror.Append(errs, fmt.Errorf("cwd %q is not an absolute path", process.Cwd)) } @@ -425,24 +425,15 @@ func (v *Validator) CheckMounts() (errs error) { if supportedTypes != nil && !supportedTypes[mountA.Type] { errs = multierror.Append(errs, fmt.Errorf("unsupported mount type %q", mountA.Type)) } - if v.platform == "windows" { - if err := pathValid(v.platform, mountA.Destination); err != nil { - errs = multierror.Append(errs, err) - } - if err := pathValid(v.platform, mountA.Source); err != nil { - errs = multierror.Append(errs, err) - } - } else { - if err := pathValid(v.platform, mountA.Destination); err != nil { - errs = multierror.Append(errs, err) - } + if !osFilepath.IsAbs(v.platform, mountA.Destination) { + errs = multierror.Append(errs, fmt.Errorf("mounts[%d].destination %q is not absolute", i, mountA.Destination)) } for j, mountB := range v.spec.Mounts { if i == j { continue } // whether B.Desination is nested within A.Destination - nested, err := nestedValid(v.platform, mountA.Destination, mountB.Destination) + nested, err := osFilepath.IsAncestor(v.platform, mountA.Destination, mountB.Destination, ".") if err != nil { errs = multierror.Append(errs, err) continue @@ -502,7 +493,7 @@ func (v *Validator) CheckLinux() (errs error) { for index := 0; index < len(v.spec.Linux.Namespaces); index++ { ns := v.spec.Linux.Namespaces[index] - if !namespaceValid(ns) { + if !v.namespaceValid(ns) { errs = multierror.Append(errs, fmt.Errorf("namespace %v is invalid", ns)) } @@ -834,7 +825,7 @@ func (v *Validator) rlimitValid(rlimit rspec.POSIXRlimit) (errs error) { return } -func namespaceValid(ns rspec.LinuxNamespace) bool { +func (v *Validator) namespaceValid(ns rspec.LinuxNamespace) bool { switch ns.Type { case rspec.PIDNamespace: case rspec.NetworkNamespace: @@ -847,72 +838,13 @@ func namespaceValid(ns rspec.LinuxNamespace) bool { return false } - if ns.Path != "" && !filepath.IsAbs(ns.Path) { + if ns.Path != "" && !osFilepath.IsAbs(v.platform, ns.Path) { return false } return true } -func pathValid(os, path string) error { - if os == "windows" { - matched, err := regexp.MatchString("^[a-zA-Z]:(\\\\[^\\\\/<>|:*?\"]+)+$", path) - if err != nil { - return err - } - if !matched { - return fmt.Errorf("invalid windows path %v", path) - } - return nil - } - if !filepath.IsAbs(path) { - return fmt.Errorf("%v is not an absolute path", path) - } - return nil -} - -// Check whether pathB is nested whithin pathA -func nestedValid(os, pathA, pathB string) (bool, error) { - if pathA == pathB { - return false, nil - } - if pathA == "/" && pathB != "" { - return true, nil - } - - var sep string - if os == "windows" { - sep = "\\" - } else { - sep = "/" - } - - splitedPathA := strings.Split(filepath.Clean(pathA), sep) - splitedPathB := strings.Split(filepath.Clean(pathB), sep) - lenA := len(splitedPathA) - lenB := len(splitedPathB) - - if lenA > lenB { - if (lenA - lenB) == 1 { - // if pathA is longer but not end with separator - if splitedPathA[lenA-1] != "" { - return false, nil - } - splitedPathA = splitedPathA[:lenA-1] - } else { - return false, nil - } - } - - for i, partA := range splitedPathA { - if partA != splitedPathB[i] { - return false, nil - } - } - - return true, nil -} - func deviceValid(d rspec.LinuxDevice) bool { switch d.Type { case "b", "c", "u":