diff --git a/api/api_test.go b/api/api_test.go index b8e11cee3159c4864d02c4a47128218b05b15cc7..d9be4fa94160c3c52821c017dfb16d834acb1a8f 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -49,21 +49,15 @@ srv.Register(r) return r } -// testResolver is a resolver New requires but Register does not use. One token -// store with no tokens is enough to construct it. +// testResolver is a resolver New requires but Register does not use. These +// tests inject their principal directly, so it needs no agent plane. func testResolver(t *testing.T) *authn.Resolver { t.Helper() - res, err := authn.NewResolver("bigbes", stubTokens{}) + res, err := authn.NewResolver("bigbes") if err != nil { t.Fatalf("NewResolver: %v", err) } return res -} - -type stubTokens struct{} - -func (stubTokens) LookupAgentToken(context.Context, []byte) (authn.AgentToken, error) { - return authn.AgentToken{}, authn.ErrUnknownToken } func agent() authn.Principal { diff --git a/api/bearer_test.go b/api/bearer_test.go index abd17e5776486b5af61b6da6aba1779df4062250..20ab7dfb24cc099aa965106de88d8f0088fd444a 100644 --- a/api/bearer_test.go +++ b/api/bearer_test.go @@ -2,8 +2,6 @@ package api_test import ( "context" - "crypto/sha256" - "encoding/hex" "net/http" "net/http/httptest" "os" @@ -45,31 +43,9 @@ os.Exit(m.Run()) } // --------------------------------------------------------------------------- -// Fixtures: the two credential planes, in front of the real REST write route. +// Fixtures: the agent credential plane, 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{} @@ -92,12 +68,11 @@ 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. +// with the agent credential plane wired: the tokens.sr.ht validator pointed at +// a fake revocation daemon. type planeFixture struct { handler http.Handler writer *fakeWriter - local *localTokens } func newPlaneFixture(t *testing.T, daemonStatus int, closeDaemon bool) *planeFixture { @@ -116,8 +91,7 @@ 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{})) + resolver, err := authn.NewResolver("bigbes", authn.WithInstancePlane(v, users{})) require.NoError(t, err) w := &fakeWriter{res: service.ProposeResult{ @@ -127,7 +101,7 @@ }} srv, err := api.New(api.Options{Writer: w, Resolver: resolver}) require.NoError(t, err) - return &planeFixture{handler: srv.Handler(), writer: w, local: local} + return &planeFixture{handler: srv.Handler(), writer: w} } func (f *planeFixture) put(token string) *httptest.ResponseRecorder { @@ -145,22 +119,15 @@ } // --------------------------------------------------------------------------- -// 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) { +// The credential every agent on the instance used to be configured with — an +// opaque secret out of agent_token — reaches nothing any more. 401 at the door, +// and the write plane never sees the request. +func TestPutWithTheOldOpaqueAgentTokenIs401(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") + assert.Equal(t, http.StatusUnauthorized, rec.Code) + assert.Empty(t, f.writer.got.Writes, "a refused credential must not reach the write plane") } // A tokens.sr.ht working token reaches the same route, and arrives carrying the @@ -177,7 +144,6 @@ 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 @@ -192,38 +158,32 @@ 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) { +// A revoked instance token is 401 at the door. There is no second door for it +// to be re-tried at any more, which is what removing the local plane bought. +func TestPutWithARevokedInstanceTokenIs401(t *testing.T) { f := newPlaneFixture(t, http.StatusNotFound, false) - tok := liveToken("spec:propose id:42") - f.local.add(tok, "shadow") - rec := f.put(tok) + rec := f.put(liveToken("spec:propose id:42")) 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. +// An unreachable tokens.sr.ht is 503 and never 401: refusing every live 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) + rec := f.put(liveToken("spec:propose id:42")) 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) { +// A token from another issuer — a meta.sr.ht PAT — used to fall through to +// spec's own store, which did not know it. With one plane it is refused where +// it is presented, and still with a 401: bearer.ErrNotOurs is a permanent +// refusal, not the 503 an unclassified error would earn. +func TestPutWithAForeignTokenIs401(t *testing.T) { f := newPlaneFixture(t, http.StatusNoContent, false) pat := &auth.BearerToken{ Version: auth.TokenVersion, @@ -235,5 +195,5 @@ } 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") + assert.Empty(t, f.writer.got.Writes) } diff --git a/authn/authn_test.go b/authn/authn_test.go index 0cbf4907756b365252b8535fd7e41fbbbb5e45f0..81856bce9c7b8bfaff1ec80a40b9419c8351bc13 100644 --- a/authn/authn_test.go +++ b/authn/authn_test.go @@ -1,17 +1,13 @@ package authn import ( - "context" "crypto/rand" "encoding/base64" - "encoding/hex" "encoding/json" - "fmt" "net/http" "net/http/httptest" "os" "testing" - "time" "github.com/fernet/fernet-go" "github.com/vaughan0/go-ini" @@ -118,58 +114,6 @@ if err != nil { t.Fatalf("InstanceFromConfig: %v", err) } return inst -} - -// stubStore is an in-memory TokenStore keyed by hex(sha256(token)). It can be -// told to fail with an arbitrary error to exercise the transient path, and -// counts calls so tests can assert the cookie path never touches it. -type stubStore struct { - rows map[string]AgentToken - err error - calls int -} - -func newStubStore() *stubStore { return &stubStore{rows: map[string]AgentToken{}} } - -func (s *stubStore) LookupAgentToken(ctx context.Context, hash []byte) (AgentToken, error) { - s.calls++ - if s.err != nil { - return AgentToken{}, s.err - } - tok, ok := s.rows[hex.EncodeToString(hash)] - if !ok { - // The contract db/ must honour: no row means ErrUnknownToken. - return AgentToken{}, fmt.Errorf("agent_token: %w", ErrUnknownToken) - } - return tok, nil -} - -// add stores a live token row for the given secret. -func (s *stubStore) add(secret, name string) AgentToken { - hash := HashToken(secret) - tok := AgentToken{ - ID: int64(len(s.rows) + 1), - Name: name, - Hash: hash, - Created: time.Date(2026, 7, 22, 10, 0, 0, 0, time.UTC), - } - s.rows[hex.EncodeToString(hash)] = tok - return tok -} - -// revoke stores a revoked token row for the given secret. -func (s *stubStore) revoke(secret, name string) AgentToken { - tok := s.add(secret, name) - when := time.Date(2026, 7, 22, 11, 0, 0, 0, time.UTC) - tok.Revoked = &when - s.rows[hex.EncodeToString(tok.Hash)] = tok - return tok -} - -// put stores an arbitrary row under an arbitrary lookup key, for the case where -// the store returns a row whose hash does not match what was presented. -func (s *stubStore) put(hash []byte, tok AgentToken) { - s.rows[hex.EncodeToString(hash)] = tok } // request builds a GET / carrying the given cookie value and headers. An empty diff --git a/authn/bearer.go b/authn/bearer.go index f1a5696665ac72a0910221b715e8bf84b01efba7..60e139e2f754390ceb765506b3ccc2c7540194bb 100644 --- a/authn/bearer.go +++ b/authn/bearer.go @@ -67,46 +67,33 @@ LookupUser(ctx context.Context, username string) (InstanceUser, error) } // resolveInstanceToken runs the tokens.sr.ht plane against a presented bearer -// credential. +// credential. Its answer is final: there is one agent credential plane, so a +// refusal here is the service's refusal. // -// 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. +// It used to report a third thing — whether the caller should fall back to +// spec's own agent_token store — and exactly two refusals said yes: // -// 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. +// - bearer.ErrInvalid, because spec's local token was 32 random bytes in +// base64, which is precisely what "did not decode as one of ours" looks +// like; +// - bearer.ErrNotOurs, because refusing a meta.sr.ht PAT was the local plane's +// business rather than this one's, and falling through cost one hash lookup +// that would miss. // -// Every other refusal is terminal and must never reach the old door: +// With that store gone both are plain refusals. The one consequence worth +// naming is ErrNotOurs: IsAuthFailure now counts it permanent, so a meta PAT +// presented here earns a 401 rather than the 503 an unclassified error would. // -// - 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. +// The rest of the mapping is unchanged and lives in StatusFor: ErrInvalid and +// ErrRevoked are 401, ErrForbidden and a foreign owner are 403, and +// ErrUnavailable is 503 — never 401, because "I could not ask tokens.sr.ht" is +// not "your token is bad". func (rs *Resolver) resolveInstanceToken( - ctx context.Context, r *http.Request, presented string, -) (Principal, bool, error) { + ctx context.Context, presented, agent, session string, +) (Principal, 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) + if err != nil { + return Anonymous(), fmt.Errorf("authn: instance token: %w", err) } // The token names a meta.sr.ht account, and spec.sr.ht has exactly one that @@ -122,7 +109,7 @@ // 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( + return Anonymous(), fmt.Errorf( "%w: the token belongs to ~%s, and this instance answers only to ~%s", ErrNotInstanceOwner, username, rs.owner) } @@ -135,19 +122,19 @@ 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 Anonymous(), 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)), + Agent: agent, + Session: session, TokenName: instanceTokenLabel(tok), Plane: PlaneInstance, Grants: tok.Grants, UserID: user.ID, - }, false, nil + }, nil } // instanceTokenLabel names the credential in a log line. A registered token has @@ -176,8 +163,11 @@ // 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 permanent about the credential itself — malformed, foreign, +// revoked — is 401. +// - ErrNoAgentPlane is 503 and not 401. An instance with no [tokens.sr.ht] +// origin cannot check any credential, and telling the holder of a good token +// that it is bad would send them to re-provision it. // - Everything else is transient by definition and answers 503, which is the // fail-closed direction: a backend outage never reads as a valid credential. func StatusFor(err error) int { diff --git a/authn/bearer_test.go b/authn/bearer_test.go index fd689b1fa66abba8f9fa010cff18c104e1f8fdd3..da9fc73ea9371d5ce1800d135a44fbcbee98d13c 100644 --- a/authn/bearer_test.go +++ b/authn/bearer_test.go @@ -1,7 +1,9 @@ package authn import ( + "bytes" "context" + "encoding/base64" "net/http" "net/http/httptest" "testing" @@ -122,11 +124,11 @@ 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. +// planeFixture wires a resolver with the one agent credential plane: the real +// ecore validator pointed at a daemon answering daemonStatus, and a stub lookup +// for the owner a token names. type planeFixture struct { rs *Resolver - store *stubStore users *stubUsers daemon *fakeDaemon } @@ -134,8 +136,8 @@ 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)) + f := &planeFixture{users: newStubUsers(), daemon: d} + rs, err := NewResolver("bigbes", WithInstancePlane(validatorFor(t, d.server.URL), f.users)) require.NoError(t, err) f.rs = rs return f @@ -151,32 +153,26 @@ }) } // 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) { +// instance used to be configured with — an opaque 32-byte secret out of +// agent_token — authenticates nowhere any more. It is not a token this instance +// sealed, and there is no longer a second store to ask. +func TestResolve_OldOpaqueAgentTokenIsRefused(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) + old := base64.RawURLEncoding.EncodeToString(bytes.Repeat([]byte{0x5a}, 32)) - 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) + p, err := f.rs.Resolve(context.Background(), bearerRequest(old)) + require.Error(t, err) + assert.ErrorIs(t, err, bearer.ErrInvalid) + assert.True(t, p.IsAnonymous(), "a refused credential must yield no authority") + assert.Equal(t, http.StatusUnauthorized, StatusFor(err)) - // 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)) + // The push path refuses it for the same reason and through the same call. + _, err = f.rs.ResolveAgent(context.Background(), old, "claude-code", "s-1") + assert.ErrorIs(t, err, bearer.ErrInvalid) - // 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") + _, code, reached := runMiddleware(t, f.rs, bearerRequest(old)) + assert.False(t, reached) + assert.Equal(t, http.StatusUnauthorized, code) } func TestResolve_InstanceTokenAccepted(t *testing.T) { @@ -200,7 +196,58 @@ 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") +} + +// ResolveAgent is what the SSH push path calls: the same validator, the same +// refusals, with the provenance passed as arguments because a hook has no +// headers to read them from. +func TestResolveAgent_IsTheSameCheckAsTheHTTPPlane(t *testing.T) { + f := newPlaneFixture(t, http.StatusNoContent) + + p, err := f.rs.ResolveAgent(context.Background(), instanceToken("spec:propose"), + "claude-code/spec-writer", "s-1") + require.NoError(t, err) + assert.True(t, p.IsAgent()) + assert.Equal(t, PlaneInstance, p.Plane) + assert.Equal(t, "bigbes", p.Owner) + assert.Equal(t, "claude-code/spec-writer", p.Agent) + assert.Equal(t, "s-1", p.Session) + assert.NoError(t, p.Authorize(ActionPropose)) + assert.ErrorIs(t, p.Authorize(ActionRead), ErrMissingGrant) + + // No credential at all is ErrNoToken, not an anonymous principal: a caller + // that asked to authenticate an agent and passed nothing has a bug. + _, err = f.rs.ResolveAgent(context.Background(), "", "claude-code", "s-1") + assert.ErrorIs(t, err, ErrNoToken) + assert.True(t, IsAuthFailure(err)) +} + +// A resolver with no agent plane — an instance whose config.ini has no +// [tokens.sr.ht] origin — refuses every credential, and does it as a backend +// failure rather than as a bad token: the holder's credential may be perfect and +// re-provisioning it would not help. +func TestResolveAgent_WithoutAPlaneIsAWiringFailure(t *testing.T) { + rs, err := NewResolver("bigbes") + require.NoError(t, err) + assert.False(t, rs.HasInstancePlane()) + + tok := instanceToken("spec:propose") + p, err := rs.ResolveAgent(context.Background(), tok, "claude-code", "s-1") + require.Error(t, err) + assert.ErrorIs(t, err, ErrNoAgentPlane) + assert.True(t, p.IsAnonymous()) + assert.False(t, IsAuthFailure(err), "a service that cannot check is not a bad credential") + assert.Equal(t, http.StatusServiceUnavailable, StatusFor(err)) + + _, code, reached := runMiddleware(t, rs, bearerRequest(tok)) + assert.False(t, reached) + assert.Equal(t, http.StatusServiceUnavailable, code) + + // The cookie plane is unaffected: browsing an instance with no tokens.sr.ht + // still works, it just has no agent to serve. + owner, err := rs.Resolve(context.Background(), request(sealCookie(t, "bigbes"), nil)) + require.NoError(t, err) + assert.True(t, owner.IsOwner()) } func TestResolve_InstanceTokenMissingAGrantStillAuthenticates(t *testing.T) { @@ -216,13 +263,12 @@ 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) { +// A revoked token is refused outright. There is no second door for it to be +// re-tried at, which is the property the local plane's removal makes structural +// rather than merely intended. +func TestResolve_RevokedInstanceTokenIsRefused(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) @@ -231,26 +277,22 @@ 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() +// An unreachable tokens.sr.ht is 503 and never 401. Reading "I could not ask" +// as "revoked" would refuse every live token on the instance while a daemon +// that is deliberately off the hot path restarts. +func TestResolve_UnreachableDaemonIs503(t *testing.T) { users := newStubUsers() - rs, err := NewResolver("bigbes", store, + rs, err := NewResolver("bigbes", 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) @@ -258,46 +300,41 @@ 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, +// The two refusals that used to fall through to spec's own store. With one +// plane they are plain 401s — and for the meta PAT that is a change of status +// as well as of path: ErrNotOurs joined IsAuthFailure when the store it used to +// be handed to went away, so it answers 401 rather than the 503 an unclassified +// error would have earned. +func TestResolve_ForeignAndUndecodableTokensAreRefused(t *testing.T) { + for name, c := range map[string]struct { + presented string + want error + }{ + "opaque secret from the old plane": {"live-token", bearer.ErrInvalid}, + "expired instance token": { + seal("bigbes", bearer.TokensClientID, "spec:propose", time.Now().Add(-time.Hour)), + bearer.ErrInvalid, + }, + "meta.sr.ht PAT": { + seal("bigbes", "meta.sr.ht", "git.sr.ht/OBJECTS:RW", time.Now().Add(time.Hour)), + bearer.ErrNotOurs, + }, } { 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") + f := newPlaneFixture(t, http.StatusNoContent) - 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)) - }) + p, err := f.rs.Resolve(context.Background(), bearerRequest(c.presented)) + require.Error(t, err) + assert.ErrorIs(t, err, c.want) + assert.True(t, p.IsAnonymous()) + assert.True(t, IsAuthFailure(err), "a credential this service does not take is permanent") + assert.Equal(t, http.StatusUnauthorized, StatusFor(err)) }) } } @@ -316,7 +353,6 @@ 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) @@ -334,40 +370,23 @@ 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())) + _, err := NewResolver("bigbes", WithInstancePlane(nil, newStubUsers())) assert.Error(t, err, "a plane with no validator must be refused") - _, err = NewResolver("bigbes", newStubStore(), WithInstancePlane(v, nil)) + _, err = NewResolver("bigbes", 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) { +// Provenance is mandatory on every agent write. Grants do not replace it and do +// not excuse it, and neither does the plane the agent came in on. +func TestAgentWriteFor_ProvenanceRequired(t *testing.T) { base := "1f0c1d1a1e2b3c4d5e6f708192a3b4c5d6e7f809" - for _, plane := range []Plane{PlaneLocal, PlaneInstance} { - t.Run(string(plane), func(t *testing.T) { + for name, plane := range map[string]Plane{ + "instance token": PlaneInstance, + "locally asserted agent": Plane(""), + } { + t.Run(name, func(t *testing.T) { complete := Principal{ Kind: KindAgent, Owner: "bigbes", Agent: "a", Session: "s-1", Plane: plane, Grants: mustGrants(t, "*"), @@ -392,8 +411,9 @@ 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 + // The CLI's locally asserted agent: no credential, so no grant to + // clip. The resolver never produces one of these. + {Kind: KindAgent, Owner: "bigbes"}, } { assert.NoError(t, p.Authorize(ActionPropose)) assert.NoError(t, p.Authorize(ActionRead)) @@ -435,8 +455,9 @@ {"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}, + {"token from another issuer", bearer.ErrNotOurs, http.StatusUnauthorized}, + {"no credential presented", ErrNoToken, http.StatusUnauthorized}, + {"no agent plane configured", ErrNoAgentPlane, http.StatusServiceUnavailable}, {"store outage", errNoSuchUser("postgres"), http.StatusServiceUnavailable}, } for _, c := range cases { diff --git a/authn/doc.go b/authn/doc.go index 70dd7755b50d280c6e31f5d49d0ba2eb24951a9b..3670a64a76f5b9036e686fdb06060e214829778a 100644 --- a/authn/doc.go +++ b/authn/doc.go @@ -14,37 +14,34 @@ // 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 +// # One agent credential plane // -// An agent is recognised on either of two planes, and Principal.Plane says -// which: +// An agent is recognised by a tokens.sr.ht working token — PlaneInstance: +// 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 it is the only credential this service authenticates an +// agent with. // -// - 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. +// spec used to mint its own as well — the agent_token row: one instance-wide +// shared secret, hashed at rest, with no owner, no expiry and no grants. That +// plane is gone. Issuance is centralised in tokens.sr.ht, so there is one door +// and nothing behind it: a credential this plane refuses is refused, rather than +// being offered to a second store that might say yes. A well-formed token from +// another issuer (a meta.sr.ht PAT — bearer.ErrNotOurs) used to fall through to +// that store and now fails at the door, which is the same answer one hash lookup +// later, said honestly. // -// 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 instance plane names an owner where the local secret had none. +// Principal.Owner means "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. // // 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. +// agent write. // // The cookie and the bearer planes are deliberately asymmetric: // @@ -71,13 +68,12 @@ // 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 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. +// This package owns no storage and opens no connections. 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 ( @@ -87,27 +83,25 @@ "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 -// not answer — 503); IsAuthFailure draws it. +// permanent (the credential is bad — 401/403) versus transient (the backend +// could not answer — 503); IsAuthFailure draws it. +// +// Everything the credential itself can be wrong about is now spelled by +// sr-ht-ecore's bearer package — ErrInvalid, ErrNotOurs, ErrRevoked — because +// there is one issuer and one validator. The sentinels below are what this +// service adds on top of that answer. var ( // ErrNoToken is returned when a bearer credential was expected but the // request carried no Authorization header, or one in another scheme. ErrNoToken = errors.New("no agent token presented") - // ErrUnknownToken is the contract a TokenStore must honour: it is what - // LookupAgentToken returns (possibly wrapped) when no row matches the - // presented hash. Any other error is treated as transient, so a Postgres - // outage reads as "try again", never as "your token is bad". - ErrUnknownToken = errors.New("unknown agent token") - - // ErrInvalidToken marks a presented credential that is malformed, or a - // stored row whose hash does not actually match what was presented. - ErrInvalidToken = errors.New("invalid agent token") - - // ErrRevokedToken marks a token that resolved to a real row which has been - // revoked. Distinct from ErrUnknownToken so operators can tell "you are - // using a token I deliberately killed" from "that token never existed". - ErrRevokedToken = errors.New("revoked agent token") + // ErrNoAgentPlane is returned when a bearer credential is presented to a + // resolver that was built without the tokens.sr.ht plane — an instance whose + // config.ini has no [tokens.sr.ht] origin. It is a wiring failure and not a + // credential failure, so it is deliberately not an IsAuthFailure: telling an + // agent its token is bad when the truth is that this service cannot check + // any token would send it off to re-provision a perfectly good credential. + ErrNoAgentPlane = errors.New("no agent credential plane is configured") // ErrNotAgent is returned when agent provenance is demanded of a principal // that is not an agent — the human push path builds no trailers. @@ -149,19 +143,18 @@ // IsAuthFailure reports whether err is a permanent credential failure — the // 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 +// transient by definition, which is the fail-closed direction: a backend 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. +// bearer.ErrNotOurs joined the set when the local plane left it. A meta.sr.ht +// PAT used to fall through to spec's own store, where it missed; with one door +// there is nothing to fall through to, and "that credential was issued by +// somebody whose tokens this service does not take" is as permanent a refusal as +// a signature that does not verify. 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, bearer.ErrInvalid) || + errors.Is(err, bearer.ErrNotOurs) || errors.Is(err, bearer.ErrRevoked) } diff --git a/authn/principal.go b/authn/principal.go index 1c4a6c142fe6e28d910af3f0feaadab2744abf70..a813f7eadf6836531527d32f0f94525c5df17df9 100644 --- a/authn/principal.go +++ b/authn/principal.go @@ -25,27 +25,27 @@ // [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 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 is a bot holding a tokens.sr.ht working token. 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. +// One plane is left, and the field outlives its sibling because the distinction +// it draws is no longer "which of two stores said yes" but "was there a +// credential at all". Only a credential carries a grant set, and a check that +// reads Grants has to know whether there were any to read. +// +// Empty for every principal that is not an agent, and for the one agent that is +// not credential-backed: `specsrht doc propose`, which runs as the operator on +// the daemon's own host and names an agent for provenance rather than +// authenticating one. Resolver never produces an agent with an empty plane — +// every agent it resolves came through the tokens.sr.ht validator. 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" @@ -85,9 +85,9 @@ // same read/write asymmetry as Agent. Session string // 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. + // tokens.sr.ht row id, or "stateless" for a token short enough that the + // daemon never wrote it down. KindAgent only, diagnostics only — it grants + // nothing. TokenName string // CookieUser is whatever username the unified-login cookie carried, even @@ -100,14 +100,13 @@ // 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. + // PlaneInstance only; the zero value everywhere else, 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. + // resolved to. PlaneInstance only, and zero for a principal no credential + // backs. UserID int } @@ -146,12 +145,12 @@ // 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. +// A principal off the instance plane passes. That is not a hole left over from +// the agent_token days: grants describe machine credentials, and the principals +// with no plane are the owner's cookie — a person, whose authority is their +// identity — and the CLI's locally asserted agent, which runs as the operator on +// the daemon's host and presented nothing to have a grant clipped out of. Every +// agent the resolver produces is on the instance plane and is checked here. func (p Principal) Authorize(action string) error { if p.Plane != PlaneInstance { return nil @@ -180,9 +179,9 @@ if session == "" { session = "(no session)" } 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. + // Only a credential-backed agent is annotated: the grant set is what the + // annotation says, and an agent a local process asserted has none to + // print. if p.Plane == PlaneInstance { line += " (tokens.sr.ht: " + p.Grants.String() + ")" } diff --git a/authn/resolver.go b/authn/resolver.go index e858647fa9e9f1182d48f1b8e916fccc17c1b796..22bc886e46846d37bedd0a6fb6b8d428c2fca194 100644 --- a/authn/resolver.go +++ b/authn/resolver.go @@ -11,28 +11,27 @@ "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 — the -// TokenStore local agent tokens are checked against, and, when the instance is -// configured for it, the tokens.sr.ht plane. +// username — the one name a cookie has to match to carry authority — and, when +// the instance is configured for it, the tokens.sr.ht plane every agent +// credential is checked against. 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 and users are the agent plane, installed by WithInstancePlane. Both + // are nil when the instance config has no [tokens.sr.ht] section, and a + // resolver in that state authenticates no agent at all: since spec stopped + // minting its own credential there is nothing else for a bearer token to be + // checked against. It is still a legal resolver — the CLI paths that + // authenticate nobody build one — but a bearer credential presented to it is + // a hard ErrNoAgentPlane, never a shrug. 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. +// second constructor because the plane is genuinely absent in some processes: +// `specsrht doc` builds a Service, resolves nobody, and has no use for an HTTP +// client to tokens.sr.ht. type ResolverOption func(*Resolver) error // WithInstancePlane wires the tokens.sr.ht bearer plane in: v validates a @@ -40,7 +39,7 @@ // 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. +// which is the whole of what an agent credential is for here. func WithInstancePlane(v BearerValidator, users UserLookup) ResolverOption { return func(rs *Resolver) error { if v == nil { @@ -58,23 +57,16 @@ // NewResolver builds a Resolver for the instance owner named in // [sr.ht] owner-name. // -// 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. -// -// 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) { +// Pass WithInstancePlane to give it an agent plane. Without one it resolves +// cookies and refuses every bearer credential with ErrNoAgentPlane; the daemon +// therefore builds one with the plane and fails startup if it cannot, while the +// CLI paths that authenticate nobody build one without. +func NewResolver(owner string, opts ...ResolverOption) (*Resolver, error) { owner = strings.TrimPrefix(owner, "~") if err := core.ValidateOwner(owner); err != nil { return nil, fmt.Errorf("authn: instance owner: %w", err) } - if store == nil { - return nil, fmt.Errorf("authn: nil TokenStore") - } - rs := &Resolver{owner: owner, store: store} + rs := &Resolver{owner: owner} for _, opt := range opts { if err := opt(rs); err != nil { return nil, err @@ -83,9 +75,8 @@ } 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. +// HasInstancePlane reports whether this resolver can authenticate an agent at +// all. 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. @@ -99,41 +90,27 @@ // 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. +// A presented bearer token goes to the tokens.sr.ht plane and nowhere else. +// There is no second store behind it since spec stopped minting its own +// credential, so every refusal that plane returns is final — see +// resolveInstanceToken. // // The error contract is asymmetric on purpose: // // - No bearer token: never an error. The cookie decides between KindOwner and // KindAnonymous, and any cookie problem is anonymity, not failure. -// - A bearer token that fails: an error. IsAuthFailure separates the 401 case -// (unknown, revoked, malformed) from the 503 case (store unreachable). +// - A bearer token that fails: an error. StatusFor separates the 401 case +// (malformed, foreign, revoked) from the 403 case (a good token this +// instance has nothing to grant) and the 503 case (tokens.sr.ht +// unreachable, or no plane wired at all). // // The agent identity and session headers are read here but not required: they // 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 - } - return Principal{ - Kind: KindAgent, - Owner: rs.owner, - Agent: strings.TrimSpace(r.Header.Get(HeaderAgent)), - Session: strings.TrimSpace(r.Header.Get(HeaderAgentSession)), - TokenName: tok.Name, - Plane: PlaneLocal, - }, nil + return rs.ResolveAgent(ctx, presented, + r.Header.Get(HeaderAgent), r.Header.Get(HeaderAgentSession)) } username := UsernameFromRequest(r) @@ -148,6 +125,37 @@ // signed in as" affordance only. return Principal{Kind: KindAnonymous, CookieUser: username}, nil } return Principal{Kind: KindOwner, Owner: username, CookieUser: username}, nil +} + +// ResolveAgent authenticates a presented agent credential, with the provenance +// the caller collected alongside it, and is what Resolve calls once it has +// pulled all three out of an HTTP request. +// +// It is exported because the push path is not an HTTP request: a `git push` +// arrives over SSH and the credential reaches the daemon in a hook's +// environment, not in an Authorization header. That path used to check the +// agent_token table directly, which is precisely how it ended up unable to +// accept an instance token while the HTTP surfaces could. One credential plane +// deserves one implementation of "is this credential good?", so hooks calls this +// and the two surfaces cannot drift. +// +// It never returns an anonymous principal on failure: a presented credential +// that does not verify is an error, so the caller refuses at the door instead of +// silently downgrading an agent to a reader. +func (rs *Resolver) ResolveAgent(ctx context.Context, presented, agent, session string) (Principal, error) { + if presented == "" { + return Anonymous(), ErrNoToken + } + if rs.bearer == nil { + // Not a bad credential: this process cannot check any credential. 503 + // via StatusFor, and the operator's clue is in the message rather than + // in an agent's incident report about a token that "stopped working". + return Anonymous(), fmt.Errorf( + "%w: spec.sr.ht authenticates agents through tokens.sr.ht, and this instance's "+ + "config.ini has no [tokens.sr.ht] origin", ErrNoAgentPlane) + } + return rs.resolveInstanceToken(ctx, presented, + strings.TrimSpace(agent), strings.TrimSpace(session)) } // Middleware attaches the resolved Principal to the request context, where diff --git a/authn/resolver_test.go b/authn/resolver_test.go index a4d5145178449c7fdbae62184f96290987f96bcd..50187aab4a4a49b950766318213323e2651d7983 100644 --- a/authn/resolver_test.go +++ b/authn/resolver_test.go @@ -7,29 +7,27 @@ "net/http" "net/http/httptest" "reflect" "testing" + + "sourcecraft.dev/bigbes/sr-ht-ecore/bearer" ) -func newTestResolver(t *testing.T, store TokenStore) *Resolver { +// newTestResolver builds a resolver whose agent plane is wired to a daemon that +// answers "live" — the production shape. The cookie tests present no bearer +// credential and so never reach it. +func newTestResolver(t *testing.T) *Resolver { t.Helper() - rs, err := NewResolver("bigbes", store) - if err != nil { - t.Fatalf("NewResolver: %v", err) - } - return rs + return newPlaneFixture(t, http.StatusNoContent).rs } func TestNewResolver_RejectsBadWiring(t *testing.T) { - if _, err := NewResolver("bigbes", nil); err == nil { - t.Fatal("nil store must be rejected") - } - if _, err := NewResolver("", newStubStore()); err == nil { + if _, err := NewResolver(""); err == nil { t.Fatal("empty owner must be rejected") } - if _, err := NewResolver("Not A Name", newStubStore()); err == nil { + if _, err := NewResolver("Not A Name"); err == nil { t.Fatal("unusable owner must be rejected") } // The canonical "~user" spelling is accepted and normalised. - rs, err := NewResolver("~bigbes", newStubStore()) + rs, err := NewResolver("~bigbes") if err != nil { t.Fatalf("NewResolver(~bigbes): %v", err) } @@ -39,10 +37,9 @@ } } func TestResolve_OwnerCookie(t *testing.T) { - store := newStubStore() - rs := newTestResolver(t, store) + f := newPlaneFixture(t, http.StatusNoContent) - p, err := rs.Resolve(context.Background(), request(sealCookie(t, "bigbes"), nil)) + p, err := f.rs.Resolve(context.Background(), request(sealCookie(t, "bigbes"), nil)) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -55,13 +52,13 @@ } if p.Owner != "bigbes" { t.Fatalf("Owner = %q, want %q", p.Owner, "bigbes") } - if store.calls != 0 { - t.Fatalf("cookie path consulted the token store %d times, want 0", store.calls) + if f.users.calls != 0 || f.daemon.hits != 0 { + t.Fatal("the cookie path must not touch the agent credential plane") } } func TestResolve_AbsentCookieIsAnonymousNotAnError(t *testing.T) { - rs := newTestResolver(t, newStubStore()) + rs := newTestResolver(t) p, err := rs.Resolve(context.Background(), request("", nil)) if err != nil { t.Fatalf("an anonymous request must never error: %v", err) @@ -72,7 +69,7 @@ } } func TestResolve_BrokenCookiesAreAnonymousNotErrors(t *testing.T) { - rs := newTestResolver(t, newStubStore()) + rs := newTestResolver(t) for name, value := range map[string]string{ "tampered": tamper(t, sealCookie(t, "bigbes")), "garbage": "not-a-valid-fernet-token", @@ -94,7 +91,7 @@ // Single-user: a real user who is not bigbes has nothing granted to them, so // they read exactly as an anonymous viewer does. The name survives for logs // only, and must not be mistaken for authority. func TestResolve_NonOwnerCookieIsAnonymous(t *testing.T) { - rs := newTestResolver(t, newStubStore()) + rs := newTestResolver(t) p, err := rs.Resolve(context.Background(), request(sealCookie(t, "someone"), nil)) if err != nil { t.Fatalf("Resolve: %v", err) @@ -114,12 +111,10 @@ } } func TestResolve_AgentTokenAccepted(t *testing.T) { - store := newStubStore() - store.add("live-token", "laptop") - rs := newTestResolver(t, store) + f := newPlaneFixture(t, http.StatusNoContent) - p, err := rs.Resolve(context.Background(), request("", map[string]string{ - "Authorization": "Bearer live-token", + p, err := f.rs.Resolve(context.Background(), request("", map[string]string{ + "Authorization": "Bearer " + instanceToken("spec:propose spec:read"), HeaderAgent: "claude-code/spec-writer", HeaderAgentSession: "8fb9c9a4-b078-4af1-89eb-d97c522f9921", })) @@ -138,47 +133,18 @@ } if p.Owner != "bigbes" { t.Fatalf("agent acts for %q, want %q", p.Owner, "bigbes") } - if p.TokenName != "laptop" { - t.Fatalf("TokenName = %q, want %q", p.TokenName, "laptop") - } -} - -func TestResolve_AgentTokenRevokedAndUnknown(t *testing.T) { - store := newStubStore() - store.add("live-token", "laptop") - store.revoke("dead-token", "cron") - rs := newTestResolver(t, store) - - cases := map[string]struct { - token string - want error - }{ - "revoked": {"dead-token", ErrRevokedToken}, - "unknown": {"never-issued", ErrUnknownToken}, - } - for name, c := range cases { - t.Run(name, func(t *testing.T) { - p, err := rs.Resolve(context.Background(), request("", - map[string]string{"Authorization": "Bearer " + c.token})) - if !errors.Is(err, c.want) { - t.Fatalf("error = %v, want %v", err, c.want) - } - if !p.IsAnonymous() { - t.Fatalf("a refused token must yield no authority: %+v", p) - } - }) + if p.TokenName != "tokens.sr.ht (stateless)" { + t.Fatalf("TokenName = %q, want the tokens.sr.ht label", p.TokenName) } } // A bearer token wins over a cookie. Letting a stale browser cookie promote a // token-bearing request to the owner would hand an agent the approved branch. func TestResolve_BearerBeatsCookie(t *testing.T) { - store := newStubStore() - store.add("live-token", "laptop") - rs := newTestResolver(t, store) + f := newPlaneFixture(t, http.StatusNoContent) - p, err := rs.Resolve(context.Background(), request(sealCookie(t, "bigbes"), - map[string]string{"Authorization": "Bearer live-token"})) + p, err := f.rs.Resolve(context.Background(), request(sealCookie(t, "bigbes"), + map[string]string{"Authorization": "Bearer " + instanceToken("spec:propose")})) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -194,13 +160,12 @@ // A bad token is a hard failure even when a valid owner cookie is present: an // agent silently downgraded to a reader fails confusingly at its first write // instead of clearly at the door. func TestResolve_BadTokenFailsEvenWithOwnerCookie(t *testing.T) { - store := newStubStore() - rs := newTestResolver(t, store) + f := newPlaneFixture(t, http.StatusNoContent) - _, err := rs.Resolve(context.Background(), request(sealCookie(t, "bigbes"), + _, err := f.rs.Resolve(context.Background(), request(sealCookie(t, "bigbes"), map[string]string{"Authorization": "Bearer never-issued"})) - if !errors.Is(err, ErrUnknownToken) { - t.Fatalf("error = %v, want ErrUnknownToken", err) + if !errors.Is(err, bearer.ErrInvalid) { + t.Fatalf("error = %v, want bearer.ErrInvalid", err) } } @@ -218,9 +183,7 @@ return got, rec.Code, reached } func TestMiddleware_AttachesPrincipal(t *testing.T) { - store := newStubStore() - store.add("live-token", "laptop") - rs := newTestResolver(t, store) + rs := newTestResolver(t) t.Run("owner", func(t *testing.T) { p, code, reached := runMiddleware(t, rs, request(sealCookie(t, "bigbes"), nil)) @@ -244,7 +207,7 @@ }) t.Run("agent", func(t *testing.T) { p, code, reached := runMiddleware(t, rs, request("", map[string]string{ - "Authorization": "Bearer live-token", + "Authorization": "Bearer " + instanceToken("spec:propose spec:read"), HeaderAgent: "claude-code/spec-writer", HeaderAgentSession: "sess-1", })) @@ -258,7 +221,7 @@ }) } func TestMiddleware_RejectsBadToken(t *testing.T) { - rs := newTestResolver(t, newStubStore()) + rs := newTestResolver(t) _, code, reached := runMiddleware(t, rs, request("", map[string]string{"Authorization": "Bearer never-issued"})) if reached { @@ -269,15 +232,16 @@ t.Fatalf("code = %d, want 401", code) } } -func TestMiddleware_StoreOutageIs503(t *testing.T) { - store := newStubStore() - store.err = errors.New("connection refused") - rs := newTestResolver(t, store) +// A user lookup that cannot answer is a backend outage, not a bad credential: +// fail closed with a 503 rather than telling a live agent its token is bad. +func TestMiddleware_BackendOutageIs503(t *testing.T) { + f := newPlaneFixture(t, http.StatusNoContent) + f.users.err = errors.New("connection refused") - _, code, reached := runMiddleware(t, rs, request("", - map[string]string{"Authorization": "Bearer live-token"})) + _, code, reached := runMiddleware(t, f.rs, request("", + map[string]string{"Authorization": "Bearer " + instanceToken("spec:read")})) if reached { - t.Fatal("a store outage must fail closed, not reach the handler") + t.Fatal("a backend outage must fail closed, not reach the handler") } if code != http.StatusServiceUnavailable { t.Fatalf("code = %d, want 503", code) @@ -336,10 +300,11 @@ 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. + // An agent a local process asserted (the CLI) carries no credential and + // so no grant set: the annotation appears only where there is something + // to say. { - Principal{Kind: KindAgent, Owner: "bigbes", Agent: "a", Session: "s-1", Plane: PlaneLocal}, + Principal{Kind: KindAgent, Owner: "bigbes", Agent: "a", Session: "s-1"}, "agent a session s-1 for ~bigbes", }, { diff --git a/authn/token.go b/authn/token.go index c638c6f5723fe1813fbd90939c48dab5dbd24784..5308176b97cbf59a6c61dbeb4703ed2f2924c0ba 100644 --- a/authn/token.go +++ b/authn/token.go @@ -1,15 +1,8 @@ package authn import ( - "context" - "crypto/sha256" - "crypto/subtle" - "database/sql" - "errors" - "fmt" "net/http" "strings" - "time" ) // bearerScheme is the Authorization scheme agents present the token under. @@ -29,71 +22,11 @@ HeaderAgent = "X-Agent" HeaderAgentSession = "X-Agent-Session" ) -// AgentToken is one row of the agent_token table, in the shape this package -// needs. It is declared here rather than in db/ so that authn owns its own -// input contract and the dependency arrow keeps pointing downward. -type AgentToken struct { - // ID is the agent_token primary key. Diagnostics and audit only. - ID int64 - - // Name is the operator-facing label of the token ("laptop", "cron"). With - // one token and no scopes it grants nothing; it exists so a revocation can - // be aimed at something a human recognises. - Name string - - // Hash is the stored sha256 of the token as issued. ResolveAgentToken - // re-checks it against the presented token in constant time rather than - // trusting that the store's lookup was an exact match. - Hash []byte - - // Created is when the token was issued. - Created time.Time - - // Revoked is when the token was revoked, or nil while it is live. A - // revoked row still resolves from the store — it is this package that - // turns it into a refusal, so the refusal can say "revoked" rather than - // "unknown". - Revoked *time.Time -} - -// IsRevoked reports whether the token has been revoked. -func (t AgentToken) IsRevoked() bool { return t.Revoked != nil } - -// TokenStore is the sliver of db/ that authn needs: look up an agent token row -// by the hash of the presented secret. service/ wires the real Postgres -// implementation in; tests wire a map. -// -// Contract: -// -// - hash is the value HashToken returned; the implementation must match it -// against agent_token.token_hash exactly, never by prefix. -// - When no row matches, return an error satisfying -// errors.Is(err, ErrUnknownToken). sql.ErrNoRows is accepted as an -// equivalent spelling, since that is what a bare QueryRow().Scan() yields. -// - Any other error is taken to be transient (Postgres down, context -// cancelled) and is surfaced as such, never as a bad credential. -// -// The interface takes no scope or space argument on purpose: v1 has one agent -// token and no per-space scoping, and the boundary that actually bounds damage -// is the refs rule in gitx. Adding scopes later is a column here and a filter -// clause there, not a reshaping of this interface. -type TokenStore interface { - LookupAgentToken(ctx context.Context, hash []byte) (AgentToken, error) -} - -// HashToken returns the sha256 of a presented agent token — the value stored in -// agent_token.token_hash and the only form of the secret this service keeps. -// The token itself is opaque and high-entropy, so a plain hash is sufficient: -// there is no low-entropy password here for a KDF to slow down guessing of. -func HashToken(token string) []byte { - sum := sha256.Sum256([]byte(token)) - return sum[:] -} - // BearerFromRequest returns the token from an "Authorization: Bearer " // header, or "" when the header is absent or uses another scheme. Anything // after the scheme is returned verbatim apart from surrounding whitespace: the -// token is opaque and this package is not the place to guess at its grammar. +// token is opaque to this function, and validating its grammar is +// BearerValidator's job and not the header parser's. func BearerFromRequest(r *http.Request) string { h := r.Header.Get("Authorization") if h == "" { @@ -105,54 +38,3 @@ return "" } return strings.TrimSpace(rest) } - -// ResolveAgentToken validates a presented agent token against the store. -// -// Unlike the cookie path this fails loudly. An agent that presented a -// credential and got silently downgraded to an anonymous reader would sail -// through its reads and then fail incomprehensibly at its first propose; a 401 -// at the door is the only useful answer. -// -// Permanent refusals (ErrNoToken, ErrUnknownToken, ErrRevokedToken, -// ErrInvalidToken) satisfy IsAuthFailure. A store failure is returned wrapped -// and does not, so callers answer 503 rather than 401 and agents retry rather -// than re-provision. -func ResolveAgentToken(ctx context.Context, store TokenStore, presented string) (AgentToken, error) { - if store == nil { - // A nil store is a wiring bug, not a credential problem. Refusing - // loudly beats resolving every token as unknown, which would look like - // a revocation storm. - return AgentToken{}, errors.New("authn: nil TokenStore") - } - if presented == "" { - return AgentToken{}, ErrNoToken - } - - hash := HashToken(presented) - tok, err := store.LookupAgentToken(ctx, hash) - switch { - case err == nil: - // fall through - case errors.Is(err, ErrUnknownToken), errors.Is(err, sql.ErrNoRows): - // Two spellings of "no such row"; ErrUnknownToken is the contract, - // sql.ErrNoRows is what an unwrapped Scan leaks. - return AgentToken{}, fmt.Errorf("%w: no agent token matches the presented secret", ErrUnknownToken) - default: - return AgentToken{}, fmt.Errorf("looking up agent token: %w", err) - } - - // Re-check the hash ourselves, in constant time. The store's WHERE clause - // already did an equality match, but this is the one comparison that - // decides authentication and it costs a memcmp to not depend on somebody - // else getting it right. - if subtle.ConstantTimeCompare(tok.Hash, hash) != 1 { - return AgentToken{}, fmt.Errorf("%w: store returned a row whose hash does not match the presented token", ErrInvalidToken) - } - - if tok.IsRevoked() { - return AgentToken{}, fmt.Errorf("%w: token %q was revoked at %s", - ErrRevokedToken, tok.Name, tok.Revoked.UTC().Format(time.RFC3339)) - } - - return tok, nil -} diff --git a/authn/token_test.go b/authn/token_test.go index 2464278bafd768bfddf5e24f3be74699a1f4db65..1b24b6d38c481e6b4f3f0909f93668bf1f6f42ab 100644 --- a/authn/token_test.go +++ b/authn/token_test.go @@ -1,29 +1,11 @@ package authn import ( - "bytes" - "context" - "database/sql" - "errors" "net/http" "net/http/httptest" "testing" ) -func TestHashToken_IsSHA256AndStable(t *testing.T) { - a := HashToken("s3cret") - b := HashToken("s3cret") - if len(a) != 32 { - t.Fatalf("hash length = %d, want 32", len(a)) - } - if !bytes.Equal(a, b) { - t.Fatal("hashing the same token twice produced different values") - } - if bytes.Equal(a, HashToken("s3cres")) { - t.Fatal("distinct tokens hashed to the same value") - } -} - func TestBearerFromRequest(t *testing.T) { cases := map[string]struct{ header, want string }{ "bearer": {"Bearer abc123", "abc123"}, @@ -46,116 +28,3 @@ } }) } } - -func TestResolveAgentToken_Accepted(t *testing.T) { - store := newStubStore() - want := store.add("live-token", "laptop") - - got, err := ResolveAgentToken(context.Background(), store, "live-token") - if err != nil { - t.Fatalf("ResolveAgentToken: %v", err) - } - if got.ID != want.ID || got.Name != "laptop" { - t.Fatalf("resolved %+v, want id %d name %q", got, want.ID, "laptop") - } - if got.IsRevoked() { - t.Fatal("live token reported as revoked") - } -} - -func TestResolveAgentToken_Revoked(t *testing.T) { - store := newStubStore() - store.revoke("dead-token", "cron") - - _, err := ResolveAgentToken(context.Background(), store, "dead-token") - if !errors.Is(err, ErrRevokedToken) { - t.Fatalf("error = %v, want ErrRevokedToken", err) - } - // A revoked token must not be reported as unknown: the operator needs to - // tell "I killed this" from "this never existed". - if errors.Is(err, ErrUnknownToken) { - t.Fatalf("revoked token also reported as unknown: %v", err) - } - if !IsAuthFailure(err) { - t.Fatalf("revocation must be a permanent auth failure: %v", err) - } -} - -func TestResolveAgentToken_Unknown(t *testing.T) { - store := newStubStore() - store.add("live-token", "laptop") - - _, err := ResolveAgentToken(context.Background(), store, "never-issued") - if !errors.Is(err, ErrUnknownToken) { - t.Fatalf("error = %v, want ErrUnknownToken", err) - } - if !IsAuthFailure(err) { - t.Fatalf("unknown token must be a permanent auth failure: %v", err) - } -} - -// db/ may hand back a bare sql.ErrNoRows from QueryRow().Scan(); it means the -// same thing as ErrUnknownToken and must not be mistaken for a store outage. -func TestResolveAgentToken_SQLNoRowsIsUnknown(t *testing.T) { - store := newStubStore() - store.err = sql.ErrNoRows - - _, err := ResolveAgentToken(context.Background(), store, "whatever") - if !errors.Is(err, ErrUnknownToken) { - t.Fatalf("error = %v, want ErrUnknownToken", err) - } -} - -func TestResolveAgentToken_EmptyIsNoToken(t *testing.T) { - store := newStubStore() - _, err := ResolveAgentToken(context.Background(), store, "") - if !errors.Is(err, ErrNoToken) { - t.Fatalf("error = %v, want ErrNoToken", err) - } - if store.calls != 0 { - t.Fatalf("store consulted %d times for an absent token, want 0", store.calls) - } -} - -// A store outage must never read as a bad credential: fail closed, but tell the -// caller it is transient so it answers 503 and the agent retries. -func TestResolveAgentToken_StoreFailureIsTransient(t *testing.T) { - boom := errors.New("connection refused") - store := newStubStore() - store.add("live-token", "laptop") - store.err = boom - - _, err := ResolveAgentToken(context.Background(), store, "live-token") - if !errors.Is(err, boom) { - t.Fatalf("error = %v, want it to wrap the store error", err) - } - if IsAuthFailure(err) { - t.Fatalf("store failure must not be a permanent auth failure: %v", err) - } -} - -// The constant-time re-check exists so that a store which matched loosely — by -// prefix, or on the wrong column — cannot authenticate anybody. -func TestResolveAgentToken_HashMismatchRejected(t *testing.T) { - store := newStubStore() - store.put(HashToken("presented"), AgentToken{ - ID: 7, - Name: "sloppy-store", - Hash: HashToken("something-else"), - }) - - _, err := ResolveAgentToken(context.Background(), store, "presented") - if !errors.Is(err, ErrInvalidToken) { - t.Fatalf("error = %v, want ErrInvalidToken", err) - } -} - -func TestResolveAgentToken_NilStoreIsNotAnAuthFailure(t *testing.T) { - _, err := ResolveAgentToken(context.Background(), nil, "live-token") - if err == nil { - t.Fatal("nil store must be an error") - } - if IsAuthFailure(err) { - t.Fatalf("a wiring bug must not read as a bad credential: %v", err) - } -} diff --git a/cmd/specsrht-migrate/main_test.go b/cmd/specsrht-migrate/main_test.go index 6ad0083658dd189e06145875c44d1e0b0cb94a69..64f65e4f4b6e58b0be055ea61eacd8fe266ca9e2 100644 --- a/cmd/specsrht-migrate/main_test.go +++ b/cmd/specsrht-migrate/main_test.go @@ -428,10 +428,16 @@ db, err := p.DB() if err != nil { t.Fatalf("db: %v", err) } - for _, table := range []string{"space", "document_id", "proposal", "agent_token", "index_stamp", "digest_mark"} { + for _, table := range []string{"space", "document_id", "proposal", "index_stamp", "digest_mark"} { var n int if err := db.QueryRow(`SELECT count(*) FROM ` + table).Scan(&n); err != nil { t.Errorf("table %s missing after up: %v", table, err) } + } + + // And agent_token must be gone: 0005 drops it, and a database that still had + // it would still have a second door into the write plane. + if err := db.QueryRow(`SELECT count(*) FROM agent_token`).Scan(new(int)); err == nil { + t.Error("agent_token survived the migrations; agent credentials come from tokens.sr.ht") } } diff --git a/cmd/specsrht/doc.go b/cmd/specsrht/doc.go index 687a5d41aed0e4592b4495667d27383a8e88d965..560d50f011017e97bc6d99ba378b60b48107db73 100644 --- a/cmd/specsrht/doc.go +++ b/cmd/specsrht/doc.go @@ -36,9 +36,11 @@ // // The two agent surfaces are remote and therefore need a bearer token; this one // is not. It runs on the host, with the repositories and Postgres already in // hand, and constructs the agent principal directly rather than resolving one -// from an agent_token row. That is not a hole: a process that can already open -// the database and the bare repositories can do anything the token would let it -// do, and demanding a credential from it would only be ceremony. Provenance is +// from a presented credential. That is not a hole: a process that can already +// open the database and the bare repositories can do anything a token would let +// it do, and demanding a credential from it would only be ceremony — which is +// also why the principal it builds carries no credential plane, and so no grant +// for authn.Principal.Authorize to check. Provenance is // *not* waived, though — --agent and --session are recorded exactly as a remote // agent's are, so a `git log` cannot tell a proposal opened here from one opened // over HTTP, and neither can a reviewer. diff --git a/cmd/specsrht/main.go b/cmd/specsrht/main.go index 6122b8e7daee4ab338b82495ef39a814178608b7..cdb6e49d81e918bfcc0dc75967dead01a1c08390 100644 --- a/cmd/specsrht/main.go +++ b/cmd/specsrht/main.go @@ -24,18 +24,21 @@ // explicit form `specsrht hook ` does the same thing by hand. // // # Admin commands // -// Three subcommands run and exit without binding anything, so they are safe to +// Two subcommands run and exit without binding anything, so they are safe to // invoke while the daemon is up: // // specsrht space create ~owner/name | specsrht space list -// specsrht token create | specsrht token list | specsrht token revoke // specsrht doc propose ~owner/space ... [--as path] [--title t] // -// The first two are the only entry points spaces and agent tokens have. A -// deployment without both is inert: nothing to hold documents, and no credential -// for the remote agent write plane, which refuses an anonymous caller by design. -// The third opens a proposal from files on this host, for when the documents and -// the operator are already here and a bearer token would be ceremony. +// The first is the only entry point a space has, and a deployment without one +// is inert: there is nothing to hold documents. The second opens a proposal +// from files on this host, for when the documents and the operator are already +// here and a bearer token would be ceremony. +// +// There is no `specsrht token`. It minted spec's own agent credential; agent +// credentials are tokens.sr.ht working tokens now, so they are minted there — +// its /tokens page, or POST /exchange with a parent token — and carry the +// spec:propose grant to write and spec:read to read. // // # Flags // @@ -132,7 +135,6 @@ // to invoke while the daemon holds the hook socket. if len(os.Args) > 1 { admin := map[string]func([]string) error{ "space": runSpace, - "token": runToken, "doc": runDoc, } if cmd := os.Args[1]; admin[cmd] != nil { @@ -269,16 +271,16 @@ return err } defer pool.Close() - // 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. + // WithInstanceTokens builds the daemon's one agent credential plane. It is + // required, not offered: spec.sr.ht mints no credential of its own any more, + // so an instance with no [tokens.sr.ht] section could authenticate no agent + // at all, over HTTP or over `git push`. service.New fails here rather than + // letting the daemon come up and refuse every agent one request at a time. 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()) + log.Info("agent credential plane", "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/cmd/specsrht/token.go b/cmd/specsrht/token.go deleted file mode 100644 index d2e5f572a42578d4c4cbe612d6a884be46f39899..0000000000000000000000000000000000000000 --- a/cmd/specsrht/token.go +++ /dev/null @@ -1,136 +0,0 @@ -package main - -import ( - "context" - "errors" - "fmt" - "os" - "strconv" - "text/tabwriter" - "time" - - "sourcecraft.dev/bigbes/sr-ht-core/config" - - "sourcecraft.dev/bigbes/sr-ht-spec/authn" - "sourcecraft.dev/bigbes/sr-ht-spec/service" -) - -const tokenUsage = "usage: specsrht token create | specsrht token list | specsrht token revoke " - -// runToken is the agent-token administration command: -// -// specsrht token create -// specsrht token list -// specsrht token revoke -// -// Agent tokens have no other entry point, and without one a freshly deployed -// instance cannot be written to at all: both agent write surfaces — the REST -// PUT and mcpsrv's spec_propose — refuse an anonymous caller, and the human -// path (native receive-pack) is approval rather than proposal. The alternative -// to this command is an operator hand-writing an INSERT with a sha256 hash, -// which is exactly the shape of mistake that ends with an unusable credential -// and no way to tell why. -// -// It goes through service/ rather than db/ directly, and constructs the owner -// principal to do so. The owner-only rule on minting is the one that makes -// revocation mean anything — an agent that could mint would survive having its -// credential revoked — so it is spelled once, in service/, and this command is -// held to it exactly as the /tokens page is. A process on this host could of -// course write the row itself; the point is not to confine it but to keep one -// implementation of what issuing a token *is*. -func runToken(args []string) error { - if len(args) == 0 { - return errors.New(tokenUsage) - } - - conf := config.LoadConfig() - cfg, err := validateConfig(conf) - if err != nil { - return err - } - pool, err := openDatabase(cfg.ConnectionString) - if err != nil { - return err - } - defer pool.Close() - - svc, err := service.New(cfg, pool) - if err != nil { - return err - } - owner := authn.Principal{Kind: authn.KindOwner, Owner: cfg.Instance.OwnerName} - ctx := context.Background() - - switch args[0] { - case "create": - if len(args) != 2 { - return errors.New("usage: specsrht token create ") - } - name := args[1] - - // The plaintext is returned by the mint and printed once. Nothing writes - // it to the log, because a token in a log file is a token in a backup. - token, row, err := svc.IssueAgentToken(ctx, owner, name) - if err != nil { - return err - } - fmt.Printf("created agent token %q (id %d)\n\n %s\n\n"+ - "This is the only time the token is shown — only its hash is stored.\n"+ - "Agents present it as: Authorization: Bearer \n", - row.Name, row.ID, token) - return nil - - case "list": - tokens, err := svc.ListAgentTokens(ctx, owner) - if err != nil { - return err - } - w := tabwriter.NewWriter(os.Stdout, 0, 0, 2, ' ', 0) - fmt.Fprintln(w, "ID\tNAME\tCREATED\tSTATE") - for _, t := range tokens { - fmt.Fprintln(w, formatTokenRow(t)) - } - return w.Flush() - - case "revoke": - if len(args) != 2 { - return errors.New("usage: specsrht token revoke ") - } - id, err := parseTokenID(args[1]) - if err != nil { - return err - } - if err := svc.RevokeAgentToken(ctx, owner, id); err != nil { - return err - } - fmt.Printf("revoked agent token %d\n", id) - return nil - - default: - return fmt.Errorf("unknown subcommand %q: want create, list or revoke", args[0]) - } -} - -// formatTokenRow renders one token as a tab-separated line for `token list`. -// The hash is deliberately not shown: it identifies nothing an operator acts -// on, and printing a column of it would only invite treating it as the -// credential. -func formatTokenRow(t service.AgentToken) string { - state := "active" - if !t.Active() { - state = "revoked " + t.Revoked.Format(time.RFC3339) - } - return fmt.Sprintf("%d\t%s\t%s\t%s", t.ID, t.Name, t.Created.Format(time.RFC3339), state) -} - -// parseTokenID reads the id argument of `token revoke`. It rejects anything -// that is not a positive integer here rather than letting a typo become an -// UPDATE that matches no row and reports "not found", which reads like the -// token is already gone. -func parseTokenID(s string) (int, error) { - id, err := strconv.Atoi(s) - if err != nil || id <= 0 { - return 0, fmt.Errorf("token id %q is not a positive integer; `specsrht token list` shows the ids", s) - } - return id, nil -} diff --git a/cmd/specsrht/token_test.go b/cmd/specsrht/token_test.go deleted file mode 100644 index 1f354acf171ad751895a35a04ee6e11cfb6932a1..0000000000000000000000000000000000000000 --- a/cmd/specsrht/token_test.go +++ /dev/null @@ -1,70 +0,0 @@ -package main - -import ( - "strings" - "testing" - "time" - - "sourcecraft.dev/bigbes/sr-ht-spec/service" -) - -func TestParseTokenIDRejectsWhatIsNotAnID(t *testing.T) { - for _, in := range []string{"", "0", "-1", "3.0", "abc", " 3", "3 "} { - if _, err := parseTokenID(in); err == nil { - t.Errorf("parseTokenID(%q) was accepted", in) - } - } - id, err := parseTokenID("42") - if err != nil { - t.Fatalf("parseTokenID(\"42\"): %v", err) - } - if id != 42 { - t.Errorf("parseTokenID(\"42\") = %d want 42", id) - } -} - -// TestParseTokenIDPointsAtTheListing keeps the failure actionable: an operator -// who mistyped an id needs to be told where the ids come from, not just that -// this one was wrong. -func TestParseTokenIDPointsAtTheListing(t *testing.T) { - _, err := parseTokenID("nope") - if err == nil { - t.Fatal("parseTokenID accepted a non-numeric id") - } - if !strings.Contains(err.Error(), "specsrht token list") { - t.Errorf("the error does not say how to find the ids:\n%v", err) - } -} - -func TestFormatTokenRow(t *testing.T) { - created := time.Date(2026, 8, 5, 9, 30, 0, 0, time.UTC) - revoked := created.Add(24 * time.Hour) - - active := formatTokenRow(service.AgentToken{ID: 1, Name: "claude", Created: created}) - if want := "1\tclaude\t2026-08-05T09:30:00Z\tactive"; active != want { - t.Errorf("active row = %q want %q", active, want) - } - - dead := formatTokenRow(service.AgentToken{ID: 2, Name: "old", Created: created, Revoked: &revoked}) - if want := "2\told\t2026-08-05T09:30:00Z\trevoked 2026-08-06T09:30:00Z"; dead != want { - t.Errorf("revoked row = %q want %q", dead, want) - } -} - -// There is no test that the listing keeps the stored hash out: service.AgentToken -// has no hash field, so the type is the guarantee and a test would only assert -// that Go's struct literals work. - -// TestRunTokenRejectsBadInvocationsBeforeTheDatabase guards the ordering that -// makes this command usable on a workstation: a usage error must be reported -// without a config file or a Postgres connection, which is what an operator -// typing it blind has. -func TestRunTokenRejectsBadInvocationsBeforeTheDatabase(t *testing.T) { - err := runToken(nil) - if err == nil { - t.Fatal("runToken accepted an empty argument list") - } - if err.Error() != tokenUsage { - t.Errorf("empty invocation did not print the usage line:\n%v", err) - } -} diff --git a/db/store.go b/db/store.go index 84ef0d3f5be2acb8a5a3e217d1d364693094b399..f7ae3c62947d2ad4173d64b56d0ca591fddbb5ea 100644 --- a/db/store.go +++ b/db/store.go @@ -1,7 +1,12 @@ // Package db is the PostgreSQL persistence layer for spec.sr.ht. It maps the -// eight tables of schema.sql — space, document_id, proposal, agent_token, -// index_stamp, digest_mark, project and project_space — to core value types -// with plain database/sql and $n placeholders (no ORM). +// tables of schema.sql — space, document_id, proposal, index_stamp, +// digest_mark, project, project_space and comment — to core value types with +// plain database/sql and $n placeholders (no ORM). +// +// It holds no credential of any kind. agent_token lived here until agent +// issuance moved to tokens.sr.ht; a working token is signed rather than stored, +// so authenticating one is a signature check in authn/ and this package has +// nothing to look up. // // The layering rule from the design is what shapes this package: **git refs are // the source of truth for whether a proposal exists and whether it merged; @@ -138,15 +143,6 @@ // ErrDocIDDuplicate is returned when one batch of documents carries the // same ID twice. Distinct from ErrDocIDTaken: the collision is inside the // push itself, not against the registry. ErrDocIDDuplicate = errors.New("db: document id appears twice in one batch") - - // ErrTokenExists is returned by CreateAgentToken when that exact token is - // already registered (agent_token.token_hash UNIQUE). - ErrTokenExists = errors.New("db: agent token already registered") - - // ErrTokenRevoked is returned by AuthenticateAgentToken for a token that - // exists but has been revoked. Kept distinct from ErrNotFound so the audit - // log can say which happened; both map to 401 at the API boundary. - ErrTokenRevoked = errors.New("db: agent token revoked") // ErrProposalNotOpen is returned by DeleteOpenProposal for a proposal that // exists but has already been resolved. Kept distinct from ErrNotFound so diff --git a/db/token.go b/db/token.go deleted file mode 100644 index 805a3af70a173c1ef4b73401826dee365185cefb..0000000000000000000000000000000000000000 --- a/db/token.go +++ /dev/null @@ -1,201 +0,0 @@ -package db - -import ( - "context" - "crypto/rand" - "crypto/sha256" - "crypto/subtle" - "database/sql" - "encoding/base64" - "errors" - "fmt" - "time" - - "github.com/lib/pq" -) - -// TokenBytes is the entropy of a minted agent token before encoding. 32 bytes -// is well past any brute-force concern and keeps the encoded form short enough -// to paste into an agent's environment. -const TokenBytes = 32 - -// AgentToken is a credential an agent presents on the write plane. The token -// value itself is never stored — only Hash — so a database dump cannot be -// replayed against the service. -// -// v1 ships one token plus mandatory provenance rather than per-agent scopes: -// the refs rule (an agent credential can only move refs under BranchPrefix) is -// the boundary that actually bounds the damage, and it holds with a single -// shared token. Per-space scoping is a later column on this row plus a filter -// clause, not an architectural change. -type AgentToken struct { - ID int - Name string - Hash []byte - Created time.Time - Revoked *time.Time -} - -// Active reports whether the token may still authenticate. -func (t *AgentToken) Active() bool { return t.Revoked == nil } - -// GenerateToken mints a fresh token value: TokenBytes of crypto/rand, URL-safe -// base64 without padding. This is the only moment the plaintext exists; the -// caller shows it to the operator once and stores only HashToken(it). -func GenerateToken() (string, error) { - b := make([]byte, TokenBytes) - if _, err := rand.Read(b); err != nil { - return "", fmt.Errorf("generate agent token: %w", err) - } - return base64.RawURLEncoding.EncodeToString(b), nil -} - -// HashToken is the one-way function between a presented token and the stored -// agent_token.token_hash. SHA-256 is the right tool here rather than a password -// KDF: the input is 256 bits of uniform randomness we generated, not a -// human-chosen secret, so there is no dictionary to stretch against. -func HashToken(token string) []byte { - sum := sha256.Sum256([]byte(token)) - return sum[:] -} - -// TokenMatches compares a stored hash with the hash of a presented token in -// constant time. Byte-wise early exit on a hash comparison leaks how many -// leading bytes an attacker guessed right, which is enough to walk a forged -// value into place one byte at a time; subtle.ConstantTimeCompare does not. -func TokenMatches(stored []byte, token string) bool { - return subtle.ConstantTimeCompare(stored, HashToken(token)) == 1 -} - -// CreateAgentToken stores a token by hash and returns the row. The plaintext is -// never passed to this function and never reaches SQL — callers hash with -// HashToken and keep the value only long enough to show it once. -// -// A token_hash UNIQUE violation means the same token was registered twice — -// for 32 random bytes, that is a caller re-registering a value it already had, -// not a collision — and is mapped to ErrTokenExists. -func (s *Store) CreateAgentToken(ctx context.Context, name string, hash []byte) (*AgentToken, error) { - if name == "" { - return nil, fmt.Errorf("create agent token: name is required") - } - if len(hash) != sha256.Size { - return nil, fmt.Errorf("create agent token: hash must be %d bytes, got %d", - sha256.Size, len(hash)) - } - const q = ` -INSERT INTO agent_token (name, token_hash, created) -VALUES ($1, $2, $3) -RETURNING id, created` - t := AgentToken{Name: name, Hash: hash} - err := s.q.QueryRowContext(ctx, q, name, hash, time.Now().UTC()).Scan(&t.ID, &t.Created) - if err != nil { - var pqErr *pq.Error - if errors.As(err, &pqErr) && pqErr.Code == "23505" { - return nil, ErrTokenExists - } - return nil, fmt.Errorf("create agent token %q: %w", name, err) - } - return &t, nil -} - -// AgentTokenByHash looks a token up by its stored hash. It does not consider -// revocation — use AuthenticateAgentToken for the authorization decision. -// Returns ErrNotFound if no such token is registered. -func (s *Store) AgentTokenByHash(ctx context.Context, hash []byte) (*AgentToken, error) { - const q = ` -SELECT id, name, token_hash, created, revoked -FROM agent_token -WHERE token_hash = $1` - var ( - t AgentToken - revoked sql.NullTime - ) - err := s.q.QueryRowContext(ctx, q, hash).Scan(&t.ID, &t.Name, &t.Hash, &t.Created, &revoked) - if errors.Is(err, sql.ErrNoRows) { - return nil, ErrNotFound - } - if err != nil { - return nil, fmt.Errorf("agent token by hash: %w", err) - } - if revoked.Valid { - r := revoked.Time - t.Revoked = &r - } - return &t, nil -} - -// AuthenticateAgentToken is the authorization boundary: it hashes the presented -// token, looks the row up by that hash, re-verifies the stored hash against the -// presentation in constant time, and rejects a revoked token. -// -// The re-verification is not redundant with the SQL equality. The index lookup -// is what finds the row; TokenMatches is what decides, and it is the one -// comparison an attacker can time. Returns ErrNotFound for an unknown token and -// ErrTokenRevoked for a known but revoked one; both are 401 at the API edge. -func (s *Store) AuthenticateAgentToken(ctx context.Context, token string) (*AgentToken, error) { - if token == "" { - return nil, ErrNotFound - } - t, err := s.AgentTokenByHash(ctx, HashToken(token)) - if err != nil { - return nil, err - } - if !TokenMatches(t.Hash, token) { - // The row was found by hash equality, so a mismatch here means the - // stored hash is not what the index matched on — corruption, not a bad - // credential. - return nil, fmt.Errorf("agent token %d: stored hash does not verify", t.ID) - } - if !t.Active() { - return nil, fmt.Errorf("%w: token %q revoked at %s", ErrTokenRevoked, t.Name, t.Revoked) - } - return t, nil -} - -// RevokeAgentToken stamps a token revoked. Revocation is a stamp rather than a -// delete so the audit trail keeps naming the token that made past proposals. -// Revoking an already-revoked token is a no-op that returns nil; re-revoking is -// not an error worth failing an operator over. Returns ErrNotFound if id does -// not exist. -func (s *Store) RevokeAgentToken(ctx context.Context, id int) error { - const q = `UPDATE agent_token SET revoked = COALESCE(revoked, $2) WHERE id = $1` - res, err := s.q.ExecContext(ctx, q, id, time.Now().UTC()) - if err != nil { - return fmt.Errorf("revoke agent token %d: %w", id, err) - } - return requireOne(res, "revoke agent token") -} - -// ListAgentTokens returns every token, newest first, so the operator can see -// what exists and pick one to revoke. Hashes are included; there is nothing -// secret about them and the reconciler-style tooling compares by them. -func (s *Store) ListAgentTokens(ctx context.Context) ([]*AgentToken, error) { - const q = ` -SELECT id, name, token_hash, created, revoked -FROM agent_token -ORDER BY created DESC, id DESC` - rows, err := s.q.QueryContext(ctx, q) - if err != nil { - return nil, fmt.Errorf("list agent tokens: %w", err) - } - defer rows.Close() - var out []*AgentToken - for rows.Next() { - var ( - t AgentToken - revoked sql.NullTime - ) - if err := rows.Scan(&t.ID, &t.Name, &t.Hash, &t.Created, &revoked); err != nil { - return nil, fmt.Errorf("scan agent token: %w", err) - } - if revoked.Valid { - r := revoked.Time - t.Revoked = &r - } - out = append(out, &t) - } - if err := rows.Err(); err != nil { - return nil, fmt.Errorf("iterate agent tokens: %w", err) - } - return out, nil -} diff --git a/db/token_test.go b/db/token_test.go deleted file mode 100644 index 3c3570fd0df1e0c0f25818e23fb73a90cea41a25..0000000000000000000000000000000000000000 --- a/db/token_test.go +++ /dev/null @@ -1,118 +0,0 @@ -package db - -import ( - "bytes" - "context" - "errors" - "testing" -) - -func TestAgentTokenLifecycle(t *testing.T) { - s, pool, cleanup := newTestStore(t) - defer cleanup() - ctx := context.Background() - - token, err := GenerateToken() - if err != nil { - t.Fatalf("generate token: %v", err) - } - hash := HashToken(token) - - created, err := s.CreateAgentToken(ctx, "spec-writer", hash) - if err != nil { - t.Fatalf("create token: %v", err) - } - if created.ID == 0 || !created.Active() { - t.Fatalf("unexpected created token: %+v", created) - } - - // The plaintext must never have reached the database. - var found int - if err := pool.QueryRowContext(ctx, - `SELECT count(*) FROM agent_token WHERE encode(token_hash, 'escape') LIKE '%' || $1 || '%'`, - token).Scan(&found); err != nil { - t.Fatalf("scan for plaintext: %v", err) - } - if found != 0 { - t.Fatal("the token plaintext is recoverable from the row") - } - - if _, err := s.CreateAgentToken(ctx, "duplicate", hash); !errors.Is(err, ErrTokenExists) { - t.Fatalf("duplicate token = %v, want ErrTokenExists", err) - } - - byHash, err := s.AgentTokenByHash(ctx, hash) - if err != nil { - t.Fatalf("lookup by hash: %v", err) - } - if byHash.ID != created.ID || byHash.Name != "spec-writer" || !bytes.Equal(byHash.Hash, hash) { - t.Fatalf("unexpected lookup: %+v", byHash) - } - if _, err := s.AgentTokenByHash(ctx, HashToken("nope")); !errors.Is(err, ErrNotFound) { - t.Fatalf("unknown hash = %v, want ErrNotFound", err) - } - - auth, err := s.AuthenticateAgentToken(ctx, token) - if err != nil { - t.Fatalf("authenticate: %v", err) - } - if auth.ID != created.ID { - t.Fatalf("authenticated the wrong token: %+v", auth) - } - if _, err := s.AuthenticateAgentToken(ctx, token+"x"); !errors.Is(err, ErrNotFound) { - t.Fatalf("wrong token = %v, want ErrNotFound", err) - } - if _, err := s.AuthenticateAgentToken(ctx, ""); !errors.Is(err, ErrNotFound) { - t.Fatalf("empty token = %v, want ErrNotFound", err) - } - - // Revocation is a stamp, so the row stays for the audit trail. - if err := s.RevokeAgentToken(ctx, created.ID); err != nil { - t.Fatalf("revoke: %v", err) - } - if _, err := s.AuthenticateAgentToken(ctx, token); !errors.Is(err, ErrTokenRevoked) { - t.Fatalf("revoked token = %v, want ErrTokenRevoked", err) - } - revoked, err := s.AgentTokenByHash(ctx, hash) - if err != nil { - t.Fatalf("lookup revoked: %v", err) - } - if revoked.Active() || revoked.Revoked == nil { - t.Fatalf("revocation did not stick: %+v", revoked) - } - first := *revoked.Revoked - - // Re-revoking keeps the original timestamp rather than moving it. - if err := s.RevokeAgentToken(ctx, created.ID); err != nil { - t.Fatalf("re-revoke: %v", err) - } - again, err := s.AgentTokenByHash(ctx, hash) - if err != nil { - t.Fatalf("lookup revoked: %v", err) - } - if !again.Revoked.Equal(first) { - t.Fatalf("re-revoking moved the timestamp: %v -> %v", first, *again.Revoked) - } - - if err := s.RevokeAgentToken(ctx, 99999); !errors.Is(err, ErrNotFound) { - t.Fatalf("revoking a missing token = %v, want ErrNotFound", err) - } - - second, err := GenerateToken() - if err != nil { - t.Fatalf("generate token: %v", err) - } - if _, err := s.CreateAgentToken(ctx, "notes-writer", HashToken(second)); err != nil { - t.Fatalf("create second token: %v", err) - } - list, err := s.ListAgentTokens(ctx) - if err != nil { - t.Fatalf("list tokens: %v", err) - } - if len(list) != 2 || list[0].Name != "notes-writer" { - t.Fatalf("unexpected token list (newest first): %+v", list) - } - if _, err := s.AuthenticateAgentToken(ctx, second); err != nil { - t.Fatalf("authenticate second token: %v", err) - } -} diff --git a/db/unit_test.go b/db/unit_test.go index d5f5cf3ae0595fe7885ab8a50dfa16ff7755be91..084912a96a814739eec8ac08b32ab4babfc80506 100644 --- a/db/unit_test.go +++ b/db/unit_test.go @@ -2,7 +2,6 @@ package db import ( "context" - "crypto/sha256" "database/sql" "errors" "os" @@ -17,9 +16,9 @@ "sourcecraft.dev/bigbes/sr-ht-spec/core" ) // These tests need no database. They cover the logic that decides things — -// transition legality, token comparison, batch duplicate detection — plus the -// agreement between schema.sql and the migration, which is otherwise only -// discovered on a fresh install. +// transition legality, batch duplicate detection — plus the agreement between +// schema.sql and the migrations, which is otherwise only discovered on a fresh +// install. func TestProposalBranch(t *testing.T) { for _, tc := range []struct { @@ -67,48 +66,6 @@ t.Errorf("db derives %q where core derives %q", mine, fromCore) } } -func TestHashTokenAndMatches(t *testing.T) { - tok, err := GenerateToken() - if err != nil { - t.Fatalf("generate token: %v", err) - } - if len(tok) < 40 { - t.Fatalf("token %q is implausibly short for %d bytes of entropy", tok, TokenBytes) - } - hash := HashToken(tok) - if len(hash) != sha256.Size { - t.Fatalf("HashToken returned %d bytes, want %d", len(hash), sha256.Size) - } - if strings.Contains(string(hash), tok) { - t.Fatal("hash must not contain the token") - } - if !TokenMatches(hash, tok) { - t.Fatal("TokenMatches rejected the token it hashed") - } - if TokenMatches(hash, tok+"x") { - t.Fatal("TokenMatches accepted a different token") - } - if TokenMatches(hash, "") { - t.Fatal("TokenMatches accepted the empty token") - } - if TokenMatches(nil, tok) { - t.Fatal("TokenMatches accepted a nil stored hash") - } - if TokenMatches(hash[:16], tok) { - t.Fatal("TokenMatches accepted a truncated stored hash") - } - - // Two mints must differ; a repeated value would mean the generator is not - // actually random and every token would be the same credential. - other, err := GenerateToken() - if err != nil { - t.Fatalf("generate token: %v", err) - } - if other == tok { - t.Fatal("GenerateToken returned the same value twice") - } -} - func TestDuplicateDocIDs(t *testing.T) { ref := func(id, path string) DocRef { return DocRef{ID: docID(t, id), Path: path} @@ -236,15 +193,6 @@ Owner: "bigbes", Name: core.MetaProjectName, }); !errors.Is(err, core.ErrReservedName) { t.Fatalf("CreateProject of the meta-project = %v, want ErrReservedName", err) } - if _, err := s.CreateAgentToken(ctx, "", HashToken("x")); err == nil { - t.Fatal("CreateAgentToken without a name must fail") - } - if _, err := s.CreateAgentToken(ctx, "ci", []byte("short")); err == nil { - t.Fatal("CreateAgentToken with a non-sha256 hash must fail") - } - if _, err := s.AuthenticateAgentToken(ctx, ""); !errors.Is(err, ErrNotFound) { - t.Fatalf("authenticating an empty token = %v, want ErrNotFound", err) - } if err := s.SetDigestMark(ctx, "", time.Now()); err == nil { t.Fatal("SetDigestMark without an owner must fail") } @@ -344,7 +292,7 @@ downs = append(downs, normalizeStatements(down)...) } fromSchema := normalizeStatements(string(schema)) - fromMigration := ups + fromMigration := applyDrops(t, ups) if len(fromSchema) != len(fromMigration) { t.Fatalf("schema.sql has %d statements, migration Up has %d:\n%v\n%v", len(fromSchema), len(fromMigration), fromSchema, fromMigration) @@ -372,13 +320,85 @@ // `comment` was on an absent-list here through v1, so that adding it needed // a design change rather than a quiet migration. Phase 5b is that design // change: the anchoring model was settled against the built review UI, so // the table is now required present like any other. - for _, want := range []string{"space", "document_id", "proposal", "agent_token", + for _, want := range []string{"space", "document_id", "proposal", "index_stamp", "digest_mark", "project", "project_space", "comment"} { if !contains(created, want) { t.Errorf("table %q is missing from schema.sql", want) } } + + // agent_token is required *absent*: agent credentials are issued by + // tokens.sr.ht and validated by signature, so a service that still had the + // table would still have a second door into the write plane. + if contains(created, "agent_token") { + t.Error("agent_token is back in schema.sql; agent credentials come from tokens.sr.ht") + } } + +// applyDrops folds a migration history the way Postgres does: a DROP TABLE +// removes the CREATE TABLE it names, the indexes on it, and itself. +// +// Without the fold the comparison above could only hold for an append-only +// history, and the first migration to remove a table — agent_token, when agent +// issuance moved to tokens.sr.ht — would have had to weaken the check instead of +// being checked by it. A statement that still names a dropped table after the +// fold is a fatal error rather than a silent pass: it means the history does +// something (an ALTER, a backfill) that this small folder does not model, and +// guessing would make the agreement test lie. +func applyDrops(t *testing.T, stmts []string) []string { + t.Helper() + var out []string + for _, stmt := range stmts { + name, dropped := droppedTable(stmt) + if !dropped { + out = append(out, stmt) + continue + } + kept := out[:0] + for _, prev := range out { + if createsTable(prev, name) || indexesTable(prev, name) { + continue + } + if mentions(prev, name) { + t.Fatalf("migration drops %q, but this earlier statement still names it "+ + "and is not a CREATE TABLE or CREATE INDEX:\n %s", name, prev) + } + kept = append(kept, prev) + } + out = kept + } + return out +} + +var ( + dropTableRe = regexp.MustCompile(`^DROP TABLE (?:IF EXISTS )?"?(\w+)"?$`) + createIndex = regexp.MustCompile(`^CREATE (?:UNIQUE )?INDEX \w+ ON "?(\w+)"?[ (]`) + identifierRe = func(name string) *regexp.Regexp { return regexp.MustCompile(`\b` + name + `\b`) } +) + +func droppedTable(stmt string) (string, bool) { + m := dropTableRe.FindStringSubmatch(strings.TrimSpace(stmt)) + if m == nil { + return "", false + } + return m[1], true +} + +func createsTable(stmt, name string) bool { + rest, ok := strings.CutPrefix(stmt, "CREATE TABLE ") + if !ok { + return false + } + return strings.HasPrefix(rest, name+"(") || strings.HasPrefix(rest, name+" ") || + strings.HasPrefix(rest, `"`+name+`"`) +} + +func indexesTable(stmt, name string) bool { + m := createIndex.FindStringSubmatch(stmt) + return m != nil && m[1] == name +} + +func mentions(stmt, name string) bool { return identifierRe(name).MatchString(stmt) } func splitBrant(src string) (up, down string, ok bool) { i := strings.Index(src, "-- +brant Up") diff --git a/docs/DESIGN.md b/docs/DESIGN.md index b83f3268e4d625fbd94eb920beeebcab81d02a26..caa6ba73cc1f970f195e196a4388f804410a29e4 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -56,7 +56,7 @@ | Review surface | **Browser, reached by a link the agent hands you.** Confirms the prose differ as a v1 build and keeps it as the Phase 0 gate. The inbox is the backstop for work no link reached, not the primary entry point. | | Read contract | **Pinned `?rev=` plus an approved/draft split.** Nearly free, since the review UI needs blob→render at arbitrary revs anyway, and it makes `X-Agent-Base` provenance auditable rather than decorative. | | Storage tiers | **One.** Git objects are read directly; the materialized checkout is dropped, leaving a bleve index and a blob-sha-keyed render cache as the only caches. | | Volume | **Tens of documents a day or fewer.** warren's batch index rebuild is absorbed unchanged; incremental indexing is explicitly not built. | -| Agent tokens | **One token plus mandatory provenance.** The refs rule (agents write only `proposals/*`) is the boundary that matters; per-space scoping deferred. | +| Agent tokens | **Issued by tokens.sr.ht, plus mandatory provenance.** v1 shipped one shared `agent_token` row; that plane is gone — see "Where agent credentials come from". The refs rule (agents write only `proposals/*`) is still the boundary that matters; per-space scoping deferred. | | Push validation | **Your pushes are validated too**, with a `--push-option=skip-validation` escape hatch. | | Name | **`spec.sr.ht`** at `spec.srht.bigb.es`; module `sourcecraft.dev/bigbes/sr-ht-spec`, binary `specsrht`. | @@ -72,8 +72,8 @@ you ──git push (ssh)───────────────────────┘ ▲ │ │ update + post-receive hooks ├──► one global bleve index (cache) ├──► render cache, keyed by blob sha (cache) - └──► Postgres (proposals, comments, agent - tokens, ID registry) + └──► Postgres (proposals, comments, + ID registry) you ──browser──────────► read + review UI (no editing) ``` @@ -821,27 +821,70 @@ approver lists, approval counts, request-changes round-trips, and per-human ACL rows. The unified-login cookie is still needed — not to tell users apart, but to tell *you* from an unauthenticated request. -**v1 ships one agent token plus mandatory provenance, not per-agent scopes.** -The distinction worth drawing is between the two boundaries this design has: +**v1 shipped one shared agent token plus mandatory provenance, not per-agent +scopes.** The distinction worth drawing is between the two boundaries this +design has: - **The refs rule is the boundary that matters, and it is free.** No agent credential can move the approved branch — only `proposals/*`. That is what - actually bounds the damage a confused or runaway agent can do, and it holds - with a single shared token. + actually bounds the damage a confused or runaway agent can do, and it held + even with a single shared token. - **Per-space scoping is the boundary that can wait.** Preventing a notes-writing agent from proposing into `specs/` is real defence in depth, but every agent here is a session you launched yourself, and a bad proposal is - visible and rejectable rather than destructive. Adding scopes later is a column - on the token row plus a filter clause — not an architectural change. + visible and rejectable rather than destructive. Adding scopes later is a + filter clause on top of the credential — not an architectural change. What is **not** optional is provenance: agent identity and session ID are -required on every write and recorded in git trailers. One token still yields a -full audit trail, because the trailers, not the credential, are what identify -who did what. +required on every write and recorded in git trailers. The credential is not what +identifies who did what — the trailers are — which is why one shared token still +yielded a full audit trail, and why per-agent credentials do not replace them. + +### Where agent credentials come from + +**Issuance is tokens.sr.ht's, not ours.** The `agent_token` table is gone: no +mint, no list, no revoke, no `specsrht token`, no row to look a presented secret +up in. An agent authenticates with a tokens.sr.ht *working token* — signed by the +instance, expiring, owned by a meta.sr.ht account, carrying a grant set — which +`authn/` verifies locally through sr-ht-ecore's `bearer` package. See +`sourcehut-tokens/SPEC.md`, chapters 2, 3 and 6. + +What that changes, and what it deliberately does not: + +- **Two vocabulary entries, declared here and not by the daemon:** + `spec:propose` for the write plane and `spec:read` for every read surface (web, + `/query`, MCP — checked per tool there, because one endpoint carries both + kinds). An unknown grant admits nobody. +- **The refs rule is untouched and is not replaced by grants.** A token carrying + `*` is still an agent to the receive path, and an agent still writes only + `proposals/*`. A grant says what a credential was minted for; the refs rule + says where a ref may point. +- **Provenance is untouched.** `X-Agent` and `X-Agent-Session` are still + mandatory on every agent write, on the credential plane that remains exactly as + they were on the one that is gone. +- **Every surface, including `git push`.** The SSH path used to check + `agent_token` directly, which is how it ended up being the one surface that + could not take an instance token; it goes through the same `authn.Resolver` now + and demands `spec:propose`, because a push by an agent is a proposal by another + transport. +- **A working token whose owner is not `[sr.ht] owner-name` is refused (403),** + not admitted as a second identity: this instance answers to one human, and + `Principal.Owner` is read by the provenance committer, the refs rule and the + coreauth bridge, all written for exactly one. +- **A `[tokens.sr.ht]` section is now required in config.ini.** With no issuer + there is no credential the service can check, so the daemon fails startup + rather than serving reads and refusing every agent write one request at a time. +- **`/tokens` redirects to tokens.sr.ht** (SPEC ch. 7): the services' own token + pages become links to the one that issues. + +**Deploy gate.** Removing the local plane locks out every agent still configured +with the shared secret — over HTTP and over `git push` — the moment it is +deployed. Every agent must hold a tokens.sr.ht working token *before* the +migration that drops the table runs. ### Provenance -One agent token (no roles, no per-space scopes — see above) plus a **required +An agent credential (no roles, no per-space scopes — see above) plus a **required agent identity string**. Every commit records it in a way that survives clone: ``` @@ -968,6 +1011,7 @@ | `[sr.ht] site-name` / `environment` | nav brand; non-`production` shows the dev banner | | `[sr.ht] internal-ipnet` | this host must fall inside it or internal GraphQL calls are rejected | | `[webhooks] private-key` | **`crypto.InitCrypto` fatally requires it even though v1 emits no webhooks** | | `[meta.sr.ht] origin` | login/logout redirects, profile fetch, PAT validation | +| `[tokens.sr.ht] origin` | **required**: the only issuer of agent credentials. The internal form is the revocation check's endpoint; the external one is where `/tokens` sends a browser. The daemon refuses to start without it | | `[git.sr.ht] repos` / `api-origin` | only if read-only mounts of `docs/` dirs in git.sr.ht repos are ever enabled (not v1) | ### Wiring checklist @@ -985,6 +1029,11 @@ attachments are allowed. 4. **internal-ipnet** — same prerequisite as both siblings. 5. **Migrations** — `specsrht-migrate`, a brant wrapper, copied from `doltsrht-migrate`. +6. **Agent credentials before migration 0005.** `0005_drop_agent_token` removes + the local credential plane, and with `migrate-on-upgrade=yes` it runs on + deploy. Every agent — including any that pushes over SSH — must already hold a + tokens.sr.ht working token with `spec:propose` (and `spec:read` to read). + Deploying first locks all of them out at once. ### GraphQL: a read schema at our own `/query` in Phase 2 @@ -1078,15 +1127,16 @@ core/ pure domain. Space/doc/rev/ID validation, .spec.yml policy, proposal state machine, sentinel errors. No external deps. gitx/ bare-repo lifecycle, tree walk, blob read, proposal branches, the tree-splice merge, refs-rule enforcement. go-git only. -db/ Postgres: proposals, ID registry, agent token, index stamps. +db/ Postgres: proposals, ID registry, index stamps. No credential + table — agent tokens are issued and signed by tokens.sr.ht. doc/ absorbed warren vault/ + render/: frontmatter, Archive, goldmark rendering, wikilink resolution. Fed by gitx trees. search/ absorbed warren index/ + search/: one global bleve index, query-time project filtering. prosediff/ word-level prose diff over rendered block structure. Net-new, riskiest, and the Phase 0 gate. -authn/ unified-login cookie -> identity, agent token validation, - provenance trailer construction. +authn/ unified-login cookie -> identity, tokens.sr.ht working-token + validation (grants included), provenance trailer construction. service/ orchestration: read, propose, merge, reconcile, digest. The single layer REST, MCP and GraphQL all call. api/ REST handlers (write plane + JSON reads). @@ -1147,13 +1197,10 @@ resolved TIMESTAMPTZ ); CREATE INDEX ON proposal (state, created DESC); -CREATE TABLE agent_token ( - id SERIAL PRIMARY KEY, - name TEXT NOT NULL, - token_hash BYTEA NOT NULL UNIQUE, - created TIMESTAMPTZ NOT NULL DEFAULT now(), - revoked TIMESTAMPTZ -); +-- There is no credential table. `agent_token` (id, name, token_hash, created, +-- revoked) stood here until agent issuance moved to tokens.sr.ht; migration +-- 0005 drops it. A working token is signed rather than stored, so there is +-- nothing to look up. -- Index staleness: compared against the space's approved head. CREATE TABLE index_stamp ( @@ -1270,7 +1317,9 @@ - `db/` — the schema above plus queries; `schema.sql` and the first brant migration. - `authn/` — cookie decrypt to identity, agent-token validation, provenance trailer construction. Tests forge cookies with a synthesized ini (random fernet - key + ed25519 seed), as dolt.sr.ht's `authn/` does. + key + ed25519 seed), as dolt.sr.ht's `authn/` does. (Phase 1 validated spec's + own `agent_token`; that plane is gone — see "Where agent credentials come + from".) **Wave B (serial).** `service/` read + reconcile paths, `hooks/` (the `update` and `post-receive` shims and their daemon RPC endpoints), `cmd/specsrht` skeleton, @@ -1332,8 +1381,10 @@ 3. `git push` a document with a duplicate `id:` → **rejected** by the `update` hook with a message naming the collision; retry with `--push-option=skip-validation` → accepted. 4. `git push --force` to the approved branch → rejected. -5. An agent token proposing to `proposals/*` → accepted; the same token - attempting the approved branch → rejected. +5. An agent token carrying `spec:propose` proposing to `proposals/*` → accepted; + the same token attempting the approved branch → rejected. A token without + `spec:propose` → 403 on both the HTTP write plane and `git push`; a token from + the removed `agent_token` plane → 401 everywhere. 6. Propose via MCP → response carries a URL; opening it shows the prose diff; approve → merges, and the document's approved text changes. 7. Propose against a stale base → 409 with the current head. @@ -1388,9 +1439,10 @@ `WalkBlobs`-shaped addition to `gitx` plus an asset index in `doc/`. Deliberately not faked: an invented href would be worse than an honest missing link. The cheaper answer may be to **prefer Mermaid in fenced blocks by convention** — it stays text, diffs properly, and needs none of the above. -- **Agent token distribution.** How a Claude Code session actually acquires a - scoped token — long-lived value in the environment, or minted per session. - Per-session is better for provenance and revocation but needs an issuing flow. +- ~~**Agent token distribution.**~~ **Answered by tokens.sr.ht.** A session holds + a long-lived parent token in its environment and exchanges it for a short + working token (`POST /exchange`); per-session credentials no longer need an + issuing flow of ours, because the issuing flow is the daemon's. - **Retention for the firehose half.** Auto-merged notes accumulate forever by default. Whether they expire, get compacted, or are simply never deleted affects repo growth and index size, and is easier to decide now than later. @@ -1407,7 +1459,7 @@ |---|---| | `?rev=` pinning and approved/draft split? | **Both.** Nearly free — the review UI needs blob→render at arbitrary revs regardless — and it makes `X-Agent-Base` auditable. | | Materialized checkout? | **Dropped.** One read path over git objects; render cache keyed by blob sha. | | Firehose volume? | **Tens of documents a day or fewer.** Batch index rebuild absorbed as-is; incremental indexing not built; retention a non-issue. | -| Agent token machinery? | **One token + mandatory provenance.** The refs rule is the boundary that matters; per-space scoping deferred to a column and a filter clause. | +| Agent token machinery? | **One token + mandatory provenance** (2026-07-22). Superseded: issuance moved to tokens.sr.ht and the local plane was removed, so a credential is now per-agent, owned and grant-carrying. The refs rule is still the boundary that matters; per-space scoping still deferred to a filter clause. | | Validate your own pushes? | **Yes, with `--push-option=skip-validation`.** Guards typos that corrupt the global ID registry, without a lockout risk. | | Import external corpora? | **Probably eventually; left unspecified.** Global IDs are the only forward compatibility required. | | Name? | **`spec.sr.ht` at `spec.srht.bigb.es`**, module `sourcecraft.dev/bigbes/sr-ht-spec`, binary `specsrht`. | diff --git a/graph/grant_test.go b/graph/grant_test.go index c45984d9eba0cd23514efe927befefb21c1bcff4..335e4509d8da4b7f6e950348364ef0cf6c70d5a6 100644 --- a/graph/grant_test.go +++ b/graph/grant_test.go @@ -37,8 +37,11 @@ }{ {"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}, + // An agent a local process asserted: no credential, so no grant set + // to read. The resolver never produces one, but the gate must not + // invent a refusal for a principal that carries none. + "locally asserted agent", + authn.Principal{Kind: authn.KindAgent, Owner: "bigbes"}, http.StatusOK, true, }, { diff --git a/graph/graph_test.go b/graph/graph_test.go index 565b773aad736d15520da5b53727326baf2bec17..3500668cde63aa591383bd30ae00d57a0d1201ea 100644 --- a/graph/graph_test.go +++ b/graph/graph_test.go @@ -19,7 +19,9 @@ "github.com/fernet/fernet-go" "github.com/go-chi/chi/v5" "github.com/vaughan0/go-ini" + "sourcecraft.dev/bigbes/sr-ht-core/auth" "sourcecraft.dev/bigbes/sr-ht-core/crypto" + "sourcecraft.dev/bigbes/sr-ht-ecore/bearer" "sourcecraft.dev/bigbes/sr-ht-spec/authn" "sourcecraft.dev/bigbes/sr-ht-spec/core" @@ -56,7 +58,6 @@ const ( headRev = "1111111111111111111111111111111111111111" oldRev = "2222222222222222222222222222222222222222" absentRev = "3333333333333333333333333333333333333333" - agentTk = "test-agent-token" ) var ( @@ -289,15 +290,45 @@ } return out, nil } -// stubTokenStore knows exactly one live agent token. -type stubTokenStore struct{} +// stubUsers resolves the owner an instance token names to a local row. +type stubUsers struct{} + +func (stubUsers) LookupUser(_ context.Context, username string) (authn.InstanceUser, error) { + return authn.InstanceUser{ID: 1, Username: username}, nil +} -func (stubTokenStore) LookupAgentToken(_ context.Context, hash []byte) (authn.AgentToken, error) { - want := authn.HashToken(agentTk) - if string(hash) != string(want) { - return authn.AgentToken{}, authn.ErrUnknownToken +// testResolver is the resolver the daemon builds, with the one agent credential +// plane wired. Its origin is never reached: these tests present stateless +// tokens, which carry no row id and so skip the revocation round trip entirely. +func testResolver(t *testing.T) *authn.Resolver { + t.Helper() + v, err := bearer.New(bearer.Options{ + Origin: "https://tokens.srht.invalid", + ClientID: "spec.sr.ht", + NodeID: "graph-test", + }) + if err != nil { + t.Fatalf("bearer.New: %v", err) } - return authn.AgentToken{ID: 1, Name: "test", Hash: want}, nil + resolver, err := authn.NewResolver("bigbes", authn.WithInstancePlane(v, stubUsers{})) + if err != nil { + t.Fatalf("NewResolver: %v", err) + } + return resolver +} + +// agentToken mints a signed tokens.sr.ht working token carrying grantString. It +// cannot be a package-level constant the way the old opaque secret was: the +// signing key is established by TestMain. +func agentToken(grantString string) string { + bt := &auth.BearerToken{ + Version: auth.TokenVersion, + Expires: auth.ToTimestamp(time.Now().Add(time.Hour)), + Grants: grantString, + ClientID: bearer.TokensClientID, + Username: "bigbes", + } + return bt.Encode() } // ---- harness -------------------------------------------------------------- @@ -310,10 +341,7 @@ } func newHarness(t *testing.T, withProposals bool) harness { t.Helper() - resolver, err := authn.NewResolver("bigbes", stubTokenStore{}) - if err != nil { - t.Fatalf("NewResolver: %v", err) - } + resolver := testResolver(t) searcher := &fakeSearcher{} opts := Options{Reader: newFakeReader(), Searcher: searcher, Resolver: resolver} var proposals *fakeProposals @@ -645,7 +673,7 @@ // whole service is for. func TestAgentTokenReads(t *testing.T) { h := newHarness(t, false) r := post(t, h, `{ document(space: "~bigbes/rfcs", id: "SPEC-0007") { title } }`, func(req *http.Request) { - req.Header.Set("Authorization", "Bearer "+agentTk) + req.Header.Set("Authorization", "Bearer "+agentToken("spec:read")) }) var got struct{ Document struct{ Title string } } ok(t, r, &got) @@ -1050,10 +1078,7 @@ // The mounting call the package documents, exercised: a chi router with no // middleware of its own, the endpoint at /query, and a query that goes through. // The daemon's one line is the line under test here. func TestMountedOnAChiRouter(t *testing.T) { - resolver, err := authn.NewResolver("bigbes", stubTokenStore{}) - if err != nil { - t.Fatalf("NewResolver: %v", err) - } + resolver := testResolver(t) srv, err := New(Options{Reader: newFakeReader(), Searcher: &fakeSearcher{}, Resolver: resolver}) if err != nil { t.Fatalf("New: %v", err) @@ -1077,10 +1102,7 @@ // New refuses a half-wired server at startup rather than failing inside the // first query. func TestNewRequiresItsSeams(t *testing.T) { - resolver, err := authn.NewResolver("bigbes", stubTokenStore{}) - if err != nil { - t.Fatalf("NewResolver: %v", err) - } + resolver := testResolver(t) cases := map[string]Options{ "no reader": {Searcher: &fakeSearcher{}, Resolver: resolver}, "no searcher": {Reader: newFakeReader(), Resolver: resolver}, diff --git a/hooks/doc.go b/hooks/doc.go index fdd8774420f62bfea6f728d1622f1df9877a7d1d..b22c654332301e5987deff0da4bd64e3e3e7109f 100644 --- a/hooks/doc.go +++ b/hooks/doc.go @@ -76,12 +76,15 @@ // The hook does not decide who is pushing; it forwards a credential and the // daemon resolves it: // // SPECSRHT_PRINCIPAL "owner" or "agent" (required) -// SPECSRHT_AGENT_TOKEN agent secret (required when kind=agent) +// SPECSRHT_AGENT_TOKEN agent credential (required when kind=agent) // SPECSRHT_AGENT agent identity string (provenance, optional) // SPECSRHT_AGENT_SESSION agent session id (provenance, optional) // -// An agent's token is validated against the database on every push, so an -// agent credential asserts nothing by itself. `owner` is different: it is an +// The agent credential is a tokens.sr.ht working token — the same one the REST +// and MCP planes take — validated on every push through the same authn.Resolver +// those surfaces authenticate with, so an agent credential asserts nothing by +// itself. It must carry the spec:propose grant: a push by an agent is a proposal +// by another transport. `owner` is different: it is an // assertion, trusted because sshd already authenticated the SSH key and the // forced-command wrapper — which owns this environment — is what sets it. // That wrapper is therefore part of the trust boundary: sshd must not diff --git a/hooks/e2e_test.go b/hooks/e2e_test.go index 64bcd763fc1e7f67a5cf14743941c0027f756079..a88d13d7e04f45dda06a31fb5f9ec2d5702140d4 100644 --- a/hooks/e2e_test.go +++ b/hooks/e2e_test.go @@ -8,8 +8,12 @@ "os/exec" "path/filepath" "strings" "testing" + "testing/fstest" "github.com/go-git/go-git/v5/plumbing" + + "sourcecraft.dev/bigbes/sr-ht-core/config" + "sourcecraft.dev/bigbes/sr-ht-core/crypto" "sourcecraft.dev/bigbes/sr-ht-spec/core" "sourcecraft.dev/bigbes/sr-ht-spec/gitx" @@ -23,10 +27,26 @@ // hook dispatches on the name git invoked it as, so pointing those symlinks at // the test binary is enough to make a real `git push` run this package's code // through git's real receive path. That is the production dispatch mechanism, // unchanged and unmocked — not a stand-in for it. +// +// It also initialises crypto, because the agent credential this package +// validates is a signed working token and the signing key lives in core-go's +// globals; the keys are the ones core-go's own tests use. A hook process skips +// that: a hook reads no config and validates nothing, which is the property +// that lets it run with no state at all. func TestMain(m *testing.M) { if _, _, isHook := ModeFromArgs(os.Args); isHook { os.Exit(Run(Runtime{Args: os.Args})) } + 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()) } @@ -158,7 +178,7 @@ if err := InstallSpace(root, testSpace, InstallOptions{Binary: binary}); err != nil { t.Fatalf("InstallSpace: %v", err) } - back := newFakeBackend(t, root, nil) + back := newFakeBackend(t, root) back.validate = realish(t, root) srv, landed := startServer(t, back) @@ -194,6 +214,20 @@ out, err := runGit(t, e.work, pushEnv(), append([]string{"push", e.repo}, args...)...) return out, err == nil } +// pushAsAgent is the same push with the environment the forced-command wrapper +// exports for an agent: a tokens.sr.ht working token and the provenance fields. +func (e *e2e) pushAsAgent(t *testing.T, token string, args ...string) (string, bool) { + t.Helper() + env := map[string]string{ + EnvPrincipal: string(PrincipalAgent), + EnvAgentToken: token, + EnvAgent: "claude-code/spec-writer", + EnvAgentSession: "8fb9c9a4-b078-4af1-89eb-d97c522f9921", + } + out, err := runGit(t, e.work, env, append([]string{"push", e.repo}, args...)...) + return out, err == nil +} + const goodDoc = `--- id: SPEC-0001 title: Storage @@ -295,6 +329,58 @@ t.Run("a proposal branch may be force-updated", func(t *testing.T) { out, ok := e.push(t, "--force", "main:refs/heads/proposals/1") if !ok { t.Fatalf("a proposal branch was refused:\n%s", out) + } + }) +} + +// TestEndToEndAgentPush is the credential change under a real `git push`: an +// agent holding a tokens.sr.ht working token can push, the refs rule still +// confines it to proposals/*, and a token without spec:propose gets nowhere. +// +// Before this, the SSH path checked agent_token directly and would have refused +// every one of these; it is now the same authn.Resolver the HTTP surfaces use. +func TestEndToEndAgentPush(t *testing.T) { + e := newE2E(t) + e.write(t, "specs/0001-storage.md", goodDoc) + e.commit(t, "add the storage spec") + + t.Run("a token without spec:propose cannot push", func(t *testing.T) { + out, ok := e.pushAsAgent(t, instanceToken("spec:read"), "main:refs/heads/proposals/1") + if ok { + t.Fatalf("a token with no propose grant was accepted:\n%s", out) + } + mentions(t, "the rejection", out, "spec.sr.ht rejected this push", "spec:propose") + if out, err := runGit(t, e.repo, nil, "rev-parse", "--verify", "refs/heads/proposals/1"); err == nil { + t.Errorf("the ref moved despite the refusal: %s", out) + } + }) + + t.Run("the old opaque agent token cannot push", func(t *testing.T) { + out, ok := e.pushAsAgent(t, "s3cret-from-agent-token", "main:refs/heads/proposals/1") + if ok { + t.Fatalf("a credential from the removed plane was accepted:\n%s", out) + } + mentionsNot(t, "the rejection", out, "s3cret-from-agent-token") + }) + + t.Run("an instance token with spec:propose pushes a proposal branch", func(t *testing.T) { + out, ok := e.pushAsAgent(t, instanceToken("spec:propose"), "main:refs/heads/proposals/1") + if !ok { + t.Fatalf("an agent holding a tokens.sr.ht token was refused:\n%s", out) + } + if head := strings.TrimSpace(gitMust(t, e.repo, "rev-parse", "refs/heads/proposals/1")); head == "" { + t.Error("the proposal branch was not created") + } + }) + + t.Run("the refs rule still confines it to the proposal prefix", func(t *testing.T) { + out, ok := e.pushAsAgent(t, instanceToken("*"), "main:refs/heads/main") + if ok { + t.Fatalf("an agent moved the approved branch:\n%s", out) + } + mentions(t, "the rejection", out, "spec.sr.ht rejected this push") + if out, err := runGit(t, e.repo, nil, "rev-parse", "--verify", "refs/heads/main"); err == nil { + t.Errorf("the approved branch moved despite the refusal: %s", out) } }) } diff --git a/hooks/fixture_test.go b/hooks/fixture_test.go index af4459576730f6d378433547c5bdfcdb2f603a39..961b244892f75af29036cd12bd4c06b06ad584fe 100644 --- a/hooks/fixture_test.go +++ b/hooks/fixture_test.go @@ -6,15 +6,19 @@ "errors" "io" "log/slog" "net" + "net/http" + "net/http/httptest" "os" "path/filepath" "strings" "testing" "time" + "sourcecraft.dev/bigbes/sr-ht-core/auth" + "sourcecraft.dev/bigbes/sr-ht-ecore/bearer" + "sourcecraft.dev/bigbes/sr-ht-spec/authn" "sourcecraft.dev/bigbes/sr-ht-spec/core" - "sourcecraft.dev/bigbes/sr-ht-spec/db" "sourcecraft.dev/bigbes/sr-ht-spec/service" ) @@ -22,25 +26,49 @@ const testOwner = "bigbes" var testSpace = core.SpaceRef{Owner: testOwner, Name: "rfcs"} -// fakeLookup is service.AgentTokenLookup over a map, so the agent path can be -// exercised without Postgres. *service.TokenStore is what turns db/'s -// ErrNotFound into authn's ErrUnknownToken, and that mapping is part of what -// the server relies on, so the real adapter is used over this fake rather than -// a fake authn.TokenStore. -type fakeLookup struct { - byHash map[string]*db.AgentToken - err error -} +// instanceToken mints a signed tokens.sr.ht working token the way the daemon +// does. It is a real credential checked by the real sr-ht-ecore validator, +// because the point of routing the push path through authn.Resolver is that it +// runs the identical check the HTTP surfaces run; a stub validator here would +// only assert that the wiring calls something. +func instanceToken(grantString string) string { return tokenFor(testOwner, grantString) } -func (f *fakeLookup) AgentTokenByHash(_ context.Context, hash []byte) (*db.AgentToken, error) { - if f.err != nil { - return nil, f.err +// foreignToken is a working token belonging to somebody who is not the instance +// owner: valid, and refused all the same. +func foreignToken(grantString string) string { return tokenFor("someone", grantString) } + +func tokenFor(username, grantString string) string { + bt := &auth.BearerToken{ + Version: auth.TokenVersion, + Expires: auth.ToTimestamp(time.Now().Add(time.Hour)), + Grants: grantString, + ClientID: bearer.TokensClientID, + Username: username, } - tok, ok := f.byHash[string(hash)] - if !ok { - return nil, db.ErrNotFound + return bt.Encode() +} + +// stubUsers resolves the owner an instance token names to a local row. +type stubUsers struct{} + +func (stubUsers) LookupUser(_ context.Context, username string) (authn.InstanceUser, error) { + return authn.InstanceUser{ID: 1, Username: username}, nil +} + +// fakeDaemonOrigin starts a stand-in for tokens.sr.ht's revocation endpoint — +// 204 is live, 404 is revoked — and returns its origin. A dead one, with +// nothing answering, is what a restarting daemon looks like from here. +func fakeDaemonOrigin(t *testing.T, status int, dead bool) string { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(status) + })) + if dead { + srv.Close() + return srv.URL } - return tok, nil + t.Cleanup(srv.Close) + return srv.URL } // fakeBackend is a Backend that answers from a script instead of a database. @@ -49,7 +77,6 @@ // can be driven by a real `git push` on a machine with no Postgres. type fakeBackend struct { root string resolver *authn.Resolver - tokens *service.TokenStore // validate is the scripted answer, and the recorder: every request reaches // it. A nil validate accepts everything. @@ -58,23 +85,32 @@ seen []service.PushRequest } -func newFakeBackend(t *testing.T, root string, tokens map[string]*db.AgentToken) *fakeBackend { +// newFakeBackend builds a backend whose resolver carries the real tokens.sr.ht +// validator, pointed at a live fake daemon. +func newFakeBackend(t *testing.T, root string) *fakeBackend { t.Helper() - lookup := &fakeLookup{byHash: map[string]*db.AgentToken{}} - for secret, row := range tokens { - lookup.byHash[string(authn.HashToken(secret))] = row + return newFakeBackendAgainst(t, root, fakeDaemonOrigin(t, http.StatusNoContent, false)) +} + +func newFakeBackendAgainst(t *testing.T, root, origin string) *fakeBackend { + t.Helper() + v, err := bearer.New(bearer.Options{ + Origin: origin, + ClientID: service.ConfigSection, + NodeID: "hooks-test", + }) + if err != nil { + t.Fatalf("bearer.New: %v", err) } - store := service.NewTokenStore(lookup) - resolver, err := authn.NewResolver(testOwner, store) + resolver, err := authn.NewResolver(testOwner, authn.WithInstancePlane(v, stubUsers{})) if err != nil { t.Fatalf("NewResolver: %v", err) } - return &fakeBackend{root: root, resolver: resolver, tokens: store} + return &fakeBackend{root: root, resolver: resolver} } -func (b *fakeBackend) ReposRoot() string { return b.root } -func (b *fakeBackend) Resolver() *authn.Resolver { return b.resolver } -func (b *fakeBackend) TokenStore() *service.TokenStore { return b.tokens } +func (b *fakeBackend) ReposRoot() string { return b.root } +func (b *fakeBackend) Resolver() *authn.Resolver { return b.resolver } func (b *fakeBackend) ValidatePush(ctx context.Context, req service.PushRequest) error { b.seen = append(b.seen, req) @@ -204,7 +240,7 @@ gitMust(t, "", "init", "--quiet", "--bare", "--initial-branch=main", dir) return dir } -// errFakeStore is a store outage: not a bad credential, and must never be +// errFakeStore is a backend outage: not a policy refusal, and must never be // reported as one. var errFakeStore = errors.New("fake store is down") diff --git a/hooks/hook_test.go b/hooks/hook_test.go index 1464b1efa5eecb7cf931526d801435244fcc0c40..a37690767b6a81d727c3381e45c3f1e9253ed038 100644 --- a/hooks/hook_test.go +++ b/hooks/hook_test.go @@ -113,7 +113,7 @@ // TestUpdateHookAcceptsAValidRef walks the two hooks of the rejecting path in // the order git runs them. func TestUpdateHookAcceptsAValidRef(t *testing.T) { - f := newServerFixture(t, nil) + f := newServerFixture(t) rt, stderr := f.hookRuntime(t, []string{"hooks/pre-receive"}, refLine(mainUpdate), nil) if code := Run(rt); code != 0 { @@ -133,7 +133,7 @@ // TestUpdateHookPrintsTheRejection: this text is the whole user interface of a // failed push, so the hook must print what the daemon wrote and exit non-zero. func TestUpdateHookPrintsTheRejection(t *testing.T) { - f := newServerFixture(t, nil) + f := newServerFixture(t) want := rejection("refs/heads/main", true, service.PushProblem{ Kind: service.ProblemFrontmatter, Path: "specs/0002-broken.md", @@ -161,7 +161,7 @@ // TestHooksFailClosed is the rule the whole design rests on: with no daemon // answering, a push is refused rather than accepted unvalidated. func TestHooksFailClosed(t *testing.T) { - f := newServerFixture(t, nil) + f := newServerFixture(t) dead := filepath.Join(f.root, "gone", "hook.sock") for _, tt := range []struct { @@ -193,7 +193,7 @@ // TestPostReceiveCannotReject: git ignores its exit status, so pretending // otherwise would only produce noise. It warns and names the backstop. func TestPostReceiveCannotReject(t *testing.T) { - f := newServerFixture(t, nil) + f := newServerFixture(t) dead := filepath.Join(f.root, "gone", "hook.sock") rt, stderr := f.hookRuntime(t, []string{"hooks/post-receive"}, refLine(mainUpdate), @@ -208,7 +208,7 @@ // TestHookRefusesAMisconfiguredEnvironment: no principal means nobody // authorized the push, and there is nothing to default to. func TestHookRefusesAMisconfiguredEnvironment(t *testing.T) { - f := newServerFixture(t, nil) + f := newServerFixture(t) rt, stderr := f.hookRuntime(t, []string{"hooks/update", mainUpdate.Ref, mainUpdate.Old, mainUpdate.New}, "", @@ -224,7 +224,7 @@ } } func TestUpdateHookNeedsThreeArguments(t *testing.T) { - f := newServerFixture(t, nil) + f := newServerFixture(t) rt, stderr := f.hookRuntime(t, []string{"hooks/update", "refs/heads/main"}, "", nil) if code := Run(rt); code == 0 { t.Fatal("update ran with the wrong number of arguments") @@ -235,7 +235,7 @@ // TestPreReceiveForwardsPushOptions: the update hook never sees them, so // whether skip-validation works at all depends on this handoff. func TestPreReceiveForwardsPushOptions(t *testing.T) { - f := newServerFixture(t, nil) + f := newServerFixture(t) rt, stderr := f.hookRuntime(t, []string{"hooks/pre-receive"}, refLine(mainUpdate), map[string]string{ @@ -260,7 +260,7 @@ } } func TestPreReceiveRefusesAMalformedRefList(t *testing.T) { - f := newServerFixture(t, nil) + f := newServerFixture(t) rt, stderr := f.hookRuntime(t, []string{"hooks/pre-receive"}, "not a ref line\n", nil) if code := Run(rt); code == 0 { t.Fatal("pre-receive accepted a ref list it could not parse") diff --git a/hooks/server.go b/hooks/server.go index 1fd43dcb18cf589e389f8c265748b938f964d3c0..f13e19a0c2068ebad032d97f1051f95c79dd8cfa 100644 --- a/hooks/server.go +++ b/hooks/server.go @@ -6,6 +6,7 @@ "errors" "fmt" "log/slog" "net" + "net/http" "os" "path/filepath" "strings" @@ -32,12 +33,12 @@ // ReposRoot is [spec.sr.ht] repos, the only directory a hook may address a // repository under. ReposRoot() string - // Resolver carries the instance owner username, the one identity an - // "owner" credential can resolve to. + // Resolver carries the instance owner username — the one identity an + // "owner" credential can resolve to — and the tokens.sr.ht plane an agent's + // credential is validated on. It is the same resolver the HTTP surfaces + // authenticate through, which is the point: one plane, one implementation of + // "is this credential good?". Resolver() *authn.Resolver - - // TokenStore is what an agent's token is validated against. - TokenStore() *service.TokenStore // ValidatePush answers whether one ref may move. A *service.PushRejection // is a policy refusal whose Error() is the text to print; anything else is @@ -460,37 +461,47 @@ // // The owner is not looked up: sshd authenticated the SSH key and the forced // command asserted it, and there is exactly one owner on this instance, so the // name comes from the resolver rather than from the wire — a hook cannot name -// somebody else. An agent's token is checked on every push. +// somebody else. An agent's credential is checked on every push. +// +// An agent goes through authn.Resolver.ResolveAgent, the same call the HTTP +// surfaces reach through their middleware. This path used to read the +// agent_token table directly, which is exactly how it ended up accepting a +// credential the HTTP planes had already stopped being the only door for: two +// implementations of one question, drifting. There is one credential plane now +// and one implementation of checking it. +// +// The grant check is here rather than in ValidatePush because it is about the +// credential and not about the ref: a push by an agent is a proposal by another +// transport, so it needs spec:propose exactly as the REST and MCP write planes +// do. The refs rule still runs afterwards, inside ValidatePush, and still +// confines the agent to proposals/* — a grant does not replace it and cannot +// widen it. func (s *Server) principal(ctx context.Context, space core.SpaceRef, cred Credential) (authn.Principal, Response, bool) { owner := s.backend.Resolver().Owner() switch cred.Kind { case PrincipalOwner: return authn.Principal{Kind: authn.KindOwner, Owner: owner, CookieUser: owner}, Response{}, true case PrincipalAgent: - tok, err := authn.ResolveAgentToken(ctx, s.backend.TokenStore(), cred.Token) + p, err := s.backend.Resolver().ResolveAgent(ctx, cred.Token, cred.Agent, cred.Session) if err != nil { - if authn.IsAuthFailure(err) { + // StatusFor is the one table: a bad or foreign credential (401) and + // a good one this instance has nothing to grant (403) are policy + // refusals the pusher can read and act on; anything else means we + // could not check, and an unanswerable check fails the push closed + // rather than reading as a bad token. + if status := authn.StatusFor(err); status < http.StatusInternalServerError { s.log.Warn("refused an agent push", "space", space.String(), "error", err) return authn.Principal{}, rejectedResponse(badTokenMessage(space, err)), false } - // The store could not answer. That is not a bad credential and - // must not read as one; fail the push closed instead. - s.log.Error("could not validate an agent token", "space", space.String(), "error", err) + s.log.Error("could not validate an agent credential", "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 + "spec.sr.ht could not check the credential 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 + if err := p.Authorize(authn.ActionPropose); err != nil { + s.log.Warn("refused an agent push", "space", space.String(), "error", err) + return authn.Principal{}, rejectedResponse(badTokenMessage(space, err)), false + } + return p, Response{}, true default: // Request.Validate rejected every other spelling already. return authn.Principal{}, errorResponse("unknown principal kind %q", cred.Kind), false @@ -593,15 +604,16 @@ fmt.Fprintf(&b, "frontmatter and document-id validation. Nothing was written.\n") return b.String() } -// badTokenMessage is what an agent sees when its token does not authenticate. -// It never echoes the token. +// badTokenMessage is what an agent sees when its credential does not +// authenticate or does not carry spec:propose. It never echoes the token. func badTokenMessage(space core.SpaceRef, cause error) string { var b strings.Builder fmt.Fprintf(&b, "spec.sr.ht rejected this push.\n\n") fmt.Fprintf(&b, " space: %s\n\n", space) - fmt.Fprintf(&b, " the agent token presented with this push was refused:\n") + fmt.Fprintf(&b, " the agent credential presented with this push was refused:\n") fmt.Fprintf(&b, " %v\n\n", cause) - fmt.Fprintf(&b, "Nothing was written. Agents write through the REST and MCP planes,\n") - fmt.Fprintf(&b, "not over git; a token that works there is not a git credential.\n") + fmt.Fprintf(&b, "Nothing was written. Agent credentials are tokens.sr.ht working tokens,\n") + fmt.Fprintf(&b, "the same ones the REST and MCP planes take, and a push needs the\n") + fmt.Fprintf(&b, "spec:propose grant just as those do.\n") return b.String() } diff --git a/hooks/server_test.go b/hooks/server_test.go index dd5307b9c8122a112b61acdb33b2b79e83648dda..c2886f0ec98ac203192ec7d3ca54a0c00c6e7495 100644 --- a/hooks/server_test.go +++ b/hooks/server_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "net/http" "path/filepath" "strings" "testing" @@ -11,7 +12,6 @@ "time" "sourcecraft.dev/bigbes/sr-ht-spec/authn" "sourcecraft.dev/bigbes/sr-ht-spec/core" - "sourcecraft.dev/bigbes/sr-ht-spec/db" "sourcecraft.dev/bigbes/sr-ht-spec/service" ) @@ -41,7 +41,15 @@ back *fakeBackend srv *Server } -func newServerFixture(t *testing.T, tokens map[string]*db.AgentToken) *serverFixture { +func newServerFixture(t *testing.T) *serverFixture { + t.Helper() + return newServerFixtureAgainst(t, fakeDaemonOrigin(t, http.StatusNoContent, false)) +} + +// newServerFixtureAgainst is the same fixture with the tokens.sr.ht origin +// chosen by the caller, so a test can put a revoking or an unreachable daemon +// behind the agent plane. +func newServerFixtureAgainst(t *testing.T, origin string) *serverFixture { t.Helper() root := shortTempDir(t) repo := filepath.Join(root, "~"+testSpace.Owner, testSpace.Name) @@ -50,7 +58,7 @@ if err := mkdirAll(filepath.Join(repo, sub)); err != nil { t.Fatalf("MkdirAll: %v", err) } } - back := newFakeBackend(t, root, tokens) + back := newFakeBackendAgainst(t, root, origin) srv, _ := startServer(t, back) return &serverFixture{root: root, repo: repo, back: back, srv: srv} } @@ -71,7 +79,7 @@ // TestPushLifecycle is the protocol as one push runs it: pre-receive records // the options, update reads them back, post-receive lands. func TestPushLifecycle(t *testing.T) { - f := newServerFixture(t, nil) + f := newServerFixture(t) pre := f.request(MethodPushOptions, "4711", mainUpdate) pre.Options = []string{OptionSkipValidation} @@ -107,7 +115,7 @@ // TestSkipValidationDefaultsOff proves the waiver is opt-in per push and does // not leak from one push into the next. func TestSkipValidationDefaultsOff(t *testing.T) { - f := newServerFixture(t, nil) + f := newServerFixture(t) waived := f.request(MethodPushOptions, "1", mainUpdate) waived.Options = []string{OptionSkipValidation} @@ -133,7 +141,7 @@ // correlation. Absence is not read as "not waived": it means the hooks are // half installed or the daemon restarted mid-push, and either deserves a // sentence rather than a guess. func TestUpdateWithoutPreReceiveIsRefused(t *testing.T) { - f := newServerFixture(t, nil) + f := newServerFixture(t) resp := call(t, f.srv, f.request(MethodValidateRef, "4711", mainUpdate)) if resp.OK { @@ -149,7 +157,7 @@ // TestUpdateForAnUnannouncedRefIsRefused is what makes the receive-pack pid // safe as a correlation key: a recycled pid would also have to be paired with // an identical ref and object names. func TestUpdateForAnUnannouncedRefIsRefused(t *testing.T) { - f := newServerFixture(t, nil) + f := newServerFixture(t) call(t, f.srv, f.request(MethodPushOptions, "4711", mainUpdate)) other := RefUpdate{Ref: "refs/heads/proposals/9", Old: zeroOID, New: twoOID} @@ -166,7 +174,7 @@ repo := filepath.Join(root, "~"+testSpace.Owner, testSpace.Name) if err := mkdirAll(repo); err != nil { t.Fatalf("MkdirAll: %v", err) } - back := newFakeBackend(t, root, nil) + back := newFakeBackend(t, root) srv, _ := startServer(t, back, func(o *Options) { o.OptionTTL = time.Nanosecond }) f := &serverFixture{root: root, repo: repo, back: back, srv: srv} @@ -181,7 +189,7 @@ // TestUnknownPushOptionIsRejected: with one option in the vocabulary, silently // ignoring a typo would reject the push for the very thing the human believed // they had waived. func TestUnknownPushOptionIsRejected(t *testing.T) { - f := newServerFixture(t, nil) + f := newServerFixture(t) req := f.request(MethodPushOptions, "4711", mainUpdate) req.Options = []string{"skip-validaton"} @@ -200,7 +208,7 @@ // TestRepositoryMustBeOurs: a hook can only ever address a repository this // daemon owns, because the path is re-derived through gitx's layout rather // than parsed out of what the hook claimed. func TestRepositoryMustBeOurs(t *testing.T) { - f := newServerFixture(t, nil) + f := newServerFixture(t) outside := []struct { name string @@ -231,7 +239,7 @@ // TestOwnerCredentialCannotNameSomebodyElse: the wire carries a kind, never a // username. The owner comes from the resolver. func TestOwnerCredentialCannotNameSomebodyElse(t *testing.T) { - f := newServerFixture(t, nil) + f := newServerFixture(t) call(t, f.srv, f.request(MethodPushOptions, "1", mainUpdate)) call(t, f.srv, f.request(MethodValidateRef, "1", mainUpdate)) @@ -243,85 +251,115 @@ t.Errorf("owner: got %q want %q", got, testOwner) } } -func TestAgentCredential(t *testing.T) { - live := &db.AgentToken{ID: 1, Name: "laptop", Hash: authn.HashToken("good"), Created: time.Now()} - revokedAt := time.Now().Add(-time.Hour) - revoked := &db.AgentToken{ID: 2, Name: "old", Hash: authn.HashToken("dead"), Revoked: &revokedAt} - f := newServerFixture(t, map[string]*db.AgentToken{"good": live, "dead": revoked}) +// agentRequest is a push by an agent presenting token. +func agentRequest(f *serverFixture, method Method, token string) Request { + req := f.request(method, "1", mainUpdate) + req.Credential = Credential{Kind: PrincipalAgent, Token: token, Agent: "claude/spec", Session: "s1"} + return req +} - agent := func(token string) Request { - req := f.request(MethodPushOptions, "1", mainUpdate) - req.Credential = Credential{Kind: PrincipalAgent, Token: token, Agent: "claude/spec", Session: "s1"} - return req +// The push path takes a tokens.sr.ht working token, which is the whole point of +// routing it through authn.Resolver: an agent can `git push` with the same +// credential it uses on the REST and MCP planes. +func TestAgentCredentialAcceptsAnInstanceToken(t *testing.T) { + f := newServerFixture(t) + tok := instanceToken("spec:propose") + + if resp := call(t, f.srv, agentRequest(f, MethodPushOptions, tok)); !resp.OK { + t.Fatalf("a valid instance token was refused: %+v", resp) + } + if resp := call(t, f.srv, agentRequest(f, MethodValidateRef, tok)); !resp.OK { + t.Fatalf("update: %+v", resp) } - t.Run("a valid token resolves to an agent", func(t *testing.T) { - if resp := call(t, f.srv, agent("good")); !resp.OK { - t.Fatalf("a valid agent token was refused: %+v", resp) - } - req := f.request(MethodValidateRef, "1", mainUpdate) - req.Credential = Credential{Kind: PrincipalAgent, Token: "good", Agent: "claude/spec", Session: "s1"} - if resp := call(t, f.srv, req); !resp.OK { - t.Fatalf("update: %+v", resp) - } - p := f.back.seen[len(f.back.seen)-1].Principal - if !p.IsAgent() || p.Agent != "claude/spec" || p.Session != "s1" || p.TokenName != "laptop" { - t.Errorf("principal: %+v", p) - } - }) + p := f.back.seen[len(f.back.seen)-1].Principal + if !p.IsAgent() || p.Agent != "claude/spec" || p.Session != "s1" { + t.Errorf("principal: %+v", p) + } + if p.Plane != authn.PlaneInstance { + t.Errorf("plane = %q, want the instance plane", p.Plane) + } + if p.TokenName != "tokens.sr.ht (stateless)" { + t.Errorf("TokenName = %q", p.TokenName) + } +} - t.Run("an unknown token is a rejection, and the token is not echoed", func(t *testing.T) { - resp := call(t, f.srv, agent("wrong")) - if resp.OK || !resp.Rejected { - t.Fatalf("an unknown token was not rejected: %+v", resp) - } - mentionsNot(t, "the rejection", resp.Message, "wrong") - }) +// The credential every agent used to push with — an opaque secret out of +// agent_token — authenticates nowhere. It is not a token this instance sealed, +// and there is no second store left to ask. +func TestAgentCredentialRefusesTheOldOpaqueToken(t *testing.T) { + f := newServerFixture(t) - t.Run("a revoked token says revoked", func(t *testing.T) { - resp := call(t, f.srv, agent("dead")) - if resp.OK || !resp.Rejected { - t.Fatalf("a revoked token was not rejected: %+v", resp) - } - mentions(t, "the rejection", resp.Message, "revoked") - }) + resp := call(t, f.srv, agentRequest(f, MethodPushOptions, "s3cret-from-agent-token")) + if resp.OK || !resp.Rejected { + t.Fatalf("an opaque secret was not rejected: %+v", resp) + } + mentionsNot(t, "the rejection", resp.Message, "s3cret-from-agent-token") } -// TestTokenStoreOutageIsNotABadCredential: a store that cannot answer must -// fail the push closed, not read as an invalid token — the difference between -// "retry later" and "reprovision your agent". -func TestTokenStoreOutageIsNotABadCredential(t *testing.T) { - root := shortTempDir(t) - repo := filepath.Join(root, "~"+testSpace.Owner, testSpace.Name) - if err := mkdirAll(repo); err != nil { - t.Fatalf("MkdirAll: %v", err) +// A push is a proposal by another transport, so it needs spec:propose exactly +// as the REST and MCP write planes do — and a token without it is refused +// before any ref is looked at. +func TestAgentCredentialRefusesATokenWithoutTheProposeGrant(t *testing.T) { + f := newServerFixture(t) + + for _, grantString := range []string{"spec:read", "bench:upload"} { + t.Run(grantString, func(t *testing.T) { + resp := call(t, f.srv, agentRequest(f, MethodPushOptions, instanceToken(grantString))) + if resp.OK || !resp.Rejected { + t.Fatalf("a token without spec:propose was not rejected: %+v", resp) + } + mentions(t, "the rejection", resp.Message, "spec:propose") + }) } - lookup := &fakeLookup{err: errFakeStore} - store := service.NewTokenStore(lookup) - resolver, err := authn.NewResolver(testOwner, store) - if err != nil { - t.Fatalf("NewResolver: %v", err) + if len(f.back.seen) != 0 { + t.Errorf("a refused credential reached the backend %d times", len(f.back.seen)) } - back := &fakeBackend{root: root, resolver: resolver, tokens: store} - srv, _ := startServer(t, back) - f := &serverFixture{root: root, repo: repo, back: back, srv: srv} +} - req := f.request(MethodPushOptions, "1", mainUpdate) - req.Credential = Credential{Kind: PrincipalAgent, Token: "anything"} - resp := call(t, f.srv, req) +// spec.sr.ht answers to one human on this instance, over git as over HTTP. +func TestAgentCredentialRefusesATokenOfAnotherOwner(t *testing.T) { + f := newServerFixture(t) + + resp := call(t, f.srv, agentRequest(f, MethodPushOptions, foreignToken("spec:propose"))) + if resp.OK || !resp.Rejected { + t.Fatalf("a foreign owner's token was not rejected: %+v", resp) + } + mentions(t, "the rejection", resp.Message, "someone") +} + +// A revoked token says revoked, so an operator can tell a credential they +// killed from one that never existed. +func TestAgentCredentialRefusesARevokedToken(t *testing.T) { + f := newServerFixtureAgainst(t, fakeDaemonOrigin(t, http.StatusNotFound, false)) + + resp := call(t, f.srv, agentRequest(f, MethodPushOptions, instanceToken("spec:propose id:42"))) + if resp.OK || !resp.Rejected { + t.Fatalf("a revoked token was not rejected: %+v", resp) + } + mentions(t, "the rejection", resp.Message, "revoked") +} + +// TestUnreachableDaemonIsNotABadCredential: a validator that cannot complete +// its revocation check must fail the push closed, not read as an invalid token +// — the difference between "retry later" and "reprovision your agent". +func TestUnreachableDaemonIsNotABadCredential(t *testing.T) { + f := newServerFixtureAgainst(t, fakeDaemonOrigin(t, http.StatusNoContent, true)) + + resp := call(t, f.srv, agentRequest(f, MethodPushOptions, instanceToken("spec:propose id:42"))) if resp.OK { - t.Fatal("a store outage let a push through") + t.Fatal("an unreachable tokens.sr.ht let a push through") } if resp.Rejected { - t.Errorf("a store outage was reported as a bad credential: %+v", resp) + t.Errorf("an unreachable daemon was reported as a bad credential: %+v", resp) } - mentions(t, "the failure", resp.Error, "could not check the agent token") + mentions(t, "the failure", resp.Error, "could not check the credential") } // TestValidatePushRejectionIsPassedThroughVerbatim: the daemon composed the // text for a terminal and this package must not reword it. func TestValidatePushRejectionIsPassedThroughVerbatim(t *testing.T) { - f := newServerFixture(t, nil) + f := newServerFixture(t) want := rejection("refs/heads/main", true, service.PushProblem{ Kind: service.ProblemFrontmatter, Path: "specs/0002-broken.md", @@ -342,7 +380,7 @@ // TestInfrastructureFailureIsNotAPolicyRejection keeps "you broke a rule" // and "we broke" apart: only the first is worth changing your push over. func TestInfrastructureFailureIsNotAPolicyRejection(t *testing.T) { - f := newServerFixture(t, nil) + f := newServerFixture(t) f.back.validate = func(context.Context, service.PushRequest) error { return fmt.Errorf("service: look up space: %w", errFakeStore) } @@ -366,7 +404,7 @@ repo := filepath.Join(root, "~"+testSpace.Owner, testSpace.Name) if err := mkdirAll(repo); err != nil { t.Fatalf("MkdirAll: %v", err) } - back := newFakeBackend(t, root, nil) + back := newFakeBackend(t, root) srv, _ := startServer(t, back, func(o *Options) { o.OnPush = func(context.Context, core.SpaceRef, []RefUpdate) error { return errors.New("the indexer is not running") @@ -383,7 +421,7 @@ } func TestNewServerRequiresItsWiring(t *testing.T) { root := shortTempDir(t) - back := newFakeBackend(t, root, nil) + back := newFakeBackend(t, root) ok := Options{Backend: back, Socket: SocketPath(root), OnPush: func(context.Context, core.SpaceRef, []RefUpdate) error { return nil }} @@ -422,7 +460,7 @@ // each validate half the pushes, and the second would silently take over the // push path from the first. func TestListenRefusesToStealALiveSocket(t *testing.T) { root := shortTempDir(t) - back := newFakeBackend(t, root, nil) + back := newFakeBackend(t, root) startServer(t, back) second, err := NewServer(Options{ @@ -452,7 +490,7 @@ if err := writeFile(socket, "not a socket"); err != nil { t.Fatalf("write a stale socket: %v", err) } - back := newFakeBackend(t, root, nil) + back := newFakeBackend(t, root) srv, err := NewServer(Options{ Backend: back, Socket: socket, Log: discardLogger(), OnPush: func(context.Context, core.SpaceRef, []RefUpdate) error { return nil }, @@ -469,7 +507,7 @@ // TestGarbageOnTheSocketIsAnswered: the peer may not be a hook at all, and a // closed connection would leave a real hook guessing. func TestGarbageOnTheSocketIsAnswered(t *testing.T) { - f := newServerFixture(t, nil) + f := newServerFixture(t) conn, err := dialUnix(f.srv.Socket()) if err != nil { t.Fatalf("dial: %v", err) diff --git a/mcpsrv/grant_internal_test.go b/mcpsrv/grant_internal_test.go index 0c22a954b8ceccf1865b8d409bbb91f34c308f4f..77a41d6312f6996ecb5b2a87f7b58b5694f30e33 100644 --- a/mcpsrv/grant_internal_test.go +++ b/mcpsrv/grant_internal_test.go @@ -33,12 +33,11 @@ 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 -} +// cliPrincipal is agentPrincipal as `specsrht doc propose` builds it: an agent +// a local process asserted, on no credential plane and therefore with no grant +// set. The resolver never produces one — it is why Plane outlived the local +// agent-token plane it used to distinguish. +func cliPrincipal() authn.Principal { return agentPrincipal() } // 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 @@ -72,12 +71,12 @@ 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. +// And every principal that carries no grants passes untouched: the owner's +// cookie is a person rather than a machine credential, and the CLI's agent +// presented nothing to have a grant clipped out of. func TestReadToolsPassEveryUngrantedCredential(t *testing.T) { for name, p := range map[string]authn.Principal{ - "local agent token": localPrincipal(), + "locally asserted CLI agent": cliPrincipal(), "owner cookie": {Kind: authn.KindOwner, Owner: "bigbes"}, "instance token, spec:read": instancePrincipal(t, "spec:read"), "instance token, universal": instancePrincipal(t, "*"), @@ -152,13 +151,12 @@ }) } } -// 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. +// And every credential that may propose clears it. func TestProposeToolAcceptsTheGrantedCredentials(t *testing.T) { svc := realWriter(t) for name, p := range map[string]authn.Principal{ - "local agent token": localPrincipal(), + "locally asserted CLI agent": cliPrincipal(), "instance token, spec:propose": instancePrincipal(t, "spec:propose"), "instance token, universal": instancePrincipal(t, "*"), } { @@ -177,12 +175,12 @@ // 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) { +func TestProposeToolStillDemandsProvenance(t *testing.T) { svc := realWriter(t) for name, p := range map[string]authn.Principal{ - "local": localPrincipal(), - "instance": instancePrincipal(t, "*"), + "locally asserted CLI agent": cliPrincipal(), + "instance token": instancePrincipal(t, "*"), } { t.Run(name, func(t *testing.T) { noSession := p diff --git a/migrations/0005_drop_agent_token.sql b/migrations/0005_drop_agent_token.sql new file mode 100644 index 0000000000000000000000000000000000000000..e93bfb576c35ae5ec18cdcccf493ee83cacbe26b --- /dev/null +++ b/migrations/0005_drop_agent_token.sql @@ -0,0 +1,30 @@ +-- +brant Up + +-- Agent issuance moved to tokens.sr.ht, so spec.sr.ht stops holding a +-- credential of its own. +-- +-- agent_token was one instance-wide shared secret with no owner, no expiry and +-- no grants: every agent on the instance presented the same string, and the only +-- boundary around it was the refs rule (an agent credential may only move refs +-- under proposals/*). That rule is untouched and still the boundary that +-- matters; what changes is that the credential is now per-agent, owned, expiring +-- and grant-carrying, and is verified by signature rather than by a lookup here. +-- +-- DEPLOY GATE: applying this locks out every agent still configured with the +-- shared secret, over HTTP and over `git push` alike. Every one of them must +-- hold a tokens.sr.ht working token *before* this migration runs. +-- +-- The rows are dropped with the table and the Down below cannot bring them back: +-- only sha256 hashes were ever stored, and nothing derives a token from its +-- hash. A rollback therefore restores the shape of the old plane and none of its +-- credentials, which have to be re-issued. +DROP TABLE agent_token; + +-- +brant Down +CREATE TABLE agent_token ( + id SERIAL PRIMARY KEY, + name TEXT NOT NULL, + token_hash BYTEA NOT NULL UNIQUE, + created TIMESTAMPTZ NOT NULL DEFAULT now(), + revoked TIMESTAMPTZ +); diff --git a/schema.sql b/schema.sql index 80bd79119709f79b7bd7b1d6f5814bc82adbb5a9..b51b83e6111ee8846a82fbbe3c0795df56e827ef 100644 --- a/schema.sql +++ b/schema.sql @@ -71,15 +71,11 @@ -- The inbox ("N proposals waiting on you") and the digest are both -- state-filtered, newest-first scans. CREATE INDEX ix_proposal_state_created ON proposal (state, created DESC); --- Only a hash is ever stored; the token itself exists exactly once, at mint --- time, in the response to the operator. -CREATE TABLE agent_token ( - id SERIAL PRIMARY KEY, - name TEXT NOT NULL, - token_hash BYTEA NOT NULL UNIQUE, - created TIMESTAMPTZ NOT NULL DEFAULT now(), - revoked TIMESTAMPTZ -); +-- There is no credential table. agent_token stood here — one instance-wide +-- shared secret, stored as a sha256 — until agent issuance moved to +-- tokens.sr.ht (migration 0005). A working token is signed by the instance and +-- carries its own owner, expiry and grants, so authenticating one is a +-- signature check in authn/ and there is nothing here to look up. -- Index staleness: compared against the space's approved head. CREATE TABLE index_stamp ( diff --git a/service/bearer.go b/service/bearer.go index 4d9fbe298586099201244990695bc40a5bbf309f..8b508cd95f426db0b097517c1e28f269a8d08a16 100644 --- a/service/bearer.go +++ b/service/bearer.go @@ -34,14 +34,14 @@ 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. +// WithInstanceTokens builds the tokens.sr.ht bearer plane from the instance +// config. It is the only plane an agent can authenticate on, so passing this +// option and having no [tokens.sr.ht] section is a startup failure — see +// instancePlane. // -// 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. +// It is an option rather than a parameter because `specsrht doc` also builds a +// Service, authenticates nobody, and has no use for a validator or the HTTP +// client behind it. func WithInstanceTokens(conf ini.File) Option { return func(o *options) { o.conf = conf @@ -49,16 +49,15 @@ 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). +// instancePlane builds the tokens.sr.ht bearer plane from the instance config. // -// 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. +// A missing [tokens.sr.ht] origin used to be an answer rather than an error: +// spec minted its own agent token, so an instance without the daemon simply kept +// using the plane it shipped with. It no longer has one. A daemon that came up +// without this plane would serve reads and refuse every agent write on the +// instance — over HTTP and over `git push` alike — so the absence fails startup, +// where an operator is looking, instead of surfacing 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 @@ -66,7 +65,9 @@ // 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 + return nil, fmt.Errorf( + "service: no [%s] origin in config.ini; spec.sr.ht issues no agent credential of "+ + "its own and cannot authenticate an agent without the daemon that does", TokensSection) } // The node id is what the daemon's internal guard logs the caller as. The diff --git a/service/bearer_test.go b/service/bearer_test.go index 51dbb6a4d5bd5a3ffe04f9447de99a4691af2233..58bfe43f612b465ba47e9bfb24a3093517666951 100644 --- a/service/bearer_test.go +++ b/service/bearer_test.go @@ -36,14 +36,12 @@ 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 -} +// cliAgent is agentPrincipal as `specsrht doc propose` builds it: an agent a +// local process asserted, on no credential plane and with no grant set, because +// it presented no credential. It is the one agent principal the resolver never +// produces, and the reason authn.Plane outlived the local plane it used to +// distinguish. +func cliAgent() authn.Principal { return agentPrincipal() } // proposeRequest is a well-formed open, so that the only thing a test varies is // the principal. @@ -80,7 +78,7 @@ // runs before anything is opened. func TestProposeAcceptsTheGrantedPrincipals(t *testing.T) { svc, _ := newService(t) cases := map[string]authn.Principal{ - "local agent token": localAgent(), + "locally asserted CLI agent": cliAgent(), "instance token, exact grant": instanceAgent(t, "spec:propose"), "instance token, both grants": instanceAgent(t, "spec:read spec:propose"), "instance token, universal": instanceAgent(t, "*"), @@ -98,37 +96,37 @@ }) } } -// The refs rule is untouched by grants and applies to both planes identically: +// The refs rule is untouched by grants and by the removal of the local plane: // 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, "*"), +func TestRefsRuleIsUnchangedByGrants(t *testing.T) { + agents := map[string]authn.Principal{ + "locally asserted CLI agent": cliAgent(), + "instance token, universal": instanceAgent(t, "*"), } newHash := plumbing.NewHash("1f0c1d1a1e2b3c4d5e6f708192a3b4c5d6e7f809") - for name, p := range planes { + for name, p := range agents { 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") + "an agent is an agent 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") + "an agent may not move the approved branch") 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") + "an agent may not create a branch outside the proposal prefix") - // Inside it: permitted, on both planes. + // Inside it: permitted. err = gitx.CheckRefUpdate(kind, gitx.DefaultApprovedBranch, gitx.RefUpdate{ Ref: "refs/heads/" + core.ProposalPrefix + "7", New: newHash, FastForward: true, }) @@ -137,14 +135,14 @@ }) } } -// 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) { +// Provenance is still mandatory. 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 TestProvenanceStillRequired(t *testing.T) { svc, _ := newService(t) for name, p := range map[string]authn.Principal{ - "local": localAgent(), - "instance": instanceAgent(t, "*"), + "locally asserted CLI agent": cliAgent(), + "instance token": instanceAgent(t, "*"), } { t.Run(name, func(t *testing.T) { noSession := p @@ -174,11 +172,12 @@ } 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) { +// A missing [tokens.sr.ht] origin used to be an answer — spec minted its own +// credential, so an instance without the daemon simply used the plane it +// shipped with. It has none now, so the absence is a startup failure: a daemon +// that came up like this would serve reads and refuse every agent write on the +// instance, over HTTP and over `git push` alike. +func TestInstancePlane_AbsentSectionIsAStartupError(t *testing.T) { for name, conf := range map[string]ini.File{ "no section at all": tokensConf(nil), "section with no origin": tokensConf(ini.Section{ @@ -186,9 +185,9 @@ "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) + _, err := instancePlane(conf, deadDB(t)) + require.Error(t, err, "there is no other plane left to fall back to") + assert.Contains(t, err.Error(), TokensSection) }) } } @@ -203,7 +202,7 @@ }), deadDB(t)) require.NoError(t, err) require.NotNil(t, plane) - rs, err := authn.NewResolver("bigbes", NewTokenStore(nil), plane) + rs, err := authn.NewResolver("bigbes", plane) require.NoError(t, err) assert.True(t, rs.HasInstancePlane()) } @@ -216,20 +215,24 @@ 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) { +// Service.New wires the plane when a config supports it, refuses when the +// option is passed and the config does not, and builds a planeless resolver for +// the CLI paths that do not ask for one. +func TestNew_InstancePlaneWiring(t *testing.T) { cfg := testConfig(t, t.TempDir()) + // `specsrht doc`: authenticates nobody, so it asks for no plane and gets a + // resolver that refuses every credential rather than pretending to check + // one. 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") + assert.False(t, svc.Resolver().HasInstancePlane()) - 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") + // The daemon: asks for the plane, and an instance that cannot give it one + // fails here rather than at every agent's first request. + _, err = New(cfg, deadDB(t), WithInstanceTokens(tokensConf(nil))) + require.Error(t, err) + assert.Contains(t, err.Error(), TokensSection) svc, err = New(cfg, deadDB(t), WithInstanceTokens(tokensConf(ini.Section{ "origin": "https://tokens.srht.bigb.es", @@ -240,13 +243,13 @@ } // --- 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) { +// The whole open path: a tokens.sr.ht token carrying spec:propose and the CLI's +// locally asserted agent both open a proposal, with the same provenance +// recorded on it. +func TestProposeOpensProposalForEveryAgentPrincipal(t *testing.T) { for name, p := range map[string]authn.Principal{ - "local agent token": localAgent(), - "instance token": instanceAgent(t, "spec:read spec:propose"), + "locally asserted CLI agent": cliAgent(), + "instance token": instanceAgent(t, "spec:read spec:propose"), } { t.Run(name, func(t *testing.T) { svc, _ := newTestService(t) diff --git a/service/service.go b/service/service.go index e3f36b1141e3f5dc5adc6c3c6e9b73c9cad312dd..44f7d19d81d03060032b25ff37353de966ac1fb9 100644 --- a/service/service.go +++ b/service/service.go @@ -215,7 +215,6 @@ type Service struct { cfg Config q db.Querier store *db.Store - tokens *TokenStore resolver *authn.Resolver // ownerUserID caches the id of the owner's "user" row, seeded by @@ -242,13 +241,14 @@ // // q is normally the *sql.DB the daemon opened from Config.ConnectionString and // 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. +// rather than tolerated: half this layer would then fail one query at a time +// instead of once, at startup, where an operator is looking. // -// 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. +// Pass WithInstanceTokens(conf) to give the resolver the tokens.sr.ht plane — +// the only plane an agent can authenticate on. The daemon passes it and fails +// startup without a [tokens.sr.ht] origin; the CLI paths (`specsrht doc`) +// authenticate nobody and omit it, and the resolver they get refuses every +// bearer credential rather than pretending to check one. func New(cfg Config, q db.Querier, opts ...Option) (*Service, error) { if err := cfg.Validate(); err != nil { return nil, err @@ -267,22 +267,17 @@ plane, err := instancePlane(o.conf, q) if err != nil { return nil, err } - if plane != nil { - ropts = append(ropts, plane) - } + ropts = append(ropts, plane) } - store := db.NewStore(q) - tokens := NewTokenStore(store) - resolver, err := authn.NewResolver(cfg.Instance.OwnerName, tokens, ropts...) + resolver, err := authn.NewResolver(cfg.Instance.OwnerName, ropts...) if err != nil { return nil, fmt.Errorf("service: build resolver: %w", err) } return &Service{ cfg: cfg, q: q, - store: store, - tokens: tokens, + store: db.NewStore(q), resolver: resolver, grace: DefaultReconcileGrace, now: time.Now, diff --git a/service/service_test.go b/service/service_test.go index 7cb0c71e7a7ee37549a4f9bdd8a2cdeaf12665b6..a1c14e82c03444e527c481727870a9158b8f35a1 100644 --- a/service/service_test.go +++ b/service/service_test.go @@ -1,14 +1,10 @@ package service import ( - "context" "errors" "strings" "testing" "time" - - "sourcecraft.dev/bigbes/sr-ht-spec/authn" - "sourcecraft.dev/bigbes/sr-ht-spec/db" ) func TestLoadConfigAcceptsACompleteConfig(t *testing.T) { @@ -104,65 +100,6 @@ t.Fatalf("err = %v, want ErrIncompleteConfig", err) } } -// fakeTokens is an agentTokenLookup that answers from a script. -type fakeTokens struct { - row *db.AgentToken - err error -} - -func (f fakeTokens) AgentTokenByHash(context.Context, []byte) (*db.AgentToken, error) { - return f.row, f.err -} - -// The whole of the adapter is this error contract: db/ says ErrNotFound, authn -// demands ErrUnknownToken, and an unmapped pass-through would turn a bad -// credential into a 503 telling the agent to retry forever. -func TestTokenStoreMapsAMissingRowToUnknownToken(t *testing.T) { - ts := NewTokenStore(fakeTokens{err: db.ErrNotFound}) - _, err := ts.LookupAgentToken(context.Background(), []byte("hash")) - if !errors.Is(err, authn.ErrUnknownToken) { - t.Fatalf("err = %v, want authn.ErrUnknownToken", err) - } - if !authn.IsAuthFailure(err) { - t.Error("an unknown token must be a permanent auth failure, not a transient one") - } -} - -func TestTokenStoreKeepsOtherFailuresTransient(t *testing.T) { - boom := errors.New("connection refused") - ts := NewTokenStore(fakeTokens{err: boom}) - _, err := ts.LookupAgentToken(context.Background(), []byte("hash")) - if !errors.Is(err, boom) { - t.Fatalf("err = %v, want it to wrap the store failure", err) - } - if authn.IsAuthFailure(err) { - t.Error("a store outage must never read as a bad credential") - } -} - -// A revoked row is returned rather than refused, so authn can say "revoked" -// instead of "unknown". -func TestTokenStoreReturnsARevokedRow(t *testing.T) { - revoked := fxTime(1) - ts := NewTokenStore(fakeTokens{row: &db.AgentToken{ - ID: 7, Name: "cron", Hash: []byte("h"), Created: fxTime(0), Revoked: &revoked, - }}) - tok, err := ts.LookupAgentToken(context.Background(), []byte("h")) - if err != nil { - t.Fatalf("LookupAgentToken: %v", err) - } - if !tok.IsRevoked() || tok.ID != 7 || tok.Name != "cron" { - t.Fatalf("token = %+v", tok) - } -} - -func TestTokenStoreRefusesANilRowWithNoError(t *testing.T) { - ts := NewTokenStore(fakeTokens{}) - if _, err := ts.LookupAgentToken(context.Background(), []byte("h")); err == nil { - t.Fatal("a nil row with no error authenticated") - } -} - func TestServiceExposesItsWiring(t *testing.T) { svc, root := newService(t) if svc.ReposRoot() != root { @@ -174,8 +111,8 @@ } if svc.Resolver() == nil || svc.Resolver().Owner() != "bigbes" { t.Errorf("resolver = %v", svc.Resolver()) } - if svc.Store() == nil || svc.TokenStore() == nil { - t.Error("store or token store is nil") + if svc.Store() == nil { + t.Error("store is nil") } if svc.grace != DefaultReconcileGrace || svc.now == nil { t.Errorf("reconciler defaults not wired: grace=%v", svc.grace) diff --git a/service/token.go b/service/token.go deleted file mode 100644 index afa5ed2e4b542d945dd66ae69156580ffedc7b9f..0000000000000000000000000000000000000000 --- a/service/token.go +++ /dev/null @@ -1,197 +0,0 @@ -package service - -import ( - "context" - "errors" - "fmt" - "strings" - "time" - "unicode/utf8" - - "sourcecraft.dev/bigbes/sr-ht-spec/authn" - "sourcecraft.dev/bigbes/sr-ht-spec/db" -) - -// MaxTokenNameLen bounds a token's label. It is a human-readable note about -// which agent holds the credential, rendered in a table and in `token list`; -// the cap keeps a pasted paragraph from becoming a row nobody can read. -const MaxTokenNameLen = 128 - -// AgentTokenLookup is the one db/ method the token adapter needs. It is an -// interface rather than a *db.Store so the error mapping below — the part that -// actually carries a contract — can be tested against a fake instead of a -// Postgres instance. *db.Store satisfies it. -type AgentTokenLookup interface { - AgentTokenByHash(ctx context.Context, hash []byte) (*db.AgentToken, error) -} - -// TokenStore adapts db/'s agent_token queries to authn.TokenStore. It exists -// because authn must not import db: authn declares the sliver of persistence it -// needs, and service/ — the layer that is allowed to know about both — wires -// them together. -// -// The whole of the adaptation is the error contract, and it is not cosmetic. -// authn's contract is that "no such token" is an error satisfying -// errors.Is(err, authn.ErrUnknownToken) and that everything else is transient. -// db/ spells the same condition ErrNotFound, which authn has never heard of, so -// an unmapped pass-through would make an unknown token look like a Postgres -// outage: a 503 telling an agent to retry a credential that will never work. -type TokenStore struct { - lookup AgentTokenLookup -} - -// NewTokenStore wires a db.Store in as authn's TokenStore. -func NewTokenStore(lookup AgentTokenLookup) *TokenStore { - return &TokenStore{lookup: lookup} -} - -// TokenStore returns the adapter the resolver authenticates agents through. -func (s *Service) TokenStore() *TokenStore { return s.tokens } - -// LookupAgentToken implements authn.TokenStore. -// -// A revoked row is returned rather than refused: authn is what turns it into a -// refusal, so the refusal can say "revoked" instead of "unknown" and an -// operator can tell a token they deliberately killed from one that never -// existed. Any other failure is returned wrapped and unclassified, which authn -// reads as transient — the fail-closed direction, since a store outage must -// never read as a valid credential. -func (t *TokenStore) LookupAgentToken(ctx context.Context, hash []byte) (authn.AgentToken, error) { - if t.lookup == nil { - return authn.AgentToken{}, errors.New("service: TokenStore has no backing store") - } - row, err := t.lookup.AgentTokenByHash(ctx, hash) - if err != nil { - if errors.Is(err, db.ErrNotFound) { - return authn.AgentToken{}, fmt.Errorf("%w: no agent_token row matches the presented token", - authn.ErrUnknownToken) - } - return authn.AgentToken{}, fmt.Errorf("service: look up agent token: %w", err) - } - if row == nil { - // db/ never returns (nil, nil); a store that did would otherwise - // authenticate a nil row as a valid token. - return authn.AgentToken{}, errors.New("service: agent token lookup returned no row and no error") - } - return authn.AgentToken{ - ID: int64(row.ID), - Name: row.Name, - Hash: row.Hash, - Created: row.Created, - Revoked: row.Revoked, - }, nil -} - -// AgentToken is one credential as the surfaces above this layer need it: no -// hash, because nothing above service/ has any use for it, and no plaintext, -// because it exists only in the response to the call that minted it. -type AgentToken struct { - ID int - Name string - Created time.Time - Revoked *time.Time -} - -// Active reports whether the token may still authenticate. -func (t AgentToken) Active() bool { return t.Revoked == nil } - -// IssueAgentToken mints a credential for the agent write plane and returns the -// plaintext exactly once, alongside the stored row. -// -// Owner-only, and that is the interesting half of the ACL: an agent holding a -// valid token may not mint another. Were it allowed to, revoking a compromised -// credential would not end the compromise — the holder would simply have issued -// itself a second one — and "revoke the token" is the entire incident response -// this design has. -// -// The plaintext is returned rather than stored. There is no second chance to -// read it, which is what makes a leaked database dump unreplayable, so a caller -// that drops the value has to mint a new token. -func (s *Service) IssueAgentToken(ctx context.Context, p authn.Principal, name string) (string, AgentToken, error) { - if !p.IsOwner() { - return "", AgentToken{}, fmt.Errorf("%w: %s may not issue agent tokens; only the instance owner may", - ErrForbidden, p) - } - name, err := validateTokenName(name) - if err != nil { - return "", AgentToken{}, err - } - - token, err := db.GenerateToken() - if err != nil { - return "", AgentToken{}, fmt.Errorf("service: %w", err) - } - row, err := s.store.CreateAgentToken(ctx, name, db.HashToken(token)) - if err != nil { - return "", AgentToken{}, fmt.Errorf("service: issue agent token %q: %w", name, err) - } - return token, tokenView(row), nil -} - -// ListAgentTokens returns every token, newest first, so the owner can see what -// exists and pick one to revoke. Owner-only for the same reason minting is: the -// list is the inventory of who can write, and an agent has no business reading -// it. -func (s *Service) ListAgentTokens(ctx context.Context, p authn.Principal) ([]AgentToken, error) { - if !p.IsOwner() { - return nil, fmt.Errorf("%w: %s may not list agent tokens; only the instance owner may", ErrForbidden, p) - } - rows, err := s.store.ListAgentTokens(ctx) - if err != nil { - return nil, fmt.Errorf("service: list agent tokens: %w", err) - } - out := make([]AgentToken, 0, len(rows)) - for _, row := range rows { - out = append(out, tokenView(row)) - } - return out, nil -} - -// RevokeAgentToken stamps a token revoked. Owner-only. Revoking is a stamp -// rather than a delete so the audit trail keeps naming the token that made past -// proposals; re-revoking is a no-op, because an operator killing a credential -// twice is not an error worth failing. -func (s *Service) RevokeAgentToken(ctx context.Context, p authn.Principal, id int) error { - if !p.IsOwner() { - return fmt.Errorf("%w: %s may not revoke agent tokens; only the instance owner may", ErrForbidden, p) - } - if id <= 0 { - return fmt.Errorf("%w: %d is not an agent token id", ErrInvalid, id) - } - if err := s.store.RevokeAgentToken(ctx, id); err != nil { - if errors.Is(err, db.ErrNotFound) { - return fmt.Errorf("%w: agent token %d", ErrNotFound, id) - } - return fmt.Errorf("service: revoke agent token %d: %w", id, err) - } - return nil -} - -// validateTokenName normalizes and checks a token label. The rules are the -// weakest ones that keep the listing readable and unambiguous: trimmed, -// non-empty, valid UTF-8, no control characters, and bounded. A name grants -// nothing, so nothing stricter would be buying anything. -func validateTokenName(name string) (string, error) { - name = strings.TrimSpace(name) - if name == "" { - return "", fmt.Errorf("%w: a token needs a name saying which agent holds it", ErrInvalid) - } - if len(name) > MaxTokenNameLen { - return "", fmt.Errorf("%w: token name is %d bytes, over the %d-byte limit", - ErrInvalid, len(name), MaxTokenNameLen) - } - if !utf8.ValidString(name) { - return "", fmt.Errorf("%w: token name is not valid UTF-8", ErrInvalid) - } - for _, r := range name { - if r < 0x20 || r == 0x7f { - return "", fmt.Errorf("%w: token name contains a control character %U", ErrInvalid, r) - } - } - return name, nil -} - -// tokenView maps a stored token onto the surface shape, dropping the hash. -func tokenView(row *db.AgentToken) AgentToken { - return AgentToken{ID: row.ID, Name: row.Name, Created: row.Created, Revoked: row.Revoked} -} diff --git a/service/token_acl_test.go b/service/token_acl_test.go deleted file mode 100644 index 9adb339f0618763bd8347db4f4efb97acc85d0ea..0000000000000000000000000000000000000000 --- a/service/token_acl_test.go +++ /dev/null @@ -1,107 +0,0 @@ -package service - -import ( - "errors" - "strings" - "testing" - - "sourcecraft.dev/bigbes/sr-ht-spec/authn" -) - -// The three token calls are owner-only, and these tests run against a database -// that cannot be reached: a refusal that needs a query is a refusal that would -// have leaked the inventory, or minted the row, before deciding. - -func TestIssueAgentTokenIsOwnerOnly(t *testing.T) { - svc, _ := newService(t) - - for name, p := range map[string]authn.Principal{ - "an agent": {Kind: authn.KindAgent, Owner: "bigbes", Agent: "claude", Session: "s1"}, - "anonymous": authn.Anonymous(), - "a zero principal": {}, - } { - token, row, err := svc.IssueAgentToken(t.Context(), p, "another") - if !errors.Is(err, ErrForbidden) { - t.Errorf("%s was not refused with ErrForbidden: %v", name, err) - } - if token != "" || row.ID != 0 { - t.Errorf("%s got a token back: %q %+v", name, token, row) - } - } -} - -// TestIssueAgentTokenRefusesAnAgentBeforeMinting is the property revocation -// depends on: an agent that could mint would survive having its own credential -// revoked, so "revoke the token" would stop being incident response. -func TestIssueAgentTokenRefusesAnAgentBeforeMinting(t *testing.T) { - svc, _ := newService(t) - agent := authn.Principal{Kind: authn.KindAgent, Owner: "bigbes", Agent: "claude", Session: "s1"} - - _, _, err := svc.IssueAgentToken(t.Context(), agent, "self-issued") - if !errors.Is(err, ErrForbidden) { - t.Fatalf("an agent minting a token was not refused: %v", err) - } - if !strings.Contains(err.Error(), "only the instance owner") { - t.Errorf("the refusal does not say who may: %v", err) - } -} - -func TestListAndRevokeAreOwnerOnly(t *testing.T) { - svc, _ := newService(t) - agent := authn.Principal{Kind: authn.KindAgent, Owner: "bigbes", Agent: "claude", Session: "s1"} - - if _, err := svc.ListAgentTokens(t.Context(), agent); !errors.Is(err, ErrForbidden) { - t.Errorf("an agent listing tokens was not refused: %v", err) - } - if err := svc.RevokeAgentToken(t.Context(), agent, 1); !errors.Is(err, ErrForbidden) { - t.Errorf("an agent revoking a token was not refused: %v", err) - } -} - -// TestRevokeAgentTokenChecksTheIDBeforeTheDatabase keeps a mistyped id from -// becoming an UPDATE that matches nothing and reports "not found", which reads -// like the token is already gone. -func TestRevokeAgentTokenChecksTheIDBeforeTheDatabase(t *testing.T) { - svc, _ := newService(t) - owner := authn.Principal{Kind: authn.KindOwner, Owner: "bigbes"} - - for _, id := range []int{0, -1} { - if err := svc.RevokeAgentToken(t.Context(), owner, id); !errors.Is(err, ErrInvalid) { - t.Errorf("revoking id %d was not refused as invalid: %v", id, err) - } - } -} - -func TestValidateTokenName(t *testing.T) { - got, err := validateTokenName(" claude-code ") - if err != nil { - t.Fatalf("validateTokenName: %v", err) - } - if got != "claude-code" { - t.Errorf("name = %q want it trimmed", got) - } - - for name, in := range map[string]string{ - "empty": "", - "only whitespace": " ", - "a control character": "claude\x00code", - "a newline": "claude\ncode", - "longer than the caps": strings.Repeat("x", MaxTokenNameLen+1), - } { - if _, err := validateTokenName(in); !errors.Is(err, ErrInvalid) { - t.Errorf("%s was accepted as a token name", name) - } - } -} - -// TestIssueAgentTokenValidatesTheNameBeforeMinting keeps a rejected name from -// consuming entropy and, more importantly, from leaving a row whose label the -// owner cannot read in the listing. -func TestIssueAgentTokenValidatesTheNameBeforeMinting(t *testing.T) { - svc, _ := newService(t) - owner := authn.Principal{Kind: authn.KindOwner, Owner: "bigbes"} - - if _, _, err := svc.IssueAgentToken(t.Context(), owner, " "); !errors.Is(err, ErrInvalid) { - t.Errorf("a blank token name was accepted: %v", err) - } -} diff --git a/web/grant_test.go b/web/grant_test.go index f7e0b939aa5d80d59c8ecc207f45eb3945f8561d..4e050c5821abe634b7fc02aed9f021117f29b3e8 100644 --- a/web/grant_test.go +++ b/web/grant_test.go @@ -28,9 +28,6 @@ // 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{ @@ -39,13 +36,14 @@ "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"}, + "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"}, + "tokens.sr.ht": ini.Section{"origin": "https://tokens.example"}, }, Reader: newFakeReader(), Searcher: &fakeSearcher{}, - Resolver: resolver, + Resolver: testResolver(t), }) require.NoError(t, err) @@ -95,11 +93,12 @@ }) } }) - // The credentials that work today, and the instance token that was minted - // for reading: all served, no 403 anywhere. + // Everything that may read: the owner's cookie, an agent a local process + // asserted (no credential, so no grant to read), and the instance tokens + // 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"}, + "locally asserted agent": {Kind: authn.KindAgent, Owner: "bigbes"}, "instance token, spec:read": instancePrincipal(t, "spec:read"), "instance token, universal": instancePrincipal(t, "*"), } { diff --git a/web/reader.go b/web/reader.go index 05e665d1de293639056021500327e66805cf7a14..e5fee448b07d7302a8a4e341dc5ed74ca00b9414 100644 --- a/web/reader.go +++ b/web/reader.go @@ -83,16 +83,9 @@ CommentOn(ctx context.Context, req service.CommentRequest) (service.Thread, error) ReplyTo(ctx context.Context, p authn.Principal, threadID int, body string) (service.Comment, error) ResolveThread(ctx context.Context, p authn.Principal, threadID int, resolved bool) error - // ListTokens, IssueToken and RevokeToken are the agent-credential surface - // behind /tokens. Like the comment writes they carry the principal - // explicitly, because the authority is not the read ACL: all three are - // owner-only, and an agent — authenticated though it is — may not read the - // inventory of who can write, let alone mint itself a second credential. - // IssueToken returns the plaintext exactly once; it is not stored and no - // later call can produce it again. - ListTokens(ctx context.Context, p authn.Principal) ([]service.AgentToken, error) - IssueToken(ctx context.Context, p authn.Principal, name string) (string, service.AgentToken, error) - RevokeToken(ctx context.Context, p authn.Principal, id int) error + // There is no token surface here. /tokens used to mint, list and revoke + // spec's own agent credential; issuance is tokens.sr.ht's now, so the route + // is a redirect and this interface has nothing to serve it. // Inbox is every open proposal on the instance, newest first — the review // queue. Digest is the recently policy-merged proposals, the firehose a human @@ -190,18 +183,6 @@ } func (r serviceReader) ResolveThread(ctx context.Context, p authn.Principal, threadID int, resolved bool) error { return r.svc.ResolveThread(ctx, p, threadID, resolved) -} - -func (r serviceReader) ListTokens(ctx context.Context, p authn.Principal) ([]service.AgentToken, error) { - return r.svc.ListAgentTokens(ctx, p) -} - -func (r serviceReader) IssueToken(ctx context.Context, p authn.Principal, name string) (string, service.AgentToken, error) { - return r.svc.IssueAgentToken(ctx, p, name) -} - -func (r serviceReader) RevokeToken(ctx context.Context, p authn.Principal, id int) error { - return r.svc.RevokeAgentToken(ctx, p, id) } func (r serviceReader) Inbox(ctx context.Context) ([]service.Proposal, error) { diff --git a/web/router.go b/web/router.go index 71dba95e4b5e87629edfc7cc3bc50c7960a517c9..52a75196877858ddbeb8784bca86dcf0085c3f7b 100644 --- a/web/router.go +++ b/web/router.go @@ -43,11 +43,11 @@ r.Get("/search", s.handleSearch) r.Get("/inbox", s.handleInbox) r.Post("/inbox/seen", s.handleInboxSeen) - // The token routes are owner-only and live at the root, not under a space: - // there is one agent credential for the instance, not one per space. + // /tokens redirects to tokens.sr.ht, which issues every agent credential on + // the instance. The POST routes that minted and revoked here went with the + // table behind them; the GET stays so that a bookmark, the dashboard button + // and every doc that ever said "see /tokens" still land somewhere useful. r.Get("/tokens", s.handleTokens) - r.Post("/tokens", s.handleTokenIssue) - r.Post("/tokens/{id}/revoke", s.handleTokenRevoke) // The proposal routes are registered before the document wildcard. chi gives // the static "p" segment priority over the "*" catch-all regardless, but diff --git a/web/server.go b/web/server.go index 8accd2a76535a7fd45310b6c2a5d615b05ed049d..6ac46f2d5186d729de2c7fab592162c5ec7f05f9 100644 --- a/web/server.go +++ b/web/server.go @@ -24,10 +24,10 @@ // An absent ?rev= means the approved head — service.ApprovedRev — because // serving drafts by default would poison every downstream agent context with // unreviewed text. // -// Two pages sit outside the space grammar: /inbox is the review queue, and -// /tokens is the owner-only agent-credential page — mint (shown once), list, -// revoke. There is one credential for the instance rather than one per space, -// so it has no owner or space in its address. +// Two routes sit outside the space grammar: /inbox is the review queue, and +// /tokens redirects to tokens.sr.ht, which issues every agent credential on the +// instance. /tokens was spec's own mint-list-revoke page until that credential +// stopped being spec's to mint. // // # Who may read // @@ -74,6 +74,12 @@ // hashedCSSRe matches the content-addressed stylesheet name so it can be served // with an immutable cache lifetime (the hash changes whenever the bytes do). var hashedCSSRe = regexp.MustCompile(`^main\.min\.[0-9a-f]{6,}\.css$`) +// tokensSection is tokens.sr.ht's config section, spelled the way the instance's +// config.ini spells it. service.TokensSection is the same string read for the +// internal origin; this package cannot import service/ (the dependency arrow +// runs the other way), so the constant is here rather than shared. +const tokensSection = "tokens.sr.ht" + // Options is everything a Server needs. Every field is required; New says which // one is missing rather than failing later inside a handler. type Options struct { @@ -108,6 +114,13 @@ metaOrigin string origin string hubOrigin string cssHref string + + // tokensOrigin is [tokens.sr.ht] origin in its *external* form. The only + // thing this package does with it is redirect a browser there, and a browser + // cannot reach the internal origin the bearer validator uses. Empty when the + // instance config has no such section, which handleTokens answers rather + // than papers over with a redirect to nowhere. + tokensOrigin string nav []navItem staticFileServer http.Handler @@ -166,6 +179,7 @@ environment: config.GetString(opts.Conf, "sr.ht", "environment", "production"), metaOrigin: metaOrigin, origin: origin, hubOrigin: config.GetOrigin(opts.Conf, "hub.sr.ht", true), + tokensOrigin: config.GetOrigin(opts.Conf, tokensSection, true), cssHref: cssHref, nav: buildNav(opts.Conf), staticFileServer: http.StripPrefix("/static/", http.FileServer(http.FS(staticSub))), diff --git a/web/templates.go b/web/templates.go index c6ea2ade9bad58238a2b49266a737efa87a03f2a..c4649773b4cba38ff0bbc5321a019268db4c0ec7 100644 --- a/web/templates.go +++ b/web/templates.go @@ -46,7 +46,7 @@ "dec": func(n int) int { return n - 1 }, } // pageNames are the content templates; each is parsed with layout.html. -var pageNames = []string{"index", "space", "document", "search", "error", "proposal", "inbox", "tokens"} +var pageNames = []string{"index", "space", "document", "search", "error", "proposal", "inbox"} // pages maps a page name to its parsed template set (layout + partials + that // page). threads.html is parsed into every set rather than only into the diff --git a/web/templates/index.html b/web/templates/index.html index 6c71683f21f335d4e84fca12cd1f07a3ecd31432..982ff70cf5cc9b872c453693e34d05c7ed068cf6 100644 --- a/web/templates/index.html +++ b/web/templates/index.html @@ -8,7 +8,7 @@ the approved text. Reads default to the approved revision; add ?rev=<sha> to pin any page to an immutable one.

