From e37baed88eb697c0cc7275047fda3cb83ae9655a Mon Sep 17 00:00:00 2001
From: Stephen J Day <stephen.day@docker.com>
Date: Thu, 15 Dec 2016 15:06:18 -0800
Subject: [PATCH 1/2] digest: cleanup digester and verifier creation

Signed-off-by: Stephen J Day <stephen.day@docker.com>
---
 digest/algorithm.go                 | 140 ++++++++++++++++++++++++++++
 digest/digest.go                    |  14 +++
 digest/digester.go                  | 137 +--------------------------
 digest/verifiers.go                 |   3 +-
 registry/handlers/api_test.go       |   2 +-
 registry/storage/blobwriter.go      |   2 +-
 registry/storage/filereader_test.go |   2 +-
 7 files changed, 159 insertions(+), 141 deletions(-)
 create mode 100644 digest/algorithm.go

diff --git a/digest/algorithm.go b/digest/algorithm.go
new file mode 100644
index 000000000..9f74e5525
--- /dev/null
+++ b/digest/algorithm.go
@@ -0,0 +1,140 @@
+package digest
+
+import (
+	"crypto"
+	"fmt"
+	"hash"
+	"io"
+)
+
+// Algorithm identifies and implementation of a digester by an identifier.
+// Note the that this defines both the hash algorithm used and the string
+// encoding.
+type Algorithm string
+
+// supported digest types
+const (
+	SHA256 Algorithm = "sha256" // sha256 with hex encoding
+	SHA384 Algorithm = "sha384" // sha384 with hex encoding
+	SHA512 Algorithm = "sha512" // sha512 with hex encoding
+
+	// Canonical is the primary digest algorithm used with the distribution
+	// project. Other digests may be used but this one is the primary storage
+	// digest.
+	Canonical = SHA256
+)
+
+var (
+	// TODO(stevvooe): Follow the pattern of the standard crypto package for
+	// registration of digests. Effectively, we are a registerable set and
+	// common symbol access.
+
+	// algorithms maps values to hash.Hash implementations. Other algorithms
+	// may be available but they cannot be calculated by the digest package.
+	algorithms = map[Algorithm]crypto.Hash{
+		SHA256: crypto.SHA256,
+		SHA384: crypto.SHA384,
+		SHA512: crypto.SHA512,
+	}
+)
+
+// Available returns true if the digest type is available for use. If this
+// returns false, New and Hash will return nil.
+func (a Algorithm) Available() bool {
+	h, ok := algorithms[a]
+	if !ok {
+		return false
+	}
+
+	// check availability of the hash, as well
+	return h.Available()
+}
+
+func (a Algorithm) String() string {
+	return string(a)
+}
+
+// Size returns number of bytes returned by the hash.
+func (a Algorithm) Size() int {
+	h, ok := algorithms[a]
+	if !ok {
+		return 0
+	}
+	return h.Size()
+}
+
+// Set implemented to allow use of Algorithm as a command line flag.
+func (a *Algorithm) Set(value string) error {
+	if value == "" {
+		*a = Canonical
+	} else {
+		// just do a type conversion, support is queried with Available.
+		*a = Algorithm(value)
+	}
+
+	return nil
+}
+
+// New returns a new digester for the specified algorithm. If the algorithm
+// does not have a digester implementation, nil will be returned. This can be
+// checked by calling Available before calling New.
+func (a Algorithm) Digester() Digester {
+	return &digester{
+		alg:  a,
+		hash: a.Hash(),
+	}
+}
+
+// New is deprecated. Use Algorithm.Digester.
+func (a Algorithm) New() Digester {
+	return a.Digester()
+}
+
+// Hash returns a new hash as used by the algorithm. If not available, the
+// method will panic. Check Algorithm.Available() before calling.
+func (a Algorithm) Hash() hash.Hash {
+	if !a.Available() {
+		// NOTE(stevvooe): A missing hash is usually a programming error that
+		// must be resolved at compile time. We don't import in the digest
+		// package to allow users to choose their hash implementation (such as
+		// when using stevvooe/resumable or a hardware accelerated package).
+		//
+		// Applications that may want to resolve the hash at runtime should
+		// call Algorithm.Available before call Algorithm.Hash().
+		panic(fmt.Sprintf("%v not available (make sure it is imported)", a))
+	}
+
+	return algorithms[a].New()
+}
+
+// FromReader returns the digest of the reader using the algorithm.
+func (a Algorithm) FromReader(rd io.Reader) (Digest, error) {
+	digester := a.New()
+
+	if _, err := io.Copy(digester.Hash(), rd); err != nil {
+		return "", err
+	}
+
+	return digester.Digest(), nil
+}
+
+// FromBytes digests the input and returns a Digest.
+func (a Algorithm) FromBytes(p []byte) Digest {
+	digester := a.New()
+
+	if _, err := digester.Hash().Write(p); err != nil {
+		// Writes to a Hash should never fail. None of the existing
+		// hash implementations in the stdlib or hashes vendored
+		// here can return errors from Write. Having a panic in this
+		// condition instead of having FromBytes return an error value
+		// avoids unnecessary error handling paths in all callers.
+		panic("write to hash function returned error: " + err.Error())
+	}
+
+	return digester.Digest()
+}
+
+// FromString digests the string input and returns a Digest.
+func (a Algorithm) FromString(s string) Digest {
+	return a.FromBytes([]byte(s))
+}
diff --git a/digest/digest.go b/digest/digest.go
index 65c6f7f0e..7e6e2d89c 100644
--- a/digest/digest.go
+++ b/digest/digest.go
@@ -123,6 +123,20 @@ func (d Digest) Algorithm() Algorithm {
 	return Algorithm(d[:d.sepIndex()])
 }
 
