diff --git a/api/api_test.go b/api/api_test.go index 40097395d86432e77cb67e74aa9a46ee690aafea..b8e11cee3159c4864d02c4a47128218b05b15cc7 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -116,7 +116,9 @@ } if len(g.Writes) != 1 || g.Writes[0].Path != "specs/0007-storage.md" || string(g.Writes[0].Content) != "the document" { t.Errorf("writes = %+v", g.Writes) } - if g.Principal != agent() { + // Principal is no longer comparable with == — it carries a grant set — so + // its rendering stands in for it here. + if g.Principal.String() != agent().String() { t.Errorf("principal = %+v, want the agent on the context", g.Principal) } } diff --git a/api/bearer_test.go b/api/bearer_test.go new file mode 100644 index 0000000000000000000000000000000000000000..abd17e5776486b5af61b6da6aba1779df4062250 --- /dev/null +++ b/api/bearer_test.go @@ -0,0 +1,239 @@ +package api_test + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "net/http" + "net/http/httptest" + "os" + "strings" + "testing" + "testing/fstest" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sourcecraft.dev/bigbes/sr-ht-core/auth" + "sourcecraft.dev/bigbes/sr-ht-core/config" + "sourcecraft.dev/bigbes/sr-ht-core/crypto" + "sourcecraft.dev/bigbes/sr-ht-ecore/bearer" + + "sourcecraft.dev/bigbes/sr-ht-spec/api" + "sourcecraft.dev/bigbes/sr-ht-spec/authn" + "sourcecraft.dev/bigbes/sr-ht-spec/core" + "sourcecraft.dev/bigbes/sr-ht-spec/service" +) + +// TestMain initialises the one piece of process-global core-go state this file +// needs: crypto.InitCrypto holds the key working tokens are signed with, derived +// from [webhooks] private-key. The keys are the ones core-go's own tests use. +// The rest of api_test does not need it, which is why it never had a TestMain. +func TestMain(m *testing.M) { + config.FS = fstest.MapFS{ + "config.ini": &fstest.MapFile{Data: []byte(` +[webhooks] +private-key=ebzsjPaN6E13ln/FeNWly1C92q6bVMVdOnDo1HPl5fc= + +[sr.ht] +network-key=tbuG-7Vh44vrDq1L_HKWkHnWrDOtJhEkPKPiauaLeuk= +`)}, + } + crypto.InitCrypto(config.LoadConfig()) + os.Exit(m.Run()) +} + +// --------------------------------------------------------------------------- +// Fixtures: the two credential planes, in front of the real REST write route. +// --------------------------------------------------------------------------- + +// localTokens is spec's own agent_token table, in memory. +type localTokens struct { + rows map[string]authn.AgentToken + calls int +} + +func newLocalTokens() *localTokens { return &localTokens{rows: map[string]authn.AgentToken{}} } + +func (s *localTokens) add(secret, name string) { + sum := sha256.Sum256([]byte(secret)) + s.rows[hex.EncodeToString(sum[:])] = authn.AgentToken{ID: 1, Name: name, Hash: sum[:]} +} + +func (s *localTokens) LookupAgentToken(_ context.Context, hash []byte) (authn.AgentToken, error) { + s.calls++ + tok, ok := s.rows[hex.EncodeToString(hash)] + if !ok { + return authn.AgentToken{}, authn.ErrUnknownToken + } + return tok, nil +} + +// users resolves the instance owner to a local row. +type users struct{} + +func (users) LookupUser(_ context.Context, username string) (authn.InstanceUser, error) { + return authn.InstanceUser{ID: 1, Username: username}, nil +} + +// sealToken mints a signed working token the way tokens.sr.ht does. +func sealToken(grantString string, expires time.Time) string { + bt := &auth.BearerToken{ + Version: auth.TokenVersion, + Expires: auth.ToTimestamp(expires), + Grants: grantString, + ClientID: bearer.TokensClientID, + Username: "bigbes", + } + return bt.Encode() +} + +func liveToken(grantString string) string { return sealToken(grantString, time.Now().Add(time.Hour)) } + +// planeFixture is the REST write plane behind the real resolver middleware, +// with both credential planes wired: the tokens.sr.ht validator pointed at a +// fake revocation daemon, and the local agent_token store. +type planeFixture struct { + handler http.Handler + writer *fakeWriter + local *localTokens +} + +func newPlaneFixture(t *testing.T, daemonStatus int, closeDaemon bool) *planeFixture { + t.Helper() + + daemon := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(daemonStatus) + })) + origin := daemon.URL + if closeDaemon { + daemon.Close() // nothing answers there any more + } else { + t.Cleanup(daemon.Close) + } + + v, err := bearer.New(bearer.Options{Origin: origin, ClientID: "spec.sr.ht", NodeID: "spec-test"}) + require.NoError(t, err) + + local := newLocalTokens() + resolver, err := authn.NewResolver("bigbes", local, authn.WithInstancePlane(v, users{})) + require.NoError(t, err) + + w := &fakeWriter{res: service.ProposeResult{ + Proposal: service.Proposal{ID: 42, Branch: "proposals/42", BaseRev: "deadbeef", State: core.StateOpen}, + URL: "https://spec.srht.bigb.es/~bigbes/rfcs/p/42", + }} + srv, err := api.New(api.Options{Writer: w, Resolver: resolver}) + require.NoError(t, err) + + return &planeFixture{handler: srv.Handler(), writer: w, local: local} +} + +func (f *planeFixture) put(token string) *httptest.ResponseRecorder { + req := httptest.NewRequest(http.MethodPut, + "/v1/spaces/~bigbes/rfcs/docs/specs/0007-storage.md?title=Storage&message=add+it", + strings.NewReader("the document")) + req.Header.Set("If-Match", "1f0c1d1a") + req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set(authn.HeaderAgent, "claude-code/spec-writer") + req.Header.Set(authn.HeaderAgentSession, "8fb9c9a4-b078-4af1-89eb-d97c522f9921") + rec := httptest.NewRecorder() + f.handler.ServeHTTP(rec, req) + return rec +} + +// --------------------------------------------------------------------------- + +// The credential every agent on the instance is configured with today still +// reaches the write plane, with the tokens.sr.ht plane wired in front of it. +func TestPutWithTheLocalAgentTokenStillWorks(t *testing.T) { + f := newPlaneFixture(t, http.StatusNoContent, false) + f.local.add("live-token", "laptop") + + rec := f.put("live-token") + require.Equal(t, http.StatusCreated, rec.Code, "body: %s", rec.Body) + + p := f.writer.got.Principal + assert.True(t, p.IsAgent()) + assert.Equal(t, authn.PlaneLocal, p.Plane) + assert.Equal(t, "laptop", p.TokenName) + assert.Equal(t, "claude-code/spec-writer", p.Agent) + assert.NoError(t, p.Authorize(authn.ActionPropose), + "the local plane carries no grants and is refused none") +} + +// A tokens.sr.ht working token reaches the same route, and arrives carrying the +// grants service.Propose will check. +func TestPutWithAnInstanceToken(t *testing.T) { + f := newPlaneFixture(t, http.StatusNoContent, false) + + rec := f.put(liveToken("spec:read spec:propose")) + require.Equal(t, http.StatusCreated, rec.Code, "body: %s", rec.Body) + + p := f.writer.got.Principal + assert.True(t, p.IsAgent()) + assert.Equal(t, authn.PlaneInstance, p.Plane) + assert.Equal(t, "bigbes", p.Owner, "the agent still acts for the instance owner") + assert.Equal(t, 1, p.UserID) + assert.NoError(t, p.Authorize(authn.ActionPropose)) + assert.Zero(t, f.local.calls, "an accepted instance token must not reach the local store") +} + +// A narrow token still authenticates here — the resolver runs before the router +// and knows no action — and is refused by service.Propose, which is where the +// action is known and where the rule is spelled once for both write surfaces. +func TestPutWithAnInstanceTokenMissingTheProposeGrant(t *testing.T) { + f := newPlaneFixture(t, http.StatusNoContent, false) + f.writer.err = service.ErrForbidden + + rec := f.put(liveToken("spec:read")) + assert.Equal(t, http.StatusForbidden, rec.Code) + assert.ErrorIs(t, f.writer.got.Principal.Authorize(authn.ActionPropose), authn.ErrMissingGrant) +} + +// A revoked instance token is 401 at the door and never gets a second chance at +// the old one — the same secret is registered locally, so a fall-through would +// visibly succeed with a 201. +func TestPutWithARevokedInstanceTokenIs401AndDoesNotFallThrough(t *testing.T) { + f := newPlaneFixture(t, http.StatusNotFound, false) + tok := liveToken("spec:propose id:42") + f.local.add(tok, "shadow") + + rec := f.put(tok) + assert.Equal(t, http.StatusUnauthorized, rec.Code) + assert.Zero(t, f.local.calls, "a revoked instance token must not reach the local store") + assert.Empty(t, f.writer.got.Writes, "and must not reach the write plane") +} + +// An unreachable tokens.sr.ht is 503, never 401 and never a downgrade to the +// legacy plane: refusing every live instance token because a daemon that is +// deliberately off the hot path is restarting is the outcome the 503 exists to +// prevent. +func TestPutWithAnUnreachableDaemonIs503(t *testing.T) { + f := newPlaneFixture(t, http.StatusNoContent, true) + tok := liveToken("spec:propose id:42") + f.local.add(tok, "shadow") + + rec := f.put(tok) + assert.Equal(t, http.StatusServiceUnavailable, rec.Code) + assert.Zero(t, f.local.calls) + assert.Empty(t, f.writer.got.Writes) +} + +// A token from another issuer — a meta.sr.ht PAT — is not this plane's to +// refuse, so it falls through to the local store, which does not know it. +func TestPutWithAForeignTokenFallsThroughAndIsRefusedLocally(t *testing.T) { + f := newPlaneFixture(t, http.StatusNoContent, false) + pat := &auth.BearerToken{ + Version: auth.TokenVersion, + Expires: auth.ToTimestamp(time.Now().Add(time.Hour)), + Grants: "git.sr.ht/OBJECTS:RW", + ClientID: "meta.sr.ht", + Username: "bigbes", + } + + rec := f.put(pat.Encode()) + assert.Equal(t, http.StatusUnauthorized, rec.Code) + assert.Equal(t, 1, f.local.calls, "the local plane is what refuses a foreign token") +} diff --git a/authn/bearer.go b/authn/bearer.go new file mode 100644 index 0000000000000000000000000000000000000000..f1a5696665ac72a0910221b715e8bf84b01efba7 --- /dev/null +++ b/authn/bearer.go @@ -0,0 +1,198 @@ +package authn + +import ( + "context" + "errors" + "fmt" + "net/http" + "strconv" + "strings" + + "sourcecraft.dev/bigbes/sr-ht-ecore/bearer" +) + +// The grant vocabulary spec.sr.ht declares for tokens.sr.ht working tokens. +// +// The daemon that mints them does not know these strings and must not: the +// tokens.sr.ht spec gives the vocabulary to the services, so that adding an +// action to spec.sr.ht is a change to spec.sr.ht. An unknown grant simply +// admits nobody. They are constants rather than literals at the check because a +// grant is compared byte for byte — a typo in one of the two places it is +// spelled is a silent widening or a silent refusal, and neither shows up until +// it matters. +const ( + // ActionPropose is what an instance token must carry to write: open a + // proposal or add documents to one. + ActionPropose = "spec:propose" + + // ActionRead is what an instance token must carry to read content through + // any of the read surfaces (the web UI, /query, MCP). + ActionRead = "spec:read" +) + +// BearerValidator is the sliver of sr-ht-ecore's bearer.Validator this package +// needs: who a presented working token belongs to, what it permits, and whether +// it is still live. +// +// Inspect and not Validate, because the resolver runs in middleware upstream of +// the router and so does not know which action is being attempted. The grant +// check happens where the action is known — service.Propose for the write +// plane, the read gates for the read plane — through Principal.Authorize. +// +// It is an interface rather than a *bearer.Validator so that this package stays +// testable without a tokens.sr.ht to talk to, exactly as TokenStore keeps it +// testable without a Postgres. +type BearerValidator interface { + Inspect(ctx context.Context, presented string) (*bearer.Token, error) +} + +// InstanceUser is the local "user" row that the owner of an instance token +// resolves to. It is the whole of what this package needs from that row: the id +// other layers key user-scoped state by, and the name it was found under. +type InstanceUser struct { + ID int + Username string +} + +// UserLookup resolves the meta.sr.ht username an instance token names into this +// service's local user row — core-go's auth.LookupUser in production. +// +// It is declared here for the same reason TokenStore is: that function reads a +// database handle and a config out of the context and panics without either, +// which is service/'s business to supply and not something a package answering +// "who is making this request?" should carry. service/ wires the real one in; +// tests wire a map. +type UserLookup interface { + LookupUser(ctx context.Context, username string) (InstanceUser, error) +} + +// resolveInstanceToken runs the tokens.sr.ht plane against a presented bearer +// credential. +// +// The middle result says whether the caller should fall back to spec's own +// agent-token plane. Exactly two refusals fall through, and which two is the +// only interesting decision in this function: +// +// - bearer.ErrInvalid — the string did not decode as a token this instance +// sealed. spec's local token is 32 random bytes in base64, which is +// precisely what that looks like. +// - bearer.ErrNotOurs — a well-formed token from another issuer (a meta.sr.ht +// PAT). spec accepts no such credential, but it is not this plane's to +// refuse, and falling through costs one hash lookup that will miss. +// +// spec's local token carries no prefix to discriminate on — unlike bench's and +// cover's — so there is no shape test that could route a request to the right +// plane up front. Trying the instance plane first and falling back on those two +// sentinels is what replaces it. +// +// Every other refusal is terminal and must never reach the old door: +// +// - bearer.ErrRevoked — the credential was withdrawn. Letting a revoked +// instance token be re-tried as a local one would answer "unknown token" for +// a token an operator deliberately killed, and would mean revocation has a +// second door to be checked at. +// - bearer.ErrForbidden — cannot arise from Inspect, which is given no action, +// but is terminal for the same reason: the credential is good. +// - bearer.ErrUnavailable — tokens.sr.ht could not be asked. Degrading to the +// legacy plane when the daemon is unreachable is exactly the silent +// downgrade the 503 of StatusFor exists to prevent. +func (rs *Resolver) resolveInstanceToken( + ctx context.Context, r *http.Request, presented string, +) (Principal, bool, error) { + tok, err := rs.bearer.Inspect(ctx, presented) + switch { + case err == nil: + // fall through + case errors.Is(err, bearer.ErrInvalid), errors.Is(err, bearer.ErrNotOurs): + return Anonymous(), true, nil + default: + return Anonymous(), false, fmt.Errorf("authn: instance token: %w", err) + } + + // The token names a meta.sr.ht account, and spec.sr.ht has exactly one that + // means anything. This is the same rule the cookie plane already applies — + // a real user who is not the instance owner reads as nobody — and applying + // it here keeps every consumer of Principal.Owner honest: the provenance + // committer, the refs rule's principal kind and coreauth's AuthContext all + // assume the human an agent acts for is the instance owner, and a foreign + // name would make each of them quietly wrong in a different way. + // + // It is a refusal rather than a downgrade to anonymous because a presented + // credential that fails must fail at the door: the asymmetry this package's + // doc comment draws between cookies and bearer tokens. + username := strings.TrimPrefix(tok.Username, "~") + if username != rs.owner { + return Anonymous(), false, fmt.Errorf( + "%w: the token belongs to ~%s, and this instance answers only to ~%s", + ErrNotInstanceOwner, username, rs.owner) + } + + // The owner is resolved to a local row even though single-user spec could + // infer it: the row id is what user-scoped state keys off, and looking it up + // here is what makes the instance plane's identity a fact about this + // database rather than a name copied out of a signed blob. + user, err := rs.users.LookupUser(ctx, username) + if err != nil { + // Unclassified, therefore transient, therefore 503: a database that + // cannot answer must never read as a bad credential. + return Anonymous(), false, fmt.Errorf("authn: resolve instance token owner ~%s: %w", username, err) + } + + return Principal{ + Kind: KindAgent, + Owner: rs.owner, + Agent: strings.TrimSpace(r.Header.Get(HeaderAgent)), + Session: strings.TrimSpace(r.Header.Get(HeaderAgentSession)), + TokenName: instanceTokenLabel(tok), + Plane: PlaneInstance, + Grants: tok.Grants, + UserID: user.ID, + }, false, nil +} + +// instanceTokenLabel names the credential in a log line. A registered token has +// a row at tokens.sr.ht an operator can find and revoke, so its id is the useful +// thing to print; a stateless one was never written down, and saying so is more +// honest than printing "0". +func instanceTokenLabel(tok *bearer.Token) string { + if tok.Registered() { + return "tokens.sr.ht #" + strconv.Itoa(tok.TokenID) + } + return "tokens.sr.ht (stateless)" +} + +// 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: +// +// - 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 +// 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 — unknown, revoked, +// malformed, on either plane — is 401. +// - Everything else is transient by definition and answers 503, which is the +// fail-closed direction: a backend outage never reads as a valid credential. +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): + return http.StatusForbidden + case IsAuthFailure(err): + return http.StatusUnauthorized + default: + return http.StatusServiceUnavailable + } +} diff --git a/authn/bearer_test.go b/authn/bearer_test.go new file mode 100644 index 0000000000000000000000000000000000000000..fd689b1fa66abba8f9fa010cff18c104e1f8fdd3 --- /dev/null +++ b/authn/bearer_test.go @@ -0,0 +1,447 @@ +package authn + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sourcecraft.dev/bigbes/sr-ht-core/auth" + "sourcecraft.dev/bigbes/sr-ht-ecore/bearer" + "sourcecraft.dev/bigbes/sr-ht-ecore/grants" +) + +// These tests run against the real sr-ht-ecore validator over real signed +// tokens, not a stub of it. The signature, the expiry, the ClientID +// discrimination and the revocation round trip are the whole of what this plane +// is, and a fake Inspect would assert only that the wiring calls something. +// crypto.InitCrypto has already run in TestMain, which is what makes minting one +// here possible at all. + +// mustGrants parses a grant string or fails the test. +func mustGrants(t *testing.T, s string) grants.Grants { + t.Helper() + g, err := grants.Parse(s) + require.NoError(t, err, "parse grants %q", s) + return g +} + +// seal mints a signed bearer token the way tokens.sr.ht does — or, with another +// clientID, the way meta.sr.ht does its PATs. +func seal(username, clientID, grantString string, expires time.Time) string { + bt := &auth.BearerToken{ + Version: auth.TokenVersion, + Expires: auth.ToTimestamp(expires), + Grants: grantString, + ClientID: clientID, + Username: username, + } + return bt.Encode() +} + +// instanceToken is a live working token from this instance's tokens.sr.ht. +func instanceToken(grantString string) string { + return seal("bigbes", bearer.TokensClientID, grantString, time.Now().Add(time.Hour)) +} + +// fakeDaemon stands in for tokens.sr.ht's revocation endpoint: 204 is live, 404 +// is revoked, and anything else is the absence of an answer. It counts requests +// so a test can assert whether the daemon was asked at all. +type fakeDaemon struct { + server *httptest.Server + status int + hits int +} + +func newFakeDaemon(t *testing.T, status int) *fakeDaemon { + t.Helper() + d := &fakeDaemon{status: status} + d.server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + d.hits++ + w.WriteHeader(d.status) + })) + t.Cleanup(d.server.Close) + return d +} + +// unreachableOrigin is a URL nothing answers on: a fake daemon that has already +// been shut down, which is what a restarting tokens.sr.ht looks like from here. +func unreachableOrigin(t *testing.T) string { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + origin := srv.URL + srv.Close() + return origin +} + +// validatorFor builds the real ecore validator pointed at origin. +func validatorFor(t *testing.T, origin string) *bearer.Validator { + t.Helper() + v, err := bearer.New(bearer.Options{ + Origin: origin, + ClientID: ConfigSection, + NodeID: "spec-test", + }) + require.NoError(t, err) + return v +} + +// stubUsers resolves usernames to fixed rows, and can be told to fail so the +// transient path is exercised. +type stubUsers struct { + rows map[string]InstanceUser + err error + calls int +} + +func newStubUsers() *stubUsers { + return &stubUsers{rows: map[string]InstanceUser{"bigbes": {ID: 1, Username: "bigbes"}}} +} + +func (s *stubUsers) LookupUser(_ context.Context, username string) (InstanceUser, error) { + s.calls++ + if s.err != nil { + return InstanceUser{}, s.err + } + row, ok := s.rows[username] + if !ok { + return InstanceUser{}, errNoSuchUser(username) + } + return row, nil +} + +func errNoSuchUser(username string) error { + return &noSuchUserError{username: username} +} + +type noSuchUserError struct{ username string } + +func (e *noSuchUserError) Error() string { return "no such user " + e.username } + +// planeFixture wires a resolver with both planes: the real validator against +// daemonStatus, and the local agent-token store the old credential lives in. +type planeFixture struct { + rs *Resolver + store *stubStore + users *stubUsers + daemon *fakeDaemon +} + +func newPlaneFixture(t *testing.T, daemonStatus int) *planeFixture { + t.Helper() + d := newFakeDaemon(t, daemonStatus) + f := &planeFixture{store: newStubStore(), users: newStubUsers(), daemon: d} + rs, err := NewResolver("bigbes", f.store, WithInstancePlane(validatorFor(t, d.server.URL), f.users)) + require.NoError(t, err) + f.rs = rs + return f +} + +// bearerRequest builds a request presenting token with full provenance headers. +func bearerRequest(token string) *http.Request { + return request("", map[string]string{ + "Authorization": "Bearer " + token, + HeaderAgent: "claude-code/spec-writer", + HeaderAgentSession: "8fb9c9a4-b078-4af1-89eb-d97c522f9921", + }) +} + +// The most important test in this change: the credential every agent on the +// instance is configured with today still authenticates, still resolves to an +// agent, and is still authorized to propose — with the instance plane wired in +// front of it. +func TestResolve_LocalAgentTokenStillWorksWithTheInstancePlaneWired(t *testing.T) { + f := newPlaneFixture(t, http.StatusNoContent) + f.store.add("live-token", "laptop") + + p, err := f.rs.Resolve(context.Background(), bearerRequest("live-token")) + require.NoError(t, err) + + assert.True(t, p.IsAgent(), "the old agent token must still resolve to an agent") + assert.Equal(t, PlaneLocal, p.Plane) + assert.Equal(t, "bigbes", p.Owner) + assert.Equal(t, "laptop", p.TokenName) + assert.Equal(t, "claude-code/spec-writer", p.Agent) + assert.Equal(t, "8fb9c9a4-b078-4af1-89eb-d97c522f9921", p.Session) + + // It carries no grants and is refused nothing: the local plane's boundary is + // the refs rule, and this change does not move it. + assert.NoError(t, p.Authorize(ActionPropose)) + assert.NoError(t, p.Authorize(ActionRead)) + + // It never became a question for tokens.sr.ht, and never could: the daemon + // is only asked about a token that decoded as one of its own. + assert.Zero(t, f.daemon.hits, "the local plane must not talk to tokens.sr.ht") + assert.Zero(t, f.users.calls, "the local plane has no owner to resolve") +} + +func TestResolve_InstanceTokenAccepted(t *testing.T) { + f := newPlaneFixture(t, http.StatusNoContent) + tok := instanceToken("spec:propose spec:read") + + p, err := f.rs.Resolve(context.Background(), bearerRequest(tok)) + require.NoError(t, err) + + assert.True(t, p.IsAgent()) + assert.Equal(t, PlaneInstance, p.Plane) + assert.Equal(t, "bigbes", p.Owner, "the agent still acts for the instance owner") + assert.Equal(t, 1, p.UserID, "the token's owner was resolved to a local row") + assert.Equal(t, "tokens.sr.ht (stateless)", p.TokenName) + assert.NoError(t, p.Authorize(ActionPropose)) + assert.NoError(t, p.Authorize(ActionRead)) + + // Provenance is read off the headers on this plane exactly as on the other. + assert.Equal(t, "claude-code/spec-writer", p.Agent) + assert.Equal(t, "8fb9c9a4-b078-4af1-89eb-d97c522f9921", p.Session) + + // A stateless token has no row, so step 4 costs nothing. + assert.Zero(t, f.daemon.hits) + assert.Zero(t, f.store.calls, "an accepted instance token must not reach the local store") +} + +func TestResolve_InstanceTokenMissingAGrantStillAuthenticates(t *testing.T) { + f := newPlaneFixture(t, http.StatusNoContent) + + // The resolver knows no action, so a narrow token authenticates here and is + // refused later, where the action is known. + p, err := f.rs.Resolve(context.Background(), bearerRequest(instanceToken("spec:read"))) + require.NoError(t, err) + assert.True(t, p.IsAgent()) + assert.NoError(t, p.Authorize(ActionRead)) + assert.ErrorIs(t, p.Authorize(ActionPropose), ErrMissingGrant) + assert.Equal(t, http.StatusForbidden, StatusFor(p.Authorize(ActionPropose))) +} + +// A revoked instance token must be refused outright and must not get a second +// chance at the old door. The same string is registered as a local agent token +// so that a fall-through would visibly succeed. +func TestResolve_RevokedInstanceTokenDoesNotFallThroughToTheLocalPlane(t *testing.T) { + f := newPlaneFixture(t, http.StatusNotFound) + tok := instanceToken("spec:propose id:42") + f.store.add(tok, "shadow") + + p, err := f.rs.Resolve(context.Background(), bearerRequest(tok)) + require.Error(t, err) + assert.ErrorIs(t, err, bearer.ErrRevoked) + assert.True(t, p.IsAnonymous()) + assert.Equal(t, http.StatusUnauthorized, StatusFor(err)) + assert.True(t, IsAuthFailure(err), "a revoked token is a permanent credential failure") + assert.Equal(t, 1, f.daemon.hits, "a registered token is checked against the daemon") + assert.Zero(t, f.store.calls, "a revoked instance token must never reach the local store") + + _, code, reached := runMiddleware(t, f.rs, bearerRequest(tok)) + assert.False(t, reached) + assert.Equal(t, http.StatusUnauthorized, code) +} + +// An unreachable tokens.sr.ht is 503 and never 401, and never a silent +// downgrade to the legacy plane. Reading "I could not ask" as "revoked" would +// refuse every live instance token while a daemon that is deliberately off the +// hot path restarts. +func TestResolve_UnreachableDaemonIs503AndDoesNotFallThrough(t *testing.T) { + store := newStubStore() + users := newStubUsers() + rs, err := NewResolver("bigbes", store, + WithInstancePlane(validatorFor(t, unreachableOrigin(t)), users)) + require.NoError(t, err) + + tok := instanceToken("spec:propose id:42") + store.add(tok, "shadow") + + p, err := rs.Resolve(context.Background(), bearerRequest(tok)) + require.Error(t, err) + assert.ErrorIs(t, err, bearer.ErrUnavailable) + assert.True(t, p.IsAnonymous()) + assert.Equal(t, http.StatusServiceUnavailable, StatusFor(err)) + assert.False(t, IsAuthFailure(err), "an unreachable daemon is not a bad credential") + assert.Zero(t, store.calls, "an unanswerable revocation must not fall back to the local store") + + _, code, reached := runMiddleware(t, rs, bearerRequest(tok)) + assert.False(t, reached) + assert.Equal(t, http.StatusServiceUnavailable, code) +} + +// The two refusals that do fall through. spec's local token has no prefix to +// discriminate on, so "did not decode as one of ours" is exactly what it looks +// like — which is why the order is instance-plane-first with a fallback rather +// than a shape test. +func TestResolve_ForeignAndUndecodableTokensFallThroughToTheLocalPlane(t *testing.T) { + metaPAT := seal("bigbes", "meta.sr.ht", "git.sr.ht/OBJECTS:RW", time.Now().Add(time.Hour)) + + for name, presented := range map[string]string{ + "opaque local secret": "live-token", + "expired instance token": seal("bigbes", bearer.TokensClientID, "spec:propose", + time.Now().Add(-time.Hour)), + "meta.sr.ht PAT": metaPAT, + } { + t.Run(name, func(t *testing.T) { + t.Run("registered locally", func(t *testing.T) { + f := newPlaneFixture(t, http.StatusNoContent) + f.store.add(presented, "laptop") + + p, err := f.rs.Resolve(context.Background(), bearerRequest(presented)) + require.NoError(t, err) + assert.True(t, p.IsAgent()) + assert.Equal(t, PlaneLocal, p.Plane) + assert.Equal(t, 1, f.store.calls) + }) + + t.Run("not registered", func(t *testing.T) { + f := newPlaneFixture(t, http.StatusNoContent) + + _, err := f.rs.Resolve(context.Background(), bearerRequest(presented)) + assert.ErrorIs(t, err, ErrUnknownToken, + "the local plane must be the one that refuses it") + assert.Equal(t, http.StatusUnauthorized, StatusFor(err)) + }) + }) + } +} + +// spec.sr.ht answers to one human. A working token belonging to somebody else is +// refused rather than admitted as a second identity: Principal.Owner is read by +// the provenance committer, the refs rule and the coreauth bridge, all of which +// are written for the instance owner. +func TestResolve_InstanceTokenOfAnotherOwnerIsRefused(t *testing.T) { + f := newPlaneFixture(t, http.StatusNoContent) + tok := seal("someone", bearer.TokensClientID, "spec:propose", time.Now().Add(time.Hour)) + + p, err := f.rs.Resolve(context.Background(), bearerRequest(tok)) + require.Error(t, err) + assert.ErrorIs(t, err, ErrNotInstanceOwner) + assert.True(t, p.IsAnonymous()) + assert.Equal(t, http.StatusForbidden, StatusFor(err)) + assert.Zero(t, f.users.calls, "a foreign owner is refused before any lookup") + assert.Zero(t, f.store.calls, "and never falls through to the local plane") + + _, code, reached := runMiddleware(t, f.rs, bearerRequest(tok)) + assert.False(t, reached) + assert.Equal(t, http.StatusForbidden, code) +} + +// A user lookup that cannot answer is transient: 503, never a bad credential. +func TestResolve_UserLookupFailureIs503(t *testing.T) { + f := newPlaneFixture(t, http.StatusNoContent) + f.users.err = errNoSuchUser("connection refused") + + _, err := f.rs.Resolve(context.Background(), bearerRequest(instanceToken("spec:read"))) + require.Error(t, err) + assert.False(t, IsAuthFailure(err)) + assert.Equal(t, http.StatusServiceUnavailable, StatusFor(err)) +} + +// An instance with no [tokens.sr.ht] section builds no instance plane, starts, +// and serves its local agent token exactly as before. +func TestResolve_WithoutTheInstancePlaneOnlyTheLocalOneExists(t *testing.T) { + store := newStubStore() + rs, err := NewResolver("bigbes", store) + require.NoError(t, err) + assert.False(t, rs.HasInstancePlane()) + + store.add("live-token", "laptop") + p, err := rs.Resolve(context.Background(), bearerRequest("live-token")) + require.NoError(t, err) + assert.True(t, p.IsAgent()) + assert.Equal(t, PlaneLocal, p.Plane) + + // A perfectly good instance token is just an unknown secret here — there is + // nothing on this instance that could validate it. + _, err = rs.Resolve(context.Background(), bearerRequest(instanceToken("spec:propose"))) + assert.ErrorIs(t, err, ErrUnknownToken) +} + +func TestWithInstancePlane_RejectsHalfWiring(t *testing.T) { + v := validatorFor(t, "https://tokens.example") + _, err := NewResolver("bigbes", newStubStore(), WithInstancePlane(nil, newStubUsers())) + assert.Error(t, err, "a plane with no validator must be refused") + _, err = NewResolver("bigbes", newStubStore(), WithInstancePlane(v, nil)) + assert.Error(t, err, "a plane with no user lookup must be refused") +} + +// Provenance is mandatory on every agent write, on both planes. Grants do not +// replace it and do not excuse it. +func TestAgentWriteFor_ProvenanceRequiredOnBothPlanes(t *testing.T) { + base := "1f0c1d1a1e2b3c4d5e6f708192a3b4c5d6e7f809" + for _, plane := range []Plane{PlaneLocal, PlaneInstance} { + t.Run(string(plane), func(t *testing.T) { + complete := Principal{ + Kind: KindAgent, Owner: "bigbes", Agent: "a", Session: "s-1", + Plane: plane, Grants: mustGrants(t, "*"), + } + _, err := complete.AgentWriteFor(base) + require.NoError(t, err) + + noSession := complete + noSession.Session = "" + _, err = noSession.AgentWriteFor(base) + assert.ErrorIs(t, err, ErrMissingProvenance) + + noAgent := complete + noAgent.Agent = "" + _, err = noAgent.AgentWriteFor(base) + assert.ErrorIs(t, err, ErrMissingProvenance) + }) + } +} + +func TestAuthorize(t *testing.T) { + t.Run("off the instance plane every action passes", func(t *testing.T) { + for _, p := range []Principal{ + {Kind: KindOwner, Owner: "bigbes"}, + {Kind: KindAgent, Owner: "bigbes", Plane: PlaneLocal}, + {Kind: KindAgent, Owner: "bigbes"}, // an unset plane is the local one + } { + assert.NoError(t, p.Authorize(ActionPropose)) + assert.NoError(t, p.Authorize(ActionRead)) + } + }) + + t.Run("on the instance plane the grant set decides", func(t *testing.T) { + narrow := Principal{ + Kind: KindAgent, Owner: "bigbes", Plane: PlaneInstance, + Grants: mustGrants(t, "spec:read"), + } + assert.NoError(t, narrow.Authorize(ActionRead)) + assert.ErrorIs(t, narrow.Authorize(ActionPropose), ErrMissingGrant) + + universal := Principal{ + Kind: KindAgent, Owner: "bigbes", Plane: PlaneInstance, + Grants: mustGrants(t, "*"), + } + assert.NoError(t, universal.Authorize(ActionPropose)) + + // The zero grant set admits nothing, which is why Authorize checks the + // plane before the set: a principal that never went through the + // resolver must not be silently universal. + empty := Principal{Kind: KindAgent, Owner: "bigbes", Plane: PlaneInstance} + assert.ErrorIs(t, empty.Authorize(ActionRead), ErrMissingGrant) + }) +} + +func TestStatusFor(t *testing.T) { + cases := []struct { + name string + err error + want int + }{ + {"nil", nil, http.StatusOK}, + {"unreachable daemon", bearer.ErrUnavailable, http.StatusServiceUnavailable}, + {"missing grant", ErrMissingGrant, http.StatusForbidden}, + {"foreign owner", ErrNotInstanceOwner, http.StatusForbidden}, + {"bearer forbidden", bearer.ErrForbidden, http.StatusForbidden}, + {"revoked instance token", bearer.ErrRevoked, http.StatusUnauthorized}, + {"undecodable instance token", bearer.ErrInvalid, http.StatusUnauthorized}, + {"unknown local token", ErrUnknownToken, http.StatusUnauthorized}, + {"revoked local token", ErrRevokedToken, http.StatusUnauthorized}, + {"store outage", errNoSuchUser("postgres"), http.StatusServiceUnavailable}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + assert.Equal(t, c.want, StatusFor(c.err)) + }) + } +} diff --git a/authn/doc.go b/authn/doc.go index 0d2cd365f1a03f09e02fa47fea125e87638bcedd..70dd7755b50d280c6e31f5d49d0ba2eb24951a9b 100644 --- a/authn/doc.go +++ b/authn/doc.go @@ -6,16 +6,47 @@ // The design has exactly two principals that carry authority: // // - the human owner, recognised by the shared `sr.ht.unified-login.v1` // cookie carrying the instance's [sr.ht] owner-name; -// - an agent, recognised by an opaque bearer token stored (hashed) in the -// agent_token table. +// - an agent, recognised by a bearer token. // // Everything else is anonymous. Single-user does not mean "no authorization"; // it relocates it onto agents, which is why Principal distinguishes those two -// and nothing finer. Per the confirmed v1 decision there is one agent token and -// no per-space scoping: the boundary that matters is the refs rule (agents may -// only write proposals/*), and that lives in gitx. +// and nothing finer. The boundary that bounds an agent's damage is the refs rule +// (agents may only write proposals/*), and that lives in gitx; nothing here +// replaces it. +// +// # Two agent credential planes +// +// An agent is recognised on either of two planes, and Principal.Plane says +// which: +// +// - PlaneLocal — spec's own agent_token row: one instance-wide shared secret, +// hashed at rest, with no owner, no expiry and no grants. This is v1's +// credential and it keeps working exactly as it did. +// - PlaneInstance — a tokens.sr.ht working token: signed by the instance, +// expiring, owned by a meta.sr.ht account, and carrying a grant set +// (ActionPropose, ActionRead). It is validated by sr-ht-ecore's bearer +// package and is present only when the instance config has a +// [tokens.sr.ht] section; where it does not, the plane is absent and the +// local one is the only door, which is a supported configuration. +// +// The instance plane is tried first and falls back to the local one on exactly +// two refusals — see Resolver.resolveInstanceToken, where the reasoning lives. +// +// The two planes are not interchangeable, and the difference this package has to +// carry is that only one of them has an owner. The local token is a secret with +// no user behind it; an instance token names one. Principal.Owner therefore +// keeps meaning "the human this agent acts for", which on this single-owner +// instance is always [sr.ht] owner-name — a token belonging to anybody else is +// refused rather than admitted as a second identity, because every consumer of +// that field (the provenance committer, the refs rule's principal kind, the +// coreauth AuthContext) is written for one human. // -// The two credentials are deliberately asymmetric: +// Grants are orthogonal to the refs rule and to provenance, and replace neither. +// A grant says what an instance token was minted for; the refs rule still says +// where an agent may point a ref, and provenance is still mandatory on every +// agent write, on both planes. +// +// The cookie and the bearer planes are deliberately asymmetric: // // - A cookie that is missing, forged, expired or unreadable yields an // anonymous principal and never an error. Browsing must keep working. @@ -40,12 +71,20 @@ // A write missing either field is rejected rather than defaulted: a commit // stamped with a guessed session is worse than no commit, because it launders // unattributable output as attributed. // -// This package owns no storage. The agent_token table lives in db/, which is -// injected through the TokenStore interface declared here — authn never imports -// db, so the dependency arrow keeps pointing downward. +// This package owns no storage and opens no connections. The agent_token table +// lives in db/, injected through the TokenStore interface declared here; the +// "user" row an instance token's owner resolves to is reached through +// UserLookup, and the tokens.sr.ht validator through BearerValidator. authn +// never imports db and never calls core-go's auth.LookupUser itself, so the +// dependency arrow keeps pointing downward and the whole package stays testable +// with no Postgres and no daemon to talk to. package authn -import "errors" +import ( + "errors" + + "sourcecraft.dev/bigbes/sr-ht-ecore/bearer" +) // Sentinel errors. Callers compare with errors.Is. The split that matters is // permanent (the credential is bad — 401/403) versus transient (the store could @@ -89,6 +128,22 @@ // ErrMissingConfig is returned by InstanceFromConfig when the instance // config lacks a key the provenance identities are built from. It is a // startup failure, not a request failure. ErrMissingConfig = errors.New("missing instance config key") + + // ErrNotInstanceOwner marks a valid tokens.sr.ht working token whose owner is + // somebody other than the instance owner. 403: the credential verifies and + // the holder is who they say they are, there is simply nothing on this + // single-owner instance to grant them. Deliberately not an IsAuthFailure — + // presenting it again will not help and neither will logging in. + ErrNotInstanceOwner = errors.New("token owner is not the instance owner") + + // ErrMissingGrant marks an instance token that authenticated fine but does + // not carry the action being attempted. 403, for the reason + // bearer.ErrForbidden is: what the holder needs is a wider grant, not another + // login. + // + // It is raised by Principal.Authorize, at the layer that knows the action — + // never by the resolver, which runs before the router and so knows none. + ErrMissingGrant = errors.New("token does not grant this action") ) // IsAuthFailure reports whether err is a permanent credential failure — the @@ -96,9 +151,17 @@ // caller should answer 401/403 — as opposed to a transient backend failure, // which should answer 503 and be retried. Everything not in this set is // transient by definition, which is the fail-closed direction: a store outage // never reads as a valid credential. +// +// The instance plane's two permanent refusals are in the set for the same +// reason the local plane's are: a signature that does not verify and a token +// tokens.sr.ht has withdrawn are both "this credential is bad", whichever door +// it was presented at. bearer.ErrUnavailable is pointedly absent — see +// StatusFor, which is what surfaces should map with. func IsAuthFailure(err error) bool { return errors.Is(err, ErrNoToken) || errors.Is(err, ErrUnknownToken) || errors.Is(err, ErrInvalidToken) || - errors.Is(err, ErrRevokedToken) + errors.Is(err, ErrRevokedToken) || + errors.Is(err, bearer.ErrInvalid) || + errors.Is(err, bearer.ErrRevoked) } diff --git a/authn/principal.go b/authn/principal.go index 78282955a185569b074ad11309acbc4652b80e75..1c4a6c142fe6e28d910af3f0feaadab2744abf70 100644 --- a/authn/principal.go +++ b/authn/principal.go @@ -3,6 +3,8 @@ import ( "context" "fmt" + + "sourcecraft.dev/bigbes/sr-ht-ecore/grants" ) // Kind enumerates the principals spec.sr.ht distinguishes. There are three @@ -23,19 +25,45 @@ // [sr.ht] owner-name. This is the principal whose git push *is* the // approval, and the only one that may approve a proposal. KindOwner Kind = "owner" - // KindAgent is a bot holding the agent bearer token. It may propose and it - // may read; the refs rule in gitx is what stops it touching the approved - // branch. + // KindAgent is a bot holding an agent bearer token, on either plane. It may + // propose and it may read; the refs rule in gitx is what stops it touching + // the approved branch. KindAgent Kind = "agent" ) +// Plane names the credential plane an agent authenticated on. +// +// It exists because the two are not interchangeable and a check that reads +// Grants has to know whether there were any to read: only the instance plane +// carries a grant set, and only it names an owner. Empty for every principal +// that is not an agent. +type Plane string + +const ( + // PlaneLocal is spec's own agent_token row: one instance-wide shared secret + // with no owner, no expiry and no grants. Its whole boundary is the refs + // rule, which is why v1 shipped it with mandatory provenance instead of + // scopes. + PlaneLocal Plane = "local" + + // PlaneInstance is a tokens.sr.ht working token: signed, expiring, owned by + // a meta.sr.ht account, and carrying the grant set Authorize checks. + PlaneInstance Plane = "instance" +) + // Principal is the resolved identity of a request. It is a value type with no // pointers into request state, so it can be stashed in a context, logged, and // passed to service/ without aliasing surprises. // // This is what the API layer and gitx's refs rule branch on, and it is // deliberately the narrowest thing that supports both: which kind, and — for an -// agent — the two provenance fields that every agent write must carry. +// agent — the two provenance fields that every agent write must carry, plus +// which credential plane it came in on and what that credential permits. +// +// It is not comparable with ==: Grants holds a set. Compare the fields that +// matter, or the String() rendering. The set itself is immutable once parsed — +// grants.Grants has no mutating method — so copies sharing it is not the +// aliasing this type's value semantics are guarding against. type Principal struct { // Kind is which of the three principals this is. The zero value is the // anonymous case, so a Principal read out of a context that never had one @@ -56,15 +84,31 @@ // Session is the agent's session ID, e.g. a UUID. KindAgent only, with the // same read/write asymmetry as Agent. Session string - // TokenName is the human-readable name of the agent_token row that - // authenticated this request. KindAgent only, diagnostics only — with one - // token and no scopes it grants nothing. + // TokenName names the credential that authenticated this request: the + // agent_token row's operator-facing label on the local plane, and the + // tokens.sr.ht row id (or "stateless") on the instance one. KindAgent only, + // diagnostics only — it grants nothing on either plane. TokenName string // CookieUser is whatever username the unified-login cookie carried, even // when that user was not the instance owner and Kind is therefore // KindAnonymous. Display and logging only: never an authorization input. CookieUser string + + // Plane is which credential plane authenticated an agent. Empty for every + // other kind. Authorize reads it to decide whether Grants means anything. + Plane Plane + + // Grants is what the instance token this request carried permits, parsed. + // PlaneInstance only; the zero value on every other plane, which grants + // nothing and is why Authorize checks Plane before it checks the set. + Grants grants.Grants + + // UserID is the id of the local "user" row the instance token's owner + // resolved to. PlaneInstance only, and zero on the local plane — which has + // no owner at all, the asymmetry between the two planes that everything + // reading this field has to respect. + UserID int } // Anonymous returns the principal for an unauthenticated request. @@ -91,6 +135,34 @@ // /query, the web UI, the MCP tools) must apply the identical policy: two read // surfaces with two spellings of it is how a corpus leaks. func (p Principal) CanRead() bool { return p.IsOwner() || p.IsAgent() } +// Authorize reports whether the credential behind this principal covers action +// — one of the ActionPropose / ActionRead constants. +// +// It is a grant check and nothing else. It says nothing about who the principal +// is, so every caller must already have made the identity decision (IsAgent for +// the write plane, CanRead for the read plane); calling this alone would +// "authorize" an anonymous request, because an anonymous request carries no +// instance token and so has no grant to be missing. The two questions are +// separate on purpose: the resolver answers identity in middleware, upstream of +// the router, and only the layer that knows the action can ask this one. +// +// Every plane but PlaneInstance passes, and that is the compatibility contract +// of this whole change. The local agent token has no grants to check and will +// not grow any: it is one instance-wide shared secret whose boundary is the refs +// rule in gitx, and inventing a grant vocabulary for it now would refuse an +// agent a permission its operator was never asked to give. The owner's cookie +// passes for the same reason — grants describe machine credentials, not people. +func (p Principal) Authorize(action string) error { + if p.Plane != PlaneInstance { + return nil + } + if !p.Grants.Has(action) { + return fmt.Errorf("%w: the instance token grants %q, which does not cover %q", + ErrMissingGrant, p.Grants.String(), action) + } + return nil +} + // String renders the principal for logs. It never includes the token name's // secret (there is none — the name is not the token) and never includes the // cookie value. @@ -107,7 +179,14 @@ session := p.Session if session == "" { session = "(no session)" } - return fmt.Sprintf("agent %s session %s for ~%s", agent, session, p.Owner) + line := fmt.Sprintf("agent %s session %s for ~%s", agent, session, p.Owner) + // Only the instance plane is annotated, so the line a local agent logs + // today reads the same tomorrow — and so that the annotation, when it + // does appear, means something rather than being noise on every line. + if p.Plane == PlaneInstance { + line += " (tokens.sr.ht: " + p.Grants.String() + ")" + } + return line default: if p.CookieUser != "" { return "anonymous (cookie user ~" + p.CookieUser + ")" diff --git a/authn/resolver.go b/authn/resolver.go index 09886491a770bb99c465cf39aa79ee02e5c21d19..e858647fa9e9f1182d48f1b8e916fccc17c1b796 100644 --- a/authn/resolver.go +++ b/authn/resolver.go @@ -11,11 +11,48 @@ "sourcecraft.dev/bigbes/sr-ht-spec/core" ) // Resolver turns a request into a Principal. It holds the instance owner -// username — the one name a cookie has to match to carry authority — and the -// TokenStore agents are checked against. +// username — the one name a cookie has to match to carry authority — the +// TokenStore local agent tokens are checked against, and, when the instance is +// configured for it, the tokens.sr.ht plane. type Resolver struct { owner string store TokenStore + + // bearer and users are the instance plane, installed by WithInstancePlane. + // Both are nil when the instance config has no [tokens.sr.ht] section, which + // is a supported configuration and not a broken one: the plane is absent and + // every bearer credential goes straight to the local store, exactly as it + // did before this plane existed. Resolve tests bearer for presence, and + // WithInstancePlane is what guarantees the two are wired together or not at + // all. + bearer BearerValidator + users UserLookup +} + +// ResolverOption configures a Resolver at construction. Options rather than a +// second constructor because the instance plane is optional in production and +// not merely in tests: an instance without tokens.sr.ht must build the same +// resolver every other caller does. +type ResolverOption func(*Resolver) error + +// WithInstancePlane wires the tokens.sr.ht bearer plane in: v validates a +// presented working token, users resolves its owner to a local row. +// +// Both are required together. A validator with no way to resolve an owner would +// authenticate a token and then have nothing to say about who presented it, +// which is the one thing the instance plane adds over the local one. +func WithInstancePlane(v BearerValidator, users UserLookup) ResolverOption { + return func(rs *Resolver) error { + if v == nil { + return fmt.Errorf("authn: nil BearerValidator") + } + if users == nil { + return fmt.Errorf("authn: nil UserLookup") + } + rs.bearer = v + rs.users = users + return nil + } } // NewResolver builds a Resolver for the instance owner named in @@ -25,7 +62,11 @@ // A nil store is rejected rather than tolerated: with no store every agent // token would resolve as unknown, which looks exactly like a mass revocation // and is a miserable thing to debug at 2am. Wire a store or do not build a // resolver. -func NewResolver(owner string, store TokenStore) (*Resolver, error) { +// +// With no options the resolver knows only the local agent-token plane — what an +// instance with no [tokens.sr.ht] section gets, and what every caller got before +// that plane existed. +func NewResolver(owner string, store TokenStore, opts ...ResolverOption) (*Resolver, error) { owner = strings.TrimPrefix(owner, "~") if err := core.ValidateOwner(owner); err != nil { return nil, fmt.Errorf("authn: instance owner: %w", err) @@ -33,9 +74,20 @@ } if store == nil { return nil, fmt.Errorf("authn: nil TokenStore") } - return &Resolver{owner: owner, store: store}, nil + rs := &Resolver{owner: owner, store: store} + for _, opt := range opts { + if err := opt(rs); err != nil { + return nil, err + } + } + return rs, nil } +// HasInstancePlane reports whether this resolver tries tokens.sr.ht before the +// local agent-token store. Startup logging and tests only; never an +// authorization input. +func (rs *Resolver) HasInstancePlane() bool { return rs.bearer != nil } + // Owner returns the instance owner username this resolver recognises. func (rs *Resolver) Owner() string { return rs.owner } @@ -47,6 +99,11 @@ // agent, and letting a stale browser cookie promote it to the owner would hand // it the approved branch. The two credentials are checked in that order and // never merged. // +// A presented bearer token is offered to the tokens.sr.ht plane first, when one +// is configured, and reaches the local agent-token store only if that plane +// says the string is not a token of the instance's. Which refusals mean that, +// and why the order is not the other way round, is in resolveInstanceToken. +// // The error contract is asymmetric on purpose: // // - No bearer token: never an error. The cookie decides between KindOwner and @@ -59,6 +116,12 @@ // are demanded at the write, by AgentWrite.Validate, which is the only place // the design requires them and the only place a missing one can do harm. func (rs *Resolver) Resolve(ctx context.Context, r *http.Request) (Principal, error) { if presented := BearerFromRequest(r); presented != "" { + if rs.bearer != nil { + p, fallBack, err := rs.resolveInstanceToken(ctx, r, presented) + if !fallBack { + return p, err + } + } tok, err := ResolveAgentToken(ctx, rs.store, presented) if err != nil { return Anonymous(), err @@ -69,6 +132,7 @@ Owner: rs.owner, Agent: strings.TrimSpace(r.Header.Get(HeaderAgent)), Session: strings.TrimSpace(r.Header.Get(HeaderAgentSession)), TokenName: tok.Name, + Plane: PlaneLocal, }, nil } @@ -89,27 +153,43 @@ // Middleware attaches the resolved Principal to the request context, where // PrincipalFromContext reads it. // -// It rejects only a failed bearer token — 401 for a bad credential, 503 for a -// store that could not answer. Everything else, including every cookie -// problem, flows through as anonymous: the read plane is anonymous-capable and -// must never answer an error page on identity grounds. +// It rejects only a failed bearer token — 401 for a bad credential, 403 for a +// good one that this instance has nothing to grant, 503 for a backend that could +// not answer, per StatusFor. Everything else, including every cookie problem, +// flows through as anonymous: the read plane is anonymous-capable and must never +// answer an error page on identity grounds. func (rs *Resolver) Middleware() func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { p, err := rs.Resolve(r.Context(), r) if err != nil { - if IsAuthFailure(err) { - http.Error(w, "invalid agent token", http.StatusUnauthorized) - return + status := StatusFor(err) + if status >= 500 { + // Fail closed and loudly. The alternative — degrading to + // anonymous — would turn a Postgres blip or an unreachable + // tokens.sr.ht into agents silently losing their write + // access. + log.Printf("authn: resolving bearer credential: %v", err) } - // Fail closed and loudly. The alternative — degrading to - // anonymous — would turn a Postgres blip into agents silently - // losing their write access. - log.Printf("authn: resolving agent token: %v", err) - http.Error(w, "authentication backend unavailable", http.StatusServiceUnavailable) + http.Error(w, resolveFailureMessage(status), status) return } next.ServeHTTP(w, r.WithContext(WithPrincipal(r.Context(), p))) }) } } + +// resolveFailureMessage is what a refused caller is told. It is keyed on the +// status and not on the error, so that nothing about which plane refused, whose +// token it was, or whether a row exists leaks to a caller holding a credential +// this service did not accept. +func resolveFailureMessage(status int) string { + switch status { + case http.StatusUnauthorized: + return "invalid agent token" + case http.StatusForbidden: + return "this token does not authorize requests to spec.sr.ht" + default: + return "authentication backend unavailable" + } +} diff --git a/authn/resolver_test.go b/authn/resolver_test.go index ab676d70d2a94b4d46075f8fbf0e9bd9b219165e..a4d5145178449c7fdbae62184f96290987f96bcd 100644 --- a/authn/resolver_test.go +++ b/authn/resolver_test.go @@ -5,6 +5,7 @@ "context" "errors" "net/http" "net/http/httptest" + "reflect" "testing" ) @@ -295,9 +296,15 @@ t.Fatalf("Kind = %q, want %q", p.Kind, KindAnonymous) } } +// Principal is not comparable with == — it carries a grant set — so the round +// trip is asserted with reflect.DeepEqual, which also covers the grants. func TestPrincipalFromContext_RoundTrip(t *testing.T) { - want := Principal{Kind: KindAgent, Owner: "bigbes", Agent: "a", Session: "s"} - if got := PrincipalFromContext(WithPrincipal(context.Background(), want)); got != want { + want := Principal{ + Kind: KindAgent, Owner: "bigbes", Agent: "a", Session: "s", + Plane: PlaneInstance, Grants: mustGrants(t, "spec:read spec:propose"), UserID: 3, + } + got := PrincipalFromContext(WithPrincipal(context.Background(), want)) + if !reflect.DeepEqual(got, want) { t.Fatalf("round trip = %+v, want %+v", got, want) } } @@ -315,16 +322,37 @@ } } func TestPrincipal_String(t *testing.T) { - cases := map[Principal]string{ - Anonymous(): "anonymous", - {Kind: KindAnonymous, CookieUser: "someone"}: "anonymous (cookie user ~someone)", - {Kind: KindOwner, Owner: "bigbes"}: "owner ~bigbes", - {Kind: KindAgent, Owner: "bigbes", Agent: "claude-code/spec-writer", Session: "s-1"}: "agent claude-code/spec-writer session s-1 for ~bigbes", - {Kind: KindAgent, Owner: "bigbes"}: "agent (unnamed) session (no session) for ~bigbes", + // A slice and not a map keyed by Principal: it carries a grant set and is + // therefore not comparable. + cases := []struct { + p Principal + want string + }{ + {Anonymous(), "anonymous"}, + {Principal{Kind: KindAnonymous, CookieUser: "someone"}, "anonymous (cookie user ~someone)"}, + {Principal{Kind: KindOwner, Owner: "bigbes"}, "owner ~bigbes"}, + { + Principal{Kind: KindAgent, Owner: "bigbes", Agent: "claude-code/spec-writer", Session: "s-1"}, + "agent claude-code/spec-writer session s-1 for ~bigbes", + }, + {Principal{Kind: KindAgent, Owner: "bigbes"}, "agent (unnamed) session (no session) for ~bigbes"}, + // The local plane renders exactly as it did before the instance plane + // existed: the annotation appears only where there is something to say. + { + Principal{Kind: KindAgent, Owner: "bigbes", Agent: "a", Session: "s-1", Plane: PlaneLocal}, + "agent a session s-1 for ~bigbes", + }, + { + Principal{ + Kind: KindAgent, Owner: "bigbes", Agent: "a", Session: "s-1", + Plane: PlaneInstance, Grants: mustGrants(t, "spec:propose"), + }, + "agent a session s-1 for ~bigbes (tokens.sr.ht: spec:propose)", + }, } - for p, want := range cases { - if got := p.String(); got != want { - t.Fatalf("String() = %q, want %q", got, want) + for _, c := range cases { + if got := c.p.String(); got != c.want { + t.Fatalf("String() = %q, want %q", got, c.want) } } } diff --git a/cmd/specsrht/main.go b/cmd/specsrht/main.go index 4afaa93515261a55685215ed494770c3cde7ef94..6122b8e7daee4ab338b82495ef39a814178608b7 100644 --- a/cmd/specsrht/main.go +++ b/cmd/specsrht/main.go @@ -269,10 +269,16 @@ return err } defer pool.Close() - svc, err := service.New(cfg, pool) + // WithInstanceTokens offers the daemon's resolver the tokens.sr.ht bearer + // plane. It is offered, not required: an instance whose config has no + // [tokens.sr.ht] section gets no such plane and keeps accepting its own + // agent token, which is a supported configuration and not a degraded one. + svc, err := service.New(cfg, pool, service.WithInstanceTokens(conf)) if err != nil { return err } + log.Info("agent credential planes", + "local", true, "tokens.sr.ht", svc.Resolver().HasInstancePlane()) // Seed the owner's user row before serving. core-go's auth.Middleware looks // a request's username up in the "user" table and, on a miss, calls out to diff --git a/go.mod b/go.mod index 65eccfd2c7bf940915d8c6fc0304223e2086834d..1179c310eb9e2ec17ca0e755736d8cb62b819ab9 100644 --- a/go.mod +++ b/go.mod @@ -22,6 +22,7 @@ github.com/yuin/goldmark v1.8.2 go.bigb.es/auxilia v0.5.0 gopkg.in/yaml.v3 v3.0.1 sourcecraft.dev/bigbes/sr-ht-core v0.0.0-20260718185800-dd418a200152 + sourcecraft.dev/bigbes/sr-ht-ecore v0.0.0-20260808143603-174115990895 ) require ( diff --git a/go.sum b/go.sum index dad85a5a02a2a38963d86166b25352a2ceedfc9c..6bad8c61703d683b5f8faad06ecefaf49b766e35 100644 --- a/go.sum +++ b/go.sum @@ -421,3 +421,5 @@ modernc.org/sqlite v1.38.2 h1:Aclu7+tgjgcQVShZqim41Bbw9Cho0y/7WzYptXqkEek= modernc.org/sqlite v1.38.2/go.mod h1:cPTJYSlgg3Sfg046yBShXENNtPrWrDX8bsbAQBzgQ5E= sourcecraft.dev/bigbes/sr-ht-core v0.0.0-20260718185800-dd418a200152 h1:9kQC+tDO2CO8avlKadb9Z0if4a6vJuEK80+4zcb6/fU= sourcecraft.dev/bigbes/sr-ht-core v0.0.0-20260718185800-dd418a200152/go.mod h1:Mu1Vx39ws/OTKWGoVERXvkdRSPLBdhuFTYv0ftVV31c= +sourcecraft.dev/bigbes/sr-ht-ecore v0.0.0-20260808143603-174115990895 h1:OGZrtBtMoXhyZGXrPqMzmrNQnStoCLBVuegGo7yF1Us= +sourcecraft.dev/bigbes/sr-ht-ecore v0.0.0-20260808143603-174115990895/go.mod h1:KeoZjm+/nnsdtc1WxB7X/0EeC+Rggt2OkwJDEc6XWnw= diff --git a/graph/grant_test.go b/graph/grant_test.go new file mode 100644 index 0000000000000000000000000000000000000000..c45984d9eba0cd23514efe927befefb21c1bcff4 --- /dev/null +++ b/graph/grant_test.go @@ -0,0 +1,92 @@ +package graph + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sourcecraft.dev/bigbes/sr-ht-ecore/grants" + + "sourcecraft.dev/bigbes/sr-ht-spec/authn" +) + +// mustGrants parses a grant string or fails the test. +func mustGrants(t *testing.T, s string) grants.Grants { + t.Helper() + g, err := grants.Parse(s) + require.NoError(t, err, "parse grants %q", s) + return g +} + +// The gate asks two questions, and they have different answers when they fail. +// Identity — may you read at all — is 401 and unchanged. The grant — was the +// credential you used minted for reading — is 403, and applies only to a +// tokens.sr.ht working token, because it is the only credential that carries +// grants: the owner's cookie and spec's own agent token pass it untouched, which +// is what keeps every client that works today working. +func TestGateChecksIdentityThenGrant(t *testing.T) { + cases := []struct { + name string + principal authn.Principal + wantCode int + wantNext bool + }{ + {"anonymous", authn.Anonymous(), http.StatusUnauthorized, false}, + {"owner cookie", authn.Principal{Kind: authn.KindOwner, Owner: "bigbes"}, http.StatusOK, true}, + { + "local agent token", + authn.Principal{Kind: authn.KindAgent, Owner: "bigbes", Plane: authn.PlaneLocal}, + http.StatusOK, true, + }, + { + "instance token with spec:read", + authn.Principal{ + Kind: authn.KindAgent, Owner: "bigbes", Plane: authn.PlaneInstance, + Grants: mustGrants(t, "spec:read"), + }, + http.StatusOK, true, + }, + { + "instance token without spec:read", + authn.Principal{ + Kind: authn.KindAgent, Owner: "bigbes", Plane: authn.PlaneInstance, + Grants: mustGrants(t, "spec:propose"), + }, + http.StatusForbidden, false, + }, + { + "instance token, universal grant", + authn.Principal{ + Kind: authn.KindAgent, Owner: "bigbes", Plane: authn.PlaneInstance, + Grants: mustGrants(t, "*"), + }, + http.StatusOK, true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var reached bool + next := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + }) + + req := httptest.NewRequest(http.MethodPost, "/query", nil) + req = req.WithContext(authn.WithPrincipal(req.Context(), tc.principal)) + rec := httptest.NewRecorder() + + gate(next).ServeHTTP(rec, req) + + assert.Equal(t, tc.wantCode, rec.Code) + assert.Equal(t, tc.wantNext, reached) + if tc.wantCode == http.StatusForbidden { + assert.Contains(t, rec.Body.String(), authn.ActionRead, + "a refused caller must be told which grant it lacks") + } + }) + } +} diff --git a/graph/server.go b/graph/server.go index 5df3e8ee2566f6b4b37c200ee7ba7b7fef59298c..280cd57b58daa49ebf7e4bdb13afe698db1c7104 100644 --- a/graph/server.go +++ b/graph/server.go @@ -150,14 +150,27 @@ // The ACL is authn.Principal.CanRead — the owner and its agents may read and // nobody else may — the same predicate web/ and mcpsrv/ apply, so the three read // surfaces cannot drift into three policies, which is how a corpus leaks. // -// The refusal is a 401 with a line of text and never a redirect to meta's -// login: every caller here is a machine, and handing a bot 200 and a page of -// login markup tells it nothing it can act on. +// A caller that clears it and authenticated with a tokens.sr.ht working token +// must also hold spec:read — the grant half of the same question, asked here +// because here is where the action ("read") is known. It is a no-op for the +// owner's cookie and for the local agent token, neither of which carries grants. +// +// The refusal is a 401 — or a 403 for the missing grant — with a line of text +// and never a redirect to meta's login: every caller here is a machine, and +// handing a bot 200 and a page of login markup tells it nothing it can act on. +// The two statuses stay apart because retrying is worth it for one and never for +// the other. func gate(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if !authn.PrincipalFromContext(r.Context()).CanRead() { + p := authn.PrincipalFromContext(r.Context()) + if !p.CanRead() { w.Header().Set("Content-Type", "text/plain; charset=utf-8") http.Error(w, "authentication required", http.StatusUnauthorized) + return + } + if err := p.Authorize(authn.ActionRead); err != nil { + w.Header().Set("Content-Type", "text/plain; charset=utf-8") + http.Error(w, "this token does not grant "+authn.ActionRead, http.StatusForbidden) return } next.ServeHTTP(w, r) diff --git a/hooks/server.go b/hooks/server.go index 41b45160fb44b44fda5bcdbbb303dcefcbdfa093..1fd43dcb18cf589e389f8c265748b938f964d3c0 100644 --- a/hooks/server.go +++ b/hooks/server.go @@ -479,12 +479,17 @@ s.log.Error("could not validate an agent token", "space", space.String(), "error", err) return authn.Principal{}, errorResponse( "spec.sr.ht could not check the agent token presented with this push: %v", err), false } + // The local plane, and only it: a push arrives over SSH with a token in + // a hook's environment, and this path checks it against agent_token and + // nothing else. A tokens.sr.ht working token is not accepted here — the + // two planes meet in authn.Resolver, which serves the HTTP surfaces. return authn.Principal{ Kind: authn.KindAgent, Owner: owner, Agent: cred.Agent, Session: cred.Session, TokenName: tok.Name, + Plane: authn.PlaneLocal, }, Response{}, true default: // Request.Validate rejected every other spelling already. diff --git a/mcpsrv/comment.go b/mcpsrv/comment.go index 65ee92726bb84d9dc8b26df169ac0543f461cd8b..b8ee287884c462b46cb7a73ab9587acfd9209395 100644 --- a/mcpsrv/comment.go +++ b/mcpsrv/comment.go @@ -93,6 +93,14 @@ Reply *commentAuthor `json:"reply,omitempty"` } func commentHandler(ctx context.Context, c Commenter, in commentInput) (commentOutput, error) { + // Both modes of this tool begin by reading the review conversation — the + // reply path lists the threads before it can answer one — so it is a read + // surface and carries the read grant. The grant vocabulary has no separate + // action for commenting, and inventing one here would put a word in the + // instance's dictionary that no token was ever minted with. + if err := requireRead(ctx); err != nil { + return commentOutput{}, err + } if in.Proposal <= 0 { return commentOutput{}, fmt.Errorf("proposal must name a proposal id") } diff --git a/mcpsrv/comment_internal_test.go b/mcpsrv/comment_internal_test.go index 126fd3024681dd6d293405c16000c4011b1c08a7..71e0b639088abac45c14040480366a5263be9fcd 100644 --- a/mcpsrv/comment_internal_test.go +++ b/mcpsrv/comment_internal_test.go @@ -206,7 +206,10 @@ out, err := commentHandler(ctx, c, commentInput{Proposal: 7, Thread: 2, Body: "reworded in the next push"}) if err != nil { t.Fatalf("commentHandler: %v", err) } - if c.sawPrincipal != principal { + // Principal is no longer comparable with == — it carries a grant set — so + // its rendering stands in: that names the kind, the agent, the session and + // the owner, which is everything this assertion is about. + if c.sawPrincipal.String() != principal.String() { t.Errorf("principal = %+v, want the one on the context %+v", c.sawPrincipal, principal) } if c.sawThreadID != 2 || c.sawBody != "reworded in the next push" { diff --git a/mcpsrv/grant_internal_test.go b/mcpsrv/grant_internal_test.go new file mode 100644 index 0000000000000000000000000000000000000000..0c22a954b8ceccf1865b8d409bbb91f34c308f4f --- /dev/null +++ b/mcpsrv/grant_internal_test.go @@ -0,0 +1,201 @@ +package mcpsrv + +import ( + "context" + "database/sql" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sourcecraft.dev/bigbes/sr-ht-ecore/grants" + + "sourcecraft.dev/bigbes/sr-ht-spec/authn" + "sourcecraft.dev/bigbes/sr-ht-spec/service" +) + +// mustGrants parses a grant string or fails the test. +func mustGrants(t *testing.T, s string) grants.Grants { + t.Helper() + g, err := grants.Parse(s) + require.NoError(t, err, "parse grants %q", s) + return g +} + +// instancePrincipal is agentPrincipal as a tokens.sr.ht working token resolves +// it: the same agent, with a grant set behind it. +func instancePrincipal(t *testing.T, grantString string) authn.Principal { + t.Helper() + p := agentPrincipal() + p.Plane = authn.PlaneInstance + p.Grants = mustGrants(t, grantString) + return p +} + +// localPrincipal is agentPrincipal as spec's own agent_token resolves it. +func localPrincipal() authn.Principal { + p := agentPrincipal() + p.Plane = authn.PlaneLocal + return p +} + +// Every tool that serves content asks for spec:read, and asks for it per tool +// rather than at the Gate — /mcp carries the write tools too, and a surface-wide +// read grant would refuse a propose-only token at `initialize`, before it named +// anything. +func TestReadToolsRequireTheReadGrant(t *testing.T) { + refused := instancePrincipal(t, "spec:propose") + + t.Run("spec_read", func(t *testing.T) { + ctx := authn.WithPrincipal(context.Background(), refused) + _, err := readHandler(ctx, Backend{}, readInput{Space: "~bigbes/rfcs", Document: "SPEC-0007"}) + assert.ErrorIs(t, err, authn.ErrMissingGrant) + }) + + t.Run("spec_list", func(t *testing.T) { + ctx := authn.WithPrincipal(context.Background(), refused) + _, err := listHandler(ctx, Backend{}, listInput{}) + assert.ErrorIs(t, err, authn.ErrMissingGrant) + }) + + t.Run("spec_search", func(t *testing.T) { + ctx := authn.WithPrincipal(context.Background(), refused) + _, err := searchHandler(ctx, Backend{}, searchInput{Query: "storage"}) + assert.ErrorIs(t, err, authn.ErrMissingGrant) + }) + + t.Run("spec_comment", func(t *testing.T) { + ctx := authn.WithPrincipal(context.Background(), refused) + _, err := commentHandler(ctx, commentFixture(t), commentInput{Proposal: 7}) + assert.ErrorIs(t, err, authn.ErrMissingGrant) + }) +} + +// And every credential that carries no grants passes untouched, which is what +// keeps the agents configured today working: the local agent token and the +// owner's cookie have nothing to check. +func TestReadToolsPassEveryUngrantedCredential(t *testing.T) { + for name, p := range map[string]authn.Principal{ + "local agent token": localPrincipal(), + "owner cookie": {Kind: authn.KindOwner, Owner: "bigbes"}, + "instance token, spec:read": instancePrincipal(t, "spec:read"), + "instance token, universal": instancePrincipal(t, "*"), + "instance token, both actions": instancePrincipal(t, "spec:read spec:propose"), + } { + t.Run(name, func(t *testing.T) { + ctx := authn.WithPrincipal(context.Background(), p) + assert.NoError(t, requireRead(ctx)) + + // Listing threads goes all the way through with a fake service. + out, err := commentHandler(ctx, commentFixture(t), commentInput{Proposal: 7}) + require.NoError(t, err) + assert.NotEmpty(t, out.Threads) + }) + } +} + +// realWriter builds an actual *service.Service over a database nothing answers +// on, so that spec_propose's grant check here is service.Propose's own and not a +// copy of it. Propose refuses a principal before it opens anything, which is +// what makes an unreachable pool enough. +func realWriter(t *testing.T) *service.Service { + t.Helper() + root := t.TempDir() + pool, err := sql.Open("postgres", + "postgres://nobody@127.0.0.1:1/nothing?sslmode=disable&connect_timeout=1") + require.NoError(t, err) + t.Cleanup(func() { pool.Close() }) + + svc, err := service.New(service.Config{ + Repos: filepath.Join(root, "repos"), + Cache: filepath.Join(root, "cache"), + Origin: "https://spec.srht.bigb.es", + ConnectionString: "postgres://nobody@127.0.0.1:1/nothing", + Instance: authn.Instance{ + OwnerName: "bigbes", + OwnerEmail: "bigbes@gmail.com", + AgentEmail: "agent@spec.srht.bigb.es", + }, + }, pool) + require.NoError(t, err) + return svc +} + +// proposeCall is one well-formed spec_propose, so the only thing a test varies +// is the credential behind it. +func proposeCall() proposeInput { + return proposeInput{ + Space: "~bigbes/rfcs", + IfMatch: "1f0c1d1a1e2b3c4d5e6f708192a3b4c5d6e7f809", + Title: "Add a note", + Message: "write it", + Documents: []proposeDoc{{Path: "notes/a.md", Content: "---\nid: N-1\n---\n"}}, + } +} + +// spec_propose asks for spec:propose, through the same service.Propose the REST +// surface calls — the rule is spelled once, below both write surfaces, and MCP +// gets it by going through it rather than by repeating it here. +func TestProposeToolRequiresTheProposeGrant(t *testing.T) { + svc := realWriter(t) + + for _, grantString := range []string{"spec:read", "bench:upload"} { + t.Run(grantString, func(t *testing.T) { + ctx := authn.WithPrincipal(context.Background(), instancePrincipal(t, grantString)) + _, err := proposeHandler(ctx, svc, proposeCall()) + require.Error(t, err) + assert.ErrorIs(t, err, service.ErrForbidden, + "the refusal is the one this surface already uses for an unauthorised call") + assert.ErrorIs(t, err, authn.ErrMissingGrant) + }) + } +} + +// And every credential that may propose clears it — the local agent token above +// all, which carries no grants and must keep working exactly as it does today. +func TestProposeToolAcceptsTheGrantedCredentials(t *testing.T) { + svc := realWriter(t) + + for name, p := range map[string]authn.Principal{ + "local agent token": localPrincipal(), + "instance token, spec:propose": instancePrincipal(t, "spec:propose"), + "instance token, universal": instancePrincipal(t, "*"), + } { + t.Run(name, func(t *testing.T) { + ctx := authn.WithPrincipal(context.Background(), p) + // The write cannot land — there is no space and no database — but it + // must not be turned away at the ACL. + _, err := proposeHandler(ctx, svc, proposeCall()) + require.Error(t, err, "the fixture has no space, so this cannot succeed") + assert.NotErrorIs(t, err, service.ErrForbidden, "%s must clear the write ACL", name) + assert.NotErrorIs(t, err, authn.ErrMissingGrant) + }) + } +} + +// A grant is not a substitute for provenance. An instance token with the widest +// grant there is still cannot write without saying who wrote it — the refusal +// changes from authorization to provenance, and does not go away. +func TestProposeToolStillDemandsProvenanceOnBothPlanes(t *testing.T) { + svc := realWriter(t) + + for name, p := range map[string]authn.Principal{ + "local": localPrincipal(), + "instance": instancePrincipal(t, "*"), + } { + t.Run(name, func(t *testing.T) { + noSession := p + noSession.Session = "" + ctx := authn.WithPrincipal(context.Background(), noSession) + + _, err := proposeHandler(ctx, svc, proposeCall()) + require.Error(t, err) + assert.NotErrorIs(t, err, service.ErrForbidden, + "a missing session is a provenance failure, not an authorization one") + + _, err = noSession.AgentWriteFor("1f0c1d1a1e2b3c4d5e6f708192a3b4c5d6e7f809") + assert.ErrorIs(t, err, authn.ErrMissingProvenance) + }) + } +} diff --git a/mcpsrv/list.go b/mcpsrv/list.go index fc3b2129e5d55bf995ed12446d9f7411df67642d..b028380984b1142d2d90e21164d36721c76b6473 100644 --- a/mcpsrv/list.go +++ b/mcpsrv/list.go @@ -50,6 +50,9 @@ Documents []documentEntry `json:"documents,omitempty"` } func listHandler(ctx context.Context, b Backend, in listInput) (listOutput, error) { + if err := requireRead(ctx); err != nil { + return listOutput{}, err + } if strings.TrimSpace(in.Space) == "" { // A rev with no space is a caller that meant to name one. Ignoring it // would answer a different question than was asked and look like it diff --git a/mcpsrv/mcpsrv.go b/mcpsrv/mcpsrv.go index 6a74c1e28650bb9b062714c5aecdc820f7009bda..f59e9c3185dff543c52562d851c1b1e95c7fa878 100644 --- a/mcpsrv/mcpsrv.go +++ b/mcpsrv/mcpsrv.go @@ -280,6 +280,13 @@ // Without it, every read tool served approved content to anyone who cleared the // Host allowlist — spec_propose was already fail-closed in service.Propose, but // spec_search/spec_read/spec_list checked nothing. The refusal is a 401 with a // line of plain text and never a login redirect: every caller here is a machine. +// +// It checks identity and not grants, deliberately. This one endpoint carries +// both the read tools and the write ones, and the tool being called is in the +// JSON-RPC body, not the request — so a surface-wide spec:read would refuse a +// tokens.sr.ht token minted for spec:propose alone at `initialize`, before it +// ever named a tool. The grant is therefore checked per tool, by requireRead and +// by service.Propose, each of which knows what is being attempted. func Gate(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if !authn.PrincipalFromContext(r.Context()).CanRead() { @@ -289,6 +296,17 @@ return } next.ServeHTTP(w, r) }) +} + +// requireRead is the grant half of the read ACL, for the tools that serve +// content: Gate has already established that the caller may read at all, and +// this asks whether the credential they used was minted for it. +// +// It is a no-op for the owner's cookie and for spec's own agent token, neither +// of which carries grants — so every client that works today keeps working — and +// refuses a tokens.sr.ht working token that lacks spec:read. +func requireRead(ctx context.Context) error { + return authn.PrincipalFromContext(ctx).Authorize(authn.ActionRead) } // hostAllowed compares a request's Host against the expected hostname, ignoring diff --git a/mcpsrv/propose_internal_test.go b/mcpsrv/propose_internal_test.go index 60382686c7d8e53ad46cb8bfce7cbe4985061fa2..9f6e8ed30b016a8cd0f63ac7831e85dc92d43cd2 100644 --- a/mcpsrv/propose_internal_test.go +++ b/mcpsrv/propose_internal_test.go @@ -59,8 +59,9 @@ if err != nil { t.Fatalf("proposeHandler: %v", err) } - // The request the service saw. - if w.got.Principal != principal { + // The request the service saw. Principal is no longer comparable with == — + // it carries a grant set — so its rendering stands in. + if w.got.Principal.String() != principal.String() { t.Errorf("principal = %+v, want the one on the context %+v", w.got.Principal, principal) } if w.got.Space != (core.SpaceRef{Owner: "bigbes", Name: "rfcs"}) { diff --git a/mcpsrv/read.go b/mcpsrv/read.go index c20941082d2e9147b351dcbec00f4b8bdec98f71..5b0c4d42ce6635cc1014731b30339094e0d2cc75 100644 --- a/mcpsrv/read.go +++ b/mcpsrv/read.go @@ -51,6 +51,9 @@ Markdown string `json:"markdown"` } func readHandler(ctx context.Context, b Backend, in readInput) (readOutput, error) { + if err := requireRead(ctx); err != nil { + return readOutput{}, err + } ref, err := parseSpace(in.Space) if err != nil { return readOutput{}, err diff --git a/mcpsrv/search.go b/mcpsrv/search.go index 4e6e73bf82ada9cf3e71c7d867b00cb9dd08b87e..bb08f0d079873ca1ebecd2c23994a82758d06958 100644 --- a/mcpsrv/search.go +++ b/mcpsrv/search.go @@ -55,6 +55,9 @@ Total uint64 `json:"total"` } func searchHandler(ctx context.Context, b Backend, in searchInput) (searchOutput, error) { + if err := requireRead(ctx); err != nil { + return searchOutput{}, err + } text := strings.TrimSpace(in.Query) if text == "" { return searchOutput{}, errors.New("query must not be empty") diff --git a/service/bearer.go b/service/bearer.go new file mode 100644 index 0000000000000000000000000000000000000000..4d9fbe298586099201244990695bc40a5bbf309f --- /dev/null +++ b/service/bearer.go @@ -0,0 +1,144 @@ +package service + +import ( + "context" + "database/sql" + "fmt" + "os" + + "github.com/vaughan0/go-ini" + "sourcecraft.dev/bigbes/sr-ht-core/auth" + "sourcecraft.dev/bigbes/sr-ht-core/config" + "sourcecraft.dev/bigbes/sr-ht-core/database" + "sourcecraft.dev/bigbes/sr-ht-ecore/bearer" + + "sourcecraft.dev/bigbes/sr-ht-spec/authn" + "sourcecraft.dev/bigbes/sr-ht-spec/db" +) + +// TokensSection is tokens.sr.ht's config section, spelled literally because it +// is what the instance's config.ini says and what every other service on the +// instance looks the daemon up by. +const TokensSection = "tokens.sr.ht" + +// Option configures a Service at construction. +type Option func(*options) + +// options is what the Option functions accumulate. +type options struct { + // conf is the instance config.ini, present only when WithInstanceTokens was + // passed. It is held here rather than on Config because it carries the + // instance's secrets — [sr.ht] network-key and [webhooks] private-key — and + // Config is a value the daemon prints. + conf ini.File + haveConf bool +} + +// WithInstanceTokens offers the instance config to the tokens.sr.ht bearer +// plane. Whether a plane is actually built depends on what is in it: see +// instancePlane, which treats a missing [tokens.sr.ht] section as "there is no +// such daemon on this instance" rather than as a misconfiguration. +// +// It is an option rather than a parameter because the two CLI paths that build a +// Service — `specsrht token` and `specsrht doc` — authenticate nobody and have +// no use for a validator or the HTTP client behind it. +func WithInstanceTokens(conf ini.File) Option { + return func(o *options) { + o.conf = conf + o.haveConf = true + } +} + +// instancePlane builds the tokens.sr.ht bearer plane from the instance config, +// or reports that there is none to build (a nil option, nil error). +// +// The absence is the case worth spelling out. An instance whose config.ini has +// no [tokens.sr.ht] section has no such daemon, and spec must start anyway and +// keep accepting its own agent token: the local plane is not a fallback for a +// broken instance plane, it is the plane this service shipped with. So a missing +// origin is an answer, not an error — while an origin that is present and +// unusable is an error, and fails startup where an operator is looking rather +// than one request at a time as an unexplained 503. +// +// The origin is read in its internal form (GetOrigin's external=false), so the +// revocation check of SPEC ch. 6 step 4 crosses the docker network directly +// instead of going out through the reverse proxy and back in. +func instancePlane(conf ini.File, q db.Querier) (authn.ResolverOption, error) { + origin := config.GetOrigin(conf, TokensSection, false) + if origin == "" { + return nil, nil + } + + // The node id is what the daemon's internal guard logs the caller as. The + // hostname is the honest answer and needs no config key to be forgotten or + // to drift; a host that cannot name itself is a startup failure rather than + // a guessed label, because a fabricated node id is worse than none — it is + // the wrong answer to the only question the revocation log can be asked. + node, err := os.Hostname() + if err != nil { + return nil, fmt.Errorf("service: the tokens.sr.ht plane needs a node id and this host cannot name itself: %w", err) + } + + v, err := bearer.New(bearer.Options{ + Origin: origin, + ClientID: ConfigSection, + NodeID: node, + }) + if err != nil { + return nil, fmt.Errorf("service: build the tokens.sr.ht validator: %w", err) + } + + // auth.LookupUser opens its own transaction, so the pool itself is needed + // and not the Querier interface a *sql.Tx also satisfies. Refusing loudly + // beats silently leaving the plane out: an instance that configured + // tokens.sr.ht and got no instance plane would look identical to one that + // did not configure it, and the difference would only surface as every agent + // token being refused. + pool, ok := q.(*sql.DB) + if !ok { + return nil, fmt.Errorf( + "service: the tokens.sr.ht plane needs the *sql.DB pool (auth.LookupUser opens its own transaction), got %T", q) + } + + return authn.WithInstancePlane(v, metaUserLookup{pool: pool, conf: conf}), nil +} + +// metaUserLookup resolves the owner of an instance token to the local "user" +// row, through core-go's auth.LookupUser — the same function dolt, cover and +// bench resolve their token owners with. +// +// The two context values it installs are not optional and not defensive. +// auth.LookupUser reads config.ServiceName out of the context on every call and +// opens a read-only transaction through core-go's database context, and both of +// those panic when absent. spec's resolver middleware runs on the *anonymous* +// router, which core-go's WithDefaultMiddleware does not decorate — it installs +// the config, database and auth middleware on the authenticated router only — +// so nothing upstream has put either there. This adapter is what supplies them, +// and it is the reason authn declares a UserLookup interface instead of calling +// core-go itself. +type metaUserLookup struct { + pool *sql.DB + conf ini.File +} + +// LookupUser implements authn.UserLookup. +// +// A user this instance has never seen is resolved by core-go against +// meta.sr.ht and written down. In practice that path is not reached: the +// resolver refuses an instance token whose owner is not [sr.ht] owner-name, and +// the daemon seeds that row with EnsureOwnerUser before it serves anything. +func (l metaUserLookup) LookupUser(ctx context.Context, username string) (authn.InstanceUser, error) { + ctx = database.Context(ctx, l.pool) + ctx = config.Context(ctx, l.conf, ConfigSection) + + var ac auth.AuthContext + if err := auth.LookupUser(ctx, username, &ac); err != nil { + return authn.InstanceUser{}, fmt.Errorf("service: look up user %q: %w", username, err) + } + if ac.UserID == 0 { + // A resolved user with no row id would key nothing and authenticate + // everything; there is no sensible value to substitute. + return authn.InstanceUser{}, fmt.Errorf("service: user %q resolved to no row id", username) + } + return authn.InstanceUser{ID: ac.UserID, Username: ac.Username}, nil +} diff --git a/service/bearer_test.go b/service/bearer_test.go new file mode 100644 index 0000000000000000000000000000000000000000..51dbb6a4d5bd5a3ffe04f9447de99a4691af2233 --- /dev/null +++ b/service/bearer_test.go @@ -0,0 +1,294 @@ +package service + +import ( + "context" + "strings" + "testing" + + "github.com/go-git/go-git/v5/plumbing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/vaughan0/go-ini" + + "sourcecraft.dev/bigbes/sr-ht-ecore/grants" + + "sourcecraft.dev/bigbes/sr-ht-spec/authn" + "sourcecraft.dev/bigbes/sr-ht-spec/core" + "sourcecraft.dev/bigbes/sr-ht-spec/gitx" +) + +// mustGrants parses a grant string or fails the test. +func mustGrants(t *testing.T, s string) grants.Grants { + t.Helper() + g, err := grants.Parse(s) + require.NoError(t, err, "parse grants %q", s) + return g +} + +// instanceAgent is agentPrincipal as a tokens.sr.ht working token resolves it: +// the same agent, acting for the same owner, with a grant set behind it. +func instanceAgent(t *testing.T, grantString string) authn.Principal { + t.Helper() + p := agentPrincipal() + p.Plane = authn.PlaneInstance + p.Grants = mustGrants(t, grantString) + p.UserID = 1 + return p +} + +// localAgent is agentPrincipal as spec's own agent_token resolves it: no owner +// of its own, no grants, nothing to authorize against. +func localAgent() authn.Principal { + p := agentPrincipal() + p.Plane = authn.PlaneLocal + p.TokenName = "laptop" + return p +} + +// proposeRequest is a well-formed open, so that the only thing a test varies is +// the principal. +func proposeRequest(p authn.Principal) ProposeRequest { + return ProposeRequest{ + Space: fxSpace, + Principal: p, + Title: "Add a note", + IfMatch: headRev, + Message: "add notes/a.md", + Writes: []DocumentWrite{{Path: "notes/a.md", Content: mdDoc("N-1", "A", "body")}}, + } +} + +// An instance token minted without spec:propose is refused, and refused as a +// forbidden principal rather than as a bad request — the agent has to be told to +// ask for a wider grant, not to fix its document. +func TestProposeRefusesAnInstanceTokenWithoutTheProposeGrant(t *testing.T) { + svc, _ := newService(t) + for _, grantString := range []string{"spec:read", "bench:upload", "cover:read cover:upload"} { + t.Run(grantString, func(t *testing.T) { + _, err := svc.Propose(context.Background(), proposeRequest(instanceAgent(t, grantString))) + require.Error(t, err) + assert.ErrorIs(t, err, ErrForbidden) + assert.ErrorIs(t, err, authn.ErrMissingGrant) + assert.Contains(t, err.Error(), authn.ActionPropose) + }) + } +} + +// The grant check is a guard on the principal, like the agent-only one beside +// it: it holds with a database that cannot be reached, which is what proves it +// runs before anything is opened. +func TestProposeAcceptsTheGrantedPrincipals(t *testing.T) { + svc, _ := newService(t) + cases := map[string]authn.Principal{ + "local agent token": localAgent(), + "instance token, exact grant": instanceAgent(t, "spec:propose"), + "instance token, both grants": instanceAgent(t, "spec:read spec:propose"), + "instance token, universal": instanceAgent(t, "*"), + "instance token, registered row": instanceAgent(t, "spec:propose id:42"), + } + for name, p := range cases { + t.Run(name, func(t *testing.T) { + // The write cannot land — the pool is dead and the space does not + // exist — but it must not be turned away at the ACL. + _, err := svc.Propose(context.Background(), proposeRequest(p)) + require.Error(t, err, "the fixture has no space, so this cannot succeed") + assert.NotErrorIs(t, err, ErrForbidden, "%s must clear the write ACL", name) + assert.NotErrorIs(t, err, authn.ErrMissingGrant) + }) + } +} + +// The refs rule is untouched by grants and applies to both planes identically: +// an agent credential moves refs under the proposal prefix and nothing else, no +// matter how wide the grant set behind it is. +func TestRefsRuleIsUnchangedOnBothPlanes(t *testing.T) { + planes := map[string]authn.Principal{ + "local": localAgent(), + "instance": instanceAgent(t, "*"), + } + newHash := plumbing.NewHash("1f0c1d1a1e2b3c4d5e6f708192a3b4c5d6e7f809") + + for name, p := range planes { + t.Run(name, func(t *testing.T) { + kind, err := principalKind(p) + require.NoError(t, err) + assert.Equal(t, gitx.PrincipalAgent, kind, + "both planes are agents to the refs rule; a universal grant does not promote one") + + // Outside the proposal prefix: refused. + err = gitx.CheckRefUpdate(kind, gitx.DefaultApprovedBranch, gitx.RefUpdate{ + Ref: "refs/heads/" + gitx.DefaultApprovedBranch, New: newHash, FastForward: true, + }) + assert.ErrorIs(t, err, gitx.ErrRefRejected, + "an agent may not move the approved branch on either plane") + + err = gitx.CheckRefUpdate(kind, gitx.DefaultApprovedBranch, gitx.RefUpdate{ + Ref: "refs/heads/scratch", New: newHash, FastForward: true, + }) + assert.ErrorIs(t, err, gitx.ErrRefRejected, + "an agent may not create a branch outside the proposal prefix on either plane") + + // Inside it: permitted, on both planes. + err = gitx.CheckRefUpdate(kind, gitx.DefaultApprovedBranch, gitx.RefUpdate{ + Ref: "refs/heads/" + core.ProposalPrefix + "7", New: newHash, FastForward: true, + }) + assert.NoError(t, err) + }) + } +} + +// Provenance is still mandatory on both planes. A grant says what a credential +// was minted for; it says nothing about who wrote a commit, and it does not +// excuse a write from saying so. +func TestProvenanceStillRequiredOnBothPlanes(t *testing.T) { + svc, _ := newService(t) + for name, p := range map[string]authn.Principal{ + "local": localAgent(), + "instance": instanceAgent(t, "*"), + } { + t.Run(name, func(t *testing.T) { + noSession := p + noSession.Session = "" + _, err := svc.Propose(context.Background(), proposeRequest(noSession)) + require.Error(t, err) + assert.NotErrorIs(t, err, ErrForbidden, + "a missing session is a provenance failure, not an authorization one") + + // The provenance builder is where it is caught, whichever plane the + // principal came in on. + _, err = noSession.AgentWriteFor(headRev) + assert.ErrorIs(t, err, authn.ErrMissingProvenance) + }) + } +} + +// --- the plane's own wiring ------------------------------------------------ + +// tokensConf is an instance config with a tokens.sr.ht section, in the shape +// GetOrigin reads. +func tokensConf(section ini.Section) ini.File { + conf := ini.File{"sr.ht": ini.Section{"owner-name": "bigbes"}} + if section != nil { + conf[TokensSection] = section + } + return conf +} + +// An instance whose config has no [tokens.sr.ht] section has no such daemon. +// The plane must then be absent rather than broken: spec starts, and its own +// agent token is the only door — which is exactly what the instance ran before +// tokens.sr.ht existed. +func TestInstancePlane_AbsentSectionIsNotAnError(t *testing.T) { + for name, conf := range map[string]ini.File{ + "no section at all": tokensConf(nil), + "section with no origin": tokensConf(ini.Section{ + "connection-string": "postgresql://tokens@postgres/tokens.sr.ht", + }), + } { + t.Run(name, func(t *testing.T) { + plane, err := instancePlane(conf, deadDB(t)) + require.NoError(t, err, "a missing origin is an answer, not a misconfiguration") + assert.Nil(t, plane) + }) + } +} + +func TestInstancePlane_BuiltFromTheInternalOrigin(t *testing.T) { + // The internal origin wins over the external one: the revocation check goes + // over the docker network rather than out through the proxy and back. + plane, err := instancePlane(tokensConf(ini.Section{ + "origin": "https://tokens.srht.bigb.es", + "internal-origin": "http://tokens.sr.ht:5094", + }), deadDB(t)) + require.NoError(t, err) + require.NotNil(t, plane) + + rs, err := authn.NewResolver("bigbes", NewTokenStore(nil), plane) + require.NoError(t, err) + assert.True(t, rs.HasInstancePlane()) +} + +// An origin that is present and unusable fails startup, where an operator is +// looking — not one request at a time as an unexplained 503. +func TestInstancePlane_UnusableOriginIsAStartupError(t *testing.T) { + _, err := instancePlane(tokensConf(ini.Section{"origin": "tokens.srht.bigb.es"}), deadDB(t)) + require.Error(t, err) + assert.Contains(t, err.Error(), "tokens.sr.ht") +} + +// Service.New wires the plane when it is offered and a config supports it, and +// builds the same local-only resolver it always did when it is not. +func TestNew_InstancePlaneIsOptional(t *testing.T) { + cfg := testConfig(t, t.TempDir()) + + svc, err := New(cfg, deadDB(t)) + require.NoError(t, err) + assert.False(t, svc.Resolver().HasInstancePlane(), + "no option means no instance plane, exactly as before it existed") + + svc, err = New(cfg, deadDB(t), WithInstanceTokens(tokensConf(nil))) + require.NoError(t, err) + assert.False(t, svc.Resolver().HasInstancePlane(), + "an instance without a [tokens.sr.ht] section must still start, on the local plane") + + svc, err = New(cfg, deadDB(t), WithInstanceTokens(tokensConf(ini.Section{ + "origin": "https://tokens.srht.bigb.es", + }))) + require.NoError(t, err) + assert.True(t, svc.Resolver().HasInstancePlane()) +} + +// --- Postgres-backed integration tests (skip when SPECSRHT_TEST_PG is unset) --- + +// The whole open path, once per plane: the credential every agent is configured +// with today and a tokens.sr.ht token carrying spec:propose both open a +// proposal, with the same provenance recorded on it. +func TestProposeOpensProposalOnBothPlanes(t *testing.T) { + for name, p := range map[string]authn.Principal{ + "local agent token": localAgent(), + "instance token": instanceAgent(t, "spec:read spec:propose"), + } { + t.Run(name, func(t *testing.T) { + svc, _ := newTestService(t) + ctx := context.Background() + sp, err := svc.CreateSpace(ctx, fxSpace) + require.NoError(t, err) + base, err := sp.Repo.ApprovedHead(ctx) + require.NoError(t, err) + + req := proposeRequest(p) + req.IfMatch = base.String() + res, err := svc.Propose(ctx, req) + require.NoError(t, err) + + assert.Equal(t, core.StateOpen, res.Proposal.State) + assert.Equal(t, "claude-code/spec-writer", res.Proposal.Agent) + assert.True(t, strings.HasSuffix(res.URL, "/p/1"), "URL = %q", res.URL) + assert.True(t, strings.HasPrefix(res.Proposal.Branch, core.ProposalPrefix), + "an agent's branch stays under the proposal prefix: %q", res.Proposal.Branch) + }) + } +} + +// And the refusal, end to end: a token minted for reading only gets 403's +// sentinel out of the same call, with the space in place and nothing else to +// blame. +func TestProposeRefusesAReadOnlyInstanceTokenEndToEnd(t *testing.T) { + svc, _ := newTestService(t) + ctx := context.Background() + sp, err := svc.CreateSpace(ctx, fxSpace) + require.NoError(t, err) + base, err := sp.Repo.ApprovedHead(ctx) + require.NoError(t, err) + + req := proposeRequest(instanceAgent(t, "spec:read")) + req.IfMatch = base.String() + _, err = svc.Propose(ctx, req) + require.Error(t, err) + assert.ErrorIs(t, err, ErrForbidden) + assert.ErrorIs(t, err, authn.ErrMissingGrant) + + open, err := svc.ListProposals(ctx, fxSpace, core.StateOpen) + require.NoError(t, err) + assert.Empty(t, open, "a refused write must leave no row behind") +} diff --git a/service/propose.go b/service/propose.go index b93f3efac4795b3cf2bf8a03f9db56f24ecc7061..258d84618ae96a0d87643888926d91d2e6475e14 100644 --- a/service/propose.go +++ b/service/propose.go @@ -101,6 +101,14 @@ func (s *Service) Propose(ctx context.Context, req ProposeRequest) (ProposeResult, error) { if !req.Principal.IsAgent() { return ProposeResult{}, fmt.Errorf("%w: %s may not propose; proposing is agent-only", ErrForbidden, req.Principal) } + // The grant check, at the layer that finally knows the action. It is a no-op + // for the local agent token — which carries no grants and never will, its + // boundary being the refs rule, unchanged by any of this — and refuses a + // tokens.sr.ht token that was minted without spec:propose. Both write + // surfaces come through here, so this is the one place it is spelled. + if err := req.Principal.Authorize(authn.ActionPropose); err != nil { + return ProposeResult{}, fmt.Errorf("%w: %s may not propose: %w", ErrForbidden, req.Principal, err) + } if len(req.Writes) == 0 { return ProposeResult{}, fmt.Errorf("%w: a proposal must write at least one document", ErrInvalid) } diff --git a/service/service.go b/service/service.go index d7fda935fec9e3281057456646e67b48635e2aae..e3f36b1141e3f5dc5adc6c3c6e9b73c9cad312dd 100644 --- a/service/service.go +++ b/service/service.go @@ -244,16 +244,37 @@ // handed to core-go's database middleware, so request-scoped queries and the // reconciler's background queries share one pool. A nil handle is refused // rather than tolerated: every agent token would then resolve as unknown, which // looks exactly like a mass revocation and is a miserable thing to debug. -func New(cfg Config, q db.Querier) (*Service, error) { +// +// Pass WithInstanceTokens(conf) to offer the resolver the tokens.sr.ht bearer +// plane alongside the local agent-token one. Without it — and with it on an +// instance whose config has no [tokens.sr.ht] section — the resolver knows only +// the local plane, which is what every caller got before that plane existed. +func New(cfg Config, q db.Querier, opts ...Option) (*Service, error) { if err := cfg.Validate(); err != nil { return nil, err } if q == nil { return nil, errors.New("service: nil database handle") } + + var o options + for _, opt := range opts { + opt(&o) + } + var ropts []authn.ResolverOption + if o.haveConf { + plane, err := instancePlane(o.conf, q) + if err != nil { + return nil, err + } + if plane != nil { + ropts = append(ropts, plane) + } + } + store := db.NewStore(q) tokens := NewTokenStore(store) - resolver, err := authn.NewResolver(cfg.Instance.OwnerName, tokens) + resolver, err := authn.NewResolver(cfg.Instance.OwnerName, tokens, ropts...) if err != nil { return nil, fmt.Errorf("service: build resolver: %w", err) } diff --git a/web/comments.go b/web/comments.go index 9c72d7912c1c9ec755e89e463989772bfdee7840..2587a1fcd56663a53ca0f03269431a8f6172c02c 100644 --- a/web/comments.go +++ b/web/comments.go @@ -347,6 +347,12 @@ if !mayRead(r) { s.renderError(w, r, http.StatusForbidden, "you may not read this proposal") return service.Proposal{}, false } + // The grant half of the same ACL. The review conversation is content, so a + // tokens.sr.ht token reaches it on spec:read like every other read here. + if err := readGrant(r); err != nil { + s.denyGrant(w, r, formatHTML) + return service.Proposal{}, false + } if err := r.ParseForm(); err != nil { s.renderError(w, r, http.StatusBadRequest, "malformed form submission") return service.Proposal{}, false diff --git a/web/grant_test.go b/web/grant_test.go new file mode 100644 index 0000000000000000000000000000000000000000..f7e0b939aa5d80d59c8ecc207f45eb3945f8561d --- /dev/null +++ b/web/grant_test.go @@ -0,0 +1,131 @@ +package web + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-chi/chi/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/vaughan0/go-ini" + + "sourcecraft.dev/bigbes/sr-ht-ecore/grants" + + "sourcecraft.dev/bigbes/sr-ht-spec/authn" +) + +// mustGrants parses a grant string or fails the test. +func mustGrants(t *testing.T, s string) grants.Grants { + t.Helper() + g, err := grants.Parse(s) + require.NoError(t, err, "parse grants %q", s) + return g +} + +// grantRouter mounts the read plane with a fixed principal injected into every +// request, bypassing token resolution: what put the principal there is the +// resolver's business, and this file is about what the handlers do with it. +func grantRouter(t *testing.T, p authn.Principal) http.Handler { + t.Helper() + resolver, err := authn.NewResolver("bigbes", stubTokenStore{}) + require.NoError(t, err) + + srv, err := New(Options{ + Conf: ini.File{ + "sr.ht": ini.Section{ + "network-key": testConf.Section("sr.ht")["network-key"], + "site-name": "sourcehut", + "environment": "development", + "owner-name": "bigbes", + }, + "webhooks": ini.Section{"private-key": testConf.Section("webhooks")["private-key"]}, + "spec.sr.ht": ini.Section{"origin": "https://spec.example"}, + "meta.sr.ht": ini.Section{"origin": "https://meta.example"}, + }, + Reader: newFakeReader(), + Searcher: &fakeSearcher{}, + Resolver: resolver, + }) + require.NoError(t, err) + + r := chi.NewRouter() + r.Use(func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + next.ServeHTTP(w, req.WithContext(authn.WithPrincipal(req.Context(), p))) + }) + }) + srv.Register(r) + return r +} + +func instancePrincipal(t *testing.T, grantString string) authn.Principal { + t.Helper() + return authn.Principal{ + Kind: authn.KindAgent, Owner: "bigbes", Agent: "claude-code", Session: "s-1", + Plane: authn.PlaneInstance, Grants: mustGrants(t, grantString), + } +} + +// Every read route asks for spec:read from a tokens.sr.ht working token, and +// asks nothing extra of the credentials that carry no grants. The refusal is a +// 403 and pointedly not a login redirect: the caller is already authenticated, +// so sending it to meta would loop it back with the same token. +func TestReadRoutesRequireTheReadGrant(t *testing.T) { + targets := []string{ + "/~bigbes/rfcs", + "/~bigbes/rfcs/specs/0007-storage", + "/~bigbes/rfcs/specs/0007-storage.md", + "/~bigbes/rfcs/specs/0007-storage.json", + "/search?q=storage", + "/inbox", + } + + t.Run("refused without it", func(t *testing.T) { + h := grantRouter(t, instancePrincipal(t, "spec:propose")) + for _, target := range targets { + t.Run(target, func(t *testing.T) { + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, target, nil)) + assert.Equal(t, http.StatusForbidden, rec.Code, "body: %s", rec.Body) + assert.Contains(t, rec.Body.String(), authn.ActionRead) + assert.Empty(t, rec.Header().Get("Location"), + "an authenticated caller must not be redirected to a login") + }) + } + }) + + // The credentials that work today, and the instance token that was minted + // for reading: all served, no 403 anywhere. + for name, p := range map[string]authn.Principal{ + "owner cookie": {Kind: authn.KindOwner, Owner: "bigbes", CookieUser: "bigbes"}, + "local agent token": {Kind: authn.KindAgent, Owner: "bigbes", Plane: authn.PlaneLocal, TokenName: "laptop"}, + "instance token, spec:read": instancePrincipal(t, "spec:read"), + "instance token, universal": instancePrincipal(t, "*"), + } { + t.Run("served for "+name, func(t *testing.T) { + h := grantRouter(t, p) + for _, target := range targets { + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, target, nil)) + assert.Equal(t, http.StatusOK, rec.Code, "%s: body %s", target, rec.Body) + } + }) + } +} + +// An anonymous browser is still sent to meta's login and an anonymous bot still +// gets a 401: the grant check sits behind the identity one and does not change +// what happens when there is no identity at all. +func TestAnonymousDenialIsUnchanged(t *testing.T) { + h := grantRouter(t, authn.Anonymous()) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/~bigbes/rfcs/specs/0007-storage", nil)) + assert.Equal(t, http.StatusFound, rec.Code) + assert.Contains(t, rec.Header().Get("Location"), "meta.example") + + rec = httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/~bigbes/rfcs/specs/0007-storage.md", nil)) + assert.Equal(t, http.StatusUnauthorized, rec.Code) +} diff --git a/web/handlers.go b/web/handlers.go index 27174f28fa82945834a575030500c104d27a83a5..7608021320617411f0d132a487c5e2a6c762193b 100644 --- a/web/handlers.go +++ b/web/handlers.go @@ -58,6 +58,31 @@ func mayRead(r *http.Request) bool { return authn.PrincipalFromContext(r.Context()).CanRead() } +// readGrant reports whether the credential behind this request was minted for +// reading. It is a no-op for the owner's cookie and for spec's own agent token, +// neither of which carries grants; it refuses a tokens.sr.ht working token +// without spec:read. +// +// Kept apart from mayRead because the two questions have different answers when +// they fail: "who are you" ends in a login, "what may this token do" does not. +func readGrant(r *http.Request) error { + return authn.PrincipalFromContext(r.Context()).Authorize(authn.ActionRead) +} + +// allowRead is the whole read gate: identity, then grant. It answers the request +// itself when either refuses and reports whether the handler may go on. +func (s *Server) allowRead(w http.ResponseWriter, r *http.Request, f format) bool { + if !mayRead(r) { + s.denyRead(w, r, f) + return false + } + if err := readGrant(r); err != nil { + s.denyGrant(w, r, f) + return false + } + return true +} + // denyRead answers a viewer with no read authority in the shape their client // can act on: a browser is sent to meta's login, a machine asking for .md or // .json gets a 401. Redirecting a bot to an HTML login page would hand it a @@ -71,6 +96,20 @@ w.Header().Set("Content-Type", "text/plain; charset=utf-8") http.Error(w, "authentication required", http.StatusUnauthorized) } +// denyGrant answers a caller whose credential is good but was not minted for +// reading. 403 in both shapes, and pointedly no login redirect: the caller is +// already authenticated, so sending them to meta would loop them back here with +// the same token and the same answer. +func (s *Server) denyGrant(w http.ResponseWriter, r *http.Request, f format) { + msg := "this token does not grant " + authn.ActionRead + if f == formatHTML { + s.renderError(w, r, http.StatusForbidden, msg) + return + } + w.Header().Set("Content-Type", "text/plain; charset=utf-8") + http.Error(w, msg, http.StatusForbidden) +} + // ---- error mapping -------------------------------------------------------- // httpStatusFor maps a service error onto a status. service.ErrNotFound already @@ -185,8 +224,7 @@ Items []treeItem } func (s *Server) handleSpace(w http.ResponseWriter, r *http.Request) { - if !mayRead(r) { - s.denyRead(w, r, formatHTML) + if !s.allowRead(w, r, formatHTML) { return } ref, err := spaceRefFrom(r) @@ -333,8 +371,7 @@ return } address, f := splitFormat(rest) - if !mayRead(r) { - s.denyRead(w, r, f) + if !s.allowRead(w, r, f) { return } ref, err := spaceRefFrom(r) @@ -522,8 +559,7 @@ Spaces []spaceLink } func (s *Server) handleSearch(w http.ResponseWriter, r *http.Request) { - if !mayRead(r) { - s.denyRead(w, r, formatHTML) + if !s.allowRead(w, r, formatHTML) { return } q := strings.TrimSpace(r.URL.Query().Get("q")) diff --git a/web/inbox.go b/web/inbox.go index 72519d237b91047796f46d26faf5e5439137120b..e108748c7adea640612f85f532ca8516a654b6f7 100644 --- a/web/inbox.go +++ b/web/inbox.go @@ -41,8 +41,7 @@ // plus the digest of recently policy-merged content. It is the backstop the // design describes — the link an agent hands you is the normal way in, and this // catches the work no link reached. func (s *Server) handleInbox(w http.ResponseWriter, r *http.Request) { - if !mayRead(r) { - s.loginRedirect(w, r) + if !s.allowRead(w, r, formatHTML) { return } open, err := s.reader.Inbox(r.Context()) diff --git a/web/proposal.go b/web/proposal.go index 6e44ccf8fbd6d219ccaba4cb34280f6e5287fd21..6c90ab70cf0e8c73c88c8546157da549fea92700 100644 --- a/web/proposal.go +++ b/web/proposal.go @@ -72,8 +72,7 @@ // not match the URL is a 404: the id is global, but the link names its space, // and answering for the wrong space would let one space's URL surface another's // proposal. func (s *Server) handleProposal(w http.ResponseWriter, r *http.Request) { - if !mayRead(r) { - s.loginRedirect(w, r) + if !s.allowRead(w, r, formatHTML) { return } ref, err := spaceRefFrom(r)