Skip to content

validate media type of manifest descendants #341

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 1, 2016
Merged

validate media type of manifest descendants #341

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions schema/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@ package schema

import (
"bytes"
"encoding/json"
"fmt"
"io"
"io/ioutil"

"github.com/opencontainers/image-spec/specs-go/v1"
"github.com/pkg/errors"
"github.com/xeipuuv/gojsonschema"
)
Expand All @@ -28,6 +30,12 @@ import (
// and implements validation against a JSON schema.
type Validator string

type validateDescendantsFunc func(r io.Reader) error

var mapValidateDescendants = map[Validator]validateDescendantsFunc{
MediaTypeManifest: validateManifestDescendants,
Copy link
Contributor

@wking wking Oct 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is currently just looking at the types of the descriptors. It doesn't follow those references to actually validate the descendants (e.g. “the manifest claims sha256:a… is a application/vnd.oci.image.config.v1+json. Does it validate as such?). I think we eventually want this callback-based validation to handle that, but I don't see an easy way to do it without a CAS-engine API (once you have that it is pretty easy), so I'm fine punting on that for this PR and only doing these local checks.

Copy link
Contributor Author

@xiekeyang xiekeyang Oct 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wking
At beginning I work this in image-tool project, but seems improper.
In schema package, validateManifestDescendants acting as the validator.Validate child is what I think the nice way.
Callback handler must impact on up-layer calling in image-tool project, to which I worry maintainers not fully agree.

Copy link
Contributor

@wking wking Oct 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that you wouldn't want all of recursive validation, all recursive validation code in image-spec, and CAS-engine code in image-tools. My personal preference there is to do non-recursive JSON Schema validation here and to handle recursive validation and the CAS-engine in image-tools. @stevvooe seems to prefer keeping more than just JSON Schema validation in image-spec, but @philips has moved a fair bit of stuff to image-tools, so I'm not sure there's a clear maintainer position on this yet. Once there is a clear position, it should be fairly straightforward to put this logic in the approved location.

Copy link
Contributor

@stevvooe stevvooe Nov 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is simply validating mediatypes. It doesn't actually validate the descendent objects. This style of validation clearly belongs within the image-spec repo.

The maintainer position is clear and we have discussed on the call multiple times, without dissent.

Please stop fomenting disagreement where there is none.

Copy link
Contributor

@wking wking Nov 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please stop fomenting disagreement where there is none.

The maintainer position has been clear on this since the 2016-10-12 meeting. I don't think it was particular clear when I made these comments on 2016-10-05.

}

// ValidationError contains all the errors that happened during validation.
type ValidationError struct {
Errs []error
Expand All @@ -44,6 +52,16 @@ func (v Validator) Validate(src io.Reader) error {
return errors.Wrap(err, "unable to read the document file")
}

if f, ok := mapValidateDescendants[v]; ok {
if f == nil {
return fmt.Errorf("internal error: mapValidateDescendents[%q] is nil", v)
}
err = f(bytes.NewReader(buf))
if err != nil {
return err
}
}

sl := gojsonschema.NewReferenceLoaderFileSystem("file:///"+specs[v], fs)
ml := gojsonschema.NewStringLoader(string(buf))

Expand Down Expand Up @@ -73,3 +91,29 @@ type unimplemented string
func (v unimplemented) Validate(src io.Reader) error {
return fmt.Errorf("%s: unimplemented", v)
}

func validateManifestDescendants(r io.Reader) error {
header := v1.Manifest{}

buf, err := ioutil.ReadAll(r)
if err != nil {
return errors.Wrapf(err, "error reading the io stream")
}

err = json.Unmarshal(buf, &header)
if err != nil {
return errors.Wrap(err, "manifest format mismatch")
}

if header.Config.MediaType != string(v1.MediaTypeImageConfig) {
fmt.Printf("warning: config %s has an unknown media type: %s\n", header.Config.Digest, header.Config.MediaType)
}

for _, layer := range header.Layers {
if layer.MediaType != string(v1.MediaTypeImageLayer) &&
layer.MediaType != string(v1.MediaTypeImageLayerNonDistributable) {
fmt.Printf("warning: layer %s has an unknown media type: %s\n", layer.Digest, layer.MediaType)
}
}
return nil
}