validate: add mounts whether nested check #256

Mashimiao · 2016-10-25T03:22:00Z

Signed-off-by: Ma Shimiao [email protected]

wking

Checking for nesting and warning for covers both sound like good ideas, but the current implementation here still needs some work.

wking · 2016-10-25T04:49:48Z

validate/validate.go

+		if supportedTypes != nil && !supportedTypes[fMount.Type] {
+			msgs = append(msgs, fmt.Sprintf("Unsupported mount type %q", fMount.Type))
+		}
+		for j, bMount := range v.spec.Mounts {


Why fMount and bMount? Maybe mount1 and mount2?

fMount means formerMount, bMount means backMount.
Maybe it's not easy to understand, I will modify them to mountA, mountB

wking · 2016-10-25T04:52:22Z

validate/validate.go

+		for j, bMount := range v.spec.Mounts {
+			if nestedValid(bMount.Destination, fMount.Destination) {
+				if v.spec.Platform.OS == "windows" {
+					msgs = append(msgs, fmt.Sprintf("On Windows, %v nested within %v is forbidden", bMount.Destination, fMount.Destination))


Looking over the spec language again, I'm not clear that it's forbidding covering mounts (e.g. mounting c:\foo after mounting c:\foo\bar). If that's the intention, I think we need stronger/clearer spec language before forbidding them here (vs. just warning about them, like you're doing for Linux).

The SPEC says "For the Windows operating system, one mount destination MUST NOT be nested within another mount (e.g., c:\foo and c:\foo\bar)."
I think it clearly says nested mounts are forbidden.

Nested yes. What I'm not clear on is whether covering mounts are also forbidden.

That's really a problem. The SEPC doesn't say.
I will add limit, let's just check nested mounts first.

wking · 2016-10-25T04:53:15Z

validate/validate.go

+					msgs = append(msgs, fmt.Sprintf("On Windows, %v nested within %v is forbidden", bMount.Destination, fMount.Destination))
+				}
+				if v.spec.Platform.OS == "linux" && i > j {
+					logrus.Warnf("On Linux, %v will be covered by %v", bMount.Destination, fMount.Destination)


This seems like something we should be handling in and else case so we also print warnings for Solaris and other OSes that don't forbid nesting/covering.

I'm not familiar with other OSes, which ones should we cover?

If warning for covering mounts is a good idea for Linux (and I think it is), I expect it is a good idea for all OSes where it's legal (and where it's illegal we should be erroring).

wking · 2016-10-25T04:58:32Z

validate/validate.go

+		return false
+	}
+
+	return strings.HasPrefix(bPath, fPath)


strings.HasPrefix is not what you want to use (e.g. strings.HasPrefix("/ab", "/a") is true, but /ab and /a are siblings). What you want is something like filepath.Rel, but using the path syntax appropriate for the config.json's declared OS instead of the path syntax for which oci-runtime-tool was compiled. So windowsfilepath.Rel or posixfilepath.Rel, depending on platform.os.

Mashimiao · 2016-10-25T07:00:37Z

@wking PTAL

wking · 2016-10-25T16:43:35Z

validate/validate.go

+			return true
+		} else if os != "windows" && strings.HasPrefix(splitedPaths[1], "/") {
+			return true
+		}


Determining how the paths are related is complicated enough; I'd rather not mix in validation here. Can we have a ancestorPath(os string, pathA string, pathB string) function that returns true if and only if pathB is an ancestor of pathA using the os path syntax?

Actually, the function should probably be ancestorPath(os string, pathA string, pathB string, strict boolean) (isAncestor boolean), where when strict is false, isAncestor is true when pathA and pathB are equivalent or pathB is a strict ancestor of pathA.

wking · 2016-10-25T16:47:57Z

validate/validate.go

