Add config validation #39

zhouhao3 · 2016-10-11T09:55:33Z

I think the config may be validated when verifying the image file

Signed-off-by: zhouhao zhouhao@cn.fujitsu.com

Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>

wking · 2016-10-12T02:41:22Z

image/config.go

+	// check if the rootfs type is 'layers'
+	if c.RootFS.Type != "layers" {
+		return errors.New("RootFs is an unknown rootfs type, MUST be 'layers'")
+	}


We don't need to check this in Go, because it's checked in JSON Schema since opencontainers/image-spec#358.

ok,thanks for your reminder

wking · 2016-10-13T08:12:20Z

On Thu, Oct 13, 2016 at 01:03:42AM -0700, Zhou Hao wrote:

Closed #39.

You were doing more here than just shifting the RootFS.Type check 1.
I'm not entirely clear on what the rest of the changes are doing, but
you may want to re-open this PR if you feel any of them still need to
be addressed.

runcom · 2016-10-13T09:56:13Z

I think the config may be validated when verifying the image file

sure, but this is just checking os and arch which isn't adding a proper config validation. Can we at least have a "CheckConfig" instead of various methods that check various stuff in the config?

Also, why checking the config while finding it? should it be validated in the caller after finding it? it really seems part of "validating", not the lookup for the config file.

zhouhao3 · 2016-10-13T10:06:55Z

Maybe I had a problem with the title name. I think config need to verify the place including rootfs (has been verified)), followed by OS and arch, I put the config validation into the findconfig function, and not a separate list of functions @runcom

wking · 2016-10-13T21:26:31Z

On Thu, Oct 13, 2016 at 02:56:14AM -0700, Antonio Murdaca wrote:

Also, why checking the config while finding it? should it be
validated in the caller after finding it? it really seems part of
"validating", not the lookup for the config file.

+1. Retrieval and validation should be separate steps (although the
current findConfig sets a bad example here).

wking · 2016-10-13T21:32:52Z

image/config.go

@@ -68,6 +73,30 @@ func findConfig(w walker, d *descriptor) (*config, error) {
 	}
 }

+func checkPlatform(c config) error {


The desired location for this logic is still up in the air (as far as I can tell), with some folks arguing for just JSON Schema validation in image-spec (me and maybe @vbatts) and some (maybe @philips and @stevvooe) arguing for as much JSON validation as you can get (JSON Schema or otherwise) without needing to look outside the given JSON blob (e.g. at children it references). With the latter approach, this platform check would be landing in the image-spec repo (somewhere under schema/?). It's probably best to wait until @stevvooe has filed his proposed package structure before moving ahead with this..

zhouhao3 · 2016-10-14T02:41:57Z

ok,I have separated retrieval and validation,
As for the other uncertain arguments, we wait for the results slowly

wking · 2016-10-14T05:56:14Z

On Thu, Oct 13, 2016 at 07:41:57PM -0700, Zhou Hao wrote:

ok,I have separated retrieval and validation,

The JSON Schema validation 1 should be moved to your new (c
*config)validate() too.

zhouhao3 · 2016-10-14T06:13:58Z

@wking Thank you for your suggestion, but I do not think so. The JSON Schema validation is to confirm that the format conforms to config, in order to confirm the found file is config file, after which to verify its specific content. In the manifest.go file, the JSON Schema validation is also placed in the findManifest function, rather than in the validate function

wking · 2016-10-14T06:28:24Z

On Thu, Oct 13, 2016 at 11:13:58PM -0700, Zhou Hao wrote:

The JSON Schema validation is to confirm that the format conforms to
config, in order to confirm the found file is config file, after
which to verify its specific content.

The JSON Schema check is validation that we want to apply to any
config, regardless of how the caller got their hands on the config.
And I don't see why you'd need to use JSON Schema checks to validate
every config extracted via findConfig. If you're following a
trusted descriptor, you can be fairly confident that the referenced
config is a config without the JSON Schema check. If you are
concerned that the descriptor might not point at a valid config, you
should run all of the validation (JSON Schema and otherwise) on the
blob. And really, “validates against the spec's
application/vnd.oci.image.config.v1+json” is the least of your worries
if you don't trust the descriptor. You should be more worried about
spec-valid configs that are setting up malicious containers.

zhouhao3 · 2016-10-19T06:43:28Z

We went on to talk about the problem, according to you, the JSON Schema validation should be moved to validate function, if so, should the findManifest function of JSON Schema validation into the corresponding also validate function? My understanding of this aspect is not enough in-depth, thank you for your guidance @wking

wking · 2016-10-19T06:53:34Z

On Tue, Oct 18, 2016 at 11:43:28PM -0700, Zhou Hao wrote:

… according to you, the JSON Schema validation should be moved to
validate function, if so, should the findManifest function of JSON
Schema validation into the corresponding also validate function?

I think all validation should be moved into validation-specific
functions which folks can call when they see fit. That applies to all
media types, although I don't think we have to clean up all of that in
this PR ;).

