From b4fccce4d183b8b12d665ed88dc3ada3d8a1ee88 Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Wed, 2 Sep 2026 22:31:04 -0500 Subject: [PATCH] hold: actually send subject, so the attestation scan guard can fire The hold declines to enqueue a scan when a pushed manifest has a subject, which is how it means to skip attestations, signatures and other referrer artifacts. The AppView never sent one: notifyHoldAboutManifest built mediaType, config, layers and manifests, and #manifestInfo defined only those four. So the condition was always true, the guard never fired, and every referrer artifact was enqueued for scanning. The AppView already parsed subject. NewManifestRecord unmarshals it into ManifestRecord.Subject, and Put hands that same pointer to notifyHoldAboutManifest. The value was in scope and simply never serialized, so this is one missing marshal step rather than a missing parse. Adds subject to the lexicon as a #blobInfo ref, and mediaType to #blobInfo, which config has always sent and the hold has always parsed. That only makes the schema honest about what is already on the wire. Hoists the hold's anonymous request struct to a named type with IsMultiArch, IsReferrer and HasScannableContent, so the predicate is written once and testable without standing up a HoldPDS. Both directions degrade safely. An older hold ignores the unknown key and behaves exactly as today, so shipping the appview alone is harmless but achieves nothing until the hold catches up. An older appview sends no subject, leaving the manifest enqueued as before. Complements dfd604b rather than duplicating it. That guard lives in the scanner after a job is created and dispatched, and catches unscannable work from any source including the hold's proactive discovery pass. This one stops the row being created at all, which matters because the row that froze all scanning for nine days was exactly such an attestation. One gap neither closes: an attestation with tar-shaped layers pushed by an old appview. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9 --- lexicons/io/atcr/hold/notifyManifest.json | 9 ++ pkg/appview/storage/manifest_store.go | 11 ++ pkg/appview/storage/notify_subject_test.go | 127 +++++++++++++++++++++ pkg/hold/oci/notify_scan_guard_test.go | 86 ++++++++++++++ pkg/hold/oci/xrpc.go | 98 ++++++++++------ 5 files changed, 296 insertions(+), 35 deletions(-) create mode 100644 pkg/appview/storage/notify_subject_test.go create mode 100644 pkg/hold/oci/notify_scan_guard_test.go diff --git a/lexicons/io/atcr/hold/notifyManifest.json b/lexicons/io/atcr/hold/notifyManifest.json index 9d58ee6..2d54d3d 100644 --- a/lexicons/io/atcr/hold/notifyManifest.json +++ b/lexicons/io/atcr/hold/notifyManifest.json @@ -113,6 +113,11 @@ "type": "ref", "ref": "#childManifestInfo" } + }, + "subject": { + "type": "ref", + "ref": "#blobInfo", + "description": "Manifest this artifact refers to (attestations, signatures, SBOMs). Absent for ordinary images." } } }, @@ -125,6 +130,10 @@ }, "size": { "type": "integer" + }, + "mediaType": { + "type": "string", + "maxLength": 256 } } }, diff --git a/pkg/appview/storage/manifest_store.go b/pkg/appview/storage/manifest_store.go index 3507a08..e4ca525 100644 --- a/pkg/appview/storage/manifest_store.go +++ b/pkg/appview/storage/manifest_store.go @@ -678,6 +678,17 @@ func (s *ManifestStore) notifyHoldAboutManifest(ctx context.Context, manifestRec manifestData["manifests"] = manifests } + // Add subject if present. An OCI referrer artifact (attestation, + // signature, SBOM) points at the manifest it describes; the hold uses + // this to tell those apart from real images and skip scanning them. + if manifestRecord.Subject != nil { + manifestData["subject"] = map[string]any{ + "digest": manifestRecord.Subject.Digest, + "size": manifestRecord.Subject.Size, + "mediaType": manifestRecord.Subject.MediaType, + } + } + notifyReq["manifest"] = manifestData } diff --git a/pkg/appview/storage/notify_subject_test.go b/pkg/appview/storage/notify_subject_test.go new file mode 100644 index 0000000..3c6d86b --- /dev/null +++ b/pkg/appview/storage/notify_subject_test.go @@ -0,0 +1,127 @@ +package storage + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "testing" + + "atcr.io/pkg/atproto" +) + +// captureNotify stands in for the hold's notifyManifest endpoint and returns the +// decoded request body the AppView sent. +func captureNotify(t *testing.T, record *atproto.ManifestRecord) map[string]any { + t.Helper() + + var captured map[string]any + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + if err != nil { + t.Errorf("read body: %v", err) + } + if err := json.Unmarshal(body, &captured); err != nil { + t.Errorf("unmarshal body: %v", err) + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"success":true,"operation":"push","statsUpdated":true}`)) + })) + defer srv.Close() + + store := NewManifestStore(&RegistryContext{ + Repository: "myapp", + DID: "did:plc:alice123", + Handle: "alice.test", + HoldURL: srv.URL, + ServiceToken: "service-token", + }, nil) + + if err := store.notifyHoldAboutManifest(context.Background(), record, "latest", "sha256:deadbeef", "push"); err != nil { + t.Fatalf("notifyHoldAboutManifest: %v", err) + } + if captured == nil { + t.Fatal("hold received no notification") + } + return captured +} + +func manifestInfoFrom(t *testing.T, payload map[string]any) map[string]any { + t.Helper() + info, ok := payload["manifest"].(map[string]any) + if !ok { + t.Fatalf("payload has no manifest object: %#v", payload) + } + return info +} + +// TestNotifyHold_IncludesSubjectForReferrer verifies the notify payload carries +// the subject descriptor when the pushed manifest is a referrer artifact. Without +// it the hold's attestation guard (which tests for a nil subject) can never fire. +func TestNotifyHold_IncludesSubjectForReferrer(t *testing.T) { + // An in-toto attestation as buildx pushes one: an ordinary image config, + // non-tar layers, and a subject pointing at the image it describes. + ociManifest := []byte(`{ + "schemaVersion": 2, + "mediaType": "application/vnd.oci.image.manifest.v1+json", + "config": {"mediaType":"application/vnd.oci.image.config.v1+json","digest":"sha256:cfg","size":233}, + "layers": [ + {"mediaType":"application/vnd.in-toto+json","digest":"sha256:att","size":1024} + ], + "subject": {"mediaType":"application/vnd.oci.image.manifest.v1+json","digest":"sha256:subj","size":528} + }`) + + record, err := atproto.NewManifestRecord("myapp", "sha256:deadbeef", ociManifest) + if err != nil { + t.Fatalf("NewManifestRecord: %v", err) + } + if record.Subject == nil { + t.Fatal("AppView did not parse subject off the pushed manifest") + } + + info := manifestInfoFrom(t, captureNotify(t, record)) + + subject, ok := info["subject"].(map[string]any) + if !ok { + t.Fatalf("notify payload omitted subject: %#v", info) + } + if subject["digest"] != "sha256:subj" { + t.Errorf("subject digest = %v, want sha256:subj", subject["digest"]) + } + if subject["mediaType"] != "application/vnd.oci.image.manifest.v1+json" { + t.Errorf("subject mediaType = %v", subject["mediaType"]) + } + if size, ok := subject["size"].(float64); !ok || int64(size) != 528 { + t.Errorf("subject size = %v, want 528", subject["size"]) + } +} + +// TestNotifyHold_OmitsSubjectForPlainImage verifies an ordinary image still +// sends no subject, so the hold keeps enqueueing real images for scanning. +func TestNotifyHold_OmitsSubjectForPlainImage(t *testing.T) { + ociManifest := []byte(`{ + "schemaVersion": 2, + "mediaType": "application/vnd.oci.image.manifest.v1+json", + "config": {"mediaType":"application/vnd.oci.image.config.v1+json","digest":"sha256:cfg","size":233}, + "layers": [ + {"mediaType":"application/vnd.oci.image.layer.v1.tar+gzip","digest":"sha256:l1","size":4096} + ] + }`) + + record, err := atproto.NewManifestRecord("myapp", "sha256:deadbeef", ociManifest) + if err != nil { + t.Fatalf("NewManifestRecord: %v", err) + } + + info := manifestInfoFrom(t, captureNotify(t, record)) + + if _, present := info["subject"]; present { + t.Errorf("notify payload carried a subject for a plain image: %#v", info["subject"]) + } + // The rest of the payload is unchanged. + layers, ok := info["layers"].([]any) + if !ok || len(layers) != 1 { + t.Errorf("layers = %#v, want one layer", info["layers"]) + } +} diff --git a/pkg/hold/oci/notify_scan_guard_test.go b/pkg/hold/oci/notify_scan_guard_test.go new file mode 100644 index 0000000..b143621 --- /dev/null +++ b/pkg/hold/oci/notify_scan_guard_test.go @@ -0,0 +1,86 @@ +package oci + +import ( + "encoding/json" + "testing" +) + +// TestNotifyManifestInfo_ScanGuard checks the decision at the scan-enqueue guard +// in HandleNotifyManifest, driven off the JSON the AppView actually sends. +func TestNotifyManifestInfo_ScanGuard(t *testing.T) { + tests := []struct { + name string + manifestJSON string + wantScannable bool + }{ + { + name: "plain image is enqueued", + manifestJSON: `{ + "mediaType": "application/vnd.oci.image.manifest.v1+json", + "config": {"digest":"sha256:cfg","size":233,"mediaType":"application/vnd.oci.image.config.v1+json"}, + "layers": [{"digest":"sha256:l1","size":4096,"mediaType":"application/vnd.oci.image.layer.v1.tar+gzip"}] + }`, + wantScannable: true, + }, + { + name: "attestation with subject is skipped", + manifestJSON: `{ + "mediaType": "application/vnd.oci.image.manifest.v1+json", + "config": {"digest":"sha256:cfg","size":233,"mediaType":"application/vnd.oci.image.config.v1+json"}, + "layers": [{"digest":"sha256:att","size":1024,"mediaType":"application/vnd.in-toto+json"}], + "subject": {"digest":"sha256:subj","size":528,"mediaType":"application/vnd.oci.image.manifest.v1+json"} + }`, + wantScannable: false, + }, + { + name: "manifest list is skipped", + manifestJSON: `{ + "mediaType": "application/vnd.oci.image.index.v1+json", + "manifests": [{"digest":"sha256:m1","size":528,"mediaType":"application/vnd.oci.image.manifest.v1+json","platform":{"os":"linux","architecture":"amd64"}}] + }`, + wantScannable: false, + }, + { + // An AppView older than the subject field sends nothing, and the + // hold must behave exactly as it did before: enqueue. + name: "payload from an older AppView still enqueues", + manifestJSON: `{ + "mediaType": "application/vnd.oci.image.manifest.v1+json", + "config": {"digest":"sha256:cfg","size":233}, + "layers": [{"digest":"sha256:l1","size":4096,"mediaType":"application/vnd.oci.image.layer.v1.tar+gzip"}] + }`, + wantScannable: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var info notifyManifestInfo + if err := json.Unmarshal([]byte(tt.manifestJSON), &info); err != nil { + t.Fatalf("unmarshal manifest info: %v", err) + } + if got := info.HasScannableContent(); got != tt.wantScannable { + t.Errorf("HasScannableContent() = %v, want %v", got, tt.wantScannable) + } + }) + } +} + +// TestNotifyManifestInfo_SubjectDecoded confirms the subject descriptor survives +// the wire, not just its presence. +func TestNotifyManifestInfo_SubjectDecoded(t *testing.T) { + var info notifyManifestInfo + body := `{"subject":{"digest":"sha256:subj","size":528,"mediaType":"application/vnd.oci.image.manifest.v1+json"}}` + if err := json.Unmarshal([]byte(body), &info); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if !info.IsReferrer() { + t.Fatal("IsReferrer() = false, want true") + } + if info.Subject.Digest != "sha256:subj" || info.Subject.Size != 528 { + t.Errorf("subject = %+v", info.Subject) + } + if info.Subject.MediaType != "application/vnd.oci.image.manifest.v1+json" { + t.Errorf("subject mediaType = %q", info.Subject.MediaType) + } +} diff --git a/pkg/hold/oci/xrpc.go b/pkg/hold/oci/xrpc.go index f4056fb..b01b36a 100644 --- a/pkg/hold/oci/xrpc.go +++ b/pkg/hold/oci/xrpc.go @@ -186,6 +186,59 @@ func (h *XRPCHandler) HandleAbortUpload(w http.ResponseWriter, r *http.Request) }) } +// notifyManifestInfo mirrors io.atcr.hold.notifyManifest#manifestInfo: the +// slice of the pushed OCI manifest the AppView forwards to the hold. +type notifyManifestInfo struct { + MediaType string `json:"mediaType"` + Config struct { + Digest string `json:"digest"` + Size int64 `json:"size"` + MediaType string `json:"mediaType"` + } `json:"config"` + Layers []struct { + Digest string `json:"digest"` + Size int64 `json:"size"` + MediaType string `json:"mediaType"` + } `json:"layers"` + Manifests []struct { + Digest string `json:"digest"` + Size int64 `json:"size"` + MediaType string `json:"mediaType"` + Platform *struct { + OS string `json:"os"` + Architecture string `json:"architecture"` + } `json:"platform"` + } `json:"manifests"` + // Subject is the descriptor an OCI referrer artifact points at. Only an + // AppView new enough to send it populates this; an older one leaves it nil, + // which reads as "ordinary image" exactly as it did before the field existed. + Subject *struct { + Digest string `json:"digest"` + Size int64 `json:"size"` + MediaType string `json:"mediaType"` + } `json:"subject"` +} + +// IsMultiArch reports whether this is a manifest list / image index, which has +// child manifests instead of layers of its own. +func (m *notifyManifestInfo) IsMultiArch() bool { + return len(m.Manifests) > 0 +} + +// IsReferrer reports whether this manifest is an OCI referrer artifact +// (attestation, signature, SBOM) attached to another manifest via subject, +// rather than an image in its own right. +func (m *notifyManifestInfo) IsReferrer() bool { + return m.Subject != nil +} + +// HasScannableContent reports whether a vulnerability scan of this manifest +// could find anything. Manifest lists carry no layers, and referrer artifacts +// carry metadata rather than a filesystem, so neither is worth enqueueing. +func (m *notifyManifestInfo) HasScannableContent() bool { + return !m.IsMultiArch() && !m.IsReferrer() +} + // HandleNotifyManifest handles manifest notifications from AppView // For pushes: Creates layer records and optionally posts to Bluesky // For pulls: Just increments stats (no layer records or posts) @@ -211,38 +264,12 @@ func (h *XRPCHandler) HandleNotifyManifest(w http.ResponseWriter, r *http.Reques // Parse request var req struct { - Repository string `json:"repository"` - Tag string `json:"tag"` - UserDID string `json:"userDid"` - ManifestDigest string `json:"manifestDigest"` // For building layer record AT-URIs - Operation string `json:"operation"` // "push" or "pull", defaults to "push" for backward compatibility - Manifest struct { - MediaType string `json:"mediaType"` - Config struct { - Digest string `json:"digest"` - Size int64 `json:"size"` - MediaType string `json:"mediaType"` - } `json:"config"` - Layers []struct { - Digest string `json:"digest"` - Size int64 `json:"size"` - MediaType string `json:"mediaType"` - } `json:"layers"` - Manifests []struct { - Digest string `json:"digest"` - Size int64 `json:"size"` - MediaType string `json:"mediaType"` - Platform *struct { - OS string `json:"os"` - Architecture string `json:"architecture"` - } `json:"platform"` - } `json:"manifests"` - Subject *struct { - Digest string `json:"digest"` - Size int64 `json:"size"` - MediaType string `json:"mediaType"` - } `json:"subject"` - } `json:"manifest"` + Repository string `json:"repository"` + Tag string `json:"tag"` + UserDID string `json:"userDid"` + ManifestDigest string `json:"manifestDigest"` // For building layer record AT-URIs + Operation string `json:"operation"` // "push" or "pull", defaults to "push" for backward compatibility + Manifest notifyManifestInfo `json:"manifest"` } if err := render.Decode(r, &req); err != nil { @@ -341,7 +368,7 @@ func (h *XRPCHandler) HandleNotifyManifest(w http.ResponseWriter, r *http.Reques } // Check if this is a multi-arch image (has manifests instead of layers) - isMultiArch := len(req.Manifest.Manifests) > 0 + isMultiArch := req.Manifest.IsMultiArch() // Calculate total size from all layers (for single-arch images) var totalSize int64 @@ -404,8 +431,9 @@ func (h *XRPCHandler) HandleNotifyManifest(w http.ResponseWriter, r *http.Reques } } - // Enqueue scan job if scanner is connected (skip manifest lists and attestations — no scannable content) - if h.scanBroadcaster != nil && !isMultiArch && req.Manifest.Subject == nil { + // Enqueue scan job if scanner is connected (skip manifest lists and + // referrer artifacts such as attestations — no scannable content). + if h.scanBroadcaster != nil && req.Manifest.HasScannableContent() { tier := "deckhand" if stats != nil && stats.Tier != "" { tier = stats.Tier