+		return false
+	} else if strings.HasPrefix(purePathB, purePathA) {
+		splitedPaths := strings.SplitN(purePathB, purePathA, 2)
+		if os == "windows" && strings.HasPrefix(splitedPaths[1], "\\") {


You'll also want to catch the case where splitedPaths (pathComponents?) has a length of one (because the initial path was / or some such).

wking · 2016-10-25T16:49:17Z

validate/validate.go

+		purePathB = strings.TrimSuffix(pathB, "\\")
+	} else {
+		purePathA = strings.TrimSuffix(pathA, "/")
+		purePathB = strings.TrimSuffix(pathB, "/")


The logic throughout this function would probably be simpler if you open with:

var sep string if os == "windows" { sep = "\\" } else { sep = "/" }

wking · 2016-10-25T16:50:01Z

validate/validate.go

@@ -490,6 +499,30 @@ func namespaceValid(ns rspec.Namespace) bool {
 	return true
 }

+func nestedValid(os, pathA, pathB string) bool {
+	var purePathA, purePathB string


The paths were passed by value, so you drop purePath* and just clobber pathA and pathB.

wking · 2016-10-25T16:50:46Z

validate/validate.go

+					msgs = append(msgs, fmt.Sprintf("On Windows, %v nested within %v is forbidden", mountB.Destination, mountA.Destination))
+				}
+				if i > j {
+					// FIXME: add erroring for OS which not support covering mounts


Not necessarily a fixme, since covering may be legal on all OSes.

Fine, will remove

wking · 2016-10-25T16:52:09Z

validate/validate.go

+				}
+				if i > j {
+					// FIXME: add erroring for OS which not support covering mounts
+					logrus.Warnf("On %v, %v will be covered by %v", v.spec.Platform.OS, mountB.Destination, mountA.Destination)


I'd drop “On %v,”. Covering is surprising behavior worth warning about regardless of the target OS. The nesting error a few lines up, on the other hand, is a Windows-specific error, so opening with “On Windows,” is fine.

wking · 2016-10-26T16:45:07Z

validate/validate.go

+	purePathA = strings.TrimSuffix(pathA, sep)
+	purePathB = strings.TrimSuffix(pathB, sep)
+
+	if sep == "/" && pathA == sep && strings.HasPrefx(pathB, pathA) {


Missing a strings import?

wking · 2016-10-26T16:59:04Z

validate/validate.go

+		sep = "/"
+	}
+	purePathA = strings.TrimSuffix(pathA, sep)
+	purePathB = strings.TrimSuffix(pathB, sep)


I still think these could be:

pathA = strings.TrimSuffix(pathA, sep)

and I don't think you need the sep == "/" && pathA == sep qualifiers below.

Mashimiao · 2016-11-04T02:25:48Z

@opencontainers/runtime-tools-maintainers @wking PTAL
I have updated the PR. Maybe the code is not very elegant.
But with my own test, I believe not matter invalid paths windows's or linux's or paths like C:\A, C:\A\B, /, /A, //A//B//C/, ///C//B, /A/C will be checked very well.

hqhq · 2016-11-04T07:11:15Z

validate/validate.go

@@ -490,6 +504,89 @@ func namespaceValid(ns rspec.Namespace) bool {
 	return true
 }

+// Check whether pathB is nested whithin pathA
+func nestedValid(os, pathA, pathB string) (bool, error) {


I didn't read through, according to your comments, isn't this something we can do with just filepath.Clean() and filepath.Rel()?

And I don't know how you handle with different oses other places in runtime-tools, but why not separate them in different files, distinguish with build tags or different command options. I'll be more extendable and make codes neat and clean.

Mashimiao · 2016-11-04T07:30:20Z

On 11/04/2016 03:11 PM, Qiang Huang wrote:

I didn't read through, according to your comments, isn't this something we can do with just |filepath.Clean()| and |filepath.Rel()|?

Well, with Clan() and Rel(), we still can't find out invalid path
and Rel() is not very good to deal with paths as /c and /b. it believes they are relative.

And I don't know how you handle with different oses other places in runtime-tools, but why not separate them in different files, distinguish with build tags or different command options. I'll be more extendable and make codes neat and clean.

There are not too many places in runtime-tools different for different oses.
So, currently I prefer to use compatible code to handle them.

wking · 2016-11-04T07:58:26Z

In case it helps make my explicit-OS filepath idea 1 clearer, I've
stubbed it out in 2. I think that makes the “Is this an ancestor”
function a lot more readable, but even in my stub there are a lot of
FIXMEs. On the other hand, I don't think either of us are properly
handling .. at the moment.

Instead of duplicating Go's (complicated and well-tested) stdlib
functionality, I'd rather fork it into a separate explicit-OS library
and consume that library here. That would cover most (all?) of the
current FIXMEs in 2.

Mashimiao · 2016-11-04T08:52:22Z

Hi @wking I don't separating explicit-OS library and build with build tags can solve the question you raised about validating Windows container's config on Linux platform or validating Linux container's config on Windows platform. As you know, Go just does separating explicit-OS library, but can't solve the above question

hqhq · 2016-11-04T09:32:15Z

Well, with Clan() and Rel(), we still can't find out invalid path

Path validation is orthogonal in this case, I don't think you should consider that, otherwise, do you do path validation in every case that a file path is involved? Even you want to do that, you can do it in a separate function named like validPath() or something, and that's another topic.

and Rel() is not very good to deal with paths as /c and /b. it believes they are relative.

What do you mean about this, I don't get it.

Mashimiao · 2016-11-04T09:53:07Z

On 11/04/2016 05:32 PM, Qiang Huang wrote:

Path validation is orthogonal in this case, I don't think you should consider that, otherwise, do you do path validation in every case that a file path is involved? Even you want to do that, you can do it in a separate function named like |validPath()| or something, and that's another topic.

I think this is a good idea. I can separate it.

and Rel() is not very good to deal with paths as /c and /b. it believes they are relative.
What do you mean about this, I don't get it.

I mean Rel() is not very good to judge whether paths like /c and /b is nested or not.
And as I reply to @wking above, separating based on OSes can't solve the problem we are facing,
validating Linux container's config on windows platform or validating windows container's config on Linux platform.

wking · 2016-11-04T15:08:57Z

On Fri, Nov 04, 2016 at 01:52:23AM -0700, Ma Shimiao wrote:

Hi @wking I don't separating explicit-OS library and build with
build tags can solve the question you raised about validating
Windows container's config on Linux platform or validating Linux
container's config on Windows platform. As you know, Go just does
separating explicit-OS library, but can't solve the above question

Go has an implicit OS file/filepath using build tags to build it for
just the $GOOS. My gist has an explicit OS API so you can call (for
example):

IsAbs("windows", "C:\a\b\c")

and get ‘true’ regardless of whether your $GOOS is windows or not.
I'm replacing Go's file/filepath's build-time OS decision with a
function-call-time OS decision.

Signed-off-by: Ma Shimiao <[email protected]>

Mashimiao · 2017-08-02T09:36:43Z

ping @wking @opencontainers/runtime-tools-maintainers

zhouhao3 · 2017-08-03T02:58:05Z

LGTM

liangchenye · 2017-08-17T09:12:16Z

LGTM

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 4dcd484 (validate: add mounts whether nested check, 2016-10-25, opencontainers#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 <[email protected]>

wking reviewed Oct 25, 2016

View reviewed changes

Mashimiao force-pushed the validate-add-mounts-nested-check branch from e619170 to 28af22a Compare October 25, 2016 07:00

wking reviewed Oct 25, 2016

View reviewed changes

Mashimiao force-pushed the validate-add-mounts-nested-check branch from 28af22a to eb43843 Compare October 26, 2016 10:51

wking reviewed Oct 26, 2016

View reviewed changes

This was referenced Oct 26, 2016

validate: add maskedpaths and readonlypaths check #259

Merged

verbose output opencontainers/image-tools#46

Open

Mashimiao force-pushed the validate-add-mounts-nested-check branch 2 times, most recently from bf64d9d to 1b23a98 Compare November 4, 2016 02:15

hqhq reviewed Nov 4, 2016

View reviewed changes

wking mentioned this pull request Mar 28, 2017

validate: Test config.json and rootfs sibling-hood #353

Merged

wking mentioned this pull request Apr 14, 2017

validate: Increase the windows system mount verification #364

Closed

Mashimiao force-pushed the validate-add-mounts-nested-check branch 2 times, most recently from 3957c6f to c071b26 Compare August 2, 2017 09:29

validate: add mounts whether nested check

4dcd484

Signed-off-by: Ma Shimiao <[email protected]>

Mashimiao force-pushed the validate-add-mounts-nested-check branch from c071b26 to 4dcd484 Compare August 2, 2017 09:37

liangchenye merged commit 9e0e42d into opencontainers:master Aug 17, 2017

wking mentioned this pull request Aug 17, 2017

filepath: Add a stand-alone package for explicit-OS path logic #445

Merged

validate: add mounts whether nested check #256

validate: add mounts whether nested check #256

Conversation

Mashimiao commented Oct 25, 2016

wking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mashimiao commented Oct 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mashimiao commented Nov 4, 2016

Choose a reason for hiding this comment

Mashimiao commented Nov 4, 2016

wking commented Nov 4, 2016

Mashimiao commented Nov 4, 2016

hqhq commented Nov 4, 2016

Mashimiao commented Nov 4, 2016

wking commented Nov 4, 2016

Mashimiao commented Aug 2, 2017

zhouhao3 commented Aug 3, 2017 • edited by caniszczyk Loading

liangchenye commented Aug 17, 2017 • edited by caniszczyk Loading

zhouhao3 commented Aug 3, 2017 •

edited by caniszczyk

Loading

liangchenye commented Aug 17, 2017 •

edited by caniszczyk

Loading