From 1b63906b63b4d7ff6647fd3899c3fda3f251d1cd Mon Sep 17 00:00:00 2001 From: MgSrdEer <30002554+MgSrdEer@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:24:58 +0800 Subject: [PATCH] fix(ownership): authorize the query tag on Docker-compatible image push MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /images/{name}/push names its subject in two pieces: the repository in the path and the tag in ?tag=. imageIdentifier strips the "/push" suffix and handed the bare repository to the ownership inspect, which resolves the daemon's default tag (typically :latest). Two failures followed: - A proxy-scoped build pushed as {name}:{tag} was denied with "owner policy could not resolve image" whenever {name}:latest happened to be absent. That is the standard `docker push registry/repo:tag` CLI spelling (the CLI sends the tag in the query, never in the path), so every tagged push against an image without a local :latest tag failed. - When {name}:latest WAS present, the push was authorized on tag-ref equality: a caller owning only :latest could push {name}:{anything}. The mutation pass now captures the query tag (ownershipRequestReferences.imagePushTag) and the authorization pass qualifies the identifier with it, so the inspect resolves exactly the local image the daemon will push. Push forms whose effective reference one image inspect cannot enumerate are refused instead of guessed at, via the denyReason path commit already uses for query-named resources: - No tag at all: distribution.Push pushes every local reference of the repository (daemon/internal/distribution/push.go: "If no tag is provided, all tags are pushed"), an effect one image inspect cannot enumerate — the same shape imageEffectDenial refuses per-image exports and deletes for. The docker CLI never sends this form (it resolves a name-only reference to ?tag=latest), so it is reachable only by direct API clients. - A repeated or case-variant tag spelling: dockerd reads the first value and Podman's compat handler the last (filter.FoldedScalarQueryValue is the existing helper for this disagreement). The retag route POST /images/{name}/tag keeps its bare-path authorization: the resource it mutates is the source image the path names, and `docker tag src dst` spells the full source reference into the path. Podman's native POST /libpod/images/{name}/push carries the full reference in the path and is unaffected. Verified end to end against dockerd 26.1.3: a tagged push of an owned image without any local :latest succeeds; the bare and repeated-tag forms get their new denials; a foreign-owned tag is refused with the ordinary cross-owner denial. --- app/internal/ownership/image_push.go | 81 +++++++ app/internal/ownership/image_push_test.go | 273 ++++++++++++++++++++++ app/internal/ownership/middleware.go | 14 +- 3 files changed, 366 insertions(+), 2 deletions(-) create mode 100644 app/internal/ownership/image_push.go create mode 100644 app/internal/ownership/image_push_test.go diff --git a/app/internal/ownership/image_push.go b/app/internal/ownership/image_push.go new file mode 100644 index 00000000..ed25adc3 --- /dev/null +++ b/app/internal/ownership/image_push.go @@ -0,0 +1,81 @@ +package ownership + +import ( + "net/http" + "strings" + + "github.com/codeswhat/sockguard/app/internal/filter" +) + +const ( + imagePushTagQueryField = "tag" + + imagePushDenyNoTag = "owner policy denied image push without an explicit tag: it pushes every local tag of the repository" + imagePushDenyAmbiguous = "owner policy denied image push with an ambiguous tag parameter" + imagePushDenyUnresolved = "owner policy could not resolve image" +) + +// isImagePushRoutePath reports whether normPath is the Docker-compatible +// per-image push route, the one both dockerd and Podman's compat handler +// serve as POST /images/{name}/push. Podman's native POST +// /libpod/images/{name}/push carries the full reference (tag included) in +// the path itself, so it has no query tag to capture and stays out of scope +// here. +func isImagePushRoutePath(method, normPath string) bool { + return method == http.MethodPost && + strings.HasPrefix(normPath, "/images/") && + strings.HasSuffix(normPath, "/push") +} + +// imagePushOwnershipReferences reads the push query and returns either the +// tag qualifier the authorization pass appends to the path identifier, or +// the reason the request is refused outright. +// +// The Docker-compatible push route names its subject in two pieces: the +// repository in the path and the tag in ?tag=. imageIdentifier strips the +// "/push" suffix and hands back the bare repository, whose inspect resolves +// the daemon's default tag (typically :latest) — a reference that is neither +// the one the daemon will push nor a stable one the authorization can rely +// on. With a tag present, the authorization pass checks exactly +// {name}:{tag}; two shapes are refused rather than guessed at: +// +// - No `tag` parameter at all. Moby's postImagesPush treats an empty tag +// as "push every local tag of the repository", an effect one image +// inspect cannot enumerate — the same reason imageEffectDenial refuses +// per-image exports and deletes outright. +// - A repeated or two-case-variant `tag`. Moby reads the first value of a +// repeated parameter and Podman's compat handler the last, so a request +// naming an owned tag and a foreign one would be checked against one and +// pushed as the other. filter.FoldedScalarQueryValue is the same helper +// the container-archive policy and commit's container parameter use for +// this disagreement. +// +// The retag route POST /images/{name}/tag deliberately keeps its bare-path +// authorization: the resource it mutates is the source image the path names, +// and docker tag src dst spells the full source reference (tag included) +// into the path. +func imagePushOwnershipReferences(r *http.Request) *ownershipRequestReferences { + refs := &ownershipRequestReferences{} + tag, found, ambiguous := filter.FoldedScalarQueryValue(r.URL.Query(), imagePushTagQueryField) + switch { + case ambiguous: + refs.denyReason = imagePushDenyAmbiguous + case !found || strings.TrimSpace(tag) == "": + refs.denyReason = imagePushDenyNoTag + default: + refs.imagePushTag = strings.TrimSpace(tag) + } + return refs +} + +// appendImagePushTag qualifies a Docker-compatible push identifier with the +// captured tag, so checkOwnedResource inspects the exact local image the +// daemon will push. The refs must come from the same request; a nil refs (a +// direct caller of the authorization functions with no mutation pass behind +// it) leaves the identifier untouched. +func appendImagePushTag(identifier string, refs *ownershipRequestReferences, method, normPath string) string { + if refs == nil || refs.imagePushTag == "" || !isImagePushRoutePath(method, normPath) { + return identifier + } + return identifier + ":" + refs.imagePushTag +} diff --git a/app/internal/ownership/image_push_test.go b/app/internal/ownership/image_push_test.go new file mode 100644 index 00000000..14acebb4 --- /dev/null +++ b/app/internal/ownership/image_push_test.go @@ -0,0 +1,273 @@ +package ownership + +import ( + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/codeswhat/sockguard/app/internal/dockerresource" +) + +func imagePushInspector(ownedTag string, owner string) *recordingInspector { + return &recordingInspector{resources: map[string]map[string]inspectResult{ + string(dockerresource.KindImage): { + ownedTag: {labels: map[string]string{"com.sockguard.owner": owner}, found: true}, + }, + }} +} + +func serveImagePush(t *testing.T, inspector *recordingInspector, opts Options, target string, upstream func(*testing.T, *http.Request)) *httptest.ResponseRecorder { + t.Helper() + handler := middlewareWithDeps( + testLogger(), + opts, + inspector.inspectResource, + inspector.inspectExec, + )(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if upstream == nil { + t.Fatal("push request reached the upstream") + } + upstream(t, r) + w.WriteHeader(http.StatusOK) + })) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, httptest.NewRequest(http.MethodPost, target, nil)) + return rec +} + +// TestImagePushAuthorizesQueryTagNotDefaultTag is the regression test for the +// push misresolution: POST /images/{name}/push?tag=X names its subject in two +// pieces, and the bare repository identifier resolved the daemon's default +// tag. A proxy-scoped build pushed as {name}:{tag} was denied whenever +// {name}:latest happened to be absent ("could not resolve image"), while a +// caller owning only {name}:latest could push {name}:{anything}. +func TestImagePushAuthorizesQueryTagNotDefaultTag(t *testing.T) { + const ( + owner = "job-123" + repoPath = "/v1.45/images/registry.example/team/app/push" + ownedRef = "registry.example/team/app:v1" + ) + + t.Run("owned tagged image without a local latest is allowed", func(t *testing.T) { + inspector := imagePushInspector(ownedRef, owner) + // No registry.example/team/app:latest and no bare repository entry: + // before the fix the inspect missed and the push was denied. + rec := serveImagePush(t, inspector, Options{Owner: owner, LabelKey: "com.sockguard.owner"}, repoPath+"?tag=v1", func(_ *testing.T, _ *http.Request) {}) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want %d; body: %s", rec.Code, http.StatusOK, rec.Body.String()) + } + if len(inspector.calls) != 1 { + t.Fatalf("inspect calls = %#v, want exactly one", inspector.calls) + } + if got := inspector.calls[0].id; got != ownedRef { + t.Fatalf("inspect identifier = %q, want the tag-qualified %q", got, ownedRef) + } + }) + + t.Run("foreign-owned tagged image is denied even when latest is owned", func(t *testing.T) { + inspector := &recordingInspector{resources: map[string]map[string]inspectResult{ + string(dockerresource.KindImage): { + "registry.example/team/app:latest": {labels: map[string]string{"com.sockguard.owner": owner}, found: true}, + "registry.example/team/app:foreign-tag": {labels: map[string]string{"com.sockguard.owner": "someone-else"}, found: true}, + }, + }} + rec := serveImagePush(t, inspector, Options{Owner: owner, LabelKey: "com.sockguard.owner"}, repoPath+"?tag=foreign-tag", nil) + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want %d; body: %s", rec.Code, http.StatusForbidden, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "owner policy") { + t.Fatalf("body should carry the owner-policy denial, got: %s", rec.Body.String()) + } + }) + + t.Run("locally absent tagged image fails closed as not resolved", func(t *testing.T) { + inspector := &recordingInspector{resources: map[string]map[string]inspectResult{ + string(dockerresource.KindImage): { + "registry.example/team/app:latest": {labels: map[string]string{"com.sockguard.owner": owner}, found: true}, + }, + }} + rec := serveImagePush(t, inspector, Options{Owner: owner, LabelKey: "com.sockguard.owner"}, repoPath+"?tag=absent-tag", nil) + if rec.Code != http.StatusNotFound { + t.Fatalf("status = %d, want %d (verdictDenyMissing); body: %s", rec.Code, http.StatusNotFound, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "could not resolve image") { + t.Fatalf("body should carry the not-resolved denial, got: %s", rec.Body.String()) + } + }) + + t.Run("unowned tagged image honors allow_unowned_images", func(t *testing.T) { + for _, allow := range []struct { + flag bool + want int + }{{true, http.StatusOK}, {false, http.StatusForbidden}} { + inspector := &recordingInspector{resources: map[string]map[string]inspectResult{ + string(dockerresource.KindImage): { + "registry.example/team/app:v1": {labels: nil, found: true}, + }, + }} + rec := serveImagePush(t, inspector, Options{Owner: owner, LabelKey: "com.sockguard.owner", AllowUnownedImages: allow.flag}, repoPath+"?tag=v1", func(_ *testing.T, _ *http.Request) {}) + if rec.Code != allow.want { + t.Fatalf("allow_unowned_images=%v status = %d, want %d; body: %s", allow.flag, rec.Code, allow.want, rec.Body.String()) + } + } + }) + + t.Run("registry host with port is not mistaken for a tag", func(t *testing.T) { + const withPort = "registry.example:5000/team/app:v1" + inspector := imagePushInspector(withPort, owner) + rec := serveImagePush(t, inspector, Options{Owner: owner, LabelKey: "com.sockguard.owner"}, "/v1.45/images/registry.example:5000/team/app/push?tag=v1", func(_ *testing.T, _ *http.Request) {}) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want %d; body: %s", rec.Code, http.StatusOK, rec.Body.String()) + } + if got := inspector.calls[0].id; got != withPort { + t.Fatalf("inspect identifier = %q, want %q", got, withPort) + } + }) +} + +// TestImagePushRefusesUnenumerableTagShapes covers the push forms one image +// inspect cannot authorize: no tag at all (moby pushes every local tag of the +// repository) and a repeated or case-variant tag (moby reads the first value, +// Podman's compat handler the last). +func TestImagePushRefusesUnenumerableTagShapes(t *testing.T) { + const owner = "job-123" + inspector := imagePushInspector("registry.example/team/app:anything", owner) + + tests := []struct { + name string + target string + reason string + }{ + {name: "no tag pushes every local tag", target: "/v1.45/images/registry.example/team/app/push", reason: imagePushDenyNoTag}, + {name: "empty tag is the same push-all shape", target: "/v1.45/images/registry.example/team/app/push?tag=", reason: imagePushDenyNoTag}, + {name: "repeated tag values disagree between engines", target: "/v1.45/images/registry.example/team/app/push?tag=v1&tag=v2", reason: imagePushDenyAmbiguous}, + {name: "case-variant tag keys disagree between engines", target: "/v1.45/images/registry.example/team/app/push?tag=v1&Tag=v2", reason: imagePushDenyAmbiguous}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rec := serveImagePush(t, inspector, Options{Owner: owner, LabelKey: "com.sockguard.owner"}, tt.target, nil) + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want %d; body: %s", rec.Code, http.StatusForbidden, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), tt.reason) { + t.Fatalf("body should carry %q, got: %s", tt.reason, rec.Body.String()) + } + }) + } +} + +// TestImagePushDoesNotTouchOtherImageRoutes pins the neighbors: the retag +// route keeps authorizing the bare path source (docker tag src dst spells the +// full source reference into the path), and the plain image inspect keeps its +// bare-identifier semantics. +func TestImagePushDoesNotTouchOtherImageRoutes(t *testing.T) { + const owner = "job-123" + + t.Run("retag authorizes the bare path source", func(t *testing.T) { + inspector := &recordingInspector{resources: map[string]map[string]inspectResult{ + string(dockerresource.KindImage): { + "src": {labels: map[string]string{"com.sockguard.owner": owner}, found: true}, + }, + }} + handler := middlewareWithDeps( + testLogger(), + Options{Owner: owner, LabelKey: "com.sockguard.owner"}, + inspector.inspectResource, + inspector.inspectExec, + )(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) })) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, httptest.NewRequest(http.MethodPost, "/v1.45/images/src/tag?repo=registry.example/team/app&tag=v2", nil)) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want %d; body: %s", rec.Code, http.StatusOK, rec.Body.String()) + } + if len(inspector.calls) != 1 || inspector.calls[0].id != "src" { + t.Fatalf("inspect calls = %#v, want exactly one for the bare source %q", inspector.calls, "src") + } + }) + + t.Run("image inspect keeps bare identifier", func(t *testing.T) { + inspector := &recordingInspector{resources: map[string]map[string]inspectResult{ + string(dockerresource.KindImage): { + "registry.example/team/app": {labels: map[string]string{"com.sockguard.owner": owner}, found: true}, + }, + }} + handler := middlewareWithDeps( + testLogger(), + Options{Owner: owner, LabelKey: "com.sockguard.owner"}, + inspector.inspectResource, + inspector.inspectExec, + )(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) })) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/v1.45/images/registry.example/team/app/json", nil)) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want %d; body: %s", rec.Code, http.StatusOK, rec.Body.String()) + } + if len(inspector.calls) != 1 || inspector.calls[0].id != "registry.example/team/app" { + t.Fatalf("inspect calls = %#v, want exactly one bare identifier", inspector.calls) + } + }) +} + +// TestIsImagePushRoutePathClassification pins the route classifier itself. +func TestIsImagePushRoutePathClassification(t *testing.T) { + tests := []struct { + method string + path string + want bool + }{ + {http.MethodPost, "/images/app/push", true}, + {http.MethodPost, "/v1.45/images/registry.example/team/app/push", false}, // normPath is version-stripped; the classifier never sees a prefix + {http.MethodPost, "/images/app/push?tag=v1", false}, // normPath never carries a query + {http.MethodGet, "/images/app/push", false}, + {http.MethodPost, "/images/app/tag", false}, + {http.MethodPost, "/images/app/json", false}, + {http.MethodPost, "/libpod/images/app/push", false}, + {http.MethodPost, "/images/create", false}, + {http.MethodPost, "/containers/app/push", false}, + } + for _, tt := range tests { + if got := isImagePushRoutePath(tt.method, tt.path); got != tt.want { + t.Errorf("isImagePushRoutePath(%q, %q) = %v, want %v", tt.method, tt.path, got, tt.want) + } + } +} + +// TestImagePushOwnershipReferencesParsing covers the query extraction in +// isolation, including whitespace-only tags. +func TestImagePushOwnershipReferencesParsing(t *testing.T) { + tests := []struct { + name string + rawQuery string + wantTag string + wantDenyFor string + }{ + {name: "plain tag", rawQuery: "tag=v1", wantTag: "v1"}, + {name: "tag with slash", rawQuery: url.QueryEscape("tag") + "=" + url.QueryEscape("v1.2/rc-3"), wantTag: "v1.2/rc-3"}, + {name: "whitespace tag", rawQuery: "tag=%20%20", wantDenyFor: imagePushDenyNoTag}, + {name: "no query", rawQuery: "", wantDenyFor: imagePushDenyNoTag}, + {name: "repeated tag", rawQuery: "tag=a&tag=b", wantDenyFor: imagePushDenyAmbiguous}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + refs := imagePushOwnershipReferences(httptest.NewRequest(http.MethodPost, "/images/app/push?"+tt.rawQuery, nil)) + switch { + case tt.wantDenyFor != "": + if refs.denyReason != tt.wantDenyFor { + t.Fatalf("denyReason = %q, want %q", refs.denyReason, tt.wantDenyFor) + } + if refs.imagePushTag != "" { + t.Fatalf("imagePushTag = %q, want empty on refusal", refs.imagePushTag) + } + default: + if refs.denyReason != "" { + t.Fatalf("denyReason = %q, want none", refs.denyReason) + } + if refs.imagePushTag != tt.wantTag { + t.Fatalf("imagePushTag = %q, want %q", refs.imagePushTag, tt.wantTag) + } + } + }) + } +} diff --git a/app/internal/ownership/middleware.go b/app/internal/ownership/middleware.go index b36b7cb3..76aaeac4 100644 --- a/app/internal/ownership/middleware.go +++ b/app/internal/ownership/middleware.go @@ -112,6 +112,13 @@ type ownershipRequestReferences struct { // in the access log under reasonCodeOwnerPolicyDeniedAccess, not the // unconditional 400 a mutation error produces. denyReason string + // imagePushTag carries the ?tag= value of POST /images/{name}/push, + // captured by the mutation pass for the authorization pass: the + // Docker-compatible push route splits its subject between the path + // (repository) and the query (tag), and only the qualified + // {name}:{tag} reference is the local image the daemon will push. See + // imagePushOwnershipReferences for the shapes that refuse instead. + imagePushTag string } // Options configures per-proxy resource ownership labeling and enforcement. @@ -337,6 +344,8 @@ func mutateOwnershipRequest(r *http.Request, normPath string, opts Options) (*ow return mutateServiceOwnershipBody(r, opts.LabelKey, opts.Owner) case r.Method == http.MethodPost && (isNodeUpdatePath(normPath) || isSwarmUpdatePath(normPath)): return nil, addOwnerLabelToBody(r, opts.LabelKey, opts.Owner) + case r.Method == http.MethodPost && isImagePushRoutePath(r.Method, normPath): + return imagePushOwnershipReferences(r), nil case r.Method == http.MethodPost && isCommitPath(normPath): return mutateCommitOwnershipRequest(r, opts) case r.Method == http.MethodPost && (normPath == "/build" || normPath == libpodPrefix+"build"): @@ -454,7 +463,7 @@ func allowOwnershipRequestUnprefixed( } } - verdict, reason, err := allowPathOwnershipRequest(ctx, method, normPath, routePath, opts, inspectResource, inspectExec) + verdict, reason, err := allowPathOwnershipRequest(ctx, method, normPath, routePath, opts, inspectResource, inspectExec, refs) if err != nil || verdict.denied() { return verdict, reason, err } @@ -472,6 +481,7 @@ func allowPathOwnershipRequest( opts Options, inspectResource func(context.Context, dockerresource.Kind, string) (map[string]string, bool, error), inspectExec func(context.Context, string) (string, bool, error), + refs *ownershipRequestReferences, ) (ownershipVerdict, string, error) { if reason, deny := imageEffectDenial(method, normPath); deny { return verdictDeny, reason, nil @@ -496,7 +506,7 @@ func allowPathOwnershipRequest( return checkOwnedResource(ctx, inspectResource, dockerresource.KindVolume, identifier, opts, false) } if identifier, ok := imageIdentifier(method, normPath); ok { - return checkOwnedResource(ctx, inspectResource, dockerresource.KindImage, identifier, opts, opts.AllowUnownedImages) + return checkOwnedResource(ctx, inspectResource, dockerresource.KindImage, appendImagePushTag(identifier, refs, method, normPath), opts, opts.AllowUnownedImages) } if identifier, ok := serviceIdentifier(method, normPath); ok { return checkOwnedResource(ctx, inspectResource, dockerresource.KindService, identifier, opts, false)