diff --git a/internal/broker/nats.go b/internal/broker/nats.go index 159fbca..579079e 100644 --- a/internal/broker/nats.go +++ b/internal/broker/nats.go @@ -76,6 +76,10 @@ type Principal struct { PasswordHash string } +// enrolmentPrefix is the space every enrolling node's user and inbox live under, so the one place the +// controller may answer an enrolment is derived from the same constant the user is named from. +const enrolmentPrefix = "enrol" + // safeSubject refuses anything that would change the meaning of a subject rather than sit inside // one. A name carrying a dot would silently widen a permission by adding a token; a name carrying // `>` or `*` would widen it to a wildcard, which is the whole authority model gone. @@ -104,7 +108,7 @@ func (p Principal) Username() string { // the mesh holds one live claim per record, and the node's name is the one identifier both // sides already have before anything else is agreed. It is also exactly what the other // transport does, where the account is named after the node and the secret is its password. - return "enrol." + p.Node + return enrolmentPrefix + "." + p.Node } return "" } @@ -163,6 +167,22 @@ func PermissionsFor(p Principal) (Permissions, error) { sub = append(sub, ControllerFollows...) pub = append(pub, "$JS.ACK.EVENTS."+ControllerName+".>") + // **Where an enrolment's answer goes**, and `allow_responses` does not cover it. That + // permits one reply to the reply subject of a message the user received — and a message a + // JetStream consumer delivers has had that field claimed for the consumer's own ack address + // (design 25 §2), so the address the controller actually answers is the one the request + // carried in its payload, which is not a reply subject as the server understands it. + // + // Verified against a real server before this line existed: the answer was refused with + // "Permissions Violation for Publish to _INBOX.enrol.anchor…", and every enrolment on the + // mesh would have timed out while the controller logged success. + // + // **The enrolment inbox space, not a blanket `_INBOX.>`.** Design 25 §4 refuses that, and + // this is not it: nothing but an enrolling node ever subscribes under this prefix, each + // scoped to its own token's, so the controller publishing here is the mesh answering + // enrolments and can reach nothing else. + pub = append(pub, "_INBOX."+enrolmentPrefix+".>") + case KindPerson: // Tools, and nothing else. Every subject a person may publish is a tool call; a person // who could publish an event would be able to claim a module said something. diff --git a/internal/broker/nats_test.go b/internal/broker/nats_test.go index f5c2500..6ba56b1 100644 --- a/internal/broker/nats_test.go +++ b/internal/broker/nats_test.go @@ -256,3 +256,71 @@ func TestAToolGrantThatNamesNoToolIsRefused(t *testing.T) { t.Fatal("a grant naming a module but no tool was accepted") } } + +// The controller can answer an enrolment, and reach no other inbox. +// +// **`allow_responses` does not cover this and that is the trap.** It permits one reply to the reply +// subject of a message the user received — and a message a JetStream consumer delivers has had that +// field claimed for the consumer's own ack address, so the address the controller actually answers is +// the one the request carried in its payload, which the server does not recognise as a reply subject +// at all. +// +// Found against a real server, after a live test on an *unpermissioned* one had passed: every +// enrolment on the mesh would have timed out while the controller logged success. +func TestTheControllerCanAnswerAnEnrolmentAndReachNoOtherInbox(t *testing.T) { + ctl, err := PermissionsFor(Principal{Kind: KindController, PasswordHash: "x"}) + if err != nil { + t.Fatal(err) + } + enrolling, err := PermissionsFor(Principal{Kind: KindEnrolment, Node: "anchor", PasswordHash: "x"}) + if err != nil { + t.Fatal(err) + } + + // Whatever the enrolling node waits on, the controller must be able to publish to. + if len(enrolling.Subscribe) != 1 { + t.Fatalf("an enrolling node subscribes %v, and this test knows only how to check one", + enrolling.Subscribe) + } + waitsOn := enrolling.Subscribe[0] + if !covers(ctl.Publish, waitsOn) { + t.Fatalf("the controller may publish %v, none of which reaches %s — so every enrolment on "+ + "the mesh times out while the controller logs success", ctl.Publish, waitsOn) + } + + // And nothing wider. A node's own inbox and a module's are not the controller's to write into: + // that is the blanket grant design 25 §4 refuses. + for _, other := range []string{"_INBOX.node.anchor.x", "_INBOX.one.shop.x", "_INBOX.person.ada.x"} { + if covers(ctl.Publish, other) { + t.Errorf("the controller can publish to %s, which is an inbox privacy the permission "+ + "list is the only thing protecting", other) + } + } +} + +// covers says whether any granted subject pattern admits one concrete subject, with NATS's own +// wildcard meanings: `*` is one token, `>` is the rest. +func covers(granted []string, subject string) bool { + want := strings.Split(subject, ".") + for _, pattern := range granted { + if admits(strings.Split(pattern, "."), want) { + return true + } + } + return false +} + +func admits(pattern, subject []string) bool { + for i, token := range pattern { + if token == ">" { + return i < len(subject) + } + if i >= len(subject) { + return false + } + if token != "*" && token != subject[i] { + return false + } + } + return len(pattern) == len(subject) +} diff --git a/internal/broker/testdata/composed.conf b/internal/broker/testdata/composed.conf index dd7da95..6bcd013 100644 --- a/internal/broker/testdata/composed.conf +++ b/internal/broker/testdata/composed.conf @@ -23,7 +23,7 @@ accounts { MESH { users = [ { user: "controller", password: "$2a$11$cccccccccccccccccccccc", permissions: { - publish: { allow: ["$JS.ACK.CONTROL.controller.>", "$JS.ACK.EVENTS.controller.>", "$JS.API.>", "mesh.build.>", "mesh.control.>", "mesh.node.>"] } + publish: { allow: ["$JS.ACK.CONTROL.controller.>", "$JS.ACK.EVENTS.controller.>", "$JS.API.>", "_INBOX.enrol.>", "mesh.build.>", "mesh.control.>", "mesh.node.>"] } subscribe: { allow: ["$JS.API.>", "_INBOX.controller.>", "mesh.build.>", "mesh.control.>", "mesh.mod.mesh-catalog.event.module.mesh-catalog.catching-up", "mesh.mod.mesh-catalog.event.module.mesh-catalog.upgraded"] } allow_responses: { max: 1, ttl: "1m" } } }