Review queue - Agent tokens + Agent tokens →
diff --git a/web/templates/tokens.html b/web/templates/tokens.html deleted file mode 100644 index 80aa4458115a7f471127d4decce0f70abf01c57d..0000000000000000000000000000000000000000 --- a/web/templates/tokens.html +++ /dev/null @@ -1,68 +0,0 @@ -{{define "content"}} -
-
-

Agent tokens

-

- The credential an agent presents as - Authorization: Bearer <token> to propose over the REST - and MCP write planes. Only the stored hash lives in the database, so a - token is shown exactly once — when it is minted. -

- - {{if .Data.Minted}} -
-

- {{.Data.MintedName}} minted. Copy it now — this is - the only time it is shown. -

-
{{.Data.Minted}}
-
- {{end}} - - -
- - -
- - - - {{if .Data.Tokens}} - - - - - - {{range .Data.Tokens}} - - - - - - - {{end}} - -
NameCreatedState
{{.Name}}{{.Created}} - {{if .Active}} - active - {{else}} - revoked - {{.Revoked}} - {{end}} - - {{if .Active}} -
- -
- {{end}} -
- {{else}} -

- No tokens yet. Without one the agent write plane refuses every caller as - anonymous, so nothing can propose. -

- {{end}} -
-
-{{end}} diff --git a/web/tokens.go b/web/tokens.go index 9d86f518fe47033633ada5d63a7b297c3c6a4fbc..e8b9f5ad78478d851c2c63d5e995bfda9651ac7a 100644 --- a/web/tokens.go +++ b/web/tokens.go @@ -2,146 +2,43 @@ package web import ( "net/http" - "strconv" - "time" - - "github.com/go-chi/chi/v5" - - "sourcecraft.dev/bigbes/sr-ht-spec/authn" - "sourcecraft.dev/bigbes/sr-ht-spec/service" ) -// tokensData is the agent-token page: the inventory, and — on the one response -// that follows a mint — the plaintext that will never be shown again. -// -// Minted is empty on every other render. It is a field on the page rather than -// a flash cookie or a redirect parameter on purpose: a secret in a URL lands in -// the browser history and in any proxy log between here and the operator, and a -// secret in a cookie is a secret stored twice. -type tokensData struct { - Tokens []tokenRow - Minted string - MintedName string -} - -// tokenRow is one credential as a listing line. There is no hash column: -// service.AgentToken carries no hash, which is the layer saying that nothing -// above it has any business with the stored value. -type tokenRow struct { - ID int - Name string - Created string - Revoked string // empty while the token is still active - Active bool -} +// tokensPath is the page at tokens.sr.ht this service points a human at. SPEC +// ch. 7 pins it: the daemon serves one page, `/tokens`, behind the unified-login +// cookie, and "services со временем просто ссылаются сюда" — which is what this +// handler is. +const tokensPath = "/tokens" -// handleTokens renders the token inventory. +// handleTokens sends a human to tokens.sr.ht. // -// Owner-only, and the refusal is a 403 rather than the read plane's login -// redirect: an agent reaching this page is authenticated already, so redirecting -// it to log in would answer a question it did not ask. An anonymous browser is -// sent to meta the usual way, because for a human the answer really is "log in". +// This route used to be spec's own agent-credential page: mint (shown once), +// list, revoke, all against the agent_token table. That table is gone and +// issuance is centralised, so what is left of the route is the one thing it can +// still honestly do — point at the place that issues the credential — and it is +// a redirect rather than a page of prose because an operator who typed /tokens +// wants the form, not an explanation of where the form moved to. +// +// No principal check. The old page was owner-only because it listed and minted +// credentials; a redirect exposes nothing but a public origin already in the +// nav, and tokens.sr.ht authenticates its own page against the same +// unified-login cookie this service reads. Sending an anonymous browser through +// meta's login first would only add a round trip to the same destination. +// +// A 303 rather than a 301: the destination of this route is an instance +// configuration value, and a permanent redirect is cached by browsers for far +// longer than a config key stays true. func (s *Server) handleTokens(w http.ResponseWriter, r *http.Request) { - p := authn.PrincipalFromContext(r.Context()) - if p.IsAnonymous() { - s.loginRedirect(w, r) - return - } - if !p.IsOwner() { - s.renderError(w, r, http.StatusForbidden, "only the instance owner may manage agent tokens") + if s.tokensOrigin == "" { + // An instance with no [tokens.sr.ht] origin has no page to send anybody + // to, and inventing one would land the operator on a dead host. It is + // also not a state this daemon can serve agents in — service.New refuses + // to build the agent plane without that origin — so the page says what is + // actually wrong. + s.renderError(w, r, http.StatusServiceUnavailable, + "agent credentials are issued by tokens.sr.ht, and this instance's config.ini "+ + "has no [tokens.sr.ht] origin") return } - s.renderTokens(w, r, p, tokensData{}) -} - -// handleTokenIssue mints a token and renders the page with the plaintext shown -// once. -// -// This is the one write in this package that does not end in a -// post-redirect-get. A redirect would either drop the secret — the whole point -// of the request — or carry it in a URL. So the POST renders, and the form's -// name field is what a reload would re-submit: minting a second token by -// accident is recoverable in one click on this very page, whereas a lost token -// is not recoverable at all. -func (s *Server) handleTokenIssue(w http.ResponseWriter, r *http.Request) { - p, ok := s.tokenWriter(w, r) - if !ok { - return - } - token, row, err := s.reader.IssueToken(r.Context(), p, r.FormValue("name")) - if err != nil { - s.fail(w, r, err) - return - } - s.renderTokens(w, r, p, tokensData{Minted: token, MintedName: row.Name}) -} - -// handleTokenRevoke stamps a token revoked and redirects back to the listing. -func (s *Server) handleTokenRevoke(w http.ResponseWriter, r *http.Request) { - p, ok := s.tokenWriter(w, r) - if !ok { - return - } - id, err := strconv.Atoi(chi.URLParam(r, "id")) - if err != nil || id <= 0 { - s.renderError(w, r, http.StatusNotFound, "no such agent token") - return - } - if err := s.reader.RevokeToken(r.Context(), p, id); err != nil { - s.fail(w, r, err) - return - } - http.Redirect(w, r, "/tokens", http.StatusSeeOther) -} - -// tokenWriter is the shared gate on both token writes: owner-only, and the same -// cross-site guard approve/reject use — the CSRF defense a form post needs when -// the session cookie is meta's and this service cannot set its SameSite. It -// answers the request itself when it refuses, so a caller only checks ok. -func (s *Server) tokenWriter(w http.ResponseWriter, r *http.Request) (authn.Principal, bool) { - p := authn.PrincipalFromContext(r.Context()) - if !p.IsOwner() { - s.renderError(w, r, http.StatusForbidden, "only the instance owner may manage agent tokens") - return authn.Principal{}, false - } - if !s.sameOrigin(r) { - s.renderError(w, r, http.StatusForbidden, "this request did not originate from this site") - return authn.Principal{}, false - } - return p, true -} - -// renderTokens reads the inventory and renders the page, carrying through -// whatever the caller already has to show (a freshly minted token, or nothing). -func (s *Server) renderTokens(w http.ResponseWriter, r *http.Request, p authn.Principal, data tokensData) { - tokens, err := s.reader.ListTokens(r.Context(), p) - if err != nil { - s.fail(w, r, err) - return - } - data.Tokens = tokenRows(tokens) - - vd := s.chrome(r) - vd.Title = "Agent tokens" - vd.Data = data - s.render(w, http.StatusOK, "tokens", vd) -} - -// tokenRows turns the service view onto listing lines, formatting the two -// timestamps here so the template holds no date logic. -func tokenRows(ts []service.AgentToken) []tokenRow { - rows := make([]tokenRow, 0, len(ts)) - for _, t := range ts { - row := tokenRow{ - ID: t.ID, - Name: t.Name, - Created: t.Created.UTC().Format(time.RFC3339), - Active: t.Active(), - } - if !row.Active { - row.Revoked = t.Revoked.UTC().Format(time.RFC3339) - } - rows = append(rows, row) - } - return rows + http.Redirect(w, r, s.tokensOrigin+tokensPath, http.StatusSeeOther) } diff --git a/web/tokens_test.go b/web/tokens_test.go index 5e27c3fe060ba24a757243781277487030012921..15f9ac975fc2f4d48147418a3400fcffbcb991db 100644 --- a/web/tokens_test.go +++ b/web/tokens_test.go @@ -3,162 +3,75 @@ import ( "net/http" "net/http/httptest" - "net/url" - "strings" "testing" - "time" - "sourcecraft.dev/bigbes/sr-ht-spec/service" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/vaughan0/go-ini" ) -// selfOrigin is this instance's origin as testServerWith configures it — the -// value the cross-site guard accepts. -const selfOrigin = "https://spec.example" - -func TestTokensPageListsWhatExists(t *testing.T) { - r := newFakeReader() - revoked := time.Date(2026, 8, 1, 10, 0, 0, 0, time.UTC) - r.tokens = []service.AgentToken{ - {ID: 2, Name: "claude-code", Created: time.Date(2026, 8, 2, 9, 0, 0, 0, time.UTC)}, - {ID: 1, Name: "old-runner", Created: time.Date(2026, 7, 1, 9, 0, 0, 0, time.UTC), Revoked: &revoked}, - } - h, _, _ := testServerWith(t, r) - - rec := get(t, h, "/tokens", "bigbes") - if rec.Code != http.StatusOK { - t.Fatalf("status = %d, want 200; body:\n%s", rec.Code, rec.Body) - } - body := rec.Body.String() - for _, want := range []string{"claude-code", "old-runner", "active", "revoked", "/tokens/2/revoke"} { - if !strings.Contains(body, want) { - t.Errorf("the page does not mention %q; body:\n%s", want, body) - } - } - // A revoked token has nothing left to revoke. - if strings.Contains(body, "/tokens/1/revoke") { - t.Errorf("the page offers to revoke an already-revoked token") - } -} +// /tokens is a signpost now. spec.sr.ht mints no credential of its own, so the +// page that used to mint, list and revoke one points at the daemon that does. +func TestTokensRedirectsToTokensSrHt(t *testing.T) { + h, _ := testServer(t) -func TestTokensPageEmpty(t *testing.T) { - h, _, _ := testServerWith(t, newFakeReader()) - rec := get(t, h, "/tokens", "bigbes") - if rec.Code != http.StatusOK { - t.Fatalf("status = %d, want 200", rec.Code) - } - if !strings.Contains(rec.Body.String(), "No tokens yet") { - t.Errorf("an empty inventory does not say so:\n%s", rec.Body) + for name, user := range map[string]string{ + "owner": "bigbes", + "anonymous": "", + } { + t.Run(name, func(t *testing.T) { + rec := get(t, h, "/tokens", user) + assert.Equal(t, http.StatusSeeOther, rec.Code, "body: %s", rec.Body) + // The external origin, because this is for a browser, and the page + // SPEC ch. 7 puts the token UI on. + assert.Equal(t, "https://tokens.example/tokens", rec.Header().Get("Location")) + }) } } -// TestTokensPageIsOwnerOnly is the whole point of the page's ACL: an agent is -// authenticated, and still may not see the inventory of who can write — nor be -// bounced to a login page it has no way to use. -func TestTokensPageIsOwnerOnly(t *testing.T) { - h, _, _ := testServerWith(t, newFakeReader()) +// The POST routes went with the table behind them: nothing here mints or +// revokes any more, and a form posted at the old address must not 404 into +// something that looks like it might have worked. +func TestTokensAcceptsNoWrites(t *testing.T) { + h, _ := testServer(t) - if rec := getAgent(t, h, "/tokens", agentTk); rec.Code != http.StatusForbidden { - t.Errorf("an agent got %d for /tokens, want 403", rec.Code) - } - rec := get(t, h, "/tokens", "") - if rec.Code != http.StatusSeeOther && rec.Code != http.StatusFound { - t.Errorf("an anonymous browser got %d, want a login redirect", rec.Code) + for _, target := range []string{"/tokens", "/tokens/1/revoke"} { + t.Run(target, func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, target, nil) + login(req, "bigbes") + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + assert.NotEqual(t, http.StatusOK, rec.Code) + assert.NotEqual(t, http.StatusSeeOther, rec.Code) + }) } } -// TestTokenMintShowsThePlaintextOnce proves the response to the mint carries the -// value the service returned. It is shown here or nowhere: nothing stores it. -func TestTokenMintShowsThePlaintextOnce(t *testing.T) { - r := newFakeReader() - h, _, _ := testServerWith(t, r) - - rec := postForm(t, h, "/tokens", "bigbes", selfOrigin, url.Values{"name": {"claude-code"}}) - if rec.Code != http.StatusOK { - t.Fatalf("status = %d, want 200; body:\n%s", rec.Code, rec.Body) - } - if len(r.issued) != 1 { - t.Fatalf("the service minted %d tokens, want 1", len(r.issued)) - } - body := rec.Body.String() - if !strings.Contains(body, r.issued[0]) { - t.Errorf("the minted token is not on the page; body:\n%s", body) - } - if !strings.Contains(body, "only time it is shown") { - t.Errorf("the page does not warn that the token is shown once") - } - - // A later view of the page must not carry it: it exists only in the - // response to the request that minted it. - if again := get(t, h, "/tokens", "bigbes"); strings.Contains(again.Body.String(), r.issued[0]) { - t.Errorf("a later page view still shows the plaintext:\n%s", again.Body) - } -} +// An instance with no [tokens.sr.ht] section has nowhere to send anybody, and +// says so instead of redirecting to a URL built out of an empty string. +func TestTokensWithoutTheSectionSaysSo(t *testing.T) { + srv, err := New(Options{ + Conf: ini.File{ + "sr.ht": ini.Section{ + "network-key": testConf.Section("sr.ht")["network-key"], + "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: testResolver(t), + }) + require.NoError(t, err) -func TestTokenMintRefusesANamelessToken(t *testing.T) { - r := newFakeReader() - h, _, _ := testServerWith(t, r) - - rec := postForm(t, h, "/tokens", "bigbes", selfOrigin, url.Values{"name": {" "}}) - if rec.Code == http.StatusOK { - t.Errorf("a blank name was accepted: %d", rec.Code) - } - if len(r.issued) != 0 { - t.Errorf("a token was minted for a blank name: %v", r.issued) - } -} - -func TestTokenWritesAreOwnerOnlyAndSameOrigin(t *testing.T) { - r := newFakeReader() - r.tokens = []service.AgentToken{{ID: 1, Name: "claude-code", Created: time.Now()}} - h, _, _ := testServerWith(t, r) - - // An agent may not mint, even with a valid bearer token. - req := httptest.NewRequest(http.MethodPost, "/tokens", strings.NewReader("name=self")) - req.Header.Set("Content-Type", "application/x-www-form-urlencoded") - req.Header.Set("Authorization", "Bearer "+agentTk) - req.Header.Set("Origin", selfOrigin) + req := httptest.NewRequest(http.MethodGet, "/tokens", nil) + login(req, "bigbes") rec := httptest.NewRecorder() - h.ServeHTTP(rec, req) - if rec.Code != http.StatusForbidden { - t.Errorf("an agent minting got %d, want 403", rec.Code) - } + srv.Handler().ServeHTTP(rec, req) - // The owner's own form post from somewhere else is a forgery. - if rec := postForm(t, h, "/tokens", "bigbes", "https://evil.example", - url.Values{"name": {"x"}}); rec.Code != http.StatusForbidden { - t.Errorf("a cross-origin mint got %d, want 403", rec.Code) - } - if rec := post(t, h, "/tokens/1/revoke", "bigbes", "https://evil.example"); rec.Code != http.StatusForbidden { - t.Errorf("a cross-origin revoke got %d, want 403", rec.Code) - } - if len(r.issued) != 0 { - t.Errorf("a refused request still minted: %v", r.issued) - } - if !r.tokens[0].Active() { - t.Errorf("a refused request still revoked the token") - } -} - -func TestTokenRevokeStampsAndRedirects(t *testing.T) { - r := newFakeReader() - r.tokens = []service.AgentToken{{ID: 4, Name: "claude-code", Created: time.Now()}} - h, _, _ := testServerWith(t, r) - - rec := post(t, h, "/tokens/4/revoke", "bigbes", selfOrigin) - if rec.Code != http.StatusSeeOther { - t.Fatalf("status = %d, want 303; body:\n%s", rec.Code, rec.Body) - } - if got := rec.Header().Get("Location"); got != "/tokens" { - t.Errorf("Location = %q want /tokens", got) - } - if r.tokens[0].Active() { - t.Errorf("the token is still active after a revoke") - } -} - -func TestTokenRevokeRejectsAMalformedID(t *testing.T) { - h, _, _ := testServerWith(t, newFakeReader()) - if rec := post(t, h, "/tokens/abc/revoke", "bigbes", selfOrigin); rec.Code != http.StatusNotFound { - t.Errorf("status = %d, want 404", rec.Code) - } + assert.Equal(t, http.StatusServiceUnavailable, rec.Code) + assert.Empty(t, rec.Header().Get("Location"), "there is no origin to redirect to") + assert.Contains(t, rec.Body.String(), "tokens.sr.ht") } diff --git a/web/web_test.go b/web/web_test.go index 42afeda1ceb254273f565aa6668241c59d11261e..f22a1357f84d4846b546271e7255b94ba6c41eaf 100644 --- a/web/web_test.go +++ b/web/web_test.go @@ -18,7 +18,9 @@ "time" "github.com/fernet/fernet-go" "github.com/vaughan0/go-ini" + "sourcecraft.dev/bigbes/sr-ht-core/auth" "sourcecraft.dev/bigbes/sr-ht-core/crypto" + "sourcecraft.dev/bigbes/sr-ht-ecore/bearer" "sourcecraft.dev/bigbes/sr-ht-spec/authn" "sourcecraft.dev/bigbes/sr-ht-spec/core" @@ -44,6 +46,9 @@ "sr.ht": ini.Section{"network-key": fk.Encode()}, "webhooks": ini.Section{"private-key": base64.StdEncoding.EncodeToString(seed)}, } crypto.InitCrypto(testConf) + // The agent credential is a signed tokens.sr.ht working token now, so it + // cannot be a constant: it is minted here, once the signing key exists. + agentTk = agentToken("spec:read") os.Exit(m.Run()) } @@ -52,8 +57,23 @@ const ( headRev = "1111111111111111111111111111111111111111" oldRev = "2222222222222222222222222222222222222222" - agentTk = "test-agent-token" ) + +// agentTk is a live working token for the instance owner, carrying spec:read — +// what an agent reading through the web UI presents. Set by TestMain. +var agentTk string + +// agentToken mints a signed working token the way tokens.sr.ht does. +func agentToken(grantString string) string { + bt := &auth.BearerToken{ + Version: auth.TokenVersion, + Expires: auth.ToTimestamp(time.Now().Add(time.Hour)), + Grants: grantString, + ClientID: bearer.TokensClientID, + Username: "bigbes", + } + return bt.Encode() +} var demoSpace = core.SpaceRef{Owner: "bigbes", Name: "rfcs"} @@ -130,15 +150,6 @@ // refuses — a page that draws a control the service would reject would pass // against a fake that accepts everything. threads map[int][]*service.Thread nextThread int - - // tokens backs the /tokens page, with ids handed out by nextToken and - // minted plaintexts recorded in issued so a test can assert the page showed - // the value the mint returned. Like the thread methods, the three token - // methods restate service/'s owner-only rule so the fake refuses what the - // real service refuses. - tokens []service.AgentToken - nextToken int - issued []string } func newFakeReader() *fakeReader { @@ -357,42 +368,6 @@ } return nil } -func (f *fakeReader) ListTokens(_ context.Context, p authn.Principal) ([]service.AgentToken, error) { - if !p.IsOwner() { - return nil, fmt.Errorf("%w: %s may not list agent tokens", service.ErrForbidden, p) - } - return append([]service.AgentToken(nil), f.tokens...), nil -} - -func (f *fakeReader) IssueToken(_ context.Context, p authn.Principal, name string) (string, service.AgentToken, error) { - if !p.IsOwner() { - return "", service.AgentToken{}, fmt.Errorf("%w: %s may not issue agent tokens", service.ErrForbidden, p) - } - if strings.TrimSpace(name) == "" { - return "", service.AgentToken{}, fmt.Errorf("%w: a token needs a name", service.ErrInvalid) - } - f.nextToken++ - tok := service.AgentToken{ID: f.nextToken, Name: strings.TrimSpace(name), Created: time.Now()} - f.tokens = append([]service.AgentToken{tok}, f.tokens...) - plaintext := fmt.Sprintf("plaintext-%d", tok.ID) - f.issued = append(f.issued, plaintext) - return plaintext, tok, nil -} - -func (f *fakeReader) RevokeToken(_ context.Context, p authn.Principal, id int) error { - if !p.IsOwner() { - return fmt.Errorf("%w: %s may not revoke agent tokens", service.ErrForbidden, p) - } - for i := range f.tokens { - if f.tokens[i].ID == id { - now := time.Now() - f.tokens[i].Revoked = &now - return nil - } - } - return fmt.Errorf("%w: agent token %d", service.ErrNotFound, id) -} - func (f *fakeReader) Inbox(_ context.Context) ([]service.Proposal, error) { var out []service.Proposal for _, p := range f.proposals { @@ -451,15 +426,31 @@ }}, }, nil } -// stubTokenStore knows exactly one live agent token. -type stubTokenStore struct{} +// stubUsers resolves the owner an instance token names to a local row. +type stubUsers struct{} -func (stubTokenStore) LookupAgentToken(_ context.Context, hash []byte) (authn.AgentToken, error) { - want := authn.HashToken(agentTk) - if string(hash) != string(want) { - return authn.AgentToken{}, authn.ErrUnknownToken +func (stubUsers) LookupUser(_ context.Context, username string) (authn.InstanceUser, error) { + return authn.InstanceUser{ID: 1, Username: username}, nil +} + +// testResolver is the resolver the daemon builds, with the one agent credential +// plane wired. Its origin is never reached: these tests present stateless +// tokens, which carry no row id and so skip the revocation round trip. +func testResolver(t *testing.T) *authn.Resolver { + t.Helper() + v, err := bearer.New(bearer.Options{ + Origin: "https://tokens.srht.invalid", + ClientID: "spec.sr.ht", + NodeID: "web-test", + }) + if err != nil { + t.Fatalf("bearer.New: %v", err) } - return authn.AgentToken{ID: 1, Name: "test", Hash: want}, nil + resolver, err := authn.NewResolver("bigbes", authn.WithInstancePlane(v, stubUsers{})) + if err != nil { + t.Fatalf("NewResolver: %v", err) + } + return resolver } // testServer wires a Server with a fresh fake reader/searcher behind the same @@ -490,11 +481,11 @@ "todo.sr.ht": ini.Section{"origin": "https://todo.example"}, "paste.sr.ht": ini.Section{"origin": "https://paste.example"}, "pages.sr.ht": ini.Section{"origin": "https://pages.example"}, "hub.sr.ht": ini.Section{"origin": "https://hub.example"}, - } - resolver, err := authn.NewResolver("bigbes", stubTokenStore{}) - if err != nil { - t.Fatalf("NewResolver: %v", err) + // /tokens redirects here, and the external origin is the one a browser + // can reach. + "tokens.sr.ht": ini.Section{"origin": "https://tokens.example"}, } + resolver := testResolver(t) searcher := &fakeSearcher{} srv, err := New(Options{ Conf: conf,