From 522d8be9253f82040a257eff6cf16deb1f079ac9 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 10 Sep 2026 23:40:37 +0200 Subject: [PATCH] Read a context's store connection from a file, not only from the environment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A store connection string carries a password, and the control plane took it from MESH_STORE_ — an environment variable, which is readable in `docker inspect`, in the process's own /proc entry, and in whatever composed it. Every other module in the catalogue is given secret material as a file the mesh sealed to the machine and the host wrote. That difference is what stopped the control plane from being an ordinary module (novox/hq ADR 0067). A manifest can put a sealed value into a file's `content` with ${secret:…}; it has no substitution into a container's `env` at all. So a control-plane manifest could be written with the password in it, or without the setting — neither honest. The fix is not to change the manifest format but to let the control plane read what everything else reads: a file. MESH_STORE__FILE names one. Exactly one of the two may be set; both is refused rather than settled by precedence, because whichever won, the other would still read as the setting in force and the process would be writing to a store nobody expects. Trailing whitespace is trimmed — a file written by a person or by a filled-in placeholder ends in a newline, and a newline inside a URL is rejected several layers from anything that could explain it. Leading whitespace is left, being a mangled value rather than a habit. The no-leak property is kept and extended: a file that cannot be read names its path, never its contents, and the parse failure now names whichever source was used because a variable name and a path are not the secret. Claude-Session: https://claude.ai/code/session_01LrgweAeERJYBg88c5cKDzF --- cmd/mesh-control/main.go | 3 +- internal/store/settings_test.go | 215 ++++++++++++++++++++++++++++++++ internal/store/store.go | 106 +++++++++++++--- 3 files changed, 309 insertions(+), 15 deletions(-) create mode 100644 internal/store/settings_test.go diff --git a/cmd/mesh-control/main.go b/cmd/mesh-control/main.go index d40d62b..2c8ef08 100644 --- a/cmd/mesh-control/main.go +++ b/cmd/mesh-control/main.go @@ -165,7 +165,8 @@ func usage() { version what this binary is Each context reaches its own store through its own credential (novox/hq ADR 0008), named -`+store.Variable("")+`. This process holds: +`+store.Variable("")+` — or `+store.FileVariable("")+`, naming a file that holds +the same thing and keeps the password out of the environment. This process holds: `) for _, c := range held { diff --git a/internal/store/settings_test.go b/internal/store/settings_test.go new file mode 100644 index 0000000..ece30c5 --- /dev/null +++ b/internal/store/settings_test.go @@ -0,0 +1,215 @@ +package store + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/jackc/pgx/v5/pgxpool" +) + +// Where a context's connection settings come from, and what happens when that is unclear. +// +// No database is needed for any of this: pgxpool connects lazily, so `Open` succeeding proves that +// a string was found and parsed, which is exactly what is under test here. The live tests next door +// prove the rest. + +// example is a context name these tests can own outright. Short, and matching `contextName`, which +// allows no dashes. +const example = "example" + +// dsn is a connection string shaped like a real one, password and all — because the property most +// of these tests assert is that this never appears in an error. +const dsn = "postgres://postgres:s3cret-in-here@127.0.0.1:5432/example?sslmode=disable" + +// alone clears both variables for a context, so a test is not passing or failing on what the +// machine running it happens to export. +func alone(t *testing.T) { + t.Helper() + t.Setenv(Variable(example), "") + t.Setenv(FileVariable(example), "") +} + +func TestTheFileVariableIsTheVariablePlusFile(t *testing.T) { + if got := FileVariable("inventory"); got != "MESH_STORE_INVENTORY_FILE" { + t.Errorf("the file variable is %q", got) + } +} + +func TestTheConnectionMayComeFromAFile(t *testing.T) { + alone(t) + path := filepath.Join(t.TempDir(), "inventory") + if err := os.WriteFile(path, []byte(dsn), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv(FileVariable(example), path) + + held, from, err := settingsFor(example) + if err != nil { + t.Fatalf("a file holding the settings was refused: %v", err) + } + if held != dsn { + t.Errorf("the settings came back as %q", held) + } + if !strings.Contains(from, FileVariable(example)) || !strings.Contains(from, path) { + t.Errorf("the source is reported as %q, which names neither the variable nor the file", from) + } + + // And the whole way through, so this is not a test of a helper nobody calls. + opened, err := Open(t.Context(), example) + if err != nil { + t.Fatalf("the store would not open from a file: %v", err) + } + opened.Close() +} + +func TestATrailingNewlineInTheFileIsTolerated(t *testing.T) { + alone(t) + path := filepath.Join(t.TempDir(), "inventory") + // What a person's editor writes, and what the host writes when it fills a ${secret:…} + // placeholder into a file whose content ended in one. Untrimmed it is a control character + // inside a URL, which is refused far from anything that could explain it. + if err := os.WriteFile(path, []byte(dsn+"\n"), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv(FileVariable(example), path) + + held, _, err := settingsFor(example) + if err != nil { + t.Fatalf("a file ending in a newline was refused: %v", err) + } + if held != dsn { + t.Errorf("the newline survived: %q", held) + } + if _, err := pgxpool.ParseConfig(held); err != nil { + t.Errorf("what came out of the file does not parse: %v", err) + } +} + +func TestBothTheVariableAndTheFileIsRefused(t *testing.T) { + alone(t) + path := filepath.Join(t.TempDir(), "inventory") + if err := os.WriteFile(path, []byte(dsn), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv(Variable(example), dsn) + t.Setenv(FileVariable(example), path) + + _, _, err := settingsFor(example) + if err == nil { + t.Fatal("both were set and one of them was silently chosen") + } + for _, want := range []string{Variable(example), FileVariable(example)} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the refusal does not name %s: %v", want, err) + } + } + mustNotLeak(t, err) + + // Open refuses for the same reason rather than reaching a database. + if _, err := Open(t.Context(), example); err == nil { + t.Fatal("Open accepted both settings at once") + } +} + +func TestAFileThatCannotBeReadIsRefusedByPath(t *testing.T) { + alone(t) + path := filepath.Join(t.TempDir(), "never-written") + t.Setenv(FileVariable(example), path) + + _, _, err := settingsFor(example) + if err == nil { + t.Fatal("a missing settings file was accepted") + } + if !strings.Contains(err.Error(), path) { + t.Errorf("the refusal does not say which file: %v", err) + } + mustNotLeak(t, err) +} + +func TestAnEmptyFileIsRefusedByPath(t *testing.T) { + alone(t) + path := filepath.Join(t.TempDir(), "inventory") + // A placeholder that was never filled in looks exactly like this on the machine. + if err := os.WriteFile(path, []byte("\n"), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv(FileVariable(example), path) + + _, _, err := settingsFor(example) + if err == nil { + t.Fatal("an empty settings file was accepted") + } + if !strings.Contains(err.Error(), path) { + t.Errorf("the refusal does not say which file: %v", err) + } +} + +func TestNeitherIsRefusedNamingBoth(t *testing.T) { + alone(t) + _, _, err := settingsFor(example) + if err == nil { + t.Fatal("a context with no settings at all was accepted") + } + for _, want := range []string{Variable(example), FileVariable(example)} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the refusal does not name %s: %v", want, err) + } + } +} + +func TestTheEnvironmentVariableStillWorksUnchanged(t *testing.T) { + alone(t) + t.Setenv(Variable(example), dsn) + + held, from, err := settingsFor(example) + if err != nil { + t.Fatalf("the variable that has always worked was refused: %v", err) + } + if held != dsn { + t.Errorf("the settings came back as %q", held) + } + if from != Variable(example) { + t.Errorf("the source is reported as %q", from) + } + + opened, err := Open(t.Context(), example) + if err != nil { + t.Fatalf("the store would not open from the variable: %v", err) + } + opened.Close() +} + +// The no-leak property, at the one place a file makes it easy to lose: a value that will not parse +// is usually a password with something wrong around it, and the error goes into a log. +func TestASettingsFileThatCannotBeParsedIsNotQuotedBack(t *testing.T) { + alone(t) + path := filepath.Join(t.TempDir(), "inventory") + const mangled = "postgres://postgres:s3cret-in-here@ 127.0.0.1:5432/example" + if err := os.WriteFile(path, []byte(mangled), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv(FileVariable(example), path) + + opened, err := Open(t.Context(), example) + if err == nil { + opened.Close() + t.Fatal("a mangled connection string was accepted") + } + if strings.Contains(err.Error(), "s3cret-in-here") { + t.Fatalf("the password is in the error: %v", err) + } + // It still says enough to be actionable: which variable, and which file. + if !strings.Contains(err.Error(), FileVariable(example)) || + !strings.Contains(err.Error(), path) { + t.Errorf("the refusal says neither where to look nor which file: %v", err) + } +} + +func mustNotLeak(t *testing.T, err error) { + t.Helper() + if strings.Contains(err.Error(), "s3cret-in-here") { + t.Fatalf("the password is in the error: %v", err) + } +} diff --git a/internal/store/store.go b/internal/store/store.go index 75e86cc..9d8ab7b 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -5,13 +5,14 @@ // makes it a rule about credentials rather than a rule about intentions. // // There is no mesh-wide connection string and no way to ask for one. A store is opened by naming -// a context, and the settings for that context come from an environment variable named after it. -// A control plane process that has been granted `inventory` holds MESH_STORE_INVENTORY and -// nothing else, so reaching another context's store is not a matter of restraint — the process -// has no address for it and no credential to present. +// a context, and the settings for that context come from an environment variable named after it, +// or from a file that variable's `_FILE` twin points at. A control plane process that has been +// granted `inventory` holds MESH_STORE_INVENTORY or MESH_STORE_INVENTORY_FILE and nothing else, +// so reaching another context's store is not a matter of restraint — the process has no address +// for it and no credential to present. // // Which is also how the rule is *checked*: what a context can reach is visible in the -// declaration that runs it, as the list of variables it was given. +// declaration that runs it, as the list of variables and files it was given. package store import ( @@ -47,6 +48,25 @@ func Variable(context string) string { return "MESH_STORE_" + strings.ToUpper(context) } +// FileVariable is the environment variable naming a FILE that holds a context's connection +// settings — the same value, kept out of the environment. +// +// **Because the connection string carries a password, and an environment variable is a poor place +// to keep one.** It is readable in `docker inspect`, in the process's own /proc entry, and in +// whatever composed it. A file is not: the mesh seals the value to the machine, the host writes it +// with a mode of its own, and nothing in between ever holds the plaintext (novox/hq ADR 0024). +// That is already how every other module in the catalogue is given secret material. +// +// It is also what lets the control plane be described by an ordinary module manifest at all +// (novox/hq ADR 0067). A manifest can put a sealed value into a *file's* content; it cannot put one +// into a container's environment, so a manifest for a process that reads its store connection from +// the environment could not be written honestly — only with the password in it, or without the +// setting at all. +// +// The plain variable remains, for a control plane a person starts by hand and for the bundle that +// raises the first one. +func FileVariable(context string) string { return Variable(context) + "_FILE" } + // Database is what a context's database is called. // // Named after the context, so that a person looking at a PostgreSQL server can see which @@ -65,23 +85,20 @@ func Open(ctx context.Context, name string) (*Store, error) { "name, so it must be lower-case letters and digits, starting with a letter", name) } - dsn := os.Getenv(Variable(name)) - if strings.TrimSpace(dsn) == "" { - return nil, fmt.Errorf( - "this process has no %s, so it was not granted the %s store. A context reaches only "+ - "the store it exclusively owns (novox/hq ADR 0008), so this is either the wrong "+ - "context or a missing grant — it is never something to work around by reusing "+ - "another context's connection", Variable(name), name) + dsn, from, err := settingsFor(name) + if err != nil { + return nil, err } config, err := pgxpool.ParseConfig(dsn) if err != nil { // Deliberately not wrapping the driver's error verbatim into a message that gets logged: // a malformed DSN often *is* the password, and the value is the one thing here that must - // not be quoted back. + // not be quoted back. Where it came from can be said, because a variable name and a path + // are not the secret. return nil, fmt.Errorf( "the connection settings in %s could not be read; the value is not quoted here "+ - "because it carries a password", Variable(name)) + "because it carries a password", from) } pool, err := pgxpool.NewWithConfig(ctx, config) @@ -91,6 +108,67 @@ func Open(ctx context.Context, name string) (*Store, error) { return &Store{context: name, pool: pool}, nil } +// settingsFor is a context's connection string, and the name of where it came from. +// +// **Where it came from is returned; what it says never is.** A variable name and a path are safe +// to put in a log and are exactly what somebody fixing this needs. The value is a password with a +// URL around it, and every error below is written on the assumption that it will be logged. +// +// A blank variable counts as unset, here as it always has: a container given an empty string was +// given nothing, and refusing it as *ambiguous* would turn an omission into a puzzle. +func settingsFor(name string) (dsn, from string, err error) { + direct := strings.TrimSpace(os.Getenv(Variable(name))) + path := strings.TrimSpace(os.Getenv(FileVariable(name))) + + if direct != "" && path != "" { + // **Refused rather than settled by precedence.** The two can name different databases, and + // whichever one won, the other would still read as the setting in force — so the process + // would be writing to a store nobody reading its configuration expects, and every check + // would pass. Which one is meant is not a question this can answer, and answering it wrongly + // is worse than stopping. + return "", "", fmt.Errorf( + "this process has both %s and %s, and they may name different databases. Exactly one "+ + "of them says where the %s store's connection settings come from — unset whichever "+ + "is not the one you meant; picking one here would leave the other looking like the "+ + "one in force", + Variable(name), FileVariable(name), name) + } + + if path != "" { + raw, err := os.ReadFile(path) + if err != nil { + // The path may be named; what is in it may not — and a file that could not be read has + // disclosed nothing, so there is nothing here to withhold. + return "", "", fmt.Errorf( + "%s names %s as where the %s store's connection settings are, and it cannot be "+ + "read: %w", FileVariable(name), path, name, err) + } + // Trailing whitespace goes; leading whitespace stays. A file written by a person, or by the + // host filling a ${secret:…} placeholder into one, ordinarily ends in a newline, and a + // newline inside a URL is rejected several layers from anything that could explain it. + // Nothing produces a leading space by accident, so one is a mangled value rather than a + // formatting habit, and quietly repairing it would hide that. + held := strings.TrimRight(string(raw), " \t\r\n") + if held == "" { + return "", "", fmt.Errorf( + "%s names %s as where the %s store's connection settings are, and that file holds "+ + "nothing. Write the connection string into it, or unset %s and set %s instead", + FileVariable(name), path, name, FileVariable(name), Variable(name)) + } + return held, fmt.Sprintf("%s (%s)", FileVariable(name), path), nil + } + + if direct == "" { + return "", "", fmt.Errorf( + "this process has neither %s nor %s, so it was not granted the %s store. A context "+ + "reaches only the store it exclusively owns (novox/hq ADR 0008), so this is either "+ + "the wrong context or a missing grant — it is never something to work around by "+ + "reusing another context's connection", + Variable(name), FileVariable(name), name) + } + return direct, Variable(name), nil +} + // Context is which context this store belongs to. func (s *Store) Context() string { return s.context }