+// Verifier returns a writer object that can be used to verify a stream of
+// content against the digest. If the digest is invalid, an error will be
+// returned.
+func (d Digest) Verifier() (Verifier, error) {
+	if err := d.Validate(); err != nil {
+		return nil, err
+	}
+
+	return hashVerifier{
+		hash:   d.Algorithm().Hash(),
+		digest: d,
+	}, nil
+}
+
 // Hex returns the hex digest portion of the digest. This will panic if the
 // underlying digest is not in a valid format.
 func (d Digest) Hex() string {
diff --git a/digest/digester.go b/digest/digester.go
index 0435a1a61..918a3f919 100644
--- a/digest/digester.go
+++ b/digest/digester.go
@@ -1,141 +1,6 @@
 package digest
 
-import (
-	"crypto"
-	"fmt"
-	"hash"
-	"io"
-)
-
-// Algorithm identifies and implementation of a digester by an identifier.
-// Note the that this defines both the hash algorithm used and the string
-// encoding.
-type Algorithm string
-
-// supported digest types
-const (
-	SHA256 Algorithm = "sha256" // sha256 with hex encoding
-	SHA384 Algorithm = "sha384" // sha384 with hex encoding
-	SHA512 Algorithm = "sha512" // sha512 with hex encoding
-
-	// Canonical is the primary digest algorithm used with the distribution
-	// project. Other digests may be used but this one is the primary storage
-	// digest.
-	Canonical = SHA256
-)
-
-var (
-	// TODO(stevvooe): Follow the pattern of the standard crypto package for
-	// registration of digests. Effectively, we are a registerable set and
-	// common symbol access.
-
-	// algorithms maps values to hash.Hash implementations. Other algorithms
-	// may be available but they cannot be calculated by the digest package.
-	algorithms = map[Algorithm]crypto.Hash{
-		SHA256: crypto.SHA256,
-		SHA384: crypto.SHA384,
-		SHA512: crypto.SHA512,
-	}
-)
-
-// Available returns true if the digest type is available for use. If this
-// returns false, New and Hash will return nil.
-func (a Algorithm) Available() bool {
-	h, ok := algorithms[a]
-	if !ok {
-		return false
-	}
-
-	// check availability of the hash, as well
-	return h.Available()
-}
-
-func (a Algorithm) String() string {
-	return string(a)
-}
-
-// Size returns number of bytes returned by the hash.
-func (a Algorithm) Size() int {
-	h, ok := algorithms[a]
-	if !ok {
-		return 0
-	}
-	return h.Size()
-}
-
-// Set implemented to allow use of Algorithm as a command line flag.
-func (a *Algorithm) Set(value string) error {
-	if value == "" {
-		*a = Canonical
-	} else {
-		// just do a type conversion, support is queried with Available.
-		*a = Algorithm(value)
-	}
-
-	return nil
-}
-
-// New returns a new digester for the specified algorithm. If the algorithm
-// does not have a digester implementation, nil will be returned. This can be
-// checked by calling Available before calling New.
-func (a Algorithm) New() Digester {
-	return &digester{
-		alg:  a,
-		hash: a.Hash(),
-	}
-}
-
-// Hash returns a new hash as used by the algorithm. If not available, the
-// method will panic. Check Algorithm.Available() before calling.
-func (a Algorithm) Hash() hash.Hash {
-	if !a.Available() {
-		// NOTE(stevvooe): A missing hash is usually a programming error that
-		// must be resolved at compile time. We don't import in the digest
-		// package to allow users to choose their hash implementation (such as
-		// when using stevvooe/resumable or a hardware accelerated package).
-		//
-		// Applications that may want to resolve the hash at runtime should
-		// call Algorithm.Available before call Algorithm.Hash().
-		panic(fmt.Sprintf("%v not available (make sure it is imported)", a))
-	}
-
-	return algorithms[a].New()
-}
-
-// FromReader returns the digest of the reader using the algorithm.
-func (a Algorithm) FromReader(rd io.Reader) (Digest, error) {
-	digester := a.New()
-
-	if _, err := io.Copy(digester.Hash(), rd); err != nil {
-		return "", err
-	}
-
-	return digester.Digest(), nil
-}
-
-// FromBytes digests the input and returns a Digest.
-func (a Algorithm) FromBytes(p []byte) Digest {
-	digester := a.New()
-
-	if _, err := digester.Hash().Write(p); err != nil {
-		// Writes to a Hash should never fail. None of the existing
-		// hash implementations in the stdlib or hashes vendored
-		// here can return errors from Write. Having a panic in this
-		// condition instead of having FromBytes return an error value
-		// avoids unnecessary error handling paths in all callers.
-		panic("write to hash function returned error: " + err.Error())
-	}
-
-	return digester.Digest()
-}
-
-// FromString digests the string input and returns a Digest.
-func (a Algorithm) FromString(s string) Digest {
-	return a.FromBytes([]byte(s))
-}
-
-// TODO(stevvooe): Allow resolution of verifiers using the digest type and
-// this registration system.
+import "hash"
 
 // Digester calculates the digest of written data. Writes should go directly
 // to the return value of Hash, while calling Digest will return the current
diff --git a/digest/verifiers.go b/digest/verifiers.go
index 9af3be134..89c1fb082 100644
--- a/digest/verifiers.go
+++ b/digest/verifiers.go
@@ -17,8 +17,7 @@ type Verifier interface {
 	Verified() bool
 }
 
-// NewDigestVerifier returns a verifier that compares the written bytes
-// against a passed in digest.
+// NewDigestVerifier is deprecated. Please use Digest.Verifier.
 func NewDigestVerifier(d Digest) (Verifier, error) {
 	if err := d.Validate(); err != nil {
 		return nil, err
diff --git a/registry/handlers/api_test.go b/registry/handlers/api_test.go
index d8e930536..eff5c67b8 100644
--- a/registry/handlers/api_test.go
+++ b/registry/handlers/api_test.go
@@ -552,7 +552,7 @@ func testBlobAPI(t *testing.T, env *testEnv, args blobArgs) *testEnv {
 	})
 
 	// Verify the body
-	verifier, err := digest.NewDigestVerifier(layerDigest)
+	verifier, err := layerDigest.Verifier()
 	if err != nil {
 		t.Fatalf("unexpected error getting digest verifier: %s", err)
 	}
diff --git a/registry/storage/blobwriter.go b/registry/storage/blobwriter.go
index 668a6fc9b..7e42a59ba 100644
--- a/registry/storage/blobwriter.go
+++ b/registry/storage/blobwriter.go
@@ -236,7 +236,7 @@ func (bw *blobWriter) validateBlob(ctx context.Context, desc distribution.Descri
 		if !verified {
 			digester := digest.Canonical.New()
 
-			digestVerifier, err := digest.NewDigestVerifier(desc.Digest)
+			digestVerifier, err := desc.Digest.Verifier()
 			if err != nil {
 				return distribution.Descriptor{}, err
 			}
diff --git a/registry/storage/filereader_test.go b/registry/storage/filereader_test.go
index 5926020cc..fa363f44a 100644
--- a/registry/storage/filereader_test.go
+++ b/registry/storage/filereader_test.go
@@ -41,7 +41,7 @@ func TestSimpleRead(t *testing.T) {
 		t.Fatalf("error allocating file reader: %v", err)
 	}
 
-	verifier, err := digest.NewDigestVerifier(dgst)
+	verifier, err := dgst.Verifier()
 	if err != nil {
 		t.Fatalf("error getting digest verifier: %s", err)
 	}

From 9159833265e2bd28c3e04c4fffb2e55cb0b3a14f Mon Sep 17 00:00:00 2001
From: Stephen J Day <stephen.day@docker.com>
Date: Thu, 15 Dec 2016 16:30:53 -0800
Subject: [PATCH 2/2] digest: remove error return from Digest.Verifier

Signed-off-by: Stephen J Day <stephen.day@docker.com>
---
 digest/algorithm.go                 |  5 +++
 digest/digest.go                    | 28 ++++++--------
 digest/digest_test.go               |  2 +
 digest/verifiers.go                 |  9 +----
 digest/verifiers_test.go            | 59 +++++++++++++++++++----------
 registry/storage/blobwriter.go      | 10 ++---
 registry/storage/filereader_test.go |  6 +--
 7 files changed, 62 insertions(+), 57 deletions(-)

diff --git a/digest/algorithm.go b/digest/algorithm.go
index 9f74e5525..f44b50dbe 100644
--- a/digest/algorithm.go
+++ b/digest/algorithm.go
@@ -94,6 +94,11 @@ func (a Algorithm) New() Digester {
 // method will panic. Check Algorithm.Available() before calling.
 func (a Algorithm) Hash() hash.Hash {
 	if !a.Available() {
+		// Empty algorithm string is invalid
+		if a == "" {
+			panic(fmt.Sprintf("empty digest algorithm, validate before calling Algorithm.Hash()"))
+		}
+
 		// NOTE(stevvooe): A missing hash is usually a programming error that
 		// must be resolved at compile time. We don't import in the digest
 		// package to allow users to choose their hash implementation (such as
diff --git a/digest/digest.go b/digest/digest.go
index 7e6e2d89c..4616788ba 100644
--- a/digest/digest.go
+++ b/digest/digest.go
@@ -104,16 +104,17 @@ func (d Digest) Validate() error {
 		return ErrDigestInvalidFormat
 	}
 
-	switch algorithm := Algorithm(s[:i]); algorithm {
-	case SHA256, SHA384, SHA512:
-		if algorithm.Size()*2 != len(s[i+1:]) {
-			return ErrDigestInvalidLength
-		}
-		break
-	default:
+	algorithm := Algorithm(s[:i])
+	if !algorithm.Available() {
 		return ErrDigestUnsupported
 	}
 
+	// Digests much always be hex-encoded, ensuring that their hex portion will
+	// always be size*2
+	if algorithm.Size()*2 != len(s[i+1:]) {
+		return ErrDigestInvalidLength
+	}
+
 	return nil
 }
 
@@ -124,17 +125,12 @@ func (d Digest) Algorithm() Algorithm {
 }
 
 // Verifier returns a writer object that can be used to verify a stream of
-// content against the digest. If the digest is invalid, an error will be
-// returned.
-func (d Digest) Verifier() (Verifier, error) {
-	if err := d.Validate(); err != nil {
-		return nil, err
-	}
-
+// content against the digest. If the digest is invalid, the method will panic.
+func (d Digest) Verifier() Verifier {
 	return hashVerifier{
 		hash:   d.Algorithm().Hash(),
 		digest: d,
-	}, nil
+	}
 }
 
 // Hex returns the hex digest portion of the digest. This will panic if the
@@ -151,7 +147,7 @@ func (d Digest) sepIndex() int {
 	i := strings.Index(string(d), ":")
 
 	if i < 0 {
-		panic("could not find ':' in digest: " + d)
+		panic(fmt.Sprintf("no ':' separator in digest %q", d))
 	}
 
 	return i
diff --git a/digest/digest_test.go b/digest/digest_test.go
index afb4ebf63..c27b81714 100644
--- a/digest/digest_test.go
+++ b/digest/digest_test.go
@@ -1,6 +1,8 @@
 package digest
 
 import (
+	_ "crypto/sha256"
+	_ "crypto/sha512"
 	"testing"
 )
 
diff --git a/digest/verifiers.go b/digest/verifiers.go
index 89c1fb082..df9872cf0 100644
--- a/digest/verifiers.go
+++ b/digest/verifiers.go
@@ -19,14 +19,7 @@ type Verifier interface {
 
 // NewDigestVerifier is deprecated. Please use Digest.Verifier.
 func NewDigestVerifier(d Digest) (Verifier, error) {
-	if err := d.Validate(); err != nil {
-		return nil, err
-	}
-
-	return hashVerifier{
-		hash:   d.Algorithm().Hash(),
-		digest: d,
-	}, nil
+	return d.Verifier(), nil
 }
 
 type hashVerifier struct {
diff --git a/digest/verifiers_test.go b/digest/verifiers_test.go
index c342d6e7c..251d4fce5 100644
--- a/digest/verifiers_test.go
+++ b/digest/verifiers_test.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"crypto/rand"
 	"io"
+	"reflect"
 	"testing"
 )
 
@@ -12,10 +13,7 @@ func TestDigestVerifier(t *testing.T) {
 	rand.Read(p)
 	digest := FromBytes(p)
 
-	verifier, err := NewDigestVerifier(digest)
-	if err != nil {
-		t.Fatalf("unexpected error getting digest verifier: %s", err)
-	}
+	verifier := digest.Verifier()
 
 	io.Copy(verifier, bytes.NewReader(p))
 
@@ -27,23 +25,42 @@ func TestDigestVerifier(t *testing.T) {
 // TestVerifierUnsupportedDigest ensures that unsupported digest validation is
 // flowing through verifier creation.
 func TestVerifierUnsupportedDigest(t *testing.T) {
-	unsupported := Digest("bean:0123456789abcdef")
+	for _, testcase := range []struct {
+		Name     string
+		Digest   Digest
+		Expected interface{} // expected panic target
+	}{
+		{
+			Name:     "Empty",
+			Digest:   "",
+			Expected: "no ':' separator in digest \"\"",
+		},
+		{
+			Name:     "EmptyAlg",
+			Digest:   ":",
+			Expected: "empty digest algorithm, validate before calling Algorithm.Hash()",
+		},
+		{
+			Name:     "Unsupported",
+			Digest:   Digest("bean:0123456789abcdef"),
+			Expected: "bean not available (make sure it is imported)",
+		},
+		{
+			Name:     "Garbage",
+			Digest:   Digest("sha256-garbage:pure"),
+			Expected: "sha256-garbage not available (make sure it is imported)",
+		},
+	} {
+		t.Run(testcase.Name, func(t *testing.T) {
+			expected := testcase.Expected
+			defer func() {
+				recovered := recover()
+				if !reflect.DeepEqual(recovered, expected) {
+					t.Fatalf("unexpected recover: %v != %v", recovered, expected)
+				}
+			}()
 
-	_, err := NewDigestVerifier(unsupported)
-	if err == nil {
-		t.Fatalf("expected error when creating verifier")
-	}
-
-	if err != ErrDigestUnsupported {
-		t.Fatalf("incorrect error for unsupported digest: %v", err)
+			_ = testcase.Digest.Verifier()
+		})
 	}
 }
-
-// TODO(stevvooe): Add benchmarks to measure bytes/second throughput for
-// DigestVerifier.
-//
-// The relevant benchmark for comparison can be run with the following
-// commands:
-//
-// 	go test -bench . crypto/sha1
-//
diff --git a/registry/storage/blobwriter.go b/registry/storage/blobwriter.go
index 7e42a59ba..e2f03f5f9 100644
--- a/registry/storage/blobwriter.go
+++ b/registry/storage/blobwriter.go
@@ -235,11 +235,7 @@ func (bw *blobWriter) validateBlob(ctx context.Context, desc distribution.Descri
 		// guarantee, so this may be defensive.
 		if !verified {
 			digester := digest.Canonical.New()
-
-			digestVerifier, err := desc.Digest.Verifier()
-			if err != nil {
-				return distribution.Descriptor{}, err
-			}
+			verifier := desc.Digest.Verifier()
 
 			// Read the file from the backend driver and validate it.
 			fr, err := newFileReader(ctx, bw.driver, bw.path, desc.Size)
@@ -250,12 +246,12 @@ func (bw *blobWriter) validateBlob(ctx context.Context, desc distribution.Descri
 
 			tr := io.TeeReader(fr, digester.Hash())
 
-			if _, err := io.Copy(digestVerifier, tr); err != nil {
+			if _, err := io.Copy(verifier, tr); err != nil {
 				return distribution.Descriptor{}, err
 			}
 
 			canonical = digester.Digest()
-			verified = digestVerifier.Verified()
+			verified = verifier.Verified()
 		}
 	}
 
diff --git a/registry/storage/filereader_test.go b/registry/storage/filereader_test.go
index fa363f44a..371a410d8 100644
--- a/registry/storage/filereader_test.go
+++ b/registry/storage/filereader_test.go
@@ -41,11 +41,7 @@ func TestSimpleRead(t *testing.T) {
 		t.Fatalf("error allocating file reader: %v", err)
 	}
 
-	verifier, err := dgst.Verifier()
-	if err != nil {
-		t.Fatalf("error getting digest verifier: %s", err)
-	}
-
+	verifier := dgst.Verifier()
 	io.Copy(verifier, fr)
 
 	if !verifier.Verified() {