validate: add maskedpaths and readonlypaths check

[Mashimiao]

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

[vbatts]

Is checking for absolute path enough to determine read-only? I wonder if there is a unit test for some of these validations. It would require permission though.

[Mashimiao]

This just check config file in bundle if it meets the requirements in runtime SPEC.
In runtime test, we will check if the readonly paths are set as readonly as expected.

[wking]

We should probably factor our explicit-platform filepath analog out into its own package to handle cross-platform validation. See previous discussion in #256.

[Mashimiao]

maskedPath and readonlyPath are Linux-specific, I think we don't need cross-platform validation here.

[wking]

You might be validating a Linux config on a Windows host. When --host-specific is not set, the validating host shouldn't matter at all.

[Mashimiao]

maskedPath and readonlyPath means to set paths inside Linux-based container to be masked and readonly in container namespace. In my opinion there is nothing to do with Windows host and there is nothing to do with --host-specific

[wking]

Nothing to do with a Windows runtime host, but you should be able to do host-agnostic validation with oci-runtime-tool validate … regardless of whether the config is for Linux/Solaris/Windows or your validating host is Linux/Solaris/Windows.

Consider the use-case where someone is running a config registry and they want to validate all the configs they host against the spec. Maybe their registry is on a Linux box, or maybe it's on a Windows box, but either way, they should still be able to check for “If this is a Linux config, are all the paths in linux.maskedPath absolute?”.

[Mashimiao]

On 10/28/2016 11:46 AM, W. Trevor King wrote:

Nothing to do with a Windows /runtime host/, but you should be able to do host-agnostic validation with |oci-runtime-tool validate …| regardless of whether the config is for Linux/Solaris/Windows or your validating host is Linux/Solaris/Windows.

Part agreed.
But masked and readonly paths are Linux-specific in current runtime SPEC,
so we should just do masked or readonly validation when config is for Linux containers.

Consider the use-case where someone is running a config registry and they want to validate all the configs they host against the spec. Maybe their registry is on a Linux box, or maybe it's on a Windows box, but either way, they should still be able to check for “If this is a Linux config, are all the paths in |linux.maskedPath| absolute?”.

Agreed, and that's also the current implement.
I still can't get what's wrong with my current code.
Or, what exactly you want the code to be?

[wking]

On Sun, Oct 30, 2016 at 08:20:37PM -0700, Ma Shimiao wrote:

But masked and readonly paths are Linux-specific in current runtime
SPEC, so we should just do masked or readonly validation when config
is for Linux containers.

Agreed, but “I'm validating a Linux container” doesn't mean “I'm
running on a Linux host”. It could be a Windows host running
oci-runtime-tool to validate a Linux container.

I still can't get what's wrong with my current code. Or, what
exactly you want the code to be?

I want to replace path/filepath with a version of filepath that lets
us explicitly set the OS (where Go's filepath assumes it is $GOOS).
We can make the transition gradually, but for the code you add here,
it would be:

if !explicitOSFilepath.IsAbs(v.spec.Platform.OS, maskedPath)

and similarly for readonlyPath. explicitOSFilepath is a terrible
name, we should just use ‘filepath’, but we'd need to import it into a
more specific name if we didn't transition this file from Go's
filepath in a single commit. So temporarily:

import (
…
"path/filepath"
…
explicitOSFilepath "github.com/opencontainers/runtime-tools/util/filepath"
…
)

or some such. And once we'd completed the transition:

import (
…
"github.com/opencontainers/runtime-tools/util/filepath"
…
)

[Mashimiao]

On 10/31/2016 12:45 PM, W. Trevor King wrote:

Agreed, but “I'm validating a Linux container” doesn't mean “I'm
running on a Linux host”. It could be a Windows host running
oci-runtime-tool to validate a Linux container.

Until here, I do really know what you mean...

I still can't get what's wrong with my current code. Or, what
exactly you want the code to be?

I want to replace path/filepath with a version of filepath that lets
us explicitly set the OS (where Go's filepath assumes it is $GOOS).
We can make the transition gradually, but for the code you add here,
it would be:

if !explicitOSFilepath.IsAbs(v.spec.Platform.OS, maskedPath)

I think there is no need to do so complicated thing.
We can just use HasPrefix('/') to relpace IsAbs() here,
as you know absolute path is always start with '/' in Linux.
And that's also how go does in source code.

[wking]

On Sun, Oct 30, 2016 at 10:36:35PM -0700, Ma Shimiao wrote:

if !explicitOSFilepath.IsAbs(v.spec.Platform.OS, maskedPath)

I think there is no need to do so complicated thing.
We can just use HasPrefix('/') to relpace IsAbs() here…

Grepping for ‘filepath.’, I turn up a few more filepath.IsAbs calls
(e.g. ‘filepath.IsAbs(process.Cwd)’) where we do need multi-OS
handling. So I think we need our own OS-explicit filepath.IsAbs. But
if you want to use HasPrefix here until an OS-explicit filepath.IsAbs
lands (via a separate PR), that's fine with me.

[Mashimiao]

ping @opencontainers/runtime-tools-maintainers

[mrunalp]

LGTM

[liangchenye]

@Mashimiao LGTM

[Mashimiao]

@liangchenye need you re-LGTM. And just comment LGTM, don't start with any other words

[liangchenye]

LGTM