Take mesh-sdk at 86f9a1a5 and name each node's bucket with its FileBucketFor, refusing a node name that could carry a wildcard (issues 1696, 1697; ADR 0488)

internal/broker/declaration_files.go

@@ -23,14 +23,14 @@ import (
 // (`$O.<bucket>.M.<base64 of the name>`) and chunks (`$O.<bucket>.C.<nuid>`).
 
 // DeclarationFilesBucket is the prefix of the buckets' names, which mesh-sdk fixes for the node-engine and the controller alike.
-const DeclarationFilesBucket = declaration.FileBucket
+const DeclarationFilesBucket = declaration.FileBucketPrefix
 
-// DeclarationFilesBucketOf is the bucket a node's files are in. One function, so the name of a node's bucket is composed in one
-// place.
-func DeclarationFilesBucketOf(node string) string { return DeclarationFilesBucket + "-" + node }
+// DeclarationFilesBucketOf is the bucket a node's files are in: mesh-sdk's FileBucketFor, the one place the name is composed. A
+// node name it refuses (one that could carry a subject wildcard or separator into a grant) is an error, never a bucket.
+func DeclarationFilesBucketOf(node string) (string, error) { return declaration.FileBucketFor(node) }
 
-// DeclarationFilesStreamOf is the stream under a node's bucket.
-func DeclarationFilesStreamOf(node string) string { return "OBJ_" + DeclarationFilesBucketOf(node) }
+// DeclarationFilesStreamOf is the stream under a node's bucket: mesh-sdk's FileStreamFor.
+func DeclarationFilesStreamOf(node string) (string, error) { return declaration.FileStreamFor(node) }
 
 // DeclarationFilesMaxBytes is a bucket's size: the objects are immutable and regenerable, and one node's hold well under a hundred
 // megabytes.
@@ -41,7 +41,13 @@ const DeclarationFilesKeptFor = 24 * time.Hour
 
 // DeclarationFilesWrites is the one pattern the controller may publish to write a node's bucket: every object's metadata and
 // chunks. The server's API for the stream is the controller's `$JS.API.>`.
-func DeclarationFilesWrites(node string) string { return "$O." + DeclarationFilesBucketOf(node) + ".>" }
+func DeclarationFilesWrites(node string) (string, error) {
+	bucket, err := DeclarationFilesBucketOf(node)
+	if err != nil {
+		return "", err
+	}
+	return "$O." + bucket + ".>", nil
+}
 
 // DeclarationFilesReads is what a node-engine's account may publish to read an object of its own bucket by its name, and nothing
 // more. The set is the one node-engine's own call (mesh-host internal/link FileObject) makes, for B the node's bucket and S its
@@ -57,14 +63,21 @@ func DeclarationFilesWrites(node string) string { return "$O." + DeclarationFile
 // **Never granted:** STREAM.INFO (a `subjects_filter` of `>` lists every name), STREAM.MSG.GET (any message by its sequence), a
 // publish to `$O.B.>` (a write), and any subject of another node's stream. A literal `*` in a subject matches the grant's `*`: it
 // reaches the objects of this node's own bucket and no other (above).
-func DeclarationFilesReads(node string) []string {
-	stream, bucket := DeclarationFilesStreamOf(node), DeclarationFilesBucketOf(node)
+func DeclarationFilesReads(node string) ([]string, error) {
+	bucket, err := DeclarationFilesBucketOf(node)
+	if err != nil {
+		return nil, err
+	}
+	stream, err := DeclarationFilesStreamOf(node)
+	if err != nil {
+		return nil, err
+	}
 	return []string{
 		"$JS.API.DIRECT.GET." + stream + ".$O." + bucket + ".M.*",
 		"$JS.API.CONSUMER.CREATE." + stream + ".*.$O." + bucket + ".C.*",
 		"$JS.API.CONSUMER.DELETE." + stream + ".*",
 		"$JS.FC." + stream + ".>",
-	}
+	}, nil
 }
 
 // EnsureDeclarationFiles creates the buckets of these nodes if absent and brings their options to match. A bucket exists whenever the
@@ -82,21 +95,29 @@ func (j *JetStream) EnsureDeclarationFiles(nodes []string) error {
 // EnsureDeclarationFiles is the same on a JetStream the caller holds.
 func EnsureDeclarationFiles(ctx context.Context, js jetstream.JetStream, nodes ...string) error {
 	for _, node := range nodes {
-		if _, err := js.CreateOrUpdateObjectStore(ctx, DeclarationFilesConfig(node)); err != nil {
-			return fmt.Errorf("asserting the object bucket %s: %w", DeclarationFilesBucketOf(node), err)
+		cfg, err := DeclarationFilesConfig(node)
+		if err != nil {
+			return err
+		}
+		if _, err := js.CreateOrUpdateObjectStore(ctx, cfg); err != nil {
+			return fmt.Errorf("asserting the object bucket %s: %w", cfg.Bucket, err)
 		}
 	}
 	return nil
 }
 
 // DeclarationFilesConfig is a node's bucket as the controller asserts it.
-func DeclarationFilesConfig(node string) jetstream.ObjectStoreConfig {
+func DeclarationFilesConfig(node string) (jetstream.ObjectStoreConfig, error) {
+	bucket, err := DeclarationFilesBucketOf(node)
+	if err != nil {
+		return jetstream.ObjectStoreConfig{}, err
+	}
 	return jetstream.ObjectStoreConfig{
-		Bucket: DeclarationFilesBucketOf(node),
+		Bucket: bucket,
 		Description: "the contents of the files over 64 KiB of the declaration of " + node + ", each under the hex of its sha256 " +
 			"digest (novox/hq ADR 0486): written by the controller alone, read by the node-engine of that node by name, removed " +
 			"a day after the node's latest declaration no longer names it",
 		MaxBytes: DeclarationFilesMaxBytes,
 		Storage:  jetstream.FileStorage,
-	}
+	}, nil
 }

internal/broker/declaration_files_live_test.go

@@ -30,7 +30,14 @@ func nodeEnginesFileObject(ctx context.Context, nc *nats.Conn, node, name string
 	if err != nil {
 		return nil, err
 	}
-	stream, bucket := DeclarationFilesStreamOf(node), DeclarationFilesBucketOf(node)
+	stream, err := DeclarationFilesStreamOf(node)
+	if err != nil {
+		return nil, err
+	}
+	bucket, err := DeclarationFilesBucketOf(node)
+	if err != nil {
+		return nil, err
+	}
 	meta, err := js.GetLastMsg(stream, "$O."+bucket+".M."+base64.URLEncoding.EncodeToString([]byte(name)),
 		nats.DirectGet(), nats.Context(ctx))
 	if err != nil {
@@ -132,7 +139,7 @@ func TestNatsANodeEngineReadsAnObjectOfItsOwnBucketByNameAndNothingElse(t *testi
 	}
 	put := func(node string, content []byte) string {
 		t.Helper()
-		store, err := cjs.ObjectStore(ctx, DeclarationFilesBucketOf(node))
+		store, err := cjs.ObjectStore(ctx, mustBucket(t, node))
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -172,8 +179,8 @@ func TestNatsANodeEngineReadsAnObjectOfItsOwnBucketByNameAndNothingElse(t *testi
 	}
 
 	// What each account is refused, judged by the server.
-	alphaStream, alphaBucket := DeclarationFilesStreamOf("alpha"), DeclarationFilesBucketOf("alpha")
-	betaStream, betaBucket := DeclarationFilesStreamOf("beta"), DeclarationFilesBucketOf("beta")
+	alphaStream, alphaBucket := mustStream(t, "alpha"), mustBucket(t, "alpha")
+	betaStream, betaBucket := mustStream(t, "beta"), mustBucket(t, "beta")
 	listing := []byte(`{"subjects_filter":">"}`)
 	bySeq := []byte(`{"seq":1}`)
 	type refusal struct {
@@ -279,7 +286,7 @@ func TestNatsANodeEngineReadsAnObjectOfItsOwnBucketByNameAndNothingElse(t *testi
 
 	// Nothing was written by the refused: each bucket holds what the controller wrote.
 	for node, want := range map[string]int{"alpha": 2, "beta": 1} {
-		store, err := cjs.ObjectStore(ctx, DeclarationFilesBucketOf(node))
+		store, err := cjs.ObjectStore(ctx, mustBucket(t, node))
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -289,3 +296,21 @@ func TestNatsANodeEngineReadsAnObjectOfItsOwnBucketByNameAndNothingElse(t *testi
 		}
 	}
 }
+
+func mustBucket(t *testing.T, node string) string {
+	t.Helper()
+	b, err := DeclarationFilesBucketOf(node)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return b
+}
+
+func mustStream(t *testing.T, node string) string {
+	t.Helper()
+	b, err := DeclarationFilesStreamOf(node)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return b
+}

internal/broker/declaration_files_test.go

@@ -107,3 +107,25 @@ func TestTheComposedUsersGiveEachNodeItsOwnBucketAndTheControllerAll(t *testing.
 		t.Fatalf("the controller writes the buckets of %v", controller.FilesFor)
 	}
 }
+
+// A node name that could carry a subject wildcard or separator into a grant is refused with an error, never composed into a bucket
+// (ADR 0488): the grants of such a name would reach another node's bucket.
+func TestANodeNameThatCarriesAWildcardGetsNoBucketAndNoGrant(t *testing.T) {
+	for _, node := range []string{"", "*", ">", "a.b", "a b", "a*"} {
+		if b, err := DeclarationFilesBucketOf(node); err == nil {
+			t.Errorf("node %q got the bucket %q", node, b)
+		}
+		if _, err := DeclarationFilesReads(node); err == nil {
+			t.Errorf("node %q got read grants", node)
+		}
+		if _, err := DeclarationFilesWrites(node); err == nil {
+			t.Errorf("node %q got write grants", node)
+		}
+		if _, err := DeclarationFilesConfig(node); err == nil {
+			t.Errorf("node %q got a bucket config", node)
+		}
+	}
+	if b, err := DeclarationFilesBucketOf("g14"); err != nil || b != "declaration-files-g14" {
+		t.Errorf("g14: %q %v", b, err)
+	}
+}

internal/broker/nats.go

@@ -743,7 +743,11 @@ func PermissionsFor(p Principal) (Permissions, error) {
 		// **And the object bucket a declaration's large files travel in** (novox/hq ADR 0486 §2, issue 1697): the
 		// controller alone writes it, as a publish to the bucket's own subjects, which `$JS.API.>` does not cover.
 		for _, node := range p.FilesFor {
-			pub = append(pub, DeclarationFilesWrites(node))
+			writes, err := DeclarationFilesWrites(node)
+			if err != nil {
+				return Permissions{}, err
+			}
+			pub = append(pub, writes)
 		}
 		// **And what the bus says about itself, read-only** (novox/hq to-be 45 §3, S9): a durable
 		// consumer that gave up on a message, or one that was deleted. The server already publishes
@@ -846,7 +850,11 @@ func PermissionsFor(p Principal) (Permissions, error) {
 		pub = append(pub, haveKeys...)
 		// And an object of the declaration bucket by its name, and nothing of the bucket's list or writing
 		// (novox/hq ADR 0486 §2, issue 1697).
-		pub = append(pub, DeclarationFilesReads(p.Node)...)
+		fileReads, err := DeclarationFilesReads(p.Node)
+		if err != nil {
+			return Permissions{}, err
+		}
+		pub = append(pub, fileReads...)
 		// And, while the vault serves it, the vault's move-in in its own name and nobody else's (issue 676): the
 		// values sealed to this machine, which only it opens, handed in sealed to the vault's key for the call.
 		if p.MovesIntoTheVault {