A build machine gets its own credential, scoped to build work
The builder was documented as holding its own broker credential and nothing else, and nothing issued one — so in practice it used whatever it was handed, which was the broker's administrative account. A program documented as holding its own credential and given somebody else's is worse than one with no story at all. `builder issue <name>` creates an account that may read the build queue and write to the mesh exchange. Not a node account: a build machine is not a node, and a node's queue carries its declarations. Two faults found by running it, both about the answer path: - the reply queue was left for the broker to name, and the account was scoped to `amq.gen-*` — one broker's convention. The builder built, could not answer, and the connection closed. Reply queues are named here now, deterministically. - the answer then went via the DEFAULT exchange, where permission is granted per exchange rather than 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. Answers go through the mesh exchange, which it already may use, and an asker binds its reply queue to the same key and filters by correlation. Verified against a real broker: a builder cannot consume a node's queue and cannot publish to the default exchange. That check nearly reported the opposite — an unconfirmed publish is asynchronous, so the refusal arrives as a channel close afterwards and a naive test sees success. With publisher confirms it is immediate. A negative security assertion made against an asynchronous call is not an assertion. Redelivery was observed working while fixing this: builders that died before answering left their work on the queue, and the next builder did all of it. Also: the queue and exchange names exist in both `broker` and `link`, because `link` imports `broker`. A test in an external package keeps them agreeing — a builder scoped to a queue nothing publishes to takes no work and says nothing about why.
This commit is contained in:
+13
-16
@@ -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
|
||||
|
||||
@@ -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 <module> [--node <n>] take a layer away
|
||||
build <repository> [--ref R] have a build machine build it, and record what came out
|
||||
builds [<module>] what has been built lately, and what came of it
|
||||
builder issue <name> a broker account for a build machine, scoped to build work
|
||||
pin <node> <provision> <from> which node this one gets a provision from
|
||||
unpin <node> <provision> put that question back
|
||||
plan <node> [--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>")
|
||||
}
|
||||
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
|
||||
}
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
@@ -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) {
|
||||
|
||||
+21
-1
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user