fix(approval): a refused notice is a limit, and does not use up max_per_day

DESIGN.md

@@ -670,13 +670,14 @@ Not masked, for example:
   They are silent (`disable_notification`). At most 20 notices go out in
   any 60 seconds, for all clients together, because Telegram allows
   about 20 messages per minute in a group. A notice that could not be
-  sent counts too. A use that would need one more notice is denied, but
-  not as for the limit per client in section 8: it is denied as when a
-  notice cannot be sent (cause `error`, code `approver_failed`), and it
-  still counts for `max_per_day`, because the core counts a use before
-  it sends the notice. So with `max_per_day = 1`, a burst of notices of
-  other clients can use up the one use of a night; then the next use asks
-  the person.
+  sent counts too. A use that would need one more notice is denied as
+  for the limit per client in section 8: with cause `limit`, and it does
+  not count for `max_per_day`, because no notice went out. So a burst of
+  notices of other clients cannot use up the one use of a night. Any
+  other failure to send a notice denies the use with cause `error` (code
+  `approver_failed`), and the use still counts, because the core cannot
+  tell if the notice went out. That holds for Telegram's own code 429
+  after the last try too: an earlier try may have sent the notice.
 
 ## 8. Rules and grants
 
@@ -751,7 +752,11 @@ name exactly one client. The config check refuses all of these (section
   messages for other clients could not be sent. A "never ask" rule with
   `notify = false` sends no notice and has no such limit. The Telegram
   code also sends at most 20 notices in any 60 seconds for all clients
-  together (section 7).
+  together (section 7). A use that this limit stops is denied in the same
+  way: cause `limit`, and it does not count for `max_per_day`. A notice
+  that the Telegram code refused, or could not send, still counts toward
+  the 6 of its client, so a client cannot try again and again within the
+  minute.
 
 ## 9. Audit log
 
@@ -1222,7 +1227,10 @@ listed hosts with that key) without a tap. To keep it small:
   the notice chat, so a strange use is seen. If that message cannot be
   sent, the request is denied. At most 6 notices go out in any 60 seconds
   for one client (section 8), and at most 20 for all clients together
-  (section 7); a use over that is denied.
+  (section 7). A use over that is denied with cause `limit`, and it does
+  not count for `max_per_day`, because no message went out. A use whose
+  message failed for another reason still counts, because the broker
+  cannot tell if the message went out.
 - Use a narrow secret: a secret that can do little, or a deploy key that
   can reach only one repository. For `bws`, the job's secret can live in its
   own project, read through its own store entry with its own machine

THREAT-MODEL.md

@@ -211,7 +211,10 @@ already wait in the queue. A request leaves the queue only with at least
 use over that is denied. So a client with such a rule cannot fill the
 notice chat, or use up Telegram's limits so that messages for other
 clients fail. The Telegram code also sends at most 20 notices in any 60
-seconds for all clients together.
+seconds for all clients together. A use over either limit is denied with
+cause `limit`, and it does not count for `max_per_day`. So a burst of
+notices of other clients cannot use up the uses of a night; it only
+denies uses while the limit is full.
 Risk that remains: low.
 
 **T10. Someone on the network reads or changes the traffic.**
@@ -387,8 +390,9 @@ Requests expire and are denied. The agent waits, then fails. A request
 whose message cannot be sent is denied. If the polling stops for good, the
 requests that wait for the person are denied at once, and a message or
 notice that is still being sent stops. A "never ask" use whose notice
-cannot be sent is denied too. If the broker host sleeps, requests and
-grants still end on the wall clock.
+cannot be sent is denied too, and it still counts for `max_per_day`,
+because the broker cannot tell if the notice went out. If the broker host
+sleeps, requests and grants still end on the wall clock.
 Risk that remains: the agent's work stops. That is the safe way to fail.
 
 **T22. A client uses a secret meant for another client.**

internal/approval/approvaltest/approver.go

@@ -171,7 +171,9 @@ func (a *Approver) FailAsk(err error) {
 	a.askErr = err
 }
 
