diff --git a/bearer/bearer.go b/bearer/bearer.go index fd811a2cd1ef34ab32b094a58c808cee349f2635..71f88b934b74126a84328d9146e86a211eaef41b 100644 --- a/bearer/bearer.go +++ b/bearer/bearer.go @@ -240,6 +240,21 @@ // difference between a credential its owner can revoke and one that can only be // waited out. func (t *Token) Registered() bool { return t.TokenID != 0 } +// Authorize is step 3 of SPEC ch. 6, split out so that it can be asked where the +// action is known — in the handler that implements it — rather than in the +// middleware that resolved the identity. See Inspect. +// +// It reports ErrForbidden and not ErrInvalid: the credential is good and the +// caller is who they say they are, and what is missing is a permission. That +// distinction is what stops an agent retrying forever with a token that will +// never grow the grant it needs. +func (t *Token) Authorize(action string) error { + if !t.Grants.Has(action) { + return fmt.Errorf("%w: %q is not in %q", ErrForbidden, action, t.Grants.String()) + } + return nil +} + // New builds a Validator, refusing options that would only fail later, one // request at a time, as an error about the network. func New(opts Options) (*Validator, error) { @@ -324,6 +339,70 @@ // } // // and one that does not turns ErrNotOurs into 401 alongside ErrInvalid. func (v *Validator) Validate(ctx context.Context, presented, action string) (*Token, error) { + tok, err := decodeOurs(presented) + if err != nil { + return tok, err + } + // Step 3 before step 4, deliberately: a token that does not carry the grant + // is refused without a round trip to the daemon. Inspect cannot keep this + // ordering — it has no action to check — which is the one cost of the split + // and the reason this method still exists for callers that know both. + if err := tok.Authorize(action); err != nil { + return nil, err + } + if err := v.revoked(ctx, tok); err != nil { + return nil, err + } + return tok, nil +} + +// Inspect is Validate without step 3: it answers who a token belongs to, what it +// may do, and whether it is still live — and leaves the question of whether that +// covers *this* action to the caller. +// +// It exists because of where the two questions get answered in a sourcehut +// service. Identity is resolved once per request in middleware, upstream of the +// router: that is where the cookie plane and the bearer plane meet and where a +// principal is put on the context, and at that point nothing knows yet which +// route will run, so nothing knows the action. The action is known one layer +// down, in the handler that implements it. A validator that insisted on both at +// once would force every service either to invent an action before it has one, +// or to lift its bearer plane out of the middleware every other plane goes +// through — and the second is how a surface ends up with two different ideas of +// who is calling. +// +// So: call Inspect in the resolver and carry the Grants on the principal, then +// call Token.Authorize in the handler. Validate stays for callers that know both +// at one point, and is exactly those two calls. +// +// Everything Validate's doc comment says — about the sentinels, about step 2 +// being per-service policy, and about the returned token being nil for every +// failure except ErrNotOurs — applies here unchanged. +func (v *Validator) Inspect(ctx context.Context, presented string) (*Token, error) { + tok, err := decodeOurs(presented) + if err != nil { + return tok, err + } + if err := v.revoked(ctx, tok); err != nil { + return nil, err + } + return tok, nil +} + +// revoked is step 4: a registered token has a revocation to ask about, a +// stateless one has no row and so completes without any network at all — which +// is the common case under the default configuration. +func (v *Validator) revoked(ctx context.Context, tok *Token) error { + if tok.TokenID == 0 { + return nil + } + return v.checkRevocation(ctx, tok.TokenID) +} + +// decodeOurs is steps 1 and 2 plus the grant parse: everything that can be +// decided from the token itself, with no clock but the real one and no network +// at all. +func decodeOurs(presented string) (*Token, error) { // Step 1. Signature, version and expiry, all of it local. // // DecodeBearerToken returns nil for all three and distinguishes none of @@ -360,20 +439,6 @@ TokenID: g.TokenID(), Expires: bt.Expires.Time(), } - // Step 3. Does it cover what is being attempted? - if !g.Has(action) { - return nil, fmt.Errorf("%w: %q is not in %q", ErrForbidden, action, g.String()) - } - - // Step 4. Only a registered token has a revocation to check. A stateless one - // has no row, so there is nothing to ask about and nothing to reach for: - // this is the common case and it completes without any network at all. - if tok.TokenID == 0 { - return tok, nil - } - if err := v.checkRevocation(ctx, tok.TokenID); err != nil { - return nil, err - } return tok, nil } diff --git a/bearer/bearer_test.go b/bearer/bearer_test.go index 9520a19e0999d0c0d3cb8039ae2da760e9d3faae..d4dc2f2b6760ce362642bc2c7bedd0ab28905186 100644 --- a/bearer/bearer_test.go +++ b/bearer/bearer_test.go @@ -308,6 +308,59 @@ assert.Zero(t, d.count(), "step 3 refuses locally; a token that cannot do the thing must not cost a round trip") } +// Inspect is what a resolver calls in middleware, where the action is not known +// yet — so it must answer for a token whose grants would not cover whatever runs +// next, and leave that refusal to the handler one layer down. +func TestInspectAnswersWithoutAnActionAndLeavesStepThreeToTheCaller(t *testing.T) { + d := newDaemon(t, http.StatusNoContent) + v := newValidator(t, d.server.URL, nil) + + tok, err := v.Inspect(context.Background(), ourToken("bench:read id:42")) + require.NoError(t, err, "a token is inspectable whatever the caller goes on to attempt") + assert.Equal(t, 42, tok.TokenID) + assert.True(t, tok.Grants.Has("bench:read")) + + // The revocation half is not the caller's to skip, so it was still asked. + // This is the one cost of the split: Inspect cannot keep Validate's + // refuse-before-the-network ordering, because it has no action to refuse on. + assert.Equal(t, 1, d.count()) + + // And step 3 is available where the action finally is known. + require.NoError(t, tok.Authorize("bench:read")) + err = tok.Authorize("bench:upload") + require.Error(t, err) + assert.True(t, errors.Is(err, ErrForbidden), "want ErrForbidden, got %v", err) +} + +// A stateless token has no row, so Inspect completes without touching the +// network at all — the common case under the default configuration. +func TestInspectOfAStatelessTokenTouchesNoNetwork(t *testing.T) { + d := newDaemon(t, http.StatusNoContent) + v := newValidator(t, d.server.URL, nil) + + tok, err := v.Inspect(context.Background(), ourToken("bench:upload")) + require.NoError(t, err) + assert.Zero(t, tok.TokenID) + assert.False(t, tok.Registered()) + assert.Zero(t, d.count()) +} + +// A foreign token is not ours whether it is asked about with an action or +// without one, and both ways hand the decoded token back so the service can +// apply its own meta-PAT policy. +func TestInspectReportsAForeignTokenTheSameWayValidateDoes(t *testing.T) { + v := newValidator(t, "https://tokens.srht.bigb.es", nil) + // A meta.sr.ht PAT: same signing key, foreign ClientID, and a grant string in + // core-go's OAuth grammar that our parser would reject — which is exactly why + // step 2 has to come before the parse. + pat := seal("bigbes", "meta.sr.ht", "git.sr.ht/OBJECTS:RW", time.Now().Add(time.Hour)) + + tok, err := v.Inspect(context.Background(), pat) + require.Error(t, err) + assert.True(t, errors.Is(err, ErrNotOurs), "want ErrNotOurs, got %v", err) + require.NotNil(t, tok, "step 2 hands the token back; that is the whole point of it") +} + func TestUniversalGrantAdmitsTheAction(t *testing.T) { v := newValidator(t, "https://tokens.srht.bigb.es", nil)