Skip to content
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

oci-image-tool: validate descriptors MediaType #262

Merged
merged 4 commits into from
Sep 7, 2016
Merged

oci-image-tool: validate descriptors MediaType #262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bedfc7e
specs-go/v1: rename to mediatype layer and config
runcom Sep 6, 2016
ca9bb8e
*: use types from specs-go/
runcom Sep 6, 2016
551f37d
specs-go/v1: fix Entrypoint
runcom Sep 6, 2016
4829143
cmd/oci-image-tool: validate descriptors MediaType
runcom Sep 6, 2016
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
20 changes: 2 additions & 18 deletions image/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,28 +26,12 @@ import (
"strings"

"github.com/opencontainers/image-spec/schema"
"github.com/opencontainers/image-spec/specs-go/v1"
"github.com/opencontainers/runtime-spec/specs-go"
"github.com/pkg/errors"
)

type cfg struct {
User string
Memory int64
MemorySwap int64
CPUShares int64 `json:"CpuShares"`
ExposedPorts map[string]struct{}
Env []string
Entrypoint []string
Cmd []string
Volumes map[string]struct{}
WorkingDir string
}

type config struct {
Architecture string `json:"architecture"`
OS string `json:"os"`
Config cfg `json:"config"`
}
type config v1.Image

func findConfig(w walker, d *descriptor) (*config, error) {
var c config
Expand Down
12 changes: 11 additions & 1 deletion image/descriptor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

var found bool
for _, mt := range mts {
if d.MediaType == mt {
found = true
break
}
}
if !found {
return fmt.Errorf("invalid descriptor MediaType %q", d.MediaType)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
}

Copy link
Member Author

@runcom runcom Sep 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

switch err := w.walk(func(path string, info os.FileInfo, r io.Reader) error {
if info.IsDir() {
return nil
Expand Down
12 changes: 9 additions & 3 deletions image/image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"os"
"path/filepath"

"github.com/opencontainers/image-spec/specs-go/v1"
"github.com/pkg/errors"
)

Expand All @@ -43,14 +44,19 @@ func Validate(tarFile string, refs []string, out *log.Logger) error {
return validate(newTarWalker(f), refs, out)
}

var validRefMediaTypes = []string{
v1.MediaTypeImageManifest,
v1.MediaTypeImageManifestList,
}

func validate(w walker, refs []string, out *log.Logger) error {
for _, r := range refs {
ref, err := findDescriptor(w, r)
if err != nil {
return err
}

if err = ref.validate(w); err != nil {
if err = ref.validate(w, validRefMediaTypes); err != nil {
return err
}

Expand Down Expand Up @@ -97,7 +103,7 @@ func unpack(w walker, dest, refName string) error {
return err
}

if err = ref.validate(w); err != nil {
if err = ref.validate(w, validRefMediaTypes); err != nil {
return err
}

Expand Down Expand Up @@ -139,7 +145,7 @@ func createRuntimeBundle(w walker, dest, refName, rootfs string) error {
return err
}

if err = ref.validate(w); err != nil {
if err = ref.validate(w, validRefMediaTypes); err != nil {
return err
}

Expand Down
5 changes: 3 additions & 2 deletions image/manifest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"time"

"github.com/opencontainers/image-spec/schema"
"github.com/opencontainers/image-spec/specs-go/v1"
"github.com/pkg/errors"
)

Expand Down Expand Up @@ -74,12 +75,12 @@ func findManifest(w walker, d *descriptor) (*manifest, error) {
}

func (m *manifest) validate(w walker) error {
if err := m.Config.validate(w); err != nil {
if err := m.Config.validate(w, []string{v1.MediaTypeImageConfig}); err != nil {
return errors.Wrap(err, "config validation failed")
}

for _, d := range m.Layers {
if err := d.validate(w); err != nil {
if err := d.validate(w, []string{v1.MediaTypeImageLayer}); err != nil {
return errors.Wrap(err, "layer validation failed")
}
}
Expand Down
16 changes: 10 additions & 6 deletions schema/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,19 @@

package schema

import "net/http"
import (
"net/http"

"github.com/opencontainers/image-spec/specs-go/v1"
)

// Media types for the OCI image formats
const (
MediaTypeDescriptor Validator = `application/vnd.oci.descriptor.v1+json`
MediaTypeManifest Validator = `application/vnd.oci.image.manifest.v1+json`
MediaTypeManifestList Validator = `application/vnd.oci.image.manifest.list.v1+json`
MediaTypeImageConfig Validator = `application/vnd.oci.image.config.v1+json`
MediaTypeImageLayer unimplemented = `application/vnd.oci.image.layer.tar+gzip`
MediaTypeDescriptor Validator = v1.MediaTypeDescriptor
MediaTypeManifest Validator = v1.MediaTypeImageManifest
MediaTypeManifestList Validator = v1.MediaTypeImageManifestList
MediaTypeImageConfig Validator = v1.MediaTypeImageConfig
MediaTypeImageLayer unimplemented = v1.MediaTypeImageLayer
)

var (
Expand Down
2 changes: 1 addition & 1 deletion specs-go/v1/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ type ImageConfig struct {
Env []string `json:"Env"`

// Entrypoint defines a list of arguments to use as the command to execute when the container starts.
EntryPoint []string `json:"EntryPoint"`
Entrypoint []string `json:"Entrypoint"`

// Cmd defines the default arguments to the entrypoint of the container.
Cmd []string `json:"Cmd"`
Expand Down
8 changes: 4 additions & 4 deletions specs-go/v1/mediatype.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ const (
// MediaTypeImageManifestList specifies the mediaType for an image manifest list.
MediaTypeImageManifestList = "application/vnd.oci.image.manifest.list.v1+json"

// MediaTypeImageSerialization is the mediaType used for layers referenced by the manifest.
MediaTypeImageSerialization = "application/vnd.oci.image.layer.tar+gzip"
// MediaTypeImageLayer is the mediaType used for layers referenced by the manifest.
MediaTypeImageLayer = "application/vnd.oci.image.layer.tar+gzip"

// MediaTypeImageSerializationConfig specifies the mediaType for the image configuration.
MediaTypeImageSerializationConfig = "application/vnd.oci.image.config.v1+json"
// MediaTypeImageConfig specifies the mediaType for the image configuration.
MediaTypeImageConfig = "application/vnd.oci.image.config.v1+json"
)