-// FailNotify makes Notify fail with err. nil makes it work again.
+// FailNotify makes Notify fail with err. nil makes it work again. An err
+// that wraps approval.ErrLimit acts as a limit of the approver itself:
+// Notify keeps nothing, as it sent nothing.
 func (a *Approver) FailNotify(err error) {
 	a.mu.Lock()
 	defer a.mu.Unlock()

internal/approval/approver.go

@@ -31,6 +31,12 @@ type Approver interface {
 	// request p without a tap (DESIGN.md section 12). The prompt has no
 	// buttons. ctx ends as for Ask. If Notify returns an error, the core
 	// denies the request, so a use without a tap is never unseen.
+	//
+	// If a limit of the approver itself stops the notice, Notify returns
+	// an error that wraps ErrLimit. Then the cause is limit, and the use
+	// does not count for max_per_day. After any other error, the notice
+	// may have gone out, so the use counts. The cause is then error (code
+	// approver_failed), or cancelled when the client hung up.
 	Notify(ctx context.Context, p Prompt) error
 
 	// Run gets the person's answers and passes each one to answer, until
@@ -46,6 +52,13 @@ type Approver interface {
 	Run(ctx context.Context, answer func(Answer) error) error
 }
 
+// ErrLimit says that a limit of the approver itself stopped a notice, such
+// as the notices per minute that Telegram allows. Notify may return it, or
+// an error that wraps it, only when it sent nothing. The core then denies
+// the use with cause limit, and the use does not count for max_per_day
+// (DESIGN.md section 8).
+var ErrLimit = errors.New("approval: over a limit of the approver")
+
 // MessageID names a message that an approver sent. The approver chooses
 // its form: for Telegram it holds the chat and the message number. An
 // answer counts only on the message of its own request, so the core

internal/approval/decide.go

@@ -51,6 +51,10 @@ type pending struct {
 	windowMinutes   int  // the message has "Approve N min" with this N; 0: no
 	notify          bool // a "never ask" rule with notify allowed it
 
+	// counted holds the uses of "never ask" rules that begin counted for
+	// p, so that notify can take them back. Under Core.mu.
+	counted []countedUse
+
 	// Under Core.mu:
 	state  state
 	msg    MessageID
@@ -207,10 +211,14 @@ func (c *Core) begin(p *pending) *Result {
 			}
 			cs.notices = cs.notices.add(now.mono, MaxNoticesPerMinute)
 		}
+		// The uses count now, under the lock, so requests at the same
+		// time cannot pass max_per_day. If the approver's limit stops the
+		// notice, notify takes them back.
 		for i, it := range p.items {
 			if vias[i] == audit.ViaNeverRule {
 				k := useKey{p.client, it.ref}
 				c.uses[k] = c.uses[k].add(useDay(it.rule, now.wall))
+				p.counted = append(p.counted, countedUse{key: k, day: c.uses[k].day})
 			}
 		}
 		p.notify = notify
@@ -527,12 +535,22 @@ func (c *Core) end(ctx context.Context, p *pending, prompt Prompt, res *Result)
 // notify sends the notice of a use of a "never ask" rule. If the notice
 // cannot be sent, the request is denied: notify = true promises that no
 // use goes unseen (DESIGN.md section 12).
+//
+// If the approver's own limit stopped the notice (ErrLimit), nothing was
+// sent, so the use did not happen: the cause is limit, as for the limit
+// per client, and the use does not count for max_per_day. After any other
+// error, or a hang-up, the notice may have gone out, so the use still
+// counts: an unclear case keeps the stricter count, and the next use may
+// ask the person.
 func (c *Core) notify(ctx context.Context, p *pending, res *Result) *Result {
 	now := c.now()
 	nctx, cancel := c.callContext(ctx, now.until(p.deadline))
 	err := c.approver.Notify(nctx, p.notice(now))
 	cancel()
 	switch {
+	case errors.Is(err, ErrLimit):
+		c.uncount(p)
+		return c.denied(p, audit.CauseLimit)
 	case ctx.Err() != nil:
 		return c.denied(p, audit.CauseCancelled)
 	case err != nil:
@@ -541,6 +559,17 @@ func (c *Core) notify(ctx context.Context, p *pending, res *Result) *Result {
 	return res
 }
 
+// uncount takes back the uses of "never ask" rules that begin counted for
+// p.
+func (c *Core) uncount(p *pending) {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	for _, u := range p.counted {
+		c.uses[u.key] = c.uses[u.key].remove(u.day)
+	}
+	p.counted = nil
+}
+
 // newResult returns the result of p, not yet decided.
 func (c *Core) newResult(p *pending) *Result {
 	s := p.subject

internal/approval/export_test.go

@@ -1,6 +1,9 @@
 package approval
 
-import "io"
+import (
+	"io"
+	"time"
+)
 
 // SetRandom replaces the source of random bytes for request IDs and short
 // codes, for tests of what happens when IDs repeat or the source fails.
@@ -28,3 +31,15 @@ func (c *Core) Live() (ids, codes, waiting int) {
 	defer c.mu.Unlock()
 	return len(c.ids), len(c.codes), len(c.waiting)
 }
+
+// Uses returns how many uses of the "never ask" rule of the secret name,
+// or of the key name if key, by client count for the wall time at.
+func (c *Core) Uses(client string, key bool, name string, at time.Time) int {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	rule := c.cfg.Secrets[name].Rule
+	if key {
+		rule = c.cfg.Keys[name].Rule
+	}
+	return c.uses[useKey{client, ref{key: key, name: name}}].on(useDay(rule, at))
+}

internal/approval/fuzz_test.go

@@ -3,6 +3,7 @@ package approval_test
 import (
 	"context"
 	"errors"
+	"fmt"
 	"slices"
 	"sync"
 	"testing"
@@ -19,8 +20,9 @@ import (
 
 // FuzzCore runs random steps against the core: requests of each kind from
 // three clients, taps (also ones that must not count), cancels, clock
-// steps, StartGrants, and an approver that fails or stops. After each step
-// it checks what the core promises:
+// steps, StartGrants, and an approver that fails, refuses notices because
+// of its own limit, or stops. After each step it checks what the core
+// promises:
 //
 //   - a request is allowed only by one counted tap on Approve, or, without
 //     a tap, by grants and "never ask" rules that a model of them allows;
@@ -31,6 +33,10 @@ import (
 //   - no message for a secret or key within 60 seconds after a Deny for
 //     it, and no grant for it from a tap before that Deny;
 //   - per client, at most 6 notices of "never ask" uses in any 60 seconds;
+//   - a use whose notice a limit of the approver refused is denied with
+//     cause limit, and it does not count for max_per_day; a use whose
+//     notice failed in another way counts: the core counts the same uses
+//     as the model;
 //   - an SSH grant is approved only before its grants end;
 //   - every message is updated exactly once, and every request ends.
 //
@@ -42,6 +48,10 @@ func FuzzCore(f *testing.F) {
 	f.Add([]byte{4, 0x00, 0x0f, 0x03, 5, 0x01, 9, 0, 3, 0x00, 0x00, 3, 0x08, 0x00, 7, 255, 7, 255, 7, 255, 7, 255})
 	f.Add([]byte{0, 0x03, 0, 0x03, 0, 0x03, 0, 0x03, 0, 0x03, 0, 0x03, 0, 0x0b, 3, 0x83, 0x02, 3, 0x83, 0x02, 7, 100, 3, 0x83, 0x02})
 	f.Add([]byte{0, 0x08, 8, 0, 6, 0, 10, 0, 0, 0x08, 10, 0, 11, 0, 0, 0x0b, 12, 0, 0, 0x08})
+	// backup-token by ci, max_per_day = 2: a notice fails, and that use
+	// counts; the approver's limit refuses two notices, and those uses do
+	// not count. So one more use goes without a tap, and the next asks.
+	f.Add([]byte{11, 0, 0x20, 11, 0, 0x20, 0, 0x20, 11, 7, 61, 0, 0x20, 0, 0x20, 0, 0x20})
 	f.Fuzz(runFuzzInput)
 }
 
@@ -113,6 +123,9 @@ func (fakeTB) Fatalf(f string, args ...any) { panic(f) }
 var (
 	errAskFails    = errors.New("fuzz: Ask fails")
 	errNotifyFails = errors.New("fuzz: Notify fails")
+	// errNotifyLimit is a limit of the approver itself: Notify sends
+	// nothing.
+	errNotifyLimit = fmt.Errorf("fuzz: Notify is over its limit: %w", approval.ErrLimit)
 )
 
 type fuzzReq struct {
@@ -163,18 +176,19 @@ type model struct {
 	quit  context.CancelFunc
 	run   chan error
 
-	mu      sync.Mutex // guards seq, asks and notices, which the core's goroutines touch
+	mu      sync.Mutex // guards seq, asks, notices and refused, which the core's goroutines touch
 	seq     int
 	asks    []fuzzAsk
 	notices []fuzzNotice
+	refused map[string]bool // the requests whose notice the approver's limit refused, by ID
 
 	reqs       []*fuzzReq
 	taps       map[string]fuzzTap // the counted taps, by request ID
 	denies     []fuzzTap
 	grants     map[scope]fuzzEnd
 	uses       map[string]int // never item and day → uses
 	failAsk    bool
-	failNotify bool
+	notifyMode int // Notify 0: works; 1: fails; 2: refuses because of its own limit
 	stopped    bool
 }
 
@@ -196,18 +210,22 @@ func (r recorder) Ask(ctx context.Context, p approval.Prompt) (approval.Shown, e
 
 func (r recorder) Notify(ctx context.Context, p approval.Prompt) error {
 	err := r.Approver.Notify(ctx, p)
-	if err == nil {
-		r.m.mu.Lock()
+	r.m.mu.Lock()
+	defer r.m.mu.Unlock()
+	switch {
+	case err == nil:
 		r.m.notices = append(r.m.notices, fuzzNotice{client: p.Client, at: r.m.clock.Now()})
-		r.m.mu.Unlock()
+	case errors.Is(err, approval.ErrLimit):
+		r.m.refused[p.RequestID] = true
 	}
 	return err
 }
 
 func newModel(t *testing.T, cfg *config.Broker, h hosts) *model {
 	m := &model{
 		t: t, cfg: cfg, h: h, clock: approvaltest.NewClock(noon), fake: approvaltest.NewApprover(approverID),
-		taps: map[string]fuzzTap{}, grants: map[scope]fuzzEnd{}, uses: map[string]int{}, run: make(chan error, 1),
+		refused: map[string]bool{}, taps: map[string]fuzzTap{}, grants: map[scope]fuzzEnd{}, uses: map[string]int{},
+		run: make(chan error, 1),
 	}
 	core, err := approval.New(cfg, recorder{m.fake, m}, approval.Options{Clock: m.clock})
 	if err != nil {
@@ -312,9 +330,9 @@ func (m *model) step(ops []byte, i int) int {
 		m.failAsk = !m.failAsk
 		m.fake.FailAsk(map[bool]error{true: errAskFails}[m.failAsk])
 		return i + 1
-	case 11:
-		m.failNotify = !m.failNotify
-		m.fake.FailNotify(map[bool]error{true: errNotifyFails}[m.failNotify])
+	case 11: // Notify works, fails, or refuses because of its own limit, in turn
+		m.notifyMode = (m.notifyMode + 1) % 3
+		m.fake.FailNotify([]error{nil, errNotifyFails, errNotifyLimit}[m.notifyMode])
 		return i + 1
 	case 12: // the approver stops, rarely
 		if arg(1) == 0x5a && !m.stopped {
@@ -466,6 +484,13 @@ func (m *model) checkResult(r *fuzzReq) {
 			t.Fatalf("request %s has a message text, but no message was sent", res.RequestID)
 		}
 	}
+	m.mu.Lock()
+	refused := m.refused[res.RequestID]
+	m.mu.Unlock()
+	if refused && (res.Allowed || res.Cause != audit.CauseLimit || res.ErrorCode != "" || res.Err != nil) {
+		t.Fatalf("the approver's limit refused the notice of request %s: allowed %v, cause %q, code %q, err %v; want cause limit",
+			res.RequestID, res.Allowed, res.Cause, res.ErrorCode, res.Err)
+	}
 	switch {
 	case res.Answer != "":
 		tap, ok := m.taps[res.RequestID]
@@ -479,7 +504,9 @@ func (m *model) checkResult(r *fuzzReq) {
 		m.checkCovered(r)
 	case errors.Is(res.Err, errNotifyFails), res.Cause == audit.CauseError && errors.Is(res.Err, context.Canceled):
 		// The notice failed, or could not go out because Run had returned.
-		// The core counted the uses before.
+		// The core counted the uses before, and keeps them: the notice may
+		// have gone out. A notice that the approver's limit refused ends
+		// with cause limit (checked above), and its uses do not count.
 		m.checkCovered(r)
 	default:
 		switch res.Cause {
@@ -579,4 +606,20 @@ func (m *model) check() {
 			t.Fatalf("%s sent %d notices in a minute", a.client, n)
 		}
 	}
+	// The core counts the same uses of the "never ask" rules as the model:
+	// also those whose notice failed, but not those whose notice the
+	// approver's limit refused.
+	now := m.clock.Wall()
+	for _, it := range []struct {
+		key  bool
+		name string
+	}{{false, "backup-token"}, {true, "backup"}} {
+		item := "secret:" + it.name
+		if it.key {
+			item = "key:" + it.name
+		}
+		if got, want := m.core.Uses("ci", it.key, it.name, now), m.uses[item+now.Format(time.DateOnly)]; got != want {
+			t.Fatalf("the core counts %d uses of %s today, the model %d", got, item, want)
+		}
+	}
 }