Merge pull request #13738 from k8s-infra-cherrypick-robot/cherry-pick-13547-to-release/2.3

[release/2.3] remotes: surface OCI error body in registry 4xx responses

core/remotes/docker/pusher.go

@@ -158,6 +158,18 @@ func (p dockerPusher) push(ctx context.Context, desc ocispec.Descriptor, ref str
 			}
 		} else if resp.StatusCode != http.StatusNotFound {
 			err := unexpectedResponseErr(resp)
+			// A HEAD 403 carries no body, so issue a follow-up GET to the
+			// same URL to surface the registry's error details for diagnostics.
+			if resp.StatusCode == http.StatusForbidden && req.method == http.MethodHead {
+				err = withGETErrorBody(ctx, err, resp, func() (*http.Response, error) {
+					getReq := p.request(host, http.MethodGet, existCheck...)
+					getReq.header.Set("Accept", strings.Join([]string{desc.MediaType, `*/*`}, ", "))
+					if addErr := getReq.addNamespace(p.refspec.Hostname()); addErr != nil {
+						return nil, addErr
+					}
+					return getReq.doWithRetries(ctx, false)
+				})
+			}
 			log.G(ctx).WithError(err).Debug("unexpected response")
 			resp.Body.Close()
 			return nil, err
@@ -574,6 +586,41 @@ func (pw *pushWriter) Truncate(size int64) error {
 	return errors.New("cannot truncate remote upload")
 }
 
+// withGETErrorBody enriches originalErr, produced from a bodyless HEAD
+// response, with the error body from a follow-up GET to the same URL. HEAD
+// responses carry no body, so a 403 only surfaces its status code; a GET
+// returns the registry's error details (e.g. "key vault access denied", IP
+// restrictions) that explain the failure.
+//
+// The GET body is only used when the GET also returns 403, so the enriched
+// error's status and body stay consistent. In that case the original HEAD
+// request's method and status are preserved and only the body is borrowed from
+// the GET; any other outcome (GET failed, or returned a different status)
+// leaves originalErr untouched.
+func withGETErrorBody(ctx context.Context, originalErr error, headResp *http.Response, doGET func() (*http.Response, error)) error {
+	getResp, err := doGET()
+	if err != nil {
+		log.G(ctx).WithError(err).Debug("failed to retrieve error body with GET fallback")
+		return originalErr
+	}
+	defer getResp.Body.Close()
+
+	if getResp.StatusCode != http.StatusForbidden {
+		log.G(ctx).WithFields(log.Fields{
+			"head_status": headResp.Status,
+			"get_status":  getResp.Status,
+		}).Debug("ignoring GET fallback response with different status")
+		return originalErr
+	}
+
+	// Preserve the original HEAD request's method and status and borrow only
+	// the body from the GET, so the error still reflects the request the caller
+	// actually made.
+	enriched := *headResp
+	enriched.Body = getResp.Body
+	return unexpectedResponseErr(&enriched)
+}
+
 func requestWithMountFrom(req *request, mount, from string) *request {
 	creq := *req
 

core/remotes/docker/pusher_test.go

@@ -739,3 +739,78 @@ func Test_dockerPusher_push(t *testing.T) {
 		})
 	}
 }
+
+func TestPusherForbiddenGETFallbackForErrorBody(t *testing.T) {
+	// When the existence-check HEAD returns 403, the pusher should issue
+	// a follow-up GET to retrieve the registry's error body for diagnostics.
+	const errorMessage = "encryption key is disabled"
+
+	p, reg, _, done := samplePusher(t)
+	defer done()
+
+	reg.defaultHandlerFunc = func(w http.ResponseWriter, r *http.Request) bool {
+		if strings.Contains(r.URL.Path, "/manifests/") || strings.Contains(r.URL.Path, "/blobs/sha256:") {
+			w.Header().Set("Content-Type", "application/json")
+			w.WriteHeader(http.StatusForbidden)
+			if r.Method == http.MethodGet {
+				w.Write([]byte(fmt.Sprintf(`{"errors":[{"code":"DENIED","message":"%s"}]}`, errorMessage)))
+			}
+			// HEAD: no body
+			return true
+		}
+		return false
+	}
+
+	ctx := context.Background()
+	desc := ocispec.Descriptor{
+		MediaType: ocispec.MediaTypeImageManifest,
+		Digest:    digest.FromString("test-manifest"),
+		Size:      100,
+	}
+
+	_, err := p.push(ctx, desc, "test-ref", true)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), errorMessage,
+		"expected registry error body from GET fallback")
+	assert.Contains(t, err.Error(), "403")
+}
+
+func TestPusherForbiddenGETFallbackProxy(t *testing.T) {
+	// When the pusher is configured as a proxy (Host != image hostname),
+	// the GET fallback must include the ?ns= query parameter so the proxy
+	// can route the request to the correct upstream registry.
+	const errorMessage = "encryption key is disabled"
+
+	var gotNsParam string
+
+	p, reg, _, done := samplePusher(t)
+	defer done()
+
+	reg.defaultHandlerFunc = func(w http.ResponseWriter, r *http.Request) bool {
+		if strings.Contains(r.URL.Path, "/manifests/") || strings.Contains(r.URL.Path, "/blobs/sha256:") {
+			if r.Method == http.MethodGet {
+				gotNsParam = r.URL.Query().Get("ns")
+			}
+			w.Header().Set("Content-Type", "application/json")
+			w.WriteHeader(http.StatusForbidden)
+			if r.Method == http.MethodGet {
+				w.Write([]byte(fmt.Sprintf(`{"errors":[{"code":"DENIED","message":"%s"}]}`, errorMessage)))
+			}
+			return true
+		}
+		return false
+	}
+
+	ctx := context.Background()
+	desc := ocispec.Descriptor{
+		MediaType: ocispec.MediaTypeImageManifest,
+		Digest:    digest.FromString("test-manifest"),
+		Size:      100,
+	}
+
+	_, err := p.push(ctx, desc, "test-ref", true)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), errorMessage)
+	assert.Equal(t, samplePusherHostname, gotNsParam,
+		"GET fallback on proxy must include ?ns= query parameter")
+}

core/remotes/docker/resolver.go

@@ -341,8 +341,25 @@ func (r *dockerResolver) Resolve(ctx context.Context, ref string) (string, ocisp
 					continue
 				}
 				if resp.StatusCode > 399 {
+					err := unexpectedResponseErr(resp)
+					// A HEAD 403 carries no body, so issue a follow-up GET to
+					// the same URL to surface the registry's error details
+					// (e.g. "key vault access denied", IP restrictions) for
+					// diagnostics.
+					if resp.StatusCode == http.StatusForbidden && req.method == http.MethodHead {
+						err = withGETErrorBody(ctx, err, resp, func() (*http.Response, error) {
+							getReq := base.request(host, http.MethodGet, u...)
+							if addErr := getReq.addNamespace(base.refspec.Hostname()); addErr != nil {
+								return nil, addErr
+							}
+							for key, value := range r.resolveHeader {
+								getReq.header[key] = append(getReq.header[key], value...)
+							}
+							return getReq.doWithRetries(ctx, false)
+						})
+					}
 					if firstErrPriority < 3 {
-						firstErr = unexpectedResponseErr(resp)
+						firstErr = err
 						firstErrPriority = 3
 					}
 					log.G(ctx).Infof("%s after status: %s", nextHostOrFail(i), resp.Status)

core/remotes/docker/resolver_test.go

@@ -1602,3 +1602,202 @@ func TestResolverErrorStatusCodeOnFetch(t *testing.T) {
 		})
 	}
 }
+
+func TestResolveForbiddenGETFallbackForErrorBody(t *testing.T) {
+	// When HEAD returns 403 with no body, the resolver should issue a
+	// follow-up GET to retrieve the registry's error details.
+	const (
+		name      = "test/repo"
+		tag       = "latest"
+		errorBody = `{"errors":[{"code":"DENIED","message":"encryption key is disabled"}]}`
+	)
+
+	handler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
+		if strings.Contains(r.URL.Path, "/token") {
+			rw.Header().Set("Content-Type", "application/json")
+			rw.Write([]byte(`{"access_token":"test"}`))
+			return
+		}
+		if strings.Contains(r.URL.Path, "/manifests/") {
+			rw.Header().Set("Content-Type", "application/json")
+			rw.WriteHeader(http.StatusForbidden)
+			if r.Method == http.MethodGet {
+				rw.Write([]byte(errorBody))
+			}
+			return
+		}
+		if r.URL.Path == "/v2/" {
+			rw.WriteHeader(http.StatusOK)
+			return
+		}
+		rw.WriteHeader(http.StatusNotFound)
+	})
+
+	base, options, close := tlsServer(handler)
+	defer close()
+
+	options.Hosts = ConfigureDefaultRegistries(WithClient(options.Client))
+	resolver := NewResolver(options)
+	ref := fmt.Sprintf("%s/%s:%s", base, name, tag)
+
+	_, _, err := resolver.Resolve(context.Background(), ref)
+	if err == nil {
+		t.Fatal("expected error from resolve, got nil")
+	}
+
+	errMsg := err.Error()
+	if !strings.Contains(errMsg, "encryption key is disabled") {
+		t.Errorf("expected error to contain registry error body, got: %s", errMsg)
+	}
+	if !strings.Contains(errMsg, "403") {
+		t.Errorf("expected error to contain status code 403, got: %s", errMsg)
+	}
+}
+
+func TestResolveForbiddenNoFallbackOnGET(t *testing.T) {
+	const (
+		name = "test/repo"
+		tag  = "latest"
+	)
+
+	var getCount int
+	handler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
+		if strings.Contains(r.URL.Path, "/token") {
+			rw.Header().Set("Content-Type", "application/json")
+			rw.Write([]byte(`{"access_token":"test"}`))
+			return
+		}
+		if strings.Contains(r.URL.Path, "/manifests/") {
+			if r.Method == http.MethodGet {
+				getCount++
+			}
+			rw.Header().Set("Content-Type", "application/json")
+			rw.WriteHeader(http.StatusForbidden)
+			rw.Write([]byte(`{"errors":[{"code":"DENIED","message":"forbidden"}]}`))
+			return
+		}
+		if r.URL.Path == "/v2/" {
+			rw.WriteHeader(http.StatusOK)
+			return
+		}
+		rw.WriteHeader(http.StatusNotFound)
+	})
+
+	base, options, close := tlsServer(handler)
+	defer close()
+
+	options.Hosts = ConfigureDefaultRegistries(WithClient(options.Client))
+	resolver := NewResolver(options)
+	ref := fmt.Sprintf("%s/%s:%s", base, name, tag)
+
+	_, _, err := resolver.Resolve(context.Background(), ref)
+	if err == nil {
+		t.Fatal("expected error from resolve, got nil")
+	}
+
+	// The resolver should issue exactly 1 GET (the fallback from HEAD 403).
+	// It must NOT issue a second fallback GET when the first GET also returns 403.
+	if getCount != 1 {
+		t.Errorf("expected exactly 1 GET fallback request for manifests, got %d", getCount)
+	}
+}
+
+func TestResolve404NoFallbackGET(t *testing.T) {
+	const (
+		name = "test/repo"
+		tag  = "latest"
+	)
+
+	var getCount int
+	handler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
+		if strings.Contains(r.URL.Path, "/token") {
+			rw.Header().Set("Content-Type", "application/json")
+			rw.Write([]byte(`{"access_token":"test"}`))
+			return
+		}
+		if strings.Contains(r.URL.Path, "/manifests/") {
+			if r.Method == http.MethodGet {
+				getCount++
+			}
+			rw.WriteHeader(http.StatusNotFound)
+			return
+		}
+		if r.URL.Path == "/v2/" {
+			rw.WriteHeader(http.StatusOK)
+			return
+		}
+		rw.WriteHeader(http.StatusNotFound)
+	})
+
+	base, options, close := tlsServer(handler)
+	defer close()
+
+	options.Hosts = ConfigureDefaultRegistries(WithClient(options.Client))
+	resolver := NewResolver(options)
+	ref := fmt.Sprintf("%s/%s:%s", base, name, tag)
+
+	_, _, err := resolver.Resolve(context.Background(), ref)
+	if err == nil {
+		t.Fatal("expected error from resolve, got nil")
+	}
+
+	if getCount != 0 {
+		t.Errorf("expected 0 GET requests for 404, got %d", getCount)
+	}
+}
+
+func TestResolveForbiddenGETFallbackNetworkError(t *testing.T) {
+	const (
+		name = "test/repo"
+		tag  = "latest"
+	)
+
+	var requestCount int
+	handler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
+		if strings.Contains(r.URL.Path, "/token") {
+			rw.Header().Set("Content-Type", "application/json")
+			rw.Write([]byte(`{"access_token":"test"}`))
+			return
+		}
+		if strings.Contains(r.URL.Path, "/manifests/") {
+			requestCount++
+			if r.Method == http.MethodHead {
+				rw.WriteHeader(http.StatusForbidden)
+				return
+			}
+			hj, ok := rw.(http.Hijacker)
+			if !ok {
+				rw.WriteHeader(http.StatusForbidden)
+				return
+			}
+			conn, _, err := hj.Hijack()
+			if err != nil {
+				rw.WriteHeader(http.StatusInternalServerError)
+				return
+			}
+			conn.Close()
+			return
+		}
+		if r.URL.Path == "/v2/" {
+			rw.WriteHeader(http.StatusOK)
+			return
+		}
+		rw.WriteHeader(http.StatusNotFound)
+	})
+
+	base, options, close := tlsServer(handler)
+	defer close()
+
+	options.Hosts = ConfigureDefaultRegistries(WithClient(options.Client))
+	resolver := NewResolver(options)
+	ref := fmt.Sprintf("%s/%s:%s", base, name, tag)
+
+	_, _, err := resolver.Resolve(context.Background(), ref)
+	if err == nil {
+		t.Fatal("expected error from resolve, got nil")
+	}
+
+	if !strings.Contains(err.Error(), "403") {
+		t.Errorf("expected 403 in error, got: %s", err.Error())
+	}
+}