diff --git a/modules/mesh-mail/cmd/mesh-mail/config.go b/modules/mesh-mail/cmd/mesh-mail/config.go index 4591f9f9..03c167d9 100644 --- a/modules/mesh-mail/cmd/mesh-mail/config.go +++ b/modules/mesh-mail/cmd/mesh-mail/config.go @@ -159,33 +159,17 @@ func (s Server) check(which string) error { return nil } -// SecretPath is where the account's own secret is placed in the secrets directory, as the manifest declares the -// family: /mail-credential-.secret. -func SecretPath(dir string, a Account) string { return filepath.Join(dir, a.Secret()+".secret") } - -// ReadSecret is the account's password, or given false when nobody gave it yet: the mesh never makes a member of the -// family, so a member not given is no file (ADR 0283 §6). -func ReadSecret(dir string, a Account) (string, bool, error) { - raw, err := os.ReadFile(SecretPath(dir, a)) - if errors.Is(err, os.ErrNotExist) { - return "", false, nil - } - if err != nil { - return "", false, err - } - pw := strings.TrimRight(string(raw), "\r\n") - if pw == "" { - return "", false, nil - } - return pw, true, nil -} +// ReadSecret and SecretPath moved into cmd/mesh-mail/internal/secret (novox/hq ADR 0436 §12, issue 1193): only +// keep (imap.go) and the sender's own actor (sendactor.go) read a password, never the tools, which the operator +// account's tool runner serves and which must never open a password file. // 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. +// A file that is there and not empty was given; whether it signs in is the keeper's to say. It computes the +// secret's path itself (not through the moved package) so the tools, which call this, never import it. func SecretGiven(dir string, a Account) bool { - info, err := os.Stat(SecretPath(dir, a)) + info, err := os.Stat(filepath.Join(dir, a.Secret()+".secret")) return err == nil && info.Mode().IsRegular() && info.Size() > 0 } diff --git a/modules/mesh-mail/cmd/mesh-mail/config_test.go b/modules/mesh-mail/cmd/mesh-mail/config_test.go index 21acbb3a..3f3560a5 100644 --- a/modules/mesh-mail/cmd/mesh-mail/config_test.go +++ b/modules/mesh-mail/cmd/mesh-mail/config_test.go @@ -6,6 +6,8 @@ import ( "reflect" "strings" "testing" + + "mesh-mail/cmd/mesh-mail/internal/secret" ) // The first stage's account, as the operator sets the setting `accounts` (ADR 0359 §5): its address and its server, @@ -22,7 +24,7 @@ func TestTheFirstStagesAccountIsReadAndItsSecretNamed(t *testing.T) { got[0].Incoming.Address() != "mail.novox.be:993" || got[0].Secret() != "mail-credential-mesh" || got[0].Outgoing != nil { t.Fatalf("%+v", got) } - if p := SecretPath("/state", got[0]); p != "/state/mail-credential-mesh.secret" { + if p := secret.Path("/state", got[0].Secret()); p != "/state/mail-credential-mesh.secret" { t.Fatalf("the secret is placed at %s", p) } } @@ -99,16 +101,16 @@ func TestASecretIsGivenByItsFileNotByReadingIt(t *testing.T) { if SecretGiven(dir, a) { t.Fatal("given before anything was written") } - _ = os.WriteFile(SecretPath(dir, a), nil, 0o600) + _ = os.WriteFile(secret.Path(dir, a.Secret()), nil, 0o600) if SecretGiven(dir, a) { t.Fatal("an empty file is a password") } - _ = os.WriteFile(SecretPath(dir, a), []byte("pw\n"), 0o600) - if pw, given, _ := ReadSecret(dir, a); !given || pw != "pw" || strings.Contains(pw, "\n") { + _ = os.WriteFile(secret.Path(dir, a.Secret()), []byte("pw\n"), 0o600) + if pw, given, _ := secret.Read(dir, a.Secret()); !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 { + if err := os.Chmod(secret.Path(dir, a.Secret()), 0); err != nil { t.Fatal(err) } if !SecretGiven(dir, a) { diff --git a/modules/mesh-mail/cmd/mesh-mail/imap.go b/modules/mesh-mail/cmd/mesh-mail/imap.go index 5488e535..caf66144 100644 --- a/modules/mesh-mail/cmd/mesh-mail/imap.go +++ b/modules/mesh-mail/cmd/mesh-mail/imap.go @@ -22,6 +22,8 @@ import ( "github.com/emersion/go-imap/v2" "github.com/emersion/go-imap/v2/imapclient" + + "mesh-mail/cmd/mesh-mail/internal/secret" ) // How the keeper paces itself. IdleCycle is how long one IDLE lasts before it is ended and begun again, which proves @@ -170,7 +172,7 @@ func (k *Keeper) Watch(ctx context.Context, a Account) { retry := orDefault(k.RetryFirst, RetryFirst) var heard, synced time.Time for ctx.Err() == nil { - pw, given, err := ReadSecret(k.SecretsDir, a) + pw, given, err := secret.Read(k.SecretsDir, a.Secret()) switch { case err != nil: k.say(ctx, a, AccountFailing, "its password was given and cannot be read: "+err.Error(), heard, synced) diff --git a/modules/mesh-mail/cmd/mesh-mail/internal/secret/secret.go b/modules/mesh-mail/cmd/mesh-mail/internal/secret/secret.go new file mode 100644 index 00000000..cfb2ef83 --- /dev/null +++ b/modules/mesh-mail/cmd/mesh-mail/internal/secret/secret.go @@ -0,0 +1,36 @@ +// Package secret reads an account's own password from the secrets directory (novox/hq ADR 0283 §6). It is +// imported only by mesh-mail's keeper (which signs in to IMAP as root) and its sender (which signs in to SMTP +// as its own account, mesh-mail-send): never by the tools, which the operator account's tool runner serves and +// which must never open a password file (novox/hq ADR 0436 §12, issue 1193). Moved out of package main's own +// files so a test (secret_boundary_test.go) can prove the tools' bundle (main.go's serve path, tools.go, +// send.go, health.go) never imports it. +package secret + +import ( + "errors" + "os" + "path/filepath" + "strings" +) + +// Path is where an account's own secret is placed in the secrets directory, given the family member name +// (mail-credential-): /.secret. +func Path(dir, member string) string { return filepath.Join(dir, member+".secret") } + +// Read is the member's password, or given false when nobody gave it yet: the mesh never makes a member of the +// family, so a member not given is no file (ADR 0283 §6). A directory that does not exist at all answers the +// same as a file that does not exist: given false, no error (novox/hq issue 1193, AC3). +func Read(dir, member string) (string, bool, error) { + raw, err := os.ReadFile(Path(dir, member)) + if errors.Is(err, os.ErrNotExist) { + return "", false, nil + } + if err != nil { + return "", false, err + } + pw := strings.TrimRight(string(raw), "\r\n") + if pw == "" { + return "", false, nil + } + return pw, true, nil +} diff --git a/modules/mesh-mail/cmd/mesh-mail/kit_test.go b/modules/mesh-mail/cmd/mesh-mail/kit_test.go index e7308661..e018e2b6 100644 --- a/modules/mesh-mail/cmd/mesh-mail/kit_test.go +++ b/modules/mesh-mail/cmd/mesh-mail/kit_test.go @@ -22,6 +22,8 @@ import ( "github.com/emersion/go-imap/v2/imapclient" "github.com/emersion/go-imap/v2/imapserver" "github.com/emersion/go-imap/v2/imapserver/imapmemserver" + + "mesh-mail/cmd/mesh-mail/internal/secret" ) func TestMain(m *testing.M) { @@ -134,7 +136,7 @@ func testAccount() Account { func giveSecret(t *testing.T, dir string, a Account, pw string) { t.Helper() - if err := os.WriteFile(SecretPath(dir, a), []byte(pw+"\n"), 0o600); err != nil { + if err := os.WriteFile(secret.Path(dir, a.Secret()), []byte(pw+"\n"), 0o600); err != nil { t.Fatal(err) } } diff --git a/modules/mesh-mail/cmd/mesh-mail/secret_boundary_test.go b/modules/mesh-mail/cmd/mesh-mail/secret_boundary_test.go new file mode 100644 index 00000000..539f330b --- /dev/null +++ b/modules/mesh-mail/cmd/mesh-mail/secret_boundary_test.go @@ -0,0 +1,120 @@ +package main + +// The tools never open a password file (novox/hq ADR 0436 §12, issue 1193): ReadSecret and SecretPath moved +// into cmd/mesh-mail/internal/secret, imported only by keep (imap.go) and the sender's own actor (sendactor.go, +// which — ahead of issue 1195's separate process — is the one piece of the Serve path still reached from +// Decide's "send" case, exactly as 1194's own report still finds it: "Decide/decideSend call SendActor +// directly"). The build seat runs every test under one account, so no permission test can show the split +// (context file, "Clause 12's proof"); this is the file-level proxy the lead chose instead: an AST check over +// the tools' own files. + +import ( + "encoding/json" + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// secretPackage is the import path the tools' bundle must never carry. +const secretPackage = "mesh-mail/cmd/mesh-mail/internal/secret" + +// 1193/AC1: none of the tools' bundle files (main.go's serve path, tools.go, send.go, health.go) imports the +// password-reading package; the failure names the offending file. +func TestTheToolsBundleNeverImportsThePasswordPackage(t *testing.T) { + for _, file := range []string{"main.go", "tools.go", "send.go", "health.go"} { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, file, nil, parser.ImportsOnly) + if err != nil { + t.Fatalf("%s: %v", file, err) + } + for _, imp := range f.Imports { + path := strings.Trim(imp.Path.Value, `"`) + if path == secretPackage { + t.Errorf("%s imports %s, which only keep and the sender's actor may (novox/hq ADR 0436 §12)", file, secretPackage) + } + } + } +} + +// 1193/AC2: keep (imap.go) and the sender's actor (sendactor.go) read the password through the moved package; +// SecretGiven (stat only) stays in config.go, available to the tools, and never reads the password itself. +func TestKeepAndTheSenderReadThePasswordThroughTheMovedPackage(t *testing.T) { + for _, file := range []string{"imap.go", "sendactor.go"} { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, file, nil, parser.ImportsOnly) + if err != nil { + t.Fatalf("%s: %v", file, err) + } + found := false + for _, imp := range f.Imports { + if strings.Trim(imp.Path.Value, `"`) == secretPackage { + found = true + } + } + if !found { + t.Errorf("%s does not import %s", file, secretPackage) + } + } + // SecretGiven itself (config.go) never calls ReadFile, os.Open or the moved package: whether a password was + // given is read from the file's presence and size alone (issue 733, kept by config_test.go's own check too). + src, err := os.ReadFile("config.go") + if err != nil { + t.Fatal(err) + } + body := string(src) + body = body[strings.Index(body, "func SecretGiven("):] + body = body[:strings.Index(body, "\n}\n")] + for _, bad := range []string{"ReadSecret", "ReadFile", "os.Open", "secret.Read"} { + if strings.Contains(body, bad) { + t.Fatalf("SecretGiven reads the password (%s):\n%s", bad, body) + } + } +} + +// 1193/AC3: with the secrets directory absent, mail_send raises its ask, the warrant of an Approve is heard, +// and mail_send_status reads it back — each succeeds, and no error mentions a secret file. (The outcome here +// is "failed: ... has no password yet", because the tools still perform the send through sendactor.go until +// issue 1194 replaces that path with queueing for the sender process; what this proves is narrower and holds +// either way: nothing on this path ever names a secret file or path.) +func TestTheToolPathSucceedsWithTheSecretsDirectoryAbsent(t *testing.T) { + dir := t.TempDir() + missing := filepath.Join(dir, "never-created") + env := Env{Accounts: filepath.Join(dir, "accounts.json"), Grants: filepath.Join(dir, "grants.json"), Secrets: missing} + writeFile(t, env.Accounts, `{"accounts": {"mesh": {"kind": "imap", "address": "jschoubben@novox.be", + "incoming": {"host": "mail.novox.be", "port": 993, "security": "tls"}, + "outgoing": {"host": "h", "port": 465, "security": "tls"}}}}`) + writeFile(t, env.Grants, `{"grants": {"mesh": ["triage"]}}`) + router, sends := newSendRouter(), newMemSends() + b := &Bundle{Env: env, Now: func() time.Time { return judgedAt }, Asks: router, Sends: sends} + rt := startBundle(t, b.Tools()) + + text, isErr := rt.call(SendTool, "novox.triage", sendArgs) + if isErr { + t.Fatalf("mail_send: %s", text) + } + var out map[string]any + _ = json.Unmarshal([]byte(text), &out) + id := out["ask"].(string) + + w := router.chose(id, OptionSend, judgedAt.Add(time.Minute)) + router.close(w) + d, err := Decide(sends, b.lookupAccount, b.sendActor(), w, judgedAt.Add(2*time.Minute)) + if err != nil { + t.Fatalf("the warrant was not heard cleanly: %v", err) + } + if strings.Contains(d.Outcome, missing) || strings.Contains(strings.ToLower(d.Outcome), "secret") { + t.Fatalf("the decision names a secret file: %q", d.Outcome) + } + + text, isErr = rt.call(SendStatusTool, "novox.triage", map[string]any{"id": id}) + if isErr { + t.Fatalf("mail_send_status: %s", text) + } + if strings.Contains(text, missing) || strings.Contains(strings.ToLower(text), "secret") { + t.Fatalf("mail_send_status names a secret file: %s", text) + } +} diff --git a/modules/mesh-mail/cmd/mesh-mail/sendactor.go b/modules/mesh-mail/cmd/mesh-mail/sendactor.go new file mode 100644 index 00000000..247b5d3c --- /dev/null +++ b/modules/mesh-mail/cmd/mesh-mail/sendactor.go @@ -0,0 +1,34 @@ +package main + +// The one piece of the Serve path that still reads a password (novox/hq ADR 0436 §12, issue 1193): moved out +// of tools.go so a test (secret_boundary_test.go) can prove the tools' own files — main.go's serve path, +// tools.go, send.go, health.go — never import the password-reading package. Decide still calls this directly +// (send.go's Heard and Sweep, tools.go's mail_send_status): issue 1194 replaces that with the tools queueing +// the approved send for the sender process instead, at which point this file's job moves to it (issue 1195) +// and this one is removed. + +import ( + "fmt" + "time" + + "mesh-mail/cmd/mesh-mail/internal/secret" +) + +// sendActor performs one send on the operator's Approve: it reads the account's own secret itself, so Decide and +// the `sends` state never hold a password. +func (b *Bundle) sendActor() SendActor { + dial := b.Dial + if dial == nil { + dial = DialSMTP + } + return func(a Account, to []string, subject, body string, now time.Time) error { + pw, given, err := secret.Read(b.Env.Secrets, a.Secret()) + if err != nil { + return fmt.Errorf("the account %s's password cannot be read: %w", a.Name, err) + } + if !given { + return fmt.Errorf("the account %s has no password yet; the operator gives it at the desk", a.Name) + } + return SendMail(dial, a, pw, to, subject, body, now) + } +} diff --git a/modules/mesh-mail/cmd/mesh-mail/tools.go b/modules/mesh-mail/cmd/mesh-mail/tools.go index 095ee3d4..fb5e3fe7 100644 --- a/modules/mesh-mail/cmd/mesh-mail/tools.go +++ b/modules/mesh-mail/cmd/mesh-mail/tools.go @@ -416,25 +416,6 @@ func accountByName(accounts []Account, name string) (Account, bool) { return Account{}, false } -// sendActor performs one send on the operator's Approve: it reads the account's own secret itself, so Decide and -// the `sends` state never hold a password. -func (b *Bundle) sendActor() SendActor { - dial := b.Dial - if dial == nil { - dial = DialSMTP - } - return func(a Account, to []string, subject, body string, now time.Time) error { - pw, given, err := ReadSecret(b.Env.Secrets, a) - if err != nil { - return fmt.Errorf("the account %s's password cannot be read: %w", a.Name, err) - } - if !given { - return fmt.Errorf("the account %s has no password yet; the operator gives it at the desk", a.Name) - } - return SendMail(dial, a, pw, to, subject, body, now) - } -} - func stringList(v any) []string { list, _ := v.([]any) out := make([]string, 0, len(list))