Validate adding digests to tagstore with go types

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

daemon/daemon.go

@@ -1024,20 +1024,16 @@ func (daemon *Daemon) changes(container *Container) ([]archive.Change, error) {
 // imageName. If force is true, an existing tag with the same name may be
 // overwritten.
 func (daemon *Daemon) TagImage(newTag reference.Named, imageName string, force bool) error {
-	if _, isDigested := newTag.(reference.Digested); isDigested {
-		return errors.New("refusing to create a tag with a digest reference")
-	}
-	if newTag.Name() == string(digest.Canonical) {
-		return errors.New("refusing to create an ambiguous tag using digest algorithm as name")
-	}
-
-	newTag = registry.NormalizeLocalReference(newTag)
 	imageID, err := daemon.GetImageID(imageName)
 	if err != nil {
 		return err
 	}
+	newTag = registry.NormalizeLocalReference(newTag)
+	if err := daemon.tagStore.AddTag(newTag, imageID, force); err != nil {
+		return err
+	}
 	daemon.EventsService.Log("tag", newTag.String(), "")
-	return daemon.tagStore.Add(newTag, imageID, force)
+	return nil
 }
 
 // PullImage initiates a pull operation. image is the repository name to pull, and

daemon/daemon_windows.go

@@ -195,7 +195,7 @@ func restoreCustomImage(driver graphdriver.Driver, is image.Store, ls layer.Stor
 				return err
 			}
 
-			if err := ts.Add(ref, id, true); err != nil {
+			if err := ts.AddTag(ref, id, true); err != nil {
 				return err
 			}
 

distribution/pull_v1.go

@@ -332,7 +332,7 @@ func (p *v1Puller) pullImage(out io.Writer, v1ID, endpoint string, localNameRef
 		return layersDownloaded, err
 	}
 
-	if err := p.config.TagStore.Add(localNameRef, imageID, true); err != nil {
+	if err := p.config.TagStore.AddTag(localNameRef, imageID, true); err != nil {
 		return layersDownloaded, err
 	}
 

distribution/pull_v2.go

@@ -406,7 +406,11 @@ func (p *v2Puller) pullV2Tag(out io.Writer, ref reference.Named) (tagUpdated boo
 	}
 
 	if tagUpdated {
-		if err = p.config.TagStore.Add(ref, imageID, true); err != nil {
+		if canonical, ok := ref.(reference.Canonical); ok {
+			if err = p.config.TagStore.AddDigest(canonical, imageID, true); err != nil {
+				return false, err
+			}
+		} else if err = p.config.TagStore.AddTag(ref, imageID, true); err != nil {
 			return false, err
 		}
 	}

image/tarexport/load.go

@@ -128,7 +128,7 @@ func (l *tarexporter) setLoadedTag(ref reference.NamedTagged, imgID image.ID, ou
 		fmt.Fprintf(outStream, "The image %s already exists, renaming the old one with ID %s to empty string\n", ref.String(), string(prevID)) // todo: this message is wrong in case of multiple tags
 	}
 
-	if err := l.ts.Add(ref, imgID, true); err != nil {
+	if err := l.ts.AddTag(ref, imgID, true); err != nil {
 		return err
 	}
 	return nil

migrate/v1/migratev1.go

@@ -190,7 +190,8 @@ func migrateContainers(root string, ls graphIDMounter, is image.Store, imageMapp
 }
 
 type tagAdder interface {
-	Add(ref reference.Named, id image.ID, force bool) error
+	AddTag(ref reference.Named, id image.ID, force bool) error
+	AddDigest(ref reference.Canonical, id image.ID, force bool) error
 }
 
 func migrateTags(root, driverName string, ts tagAdder, mappings map[string]image.ID) error {
@@ -226,20 +227,23 @@ func migrateTags(root, driverName string, ts tagAdder, mappings map[string]image
 					continue
 				}
 				if dgst, err := digest.ParseDigest(tag); err == nil {
-					ref, err = reference.WithDigest(ref, dgst)
+					canonical, err := reference.WithDigest(ref, dgst)
 					if err != nil {
 						logrus.Errorf("migrate tags: invalid digest %q, %q", dgst, err)
 						continue
 					}
+					if err := ts.AddDigest(canonical, strongID, false); err != nil {
+						logrus.Errorf("can't migrate digest %q for %q, err: %q", ref.String(), strongID, err)
+					}
 				} else {
-					ref, err = reference.WithTag(ref, tag)
+					tagRef, err := reference.WithTag(ref, tag)
 					if err != nil {
 						logrus.Errorf("migrate tags: invalid tag %q, %q", tag, err)
 						continue
 					}
-				}
+					if err := ts.AddTag(tagRef, strongID, false); err != nil {
-				if err := ts.Add(ref, strongID, false); err != nil {
+						logrus.Errorf("can't migrate tag %q for %q, err: %q", ref.String(), strongID, err)
-					logrus.Errorf("can't migrate tag %q for %q, err: %q", ref.String(), strongID, err)
+					}
 				}
 				logrus.Infof("migrated tag %s:%s to point to %s", name, tag, strongID)
 			}

migrate/v1/migratev1_test.go

@@ -289,13 +289,16 @@ type mockTagAdder struct {
 	refs map[string]string
 }
 
-func (t *mockTagAdder) Add(ref reference.Named, id image.ID, force bool) error {
+func (t *mockTagAdder) AddTag(ref reference.Named, id image.ID, force bool) error {
 	if t.refs == nil {
 		t.refs = make(map[string]string)
 	}
 	t.refs[ref.String()] = id.String()
 	return nil
 }
+func (t *mockTagAdder) AddDigest(ref reference.Canonical, id image.ID, force bool) error {
+	return t.AddTag(ref, id, force)
+}
 
 type mockRegistrar struct {
 	layers map[layer.ChainID]*mockLayer

tag/store.go

@@ -9,6 +9,7 @@ import (
 	"path/filepath"
 	"sync"
 
+	"github.com/docker/distribution/digest"
 	"github.com/docker/distribution/reference"
 	"github.com/docker/docker/image"
 )
@@ -32,7 +33,8 @@ type Association struct {
 type Store interface {
 	References(id image.ID) []reference.Named
 	ReferencesByName(ref reference.Named) []Association
-	Add(ref reference.Named, id image.ID, force bool) error
+	AddTag(ref reference.Named, id image.ID, force bool) error
+	AddDigest(ref reference.Canonical, id image.ID, force bool) error
 	Delete(ref reference.Named) (bool, error)
 	Get(ref reference.Named) (image.ID, error)
 }
@@ -90,10 +92,24 @@ func NewTagStore(jsonPath string) (Store, error) {
 	return store, nil
 }
 
-// Add adds a tag or digest to the store. If force is set to true, existing
+// Add adds a tag to the store. If force is set to true, existing
 // references can be overwritten. This only works for tags, not digests.
-func (store *store) Add(ref reference.Named, id image.ID, force bool) error {
+func (store *store) AddTag(ref reference.Named, id image.ID, force bool) error {
-	ref = defaultTagIfNameOnly(ref)
+	if _, isDigested := ref.(reference.Digested); isDigested {
+		return errors.New("refusing to create a tag with a digest reference")
+	}
+	return store.addReference(defaultTagIfNameOnly(ref), id, force)
+}
+
+// Add adds a digest reference to the store.
+func (store *store) AddDigest(ref reference.Canonical, id image.ID, force bool) error {
+	return store.addReference(ref, id, force)
+}
+
+func (store *store) addReference(ref reference.Named, id image.ID, force bool) error {
+	if ref.Name() == string(digest.Canonical) {
+		return errors.New("refusing to create an ambiguous tag using digest algorithm as name")
+	}
 
 	store.mu.Lock()
 	defer store.mu.Unlock()

tag/store_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"io/ioutil"
 	"os"
+	"path/filepath"
 	"sort"
 	"strings"
 	"testing"
@@ -79,9 +80,16 @@ func TestSave(t *testing.T) {
 		if err != nil {
 			t.Fatalf("failed to parse reference: %v", err)
 		}
-		err = store.Add(ref, id, false)
+		if canonical, ok := ref.(reference.Canonical); ok {
-		if err != nil {
+			err = store.AddDigest(canonical, id, false)
-			t.Fatalf("could not add reference %s: %v", refStr, err)
+			if err != nil {
+				t.Fatalf("could not add digest reference %s: %v", refStr, err)
+			}
+		} else {
+			err = store.AddTag(ref, id, false)
+			if err != nil {
+				t.Fatalf("could not add reference %s: %v", refStr, err)
+			}
 		}
 	}
 
@@ -130,7 +138,7 @@ func TestAddDeleteGet(t *testing.T) {
 	if err != nil {
 		t.Fatalf("could not parse reference: %v", err)
 	}
-	if err = store.Add(nameOnly, testImageID1, false); err != nil {
+	if err = store.AddTag(nameOnly, testImageID1, false); err != nil {
 		t.Fatalf("error adding to store: %v", err)
 	}
 
@@ -139,7 +147,7 @@ func TestAddDeleteGet(t *testing.T) {
 	if err != nil {
 		t.Fatalf("could not parse reference: %v", err)
 	}
-	if err = store.Add(ref1, testImageID1, false); err != nil {
+	if err = store.AddTag(ref1, testImageID1, false); err != nil {
 		t.Fatalf("error adding to store: %v", err)
 	}
 
@@ -147,7 +155,7 @@ func TestAddDeleteGet(t *testing.T) {
 	if err != nil {
 		t.Fatalf("could not parse reference: %v", err)
 	}
-	if err = store.Add(ref2, testImageID2, false); err != nil {
+	if err = store.AddTag(ref2, testImageID2, false); err != nil {
 		t.Fatalf("error adding to store: %v", err)
 	}
 
@@ -155,7 +163,7 @@ func TestAddDeleteGet(t *testing.T) {
 	if err != nil {
 		t.Fatalf("could not parse reference: %v", err)
 	}
-	if err = store.Add(ref3, testImageID1, false); err != nil {
+	if err = store.AddTag(ref3, testImageID1, false); err != nil {
 		t.Fatalf("error adding to store: %v", err)
 	}
 
@@ -163,7 +171,7 @@ func TestAddDeleteGet(t *testing.T) {
 	if err != nil {
 		t.Fatalf("could not parse reference: %v", err)
 	}
-	if err = store.Add(ref4, testImageID2, false); err != nil {
+	if err = store.AddTag(ref4, testImageID2, false); err != nil {
 		t.Fatalf("error adding to store: %v", err)
 	}
 
@@ -171,16 +179,16 @@ func TestAddDeleteGet(t *testing.T) {
 	if err != nil {
 		t.Fatalf("could not parse reference: %v", err)
 	}
-	if err = store.Add(ref5, testImageID2, false); err != nil {
+	if err = store.AddDigest(ref5.(reference.Canonical), testImageID2, false); err != nil {
 		t.Fatalf("error adding to store: %v", err)
 	}
 
 	// Attempt to overwrite with force == false
-	if err = store.Add(ref4, testImageID3, false); err == nil || !strings.HasPrefix(err.Error(), "Conflict:") {
+	if err = store.AddTag(ref4, testImageID3, false); err == nil || !strings.HasPrefix(err.Error(), "Conflict:") {
 		t.Fatalf("did not get expected error on overwrite attempt - got %v", err)
 	}
 	// Repeat to overwrite with force == true
-	if err = store.Add(ref4, testImageID3, true); err != nil {
+	if err = store.AddTag(ref4, testImageID3, true); err != nil {
 		t.Fatalf("failed to force tag overwrite: %v", err)
 	}
 
@@ -326,3 +334,35 @@ func TestAddDeleteGet(t *testing.T) {
 		t.Fatal("Expected ErrDoesNotExist from Get")
 	}
 }
+
+func TestInvalidTags(t *testing.T) {
+	tmpDir, err := ioutil.TempDir("", "tag-store-test")
+	defer os.RemoveAll(tmpDir)
+
+	store, err := NewTagStore(filepath.Join(tmpDir, "repositories.json"))
+	if err != nil {
+		t.Fatalf("error creating tag store: %v", err)
+	}
+	id := image.ID("sha256:470022b8af682154f57a2163d030eb369549549cba00edc69e1b99b46bb924d6")
+
+	// sha256 as repo name
+	ref, err := reference.ParseNamed("sha256:abc")
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = store.AddTag(ref, id, true)
+	if err == nil {
+		t.Fatalf("expected setting tag %q to fail", ref)
+	}
+
+	// setting digest as a tag
+	ref, err = reference.ParseNamed("registry@sha256:367eb40fd0330a7e464777121e39d2f5b3e8e23a1e159342e53ab05c9e4d94e6")
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = store.AddTag(ref, id, true)
+	if err == nil {
+		t.Fatalf("expected setting digest %q to fail", ref)
+	}
+
+}