diff --git a/GETTING_STARTED.md b/GETTING_STARTED.md index 18cd89e6..66815db2 100644 --- a/GETTING_STARTED.md +++ b/GETTING_STARTED.md @@ -899,7 +899,11 @@ request. **Closing an account.** `srv.RevokeSubjectGrants(ctx, subject)` revokes every grant the registry lists for the user, and, with a CIBA inbox, `srv.DenyBackchannelInteractions(ctx, subject, reason)` denies the -requests still awaiting them. If you map pairwise subs +requests still awaiting them. Neither sees a grant still in flight, an +authorization code not yet exchanged or a CIBA approval not yet polled: +refuse the subject at sign-in from then on, and call +`RevokeSubjectGrants` again once `Limits.AuthorizationCodeLifetime` and +`Limits.BackchannelAuthenticationRequestLifetime` have passed. If you map pairwise subs (`pairwise.NewMapping`), also call the store's `ForgetSubject(ctx, issuer, subject)` (`storage.PairwiseSubjectEraser`, which `memstore` and `sqlstore` implement): the subs relying parties hold for the user stop resolving, diff --git a/server/backchannel_authentication.go b/server/backchannel_authentication.go index 786c8325..f6d3fef6 100644 --- a/server/backchannel_authentication.go +++ b/server/backchannel_authentication.go @@ -768,27 +768,44 @@ func (s *Server) backchannelInteraction(client storage.RegisteredClient, params // decided or has expired (BackchannelAuthenticationStore's lookup doesn't // say); CompleteBackchannelAuthentication is what enforces both, so a // stale lookup can at most show a request that can no longer be -// completed. An unknown handle is an ErrorInvalidRequest *Error. +// completed. An unknown handle is an ErrorInvalidRequest *Error, and a +// request whose client is no longer registered ErrorUnauthorizedClient; +// a store or client repository that couldn't answer is +// ErrorServerError. func (s *Server) LookupBackchannelInteraction(ctx context.Context, handle BackchannelAuthenticationHandle) (BackchannelInteractionRequest, error) { if s.deps.Backchannel == nil { return BackchannelInteractionRequest{}, newError(ErrorServerError, 500, "backchannel authentication is not configured", nil) } + interaction, _, err := s.lookupBackchannelInteraction(ctx, handle) + return interaction, err +} + +// lookupBackchannelInteraction is LookupBackchannelInteraction, also +// returning the subject Dependencies.BackchannelSubjects bound the +// request to ("" without one). +func (s *Server) lookupBackchannelInteraction(ctx context.Context, handle BackchannelAuthenticationHandle) (BackchannelInteractionRequest, string, error) { pending, err := s.deps.Backchannel.LookupBackchannelAuthentication(ctx, backchannelHandleHash(handle)) if err != nil { - return BackchannelInteractionRequest{}, newError(ErrorInvalidRequest, 400, "backchannel authentication handle is unknown", err) + if clientStoreUnavailable(ctx, err) { + return BackchannelInteractionRequest{}, "", newError(ErrorServerError, 500, "failed to look up the backchannel authentication request", err) + } + return BackchannelInteractionRequest{}, "", newError(ErrorInvalidRequest, 400, "backchannel authentication handle is unknown", err) } request, err := s.decodeOwnRequestRecord(pending.Request) if err != nil { - return BackchannelInteractionRequest{}, recordFailure(err, newError(ErrorInvalidRequest, 400, "backchannel authentication handle is unknown", nil), "backchannel authentication request") + return BackchannelInteractionRequest{}, "", recordFailure(err, newError(ErrorInvalidRequest, 400, "backchannel authentication handle is unknown", nil), "backchannel authentication request") } client, err := s.deps.Clients.ResolveClient(ctx, pending.ClientID) if err != nil { - return BackchannelInteractionRequest{}, newError(ErrorServerError, 500, "the request's client could not be resolved", err) + if clientStoreUnavailable(ctx, err) { + return BackchannelInteractionRequest{}, "", newError(ErrorServerError, 500, "the request's client could not be resolved", err) + } + return BackchannelInteractionRequest{}, "", newError(ErrorUnauthorizedClient, 400, "client is no longer registered", err) } interaction, err := s.backchannelInteraction(client, request.Parameters, request.Resources) if err != nil { - return BackchannelInteractionRequest{}, newError(ErrorServerError, 500, "the request's extension parameters could not be read back", err) + return BackchannelInteractionRequest{}, "", newError(ErrorServerError, 500, "the request's extension parameters could not be read back", err) } interaction.RequiredSubject = request.requiredSubjectForApplication() - return interaction, nil + return interaction, request.ResolvedSubject, nil } diff --git a/server/backchannel_inbox.go b/server/backchannel_inbox.go index e6ab38e7..5fd8ca87 100644 --- a/server/backchannel_inbox.go +++ b/server/backchannel_inbox.go @@ -147,38 +147,57 @@ func (s *Server) fileBackchannelRequest(ctx context.Context, clientID fapi.Clien // application must make sure the user is subject, as it does before // approving. func (s *Server) ListBackchannelInteractions(ctx context.Context, subject SubjectID) ([]PendingBackchannelInteraction, error) { + pending, unreadable, err := s.listBackchannelInteractions(ctx, subject) + if err != nil { + return nil, err + } + if unreadable != nil { + return nil, unreadable + } + return pending, nil +} + +// listBackchannelInteractions returns the requests awaiting subject, +// and, separately, the entries it couldn't read (an undecodable record, +// or a request the backchannel store couldn't answer for) joined into +// unreadable, so one bad entry doesn't hide the others from +// DenyBackchannelInteractions. err is for a listing that failed as a +// whole. +func (s *Server) listBackchannelInteractions(ctx context.Context, subject SubjectID) (pending []PendingBackchannelInteraction, unreadable, err error) { if s.deps.BackchannelInbox == nil { - return nil, errors.New("server: list backchannel interactions: Dependencies.BackchannelInbox isn't set") + return nil, nil, errors.New("server: list backchannel interactions: Dependencies.BackchannelInbox isn't set") } if subject.String() == "" { - return nil, errors.New("server: list backchannel interactions: subject is empty") + return nil, nil, errors.New("server: list backchannel interactions: subject is empty") } entries, err := s.deps.BackchannelInbox.ListBackchannelRequests(ctx, storage.BackchannelInboxListing{ SubjectKey: s.backchannelInboxSubjectKey(subject.String()), Now: s.deps.Clock.Now(), }) if err != nil { - return nil, fmt.Errorf("server: list backchannel interactions: %w", err) + return nil, nil, fmt.Errorf("server: list backchannel interactions: %w", err) } - var out []PendingBackchannelInteraction + var bad []error for _, entry := range entries { - pending, ok, err := s.pendingBackchannelInteraction(ctx, subject, entry) + p, ok, err := s.pendingBackchannelInteraction(ctx, subject, entry) if err != nil { - return nil, fmt.Errorf("server: list backchannel interactions: %w", err) + bad = append(bad, fmt.Errorf("server: list backchannel interactions: %w", err)) + continue } if ok { - out = append(out, pending) + pending = append(pending, p) } } - slices.SortFunc(out, func(a, b PendingBackchannelInteraction) int { + slices.SortFunc(pending, func(a, b PendingBackchannelInteraction) int { return cmp.Or(a.ExpiresAt.Compare(b.ExpiresAt), cmp.Compare(a.ID, b.ID)) }) - return out, nil + return pending, errors.Join(bad...), nil } // pendingBackchannelInteraction rebuilds entry's request, reporting // false for one that isn't a live request of subject's at this server: -// written by another issuer, expired, or gone from the backchannel -// store. +// written by another issuer, expired, gone from the backchannel store, +// from a client no longer registered, or bound to another subject than +// the one it's filed under. func (s *Server) pendingBackchannelInteraction(ctx context.Context, subject SubjectID, entry storage.BackchannelInboxEntry) (PendingBackchannelInteraction, bool, error) { var record backchannelInboxRecord if err := decodeRecord(entry.Record, &record.Version, &record); err != nil { @@ -191,14 +210,19 @@ func (s *Server) pendingBackchannelInteraction(ctx context.Context, subject Subj if err != nil || sha256.Sum256([]byte(record.Handle)) != entry.HandleHash { return PendingBackchannelInteraction{}, false, nil } - interaction, err := s.LookupBackchannelInteraction(ctx, handle) + interaction, resolved, err := s.lookupBackchannelInteraction(ctx, handle) if err != nil { var serverErr *Error - if errors.As(err, &serverErr) && serverErr.Code() == ErrorInvalidRequest { + if errors.As(err, &serverErr) && (serverErr.Code() == ErrorInvalidRequest || serverErr.Code() == ErrorUnauthorizedClient) { return PendingBackchannelInteraction{}, false, nil } return PendingBackchannelInteraction{}, false, err } + // The request itself names who may approve it: an entry filed under + // someone else, as one a database writer moved could be, isn't theirs. + if resolved != subject.String() { + return PendingBackchannelInteraction{}, false, nil + } return PendingBackchannelInteraction{ID: backchannelInboxID(entry.HandleHash), Handle: handle, Interaction: interaction, ExpiresAt: entry.ExpiresAt}, true, nil } @@ -224,19 +248,21 @@ func (s *Server) unfileBackchannelRequest(ctx context.Context, handleHash [32]by // call or instance, or expired — or not recorded because the store // failed, which CompleteBackchannelAuthentication reports the same way, // so check ListBackchannelInteractions when every request must be gone. -// It stops at any other failure, returning how many it had denied and -// the error; the rest are still listed, so calling it again finishes -// the job. +// Any other failure, or an entry it can't read, doesn't stop it: it +// denies every other request, then returns how many it denied with every +// failure joined. What failed is still listed, so calling it again +// finishes the job. // // DenyBackchannelInteractions doesn't check who is asking: the // application must make sure the request is subject's own, or an // administrator's. func (s *Server) DenyBackchannelInteractions(ctx context.Context, subject SubjectID, reason string) (int, error) { - pending, err := s.ListBackchannelInteractions(ctx, subject) + pending, unreadable, err := s.listBackchannelInteractions(ctx, subject) if err != nil { return 0, fmt.Errorf("server: deny backchannel interactions: %w", err) } denied := 0 + failures := []error{unreadable} for _, p := range pending { err := s.CompleteBackchannelAuthentication(ctx, CompleteBackchannelAuthenticationRequest{Handle: p.Handle, Result: Deny(reason)}) var serverErr *Error @@ -244,10 +270,14 @@ func (s *Server) DenyBackchannelInteractions(ctx context.Context, subject Subjec case err == nil: denied++ case errors.As(err, &serverErr) && (serverErr.Code() == ErrorInvalidRequest || serverErr.Code() == ErrorExpiredToken): - // Decided or expired since it was listed. + // Decided or expired since it was listed: its entry is stale. + s.unfileBackchannelRequest(ctx, backchannelHandleHash(p.Handle)) default: - return denied, fmt.Errorf("server: deny backchannel interactions: %w", err) + failures = append(failures, err) } } + if err := errors.Join(failures...); err != nil { + return denied, fmt.Errorf("server: deny backchannel interactions: %w", err) + } return denied, nil } diff --git a/server/backchannel_inbox_test.go b/server/backchannel_inbox_test.go index e844c614..f6b14537 100644 --- a/server/backchannel_inbox_test.go +++ b/server/backchannel_inbox_test.go @@ -455,3 +455,97 @@ func TestBackchannelSubjectsMustAgreeWithTheIDTokenHint(t *testing.T) { t.Fatalf("begin with a resolver disagreeing with the id_token_hint = %+v, want server_error", action) } } + +// TestBackchannelInboxSkipsADeregisteredClient: a request from a client +// no longer registered leaves the listing, rather than failing it for +// every request the user has. +func TestBackchannelInboxSkipsADeregisteredClient(t *testing.T) { + h := inboxHarness(t, memstore.NewBackchannelInbox()) + beginFiled(t, h, "user-1") + delete(h.clients.clients, testClientID) + if pending := inboxOf(t, h, "user-1"); len(pending) != 0 { + t.Errorf("inbox with the client deregistered = %d requests, want none", len(pending)) + } + if n, err := h.server.DenyBackchannelInteractions(context.Background(), mustSubjectID(t, "user-1"), "closed"); err != nil || n != 0 { + t.Errorf("DenyBackchannelInteractions = %d, %v, want 0, nil", n, err) + } +} + +// outageBackchannelStore fails lookups as an outage while down is set. +type outageBackchannelStore struct { + storage.BackchannelAuthenticationStore + down *bool +} + +func (s outageBackchannelStore) LookupBackchannelAuthentication(ctx context.Context, handleHash [32]byte) (storage.LookedUpBackchannelAuthentication, error) { + if *s.down { + return storage.LookedUpBackchannelAuthentication{}, fmt.Errorf("db: %w", storage.ErrStoreUnavailable) + } + return s.BackchannelAuthenticationStore.LookupBackchannelAuthentication(ctx, handleHash) +} + +// TestBackchannelInboxReportsAStoreOutage: a backchannel store that +// can't answer fails the listing and the bulk denial, rather than +// showing an empty inbox an account closure would take as done. +func TestBackchannelInboxReportsAStoreOutage(t *testing.T) { + down := false + h := newHarnessWithBackchannelOptions(t, outageBackchannelStore{memstore.NewBackchannelAuthenticationStore(), &down}, nil, func(d *server.Dependencies) { + d.BackchannelSubjects = hintSubjects{known: map[string]bool{"user-1": true}} + d.BackchannelInbox = memstore.NewBackchannelInbox() + }) + beginFiled(t, h, "user-1") + down = true + if _, err := h.server.ListBackchannelInteractions(context.Background(), mustSubjectID(t, "user-1")); err == nil { + t.Error("ListBackchannelInteractions with the store down = nil error, want the outage") + } + if n, err := h.server.DenyBackchannelInteractions(context.Background(), mustSubjectID(t, "user-1"), "closed"); err == nil || n != 0 { + t.Errorf("DenyBackchannelInteractions with the store down = %d, %v, want 0 and the outage", n, err) + } +} + +// flakyRemoveInbox fails its first RemoveBackchannelRequest. +type flakyRemoveInbox struct { + *memstore.BackchannelInbox + failed *bool +} + +func (b flakyRemoveInbox) RemoveBackchannelRequest(ctx context.Context, handleHash [32]byte) error { + if !*b.failed { + *b.failed = true + return errors.New("inbox down") + } + return b.BackchannelInbox.RemoveBackchannelRequest(ctx, handleHash) +} + +// TestDenyBackchannelInteractionsClearsStaleEntries: an entry left +// listed after its request was decided (its removal failed) is removed +// when a bulk denial finds the request already decided. +func TestDenyBackchannelInteractionsClearsStaleEntries(t *testing.T) { + failed := false + h := inboxHarness(t, flakyRemoveInbox{memstore.NewBackchannelInbox(), &failed}) + required := beginFiled(t, h, "user-1") + approveAs(t, h, required.Handle, required.Interaction.Scope, "user-1") + if len(inboxOf(t, h, "user-1")) != 1 { + t.Fatal("the decided request isn't still listed, so the removal failure wasn't exercised") + } + if n, err := h.server.DenyBackchannelInteractions(context.Background(), mustSubjectID(t, "user-1"), "closed"); err != nil || n != 0 { + t.Fatalf("DenyBackchannelInteractions = %d, %v, want 0, nil", n, err) + } + if left := inboxOf(t, h, "user-1"); len(left) != 0 { + t.Errorf("inbox after the denial = %d requests, want the stale entry cleared", len(left)) + } +} + +// TestBackchannelInboxSkipsAnEntryFiledUnderSomeoneElse: an entry whose +// request is bound to another user than the one it's filed under, as an +// entry a database writer moved would be, isn't listed for them. +func TestBackchannelInboxSkipsAnEntryFiledUnderSomeoneElse(t *testing.T) { + inbox := &stubInbox{BackchannelInbox: memstore.NewBackchannelInbox()} + h := inboxHarness(t, inbox) + required := beginFiled(t, h, "user-1") + handle := required.Handle.String() + inbox.extra = []storage.BackchannelInboxEntry{inboxEntry(t, h, testIssuer, "user-2", handle, sha256.Sum256([]byte(handle)))} + if pending := inboxOf(t, h, "user-2"); len(pending) != 0 { + t.Errorf("inbox of user-2 = %d requests, want user-1's request left out", len(pending)) + } +} diff --git a/server/dependencies.go b/server/dependencies.go index 0bca5b7d..4e68208b 100644 --- a/server/dependencies.go +++ b/server/dependencies.go @@ -192,9 +192,11 @@ type Dependencies struct { // shows, from any instance. It needs BackchannelSubjects. A request // is filed before it's stored, so a filing failure fails the // request. Its entries hold the requests' backchannel authentication - // handles, so give it storage that encrypts at rest; the subject + // handles, so give it storage that encrypts at rest. The subject // binding means a handle read from it can still only be approved by - // the user it was filed for. Under AssuranceProduction it must + // the user it was filed for; it can still end the request, though, as + // any handle can: CompleteBackchannelAuthentication records a Deny, + // or an approval by someone else, for whoever presents the handle. Under AssuranceProduction it must // declare storage.StoreAssurance (Durable, and // CrossInstanceConsistent with DeploymentHorizontallyScaled). // Ignored unless Config.Endpoints.BackchannelAuthentication is set. diff --git a/server/export_test.go b/server/export_test.go new file mode 100644 index 00000000..847ca118 --- /dev/null +++ b/server/export_test.go @@ -0,0 +1,5 @@ +package server + +// GrantEntryKey exposes grantEntryKey to the server_test package, so a +// test can write a GrantRegistry entry as the server would. +func GrantEntryKey(refreshTokenHash [32]byte) string { return grantEntryKey(refreshTokenHash) } diff --git a/server/grant_registry.go b/server/grant_registry.go index db80584e..501b7486 100644 --- a/server/grant_registry.go +++ b/server/grant_registry.go @@ -168,10 +168,13 @@ type listedGrant struct { // ListGrants doesn't check who is asking: the application must make sure // the user is subject. func (s *Server) ListGrants(ctx context.Context, subject SubjectID) ([]ListedGrant, error) { - grants, err := s.listGrants(ctx, subject) + grants, unreadable, err := s.listGrants(ctx, subject) if err != nil { return nil, err } + if unreadable != nil { + return nil, unreadable + } out := make([]ListedGrant, len(grants)) for i, g := range grants { out[i] = g.listed @@ -179,31 +182,37 @@ func (s *Server) ListGrants(ctx context.Context, subject SubjectID) ([]ListedGra return out, nil } -func (s *Server) listGrants(ctx context.Context, subject SubjectID) ([]listedGrant, error) { +// listGrants returns subject's live grants, and, separately, the +// entries it couldn't read (an undecodable record, or a revocation check +// that failed) joined into unreadable: one bad entry doesn't hide the +// others from a caller revoking them. err is for a listing that failed +// as a whole. +func (s *Server) listGrants(ctx context.Context, subject SubjectID) (grants []listedGrant, unreadable, err error) { if s.deps.GrantRegistry == nil { - return nil, errors.New("server: list grants: Dependencies.GrantRegistry isn't set") + return nil, nil, errors.New("server: list grants: Dependencies.GrantRegistry isn't set") } if subject.String() == "" { - return nil, errors.New("server: list grants: subject is empty") + return nil, nil, errors.New("server: list grants: subject is empty") } entries, err := s.deps.GrantRegistry.ListGrants(ctx, storage.GrantListing{SubjectKey: s.grantSubjectKey(subject.String()), Now: s.deps.Clock.Now()}) if err != nil { - return nil, fmt.Errorf("server: list grants: %w", err) + return nil, nil, fmt.Errorf("server: list grants: %w", err) } - var out []listedGrant + var bad []error for _, entry := range entries { g, ok, err := s.listedGrantFrom(ctx, subject, entry) if err != nil { - return nil, fmt.Errorf("server: list grants: %w", err) + bad = append(bad, fmt.Errorf("server: list grants: entry %s: %w", entry.Key, err)) + continue } if ok { - out = append(out, g) + grants = append(grants, g) } } - slices.SortFunc(out, func(a, b listedGrant) int { + slices.SortFunc(grants, func(a, b listedGrant) int { return cmp.Or(a.listed.IssuedAt.Compare(b.listed.IssuedAt), cmp.Compare(a.listed.ID, b.listed.ID)) }) - return out, nil + return grants, errors.Join(bad...), nil } // listedGrantFrom decodes entry, reporting false for one that isn't a @@ -217,6 +226,13 @@ func (s *Server) listedGrantFrom(ctx context.Context, subject SubjectID, entry s if !s.ownsRecord(record.Issuer) || record.Subject != subject.String() || !s.deps.Clock.Now().Before(entry.ExpiresAt) { return listedGrant{}, false, nil } + // The entry's key is derived from the refresh token it revokes: an + // entry naming another token's hash than its own key's, as an + // unsealed record a database writer forged could, isn't the grant + // it's listed as. + if hash, ok := refreshTokenHashOf(record.RefreshTokenHash); !ok || grantEntryKey(hash) != entry.Key { + return listedGrant{}, false, nil + } if revokedErr := s.checkGrantNotRevoked(ctx, grantRecord{GrantID: record.GrantID, CodeGrantID: record.CodeGrantID}); revokedErr != nil { if revokedErr.Code() == ErrorInvalidGrant { return listedGrant{}, false, nil @@ -252,12 +268,16 @@ func (s *Server) listedGrantFrom(ctx context.Context, subject SubjectID, entry s // grant of the subject the application has authenticated: the // application must still make sure the user is subject. func (s *Server) RevokeListedGrant(ctx context.Context, subject SubjectID, id string) error { - grants, err := s.listGrants(ctx, subject) + grants, unreadable, err := s.listGrants(ctx, subject) if err != nil { return fmt.Errorf("server: revoke listed grant: %w", err) } i := slices.IndexFunc(grants, func(g listedGrant) bool { return g.listed.ID == id }) if i < 0 { + if unreadable != nil { + // It may be one of the entries that couldn't be read. + return fmt.Errorf("server: revoke listed grant: %w", unreadable) + } return ErrGrantNotListed } return s.revokeListed(ctx, grants[i]) @@ -310,9 +330,19 @@ func (s *Server) forgetRevokedRefreshToken(ctx context.Context, refreshTokenHash // RevokeSubjectGrants revokes every grant ListGrants lists for subject, // as RevokeListedGrant revokes one, and returns how many it revoked: for // ending a user's access when their account closes, or after it's -// compromised. It stops at the first failure, returning how many it had -// revoked and the error; the rest are still listed, so calling it again -// finishes the job. +// compromised. A grant it fails to revoke, or an entry it can't read, +// doesn't stop it: it revokes every other grant, then returns how many +// it revoked with every failure joined. What failed is still listed, so +// calling it again finishes the job. +// +// A grant whose refresh token isn't issued yet isn't listed: an +// authorization code not yet exchanged, or an approved CIBA request not +// yet polled, can still be redeemed for one within +// Limits.AuthorizationCodeLifetime or +// Limits.BackchannelAuthenticationRequestLifetime. To be sure nothing +// outlives an account, refuse its subject at authentication from then +// on, and call RevokeSubjectGrants again once the longer of those has +// passed. // // Closing an account also means denying the CIBA requests awaiting the // user, with a BackchannelInbox (DenyBackchannelInteractions), and @@ -326,14 +356,21 @@ func (s *Server) forgetRevokedRefreshToken(ctx context.Context, refreshTokenHash // RevokeSubjectGrants doesn't check who is asking: the application must // make sure the request is subject's own, or an administrator's. func (s *Server) RevokeSubjectGrants(ctx context.Context, subject SubjectID) (int, error) { - grants, err := s.listGrants(ctx, subject) + grants, unreadable, err := s.listGrants(ctx, subject) if err != nil { return 0, fmt.Errorf("server: revoke subject grants: %w", err) } - for i, g := range grants { + revoked := 0 + failures := []error{unreadable} + for _, g := range grants { if err := s.revokeListed(ctx, g); err != nil { - return i, err + failures = append(failures, err) + continue } + revoked++ + } + if err := errors.Join(failures...); err != nil { + return revoked, fmt.Errorf("server: revoke subject grants: %w", err) } - return len(grants), nil + return revoked, nil } diff --git a/server/grant_registry_test.go b/server/grant_registry_test.go index 3c171ce7..e634cf3c 100644 --- a/server/grant_registry_test.go +++ b/server/grant_registry_test.go @@ -2,8 +2,10 @@ package server_test import ( "context" + "crypto/sha256" "encoding/json" "errors" + "slices" "strings" "testing" "time" @@ -227,9 +229,10 @@ func (r *stubRegistry) DeleteGrant(_ context.Context, key string) error { // registryEntry is a GrantEntry holding record fields as the server // writes them, for subject, issued at issuedAt. -func registryEntry(t *testing.T, h harness, key, issuer, subject string, issuedAt time.Time, extra map[string]any) storage.GrantEntry { +func registryEntry(t *testing.T, h harness, name, issuer, subject string, issuedAt time.Time, extra map[string]any) storage.GrantEntry { t.Helper() - record := map[string]any{"v": 1, "iss": issuer, "rth": make([]byte, 32), "sub": subject, "issued_at": issuedAt} + hash := sha256.Sum256([]byte(name)) + record := map[string]any{"v": 1, "iss": issuer, "rth": hash[:], "sub": subject, "issued_at": issuedAt} for k, v := range extra { record[k] = v } @@ -237,7 +240,13 @@ func registryEntry(t *testing.T, h harness, key, issuer, subject string, issuedA if err != nil { t.Fatal(err) } - return storage.GrantEntry{Key: key, ClientID: testClientID, Record: raw, ExpiresAt: h.now.Add(time.Hour)} + return storage.GrantEntry{Key: registryKey(name), ClientID: testClientID, Record: raw, ExpiresAt: h.now.Add(time.Hour)} +} + +// registryKey is the entry key registryEntry gives name: derived from +// its refresh token hash, as the server derives one. +func registryKey(name string) string { + return server.GrantEntryKey(sha256.Sum256([]byte(name))) } func stubRegistryHarness(t *testing.T, registry *stubRegistry) harness { @@ -264,7 +273,7 @@ func TestListGrantsReadsEntries(t *testing.T) { expired, } grants := listGrants(t, h, "user-1") - if len(grants) != 2 || grants[0].ID != "older" || grants[1].ID != "newer" { + if len(grants) != 2 || grants[0].ID != registryKey("older") || grants[1].ID != registryKey("newer") { t.Fatalf("ListGrants = %+v, want older then newer", grants) } if len(grants[0].AuthorizationDetails) != 1 || !strings.Contains(string(grants[0].AuthorizationDetails[0]), `"payment"`) { @@ -289,7 +298,7 @@ func TestListGrantsRefusesUnreadableEntries(t *testing.T) { if _, err := h.server.ListGrants(context.Background(), mustSubjectID(t, "user-1")); err == nil { t.Fatal("ListGrants = nil error, want error") } - if err := h.server.RevokeListedGrant(context.Background(), mustSubjectID(t, "user-1"), "k"); err == nil || errors.Is(err, server.ErrGrantNotListed) { + if err := h.server.RevokeListedGrant(context.Background(), mustSubjectID(t, "user-1"), registryKey("k")); err == nil || errors.Is(err, server.ErrGrantNotListed) { t.Fatal("revoking = nil error, want error") } }) @@ -309,7 +318,7 @@ func TestRevokeListedGrantKeepsAnEntryItCouldntFinish(t *testing.T) { registry := &stubRegistry{deleteErr: errors.New("registry down")} h := stubRegistryHarness(t, registry) registry.entries = []storage.GrantEntry{registryEntry(t, h, "k", testIssuer, "user-1", h.now, nil)} - if err := h.server.RevokeListedGrant(context.Background(), mustSubjectID(t, "user-1"), "k"); err == nil { + if err := h.server.RevokeListedGrant(context.Background(), mustSubjectID(t, "user-1"), registryKey("k")); err == nil { t.Fatal("RevokeListedGrant with the registry down = nil error, want error") } if len(registry.deleted) != 1 { @@ -318,16 +327,28 @@ func TestRevokeListedGrantKeepsAnEntryItCouldntFinish(t *testing.T) { } // TestRevokeListedGrantRefusesAnEntryWithoutItsToken: an entry whose -// refresh token hash is missing can't be revoked as listed. +// refresh token hash isn't the one its key is derived from (missing, +// short, or another token's, as a forged unsealed record's could be) +// isn't listed, so can't be revoked as the grant it claims to be. func TestRevokeListedGrantRefusesAnEntryWithoutItsToken(t *testing.T) { - registry := &stubRegistry{} - h := stubRegistryHarness(t, registry) - registry.entries = []storage.GrantEntry{registryEntry(t, h, "k", testIssuer, "user-1", h.now, map[string]any{"rth": []byte{1, 2}})} - if err := h.server.RevokeListedGrant(context.Background(), mustSubjectID(t, "user-1"), "k"); err == nil || errors.Is(err, server.ErrGrantNotListed) { - t.Fatalf("RevokeListedGrant(no refresh token hash) = %v, want an error naming it", err) - } - if len(registry.deleted) != 0 { - t.Error("the entry was deleted though its refresh token wasn't revoked") + for name, rth := range map[string]any{"short": []byte{1, 2}, "another token's": sha256.Sum256([]byte("other"))} { + t.Run(name, func(t *testing.T) { + registry := &stubRegistry{} + h := stubRegistryHarness(t, registry) + if hash, ok := rth.([32]byte); ok { + rth = hash[:] + } + registry.entries = []storage.GrantEntry{registryEntry(t, h, "k", testIssuer, "user-1", h.now, map[string]any{"rth": rth})} + if grants := listGrants(t, h, "user-1"); len(grants) != 0 { + t.Errorf("ListGrants = %+v, want the mismatched entry left out", grants) + } + if err := h.server.RevokeListedGrant(context.Background(), mustSubjectID(t, "user-1"), registryKey("k")); !errors.Is(err, server.ErrGrantNotListed) { + t.Fatalf("RevokeListedGrant(mismatched entry) = %v, want ErrGrantNotListed", err) + } + if len(registry.deleted) != 0 { + t.Error("the mismatched entry was deleted") + } + }) } } @@ -347,7 +368,7 @@ func TestRevokeListedGrantReportsEachFailure(t *testing.T) { h := stubRegistryHarness(t, registry) registry.entries = []storage.GrantEntry{registryEntry(t, h, "k", testIssuer, "user-1", h.now, tc.extra)} tc.fail(h) - if err := h.server.RevokeListedGrant(context.Background(), mustSubjectID(t, "user-1"), "k"); err == nil { + if err := h.server.RevokeListedGrant(context.Background(), mustSubjectID(t, "user-1"), registryKey("k")); err == nil { t.Fatal("RevokeListedGrant = nil error, want the failure") } if len(registry.deleted) != 0 { @@ -421,6 +442,86 @@ func TestRevokeSubjectGrantsReportsFailures(t *testing.T) { }) } +// deleteRecordingRegistry records the keys DeleteGrant is called with. +type deleteRecordingRegistry struct { + *memstore.GrantRegistry + deleted *[]string +} + +func (r deleteRecordingRegistry) DeleteGrant(ctx context.Context, key string) error { + *r.deleted = append(*r.deleted, key) + return r.GrantRegistry.DeleteGrant(ctx, key) +} + +// TestCodeReuseForgetsTheGrant: reusing an authorization code revokes +// the refresh token it was exchanged for, and removes its grant from the +// registry, so a "connected apps" page doesn't list it. +func TestCodeReuseForgetsTheGrant(t *testing.T) { + var deleted []string + h, owner := registryHarness(t, deleteRecordingRegistry{memstore.NewGrantRegistry(), &deleted}) + code := owner.attestedCode() + if _, err := owner.exchangeCode(code, testCodeVerifier); err != nil { + t.Fatalf("first exchange: %v", err) + } + listed := listGrants(t, h, "user-1") + if len(listed) != 1 { + t.Fatalf("ListGrants = %d grants, want 1", len(listed)) + } + if _, err := owner.exchangeCode(code, testCodeVerifier); err == nil { + t.Fatal("reusing the code succeeded") + } + if !slices.Contains(deleted, listed[0].ID) { + t.Errorf("DeleteGrant calls = %v, want the reused code's grant %q", deleted, listed[0].ID) + } +} + +// TestRevokeSubjectGrantsKeepsGoing: an unreadable entry, or a grant +// that fails to revoke, doesn't stop the rest being revoked; every +// failure is reported, with how many were revoked. +func TestRevokeSubjectGrantsKeepsGoing(t *testing.T) { + registry := &stubRegistry{} + h := stubRegistryHarness(t, registry) + registry.entries = []storage.GrantEntry{ + {Key: "bad", ClientID: testClientID, Record: json.RawMessage(`{`), ExpiresAt: h.now.Add(time.Hour)}, + registryEntry(t, h, "good", testIssuer, "user-1", h.now, nil), + } + n, err := h.server.RevokeSubjectGrants(context.Background(), mustSubjectID(t, "user-1")) + if n != 1 || err == nil { + t.Fatalf("RevokeSubjectGrants = %d, %v, want 1 revoked and the unreadable entry reported", n, err) + } + if !slices.Contains(registry.deleted, registryKey("good")) { + t.Errorf("deleted = %v, want the readable grant revoked", registry.deleted) + } + // One that fails to revoke doesn't stop the next either. + registry.entries = []storage.GrantEntry{ + registryEntry(t, h, "first", testIssuer, "user-1", h.now, nil), + registryEntry(t, h, "second", testIssuer, "user-1", h.now.Add(time.Second), nil), + } + registry.deleted, registry.deleteErr = nil, errors.New("registry down") + if n, err := h.server.RevokeSubjectGrants(context.Background(), mustSubjectID(t, "user-1")); n != 0 || err == nil || len(registry.deleted) != 2 { + t.Fatalf("RevokeSubjectGrants = %d, %v after %d deletes, want 0, an error, and both attempted", n, err, len(registry.deleted)) + } +} + +// TestRevokeListedGrantPastAnUnreadableEntry: a readable grant is +// revocable though another of the subject's entries can't be read; an +// ID not among the readable ones reports the unreadable entry rather +// than claiming it isn't listed. +func TestRevokeListedGrantPastAnUnreadableEntry(t *testing.T) { + registry := &stubRegistry{} + h := stubRegistryHarness(t, registry) + registry.entries = []storage.GrantEntry{ + {Key: "bad", ClientID: testClientID, Record: json.RawMessage(`{`), ExpiresAt: h.now.Add(time.Hour)}, + registryEntry(t, h, "good", testIssuer, "user-1", h.now, nil), + } + if err := h.server.RevokeListedGrant(context.Background(), mustSubjectID(t, "user-1"), registryKey("good")); err != nil { + t.Fatalf("RevokeListedGrant(readable) = %v, want nil", err) + } + if err := h.server.RevokeListedGrant(context.Background(), mustSubjectID(t, "user-1"), "bad"); err == nil || errors.Is(err, server.ErrGrantNotListed) { + t.Fatalf("RevokeListedGrant(unreadable) = %v, want the unreadable entry reported", err) + } +} + // undeletableRegistry records and lists grants, but fails every // DeleteGrant. type undeletableRegistry struct{ *memstore.GrantRegistry } diff --git a/server/lookup_backchannel_interaction_test.go b/server/lookup_backchannel_interaction_test.go index 7152b8b9..0dec3e13 100644 --- a/server/lookup_backchannel_interaction_test.go +++ b/server/lookup_backchannel_interaction_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "reflect" "testing" @@ -70,20 +71,36 @@ func TestLookupBackchannelInteractionServerErrors(t *testing.T) { store := memstore.NewBackchannelAuthenticationStore() required := beginBackchannel(t, newHarnessWithBackchannelStore(t, store), standardBackchannelParams(t)) - for name, h := range map[string]harness{ - "client no longer resolvable": newHarnessWithBackchannelOptions(t, store, nil, func(d *server.Dependencies) { + for name, tc := range map[string]struct { + h harness + want server.ErrorCode + }{ + "client no longer registered": {newHarnessWithBackchannelOptions(t, store, nil, func(d *server.Dependencies) { d.Clients = &fakeClientRepository{clients: map[fapi.ClientID]storage.RegisteredClient{}} - }), - "stored request unreadable": newHarnessWithBackchannelStore(t, corruptLookupStore{store}), + }), server.ErrorUnauthorizedClient}, + "client store unavailable": {newHarnessWithBackchannelOptions(t, store, nil, func(d *server.Dependencies) { + d.Clients = &fakeClientRepository{clients: map[fapi.ClientID]storage.RegisteredClient{}, err: fmt.Errorf("db: %w", storage.ErrStoreUnavailable)} + }), server.ErrorServerError}, + "stored request unreadable": {newHarnessWithBackchannelStore(t, corruptLookupStore{store}), server.ErrorServerError}, + "backchannel store unavailable": {newHarnessWithBackchannelStore(t, unavailableLookupStore{store}), server.ErrorServerError}, } { - _, err := h.server.LookupBackchannelInteraction(context.Background(), required.Handle) + _, err := tc.h.server.LookupBackchannelInteraction(context.Background(), required.Handle) var serverErr *server.Error - if !errors.As(err, &serverErr) || serverErr.Code() != server.ErrorServerError { - t.Errorf("%s: LookupBackchannelInteraction = %v, want server_error", name, err) + if !errors.As(err, &serverErr) || serverErr.Code() != tc.want { + t.Errorf("%s: LookupBackchannelInteraction = %v, want %s", name, err, tc.want) } } } +// unavailableLookupStore's lookups fail as an outage. +type unavailableLookupStore struct { + *memstore.BackchannelAuthenticationStore +} + +func (unavailableLookupStore) LookupBackchannelAuthentication(context.Context, [32]byte) (storage.LookedUpBackchannelAuthentication, error) { + return storage.LookedUpBackchannelAuthentication{}, fmt.Errorf("db: %w", storage.ErrStoreUnavailable) +} + // fixedRecordStore returns record as every looked-up request. type fixedRecordStore struct { *memstore.BackchannelAuthenticationStore diff --git a/server/token.go b/server/token.go index 9382794b..98969a67 100644 --- a/server/token.go +++ b/server/token.go @@ -473,7 +473,9 @@ func (s *Server) revokeTokensForReusedCode(ctx context.Context, err error, prese _ = s.deps.Revocation.Revoke(ctx, alreadyRedeemed.IssuedAccessTokenKey, s.deps.Clock.Now().Add(timeutil.SaturatingAdd(s.cfg.Limits.AccessTokenLifetime, s.cfg.Limits.MaxClockSkew, s.cfg.Limits.MaxClockSkew))) } if alreadyRedeemed.IssuedRefreshTokenHash != nil { - _ = s.deps.Grants.RevokeRefreshToken(ctx, *alreadyRedeemed.IssuedRefreshTokenHash) + if s.deps.Grants.RevokeRefreshToken(ctx, *alreadyRedeemed.IssuedRefreshTokenHash) == nil { + _ = s.forgetRevokedRefreshToken(ctx, *alreadyRedeemed.IssuedRefreshTokenHash) + } } if _, ok := s.grantRevocationReader(); ok { until := s.deps.Clock.Now().Add(s.grantRevocationHorizon())