Merge pull request 'A secret reaches a process as a file (ADR 0086, closes issue 041)' (#35) from feat/secret-not-in-environment into main

internal/broker/broker.go

@@ -16,6 +16,7 @@ import (
 	"encoding/pem"
 	"errors"
 	"fmt"
+	"github.com/novox/mesh-controller/internal/envfile"
 	"os"
 	"strings"
 )
@@ -40,7 +41,10 @@ var ErrNotConfigured = errors.New("this control plane has not been told about it
 
 // FromEnvironment reads the two settings, if they are there.
 func FromEnvironment() (Broker, error) {
-	address := strings.TrimSpace(os.Getenv(AddressVar))
+	address, err := envfile.Value(AddressVar)
+	if err != nil {
+		return Broker{}, err
+	}
 	path := strings.TrimSpace(os.Getenv(CertificateVar))
 
 	if address == "" && path == "" {

internal/broker/management.go

@@ -5,10 +5,10 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"github.com/novox/mesh-controller/internal/envfile"
 	"io"
 	"net/http"
 	"net/url"
-	"os"
 	"regexp"
 	"strings"
 	"time"
@@ -42,7 +42,10 @@ type Management struct {
 
 // ManagementFromEnvironment reads where the management API is, if it is configured.
 func ManagementFromEnvironment() (*Management, error) {
-	raw := strings.TrimSpace(os.Getenv(ManagementVar))
+	raw, err := envfile.Value(ManagementVar)
+	if err != nil {
+		return nil, err
+	}
 	if raw == "" {
 		return nil, ErrNotConfigured
 	}

internal/catalogue/declaration.go

@@ -271,9 +271,9 @@ func (r Resolution) Declaration(with Rendering) ([]map[string]any, error) {
 				return nil, fmt.Errorf(
 					"%s needs a secret called %q and none was made for it", m.Module, name)
 			}
-			first = append(first, map[string]any{
+			first = append(first, ownedBy(m.SecretsOwner, map[string]any{
 				"id": NeedID(name), "type": "file", "path": m.OwnSecrets[name], "sealed": sealed,
-			})
+			}))
 		}
 		// Operator-owned paths this module is granted use of (novox/hq ADR 0051). Written before
 		// the module's own resources, and so before the container that mounts them: the host must
@@ -320,10 +320,10 @@ func (r Resolution) Declaration(with Rendering) ([]map[string]any, error) {
 				// worse than none: something would read it and fail authenticating.
 				continue
 			}
-			first = append(first, map[string]any{
+			first = append(first, ownedBy(m.SecretsOwner, map[string]any{
 				"id": SecretID(to), "type": "file", "path": m.Secrets[to],
 				"sealed": found.Sealed,
-			})
+			}))
 		}
 		for _, to := range sortedKeys(m.Grants) {
 			for _, g := range with.Grants {
@@ -442,6 +442,10 @@ func (r Resolution) Declaration(with Rendering) ([]map[string]any, error) {
 		// And the machine underneath, which no binding of its own can tell it.
 		thisMachine := machineFacts(r)
 
+		// Which of this module's files carry a secret, for the rule that a container may not read
+		// one of them as its environment without saying so (ADR 0086, issue 041).
+		secretFiles := secretFilesOf(resources)
+
 		for _, unsettled := range resources {
 			resource, err := ApplySettings(unsettled, with.Settings[m.Module])
 			if err != nil {
@@ -451,6 +455,12 @@ func (r Resolution) Declaration(with Rendering) ([]map[string]any, error) {
 			for k, v := range resource {
 				copied[k] = v
 			}
+			if err := refuseSecretsInEnvironment(copied, secretFiles, m.Module); err != nil {
+				return nil, err
+			}
+			// Said in the catalogue, not on the machine: the host parses strictly and knows no
+			// such field, and the reason is for a reader of the manifest.
+			delete(copied, SecretsInEnvironment)
 			// **After settings, and that is the whole reason it is here.** A module's file
 			// content is where a setting lands, so a placeholder may only exist once the setting
 			// has been put in — filling secrets first would look at content that is not yet what
@@ -789,6 +799,14 @@ func keptFile(dir string, kept *KeptExport) (map[string]any, error) {
 	}, nil
 }
 
+// ownedBy gives a secret file the owner the module named, when it named one (Manifest.SecretsOwner).
+func ownedBy(owner string, file map[string]any) map[string]any {
+	if owner != "" {
+		file["owner"] = owner
+	}
+	return file
+}
+
 // sortedKeys is map iteration made repeatable, which everything written to a machine needs.
 func sortedKeys[V any](m map[string]V) []string {
 	out := make([]string, 0, len(m))

internal/catalogue/manifest.go

@@ -297,6 +297,18 @@ type Manifest struct {
 	// module, in a file anybody can read, for ever.
 	OwnSecrets map[string]string `json:"own-secrets,omitempty"`
 
+	// SecretsOwner is who the files holding this module's secrets belong to on the machine —
+	// `uid:gid`, or a name — when its process is not root.
+	//
+	// **A secret reaches a process as a file** (novox/hq ADR 0086), and a file the host writes at
+	// 0600 as root is a file a container running as another account cannot read: the control
+	// plane, `USER 65534` in a scratch image, crash-looped on `permission denied` the first time
+	// its credentials were mounted instead of read from an env-file (which the daemon reads, as
+	// root, on the host side — which is exactly why that shape hid the problem). Absent means
+	// root, which is what a process that runs as root needs and what a process that does not
+	// cannot use.
+	SecretsOwner string `json:"secrets-owner,omitempty"`
+
 	// Keeps is where this module wants every operator-sealed secret in the mesh written — the
 	// vault's field, and so far nobody else's (novox/hq ADR 0085, amended).
 	//

internal/catalogue/secrets_in_environment_test.go

@@ -0,0 +1,103 @@
+package catalogue
+
+import (
+	"strings"
+	"testing"
+)
+
+func aModuleWithAnEnvFileSecret(exception string) Manifest {
+	container := map[string]any{
+		"id": "server", "type": "container", "name": "app", "image": "app@sha256:" + strings.Repeat("a", 64),
+		"env-file": []any{"/var/lib/app/server.env"},
+	}
+	if exception != "" {
+		container[SecretsInEnvironment] = exception
+	}
+	return Manifest{
+		Module: "app", Version: "1",
+		OwnSecrets: map[string]string{"token": "/var/lib/app/token.secret"},
+		Resources: []map[string]any{
+			{"id": "env", "type": "file", "path": "/var/lib/app/server.env", "mode": "0600",
+				"content": "APP_TOKEN=${secret:token}\n"},
+			container,
+		},
+	}
+}
+
+func declare(t *testing.T, m Manifest) ([]map[string]any, error) {
+	t.Helper()
+	got, err := Resolve(shelf(m), []string{m.Module},
+		Node{Name: "anchor", At: "10.0.0.1", Capabilities: map[string]bool{}}, World{})
+	if err != nil {
+		t.Fatal(err)
+	}
+	return got.Declaration(Rendering{Needed: map[string]map[string]string{"app": {"token": "SEALED"}}})
+}
+
+// A container that reads a file carrying a secret as its environment is refused, unless it says
+// why — and then the reason stays in the catalogue and never reaches the machine.
+func TestASecretInAnEnvFileIsRefusedUnlessDeclared(t *testing.T) {
+	_, err := declare(t, aModuleWithAnEnvFileSecret(""))
+	if err == nil || !strings.Contains(err.Error(), "04-ISSUES/041") {
+		t.Fatalf("an env-file carrying a secret was declared without a word: %v", err)
+	}
+
+	out, err := declare(t, aModuleWithAnEnvFileSecret("the image reads its configuration from the environment only"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, r := range out {
+		if _, leaked := r[SecretsInEnvironment]; leaked {
+			t.Fatalf("the catalogue's reason was sent to the machine: %v", r)
+		}
+	}
+}
+
+// A secret placeholder inside a container's env is refused outright: nothing fills it there.
+func TestASecretInEnvIsAlwaysRefused(t *testing.T) {
+	m := aModuleWithAnEnvFileSecret("said")
+	m.Resources[1]["env"] = map[string]any{"APP_TOKEN": "${secret:token}"}
+	if _, err := declare(t, m); err == nil || !strings.Contains(err.Error(), "never filled into an environment variable") {
+		t.Fatalf("a secret in env was accepted: %v", err)
+	}
+}
+
+// A file with no secret in it is an ordinary env-file and needs no word.
+func TestAnEnvFileWithoutASecretNeedsNothing(t *testing.T) {
+	m := aModuleWithAnEnvFileSecret("")
+	m.Resources[0]["content"] = "APP_MODE=production\n"
+	if _, err := declare(t, m); err != nil {
+		t.Fatal(err)
+	}
+}
+
+// A module whose process is not root names who its secret files belong to, and every secret file
+// the composer writes for it carries that owner — the mounted file is readable where it is used.
+func TestSecretFilesCarryTheOwnerTheModuleNamed(t *testing.T) {
+	m := aModuleWithAnEnvFileSecret("said")
+	m.SecretsOwner = "65534:65534"
+	out, err := declare(t, m)
+	if err != nil {
+		t.Fatal(err)
+	}
+	var seen bool
+	for _, r := range out {
+		if r["path"] != "/var/lib/app/token.secret" {
+			continue
+		}
+		seen = true
+		if r["owner"] != "65534:65534" {
+			t.Errorf("the secret file is owned by %v", r["owner"])
+		}
+	}
+	if !seen {
+		t.Fatal("no secret file in the declaration")
+	}
+	m.SecretsOwner = ""
+	out, _ = declare(t, m)
+	for _, r := range out {
+		if r["path"] == "/var/lib/app/token.secret" && r["owner"] != nil {
+			t.Errorf("an owner was invented: %v", r["owner"])
+		}
+	}
+}

internal/catalogue/secrets_into_files.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"regexp"
 	"sort"
+	"strings"
 )
 
 // A credential and a configuration file meeting.
@@ -83,6 +84,70 @@ func sealedFor(m Manifest, needs []Needed, with Rendering) (map[string]string, e
 	return sealed, nil
 }
 
+// SecretsInEnvironment is the key a container carries to say, out loud, that a secret reaches
+// its process through the environment — and why.
+//
+// **A secret reaches a process as a file** (novox/hq ADR 0086). The mesh seals a value to the
+// machine and the host writes it at 0600; a container that then reads it from an env-file hands
+// it to the runtime, which prints it in `docker inspect` and keeps it in the process's /proc entry
+// for anything on the machine that can talk to the runtime (novox/hq 04-ISSUES/041). Some software
+// reads its configuration from the environment and nothing else, and for that this key exists: a
+// reason, on the container, so a reader can tell from the manifest which secrets are exposed that
+// way and which are not. Without it, a container whose env-file carries a secret is refused.
+//
+// Catalogue-level: the host never sees this key.
+const SecretsInEnvironment = "secrets-in-environment"
+
+// refuseSecretsInEnvironment is the check. `secretFiles` is every file of this module whose
+// content names a secret.
+func refuseSecretsInEnvironment(resource map[string]any, secretFiles map[string]bool, module string) error {
+	if fmt.Sprint(resource["type"]) != "container" {
+		return nil
+	}
+	name := fmt.Sprint(resource["name"])
+	if env, ok := resource["env"].(map[string]any); ok {
+		for key, value := range env {
+			if strings.Contains(fmt.Sprint(value), "${secret:") {
+				return fmt.Errorf(
+					"%s's container %s puts ${secret:…} in its env (%s). A secret is never filled into an "+
+						"environment variable: put it in a file the module declares and mount that, or name the "+
+						"file in env-file and say %q why", module, name, key, SecretsInEnvironment)
+			}
+		}
+	}
+	reason, _ := resource[SecretsInEnvironment].(string)
+	if strings.TrimSpace(reason) != "" {
+		return nil
+	}
+	if files, ok := resource["env-file"].([]any); ok {
+		for _, f := range files {
+			if secretFiles[fmt.Sprint(f)] {
+				return fmt.Errorf(
+					"%s's container %s reads %s as an env-file, and that file carries a secret, so the secret "+
+						"reaches the process environment — readable in `docker inspect` and /proc (novox/hq "+
+						"04-ISSUES/041). Mount the secret's file and point the program at it, or, if the program "+
+						"reads only its environment, say so on the container: %q: \"<why>\"",
+					module, name, f, SecretsInEnvironment)
+			}
+		}
+	}
+	return nil
+}
+
+// secretFilesOf is the paths of a module's file resources whose content names a secret.
+func secretFilesOf(resources []map[string]any) map[string]bool {
+	out := map[string]bool{}
+	for _, r := range resources {
+		if fmt.Sprint(r["type"]) != "file" {
+			continue
+		}
+		if content, ok := r["content"].(string); ok && len(secretsUsed(content)) > 0 {
+			out[fmt.Sprint(r["path"])] = true
+		}
+	}
+	return out
+}
+
 // intoFile gives a file the sealed values its content asks for.
 //
 // A name the module never declared is refused here rather than on the machine. The host would

internal/envfile/envfile.go

@@ -0,0 +1,39 @@
+// Package envfile reads a setting that may be a secret from the environment or, preferably, from
+// a file the environment names.
+//
+// **A secret reaches a process as a file** (novox/hq ADR 0086). An environment variable is
+// readable in `docker inspect`, in the process's /proc entry and in whatever composed it; a file
+// the mesh sealed to the machine and the host wrote at 0600 is readable where it is used and
+// nowhere else. So every variable of the control plane's that carries a credential has a `_FILE`
+// twin naming such a file, and the plain form remains only for a control plane a person starts by
+// hand and for the bundle that raises the first one. The store's connections had this shape
+// already (internal/store); this is the same rule for the rest.
+package envfile
+
+import (
+	"fmt"
+	"os"
+	"strings"
+)
+
+// Value is the setting named by `name`: the content of the file `name_FILE` points at when that
+// is set, else the variable itself. Both set is refused — two sources that could disagree is how
+// a setting silently stops meaning what it says. Neither set is "", nil.
+func Value(name string) (string, error) {
+	plain, hasPlain := os.LookupEnv(name)
+	path, hasFile := os.LookupEnv(name + "_FILE")
+	switch {
+	case hasFile && hasPlain && strings.TrimSpace(plain) != "" && strings.TrimSpace(path) != "":
+		return "", fmt.Errorf("both %s and %s_FILE are set; one of them, not both", name, name)
+	case hasFile && strings.TrimSpace(path) != "":
+		raw, err := os.ReadFile(strings.TrimSpace(path))
+		if err != nil {
+			return "", fmt.Errorf("%s_FILE names %s, which cannot be read: %w", name, path, err)
+		}
+		// A file has a line ending and a value does not — trimmed, and only the ending, because a
+		// value may begin or end with a space and still be the value.
+		return strings.TrimRight(string(raw), "\r\n"), nil
+	default:
+		return strings.TrimSpace(plain), nil
+	}
+}

internal/envfile/envfile_test.go

@@ -0,0 +1,44 @@
+package envfile
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+func TestAFileTwinIsPreferredAndOnlyItsLineEndingGoes(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "value")
+	if err := os.WriteFile(path, []byte(" amqp://u:p@h/ \n"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	t.Setenv("MESH_X", "")
+	t.Setenv("MESH_X_FILE", path)
+	got, err := Value("MESH_X")
+	if err != nil || got != " amqp://u:p@h/ " {
+		t.Fatalf("got %q, %v", got, err)
+	}
+}
+
+func TestThePlainVariableStillWorks(t *testing.T) {
+	t.Setenv("MESH_Y", " plain ")
+	if got, err := Value("MESH_Y"); err != nil || got != "plain" {
+		t.Fatalf("got %q, %v", got, err)
+	}
+}
+
+func TestBothSetIsRefused(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "value")
+	_ = os.WriteFile(path, []byte("a"), 0o600)
+	t.Setenv("MESH_Z", "b")
+	t.Setenv("MESH_Z_FILE", path)
+	if _, err := Value("MESH_Z"); err == nil {
+		t.Fatal("two sources were accepted")
+	}
+}
+
+func TestAMissingFileIsSaid(t *testing.T) {
+	t.Setenv("MESH_W_FILE", filepath.Join(t.TempDir(), "absent"))
+	if _, err := Value("MESH_W"); err == nil {
+		t.Fatal("a missing file produced a value")
+	}
+}

internal/link/serve.go

@@ -5,9 +5,9 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"github.com/novox/mesh-controller/internal/envfile"
 	"log"
 	"os"
-	"strings"
 	"time"
 
 	amqp "github.com/rabbitmq/amqp091-go"
@@ -107,7 +107,10 @@ func (s *Server) Answers(r Replayer) error {
 
 // Connect opens the control plane's own connection to the broker.
 func Connect(enroller Enroller, listener Listener) (*Server, error) {
-	url := strings.TrimSpace(os.Getenv(AMQPVar))
+	url, err := envfile.Value(AMQPVar)
+	if err != nil {
+		return nil, err
+	}
 	if url == "" {
 		return nil, fmt.Errorf(
 			"this control plane has no %s, so it cannot reach its broker. Nodes talk to it over "+

module.json

@@ -19,6 +19,7 @@
     "broker-management": "/var/lib/mesh/mesh-controller/broker-management",
     "broker-address": "/var/lib/mesh/mesh-controller/broker-address"
   },
+  "secrets-owner": "65534:65534",
   "resources": [
     {
       "id": "mesh-state",
@@ -26,13 +27,6 @@
       "path": "/var/lib/mesh/mesh-controller",
       "mode": "0700"
     },
-    {
-      "id": "control-env",
-      "type": "file",
-      "path": "/var/lib/mesh/mesh-controller/control.env",
-      "mode": "0600",
-      "content": "MESH_STORE_INVENTORY=${secret:inventory}\nMESH_STORE_IDENTITY=${secret:identity}\nMESH_STORE_LICENCES=${secret:licences}\nMESH_BROKER_AMQP=${secret:broker}\nMESH_BROKER_MANAGEMENT=${secret:broker-management}\nMESH_BROKER_ADDRESS=${secret:broker-address}\n"
-    },
     {
       "id": "server",
       "type": "container",
@@ -41,19 +35,28 @@
       "args": [
         "serve"
       ],
-      "env-file": [
-        "/var/lib/mesh/mesh-controller/control.env"
-      ],
       "env": {
-        "MESH_BROKER_CERTIFICATE": "/broker-tls/tls.crt"
+        "MESH_BROKER_CERTIFICATE": "/broker-tls/tls.crt",
+        "MESH_STORE_INVENTORY_FILE": "/run/secrets/inventory",
+        "MESH_STORE_IDENTITY_FILE": "/run/secrets/identity",
+        "MESH_STORE_LICENCES_FILE": "/run/secrets/licences",
+        "MESH_BROKER_AMQP_FILE": "/run/secrets/broker",
+        "MESH_BROKER_MANAGEMENT_FILE": "/run/secrets/broker-management",
+        "MESH_BROKER_ADDRESS_FILE": "/run/secrets/broker-address"
       },
       "volumes": [
-        "mesh-broker-tls:/broker-tls:ro"
+        "mesh-broker-tls:/broker-tls:ro",
+        "/var/lib/mesh/mesh-controller/inventory:/run/secrets/inventory:ro",
+        "/var/lib/mesh/mesh-controller/identity:/run/secrets/identity:ro",
+        "/var/lib/mesh/mesh-controller/licences:/run/secrets/licences:ro",
+        "/var/lib/mesh/mesh-controller/broker:/run/secrets/broker:ro",
+        "/var/lib/mesh/mesh-controller/broker-management:/run/secrets/broker-management:ro",
+        "/var/lib/mesh/mesh-controller/broker-address:/run/secrets/broker-address:ro"
       ],
+      "artifact": "server",
       "restart-on": [
         "control-env"
-      ],
-      "artifact": "server"
+      ]
     }
   ],
   "build": {