Once #5 or something like it lands, I'd like to switch to
callback-based validation and unpacking, where we have a registry of
validators (or unpackers, or whatever) with entries for each supported
media type. I've sketched this out in 1 if you want to take a look.

stevvooe · 2016-10-19T18:31:53Z

image/config.go

@@ -68,6 +64,44 @@ func findConfig(w walker, d *descriptor) (*config, error) {
 	}
 }

+func (c *config)validate() error {


Please Go format your code.

@vbatts @philips Do we have a Go style guide in place?

stevvooe · 2016-10-19T18:32:54Z

This seems like it belongs upstream in the spec validation code.

Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>

zhouhao3 · 2016-10-20T02:12:46Z

This seems like it belongs upstream in the spec validation code.

But I think it also needs to be verified here

wking · 2016-10-20T03:38:44Z

On Wed, Oct 19, 2016 at 07:12:47PM -0700, Zhou Hao wrote:

This seems like it belongs upstream in the spec validation code.

But I think it also needs to be verified here

You can still verify it here. There are several possible approaches
to splitting validation between image-tools and image-spec. @stevvooe
seems to have convinced @vbatts and formed a consensus around keeping
all intra-blob JSON validation in image-spec 1. To fit that
approach, we need to port this platform check (and likely all of
image/config.go that deals with validation) to image-spec (as
schema/config.go?). Once that lands in image-spec, we can bump the
image-spec dependency here and call the image-spec validators where we
need intra-blob validation.

Mashimiao · 2016-11-16T06:11:54Z

@opencontainers/image-tools-maintainers PTAL

wking · 2016-11-16T15:33:17Z

@stevvooe and I are pretty sure checkPlatform belongs in image-spec.

zhouhao3 · 2016-11-17T07:41:36Z

@wking I think it is very difficult to check Platform in the image-spec, because os and arch are a range of values rather than a fixed value.

wking · 2016-11-17T14:40:33Z

I think it is very difficult to check Platform in the image-spec, because os and arch are a range of values rather than a fixed value.

I don't understand. You should just be able to copy your function over into image-spec. I can work up a PR for that if it would help clarify.

stevvooe · 2016-11-18T20:26:25Z

This validation component should be using image-spec as an upstream. Is there something that I am not seeing that is preventing this from occurring?

zhouhao3 · 2016-11-21T03:00:58Z

@stevvooe In the image-spec, the verification is to rely on json file to verify, and json file can only verify the specific value and type, os and arch are uncertain values (in the array of values ), So in the json file validation only verify its type, and its value is not within the scope of the specified array is difficult to achieve (at least for me, can not be achieved). This is where I have been puzzled.

wking · 2016-11-21T05:12:25Z

On Sun, Nov 20, 2016 at 07:00:58PM -0800, Zhou Hao wrote:

In the image-spec, the verification is to rely on json file to
verify, and json file can only verify the specific value and type…

A lot of image-spec validation is based on JSON Schema, but image-spec
has some other checks as well. See the landed
opencontainers/image-spec#341 and the in-flight
opencontainers/image-spec#403 for examples.

zhouhao3 · 2016-11-21T06:39:05Z

@wking I got it,I understand the wrong before, thank you

stevvooe · 2016-11-21T23:14:25Z

@q384566678 It sounds to me like json-schema is a big bust. It can't seem to do the most basic of validation. More complex cases, like this, are impossible.

stevvooe · 2016-11-21T23:17:16Z

Closing in favor of opencontainers/image-spec#469

add config validate

b88fd0b

Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>

zhouhao3 force-pushed the test-zz branch 7 times, most recently from 5a87b2f to 14935fb Compare October 12, 2016 02:16

wking reviewed Oct 12, 2016

View reviewed changes

zhouhao3 closed this Oct 13, 2016

zhouhao3 reopened this Oct 13, 2016

zhouhao3 force-pushed the test-zz branch 3 times, most recently from a7f5cc0 to b5b670b Compare October 13, 2016 09:31

zhouhao3 changed the title ~~Add config validate~~ check os and arch in config Oct 13, 2016

wking reviewed Oct 13, 2016

View reviewed changes

zhouhao3 force-pushed the test-zz branch from b5b670b to 55b2a75 Compare October 14, 2016 01:31

zhouhao3 changed the title ~~check os and arch in config~~ Add config validation Oct 14, 2016

zhouhao3 force-pushed the test-zz branch 7 times, most recently from f7c0d9c to 99f9203 Compare October 14, 2016 02:22

stevvooe reviewed Oct 19, 2016

View reviewed changes

add config validate

2427488

Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>

zhouhao3 force-pushed the test-zz branch from 99f9203 to 2427488 Compare October 20, 2016 01:53

This was referenced Oct 20, 2016

Intra-blob JSON validation correctness and image-spec release policy opencontainers/image-spec#406

Open

Add index.json validation #51

Merged

zhouhao3 mentioned this pull request Nov 21, 2016

schema: add checkPlatform opencontainers/image-spec#469

Closed

stevvooe closed this Nov 21, 2016

zhouhao3 deleted the test-zz branch November 22, 2016 01:00

zhouhao3 mentioned this pull request Dec 12, 2016

schema: add checkPlatform opencontainers/image-spec#491

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config validation #39

Add config validation #39

zhouhao3 commented Oct 11, 2016

wking Oct 12, 2016

zhouhao3 Oct 13, 2016

wking commented Oct 13, 2016

runcom commented Oct 13, 2016 •

edited

Loading

zhouhao3 commented Oct 13, 2016

wking commented Oct 13, 2016

wking Oct 13, 2016

zhouhao3 commented Oct 14, 2016

wking commented Oct 14, 2016

zhouhao3 commented Oct 14, 2016 •

edited

Loading

wking commented Oct 14, 2016

zhouhao3 commented Oct 19, 2016

wking commented Oct 19, 2016

stevvooe Oct 19, 2016

stevvooe commented Oct 19, 2016

zhouhao3 commented Oct 20, 2016

wking commented Oct 20, 2016

Mashimiao commented Nov 16, 2016

wking commented Nov 16, 2016 •

edited

Loading

zhouhao3 commented Nov 17, 2016

wking commented Nov 17, 2016

stevvooe commented Nov 18, 2016

zhouhao3 commented Nov 21, 2016

wking commented Nov 21, 2016

zhouhao3 commented Nov 21, 2016

stevvooe commented Nov 21, 2016

stevvooe commented Nov 21, 2016

Add config validation #39

Add config validation #39

Conversation

zhouhao3 commented Oct 11, 2016

wking Oct 12, 2016

Choose a reason for hiding this comment

zhouhao3 Oct 13, 2016

Choose a reason for hiding this comment

wking commented Oct 13, 2016

runcom commented Oct 13, 2016 • edited Loading

zhouhao3 commented Oct 13, 2016

wking commented Oct 13, 2016

wking Oct 13, 2016

Choose a reason for hiding this comment

zhouhao3 commented Oct 14, 2016

wking commented Oct 14, 2016

zhouhao3 commented Oct 14, 2016 • edited Loading

wking commented Oct 14, 2016

zhouhao3 commented Oct 19, 2016

wking commented Oct 19, 2016

stevvooe Oct 19, 2016

Choose a reason for hiding this comment

stevvooe commented Oct 19, 2016

zhouhao3 commented Oct 20, 2016

wking commented Oct 20, 2016

Mashimiao commented Nov 16, 2016

wking commented Nov 16, 2016 • edited Loading

zhouhao3 commented Nov 17, 2016

wking commented Nov 17, 2016

stevvooe commented Nov 18, 2016

zhouhao3 commented Nov 21, 2016

wking commented Nov 21, 2016

zhouhao3 commented Nov 21, 2016

stevvooe commented Nov 21, 2016

stevvooe commented Nov 21, 2016

runcom commented Oct 13, 2016 •

edited

Loading

zhouhao3 commented Oct 14, 2016 •

edited

Loading

wking commented Nov 16, 2016 •

edited

Loading