Read a context's store connection from a file, not only from the environment

A store connection string carries a password, and the control plane took it from MESH_STORE_<CONTEXT> — 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_<CONTEXT>_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
2026-09-10 23:40:37 +02:00
parent c6029504e4
commit 522d8be925
3 changed files with 309 additions and 15 deletions
@@ -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("<context>")+`. This process holds:
+`+store.Variable("<context>")+` — or `+store.FileVariable("<context>")+`, naming a file that holds
+the same thing and keeps the password out of the environment. This process holds:

 `)
 	for _, c := range held {
@@ -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)
+	}
+}
@@ -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 }