diff --git a/DESIGN.md b/DESIGN.md index fe6a64d..ef55b99 100644 --- a/DESIGN.md +++ b/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 diff --git a/THREAT-MODEL.md b/THREAT-MODEL.md index 6882bfe..b673136 100644 --- a/THREAT-MODEL.md +++ b/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.** diff --git a/internal/approval/approvaltest/approver.go b/internal/approval/approvaltest/approver.go index a7ec814..6682d87 100644 --- a/internal/approval/approvaltest/approver.go +++ b/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() diff --git a/internal/approval/approver.go b/internal/approval/approver.go index c60856d..17420f8 100644 --- a/internal/approval/approver.go +++ b/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 diff --git a/internal/approval/decide.go b/internal/approval/decide.go index 32e0f87..72473e4 100644 --- a/internal/approval/decide.go +++ b/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 diff --git a/internal/approval/export_test.go b/internal/approval/export_test.go index 66ac145..705fc19 100644 --- a/internal/approval/export_test.go +++ b/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)) +} diff --git a/internal/approval/fuzz_test.go b/internal/approval/fuzz_test.go index f4b47ef..1da049c 100644 --- a/internal/approval/fuzz_test.go +++ b/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,10 +176,11 @@ 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 @@ -174,7 +188,7 @@ type model struct { 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,10 +210,13 @@ 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 } @@ -207,7 +224,8 @@ func (r recorder) Notify(ctx context.Context, p approval.Prompt) error { 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) + } + } } diff --git a/internal/approval/internal_test.go b/internal/approval/internal_test.go index e2df0a2..5d9ed7f 100644 --- a/internal/approval/internal_test.go +++ b/internal/approval/internal_test.go @@ -61,6 +61,40 @@ func TestDayUses(t *testing.T) { } } +// TestDayUsesRemove checks that a use taken back comes off the count of +// its own day only: not off a count that a later day started again, and +// never below 0. (Issue #47: a notice that the approver's limit stopped.) +func TestDayUsesRemove(t *testing.T) { + d1 := dayOf(time.Date(2026, 10, 14, 23, 59, 59, 0, time.UTC)) + d2 := dayOf(time.Date(2026, 10, 15, 0, 0, 0, 0, time.UTC)) + d0 := dayOf(time.Date(2026, 10, 13, 12, 0, 0, 0, time.UTC)) + + u := dayUses{}.add(d1).add(d1) + if u = u.remove(d1); u.on(d1) != 1 { + t.Errorf("after one use was taken back, the count is %d, want 1", u.on(d1)) + } + if u = u.remove(d1).remove(d1); u.on(d1) != 0 { + t.Errorf("the count is %d, want 0", u.on(d1)) + } + // A later day started again: a use of the day before is not in it. + u = u.add(d1).add(d2) + if u = u.remove(d1); u.on(d2) != 1 { + t.Errorf("a use of the day before came off the new count: %d, want 1", u.on(d2)) + } + // While the clock is back, a use counts on the later day. add returns + // that day, and the use comes off its count. + u = u.add(d0) + if u.day != d2 || u.on(d2) != 2 { + t.Fatalf("a use while the clock is back: day %v, count %d; want %v and 2", u.day, u.on(d2), d2) + } + if u = u.remove(u.day); u.on(d2) != 1 { + t.Errorf("the count is %d, want 1", u.on(d2)) + } + if u = u.remove(d0); u.on(d2) != 1 { + t.Errorf("a remove for an earlier day changed the count to %d", u.on(d2)) + } +} + // TestWindowMinutes checks the N of "Approve N min". func TestWindowMinutes(t *testing.T) { w := func(d time.Duration) item { return item{rule: config.Rule{Ask: config.AskWindow, Window: d}} } diff --git a/internal/approval/limits_test.go b/internal/approval/limits_test.go index 3908b6b..d89781c 100644 --- a/internal/approval/limits_test.go +++ b/internal/approval/limits_test.go @@ -2,6 +2,7 @@ package approval_test import ( "context" + "fmt" "testing" "testing/synctest" "time" @@ -464,6 +465,56 @@ func TestLimitNotices(t *testing.T) { }) } +// TestLimitApproverRefusesNotice checks a "never ask" use whose notice a +// limit of the approver itself stops, such as the limit of the Telegram +// code for all clients together. The use is denied with cause limit, as +// for the limit per client, without an error entry, and it does not count +// for max_per_day: with max_per_day = 1, the next use in the same night +// still works. The refused notices still count for the limit per client. +// Before, such a use was an approver failure, and it used up the one use +// of the night. (Issue #47.) +func TestLimitApproverRefusesNotice(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + cfg, h := testConfig(t) + night := parseHours(t, "23:30-00:30") + setSecretRule(cfg, "backup-token", config.Rule{Ask: config.AskNever, Notify: true, Hours: &night, MaxPerDay: 1}) + e := newEnvWith(t, cfg, h, approvaltest.NewClock(time.Date(2026, 10, 14, 23, 50, 0, 0, time.UTC))) + defer e.stop() + + e.fake.FailNotify(fmt.Errorf("the notice chat is full: %w", approval.ErrLimit)) + for range approval.MaxNoticesPerMinute { + r := e.now(secretsReq("ci", "backup-token")) + checkDenied(t, r, audit.CauseLimit) + if r.ErrorCode != "" || r.Err != nil { + t.Errorf("code %q, err %v; want no error", r.ErrorCode, r.Err) + } + checkEvents(t, r, "request", "denied:limit") + } + // The refused notices count for the limit per client: in this + // minute no notice goes out, also when the approver has room again. + e.fake.FailNotify(nil) + checkDenied(t, e.now(secretsReq("ci", "backup-token")), audit.CauseLimit) + if n := len(e.fake.Notices()); n != 0 { + t.Errorf("%d notices, want 0", n) + } + e.asked(0) + + // 00:10 is the next day, but the same night: its uses count for + // October 14, and none of the refused uses counted. + e.clock.Advance(20 * time.Minute) + r := e.now(secretsReq("ci", "backup-token")) + checkAllowed(t, r) + checkEvents(t, r, "request", "grant_used:never_rule") + if n := len(e.fake.Notices()); n != 1 { + t.Errorf("%d notices, want 1", n) + } + // That was the one use of the night: the next one asks the person. + ch := e.decide(secretsReq("ci", "backup-token")) + mustWait(t, ch) + e.asked(1) + }) +} + // slowNotify is an approver whose Notify waits until release is closed, // as Telegram does when it is slow or limits the bot. type slowNotify struct { diff --git a/internal/approval/notice_limit_test.go b/internal/approval/notice_limit_test.go new file mode 100644 index 0000000..0634c0d --- /dev/null +++ b/internal/approval/notice_limit_test.go @@ -0,0 +1,203 @@ +package approval_test + +// Attack tests from the security review of issue #47: can taking back a +// use after an approver limit (uncount) give more than max_per_day uses +// without a tap? + +import ( + "context" + "fmt" + "sync" + "testing" + "testing/synctest" + "time" + + "github.com/code6x/nodkey/internal/approval" + "github.com/code6x/nodkey/internal/approval/approvaltest" + "github.com/code6x/nodkey/internal/audit" + "github.com/code6x/nodkey/internal/config" +) + +// holdFirstNotice holds the first Notify until release is closed, then +// refuses it with the approver's limit. Later calls go to the fake. +type holdFirstNotice struct { + *approvaltest.Approver + mu sync.Mutex + calls int + held chan struct{} // closed when the first call waits + release chan struct{} +} + +func (a *holdFirstNotice) Notify(ctx context.Context, p approval.Prompt) error { + a.mu.Lock() + a.calls++ + first := a.calls == 1 + a.mu.Unlock() + if first { + close(a.held) + <-a.release + return fmt.Errorf("refused: %w", approval.ErrLimit) + } + return a.Approver.Notify(ctx, p) +} + +// nlEnv starts a core with backup-token as never, notify, max_per_day = 1 +// and the approver a, at start. +func nlEnv(t *testing.T, start time.Time, hours *config.Hours) (*approval.Core, *holdFirstNotice, *approvaltest.Clock, context.CancelFunc) { + t.Helper() + cfg, _ := testConfig(t) + setSecretRule(cfg, "backup-token", config.Rule{Ask: config.AskNever, Notify: true, MaxPerDay: 1, Hours: hours}) + a := &holdFirstNotice{Approver: approvaltest.NewApprover(approverID), held: make(chan struct{}), release: make(chan struct{})} + clock := approvaltest.NewClock(start) + core, err := approval.New(cfg, a, approval.Options{Clock: clock}) + if err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithCancel(context.Background()) + go func() { _ = core.Run(ctx) }() + synctest.Wait() + return core, a, clock, cancel +} + +// decideAsync runs Decide with a context that the test can end, so a +// request that asks the person does not wait for its timeout. +func decideAsync(core *approval.Core) (<-chan *approval.Result, context.CancelFunc) { + ctx, cancel := context.WithCancel(context.Background()) + ch := make(chan *approval.Result, 1) + go func() { ch <- core.Decide(ctx, secretsReq("ci", "backup-token")) }() + return ch, cancel +} + +// TestNLConcurrentRefusalGivesNoExtraUse: A holds the one use while its +// notice waits; B, at the same time, must ask. A is refused by the limit +// and its use comes back; C then goes without a tap, and D asks. One use +// without a tap in all. +func TestLimitRefusalAtTheSameTimeGivesNoExtraUse(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + core, a, _, cancel := nlEnv(t, noon, nil) + defer cancel() + chA, cA := decideAsync(core) + defer cA() + <-a.held + chB, cB := decideAsync(core) + synctest.Wait() + if n := len(a.Asked()); n != 1 { + t.Fatalf("B: %d messages, want 1 (B must ask while A holds the use)", n) + } + cB() + synctest.Wait() + checkDenied(t, <-chB, audit.CauseCancelled) + close(a.release) + synctest.Wait() + checkDenied(t, <-chA, audit.CauseLimit) + if n := core.Uses("ci", false, "backup-token", noon); n != 0 { + t.Fatalf("uses %d after the refusal, want 0", n) + } + chC, cC := decideAsync(core) + defer cC() + synctest.Wait() + checkAllowed(t, <-chC) + chD, cD := decideAsync(core) + synctest.Wait() + if n := len(a.Asked()); n != 2 { + t.Fatalf("D: %d messages, want 2 (the one use is gone)", n) + } + cD() + synctest.Wait() + <-chD + }) +} + +// TestNLRefusalAfterMidnightKeepsTheNewDay: A counts at 23:59:59 and is +// refused after midnight, when B has already counted for the new day. The +// take-back of A must not touch the count of the new day. +func TestLimitRefusalAfterMidnightKeepsTheNewDay(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + start := time.Date(2026, 10, 14, 23, 59, 59, 0, time.UTC) + core, a, clock, cancel := nlEnv(t, start, nil) + defer cancel() + chA, cA := decideAsync(core) + defer cA() + <-a.held + clock.Advance(2 * time.Second) // 00:00:01 on October 15 + chB, cB := decideAsync(core) + defer cB() + synctest.Wait() + checkAllowed(t, <-chB) // a new day + close(a.release) + synctest.Wait() + checkDenied(t, <-chA, audit.CauseLimit) + if n := core.Uses("ci", false, "backup-token", clock.Wall()); n != 1 { + t.Fatalf("uses of the new day %d, want 1 (A's take-back changed it)", n) + } + chC, cC := decideAsync(core) + synctest.Wait() + if n := len(a.Asked()); n != 1 { + t.Fatalf("C: %d messages, want 1 (C must ask)", n) + } + cC() + synctest.Wait() + <-chC + }) +} + +// TestNLRefusalWithTheClockSetBack: A counts on the 15th, the wall clock +// goes back to the 14th, and B must ask (the later day still counts). A's +// refusal takes its use back from the count of the 15th, so C may go; D +// asks. One use without a tap. +func TestLimitRefusalWithTheClockSetBack(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + core, a, clock, cancel := nlEnv(t, noon, nil) + defer cancel() + clock.JumpWall(24 * time.Hour) // the 15th + chA, cA := decideAsync(core) + defer cA() + <-a.held + clock.JumpWall(-24 * time.Hour) // back to the 14th + chB, cB := decideAsync(core) + synctest.Wait() + if n := len(a.Asked()); n != 1 { + t.Fatalf("B: %d messages, want 1", n) + } + cB() + synctest.Wait() + <-chB + close(a.release) + synctest.Wait() + checkDenied(t, <-chA, audit.CauseLimit) + chC, cC := decideAsync(core) + defer cC() + synctest.Wait() + checkAllowed(t, <-chC) + chD, cD := decideAsync(core) + synctest.Wait() + if n := len(a.Asked()); n != 2 { + t.Fatalf("D: %d messages, want 2", n) + } + cD() + synctest.Wait() + <-chD + }) +} + +// TestNLOnlyErrLimitIsALimit: errors that only look like a limit by their +// text, or that wrap another error, are failures, and the use counts. +func TestOnlyErrLimitIsALimit(t *testing.T) { + for _, err := range []error{ + fmt.Errorf("approval: over a limit of the approver"), // same text, not the value + fmt.Errorf("429 too many requests: %w", context.DeadlineExceeded), + } { + synctest.Test(t, func(t *testing.T) { + e := newEnv(t, func(cfg *config.Broker) { + setSecretRule(cfg, "backup-token", config.Rule{Ask: config.AskNever, Notify: true, MaxPerDay: 1}) + }) + defer e.stop() + e.fake.FailNotify(err) + r := e.now(secretsReq("ci", "backup-token")) + checkDenied(t, r, audit.CauseError) + if n := e.core.Uses("ci", false, "backup-token", noon); n != 1 { + t.Fatalf("%v: uses %d, want 1 (the notice may have gone out)", err, n) + } + }) + } +} diff --git a/internal/approval/rules_test.go b/internal/approval/rules_test.go index 85f61e0..02d0ce2 100644 --- a/internal/approval/rules_test.go +++ b/internal/approval/rules_test.go @@ -122,7 +122,7 @@ func TestRuleNever(t *testing.T) { t.Errorf("notice %+v; want the request without buttons", n) } - // The deny path: no notice, no use. + // The deny path: no notice, so the use is denied. e.fake.FailNotify(errors.New("telegram is down")) r = e.now(secretsReq("ci", "backup-token")) checkDenied(t, r, audit.CauseError) @@ -133,6 +133,37 @@ func TestRuleNever(t *testing.T) { }) } +// TestRuleNeverNoticeFails checks a "never ask" use whose notice fails for +// another reason than a limit of the approver: it is still denied with +// cause error, and it still counts for max_per_day, because the notice may +// have gone out. So with max_per_day = 1, the next use asks the person. +// Only a limit of the approver does not count (issue #47). +func TestRuleNeverNoticeFails(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + e := newEnv(t, func(cfg *config.Broker) { + setSecretRule(cfg, "backup-token", config.Rule{Ask: config.AskNever, Notify: true, MaxPerDay: 1}) + }) + defer e.stop() + down := errors.New("telegram cannot be reached") + e.fake.FailNotify(down) + r := e.now(secretsReq("ci", "backup-token")) + checkDenied(t, r, audit.CauseError) + if r.ErrorCode != audit.CodeApproverFailed || !errors.Is(r.Err, down) || errors.Is(r.Err, approval.ErrLimit) { + t.Errorf("code %q, err %v; want approver_failed and the error", r.ErrorCode, r.Err) + } + checkEvents(t, r, "request", "error:approver_failed", "denied:error") + + // The use counted: the next one asks, also when notices work again. + e.fake.FailNotify(nil) + ch := e.decide(secretsReq("ci", "backup-token")) + mustWait(t, ch) + e.asked(1) + if n := len(e.fake.Notices()); n != 0 { + t.Errorf("%d notices, want 0", n) + } + }) +} + // TestRuleNeverWithoutNotify checks that notify = false sends no notice. func TestRuleNeverWithoutNotify(t *testing.T) { synctest.Test(t, func(t *testing.T) { diff --git a/internal/approval/state.go b/internal/approval/state.go index 3d36aa2..85a2758 100644 --- a/internal/approval/state.go +++ b/internal/approval/state.go @@ -95,6 +95,23 @@ func (u dayUses) add(d day) dayUses { return dayUses{day: u.day, n: u.n + 1} } +// remove takes back one use from the count of the day d: the day that +// the count had after add counted the use. If the count has moved on to a +// later day since, the use is not in it, and nothing changes. +func (u dayUses) remove(d day) dayUses { + if u.day == d && u.n > 0 { + u.n-- + } + return u +} + +// countedUse is a use of a "never ask" rule that the core counted: the +// rule's client and item, and the day of the count that holds the use. +type countedUse struct { + key useKey + day day +} + // rateLog holds the times of the newest events of one kind, oldest first, // on the monotonic clock: at most as many as the limit per minute allows. type rateLog []time.Time diff --git a/internal/audit/event.go b/internal/audit/event.go index ef4b4f1..43b4f42 100644 --- a/internal/audit/event.go +++ b/internal/audit/event.go @@ -62,7 +62,7 @@ const ( CauseExpired Cause = "expired" // no answer in time CauseCancelled Cause = "cancelled" // the client hung up CauseNotAllowed Cause = "not_allowed" // a check failed: an unknown name, a name this client may not use, bad input - CauseLimit Cause = "limit" // a limit per client, the queue, or the pause after a Deny (DESIGN.md section 8) + CauseLimit Cause = "limit" // a limit per client, the queue, the pause after a Deny, or a limit of the approver itself (DESIGN.md section 8) CauseError Cause = "error" // a failure; an error entry says which ) diff --git a/internal/telegram/approver_test.go b/internal/telegram/approver_test.go index e69887a..7969d25 100644 --- a/internal/telegram/approver_test.go +++ b/internal/telegram/approver_test.go @@ -172,8 +172,8 @@ func TestNotifyRateLimit(t *testing.T) { } now = now.Add(time.Second) } - if err := e.a.Notify(context.Background(), p); !errors.Is(err, ErrNoticeLimit) { - t.Errorf("notice %d: %v, want ErrNoticeLimit", NoticesPerMinute+1, err) + if err := e.a.Notify(context.Background(), p); !errors.Is(err, ErrNoticeLimit) || !errors.Is(err, approval.ErrLimit) { + t.Errorf("notice %d: %v, want ErrNoticeLimit, a limit for the core", NoticesPerMinute+1, err) } if n := len(e.f.callsOf("sendMessage")); n != NoticesPerMinute { t.Errorf("%d notices sent, want %d", n, NoticesPerMinute) @@ -192,8 +192,8 @@ func TestNotifyRateLimit(t *testing.T) { e.f.reply("sendMessage", 400, `{"ok":false,"error_code":400,"description":"Bad Request: chat not found"}`) } for range NoticesPerMinute { - if err := e.a.Notify(context.Background(), p); err == nil || errors.Is(err, ErrNoticeLimit) { - t.Fatalf("Notify: %v, want the error of Telegram", err) + if err := e.a.Notify(context.Background(), p); err == nil || errors.Is(err, approval.ErrLimit) { + t.Fatalf("Notify: %v, want the error of Telegram, not a limit", err) } } if err := e.a.Notify(context.Background(), p); !errors.Is(err, ErrNoticeLimit) { @@ -204,6 +204,25 @@ func TestNotifyRateLimit(t *testing.T) { } } +// TestNoticeLimitIsALimit checks which errors of Notify the approval core +// takes as a limit of the approver (approval.ErrLimit): ErrNoticeLimit, +// when Notify sent nothing. An error of Telegram is not one, also not its +// code 429 "too many requests": an earlier try may have sent the notice. +// (Issue #47.) +func TestNoticeLimitIsALimit(t *testing.T) { + if !errors.Is(ErrNoticeLimit, approval.ErrLimit) { + t.Error("ErrNoticeLimit does not wrap approval.ErrLimit") + } + e := newEnv(t) + e.f.reply("sendMessage", 429, `{"ok":false,"error_code":429,"description":"Too Many Requests","parameters":{"retry_after":60}}`) + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + err := e.a.Notify(ctx, noticeOf(secretsPrompt())) + if ce := callErr(t, err); ce.code != 429 || errors.Is(err, approval.ErrLimit) { + t.Errorf("Notify after a 429: %v, want the 429 error, not a limit", err) + } +} + // TestAlert checks that an alert goes to the approver with a sound, and // that its text is escaped. func TestAlert(t *testing.T) { diff --git a/internal/telegram/core_test.go b/internal/telegram/core_test.go index 1c76028..6384a5e 100644 --- a/internal/telegram/core_test.go +++ b/internal/telegram/core_test.go @@ -519,12 +519,15 @@ func TestCoreSendFails(t *testing.T) { } } -// TestCoreGlobalNoticeLimit shows what happens to a "never ask" use when -// Notify refuses it because of the limit for all clients together: the -// core denies it with cause error and code approver_failed, as when a -// notice cannot be sent, and the use still counts for max_per_day, -// because the core counts it before it sends the notice. So the next use -// asks the person. DESIGN.md section 7 says so. +// TestCoreGlobalNoticeLimit checks a "never ask" use when Notify refuses +// it because of the limit for all clients together. For the core, +// ErrNoticeLimit is a limit: it denies the use with cause limit, as for +// its own limit per client, without an error, and the use does not count +// for max_per_day. So with max_per_day = 1, the next use on the same day +// still goes without a tap once the limit has room, and the one after it +// asks the person. Before, the refused use was an approver failure, and +// it used up the one use of the day. DESIGN.md section 7 says so. (Issue +// #47.) func TestCoreGlobalNoticeLimit(t *testing.T) { f := newFake(t) e := newEnvFor(t, f) @@ -550,17 +553,35 @@ func TestCoreGlobalNoticeLimit(t *testing.T) { req := &approval.Request{Client: "ci", Kind: audit.KindSecrets, Secrets: []string{"backup-token"}, Claim: approval.Claim{Command: "restic backup /srv"}} r := core.Decide(context.Background(), req) - if r.Allowed || r.Cause != audit.CauseError || r.ErrorCode != audit.CodeApproverFailed || !errors.Is(r.Err, ErrNoticeLimit) { - t.Errorf("result %+v, want denied with approver_failed and ErrNoticeLimit", r) + if r.Allowed || r.Cause != audit.CauseLimit || r.ErrorCode != "" || r.Err != nil { + t.Errorf("result %+v, want denied with cause limit, and no error", r) } if n := len(f.callsOf("sendMessage")); n != 0 { t.Errorf("%d messages, want 0", n) } + if !strings.Contains(e.logs.String(), "too many notices") { + t.Error("the refused notice is not logged") + } + + // The limit has room again. The refused use did not count, so this use + // goes without a tap, and its notice goes out. (If it asked the person, + // the context would end it after waitLimit.) clock.add(2 * time.Minute) + actx, acancel := context.WithTimeout(context.Background(), waitLimit) + defer acancel() + r = core.Decide(actx, req) + if !r.Allowed || r.Answer != "" { + t.Fatalf("result %+v, want allowed without a tap", r) + } + if notice := f.waitCalls("sendMessage", 1)[0]; notice.num("chat_id") != noticeChatID || notice.has("reply_markup") { + t.Errorf("notice %s", notice.Raw) + } + + // That was the one use of the day: the next use asks the person. dctx, dcancel := context.WithCancel(context.Background()) done := make(chan *approval.Result, 1) go func() { done <- core.Decide(dctx, req) }() - asked := f.waitCalls("sendMessage", 1)[0] + asked := f.waitCalls("sendMessage", 2)[1] dcancel() <-done if asked.num("chat_id") != approverID || !asked.has("reply_markup") { diff --git a/internal/telegram/telegram.go b/internal/telegram/telegram.go index f7acadd..d0f64f0 100644 --- a/internal/telegram/telegram.go +++ b/internal/telegram/telegram.go @@ -75,8 +75,10 @@ var ( ErrPollConflict = errors.New("telegram: another program polls with the bot token, or a webhook is set; " + "the bot token may be stolen") // ErrNoticeLimit: Notify sent NoticesPerMinute notices in the last 60 - // seconds, so it sends no more. The core then denies the use. - ErrNoticeLimit = errors.New("telegram: too many notices in the last minute") + // seconds, so it sends no more. It wraps approval.ErrLimit, because + // nothing was sent: the core denies the use with cause limit, and the + // use does not count for max_per_day. + ErrNoticeLimit = fmt.Errorf("telegram: too many notices in the last minute: %w", approval.ErrLimit) ) // Options are the optional parts of an Approver. @@ -254,7 +256,9 @@ func (a *Approver) End(ctx context.Context, m approval.MessageID, p approval.Pro // Notify sends a silent notice of the "never ask" use p to the notice // chat. It sends nothing when ctx has ended, and nothing when it sent // NoticesPerMinute notices in the last 60 seconds: then it returns -// ErrNoticeLimit, and the core denies the use. +// ErrNoticeLimit, and the core denies the use with cause limit. Only +// ErrNoticeLimit is a limit for the core. Telegram's own code 429 after +// the last try is not: an earlier try may have sent the notice. func (a *Approver) Notify(ctx context.Context, p approval.Prompt) error { if err := ctx.Err(); err != nil { return err