Merge pull request #103 from mtrmac/image-layer-digests

Move parsing layer digests from copy.go to types.Image

cmd/skopeo/copy.go

@@ -1,54 +1,24 @@
 package main
 
 import (
-	"encoding/json"
 	"errors"
 	"fmt"
 
-	"github.com/projectatomic/skopeo/manifest"
+	"github.com/projectatomic/skopeo/image"
 	"github.com/projectatomic/skopeo/signature"
 	"github.com/urfave/cli"
 )
 
-// FIXME: Also handle schema2, and put this elsewhere:
-// docker.go contains similar code, more sophisticated
-// (at the very least the deduplication should be reused from there).
-func manifestLayers(manifest []byte) ([]string, error) {
-	man := manifestSchema1{}
-	if err := json.Unmarshal(manifest, &man); err != nil {
-		return nil, err
-	}
-
-	layers := []string{}
-	for _, layer := range man.FSLayers {
-		layers = append(layers, layer.BlobSum)
-	}
-	return layers, nil
-}
-
-// FIXME: this is a copy from docker_image.go and does not belong here.
-type manifestSchema1 struct {
-	Name     string
-	Tag      string
-	FSLayers []struct {
-		BlobSum string `json:"blobSum"`
-	} `json:"fsLayers"`
-	History []struct {
-		V1Compatibility string `json:"v1Compatibility"`
-	} `json:"history"`
-	// TODO(runcom) verify the downloaded manifest
-	//Signature []byte `json:"signature"`
-}
-
 func copyHandler(context *cli.Context) error {
 	if len(context.Args()) != 2 {
 		return errors.New("Usage: copy source destination")
 	}
 
-	src, err := parseImageSource(context, context.Args()[0])
+	rawSource, err := parseImageSource(context, context.Args()[0])
 	if err != nil {
 		return fmt.Errorf("Error initializing %s: %v", context.Args()[0], err)
 	}
+	src := image.FromSource(rawSource)
 
 	dest, err := parseImageDestination(context, context.Args()[1])
 	if err != nil {
@@ -56,18 +26,18 @@ func copyHandler(context *cli.Context) error {
 	}
 	signBy := context.String("sign-by")
 
-	m, _, err := src.GetManifest([]string{manifest.DockerV2Schema1MIMEType})
+	manifest, err := src.Manifest()
 	if err != nil {
 		return fmt.Errorf("Error reading manifest: %v", err)
 	}
 
-	layers, err := manifestLayers(m)
+	layers, err := src.LayerDigests()
 	if err != nil {
 		return fmt.Errorf("Error parsing manifest: %v", err)
 	}
 	for _, layer := range layers {
 		// TODO(mitr): do not ignore the size param returned here
-		stream, _, err := src.GetBlob(layer)
+		stream, _, err := rawSource.GetBlob(layer)
 		if err != nil {
 			return fmt.Errorf("Error reading layer %s: %v", layer, err)
 		}
@@ -77,7 +47,7 @@ func copyHandler(context *cli.Context) error {
 		}
 	}
 
-	sigs, err := src.GetSignatures()
+	sigs, err := src.Signatures()
 	if err != nil {
 		return fmt.Errorf("Error reading signatures: %v", err)
 	}
@@ -92,7 +62,7 @@ func copyHandler(context *cli.Context) error {
 			return fmt.Errorf("Error determining canonical Docker reference: %v", err)
 		}
 
-		newSig, err := signature.SignDockerManifest(m, dockerReference, mech, signBy)
+		newSig, err := signature.SignDockerManifest(manifest, dockerReference, mech, signBy)
 		if err != nil {
 			return fmt.Errorf("Error creating signature: %v", err)
 		}
@@ -104,7 +74,7 @@ func copyHandler(context *cli.Context) error {
 	}
 
 	// FIXME: We need to call PutManifest after PutBlob and PutSignatures. This seems ugly; move to a "set properties" + "commit" model?
-	if err := dest.PutManifest(m); err != nil {
+	if err := dest.PutManifest(manifest); err != nil {
 		return fmt.Errorf("Error writing manifest: %v", err)
 	}
 	return nil

cmd/skopeo/layers.go

@@ -13,7 +13,7 @@ var layersCmd = cli.Command{
 		if err != nil {
 			return err
 		}
-		if err := img.Layers(c.Args().Tail()...); err != nil {
+		if err := img.LayersCommand(c.Args().Tail()...); err != nil {
 			return err
 		}
 		return nil

image/image.go

@@ -45,6 +45,7 @@ func (i *genericImage) IntendedDockerReference() string {
 }
 
 // Manifest is like ImageSource.GetManifest, but the result is cached; it is OK to call this however often you need.
+// NOTE: It is essential for signature verification that Manifest returns the manifest from which LayerDigests is computed.
 func (i *genericImage) Manifest() ([]byte, error) {
 	if i.cachedManifest == nil {
 		m, _, err := i.src.GetManifest([]string{manifest.DockerV2Schema1MIMEType})
@@ -121,13 +122,15 @@ type genericManifest interface {
 	GetLayers() []string
 }
 
+type fsLayersSchema1 struct {
+	BlobSum string `json:"blobSum"`
+}
+
 type manifestSchema1 struct {
 	Name     string
 	Tag      string
-	FSLayers []struct {
-		BlobSum string `json:"blobSum"`
-	} `json:"fsLayers"`
-	History []struct {
+	FSLayers []fsLayersSchema1 `json:"fsLayers"`
+	History  []struct {
 		V1Compatibility string `json:"v1Compatibility"`
 	} `json:"history"`
 	// TODO(runcom) verify the downloaded manifest
@@ -150,6 +153,10 @@ func sanitize(s string) string {
 	return strings.Replace(s, "/", "-", -1)
 }
 
+// getSchema1Manifest parses the manifest into a data structure, cleans it up, and returns it.
+// NOTE: The manifest may have been modified in the process; DO NOT reserialize and store the return value
+// if you want to preserve the original manifest; use the blob returned by Manifest() directly.
+// NOTE: It is essential for signature verification that the object is computed from the same manifest which is returned by Manifest().
 func (i *genericImage) getSchema1Manifest() (genericManifest, error) {
 	manblob, err := i.Manifest()
 	if err != nil {
@@ -172,7 +179,33 @@ func (i *genericImage) getSchema1Manifest() (genericManifest, error) {
 	return mschema1, nil
 }
 
-func (i *genericImage) Layers(layers ...string) error {
+// uniqueLayerDigests returns a list of layer digests referenced from a manifest.
+// The list will not contain duplicates; it is not intended to correspond to the "history" or "parent chain" of a Docker image.
+func uniqueLayerDigests(m genericManifest) []string {
+	var res []string
+	seen := make(map[string]struct{})
+	for _, digest := range m.GetLayers() {
+		if _, ok := seen[digest]; ok {
+			continue
+		}
+		seen[digest] = struct{}{}
+		res = append(res, digest)
+	}
+	return res
+}
+
+// LayerDigests returns a list of layer digests referenced by this image.
+// The list will not contain duplicates; it is not intended to correspond to the "history" or "parent chain" of a Docker image.
+// NOTE: It is essential for signature verification that LayerDigests is computed from the same manifest which is returned by Manifest().
+func (i *genericImage) LayerDigests() ([]string, error) {
+	m, err := i.getSchema1Manifest()
+	if err != nil {
+		return nil, err
+	}
+	return uniqueLayerDigests(m), nil
+}
+
+func (i *genericImage) LayersCommand(layers ...string) error {
 	m, err := i.getSchema1Manifest()
 	if err != nil {
 		return err
@@ -190,7 +223,7 @@ func (i *genericImage) Layers(layers ...string) error {
 		return err
 	}
 	if len(layers) == 0 {
-		layers = m.GetLayers()
+		layers = uniqueLayerDigests(m)
 	}
 	for _, l := range layers {
 		if !strings.HasPrefix(l, "sha256:") {

image/image_test.go

@@ -0,0 +1,32 @@
+package image
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestUniqueLayerDigests(t *testing.T) {
+	for _, test := range []struct{ input, expected []string }{
+		// Ensure that every element of expected: is unique!
+		{input: []string{}, expected: []string{}},
+		{input: []string{"a"}, expected: []string{"a"}},
+		{input: []string{"a", "b", "c"}, expected: []string{"a", "b", "c"}},
+		{input: []string{"a", "a", "c"}, expected: []string{"a", "c"}},
+		{input: []string{"a", "b", "a"}, expected: []string{"a", "b"}},
+	} {
+		in := []fsLayersSchema1{}
+		for _, e := range test.input {
+			in = append(in, fsLayersSchema1{e})
+		}
+
+		m := manifestSchema1{FSLayers: in}
+		res := uniqueLayerDigests(&m)
+		// Test that the length is the same and each expected element is present.
+		// This requires each element of test.expected to be unique, as noted above.
+		assert.Len(t, res, len(test.expected))
+		for _, e := range test.expected {
+			assert.Contains(t, res, e)
+		}
+	}
+}

signature/policy_reference_match_test.go

@@ -55,7 +55,10 @@ func (ref refImageMock) ManifestMatchesDigest(expectedDigest string) (bool, erro
 func (ref refImageMock) Signatures() ([][]byte, error) {
 	panic("unexpected call to a mock function")
 }
-func (ref refImageMock) Layers(layers ...string) error {
+func (ref refImageMock) LayerDigests() ([]string, error) {
+	panic("unexpected call to a mock function")
+}
+func (ref refImageMock) LayersCommand(layers ...string) error {
 	panic("unexpected call to a mock function")
 }
 func (ref refImageMock) Inspect() (*types.ImageInspectInfo, error) {
@@ -168,7 +171,10 @@ func (ref forbiddenImageMock) ManifestMatchesDigest(expectedDigest string) (bool
 func (ref forbiddenImageMock) Signatures() ([][]byte, error) {
 	panic("unexpected call to a mock function")
 }
-func (ref forbiddenImageMock) Layers(layers ...string) error {
+func (ref forbiddenImageMock) LayerDigests() ([]string, error) {
+	panic("unexpected call to a mock function")
+}
+func (ref forbiddenImageMock) LayersCommand(layers ...string) error {
 	panic("unexpected call to a mock function")
 }
 func (ref forbiddenImageMock) Inspect() (*types.ImageInspectInfo, error) {

types/types.go

@@ -57,11 +57,18 @@ type Image interface {
 	// May be "" if unknown.
 	IntendedDockerReference() string
 	// Manifest is like ImageSource.GetManifest, but the result is cached; it is OK to call this however often you need.
-	// FIXME? This should also return a MIME type if known, to differentiate between schema versions.
+	// NOTE: It is essential for signature verification that Manifest returns the manifest from which LayerDigests is computed.
 	Manifest() ([]byte, error)
 	// Signatures is like ImageSource.GetSignatures, but the result is cached; it is OK to call this however often you need.
 	Signatures() ([][]byte, error)
-	Layers(layers ...string) error // configure download directory? Call it DownloadLayers?
+	// LayerDigests returns a list of layer digests referenced by this image.
+	// The list will not contain duplicates; it is not intended to correspond to the "history" or "parent chain" of a Docker image.
+	// NOTE: It is essential for signature verification that LayerDigests is computed from the same manifest which is returned by Manifest().
+	LayerDigests() ([]string, error)
+	// LayersCommand implements (skopeo layers).  Do not use for any other purpose.
+	// Longer-term we would like to move the command-specific code up to the command handler,
+	// but the command has functionality specific to util.DockerV2Schema1MIMEType manifests.
+	LayersCommand(layers ...string) error // configure download directory?
 	// Inspect returns various information for (skopeo inspect) parsed from the manifest and configuration.
 	Inspect() (*ImageInspectInfo, error)
 	DockerTar() ([]byte, error) // ??? also, configure output directory