From be53f5167a39fdf6d4046c6a5fc64bd46e9b1394 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 14 Oct 2026 22:24:11 +0200 Subject: [PATCH] mesh-mail: bound a fetch by bytes, show mail_accounts only to its grantees, say why grants are unread, judge a secret without reading it (hq issue 733) The review of #300 found a batch of 25 whole messages could hold a gigabyte, mail_accounts told ungranted callers folder names, counts and server errors, an unreadable grants setting was refused without its reason, and the watch read the password only to learn it was there. --- modules/mesh-mail/cmd/mesh-mail/config.go | 12 ++-- .../mesh-mail/cmd/mesh-mail/config_test.go | 20 +++++- modules/mesh-mail/cmd/mesh-mail/imap.go | 70 +++++++++++++++++-- modules/mesh-mail/cmd/mesh-mail/imap_test.go | 53 ++++++++++++++ modules/mesh-mail/cmd/mesh-mail/tools.go | 63 ++++++++++++----- modules/mesh-mail/cmd/mesh-mail/tools_test.go | 34 ++++++++- 6 files changed, 216 insertions(+), 36 deletions(-) diff --git a/modules/mesh-mail/cmd/mesh-mail/config.go b/modules/mesh-mail/cmd/mesh-mail/config.go index e100dd62..4591f9f9 100644 --- a/modules/mesh-mail/cmd/mesh-mail/config.go +++ b/modules/mesh-mail/cmd/mesh-mail/config.go @@ -181,14 +181,12 @@ func ReadSecret(dir string, a Account) (string, bool, error) { } // SecretGiven says whether the account's own secret was given. +// +// It looks at the file and never reads it: whoever judges whether a password was given has no need of the password. +// A file that is there and not empty was given; whether it signs in is the keeper's to say. func SecretGiven(dir string, a Account) bool { - _, given, err := ReadSecret(dir, a) - if err != nil { - // A file there that cannot be read was given: the keeper says what is wrong with it. - _, statErr := os.Stat(SecretPath(dir, a)) - return statErr == nil - } - return given + info, err := os.Stat(SecretPath(dir, a)) + return err == nil && info.Mode().IsRegular() && info.Size() > 0 } // ---- grants ------------------------------------------------------------------------------------------------------ diff --git a/modules/mesh-mail/cmd/mesh-mail/config_test.go b/modules/mesh-mail/cmd/mesh-mail/config_test.go index c904d1a6..1fb38430 100644 --- a/modules/mesh-mail/cmd/mesh-mail/config_test.go +++ b/modules/mesh-mail/cmd/mesh-mail/config_test.go @@ -91,13 +91,15 @@ func TestGrantsAreByAccountAndByProvenCaller(t *testing.T) { } } -func TestASecretGivenEmptyIsNotGiven(t *testing.T) { +// Whether a password was given is read from the file's presence and size alone (issue 733): an empty file is not +// given, a file the judge cannot read is still given, and the password is never read to say so. +func TestASecretIsGivenByItsFileNotByReadingIt(t *testing.T) { dir := t.TempDir() a := testAccount() if SecretGiven(dir, a) { t.Fatal("given before anything was written") } - _ = os.WriteFile(SecretPath(dir, a), []byte("\n"), 0o600) + _ = os.WriteFile(SecretPath(dir, a), nil, 0o600) if SecretGiven(dir, a) { t.Fatal("an empty file is a password") } @@ -105,4 +107,18 @@ func TestASecretGivenEmptyIsNotGiven(t *testing.T) { if pw, given, _ := ReadSecret(dir, a); !given || pw != "pw" || strings.Contains(pw, "\n") { t.Fatalf("%q %v", pw, given) } + // Unreadable to the judge, as a secret owned by root is to the watch: still given. + if err := os.Chmod(SecretPath(dir, a), 0); err != nil { + t.Fatal(err) + } + if !SecretGiven(dir, a) { + t.Fatal("a password the judge cannot read is said not given") + } + src, _ := os.ReadFile("config.go") + body := string(src) + body = body[strings.Index(body, "func SecretGiven("):] + body = body[:strings.Index(body, "\n}\n")] + if strings.Contains(body, "ReadSecret") || strings.Contains(body, "ReadFile") || strings.Contains(body, "os.Open") { + t.Fatalf("SecretGiven reads the password:\n%s", body) + } } diff --git a/modules/mesh-mail/cmd/mesh-mail/imap.go b/modules/mesh-mail/cmd/mesh-mail/imap.go index ad68d969..5583781c 100644 --- a/modules/mesh-mail/cmd/mesh-mail/imap.go +++ b/modules/mesh-mail/cmd/mesh-mail/imap.go @@ -33,8 +33,48 @@ const ( RetryFirst = 30 * time.Second RetryMost = 5 * time.Minute FetchBatch = 25 + // BatchBytes bounds the mail one fetch of bodies holds; SizeBatch is how many sizes one fetch asks. + BatchBytes = 64 << 20 + SizeBatch = 500 ) +// Batches cuts the uids, in order, into batches of at most most messages whose sizes add up to at most limit bytes. A +// message larger than limit is a batch of its own. A size not known counts as nothing: the server listed the message, +// and the fetch says what it holds. +func Batches(uids []uint32, sizes map[uint32]int64, limit int64, most int) [][]uint32 { + var out [][]uint32 + var cur []uint32 + var bytes int64 + for _, u := range uids { + size := sizes[u] + if len(cur) > 0 && (len(cur) >= most || bytes+size > limit) { + out = append(out, cur) + cur, bytes = nil, 0 + } + cur = append(cur, u) + bytes += size + } + if len(cur) > 0 { + out = append(out, cur) + } + return out +} + +func uidSet(uids []uint32) imap.UIDSet { + set := make([]imap.UID, len(uids)) + for i, u := range uids { + set[i] = imap.UID(u) + } + return imap.UIDSetNum(set...) +} + +func orBytes(n, def int64) int64 { + if n > 0 { + return n + } + return def +} + // reader is the part of an IMAP client the keeper uses: nothing in it changes a mailbox. type reader interface { Login(username, password string) *imapclient.Command @@ -81,6 +121,8 @@ type Keeper struct { // Paces, for the tests; the constants above when zero. IdleCycle, PollEvery, RetryFirst time.Duration + // BatchBytes, for the tests; the constant when zero. + BatchBytes int64 } func (k *Keeper) now() time.Time { @@ -384,15 +426,29 @@ func (k *Keeper) syncFolder(ctx context.Context, c reader, a Account, folder str return err } } - // Added: on the server, not yet in the index; fetched whole, in batches, without marking them seen. + // Added: on the server, not yet in the index. Their sizes first, then their bodies in batches bounded by bytes as + // well as by count, so one batch never holds more than BatchBytes of mail in memory; a message larger than that + // is fetched alone, and said. Fetched without marking them seen. sort.Slice(added, func(i, j int) bool { return added[i] < added[j] }) - for start := 0; start < len(added); start += FetchBatch { - batch := added[start:min(start+FetchBatch, len(added))] - uids := make([]imap.UID, len(batch)) - for i, u := range batch { - uids[i] = imap.UID(u) + sizes := map[uint32]int64{} + for start := 0; start < len(added); start += SizeBatch { + part := added[start:min(start+SizeBatch, len(added))] + got, err := c.Fetch(uidSet(part), &imap.FetchOptions{UID: true, RFC822Size: true}).Collect() + if err != nil { + return fmt.Errorf("the folder %s's new messages cannot be measured: %v", folder, err) } - msgs, err := c.Fetch(imap.UIDSetNum(uids...), &imap.FetchOptions{ + answered() + for _, m := range got { + sizes[uint32(m.UID)] = m.RFC822Size + } + } + cap := orBytes(k.BatchBytes, BatchBytes) + for _, batch := range Batches(added, sizes, cap, FetchBatch) { + if len(batch) == 1 && sizes[batch[0]] > cap { + k.logf("[mesh-mail] the account %s's message %d in %s is %d bytes, more than a batch holds; it is fetched alone", + a.Name, batch[0], folder, sizes[batch[0]]) + } + msgs, err := c.Fetch(uidSet(batch), &imap.FetchOptions{ UID: true, Flags: true, Envelope: true, RFC822Size: true, InternalDate: true, BodySection: []*imap.FetchItemBodySection{{Peek: true}}, }).Collect() diff --git a/modules/mesh-mail/cmd/mesh-mail/imap_test.go b/modules/mesh-mail/cmd/mesh-mail/imap_test.go index db1e9b26..6a62105e 100644 --- a/modules/mesh-mail/cmd/mesh-mail/imap_test.go +++ b/modules/mesh-mail/cmd/mesh-mail/imap_test.go @@ -5,10 +5,12 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "fmt" "os" "path/filepath" "regexp" "strings" + "sync" "testing" "time" @@ -237,3 +239,54 @@ func TestNothingButTheTestsWritesToAMailbox(t *testing.T) { t.Fatal("folders are not opened read-only, or bodies not peeked") } } + +// A fetch of bodies is bounded by bytes (issue 733): batches cut by size and by count, and a message larger than a +// batch holds alone. +func TestBatchesAreBoundedByBytesAndALargerMessageGoesAlone(t *testing.T) { + sizes := map[uint32]int64{1: 40, 2: 40, 3: 500, 4: 10, 5: 10, 6: 10} + got := Batches([]uint32{1, 2, 3, 4, 5, 6}, sizes, 100, 2) + want := [][]uint32{{1, 2}, {3}, {4, 5}, {6}} + if fmt.Sprint(got) != fmt.Sprint(want) { + t.Fatalf("%v, not %v", got, want) + } + if got := Batches(nil, nil, 100, 25); len(got) != 0 { + t.Fatalf("%v", got) + } +} + +// Over a real conversation: with a cap of a few hundred bytes, a large mail is fetched alone and said so, and every +// mail is still indexed and said. +func TestTheKeeperFetchesUnderAByteCap(t *testing.T) { + s := newMailServer(t) + op := s.operator(t) + st := newMemStore() + secrets := t.TempDir() + giveSecret(t, secrets, testAccount(), testPassword) + var mu sync.Mutex + var said []string + k := &Keeper{Store: st, Objects: newMemObjects(), Dial: s.dialer(), SecretsDir: secrets, + Logf: func(f string, a ...any) { mu.Lock(); said = append(said, fmt.Sprintf(f, a...)); mu.Unlock() }, + IdleCycle: 300 * time.Millisecond, PollEvery: 400 * time.Millisecond, RetryFirst: 100 * time.Millisecond, BatchBytes: 600} + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { k.Watch(ctx, testAccount()); close(done) }() + t.Cleanup(func() { cancel(); <-done }) + eventually(t, 10*time.Second, "the first look", func() (bool, string) { return stateOf(st, "mesh").State == AccountCurrent, describe(st) }) + deliver(t, op, "INBOX", plainMail("small one", "short")) + deliver(t, op, "INBOX", plainMail("large one", strings.Repeat("a long letter ", 200))) + deliver(t, op, "INBOX", plainMail("small two", "short")) + eventually(t, 5*time.Second, "three mails said", func() (bool, string) { + return len(eventsNamed(st, "mail.mesh.message.added")) == 3, describe(st) + }) + mu.Lock() + defer mu.Unlock() + alone := 0 + for _, line := range said { + if strings.Contains(line, "fetched alone") { + alone++ + } + } + if alone != 1 { + t.Fatalf("said %d message(s) fetched alone: %v", alone, said) + } +} diff --git a/modules/mesh-mail/cmd/mesh-mail/tools.go b/modules/mesh-mail/cmd/mesh-mail/tools.go index 5344cc28..f1d57a09 100644 --- a/modules/mesh-mail/cmd/mesh-mail/tools.go +++ b/modules/mesh-mail/cmd/mesh-mail/tools.go @@ -46,27 +46,48 @@ func (b *Bundle) now() time.Time { return time.Now() } -func (b *Bundle) settings() ([]Account, Grants, error) { +// settings are the accounts and the grants. A grants setting that cannot be read grants nothing, and its reason is +// kept, so a refusal says why rather than only that the caller is not granted. +func (b *Bundle) settings() ([]Account, access, error) { accounts, err := ReadAccounts(b.Env.Accounts) if err != nil { - return nil, nil, err + return nil, access{}, err } grants, err := ReadGrants(b.Env.Grants) if err != nil { - // A grants setting that cannot be read grants nothing; the refusal says why. - return accounts, Grants{}, nil + return accounts, access{grants: Grants{}, unread: err}, nil } - return accounts, grants, nil + return accounts, access{grants: grants}, nil +} + +// access is the grants as read, and why they could not be, when they could not. +type access struct { + grants Grants + unread error +} + +func (a access) Allows(caller, account string) bool { return a.grants.Allows(caller, account) } + +func (a access) Granted(caller string, accounts []Account) []string { + return a.grants.Granted(caller, accounts) +} + +// refuse is the refusal of a caller for an account, saying why: the grants unread, or the caller not in them. +func (a access) refuse(caller, account string) error { + if a.unread != nil { + return fmt.Errorf("refused: the setting grants cannot be read, so it grants nobody: %v", a.unread) + } + return refused(caller, account) } // Tools are the module's tools. func (b *Bundle) Tools() []stdio.Tool { return []stdio.Tool{ {Name: AccountsTool, - Description: "The operator's mail accounts this module watches, each with its kind, its state (syncing, current, " + - "waits-for-sign-in or failing, and why), when its server last answered, when it was last brought up to date, and " + - "its folders with how many messages each holds. An account's mail address is said only to a caller granted that " + - "account; whether you are is said per account. Read-only.", + Description: "The operator's mail accounts you are granted, each with its kind, its address, its state (syncing, " + + "current, waits-for-sign-in or failing, and why), when its server last answered, when it was last brought up to " + + "date, and its folders with how many messages each holds. An account you are not granted is not listed, and " + + "nothing of it is said. How the module as a whole is, is mail_health's to say. Read-only.", Input: schema(nil, map[string]any{}), Run: b.accountsTool}, {Name: SearchTool, @@ -128,9 +149,13 @@ func (b *Bundle) accountsTool(args map[string]any) (any, error) { return nil, err } caller := b.caller(args) - var out []map[string]any + out := []map[string]any{} for _, a := range accounts { - entry := map[string]any{"account": a.Name, "kind": a.Kind, "granted": grants.Allows(caller, a.Name)} + // Only the accounts the caller is granted: of any other it learns nothing, not even that it exists. + if !grants.Allows(caller, a.Name) { + continue + } + entry := map[string]any{"account": a.Name, "kind": a.Kind, "address": a.Address} if s, ok := states[a.Name]; ok { entry["state"], entry["why"] = s.State, s.Why if !s.Heard.IsZero() { @@ -142,9 +167,6 @@ func (b *Bundle) accountsTool(args map[string]any) (any, error) { } else { entry["state"], entry["why"] = "unknown", "its keeper has said nothing of it yet" } - if grants.Allows(caller, a.Name) { - entry["address"] = a.Address - } folders, err := st.Folders(ctx, a.Name) if err != nil { return nil, err @@ -152,7 +174,11 @@ func (b *Bundle) accountsTool(args map[string]any) (any, error) { entry["folders"] = folders out = append(out, entry) } - return map[string]any{"accounts": out, "caller": caller}, nil + answer := map[string]any{"accounts": out, "caller": caller} + if grants.unread != nil { + answer["why"] = "the setting grants cannot be read, so it grants nobody: " + grants.unread.Error() + } + return answer, nil } func (b *Bundle) searchTool(args map[string]any) (any, error) { @@ -180,11 +206,14 @@ func (b *Bundle) searchTool(args map[string]any) (any, error) { granted := grants.Granted(caller, accounts) if account != "" { if !grants.Allows(caller, account) { - return nil, refused(caller, account) + return nil, grants.refuse(caller, account) } granted = []string{account} } if len(granted) == 0 { + if grants.unread != nil { + return nil, grants.refuse(caller, "") + } return nil, fmt.Errorf("refused: %s is granted no mail account; the operator grants one through the setting grants", caller) } ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) @@ -224,7 +253,7 @@ func (b *Bundle) readTool(args map[string]any) (any, error) { } caller := b.caller(args) if !grants.Allows(caller, account) { - return nil, refused(caller, account) + return nil, grants.refuse(caller, account) } ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) defer cancel() diff --git a/modules/mesh-mail/cmd/mesh-mail/tools_test.go b/modules/mesh-mail/cmd/mesh-mail/tools_test.go index 7f7a61c0..fbc9c6c7 100644 --- a/modules/mesh-mail/cmd/mesh-mail/tools_test.go +++ b/modules/mesh-mail/cmd/mesh-mail/tools_test.go @@ -193,15 +193,24 @@ func TestTheBodyToolsAnswerAGrantedCallerAndRefuseAnyOther(t *testing.T) { } // mail_accounts says each account's state to anyone, and its address only to a caller granted it. -func TestTheAccountsToolSaysTheAddressOnlyToAGrantedCaller(t *testing.T) { +func TestTheAccountsToolListsOnlyTheAccountsTheCallerIsGranted(t *testing.T) { b, _, _ := aBundle(t) rt := startBundle(t, b.Tools()) + _ = b.Env // the index holds a folder of the account, so an ungranted answer has something to leak + st, _ := b.Index(context.Background()) + _ = st.SetFolder(context.Background(), Folder{Account: "mesh", Name: "Secret-Project", Messages: 41, Synced: judgedAt}) + _ = st.SetState(context.Background(), AccountState{Account: "mesh", State: AccountFailing, Why: "the server said NO [AUTHENTICATIONFAILED]", Said: judgedAt}) text, isErr := rt.call(AccountsTool, "novox.other", map[string]any{}) - if isErr || !strings.Contains(text, `"state":"current"`) || strings.Contains(text, "jschoubben@") || !strings.Contains(text, `"granted":false`) { + if isErr || !strings.Contains(text, `"accounts":[]`) { t.Fatalf("ungranted: %s", text) } + for _, leak := range []string{"mesh\"", "jschoubben@", "Secret-Project", "41", "AUTHENTICATIONFAILED", "failing"} { + if strings.Contains(text, leak) { + t.Fatalf("an ungranted caller learns %q: %s", leak, text) + } + } text, _ = rt.call(AccountsTool, "novox.triage", map[string]any{}) - if !strings.Contains(text, `"address":"jschoubben@novox.be"`) || !strings.Contains(text, `"granted":true`) { + if !strings.Contains(text, `"address":"jschoubben@novox.be"`) || !strings.Contains(text, "Secret-Project") || !strings.Contains(text, "AUTHENTICATIONFAILED") { t.Fatalf("granted: %s", text) } } @@ -245,3 +254,22 @@ func TestTheEventsAreSaidOnTheBusThroughTheRuntime(t *testing.T) { t.Fatalf("published %s", raw) } } + +// A grants setting that cannot be read grants nobody, and every refusal says why (issue 733). +func TestARefusalSaysWhyTheGrantsCouldNotBeRead(t *testing.T) { + b, _, _ := aBundle(t) + writeFile(t, b.Env.Grants, `{"grants": {"mesh": "triage"}}`) + rt := startBundle(t, b.Tools()) + text, isErr := rt.call(ReadTool, "novox.triage", map[string]any{"account": "mesh", "folder": "INBOX", "uid": 7}) + if !isErr || !strings.Contains(text, "the setting grants cannot be read") || !strings.Contains(text, "not the module's") || strings.Contains(text, "Please find") { + t.Fatalf("read: %v %s", isErr, text) + } + text, isErr = rt.call(SearchTool, "novox.triage", map[string]any{"query": "invoice"}) + if !isErr || !strings.Contains(text, "the setting grants cannot be read") { + t.Fatalf("search: %v %s", isErr, text) + } + text, _ = rt.call(AccountsTool, "novox.triage", map[string]any{}) + if !strings.Contains(text, "the setting grants cannot be read") || !strings.Contains(text, `"accounts":[]`) { + t.Fatalf("accounts: %s", text) + } +} -- 2.54.0