diff --git a/config-appview.example.yaml b/config-appview.example.yaml index ce56aed..eb319bb 100644 --- a/config-appview.example.yaml +++ b/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. diff --git a/deploy/upcloud/configs/appview.yaml.tmpl b/deploy/upcloud/configs/appview.yaml.tmpl index c0cf917..e48b4ce 100644 --- a/deploy/upcloud/configs/appview.yaml.tmpl +++ b/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 diff --git a/internal/testharness/harness.go b/internal/testharness/harness.go index d738421..2c5f734 100644 --- a/internal/testharness/harness.go +++ b/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) diff --git a/pkg/appview/config.go b/pkg/appview/config.go index 6442d81..41fe578 100644 --- a/pkg/appview/config.go +++ b/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) diff --git a/pkg/appview/middleware/registry.go b/pkg/appview/middleware/registry.go index 0aea1a5..6d13f07 100644 --- a/pkg/appview/middleware/registry.go +++ b/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 diff --git a/pkg/appview/server.go b/pkg/appview/server.go index 7028242..d173271 100644 --- a/pkg/appview/server.go +++ b/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 diff --git a/pkg/appview/storage/proxy_blob_store.go b/pkg/appview/storage/proxy_blob_store.go index 4b79774..bb03ad6 100644 --- a/pkg/appview/storage/proxy_blob_store.go +++ b/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) } diff --git a/pkg/auth/hold_authorizer.go b/pkg/auth/hold_authorizer.go index 1333144..1ec0c0c 100644 --- a/pkg/auth/hold_authorizer.go +++ b/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 } diff --git a/pkg/auth/hold_authorizer_test.go b/pkg/auth/hold_authorizer_test.go index 24a0108..7d5cef1 100644 --- a/pkg/auth/hold_authorizer_test.go +++ b/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") } } diff --git a/pkg/auth/hold_remote.go b/pkg/auth/hold_remote.go index 6fbdfe4..dcc7b9b 100644 --- a/pkg/auth/hold_remote.go +++ b/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 diff --git a/pkg/auth/holdlocal/holdlocal.go b/pkg/auth/holdlocal/holdlocal.go index b238ccc..ef5edc7 100644 --- a/pkg/auth/holdlocal/holdlocal.go +++ b/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. diff --git a/pkg/auth/token/claims.go b/pkg/auth/token/claims.go index 7b21dec..b6bf0b9 100644 --- a/pkg/auth/token/claims.go +++ b/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 diff --git a/pkg/auth/token/handler.go b/pkg/auth/token/handler.go index f742db7..1d99603 100644 --- a/pkg/auth/token/handler.go +++ b/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") diff --git a/pkg/auth/token/handler_test.go b/pkg/auth/token/handler_test.go index 06f0dbb..0b9cac7 100644 --- a/pkg/auth/token/handler_test.go +++ b/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) + } + }) } } diff --git a/test/integration/auth_matrix_test.go b/test/integration/auth_matrix_test.go index ff600ec..f09a730 100644 --- a/test/integration/auth_matrix_test.go +++ b/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 diff --git a/test/integration/clients.go b/test/integration/clients.go index d84a031..1249a3c 100644 --- a/test/integration/clients.go +++ b/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 }