Repository: google/go-containerregistry
Pull request #1075 (2021-07-12 00:07:54)

Handle multiple www-authenticate headers
It might happen there are multiple `www-authenticate` headers returned from a registry, e.g. `Negotiate` and `Basic`. In the current code, in such situation the first challenge would be picked with no further checks, which would result eventually in `unrecognized challenge` error failing the whole build, even though `Basic` challenge could be used instead. `pickFromMultipleChallenges` function was added to loop through the challenges in search for a supported one.

jonjohnsonjr: Seems related to kerberos? https://stackoverflow.com/questions/4265975/authentication-issues-with-www-authenticate-negotiate

Reminds me of https://github.com/bazelbuild/rules_k8s/issues/141
seba-ban: Thanks for the comment. I'll try to add a unit test later this week.

So, yeah, it can be related to Kerberos. In fact, the real-world scenario I'm aware of is using [SPNEGO NGINX module](https://github.com/stnoonan/spnego-http-auth-nginx-module) with basic authentication fallback - in such configuration two www-authenticate headers will be sent: `Negotiate` and `Basic`.
jonjohnsonjr: I don't know much about kerberos, but would it make sense to attempt to support it? I assume not, but it sounds like it would be neat :)
seba-ban: To be honest, I don't know much about Kerberos myself either. :( I'm also not sure how common it is to have a Kerberos (Negotiate) only registry, but I guess not that common. I don't think it's supported by Docker either [(here?)](https://github.com/docker/cli/blob/5f07d7d5a12423c0bc1fb507f4d006ad0cdfef42/vendor/github.com/containerd/containerd/remotes/docker/auth/parse.go#L98).
seba-ban: Unit test added.

Patch:
diff --git a/cmd/crane/cmd/mutate.go b/cmd/crane/cmd/mutate.go
index 5eade783..72789c2b 100644
--- a/cmd/crane/cmd/mutate.go
+++ b/cmd/crane/cmd/mutate.go
@@ -21,6 +21,7 @@ import (
 
 	"github.com/google/go-containerregistry/pkg/crane"
 	"github.com/google/go-containerregistry/pkg/name"
+	v1 "github.com/google/go-containerregistry/pkg/v1"
 	"github.com/google/go-containerregistry/pkg/v1/mutate"
 	"github.com/spf13/cobra"
 )
@@ -91,7 +92,7 @@ func NewCmdMutate(options *[]crane.Option) *cobra.Command {
 				log.Fatalf("mutating config: %v", err)
 			}
 
-			img = mutate.Annotations(img, annotations)
+			img = mutate.Annotations(img, annotations).(v1.Image)
 
 			// If the new ref isn't provided, write over the original image.
 			// If that ref was provided by digest (e.g., output from
diff --git a/go.mod b/go.mod
index dca023df..086b43ba 100644
--- a/go.mod
+++ b/go.mod
@@ -6,7 +6,7 @@ require (
 	cloud.google.com/go v0.83.0 // indirect
 	github.com/Microsoft/go-winio v0.5.0 // indirect
 	github.com/containerd/containerd v1.5.2 // indirect
-	github.com/containerd/stargz-snapshotter/estargz v0.6.4
+	github.com/containerd/stargz-snapshotter/estargz v0.7.0
 	github.com/docker/cli v20.10.7+incompatible
 	github.com/docker/distribution v2.7.1+incompatible
 	github.com/docker/docker v20.10.7+incompatible
diff --git a/go.sum b/go.sum
index cec981a9..026976da 100644
--- a/go.sum
+++ b/go.sum
@@ -182,8 +182,8 @@ github.com/containerd/imgcrypt v1.1.1/go.mod h1:xpLnwiQmEUJPvQoAapeb2SNCxz7Xr6PJ
 github.com/containerd/nri v0.0.0-20201007170849-eb1350a75164/go.mod h1:+2wGSDGFYfE5+So4M5syatU0N0f0LbWpuqyMi4/BE8c=
 github.com/containerd/nri v0.0.0-20210316161719-dbaa18c31c14/go.mod h1:lmxnXF6oMkbqs39FiCt1s0R2HSMhcLel9vNL3m4AaeY=
 github.com/containerd/nri v0.1.0/go.mod h1:lmxnXF6oMkbqs39FiCt1s0R2HSMhcLel9vNL3m4AaeY=
-github.com/containerd/stargz-snapshotter/estargz v0.6.4 h1:lhJppIf4ULGQXbcjUJIy1sq79UegNTEebDTtfU8MlcA=
-github.com/containerd/stargz-snapshotter/estargz v0.6.4/go.mod h1:83VWDqHnurTKliEB0YvWMiCfLDwv4Cjj1X9Vk98GJZw=
+github.com/containerd/stargz-snapshotter/estargz v0.7.0 h1:1d/rydzTywc76lnjJb6qbPCiTiCwts49AzKps/Ecblw=
+github.com/containerd/stargz-snapshotter/estargz v0.7.0/go.mod h1:83VWDqHnurTKliEB0YvWMiCfLDwv4Cjj1X9Vk98GJZw=
 github.com/containerd/ttrpc v0.0.0-20190828154514-0e0f228740de/go.mod h1:PvCDdDGpgqzQIzDW1TphrGLssLDZp2GuS+X5DkEJB8o=
 github.com/containerd/ttrpc v0.0.0-20190828172938-92c8520ef9f8/go.mod h1:PvCDdDGpgqzQIzDW1TphrGLssLDZp2GuS+X5DkEJB8o=
 github.com/containerd/ttrpc v0.0.0-20191028202541-4f1b8fe65a5c/go.mod h1:LPm1u0xBw8r8NOKoOdNMeVHSawSsltak+Ihv+etqsE8=
diff --git a/pkg/v1/mutate/index.go b/pkg/v1/mutate/index.go
index da3eae8d..767b1b74 100644
--- a/pkg/v1/mutate/index.go
+++ b/pkg/v1/mutate/index.go
@@ -61,12 +61,13 @@ type index struct {
 	// remove is removed before adds
 	remove match.Matcher
 
-	computed  bool
-	manifest  *v1.IndexManifest
-	mediaType *types.MediaType
-	imageMap  map[v1.Hash]v1.Image
-	indexMap  map[v1.Hash]v1.ImageIndex
-	layerMap  map[v1.Hash]v1.Layer
+	computed    bool
+	manifest    *v1.IndexManifest
+	annotations map[string]string
+	mediaType   *types.MediaType
+	imageMap    map[v1.Hash]v1.Image
+	indexMap    map[v1.Hash]v1.ImageIndex
+	layerMap    map[v1.Hash]v1.Layer
 }
 
 var _ v1.ImageIndex = (*index)(nil)
@@ -137,6 +138,15 @@ func (i *index) compute() error {
 		}
 	}
 
+	if i.annotations != nil {
+		if manifest.Annotations == nil {
+			manifest.Annotations = map[string]string{}
+		}
+		for k, v := range i.annotations {
+			manifest.Annotations[k] = v
+		}
+	}
+
 	i.manifest = manifest
 	i.computed = true
 	return nil
diff --git a/pkg/v1/mutate/mutate.go b/pkg/v1/mutate/mutate.go
index be780fec..f94ad0b7 100644
--- a/pkg/v1/mutate/mutate.go
+++ b/pkg/v1/mutate/mutate.go
@@ -17,6 +17,7 @@ package mutate
 import (
 	"archive/tar"
 	"bytes"
+	"encoding/json"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -28,6 +29,7 @@ import (
 	v1 "github.com/google/go-containerregistry/pkg/v1"
 	"github.com/google/go-containerregistry/pkg/v1/empty"
 	"github.com/google/go-containerregistry/pkg/v1/match"
+	"github.com/google/go-containerregistry/pkg/v1/partial"
 	"github.com/google/go-containerregistry/pkg/v1/tarball"
 	"github.com/google/go-containerregistry/pkg/v1/types"
 )
@@ -113,12 +115,70 @@ func Config(base v1.Image, cfg v1.Config) (v1.Image, error) {
 	return ConfigFile(base, cf)
 }
 
-// Annotations mutates the provided v1.Image to have the provided annotations
-func Annotations(base v1.Image, annotations map[string]string) v1.Image {
-	return &image{
-		base:        base,
-		annotations: annotations,
+// Annotatable represents a manifest that can carry annotations.
+type Annotatable interface {
+	partial.WithRawManifest
+}
+
+// Annotations mutates the annotations on an annotatable image or index manifest.
+//
+// The annotatable input is expected to be a v1.Image or v1.ImageIndex, and
+// returns the same type. You can type-assert the result like so:
+//
+//     img := Annotations(empty.Image, map[string]string{
+//         "foo": "bar",
+//     }).(v1.Image)
+//
+// Or for an index:
+//
+//     idx := Annotations(empty.Index, map[string]string{
+//         "foo": "bar",
+//     }).(v1.ImageIndex)
+//
+// If the input Annotatable is not an Image or ImageIndex, the result will
+// attempt to lazily annotate the raw manifest.
+func Annotations(f Annotatable, anns map[string]string) Annotatable {
+	if img, ok := f.(v1.Image); ok {
+		return &image{
+			base:        img,
+			annotations: anns,
+		}
+	}
+	if idx, ok := f.(v1.ImageIndex); ok {
+		return &index{
+			base:        idx,
+			annotations: anns,
+		}
+	}
+	return arbitraryRawManifest{f, anns}
+}
+
+type arbitraryRawManifest struct {
+	a    Annotatable
+	anns map[string]string
+}
+
+func (a arbitraryRawManifest) RawManifest() ([]byte, error) {
+	b, err := a.a.RawManifest()
+	if err != nil {
+		return nil, err
+	}
+	var m map[string]interface{}
+	if err := json.Unmarshal(b, &m); err != nil {
+		return nil, err
+	}
+	if ann, ok := m["annotations"]; ok {
+		if annm, ok := ann.(map[string]string); ok {
+			for k, v := range a.anns {
+				annm[k] = v
+			}
+		} else {
+			return nil, fmt.Errorf(".annotations is not a map: %T", ann)
+		}
+	} else {
+		m["annotations"] = a.anns
 	}
+	return json.Marshal(m)
 }
 
 // ConfigFile mutates the provided v1.Image to have the provided v1.ConfigFile
diff --git a/pkg/v1/remote/transport/ping.go b/pkg/v1/remote/transport/ping.go
index 396d4e03..c25d1b24 100644
--- a/pkg/v1/remote/transport/ping.go
+++ b/pkg/v1/remote/transport/ping.go
@@ -108,8 +108,8 @@ func ping(ctx context.Context, reg name.Registry, t http.RoundTripper) (*pingRes
 			}, nil
 		case http.StatusUnauthorized:
 			if challenges := authchallenge.ResponseChallenges(resp); len(challenges) != 0 {
-				// If we hit more than one, I'm not even sure what to do.
-				wac := challenges[0]
+				// If we hit more than one, let's try to find one that we know how to handle.
+				wac := pickFromMultipleChallenges(challenges)
 				return &pingResp{
 					challenge:  challenge(wac.Scheme).Canonical(),
 					parameters: wac.Parameters,
@@ -127,3 +127,22 @@ func ping(ctx context.Context, reg name.Registry, t http.RoundTripper) (*pingRes
 	}
 	return nil, errors.New(strings.Join(errs, "; "))
 }
+
+func pickFromMultipleChallenges(challenges []authchallenge.Challenge) authchallenge.Challenge {
+
+	// It might happen there are multiple www-authenticate headers, e.g. `Negotiate` and `Basic`.
+	// Picking simply the first one could result eventually in `unrecognized challenge` error,
+	// that's why we're looping through the challenges in search for one that can be handled.
+	allowedSchemes := []string{"basic", "bearer"}
+
+	for _, wac := range challenges {
+		currentScheme := strings.ToLower(wac.Scheme)
+		for _, allowed := range allowedSchemes {
+			if allowed == currentScheme {
+				return wac
+			}
+		}
+	}
+
+	return challenges[0]
+}
diff --git a/vendor/modules.txt b/vendor/modules.txt
index 4d5e2e6d..20335dc8 100644
--- a/vendor/modules.txt
+++ b/vendor/modules.txt
@@ -10,7 +10,7 @@ github.com/Microsoft/go-winio/pkg/guid
 github.com/containerd/containerd/errdefs
 github.com/containerd/containerd/log
 github.com/containerd/containerd/platforms
-# github.com/containerd/stargz-snapshotter/estargz v0.6.4
+# github.com/containerd/stargz-snapshotter/estargz v0.7.0
 ## explicit
 github.com/containerd/stargz-snapshotter/estargz
 github.com/containerd/stargz-snapshotter/estargz/errorutil


Test patch:
diff --git a/pkg/v1/mutate/mutate_test.go b/pkg/v1/mutate/mutate_test.go
index 5aeb66d6..940234a2 100644
--- a/pkg/v1/mutate/mutate_test.go
+++ b/pkg/v1/mutate/mutate_test.go
@@ -266,21 +266,43 @@ func TestMutateConfig(t *testing.T) {
 	}
 }
 
-func TestAnnotations(t *testing.T) {
-	source := sourceImage(t)
-
-	newAnnotations := map[string]string{
-		"im.the.first.annotation": "hello world",
-	}
-
-	result := mutate.Annotations(source, newAnnotations)
-
-	if configDigestsAreEqual(t, source, result) {
-		t.Errorf("mutating the manifest annotations MUST mutate the config digest")
-	}
+type arbitrary struct {
+}
 
-	if err := validate.Image(result); err != nil {
-		t.Errorf("validate.Image() = %v", err)
+func (arbitrary) RawManifest() ([]byte, error) {
+	return []byte(`{"hello":"world"}`), nil
+}
+func TestAnnotations(t *testing.T) {
+	anns := map[string]string{
+		"foo": "bar",
+	}
+
+	for _, c := range []struct {
+		desc string
+		in   mutate.Annotatable
+		want string
+	}{{
+		desc: "image",
+		in:   empty.Image,
+		want: `{"schemaVersion":2,"mediaType":"application/vnd.docker.distribution.manifest.v2+json","config":{"mediaType":"application/vnd.docker.container.image.v1+json","size":115,"digest":"sha256:5b943e2b943f6c81dbbd4e2eca5121f4fcc39139e3d1219d6d89bd925b77d9fe"},"layers":[],"annotations":{"foo":"bar"}}`,
+	}, {
+		desc: "index",
+		in:   empty.Index,
+		want: `{"schemaVersion":2,"manifests":null,"annotations":{"foo":"bar"}}`,
+	}, {
+		desc: "arbitrary",
+		in:   arbitrary{},
+		want: `{"annotations":{"foo":"bar"},"hello":"world"}`,
+	}} {
+		t.Run(c.desc, func(t *testing.T) {
+			got, err := mutate.Annotations(c.in, anns).RawManifest()
+			if err != nil {
+				t.Fatalf("Annotations: %v", err)
+			}
+			if d := cmp.Diff(c.want, string(got)); d != "" {
+				t.Errorf("Diff(-want,+got): %s", d)
+			}
+		})
 	}
 }
 
diff --git a/pkg/v1/remote/transport/ping_test.go b/pkg/v1/remote/transport/ping_test.go
index aa3a42f3..afddd011 100644
--- a/pkg/v1/remote/transport/ping_test.go
+++ b/pkg/v1/remote/transport/ping_test.go
@@ -147,6 +147,55 @@ func TestPingBearerChallengeWithParams(t *testing.T) {
 	}
 }
 
+func TestPingMultipleChallenges(t *testing.T) {
+	server := httptest.NewServer(
+		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Header().Add("WWW-Authenticate", "Negotiate")
+			w.Header().Add("WWW-Authenticate", `Basic realm="http://auth.example.com/token"`)
+			http.Error(w, "Unauthorized", http.StatusUnauthorized)
+		}))
+	defer server.Close()
+	tprt := &http.Transport{
+		Proxy: func(req *http.Request) (*url.URL, error) {
+			return url.Parse(server.URL)
+		},
+	}
+
+	pr, err := ping(context.Background(), testRegistry, tprt)
+	if err != nil {
+		t.Errorf("ping() = %v", err)
+	}
+	if pr.challenge != basic {
+		t.Errorf("ping(); got %v, want %v", pr.challenge, basic)
+	}
+	if got, want := len(pr.parameters), 1; got != want {
+		t.Errorf("ping(); got %v, want %v", got, want)
+	}
+}
+
+func TestPingMultipleNotSupportedChallenges(t *testing.T) {
+	server := httptest.NewServer(
+		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Header().Add("WWW-Authenticate", "Negotiate")
+			w.Header().Add("WWW-Authenticate", "Digest")
+			http.Error(w, "Unauthorized", http.StatusUnauthorized)
+		}))
+	defer server.Close()
+	tprt := &http.Transport{
+		Proxy: func(req *http.Request) (*url.URL, error) {
+			return url.Parse(server.URL)
+		},
+	}
+
+	pr, err := ping(context.Background(), testRegistry, tprt)
+	if err != nil {
+		t.Errorf("ping() = %v", err)
+	}
+	if pr.challenge != "negotiate" {
+		t.Errorf("ping(); got %v, want %v", pr.challenge, "negotiate")
+	}
+}
+
 func TestUnsupportedStatus(t *testing.T) {
 	server := httptest.NewServer(
 		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {