From 5c66703ee37a122f813f9cd1cb37c792e9f7097c Mon Sep 17 00:00:00 2001 From: cshung Date: Mon, 6 Jul 2026 15:14:45 +0000 Subject: [PATCH] remotes: surface OCI error body on HEAD 403 via GET fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a registry returns 403 Forbidden on a HEAD request (e.g., manifest resolve or push existence check), the diagnostic error body is lost because HEAD responses carry no body per HTTP spec. This leaves users with an opaque "403 Forbidden" message and no actionable guidance. Add a follow-up GET on HEAD 403 to retrieve the registry's OCI error body. The existing unexpectedResponseErr machinery already parses the body into structured errors — it just needs the body to be present. The fallback lives in a shared withGETErrorBody helper used by both the pusher and resolver: it only enriches when the GET also returns 403, and preserves the original HEAD request's method and status while borrowing just the body, so the resulting error's status and body stay consistent. Scoped to 403 only because it is rare (CMK key disabled, IP firewall, RBAC misconfiguration) and its body is highly diagnostic, while other status codes either already use GET or have bodies that add no value. Fixes #8969 Signed-off-by: Andrew Au --- core/remotes/docker/pusher.go | 47 +++++++ core/remotes/docker/pusher_test.go | 75 ++++++++++ core/remotes/docker/resolver.go | 19 ++- core/remotes/docker/resolver_test.go | 199 +++++++++++++++++++++++++++ 4 files changed, 339 insertions(+), 1 deletion(-) diff --git a/core/remotes/docker/pusher.go b/core/remotes/docker/pusher.go index 900389a0fb143..94ef661d90d85 100644 --- a/core/remotes/docker/pusher.go +++ b/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 diff --git a/core/remotes/docker/pusher_test.go b/core/remotes/docker/pusher_test.go index 9a68f93c31894..2e2a8e807099c 100644 --- a/core/remotes/docker/pusher_test.go +++ b/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") +} diff --git a/core/remotes/docker/resolver.go b/core/remotes/docker/resolver.go index dd89921562a87..45ce4dd59ddf9 100644 --- a/core/remotes/docker/resolver.go +++ b/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) diff --git a/core/remotes/docker/resolver_test.go b/core/remotes/docker/resolver_test.go index 1bd65a9eb3737..c691949f7283c 100644 --- a/core/remotes/docker/resolver_test.go +++ b/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()) + } +}