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) + } +}