diff --git a/pkg/auth/token/handler.go b/pkg/auth/token/handler.go index 130dd07..658a16a 100644 --- a/pkg/auth/token/handler.go +++ b/pkg/auth/token/handler.go @@ -221,16 +221,39 @@ func getBaseURL(r *http.Request) string { return baseURL } -// sendAuthError sends a formatted authentication error response -func sendAuthError(w http.ResponseWriter, r *http.Request, message string) { - baseURL := getBaseURL(r) +// sendAuthError sends the plain-text guidance a user sees when docker login fails. +// +// registryHost is the registry domain the token is being issued for, as +// resolved by resolveService. It is deliberately not r.Host: /auth/token is +// served on the UI domain as well as on every registry domain (the realm below +// points at the UI domain's copy, and DomainRoutingMiddleware serves the +// endpoint directly on registry domains so the Authorization header survives). +// On a split-domain deployment the UI host refuses /v2/* with an OCI +// UNSUPPORTED error, so "docker login " printed there names a host +// where the handshake cannot succeed. resolveService falls back to the +// deployment's primary registry domain, which is also right when the UI host +// and the registry domain are the same host. +// +// The install URL keeps using the request's own base URL: that page lives on +// the UI domain, and a registry domain redirects non-registry paths there. +// +// An empty registryHost (only reachable from a handler whose issuer has no +// service configured) drops step 2 rather than printing a hostname that cannot +// work. +func sendAuthError(w http.ResponseWriter, r *http.Request, registryHost, message string) { w.Header().Set("WWW-Authenticate", `Basic realm="ATCR Registry"`) - http.Error(w, fmt.Sprintf(`%s + + guidance := fmt.Sprintf(`%s To authenticate: - 1. Install credential helper: %s/install + 1. Install credential helper: %s/install`, message, getBaseURL(r)) + if registryHost != "" { + guidance += fmt.Sprintf(` 2. Or run: docker login %s - (use your ATProto handle + app-password)`, message, baseURL, r.Host), http.StatusUnauthorized) + (use your ATProto handle + app-password)`, registryHost) + } + + http.Error(w, guidance, http.StatusUnauthorized) } // AuthErrorResponse is returned when authentication fails in a way the credential helper can handle @@ -280,6 +303,43 @@ func writeOAuthError(w http.ResponseWriter, status int, code, description string } } +// collectScopes flattens the scope parameter into the individual scope strings +// ParseScope consumes. +// +// The Docker token spec lets the requested scope arrive in either of two forms, +// and clients use both: one space-separated value +// (scope=repository:a/b:pull repository:c/d:pull) or the parameter repeated +// (scope=repository:a/b:pull&scope=repository:c/d:pull). Reading only the first +// value dropped every repository after the first without comment, so a client +// asking for two got a token covering one and a 401 on the other. Both +// dimensions are flattened here, so the two forms produce identical access. +// +// strings.Fields skips empty and whitespace-only values, so a bare "scope=" +// contributes nothing instead of an entry ParseScope would reject. +// +// An exactly repeated scope string collapses to one. Duplicates are dropped, +// never merged: folding two entries that name the same repository with +// different actions would union their action sets, and that is a widening, +// while every gate downstream (ValidateAccess, Authorize, AuthorizeAnonymous) +// is written only to narrow. A repeat of an identical string cannot mean more +// than the string itself, which is what makes removing it safe; two entries for +// one repository with different actions are left as they arrived, exactly as +// the space-separated form has always delivered them. +func collectScopes(values []string) []string { + var scopes []string + seen := make(map[string]bool) + for _, value := range values { + for _, scope := range strings.Fields(value) { + if seen[scope] { + continue + } + seen[scope] = true + scopes = append(scopes, scope) + } + } + return scopes +} + // ServeHTTP handles the token request. // // Both Docker token specs land here. The original spec is GET with HTTP Basic @@ -292,7 +352,8 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { phaseStart := time.Now() slog.Debug("Received token request", "method", r.Method, "path", r.URL.Path) - var username, password, scopeParam, requestedService string + var username, password, requestedService string + var scopes []string switch r.Method { case http.MethodGet: @@ -300,7 +361,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // request is served without them, and whether that applies depends on the // scope, which is parsed below. An empty username is the signal. username, password, _ = r.BasicAuth() - scopeParam = r.URL.Query().Get("scope") + scopes = collectScopes(r.URL.Query()["scope"]) requestedService = r.URL.Query().Get("service") case http.MethodPost: @@ -334,7 +395,9 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { "username and password are required") return } - scopeParam = r.PostFormValue("scope") + // ParseForm above has already populated PostForm, so the repeated form + // is readable here too. PostFormValue would take only the first value. + scopes = collectScopes(r.PostForm["scope"]) requestedService = r.PostFormValue("service") default: @@ -355,12 +418,6 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // configured registry domains. service := h.resolveService(r, requestedService) - // Parse scopes - var scopes []string - if scopeParam != "" { - scopes = strings.Split(scopeParam, " ") - } - access, err := auth.ParseScope(scopes) if err != nil { http.Error(w, fmt.Sprintf("invalid scope: %v", err), http.StatusBadRequest) @@ -392,11 +449,11 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } slog.Debug("Anonymous pull denied: no requested repository admits anonymous reads") - sendAuthError(w, r, "authentication required") + sendAuthError(w, r, service, "authentication required") return } slog.Debug("No Basic auth credentials provided") - sendAuthError(w, r, "authentication required") + sendAuthError(w, r, service, "authentication required") return } @@ -410,7 +467,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { device, err := h.deviceStore.ValidateDeviceSecret(password) if err != nil { slog.Debug("Device secret validation failed", "error", err) - sendAuthError(w, r, "authentication failed") + sendAuthError(w, r, service, "authentication failed") return } @@ -439,16 +496,16 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // Log at WARN level with specific error type if errors.Is(err, auth.ErrIdentityResolution) { slog.Warn("Identity resolution failed", "error", err, "username", username) - sendAuthError(w, r, "authentication failed: could not resolve handle") + sendAuthError(w, r, service, "authentication failed: could not resolve handle") } else if errors.Is(err, auth.ErrInvalidCredentials) { slog.Warn("Invalid credentials", "username", username) - sendAuthError(w, r, "authentication failed: invalid credentials") + sendAuthError(w, r, service, "authentication failed: invalid credentials") } else if errors.Is(err, auth.ErrPDSUnavailable) { slog.Warn("PDS unavailable", "error", err, "username", username) - sendAuthError(w, r, "authentication failed: PDS unavailable") + sendAuthError(w, r, service, "authentication failed: PDS unavailable") } else { slog.Warn("Authentication failed", "error", err, "username", username) - sendAuthError(w, r, "authentication failed") + sendAuthError(w, r, service, "authentication failed") } return } diff --git a/pkg/auth/token/handler_auth_error_test.go b/pkg/auth/token/handler_auth_error_test.go new file mode 100644 index 0000000..e8aeeda --- /dev/null +++ b/pkg/auth/token/handler_auth_error_test.go @@ -0,0 +1,145 @@ +package token + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" +) + +// The plain-text guidance sent with a 401 has to name a host `docker login` +// can actually handshake against. /auth/token answers on the UI domain (that +// is where the WWW-Authenticate realm points) as well as on every registry +// domain, and the UI domain refuses /v2/* outright, so the guidance must name +// the resolved registry service rather than echoing the request's own host. +func TestSendAuthError_NamesRegistryDomainNotRequestHost(t *testing.T) { + keyPath := getSharedTestKey(t) + + tests := []struct { + name string + // services declared on the handler; the issuer's service is the first. + services []string + // host the request arrives on. + host string + // service parameter the client echoes back, if any. + requested string + + wantLogin string // hostname step 2 must name + wantAbsent []string // hostnames step 2 must not name + }{ + { + // seamark-shaped: UI on seamark.dev, registry on seamark.cr. A + // client following the realm lands on the UI host. + name: "split domain, request on the UI host", + services: []string{"seamark.cr"}, + host: "seamark.dev", + wantLogin: "seamark.cr", + wantAbsent: []string{"docker login seamark.dev"}, + }, + { + name: "split domain, request on the registry host", + services: []string{"seamark.cr"}, + host: "seamark.cr", + wantLogin: "seamark.cr", + }, + { + // Multiple front doors: the guidance follows the service the + // client is authenticating against, not the primary. + name: "secondary registry domain via ?service=", + services: []string{"seamark.cr", "buoy.cr"}, + host: "seamark.dev", + requested: "buoy.cr", + wantLogin: "buoy.cr", + wantAbsent: []string{"docker login seamark.dev"}, + }, + { + // ?service= is client-influenced, so an unconfigured value falls + // back to the primary registry domain, never to r.Host. + name: "unconfigured ?service= falls back to the primary", + services: []string{"seamark.cr"}, + host: "seamark.dev", + requested: "evil.example", + wantLogin: "seamark.cr", + wantAbsent: []string{"docker login seamark.dev", "evil.example"}, + }, + { + // Single-domain deployment (the dev stack): the UI host is the + // registry domain, and the printed hostname is still correct. + name: "single domain, UI host is the registry domain", + services: []string{"localhost"}, + host: "localhost:5000", + wantLogin: "localhost", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + issuer, err := NewIssuer(keyPath, "atcr.io", tt.services[0], 15*time.Minute) + if err != nil { + t.Fatalf("NewIssuer() error = %v", err) + } + handler := NewHandler(issuer, nil) + handler.SetServices(tt.services) + + // A push-only scope with no credentials has no anonymous + // component, so it draws the plain-text challenge. + target := "/auth/token?scope=repository:bob.bsky.social/myapp:push" + if tt.requested != "" { + target += "&service=" + tt.requested + } + req := httptest.NewRequest(http.MethodGet, target, nil) + req.Host = tt.host + + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + if w.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want %d. Body: %s", w.Code, http.StatusUnauthorized, w.Body.String()) + } + + body := w.Body.String() + want := "docker login " + tt.wantLogin + if !strings.Contains(body, want) { + t.Errorf("guidance does not contain %q.\nBody:\n%s", want, body) + } + for _, absent := range tt.wantAbsent { + if strings.Contains(body, absent) { + t.Errorf("guidance contains %q, which cannot serve /v2/.\nBody:\n%s", absent, body) + } + } + + // Step 1 stays on the request's own base URL: the install page + // lives on the UI domain and registry domains redirect there. + if wantInstall := getBaseURL(req) + "/install"; !strings.Contains(body, wantInstall) { + t.Errorf("guidance does not contain install URL %q.\nBody:\n%s", wantInstall, body) + } + }) + } +} + +// A handler whose issuer has no service configured has no registry domain to +// name, so it drops the docker login step rather than printing a host that +// cannot work. +func TestSendAuthError_NoServiceOmitsDockerLoginStep(t *testing.T) { + keyPath := getSharedTestKey(t) + + issuer, err := NewIssuer(keyPath, "atcr.io", "", 15*time.Minute) + if err != nil { + t.Fatalf("NewIssuer() error = %v", err) + } + handler := NewHandler(issuer, nil) + + req := httptest.NewRequest(http.MethodGet, "/auth/token?scope=repository:bob.bsky.social/myapp:push", nil) + req.Host = "seamark.dev" + + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + if w.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want %d", w.Code, http.StatusUnauthorized) + } + if body := w.Body.String(); strings.Contains(body, "docker login") { + t.Errorf("guidance names a docker login host with no service configured.\nBody:\n%s", body) + } +} diff --git a/pkg/auth/token/handler_scope_repeat_test.go b/pkg/auth/token/handler_scope_repeat_test.go new file mode 100644 index 0000000..ae7d1c1 --- /dev/null +++ b/pkg/auth/token/handler_scope_repeat_test.go @@ -0,0 +1,219 @@ +package token + +import ( + "net/http" + "net/http/httptest" + "net/url" + "slices" + "sort" + "strings" + "testing" + "time" + + "atcr.io/pkg/auth" +) + +// accessNames returns the repository names in the token, sorted, so an +// assertion does not depend on parameter ordering. +func accessNames(access []auth.AccessEntry) []string { + names := make([]string, 0, len(access)) + for _, entry := range access { + names = append(names, entry.Name) + } + sort.Strings(names) + return names +} + +// scopeTestHandler wires an issuer, a device store and a device secret together +// so each test drives a real authenticated request. +func scopeTestHandler(t *testing.T) (*Handler, string) { + t.Helper() + issuer, err := NewIssuer(getSharedTestKey(t), "atcr.io", "registry", 5*time.Minute) + if err != nil { + t.Fatalf("NewIssuer() error = %v", err) + } + deviceStore, database := setupTestDeviceStore(t) + secret := createTestDevice(t, deviceStore, database, "did:plc:alice123", "alice.bsky.social") + return NewHandler(issuer, deviceStore), secret +} + +// getAccess runs the GET form with a pre-built raw query and returns the access +// list from the issued JWT. +func getAccess(t *testing.T, rawQuery string) []auth.AccessEntry { + t.Helper() + h, secret := scopeTestHandler(t) + req := httptest.NewRequest(http.MethodGet, "/auth/token?"+rawQuery, nil) + req.SetBasicAuth("alice", secret) + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d. Body: %s", w.Code, w.Body.String()) + } + return ExtractAccess(decodeToken(t, w)) +} + +// The defect: the handler read only the first ?scope= value, so a client asking +// for two repositories was issued a token covering one and got a 401 on the +// other. The Docker token spec allows the parameter to repeat, and clients use +// it. This test fails before the fix with exactly one entry. +func TestHandler_Scope_RepeatedParamsYieldAllEntries(t *testing.T) { + access := getAccess(t, + "service=registry"+ + "&scope=repository:alice.bsky.social/first:pull"+ + "&scope=repository:alice.bsky.social/second:pull") + + want := []string{"alice.bsky.social/first", "alice.bsky.social/second"} + if got := accessNames(access); !slices.Equal(got, want) { + t.Fatalf("repeated scope params dropped entries: got %v, want %v (full access %+v)", got, want, access) + } + for _, entry := range access { + if !slices.Equal(entry.Actions, []string{"pull"}) { + t.Errorf("entry %q: expected actions [pull], got %v", entry.Name, entry.Actions) + } + } +} + +// Both forms in one request: a repeated parameter where one of the values is +// itself space-separated. Flattening has to happen in both dimensions. +func TestHandler_Scope_MixedRepeatedAndSpaceSeparated(t *testing.T) { + access := getAccess(t, + "service=registry"+ + "&scope="+url.QueryEscape("repository:alice.bsky.social/first:pull repository:alice.bsky.social/second:pull")+ + "&scope=repository:alice.bsky.social/third:pull") + + want := []string{"alice.bsky.social/first", "alice.bsky.social/second", "alice.bsky.social/third"} + if got := accessNames(access); !slices.Equal(got, want) { + t.Fatalf("mixed scope forms lost entries: got %v, want %v", got, want) + } +} + +// Regression guard: the space-separated form is what works today and must keep +// working byte for byte. +func TestHandler_Scope_SingleSpaceSeparatedValueStillWorks(t *testing.T) { + access := getAccess(t, + "service=registry"+ + "&scope="+url.QueryEscape("repository:alice.bsky.social/first:pull,push repository:alice.bsky.social/second:pull")) + + want := []string{"alice.bsky.social/first", "alice.bsky.social/second"} + if got := accessNames(access); !slices.Equal(got, want) { + t.Fatalf("space-separated scope regressed: got %v, want %v", got, want) + } + for _, entry := range access { + if entry.Name == "alice.bsky.social/first" && !slices.Equal(entry.Actions, []string{"pull", "push"}) { + t.Errorf("expected actions [pull push] to survive, got %v", entry.Actions) + } + } +} + +// An empty scope value must contribute nothing: ParseScope would reject a bare +// "" as a malformed scope if it ever reached it, and a bogus entry in the token +// is worse than none. +func TestHandler_Scope_EmptyValuesAreIgnored(t *testing.T) { + access := getAccess(t, "service=registry&scope=&scope=&scope="+url.QueryEscape(" ")) + if len(access) != 0 { + t.Fatalf("expected empty scope params to yield no access entries, got %+v", access) + } + + // An empty value alongside a real one must not disturb the real one. + access = getAccess(t, "service=registry&scope=&scope=repository:alice.bsky.social/first:pull") + if got := accessNames(access); !slices.Equal(got, []string{"alice.bsky.social/first"}) { + t.Fatalf("an empty scope param disturbed the real one: got %v", got) + } +} + +// The same scope string twice cannot mean more than once, so it collapses. +// Duplicates are dropped rather than merged: merging entries that name one +// repository with different action sets would union them, and nothing on this +// path is allowed to widen. +func TestHandler_Scope_ExactDuplicatesCollapse(t *testing.T) { + access := getAccess(t, + "service=registry"+ + "&scope=repository:alice.bsky.social/first:pull"+ + "&scope=repository:alice.bsky.social/first:pull") + + if len(access) != 1 { + t.Fatalf("expected the duplicate scope to collapse to one entry, got %+v", access) + } + if !slices.Equal(access[0].Actions, []string{"pull"}) { + t.Errorf("a duplicate must not widen the actions, got %v", access[0].Actions) + } +} + +// The POST form (the OAuth2 token spec containerd and Docker try first) reads +// its scope from the form body, and r.PostFormValue has the same first-value +// trap as r.URL.Query().Get. +func TestHandler_Scope_PostFormRepeatedValues(t *testing.T) { + h, secret := scopeTestHandler(t) + + form := url.Values{} + form.Set("grant_type", "password") + form.Set("username", "alice") + form.Set("password", secret) + form.Set("service", "registry") + form["scope"] = []string{ + "repository:alice.bsky.social/first:pull", + "repository:alice.bsky.social/second:pull repository:alice.bsky.social/third:pull", + "", + } + + req := httptest.NewRequest(http.MethodPost, "/auth/token", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200 on the POST form, got %d. Body: %s", w.Code, w.Body.String()) + } + want := []string{"alice.bsky.social/first", "alice.bsky.social/second", "alice.bsky.social/third"} + if got := accessNames(ExtractAccess(decodeToken(t, w))); !slices.Equal(got, want) { + t.Fatalf("POST form dropped scope entries: got %v, want %v", got, want) + } +} + +// More entries now reach the anonymous gate, which is the point: the gate has +// to keep deciding per entry. Public survives, private is dropped. +func TestHandler_Scope_RepeatedParamsAnonymousGateStillNarrows(t *testing.T) { + gate := &stubAnonymousAuthorizer{public: []string{"alice.test/public-app"}} + h := newGateTestHandler(t, gate) + + w := anonymousGet(t, h, + "service=registry"+ + "&scope=repository:alice.test/public-app:pull"+ + "&scope=repository:bob.test/private-app:pull,push") + if w.Code != http.StatusOK { + t.Fatalf("expected 200 when one repeated entry is public, got %d. Body: %s", w.Code, w.Body.String()) + } + + access := ExtractAccess(decodeToken(t, w)) + if got := accessNames(access); !slices.Equal(got, []string{"alice.test/public-app"}) { + t.Fatalf("expected only the public entry to survive, got %v", got) + } + if !slices.Equal(access[0].Actions, []string{"pull"}) { + t.Errorf("anonymous token must carry pull and nothing else, got %v", access[0].Actions) + } + // Both entries have to reach the gate now, not just the first one. + if len(gate.saw) != 2 { + t.Errorf("expected the gate to see both repeated entries, saw %+v", gate.saw) + } +} + +// Nothing grantable across every repeated entry must still challenge rather +// than mint a token the registry would refuse. +func TestHandler_Scope_RepeatedParamsAllPrivateStillChallenges(t *testing.T) { + gate := &stubAnonymousAuthorizer{} // nothing is public + h := newGateTestHandler(t, gate) + + w := anonymousGet(t, h, + "service=registry"+ + "&scope=repository:alice.test/private-one:pull"+ + "&scope=repository:bob.test/private-two:pull") + if w.Code != http.StatusUnauthorized { + t.Fatalf("expected 401 when no repeated entry is grantable, got %d. Body: %s", w.Code, w.Body.String()) + } + if w.Header().Get("WWW-Authenticate") == "" { + t.Error("expected a WWW-Authenticate challenge so docker prompts for credentials") + } + if strings.Contains(w.Body.String(), `"token"`) { + t.Errorf("expected no token in the challenge body, got %q", w.Body.String()) + } +}