auth: make anonymous pull work, and let the hold decide it

a7569a7 added credential-less pulls of public images. Three things about it
were wrong, all of them in how the appview handled the decision that belongs
to the hold.

**Scope handling was all-or-nothing.** IsPullOnlyScope required every
requested action to already be "pull", but clients routinely ask for more
than the operation needs — pull,push is common for a plain read, and some
ask for pull,push,delete up front. Those were rejected and challenged,
leaving a credential-less client no way to pull even a public image, which
is the entire feature. NarrowToPullOnly drops the write actions and issues a
token carrying "pull" and nothing else. Granting a subset is what the
distribution token spec expects. The allowlist property is preserved: "pull"
is the only action that survives, and "*" is deliberately not expanded into
it, since a wildcard request is not evidence the caller wants a read.

**The appview-side read gate was inert.** checkReadAccess passed p.ctx.DID,
the DID of the repository *owner*, not the requester. Any non-empty DID
satisfies a private hold's check, and the owner's is never empty, so it asked
"may the owner read their own hold", answered yes, and admitted everyone.
Worse, CheckReadAccessWithCaptain admitted any authenticated DID to a private
hold at all, on an explicitly-MVP assumption that holding a DID was close
enough to being a sailor. Every doc says otherwise (docs/hold.md:109 "Crew
with blob:read", CLAUDE.md:140, docs/BYOS.md:280) and so does the hold
(ValidateBlobReadAccess: owner, or crew carrying blob:read/blob:write). It
now takes isCrew and requires owner-or-crew, and callers only pay for the
crew lookup when it can change the answer — a public hold or an anonymous
caller is decided by the captain record alone. Nothing here loosens access;
it brings the local gate into agreement with the authority.

**Denials could not reach the client.** distribution's blobHandler.GetBlob
maps everything except ErrBlobUnknown to ErrorCodeUnknown, so a 401 raised
in the blob store left as a 500 — misreporting an auth failure as a server
fault, and giving BearerChallenge no 401 to attach WWW-Authenticate to, so
Docker was told "server error" instead of being prompted for credentials.
Clients that retry 5xx looped: 4.1s per case in the matrix, now 0.01s. The
check moves to Repository(), where an errcode.Error is passed through
verbatim by the registry app — the same mechanism a7569a7 used for
NAME_UNKNOWN. It fails open on a lookup error, since the hold is the
authority and a transient failure should not break public pulls.

Removes auth.allow_anonymous_pull. It could only ever withhold — captain.Public
is what grants — so it was a second flag for a decision the hold already owns,
and gating it appview-side was never the intent. Layer bytes 307 straight to
S3, so the appview is not even in the path whose cost might have justified an
operator-side lever.

Tests: TestAuthMatrix only ever ran against a public hold, and its pull cases
never fetched a layer — crane.Pull is lazy and img.Digest() needs only the
manifest, which ATCR serves from the user's PDS where it is world-readable, so
no pull row in the matrix touched blob authorization at all. Pulls now
materialize layer bytes, and testharness.WithPrivateHold plus
TestAuthMatrixPrivateHold cover public:false + allow_all_crew:true — the
production shape, where anyone with an account pulls and pushes and anonymous
gets nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

config-appview.example.yaml

@@ -76,8 +76,6 @@ jetstream:
 auth:
     # X.509 certificate matching the JWT signing key (auto-generated on each boot from the JWT key in the database).
     cert_path: /var/lib/atcr/auth/private-key.crt
-    # Allow unauthenticated Docker pulls from public holds. Per-hold privacy (captain.Public) still applies. Default true.
-    allow_anonymous_pull: true
 # Credential helper download settings.
 credential_helper:
     # Tangled repository URL for credential helper downloads.

deploy/upcloud/configs/appview.yaml.tmpl

@@ -41,9 +41,6 @@ jetstream:
         - https://relay1.us-west.bsky.network
 auth:
     cert_path: "{{.BasePath}}/auth/private-key.crt"
-    # Allow unauthenticated Docker pulls from public holds. Per-hold privacy
-    # (captain.Public) still applies, and push always requires credentials.
-    allow_anonymous_pull: true
 legal:
     company_name: Seamark
     jurisdiction: State of Texas, United States

internal/testharness/harness.go

@@ -40,8 +40,19 @@ import (
 type Option func(*options)
 
 type options struct {
-	quota   *quota.Config
-	billing *billing.Config
+	quota       *quota.Config
+	billing     *billing.Config
+	privateHold bool
+}
+
+// WithPrivateHold builds the hold with captain.Public = false. Reads then
+// require the owner or a crew member, so anonymous pulls are refused and a
+// PDS-known stranger is refused too — the mirror of the default public hold,
+// where anyone may pull and only crew may push.
+func WithPrivateHold() Option {
+	return func(o *options) {
+		o.privateHold = true
+	}
 }
 
 // WithQuotaTiers configures the hold's quota manager with the given tier
@@ -170,7 +181,7 @@ func New(t *testing.T, opts ...Option) *Harness {
 		Server: hold.ServerConfig{
 			Addr:         holdAddr,
 			PublicURL:    holdPublicURL,
-			Public:       true, // anonymous pulls allowed; pushes still need crew
+			Public:       !o.privateHold, // public: anyone may pull, crew may push. private: crew only, both ways.
 			TestMode:     true,
 			ReadTimeout:  60 * time.Second,
 			WriteTimeout: 5 * time.Minute,
@@ -275,7 +286,7 @@ func New(t *testing.T, opts ...Option) *Harness {
 	// In production these are populated by the Jetstream worker consuming
 	// hold firehose events; tests disable Jetstream and seed directly so
 	// authorization decisions resolve against known state.
-	h.seedCaptainRecord(captainIdent)
+	h.seedCaptainRecord(captainIdent, !o.privateHold)
 	h.seedUserRow(captainIdent)
 	h.seedCrewMember(captainIdent, []string{"blob:read", "blob:write", "crew:admin"})
 
@@ -330,11 +341,16 @@ func (h *Harness) AnonAuth() authn.Authenticator {
 
 // seedCaptainRecord writes a row to hold_captain_records so the auth gate's
 // isCaptain check returns true for the captain.
-func (h *Harness) seedCaptainRecord(captain *testpds.Identity) {
+//
+// public must match the hold's own captain.Public. This row is what the
+// appview's hold authorizer reads, so seeding it true against a private hold
+// would let the appview admit reads the hold then refuses, which is the
+// disagreement these tests exist to catch.
+func (h *Harness) seedCaptainRecord(captain *testpds.Identity, public bool) {
 	h.t.Helper()
 	_, err := h.AppView.Database.Exec(
 		`INSERT INTO hold_captain_records (hold_did, owner_did, public, allow_all_crew) VALUES (?, ?, ?, ?)`,
-		h.HoldDID, captain.DID.String(), true, true,
+		h.HoldDID, captain.DID.String(), public, true,
 	)
 	if err != nil {
 		h.t.Fatalf("seed captain record: %v", err)

pkg/appview/config.go

@@ -126,10 +126,6 @@ type AuthConfig struct {
 	// X.509 certificate matching the JWT signing key.
 	CertPath string `yaml:"cert_path" comment:"X.509 certificate matching the JWT signing key (auto-generated on each boot from the JWT key in the database)."`
 
-	// AllowAnonymousPull permits credential-less Docker pulls. Per-hold privacy
-	// (captain.Public) still applies — a private hold rejects anonymous reads.
-	AllowAnonymousPull bool `yaml:"allow_anonymous_pull" comment:"Allow unauthenticated Docker pulls from public holds. Per-hold privacy (captain.Public) still applies. Default true."`
-
 	// TokenExpiration is the JWT expiration duration (5 minutes, not configurable)
 	TokenExpiration time.Duration `yaml:"-"`
 
@@ -239,7 +235,6 @@ func setDefaults(v *viper.Viper) {
 
 	// Auth defaults
 	v.SetDefault("auth.cert_path", "/var/lib/atcr/auth/private-key.crt")
-	v.SetDefault("auth.allow_anonymous_pull", true)
 
 	// Log shipper defaults
 	v.SetDefault("log_shipper.batch_size", 100)

pkg/appview/middleware/registry.go

@@ -516,6 +516,34 @@ func (nr *NamespaceResolver) Repository(ctx context.Context, name reference.Name
 		authMethod = token.AuthMethodOAuth
 	}
 
+	// Anonymous request to a private hold: refuse here rather than deeper in
+	// the stack.
+	//
+	// captain.Public is the only thing that admits a reader with no identity.
+	// The hold enforces that too, but a denial raised from the blob store
+	// cannot reach the client intact: distribution's blobHandler.GetBlob maps
+	// everything except ErrBlobUnknown to ErrorCodeUnknown, so a 401 leaves
+	// here as a 500 — misreporting an auth failure as a server fault and
+	// sending clients that retry 5xx into a loop. An errcode.Error returned
+	// from Repository() is passed through verbatim by the registry app, so the
+	// client gets a real 401 and BearerChallenge can attach WWW-Authenticate,
+	// which is what makes Docker prompt for credentials.
+	//
+	// Fail open on a lookup error: the hold is the enforcing authority, and a
+	// transient failure here should not break anonymous pulls of public
+	// images.
+	if pullerDID == "" && nr.authorizer != nil && holdDID != "" {
+		allowed, authErr := nr.authorizer.CheckReadAccess(ctx, holdDID, "")
+		if authErr != nil {
+			slog.Warn("Anonymous read check failed, deferring to hold",
+				"holdDID", holdDID, "repository", repositoryName, "error", authErr)
+		} else if !allowed {
+			slog.Debug("Anonymous read denied: hold is not public",
+				"holdDID", holdDID, "repository", repositoryName)
+			return nil, errcode.ErrorCodeUnauthorized.WithMessage("authentication required")
+		}
+	}
+
 	// Create routing repository - routes manifests to ATProto, blobs to hold service
 	// The registry is stateless - no local storage is used
 	// Bundle all context into a single RegistryContext struct

pkg/appview/server.go

@@ -593,11 +593,6 @@ func NewAppViewServer(cfg *Config, branding *BrandingOverrides) (*AppViewServer,
 		// the audience names the front door actually used.
 		tokenHandler.SetServices(cfg.Auth.Services)
 
-		// Anonymous pull: when enabled, credential-less pull-only requests get a
-		// token with an empty subject; the destination hold still enforces
-		// captain.Public. Push always requires credentials.
-		tokenHandler.SetAllowAnonymousPull(cfg.Auth.AllowAnonymousPull)
-
 		tokenHandler.SetOAuthSessionValidator(s.Refresher)
 
 		// Auth-phase gate: crew reconciliation for any token request, plus

pkg/appview/storage/proxy_blob_store.go

@@ -82,7 +82,18 @@ func (p *ProxyBlobStore) checkReadAccess(ctx context.Context) error {
 	if p.ctx.Authorizer == nil {
 		return nil // No authorization check if authorizer not configured
 	}
-	allowed, err := p.ctx.Authorizer.CheckReadAccess(ctx, p.ctx.HoldDID, p.ctx.DID)
+	// Authorize the *requester*, not the repository owner. p.ctx.DID is the
+	// owner whose namespace is being read; passing it here asked "may the owner
+	// read their own hold", which is true for every private hold (any non-empty
+	// DID satisfies CheckReadAccessWithCaptain), so an anonymous request sailed
+	// through this gate and the Anonymous branch below was unreachable. An
+	// anonymous request has no identity, so it must be judged as one: only
+	// captain.Public can admit it.
+	requesterDID := p.ctx.DID
+	if p.ctx.Anonymous {
+		requesterDID = ""
+	}
+	allowed, err := p.ctx.Authorizer.CheckReadAccess(ctx, p.ctx.HoldDID, requesterDID)
 	if err != nil {
 		return fmt.Errorf("authorization check failed: %w", err)
 	}

pkg/auth/hold_authorizer.go

@@ -46,22 +46,50 @@ type HoldAuthorizer interface {
 // This is shared across all HoldAuthorizer implementations
 // Read access rules:
 // - Public hold: allow anyone (even anonymous)
-// - Private hold: require authentication (any authenticated user)
-func CheckReadAccessWithCaptain(captain *atproto.CaptainRecord, userDID string) bool {
+// - Private hold: hold owner or crew member only
+//
+// The two settings on a captain record are orthogonal and this is the read
+// half: public decides who may pull (anyone, or crew only), while
+// allowAllCrew decides who may become crew and therefore who may push. An
+// anonymous reader has no identity to be crew with, so public is the only
+// thing that can admit one.
+//
+// This previously admitted any authenticated DID to a private hold, on an
+// explicitly-MVP assumption that holding a DID was close enough to being a
+// sailor. It is not: "private" means crew-only, and every authenticated user
+// on the network has a DID. The hold has always enforced the correct rule
+// (ValidateBlobReadAccess: owner, or crew carrying blob:read/blob:write), so
+// this brings the appview's local gate into agreement with the authority
+// rather than loosening anything.
+func CheckReadAccessWithCaptain(captain *atproto.CaptainRecord, userDID string, isCrew bool) bool {
 	if captain.Public {
 		// Public hold - allow anyone (even anonymous)
 		return true
 	}
 
 	// Private hold - require authentication
-	// Any authenticated user with a DID can read
 	if userDID == "" {
 		// Anonymous user trying to access private hold
+		slog.Debug("Read access denied",
+			"denial_reason", "anonymous_on_private_hold",
+			"message", "anonymous reads require a public hold")
+		return false
+	}
+
+	// Owner always has read access to their own hold
+	if userDID == captain.Owner {
+		return true
+	}
+
+	if !isCrew {
+		slog.Debug("Read access denied",
+			"userDID", userDID,
+			"owner", captain.Owner,
+			"denial_reason", "not_owner_or_crew",
+			"message", "private hold reads require crew membership")
 		return false
 	}
 
-	// For MVP: assume DID presence means they have sailor.profile
-	// Future: could query PDS to verify sailor.profile exists
 	return true
 }
 

pkg/auth/hold_authorizer_test.go

@@ -13,13 +13,13 @@ func TestCheckReadAccessWithCaptain_PublicHold(t *testing.T) {
 	}
 
 	// Public hold - anonymous user should be allowed
-	allowed := CheckReadAccessWithCaptain(captain, "")
+	allowed := CheckReadAccessWithCaptain(captain, "", false)
 	if !allowed {
 		t.Error("Expected anonymous user to have read access to public hold")
 	}
 
-	// Public hold - authenticated user should be allowed
-	allowed = CheckReadAccessWithCaptain(captain, "did:plc:user123")
+	// Public hold - authenticated non-crew user should be allowed
+	allowed = CheckReadAccessWithCaptain(captain, "did:plc:user123", false)
 	if !allowed {
 		t.Error("Expected authenticated user to have read access to public hold")
 	}
@@ -32,15 +32,29 @@ func TestCheckReadAccessWithCaptain_PrivateHold(t *testing.T) {
 	}
 
 	// Private hold - anonymous user should be denied
-	allowed := CheckReadAccessWithCaptain(captain, "")
+	allowed := CheckReadAccessWithCaptain(captain, "", false)
 	if allowed {
 		t.Error("Expected anonymous user to be denied read access to private hold")
 	}
 
-	// Private hold - authenticated user should be allowed
-	allowed = CheckReadAccessWithCaptain(captain, "did:plc:user123")
+	// Private hold - an authenticated stranger is NOT enough. "Private" means
+	// crew-only; every user on the network has a DID, so admitting any DID
+	// would make private holds world-readable to signed-in users.
+	allowed = CheckReadAccessWithCaptain(captain, "did:plc:user123", false)
+	if allowed {
+		t.Error("Expected authenticated non-crew user to be denied read access to private hold")
+	}
+
+	// Private hold - crew member should be allowed
+	allowed = CheckReadAccessWithCaptain(captain, "did:plc:user123", true)
 	if !allowed {
-		t.Error("Expected authenticated user to have read access to private hold")
+		t.Error("Expected crew member to have read access to private hold")
+	}
+
+	// Private hold - owner should be allowed without being listed as crew
+	allowed = CheckReadAccessWithCaptain(captain, "did:plc:owner123", false)
+	if !allowed {
+		t.Error("Expected hold owner to have read access to their own private hold")
 	}
 }
 

pkg/auth/hold_remote.go

@@ -426,7 +426,19 @@ func (a *RemoteHoldAuthorizer) CheckReadAccess(ctx context.Context, holdDID, use
 		return false, err
 	}
 
-	return CheckReadAccessWithCaptain(captain, userDID), nil
+	// Only a private hold needs the crew lookup, and only for a caller who
+	// could be crew. Public holds and anonymous callers are decided by the
+	// captain record alone, which keeps the common pull path free of the
+	// crew query (a cached XRPC round trip on this implementation).
+	isCrew := false
+	if !captain.Public && userDID != "" && userDID != captain.Owner {
+		isCrew, err = a.IsCrewMember(ctx, holdDID, userDID)
+		if err != nil {
+			return false, err
+		}
+	}
+
+	return CheckReadAccessWithCaptain(captain, userDID, isCrew), nil
 }
 
 // CheckWriteAccess implements write authorization using shared logic

pkg/auth/holdlocal/holdlocal.go

@@ -79,7 +79,18 @@ func (a *Authorizer) CheckReadAccess(ctx context.Context, holdDID, userDID strin
 		return false, err
 	}
 
-	return auth.CheckReadAccessWithCaptain(captain, userDID), nil
+	// Only a private hold needs the crew lookup, and only for a caller who
+	// could be crew: public holds and anonymous callers are decided by the
+	// captain record alone.
+	isCrew := false
+	if !captain.Public && userDID != "" && userDID != captain.Owner {
+		isCrew, err = a.IsCrewMember(ctx, holdDID, userDID)
+		if err != nil {
+			return false, err
+		}
+	}
+
+	return auth.CheckReadAccessWithCaptain(captain, userDID, isCrew), nil
 }
 
 // CheckWriteAccess implements write authorization using shared logic.

pkg/auth/token/claims.go

@@ -113,6 +113,50 @@ func IsPullOnlyScope(access []auth.AccessEntry) bool {
 	return true
 }
 
+// NarrowToPullOnly returns a copy of access holding only the "pull" action, and
+// reports whether the result is worth issuing a token for.
+//
+// Clients routinely request more than they need for the operation in hand —
+// `repository:x:pull,push` for a plain pull is common, and some request
+// pull,push,delete up front — so an all-or-nothing IsPullOnlyScope test rejects
+// the request, challenges, and leaves a credential-less client with no way
+// forward even for a public image. Granting a subset is the behavior the
+// distribution token spec expects: the server issues what it is willing to
+// authorize and the client proceeds with that.
+//
+// The allowlist property from IsPullOnlyScope is preserved exactly: "pull" is
+// the only action that survives, so nothing here can emit a token carrying
+// push, delete, or "*". An entry left with no actions is dropped rather than
+// emitted empty, since distribution treats an empty action set as granting
+// nothing and it only adds noise to the token.
+//
+// "*" is deliberately NOT expanded into "pull". Rewriting it would be safe in
+// the narrow sense (the issued token would name only "pull"), but a wildcard
+// request is not evidence the caller wants a read, and refusing it keeps the
+// anonymous path free of any case where a wildcard turns into a grant.
+func NarrowToPullOnly(access []auth.AccessEntry) ([]auth.AccessEntry, bool) {
+	narrowed := make([]auth.AccessEntry, 0, len(access))
+	grantable := false
+
+	for _, entry := range access {
+		if len(entry.Actions) == 0 {
+			// No actions requested (the /v2/ ping shape). Preserve it as-is:
+			// it grants nothing and callers rely on the entry surviving.
+			narrowed = append(narrowed, entry)
+			continue
+		}
+		if !slices.Contains(entry.Actions, "pull") {
+			continue
+		}
+		pullOnly := entry
+		pullOnly.Actions = []string{"pull"}
+		narrowed = append(narrowed, pullOnly)
+		grantable = true
+	}
+
+	return narrowed, grantable || len(access) == 0
+}
+
 // ExtractSubject parses a JWT token string and extracts the Subject claim (the user's DID)
 // Returns the subject or empty string if not found or token is invalid
 // This does NOT validate the token - it only parses it to extract the claim

pkg/auth/token/handler.go

@@ -78,7 +78,6 @@ type Handler struct {
 	oauthSessionValidator OAuthSessionValidator
 	authorizer            Authorizer
 	serviceAuthFetcher    ServiceAuthFetcher
-	allowAnonymousPull    bool // issue credential-less tokens for pull-only scopes
 
 	// services is the set of registry domains this AppView fronts, keyed by
 	// normalized hostname. Nil means single-domain: the lookups in
@@ -89,10 +88,9 @@ type Handler struct {
 // NewHandler creates a new token handler
 func NewHandler(issuer *Issuer, deviceStore *db.DeviceStore) *Handler {
 	return &Handler{
-		issuer:             issuer,
-		validator:          auth.NewSessionValidator(),
-		deviceStore:        deviceStore,
-		allowAnonymousPull: true,
+		issuer:      issuer,
+		validator:   auth.NewSessionValidator(),
+		deviceStore: deviceStore,
 	}
 }
 
@@ -123,14 +121,6 @@ func (h *Handler) SetServiceAuthFetcher(fetcher ServiceAuthFetcher) {
 	h.serviceAuthFetcher = fetcher
 }
 
-// SetAllowAnonymousPull toggles credential-less token issuance for pull-only
-// scopes. When false, requests without credentials are always challenged, so
-// the handler behaves exactly as it did before anonymous pull existed. Per-hold
-// privacy (captain.Public) still applies regardless of this setting.
-func (h *Handler) SetAllowAnonymousPull(allow bool) {
-	h.allowAnonymousPull = allow
-}
-
 // SetServices declares the registry domains this AppView fronts, e.g.
 // ["buoy.cr", "seamark.cr", "atcr.io"]. Each issued JWT is stamped with
 // whichever of these the client is authenticating against, so the audience
@@ -346,13 +336,22 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 	// No credentials. Only reachable on the GET form — the POST form rejects an
 	// empty username above, and a client that lands there without one is sent to
-	// the GET form by that 401. Issue an anonymous, pull-only token if enabled
-	// and the requested scope carries no write actions; the destination hold then
-	// decides whether anonymous reads are allowed (captain.Public). Anything
-	// requesting push/delete still gets the standard auth challenge.
+	// the GET form by that 401.
+	//
+	// Narrow the request to its pull component rather than demanding it already
+	// be pull-only: clients commonly ask for pull,push (or pull,push,delete) for
+	// an operation that only reads, and rejecting those outright makes anonymous
+	// pull unreachable for them. What comes back carries "pull" and nothing else,
+	// so a push or delete still requires credentials. A scope with no pull
+	// component at all gets the standard challenge.
+	//
+	// Whether an anonymous reader is actually admitted is not decided here. The
+	// hold owns that call via captain.Public, and the appview enforces the same
+	// answer from its local captain cache before serving anything. Minting a
+	// pull-only token is not a grant.
 	if username == "" {
-		if h.allowAnonymousPull && IsPullOnlyScope(access) {
-			h.issueAnonymousToken(w, r, access, service)
+		if pullAccess, ok := NarrowToPullOnly(access); ok {
+			h.issueAnonymousToken(w, r, pullAccess, service)
 			return
 		}
 		slog.Debug("No Basic auth credentials provided")

pkg/auth/token/handler_test.go

@@ -132,33 +132,6 @@ func TestHandler_SetPostAuthCallback(t *testing.T) {
 	}
 }
 
-func TestHandler_ServeHTTP_NoAuth(t *testing.T) {
-	keyPath := getSharedTestKey(t)
-
-	issuer, err := NewIssuer(keyPath, "atcr.io", "registry", 15*time.Minute)
-	if err != nil {
-		t.Fatalf("NewIssuer() error = %v", err)
-	}
-
-	handler := NewHandler(issuer, nil)
-	// With anonymous pull disabled, a credential-less request is challenged.
-	handler.SetAllowAnonymousPull(false)
-
-	req := httptest.NewRequest(http.MethodGet, "/auth/token?service=registry", nil)
-	w := httptest.NewRecorder()
-
-	handler.ServeHTTP(w, req)
-
-	if w.Code != http.StatusUnauthorized {
-		t.Errorf("Expected status %d, got %d", http.StatusUnauthorized, w.Code)
-	}
-
-	// Check for WWW-Authenticate header
-	if w.Header().Get("WWW-Authenticate") == "" {
-		t.Error("Expected WWW-Authenticate header")
-	}
-}
-
 func TestHandler_ServeHTTP_AnonymousPull(t *testing.T) {
 	keyPath := getSharedTestKey(t)
 
@@ -243,7 +216,10 @@ func TestHandler_ServeHTTP_AnonymousPushChallenged(t *testing.T) {
 
 	handler := NewHandler(issuer, nil)
 
-	for _, action := range []string{"push", "delete", "pull,push"} {
+	// A scope with no pull component is nothing an anonymous caller can be
+	// granted, so it still draws the standard challenge. "*" is included
+	// deliberately: it is not treated as a request to read.
+	for _, action := range []string{"push", "delete", "push,delete", "*"} {
 		t.Run(action, func(t *testing.T) {
 			req := httptest.NewRequest(http.MethodGet, "/auth/token?service=registry&scope=repository:bob.bsky.social/myapp:"+action, nil)
 			w := httptest.NewRecorder()
@@ -260,7 +236,12 @@ func TestHandler_ServeHTTP_AnonymousPushChallenged(t *testing.T) {
 	}
 }
 
-func TestHandler_ServeHTTP_AnonymousDisabledChallengesPull(t *testing.T) {
+// TestHandler_ServeHTTP_AnonymousMixedScopeNarrowedToPull pins the behavior
+// clients actually depend on: many request pull,push (or pull,push,delete) for
+// an operation that only reads. Demanding the request already be pull-only made
+// anonymous pull unreachable for those clients, so the write actions are dropped
+// and a pull-only token is issued instead of a challenge.
+func TestHandler_ServeHTTP_AnonymousMixedScopeNarrowedToPull(t *testing.T) {
 	keyPath := getSharedTestKey(t)
 
 	issuer, err := NewIssuer(keyPath, "atcr.io", "registry", 15*time.Minute)
@@ -269,15 +250,34 @@ func TestHandler_ServeHTTP_AnonymousDisabledChallengesPull(t *testing.T) {
 	}
 
 	handler := NewHandler(issuer, nil)
-	handler.SetAllowAnonymousPull(false)
 
-	req := httptest.NewRequest(http.MethodGet, "/auth/token?service=registry&scope=repository:bob.bsky.social/myapp:pull", nil)
-	w := httptest.NewRecorder()
+	for _, action := range []string{"pull,push", "pull,push,delete", "push,pull"} {
+		t.Run(action, func(t *testing.T) {
+			req := httptest.NewRequest(http.MethodGet, "/auth/token?service=registry&scope=repository:bob.bsky.social/myapp:"+action, nil)
+			w := httptest.NewRecorder()
 
-	handler.ServeHTTP(w, req)
+			handler.ServeHTTP(w, req)
 
-	if w.Code != http.StatusUnauthorized {
-		t.Errorf("Expected status %d when anonymous pull disabled, got %d. Body: %s", http.StatusUnauthorized, w.Code, w.Body.String())
+			if w.Code != http.StatusOK {
+				t.Fatalf("Expected status %d for anonymous %s, got %d. Body: %s", http.StatusOK, action, w.Code, w.Body.String())
+			}
+
+			var resp TokenResponse
+			if err := json.NewDecoder(w.Body).Decode(&resp); err != nil {
+				t.Fatalf("Failed to decode response: %v", err)
+			}
+
+			granted := ExtractAccess(resp.Token)
+			if len(granted) != 1 {
+				t.Fatalf("Expected exactly one access entry, got %+v", granted)
+			}
+			if diff := len(granted[0].Actions); diff != 1 || granted[0].Actions[0] != "pull" {
+				t.Errorf("Expected granted actions [pull], got %v", granted[0].Actions)
+			}
+			if granted[0].Name != "bob.bsky.social/myapp" {
+				t.Errorf("Expected repository name preserved, got %q", granted[0].Name)
+			}
+		})
 	}
 }
 

test/integration/auth_matrix_test.go

@@ -68,10 +68,12 @@ func TestAuthMatrix(t *testing.T) {
 		errContains []string
 	}{
 		{
-			name:   "captain_push",
-			creds:  h.RegistryCreds(captain),
-			op:     "push",
-			repoFn: func(c string) string { return fmt.Sprintf("%s/%s/own-%s:tag", h.AppViewHostPort(), captain.Handle(), c) },
+			name:  "captain_push",
+			creds: h.RegistryCreds(captain),
+			op:    "push",
+			repoFn: func(c string) string {
+				return fmt.Sprintf("%s/%s/own-%s:tag", h.AppViewHostPort(), captain.Handle(), c)
+			},
 		},
 		{
 			name:   "captain_pull",
@@ -80,10 +82,12 @@ func TestAuthMatrix(t *testing.T) {
 			repoFn: func(_ string) string { return seedRef.String() },
 		},
 		{
-			name:   "crew_write_push",
-			creds:  h.RegistryCreds(crewWriter),
-			op:     "push",
-			repoFn: func(c string) string { return fmt.Sprintf("%s/%s/own-%s:tag", h.AppViewHostPort(), crewWriter.Handle(), c) },
+			name:  "crew_write_push",
+			creds: h.RegistryCreds(crewWriter),
+			op:    "push",
+			repoFn: func(c string) string {
+				return fmt.Sprintf("%s/%s/own-%s:tag", h.AppViewHostPort(), crewWriter.Handle(), c)
+			},
 		},
 		{
 			name:   "crew_write_pull",
@@ -92,10 +96,12 @@ func TestAuthMatrix(t *testing.T) {
 			repoFn: func(_ string) string { return seedRef.String() },
 		},
 		{
-			name:   "crew_read_only_push_denied",
-			creds:  h.RegistryCreds(crewReader),
-			op:     "push",
-			repoFn: func(c string) string { return fmt.Sprintf("%s/%s/own-%s:tag", h.AppViewHostPort(), crewReader.Handle(), c) },
+			name:  "crew_read_only_push_denied",
+			creds: h.RegistryCreds(crewReader),
+			op:    "push",
+			repoFn: func(c string) string {
+				return fmt.Sprintf("%s/%s/own-%s:tag", h.AppViewHostPort(), crewReader.Handle(), c)
+			},
 			// authgate's checkCrewBlobWrite surfaces "lacks blob:write" through
 			// errcode.ErrorCodeDenied. The OCI client wraps it with "DENIED".
 			wantErr:     true,
@@ -108,10 +114,12 @@ func TestAuthMatrix(t *testing.T) {
 			repoFn: func(_ string) string { return seedRef.String() },
 		},
 		{
-			name:   "stranger_push_denied",
-			creds:  h.RegistryCreds(stranger),
-			op:     "push",
-			repoFn: func(c string) string { return fmt.Sprintf("%s/%s/own-%s:tag", h.AppViewHostPort(), stranger.Handle(), c) },
+			name:  "stranger_push_denied",
+			creds: h.RegistryCreds(stranger),
+			op:    "push",
+			repoFn: func(c string) string {
+				return fmt.Sprintf("%s/%s/own-%s:tag", h.AppViewHostPort(), stranger.Handle(), c)
+			},
 			// hold_crew_members has no row for stranger → checkCrewBlobWrite
 			// returns "crew membership required".
 			wantErr:     true,
@@ -156,8 +164,8 @@ func TestAuthMatrix(t *testing.T) {
 			// An unresolvable identity must be a clean NAME_UNKNOWN, not a 500.
 			// Anonymous pull makes this path reachable without credentials, and a
 			// 5xx would send clients that retry server errors into a retry loop.
-			creds:  h.AnonCreds(),
-			op:     "pull",
+			creds: h.AnonCreds(),
+			op:    "pull",
 			repoFn: func(c string) string {
 				return fmt.Sprintf("%s/not-a-real-handle/%s:tag", h.AppViewHostPort(), c)
 			},
@@ -189,6 +197,133 @@ func TestAuthMatrix(t *testing.T) {
 	}
 }
 
+// TestAuthMatrixPrivateHold covers `public: false` + `allow_all_crew: true`,
+// the production configuration: anyone may pull and push, but they must have
+// an account. Anonymous gets nothing.
+//
+// The two captain settings are orthogonal. public decides whether a reader
+// with no identity is admitted; allowAllCrew decides whether any authenticated
+// user may self-register as crew. With allowAllCrew on, a signed-in stranger
+// is auto-enrolled with blob:read+blob:write on first contact (the appview
+// reconciles crew membership for pull-only token requests too), so "crew only"
+// and "anyone with an account" are the same set. That is the intent, and
+// stranger_pull below pins it.
+//
+// TestAuthMatrix only ever ran against a public hold, so the anonymous-denied
+// half of this had no end-to-end coverage at all.
+func TestAuthMatrixPrivateHold(t *testing.T) {
+	h := testharness.New(t, testharness.WithPrivateHold())
+
+	captain := h.Captain
+	crewWriter := h.AddSailor("writer.test")
+	crewReader := h.AddSailorWithPermissions("reader.test", []string{"blob:read"})
+	stranger := h.AddStranger("stranger.test")
+
+	// Seed as the captain: the owner can always write to their own hold.
+	seedRef := mustParseRef(t, fmt.Sprintf("%s/%s/seed:tag", h.AppViewHostPort(), captain.Handle()))
+	seedImage, err := random.Image(1<<18, 2)
+	if err != nil {
+		t.Fatalf("build seed image: %v", err)
+	}
+	if err := (craneClient{}).Push(t.Context(), t, seedRef.String(), seedImage, h.RegistryCreds(captain)); err != nil {
+		t.Fatalf("seed push: %v", err)
+	}
+
+	// Denials on a private hold are authorization failures, so accept the
+	// signals the various clients surface for a 401/403.
+	denied := []string{"unauthorized", "Unauthorized", "authentication required",
+		"read access denied", "not a crew member", "denied", "403", "401"}
+
+	cases := []struct {
+		name        string
+		creds       testharness.Auth
+		op          string
+		repoFn      func(client string) string
+		wantErr     bool
+		errContains []string
+	}{
+		{
+			name:   "captain_pull",
+			creds:  h.RegistryCreds(captain),
+			op:     "pull",
+			repoFn: func(_ string) string { return seedRef.String() },
+		},
+		{
+			name: "crew_write_pull",
+			// blob:write implies blob:read.
+			creds:  h.RegistryCreds(crewWriter),
+			op:     "pull",
+			repoFn: func(_ string) string { return seedRef.String() },
+		},
+		{
+			name:   "crew_read_only_pull",
+			creds:  h.RegistryCreds(crewReader),
+			op:     "pull",
+			repoFn: func(_ string) string { return seedRef.String() },
+		},
+		{
+			name: "stranger_pull",
+			// Allowed, and deliberately so: allow_all_crew means a signed-in
+			// stranger self-registers as crew on first contact and is granted
+			// blob:read+blob:write. "You need an account" is the rule here,
+			// not "the captain must have added you".
+			creds:  h.RegistryCreds(stranger),
+			op:     "pull",
+			repoFn: func(_ string) string { return seedRef.String() },
+		},
+		{
+			name: "anonymous_pull_denied",
+			// captain.Public is the only thing that admits an anonymous
+			// reader, and it is false here.
+			creds:       h.AnonCreds(),
+			op:          "pull",
+			repoFn:      func(_ string) string { return seedRef.String() },
+			wantErr:     true,
+			errContains: denied,
+		},
+		{
+			name:  "captain_push",
+			creds: h.RegistryCreds(captain),
+			op:    "push",
+			repoFn: func(c string) string {
+				return fmt.Sprintf("%s/%s/priv-%s:tag", h.AppViewHostPort(), captain.Handle(), c)
+			},
+		},
+		{
+			name:  "anonymous_push_denied",
+			creds: h.AnonCreds(),
+			op:    "push",
+			repoFn: func(c string) string {
+				return fmt.Sprintf("%s/%s/anon-push-%s:tag", h.AppViewHostPort(), captain.Handle(), c)
+			},
+			wantErr:     true,
+			errContains: denied,
+		},
+	}
+
+	for _, c := range Clients {
+		t.Run(c.Name(), func(t *testing.T) {
+			for _, tc := range cases {
+				t.Run(tc.name, func(t *testing.T) {
+					err := runOp(t.Context(), t, c, tc.op, tc.repoFn(c.Name()), tc.creds)
+					if tc.wantErr {
+						if err == nil {
+							t.Fatalf("%s: expected error, got nil", tc.name)
+						}
+						if len(tc.errContains) > 0 && !containsAny(err.Error(), tc.errContains) {
+							t.Errorf("%s: expected error containing any of %q, got: %v", tc.name, tc.errContains, err)
+						}
+						return
+					}
+					if err != nil {
+						t.Fatalf("%s: unexpected error: %v", tc.name, err)
+					}
+				})
+			}
+		})
+	}
+}
+
 // runOp performs the chosen op against the given ref using the supplied
 // client and credentials. Push builds a fresh random image so concurrent or
 // subsequent runs don't collide on shared blob digests at the registry; pull

test/integration/clients.go

@@ -6,6 +6,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"io"
 	"path/filepath"
 	"testing"
 
@@ -83,6 +84,26 @@ func (craneClient) Pull(_ context.Context, ref string, a testharness.Auth) (v1.H
 	if err != nil {
 		return v1.Hash{}, fmt.Errorf("crane pulled image digest: %w", err)
 	}
+	// Materialize the layer bytes. crane.Pull is lazy and img.Digest() needs
+	// only the manifest, which ATCR serves from the user's PDS where it is
+	// world-readable by design — so a "pull" that stops here never touches the
+	// hold and never exercises blob authorization at all. Reading the layers is
+	// what makes a pull case a real test of who may read blobs.
+	layers, err := img.Layers()
+	if err != nil {
+		return v1.Hash{}, normalizeErr(err)
+	}
+	for _, l := range layers {
+		rc, err := l.Compressed()
+		if err != nil {
+			return v1.Hash{}, normalizeErr(err)
+		}
+		_, cerr := io.Copy(io.Discard, rc)
+		rc.Close()
+		if cerr != nil {
+			return v1.Hash{}, normalizeErr(cerr)
+		}
+	}
 	return d, nil
 }