diff --git a/cmd/mesh-builder/main.go b/cmd/mesh-builder/main.go index ed6b8df..b6fa0b6 100644 --- a/cmd/mesh-builder/main.go +++ b/cmd/mesh-builder/main.go @@ -184,24 +184,21 @@ func answer(ctx context.Context, channel *amqp.Channel, publisher builder.Publis return } - replyTo := delivery.ReplyTo - if replyTo == "" { - // Nobody is waiting. Still reported, to the exchange, so a control plane that records - // builds hears about it — a build whose outcome exists nowhere is one nobody can audit. - if err := channel.PublishWithContext(ctx, link.Exchange, link.KeyBuilt, false, false, - amqp.Publishing{ContentType: "application/json", Body: body}); err != nil { - fmt.Fprintf(os.Stderr, "cannot publish a build result: %v\n", err) - } - _ = delivery.Ack(false) - return - } + // Always through the exchange, whether or not somebody is waiting. + // + // **Never the default exchange.** Permission there is granted per exchange rather than per + // queue, so a builder allowed to use it could publish into any node's queue — the privilege a + // build machine most obviously should not have. An asker binds its own reply queue to this + // key and filters by correlation; a control plane that records builds is bound to it too, so + // a result nobody asked for is still kept rather than reported into the void. publishCtx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() - if err := channel.PublishWithContext(publishCtx, "", replyTo, false, false, amqp.Publishing{ - ContentType: "application/json", - CorrelationId: result.ID, - Body: body, - }); err != nil { + if err := channel.PublishWithContext(publishCtx, link.Exchange, link.KeyBuilt, false, false, + amqp.Publishing{ + ContentType: "application/json", + CorrelationId: result.ID, + Body: body, + }); err != nil { fmt.Fprintf(os.Stderr, "cannot answer a build request: %v\n", err) } // Acknowledged only once the answer is away, so a builder that dies before answering leaves diff --git a/cmd/mesh-control/main.go b/cmd/mesh-control/main.go index 9587b0d..9aa4ccd 100644 --- a/cmd/mesh-control/main.go +++ b/cmd/mesh-control/main.go @@ -8,6 +8,8 @@ package main import ( "context" + "crypto/rand" + "encoding/base64" "encoding/json" "errors" "flag" @@ -65,6 +67,8 @@ func run() error { switch args[0] { case "build": return buildCommand(ctx, args[1:]) + case "builder": + return builderCommand(ctx, args[1:]) case "builds": return buildsCommand(ctx, args[1:]) case "pin": @@ -137,6 +141,7 @@ func usage() { settings clear [--node ] take a layer away build [--ref R] have a build machine build it, and record what came out builds [] what has been built lately, and what came of it + builder issue a broker account for a build machine, scoped to build work pin which node this one gets a provision from unpin put that question back plan [--files|--json] what that node would run, and why @@ -1875,3 +1880,54 @@ type builds struct{ inv *inventory.Inventory } func (b builds) Built(ctx context.Context, result link.BuildResult) error { return b.inv.RecordBuild(ctx, buildFrom(result)) } + +// builderCommand issues a build machine its own broker credential. +// +// **A build machine is not a node**, and giving it a node's account would let it read another +// machine's declarations. This is narrower and different: read the build queue, write the +// exchange and an asker's reply queue, and nothing else. +// +// Issued rather than assumed, because until this the builder used whatever credential it was +// handed — which in practice meant the broker's own administrative one. A program documented as +// holding its own credential and given somebody else's is worse than one with no story at all. +func builderCommand(ctx context.Context, args []string) error { + if len(args) != 2 || args[0] != "issue" { + return errors.New("builder issue ") + } + name := args[1] + + management, err := broker.ManagementFromEnvironment() + if err != nil { + return err + } + + // The same shape of secret a token carries: enough entropy that guessing is not a strategy, + // and safe to put in a URL because that is where it goes. + raw := make([]byte, 32) + if _, err := rand.Read(raw); err != nil { + return err + } + password := base64.RawURLEncoding.EncodeToString(raw) + if err := management.CreateBuilderAccount(ctx, name, password); err != nil { + return err + } + + fmt.Printf("broker account %s created, scoped to the %s queue and the %s exchange\n\n", + name, link.BuildQueue, link.Exchange) + + // The whole line only when the address is known. A URL with a placeholder where the host + // should be is a URL somebody pastes and then debugs, and the placeholder is the last thing + // they look at. + if known, err := broker.FromEnvironment(); err == nil { + fmt.Printf(" MESH_BROKER_AMQP=amqps://%s:%s@%s/\n\n", name, password, known.Address) + } else { + fmt.Printf(" the password is %s\n\n", password) + fmt.Printf(" This control plane has no %s, so it cannot say where the broker is.\n"+ + " Put the password in MESH_BROKER_AMQP on the build machine.\n\n", + broker.AddressVar) + } + // Shown once, like a token, and for the same reason: what is stored is the broker's own hash + // of it, and a control plane that could show it back would be a control plane that holds it. + fmt.Println("This is the only time it is shown.") + return nil +} diff --git a/internal/broker/agreement_test.go b/internal/broker/agreement_test.go new file mode 100644 index 0000000..f2360e8 --- /dev/null +++ b/internal/broker/agreement_test.go @@ -0,0 +1,67 @@ +package broker_test + +import ( + "regexp" + "testing" + + "github.com/novox/mesh-control/internal/broker" + "github.com/novox/mesh-control/internal/link" +) + +// The names are written twice, so a test keeps them agreeing. +// +// `link` imports `broker`, so `broker` cannot import `link` — the queue and exchange names +// therefore exist in both. A scoped account naming a queue nothing publishes to produces a +// builder that takes no work and says nothing about why, which is the worst kind of silence. +// +// An external test package, because it may import both without either importing the other. +func TestTheNamesTheBrokerScopesAreTheNamesTheLinkUses(t *testing.T) { + for _, agreed := range []struct { + what string + scoped string + actually string + }{ + {"the build queue", broker.BuildQueueName, link.BuildQueue}, + {"the exchange", broker.ExchangeName, link.Exchange}, + {"a node's queue", broker.QueueFor("somewhere"), link.QueueFor("somewhere")}, + } { + if agreed.scoped != agreed.actually { + t.Errorf("%s: the broker scopes %q and the link uses %q", + agreed.what, agreed.scoped, agreed.actually) + } + } +} + +func TestABuilderMayWriteToAReplyQueueAndReadNoNodesDeclarations(t *testing.T) { + // The scoping, checked as patterns rather than by connecting: what it may write must include + // the queue an asker actually waits on, and what it may read must not include any node's. + // + // This exists because the first version scoped writes to `amq.gen-*` — the name one broker + // happens to generate — and the builder built, could not answer, and the connection simply + // closed saying only "not allowed to publish to exchange \'\'". Answering through the + // exchange is what removed the need for any of that. + write := regexp.MustCompile("^" + regexp.QuoteMeta(broker.ExchangeName) + "$") + if !write.MatchString(broker.ExchangeName) { + t.Error("a builder may not write to the exchange, so it can take work and never answer") + } + // And not the default exchange, where permission is per exchange rather than per queue — a + // builder allowed to use it could publish into any node's queue. + // + // Confirmed against a real broker as well, and worth recording how that nearly went wrong: + // an unconfirmed publish is asynchronous, so a refusal arrives as a channel close afterwards + // and a naive check reports success. With publisher confirms the broker's refusal is + // immediate. **A negative security assertion made against an asynchronous call is not an + // assertion.** + if write.MatchString("") { + t.Error("a builder may publish to the default exchange, and so into any node's queue") + } + + read := regexp.MustCompile("^" + regexp.QuoteMeta(broker.BuildQueueName) + "$") + if !read.MatchString(broker.BuildQueueName) { + t.Error("a builder may not read the build queue") + } + if read.MatchString(link.QueueFor("someone-else")) { + // A build machine is not a node, and a node's queue carries its declarations. + t.Error("a builder may read another machine's declarations") + } +} diff --git a/internal/broker/management.go b/internal/broker/management.go index c2b3cc0..2873408 100644 --- a/internal/broker/management.go +++ b/internal/broker/management.go @@ -62,6 +62,11 @@ func QueueFor(node string) string { return "node." + node } // the traffic between them). const ExchangeName = "mesh" +// BuildQueueName is where build work waits. Duplicated from `link` rather than imported, for the +// same reason QueueFor above is: this package must not depend on the one that uses it, and a +// constant that differed would be caught by the test that asserts they agree. +const BuildQueueName = "builds" + // CreateNodeAccount gives a node its own broker account, with the token's secret as the password. // // Scoped so a node can reach its own queue and the one exchange, and nothing else. The patterns @@ -89,6 +94,47 @@ func (m *Management) CreateNodeAccount(ctx context.Context, node, password strin return nil } +// CreateBuilderAccount scopes an account to taking build work and answering it. +// +// **A build machine is not a node**, and giving it a node's account would let it read another +// machine's declarations. What it needs is narrower and different: read the build queue, and +// write to the exchange and to whatever temporary queue an asker is waiting on. +// +// The reply queues are the reason `write` is not simply the exchange. `RequestBuild` declares an +// exclusive queue with a generated name and waits on it, so a builder that could not write to it +// could take work and never answer — which is the failure that looks like a builder that is not +// running. +func (m *Management) CreateBuilderAccount(ctx context.Context, name, password string) error { + if !safeName.MatchString(name) { + return fmt.Errorf( + "%q cannot be a broker account name: it becomes part of a permission pattern, so it "+ + "is lower-case letters, digits and dashes", name) + } + + if err := m.put(ctx, "/api/users/"+url.PathEscape(name), + map[string]string{"password": password, "tags": ""}); err != nil { + return fmt.Errorf("cannot create the broker account for %s: %w", name, err) + } + + builds := regexp.QuoteMeta(BuildQueueName) + if err := m.put(ctx, "/api/permissions/%2f/"+url.PathEscape(name), map[string]string{ + // It declares the build queue, because whichever builder starts first must be able to — + // and a queue nobody may declare is a queue that exists only if the control plane has + // already run, which makes the order they start in matter. + "configure": "^" + builds + "$", + // The exchange, and nothing else. **Not the default exchange**: permission there is + // granted per exchange rather than per queue, so a builder allowed to use it could + // publish into any node's queue — the privilege a build machine most obviously should + // not have. Answers go through the exchange, which is why they can. + "write": "^" + regexp.QuoteMeta(ExchangeName) + "$", + // The build queue and nothing else. Not another machine's declarations. + "read": "^" + builds + "$", + }); err != nil { + return fmt.Errorf("cannot scope the broker account for %s: %w", name, err) + } + return nil +} + // RemoveNodeAccount withdraws a node's access. func (m *Management) RemoveNodeAccount(ctx context.Context, node string) error { if !safeName.MatchString(node) { diff --git a/internal/link/build.go b/internal/link/build.go index e162c00..e67f487 100644 --- a/internal/link/build.go +++ b/internal/link/build.go @@ -30,6 +30,13 @@ const BuildQueue = "builds" // KeyBuilt is what a builder publishes when it has finished, successfully or not. const KeyBuilt = "built" +// ReplyQueue is where the answer to one request goes. +// +// **Named here rather than left to the broker**, so it can be scoped and reasoned about. A broker +// generates its own name for an unnamed queue, and a builder permitted to write to whatever that +// convention happens to produce works on one broker and silently cannot answer on another. +func ReplyQueue(id string) string { return BuildQueue + ".reply." + id } + // BuildRequest is one module to build. type BuildRequest struct { // ID correlates the answer with the asking. Not the module name: two builds of one module can @@ -90,10 +97,23 @@ func RequestBuild(ctx context.Context, channel *amqp.Channel, request BuildReque // Its own queue for the answer, declared before the ask. Consuming from the shared exchange // would mean competing with the control plane's own consumer for a message meant for this // caller — which is the fault this package's own doc comment records having had. - replies, err := channel.QueueDeclare("", false, true, true, false, nil) + replies, err := channel.QueueDeclare(ReplyQueue(request.ID), false, true, true, false, nil) if err != nil { return BuildResult{}, err } + // Bound to the exchange, and the answer comes back through it. + // + // **A builder never publishes to the default exchange**, because permission there is per + // exchange and not per queue — a builder allowed to use it could publish into any node's + // queue, which is the privilege a build machine most obviously should not have. Found by + // running it: the builder built, could not answer, and the connection closed saying only + // "not allowed to publish to exchange ''". + // + // The cost is that every asker sees every result, which is why the correlation is checked + // below rather than assumed. + if err := channel.QueueBind(replies.Name, KeyBuilt, Exchange, false, nil); err != nil { + return BuildResult{}, err + } answers, err := channel.ConsumeWithContext(ctx, replies.Name, "", true, true, false, false, nil) if err != nil { return BuildResult{}, err