Add index.json validation #51

zhouhao3 · 2016-10-25T08:47:28Z

if the ManifestList exists, it needs to be validated
Signed-off-by: zhouhao [email protected]

vbatts · 2016-10-25T17:00:37Z

LGTM
for a first pass

wking

The validation approach here is strange. Instead of:

Walk from the initial descriptor to a manifest, validating the manifest if you find one.
Walk from the initial descriptor to a manifest list, validating the manifest list if you find one.
Walk from the initial descriptor to …

I'd rather land opencontainers/image-spec#403 and use:

Get an initial descriptor.
Fetch the object referenced by the descriptor.
Run the appropriate intra-blob validation on it (opencontainers/image-spec#403).
For any children referenced from the blob, recurse to (2).

wking · 2016-10-25T17:02:40Z

image/manifest_list.go

+	return nil
+}
+
+func checkPlatform(m Manifests) error {


The consensus seems to be that this sort of intra-JSON-blob validation should live in image-spec.

wking · 2016-10-25T17:05:28Z

image/manifest_list.go

+	Platform Platform `json:"platform"`
+}
+
+func findManifest_list(w walker, d *descriptor) (*manifest_list, error) {


Mixing camelCase and underscores is strange. This should probably be findManifestList, although now that we have #29's (r *tarReader) Get and will hopefully soon have #5's CAS engine, I don't see much point to the find* functions.

wking · 2016-10-28T07:28:29Z

image/manifest_list.go

+
+func (ml *manifestlist) validate(w walker) error {
+	for _, d := range ml.Manifests {
+		if err := d.Descriptor.validate(w, []string{v1.MediaTypeImageConfig}); err != nil {


This is currently raising:

d.Descriptor.validate undefined (type "github.com/opencontainers/image-tools/vendor/github.com/opencontainers/image-spec/specs-go".Descriptor has no field or method validate)

That's because this repo currently defines it's own descriptor type which has a validate method, but you're using the spec's Descriptor which does not have that method. Validating descriptors will become easier with opencontainers/image-spec#403, but until then you'll want to convert your image-spec Descriptor to an image-tools descriptor before calling the local validate method.

But the manifest also have spec's Descriptor, it can use validate ,I think as long as the manifest file to the Descriptor as specific, you can call.

wking · 2016-10-28T07:32:13Z

The Manifests → ManifestDescriptor change has hidden my earlier comment, but I think it's important enough to re-post with more visibility:

The consensus seems to be that this sort of intra-JSON-blob validation should live in image-spec. So I think checkPlatform belongs in image-spec, and not here.

coolljt0725 · 2016-10-31T03:27:50Z

@q384566678 build failed

image/image.go:98: cannot use err (type error) as type string in argument to strings.Contains
image/manifest_list.go:37: undefined: io in io.Reader
image/manifest_list.go:42: undefined: ioutil in ioutil.ReadAll
image/manifest_list.go:47: undefined: bytes in bytes.NewReader
image/manifest_list.go:72: d.Descriptor.validate undefined (type "github.com/opencontainers/image-tools/vendor/github.com/opencontainers/image-spec/specs-go".Descriptor has no field or method validate)

zhouhao3 · 2016-10-31T07:23:30Z

I know,I'm trying to fix that @coolljt0725

wking · 2016-10-31T15:08:16Z

On Mon, Oct 31, 2016 at 01:54:49AM -0700, Zhou Hao wrote:

if err := d.Descriptor.validate(w, []string{v1.MediaTypeImageConfig}); err != nil {
But the manifest also have spec's
Descriptor,
it can use
validate
,I think as long as the manifest file to the Descriptor as specific,
you can call.

That validate call is using the image-tools-specific manifest 1,
which uses the image-tools-specific descriptor 2. I don't think
image-tools-specific types are a good pattern to follow, but you'll
need to convert v1.Descriptors to image-tools-specific descriptors
before you can call the image-tools-specific-descriptor's Validate
3. Ideally these would all be functions that took the image-spec
types.

coolljt0725 · 2016-11-01T07:04:25Z

image/image.go

+				return err
+			}
+		} else {
+			if !strings.Contains(string(err), "manifestlist not found") {


At the moment, the manifest list is OPTIONAL(https://github.com/opencontainers/image-spec/blob/master/manifest-list.md#oci-image-manifest-list-specification) , so we should not error out if there is no manifest list

Yes, I was in accordance with the idea to write, I first made a judgment on the error message, if it is because there is no manifest list and return, not error out.（There is one“!” symbol follows the if）

coolljt0725 · 2016-11-01T07:04:31Z

image/image.go

+			}
+		}
+
 		if out != nil {


Should we findManifestList first and if manifest_list exists, then extract the manifest from the manifest_list and then validate the manifest?

I +1 with @wking 's proposal

I'd rather land opencontainers/image-spec#403 and use:

Get an initial descriptor.
Fetch the object referenced by the descriptor.
Run the appropriate intra-blob validation on it (opencontainers/image-spec#403).
For any children referenced from the blob, recurse to (2).

coolljt0725

You need to fix the exist test:

createRefFile needs to point to the manifest list
add a createManifestlistFile to add a manifest list blob which contains the manifest.

coolljt0725 · 2016-11-03T06:33:48Z

image/image.go

 			return err
 		}
+
+		ml, err := findManifestList(w, d)


This seems not correct, findManifestList(w, d) here just assume the d is a manifest list, but actually, d could be manifest or manifest list. This is why the test failed.

--- FAIL: TestValidateLayout (0.03s) image_test.go:175: blobs/sha256/3b6be0edf862ac9e6e790fb010cf26770d9ab266f3ff81b5b82f0cf8d6f7d1a6: manifestlist validation failed: [manifests: manifests is required]

I think we should check the d after d.validate(w, validRefMediaTypes), if it is a manifest, then go to findManifest, if it is a manifest list, then go to findManifestList

Nice,I will do that.Thank you for your suggestion.

coolljt0725 · 2016-11-18T02:10:31Z

test failed

go test -race -cover github.com/opencontainers/image-tools/cmd/oci-create-runtime-bundle github.com/opencontainers/image-tools/cmd/oci-image-validate github.com/opencontainers/image-tools/cmd/oci-unpack github.com/opencontainers/image-tools/image
?       github.com/opencontainers/image-tools/cmd/oci-create-runtime-bundle [no test files]
?       github.com/opencontainers/image-tools/cmd/oci-image-validate    [no test files]
?       github.com/opencontainers/image-tools/cmd/oci-unpack    [no test files]
--- FAIL: TestValidateLayout (0.03s)
    image_test.go:175: blobs/sha256/f175ad817b1c75958e74e224f57b17d3a3e746bc434737b1bde837dbe7d33409: manifest validation failed: [config: config is required layers: layers is required]
FAIL
coverage: 29.3% of statements
FAIL    github.com/opencontainers/image-tools/image 0.096s
make: *** [test] Error 1

zhouhao3 · 2017-07-17T07:43:47Z

@vbatts @coolljt0725 @cyphar @xiekeyang @stevvooe @jonboulle I updated test file, PTAL.

stevvooe · 2017-07-17T20:35:41Z

cmd/oci-image-tool/create.go

 		},
+		cli.StringSliceFlag{
+			Name:  "platform",
+			Usage: "The platform contains os and arch. Filter manifests according to the conditions provided. Only applicable if reftype is index.",


I still don't know how to use this from this description. Is there an example?

zhouhao3 · 2017-07-18T03:00:00Z

man/oci-image-tool-unpack.1.md


+**--platform**=""
+  Specify the os and architecture of the manifest, format is OS:Architecture.
+  e.g. --platform linux:amd64


@stevvooe I have modified the introduction of information and added examples. PTAL

zhouhao3 · 2017-07-18T03:00:47Z

reping @opencontainers/image-tools-maintainers

cyphar · 2017-07-18T03:58:57Z

Just to make sure, if I want to validate an entire image with ./oci-image-tool validate --type imageLayout it will still work? In other words, is the --platform filter the same as the --ref filter in that they don't filter anything if you don't specify them?

cyphar · 2017-07-18T04:01:18Z

image/image.go

+		return manifests, nil
+	}
+
+	if len(platform) != 2 {


Either this should be checked far earlier (when you do the splitting) or you should only split it here. I would prefer if you just passed the string down to filterManifest and then you verify the arguments here.

updated.Thanks for your advice.

zhouhao3 · 2017-07-18T05:35:31Z

Just to make sure, if I want to validate an entire image with ./oci-image-tool validate --type imageLayout it will still work?

Yes.

In other words, is the --platform filter the same as the --ref filter in that they don't filter anything if you don't specify them?

Yes, just to filter the manifest in certain circumstances.

cyphar · 2017-07-18T10:13:18Z

LGTM, thanks.

zhouhao3 · 2017-07-19T00:50:51Z

ping @coolljt0725 @xiekeyang @stevvooe

xiekeyang · 2017-07-19T01:51:02Z

LGTM

It is OK from my side, PTAL from others.

zhouhao3 · 2017-07-20T01:12:18Z

If someone else has no objection, I will merge it tomorrow.

Signed-off-by: zhouhao <[email protected]>

zhouhao3 · 2017-07-20T06:46:17Z

@cyphar @xiekeyang @coolljt0725 rebased, PTAL, thanks

zhouhao3 · 2017-07-21T01:13:54Z

reping @vbatts @coolljt0725 @xiekeyang @cyphar @stevvooe PTAL

xiekeyang · 2017-07-21T01:35:17Z

LGTM

cyphar · 2017-07-21T03:53:46Z

LGTM.

@cyphar

LGTMs: @cyphar @xiekeyang Closes #51

zhouhao3 force-pushed the test-mlist branch 2 times, most recently from 13a1d21 to bf107ce Compare October 25, 2016 09:10

wking reviewed Oct 25, 2016

View reviewed changes

zhouhao3 force-pushed the test-mlist branch 7 times, most recently from 320efcb to cd48670 Compare October 27, 2016 03:50

wking mentioned this pull request Oct 28, 2016

Descriptor should be materialized opencontainers/image-spec#416

Closed

wking reviewed Oct 28, 2016

View reviewed changes

zhouhao3 force-pushed the test-mlist branch 2 times, most recently from c1c6538 to 0317d75 Compare November 1, 2016 06:52

coolljt0725 reviewed Nov 1, 2016

View reviewed changes

zhouhao3 force-pushed the test-mlist branch 5 times, most recently from 17713f4 to d38a4ba Compare November 2, 2016 07:13

coolljt0725 reviewed Nov 3, 2016

View reviewed changes

zhouhao3 force-pushed the test-mlist branch 3 times, most recently from 77f3953 to 052ade8 Compare November 4, 2016 06:56

zhouhao3 force-pushed the test-mlist branch 2 times, most recently from a622eaf to 428027f Compare July 17, 2017 07:31

stevvooe reviewed Jul 17, 2017

View reviewed changes

zhouhao3 force-pushed the test-mlist branch from 428027f to b16aaa3 Compare July 18, 2017 02:53

zhouhao3 commented Jul 18, 2017

View reviewed changes

cyphar requested changes Jul 18, 2017

View reviewed changes

zhouhao3 force-pushed the test-mlist branch from b16aaa3 to 10e8442 Compare July 18, 2017 05:30

zhouhao3 force-pushed the test-mlist branch from 10e8442 to 4273961 Compare July 18, 2017 06:40

cyphar approved these changes Jul 18, 2017

View reviewed changes

zhouhao added 2 commits July 20, 2017 14:34

Add index validation

ca603fc

Signed-off-by: zhouhao <[email protected]>

Increase platform option

4829194

Signed-off-by: zhouhao <[email protected]>

zhouhao3 force-pushed the test-mlist branch from 4273961 to ffc03f7 Compare July 20, 2017 06:39

add test to index

3c7c0e7

Signed-off-by: zhouhao <[email protected]>

zhouhao3 force-pushed the test-mlist branch from ffc03f7 to 3c7c0e7 Compare July 20, 2017 06:42

cyphar merged commit 3c7c0e7 into opencontainers:master Jul 21, 2017

cyphar added a commit that referenced this pull request Jul 21, 2017

merge branch 'pr-51'

b686775

LGTMs: @cyphar @xiekeyang Closes #51

zhouhao3 deleted the test-mlist branch July 21, 2017 04:09

zhouhao3 mentioned this pull request Sep 8, 2017

version： bump to v0.3.0 #183

Merged

Add index.json validation #51

Add index.json validation #51

Uh oh!

Conversation

zhouhao3 commented Oct 25, 2016

Uh oh!

vbatts commented Oct 25, 2016

Uh oh!

wking left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouhao3 Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking commented Oct 28, 2016

Uh oh!

coolljt0725 commented Oct 31, 2016

Uh oh!

zhouhao3 commented Oct 31, 2016

Uh oh!

wking commented Oct 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouhao3 Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coolljt0725 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coolljt0725 commented Nov 18, 2016

Uh oh!

zhouhao3 commented Jul 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouhao3 commented Jul 18, 2017

Uh oh!

cyphar commented Jul 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouhao3 Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouhao3 commented Jul 18, 2017

Uh oh!

cyphar commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhouhao3 commented Jul 19, 2017

Uh oh!

xiekeyang commented Jul 19, 2017 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhouhao3 commented Jul 20, 2017

Uh oh!

zhouhao3 commented Jul 20, 2017

Uh oh!

zhouhao3 commented Jul 21, 2017

Uh oh!

xiekeyang commented Jul 21, 2017 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

zhouhao3 Oct 31, 2016 •

edited

Loading

zhouhao3 Nov 1, 2016 •

edited

Loading

zhouhao3 Jul 18, 2017 •

edited

Loading

cyphar commented Jul 18, 2017 •

edited

Loading

xiekeyang commented Jul 19, 2017 •

edited by caniszczyk

Loading

xiekeyang commented Jul 21, 2017 •

edited by caniszczyk

Loading

cyphar commented Jul 21, 2017 •

edited by caniszczyk

Loading