fix: signature verification registry authentication and rate limits (#1412 & #1414) (#1416)

iwilltry42 · web-flow · commit 09f636a2be3f · 2023-04-03T08:24:05.000+02:00
We faced issues with failing signature verification when using private/authenticated registries and also hit rate-limits on DockerHub due to lots of GET requests (pulling images, signature artifacts and digest requests).
I made some changes that should fix/improve this:

- refactor: reduce the number of places where GET requests can occur (i.e. pulling them out of the for-loops where possible)
- before, crane functions used the default keychain instead of the one
  derived from the Acorn environment
- before, we used ociremote functions to determine the digests, which
  defaulted to GET requests that count against e.g. DockerHub's pull
  rate limits -&gt; switching to crane functions should help here
diff --git a/pkg/cosign/cosign.go b/pkg/cosign/cosign.go
@@ -11,11 +11,9 @@ import (
 
 	v1 "github.com/acorn-io/acorn/pkg/apis/internal.acorn.io/v1"
 	"github.com/acorn-io/acorn/pkg/imagesystem"
-	"github.com/google/go-containerregistry/pkg/authn"
 	"github.com/google/go-containerregistry/pkg/crane"
 	"github.com/google/go-containerregistry/pkg/name"
 	ggcrv1 "github.com/google/go-containerregistry/pkg/v1"
-	"github.com/google/go-containerregistry/pkg/v1/remote"
 	"github.com/google/go-containerregistry/pkg/v1/remote/transport"
 	"github.com/sigstore/cosign/pkg/cosign"
 	"github.com/sigstore/cosign/pkg/oci"
@@ -31,89 +29,83 @@ import (
 )
 
 type VerifyOpts struct {
-	ImageRef           string
+	ImageRef           name.Digest
+	SignatureRef       name.Reference
 	Namespace          string
 	AnnotationRules    v1.SignatureAnnotations
 	Key                string
 	SignatureAlgorithm string
-	RegistryClientOpts []ociremote.Option
+	OciRemoteOpts      []ociremote.Option
+	CraneOpts          []crane.Option
 	NoCache            bool
 }
 
-// verifySignature checks if the image is signed with the given key and if the annotations match the given rules
-func VerifySignature(ctx context.Context, c client.Reader, opts VerifyOpts) error {
+// EnsureReferences will enrich the VerifyOpts with the image digest and signature reference.
+// It's outsourced here, so we can ensure that it's used as few times as possible to reduce the number of potential
+// GET requests to the registry which would count against potential rate limits.
+func EnsureReferences(ctx context.Context, c client.Reader, img string, opts *VerifyOpts) error {
 	// --- image name to digest hash
-	imgRef, err := name.ParseReference(opts.ImageRef)
+	imgRef, err := name.ParseReference(img)
 	if err != nil {
-		return fmt.Errorf("failed to parse image %s: %w", opts.ImageRef, err)
+		return fmt.Errorf("failed to parse image %s: %w", img, err)
 	}
 
-	imgDigest, err := ociremote.ResolveDigest(imgRef, opts.RegistryClientOpts...)
+	imgDigest, err := crane.Digest(imgRef.Name(), opts.CraneOpts...) // this uses HEAD to determine the digest, but falls back to GET if HEAD fails
 	if err != nil {
 		return fmt.Errorf("failed to resolve image digest: %w", err)
 	}
 
-	imgDigestHash, err := ggcrv1.NewHash(imgDigest.Identifier())
-	if err != nil {
-		return err
-	}
+	opts.ImageRef = imgRef.Context().Digest(imgDigest)
 
-	// --- cosign verifier options
-
-	cosignOpts := &cosign.CheckOpts{
-		Annotations:        map[string]interface{}{},
-		ClaimVerifier:      cosign.SimpleClaimVerifier,
-		RegistryClientOpts: opts.RegistryClientOpts,
+	signatureRef, err := ensureSignatureArtifact(ctx, c, opts.Namespace, opts.ImageRef, opts.NoCache, opts.OciRemoteOpts, opts.CraneOpts)
+	if err != nil {
+		return fmt.Errorf("failed to ensure signature artifact: %w", err)
 	}
+	opts.SignatureRef = signatureRef
 
-	// --- parse key
-	if opts.Key != "" {
-		verifier, err := LoadKey(ctx, opts.Key, opts.SignatureAlgorithm)
-		if err != nil {
-			return fmt.Errorf("failed to load key: %w", err)
-		}
-		cosignOpts.SigVerifier = verifier
-	}
+	return nil
+}
 
+func ensureSignatureArtifact(ctx context.Context, c client.Reader, namespace string, img name.Digest, noCache bool, ociRemoteOpts []ociremote.Option, craneOpts []crane.Option) (name.Reference, error) {
 	// -- signature hash
-	sigTag, err := ociremote.SignatureTag(imgRef, opts.RegistryClientOpts...)
+	sigTag, err := ociremote.SignatureTag(img, ociRemoteOpts...) // we force imgRef to be a digest above, so this should *not* make a GET request to the registry
 	if err != nil {
-		return fmt.Errorf("failed to get signature tag: %w", err)
+		return nil, fmt.Errorf("failed to get signature tag: %w", err)
 	}
 
-	sigDigest, err := crane.Digest(sigTag.Name(), crane.WithAuthFromKeychain(authn.DefaultKeychain))
+	sigDigest, err := crane.Digest(sigTag.Name(), craneOpts...) // this uses HEAD to determine the digest, but falls back to GET if HEAD fails
 	if err != nil {
 		if terr, ok := err.(*transport.Error); ok && terr.StatusCode == http.StatusNotFound {
 			// signature artifact not found -> that's an actual verification error
-			return fmt.Errorf("%w: expected signature artifact %s not found", cosign.ErrNoMatchingSignatures, sigTag.Name())
+			return nil, fmt.Errorf("%w: expected signature artifact %s not found", cosign.ErrNoMatchingSignatures, sigTag.Name())
 		}
-		return fmt.Errorf("failed to get signature digest: %w", err)
+		return nil, fmt.Errorf("failed to get signature digest: %w", err)
 	}
 
 	sigRefToUse, err := name.ParseReference(sigTag.Name(), name.WeakValidation)
 	if err != nil {
-		return fmt.Errorf("failed to parse signature reference: %w", err)
+		return nil, fmt.Errorf("failed to parse signature reference: %w", err)
 	}
 	logrus.Debugf("Signature %s has digest: %s", sigRefToUse.Name(), sigDigest)
 
-	if !opts.NoCache {
-		internalRepo, _, err := imagesystem.GetInternalRepoForNamespace(ctx, c, opts.Namespace)
+	if !noCache {
+		internalRepo, _, err := imagesystem.GetInternalRepoForNamespace(ctx, c, namespace)
 		if err != nil {
-			return fmt.Errorf("failed to get internal repo for namespace %s: %w", opts.Namespace, err)
+			return nil, fmt.Errorf("failed to get internal repo for namespace %s: %w", namespace, err)
 		}
 
 		localSignatureArtifact := fmt.Sprintf("%s:%s", internalRepo, sigTag.Identifier())
 
 		// --- check if we have the signature artifact locally, if not, copy it over from external registry
 		mustPull := false
-		localSigSHA, err := crane.Digest(localSignatureArtifact, crane.WithAuthFromKeychain(authn.DefaultKeychain))
+		localSigSHA, err := crane.Digest(localSignatureArtifact, craneOpts...) // this uses HEAD to determine the digest, but falls back to GET if HEAD fails
 		if err != nil {
 			var terr *transport.Error
 			if ok := errors.As(err, &terr); ok && terr.StatusCode == http.StatusNotFound {
 				logrus.Debugf("signature artifact %s not found locally, will try to pull it", localSignatureArtifact)
 				mustPull = true
 			} else {
-				return fmt.Errorf("failed to get local signature digest, cannot check if we have it cached locally: %w", err)
+				return nil, fmt.Errorf("failed to get local signature digest, cannot check if we have it cached locally: %w", err)
 			}
 		} else if localSigSHA != sigDigest {
 			logrus.Debugf("Local signature digest %s does not match remote signature digest %s, will try to pull it", localSigSHA, sigDigest)
@@ -122,27 +114,57 @@ func VerifySignature(ctx context.Context, c client.Reader, opts VerifyOpts) erro
 
 		if mustPull {
 			// --- pull signature artifact
-			err := crane.Copy(sigTag.Name(), localSignatureArtifact, crane.WithAuthFromKeychain(authn.DefaultKeychain))
+			err := crane.Copy(sigTag.Name(), localSignatureArtifact, craneOpts...) // Pull (GET) counts against the rate limits, so this shouldn't be done too often
 			if err != nil {
-				return fmt.Errorf("failed to copy signature artifact: %w", err)
+				return nil, fmt.Errorf("failed to copy signature artifact: %w", err)
 			}
 		}
 
 		lname, err := name.ParseReference(localSignatureArtifact, name.WeakValidation)
 		if err != nil {
-			return fmt.Errorf("failed to parse local signature artifact %s: %w", localSignatureArtifact, err)
+			return nil, fmt.Errorf("failed to parse local signature artifact %s: %w", localSignatureArtifact, err)
 		}
 
 		sigRefToUse = lname
 
-		logrus.Debugf("Checking if image %s is signed with %s (cache: %s)", imgRef, sigTag, localSignatureArtifact)
+		logrus.Debugf("Checking if image %s is signed with %s (cache: %s)", img, sigTag, localSignatureArtifact)
 	}
 
-	sigs, err := ociremote.Signatures(sigRefToUse, ociremote.WithRemoteOptions(remote.WithAuthFromKeychain(authn.DefaultKeychain)))
+	return sigRefToUse, nil
+}
+
+// VerifySignature checks if the image is signed with the given key and if the annotations match the given rules
+// This does a lot of image and image manifest juggling to fetch artifacts, digests, etc. from the registry, so we have to be
+// careful to not do too many GET requests that count against registry rate limits (e.g. for DockerHub).
+// Crane uses HEAD (with GET as a fallback) wherever it can, so it's a good choice here e.g. for fetching digests.
+func VerifySignature(ctx context.Context, opts VerifyOpts) error {
+	sigs, err := ociremote.Signatures(opts.SignatureRef, opts.OciRemoteOpts...) // this runs against our internal registry, so it should not count against the rate limits
 	if err != nil {
 		return fmt.Errorf("failed to get signatures: %w", err)
 	}
 
+	imgDigestHash, err := ggcrv1.NewHash(opts.ImageRef.DigestStr())
+	if err != nil {
+		return err
+	}
+
+	// --- cosign verifier options
+
+	cosignOpts := &cosign.CheckOpts{
+		Annotations:        map[string]interface{}{},
+		ClaimVerifier:      cosign.SimpleClaimVerifier,
+		RegistryClientOpts: opts.OciRemoteOpts,
+	}
+
+	// --- parse key
+	if opts.Key != "" {
+		verifier, err := LoadKey(ctx, opts.Key, opts.SignatureAlgorithm)
+		if err != nil {
+			return fmt.Errorf("failed to load key: %w", err)
+		}
+		cosignOpts.SigVerifier = verifier
+	}
+
 	// --- get and verify signatures
 	signatures, bundlesVerified, err := verifySignatures(ctx, sigs, imgDigestHash, cosignOpts)
 	if err != nil {
@@ -152,7 +174,7 @@ func VerifySignature(ctx context.Context, c client.Reader, opts VerifyOpts) erro
 		return fmt.Errorf("failed to verify image signatures: %w", err)
 	}
 
-	logrus.Debugf("image %s: %d signatures verified (bundle verified: %v)", imgRef, len(signatures), bundlesVerified)
+	logrus.Debugf("image %s: %d signatures verified (bundle verified: %v)", opts.ImageRef.Name(), len(signatures), bundlesVerified)
 
 	// --- extract payloads for subsequent checks
 	payloads, err := extractPayload(signatures)
@@ -167,7 +189,7 @@ func VerifySignature(ctx context.Context, c client.Reader, opts VerifyOpts) erro
 		}
 		return fmt.Errorf("failed to check annotations: %w", err)
 	}
-	logrus.Debugf("image %s: Annotations (%+v) verified", imgRef, opts.AnnotationRules)
+	logrus.Debugf("image %s: Annotations (%+v) verified", opts.ImageRef.Name(), opts.AnnotationRules)
 
 	return nil
 }
diff --git a/pkg/cosign/cosign_test.go b/pkg/cosign/cosign_test.go
@@ -82,7 +82,6 @@ func pushOCIDir(path string, ref name.Reference) error {
 	return nil
 }
 
-// TODO: mock oci registry and signatures
 func TestVerifySignature(t *testing.T) {
 	imgPath := "./testdata/img.oci"
 	imgDigestExpected := "sha256:245864d0312e7e33201eff111cfc071727f4eaa9edd10a395c367077e200cad2"
@@ -177,12 +176,13 @@ func TestVerifySignature(t *testing.T) {
 	}
 
 	opts := VerifyOpts{
-		ImageRef:           fmt.Sprintf("%s@%s", imgRepo, imgDigestExpected),
 		SignatureAlgorithm: "sha256",
 		NoCache:            true,
 	}
 
-	ref, err := name.ParseReference(opts.ImageRef)
+	imgName := fmt.Sprintf("%s@%s", imgRepo, imgDigestExpected)
+
+	ref, err := name.ParseReference(imgName)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -191,7 +191,7 @@ func TestVerifySignature(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	digest, err := crane.Digest(opts.ImageRef)
+	digest, err := crane.Digest(ref.Name())
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -218,7 +218,11 @@ func TestVerifySignature(t *testing.T) {
 		opts.Key = tc.key
 		opts.AnnotationRules = tc.annotationrules
 
-		err = VerifySignature(context.Background(), nil, opts)
+		if err := EnsureReferences(context.Background(), nil, imgName, &opts); err != nil {
+			t.Fatal(err)
+		}
+
+		err = VerifySignature(context.Background(), opts)
 		if err != nil && !tc.shouldError {
 			t.Fatalf("[%d] unexpected error: %v, but %s", ti, err, tc.description)
 		}
diff --git a/pkg/imageallowrules/imageallowrules.go b/pkg/imageallowrules/imageallowrules.go
@@ -7,6 +7,8 @@ import (
 	v1 "github.com/acorn-io/acorn/pkg/apis/internal.acorn.io/v1"
 	"github.com/acorn-io/acorn/pkg/cosign"
 	"github.com/acorn-io/acorn/pkg/images"
+	"github.com/google/go-containerregistry/pkg/authn"
+	"github.com/google/go-containerregistry/pkg/crane"
 	"github.com/google/go-containerregistry/pkg/v1/remote"
 	"github.com/rancher/wrangler/pkg/merr"
 	ocosign "github.com/sigstore/cosign/pkg/cosign"
@@ -44,10 +46,15 @@ func CheckImageAllowed(ctx context.Context, c client.Reader, namespace, image st
 		return err
 	}
 
-	return CheckImageAgainstRules(ctx, c, namespace, image, rulesList.Items, opts...)
+	keychain, err := images.GetAuthenticationRemoteKeychainWithLocalAuth(ctx, nil, nil, c, namespace)
+	if err != nil {
+		return err
+	}
+
+	return CheckImageAgainstRules(ctx, c, namespace, image, rulesList.Items, keychain, opts...)
 }
 
-func CheckImageAgainstRules(ctx context.Context, c client.Reader, namespace string, image string, imageAllowRules []v1.ImageAllowRuleInstance, opts ...remote.Option) error {
+func CheckImageAgainstRules(ctx context.Context, c client.Reader, namespace string, image string, imageAllowRules []v1.ImageAllowRuleInstance, keychain authn.Keychain, opts ...remote.Option) error {
 	if len(imageAllowRules) == 0 {
 		// No ImageAllowRules found, so allow the image
 		return nil
@@ -57,13 +64,18 @@ func CheckImageAgainstRules(ctx context.Context, c client.Reader, namespace stri
 
 	// Check if the image is allowed
 	verifyOpts := cosign.VerifyOpts{
-		ImageRef:           image,
 		Namespace:          namespace,
 		AnnotationRules:    v1.SignatureAnnotations{},
 		Key:                "",
 		SignatureAlgorithm: "sha256", // FIXME: make signature algorithm configurable (?)
-		RegistryClientOpts: []ociremote.Option{ociremote.WithRemoteOptions(opts...)},
+		OciRemoteOpts:      []ociremote.Option{ociremote.WithRemoteOptions(opts...)},
+		CraneOpts:          []crane.Option{crane.WithContext(ctx), crane.WithAuthFromKeychain(keychain)},
 	}
+
+	if err := cosign.EnsureReferences(ctx, c, image, &verifyOpts); err != nil {
+		return fmt.Errorf("error ensuring references for image %s: %w", image, err)
+	}
+
 	for _, imageAllowRule := range imageAllowRules {
 		notAllowedErr := &ErrImageNotAllowed{Rule: fmt.Sprintf("%s/%s", imageAllowRule.Namespace, imageAllowRule.Name), Image: image}
 
@@ -78,7 +90,7 @@ func CheckImageAgainstRules(ctx context.Context, c client.Reader, namespace stri
 				for allOfRuleIndex, signer := range rule.SignedBy.AllOf {
 					logrus.Debugf("Checking image %s against %s/%s.signatures.allOf.%d", image, imageAllowRule.Namespace, imageAllowRule.Name, allOfRuleIndex)
 					verifyOpts.Key = signer
-					err := cosign.VerifySignature(ctx, c, verifyOpts)
+					err := cosign.VerifySignature(ctx, verifyOpts)
 					if err != nil {
 						if _, ok := err.(*ocosign.VerificationError); ok {
 							notAllowedErr.SubrulePath += fmt.Sprintf(".allOf.%d (%v)", allOfRuleIndex, err)
@@ -96,7 +108,7 @@ func CheckImageAgainstRules(ctx context.Context, c client.Reader, namespace stri
 				for anyOfRuleIndex, signer := range rule.SignedBy.AnyOf {
 					logrus.Debugf("Checking image %s against %s/%s.signatures.anyOf.%d", image, imageAllowRule.Namespace, imageAllowRule.Name, anyOfRuleIndex)
 					verifyOpts.Key = signer
-					err := cosign.VerifySignature(ctx, c, verifyOpts)
+					err := cosign.VerifySignature(ctx, verifyOpts)
 					if err == nil {
 						anyOfOK = true
 						break
diff --git a/pkg/images/operations.go b/pkg/images/operations.go
@@ -162,7 +162,7 @@ func GetImageReference(ctx context.Context, c client.Reader, namespace, image st
 	return imagename.ParseReference(image)
 }
 
-func GetAuthenticationRemoteOptionsWithLocalAuth(ctx context.Context, registry authn.Resource, localAuth *apiv1.RegistryAuth, client client.Reader, namespace string, additionalOpts ...remote.Option) ([]remote.Option, error) {
+func GetAuthenticationRemoteKeychainWithLocalAuth(ctx context.Context, registry authn.Resource, localAuth *apiv1.RegistryAuth, client client.Reader, namespace string) (authn.Keychain, error) {
 	authn, err := pullsecret.Keychain(ctx, client, namespace)
 	if err != nil {
 		return nil, err
@@ -172,6 +172,15 @@ func GetAuthenticationRemoteOptionsWithLocalAuth(ctx context.Context, registry a
 		authn = NewSimpleKeychain(registry, *localAuth, authn)
 	}
 
+	return authn, nil
+}
+
+func GetAuthenticationRemoteOptionsWithLocalAuth(ctx context.Context, registry authn.Resource, localAuth *apiv1.RegistryAuth, client client.Reader, namespace string, additionalOpts ...remote.Option) ([]remote.Option, error) {
+	authn, err := GetAuthenticationRemoteKeychainWithLocalAuth(ctx, registry, localAuth, client, namespace)
+	if err != nil {
+		return nil, err
+	}
+
 	result := []remote.Option{
 		remote.WithContext(ctx),
 		remote.WithAuthFromKeychain(authn),

Original file line number	Diff line number	Diff line change
`@@ -82,7 +82,6 @@ func pushOCIDir(path string, ref name.Reference) error {`
`82`	`82`	`return nil`
`83`	`83`	`}`
`84`	`84`
`85`		`-// TODO: mock oci registry and signatures`
`86`	`85`	`func TestVerifySignature(t *testing.T) {`
`87`	`86`	`imgPath := "./testdata/img.oci"`
`88`	`87`	`imgDigestExpected := "sha256:245864d0312e7e33201eff111cfc071727f4eaa9edd10a395c367077e200cad2"`
`@@ -177,12 +176,13 @@ func TestVerifySignature(t *testing.T) {`
`177`	`176`	`}`
`178`	`177`
`179`	`178`	`opts := VerifyOpts{`
`180`		`- ImageRef: fmt.Sprintf("%s@%s", imgRepo, imgDigestExpected),`
`181`	`179`	`SignatureAlgorithm: "sha256",`
`182`	`180`	`NoCache: true,`
`183`	`181`	`}`
`184`	`182`
`185`		`- ref, err := name.ParseReference(opts.ImageRef)`
	`183`	`+ imgName := fmt.Sprintf("%s@%s", imgRepo, imgDigestExpected)`
	`184`	`+`
	`185`	`+ ref, err := name.ParseReference(imgName)`
`186`	`186`	`if err != nil {`
`187`	`187`	`t.Fatal(err)`
`188`	`188`	`}`
`@@ -191,7 +191,7 @@ func TestVerifySignature(t *testing.T) {`
`191`	`191`	`t.Fatal(err)`
`192`	`192`	`}`
`193`	`193`
`194`		`- digest, err := crane.Digest(opts.ImageRef)`
	`194`	`+ digest, err := crane.Digest(ref.Name())`
`195`	`195`	`if err != nil {`
`196`	`196`	`t.Fatal(err)`
`197`	`197`	`}`
`@@ -218,7 +218,11 @@ func TestVerifySignature(t *testing.T) {`
`218`	`218`	`opts.Key = tc.key`
`219`	`219`	`opts.AnnotationRules = tc.annotationrules`
`220`	`220`
`221`		`- err = VerifySignature(context.Background(), nil, opts)`
	`221`	`+ if err := EnsureReferences(context.Background(), nil, imgName, &opts); err != nil {`
	`222`	`+ t.Fatal(err)`
	`223`	`+ }`
	`224`	`+`
	`225`	`+ err = VerifySignature(context.Background(), opts)`
`222`	`226`	`if err != nil && !tc.shouldError {`
`223`	`227`	`t.Fatalf("[%d] unexpected error: %v, but %s", ti, err, tc.description)`
`224`	`228`	`}`