From 206a8e3eed29ac40f3d082912d1e1e8253d4cd3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Jun 2016 09:44:27 +0200 Subject: [PATCH 1/4] Remove a FIXME? about types.Image.Manifest. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per the discussion in https://github.com/projectatomic/skopeo/pull/73 , types.Image.Manifest should not need to expose MIME types: ImageSource.GetManifest allows supplying MIME types; the intent is for clients who want to parse the manifests to use an ImageSource. Clients who want to use skopeo’s parsing should use types.Image, and then they don’t need to care about MIME types. In fact, types.Image needs to decide among the various manifest alternatives which one to parse (and which one to match against the provided or signed manifest digest). So, Image.Manifest will not be all that useful for parsing the contents, it is basically useful only for verifying against a digest. --- types/types.go | 1 - 1 file changed, 1 deletion(-) diff --git a/types/types.go b/types/types.go index 09640fad..cacb58a5 100644 --- a/types/types.go +++ b/types/types.go @@ -57,7 +57,6 @@ 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. 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) From c81541de0ad616e0da01bf7758e4af00697574b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Jun 2016 09:08:29 +0200 Subject: [PATCH 2/4] Rename types.Image.Layers to LayersCommand The .Layers() method name is too nice to contain this layering violation; make it more explicit in the naming. --- cmd/skopeo/layers.go | 2 +- image/image.go | 2 +- signature/policy_reference_match_test.go | 4 ++-- types/types.go | 5 ++++- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/cmd/skopeo/layers.go b/cmd/skopeo/layers.go index 0ba56cf9..d3f11f9e 100644 --- a/cmd/skopeo/layers.go +++ b/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 diff --git a/image/image.go b/image/image.go index a8c33e8a..2e49d3c2 100644 --- a/image/image.go +++ b/image/image.go @@ -172,7 +172,7 @@ func (i *genericImage) getSchema1Manifest() (genericManifest, error) { return mschema1, nil } -func (i *genericImage) Layers(layers ...string) error { +func (i *genericImage) LayersCommand(layers ...string) error { m, err := i.getSchema1Manifest() if err != nil { return err diff --git a/signature/policy_reference_match_test.go b/signature/policy_reference_match_test.go index 0b00214b..adbf2a3c 100644 --- a/signature/policy_reference_match_test.go +++ b/signature/policy_reference_match_test.go @@ -55,7 +55,7 @@ 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) LayersCommand(layers ...string) error { panic("unexpected call to a mock function") } func (ref refImageMock) Inspect() (*types.ImageInspectInfo, error) { @@ -168,7 +168,7 @@ 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) LayersCommand(layers ...string) error { panic("unexpected call to a mock function") } func (ref forbiddenImageMock) Inspect() (*types.ImageInspectInfo, error) { diff --git a/types/types.go b/types/types.go index cacb58a5..c444718f 100644 --- a/types/types.go +++ b/types/types.go @@ -60,7 +60,10 @@ type Image interface { 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? + // 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 From a23befcbf488579b8a136ad930ca3da9fbe881c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Jun 2016 09:50:04 +0200 Subject: [PATCH 3/4] Add types.Image.LayerDigests, use it in (skopeo copy) To do so, have (skopeo copy) work with a types.Image, and replace uses of types.ImageSource with types.Image where possible to allow the caching in types.Image to work. This is a slight behavior change: - The manifest is now processed through fixManifestLayers - Duplicate layers (created e.g. when a non-filesystem-altering command is used in a Dockerfile) are only copied once. --- cmd/skopeo/copy.go | 48 +++++------------------- image/image.go | 41 ++++++++++++++++++-- image/image_test.go | 32 ++++++++++++++++ signature/policy_reference_match_test.go | 6 +++ types/types.go | 5 +++ 5 files changed, 89 insertions(+), 43 deletions(-) create mode 100644 image/image_test.go diff --git a/cmd/skopeo/copy.go b/cmd/skopeo/copy.go index 8b415457..3e9b3153 100644 --- a/cmd/skopeo/copy.go +++ b/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 diff --git a/image/image.go b/image/image.go index 2e49d3c2..d2bd2607 100644 --- a/image/image.go +++ b/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,6 +179,32 @@ func (i *genericImage) getSchema1Manifest() (genericManifest, error) { return mschema1, nil } +// 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 { diff --git a/image/image_test.go b/image/image_test.go new file mode 100644 index 00000000..ba1b653b --- /dev/null +++ b/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) + } + } +} diff --git a/signature/policy_reference_match_test.go b/signature/policy_reference_match_test.go index adbf2a3c..3e477356 100644 --- a/signature/policy_reference_match_test.go +++ b/signature/policy_reference_match_test.go @@ -55,6 +55,9 @@ func (ref refImageMock) ManifestMatchesDigest(expectedDigest string) (bool, erro func (ref refImageMock) Signatures() ([][]byte, error) { panic("unexpected call to a mock function") } +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") } @@ -168,6 +171,9 @@ func (ref forbiddenImageMock) ManifestMatchesDigest(expectedDigest string) (bool func (ref forbiddenImageMock) Signatures() ([][]byte, error) { panic("unexpected call to a mock function") } +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") } diff --git a/types/types.go b/types/types.go index c444718f..b7fbe680 100644 --- a/types/types.go +++ b/types/types.go @@ -57,9 +57,14 @@ 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. + // 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) + // 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. From 5b1ca76131ba03aedcd65964f1aeefa97642daf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Jun 2016 09:56:50 +0200 Subject: [PATCH 4/4] Only copy each layer once in (skopeo layers) ... using the new uniqueLayerDigests(). --- image/image.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/image/image.go b/image/image.go index d2bd2607..aced0802 100644 --- a/image/image.go +++ b/image/image.go @@ -223,7 +223,7 @@ func (i *genericImage) LayersCommand(layers ...string) error { return err } if len(layers) == 0 { - layers = m.GetLayers() + layers = uniqueLayerDigests(m) } for _, l := range layers { if !strings.HasPrefix(l, "sha256:") {