diff --git a/authn/bearer.go b/authn/bearer.go index 60e139e2f754390ceb765506b3ccc2c7540194bb..d73a947f30cb406fa2c9ecff15adfd94587996f1 100644 --- a/authn/bearer.go +++ b/authn/bearer.go @@ -152,37 +152,70 @@ // StatusFor maps an error out of Resolve — or out of a later Authorize — onto // the status the surface must answer with. It is one function so that the three // surfaces cannot each invent their own table. // -// The mapping, and the one line of it that has to be defended: +// Everything the credential itself can be wrong about is bearer.StatusFor's +// answer, not a second copy of it: ErrForbidden is 403, ErrUnavailable is 503 +// and never 401, and ErrInvalid, ErrRevoked and ErrNotOurs are 401. That last +// arm is only reached because ErrNotOurs is decided before we ask — a meta.sr.ht +// PAT used to fall through to spec's own token store, and with that store gone +// it is a refusal at the door. // -// - bearer.ErrUnavailable is 503 and never 401. Reading "I could not reach -// tokens.sr.ht" as "your token is revoked" would refuse every live instance -// token on the instance for as long as a daemon that is deliberately off the -// hot path is restarting, and would tell a thousand clients their -// credentials are bad when the truth is that one service is down. 503 says -// the true thing and keeps the operator's attention where the fault is. -// - ErrMissingGrant and ErrNotInstanceOwner are 403: the credential verifies +// The ErrUnavailable line is the one worth restating even though it is no longer +// spelled here. Reading "I could not reach tokens.sr.ht" as "your token is +// revoked" would refuse every live instance token for as long as a daemon that +// is deliberately off the hot path takes to restart, and would tell a thousand +// clients their credentials are bad when the truth is that one service is down. +// +// What this function adds is what bearer cannot know: +// +// - ErrMissingGrant and ErrNotInstanceOwner are 403. The credential verifies // and the holder is who they say they are, so retrying is pointless and what -// they need is a wider grant, not another login. -// - Everything permanent about the credential itself — malformed, foreign, -// revoked — is 401. -// - ErrNoAgentPlane is 503 and not 401. An instance with no [tokens.sr.ht] -// origin cannot check any credential, and telling the holder of a good token -// that it is bad would send them to re-provision it. -// - Everything else is transient by definition and answers 503, which is the -// fail-closed direction: a backend outage never reads as a valid credential. +// they need is a wider grant, not another login. Both are asked before the +// bearer table, because ErrMissingGrant is raised beside a token that +// verified and must not be read as one that did not. +// - ErrNoToken is 401: nothing was presented on a surface that requires a +// credential. +// - ErrNoAgentPlane, and anything else at all, is 503. An instance with no +// [tokens.sr.ht] origin cannot check any credential, and telling the holder +// of a good token that it is bad would send them to re-provision it; an +// unclassified error is a backend that could not answer. This is where the +// two tables' defaults deliberately differ — bearer's unrecognised failure +// is the caller's credential, because everything reaching it is about a +// credential, while an unrecognised failure here can be the database this +// resolver had to consult, which must never read as a bad token. func StatusFor(err error) int { switch { case err == nil: return http.StatusOK - case errors.Is(err, bearer.ErrUnavailable): - return http.StatusServiceUnavailable - case errors.Is(err, bearer.ErrForbidden), - errors.Is(err, ErrMissingGrant), - errors.Is(err, ErrNotInstanceOwner): + case errors.Is(err, ErrMissingGrant), errors.Is(err, ErrNotInstanceOwner): return http.StatusForbidden - case IsAuthFailure(err): + case isBearerRefusal(err): + return bearer.StatusFor(err) + case errors.Is(err, ErrNoToken): return http.StatusUnauthorized default: return http.StatusServiceUnavailable } } + +// isBearerRefusal reports whether err is one of the sentinels bearer.StatusFor +// has an answer for. The list is here rather than in a helper over there because +// it is the question "did the shared validator decide this?", and a wrong answer +// to it is what would let the 503 default below swallow a 401 — or, worse, let +// bearer's own 401 default swallow a database outage. +func isBearerRefusal(err error) bool { + return errors.Is(err, bearer.ErrForbidden) || + errors.Is(err, bearer.ErrUnavailable) || + errors.Is(err, bearer.ErrInvalid) || + errors.Is(err, bearer.ErrRevoked) || + errors.Is(err, bearer.ErrNotOurs) +} + +// Challenge is the WWW-Authenticate value every 401 this service answers must +// carry, per RFC 9110 §11.6.1 — the scheme, and this service's config section as +// the realm, which is what names it in the config, in the nav and in a grant +// everywhere else on the instance. +// +// It is bearer.Challenge with our section already in it, so that the four +// surfaces that refuse a credential (the resolver's middleware, MCP, /query and +// the read plane's machine formats) cannot name four realms. +func Challenge() string { return bearer.Challenge(ConfigSection) } diff --git a/authn/bearer_test.go b/authn/bearer_test.go index da9fc73ea9371d5ce1800d135a44fbcbee98d13c..85017989cfc49e50aa2078b6330c1b1d17df981f 100644 --- a/authn/bearer_test.go +++ b/authn/bearer_test.go @@ -466,3 +466,11 @@ assert.Equal(t, c.want, StatusFor(c.err)) }) } } + +// RFC 9110 asks a 401 to name the scheme it would accept, and the realm is this +// service's config section — the string that identifies it in the config, in the +// nav and in a grant. One spelling, so that the four surfaces which refuse a +// credential cannot name four realms. +func TestChallenge(t *testing.T) { + assert.Equal(t, `Bearer realm="spec.sr.ht"`, Challenge()) +} diff --git a/authn/resolver.go b/authn/resolver.go index c9664488b39e0b2f23767e332fdc7c92e3ad0202..fc5ed47383398c022791e3ad4029baca19ec78e8 100644 --- a/authn/resolver.go +++ b/authn/resolver.go @@ -207,6 +207,13 @@ slog.ErrorContext(r.Context(), "resolving an agent credential failed", "method", r.Method, "path", r.URL.Path, "status", status, scribe.Err(err)) } + if status == http.StatusUnauthorized { + // RFC 9110 requires the challenge on a 401, and the caller + // here is always a machine holding a bearer token: naming the + // scheme and the realm is what tells it which credential this + // service was refusing. + w.Header().Set("WWW-Authenticate", Challenge()) + } http.Error(w, resolveFailureMessage(status), status) return } diff --git a/authn/resolver_test.go b/authn/resolver_test.go index 041a9501a0d7974b81a9f60e711e06752a610d05..840a533433b91799108902c4cefeae4538a80927 100644 --- a/authn/resolver_test.go +++ b/authn/resolver_test.go @@ -263,6 +263,30 @@ t.Fatalf("code = %d, want 401", code) } } +// A 401 out of this middleware names the scheme and the realm, which is what an +// agent library reads to know which credential was refused — and what a 503 must +// not carry, since nothing about the credential was decided. +func TestMiddleware_RefusalCarriesTheChallenge(t *testing.T) { + rs := newTestResolver(t) + + h := rs.Middleware()(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, request("", map[string]string{"Authorization": "Bearer never-issued"})) + require.Equal(t, http.StatusUnauthorized, rec.Code) + assert.Equal(t, Challenge(), rec.Header().Get("WWW-Authenticate")) + + f := newPlaneFixture(t, http.StatusNoContent) + f.users.err = errors.New("connection refused") + h = f.rs.Middleware()(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + rec = httptest.NewRecorder() + h.ServeHTTP(rec, request("", map[string]string{ + "Authorization": "Bearer " + instanceToken("spec:read"), + })) + require.Equal(t, http.StatusServiceUnavailable, rec.Code) + assert.Empty(t, rec.Header().Get("WWW-Authenticate"), + "a backend outage says nothing about the credential") +} + // A user lookup that cannot answer is a backend outage, not a bad credential: // fail closed with a 503 rather than telling a live agent its token is bad. func TestMiddleware_BackendOutageIs503(t *testing.T) { diff --git a/graph/server.go b/graph/server.go index 280cd57b58daa49ebf7e4bdb13afe698db1c7104..ec2f2e62d04c19bbf655ab0c7796f64e122f0bb7 100644 --- a/graph/server.go +++ b/graph/server.go @@ -165,6 +165,7 @@ return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { p := authn.PrincipalFromContext(r.Context()) if !p.CanRead() { w.Header().Set("Content-Type", "text/plain; charset=utf-8") + w.Header().Set("WWW-Authenticate", authn.Challenge()) http.Error(w, "authentication required", http.StatusUnauthorized) return } diff --git a/mcpsrv/mcpsrv.go b/mcpsrv/mcpsrv.go index 132a4774bbf160124f655cfa92843f96424d2cd7..84a008d482d02eba36341f0931bd1f76cb78fb35 100644 --- a/mcpsrv/mcpsrv.go +++ b/mcpsrv/mcpsrv.go @@ -297,6 +297,7 @@ func Gate(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if !authn.PrincipalFromContext(r.Context()).CanRead() { w.Header().Set("Content-Type", "text/plain; charset=utf-8") + w.Header().Set("WWW-Authenticate", authn.Challenge()) http.Error(w, "authentication required", http.StatusUnauthorized) return } diff --git a/web/handlers.go b/web/handlers.go index 933634241fb82f953484508b039790c58992a5e7..9a8bc6282059e3c9725336b33415b55b8772fe41 100644 --- a/web/handlers.go +++ b/web/handlers.go @@ -95,6 +95,12 @@ s.loginRedirect(w, r) return } w.Header().Set("Content-Type", "text/plain; charset=utf-8") + // The challenge belongs on the machine shape and not on the browser one: + // this is the branch a client asking for .md or .json takes, and RFC 9110 + // asks a 401 to name the scheme it would accept. A browser never reaches + // here — it gets the redirect above, because the session is a cookie + // meta.sr.ht sets and there is no HTTP authentication scheme for it. + w.Header().Set("WWW-Authenticate", authn.Challenge()) http.Error(w, "authentication required", http.StatusUnauthorized) }