appview: fix two /auth/token defects, wrong login host and dropped scopes

Both are pre-existing and were found while working on finding 27.

sendAuthError built its "docker login <host>" line from r.Host. /auth/token is
served on the UI domain as well as on every registry domain, and the
WWW-Authenticate realm points at the UI domain's copy, so a client following
the realm was told to run "docker login seamark.dev" - the one host that
deliberately refuses /v2/* with an OCI UNSUPPORTED error pointing at
seamark.cr. It now uses the service resolved for the token, which is the
registry domain, falling back to the deployment's primary rather than to
r.Host. The single-domain case still prints a host that serves /v2/, and an
unconfigured service supplied by the client cannot steer it.

Separately, the scope parameter was read with .Get, taking the first value
only. The Docker token spec allows scope to be repeated, so a client asking for
two repositories was issued a token covering one and got a 401 on the other.
Both wire forms are now flattened, on the GET query string and on the OAuth2
POST body, which had the same defect via PostFormValue.

Empty and whitespace-only values are dropped. Exact duplicate scope strings
collapse, but two entries naming the same repository with different actions are
left alone: merging them would union the action sets, and every gate downstream
is written only to narrow.

More entries now reach the anonymous gate added in af7522b, which is the
intended effect. Its per-entry verdict is unchanged: public entries survive,
private ones are dropped, and an all-private request still gets the challenge.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
This commit is contained in:
Evan Jarrett
2026-09-02 21:37:56 -05:00
co-authored by Claude Opus 5
parent 84872580a3
commit 2743445e65
3 changed files with 443 additions and 22 deletions
+79 -22
View File
@@ -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 <r.Host>" 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
}
+145
View File
@@ -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)
}
}
+219
View File
@@ -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())
}
}