From a4024b2f9016ba94afe19dc047739a3b5965c92e Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Fri, 2 Jan 2015 13:21:29 -0800 Subject: [PATCH 1/4] Move manifest to discrete package Because manifests and their signatures are a discrete component of the registry, we are moving the definitions into a separate package. This causes us to lose some test coverage, but we can fill this in shortly. No changes have been made to the external interfaces, but they are likely to come. Signed-off-by: Stephen J Day --- api_test.go | 10 +-- client/client.go | 12 ++-- client/client_test.go | 64 +++++++++---------- client/objectstore.go | 12 ++-- client/pull.go | 6 +- client/push.go | 6 +- images.go | 3 +- {storage => manifest}/manifest.go | 4 +- storage/layer.go | 5 +- storage/layerstore.go | 5 +- storage/layerupload.go | 3 +- storage/manifeststore.go | 9 +-- ...manifest_test.go => manifeststore_test.go} | 10 +-- storage/services.go | 5 +- 14 files changed, 80 insertions(+), 74 deletions(-) rename {storage => manifest}/manifest.go (99%) rename storage/{manifest_test.go => manifeststore_test.go} (96%) diff --git a/api_test.go b/api_test.go index 76c3b348..d3085ade 100644 --- a/api_test.go +++ b/api_test.go @@ -17,7 +17,7 @@ import ( "github.com/docker/distribution/common/testutil" "github.com/docker/distribution/configuration" "github.com/docker/distribution/digest" - "github.com/docker/distribution/storage" + "github.com/docker/distribution/manifest" _ "github.com/docker/distribution/storagedriver/inmemory" "github.com/docker/libtrust" "github.com/gorilla/handlers" @@ -268,10 +268,10 @@ func TestManifestAPI(t *testing.T) { // -------------------------------- // Attempt to push unsigned manifest with missing layers - unsignedManifest := &storage.Manifest{ + unsignedManifest := &manifest.Manifest{ Name: imageName, Tag: tag, - FSLayers: []storage.FSLayer{ + FSLayers: []manifest.FSLayer{ { BlobSum: "asdf", }, @@ -362,7 +362,7 @@ func TestManifestAPI(t *testing.T) { checkResponse(t, "fetching uploaded manifest", resp, http.StatusOK) - var fetchedManifest storage.SignedManifest + var fetchedManifest manifest.SignedManifest dec = json.NewDecoder(resp.Body) if err := dec.Decode(&fetchedManifest); err != nil { t.Fatalf("error decoding fetched manifest: %v", err) @@ -404,7 +404,7 @@ func TestManifestAPI(t *testing.T) { func putManifest(t *testing.T, msg, url string, v interface{}) *http.Response { var body []byte - if sm, ok := v.(*storage.SignedManifest); ok { + if sm, ok := v.(*manifest.SignedManifest); ok { body = sm.Raw } else { var err error diff --git a/client/client.go b/client/client.go index d13d6f21..f53c0e15 100644 --- a/client/client.go +++ b/client/client.go @@ -12,18 +12,18 @@ import ( "github.com/docker/distribution/api/v2" "github.com/docker/distribution/digest" - "github.com/docker/distribution/storage" + "github.com/docker/distribution/manifest" ) // Client implements the client interface to the registry http api type Client interface { // GetImageManifest returns an image manifest for the image at the given // name, tag pair. - GetImageManifest(name, tag string) (*storage.SignedManifest, error) + GetImageManifest(name, tag string) (*manifest.SignedManifest, error) // PutImageManifest uploads an image manifest for the image at the given // name, tag pair. - PutImageManifest(name, tag string, imageManifest *storage.SignedManifest) error + PutImageManifest(name, tag string, imageManifest *manifest.SignedManifest) error // DeleteImage removes the image at the given name, tag pair. DeleteImage(name, tag string) error @@ -91,7 +91,7 @@ type clientImpl struct { // TODO(bbland): use consistent route generation between server and client -func (r *clientImpl) GetImageManifest(name, tag string) (*storage.SignedManifest, error) { +func (r *clientImpl) GetImageManifest(name, tag string) (*manifest.SignedManifest, error) { manifestURL, err := r.ub.BuildManifestURL(name, tag) if err != nil { return nil, err @@ -124,7 +124,7 @@ func (r *clientImpl) GetImageManifest(name, tag string) (*storage.SignedManifest decoder := json.NewDecoder(response.Body) - manifest := new(storage.SignedManifest) + manifest := new(manifest.SignedManifest) err = decoder.Decode(manifest) if err != nil { return nil, err @@ -132,7 +132,7 @@ func (r *clientImpl) GetImageManifest(name, tag string) (*storage.SignedManifest return manifest, nil } -func (r *clientImpl) PutImageManifest(name, tag string, manifest *storage.SignedManifest) error { +func (r *clientImpl) PutImageManifest(name, tag string, manifest *manifest.SignedManifest) error { manifestURL, err := r.ub.BuildManifestURL(name, tag) if err != nil { return err diff --git a/client/client_test.go b/client/client_test.go index bc881831..f32bc8f4 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -11,7 +11,7 @@ import ( "github.com/docker/distribution/common/testutil" "github.com/docker/distribution/digest" - "github.com/docker/distribution/storage" + "github.com/docker/distribution/manifest" ) type testBlob struct { @@ -33,8 +33,8 @@ func TestPush(t *testing.T) { }, } uploadLocations := make([]string, len(testBlobs)) - blobs := make([]storage.FSLayer, len(testBlobs)) - history := make([]storage.ManifestHistory, len(testBlobs)) + blobs := make([]manifest.FSLayer, len(testBlobs)) + history := make([]manifest.ManifestHistory, len(testBlobs)) for i, blob := range testBlobs { // TODO(bbland): this is returning the same location for all uploads, @@ -42,24 +42,24 @@ func TestPush(t *testing.T) { // It's sort of okay because we're using unique digests, but this needs // to change at some point. uploadLocations[i] = fmt.Sprintf("/v2/%s/blobs/test-uuid", name) - blobs[i] = storage.FSLayer{BlobSum: blob.digest} - history[i] = storage.ManifestHistory{V1Compatibility: blob.digest.String()} + blobs[i] = manifest.FSLayer{BlobSum: blob.digest} + history[i] = manifest.ManifestHistory{V1Compatibility: blob.digest.String()} } - manifest := &storage.SignedManifest{ - Manifest: storage.Manifest{ + m := &manifest.SignedManifest{ + Manifest: manifest.Manifest{ Name: name, Tag: tag, Architecture: "x86", FSLayers: blobs, History: history, - Versioned: storage.Versioned{ + Versioned: manifest.Versioned{ SchemaVersion: 1, }, }, } var err error - manifest.Raw, err = json.Marshal(manifest) + m.Raw, err = json.Marshal(m) blobRequestResponseMappings := make([]testutil.RequestResponseMapping, 2*len(testBlobs)) for i, blob := range testBlobs { @@ -94,7 +94,7 @@ func TestPush(t *testing.T) { Request: testutil.Request{ Method: "PUT", Route: "/v2/" + name + "/manifests/" + tag, - Body: manifest.Raw, + Body: m.Raw, }, Response: testutil.Response{ StatusCode: http.StatusOK, @@ -119,7 +119,7 @@ func TestPush(t *testing.T) { } objectStore := &memoryObjectStore{ mutex: new(sync.Mutex), - manifestStorage: make(map[string]*storage.SignedManifest), + manifestStorage: make(map[string]*manifest.SignedManifest), layerStorage: make(map[digest.Digest]Layer), } @@ -139,7 +139,7 @@ func TestPush(t *testing.T) { writer.Close() } - objectStore.WriteManifest(name, tag, manifest) + objectStore.WriteManifest(name, tag, m) err = Push(client, objectStore, name, tag) if err != nil { @@ -160,27 +160,27 @@ func TestPull(t *testing.T) { contents: []byte("some other contents"), }, } - blobs := make([]storage.FSLayer, len(testBlobs)) - history := make([]storage.ManifestHistory, len(testBlobs)) + blobs := make([]manifest.FSLayer, len(testBlobs)) + history := make([]manifest.ManifestHistory, len(testBlobs)) for i, blob := range testBlobs { - blobs[i] = storage.FSLayer{BlobSum: blob.digest} - history[i] = storage.ManifestHistory{V1Compatibility: blob.digest.String()} + blobs[i] = manifest.FSLayer{BlobSum: blob.digest} + history[i] = manifest.ManifestHistory{V1Compatibility: blob.digest.String()} } - manifest := &storage.SignedManifest{ - Manifest: storage.Manifest{ + m := &manifest.SignedManifest{ + Manifest: manifest.Manifest{ Name: name, Tag: tag, Architecture: "x86", FSLayers: blobs, History: history, - Versioned: storage.Versioned{ + Versioned: manifest.Versioned{ SchemaVersion: 1, }, }, } - manifestBytes, err := json.Marshal(manifest) + manifestBytes, err := json.Marshal(m) blobRequestResponseMappings := make([]testutil.RequestResponseMapping, len(testBlobs)) for i, blob := range testBlobs { @@ -213,7 +213,7 @@ func TestPull(t *testing.T) { } objectStore := &memoryObjectStore{ mutex: new(sync.Mutex), - manifestStorage: make(map[string]*storage.SignedManifest), + manifestStorage: make(map[string]*manifest.SignedManifest), layerStorage: make(map[digest.Digest]Layer), } @@ -222,7 +222,7 @@ func TestPull(t *testing.T) { t.Fatal(err) } - m, err := objectStore.Manifest(name, tag) + m, err = objectStore.Manifest(name, tag) if err != nil { t.Fatal(err) } @@ -272,25 +272,25 @@ func TestPullResume(t *testing.T) { contents: []byte("some other contents"), }, } - layers := make([]storage.FSLayer, len(testBlobs)) - history := make([]storage.ManifestHistory, len(testBlobs)) + layers := make([]manifest.FSLayer, len(testBlobs)) + history := make([]manifest.ManifestHistory, len(testBlobs)) for i, layer := range testBlobs { - layers[i] = storage.FSLayer{BlobSum: layer.digest} - history[i] = storage.ManifestHistory{V1Compatibility: layer.digest.String()} + layers[i] = manifest.FSLayer{BlobSum: layer.digest} + history[i] = manifest.ManifestHistory{V1Compatibility: layer.digest.String()} } - manifest := &storage.Manifest{ + m := &manifest.Manifest{ Name: name, Tag: tag, Architecture: "x86", FSLayers: layers, History: history, - Versioned: storage.Versioned{ + Versioned: manifest.Versioned{ SchemaVersion: 1, }, } - manifestBytes, err := json.Marshal(manifest) + manifestBytes, err := json.Marshal(m) layerRequestResponseMappings := make([]testutil.RequestResponseMapping, 2*len(testBlobs)) for i, blob := range testBlobs { @@ -340,7 +340,7 @@ func TestPullResume(t *testing.T) { } objectStore := &memoryObjectStore{ mutex: new(sync.Mutex), - manifestStorage: make(map[string]*storage.SignedManifest), + manifestStorage: make(map[string]*manifest.SignedManifest), layerStorage: make(map[digest.Digest]Layer), } @@ -355,12 +355,12 @@ func TestPullResume(t *testing.T) { t.Fatal(err) } - m, err := objectStore.Manifest(name, tag) + sm, err := objectStore.Manifest(name, tag) if err != nil { t.Fatal(err) } - mBytes, err := json.Marshal(m) + mBytes, err := json.Marshal(sm) if err != nil { t.Fatal(err) } diff --git a/client/objectstore.go b/client/objectstore.go index 5df7f699..5969c9d2 100644 --- a/client/objectstore.go +++ b/client/objectstore.go @@ -7,7 +7,7 @@ import ( "sync" "github.com/docker/distribution/digest" - "github.com/docker/distribution/storage" + "github.com/docker/distribution/manifest" ) var ( @@ -26,11 +26,11 @@ var ( type ObjectStore interface { // Manifest retrieves the image manifest stored at the given repository name // and tag - Manifest(name, tag string) (*storage.SignedManifest, error) + Manifest(name, tag string) (*manifest.SignedManifest, error) // WriteManifest stores an image manifest at the given repository name and // tag - WriteManifest(name, tag string, manifest *storage.SignedManifest) error + WriteManifest(name, tag string, manifest *manifest.SignedManifest) error // Layer returns a handle to a layer for reading and writing Layer(dgst digest.Digest) (Layer, error) @@ -83,11 +83,11 @@ type LayerWriter interface { // memoryObjectStore is an in-memory implementation of the ObjectStore interface type memoryObjectStore struct { mutex *sync.Mutex - manifestStorage map[string]*storage.SignedManifest + manifestStorage map[string]*manifest.SignedManifest layerStorage map[digest.Digest]Layer } -func (objStore *memoryObjectStore) Manifest(name, tag string) (*storage.SignedManifest, error) { +func (objStore *memoryObjectStore) Manifest(name, tag string) (*manifest.SignedManifest, error) { objStore.mutex.Lock() defer objStore.mutex.Unlock() @@ -98,7 +98,7 @@ func (objStore *memoryObjectStore) Manifest(name, tag string) (*storage.SignedMa return manifest, nil } -func (objStore *memoryObjectStore) WriteManifest(name, tag string, manifest *storage.SignedManifest) error { +func (objStore *memoryObjectStore) WriteManifest(name, tag string, manifest *manifest.SignedManifest) error { objStore.mutex.Lock() defer objStore.mutex.Unlock() diff --git a/client/pull.go b/client/pull.go index c7f3c25c..385158db 100644 --- a/client/pull.go +++ b/client/pull.go @@ -4,9 +4,9 @@ import ( "fmt" "io" - "github.com/docker/distribution/storage" - log "github.com/Sirupsen/logrus" + + "github.com/docker/distribution/manifest" ) // simultaneousLayerPullWindow is the size of the parallel layer pull window. @@ -77,7 +77,7 @@ func Pull(c Client, objectStore ObjectStore, name, tag string) error { return nil } -func pullLayer(c Client, objectStore ObjectStore, name string, fsLayer storage.FSLayer) error { +func pullLayer(c Client, objectStore ObjectStore, name string, fsLayer manifest.FSLayer) error { log.WithField("layer", fsLayer).Info("Pulling layer") layer, err := objectStore.Layer(fsLayer.BlobSum) diff --git a/client/push.go b/client/push.go index 7626cd16..c26bd174 100644 --- a/client/push.go +++ b/client/push.go @@ -4,7 +4,7 @@ import ( "fmt" log "github.com/Sirupsen/logrus" - "github.com/docker/distribution/storage" + "github.com/docker/distribution/manifest" ) // simultaneousLayerPushWindow is the size of the parallel layer push window. @@ -12,7 +12,7 @@ import ( // push window has been successfully pushed. const simultaneousLayerPushWindow = 4 -type pushFunction func(fsLayer storage.FSLayer) error +type pushFunction func(fsLayer manifest.FSLayer) error // Push implements a client push workflow for the image defined by the given // name and tag pair, using the given ObjectStore for local manifest and layer @@ -71,7 +71,7 @@ func Push(c Client, objectStore ObjectStore, name, tag string) error { return nil } -func pushLayer(c Client, objectStore ObjectStore, name string, fsLayer storage.FSLayer) error { +func pushLayer(c Client, objectStore ObjectStore, name string, fsLayer manifest.FSLayer) error { log.WithField("layer", fsLayer).Info("Pushing layer") layer, err := objectStore.Layer(fsLayer.BlobSum) diff --git a/images.go b/images.go index 6a972109..95b3917f 100644 --- a/images.go +++ b/images.go @@ -7,6 +7,7 @@ import ( "github.com/docker/distribution/api/v2" "github.com/docker/distribution/digest" + "github.com/docker/distribution/manifest" "github.com/docker/distribution/storage" "github.com/gorilla/handlers" ) @@ -56,7 +57,7 @@ func (imh *imageManifestHandler) PutImageManifest(w http.ResponseWriter, r *http manifests := imh.services.Manifests() dec := json.NewDecoder(r.Body) - var manifest storage.SignedManifest + var manifest manifest.SignedManifest if err := dec.Decode(&manifest); err != nil { imh.Errors.Push(v2.ErrorCodeManifestInvalid, err) w.WriteHeader(http.StatusBadRequest) diff --git a/storage/manifest.go b/manifest/manifest.go similarity index 99% rename from storage/manifest.go rename to manifest/manifest.go index be70a868..75816a80 100644 --- a/storage/manifest.go +++ b/manifest/manifest.go @@ -1,4 +1,4 @@ -package storage +package manifest import ( "crypto/x509" @@ -149,7 +149,7 @@ func (sm *SignedManifest) UnmarshalJSON(b []byte) error { // MarshalJSON returns the contents of raw. If Raw is nil, marshals the inner // contents. Applications requiring a marshaled signed manifest should simply -// use Raw directly, since the the content produced by json.Marshal will +// use Raw directly, since the the content produced by json.Marshal will be // compacted and will fail signature checks. func (sm *SignedManifest) MarshalJSON() ([]byte, error) { if len(sm.Raw) > 0 { diff --git a/storage/layer.go b/storage/layer.go index 4bb6699c..ec5f0f9d 100644 --- a/storage/layer.go +++ b/storage/layer.go @@ -6,6 +6,7 @@ import ( "time" "github.com/docker/distribution/digest" + "github.com/docker/distribution/manifest" ) // Layer provides a readable and seekable layer object. Typically, @@ -74,7 +75,7 @@ var ( // ErrUnknownLayer returned when layer cannot be found. type ErrUnknownLayer struct { - FSLayer FSLayer + FSLayer manifest.FSLayer } func (err ErrUnknownLayer) Error() string { @@ -83,7 +84,7 @@ func (err ErrUnknownLayer) Error() string { // ErrLayerInvalidDigest returned when tarsum check fails. type ErrLayerInvalidDigest struct { - FSLayer FSLayer + FSLayer manifest.FSLayer } func (err ErrLayerInvalidDigest) Error() string { diff --git a/storage/layerstore.go b/storage/layerstore.go index 7b5e05f7..d945b767 100644 --- a/storage/layerstore.go +++ b/storage/layerstore.go @@ -2,6 +2,7 @@ package storage import ( "github.com/docker/distribution/digest" + "github.com/docker/distribution/manifest" "github.com/docker/distribution/storagedriver" ) @@ -33,7 +34,7 @@ func (ls *layerStore) Fetch(name string, digest digest.Digest) (Layer, error) { if err != nil { switch err := err.(type) { case storagedriver.PathNotFoundError, *storagedriver.PathNotFoundError: - return nil, ErrUnknownLayer{FSLayer{BlobSum: digest}} + return nil, ErrUnknownLayer{manifest.FSLayer{BlobSum: digest}} default: return nil, err } @@ -43,7 +44,7 @@ func (ls *layerStore) Fetch(name string, digest digest.Digest) (Layer, error) { if err != nil { switch err := err.(type) { case storagedriver.PathNotFoundError, *storagedriver.PathNotFoundError: - return nil, ErrUnknownLayer{FSLayer{BlobSum: digest}} + return nil, ErrUnknownLayer{manifest.FSLayer{BlobSum: digest}} default: return nil, err } diff --git a/storage/layerupload.go b/storage/layerupload.go index 0754a7e2..4c991d7c 100644 --- a/storage/layerupload.go +++ b/storage/layerupload.go @@ -10,6 +10,7 @@ import ( "code.google.com/p/go-uuid/uuid" "github.com/docker/distribution/digest" + "github.com/docker/distribution/manifest" "github.com/docker/distribution/storagedriver" "github.com/docker/docker/pkg/tarsum" @@ -284,7 +285,7 @@ func (luc *layerUploadController) validateLayer(fp layerFile, size int64, dgst d } if !digestVerifier.Verified() { - return "", ErrLayerInvalidDigest{FSLayer{BlobSum: dgst}} + return "", ErrLayerInvalidDigest{manifest.FSLayer{BlobSum: dgst}} } return dgst, nil diff --git a/storage/manifeststore.go b/storage/manifeststore.go index 25e55107..8e5be6a7 100644 --- a/storage/manifeststore.go +++ b/storage/manifeststore.go @@ -6,6 +6,7 @@ import ( "path" "strings" + "github.com/docker/distribution/manifest" "github.com/docker/distribution/storagedriver" "github.com/docker/libtrust" ) @@ -116,7 +117,7 @@ func (ms *manifestStore) Exists(name, tag string) (bool, error) { return true, nil } -func (ms *manifestStore) Get(name, tag string) (*SignedManifest, error) { +func (ms *manifestStore) Get(name, tag string) (*manifest.SignedManifest, error) { p, err := ms.path(name, tag) if err != nil { return nil, err @@ -132,7 +133,7 @@ func (ms *manifestStore) Get(name, tag string) (*SignedManifest, error) { } } - var manifest SignedManifest + var manifest manifest.SignedManifest if err := json.Unmarshal(content, &manifest); err != nil { // TODO(stevvooe): Corrupted manifest error? @@ -144,7 +145,7 @@ func (ms *manifestStore) Get(name, tag string) (*SignedManifest, error) { return &manifest, nil } -func (ms *manifestStore) Put(name, tag string, manifest *SignedManifest) error { +func (ms *manifestStore) Put(name, tag string, manifest *manifest.SignedManifest) error { p, err := ms.path(name, tag) if err != nil { return err @@ -185,7 +186,7 @@ func (ms *manifestStore) path(name, tag string) (string, error) { }) } -func (ms *manifestStore) verifyManifest(name, tag string, manifest *SignedManifest) error { +func (ms *manifestStore) verifyManifest(name, tag string, manifest *manifest.SignedManifest) error { // TODO(stevvooe): This verification is present here, but this needs to be // lifted out of the storage infrastructure and moved into a package // oriented towards defining verifiers and reporting them with diff --git a/storage/manifest_test.go b/storage/manifeststore_test.go similarity index 96% rename from storage/manifest_test.go rename to storage/manifeststore_test.go index 3bfee2dd..313c30cf 100644 --- a/storage/manifest_test.go +++ b/storage/manifeststore_test.go @@ -4,10 +4,10 @@ import ( "reflect" "testing" - "github.com/docker/libtrust" - "github.com/docker/distribution/digest" + "github.com/docker/distribution/manifest" "github.com/docker/distribution/storagedriver/inmemory" + "github.com/docker/libtrust" ) func TestManifestStorage(t *testing.T) { @@ -42,13 +42,13 @@ func TestManifestStorage(t *testing.T) { } } - manifest := Manifest{ - Versioned: Versioned{ + manifest := manifest.Manifest{ + Versioned: manifest.Versioned{ SchemaVersion: 1, }, Name: name, Tag: tag, - FSLayers: []FSLayer{ + FSLayers: []manifest.FSLayer{ { BlobSum: "asdf", }, diff --git a/storage/services.go b/storage/services.go index ed33f936..a6025581 100644 --- a/storage/services.go +++ b/storage/services.go @@ -2,6 +2,7 @@ package storage import ( "github.com/docker/distribution/digest" + "github.com/docker/distribution/manifest" "github.com/docker/distribution/storagedriver" ) @@ -59,10 +60,10 @@ type ManifestService interface { Exists(name, tag string) (bool, error) // Get retrieves the named manifest, if it exists. - Get(name, tag string) (*SignedManifest, error) + Get(name, tag string) (*manifest.SignedManifest, error) // Put creates or updates the named manifest. - Put(name, tag string, manifest *SignedManifest) error + Put(name, tag string, manifest *manifest.SignedManifest) error // Delete removes the named manifest, if it exists. Delete(name, tag string) error From 579aa3b617438433d5c9f14c833c5348bf453818 Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Fri, 2 Jan 2015 15:24:10 -0800 Subject: [PATCH 2/4] Add unit tests for manifest package Signed-off-by: Stephen J Day --- manifest/manifest_test.go | 110 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) create mode 100644 manifest/manifest_test.go diff --git a/manifest/manifest_test.go b/manifest/manifest_test.go new file mode 100644 index 00000000..5d564ec8 --- /dev/null +++ b/manifest/manifest_test.go @@ -0,0 +1,110 @@ +package manifest + +import ( + "bytes" + "encoding/json" + "reflect" + "testing" + + "github.com/docker/libtrust" +) + +type testEnv struct { + name, tag string + manifest *Manifest + signed *SignedManifest + pk libtrust.PrivateKey +} + +func TestManifestMarshaling(t *testing.T) { + env := genEnv(t) + + // Check that the Raw field is the same as json.MarshalIndent with these + // parameters. + p, err := json.MarshalIndent(env.signed, "", " ") + if err != nil { + t.Fatalf("error marshaling manifest: %v", err) + } + + if !bytes.Equal(p, env.signed.Raw) { + t.Fatalf("manifest bytes not equal: %q != %q", string(env.signed.Raw), string(p)) + } +} + +func TestManifestUnmarshaling(t *testing.T) { + env := genEnv(t) + + var signed SignedManifest + if err := json.Unmarshal(env.signed.Raw, &signed); err != nil { + t.Fatalf("error unmarshaling signed manifest: %v", err) + } + + if !reflect.DeepEqual(&signed, env.signed) { + t.Fatalf("manifests are different after unmarshaling: %v != %v", signed, env.signed) + } +} + +func TestManifestVerification(t *testing.T) { + env := genEnv(t) + + publicKeys, err := env.signed.Verify() + if err != nil { + t.Fatalf("error verifying manifest: %v", err) + } + + if len(publicKeys) == 0 { + t.Fatalf("no public keys found in signature") + } + + var found bool + publicKey := env.pk.PublicKey() + // ensure that one of the extracted public keys matches the private key. + for _, candidate := range publicKeys { + if candidate.KeyID() == publicKey.KeyID() { + found = true + break + } + } + + if !found { + t.Fatalf("expected public key, %v, not found in verified keys: %v", publicKey, publicKeys) + } +} + +func genEnv(t *testing.T) *testEnv { + pk, err := libtrust.GenerateECP256PrivateKey() + if err != nil { + t.Fatalf("error generating test key: %v", err) + } + + name, tag := "foo/bar", "test" + + m := Manifest{ + Versioned: Versioned{ + SchemaVersion: 1, + }, + Name: name, + Tag: tag, + FSLayers: []FSLayer{ + { + BlobSum: "asdf", + }, + { + BlobSum: "qwer", + }, + }, + } + + sm, err := m.Sign(pk) + if err != nil { + t.Fatalf("error signing manifest: %v", err) + } + + return &testEnv{ + name: name, + tag: tag, + manifest: &m, + signed: sm, + pk: pk, + } +} From f1f610c6cd6124a58441b3e659e601a468855f65 Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Fri, 2 Jan 2015 15:46:47 -0800 Subject: [PATCH 3/4] Decouple manifest signing and verification It was probably ill-advised to couple manifest signing and verification to their respective types. This changeset simply changes them from methods to functions. These might not even be in this package in the future. Signed-off-by: Stephen J Day --- api_test.go | 2 +- manifest/manifest.go | 85 ----------------------------------- manifest/manifest_test.go | 4 +- manifest/sign.go | 66 +++++++++++++++++++++++++++ manifest/verify.go | 32 +++++++++++++ storage/manifeststore.go | 10 ++--- storage/manifeststore_test.go | 4 +- 7 files changed, 108 insertions(+), 95 deletions(-) create mode 100644 manifest/sign.go create mode 100644 manifest/verify.go diff --git a/api_test.go b/api_test.go index d3085ade..6e8c403c 100644 --- a/api_test.go +++ b/api_test.go @@ -345,7 +345,7 @@ func TestManifestAPI(t *testing.T) { // ------------------- // Push the signed manifest with all layers pushed. - signedManifest, err := unsignedManifest.Sign(pk) + signedManifest, err := manifest.Sign(unsignedManifest, pk) if err != nil { t.Fatalf("unexpected error signing manifest: %v", err) } diff --git a/manifest/manifest.go b/manifest/manifest.go index 75816a80..3fc1dc45 100644 --- a/manifest/manifest.go +++ b/manifest/manifest.go @@ -1,12 +1,9 @@ package manifest import ( - "crypto/x509" "encoding/json" - "github.com/Sirupsen/logrus" "github.com/docker/distribution/digest" - "github.com/docker/libtrust" ) // Versioned provides a struct with just the manifest schemaVersion. Incoming @@ -39,64 +36,6 @@ type Manifest struct { History []ManifestHistory `json:"history"` } -// Sign signs the manifest with the provided private key, returning a -// SignedManifest. This typically won't be used within the registry, except -// for testing. -func (m *Manifest) Sign(pk libtrust.PrivateKey) (*SignedManifest, error) { - p, err := json.MarshalIndent(m, "", " ") - if err != nil { - return nil, err - } - - js, err := libtrust.NewJSONSignature(p) - if err != nil { - return nil, err - } - - if err := js.Sign(pk); err != nil { - return nil, err - } - - pretty, err := js.PrettySignature("signatures") - if err != nil { - return nil, err - } - - return &SignedManifest{ - Manifest: *m, - Raw: pretty, - }, nil -} - -// SignWithChain signs the manifest with the given private key and x509 chain. -// The public key of the first element in the chain must be the public key -// corresponding with the sign key. -func (m *Manifest) SignWithChain(key libtrust.PrivateKey, chain []*x509.Certificate) (*SignedManifest, error) { - p, err := json.MarshalIndent(m, "", " ") - if err != nil { - return nil, err - } - - js, err := libtrust.NewJSONSignature(p) - if err != nil { - return nil, err - } - - if err := js.SignWithChain(key, chain); err != nil { - return nil, err - } - - pretty, err := js.PrettySignature("signatures") - if err != nil { - return nil, err - } - - return &SignedManifest{ - Manifest: *m, - Raw: pretty, - }, nil -} - // SignedManifest provides an envelope for a signed image manifest, including // the format sensitive raw bytes. It contains fields to type SignedManifest struct { @@ -109,30 +48,6 @@ type SignedManifest struct { Raw []byte `json:"-"` } -// Verify verifies the signature of the signed manifest returning the public -// keys used during signing. -func (sm *SignedManifest) Verify() ([]libtrust.PublicKey, error) { - js, err := libtrust.ParsePrettySignature(sm.Raw, "signatures") - if err != nil { - logrus.WithField("err", err).Debugf("(*SignedManifest).Verify") - return nil, err - } - - return js.Verify() -} - -// VerifyChains verifies the signature of the signed manifest against the -// certificate pool returning the list of verified chains. Signatures without -// an x509 chain are not checked. -func (sm *SignedManifest) VerifyChains(ca *x509.CertPool) ([][]*x509.Certificate, error) { - js, err := libtrust.ParsePrettySignature(sm.Raw, "signatures") - if err != nil { - return nil, err - } - - return js.VerifyChains(ca) -} - // UnmarshalJSON populates a new ImageManifest struct from JSON data. func (sm *SignedManifest) UnmarshalJSON(b []byte) error { var manifest Manifest diff --git a/manifest/manifest_test.go b/manifest/manifest_test.go index 5d564ec8..941bfde9 100644 --- a/manifest/manifest_test.go +++ b/manifest/manifest_test.go @@ -47,7 +47,7 @@ func TestManifestUnmarshaling(t *testing.T) { func TestManifestVerification(t *testing.T) { env := genEnv(t) - publicKeys, err := env.signed.Verify() + publicKeys, err := Verify(env.signed) if err != nil { t.Fatalf("error verifying manifest: %v", err) } @@ -95,7 +95,7 @@ func genEnv(t *testing.T) *testEnv { }, } - sm, err := m.Sign(pk) + sm, err := Sign(&m, pk) if err != nil { t.Fatalf("error signing manifest: %v", err) } diff --git a/manifest/sign.go b/manifest/sign.go new file mode 100644 index 00000000..a4c37652 --- /dev/null +++ b/manifest/sign.go @@ -0,0 +1,66 @@ +package manifest + +import ( + "crypto/x509" + "encoding/json" + + "github.com/docker/libtrust" +) + +// Sign signs the manifest with the provided private key, returning a +// SignedManifest. This typically won't be used within the registry, except +// for testing. +func Sign(m *Manifest, pk libtrust.PrivateKey) (*SignedManifest, error) { + p, err := json.MarshalIndent(m, "", " ") + if err != nil { + return nil, err + } + + js, err := libtrust.NewJSONSignature(p) + if err != nil { + return nil, err + } + + if err := js.Sign(pk); err != nil { + return nil, err + } + + pretty, err := js.PrettySignature("signatures") + if err != nil { + return nil, err + } + + return &SignedManifest{ + Manifest: *m, + Raw: pretty, + }, nil +} + +// SignWithChain signs the manifest with the given private key and x509 chain. +// The public key of the first element in the chain must be the public key +// corresponding with the sign key. +func SignWithChain(m *Manifest, key libtrust.PrivateKey, chain []*x509.Certificate) (*SignedManifest, error) { + p, err := json.MarshalIndent(m, "", " ") + if err != nil { + return nil, err + } + + js, err := libtrust.NewJSONSignature(p) + if err != nil { + return nil, err + } + + if err := js.SignWithChain(key, chain); err != nil { + return nil, err + } + + pretty, err := js.PrettySignature("signatures") + if err != nil { + return nil, err + } + + return &SignedManifest{ + Manifest: *m, + Raw: pretty, + }, nil +} diff --git a/manifest/verify.go b/manifest/verify.go new file mode 100644 index 00000000..3e051b26 --- /dev/null +++ b/manifest/verify.go @@ -0,0 +1,32 @@ +package manifest + +import ( + "crypto/x509" + + "github.com/Sirupsen/logrus" + "github.com/docker/libtrust" +) + +// Verify verifies the signature of the signed manifest returning the public +// keys used during signing. +func Verify(sm *SignedManifest) ([]libtrust.PublicKey, error) { + js, err := libtrust.ParsePrettySignature(sm.Raw, "signatures") + if err != nil { + logrus.WithField("err", err).Debugf("(*SignedManifest).Verify") + return nil, err + } + + return js.Verify() +} + +// VerifyChains verifies the signature of the signed manifest against the +// certificate pool returning the list of verified chains. Signatures without +// an x509 chain are not checked. +func VerifyChains(sm *SignedManifest, ca *x509.CertPool) ([][]*x509.Certificate, error) { + js, err := libtrust.ParsePrettySignature(sm.Raw, "signatures") + if err != nil { + return nil, err + } + + return js.VerifyChains(ca) +} diff --git a/storage/manifeststore.go b/storage/manifeststore.go index 8e5be6a7..af16dcf3 100644 --- a/storage/manifeststore.go +++ b/storage/manifeststore.go @@ -186,19 +186,19 @@ func (ms *manifestStore) path(name, tag string) (string, error) { }) } -func (ms *manifestStore) verifyManifest(name, tag string, manifest *manifest.SignedManifest) error { +func (ms *manifestStore) verifyManifest(name, tag string, mnfst *manifest.SignedManifest) error { // TODO(stevvooe): This verification is present here, but this needs to be // lifted out of the storage infrastructure and moved into a package // oriented towards defining verifiers and reporting them with // granularity. var errs ErrManifestVerification - if manifest.Name != name { + if mnfst.Name != name { // TODO(stevvooe): This needs to be an exported error errs = append(errs, fmt.Errorf("name does not match manifest name")) } - if manifest.Tag != tag { + if mnfst.Tag != tag { // TODO(stevvooe): This needs to be an exported error. errs = append(errs, fmt.Errorf("tag does not match manifest tag")) } @@ -207,7 +207,7 @@ func (ms *manifestStore) verifyManifest(name, tag string, manifest *manifest.Sig // VerifyWithChains. We need to define the exact source of the CA. // Perhaps, its a configuration value injected into manifest store. - if _, err := manifest.Verify(); err != nil { + if _, err := manifest.Verify(mnfst); err != nil { switch err { case libtrust.ErrMissingSignatureKey, libtrust.ErrInvalidJSONContent, libtrust.ErrMissingSignatureKey: errs = append(errs, ErrManifestUnverified{}) @@ -220,7 +220,7 @@ func (ms *manifestStore) verifyManifest(name, tag string, manifest *manifest.Sig } } - for _, fsLayer := range manifest.FSLayers { + for _, fsLayer := range mnfst.FSLayers { exists, err := ms.layerService.Exists(name, fsLayer.BlobSum) if err != nil { errs = append(errs, err) diff --git a/storage/manifeststore_test.go b/storage/manifeststore_test.go index 313c30cf..a6a00aa1 100644 --- a/storage/manifeststore_test.go +++ b/storage/manifeststore_test.go @@ -42,7 +42,7 @@ func TestManifestStorage(t *testing.T) { } } - manifest := manifest.Manifest{ + m := manifest.Manifest{ Versioned: manifest.Versioned{ SchemaVersion: 1, }, @@ -63,7 +63,7 @@ func TestManifestStorage(t *testing.T) { t.Fatalf("unexpected error generating private key: %v", err) } - sm, err := manifest.Sign(pk) + sm, err := manifest.Sign(&m, pk) if err != nil { t.Fatalf("error signing manifest: %v", err) } From 2653f7377965ee7a8a177fcaa98e663194536afc Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Fri, 2 Jan 2015 17:54:01 -0800 Subject: [PATCH 4/4] Rename History object to comply with golint Signed-off-by: Stephen J Day --- client/client_test.go | 12 ++++++------ manifest/manifest.go | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/client/client_test.go b/client/client_test.go index f32bc8f4..76867bcf 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -34,7 +34,7 @@ func TestPush(t *testing.T) { } uploadLocations := make([]string, len(testBlobs)) blobs := make([]manifest.FSLayer, len(testBlobs)) - history := make([]manifest.ManifestHistory, len(testBlobs)) + history := make([]manifest.History, len(testBlobs)) for i, blob := range testBlobs { // TODO(bbland): this is returning the same location for all uploads, @@ -43,7 +43,7 @@ func TestPush(t *testing.T) { // to change at some point. uploadLocations[i] = fmt.Sprintf("/v2/%s/blobs/test-uuid", name) blobs[i] = manifest.FSLayer{BlobSum: blob.digest} - history[i] = manifest.ManifestHistory{V1Compatibility: blob.digest.String()} + history[i] = manifest.History{V1Compatibility: blob.digest.String()} } m := &manifest.SignedManifest{ @@ -161,11 +161,11 @@ func TestPull(t *testing.T) { }, } blobs := make([]manifest.FSLayer, len(testBlobs)) - history := make([]manifest.ManifestHistory, len(testBlobs)) + history := make([]manifest.History, len(testBlobs)) for i, blob := range testBlobs { blobs[i] = manifest.FSLayer{BlobSum: blob.digest} - history[i] = manifest.ManifestHistory{V1Compatibility: blob.digest.String()} + history[i] = manifest.History{V1Compatibility: blob.digest.String()} } m := &manifest.SignedManifest{ @@ -273,11 +273,11 @@ func TestPullResume(t *testing.T) { }, } layers := make([]manifest.FSLayer, len(testBlobs)) - history := make([]manifest.ManifestHistory, len(testBlobs)) + history := make([]manifest.History, len(testBlobs)) for i, layer := range testBlobs { layers[i] = manifest.FSLayer{BlobSum: layer.digest} - history[i] = manifest.ManifestHistory{V1Compatibility: layer.digest.String()} + history[i] = manifest.History{V1Compatibility: layer.digest.String()} } m := &manifest.Manifest{ diff --git a/manifest/manifest.go b/manifest/manifest.go index 3fc1dc45..ad3ecd00 100644 --- a/manifest/manifest.go +++ b/manifest/manifest.go @@ -33,7 +33,7 @@ type Manifest struct { FSLayers []FSLayer `json:"fsLayers"` // History is a list of unstructured historical data for v1 compatibility - History []ManifestHistory `json:"history"` + History []History `json:"history"` } // SignedManifest provides an envelope for a signed image manifest, including @@ -81,8 +81,8 @@ type FSLayer struct { BlobSum digest.Digest `json:"blobSum"` } -// ManifestHistory stores unstructured v1 compatibility information -type ManifestHistory struct { +// History stores unstructured v1 compatibility information +type History struct { // V1Compatibility is the raw v1 compatibility information V1Compatibility string `json:"v1Compatibility"` }