oci-image-tool: validate descriptors MediaType

[runcom]

Fix #260

/cc @coolljt0725 @s-urbaniak @xiekeyang

[xiekeyang]

if inner func is better here?

if !func(dmt string, mts []string) bool {
        for _, mt := range mts {
            if dmt == mt {
                return true
            }
        }
        return false
}(d.MediaType, mts) {
        return fmt.Errorf("invalid descriptor MediaType %q", d.MediaType)
}

[runcom]

I honestly don't like it, happy to change it if the majority thinks otherwise, I don't like it cause it isn't readable as the plain version I wrote

[stevvooe]

You can just exit in line. No need for flag variable.

[coolljt0725]

I like this 👍

[xiekeyang]

@runcom @coolljt0725 well, it is just my private custom, which brings little benefit.

[runcom]

/cc @vbatts

[runcom]

rebased!

[philips]

LGTM

[stevvooe]

LGTM

[wking]

On Tue, Sep 06, 2016 at 01:28:02AM -0700, Antonio Murdaca wrote:

• *: use types from specs-go/

This commit should also replace the current ‘descriptor’ (in
image/descriptor.go) with Descriptor (from specs-go/descriptor.go).
In #159, I do that in f2dc0e2 (replacing descriptor.validate with
validateDescriptor, although eventually we'll want a public function
for descriptor validation). That commit is also doing a lot of
walker→engine replacing, but feel free to borrow anything that helps
with descriptor→Descriptor replacing.

[wking]

I don't think this is the right API. There interesting descriptor-validation cases for each descriptor property:

a. The referenced blob is exists in (or is missing from) CAS (digest).
b. The media type can (or cannot) be validated by this package (mediaType).
c. The referenced blob exists in CAS with the right (or wrong) size (size).

Those also apply recursively, if you want to walk the Merkle tree of blob ancestors.

If the user wants to validate a descriptor, they should just be able to call image.ValidateDescriptor(walker, descriptor, recursive) or some such. If they also want to restrict the media type of the root descriptor, they can do that outside of ValidateDescriptor (because there's not a convenient way to specify those restrictions recursively, i.e. “if an ancestor contains a manifest, only allow layer types from {set}”).

If you have an image-layout with a ref with an image/png media type pointing to a valid PNG blob, I oci-image-tool validate image.tar png-ref could do any of:

i. Print something like “unrecognized media type 'image/png'" and exit nonzero (“I don't know”).
ii. Confirm that the referenced blob is a PNG of the right size and exit zero (“This is valid”).
iii. Print something like “invalid media type 'image/png'" and exit nonzero (“This is not valid”).

It is a valid reference (so (i) and (ii) are ok, but (iii) is not). The PNG is just not a reference that oci-image-tool unpack can handle. I think the right API for that is to have a separate “unpackable” check bound to a flag. So the following all pass:

$ oci-image-tool validate --recursive --unpackable image.tar $VALID_MANIFEST_REF
$ oci-image-tool validate --unpackable image.tar $BUSTED_ANCESTOR_MANIFEST_REF
$ oci-image-tool validate --recursive image.tar $VALID_LAYER_REF
$ oci-image-tool validate --recursive image.tar $VALID_CONFIG_REF
$ oci-image-tool validate image.tar $VALID_MANIFEST_REF
$ oci-image-tool validate image.tar $BUSTED_ANCESTOR_MANIFEST_REF
$ oci-image-tool validate image.tar $VALID_LAYER_REF
$ oci-image-tool validate image.tar $VALID_CONFIG_REF

and the following all fail:

$ oci-image-tool validate --recursive --unpackable image.tar $BUSTED_ANCESTOR_MANIFEST_REF
$ oci-image-tool validate --recursive --unpackable image.tar $VALID_LAYER_REF
$ oci-image-tool validate --recursive --unpackable image.tar $VALID_CONFIG_REF
$ oci-image-tool validate --unpackable image.tar $VALID_LAYER_REF
$ oci-image-tool validate --unpackable image.tar $VALID_CONFIG_REF

[runcom]

Isn't it better to open another issue tracking this?

[wking]

On Wed, Sep 07, 2016 at 12:00:38AM -0700, Antonio Murdaca wrote:

@@ -73,7 +73,17 @@ func findDescriptor(w walker, name string) (*descriptor, error) {
}
}

-func (d *descriptor) validate(w walker) error {
+func (d *descriptor) validate(w walker, mts []string) error {

Isn't it better to open another issue tracking this?

You're adding ‘mts’ here, and I don't think we want it ;). If folks
feel like the rest of this PR is a step forward, I'm ok with this
landing and me filing a subsequent PR to roll this part back out in
favor of --unpackable.

[runcom]

@wking, can we not block PRs if you already took care of using the right descriptor struct in your PR? Also, it's better to have a new PR altogether instead of embedding that in #159.

[wking]

On Wed, Sep 07, 2016 at 12:05:13AM -0700, Antonio Murdaca wrote:

@wking, can we not block PRs if you already took care of using the
right descriptor struct in your PR?

If you don't feel like covering it in this PR, it can land with #159,
or via some other followup PR, or never ;). No big deal. It just
seemed like a natural fit for a “use types from specs-go” commit.

Also, it's better to have a new PR altogether instead of embedding
that in #159.

#159 has floated for a lot longer than I'd initial expected. I may
start splitting off smaller shifts like that, but I really don't like
walkers, so I'm trying to avoid touching them ;).

[s-urbaniak]

@runcom LGTM 👍

[vbatts]

LGTM